| Message ID | 1422289494-31461-1-git-send-email-svarbanov@mm-sol.com |
|---|---|
| State | New |
| Headers | show |
On 01/26/15 08:24, Stanimir Varbanov wrote: > Enables generic pinconf support and add handling for 'input-enable' > pinconf property. > > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 17 ++++++++++++----- > 1 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index e730935..4a47eb1 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -193,11 +193,11 @@ 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; > default: > - dev_err(pctrl->dev, "Invalid config param %04x\n", param); Is this removed because the generic framework already takes care of printing this information on such errors? > return -ENOTSUPP; > } > > @@ -208,14 +208,12 @@ 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"); Same for here. > 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"); And here. > return -ENOTSUPP; > } > > @@ -276,9 +274,11 @@ 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: > + val = readl(pctrl->regs + g->io_reg); > + arg = !!(val & BIT(g->in_bit)); > + break; This bit is used to read the value of the gpio. If the gpio is high, this bit will read back as a 1. If the gpio is low, this bit will read back as a 0. I don't see how this is related to input-enable? Doesn't input-enable mean we configure the pin to accept input? In that sense, configuring the pin to accept input would sort of be like configuring the pin for function "gpio" so that we can read this bit and see if the pin is high or low, but I don't know if we care to support that. I think we rely on users configuring the pin for the gpio function though. > default: > - dev_err(pctrl->dev, "Unsupported config parameter: %x\n", > - param); > return -EINVAL; > } > > @@ -348,6 +348,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); > @@ -372,6 +376,9 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, > } > > static const struct pinconf_ops msm_pinconf_ops = { > +#ifdef CONFIG_GENERIC_PINCONF > + .is_generic = true, > +#endif Shouldn't we have set this when we started this driver? I guess this only makes debugfs output change, but it seems like this is a "fix" of some kind that is different from input-enable support.
On 01/26/15 17:18, Stephen Boyd wrote: > On 01/26/15 08:24, Stanimir Varbanov wrote: > >> return -ENOTSUPP; >> } >> >> @@ -208,14 +208,12 @@ 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"); > Same for here. > >> 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"); > And here. Actually it looks like we can just remove these two functions and the core does the right thing and prints errors. That would be a good patch.
Hi Stephen, Thanks for the comments! On 01/27/2015 03:18 AM, Stephen Boyd wrote: > On 01/26/15 08:24, Stanimir Varbanov wrote: >> Enables generic pinconf support and add handling for 'input-enable' >> pinconf property. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 17 ++++++++++++----- >> 1 files changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index e730935..4a47eb1 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -193,11 +193,11 @@ 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; >> default: >> - dev_err(pctrl->dev, "Invalid config param %04x\n", param); > > Is this removed because the generic framework already takes care of > printing this information on such errors? yes, more or less. These error messages just floods the screen when using debugfs entries. Even more ENOTSUPP is legal error for generic pinconf. > >> return -ENOTSUPP; >> } >> >> @@ -208,14 +208,12 @@ 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"); > > Same for here. > >> 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"); > > And here. > >> return -ENOTSUPP; >> } >> >> @@ -276,9 +274,11 @@ 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: >> + val = readl(pctrl->regs + g->io_reg); >> + arg = !!(val & BIT(g->in_bit)); >> + break; > > This bit is used to read the value of the gpio. If the gpio is high, > this bit will read back as a 1. If the gpio is low, this bit will read > back as a 0. I don't see how this is related to input-enable? Doesn't Maybe I had to split on few patches. The change in .pin_config_group_get is related to debugfs dump. The change which adds input-enable support is in .pin_config_group_set. > input-enable mean we configure the pin to accept input? In that sense, > configuring the pin to accept input would sort of be like configuring > the pin for function "gpio" so that we can read this bit and see if the > pin is high or low, but I don't know if we care to support that. I think > we rely on users configuring the pin for the gpio function though. Why you do not care, because you think that tlmm doesn't support it or because it is a driver responsibility to set up gpio function plus input/output configuration? I can imagine usecase where the driver implements an 'idle' pinctrl state which configure its pins to be gpio input enabled to save power assuming that the previous 'default' pinctrl state it was gpio output. In that case the driver doesn't need to call gpio_direction_input() for every gpio, only call pinctrl_select_state('idle'). What's wrong with this? > >> default: >> - dev_err(pctrl->dev, "Unsupported config parameter: %x\n", >> - param); >> return -EINVAL; >> } >> >> @@ -348,6 +348,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); >> @@ -372,6 +376,9 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, >> } >> >> static const struct pinconf_ops msm_pinconf_ops = { >> +#ifdef CONFIG_GENERIC_PINCONF >> + .is_generic = true, >> +#endif > > Shouldn't we have set this when we started this driver? I guess this > only makes debugfs output change, but it seems like this is a "fix" of > some kind that is different from input-enable support. > yes, it should be different fix, I will split this on separate patch.
On 01/27/2015 03:23 AM, Stephen Boyd wrote: > On 01/26/15 17:18, Stephen Boyd wrote: >> On 01/26/15 08:24, Stanimir Varbanov wrote: >> >>> return -ENOTSUPP; >>> } >>> >>> @@ -208,14 +208,12 @@ 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"); >> Same for here. >> >>> 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"); >> And here. > > Actually it looks like we can just remove these two functions and the > core does the right thing and prints errors. That would be a good patch. > Agreed.
On Mon 26 Jan 17:23 PST 2015, Stephen Boyd wrote: > On 01/26/15 17:18, Stephen Boyd wrote: > > On 01/26/15 08:24, Stanimir Varbanov wrote: > > > >> return -ENOTSUPP; > >> } > >> > >> @@ -208,14 +208,12 @@ 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"); > > Same for here. > > > >> 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"); > > And here. > > Actually it looks like we can just remove these two functions and the > core does the right thing and prints errors. That would be a good patch. > Yeah, seems c420619d51 ("pinctrl: pinconf: remove checks on ops->pin_config_get") introduced in 3.14 reduced the requirements here. Seems like there is a whole bunch of drivers that would benefit from this cleanup still. Regards, Bjorn -- 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
On Tue 27 Jan 05:28 PST 2015, Stanimir Varbanov wrote: > Hi Stephen, > > Thanks for the comments! > > On 01/27/2015 03:18 AM, Stephen Boyd wrote: > > On 01/26/15 08:24, Stanimir Varbanov wrote: > >> Enables generic pinconf support and add handling for 'input-enable' > >> pinconf property. > >> > >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> > >> --- Hi Stanimir, Good stuff, but please split this into (although I think you already figured it out): * one patch that introduces input-enable * one that enable the usage of pinconf-generic debugfs (is_generic + drop the prints) * one that drops the msm_config_get/msm_config_set > >> drivers/pinctrl/qcom/pinctrl-msm.c | 17 ++++++++++++----- > >> 1 files changed, 12 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > >> index e730935..4a47eb1 100644 > >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c > >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > >> @@ -193,11 +193,11 @@ static int msm_config_reg(struct msm_pinctrl *pctrl, > >> *mask = 7; > >> break; > >> case PIN_CONFIG_OUTPUT: > >> + case PIN_CONFIG_INPUT_ENABLE: Looks good. > >> *bit = g->oe_bit; > >> *mask = 1; > >> break; > >> default: > >> - dev_err(pctrl->dev, "Invalid config param %04x\n", param); > > > > Is this removed because the generic framework already takes care of > > printing this information on such errors? > > yes, more or less. These error messages just floods the screen when > using debugfs entries. Even more ENOTSUPP is legal error for generic > pinconf. > > > [..] > >> > >> @@ -276,9 +274,11 @@ 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: > >> + val = readl(pctrl->regs + g->io_reg); > >> + arg = !!(val & BIT(g->in_bit)); > >> + break; > > > > This bit is used to read the value of the gpio. If the gpio is high, > > this bit will read back as a 1. If the gpio is low, this bit will read > > back as a 0. I don't see how this is related to input-enable? Doesn't > > Maybe I had to split on few patches. The change in .pin_config_group_get > is related to debugfs dump. The change which adds input-enable support > is in .pin_config_group_set. > > > input-enable mean we configure the pin to accept input? In that sense, > > configuring the pin to accept input would sort of be like configuring > > the pin for function "gpio" so that we can read this bit and see if the > > pin is high or low, but I don't know if we care to support that. I think > > we rely on users configuring the pin for the gpio function though. > > Why you do not care, because you think that tlmm doesn't support it or > because it is a driver responsibility to set up gpio function plus > input/output configuration? > > I can imagine usecase where the driver implements an 'idle' pinctrl > state which configure its pins to be gpio input enabled to save power > assuming that the previous 'default' pinctrl state it was gpio output. > In that case the driver doesn't need to call gpio_direction_input() for > every gpio, only call pinctrl_select_state('idle'). > > What's wrong with this? > It makes a lot of sense to be able to have a input-enabled state that you can switch to. Such state would specify function = "gpio" and input-enable; I do however think that querying PIN_CONFIG_INPUT_ENABLE should not return the value of the pin, but rather !(readl(g->ctl_reg) & BIT(g->oe_bit)) to indicate if the pin is configured in input enable or not. > > > >> default: > >> - dev_err(pctrl->dev, "Unsupported config parameter: %x\n", > >> - param); Is this part of the debugfs cleanup? > >> return -EINVAL; > >> } > >> > >> @@ -348,6 +348,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; Looks good. > >> default: > >> dev_err(pctrl->dev, "Unsupported config parameter: %x\n", > >> param); > >> @@ -372,6 +376,9 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, > >> } > >> > >> static const struct pinconf_ops msm_pinconf_ops = { > >> +#ifdef CONFIG_GENERIC_PINCONF > >> + .is_generic = true, > >> +#endif > > > > Shouldn't we have set this when we started this driver? I guess this > > only makes debugfs output change, but it seems like this is a "fix" of > > some kind that is different from input-enable support. > > > > yes, it should be different fix, I will split this on separate patch. > PINCTRL_MSM does "select GENERIC_PINCONF", so the #ifdef is not needed. And Stephen you're right, it should have been there in my original version. Regards, Bjorn -- 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
On 01/27/15 05:28, Stanimir Varbanov wrote: > Hi Stephen, > > Thanks for the comments! > > On 01/27/2015 03:18 AM, Stephen Boyd wrote: >> On 01/26/15 08:24, Stanimir Varbanov wrote: >> >>> return -ENOTSUPP; >>> } >>> >>> @@ -276,9 +274,11 @@ 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: >>> + val = readl(pctrl->regs + g->io_reg); >>> + arg = !!(val & BIT(g->in_bit)); >>> + break; >> This bit is used to read the value of the gpio. If the gpio is high, >> this bit will read back as a 1. If the gpio is low, this bit will read >> back as a 0. I don't see how this is related to input-enable? Doesn't > Maybe I had to split on few patches. The change in .pin_config_group_get > is related to debugfs dump. The change which adds input-enable support > is in .pin_config_group_set. > >> input-enable mean we configure the pin to accept input? In that sense, >> configuring the pin to accept input would sort of be like configuring >> the pin for function "gpio" so that we can read this bit and see if the >> pin is high or low, but I don't know if we care to support that. I think >> we rely on users configuring the pin for the gpio function though. > Why you do not care, because you think that tlmm doesn't support it or > because it is a driver responsibility to set up gpio function plus > input/output configuration? Ah ok, I see that the driver clears the output enable bit for gpio_direction_input(). Technically that isn't even required because from what I can recall the output enable bit only controls if the OE bit is driven out on the line. We can always read what's there by reading the IN bit even when the output is enabled. > > I can imagine usecase where the driver implements an 'idle' pinctrl > state which configure its pins to be gpio input enabled to save power > assuming that the previous 'default' pinctrl state it was gpio output. > In that case the driver doesn't need to call gpio_direction_input() for > every gpio, only call pinctrl_select_state('idle'). > > What's wrong with this? Yes that's fine. It sounds like you want to disabled the output and the way that works today is by switching the gpio direction to input. I just got caught up because in my mind the input is always enabled because we can always read what's on the pin. I guess output-disable == input-enable. Either way, the implementation doesn't look right for this part where we want to see if the input is enabled. Looks like Bjorn has the right idea, so let's see how v2 goes.
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index e730935..4a47eb1 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -193,11 +193,11 @@ 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; default: - dev_err(pctrl->dev, "Invalid config param %04x\n", param); return -ENOTSUPP; } @@ -208,14 +208,12 @@ 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; } @@ -276,9 +274,11 @@ 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: + val = readl(pctrl->regs + g->io_reg); + arg = !!(val & BIT(g->in_bit)); + break; default: - dev_err(pctrl->dev, "Unsupported config parameter: %x\n", - param); return -EINVAL; } @@ -348,6 +348,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); @@ -372,6 +376,9 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, } static const struct pinconf_ops msm_pinconf_ops = { +#ifdef CONFIG_GENERIC_PINCONF + .is_generic = true, +#endif .pin_config_get = msm_config_get, .pin_config_set = msm_config_set, .pin_config_group_get = msm_config_group_get,
Enables generic pinconf support and add handling for 'input-enable' pinconf property. Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> --- drivers/pinctrl/qcom/pinctrl-msm.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)