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

[Hans de Goede <hansg@kernel.org>]

Hi,

On 11-Aug-25 7:59 PM, Dmitry Torokhov wrote:
> 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.

Ok lets keep using the current swnode.name based approach then.

That certainly makes things easier for the x86-android-tablets
code.

Regards,

Hans