[V1] pinctrl: qcom: spmi-gpio: Update GPIO EN_CTL when setting pin config

Message ID 20170912003331.3092-1-fenglinw@codeaurora.org
State New
Headers show
Series
  • [V1] pinctrl: qcom: spmi-gpio: Update GPIO EN_CTL when setting pin config
Related show

Commit Message

Fenglin Wu Sept. 12, 2017, 12:32 a.m.
From: Fenglin Wu <fenglinw@codeaurora.org>

GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
configured. Update is_enabled flag in config_set() so that it can
reflect GPIO status correctly. Also modify EN_CTL register based on
is_enabled flag in config_set() to configure the GPIO properly.

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Linus Walleij Sept. 19, 2017, 8:20 a.m. | #1
On Tue, Sep 12, 2017 at 2:32 AM,  <fenglinw@codeaurora.org> wrote:

> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
> configured. Update is_enabled flag in config_set() so that it can
> reflect GPIO status correctly. Also modify EN_CTL register based on
> is_enabled flag in config_set() to configure the GPIO properly.
>
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>

Pending on Björn's review here.

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
Bjorn Andersson Oct. 5, 2017, 4:27 p.m. | #2
On Mon 11 Sep 17:32 PDT 2017, fenglinw@codeaurora.org wrote:

> From: Fenglin Wu <fenglinw@codeaurora.org>
> 
> GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
> configured. Update is_enabled flag in config_set() so that it can
> reflect GPIO status correctly. Also modify EN_CTL register based on
> is_enabled flag in config_set() to configure the GPIO properly.
> 
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index c2c0bab..a0edaa8 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -453,6 +453,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  
>  	pad = pctldev->desc->pins[pin].drv_data;
>  
> +	pad->is_enabled = true;
>  	for (i = 0; i < nconfs; i++) {
>  		param = pinconf_to_config_param(configs[i]);
>  		arg = pinconf_to_config_argument(configs[i]);
> @@ -600,6 +601,10 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  			return ret;
>  	}
>  
> +	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
> +
> +	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
> +

This looks good.

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>


But I spotted another issue while reviewing this; currently the initial
state of is_enabled is unconditionally set to enabled in
pmic_gpio_populate(), so reading the initial pinconf or configuring a
pinmux before setting a pinconf will operate on the potentially wrong
information.

So I think the initial value should be read out from REG_EN_CTL rather
than being just "true".

Can you please either submit another patch for this?

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
Fenglin Wu Oct. 9, 2017, 5:34 a.m. | #3
On 10/6/2017 12:27 AM, Bjorn Andersson wrote:
> On Mon 11 Sep 17:32 PDT 2017, fenglinw@codeaurora.org wrote:
> 
>> From: Fenglin Wu <fenglinw@codeaurora.org>
>>
>> GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
>> configured. Update is_enabled flag in config_set() so that it can
>> reflect GPIO status correctly. Also modify EN_CTL register based on
>> is_enabled flag in config_set() to configure the GPIO properly.
>>
>> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
>> ---
>>   drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> index c2c0bab..a0edaa8 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> @@ -453,6 +453,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>>   
>>   	pad = pctldev->desc->pins[pin].drv_data;
>>   
>> +	pad->is_enabled = true;
>>   	for (i = 0; i < nconfs; i++) {
>>   		param = pinconf_to_config_param(configs[i]);
>>   		arg = pinconf_to_config_argument(configs[i]);
>> @@ -600,6 +601,10 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>>   			return ret;
>>   	}
>>   
>> +	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
>> +
>> +	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
>> +
> 
> This looks good.
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> 
> But I spotted another issue while reviewing this; currently the initial
> state of is_enabled is unconditionally set to enabled in
> pmic_gpio_populate(), so reading the initial pinconf or configuring a
> pinmux before setting a pinconf will operate on the potentially wrong
> information.
> 
> So I think the initial value should be read out from REG_EN_CTL rather
> than being just "true".
> 
> Can you please either submit another patch for this?

Hmm, considering a GPIO which is disabled by default in hardware
setting, what's its expected state if we only define "function" for it?
I was thinking we need to enable it once it has any setting in pinmux or
pinconf. If you think that we need to keep its original state until we
set pinconf for it, yes, I can submit a change to address this.


> 
> Regards,
> Bjorn
>
Bjorn Andersson Oct. 9, 2017, 5:56 a.m. | #4
On Sun 08 Oct 22:34 PDT 2017, Fenglin Wu wrote:

> On 10/6/2017 12:27 AM, Bjorn Andersson wrote:
> > On Mon 11 Sep 17:32 PDT 2017, fenglinw@codeaurora.org wrote:
> > 
> > > From: Fenglin Wu <fenglinw@codeaurora.org>
> > > 
> > > GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
> > > configured. Update is_enabled flag in config_set() so that it can
> > > reflect GPIO status correctly. Also modify EN_CTL register based on
> > > is_enabled flag in config_set() to configure the GPIO properly.
> > > 
> > > Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> > > ---
> > >   drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > index c2c0bab..a0edaa8 100644
> > > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > @@ -453,6 +453,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > >   	pad = pctldev->desc->pins[pin].drv_data;
> > > +	pad->is_enabled = true;
> > >   	for (i = 0; i < nconfs; i++) {
> > >   		param = pinconf_to_config_param(configs[i]);
> > >   		arg = pinconf_to_config_argument(configs[i]);
> > > @@ -600,6 +601,10 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > >   			return ret;
> > >   	}
> > > +	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
> > > +
> > > +	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
> > > +
> > 
> > This looks good.
> > 
> > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > 
> > But I spotted another issue while reviewing this; currently the initial
> > state of is_enabled is unconditionally set to enabled in
> > pmic_gpio_populate(), so reading the initial pinconf or configuring a
> > pinmux before setting a pinconf will operate on the potentially wrong
> > information.
> > 
> > So I think the initial value should be read out from REG_EN_CTL rather
> > than being just "true".
> > 
> > Can you please either submit another patch for this?
> 
> Hmm, considering a GPIO which is disabled by default in hardware
> setting, what's its expected state if we only define "function" for it?
> I was thinking we need to enable it once it has any setting in pinmux or
> pinconf. If you think that we need to keep its original state until we
> set pinconf for it, yes, I can submit a change to address this.
> 

Are there valid cases where only function should be selected and no
other configuration is used? If so it makes sense to make
pmic_gpio_set_mux() enable the block.


Regardless of this, if there are disabled pins that are not mentioned in
DT they will still appear as enabled in the debugfs interface; and this
I consider an error worth fixing.

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
Fenglin Wu Oct. 10, 2017, 12:17 a.m. | #5
On 10/9/2017 1:56 PM, Bjorn Andersson wrote:
> On Sun 08 Oct 22:34 PDT 2017, Fenglin Wu wrote:
> 
>> On 10/6/2017 12:27 AM, Bjorn Andersson wrote:
>>> On Mon 11 Sep 17:32 PDT 2017, fenglinw@codeaurora.org wrote:
>>>
>>>> From: Fenglin Wu <fenglinw@codeaurora.org>
>>>>
>>>> GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
>>>> configured. Update is_enabled flag in config_set() so that it can
>>>> reflect GPIO status correctly. Also modify EN_CTL register based on
>>>> is_enabled flag in config_set() to configure the GPIO properly.
>>>>
>>>> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
>>>> ---
>>>>    drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>>>> index c2c0bab..a0edaa8 100644
>>>> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>>>> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>>>> @@ -453,6 +453,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>>>>    	pad = pctldev->desc->pins[pin].drv_data;
>>>> +	pad->is_enabled = true;
>>>>    	for (i = 0; i < nconfs; i++) {
>>>>    		param = pinconf_to_config_param(configs[i]);
>>>>    		arg = pinconf_to_config_argument(configs[i]);
>>>> @@ -600,6 +601,10 @@ static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>>>>    			return ret;
>>>>    	}
>>>> +	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
>>>> +
>>>> +	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
>>>> +
>>>
>>> This looks good.
>>>
>>> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>
>>>
>>> But I spotted another issue while reviewing this; currently the initial
>>> state of is_enabled is unconditionally set to enabled in
>>> pmic_gpio_populate(), so reading the initial pinconf or configuring a
>>> pinmux before setting a pinconf will operate on the potentially wrong
>>> information.
>>>
>>> So I think the initial value should be read out from REG_EN_CTL rather
>>> than being just "true".
>>>
>>> Can you please either submit another patch for this?
>>
>> Hmm, considering a GPIO which is disabled by default in hardware
>> setting, what's its expected state if we only define "function" for it?
>> I was thinking we need to enable it once it has any setting in pinmux or
>> pinconf. If you think that we need to keep its original state until we
>> set pinconf for it, yes, I can submit a change to address this.
>>
> 
> Are there valid cases where only function should be selected and no
> other configuration is used? If so it makes sense to make
> pmic_gpio_set_mux() enable the block.
> 
> 
> Regardless of this, if there are disabled pins that are not mentioned in
> DT they will still appear as enabled in the debugfs interface; and this
> I consider an error worth fixing.
How about we do both: read the HW initial state in pmic_gpio_populate(),
and also enable the GPIO block in pmic_gpio_set_mux()?

> 
> Regards,
> Bjorn
>
Bjorn Andersson Oct. 11, 2017, 5:48 a.m. | #6
On Mon 09 Oct 17:17 PDT 2017, Fenglin Wu wrote:

> On 10/9/2017 1:56 PM, Bjorn Andersson wrote:
> > On Sun 08 Oct 22:34 PDT 2017, Fenglin Wu wrote:
> > 
> > > On 10/6/2017 12:27 AM, Bjorn Andersson wrote:
[..]
> > > > But I spotted another issue while reviewing this; currently the initial
> > > > state of is_enabled is unconditionally set to enabled in
> > > > pmic_gpio_populate(), so reading the initial pinconf or configuring a
> > > > pinmux before setting a pinconf will operate on the potentially wrong
> > > > information.
> > > > 
> > > > So I think the initial value should be read out from REG_EN_CTL rather
> > > > than being just "true".
> > > > 
> > > > Can you please either submit another patch for this?
> > > 
> > > Hmm, considering a GPIO which is disabled by default in hardware
> > > setting, what's its expected state if we only define "function" for it?
> > > I was thinking we need to enable it once it has any setting in pinmux or
> > > pinconf. If you think that we need to keep its original state until we
> > > set pinconf for it, yes, I can submit a change to address this.
> > > 
> > 
> > Are there valid cases where only function should be selected and no
> > other configuration is used? If so it makes sense to make
> > pmic_gpio_set_mux() enable the block.
> > 
> > 
> > Regardless of this, if there are disabled pins that are not mentioned in
> > DT they will still appear as enabled in the debugfs interface; and this
> > I consider an error worth fixing.
> How about we do both: read the HW initial state in pmic_gpio_populate(),
> and also enable the GPIO block in pmic_gpio_set_mux()?
> 

That sounds good.

Please do this as two separate patches, with the commit message clearly
describing a case where the pinconf does not affect the function of the
pin, so a pinmux is the only thing needed.

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
Linus Walleij Oct. 11, 2017, 8:24 a.m. | #7
On Tue, Sep 12, 2017 at 2:32 AM,  <fenglinw@codeaurora.org> wrote:

> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> GPIO is expected to be disabled iff PIN_CONFIG_BIAS_HIGH_IMPEDANCE is
> configured. Update is_enabled flag in config_set() so that it can
> reflect GPIO status correctly. Also modify EN_CTL register based on
> is_enabled flag in config_set() to configure the GPIO properly.
>
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>

Patch applied with Björn's ACK.

Expecting follow-up fixes as discussed in this thread.

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

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index c2c0bab..a0edaa8 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -453,6 +453,7 @@  static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 	pad = pctldev->desc->pins[pin].drv_data;
 
+	pad->is_enabled = true;
 	for (i = 0; i < nconfs; i++) {
 		param = pinconf_to_config_param(configs[i]);
 		arg = pinconf_to_config_argument(configs[i]);
@@ -600,6 +601,10 @@  static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			return ret;
 	}
 
+	val = pad->is_enabled << PMIC_GPIO_REG_MASTER_EN_SHIFT;
+
+	ret = pmic_gpio_write(state, pad, PMIC_GPIO_REG_EN_CTL, val);
+
 	return ret;
 }