Re: [PATCH v1 1/1] gpiolib: Read "gpio-line-names" from a firmware node

[Andy Shevchenko]

On Sun, Mar 7, 2021 at 4:22 PM Bartosz Golaszewski
<bgolaszewski@xxxxxxxxxxxx> wrote:
>
> On Fri, Mar 5, 2021 at 1:02 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxx>
> > Reported-by: Roman Guskov <rguskov@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> >  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