[PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Christopher Bostic]

On Sun, Oct 9, 2016 at 7:56 PM, Jeremy Kerr <jk at ozlabs.org> wrote:
> Hi Chris,
>
>> +/*
>> + * Issue a break commad on this link
>> + */
>> +static int fsi_master_break(struct fsi_master *master, int link)
>> +{
>> +     int rc = 0;
>> +
>> +     if (master->send_break)
>> +             rc = master->send_break(master, link);
>> +
>> +     return rc;
>> +}
>
> OK, looks good.
>
>> +static int fsi_master_fake_break(struct fsi_master *master, int link)
>> +{
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int fsi_master_fake_set_smode(struct fsi_master *master, int link,
>> +                                     uint32_t smode)
>> +{
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>
> Since these are no-ops...
>
>>  static int fsi_master_fake_probe(struct platform_device *pdev)
>>  {
>>       struct fsi_master *master;
>> @@ -63,6 +80,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
>>       master->n_links = 1;
>>       master->read = fsi_master_fake_read;
>>       master->write = fsi_master_fake_write;
>> +     master->send_break = fsi_master_fake_break;
>> +     master->set_smode = fsi_master_fake_set_smode;
>
> ... we can just leave the callbacks NULL (the wrapper functions check
> this).
>

Hi Jeremy,

For break and smode callbacks I did what you had done for
fsi_master_fake_write( ).
I'm not sure how mine are any different?

> However:
>
>> +static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
>> +                                             uint32_t smode)
>> +{
>> +     int rc;
>> +
>> +     rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
>> +                     &smode, sizeof(smode));
>> +
>> +     return rc;
>> +}
>> +
>
> Won't this always be implemented the same way on all masters (ie, a
> write to a particular slave register)? Why the need for a
> master-specific callback here?


This will be different for various masters based on the hardware
issues mentioned
in previous notes.  For example the cascaded master needs to set a different ID
in smode.

>
>> +#define FSI_CMD_BREAK                0xc0de0000      /* Break command */
>> +
>
> This isn't used anywhere; what's it for? It doesn't look like something
> that all masters will use, so shouldn't be in the global master header.

OK, I'll move this out of the global master header.

>
>> @@ -29,6 +31,9 @@ struct fsi_master {
>>       int             (*write)(struct fsi_master *, int link,
>>                               uint8_t slave, uint32_t addr,
>>                               const void *val, size_t size);
>> +     int             (*send_break)(struct fsi_master *, int link);
>> +     int             (*set_smode)(struct fsi_master *, int link,
>> +                             uint32_t smode);
>
> See above comments - do we really need a set_smode callback?
>
> Cheers,
>
>
> Jeremy