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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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?

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

-- 
 i.