* [PATCH 0/3] pinctrl: qcom: enable generic pinconf and input-enable
@ 2015-01-30 10:27 Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations Stanimir Varbanov
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Stanimir Varbanov @ 2015-01-30 10:27 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson,
Stephen Boyd, Stanimir Varbanov
Here is splitted version of the previous patch at [1].
The major change is in 3/3 where now we check oe_bit in control register
instead of in_bit in io register.
regards
Stan
[1] https://lkml.org/lkml/2015/1/26/514
Stanimir Varbanov (3):
pinctrl: qcom: delete pin_config_get/set pinconf operations
pinctrl: qcom: enable generic pinconf
pinctrl: qcom: handle input-enable pinconf property
drivers/pinctrl/qcom/pinctrl-msm.c | 32 ++++++++++++--------------------
1 files changed, 12 insertions(+), 20 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations
2015-01-30 10:27 [PATCH 0/3] pinctrl: qcom: enable generic pinconf and input-enable Stanimir Varbanov
@ 2015-01-30 10:27 ` Stanimir Varbanov
2015-01-30 13:37 ` Linus Walleij
2015-01-30 10:27 ` [PATCH 2/3] pinctrl: qcom: enable generic pinconf Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property Stanimir Varbanov
2 siblings, 1 reply; 10+ messages in thread
From: Stanimir Varbanov @ 2015-01-30 10:27 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson,
Stephen Boyd, Stanimir Varbanov
The .pin_config_get/set operation are not supported in qcom pinctrl
driver. As the pinconf core is smart enough it doesn't complain
about that.
Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 17 -----------------
1 files changed, 0 insertions(+), 17 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index e730935..49d3f4f 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -204,21 +204,6 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
return 0;
}
-static int msm_config_get(struct pinctrl_dev *pctldev,
- unsigned int pin,
- unsigned long *config)
-{
- dev_err(pctldev->dev, "pin_config_set op not supported\n");
- return -ENOTSUPP;
-}
-
-static int msm_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
- unsigned long *configs, unsigned num_configs)
-{
- dev_err(pctldev->dev, "pin_config_set op not supported\n");
- return -ENOTSUPP;
-}
-
#define MSM_NO_PULL 0
#define MSM_PULL_DOWN 1
#define MSM_KEEPER 2
@@ -372,8 +357,6 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
}
static const struct pinconf_ops msm_pinconf_ops = {
- .pin_config_get = msm_config_get,
- .pin_config_set = msm_config_set,
.pin_config_group_get = msm_config_group_get,
.pin_config_group_set = msm_config_group_set,
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] pinctrl: qcom: enable generic pinconf
2015-01-30 10:27 [PATCH 0/3] pinctrl: qcom: enable generic pinconf and input-enable Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations Stanimir Varbanov
@ 2015-01-30 10:27 ` Stanimir Varbanov
2015-01-30 13:39 ` Linus Walleij
2015-01-30 10:27 ` [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property Stanimir Varbanov
2 siblings, 1 reply; 10+ messages in thread
From: Stanimir Varbanov @ 2015-01-30 10:27 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson,
Stephen Boyd, Stanimir Varbanov
This makes the pinctrl driver to use the generic pinconf
interface. Mainly it gives us a way to use debugfs to dump
group configurations.
Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 49d3f4f..b66cd58 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -197,7 +197,6 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
*mask = 1;
break;
default:
- dev_err(pctrl->dev, "Invalid config param %04x\n", param);
return -ENOTSUPP;
}
@@ -262,8 +261,6 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
arg = !!(val & BIT(g->in_bit));
break;
default:
- dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
- param);
return -EINVAL;
}
@@ -357,6 +354,7 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
}
static const struct pinconf_ops msm_pinconf_ops = {
+ .is_generic = true,
.pin_config_group_get = msm_config_group_get,
.pin_config_group_set = msm_config_group_set,
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property
2015-01-30 10:27 [PATCH 0/3] pinctrl: qcom: enable generic pinconf and input-enable Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 2/3] pinctrl: qcom: enable generic pinconf Stanimir Varbanov
@ 2015-01-30 10:27 ` Stanimir Varbanov
2015-01-30 16:20 ` Bjorn Andersson
2 siblings, 1 reply; 10+ messages in thread
From: Stanimir Varbanov @ 2015-01-30 10:27 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson,
Stephen Boyd, Stanimir Varbanov
This enables support of 'input-enable' pinconf generic property in
the pinctrl driver.
Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index b66cd58..55a64ea 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -193,6 +193,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
*mask = 7;
break;
case PIN_CONFIG_OUTPUT:
+ case PIN_CONFIG_INPUT_ENABLE:
*bit = g->oe_bit;
*mask = 1;
break;
@@ -260,6 +261,12 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
val = readl(pctrl->regs + g->io_reg);
arg = !!(val & BIT(g->in_bit));
break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ /* Pin is output */
+ if (arg)
+ return -EINVAL;
+ arg = 1;
+ break;
default:
return -EINVAL;
}
@@ -330,6 +337,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
/* enable output */
arg = 1;
break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ /* disable output */
+ arg = 0;
+ break;
default:
dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
param);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations
2015-01-30 10:27 ` [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations Stanimir Varbanov
@ 2015-01-30 13:37 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2015-01-30 13:37 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson, Stephen Boyd
On Fri, Jan 30, 2015 at 11:27 AM, Stanimir Varbanov
<svarbanov@mm-sol.com> wrote:
> The .pin_config_get/set operation are not supported in qcom pinctrl
> driver. As the pinconf core is smart enough it doesn't complain
> about that.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Patch applied as it is obviously correct.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] pinctrl: qcom: enable generic pinconf
2015-01-30 10:27 ` [PATCH 2/3] pinctrl: qcom: enable generic pinconf Stanimir Varbanov
@ 2015-01-30 13:39 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2015-01-30 13:39 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson, Stephen Boyd
On Fri, Jan 30, 2015 at 11:27 AM, Stanimir Varbanov
<svarbanov@mm-sol.com> wrote:
> This makes the pinctrl driver to use the generic pinconf
> interface. Mainly it gives us a way to use debugfs to dump
> group configurations.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Nice! Looking for Björn's review though.
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -197,7 +197,6 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
> *mask = 1;
> break;
> default:
> - dev_err(pctrl->dev, "Invalid config param %04x\n", param);
> return -ENOTSUPP;
> }
>
> @@ -262,8 +261,6 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
> arg = !!(val & BIT(g->in_bit));
> break;
> default:
> - dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> - param);
> return -EINVAL;
Please change this one to -ENOTSUPP as well in this patch.
Apart from that it looks good to me.
Yours,
Linus Walleij
--
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] pinctrl: qcom: enable generic pinconf
@ 2015-01-30 13:39 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2015-01-30 13:39 UTC (permalink / raw)
To: Stanimir Varbanov
Cc: linux-arm-msm, linux-kernel, linux-gpio, Bjorn Andersson, Stephen Boyd
On Fri, Jan 30, 2015 at 11:27 AM, Stanimir Varbanov
<svarbanov@mm-sol.com> wrote:
> This makes the pinctrl driver to use the generic pinconf
> interface. Mainly it gives us a way to use debugfs to dump
> group configurations.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Nice! Looking for Björn's review though.
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -197,7 +197,6 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
> *mask = 1;
> break;
> default:
> - dev_err(pctrl->dev, "Invalid config param %04x\n", param);
> return -ENOTSUPP;
> }
>
> @@ -262,8 +261,6 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
> arg = !!(val & BIT(g->in_bit));
> break;
> default:
> - dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> - param);
> return -EINVAL;
Please change this one to -ENOTSUPP as well in this patch.
Apart from that it looks good to me.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property
2015-01-30 10:27 ` [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property Stanimir Varbanov
@ 2015-01-30 16:20 ` Bjorn Andersson
2015-02-04 10:03 ` Linus Walleij
0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2015-01-30 16:20 UTC (permalink / raw)
To: Stanimir Varbanov, Linus Walleij
Cc: linux-arm-msm, linux-kernel, linux-gpio, Stephen Boyd
On Fri 30 Jan 02:27 PST 2015, Stanimir Varbanov wrote:
> This enables support of 'input-enable' pinconf generic property in
> the pinctrl driver.
Patch looks good, but I think the api is broken for boolean properties.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index b66cd58..55a64ea 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -193,6 +193,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
> *mask = 7;
> break;
> case PIN_CONFIG_OUTPUT:
> + case PIN_CONFIG_INPUT_ENABLE:
> *bit = g->oe_bit;
> *mask = 1;
> break;
> @@ -260,6 +261,12 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
> val = readl(pctrl->regs + g->io_reg);
> arg = !!(val & BIT(g->in_bit));
> break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + /* Pin is output */
> + if (arg)
> + return -EINVAL;
> + arg = 1;
> + break;
My idea of this function is to query if we have the specific option
enabled, so I don't like the fact that we're returning an error here, we
should just return 0 with arg 0 (or something like that).
However, that would not give the results we expect and your patch is
"correct".
Linus, conf_items in pinconf_generic_dump_one() seems to represent
boolean properties of the pins. Returning 0 from pin_config_*_get()
should in my view then be treated as it not being active.
Is this in line with your view and should we modify
pinconf_generic_dump_one() to continue for these values if the getter
returns 0?
If not, at least all the bias properties here should return -EINVAL as
well. (which I think is wrong)
> default:
> return -EINVAL;
> }
> @@ -330,6 +337,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
> /* enable output */
> arg = 1;
> break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + /* disable output */
> + arg = 0;
> + break;
> default:
> dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> param);
Regards,
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property
2015-01-30 16:20 ` Bjorn Andersson
@ 2015-02-04 10:03 ` Linus Walleij
2015-02-04 17:49 ` Bjorn Andersson
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-02-04 10:03 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-gpio, Stephen Boyd
On Fri, Jan 30, 2015 at 5:20 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Fri 30 Jan 02:27 PST 2015, Stanimir Varbanov wrote:
>
>> + case PIN_CONFIG_INPUT_ENABLE:
>> + /* Pin is output */
>> + if (arg)
>> + return -EINVAL;
>> + arg = 1;
>> + break;
>
> My idea of this function is to query if we have the specific option
> enabled, so I don't like the fact that we're returning an error here, we
> should just return 0 with arg 0 (or something like that).
>
> However, that would not give the results we expect and your patch is
> "correct".
>
> Linus, conf_items in pinconf_generic_dump_one() seems to represent
> boolean properties of the pins. Returning 0 from pin_config_*_get()
> should in my view then be treated as it not being active.
>
> Is this in line with your view and should we modify
> pinconf_generic_dump_one() to continue for these values if the getter
> returns 0?
>
> If not, at least all the bias properties here should return -EINVAL as
> well. (which I think is wrong)
Well currently the semantics are:
- ENOTSUPP = this property is not even supported
- EINVAL = this value exists but can not be determined
It has this form primarily to serve the non-boolean properties.
For example pull-up can return -EINVAL if pull-up is supported
but pull-down is currently active, so it cannot say what
resistance it is pulled up with, as it is "infinite" (NAN,
thus translated -EINVAL).
It just folds over to the boolean props doing things in the
same way to simplify things... -EINVAL just means
"false". If we should return 1/0 from boolean props we need
to handle them as a special case in the pinconf-generic.
code, by extending the struct pinconf_generic_params,
which is possible of course.
Further: as of now pinconf_generic_dump_one() doesn't print
anything for inactive pulls etc return -EINVAL, but maybe
it should? It was just handy on some system to only see
the stuff that was really active, not to get a list of stuff that
was not active as well.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property
2015-02-04 10:03 ` Linus Walleij
@ 2015-02-04 17:49 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2015-02-04 17:49 UTC (permalink / raw)
To: Linus Walleij
Cc: Bjorn Andersson, Stanimir Varbanov, linux-arm-msm, linux-kernel,
linux-gpio, Stephen Boyd
On Wed, Feb 4, 2015 at 2:03 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 30, 2015 at 5:20 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>> On Fri 30 Jan 02:27 PST 2015, Stanimir Varbanov wrote:
>>
>>> + case PIN_CONFIG_INPUT_ENABLE:
>>> + /* Pin is output */
>>> + if (arg)
>>> + return -EINVAL;
>>> + arg = 1;
>>> + break;
>>
>> My idea of this function is to query if we have the specific option
>> enabled, so I don't like the fact that we're returning an error here, we
>> should just return 0 with arg 0 (or something like that).
>>
>> However, that would not give the results we expect and your patch is
>> "correct".
>>
>> Linus, conf_items in pinconf_generic_dump_one() seems to represent
>> boolean properties of the pins. Returning 0 from pin_config_*_get()
>> should in my view then be treated as it not being active.
>>
>> Is this in line with your view and should we modify
>> pinconf_generic_dump_one() to continue for these values if the getter
>> returns 0?
>>
>> If not, at least all the bias properties here should return -EINVAL as
>> well. (which I think is wrong)
>
> Well currently the semantics are:
>
> - ENOTSUPP = this property is not even supported
> - EINVAL = this value exists but can not be determined
>
> It has this form primarily to serve the non-boolean properties.
> For example pull-up can return -EINVAL if pull-up is supported
> but pull-down is currently active, so it cannot say what
> resistance it is pulled up with, as it is "infinite" (NAN,
> thus translated -EINVAL).
>
That makes sense, however according to both the dt binding and
pinconf-generic e.g. pull up is a boolean property. And "input-enable"
can always be queried (at least in our case).
> It just folds over to the boolean props doing things in the
> same way to simplify things... -EINVAL just means
> "false". If we should return 1/0 from boolean props we need
> to handle them as a special case in the pinconf-generic.
> code, by extending the struct pinconf_generic_params,
> which is possible of course.
>
That's what I figured. But I would like to argue that it's not
completely intuitive.
Don't we have all the info we need already? See below.
> Further: as of now pinconf_generic_dump_one() doesn't print
> anything for inactive pulls etc return -EINVAL, but maybe
> it should? It was just handy on some system to only see
> the stuff that was really active, not to get a list of stuff that
> was not active as well.
>
That's the way it should be, so any changes to the API would need to
retain this behavior. Something like adding:
if (!pinconf_to_config_argument(config) && !conf_items[i].has_arg)
continue;
But unless we expect any other users of this api I think we could just
leave it. I mostly wanted to clarify what the current expectations
was. Let me know if you want a patch.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-02-04 17:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 10:27 [PATCH 0/3] pinctrl: qcom: enable generic pinconf and input-enable Stanimir Varbanov
2015-01-30 10:27 ` [PATCH 1/3] pinctrl: qcom: delete pin_config_get/set pinconf operations Stanimir Varbanov
2015-01-30 13:37 ` Linus Walleij
2015-01-30 10:27 ` [PATCH 2/3] pinctrl: qcom: enable generic pinconf Stanimir Varbanov
2015-01-30 13:39 ` Linus Walleij
2015-01-30 13:39 ` Linus Walleij
2015-01-30 10:27 ` [PATCH 3/3] pinctrl: qcom: handle input-enable pinconf property Stanimir Varbanov
2015-01-30 16:20 ` Bjorn Andersson
2015-02-04 10:03 ` Linus Walleij
2015-02-04 17:49 ` Bjorn Andersson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.