[PATCH linux v3 16/18] drivers/fsi: Set up CFAMs for communication

[Jeremy Kerr]

Hi Chris,

>> You'll probably be better returning rc for both of these cases, no need
>> to squash it to ENODEV (and if you do, EIO might be more appropriate).
>>
> 
> The main way to detect if there is a real and functional slave on the other
> end of the link, at least in the past, was to see if the break command failed
> or not.

But the break has no response, so any failure there would only indicate
a problem with the master, right? Wouldn't a slave error only be
detected by the smode write?

But yes, in that case, -ENODEV is appropriate for the smode write.

>> OK, looks good for a link_enable function of the masters. However, can
>> you add the new callbacks (link_enable & send_break) and their wrappers
>> in a separate earlier patch?
> 
> Is this the same change you discuss above where you'd like to see
> link_enable and send_break code in a separate patch from the set_smode
> code? Or is this a third patch to spell out sub details on link_enable and
> send_break?

No, same as above. Sorry for the confusion!

>>>  static int fsi_master_scan(struct fsi_master *master)
>>>  {
>>> -     int link, slave_id;
>>> +     int link, slave_id, rc;
>>> +
>>> +     for (link = 0; link < master->n_links; link++) {
>>> +             rc = fsi_master_link_enable(master, link);
>>> +             if (rc)
>>> +                     continue;
>>
>> Shouldn't we fail on that?
>>
> 
> Yes, but what that means right now is uncertain.  I can put in a
> dev_dbg( ) to track the fail but I don't want to exit immediately on
> that error since there may still be links to deal with in the for
> loop.

OK, makes sense.

>>> diff --git a/drivers/fsi/fsi-slave.h b/drivers/fsi/fsi-slave.h
>>> new file mode 100644
>>> index 0000000..ea7a760
>>> --- /dev/null
>>> +++ b/drivers/fsi/fsi-slave.h
>>> @@ -0,0 +1,62 @@
>>> +/*
>>> + * FSI slave engine definitions.
>>> + *
>>
>> Why does this need to be in a header file? Isn't it only used by the FSI
>> core?
> 
> I suppose this is me not being familiar with the common coding practices
> of the Linux community.  I would typically put constants and structs in an
> associated header file even if there was no intent to have them be used
> by anything outside the core.   Is it a requirement that this all be moved
> into fsi-core.c as a means of limiting accessibility?

It's not a requirement, but a strong convention. Only use headers for
stuff that you need to share across other files; this gives future
developers a definite indication of the scope of these macros, consts
and structs.

The less that is globally accessible, the better.

Regards,


Jeremy