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

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

Hi Ilpo,

On 8-Sep-25 2:48 PM, Ilpo Järvinen wrote:
> On Tue, 12 Aug 2025, Hans de Goede wrote:
> 
>> 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.
> 
> Hi all,
> 
> I'm left uncertain if there are any remaining concerns with all these gpio 
> conversion patches (the 3 independent ones and the larger 
> x86-android-tablets series)? While I see there were a few back and forth
> items between you three, it sounded like there's nothing left to do and it 
> was all just based on wrong impressions/understanding, is that correct 
> deduction from my part?

Yes that is correct. The current patches theoretically (untested) is good
to go. The only thing which still needs to happen is test it.

> There was also an almost promise from Hans to test the x86-android-tablets 
> series, is that still on plan/pending?

Yes I hope to be able to test this the x86-android-tablets series this
week (likely in the weekend). Once that has been tested I think we can
assume that the 3 independent patches will also work and merge all of
them.

Regards,

Hans