Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Bartosz Golaszewski]

On Thu, Apr 2, 2026 at 5:53 PM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> On Thu, Apr 02, 2026 at 01:29:35AM -0700, Bartosz Golaszewski wrote:
> > On Wed, 1 Apr 2026 04:09:21 +0200, Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx> said:
> > > On Tue, Mar 31, 2026 at 01:28:19PM +0200, Bartosz Golaszewski wrote:
> > >> The software node representing the GPIO controller to consumers is
> > >> "dangling": it's not really attached to the device. The GPIO lookup
> > >> relies on matching the name of the node to the chip's label. Set it as
> > >> the secondary firmware node of the platform device to enable proper
> > >> fwnode-based GPIO lookup.
> > >>
> > >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> > >> ---
> > >>  drivers/platform/x86/barco-p50-gpio.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
> > >> index 2a6d8607c402..5f4ffa584295 100644
> > >> --- a/drivers/platform/x86/barco-p50-gpio.c
> > >> +++ b/drivers/platform/x86/barco-p50-gpio.c
> > >> @@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
> > >>    if (ret)
> > >>            return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
> > >>
> > >> +  set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
> > >> +
> > >>    led_info.fwnode = software_node_fwnode(&gpio_leds_node);
> > >>    p50->leds_pdev = platform_device_register_full(&led_info);
> > >>    if (IS_ERR(p50->leds_pdev)) {
> > >
> > > I looks like http://sashiko.dev patch critique is on point:
> > >
> >
> > Ok, let's unpack it.
> >
> > > "
> > > Is the software node attached too late to take effect?
> > >
> > > In the probe function, devm_gpiochip_add_data() is called before this
> > > set_secondary_fwnode() call. During GPIO chip registration, the gpiolib
> > > core snapshots the parent device's fwnode.
> > >
> >
> > What does it mean "to snapshot" the parent's fwnode?
> >
> > What happens is this:
> >
> > static struct fwnode_handle *gpiochip_choose_fwnode(struct gpio_chip *gc)
> > {
> >       if (gc->fwnode)
> >               return gc->fwnode;
> >
> >       if (gc->parent)
> >               return dev_fwnode(gc->parent);
> >
> >       return NULL;
> > }
> >
> > gc->fwnode is NULL so we set gc->parent as the GPIO controller's fwnode.
>
> However if parent does not have fwnode assigned (dev->fwnode is NULL)
> this will return NULL as well. This effectively severs the link between
> the gpiochip and the parent device.
>
> >
> > > Because the secondary fwnode is not yet set on pdev->dev when this snapshot
> > > happens, the GPIO device is registered with a NULL fwnode, which seems to
> > > defeat the purpose of enabling fwnode-based lookups.
> > >
> >
> > Sashiko is completely wrong here: not only is the device registered with the
> > parent's fwnode assigned, the secondary fwnode is a property of the *fwnode*,
> > not of the device. We set the secondary fwnode of the parent's real fwnode.
> > This is carried over to the GPIO controller's device properties even after
> > it's been created.
>
> I do not think it is wrong. If the parent does not have fwnode assigned
> (and it does not) then you do not have a fwnode object to attach a
> secondary node to. The node you are attaching as a secondary becomes
> primary, but it is *too late*. The link has been severed, gpiochip's
> fwnode is a NULL pointer and has no idea that the parent now has a valid
> fwnode.
>
> I think this woudl be a better approach:
>
> @@ -426,7 +419,6 @@ MODULE_DEVICE_TABLE(dmi, dmi_ids);
>
>  static int __init p50_module_init(void)
>  {
> -       struct resource res = DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1);
>         int ret;
>
>         if (!dmi_first_match(dmi_ids))
> @@ -436,7 +428,15 @@ static int __init p50_module_init(void)
>         if (ret)
>                 return ret;
>
> -       gpio_pdev = platform_device_register_simple(DRIVER_NAME, PLATFORM_DEVID_NONE, &res, 1);
> +       gpio_pdev = platform_device_register_full(&(struct platform_device_info){
> +               .name = DRIVER_NAME,
> +               .id = PLATFORM_DEVID_NONE,
> +               .res = (const struct resource[]){
> +                       DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1),
> +               },
> +               .num_res = 1,
> +               .swnode = &gpiochip_node,
> +       });
>         if (IS_ERR(gpio_pdev)) {
>                 pr_err("failed registering %s: %ld\n", DRIVER_NAME, PTR_ERR(gpio_pdev));
>                 platform_driver_unregister(&p50_gpio_driver);
>
> This way you start with the right node right away.
>
> Thanks.
>
> --
> Dmitry

Ah, maybe sashiko wasn't wrong after all but it definitely could do a
better job explaining the actual issue. I missed the fact that this is
not a device described in firmware and as such doesn't have a firmware
node. I see it now. Ok, I'll fix it in v2. In fact, I'll probably wait
until v7.1-rc1 and use the new .swnode field.

Bart