Messages in this thread Patch in this message |  | | Date | Thu, 2 Apr 2026 08:53:48 -0700 | | From | Dmitry Torokhov <> | | Subject | Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device |
| |
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
|  |