diff mbox series

Revert "pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs"

Message ID 1528321222-10193-1-git-send-email-festevam@gmail.com
State New
Headers show
Series Revert "pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs" | expand

Commit Message

Fabio Estevam June 6, 2018, 9:40 p.m. UTC 
From: Fabio Estevam <fabio.estevam@nxp.com>

This reverts commit b89405b6102fcc3746f43697b826028caa94c823.

commit b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config
handling of hogs") causes all the pinctrl hog pins to be ignored
leaving them with the IOMUX settings untouched.

This causes several regressions on i.MX such as:

- OV5640 camera driver can not be probed anymore on imx6qdl-sabresd
because the camera clock pin is in a pinctrl_hog group and since
its pinctrl initialization is skipped, the camera clock is kept
in GPIO functionality instead of CLK_CKO function.

- Audio stopped working on imx6qdl-wandboard and imx53-qsb for
the same reason.

Reported-by: Steve Longerbeam <slongerbeam@gmail.com>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/pinctrl/devicetree.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Richard Fitzgerald June 7, 2018, 10:14 a.m. UTC | #1 
On 06/06/18 22:40, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> This reverts commit b89405b6102fcc3746f43697b826028caa94c823.
> 
> commit b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config
> handling of hogs") causes all the pinctrl hog pins to be ignored
> leaving them with the IOMUX settings untouched.
> 
> This causes several regressions on i.MX such as:
> 

I don't mind you reverting it if you are also providing an alternative
fix for the bug in the original code.

Also by the way I'm not convinced that this patch "causes all the
pinctrl hog pins to be ignored" - it doesn't on our pin controller using
the generic pinctrl DT functionality. So there's something more specific
to the iMX configuration going on.

> - OV5640 camera driver can not be probed anymore on imx6qdl-sabresd
> because the camera clock pin is in a pinctrl_hog group and since
> its pinctrl initialization is skipped, the camera clock is kept
> in GPIO functionality instead of CLK_CKO function.
> 

Have you determined why the iMX driver's hogs fail the check
(np_pctldev == p->dev->of_node)? That would seem to be the reason
why it's not working for you and an important question to answer. It
implies that either your hog is not in its parent pinctrl DT node (so
how can it be a hog?) or I've missed other possible of_node
relationships that a hog is allowed to have to its parent pinctrl driver
node, in which case it would be better to do a patch to fix my if
statement rather than reintroduce the bug the iMX code relies on. My
understanding was that a "hog" is when a pinctrl entry is a child node
of the pinctrl driver that supplies it, therefore np_pctldev ==
p->dev->of_node.

> - Audio stopped working on imx6qdl-wandboard and imx53-qsb for
> the same reason.
> 
> Reported-by: Steve Longerbeam <slongerbeam@gmail.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>   drivers/pinctrl/devicetree.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index b601039..1ff6c3573 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -122,10 +122,8 @@ static int dt_to_map_one_config(struct pinctrl *p,
>   			/* OK let's just assume this will appear later then */
>   			return -EPROBE_DEFER;
>   		}
> -		/* 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)
> +			pctldev = get_pinctrl_dev_from_of_node(np_pctldev);

However these two lines you are putting back will apply any pinctrl
entry to pctldev even if it doesn't refer to pctldev. There could
be dependencies on other pin controllers and those are invariant
regardless of the value of pctldev passed into this function.

pinctrl_dt_to_map() walks through all phandles in the "pinctrl-n"
property and calls dt_to_map_one_config() on each. Those phandles could
point to nodes in other pin controllers. The original code would apply
all of them to pctldev because pctldev!=NULL.

>   		if (pctldev)
>   			break;
>   		/* Do not defer probing of hogs (circular loop) */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam June 7, 2018, 2:09 p.m. UTC | #2 
Hi Richard,

On Thu, Jun 7, 2018 at 7:14 AM, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:

> Have you determined why the iMX driver's hogs fail the check
> (np_pctldev == p->dev->of_node)? That would seem to be the reason

I applied the following change in the original 4.17 kernel:

--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -122,6 +122,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
                        /* OK let's just assume this will appear later then */
                        return -EPROBE_DEFER;
                }
+
+               if (np_pctldev == p->dev->of_node)
+                       dev_info(p->dev, "***** Matched: %s\n",
p->dev->of_node->name);
+               else
+                       dev_info(p->dev, "***** Not Matched: %s\n",
p->dev->of_node->name);
                /* If we're creating a hog we can use the passed pctldev */
                if (pctldev && (np_pctldev == p->dev->of_node))
                        break;

and this is what I get:

[    0.082643] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
[    0.082803] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
[    0.084386] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
[    0.084413] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
[    0.155791] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
[    0.155828] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
[    0.158001] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
[    0.158033] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
[    0.159389] mc13xxx 0-0008: ***** Not Matched: mc34708
[    0.159440] mc13xxx 0-0008: ***** Not Matched: mc34708
[    0.247099] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
[    0.247130] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
[    0.340774] imx-uart 53fbc000.serial: ***** Not Matched: serial
[    0.340819] imx-uart 53fbc000.serial: ***** Not Matched: serial
[    1.189378] imx-tve 63ff0000.tve: ***** Not Matched: tve
[    1.194917] imx-tve 63ff0000.tve: ***** Not Matched: tve
[    1.485778] fec 63fec000.ethernet: ***** Not Matched: ethernet
[    1.491713] fec 63fec000.ethernet: ***** Not Matched: ethernet
[    1.746872] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
[    1.753489] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
[    1.818500] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
[    1.825480] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
[    1.883043] leds-gpio leds: ***** Not Matched: leds
[    1.887976] leds-gpio leds: ***** Not Matched: leds


I am running imx53-qdb.dtb which includes
arch/arm/boot/dts/imx53-qsb-common.dtsi.

The pinctrl driver is drivers/pinctrl/freescale/pinctrl-imx53.c

Any ideas?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Fitzgerald June 7, 2018, 3:23 p.m. UTC | #3 
On 07/06/18 15:09, Fabio Estevam wrote:
> Hi Richard,
> 
> On Thu, Jun 7, 2018 at 7:14 AM, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> 
>> Have you determined why the iMX driver's hogs fail the check
>> (np_pctldev == p->dev->of_node)? That would seem to be the reason
> 
> I applied the following change in the original 4.17 kernel:
> 
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -122,6 +122,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
>                          /* OK let's just assume this will appear later then */
>                          return -EPROBE_DEFER;
>                  }
> +
> +               if (np_pctldev == p->dev->of_node)
> +                       dev_info(p->dev, "***** Matched: %s\n",
> p->dev->of_node->name);
> +               else
> +                       dev_info(p->dev, "***** Not Matched: %s\n",
> p->dev->of_node->name);
>                  /* If we're creating a hog we can use the passed pctldev */
>                  if (pctldev && (np_pctldev == p->dev->of_node))
>                          break;
> 
> and this is what I get:
> 
> [    0.082643] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
> [    0.082803] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
> [    0.084386] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
> [    0.084413] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
> [    0.155791] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
> [    0.155828] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
> [    0.158001] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
> [    0.158033] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
> [    0.159389] mc13xxx 0-0008: ***** Not Matched: mc34708
> [    0.159440] mc13xxx 0-0008: ***** Not Matched: mc34708
> [    0.247099] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
> [    0.247130] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
> [    0.340774] imx-uart 53fbc000.serial: ***** Not Matched: serial
> [    0.340819] imx-uart 53fbc000.serial: ***** Not Matched: serial
> [    1.189378] imx-tve 63ff0000.tve: ***** Not Matched: tve
> [    1.194917] imx-tve 63ff0000.tve: ***** Not Matched: tve
> [    1.485778] fec 63fec000.ethernet: ***** Not Matched: ethernet
> [    1.491713] fec 63fec000.ethernet: ***** Not Matched: ethernet
> [    1.746872] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
> [    1.753489] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
> [    1.818500] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
> [    1.825480] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
> [    1.883043] leds-gpio leds: ***** Not Matched: leds
> [    1.887976] leds-gpio leds: ***** Not Matched: leds
> 
> 
> I am running imx53-qdb.dtb which includes
> arch/arm/boot/dts/imx53-qsb-common.dtsi.
> 
> The pinctrl driver is drivers/pinctrl/freescale/pinctrl-imx53.c
> 
> Any ideas?
> 

Most likely the bug that my patch fixed was masking a different
problem with the iMX system.

In imx53-qsb-common.dtsi the iomuxc pinctrl driver node has some hogs
and also has many other pinctrl configurations that are not hogs and
are used by other drivers. What I think is happening is that because of
the bug in the original dt_to_map_one_config() all the child nodes under
iomux would have been treated as hogs, including the ones that are not
hogs. So when the iomuxc driver is probed all the possible pinctrl
configurations would be applied immediately and you would see correctly
configured pin muxes even though the way they got configured was wrong.

I assume your problem is that those settings which are not hogs are not
getting set up now? That indicates that for some reason when the
individual drivers call pinctrl_get() the mappings do not get applied.
So the interesting case to debug is what happens when
dt_to_map_one_config() is called with pctldev==NULL, why are the
pinctrl mappings requested by all those drivers not mapped to the
iomuxc driver?

Which of the pinctrl configuration are not applied? Are all missing?
Or only some?

In your log:

iomuxc - this is a pinctrl driver with hogs and the debug shows that
          the hogs were matched on the 2nd try (they are 2 levels down
          from the parent node)

          It also contains many other pinctrl configurations used by
          other drivers. These are _not_ hogs (because they are not used
          by the iomux driver).

All the other drivers are not pinctrl drivers and they have their own DT
nodes. So it is expected that np_pctldev != p->dev->of_node.
   np_pctldev is the pointer to the pinctrl definition node under &iomuxc
   p->dev_of_node points to the client driver DT node, like &esdhc1
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Fitzgerald June 7, 2018, 3:32 p.m. UTC | #4 
On 07/06/18 16:23, Richard Fitzgerald wrote:
> On 07/06/18 15:09, Fabio Estevam wrote:
>> Hi Richard,
>>
>> On Thu, Jun 7, 2018 at 7:14 AM, Richard Fitzgerald
>> <rf@opensource.cirrus.com> wrote:
>>
>>> Have you determined why the iMX driver's hogs fail the check
>>> (np_pctldev == p->dev->of_node)? That would seem to be the reason
>>
>> I applied the following change in the original 4.17 kernel:
>>
>> --- a/drivers/pinctrl/devicetree.c
>> +++ b/drivers/pinctrl/devicetree.c
>> @@ -122,6 +122,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
>>                          /* OK let's just assume this will appear 
>> later then */
>>                          return -EPROBE_DEFER;
>>                  }
>> +
>> +               if (np_pctldev == p->dev->of_node)
>> +                       dev_info(p->dev, "***** Matched: %s\n",
>> p->dev->of_node->name);
>> +               else
>> +                       dev_info(p->dev, "***** Not Matched: %s\n",
>> p->dev->of_node->name);
>>                  /* If we're creating a hog we can use the passed 
>> pctldev */
>>                  if (pctldev && (np_pctldev == p->dev->of_node))
>>                          break;
>>
>> and this is what I get:
>>
>> [    0.082643] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
>> [    0.082803] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
>> [    0.084386] imx53-pinctrl 53fa8000.iomuxc: ***** Not Matched: iomuxc
>> [    0.084413] imx53-pinctrl 53fa8000.iomuxc: ***** Matched: iomuxc
>> [    0.155791] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
>> [    0.155828] imx-i2c 63fc4000.i2c: ***** Not Matched: i2c
>> [    0.158001] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
>> [    0.158033] imx-i2c 63fc8000.i2c: ***** Not Matched: i2c
>> [    0.159389] mc13xxx 0-0008: ***** Not Matched: mc34708
>> [    0.159440] mc13xxx 0-0008: ***** Not Matched: mc34708
>> [    0.247099] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
>> [    0.247130] imx-audmux 63fd0000.audmux: ***** Not Matched: audmux
>> [    0.340774] imx-uart 53fbc000.serial: ***** Not Matched: serial
>> [    0.340819] imx-uart 53fbc000.serial: ***** Not Matched: serial
>> [    1.189378] imx-tve 63ff0000.tve: ***** Not Matched: tve
>> [    1.194917] imx-tve 63ff0000.tve: ***** Not Matched: tve
>> [    1.485778] fec 63fec000.ethernet: ***** Not Matched: ethernet
>> [    1.491713] fec 63fec000.ethernet: ***** Not Matched: ethernet
>> [    1.746872] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
>> [    1.753489] sdhci-esdhc-imx 50004000.esdhc: ***** Not Matched: esdhc
>> [    1.818500] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
>> [    1.825480] sdhci-esdhc-imx 50020000.esdhc: ***** Not Matched: esdhc
>> [    1.883043] leds-gpio leds: ***** Not Matched: leds
>> [    1.887976] leds-gpio leds: ***** Not Matched: leds
>>
>>
>> I am running imx53-qdb.dtb which includes
>> arch/arm/boot/dts/imx53-qsb-common.dtsi.
>>
>> The pinctrl driver is drivers/pinctrl/freescale/pinctrl-imx53.c
>>
>> Any ideas?
>>
> 
> Most likely the bug that my patch fixed was masking a different
> problem with the iMX system.
> 
> In imx53-qsb-common.dtsi the iomuxc pinctrl driver node has some hogs
> and also has many other pinctrl configurations that are not hogs and
> are used by other drivers. What I think is happening is that because of
> the bug in the original dt_to_map_one_config() all the child nodes under
> iomux would have been treated as hogs, including the ones that are not
> hogs. So when the iomuxc driver is probed all the possible pinctrl
> configurations would be applied immediately and you would see correctly
> configured pin muxes even though the way they got configured was wrong.
> 
> I assume your problem is that those settings which are not hogs are not
> getting set up now? That indicates that for some reason when the
> individual drivers call pinctrl_get() the mappings do not get applied.
> So the interesting case to debug is what happens when
> dt_to_map_one_config() is called with pctldev==NULL, why are the
> pinctrl mappings requested by all those drivers not mapped to the
> iomuxc driver?
> 
> Which of the pinctrl configuration are not applied? Are all missing?
> Or only some?
> 
> In your log:
> 
> iomuxc - this is a pinctrl driver with hogs and the debug shows that
>           the hogs were matched on the 2nd try (they are 2 levels down
>           from the parent node)
> 
>           It also contains many other pinctrl configurations used by
>           other drivers. These are _not_ hogs (because they are not used
>           by the iomux driver).
> 
> All the other drivers are not pinctrl drivers and they have their own DT
> nodes. So it is expected that np_pctldev != p->dev->of_node.
>    np_pctldev is the pointer to the pinctrl definition node under &iomuxc
>    p->dev_of_node points to the client driver DT node, like &esdhc1

Another thing, you should be extremely careful about using "default"
state because this probably doesn't do what you think. What it does is

"Try to apply these dependencies before calling the driver probe() BUT
DO NOT ABORT PROBE IF THE PINCTRL MAPPING FAILS, GO AHEAD AND CALL
probe() ANYWAY".

Thus, even if the driver core failed to apply your pinctrl "default"
mapping, it will still probe your driver. You can't assume that
"default" will have been applied when your driver probes. It's much
safer to define your own state and explicitly request it from your
driver probe, so that you can EPROBE_DEFER if the pinctrl mapping fails.
You might be able to explicitly request "default" but I seem to recall
there was a reason that didn't work.

(We've had a look at making "default" behave as you might expect but it
looked difficult to do and likely to break so many things it's easier to
avoid it.)
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam June 7, 2018, 3:44 p.m. UTC | #5 
On Thu, Jun 7, 2018 at 12:23 PM, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:

> Most likely the bug that my patch fixed was masking a different
> problem with the iMX system.
>
> In imx53-qsb-common.dtsi the iomuxc pinctrl driver node has some hogs
> and also has many other pinctrl configurations that are not hogs and
> are used by other drivers. What I think is happening is that because of
> the bug in the original dt_to_map_one_config() all the child nodes under
> iomux would have been treated as hogs, including the ones that are not
> hogs. So when the iomuxc driver is probed all the possible pinctrl
> configurations would be applied immediately and you would see correctly
> configured pin muxes even though the way they got configured was wrong.
>
> I assume your problem is that those settings which are not hogs are not
> getting set up now? That indicates that for some reason when the

No, only pinctrl_hog group is not being set up now.

> individual drivers call pinctrl_get() the mappings do not get applied.
> So the interesting case to debug is what happens when
> dt_to_map_one_config() is called with pctldev==NULL, why are the
> pinctrl mappings requested by all those drivers not mapped to the
> iomuxc driver?
>
> Which of the pinctrl configuration are not applied? Are all missing?
> Or only some?

Only pinctrl_hog configuration is not taking effect.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Fitzgerald June 7, 2018, 3:45 p.m. UTC | #6 
On 07/06/18 16:44, Fabio Estevam wrote:
> On Thu, Jun 7, 2018 at 12:23 PM, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> 
>> Most likely the bug that my patch fixed was masking a different
>> problem with the iMX system.
>>
>> In imx53-qsb-common.dtsi the iomuxc pinctrl driver node has some hogs
>> and also has many other pinctrl configurations that are not hogs and
>> are used by other drivers. What I think is happening is that because of
>> the bug in the original dt_to_map_one_config() all the child nodes under
>> iomux would have been treated as hogs, including the ones that are not
>> hogs. So when the iomuxc driver is probed all the possible pinctrl
>> configurations would be applied immediately and you would see correctly
>> configured pin muxes even though the way they got configured was wrong.
>>
>> I assume your problem is that those settings which are not hogs are not
>> getting set up now? That indicates that for some reason when the
> 
> No, only pinctrl_hog group is not being set up now.
> 
>> individual drivers call pinctrl_get() the mappings do not get applied.
>> So the interesting case to debug is what happens when
>> dt_to_map_one_config() is called with pctldev==NULL, why are the
>> pinctrl mappings requested by all those drivers not mapped to the
>> iomuxc driver?
>>
>> Which of the pinctrl configuration are not applied? Are all missing?
>> Or only some?
> 
> Only pinctrl_hog configuration is not taking effect.
> 

Interesting, I'll take another look with that information. Your log
shows that it matches.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Fitzgerald June 7, 2018, 4:17 p.m. UTC | #7 
On 07/06/18 16:44, Fabio Estevam wrote:
> On Thu, Jun 7, 2018 at 12:23 PM, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> 
>> Most likely the bug that my patch fixed was masking a different
>> problem with the iMX system.
>>
>> In imx53-qsb-common.dtsi the iomuxc pinctrl driver node has some hogs
>> and also has many other pinctrl configurations that are not hogs and
>> are used by other drivers. What I think is happening is that because of
>> the bug in the original dt_to_map_one_config() all the child nodes under
>> iomux would have been treated as hogs, including the ones that are not
>> hogs. So when the iomuxc driver is probed all the possible pinctrl
>> configurations would be applied immediately and you would see correctly
>> configured pin muxes even though the way they got configured was wrong.
>>
>> I assume your problem is that those settings which are not hogs are not
>> getting set up now? That indicates that for some reason when the
> 
> No, only pinctrl_hog group is not being set up now.
> 
>> individual drivers call pinctrl_get() the mappings do not get applied.
>> So the interesting case to debug is what happens when
>> dt_to_map_one_config() is called with pctldev==NULL, why are the
>> pinctrl mappings requested by all those drivers not mapped to the
>> iomuxc driver?
>>
>> Which of the pinctrl configuration are not applied? Are all missing?
>> Or only some?
> 
> Only pinctrl_hog configuration is not taking effect.
> 

I see the bug. If the hog node isn't a 1st level child of the pinctrl
parent node it will go around the for(;;) loop again but on the first
pass I overwrite pctldev with the result of
get_pinctrl_dev_from_of_node() so it doesn't point to the pinctrl driver
any more.

It's end of the day here, I'll do a patch tomorrow to stash the original
pctldev so it doesn't get overwritten.

Something like this should work.

1) Rename the "pctldev" argument to the function to "hog_pctldev"
2) Add a local variable
	struct pinctrl_dev *pctldev = NULL;

3) Change the if statement to:

		if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
			pctrldev = hog_pctldev;
			break;
		}

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam June 7, 2018, 4:42 p.m. UTC | #8 
Hi Richard,

On Thu, Jun 7, 2018 at 1:17 PM, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:

> I see the bug. If the hog node isn't a 1st level child of the pinctrl
> parent node it will go around the for(;;) loop again but on the first
> pass I overwrite pctldev with the result of
> get_pinctrl_dev_from_of_node() so it doesn't point to the pinctrl driver
> any more.
>
> It's end of the day here, I'll do a patch tomorrow to stash the original
> pctldev so it doesn't get overwritten.
>
> Something like this should work.
>
> 1) Rename the "pctldev" argument to the function to "hog_pctldev"
> 2) Add a local variable
>         struct pinctrl_dev *pctldev = NULL;
>
> 3) Change the if statement to:
>
>                 if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
>                         pctrldev = hog_pctldev;
>                         break;
>                 }
>

Yes, this works fine. Thanks.

Will send your suggested fix.

Thanks for your help!
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index b601039..1ff6c3573 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -122,10 +122,8 @@  static int dt_to_map_one_config(struct pinctrl *p,
 			/* OK let's just assume this will appear later then */
 			return -EPROBE_DEFER;
 		}
-		/* 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)
+			pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
 		if (pctldev)
 			break;
 		/* Do not defer probing of hogs (circular loop) */