Re: [PATCH V1 2/2] pinctrl: qcom: spmi-gpio: Set is_enabled flag in set_mux()

2017-10-16 Thread Fenglin Wu 

On 10/17/2017 6:29 AM, Bjorn Andersson wrote:

On Thu 12 Oct 23:15 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@codeaurora.org>

The initial value of is_enabled flag is read out from hardware in
pmic_gpio_populate(), and it will be set in pmic_gpio_config_set() if
pinconf is defined. For any GPIOs disabled initially in hardware which
only have pinmux defined, they won't be enabled in pmic_gpio_set_mux()
calling. So set is_enabled flag in set_mux() to ensure the GPIO module
could be enabled in above case.



I'm still interested in knowing when it is valid to configure a pin with
only mux, no config.  I.e. in what cases does setting a alternative
function make the pinconfig not count.

I agreed that any pins should be configured with pinmux as well as
pinconf, but the driver doesn't prevent the case of only pinmux defined,
right? I am not sure if this is valid case but it would happen: The
hardware or the sw prior to linux kernel has the default setting of the
function and config for one GPIO but we need to keep it disabled until
the consumer request it, in this case, we just need to define the pinmux
and ignore the pinconf definition in its device node.



Regards,
Bjorn


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

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 0a1e173..219c934 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -312,6 +312,7 @@ static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, 
unsigned function,
}
  
  	pad = pctldev->desc->pins[pin].drv_data;

+   pad->is_enabled = true;
/*
 * Non-LV/MV subtypes only support 2 special functions,
 * offsetting the dtestx function values by 2
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH V1 2/2] pinctrl: qcom: spmi-gpio: Set is_enabled flag in set_mux()

2017-10-16 Thread Fenglin Wu 

On 10/17/2017 6:29 AM, Bjorn Andersson wrote:

On Thu 12 Oct 23:15 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

The initial value of is_enabled flag is read out from hardware in
pmic_gpio_populate(), and it will be set in pmic_gpio_config_set() if
pinconf is defined. For any GPIOs disabled initially in hardware which
only have pinmux defined, they won't be enabled in pmic_gpio_set_mux()
calling. So set is_enabled flag in set_mux() to ensure the GPIO module
could be enabled in above case.



I'm still interested in knowing when it is valid to configure a pin with
only mux, no config.  I.e. in what cases does setting a alternative
function make the pinconfig not count.

I agreed that any pins should be configured with pinmux as well as
pinconf, but the driver doesn't prevent the case of only pinmux defined,
right? I am not sure if this is valid case but it would happen: The
hardware or the sw prior to linux kernel has the default setting of the
function and config for one GPIO but we need to keep it disabled until
the consumer request it, in this case, we just need to define the pinmux
and ignore the pinconf definition in its device node.



Regards,
Bjorn


Signed-off-by: Fenglin Wu 
---
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 0a1e173..219c934 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -312,6 +312,7 @@ static int pmic_gpio_set_mux(struct pinctrl_dev *pctldev, 
unsigned function,
}
  
  	pad = pctldev->desc->pins[pin].drv_data;

+   pad->is_enabled = true;
/*
 * Non-LV/MV subtypes only support 2 special functions,
 * offsetting the dtestx function values by 2
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

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

2017-10-09 Thread Fenglin Wu 

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, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@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 <fengl...@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.anders...@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



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

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

2017-10-09 Thread Fenglin Wu 

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, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

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 
---
   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 


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



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

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

2017-10-08 Thread Fenglin Wu 

On 10/6/2017 12:27 AM, Bjorn Andersson wrote:

On Mon 11 Sep 17:32 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@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 <fengl...@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.anders...@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



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

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

2017-10-08 Thread Fenglin Wu 

On 10/6/2017 12:27 AM, Bjorn Andersson wrote:

On Mon 11 Sep 17:32 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

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 
---
  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 


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



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

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-08-28 Thread Fenglin Wu 

On 8/29/2017 9:51 AM, Shawn Guo wrote:

On Tue, Aug 29, 2017 at 09:03:02AM +0800, Fenglin Wu wrote:

I agree the GPIO's ownership is configurable and it always configured at
the very beginning of the device boot up which is not visible by linux
kernel drivers/image. Normally, this configuration is fixed in one
platform and it's been protected and not allowed to be configured in
linux kernel driver. So from linux driver point of view, this is a
hardware configuration. I agree the coming patch "spmi: pmic-arb: Move
the ownership check to irq_chip callback" would fix the pinctrl-
spmi-gpio driver probe failure caused by the ownership mismatch, but
this is just hiding the mistake of the kernel configured the GPIOs which
not owned by APPS processor.


The kernel does everything just right, using the GPIO that device tree
tells to use.  If there is something wrong about ownership check, it
should be fault of that device tree specifies the wrong GPIO, or
firmware doesn't configure ownership as needed.
> Shawn



If you thought that the driver registers pins for the GPIOs not owned by
APPS processor is correct, then this patch is no needed.
I agreed with others.
Thanks

Fenglin


And these GPIOs will be registered
successfully as pinctrl pins and any APPS processor consumer drivers
could use this pins. This is not correct even the select_state operation
for these pins would failed due to the mode protection in spmi write_cmd
calling. I am thinking that not allowing these pins to be register as
pinctrl pins should be more straightforward and easy understanding. So I
think this patch still have value even the probe failure has been fixed
by the coming spmi patch.


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

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-08-28 Thread Fenglin Wu 

On 8/29/2017 9:51 AM, Shawn Guo wrote:

On Tue, Aug 29, 2017 at 09:03:02AM +0800, Fenglin Wu wrote:

I agree the GPIO's ownership is configurable and it always configured at
the very beginning of the device boot up which is not visible by linux
kernel drivers/image. Normally, this configuration is fixed in one
platform and it's been protected and not allowed to be configured in
linux kernel driver. So from linux driver point of view, this is a
hardware configuration. I agree the coming patch "spmi: pmic-arb: Move
the ownership check to irq_chip callback" would fix the pinctrl-
spmi-gpio driver probe failure caused by the ownership mismatch, but
this is just hiding the mistake of the kernel configured the GPIOs which
not owned by APPS processor.


The kernel does everything just right, using the GPIO that device tree
tells to use.  If there is something wrong about ownership check, it
should be fault of that device tree specifies the wrong GPIO, or
firmware doesn't configure ownership as needed.
> Shawn



If you thought that the driver registers pins for the GPIOs not owned by
APPS processor is correct, then this patch is no needed.
I agreed with others.
Thanks

Fenglin


And these GPIOs will be registered
successfully as pinctrl pins and any APPS processor consumer drivers
could use this pins. This is not correct even the select_state operation
for these pins would failed due to the mode protection in spmi write_cmd
calling. I am thinking that not allowing these pins to be register as
pinctrl pins should be more straightforward and easy understanding. So I
think this patch still have value even the probe failure has been fixed
by the coming spmi patch.


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

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-08-28 Thread Fenglin Wu 

On 8/28/2017 10:54 PM, Shawn Guo wrote:

On Wed, Jul 19, 2017 at 03:17:07PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu <fengl...@codeaurora.org>

Add support for qcom,gpios-disallowed property which is used to exclude
PMIC GPIOs not owned by the APSS processor from the pinctrl device.


If I understand it correctly, whether PMIC GPIOs is owned by APSS or not
can be configured by firmware.  If that's true, I do not think we should
have this qcom,gpios-disallowed thing in device tree, which is supposed
to describe hardware not something software configurable.

Shawn


Hi Shawn,

I agree the GPIO's ownership is configurable and it always configured at
the very beginning of the device boot up which is not visible by linux
kernel drivers/image. Normally, this configuration is fixed in one
platform and it's been protected and not allowed to be configured in
linux kernel driver. So from linux driver point of view, this is a
hardware configuration. I agree the coming patch "spmi: pmic-arb: Move
the ownership check to irq_chip callback" would fix the pinctrl-
spmi-gpio driver probe failure caused by the ownership mismatch, but
this is just hiding the mistake of the kernel configured the GPIOs which
not owned by APPS processor. And these GPIOs will be registered
successfully as pinctrl pins and any APPS processor consumer drivers
could use this pins. This is not correct even the select_state operation
for these pins would failed due to the mode protection in spmi write_cmd
calling. I am thinking that not allowing these pins to be register as
pinctrl pins should be more straightforward and easy understanding. So I
think this patch still have value even the probe failure has been fixed
by the coming spmi patch.


Fenglin




Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  12 ++
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 202 +
  2 files changed, 176 insertions(+), 38 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..435efe8 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -43,6 +43,17 @@ PMIC's from Qualcomm.
the first cell will be used to define gpio number and the
second denotes the flags for this gpio
  
+- qcom,gpios-disallowed:

+   Usage: optional
+   Value type: 
+   Definition: Array of the GPIO hardware numbers corresponding to GPIOs
+   which the APSS processor is not allowed to configure.
+   The hardware numbers are indexed from 1.
+   The interrupt resources for these GPIOs must not be defined
+   in "interrupts" and "interrupt-names" properties.
+   GPIOs defined in this array won't be registered as pins
+   in the pinctrl device or gpios in the gpio chip.
+
  Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt 
for
  a general description of GPIO and interrupt bindings.
  
@@ -206,6 +217,7 @@ Example:
  
  		gpio-controller;

#gpio-cells = <2>;
+   qcom,gpios-disallowed = <1 20>;
  
  		pm8921_gpio_keys: gpio-keys {

volume-keys {
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..74821af 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -96,6 +96,7 @@
   * struct pmic_gpio_pad - keep current GPIO settings
   * @base: Address base in SPMI device.
   * @irq: IRQ number which this GPIO generate.
+ * @gpio_idx: The index in GPIO's hardware number space (1-based)
   * @is_enabled: Set to false when GPIO should be put in high Z state.
   * @out_value: Cached pin output value
   * @have_buffer: Set to true if GPIO output could be configured in push-pull,
@@ -112,6 +113,7 @@
  struct pmic_gpio_pad {
u16 base;
int irq;
+   int gpio_idx;
boolis_enabled;
boolout_value;
boolhave_buffer;
@@ -130,6 +132,7 @@ struct pmic_gpio_state {
struct regmap   *map;
struct pinctrl_dev *ctrl;
struct gpio_chip chip;
+   const char **gpio_groups;
  };
  
  static const struct pinconf_generic_params pmic_gpio_bindings[] = {

@@ -231,7 +234,9 @@ static int pmic_gpio_get_function_groups(struct pinctrl_dev 
*pctldev,
 const char *const **groups,
 unsigned *const num_qgroups)
  {
-   *groups = pmic_gpio_groups;
+   struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
+
+   *groups = st

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-08-28 Thread Fenglin Wu 

On 8/28/2017 10:54 PM, Shawn Guo wrote:

On Wed, Jul 19, 2017 at 03:17:07PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu 

Add support for qcom,gpios-disallowed property which is used to exclude
PMIC GPIOs not owned by the APSS processor from the pinctrl device.


If I understand it correctly, whether PMIC GPIOs is owned by APSS or not
can be configured by firmware.  If that's true, I do not think we should
have this qcom,gpios-disallowed thing in device tree, which is supposed
to describe hardware not something software configurable.

Shawn


Hi Shawn,

I agree the GPIO's ownership is configurable and it always configured at
the very beginning of the device boot up which is not visible by linux
kernel drivers/image. Normally, this configuration is fixed in one
platform and it's been protected and not allowed to be configured in
linux kernel driver. So from linux driver point of view, this is a
hardware configuration. I agree the coming patch "spmi: pmic-arb: Move
the ownership check to irq_chip callback" would fix the pinctrl-
spmi-gpio driver probe failure caused by the ownership mismatch, but
this is just hiding the mistake of the kernel configured the GPIOs which
not owned by APPS processor. And these GPIOs will be registered
successfully as pinctrl pins and any APPS processor consumer drivers
could use this pins. This is not correct even the select_state operation
for these pins would failed due to the mode protection in spmi write_cmd
calling. I am thinking that not allowing these pins to be register as
pinctrl pins should be more straightforward and easy understanding. So I
think this patch still have value even the probe failure has been fixed
by the coming spmi patch.


Fenglin




Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  12 ++
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 202 +
  2 files changed, 176 insertions(+), 38 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..435efe8 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -43,6 +43,17 @@ PMIC's from Qualcomm.
the first cell will be used to define gpio number and the
second denotes the flags for this gpio
  
+- qcom,gpios-disallowed:

+   Usage: optional
+   Value type: 
+   Definition: Array of the GPIO hardware numbers corresponding to GPIOs
+   which the APSS processor is not allowed to configure.
+   The hardware numbers are indexed from 1.
+   The interrupt resources for these GPIOs must not be defined
+   in "interrupts" and "interrupt-names" properties.
+   GPIOs defined in this array won't be registered as pins
+   in the pinctrl device or gpios in the gpio chip.
+
  Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt 
for
  a general description of GPIO and interrupt bindings.
  
@@ -206,6 +217,7 @@ Example:
  
  		gpio-controller;

#gpio-cells = <2>;
+   qcom,gpios-disallowed = <1 20>;
  
  		pm8921_gpio_keys: gpio-keys {

volume-keys {
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..74821af 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -96,6 +96,7 @@
   * struct pmic_gpio_pad - keep current GPIO settings
   * @base: Address base in SPMI device.
   * @irq: IRQ number which this GPIO generate.
+ * @gpio_idx: The index in GPIO's hardware number space (1-based)
   * @is_enabled: Set to false when GPIO should be put in high Z state.
   * @out_value: Cached pin output value
   * @have_buffer: Set to true if GPIO output could be configured in push-pull,
@@ -112,6 +113,7 @@
  struct pmic_gpio_pad {
u16 base;
int irq;
+   int gpio_idx;
boolis_enabled;
boolout_value;
boolhave_buffer;
@@ -130,6 +132,7 @@ struct pmic_gpio_state {
struct regmap   *map;
struct pinctrl_dev *ctrl;
struct gpio_chip chip;
+   const char **gpio_groups;
  };
  
  static const struct pinconf_generic_params pmic_gpio_bindings[] = {

@@ -231,7 +234,9 @@ static int pmic_gpio_get_function_groups(struct pinctrl_dev 
*pctldev,
 const char *const **groups,
 unsigned *const num_qgroups)
  {
-   *groups = pmic_gpio_groups;
+   struct pmic_gpio_state *state = pinctrl_dev_get_drvdata(pctldev);
+
+   *groups = state->gpio_groups;
*num_qgroups = pctldev->des

Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally

2017-08-28 Thread Fenglin Wu 

On 8/22/2017 4:55 PM, Shawn Guo wrote:

On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:

On 08/18/2017 08:28 AM, Kiran Gunda wrote:

The peripheral ownership check is not necessary on single master
platforms. Hence, enforce the peripheral ownership check optionally.

Signed-off-by: Kiran Gunda 
Tested-by: Shawn Guo 
---


This sounds like a band-aid. Isn't the gpio driver going to keep probing
all the pins that are not supposed to be accessed due to security
constraints? What exactly is failing in the gpio case?


There is a platform_irq_count() call in pinctrl-spmi-gpio probe
function.  Due to the owner check in spmi-pmic-arb IRQ domain
qpnpint_irq_domain_dt_translate() function, the call will return irq
number as zero and cause pmic_gpio_probe() fail with -EINVAL error.

[1.608516] [] qpnpint_irq_domain_dt_translate+0x168/0x194
[1.613557] [] irq_create_fwspec_mapping+0x17c/0x2d8
[1.620672] [] irq_create_of_mapping+0x64/0x74
[1.627008] [] of_irq_get+0x54/0x64
[1.633169] [] platform_get_irq+0x20/0x150
[1.638117] [] platform_irq_count+0x28/0x44
[1.643850] [] pmic_gpio_probe+0x50/0x544

ShawnI just realize this patch is trying to fix this issue from spmi driver

level. Actually I had submitted a change in spmi-gpio driver to fix
this by ignoring the GPIOs which the IRQ is not owned by APPS
processor. The maintainer hasn't reviewed it yet:
https://www.spinics.net/lists/linux-arm-msm/msg28849.html
I am trying to understand if my patch is still needed if Kiran's patch
get merged, the intention for my patch originally is for fixing the same
probe failure, but it could hide the GPIOs which are not allowed to use
from the pinctrl driver level. Please help to suggest.
Thanks

Fenglin


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally

2017-08-28 Thread Fenglin Wu 

On 8/22/2017 4:55 PM, Shawn Guo wrote:

On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:

On 08/18/2017 08:28 AM, Kiran Gunda wrote:

The peripheral ownership check is not necessary on single master
platforms. Hence, enforce the peripheral ownership check optionally.

Signed-off-by: Kiran Gunda 
Tested-by: Shawn Guo 
---


This sounds like a band-aid. Isn't the gpio driver going to keep probing
all the pins that are not supposed to be accessed due to security
constraints? What exactly is failing in the gpio case?


There is a platform_irq_count() call in pinctrl-spmi-gpio probe
function.  Due to the owner check in spmi-pmic-arb IRQ domain
qpnpint_irq_domain_dt_translate() function, the call will return irq
number as zero and cause pmic_gpio_probe() fail with -EINVAL error.

[1.608516] [] qpnpint_irq_domain_dt_translate+0x168/0x194
[1.613557] [] irq_create_fwspec_mapping+0x17c/0x2d8
[1.620672] [] irq_create_of_mapping+0x64/0x74
[1.627008] [] of_irq_get+0x54/0x64
[1.633169] [] platform_get_irq+0x20/0x150
[1.638117] [] platform_irq_count+0x28/0x44
[1.643850] [] pmic_gpio_probe+0x50/0x544

ShawnI just realize this patch is trying to fix this issue from spmi driver

level. Actually I had submitted a change in spmi-gpio driver to fix
this by ignoring the GPIOs which the IRQ is not owned by APPS
processor. The maintainer hasn't reviewed it yet:
https://www.spinics.net/lists/linux-arm-msm/msg28849.html
I am trying to understand if my patch is still needed if Kiran's patch
get merged, the intention for my patch originally is for fixing the same
probe failure, but it could hide the GPIOs which are not allowed to use
from the pinctrl driver level. Please help to suggest.
Thanks

Fenglin


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-07-24 Thread Fenglin Wu 

On 7/25/2017 3:09 AM, Rob Herring wrote:

  
+   Definition: Array of the GPIO hardware numbers corresponding to GPIOs
+   which the APSS processor is not allowed to configure.
+   The hardware numbers are indexed from 1.
+   The interrupt resources for these GPIOs must not be defined
+   in "interrupts" and "interrupt-names" properties.
+   GPIOs defined in this array won't be registered as pins
+   in the pinctrl device or gpios in the gpio chip.

Isn't simply not assigning GPIOs to anything in the DT sufficient to not


Thanks for the question, Ron.
Previous implementation assumes all GPIOs are accessible from APSS
processor and it gets the total pin numbers by counting IRQs. It
registers pinctrl devices with the total pin numbers and assumes all the
pin are indexed continuously from hardware.
Current case is, some GPIOs' interrupt are not owned by APSS processor
and there would be errors when creating IRQ mapping for them. Yes, We
can exclude them from the "interrupts" property but the driver won't
shift the GPIO pad index automatically. Such as: PMI8998 has 14 GPIOs
from GPIO1 to GPIO14, and GPIO4/GPIO7/GPIO13 are not accessible from
APPS processor, we can excluded them from the interrupt assignment (in
following sample) and DON'T expect to register pins for them, but the
driver would count the IRQ numbers to 11 and register pins for
GPIO1 ~ GPIO11.
So I am adding this property "qcom,gpios-disallowed"  for these
inaccessible GPIOs then the driver would exclude them and register pins
for the right GPIO pads.

Samples:

interrupts = <0x2 0xc0 0 IRQ_TYPE_NONE>,
<0x2 0xc1 0 IRQ_TYPE_NONE>,
<0x2 0xc2 0 IRQ_TYPE_NONE>,
<0x2 0xc4 0 IRQ_TYPE_NONE>,
<0x2 0xc5 0 IRQ_TYPE_NONE>,
<0x2 0xc7 0 IRQ_TYPE_NONE>,
<0x2 0xc8 0 IRQ_TYPE_NONE>,
<0x2 0xc9 0 IRQ_TYPE_NONE>,
<0x2 0xca 0 IRQ_TYPE_NONE>,
<0x2 0xcb 0 IRQ_TYPE_NONE>,
<0x2 0xcd 0 IRQ_TYPE_NONE>;
interrupt-names = "pmi8998_gpio1", "pmi8998_gpio2",
"pmi8998_gpio3", "pmi8998_gpio5",
"pmi8998_gpio6", "pmi8998_gpio8",
"pmi8998_gpio9", "pmi8998_gpio10",
"pmi8998_gpio11", "pmi8998_gpio12",
"pmi8998_gpio14";
qcom,gpios-disallowed = <4 7 13>;

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

Re: [PATCH V1] pinctrl: qcom: spmi-gpio: Add support for qcom,gpios-disallowed property

2017-07-24 Thread Fenglin Wu 

On 7/25/2017 3:09 AM, Rob Herring wrote:

  
+   Definition: Array of the GPIO hardware numbers corresponding to GPIOs
+   which the APSS processor is not allowed to configure.
+   The hardware numbers are indexed from 1.
+   The interrupt resources for these GPIOs must not be defined
+   in "interrupts" and "interrupt-names" properties.
+   GPIOs defined in this array won't be registered as pins
+   in the pinctrl device or gpios in the gpio chip.

Isn't simply not assigning GPIOs to anything in the DT sufficient to not


Thanks for the question, Ron.
Previous implementation assumes all GPIOs are accessible from APSS
processor and it gets the total pin numbers by counting IRQs. It
registers pinctrl devices with the total pin numbers and assumes all the
pin are indexed continuously from hardware.
Current case is, some GPIOs' interrupt are not owned by APSS processor
and there would be errors when creating IRQ mapping for them. Yes, We
can exclude them from the "interrupts" property but the driver won't
shift the GPIO pad index automatically. Such as: PMI8998 has 14 GPIOs
from GPIO1 to GPIO14, and GPIO4/GPIO7/GPIO13 are not accessible from
APPS processor, we can excluded them from the interrupt assignment (in
following sample) and DON'T expect to register pins for them, but the
driver would count the IRQ numbers to 11 and register pins for
GPIO1 ~ GPIO11.
So I am adding this property "qcom,gpios-disallowed"  for these
inaccessible GPIOs then the driver would exclude them and register pins
for the right GPIO pads.

Samples:

interrupts = <0x2 0xc0 0 IRQ_TYPE_NONE>,
<0x2 0xc1 0 IRQ_TYPE_NONE>,
<0x2 0xc2 0 IRQ_TYPE_NONE>,
<0x2 0xc4 0 IRQ_TYPE_NONE>,
<0x2 0xc5 0 IRQ_TYPE_NONE>,
<0x2 0xc7 0 IRQ_TYPE_NONE>,
<0x2 0xc8 0 IRQ_TYPE_NONE>,
<0x2 0xc9 0 IRQ_TYPE_NONE>,
<0x2 0xca 0 IRQ_TYPE_NONE>,
<0x2 0xcb 0 IRQ_TYPE_NONE>,
<0x2 0xcd 0 IRQ_TYPE_NONE>;
interrupt-names = "pmi8998_gpio1", "pmi8998_gpio2",
"pmi8998_gpio3", "pmi8998_gpio5",
"pmi8998_gpio6", "pmi8998_gpio8",
"pmi8998_gpio9", "pmi8998_gpio10",
"pmi8998_gpio11", "pmi8998_gpio12",
"pmi8998_gpio14";
qcom,gpios-disallowed = <4 7 13>;

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

Re: [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 5:33 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@codeaurora.org>

Power source selection in DIG_VIN_CTL is indexed from 0, in the range
check it shouldn't be equal to the total number of power sources.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>


Reviewed-by: Bjorn Andersson <bjorn.anders...@linaro.org>

This patch is unrelated to the other patches in the series, when this is
the case it's better to send it on its own.

Regards,
Bjorn


Sure, I will send it as an independent patch.

---
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 581309d..1fd677c 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
pad->is_enabled = false;
break;
case PIN_CONFIG_POWER_SOURCE:
-   if (arg > pad->num_sources)
+   if (arg >= pad->num_sources)
return -EINVAL;
pad->power_source = arg;
break;
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.



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

Re: [PATCH V1 3/3] pinctrl: qcom: spmi-gpio: Correct power_source range check

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 5:33 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

Power source selection in DIG_VIN_CTL is indexed from 0, in the range
check it shouldn't be equal to the total number of power sources.

Signed-off-by: Fenglin Wu 


Reviewed-by: Bjorn Andersson 

This patch is unrelated to the other patches in the series, when this is
the case it's better to send it on its own.

Regards,
Bjorn


Sure, I will send it as an independent patch.

---
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 581309d..1fd677c 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -500,7 +500,7 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
pad->is_enabled = false;
break;
case PIN_CONFIG_POWER_SOURCE:
-   if (arg > pad->num_sources)
+   if (arg >= pad->num_sources)
return -EINVAL;
pad->power_source = arg;
break;
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.



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

Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 5:24 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@codeaurora.org>

Add property "qcom,dtest-buffer" to specify which dtest rail to feed
when the pin is configured as a digital input.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt | 15 
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 45 ++
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |  6 +++
  3 files changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 1493c0a..521c783 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -195,6 +195,21 @@ to specify in a pin configuration subnode:
Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
defined in .
  
+- qcom,dtest-buffer:

+   Usage: optional
+   Value type: 
+   Definition: Selects DTEST rail to route to GPIO when it's configured
+   as a digital input.
+   For LV/MV GPIO subtypes, the valid values are 0-3
+   corresponding to PMIC_GPIO_DIN_DTESTx defined in
+   . Only one
+   DTEST rail can be selected at a time.


As with the analog lines, this is a natural number and I think we should
encode it as such in the DeviceTree.


+   For 4CH/8CH GPIO subtypes, supported values are 1-15.
+   4 DTEST rails are supported in total and more than 1 DTEST
+   rail can be selected simultaneously. Each bit of the
+   4 LSBs represent one DTEST rail, such as [3:0] = 0101
+   means both DTEST1 and DTEST3 are selected.


I'm not too keen on encoding this as a bitmask. I would much rather
encode it as multiple values - although that complicates the
implementation.

Or can we just ignore it? (Is the lack of support in the newer chips a
result of no-one using this?)


I am not quite sure if any real cases would route multiple DTEST line to
single GPIO, but the register mapping uses the bit mask for 4CH/8CH
subtypes and I think we should support it accordingly. Even if I drop
the support, we still have the difference of the register mapping on the
dtest selection between MV/LV subtypes and legacy 4CH/8CH subtypes,
which means we need a place to unify the dtest definition. I put the
complication here in dtsi which the end user would choose the right
value according to the hardware. I am also fine with putting the
complication in C code, but that would drop the supporting of multiple
DTEST lines selection for 4CH/8CH subtype.




+
  Example:
  
  	pm8921_gpio: gpio@150 {

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

[..]

@@ -512,6 +526,13 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
return -EINVAL;
pad->atest = arg;
break;
+   case PMIC_GPIO_CONF_DTEST_BUFFER:
+   if ((pad->lv_mv_type && arg > PMIC_GPIO_DIN_DTEST4)
+   || (!pad->lv_mv_type && arg >
+   PMIC_GPIO_DIG_IN_DTEST_SEL_MASK))
+   return -EINVAL;
+   pad->dtest_buffer = arg;
+   break;
default:
return -EINVAL;
}
@@ -544,6 +565,17 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
val = PMIC_GPIO_MODE_DIGITAL_OUTPUT;
}
  


Remember that you're supposed to be able to have two different states
defines, one with dtest-buffer and one without - and switching between
them should enable _and_ disable the dtest buffer.

So you need to detect the absence of qcom,dtest-buffer and you need to
write the register in this case as well. So before looping over all the
config parameters, set pad->dtest_buffer to "disabled" and when you get
here it will either be disabled or have the specified value.




+   if (pad->dtest_buffer != INT_MAX) {
+   val = pad->dtest_buffer;
+   if (pad->lv_mv_type)
+   val |= PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN;
+


Instead of representing "disabled" as INT_MAX, you could keep track of
it in the same representation as the hardware, i.e. 0 would be disabled.

The additional effort would be in the lv_mv case that you need to mask
in PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN in the few places where you actually
enable dtest buffering.



Thanks for your suggestion

Re: [PATCH V1 2/3] pinctrl: qcom: spmi-gpio: Add dtest route for digital input

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 5:24 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

Add property "qcom,dtest-buffer" to specify which dtest rail to feed
when the pin is configured as a digital input.

Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt | 15 
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 45 ++
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |  6 +++
  3 files changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 1493c0a..521c783 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -195,6 +195,21 @@ to specify in a pin configuration subnode:
Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
defined in .
  
+- qcom,dtest-buffer:

+   Usage: optional
+   Value type: 
+   Definition: Selects DTEST rail to route to GPIO when it's configured
+   as a digital input.
+   For LV/MV GPIO subtypes, the valid values are 0-3
+   corresponding to PMIC_GPIO_DIN_DTESTx defined in
+   . Only one
+   DTEST rail can be selected at a time.


As with the analog lines, this is a natural number and I think we should
encode it as such in the DeviceTree.


+   For 4CH/8CH GPIO subtypes, supported values are 1-15.
+   4 DTEST rails are supported in total and more than 1 DTEST
+   rail can be selected simultaneously. Each bit of the
+   4 LSBs represent one DTEST rail, such as [3:0] = 0101
+   means both DTEST1 and DTEST3 are selected.


I'm not too keen on encoding this as a bitmask. I would much rather
encode it as multiple values - although that complicates the
implementation.

Or can we just ignore it? (Is the lack of support in the newer chips a
result of no-one using this?)


I am not quite sure if any real cases would route multiple DTEST line to
single GPIO, but the register mapping uses the bit mask for 4CH/8CH
subtypes and I think we should support it accordingly. Even if I drop
the support, we still have the difference of the register mapping on the
dtest selection between MV/LV subtypes and legacy 4CH/8CH subtypes,
which means we need a place to unify the dtest definition. I put the
complication here in dtsi which the end user would choose the right
value according to the hardware. I am also fine with putting the
complication in C code, but that would drop the supporting of multiple
DTEST lines selection for 4CH/8CH subtype.




+
  Example:
  
  	pm8921_gpio: gpio@150 {

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

[..]

@@ -512,6 +526,13 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
return -EINVAL;
pad->atest = arg;
break;
+   case PMIC_GPIO_CONF_DTEST_BUFFER:
+   if ((pad->lv_mv_type && arg > PMIC_GPIO_DIN_DTEST4)
+   || (!pad->lv_mv_type && arg >
+   PMIC_GPIO_DIG_IN_DTEST_SEL_MASK))
+   return -EINVAL;
+   pad->dtest_buffer = arg;
+   break;
default:
return -EINVAL;
}
@@ -544,6 +565,17 @@ static int pmic_gpio_config_set(struct pinctrl_dev 
*pctldev, unsigned int pin,
val = PMIC_GPIO_MODE_DIGITAL_OUTPUT;
}
  


Remember that you're supposed to be able to have two different states
defines, one with dtest-buffer and one without - and switching between
them should enable _and_ disable the dtest buffer.

So you need to detect the absence of qcom,dtest-buffer and you need to
write the register in this case as well. So before looping over all the
config parameters, set pad->dtest_buffer to "disabled" and when you get
here it will either be disabled or have the specified value.




+   if (pad->dtest_buffer != INT_MAX) {
+   val = pad->dtest_buffer;
+   if (pad->lv_mv_type)
+   val |= PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN;
+


Instead of representing "disabled" as INT_MAX, you could keep track of
it in the same representation as the hardware, i.e. 0 would be disabled.

The additional effort would be in the lv_mv case that you need to mask
in PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN in the few places where you actually
enable dtest buffering.



Thanks for your suggestion, I got the issue here. 
PMIC_GPIO_LV_MV_DIG_IN_DTEST_EN need to be

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 4:55 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu <fengl...@codeaurora.org>

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..1493c0a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -91,14 +91,18 @@ to specify in a pin configuration subnode:
Value type: 
Definition: Specify the alternative function to be configured for the
specified pins.  Valid values are:
-   "normal",
-   "paired",
-   "func1",
-   "func2",
-   "dtest1",
-   "dtest2",
-   "dtest3",
-   "dtest4"
+   "normal",
+   "paired",
+   "func1",
+   "func2",
+   "dtest1",
+   "dtest2",
+   "dtest3",
+   "dtest4",
+   And following values are supported by LV/MV GPIO subtypes:
+   "func3",
+   "func4",
+   "analog"


Please keep the indentation of the existing lines.

Sure, I will fix the indent.


  
  - bias-disable:

Usage: optional
@@ -183,6 +187,14 @@ to specify in a pin configuration subnode:
Value type: 
Definition: The specified pins are configured in open-source mode.
  
+- qcom,atest:

+   Usage: optional
+   Value type: 
+   Definition: Selects ATEST rail to route to GPIO when it's configured
+   in analog-pass-through mode by specifying "analog" function.
+   Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
+   defined in .
+
  Example:
  
  	pm8921_gpio: gpio@150 {

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..caa07e9 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -40,6 +40,8 @@
  #define PMIC_GPIO_SUBTYPE_GPIOC_4CH   0x5
  #define PMIC_GPIO_SUBTYPE_GPIO_8CH0x9
  #define PMIC_GPIO_SUBTYPE_GPIOC_8CH   0xd
+#define PMIC_GPIO_SUBTYPE_GPIO_LV  0x10
+#define PMIC_GPIO_SUBTYPE_GPIO_MV  0x11
  
  #define PMIC_MPP_REG_RT_STS			0x10

  #define PMIC_MPP_REG_RT_STS_VAL_MASK  0x1
@@ -48,8 +50,10 @@
  #define PMIC_GPIO_REG_MODE_CTL0x40
  #define PMIC_GPIO_REG_DIG_VIN_CTL 0x41
  #define PMIC_GPIO_REG_DIG_PULL_CTL0x42
+#define PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL 0x44
  #define PMIC_GPIO_REG_DIG_OUT_CTL 0x45
  #define PMIC_GPIO_REG_EN_CTL  0x46
+#define PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL  0x4A
  
  /* PMIC_GPIO_REG_MODE_CTL */

  #define PMIC_GPIO_REG_MODE_VALUE_SHIFT0x1
@@ -58,6 +62,13 @@
  #define PMIC_GPIO_REG_MODE_DIR_SHIFT  4
  #define PMIC_GPIO_REG_MODE_DIR_MASK   0x7
  
+#define PMIC_GPIO_MODE_DIGITAL_INPUT		0

+#define PMIC_GPIO_MODE_DIGITAL_OUTPUT  1
+#define PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT2
+#define PMIC_GPIO_MODE_ANALOG_PASS_THRU3
+
+#define PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK  0x3
+
  /* PMIC_GPIO_REG_DIG_VIN_CTL */
  #define PMIC_GPIO_REG_VIN_SHIFT   0
  #define PMIC_GPIO_REG_VIN_MASK0x7
@@ -69,6 +80,11 @@
  #define PMIC_GPIO_PULL_DOWN   4
  #define PMIC_GPIO_PULL_DISABLE5
  
+/* PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL for LV/MV */

+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT  0x80
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT7
+#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
+
  /* PMIC_GPIO_REG_DIG_OUT_CTL */
  #define PMIC_GPIO_REG_OUT_STRENGTH_SHIFT  0
  #define PMIC_GPIO_REG_OUT_STRENGTH_MASK   0x3
@@ -88,9 +104,28 @@
  
  #define PMIC_GPIO_PHYSICAL_OFFSET		1
  
+/* PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL */

+#define PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK   0

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-07-12 Thread Fenglin Wu 

On 7/13/2017 4:55 AM, Bjorn Andersson wrote:

On Mon 12 Jun 23:16 PDT 2017, fengl...@codeaurora.org wrote:


From: Fenglin Wu 

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..1493c0a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -91,14 +91,18 @@ to specify in a pin configuration subnode:
Value type: 
Definition: Specify the alternative function to be configured for the
specified pins.  Valid values are:
-   "normal",
-   "paired",
-   "func1",
-   "func2",
-   "dtest1",
-   "dtest2",
-   "dtest3",
-   "dtest4"
+   "normal",
+   "paired",
+   "func1",
+   "func2",
+   "dtest1",
+   "dtest2",
+   "dtest3",
+   "dtest4",
+   And following values are supported by LV/MV GPIO subtypes:
+   "func3",
+   "func4",
+   "analog"


Please keep the indentation of the existing lines.

Sure, I will fix the indent.


  
  - bias-disable:

Usage: optional
@@ -183,6 +187,14 @@ to specify in a pin configuration subnode:
Value type: 
Definition: The specified pins are configured in open-source mode.
  
+- qcom,atest:

+   Usage: optional
+   Value type: 
+   Definition: Selects ATEST rail to route to GPIO when it's configured
+   in analog-pass-through mode by specifying "analog" function.
+   Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
+   defined in .
+
  Example:
  
  	pm8921_gpio: gpio@150 {

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..caa07e9 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -40,6 +40,8 @@
  #define PMIC_GPIO_SUBTYPE_GPIOC_4CH   0x5
  #define PMIC_GPIO_SUBTYPE_GPIO_8CH0x9
  #define PMIC_GPIO_SUBTYPE_GPIOC_8CH   0xd
+#define PMIC_GPIO_SUBTYPE_GPIO_LV  0x10
+#define PMIC_GPIO_SUBTYPE_GPIO_MV  0x11
  
  #define PMIC_MPP_REG_RT_STS			0x10

  #define PMIC_MPP_REG_RT_STS_VAL_MASK  0x1
@@ -48,8 +50,10 @@
  #define PMIC_GPIO_REG_MODE_CTL0x40
  #define PMIC_GPIO_REG_DIG_VIN_CTL 0x41
  #define PMIC_GPIO_REG_DIG_PULL_CTL0x42
+#define PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL 0x44
  #define PMIC_GPIO_REG_DIG_OUT_CTL 0x45
  #define PMIC_GPIO_REG_EN_CTL  0x46
+#define PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL  0x4A
  
  /* PMIC_GPIO_REG_MODE_CTL */

  #define PMIC_GPIO_REG_MODE_VALUE_SHIFT0x1
@@ -58,6 +62,13 @@
  #define PMIC_GPIO_REG_MODE_DIR_SHIFT  4
  #define PMIC_GPIO_REG_MODE_DIR_MASK   0x7
  
+#define PMIC_GPIO_MODE_DIGITAL_INPUT		0

+#define PMIC_GPIO_MODE_DIGITAL_OUTPUT  1
+#define PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT2
+#define PMIC_GPIO_MODE_ANALOG_PASS_THRU3
+
+#define PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK  0x3
+
  /* PMIC_GPIO_REG_DIG_VIN_CTL */
  #define PMIC_GPIO_REG_VIN_SHIFT   0
  #define PMIC_GPIO_REG_VIN_MASK0x7
@@ -69,6 +80,11 @@
  #define PMIC_GPIO_PULL_DOWN   4
  #define PMIC_GPIO_PULL_DISABLE5
  
+/* PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL for LV/MV */

+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT  0x80
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT7
+#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
+
  /* PMIC_GPIO_REG_DIG_OUT_CTL */
  #define PMIC_GPIO_REG_OUT_STRENGTH_SHIFT  0
  #define PMIC_GPIO_REG_OUT_STRENGTH_MASK   0x3
@@ -88,9 +104,28 @@
  
  #define PMIC_GPIO_PHYSICAL_OFFSET		1
  
+/* PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL */

+#define PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK   0x3
+
  /* Qualcomm specific pin configurations */
  #define PMIC_GPIO_C

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-07-05 Thread Fenglin Wu 

Hi Bjorn and Ivan,

Could you help to take some time to look at these spmi-gpio pinctrl
patches?
Thanks.

On 6/20/2017 7:15 PM, Linus Walleij wrote:

Bjrön and/or Ivan: please look at this.

Yours,
Linus Walleij

On Tue, Jun 13, 2017 at 8:16 AM,  <fengl...@codeaurora.org> wrote:


From: Fenglin Wu <fengl...@codeaurora.org>

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..1493c0a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -91,14 +91,18 @@ to specify in a pin configuration subnode:
 Value type: 
 Definition: Specify the alternative function to be configured for the
 specified pins.  Valid values are:
-   "normal",
-   "paired",
-   "func1",
-   "func2",
-   "dtest1",
-   "dtest2",
-   "dtest3",
-   "dtest4"
+   "normal",
+   "paired",
+   "func1",
+   "func2",
+   "dtest1",
+   "dtest2",
+   "dtest3",
+   "dtest4",
+   And following values are supported by LV/MV GPIO subtypes:
+   "func3",
+   "func4",
+   "analog"

  - bias-disable:
 Usage: optional
@@ -183,6 +187,14 @@ to specify in a pin configuration subnode:
 Value type: 
 Definition: The specified pins are configured in open-source mode.

+- qcom,atest:
+   Usage: optional
+   Value type: 
+   Definition: Selects ATEST rail to route to GPIO when it's configured
+   in analog-pass-through mode by specifying "analog" function.
+   Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
+   defined in .
+
  Example:

 pm8921_gpio: gpio@150 {
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..caa07e9 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -40,6 +40,8 @@
  #define PMIC_GPIO_SUBTYPE_GPIOC_4CH0x5
  #define PMIC_GPIO_SUBTYPE_GPIO_8CH 0x9
  #define PMIC_GPIO_SUBTYPE_GPIOC_8CH0xd
+#define PMIC_GPIO_SUBTYPE_GPIO_LV  0x10
+#define PMIC_GPIO_SUBTYPE_GPIO_MV  0x11

  #define PMIC_MPP_REG_RT_STS0x10
  #define PMIC_MPP_REG_RT_STS_VAL_MASK   0x1
@@ -48,8 +50,10 @@
  #define PMIC_GPIO_REG_MODE_CTL 0x40
  #define PMIC_GPIO_REG_DIG_VIN_CTL  0x41
  #define PMIC_GPIO_REG_DIG_PULL_CTL 0x42
+#define PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL 0x44
  #define PMIC_GPIO_REG_DIG_OUT_CTL  0x45
  #define PMIC_GPIO_REG_EN_CTL   0x46
+#define PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL  0x4A

  /* PMIC_GPIO_REG_MODE_CTL */
  #define PMIC_GPIO_REG_MODE_VALUE_SHIFT 0x1
@@ -58,6 +62,13 @@
  #define PMIC_GPIO_REG_MODE_DIR_SHIFT   4
  #define PMIC_GPIO_REG_MODE_DIR_MASK0x7

+#define PMIC_GPIO_MODE_DIGITAL_INPUT   0
+#define PMIC_GPIO_MODE_DIGITAL_OUTPUT  1
+#define PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT2
+#define PMIC_GPIO_MODE_ANALOG_PASS_THRU3
+
+#define PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK  0x3
+
  /* PMIC_GPIO_REG_DIG_VIN_CTL */
  #define PMIC_GPIO_REG_VIN_SHIFT0
  #define PMIC_GPIO_REG_VIN_MASK 0x7
@@ -69,6 +80,11 @@
  #define PMIC_GPIO_PULL_DOWN4
  #define PMIC_GPIO_PULL_DISABLE 5

+/* PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL for LV/MV */
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT  0x80
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT7
+#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
+
  /* PMIC_GPIO_REG_DIG_OUT_CTL */
  #define PMIC_GPIO_REG_OUT_STRENGTH_SHIFT   0
  #define PMIC_GPIO_REG_OUT_STRENGTH_MASK0x3
@@ -88,9 +104,28 @@

  #define PMIC_GPIO_PHYSICAL_OFFSET  1

+/

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-07-05 Thread Fenglin Wu 

Hi Bjorn and Ivan,

Could you help to take some time to look at these spmi-gpio pinctrl
patches?
Thanks.

On 6/20/2017 7:15 PM, Linus Walleij wrote:

Bjrön and/or Ivan: please look at this.

Yours,
Linus Walleij

On Tue, Jun 13, 2017 at 8:16 AM,   wrote:


From: Fenglin Wu 

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt 
b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
index 8d893a8..1493c0a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.txt
@@ -91,14 +91,18 @@ to specify in a pin configuration subnode:
 Value type: 
 Definition: Specify the alternative function to be configured for the
 specified pins.  Valid values are:
-   "normal",
-   "paired",
-   "func1",
-   "func2",
-   "dtest1",
-   "dtest2",
-   "dtest3",
-   "dtest4"
+   "normal",
+   "paired",
+   "func1",
+   "func2",
+   "dtest1",
+   "dtest2",
+   "dtest3",
+   "dtest4",
+   And following values are supported by LV/MV GPIO subtypes:
+   "func3",
+   "func4",
+   "analog"

  - bias-disable:
 Usage: optional
@@ -183,6 +187,14 @@ to specify in a pin configuration subnode:
 Value type: 
 Definition: The specified pins are configured in open-source mode.

+- qcom,atest:
+   Usage: optional
+   Value type: 
+   Definition: Selects ATEST rail to route to GPIO when it's configured
+   in analog-pass-through mode by specifying "analog" function.
+   Valid values are 0-3 corresponding to PMIC_GPIO_AOUT_ATESTx
+   defined in .
+
  Example:

 pm8921_gpio: gpio@150 {
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 664b641..caa07e9 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -40,6 +40,8 @@
  #define PMIC_GPIO_SUBTYPE_GPIOC_4CH0x5
  #define PMIC_GPIO_SUBTYPE_GPIO_8CH 0x9
  #define PMIC_GPIO_SUBTYPE_GPIOC_8CH0xd
+#define PMIC_GPIO_SUBTYPE_GPIO_LV  0x10
+#define PMIC_GPIO_SUBTYPE_GPIO_MV  0x11

  #define PMIC_MPP_REG_RT_STS0x10
  #define PMIC_MPP_REG_RT_STS_VAL_MASK   0x1
@@ -48,8 +50,10 @@
  #define PMIC_GPIO_REG_MODE_CTL 0x40
  #define PMIC_GPIO_REG_DIG_VIN_CTL  0x41
  #define PMIC_GPIO_REG_DIG_PULL_CTL 0x42
+#define PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL 0x44
  #define PMIC_GPIO_REG_DIG_OUT_CTL  0x45
  #define PMIC_GPIO_REG_EN_CTL   0x46
+#define PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL  0x4A

  /* PMIC_GPIO_REG_MODE_CTL */
  #define PMIC_GPIO_REG_MODE_VALUE_SHIFT 0x1
@@ -58,6 +62,13 @@
  #define PMIC_GPIO_REG_MODE_DIR_SHIFT   4
  #define PMIC_GPIO_REG_MODE_DIR_MASK0x7

+#define PMIC_GPIO_MODE_DIGITAL_INPUT   0
+#define PMIC_GPIO_MODE_DIGITAL_OUTPUT  1
+#define PMIC_GPIO_MODE_DIGITAL_INPUT_OUTPUT2
+#define PMIC_GPIO_MODE_ANALOG_PASS_THRU3
+
+#define PMIC_GPIO_REG_LV_MV_MODE_DIR_MASK  0x3
+
  /* PMIC_GPIO_REG_DIG_VIN_CTL */
  #define PMIC_GPIO_REG_VIN_SHIFT0
  #define PMIC_GPIO_REG_VIN_MASK 0x7
@@ -69,6 +80,11 @@
  #define PMIC_GPIO_PULL_DOWN4
  #define PMIC_GPIO_PULL_DISABLE 5

+/* PMIC_GPIO_REG_LV_MV_DIG_OUT_SOURCE_CTL for LV/MV */
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT  0x80
+#define PMIC_GPIO_LV_MV_OUTPUT_INVERT_SHIFT7
+#define PMIC_GPIO_LV_MV_OUTPUT_SOURCE_SEL_MASK 0xF
+
  /* PMIC_GPIO_REG_DIG_OUT_CTL */
  #define PMIC_GPIO_REG_OUT_STRENGTH_SHIFT   0
  #define PMIC_GPIO_REG_OUT_STRENGTH_MASK0x3
@@ -88,9 +104,28 @@

  #define PMIC_GPIO_PHYSICAL_OFFSET  1

+/* PMIC_GPIO_REG_LV_MV_ANA_PASS_THRU_SEL */
+#define PMIC_GPIO_LV_MV_ANA_MUX_SEL_MASK

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-06-18 Thread Fenglin Wu 

On 6/19/2017 9:00 AM, Fenglin Wu wrote:

On 6/18/2017 10:04 PM, Rob Herring wrote:

On Tue, Jun 13, 2017 at 02:16:03PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu <fengl...@codeaurora.org>

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 
++---

  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)


[...]

diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h 
b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h

index d33f17c..85d8809 100644
--- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
@@ -93,15 +93,24 @@
  #define PM8994_GPIO_S42
  #define PM8994_GPIO_L123
+/* ATEST MUX selection for analog-pass-through mode */
+#define PMIC_GPIO_AOUT_ATEST10
+#define PMIC_GPIO_AOUT_ATEST21
+#define PMIC_GPIO_AOUT_ATEST32
+#define PMIC_GPIO_AOUT_ATEST43
+
  /* To be used with "function" */
  #define PMIC_GPIO_FUNC_NORMAL"normal"
  #define PMIC_GPIO_FUNC_PAIRED"paired"
  #define PMIC_GPIO_FUNC_FUNC1"func1"
  #define PMIC_GPIO_FUNC_FUNC2"func2"
+#define PMIC_GPIO_FUNC_FUNC3"func3"
+#define PMIC_GPIO_FUNC_FUNC4"func4"
  #define PMIC_GPIO_FUNC_DTEST1"dtest1"
  #define PMIC_GPIO_FUNC_DTEST2"dtest2"
  #define PMIC_GPIO_FUNC_DTEST3"dtest3"
  #define PMIC_GPIO_FUNC_DTEST4"dtest4"
+#define PMIC_GPIO_FUNC_ANALOG"analog"


defines for strings? That's really pointless. I'd prefer you drop using
them than add more.

Thanks for the suggestion, I will remove these string definitions in 
next patch.

Does other part look good? I would post a new patch if no other comments.

Sorry, I hadn't noticed there are so many definitions depend on them. I 
take my word back and I think it deserves more discussion.


The function names "func1/func2/func3/func4" defined for GPIO hardware 
module are very generic. Each GPIO located in different PMICs would have 
its special function according to different hardware design, such as: 
batt_alarm, keypad_drv, div_clk, etc.


The dt-binding header file defines the PMIC GPIOs' special functions 
which depending on these string definitions (samples list below). This 
gives a good understanding to end user if they requires to set the GPIO 
special function but not having a hardware specification to explain the 
details.


If we remove these string definitions, we would have another place to 
explain these mapping of "funcx" to any GPIOs' special functions in each 
PMICs, would dt-binding document be a good place to have them?



#define PM8038_GPIO1_2_LPG_DRV  PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_ENPMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK   PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN   PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK  PMIC_GPIO_FUNC_FUNC1
...



Rob
--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html





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

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-06-18 Thread Fenglin Wu 

On 6/19/2017 9:00 AM, Fenglin Wu wrote:

On 6/18/2017 10:04 PM, Rob Herring wrote:

On Tue, Jun 13, 2017 at 02:16:03PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu 

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 
++---

  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)


[...]

diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h 
b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h

index d33f17c..85d8809 100644
--- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
@@ -93,15 +93,24 @@
  #define PM8994_GPIO_S42
  #define PM8994_GPIO_L123
+/* ATEST MUX selection for analog-pass-through mode */
+#define PMIC_GPIO_AOUT_ATEST10
+#define PMIC_GPIO_AOUT_ATEST21
+#define PMIC_GPIO_AOUT_ATEST32
+#define PMIC_GPIO_AOUT_ATEST43
+
  /* To be used with "function" */
  #define PMIC_GPIO_FUNC_NORMAL"normal"
  #define PMIC_GPIO_FUNC_PAIRED"paired"
  #define PMIC_GPIO_FUNC_FUNC1"func1"
  #define PMIC_GPIO_FUNC_FUNC2"func2"
+#define PMIC_GPIO_FUNC_FUNC3"func3"
+#define PMIC_GPIO_FUNC_FUNC4"func4"
  #define PMIC_GPIO_FUNC_DTEST1"dtest1"
  #define PMIC_GPIO_FUNC_DTEST2"dtest2"
  #define PMIC_GPIO_FUNC_DTEST3"dtest3"
  #define PMIC_GPIO_FUNC_DTEST4"dtest4"
+#define PMIC_GPIO_FUNC_ANALOG"analog"


defines for strings? That's really pointless. I'd prefer you drop using
them than add more.

Thanks for the suggestion, I will remove these string definitions in 
next patch.

Does other part look good? I would post a new patch if no other comments.

Sorry, I hadn't noticed there are so many definitions depend on them. I 
take my word back and I think it deserves more discussion.


The function names "func1/func2/func3/func4" defined for GPIO hardware 
module are very generic. Each GPIO located in different PMICs would have 
its special function according to different hardware design, such as: 
batt_alarm, keypad_drv, div_clk, etc.


The dt-binding header file defines the PMIC GPIOs' special functions 
which depending on these string definitions (samples list below). This 
gives a good understanding to end user if they requires to set the GPIO 
special function but not having a hardware specification to explain the 
details.


If we remove these string definitions, we would have another place to 
explain these mapping of "funcx" to any GPIOs' special functions in each 
PMICs, would dt-binding document be a good place to have them?



#define PM8038_GPIO1_2_LPG_DRV  PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_ENPMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK   PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN   PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN PMIC_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK  PMIC_GPIO_FUNC_FUNC1
...



Rob
--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html





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

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-06-18 Thread Fenglin Wu 

On 6/18/2017 10:04 PM, Rob Herring wrote:

On Tue, Jun 13, 2017 at 02:16:03PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu <fengl...@codeaurora.org>

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu <fengl...@codeaurora.org>
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)


[...]


diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h 
b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
index d33f17c..85d8809 100644
--- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
@@ -93,15 +93,24 @@
  #define PM8994_GPIO_S42
  #define PM8994_GPIO_L12   3
  
+/* ATEST MUX selection for analog-pass-through mode */

+#define PMIC_GPIO_AOUT_ATEST1  0
+#define PMIC_GPIO_AOUT_ATEST2  1
+#define PMIC_GPIO_AOUT_ATEST3  2
+#define PMIC_GPIO_AOUT_ATEST4  3
+
  /* To be used with "function" */
  #define PMIC_GPIO_FUNC_NORMAL "normal"
  #define PMIC_GPIO_FUNC_PAIRED "paired"
  #define PMIC_GPIO_FUNC_FUNC1  "func1"
  #define PMIC_GPIO_FUNC_FUNC2  "func2"
+#define PMIC_GPIO_FUNC_FUNC3   "func3"
+#define PMIC_GPIO_FUNC_FUNC4   "func4"
  #define PMIC_GPIO_FUNC_DTEST1 "dtest1"
  #define PMIC_GPIO_FUNC_DTEST2 "dtest2"
  #define PMIC_GPIO_FUNC_DTEST3 "dtest3"
  #define PMIC_GPIO_FUNC_DTEST4 "dtest4"
+#define PMIC_GPIO_FUNC_ANALOG  "analog"


defines for strings? That's really pointless. I'd prefer you drop using
them than add more.

Thanks for the suggestion, I will remove these string definitions in 
next patch.

Does other part look good? I would post a new patch if no other comments.


Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH V1 1/3] pinctrl: qcom: spmi-gpio: Add support for GPIO LV/MV subtype

2017-06-18 Thread Fenglin Wu 

On 6/18/2017 10:04 PM, Rob Herring wrote:

On Tue, Jun 13, 2017 at 02:16:03PM +0800, fengl...@codeaurora.org wrote:

From: Fenglin Wu 

GPIO LV (low voltage)/MV (medium voltage) subtypes have different
features and register mappings than 4CH/8CH subtypes. Add support
for LV and MV subtypes.

Signed-off-by: Fenglin Wu 
---
  .../devicetree/bindings/pinctrl/qcom,pmic-gpio.txt |  28 ++-
  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c   | 269 ++---
  include/dt-bindings/pinctrl/qcom,pmic-gpio.h   |   9 +
  3 files changed, 264 insertions(+), 42 deletions(-)


[...]


diff --git a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h 
b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
index d33f17c..85d8809 100644
--- a/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pmic-gpio.h
@@ -93,15 +93,24 @@
  #define PM8994_GPIO_S42
  #define PM8994_GPIO_L12   3
  
+/* ATEST MUX selection for analog-pass-through mode */

+#define PMIC_GPIO_AOUT_ATEST1  0
+#define PMIC_GPIO_AOUT_ATEST2  1
+#define PMIC_GPIO_AOUT_ATEST3  2
+#define PMIC_GPIO_AOUT_ATEST4  3
+
  /* To be used with "function" */
  #define PMIC_GPIO_FUNC_NORMAL "normal"
  #define PMIC_GPIO_FUNC_PAIRED "paired"
  #define PMIC_GPIO_FUNC_FUNC1  "func1"
  #define PMIC_GPIO_FUNC_FUNC2  "func2"
+#define PMIC_GPIO_FUNC_FUNC3   "func3"
+#define PMIC_GPIO_FUNC_FUNC4   "func4"
  #define PMIC_GPIO_FUNC_DTEST1 "dtest1"
  #define PMIC_GPIO_FUNC_DTEST2 "dtest2"
  #define PMIC_GPIO_FUNC_DTEST3 "dtest3"
  #define PMIC_GPIO_FUNC_DTEST4 "dtest4"
+#define PMIC_GPIO_FUNC_ANALOG  "analog"


defines for strings? That's really pointless. I'd prefer you drop using
them than add more.

Thanks for the suggestion, I will remove these string definitions in 
next patch.

Does other part look good? I would post a new patch if no other comments.


Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



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