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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@gmail.com> 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@oss.qualcomm.com>
> >> ---
> >>  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