[PATCH] pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs

[PATCH] pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs

[Richard Fitzgerald]

When dt_to_map_one_config() is called with a pinctrl_dev passed
in, it should only be using this if the node being looked up
is a hog. The code was always using the passed pinctrl_dev
without checking whether the dt node referred to it.

A pin controller can have pinctrl-n dependencies on other pin
controllers in these cases:

- the pin controller hardware is external, for example I2C, so
  needs other pin controller(s) to be setup to communicate with
  the hardware device.

- it is a child of a composite MFD so its of_node is shared with
  the parent MFD and other children of that MFD. Any part of that
  MFD could have dependencies on other pin controllers.

Because of this, dt_to_map_one_config() can't assume that if it
has a pinctrl_dev passed in then the node it looks up must be
a hog. It could be a reference to some other pin controller.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/pinctrl/devicetree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 1ff6c3573493..b601039d6c69 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -122,8 +122,10 @@ static int dt_to_map_one_config(struct pinctrl *p,
 			/* OK let's just assume this will appear later then */
 			return -EPROBE_DEFER;
 		}
-		if (!pctldev)
-			pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
+		/* If we're creating a hog we can use the passed pctldev */
+		if (pctldev && (np_pctldev == p->dev->of_node))
+			break;
+		pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
 		if (pctldev)
 			break;
 		/* Do not defer probing of hogs (circular loop) */
-- 
2.11.0

Re: [PATCH] pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs

[Linus Walleij]

On Wed, Feb 28, 2018 at 4:53 PM, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:

> When dt_to_map_one_config() is called with a pinctrl_dev passed
> in, it should only be using this if the node being looked up
> is a hog. The code was always using the passed pinctrl_dev
> without checking whether the dt node referred to it.
>
> A pin controller can have pinctrl-n dependencies on other pin
> controllers in these cases:
>
> - the pin controller hardware is external, for example I2C, so
>   needs other pin controller(s) to be setup to communicate with
>   the hardware device.
>
> - it is a child of a composite MFD so its of_node is shared with
>   the parent MFD and other children of that MFD. Any part of that
>   MFD could have dependencies on other pin controllers.
>
> Because of this, dt_to_map_one_config() can't assume that if it
> has a pinctrl_dev passed in then the node it looks up must be
> a hog. It could be a reference to some other pin controller.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Seems correct. Also the code is a little more clear
after the patch.

Patch applied.

It'd be nice to see some testing of this, I hope linux-next is a good
testing ground.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs

[Charles Keepax]

On Fri, Mar 02, 2018 at 09:42:54AM +0100, Linus Walleij wrote:
> On Wed, Feb 28, 2018 at 4:53 PM, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> 
> > When dt_to_map_one_config() is called with a pinctrl_dev passed
> > in, it should only be using this if the node being looked up
> > is a hog. The code was always using the passed pinctrl_dev
> > without checking whether the dt node referred to it.
> >
> > A pin controller can have pinctrl-n dependencies on other pin
> > controllers in these cases:
> >
> > - the pin controller hardware is external, for example I2C, so
> >   needs other pin controller(s) to be setup to communicate with
> >   the hardware device.
> >
> > - it is a child of a composite MFD so its of_node is shared with
> >   the parent MFD and other children of that MFD. Any part of that
> >   MFD could have dependencies on other pin controllers.
> >
> > Because of this, dt_to_map_one_config() can't assume that if it
> > has a pinctrl_dev passed in then the node it looks up must be
> > a hog. It could be a reference to some other pin controller.
> >
> > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> 
> Seems correct. Also the code is a little more clear
> after the patch.
> 
> Patch applied.
> 
> It'd be nice to see some testing of this, I hope linux-next is a good
> testing ground.
> 

Once we are happy on the testing front, how would you feel about
getting this forwarded to 4.14 stable as well?

The trouble is for our customers on 4.14 this bug would cause
us to have to change the layout of the device tree bindings. We
end up having to make a subnode for the pinctrl and move the
hogs into there, keeping the main pin settings on the MFD device
itself. And we know from the Madera series that such a DT
arrangement is not acceptable upstream. Meaning without this
patch on 4.14 our customers are forced to have DT bindings that
won't be compatible with a mainline kernel.

Thanks,
Charles

Re: [PATCH] pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs

[Linus Walleij]

On Wed, Mar 7, 2018 at 5:12 PM, Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Fri, Mar 02, 2018 at 09:42:54AM +0100, Linus Walleij wrote:
>> On Wed, Feb 28, 2018 at 4:53 PM, Richard Fitzgerald
>> <rf@opensource.cirrus.com> wrote:
>>
>> > When dt_to_map_one_config() is called with a pinctrl_dev passed
>> > in, it should only be using this if the node being looked up
>> > is a hog. The code was always using the passed pinctrl_dev
>> > without checking whether the dt node referred to it.
>> >
>> > A pin controller can have pinctrl-n dependencies on other pin
>> > controllers in these cases:
>> >
>> > - the pin controller hardware is external, for example I2C, so
>> >   needs other pin controller(s) to be setup to communicate with
>> >   the hardware device.
>> >
>> > - it is a child of a composite MFD so its of_node is shared with
>> >   the parent MFD and other children of that MFD. Any part of that
>> >   MFD could have dependencies on other pin controllers.
>> >
>> > Because of this, dt_to_map_one_config() can't assume that if it
>> > has a pinctrl_dev passed in then the node it looks up must be
>> > a hog. It could be a reference to some other pin controller.
>> >
>> > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>>
>> Seems correct. Also the code is a little more clear
>> after the patch.
>>
>> Patch applied.
>>
>> It'd be nice to see some testing of this, I hope linux-next is a good
>> testing ground.
>>
>
> Once we are happy on the testing front, how would you feel about
> getting this forwarded to 4.14 stable as well?

Sadly too late for me to tag as stable, but you can suggest to
Greg to pick it up for stable individually after v4.17-rc1.

Yours,
Linus Walleij