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

PATCH In review [email protected] 3 participants 8 messages Sep 12, 2017 – Oct 11, 2017

Subsystems (5)

Arm Qualcomm mailing list GPIO Pin control Pin controller - Qualcomm +1 more subsystems

lore

Share & tools

lore

[email protected]

author

2017-09-12 00:32 UTC #0 lore raw

PATCH 1/1 pinctrl: qcom: spmi-gpio: Update GPIO EN_CTL when setting pin config

From: Fenglin Wu <[email protected]>

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 <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +5 -0

index c2c0bab..a0edaa8 100644
@@ -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;
 }

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Linus Walleij

2017-09-19 08:20 UTC #1 lore raw

Quoted 8 lines · On Tue, Sep 12, 2017 at 2:32 AM, <[email protected]> wrote:

From: Fenglin Wu <[email protected]>
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 <[email protected]>

Pending on Björn's review here.

Yours,
Linus Walleij

Bjorn Andersson

Acked-by on v1 1/1

2017-10-05 16:27 UTC #2 lore raw

In reply to [email protected]

Quoted 25 lines · On Mon 11 Sep 17:32 PDT 2017, [email protected] wrote:

From: Fenglin Wu <[email protected]>
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 <[email protected]>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +1 -0
index c2c0bab..a0edaa8 100644
@@ -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]);

drivers/pinctrl/qcom/pinctrl-spmi-gpio.c static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 607

Quoted 7 lines

drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +4 -0

@@ -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 <[email protected]>

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

Fenglin Wu

author

2017-10-09 05:34 UTC #3 lore raw

Quoted 46 lines · Bjorn Andersson

Quoted 25 lines · On Mon 11 Sep 17:32 PDT 2017, [email protected] wrote:
From: Fenglin Wu <[email protected]>
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 <[email protected]>
---
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
  1 file changed, 5 insertions(+)
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +1 -0
index c2c0bab..a0edaa8 100644
@@ -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]);
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 607
Quoted 7 lines
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +4 -0
@@ -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 <[email protected]>
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.

Quoted 4 lines

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.

Quoted 4 lines

Regards,
Bjorn

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Bjorn Andersson

2017-10-09 05:56 UTC #4 lore raw

Quoted 47 lines · Fenglin Wu

Quoted 46 lines · Bjorn Andersson
Quoted 23 lines · On Mon 11 Sep 17:32 PDT 2017, [email protected] wrote:
From: Fenglin Wu <[email protected]>
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 <[email protected]>
---
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
  1 file changed, 5 insertions(+)
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +1 -0
index c2c0bab..a0edaa8 100644
@@ -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]);
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 606
Quoted 6 lines
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +4 -0
@@ -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 <[email protected]>
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".

Quoted 8 lines

Quoted 1 line
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

Fenglin Wu

author

2017-10-10 00:17 UTC #5 lore raw

Quoted 58 lines · Bjorn Andersson

Quoted 47 lines · Fenglin Wu
Quoted 46 lines · Bjorn Andersson
Quoted 23 lines · On Mon 11 Sep 17:32 PDT 2017, [email protected] wrote:
From: Fenglin Wu <[email protected]>
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 <[email protected]>
---
   drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 5 +++++
   1 file changed, 5 insertions(+)
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +1 -0
index c2c0bab..a0edaa8 100644
@@ -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]);
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c static int pmic_gpio_config_set(struct pinctrl_dev *pctldev, unsigned int pin, · line 606
Quoted 6 lines
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +4 -0
@@ -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 <[email protected]>
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".
Quoted 8 lines
Quoted 1 line
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.

Quoted 8 lines

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()?

Quoted 4 lines

Regards,
Bjorn

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Bjorn Andersson

2017-10-11 05:48 UTC #6 lore raw

Quoted 4 lines · Fenglin Wu

Quoted 3 lines · Bjorn Andersson
Quoted 1 line · Fenglin Wu
On 10/6/2017 12:27 AM, Bjorn Andersson wrote:

[..]

Quoted 23 lines

Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
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".
Quoted 8 lines
Quoted 1 line
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.

Quoted 6 lines

Quoted 3 lines
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

Linus Walleij

2017-10-11 08:24 UTC #7 lore raw

In reply to [email protected]

Quoted 8 lines · On Tue, Sep 12, 2017 at 2:32 AM, <[email protected]> wrote:

From: Fenglin Wu <[email protected]>
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 <[email protected]>

Patch applied with Björn's ACK.

Expecting follow-up fixes as discussed in this thread.

Yours,
Linus Walleij