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@xxxxxxxxxxxxxxx> 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
