| Message ID | 20210305120240.42830-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | b41ba2ec54a70908067034f139aa23d0dd2985ce |
| Headers | show |
| Series | [v1,1/1] gpiolib: Read "gpio-line-names" from a firmware node | expand |
On Fri, Mar 5, 2021 at 1:02 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, > see arch/arm/boot/dts/stm32mp151.dtsi. The driver for > pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c > and iterates over all of its DT subnodes when registering each GPIO > bank gpiochip. Each gpiochip has: > > - gpio_chip.parent = dev, > where dev is the device node of the pin controller > - gpio_chip.of_node = np, > which is the OF node of the GPIO bank > > Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), > i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. > > The original code behaved correctly, as it extracted the "gpio-line-names" > from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. > > To achieve the same behaviour, read property from the firmware node. > > Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device properties") > Reported-by: Marek Vasut <marex@denx.de> > Reported-by: Roman Guskov <rguskov@dh-electronics.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 3bc25a9c4cd6..ba88011cc79d 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -367,22 +367,18 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) > * > * Looks for device property "gpio-line-names" and if it exists assigns > * GPIO line names for the chip. The memory allocated for the assigned > - * names belong to the underlying software node and should not be released > + * names belong to the underlying firmware node and should not be released > * by the caller. > */ > static int devprop_gpiochip_set_names(struct gpio_chip *chip) > { > struct gpio_device *gdev = chip->gpiodev; > - struct device *dev = chip->parent; > + struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev); > const char **names; > int ret, i; > int count; > > - /* GPIO chip may not have a parent device whose properties we inspect. */ > - if (!dev) > - return 0; > - > - count = device_property_string_array_count(dev, "gpio-line-names"); > + count = fwnode_property_string_array_count(fwnode, "gpio-line-names"); > if (count < 0) > return 0; > > @@ -396,7 +392,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) > if (!names) > return -ENOMEM; > > - ret = device_property_read_string_array(dev, "gpio-line-names", > + ret = fwnode_property_read_string_array(fwnode, "gpio-line-names", > names, count); > if (ret < 0) { > dev_warn(&gdev->dev, "failed to read GPIO line names\n"); > -- > 2.30.1 > Did you run the OF unit tests on this? The check for the parent dev was added after a bug was reported that was only triggered in unit tests. Bartosz
On Sun, Mar 7, 2021 at 4:22 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Fri, Mar 5, 2021 at 1:02 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, > > see arch/arm/boot/dts/stm32mp151.dtsi. The driver for > > pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c > > and iterates over all of its DT subnodes when registering each GPIO > > bank gpiochip. Each gpiochip has: > > > > - gpio_chip.parent = dev, > > where dev is the device node of the pin controller > > - gpio_chip.of_node = np, > > which is the OF node of the GPIO bank > > > > Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), > > i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. > > > > The original code behaved correctly, as it extracted the "gpio-line-names" > > from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. > > > > To achieve the same behaviour, read property from the firmware node. > > > > Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device properties") > > Reported-by: Marek Vasut <marex@denx.de> > > Reported-by: Roman Guskov <rguskov@dh-electronics.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/gpio/gpiolib.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 3bc25a9c4cd6..ba88011cc79d 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -367,22 +367,18 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) > > * > > * Looks for device property "gpio-line-names" and if it exists assigns > > * GPIO line names for the chip. The memory allocated for the assigned > > - * names belong to the underlying software node and should not be released > > + * names belong to the underlying firmware node and should not be released > > * by the caller. > > */ > > static int devprop_gpiochip_set_names(struct gpio_chip *chip) > > { > > struct gpio_device *gdev = chip->gpiodev; > > - struct device *dev = chip->parent; > > + struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev); > > const char **names; > > int ret, i; > > int count; > > > > - /* GPIO chip may not have a parent device whose properties we inspect. */ > > - if (!dev) > > - return 0; > > - > > - count = device_property_string_array_count(dev, "gpio-line-names"); > > + count = fwnode_property_string_array_count(fwnode, "gpio-line-names"); > > if (count < 0) > > return 0; > > > > @@ -396,7 +392,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) > > if (!names) > > return -ENOMEM; > > > > - ret = device_property_read_string_array(dev, "gpio-line-names", > > + ret = fwnode_property_read_string_array(fwnode, "gpio-line-names", > > names, count); > > if (ret < 0) { > > dev_warn(&gdev->dev, "failed to read GPIO line names\n"); > > -- > > 2.30.1 > > > > Did you run the OF unit tests on this? The check for the parent dev > was added after a bug was reported that was only triggered in unit > tests. Parent is not used anymore. But I can run unittests next week (or if you know that they are failing now, can you please show the failure?). -- With Best Regards, Andy Shevchenko
On Sun, Mar 07, 2021 at 06:14:49PM +0200, Andy Shevchenko wrote: > On Sun, Mar 7, 2021 at 4:22 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > On Fri, Mar 5, 2021 at 1:02 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, > > > see arch/arm/boot/dts/stm32mp151.dtsi. The driver for > > > pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c > > > and iterates over all of its DT subnodes when registering each GPIO > > > bank gpiochip. Each gpiochip has: > > > > > > - gpio_chip.parent = dev, > > > where dev is the device node of the pin controller > > > - gpio_chip.of_node = np, > > > which is the OF node of the GPIO bank > > > > > > Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), > > > i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. > > > > > > The original code behaved correctly, as it extracted the "gpio-line-names" > > > from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. > > > > > > To achieve the same behaviour, read property from the firmware node. ... > > Did you run the OF unit tests on this? The check for the parent dev > > was added after a bug was reported that was only triggered in unit > > tests. > > Parent is not used anymore. But I can run unittests next week (or if > you know that they are failing now, can you please show the failure?). For the record: [ 40.587868] ### dt-test ### end of unittest - 190 passed, 0 failed If you have tests failed, we need more information about what line fails, etc. -- With Best Regards, Andy Shevchenko
On Mon, Mar 8, 2021 at 12:45 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Mar 07, 2021 at 06:14:49PM +0200, Andy Shevchenko wrote: > > On Sun, Mar 7, 2021 at 4:22 PM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > > > On Fri, Mar 5, 2021 at 1:02 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, > > > > see arch/arm/boot/dts/stm32mp151.dtsi. The driver for > > > > pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c > > > > and iterates over all of its DT subnodes when registering each GPIO > > > > bank gpiochip. Each gpiochip has: > > > > > > > > - gpio_chip.parent = dev, > > > > where dev is the device node of the pin controller > > > > - gpio_chip.of_node = np, > > > > which is the OF node of the GPIO bank > > > > > > > > Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), > > > > i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. > > > > > > > > The original code behaved correctly, as it extracted the "gpio-line-names" > > > > from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. > > > > > > > > To achieve the same behaviour, read property from the firmware node. > > ... > > > > Did you run the OF unit tests on this? The check for the parent dev > > > was added after a bug was reported that was only triggered in unit > > > tests. > > > > Parent is not used anymore. But I can run unittests next week (or if > > you know that they are failing now, can you please show the failure?). > > For the record: > [ 40.587868] ### dt-test ### end of unittest - 190 passed, 0 failed > > If you have tests failed, we need more information about what line fails, etc. > > -- > With Best Regards, > Andy Shevchenko > > No it's fine, I just wanted to make sure. Patch applied, thanks! Bartosz
On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, > see arch/arm/boot/dts/stm32mp151.dtsi. The driver for > pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c > and iterates over all of its DT subnodes when registering each GPIO > bank gpiochip. Each gpiochip has: > > - gpio_chip.parent = dev, > where dev is the device node of the pin controller > - gpio_chip.of_node = np, > which is the OF node of the GPIO bank > > Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), > i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. > > The original code behaved correctly, as it extracted the "gpio-line-names" > from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. > > To achieve the same behaviour, read property from the firmware node. > > Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device properties") > Reported-by: Marek Vasut <marex@denx.de> > Reported-by: Roman Guskov <rguskov@dh-electronics.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 3bc25a9c4cd6..ba88011cc79d 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -367,22 +367,18 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) > * > * Looks for device property "gpio-line-names" and if it exists assigns > * GPIO line names for the chip. The memory allocated for the assigned > - * names belong to the underlying software node and should not be released > + * names belong to the underlying firmware node and should not be released > * by the caller. > */ > static int devprop_gpiochip_set_names(struct gpio_chip *chip) > { > struct gpio_device *gdev = chip->gpiodev; > - struct device *dev = chip->parent; > + struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev); > const char **names; > int ret, i; > int count; > > - /* GPIO chip may not have a parent device whose properties we inspect. */ > - if (!dev) > - return 0; > - > - count = device_property_string_array_count(dev, "gpio-line-names"); > + count = fwnode_property_string_array_count(fwnode, "gpio-line-names"); > if (count < 0) > return 0; > > @@ -396,7 +392,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) > if (!names) > return -ENOMEM; > > - ret = device_property_read_string_array(dev, "gpio-line-names", > + ret = fwnode_property_read_string_array(fwnode, "gpio-line-names", > names, count); > if (ret < 0) { > dev_warn(&gdev->dev, "failed to read GPIO line names\n"); > -- > 2.30.1 > Hi Andy! Unfortunately while this may fix the particular use-case on STM32, it breaks all other users as the 'gpio-line-names' property doesn't live on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). How about we first look for this property on the latter and only if it's not present descend down to the former fwnode? Bart
On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > Unfortunately while this may fix the particular use-case on STM32, it > breaks all other users as the 'gpio-line-names' property doesn't live > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > How about we first look for this property on the latter and only if > it's not present descend down to the former fwnode? Oops, I have tested on x86 and it worked the same way. Lemme check this, but I think the issue rather in ordering when we apply fwnode to the newly created device and when we actually retrieve gpio-line-names property. -- With Best Regards, Andy Shevchenko
On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > Unfortunately while this may fix the particular use-case on STM32, it > > breaks all other users as the 'gpio-line-names' property doesn't live > > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > > > How about we first look for this property on the latter and only if > > it's not present descend down to the former fwnode? > > Oops, I have tested on x86 and it worked the same way. > > Lemme check this, but I think the issue rather in ordering when we apply fwnode > to the newly created device and when we actually retrieve gpio-line-names > property. Hmm... I can't see how it's possible can be. Can you provide a platform name and pointers to the DTS that has been broken by the change? -- With Best Regards, Andy Shevchenko
On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > > On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > Unfortunately while this may fix the particular use-case on STM32, it > > > breaks all other users as the 'gpio-line-names' property doesn't live > > > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > > > > > How about we first look for this property on the latter and only if > > > it's not present descend down to the former fwnode? > > > > Oops, I have tested on x86 and it worked the same way. > > > > Lemme check this, but I think the issue rather in ordering when we apply fwnode > > to the newly created device and when we actually retrieve gpio-line-names > > property. > > Hmm... I can't see how it's possible can be. Can you provide a platform name > and pointers to the DTS that has been broken by the change? > I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and the WiP gpio-sim - but it's the same on most DT platforms. The node that contains the `gpio-line-names` is the one associated with the platform device passed to the GPIO driver. The gpiolib then creates another struct device that becomes the child of that node but it doesn't copy the parent's properties to it (nor should it). Every driver that reads device properties does it from the parent device, not the one in gdev - whether it uses of_, fwnode_ or generic device_ properties. Bartosz
On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: > On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > > > On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > > > > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Unfortunately while this may fix the particular use-case on STM32, it > > > > breaks all other users as the 'gpio-line-names' property doesn't live > > > > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > > > > > > > How about we first look for this property on the latter and only if > > > > it's not present descend down to the former fwnode? > > > > > > Oops, I have tested on x86 and it worked the same way. > > > > > > Lemme check this, but I think the issue rather in ordering when we apply fwnode > > > to the newly created device and when we actually retrieve gpio-line-names > > > property. > > > > Hmm... I can't see how it's possible can be. Can you provide a platform name > > and pointers to the DTS that has been broken by the change? > > > > I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and > the WiP gpio-sim - but it's the same on most DT platforms. The node > that contains the `gpio-line-names` is the one associated with the > platform device passed to the GPIO driver. The gpiolib then creates > another struct device that becomes the child of that node but it > doesn't copy the parent's properties to it (nor should it). > > Every driver that reads device properties does it from the parent > device, not the one in gdev - whether it uses of_, fwnode_ or generic > device_ properties. What you are telling contradicts with the idea of copying parent's fwnode (or OF node) in the current code. Basically to access the properties we have to use either what specific driver supplied (by setting gpiochip->of_node or by leaving it NULL and in this case gpiochip_add_data_with_key() will copy it from the parent. That said, we shouldn't care about parent vs. GPIO device fwnode when reading properties. So, bug is somewhere else. In any case, I will test with the gpio-mockup, thanks! -- With Best Regards, Andy Shevchenko
On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: > > On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > > > > On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > > > > > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > Unfortunately while this may fix the particular use-case on STM32, it > > > > > breaks all other users as the 'gpio-line-names' property doesn't live > > > > > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > > > > > > > > > How about we first look for this property on the latter and only if > > > > > it's not present descend down to the former fwnode? > > > > > > > > Oops, I have tested on x86 and it worked the same way. > > > > > > > > Lemme check this, but I think the issue rather in ordering when we apply fwnode > > > > to the newly created device and when we actually retrieve gpio-line-names > > > > property. > > > > > > Hmm... I can't see how it's possible can be. Can you provide a platform name > > > and pointers to the DTS that has been broken by the change? > > > > > > > I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and > > the WiP gpio-sim - but it's the same on most DT platforms. The node > > that contains the `gpio-line-names` is the one associated with the > > platform device passed to the GPIO driver. The gpiolib then creates > > another struct device that becomes the child of that node but it > > doesn't copy the parent's properties to it (nor should it). > > > > Every driver that reads device properties does it from the parent > > device, not the one in gdev - whether it uses of_, fwnode_ or generic > > device_ properties. > > What you are telling contradicts with the idea of copying parent's fwnode > (or OF node) in the current code. > Ha! While the OF node of the parent device is indeed assigned to the gdev's dev, the same isn't done in the core code for fwnodes and simulated chips don't have an associated OF node, so this is the culprit I suppose. Bart > Basically to access the properties we have to use either what specific driver > supplied (by setting gpiochip->of_node or by leaving it NULL and in this case > gpiochip_add_data_with_key() will copy it from the parent. > > That said, we shouldn't care about parent vs. GPIO device fwnode when reading > properties. So, bug is somewhere else. > > In any case, I will test with the gpio-mockup, thanks! > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: > > > On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > > > > > On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > > > > > > On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > > Unfortunately while this may fix the particular use-case on STM32, it > > > > > > breaks all other users as the 'gpio-line-names' property doesn't live > > > > > > on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > > > > > > > > > > > > How about we first look for this property on the latter and only if > > > > > > it's not present descend down to the former fwnode? > > > > > > > > > > Oops, I have tested on x86 and it worked the same way. > > > > > > > > > > Lemme check this, but I think the issue rather in ordering when we apply fwnode > > > > > to the newly created device and when we actually retrieve gpio-line-names > > > > > property. > > > > > > > > Hmm... I can't see how it's possible can be. Can you provide a platform name > > > > and pointers to the DTS that has been broken by the change? > > > > > > > > > > I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and > > > the WiP gpio-sim - but it's the same on most DT platforms. The node > > > that contains the `gpio-line-names` is the one associated with the > > > platform device passed to the GPIO driver. The gpiolib then creates > > > another struct device that becomes the child of that node but it > > > doesn't copy the parent's properties to it (nor should it). > > > > > > Every driver that reads device properties does it from the parent > > > device, not the one in gdev - whether it uses of_, fwnode_ or generic > > > device_ properties. > > > > What you are telling contradicts with the idea of copying parent's fwnode > > (or OF node) in the current code. > > > > Ha! While the OF node of the parent device is indeed assigned to the > gdev's dev, the same isn't done in the core code for fwnodes and > simulated chips don't have an associated OF node, so this is the > culprit I suppose. Close, but not fully correct. First of all it depends on the OF / ACPI / platform enumeration. Second, we are talking about secondary fwnode in the case where it happens. I'm in the middle of debugging this, I'll come up with something soon I believe. > > Basically to access the properties we have to use either what specific driver > > supplied (by setting gpiochip->of_node or by leaving it NULL and in this case > > gpiochip_add_data_with_key() will copy it from the parent. > > > > That said, we shouldn't care about parent vs. GPIO device fwnode when reading > > properties. So, bug is somewhere else. > > > > In any case, I will test with the gpio-mockup, thanks! -- With Best Regards, Andy Shevchenko
On 3/15/21 6:04 PM, Andy Shevchenko wrote: > On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: >> >> On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >>> >>> On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: >>>> On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko >>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>> >>>>> On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: >>>>>> On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: >>>>>>> On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko >>>>>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>>> >>>>>>> Unfortunately while this may fix the particular use-case on STM32, it >>>>>>> breaks all other users as the 'gpio-line-names' property doesn't live >>>>>>> on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). >>>>>>> >>>>>>> How about we first look for this property on the latter and only if >>>>>>> it's not present descend down to the former fwnode? >>>>>> >>>>>> Oops, I have tested on x86 and it worked the same way. >>>>>> >>>>>> Lemme check this, but I think the issue rather in ordering when we apply fwnode >>>>>> to the newly created device and when we actually retrieve gpio-line-names >>>>>> property. >>>>> >>>>> Hmm... I can't see how it's possible can be. Can you provide a platform name >>>>> and pointers to the DTS that has been broken by the change? >>>>> >>>> >>>> I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and >>>> the WiP gpio-sim - but it's the same on most DT platforms. The node >>>> that contains the `gpio-line-names` is the one associated with the >>>> platform device passed to the GPIO driver. The gpiolib then creates >>>> another struct device that becomes the child of that node but it >>>> doesn't copy the parent's properties to it (nor should it). >>>> >>>> Every driver that reads device properties does it from the parent >>>> device, not the one in gdev - whether it uses of_, fwnode_ or generic >>>> device_ properties. >>> >>> What you are telling contradicts with the idea of copying parent's fwnode >>> (or OF node) in the current code. >>> >> >> Ha! While the OF node of the parent device is indeed assigned to the >> gdev's dev, the same isn't done in the core code for fwnodes and >> simulated chips don't have an associated OF node, so this is the >> culprit I suppose. > > Close, but not fully correct. > First of all it depends on the OF / ACPI / platform enumeration. > Second, we are talking about secondary fwnode in the case where it happens. > > I'm in the middle of debugging this, I'll come up with something soon I believe. Was there ever any follow up on this ? I would like to point out that on STM32MP1 in Linux 5.10.y, the gpio-line-names are still broken, and a revert of "gpiolib: generalize devprop_gpiochip_set_names() for device properties" is still necessary.
On Sat, Apr 10, 2021 at 2:46 AM Marek Vasut <marex@denx.de> wrote: > > On 3/15/21 6:04 PM, Andy Shevchenko wrote: > > On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > >> > >> On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >>> > >>> On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: > >>>> On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko > >>>> <andriy.shevchenko@linux.intel.com> wrote: > >>>>> > >>>>> On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: > >>>>>> On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: > >>>>>>> On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko > >>>>>>> <andriy.shevchenko@linux.intel.com> wrote: > >>>>>> > >>>>>>> Unfortunately while this may fix the particular use-case on STM32, it > >>>>>>> breaks all other users as the 'gpio-line-names' property doesn't live > >>>>>>> on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). > >>>>>>> > >>>>>>> How about we first look for this property on the latter and only if > >>>>>>> it's not present descend down to the former fwnode? > >>>>>> > >>>>>> Oops, I have tested on x86 and it worked the same way. > >>>>>> > >>>>>> Lemme check this, but I think the issue rather in ordering when we apply fwnode > >>>>>> to the newly created device and when we actually retrieve gpio-line-names > >>>>>> property. > >>>>> > >>>>> Hmm... I can't see how it's possible can be. Can you provide a platform name > >>>>> and pointers to the DTS that has been broken by the change? > >>>>> > >>>> > >>>> I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and > >>>> the WiP gpio-sim - but it's the same on most DT platforms. The node > >>>> that contains the `gpio-line-names` is the one associated with the > >>>> platform device passed to the GPIO driver. The gpiolib then creates > >>>> another struct device that becomes the child of that node but it > >>>> doesn't copy the parent's properties to it (nor should it). > >>>> > >>>> Every driver that reads device properties does it from the parent > >>>> device, not the one in gdev - whether it uses of_, fwnode_ or generic > >>>> device_ properties. > >>> > >>> What you are telling contradicts with the idea of copying parent's fwnode > >>> (or OF node) in the current code. > >>> > >> > >> Ha! While the OF node of the parent device is indeed assigned to the > >> gdev's dev, the same isn't done in the core code for fwnodes and > >> simulated chips don't have an associated OF node, so this is the > >> culprit I suppose. > > > > Close, but not fully correct. > > First of all it depends on the OF / ACPI / platform enumeration. > > Second, we are talking about secondary fwnode in the case where it happens. > > > > I'm in the middle of debugging this, I'll come up with something soon I believe. > > Was there ever any follow up on this ? > > I would like to point out that on STM32MP1 in Linux 5.10.y, the > gpio-line-names are still broken, and a revert of "gpiolib: generalize > devprop_gpiochip_set_names() for device properties" is still necessary. Yes, Andy has fixed that in commit b41ba2ec54a7 ("gpiolib: Read "gpio-line-names" from a firmware node") but for some reason this has never made its way into stable. I'll resend it. Bartosz
On 4/10/21 11:06 AM, Bartosz Golaszewski wrote: > On Sat, Apr 10, 2021 at 2:46 AM Marek Vasut <marex@denx.de> wrote: >> >> On 3/15/21 6:04 PM, Andy Shevchenko wrote: >>> On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski >>> <bgolaszewski@baylibre.com> wrote: >>>> >>>> On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko >>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>> >>>>> On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: >>>>>> On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko >>>>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>>>> >>>>>>> On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: >>>>>>>> On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: >>>>>>>>> On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko >>>>>>>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>>>>> >>>>>>>>> Unfortunately while this may fix the particular use-case on STM32, it >>>>>>>>> breaks all other users as the 'gpio-line-names' property doesn't live >>>>>>>>> on dev_fwnode(&gdev->dev) but on dev_fwnode(chip->parent). >>>>>>>>> >>>>>>>>> How about we first look for this property on the latter and only if >>>>>>>>> it's not present descend down to the former fwnode? >>>>>>>> >>>>>>>> Oops, I have tested on x86 and it worked the same way. >>>>>>>> >>>>>>>> Lemme check this, but I think the issue rather in ordering when we apply fwnode >>>>>>>> to the newly created device and when we actually retrieve gpio-line-names >>>>>>>> property. >>>>>>> >>>>>>> Hmm... I can't see how it's possible can be. Can you provide a platform name >>>>>>> and pointers to the DTS that has been broken by the change? >>>>>>> >>>>>> >>>>>> I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and >>>>>> the WiP gpio-sim - but it's the same on most DT platforms. The node >>>>>> that contains the `gpio-line-names` is the one associated with the >>>>>> platform device passed to the GPIO driver. The gpiolib then creates >>>>>> another struct device that becomes the child of that node but it >>>>>> doesn't copy the parent's properties to it (nor should it). >>>>>> >>>>>> Every driver that reads device properties does it from the parent >>>>>> device, not the one in gdev - whether it uses of_, fwnode_ or generic >>>>>> device_ properties. >>>>> >>>>> What you are telling contradicts with the idea of copying parent's fwnode >>>>> (or OF node) in the current code. >>>>> >>>> >>>> Ha! While the OF node of the parent device is indeed assigned to the >>>> gdev's dev, the same isn't done in the core code for fwnodes and >>>> simulated chips don't have an associated OF node, so this is the >>>> culprit I suppose. >>> >>> Close, but not fully correct. >>> First of all it depends on the OF / ACPI / platform enumeration. >>> Second, we are talking about secondary fwnode in the case where it happens. >>> >>> I'm in the middle of debugging this, I'll come up with something soon I believe. >> >> Was there ever any follow up on this ? >> >> I would like to point out that on STM32MP1 in Linux 5.10.y, the >> gpio-line-names are still broken, and a revert of "gpiolib: generalize >> devprop_gpiochip_set_names() for device properties" is still necessary. > > Yes, Andy has fixed that in commit b41ba2ec54a7 ("gpiolib: Read > "gpio-line-names" from a firmware node") but for some reason this has > never made its way into stable. I'll resend it. Yes, that's the missing one, thanks. With that picked, the mp1 is fine.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 3bc25a9c4cd6..ba88011cc79d 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -367,22 +367,18 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) * * Looks for device property "gpio-line-names" and if it exists assigns * GPIO line names for the chip. The memory allocated for the assigned - * names belong to the underlying software node and should not be released + * names belong to the underlying firmware node and should not be released * by the caller. */ static int devprop_gpiochip_set_names(struct gpio_chip *chip) { struct gpio_device *gdev = chip->gpiodev; - struct device *dev = chip->parent; + struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev); const char **names; int ret, i; int count; - /* GPIO chip may not have a parent device whose properties we inspect. */ - if (!dev) - return 0; - - count = device_property_string_array_count(dev, "gpio-line-names"); + count = fwnode_property_string_array_count(fwnode, "gpio-line-names"); if (count < 0) return 0; @@ -396,7 +392,7 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) if (!names) return -ENOMEM; - ret = device_property_read_string_array(dev, "gpio-line-names", + ret = fwnode_property_read_string_array(fwnode, "gpio-line-names", names, count); if (ret < 0) { dev_warn(&gdev->dev, "failed to read GPIO line names\n");
On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, see arch/arm/boot/dts/stm32mp151.dtsi. The driver for pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c and iterates over all of its DT subnodes when registering each GPIO bank gpiochip. Each gpiochip has: - gpio_chip.parent = dev, where dev is the device node of the pin controller - gpio_chip.of_node = np, which is the OF node of the GPIO bank Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. The original code behaved correctly, as it extracted the "gpio-line-names" from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. To achieve the same behaviour, read property from the firmware node. Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device properties") Reported-by: Marek Vasut <marex@denx.de> Reported-by: Roman Guskov <rguskov@dh-electronics.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)