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

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

[Andy Shevchenko]

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

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

[Marek Vasut]

On 3/5/21 1:02 PM, Andy Shevchenko 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>

Tested-by: Marek Vasut <marex@denx.de>
Reviewed-by: Marek Vasut <marex@denx.de>

Thanks

>   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);

You could make the order here a reverse xmas tree, but that's a nitpick.

[...]

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

[Andy Shevchenko]

On Fri, Mar 05, 2021 at 01:11:39PM +0100, Marek Vasut wrote:
> On 3/5/21 1:02 PM, Andy Shevchenko 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>
> 
> Tested-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Marek Vasut <marex@denx.de>

Thanks!

> Thanks
> 
> >   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);
> 
> You could make the order here a reverse xmas tree, but that's a nitpick.

They are dependent, can't be reordered.

-- 
With Best Regards,
Andy Shevchenko

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

[Marek Vasut]

On 3/5/21 1:24 PM, Andy Shevchenko wrote:
> On Fri, Mar 05, 2021 at 01:11:39PM +0100, Marek Vasut wrote:
>> On 3/5/21 1:02 PM, Andy Shevchenko 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>
>>
>> Tested-by: Marek Vasut <marex@denx.de>
>> Reviewed-by: Marek Vasut <marex@denx.de>
> 
> Thanks!
> 
>> Thanks
>>
>>>    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);
>>
>> You could make the order here a reverse xmas tree, but that's a nitpick.
> 
> They are dependent, can't be reordered.

Doh, you're right.

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

[Bartosz Golaszewski]

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

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@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

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

[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

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

[Bartosz Golaszewski]

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

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

[Bartosz Golaszewski]

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

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

[Andy Shevchenko]

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

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

[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

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

[Bartosz Golaszewski]

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

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

[Andy Shevchenko]

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

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

[Bartosz Golaszewski]

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
>
>

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

[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

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

[Marek Vasut]

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.

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

[Bartosz Golaszewski]

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

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

[Marek Vasut]

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.