Messages in this thread |  | | From | Bartosz Golaszewski <> | | Date | Thu, 2 Apr 2026 18:39:16 +0200 | | Subject | Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device |
| |
On Thu, Apr 2, 2026 at 5:53 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> 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@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
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
|  |