Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

["Clément Léger" <clement.leger@bootlin.com>]

Le Wed, 23 Feb 2022 16:46:45 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

[...]

> > 
> > Converting existing OF support to fwnode support and thus allowing
> > drivers and subsystems to be compatible with software nodes seemed like
> > the easiest way to do what I needed by keeping all existing drivers.
> > With this support, the driver is completely self-contained and does
> > allow the card to be plugged on whatever platform the user may have.  
> 
> I agree with Hans on the point that converting to / supporting fwnode is
> a good thing by its own.
> 
> > Again, the PCI card is independent of the platform, I do not really see
> > why it should be described using platform description language.  
> 
> Yep, and that why it should cope with the platforms it's designed to be used
> with.

I don't think PCIe card manufacturer expect them to be used solely on a
x86 platform with ACPI. So why should I used ACPI to describe it (or DT
by the way), that's my point.

[...]

> > 
> > For the moment, I only added fwnode API support as an alternative to
> > support both OF and software nodes. ACPI is not meant to be handled by
> > this code "as-is". There is for sure some modifications to be made and
> > I do not know how clocks are handled when using ACPI. Based on some
> > thread dating back to 2018 [1], it seem it was even not supported at
> > all.
> > 
> > To be clear, I added the equivalent of the OF support but using
> > fwnode API because I was interested primarly in using it with software
> > nodes and still wanted OF support to work. I did not planned it to be
> > "ACPI compliant" right now since I do not have any knowledge in that
> > field.  
> 
> And here is the problem. We have a few different resource providers
> (a.k.a. firmware interfaces) which we need to cope with.

Understood that but does adding fwnode support means it should work
as-is with both DT and ACPI ? ACPI code is still in place and only the
of part was converted. But maybe you expect the fwnode prot to be
conformant with ACPI.

> 
> What is going on in this series seems to me quite a violation of the
> layers and technologies. But I guess you may find a supporter of your
> ideas (I mean Enrico). However, I'm on the other side and do not like
> this approach.

As I said in the cover-letter, this approach is the only one that I did
found acceptable without being tied to some firmware description. If you
have another more portable approach, I'm ok with that. But this
solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
i2c-mux without rewriting half of the code. And also allows to easily
swap the PCIe card to other slots/computer without having to modify the
description.

> 
> > 
> > Ok, before going down that way, should the fwnode support be the "only"
> > one, ie remove of_clk_register and others and convert them to
> > fwnode_clk_register for instance or should it be left to avoid
> > modifying all clock drivers ?  
> 
> IRQ domain framework decided to cohabit both, while deprecating the OF one.
> (see "add" vs. "create" APIs there). I think it's a sane choice.

Ok, thanks for the info.

[...]

> > > > static const struct property_entry ddr_clk_props[] = {
> > > >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),    
> > >   
> > > >         PROPERTY_ENTRY_U32("#clock-cells", 0),    
> > > 
> > > Why this is used?  
> > 
> > These props actually describes a fixed-clock properties. When adding
> > fwnode support to clk framework, it was needed to add the
> > equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
> > cells used to describe a reference is still needed to do the
> > translation using fwnode_property_get_reference_args() and give the
> > correct arguments to fwnode_xlate().  
> 
> What you described is the programming (overkilled) point. But does hardware
> needs this? I.o.w. does it make sense in the _hardware_ description?

This does not makes sense for the hardware of course. It also does not
makes sense for the hardware to provide that in the device-tree though.
I actually think this should be only provided by the drivers but it
might be difficult to parse the descriptions then (either DT or
software_node), at least that's how it works right now.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com