[PATCH 1/3] pinctrl: don't fall back to pinctrl_select_state_simple()

[PATCH 1/3] pinctrl: don't fall back to pinctrl_select_state_simple()

[Michael Walle]

If CONFIG_PINCTRL_FULL is enabled, never fall back to the simple
implementation. pinctrl_select_state() is called for each device and it
is expected to fail. A fallback to the simple imeplementation doesn't
make much sense.

To keep the return code consistent, we need to change the -EINVAL (which
was ignored before) to -ENOSYS.

Signed-off-by: Michael Walle <michael@walle.cc>
---
The real underlying problem is that the pinctrl_select_state_simple()
doesn't really work before relocation. This implementation calls
uclass_get_device() which will try to probe the pinctrl, but that device
might not be ready yet, i.e. in my case, it is an MFD and it needs its
parent. For each device in the devicetree, there will be a
device_probe(pinctrl) attempt and if that device has some kind of
private or platform data, it will eat away the early memory, because
there is no free.

I'm not really sure how that could be fixed. But still, if there is a
full implementation, it shouldn't fall back to the simple one.
---
 drivers/pinctrl/pinctrl-uclass.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index ce2d5ddf6d..419ef5f52f 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -71,13 +71,13 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
 		 */
 		state = dectoul(statename, &end);
 		if (*end)
-			return -EINVAL;
+			return -ENOSYS;
 	}
 
 	snprintf(propname, sizeof(propname), "pinctrl-%d", state);
 	list = dev_read_prop(dev, propname, &size);
 	if (!list)
-		return -EINVAL;
+		return -ENOSYS;
 
 	size /= sizeof(*list);
 	for (i = 0; i < size; i++) {
@@ -162,11 +162,6 @@ U_BOOT_DRIVER(pinconfig_generic) = {
 };
 
 #else
-static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
-{
-	return -ENODEV;
-}
-
 static int pinconfig_post_bind(struct udevice *dev)
 {
 	return 0;
@@ -317,10 +312,10 @@ int pinctrl_select_state(struct udevice *dev, const char *statename)
 	 * Try full-implemented pinctrl first.
 	 * If it fails or is not implemented, try simple one.
 	 */
-	if (pinctrl_select_state_full(dev, statename))
-		return pinctrl_select_state_simple(dev);
+	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
+		return pinctrl_select_state_full(dev, statename);
 
-	return 0;
+	return pinctrl_select_state_simple(dev);
 }
 
 int pinctrl_request(struct udevice *dev, int func, int flags)
-- 
2.30.2

[PATCH 2/3] pinctrl: get rid of some ifdeffery

[Michael Walle]

Don't define an empty version for pinconfig_post_bind(). Just guard the
call and let the linker garbage collection do the rest. This way, we
also don't have to do any guesswork.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-uclass.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index 419ef5f52f..e6cd0889b0 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -20,7 +20,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if CONFIG_IS_ENABLED(PINCTRL_FULL)
 /**
  * pinctrl_config_one() - apply pinctrl settings for a single node
  *
@@ -148,6 +147,7 @@ static int pinconfig_post_bind(struct udevice *dev)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(PINCTRL_FULL)
 UCLASS_DRIVER(pinconfig) = {
 	.id = UCLASS_PINCONFIG,
 #if CONFIG_IS_ENABLED(PINCONF_RECURSIVE)
@@ -160,12 +160,6 @@ U_BOOT_DRIVER(pinconfig_generic) = {
 	.name = "pinconfig",
 	.id = UCLASS_PINCONFIG,
 };
-
-#else
-static int pinconfig_post_bind(struct udevice *dev)
-{
-	return 0;
-}
 #endif
 
 static int
@@ -411,12 +405,11 @@ static int __maybe_unused pinctrl_post_bind(struct udevice *dev)
 	}
 
 	/*
-	 * If set_state callback is set, we assume this pinctrl driver is the
-	 * full implementation.  In this case, its child nodes should be bound
-	 * so that peripheral devices can easily search in parent devices
-	 * during later DT-parsing.
+	 * If the pinctrl driver has the full implementation, its child nodes
+	 * should be bound so that peripheral devices can easily search in
+	 * parent devices during later DT-parsing.
 	 */
-	if (ops->set_state)
+	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
 		return pinconfig_post_bind(dev);
 
 	return 0;
-- 
2.30.2

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Marek Vasut]

On 1/18/23 13:12, Michael Walle wrote:

[...]

> @@ -411,12 +405,11 @@ static int __maybe_unused pinctrl_post_bind(struct udevice *dev)
>   	}
>   
>   	/*
> -	 * If set_state callback is set, we assume this pinctrl driver is the
> -	 * full implementation.  In this case, its child nodes should be bound
> -	 * so that peripheral devices can easily search in parent devices
> -	 * during later DT-parsing.
> +	 * If the pinctrl driver has the full implementation, its child nodes
> +	 * should be bound so that peripheral devices can easily search in
> +	 * parent devices during later DT-parsing.
>   	 */
> -	if (ops->set_state)
> +	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
>   		return pinconfig_post_bind(dev);

Is it correct to drop the ops->set_state non-NULL check ?

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Michael Walle]

Am 2023-01-18 13:18, schrieb Marek Vasut:
> On 1/18/23 13:12, Michael Walle wrote:
> 
> [...]
> 
>> @@ -411,12 +405,11 @@ static int __maybe_unused 
>> pinctrl_post_bind(struct udevice *dev)
>>   	}
>>     	/*
>> -	 * If set_state callback is set, we assume this pinctrl driver is 
>> the
>> -	 * full implementation.  In this case, its child nodes should be 
>> bound
>> -	 * so that peripheral devices can easily search in parent devices
>> -	 * during later DT-parsing.
>> +	 * If the pinctrl driver has the full implementation, its child 
>> nodes
>> +	 * should be bound so that peripheral devices can easily search in
>> +	 * parent devices during later DT-parsing.
>>   	 */
>> -	if (ops->set_state)
>> +	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
>>   		return pinconfig_post_bind(dev);
> 
> Is it correct to drop the ops->set_state non-NULL check ?

See include/dm/pinctrl.h. set_state() is mandatory. Although,
that seems to be wrong for the simple implementation.

-michael

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Marek Vasut]

On 1/18/23 13:43, Michael Walle wrote:
> Am 2023-01-18 13:18, schrieb Marek Vasut:
>> On 1/18/23 13:12, Michael Walle wrote:
>>
>> [...]
>>
>>> @@ -411,12 +405,11 @@ static int __maybe_unused 
>>> pinctrl_post_bind(struct udevice *dev)
>>>       }
>>>         /*
>>> -     * If set_state callback is set, we assume this pinctrl driver 
>>> is the
>>> -     * full implementation.  In this case, its child nodes should be 
>>> bound
>>> -     * so that peripheral devices can easily search in parent devices
>>> -     * during later DT-parsing.
>>> +     * If the pinctrl driver has the full implementation, its child 
>>> nodes
>>> +     * should be bound so that peripheral devices can easily search in
>>> +     * parent devices during later DT-parsing.
>>>        */
>>> -    if (ops->set_state)
>>> +    if (CONFIG_IS_ENABLED(PINCTRL_FULL))
>>>           return pinconfig_post_bind(dev);
>>
>> Is it correct to drop the ops->set_state non-NULL check ?
> 
> See include/dm/pinctrl.h. set_state() is mandatory. Although,
> that seems to be wrong for the simple implementation.

Could that be fixed for the simple case ? I think that should be easy to 
do, right ?

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Michael Walle]

Am 2023-01-18 14:08, schrieb Marek Vasut:
> On 1/18/23 13:43, Michael Walle wrote:
>> Am 2023-01-18 13:18, schrieb Marek Vasut:
>>> On 1/18/23 13:12, Michael Walle wrote:
>>> 
>>> [...]
>>> 
>>>> @@ -411,12 +405,11 @@ static int __maybe_unused 
>>>> pinctrl_post_bind(struct udevice *dev)
>>>>       }
>>>>         /*
>>>> -     * If set_state callback is set, we assume this pinctrl driver 
>>>> is the
>>>> -     * full implementation.  In this case, its child nodes should 
>>>> be bound
>>>> -     * so that peripheral devices can easily search in parent 
>>>> devices
>>>> -     * during later DT-parsing.
>>>> +     * If the pinctrl driver has the full implementation, its child 
>>>> nodes
>>>> +     * should be bound so that peripheral devices can easily search 
>>>> in
>>>> +     * parent devices during later DT-parsing.
>>>>        */
>>>> -    if (ops->set_state)
>>>> +    if (CONFIG_IS_ENABLED(PINCTRL_FULL))
>>>>           return pinconfig_post_bind(dev);
>>> 
>>> Is it correct to drop the ops->set_state non-NULL check ?
>> 
>> See include/dm/pinctrl.h. set_state() is mandatory. Although,
>> that seems to be wrong for the simple implementation.
> 
> Could that be fixed for the simple case ? I think that should be easy
> to do, right ?

You mean a documentation fix?

-michael

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Marek Vasut]

On 1/18/23 14:10, Michael Walle wrote:
> Am 2023-01-18 14:08, schrieb Marek Vasut:
>> On 1/18/23 13:43, Michael Walle wrote:
>>> Am 2023-01-18 13:18, schrieb Marek Vasut:
>>>> On 1/18/23 13:12, Michael Walle wrote:
>>>>
>>>> [...]
>>>>
>>>>> @@ -411,12 +405,11 @@ static int __maybe_unused 
>>>>> pinctrl_post_bind(struct udevice *dev)
>>>>>       }
>>>>>         /*
>>>>> -     * If set_state callback is set, we assume this pinctrl driver 
>>>>> is the
>>>>> -     * full implementation.  In this case, its child nodes should 
>>>>> be bound
>>>>> -     * so that peripheral devices can easily search in parent devices
>>>>> -     * during later DT-parsing.
>>>>> +     * If the pinctrl driver has the full implementation, its 
>>>>> child nodes
>>>>> +     * should be bound so that peripheral devices can easily 
>>>>> search in
>>>>> +     * parent devices during later DT-parsing.
>>>>>        */
>>>>> -    if (ops->set_state)
>>>>> +    if (CONFIG_IS_ENABLED(PINCTRL_FULL))
>>>>>           return pinconfig_post_bind(dev);
>>>>
>>>> Is it correct to drop the ops->set_state non-NULL check ?
>>>
>>> See include/dm/pinctrl.h. set_state() is mandatory. Although,
>>> that seems to be wrong for the simple implementation.
>>
>> Could that be fixed for the simple case ? I think that should be easy
>> to do, right ?
> 
> You mean a documentation fix?

Reading the original comment one more time, no, I think this patch is 
fine as is, sorry for the noise.

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

Re: [PATCH 2/3] pinctrl: get rid of some ifdeffery

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Wed, Jan 18, 2023 at 01:12:23PM +0100, Michael Walle wrote:

> Don't define an empty version for pinconfig_post_bind(). Just guard the
> call and let the linker garbage collection do the rest. This way, we
> also don't have to do any guesswork.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Marek Vasut <marex@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/3] pinctrl: fix docstring

[Michael Walle]

Fix the copy and paste error.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index e6cd0889b0..23a1504716 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -382,7 +382,7 @@ int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
 }
 
 /**
- * pinconfig_post_bind() - post binding for PINCTRL uclass
+ * pinctrl_post_bind() - post binding for PINCTRL uclass
  * Recursively bind child nodes as pinconfig devices in case of full pinctrl.
  *
  * @dev: pinctrl device
-- 
2.30.2

Re: [PATCH 3/3] pinctrl: fix docstring

[Simon Glass]

On Wed, 18 Jan 2023 at 05:13, Michael Walle <michael@walle.cc> wrote:
>
> Fix the copy and paste error.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pinctrl/pinctrl-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/3] pinctrl: fix docstring

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

On Wed, Jan 18, 2023 at 01:12:24PM +0100, Michael Walle wrote:

> Fix the copy and paste error.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/3] pinctrl: don't fall back to pinctrl_select_state_simple()

[Marek Vasut]

On 1/18/23 13:12, Michael Walle wrote:
> If CONFIG_PINCTRL_FULL is enabled, never fall back to the simple
> implementation. pinctrl_select_state() is called for each device and it
> is expected to fail. A fallback to the simple imeplementation doesn't
> make much sense.
> 
> To keep the return code consistent, we need to change the -EINVAL (which
> was ignored before) to -ENOSYS.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

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

Re: [PATCH 1/3] pinctrl: don't fall back to pinctrl_select_state_simple()

[Simon Glass]

On Wed, 18 Jan 2023 at 06:45, Marek Vasut <marex@denx.de> wrote:
>
> On 1/18/23 13:12, Michael Walle wrote:
> > If CONFIG_PINCTRL_FULL is enabled, never fall back to the simple
> > implementation. pinctrl_select_state() is called for each device and it
> > is expected to fail. A fallback to the simple imeplementation doesn't
> > make much sense.
> >
> > To keep the return code consistent, we need to change the -EINVAL (which
> > was ignored before) to -ENOSYS.
> >
> > Signed-off-by: Michael Walle <michael@walle.cc>
>
> Reviewed-by: Marek Vasut <marex@denx.de>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/3] pinctrl: don't fall back to pinctrl_select_state_simple()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Wed, Jan 18, 2023 at 01:12:22PM +0100, Michael Walle wrote:

> If CONFIG_PINCTRL_FULL is enabled, never fall back to the simple
> implementation. pinctrl_select_state() is called for each device and it
> is expected to fail. A fallback to the simple imeplementation doesn't
> make much sense.
> 
> To keep the return code consistent, we need to change the -EINVAL (which
> was ignored before) to -ENOSYS.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]