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 for Android: free password hash cracker in your pocket

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

Message-ID: <4151a14f-8427-41a9-84cf-e901080d0eb1@kernel.org>
Date: Mon, 11 Aug 2025 19:44:01 +0200
From: Hans de Goede <hansg@...nel.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: 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 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.
> 
> 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.
> 
> 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.

Regards,

Hans