Re: [PATCH] platform/x86: barco-p50-gpio: use software nodes for gpio-leds/keys

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Aug 11, 2025 at 07:40:27PM +0200, Hans de Goede wrote:
> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
> > On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> >> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> >>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> >>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:

...

> >>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
> >>>> hence no need some additional magic Hans mentioned in the other series.
> >>>
> >>> Not entirely like with the x86-android-tablets patches this
> >>> declares a software-node for the gpiochip:
> >>>
> >>> static const struct software_node gpiochip_node = {
> >>> 	.name = DRIVER_NAME,
> >>> };
> >>>
> >>> and registers that node, but nowhere does it actually
> >>> get assigned to the gpiochip.
> >>>
> >>> This is going to need a line like this added to probe():
> >>>
> >>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> >>>
> >>> note the software_node_fwnode() call MUST be made after
> >>> registering the software-nodes (group).
> >>>
> >>> Other then needing this single line things are indeed
> >>> much easier when the code containing the software
> >>> properties / nodes is the same code as which is
> >>> registering the gpiochip.
> >>
> >> Ah, good point!
> > 
> > This is wrong though, the software node need not be attached to the
> > gpiochip (and I wonder if it is even safe to do so). It simply provides
> > a name by which gpiochip is looked up in swnode_get_gpio_device().
> 
> Ah interesting. This is very different from how fwnodes generally
> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
> the reference to the fwnode of the type of device to which the
> reference points.
> 
> IOW the standard way how this works for most other subsystems
> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
> gpio_device_find() with a compare function which uses
> device_match_fwnode().
> 
> I see that instead it uses the swnode name and passes that to
> gpio_device_find_by_label().
> 
> I must say that AFAIK this is not how swnodes are supposed to
> be used the swnode name field is supposed to only be there
> for debugging use and may normally be left empty all together.
> 
> I guess using the swnode-name + gpio_device_find_by_label()
> works but it goes against the design of how fw-nodes
> and especially fwnode-references are supposed to be used...
> 
> Having a fwnode reference pointing to what is in essence
> a dangling (not attached to any device) fwnode is weird.
> 
> Are there already any users of PROPERTY_ENTRY_GPIO() in
> the kernel? If not then I think that we should fix things
> up to actually do a reference match and not a name based
> lookup.

IIRC we have several users already.

> Andy IIRC you've done quite a bit of work on software-nodes,
> what is your take on this ?

I remember seeing this series that added functionality and even tried reviewing
it, but I must admit I haven't noticed this detail. I tend to agree with you
that it's better to keep handling of fwnodes uniform.

> Note this is likely my last email in this thread for
> a while since I will be traveling without email access.

-- 
With Best Regards,
Andy Shevchenko