Re: [RFC 10/10] net: sfp: add support for fwnode

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/23/22 12:22, Andy Shevchenko wrote:
> On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
>> Le Mon, 21 Feb 2022 19:57:39 +0200,
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>>
>>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
>>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
>>>> is using the fwnode API which also works with device-tree and ACPI.
>>>> For this purpose, the device-tree and ACPI code handling the i2c
>>>> adapter retrieval was factorized with the new code. This also allows
>>>> i2c devices using a software_node description to be used by sfp code.  
>>>
>>> If I'm not mistaken this patch can even go separately right now, since all used
>>> APIs are already available.
>>
>> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
>> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
>> probably be contributed both in a separate series.
> 
> I summon Hans into the discussion since I remember he recently refactored
> a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> picture approach with this series based on his ACPI experience.

If I understand this series correctly then this is about a PCI-E card
which has an I2C controller on the card and behind that I2C-controller
there are a couple if I2C muxes + I2C clients.

And the goal of the series is to describe those I2C muxes + I2C clients
with software nodes so that the existing I2C enumeration code can be
used (after porting the existing I2C enumeration code from OF functions
to generic fwnode functions).

Did I understand this bit correctly? I believe that a lot of the
discussion here is caused by the initial problem / hw-setup this
series tries to address / support is not described very well ?

Assuming I did understand the above correctly. One alternative would be
to simply manually instantiate the I2C muxes + clients using
i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
will work for the muxes without adding some software_nodes which
brings us back to something like this patch-set.

In general I believe that porting things away from OF specific parsing
to the generic fwnode APIs is a good thing.

Making device_get_match_data() for devices with only a software fwnode
use of_device_id matching feels a bit weird. But it also makes sense
since that requires just adding a compatible string to the software
fwnode properties which is easy and it allows re-uses existing
matching code in the drivers.

I understand various people falling over this weirdness but AFAICT
the alternative would be adding some special swnode_id type + matching
code for devices where the primary fwnode is a software fwnode, which
would just be a whole bunch of extra code ending up with something
similar.

So re-using of_device_id-s for matching of devices where the primary
fwnode is a software fwnode seems like a good idea. *But* this all
needs to be explained in the commit message a lot better. It really
needs to be spelled out that this is:

a) Only for matching devices where the primary fwnode is a software fwnode 

b) Really has nothing to do with of/dt but of_device_id matching is
   used here to avoid having to introduce a new matching mechanism just
   for devices where the primary fwnode is a software fwnode

c) That introducing a new software fwnode matching mechanism would be
   a bad idea since this will require adding new swnode_match tables
   to many drivers, where as re-using of_device_id will make drivers
   which already have an of_match_table just work.

And this should be spelled out in both the commit message as well
as in some documentation / kdoc comments. Because although a useful
trick, reusing the of_match_id-s is also confusing which I believe
is what has led to a lot of the discussion on this patch-set so far.

Note the above all relies on my interpretation of this patch set,
which may be wrong, since as said the patch-set does seem to be
lacking when it comes to documentation / motivation of the patches.

Regards,

Hans