[PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Laxman Dewangan]

Recent movement of all configurations of pin in the single call of
pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
is selected as return of configuration.

The original idea was to just avoid any update on register for pull up/down
configuration if this option is selected.

Fixing this by just bypassing any register update for BIAS_PULL_PIN_DEFAULT
and continuing the remaining configuration.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/pinctrl/pinctrl-palmas.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-palmas.c b/drivers/pinctrl/pinctrl-palmas.c
index 82638fa..30c4d35 100644
--- a/drivers/pinctrl/pinctrl-palmas.c
+++ b/drivers/pinctrl/pinctrl-palmas.c
@@ -891,9 +891,10 @@ static int palmas_pinconf_set(struct pinctrl_dev *pctldev,
 		param = pinconf_to_config_param(configs[i]);
 		param_val = pinconf_to_config_argument(configs[i]);
 
+		if (param == PIN_CONFIG_BIAS_PULL_PIN_DEFAULT)
+			continue;
+
 		switch (param) {
-		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
-			return 0;
 		case PIN_CONFIG_BIAS_DISABLE:
 		case PIN_CONFIG_BIAS_PULL_UP:
 		case PIN_CONFIG_BIAS_PULL_DOWN:
-- 
1.7.1.1

[PATCH 2/2] pinctrl: palmas: remove non-require function

[Laxman Dewangan]

Palmas pinmux and pin configuration support the single pin level
configuration in place of pin group.

Hence it is only require to pin_config_{set|get} and do not require
pin_config_group_{set|get}. As core framework already check for
require APIs availability, it is not require to implement as dummy
for non-require ops and so removing it.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/pinctrl/pinctrl-palmas.c |   17 -----------------
 1 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-palmas.c b/drivers/pinctrl/pinctrl-palmas.c
index 30c4d35..6164381 100644
--- a/drivers/pinctrl/pinctrl-palmas.c
+++ b/drivers/pinctrl/pinctrl-palmas.c
@@ -962,26 +962,9 @@ static int palmas_pinconf_set(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static int palmas_pinconf_group_get(struct pinctrl_dev *pctldev,
-				unsigned group, unsigned long *config)
-{
-	dev_err(pctldev->dev, "palmas_pinconf_group_get op not supported\n");
-	return -ENOTSUPP;
-}
-
-static int palmas_pinconf_group_set(struct pinctrl_dev *pctldev,
-				unsigned group, unsigned long *configs,
-				unsigned num_configs)
-{
-	dev_err(pctldev->dev, "palmas_pinconf_group_set op not supported\n");
-	return -ENOTSUPP;
-}
-
 static const struct pinconf_ops palmas_pinconf_ops = {
 	.pin_config_get = palmas_pinconf_get,
 	.pin_config_set = palmas_pinconf_set,
-	.pin_config_group_get = palmas_pinconf_group_get,
-	.pin_config_group_set = palmas_pinconf_group_set,
 };
 
 static struct pinctrl_desc palmas_pinctrl_desc = {
-- 
1.7.1.1

Re: [PATCH 2/2] pinctrl: palmas: remove non-require function

[Linus Walleij]

On Thu, Sep 26, 2013 at 2:48 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> Palmas pinmux and pin configuration support the single pin level
> configuration in place of pin group.
>
> Hence it is only require to pin_config_{set|get} and do not require
> pin_config_group_{set|get}. As core framework already check for
> require APIs availability, it is not require to implement as dummy
> for non-require ops and so removing it.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Stephen Warren]

On 09/26/2013 06:48 AM, Laxman Dewangan wrote:
> Recent movement of all configurations of pin in the single call of
> pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
> is selected as return of configuration.
> 
> The original idea was to just avoid any update on register for pull up/down
> configuration if this option is selected.

That doesn't sound correct. If a config option is specified in DT or the
mapping table, it should be applied to HW. If someone doesn't want a
particular config option applied, then it simply shouldn't be mentioned
in DT or the mapping table.

IIUC, BIAS_DEFAULT should be used only on HW where there is a concept of
a true default bias, and in that case, that is what should be applied.

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Laxman Dewangan]

On Thursday 26 September 2013 09:08 PM, Stephen Warren wrote:
> On 09/26/2013 06:48 AM, Laxman Dewangan wrote:
>> Recent movement of all configurations of pin in the single call of
>> pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
>> is selected as return of configuration.
>>
>> The original idea was to just avoid any update on register for pull up/down
>> configuration if this option is selected.
> That doesn't sound correct. If a config option is specified in DT or the
> mapping table, it should be applied to HW. If someone doesn't want a
> particular config option applied, then it simply shouldn't be mentioned
> in DT or the mapping table.
>
> IIUC, BIAS_DEFAULT should be used only on HW where there is a concept of
> a true default bias, and in that case, that is what should be applied.
>

Hmm.. When I added the PIN_DEFAULT, I just though that do not update 
anything in the register and implemented like that.
There is nothing "default" option in HW.

In this patch, I just fixed the broken stuff due to recent change.

However, it the PIN_DEFAULT is not valid option then I need to remove it 
from driver as well as from dt document of Palmas.

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Stephen Warren]

On 09/27/2013 07:30 AM, Laxman Dewangan wrote:
> On Thursday 26 September 2013 09:08 PM, Stephen Warren wrote:
>> On 09/26/2013 06:48 AM, Laxman Dewangan wrote:
>>> Recent movement of all configurations of pin in the single call of
>>> pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
>>> is selected as return of configuration.
>>>
>>> The original idea was to just avoid any update on register for pull
>>> up/down
>>> configuration if this option is selected.
>> That doesn't sound correct. If a config option is specified in DT or the
>> mapping table, it should be applied to HW. If someone doesn't want a
>> particular config option applied, then it simply shouldn't be mentioned
>> in DT or the mapping table.
>>
>> IIUC, BIAS_DEFAULT should be used only on HW where there is a concept of
>> a true default bias, and in that case, that is what should be applied.
>>
> 
> Hmm.. When I added the PIN_DEFAULT, I just though that do not update
> anything in the register and implemented like that.
> There is nothing "default" option in HW.

The description of that pinconfig option is:

> 7970cb77 (Heiko Stübner    2013-06-06 16:44:25 +0200  43)  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  44)  *    on embedded knowledge of the controller hardware, like current mux
> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  45)  *    function. The pull direction and possibly strength too will normally
> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  46)  *    be decided completely inside the hardware block and not be readable
> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  47)  *    from the kernel side.
> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  48)  *    If the argument is != 0 pull up/down is enabled, if it is 0, the
> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  49)  *    configuration is ignored. The proper way to disable it is to use
> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  50)  *    @PIN_CONFIG_BIAS_DISABLE.

If the HW doesn't support any concept of a default pull, I think the
driver shouldn't support that option; it should return an error if asked
to program it.

> In this patch, I just fixed the broken stuff due to recent change.

> However, it the PIN_DEFAULT is not valid option then I need to remove it
> from driver as well as from dt document of Palmas.

Yes.

But what made you come across this issue? Is some pin mapping table or
DT pinctrl node actually using that value? If so, then presumably that
needs to be fixed, as well as removing driver support for that option.

Presumably given this, LinusW shouldn't have actually applied this
patch, since presumably it prevents any other driver from accepting
PIN_CONFIG_BIAS_DISABLE even in cases where it is appropriate?

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Laxman Dewangan]

On Friday 27 September 2013 09:36 PM, Stephen Warren wrote:
> On 09/27/2013 07:30 AM, Laxman Dewangan wrote:
>> On Thursday 26 September 2013 09:08 PM, Stephen Warren wrote:
>>> On 09/26/2013 06:48 AM, Laxman Dewangan wrote:
>>>> Recent movement of all configurations of pin in the single call of
>>>> pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
>>>> is selected as return of configuration.
>>>>
>>>> The original idea was to just avoid any update on register for pull
>>>> up/down
>>>> configuration if this option is selected.
>>> That doesn't sound correct. If a config option is specified in DT or the
>>> mapping table, it should be applied to HW. If someone doesn't want a
>>> particular config option applied, then it simply shouldn't be mentioned
>>> in DT or the mapping table.
>>>
>>> IIUC, BIAS_DEFAULT should be used only on HW where there is a concept of
>>> a true default bias, and in that case, that is what should be applied.
>>>
>> Hmm.. When I added the PIN_DEFAULT, I just though that do not update
>> anything in the register and implemented like that.
>> There is nothing "default" option in HW.
> The description of that pinconfig option is:
>
>> 7970cb77 (Heiko Stübner    2013-06-06 16:44:25 +0200  43)  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  44)  *    on embedded knowledge of the controller hardware, like current mux
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  45)  *    function. The pull direction and possibly strength too will normally
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  46)  *    be decided completely inside the hardware block and not be readable
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  47)  *    from the kernel side.
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  48)  *    If the argument is != 0 pull up/down is enabled, if it is 0, the
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  49)  *    configuration is ignored. The proper way to disable it is to use
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  50)  *    @PIN_CONFIG_BIAS_DISABLE.
> If the HW doesn't support any concept of a default pull, I think the
> driver shouldn't support that option; it should return an error if asked
> to program it.

Yes, I will remove this option as I have not seen default option for pins.

>
>
> But what made you come across this issue? Is some pin mapping table or
> DT pinctrl node actually using that value? If so, then presumably that
> needs to be fixed, as well as removing driver support for that option.

When referring the code for the AMSAS3722 pincontrol driver, I just 
found that it is breaking the earlier code.

Removing this option makes more reasonable here and will post the next 
patch.

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Linus Walleij]

On Fri, Sep 27, 2013 at 6:06 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> [Laxman]
>> Hmm.. When I added the PIN_DEFAULT, I just though that do not update
>> anything in the register and implemented like that.
>> There is nothing "default" option in HW.
>
> The description of that pinconfig option is:
>
>> 7970cb77 (Heiko Stübner    2013-06-06 16:44:25 +0200  43)  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  44)  *    on embedded knowledge of the controller hardware, like current mux
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  45)  *    function. The pull direction and possibly strength too will normally
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  46)  *    be decided completely inside the hardware block and not be readable
>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  47)  *    from the kernel side.
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  48)  *    If the argument is != 0 pull up/down is enabled, if it is 0, the
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  49)  *    configuration is ignored. The proper way to disable it is to use
>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  50)  *    @PIN_CONFIG_BIAS_DISABLE.
>
> If the HW doesn't support any concept of a default pull, I think the
> driver shouldn't support that option; it should return an error if asked
> to program it.

Yes that's how I remember it and how we specified it.
Correct Heiko?

> Presumably given this, LinusW shouldn't have actually applied this
> patch, since presumably it prevents any other driver from accepting
> PIN_CONFIG_BIAS_DISABLE even in cases where it is appropriate?

There are many patches I shouldn't have applied ...

Anyway I'm not quite following, this patch affected the Palmas
driver only I think so how can it prevent any other drivers from doing
the right thing?

We'll just have to follow up with a patch that return -EINVAL for
PIN_CONFIG_BIAS_DISABLE I guess, Laxman is this what you're
doing now?

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Laxman Dewangan]

On Wednesday 02 October 2013 04:10 PM, Linus Walleij wrote:
> On Fri, Sep 27, 2013 at 6:06 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Presumably given this, LinusW shouldn't have actually applied this
> patch, since presumably it prevents any other driver from accepting
> PIN_CONFIG_BIAS_DISABLE even in cases where it is appropriate?
> There are many patches I shouldn't have applied ...
>
> Anyway I'm not quite following, this patch affected the Palmas
> driver only I think so how can it prevent any other drivers from doing
> the right thing?
>
> We'll just have to follow up with a patch that return -EINVAL for
> PIN_CONFIG_BIAS_DISABLE I guess, Laxman is this what you're
> doing now?
>

Yes, I am going to remove this option so that it can fall to default 
where it says

dev_err(pci->dev, "Properties not supported\n");
return -ENOTSUPP;


Will post the patch soon.

Thanks,
Laxman

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Heiko Stübner]

Am Mittwoch, 2. Oktober 2013, 12:40:14 schrieb Linus Walleij:
> On Fri, Sep 27, 2013 at 6:06 PM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> > [Laxman]
> > 
> >> Hmm.. When I added the PIN_DEFAULT, I just though that do not update
> >> anything in the register and implemented like that.
> >> There is nothing "default" option in HW.
> > 
> > The description of that pinconfig option is:
> >> 7970cb77 (Heiko Stübner    2013-06-06 16:44:25 +0200  43)  *
> >> @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down
> >> based 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  44)  *   
> >> on embedded knowledge of the controller hardware, like current mux
> >> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  45)  *   
> >> function. The pull direction and possibly strength too will normally
> >> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  46)  *    be
> >> decided completely inside the hardware block and not be readable
> >> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  47)  *    from
> >> the kernel side. 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200 
> >> 48)  *    If the argument is != 0 pull up/down is enabled, if it is 0,
> >> the 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  49)  *   
> >> configuration is ignored. The proper way to disable it is to use
> >> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  50)  *   
> >> @PIN_CONFIG_BIAS_DISABLE.
> > 
> > If the HW doesn't support any concept of a default pull, I think the
> > driver shouldn't support that option; it should return an error if asked
> > to program it.
> 
> Yes that's how I remember it and how we specified it.
> Correct Heiko?

Correct. The option was/is meant for hardware that sets pull settings without 
telling the kernel specifics and the kernel also not being able to influence 
them.


Heiko

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Stephen Warren]

On 10/02/2013 04:40 AM, Linus Walleij wrote:
> On Fri, Sep 27, 2013 at 6:06 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> [Laxman]
>>> Hmm.. When I added the PIN_DEFAULT, I just though that do not update
>>> anything in the register and implemented like that.
>>> There is nothing "default" option in HW.
>>
>> The description of that pinconfig option is:
>>
>>> 7970cb77 (Heiko Stübner    2013-06-06 16:44:25 +0200  43)  * @PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: the pin will be pulled up or down based
>>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  44)  *    on embedded knowledge of the controller hardware, like current mux
>>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  45)  *    function. The pull direction and possibly strength too will normally
>>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  46)  *    be decided completely inside the hardware block and not be readable
>>> 70637a6d (Heiko Stübner    2013-06-25 14:55:42 +0200  47)  *    from the kernel side.
>>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  48)  *    If the argument is != 0 pull up/down is enabled, if it is 0, the
>>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  49)  *    configuration is ignored. The proper way to disable it is to use
>>> 5ca3353b (Linus Walleij    2013-06-16 12:43:06 +0200  50)  *    @PIN_CONFIG_BIAS_DISABLE.
>>
>> If the HW doesn't support any concept of a default pull, I think the
>> driver shouldn't support that option; it should return an error if asked
>> to program it.
> 
> Yes that's how I remember it and how we specified it.
> Correct Heiko?
> 
>> Presumably given this, LinusW shouldn't have actually applied this
>> patch, since presumably it prevents any other driver from accepting
>> PIN_CONFIG_BIAS_DISABLE even in cases where it is appropriate?
> 
> There are many patches I shouldn't have applied ...
> 
> Anyway I'm not quite following, this patch affected the Palmas
> driver only I think so how can it prevent any other drivers from doing
> the right thing?

Sorry, for some reason I thought this patch was touching core code
rather than the specific driver:-(

Re: [PATCH 1/2] pinctrl: palmas: do not abort pin configuration for BIAS_DEFAULT

[Linus Walleij]

On Thu, Sep 26, 2013 at 2:48 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> Recent movement of all configurations of pin in the single call of
> pin_config_set(), it is aborting configuration if BIAS_PULL_PIN_DEFAULT
> is selected as return of configuration.
>
> The original idea was to just avoid any update on register for pull up/down
> configuration if this option is selected.
>
> Fixing this by just bypassing any register update for BIAS_PULL_PIN_DEFAULT
> and continuing the remaining configuration.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

I've applied the patch as it just brings back the semantics before
commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
"pinctrl: Pass all configs to driver on pin_config_set()"

(Normally you should mention the above in your commit message
with patches like these that are regressions.)
But whatever.

Please look into the related issue pointed out by Stephen as well.

Yours,
Linus Walleij