| Message ID | 20211203133003.31786-4-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-sim: configfs-based GPIO simulator | expand |
On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > If the driver sets the fwnode in struct gpio_chip, let it take > precedence over the of_node. > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > drivers/gpio/gpiolib-of.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index 0ad288ab6262..91dcf2c6cdd8 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -1046,6 +1046,9 @@ void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) > if (gc->parent) > gdev->dev.of_node = gc->parent->of_node; > > + if (gc->fwnode) > + gc->of_node = to_of_node(gc->fwnode); > + > /* If the gpiochip has an assigned OF node this takes precedence */ > if (gc->of_node) > gdev->dev.of_node = gc->of_node; Similar should be done in acpi_gpio_dev_init(): if (gc->fwnode) device_set_node(&gdev->dev, gc->fwnode);
On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: ... > > if (gc->parent) > > gdev->dev.of_node = gc->parent->of_node; > > > > + if (gc->fwnode) > > + gc->of_node = to_of_node(gc->fwnode); > > + > > /* If the gpiochip has an assigned OF node this takes precedence */ > > if (gc->of_node) > > gdev->dev.of_node = gc->of_node; > > Similar should be done in acpi_gpio_dev_init(): > > if (gc->fwnode) > device_set_node(&gdev->dev, gc->fwnode); Hmm... On the second though this should be rather if (gc->fwnode) set_secondary_fwnode(&gdev->dev, gc->fwnode); So the logic will be that: - if we have parent, set primary fwnode to it - if we have fwnode, set secondary one to it - otherwise do nothing
On Fri, Dec 03, 2021 at 08:56:27PM +0200, Andy Shevchenko wrote: > On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > ... > > > > if (gc->parent) > > > gdev->dev.of_node = gc->parent->of_node; > > > > > > + if (gc->fwnode) > > > + gc->of_node = to_of_node(gc->fwnode); > > > + > > > /* If the gpiochip has an assigned OF node this takes precedence */ > > > if (gc->of_node) > > > gdev->dev.of_node = gc->of_node; > > > > Similar should be done in acpi_gpio_dev_init(): > > > > if (gc->fwnode) > > device_set_node(&gdev->dev, gc->fwnode); > > Hmm... On the second though this should be rather > > if (gc->fwnode) > set_secondary_fwnode(&gdev->dev, gc->fwnode); > > So the logic will be that: > - if we have parent, set primary fwnode to it > - if we have fwnode, set secondary one to it > - otherwise do nothing Heck, it's Friday... If we have parent device for several GPIO devices, this won't work right now due to limitations of fwnode regarding to the sturct device. So, it means we may not have shared primary with different secondary fwnodes. So, come back to the initial suggestion (overwrite it for now): /* * If custom fwnode provided, use it. Currently we may not * handle the case where shared primary node has different * secondary ones. Ideally we have to use * set_secondary_fwnode() here. */ if (gc->fwnode) device_set_node(&gdev->dev, gc->fwnode);
On Fri, Dec 3, 2021 at 8:04 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 08:56:27PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > > > ... > > > > > > if (gc->parent) > > > > gdev->dev.of_node = gc->parent->of_node; > > > > > > > > + if (gc->fwnode) > > > > + gc->of_node = to_of_node(gc->fwnode); > > > > + > > > > /* If the gpiochip has an assigned OF node this takes precedence */ > > > > if (gc->of_node) > > > > gdev->dev.of_node = gc->of_node; > > > > > > Similar should be done in acpi_gpio_dev_init(): > > > > > > if (gc->fwnode) > > > device_set_node(&gdev->dev, gc->fwnode); > > > > Hmm... On the second though this should be rather > > > > if (gc->fwnode) > > set_secondary_fwnode(&gdev->dev, gc->fwnode); > > > > So the logic will be that: > > - if we have parent, set primary fwnode to it > > - if we have fwnode, set secondary one to it > > - otherwise do nothing > > Heck, it's Friday... > > If we have parent device for several GPIO devices, this won't work right now > due to limitations of fwnode regarding to the sturct device. > > So, it means we may not have shared primary with different secondary fwnodes. > > So, come back to the initial suggestion (overwrite it for now): > > /* > * If custom fwnode provided, use it. Currently we may not > * handle the case where shared primary node has different > * secondary ones. Ideally we have to use > * set_secondary_fwnode() here. > */ > if (gc->fwnode) > device_set_node(&gdev->dev, gc->fwnode); > Other parts of gpiolib-of depend on the of_node being there. Converting it to fwnode is a whole other task so for now I suggest we just convert the fwnode to of_node in struct gpio_chip as per my patch. Bart
On Fri, Dec 03, 2021 at 08:28:34PM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 3, 2021 at 8:04 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 08:56:27PM +0200, Andy Shevchenko wrote: > > > On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: ... > > > > > if (gc->parent) > > > > > gdev->dev.of_node = gc->parent->of_node; > > > > > > > > > > + if (gc->fwnode) > > > > > + gc->of_node = to_of_node(gc->fwnode); > > > > > + > > > > > /* If the gpiochip has an assigned OF node this takes precedence */ > > > > > if (gc->of_node) > > > > > gdev->dev.of_node = gc->of_node; > > > > > > > > Similar should be done in acpi_gpio_dev_init(): ^^^^^^ (1) ... > > If we have parent device for several GPIO devices, this won't work right now > > due to limitations of fwnode regarding to the sturct device. > > > > So, it means we may not have shared primary with different secondary fwnodes. > > > > So, come back to the initial suggestion (overwrite it for now): > > > > /* > > * If custom fwnode provided, use it. Currently we may not > > * handle the case where shared primary node has different > > * secondary ones. Ideally we have to use > > * set_secondary_fwnode() here. > > */ > > if (gc->fwnode) > > device_set_node(&gdev->dev, gc->fwnode); > > > > Other parts of gpiolib-of depend on the of_node being there. > Converting it to fwnode is a whole other task so for now I suggest we > just convert the fwnode to of_node in struct gpio_chip as per my > patch. But this is about ACPI counterpart. If you do this, do this in both cases. Above code for ACPI (1).
On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 08:28:34PM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 3, 2021 at 8:04 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Dec 03, 2021 at 08:56:27PM +0200, Andy Shevchenko wrote: > > > > On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > > > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > ... > > > > > > > if (gc->parent) > > > > > > gdev->dev.of_node = gc->parent->of_node; > > > > > > > > > > > > + if (gc->fwnode) > > > > > > + gc->of_node = to_of_node(gc->fwnode); > > > > > > + > > > > > > /* If the gpiochip has an assigned OF node this takes precedence */ > > > > > > if (gc->of_node) > > > > > > gdev->dev.of_node = gc->of_node; > > > > > > > > > > Similar should be done in acpi_gpio_dev_init(): > > > ^^^^^^ (1) > > ... > > > > If we have parent device for several GPIO devices, this won't work right now > > > due to limitations of fwnode regarding to the sturct device. > > > > > > So, it means we may not have shared primary with different secondary fwnodes. > > > > > > So, come back to the initial suggestion (overwrite it for now): > > > > > > /* > > > * If custom fwnode provided, use it. Currently we may not > > > * handle the case where shared primary node has different > > > * secondary ones. Ideally we have to use > > > * set_secondary_fwnode() here. > > > */ > > > if (gc->fwnode) > > > device_set_node(&gdev->dev, gc->fwnode); > > > > > > > Other parts of gpiolib-of depend on the of_node being there. > > Converting it to fwnode is a whole other task so for now I suggest we > > just convert the fwnode to of_node in struct gpio_chip as per my > > patch. > > But this is about ACPI counterpart. If you do this, do this in both cases. > Above code for ACPI (1). > This series concerns the gpio-sim driver and it only uses configfs (with manually created platform devices) or device-tree. I would prefer to do ACPI separately and I'd like you to lead that because I neither have any HW to test nor claim to understand it. :) Bart
On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > This series concerns the gpio-sim driver and it only uses configfs > (with manually created platform devices) or device-tree. I would > prefer to do ACPI separately and I'd like you to lead that because I > neither have any HW to test nor claim to understand it. :) Please, mention this in the commit message that ACPI is not covered (yet).
On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > This series concerns the gpio-sim driver and it only uses configfs > > (with manually created platform devices) or device-tree. I would > > prefer to do ACPI separately and I'd like you to lead that because I > > neither have any HW to test nor claim to understand it. :) > > Please, mention this in the commit message that ACPI is not covered (yet). > But the commit message says: "gpiolib: of: make fwnode take precedence in struct gpio_chip" - it says OF right here. :) Bart
On Mon, Dec 06, 2021 at 02:40:36PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > ... > > > > > This series concerns the gpio-sim driver and it only uses configfs > > > (with manually created platform devices) or device-tree. I would > > > prefer to do ACPI separately and I'd like you to lead that because I > > > neither have any HW to test nor claim to understand it. :) > > > > Please, mention this in the commit message that ACPI is not covered (yet). > > But the commit message says: "gpiolib: of: make fwnode take precedence > in struct gpio_chip" - it says OF right here. :) It implies that reader should have a 6th sense to know about ACPI and what else? Please, be explicit over implicit.
On Mon, Dec 06, 2021 at 03:48:39PM +0200, Andy Shevchenko wrote: > On Mon, Dec 06, 2021 at 02:40:36PM +0100, Bartosz Golaszewski wrote: > > On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > ... > > > > > > > This series concerns the gpio-sim driver and it only uses configfs > > > > (with manually created platform devices) or device-tree. I would > > > > prefer to do ACPI separately and I'd like you to lead that because I > > > > neither have any HW to test nor claim to understand it. :) > > > > > > Please, mention this in the commit message that ACPI is not covered (yet). > > > > But the commit message says: "gpiolib: of: make fwnode take precedence > > in struct gpio_chip" - it says OF right here. :) > > It implies that reader should have a 6th sense to know about ACPI and what > else? Please, be explicit over implicit. The problem here is that you change one case and haven't touched the rest which brings inconsistency in the behaviour on different resource providers. We need a record to be sure what disbalance this patch brought. That's why I asked to mention ACPI branch.
On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > If the driver sets the fwnode in struct gpio_chip, let it take > precedence over the of_node. By the way, have you tried this on pure DT-less/ACPI-less platform (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, because this doesn't affect swnode case, right?
On Mon, Dec 6, 2021 at 2:55 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > If the driver sets the fwnode in struct gpio_chip, let it take > > precedence over the of_node. > > By the way, have you tried this on pure DT-less/ACPI-less platform > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > because this doesn't affect swnode case, right? > Works just fine on a BeagleBone Black - both the regular GPIO controllers as well as DT-instantiated gpio-sim. Bart
On Mon, Dec 06, 2021 at 03:54:09PM +0200, Andy Shevchenko wrote: > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > If the driver sets the fwnode in struct gpio_chip, let it take > > precedence over the of_node. > > By the way, have you tried this on pure DT-less/ACPI-less platform > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > because this doesn't affect swnode case, right? Okay, swnode will work (*) due to previous patch.
On Mon, Dec 06, 2021 at 03:03:31PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 6, 2021 at 2:55 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > > If the driver sets the fwnode in struct gpio_chip, let it take > > > precedence over the of_node. > > > > By the way, have you tried this on pure DT-less/ACPI-less platform > > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > > because this doesn't affect swnode case, right? > > > > Works just fine on a BeagleBone Black - both the regular GPIO > controllers as well as DT-instantiated gpio-sim. Yeah, I realized that myself why.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 0ad288ab6262..91dcf2c6cdd8 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -1046,6 +1046,9 @@ void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) if (gc->parent) gdev->dev.of_node = gc->parent->of_node; + if (gc->fwnode) + gc->of_node = to_of_node(gc->fwnode); + /* If the gpiochip has an assigned OF node this takes precedence */ if (gc->of_node) gdev->dev.of_node = gc->of_node;
If the driver sets the fwnode in struct gpio_chip, let it take precedence over the of_node. Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- drivers/gpio/gpiolib-of.c | 3 +++ 1 file changed, 3 insertions(+)