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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bylxufp3r5qzf5axqrtytamkveaw5dpsidmdyiany4wkexbpd@s4yremtvct4a>
Date: Mon, 11 Aug 2025 10:59:05 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Hans de Goede <hansg@...nel.org>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Santosh Kumar Yadav <santoshkumar.yadav@...co.com>, Peter Korsgaard <peter.korsgaard@...co.com>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Arnd Bergmann <arnd@...db.de>, platform-driver-x86@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: barco-p50-gpio: use software nodes for
 gpio-leds/keys

On Mon, Aug 11, 2025 at 07:44:01PM +0200, Hans de Goede wrote:
> On 11-Aug-25 7:40 PM, Hans de Goede wrote:
> > Hi,
> > 
> > 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.

Hmm, given that I wrote both the references support for software nodes
and gpiolib-swnode.c they work exactly as I wanted them ;) Yes, in
general name is optional, but for GPIOs it is needed.

> > 
> > 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.

I agree it is a bit weird, but this allows to disconnect the board file
from the GPIO driver and makes it easier to convert to device tree down
the road as it can be done in a piecemeal fashion. If you want fwnode
actually attached to the gpiochip then:

1. You can't really have static/const initializers in most of the cases
2. Fishing it out from an unrelated subsystem is much harder than
matching on a name.

> > 
> > 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.

I converted spitz and a few other drivers. Some of that has landed.

> > 
> > Andy IIRC you've done quite a bit of work on software-nodes,
> > what is your take on this ?
> > 
> > Note this is likely my last email in this thread for
> > a while since I will be traveling without email access.
> 
> p.s.
> 
> It seems that atm device_match_fwnode() only checks
> that the passed in fwnode to match on matches the primary
> fwnode of the device. This should be modified to also
> match on the secondary node if matching the first node
> fails. Like how e.g. fwnode_property_present() falls
> back to checking the secondary node if the requested
> property is not present in the primary fwnode.

Thanks.

-- 
Dmitry