[RESEND,v2,1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

Commit Message

Ivan T. Ivanov July 17, 2014, 7:41 p.m. UTC

From: "Ivan T. Ivanov" <[e-mail]>

Available 'power-source' labels differ between chips.
Use just VIN0-VIN14 in the input source names.

PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
support only one function 'gpio'. Currently GPIO's will
support only 'normal' mode. Rest of the modes will be added
later, if needed.

We can not use generic drive-strength because Qualcomm hardware
define those values as low, medium and high. Use qcom,strength
for this.

We can not use generic bias-pull-up because Qualcomm hardware
define those values in uA's. Use qcom,pull-up for this.

Add qcom,pm8941-gpio and qcom,pma8084-gpio to chips, which
support these DT bindings.

Signed-off-by: Ivan T. Ivanov <[e-mail]>
---
 .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          | 97 +++++++++++-----------
 drivers/pinctrl/pinctrl-pm8xxx-gpio.c              | 34 ++++----
 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     | 33 ++++----
 3 files changed, 81 insertions(+), 83 deletions(-)

Comments

Ivan T. Ivanov July 22, 2014, 2:51 p.m. UTC | #1

On Thu, 2014-07-17 at 22:41 +0300, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <[e-mail]>
> 
> Available 'power-source' labels differ between chips.
> Use just VIN0-VIN14 in the input source names.
> 
> PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> support only one function 'gpio'. Currently GPIO's will
> support only 'normal' mode. Rest of the modes will be added
> later, if needed.
> 
> We can not use generic drive-strength because Qualcomm hardware
> define those values as low, medium and high. Use qcom,strength
> for this.
> 
> We can not use generic bias-pull-up because Qualcomm hardware
> define those values in uA's. Use qcom,pull-up for this.
> 
> Add qcom,pm8941-gpio and qcom,pma8084-gpio to chips, which
> support these DT bindings.
> 

Hi Bjorn,  

Are you ok with these changes? I plan to send next version which
adds parsing code for new "qcom,strength" and "qcom,pull-up".

Regards,
Ivan


> Signed-off-by: Ivan T. Ivanov <[e-mail]>
> ---
>  .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          | 97 +++++++++++-----------
>  drivers/pinctrl/pinctrl-pm8xxx-gpio.c              | 34 ++++----
>  include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     | 33 ++++----
>  3 files changed, 81 insertions(+), 83 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> index 0035dd8..f17580a 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> @@ -12,6 +12,8 @@ Qualcomm.
>  			"qcom,pm8058-gpio"
>  			"qcom,pm8917-gpio"
>  			"qcom,pm8921-gpio"
> +			"qcom,pm8941-gpio"
> +			"qcom,pma8084-gpio"
>  
>  - reg:
>  	Usage: required
> @@ -74,20 +76,14 @@ to specify in a pin configuration subnode:
>  			gpio1-gpio40 for pm8058
>  			gpio1-gpio38 for pm8917
>  			gpio1-gpio44 for pm8921
> +			gpio1-gpio36 for pm8941
> +			gpio1-gpio22 for pma8084
>  
>  - function:
> -	Usage: optional
> +	Usage: mandatory
>  	Value type: <string>
>  	Definition: Specify the alternative function to be configured for the
> -		    specified pins.  Valid values are:
> -			"normal",
> -			"paired",
> -			"func1",
> -			"func2",
> -			"dtest1",
> -			"dtest2",
> -			"dtest3",
> -			"dtest4"
> +		    specified pins.  Valid values is: "gpio"
>  
>  - bias-disable:
>  	Usage: optional
> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>  	Value type: <none>
>  	Definition: The specified pins should be configued as pull down.
>  
> -- bias-pull-up:
> -	Usage: optional
> -	Value type: <u32> (optional)
> -	Definition: The specified pins should be configued as pull up. An
> -		    optional argument can be used to configure the strength.
> -		    Valid values are; as defined in
> -		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
> -		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
> -		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
> -		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
> -
>  - bias-high-impedance:
>  	Usage: optional
>  	Value type: <none>
> @@ -139,47 +123,37 @@ to specify in a pin configuration subnode:
>  	Definition: Selects the power source for the specified pins. Valid
>  		    power sources are, as defined in
>  		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -			0: bb (PM8XXX_GPIO_VIN_BB)
> +			0: bb (PM8XXX_GPIO_VIN0)
>  				valid for pm8038, pm8058, pm8917, pm8921
> -			1: ldo2 (PM8XXX_GPIO_VIN_L2)
> +			1: ldo2 (PM8XXX_GPIO_VIN1)
>  				valid for pm8018, pm8038, pm8917,pm8921
> -			2: ldo3 (PM8XXX_GPIO_VIN_L3)
> +			2: ldo3 (PM8XXX_GPIO_VIN2)
>  				valid for pm8038, pm8058, pm8917, pm8921
> -			3: ldo4 (PM8XXX_GPIO_VIN_L4)
> +			3: ldo4 (PM8XXX_GPIO_VIN3)
>  				valid for pm8018, pm8917, pm8921
> -			4: ldo5 (PM8XXX_GPIO_VIN_L5)
> +			4: ldo5 (PM8XXX_GPIO_VIN4)
>  				valid for pm8018, pm8058
> -			5: ldo6 (PM8XXX_GPIO_VIN_L6)
> +			5: ldo6 (PM8XXX_GPIO_VIN5)
>  				valid for pm8018, pm8058
> -			6: ldo7 (PM8XXX_GPIO_VIN_L7)
> +			6: ldo7 (PM8XXX_GPIO_VIN6)
>  				valid for pm8058
> -			7: ldo8 (PM8XXX_GPIO_VIN_L8)
> +			7: ldo8 (PM8XXX_GPIO_VIN7)
>  				valid for pm8018
> -			8: ldo11 (PM8XXX_GPIO_VIN_L11)
> +			8: ldo11 (PM8XXX_GPIO_VIN8)
>  				valid for pm8038
> -			9: ldo14 (PM8XXX_GPIO_VIN_L14)
> +			9: ldo14 (PM8XXX_GPIO_VIN9)
>  				valid for pm8018
> -			10: ldo15 (PM8XXX_GPIO_VIN_L15)
> +			10: ldo15 (PM8XXX_GPIO_VIN10)
>  				valid for pm8038, pm8917, pm8921
> -			11: ldo17 (PM8XXX_GPIO_VIN_L17)
> +			11: ldo17 (PM8XXX_GPIO_VIN11)
>  				valid for pm8038, pm8917, pm8921
> -			12: smps3 (PM8XXX_GPIO_VIN_S3)
> +			12: smps3 (PM8XXX_GPIO_VIN12)
>  				valid for pm8018, pm8058
> -			13: smps4 (PM8XXX_GPIO_VIN_S4)
> +			13: smps4 (PM8XXX_GPIO_VIN13)
>  				valid for pm8921
> -			14: vph (PM8XXX_GPIO_VIN_VPH)
> +			14: vph (PM8XXX_GPIO_VIN14)
>  				valid for pm8018, pm8038, pm8058, pm8917 pm8921
>  
> -- drive-strength:
> -	Usage: optional
> -	Value type: <u32>
> -	Definition: Selects the drive strength for the specified pins. Value
> -		    drive strengths are:
> -			0: no	(PM8XXX_GPIO_STRENGTH_NO)
> -			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
> -			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
> -			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
> -
>  - drive-push-pull:
>  	Usage: optional
>  	Value type: <none>
> @@ -190,6 +164,28 @@ to specify in a pin configuration subnode:
>  	Value type: <none>
>  	Definition: The specified pins are configured in open-drain mode.
>  
> +- qcom,pull-up:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: The specified pins should be configued as pull up. An
> +		    optional argument can be used to configure the strength.
> +		    Valid values are as defined in
> +		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
> +		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
> +		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
> +		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
> +
> +- qcom,strength:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: Selects the drive strength for the specified pins.
> +		    Valid values are as defined in
> +		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +			0: no	(PM8XXX_GPIO_STRENGTH_NO)
> +			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
> +			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
> +			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
>  
>  Example:
>  
> @@ -218,13 +214,14 @@ Example:
>  		pm8921_gpio_keys: gpio-keys {
>  			volume-keys {
>  				pins = "gpio20", "gpio21";
> -				function = "normal";
> +				function = "gpio";
>  
>  				input-enable;
>  				bias-pull-up;
>  				drive-push-pull;
> -				drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
> -				power-source = <PM8XXX_GPIO_VIN_S4>;
> +
> +				power-source = <PM8XXX_GPIO_VIN13>;
> +				qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
>  			};
>  		};
>  	};
> diff --git a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> index 5aaf914..68feb2f 100644
> --- a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> +++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> @@ -95,9 +95,7 @@ static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
>  };
>  
>  static const char * const pm8xxx_gpio_functions[] = {
> -	"normal", "paired",
> -	"func1", "func2",
> -	"dtest1", "dtest2", "dtest3", "dtest4",
> +	"gpio",
>  };
>  
>  static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
> @@ -622,9 +620,9 @@ static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
>  static const struct pm8xxx_gpio_data pm8018_gpio_data = {
>  	.ngpio = 6,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L14, PM8XXX_GPIO_VIN_S3,
> -		PM8XXX_GPIO_VIN_L6, PM8XXX_GPIO_VIN_L2, PM8XXX_GPIO_VIN_L5,
> -		PM8XXX_GPIO_VIN_L8, PM8XXX_GPIO_VIN_VPH
> +		PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN9, PM8XXX_GPIO_VIN12,
> +		PM8XXX_GPIO_VIN5, PM8XXX_GPIO_VIN1, PM8XXX_GPIO_VIN4,
> +		PM8XXX_GPIO_VIN7, PM8XXX_GPIO_VIN14
>  	},
>  	.npower_sources = 8,
>  };
> @@ -632,9 +630,9 @@ static const struct pm8xxx_gpio_data pm8018_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8038_gpio_data = {
>  	.ngpio = 12,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_L11,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN8,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> @@ -642,18 +640,18 @@ static const struct pm8xxx_gpio_data pm8038_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8058_gpio_data = {
>  	.ngpio = 40,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S3,
> -		PM8XXX_GPIO_VIN_L3, PM8XXX_GPIO_VIN_L7, PM8XXX_GPIO_VIN_L6,
> -		PM8XXX_GPIO_VIN_L5, PM8XXX_GPIO_VIN_L2
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN12,
> +		PM8XXX_GPIO_VIN2, PM8XXX_GPIO_VIN6, PM8XXX_GPIO_VIN5,
> +		PM8XXX_GPIO_VIN4, PM8XXX_GPIO_VIN1
>  	},
>  	.npower_sources = 8,
>  };
>  static const struct pm8xxx_gpio_data pm8917_gpio_data = {
>  	.ngpio = 38,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> @@ -661,9 +659,9 @@ static const struct pm8xxx_gpio_data pm8917_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8921_gpio_data = {
>  	.ngpio = 44,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> index 6b66fff..564fd05 100644
> --- a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> +++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> @@ -5,27 +5,30 @@
>  #ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
>  #define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
>  
> +/* To be used with "qcom,pull-up = <>" */
>  #define PM8XXX_GPIO_PULL_UP_30		1
>  #define PM8XXX_GPIO_PULL_UP_1P5		2
>  #define PM8XXX_GPIO_PULL_UP_31P5	3
>  #define PM8XXX_GPIO_PULL_UP_1P5_30	4
>  
> -#define PM8XXX_GPIO_VIN_BB		0
> -#define PM8XXX_GPIO_VIN_L2		1
> -#define PM8XXX_GPIO_VIN_L3		2
> -#define PM8XXX_GPIO_VIN_L4		3
> -#define PM8XXX_GPIO_VIN_L5		4
> -#define PM8XXX_GPIO_VIN_L6		5
> -#define PM8XXX_GPIO_VIN_L7		6
> -#define PM8XXX_GPIO_VIN_L8		7
> -#define PM8XXX_GPIO_VIN_L11		8
> -#define PM8XXX_GPIO_VIN_L14		9
> -#define PM8XXX_GPIO_VIN_L15		10
> -#define PM8XXX_GPIO_VIN_L17		11
> -#define PM8XXX_GPIO_VIN_S3		12
> -#define PM8XXX_GPIO_VIN_S4		13
> -#define PM8XXX_GPIO_VIN_VPH		14
> +/* power-source */
> +#define PM8XXX_GPIO_VIN0		0
> +#define PM8XXX_GPIO_VIN1		1
> +#define PM8XXX_GPIO_VIN2		2
> +#define PM8XXX_GPIO_VIN3		3
> +#define PM8XXX_GPIO_VIN4		4
> +#define PM8XXX_GPIO_VIN5		5
> +#define PM8XXX_GPIO_VIN6		6
> +#define PM8XXX_GPIO_VIN7		7
> +#define PM8XXX_GPIO_VIN8		8
> +#define PM8XXX_GPIO_VIN9		9
> +#define PM8XXX_GPIO_VIN10		10
> +#define PM8XXX_GPIO_VIN11		11
> +#define PM8XXX_GPIO_VIN12		12
> +#define PM8XXX_GPIO_VIN13		13
> +#define PM8XXX_GPIO_VIN14		14
>  
> +/* To be used with "qcom,strength = <>" */
>  #define	PM8XXX_GPIO_STRENGTH_NO		0
>  #define	PM8XXX_GPIO_STRENGTH_HIGH	1
>  #define	PM8XXX_GPIO_STRENGTH_MED	2


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

Bjorn Andersson July 22, 2014, 9:46 p.m. UTC | #2

On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <[e-mail]> wrote:
> From: "Ivan T. Ivanov" <[e-mail]>
>

Hi Ivan,

Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
out some resonable answers to you.

> Available 'power-source' labels differ between chips.
> Use just VIN0-VIN14 in the input source names.
>

The documentation uses VIN0-VIN7 to define the actual register value 0-7. As
with most other things in DT we don't want to encode the actual bits that
should go in the register, but rather give them some human readable name. This
is why I have the mapping tables in my proposal.

Inventing some magic mapping where you're supposed to know that you should type
VIN14 when you mean VPH on pm8921 but VIN0 on pm8941 doesn't make sense.

So, either we put the actual register values in the binding, or we use the enum
(as I proposed) to encode human readable names.

For pm8941 the valid power supply values are:
 GPIO 1-14
   0: VPH
   2: SMPS3
   3: LDO6

 GPIO 15-18
  2: SMPS3
  3: LDO6

 GPIO 19-36
  0: VPH
  1: VDD_TORCH
  2: SPMS3
  3: LDO6

 MPP 1-8
  0: VPH
  1: LDO1
  2: SPMS3
  3: LDO6

For pma8084 the valid power supply values are:
 GPIO 1-22
  0: VPH
  2: SPMS4
  3: LDO6

 MPP 1-8
  0: VPH
  1: LDO1
  2: SMPS4
  3: LDO6

Please add these constants to the table of valid power-source values and use
something like I did to translate them to register values - it makes the DT
much more readable.

> PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> support only one function 'gpio'. Currently GPIO's will
> support only 'normal' mode. Rest of the modes will be added
> later, if needed.
>

This is not true.

As I said before, there is no such thing as "pin controller hardware"; both on
pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
MPP. And if you look in your pinconf_set function you will see that they are
very different.

I'm still trying to figure out the correct pinmux mapping for the various
pmics, but the current indication is a list that looks like this:
  "gpio"
  "paired"
  "ext_reg_en"
  "ext_smps_en"
  "fclk"
  "kypd_drv"
  "kypd_sns"
  "lpa"
  "lpg"
  "mp3_clk"
  "sleep_clk"
  "uart"
  "uim"
  "upl"

I haven't looked through the dts files for 8974 and 8084, but it's not possible
to describe the previous Qualcomm reference formfactor devices (MTP) with only
"gpio".

> We can not use generic drive-strength because Qualcomm hardware
> define those values as low, medium and high. Use qcom,strength
> for this.
>
> We can not use generic bias-pull-up because Qualcomm hardware
> define those values in uA's. Use qcom,pull-up for this.
>

I'm not to fond of the lack of symetry we introduce by having "bias-disable",
"bias-pull-down" and "qcom,pull-up". Furhter more, at least for 8x60, 8960 and
8064 almost all pins are configured with 30uA.

So my suggestion is that we keep the symmetry by continue to select the pull up
mode by the use of "bias-pull-up" and then we introduce a property named
"qcom,pull-up-strength" that optionally can be used to select a different
strength than the 30uA.

[...]
>  - function:
> -       Usage: optional
> +       Usage: mandatory

Usage: required

>         Value type: <string>
>         Definition: Specify the alternative function to be configured for the
> -                   specified pins.  Valid values are:
> -                       "normal",
> -                       "paired",
> -                       "func1",
> -                       "func2",
> -                       "dtest1",
> -                       "dtest2",
> -                       "dtest3",
> -                       "dtest4"
> +                   specified pins.  Valid values is: "gpio"
>
>  - bias-disable:
>         Usage: optional
> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>         Value type: <none>
>         Definition: The specified pins should be configued as pull down.
>
> -- bias-pull-up:
> -       Usage: optional
> -       Value type: <u32> (optional)
> -       Definition: The specified pins should be configued as pull up. An
> -                   optional argument can be used to configure the strength.
> -                   Valid values are; as defined in
> -                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
> -                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
> -                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
> -                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> -

As described above, I belive we should make this:

- bias-pull-up:
	Usage: optional
	Value type: <empty>
	Definition: The specified pins should be configured as pull up.

- qcom,pull-up-strength:
	Usage: optional
	Value type: <u32>
	Definition: Specifies the strength to use for pull up, if selected.
                    Valid values are; as defined in
                    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
                    1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
                    2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
                    3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
                    4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
		    If this property is ommited 30uA strength will be used if
		    pull up is selected.

>  - bias-high-impedance:
>         Usage: optional
>         Value type: <none>
> @@ -139,47 +123,37 @@ to specify in a pin configuration subnode:
>         Definition: Selects the power source for the specified pins. Valid
>                     power sources are, as defined in
>                     <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -                       0: bb (PM8XXX_GPIO_VIN_BB)
> +                       0: bb (PM8XXX_GPIO_VIN0)
>                                 valid for pm8038, pm8058, pm8917, pm8921
> -                       1: ldo2 (PM8XXX_GPIO_VIN_L2)
> +                       1: ldo2 (PM8XXX_GPIO_VIN1)
>                                 valid for pm8018, pm8038, pm8917,pm8921
> -                       2: ldo3 (PM8XXX_GPIO_VIN_L3)
> +                       2: ldo3 (PM8XXX_GPIO_VIN2)
>                                 valid for pm8038, pm8058, pm8917, pm8921
> -                       3: ldo4 (PM8XXX_GPIO_VIN_L4)
> +                       3: ldo4 (PM8XXX_GPIO_VIN3)
>                                 valid for pm8018, pm8917, pm8921
> -                       4: ldo5 (PM8XXX_GPIO_VIN_L5)
> +                       4: ldo5 (PM8XXX_GPIO_VIN4)
>                                 valid for pm8018, pm8058
> -                       5: ldo6 (PM8XXX_GPIO_VIN_L6)
> +                       5: ldo6 (PM8XXX_GPIO_VIN5)
>                                 valid for pm8018, pm8058
> -                       6: ldo7 (PM8XXX_GPIO_VIN_L7)
> +                       6: ldo7 (PM8XXX_GPIO_VIN6)
>                                 valid for pm8058
> -                       7: ldo8 (PM8XXX_GPIO_VIN_L8)
> +                       7: ldo8 (PM8XXX_GPIO_VIN7)
>                                 valid for pm8018
> -                       8: ldo11 (PM8XXX_GPIO_VIN_L11)
> +                       8: ldo11 (PM8XXX_GPIO_VIN8)
>                                 valid for pm8038
> -                       9: ldo14 (PM8XXX_GPIO_VIN_L14)
> +                       9: ldo14 (PM8XXX_GPIO_VIN9)
>                                 valid for pm8018
> -                       10: ldo15 (PM8XXX_GPIO_VIN_L15)
> +                       10: ldo15 (PM8XXX_GPIO_VIN10)
>                                 valid for pm8038, pm8917, pm8921
> -                       11: ldo17 (PM8XXX_GPIO_VIN_L17)
> +                       11: ldo17 (PM8XXX_GPIO_VIN11)
>                                 valid for pm8038, pm8917, pm8921
> -                       12: smps3 (PM8XXX_GPIO_VIN_S3)
> +                       12: smps3 (PM8XXX_GPIO_VIN12)
>                                 valid for pm8018, pm8058
> -                       13: smps4 (PM8XXX_GPIO_VIN_S4)
> +                       13: smps4 (PM8XXX_GPIO_VIN13)
>                                 valid for pm8921
> -                       14: vph (PM8XXX_GPIO_VIN_VPH)
> +                       14: vph (PM8XXX_GPIO_VIN14)
>                                 valid for pm8018, pm8038, pm8058, pm8917 pm8921

As described this doesn't make sense, please add the necessary enumeration for
your pins or make an argument for just using register values directly here.

If we choose to go with register values directly in the dt binding we should
document the valid values and their mapping/meaning here.

>
> -- drive-strength:
> -       Usage: optional
> -       Value type: <u32>
> -       Definition: Selects the drive strength for the specified pins. Value
> -                   drive strengths are:
> -                       0: no   (PM8XXX_GPIO_STRENGTH_NO)
> -                       1: high (PM8XXX_GPIO_STRENGTH_HIGH)
> -                       2: medium       (PM8XXX_GPIO_STRENGTH_MED)
> -                       3: low  (PM8XXX_GPIO_STRENGTH_LOW)
> -
>  - drive-push-pull:
>         Usage: optional
>         Value type: <none>
> @@ -190,6 +164,28 @@ to specify in a pin configuration subnode:
>         Value type: <none>
>         Definition: The specified pins are configured in open-drain mode.
>
> +- qcom,pull-up:
> +       Usage: optional
> +       Value type: <u32>
> +       Definition: The specified pins should be configued as pull up. An
> +                   optional argument can be used to configure the strength.
> +                   Valid values are as defined in
> +                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
> +                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
> +                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
> +                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> +
> +- qcom,strength:

Better follow the standard naming and use "qcom,drive-strength" to actually
specify what strength we're talking about.

> +       Usage: optional
> +       Value type: <u32>
> +       Definition: Selects the drive strength for the specified pins.
> +                   Valid values are as defined in
> +                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +                       0: no   (PM8XXX_GPIO_STRENGTH_NO)
> +                       1: high (PM8XXX_GPIO_STRENGTH_HIGH)
To be clearer what these actually means we could probably add the following:
				0.9mA @ 1.8V - 1.9mA @ 2.6V
> +                       2: medium       (PM8XXX_GPIO_STRENGTH_MED)
				0.6mA @ 1.8V - 1.25mA @ 2.6V
> +                       3: low  (PM8XXX_GPIO_STRENGTH_LOW)
				0.15mA @ 1.8V - 0.3mA @ 2.6V
>
>  Example:
>
> @@ -218,13 +214,14 @@ Example:
>                 pm8921_gpio_keys: gpio-keys {
>                         volume-keys {
>                                 pins = "gpio20", "gpio21";
> -                               function = "normal";
> +                               function = "gpio";

Sounds reasonable.

>
>                                 input-enable;
>                                 bias-pull-up;
>                                 drive-push-pull;
> -                               drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
> -                               power-source = <PM8XXX_GPIO_VIN_S4>;
> +
> +                               power-source = <PM8XXX_GPIO_VIN13>;

Here you can see why VIN13 doesn't make any sense; anyone writing a dts for
this would expect this to either be <VIN_S4> or <2>.

> +                               qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
>                         };
>                 };
>         };

Please let me know what you think and I can send out an updated version of my
binding documentation.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov July 23, 2014, 12:47 p.m. UTC | #3

On Tue, 2014-07-22 at 14:46 -0700, Bjorn Andersson wrote:
> On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <[e-mail]> wrote:
> > From: "Ivan T. Ivanov" <[e-mail]>
> >
> 
> Hi Ivan,
> 
> Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
> out some resonable answers to you.
> 

<snip>

> 
> > PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> > support only one function 'gpio'. Currently GPIO's will
> > support only 'normal' mode. Rest of the modes will be added
> > later, if needed.
> >
> 
> This is not true.
> 
> As I said before, there is no such thing as "pin controller hardware"; both on
> pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
> MPP. And if you look in your pinconf_set function you will see that they are
> very different.
> 
> I'm still trying to figure out the correct pinmux mapping for the various
> pmics, but the current indication is a list that looks like this:
>   "gpio"
>   "paired"
>   "ext_reg_en"
>   "ext_smps_en"
>   "fclk"
>   "kypd_drv"
>   "kypd_sns"
>   "lpa"
>   "lpg"
>   "mp3_clk"
>   "sleep_clk"
>   "uart"
>   "uim"
>   "upl"
> 
> I haven't looked through the dts files for 8974 and 8084, but it's not possible
> to describe the previous Qualcomm reference formfactor devices (MTP) with only
> "gpio".


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

Ivan T. Ivanov July 23, 2014, 4:05 p.m. UTC | #4

Hi,  

I have accidentally pressed send in the earlier message.

On Wed, 2014-07-23 at 15:47 +0300, Ivan T. Ivanov wrote:
> On Tue, 2014-07-22 at 14:46 -0700, Bjorn Andersson wrote:
> > On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <[e-mail]> wrote:
> > > From: "Ivan T. Ivanov" <[e-mail]>
> > >
> > 
> > Hi Ivan,
> > 
> > Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
> > out some resonable answers to you.
> > 
> 
> <snip>
> 
> > 
> > > PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> > > support only one function 'gpio'. Currently GPIO's will
> > > support only 'normal' mode. Rest of the modes will be added
> > > later, if needed.
> > >
> > 
> > This is not true.
> > 
> > As I said before, there is no such thing as "pin controller hardware"; 

This is matter of interpretation :-)

> both on
> > pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
> > MPP. And if you look in your pinconf_set function you will see that they are
> > very different.

I bet that the hardware blocks are almost identical, just register
map is different.

> > 
> > I'm still trying to figure out the correct pinmux mapping for the various
> > pmics, but the current indication is a list that looks like this:
> >   "gpio"
> >   "paired"
> >   "ext_reg_en"
> >   "ext_smps_en"
> >   "fclk"
> >   "kypd_drv"
> >   "kypd_sns"
> >   "lpa"
> >   "lpg"
> >   "mp3_clk"
> >   "sleep_clk"
> >   "uart"
> >   "uim"
> >   "upl"
> > 

Ok, I see. But lets make things simpler. 

It seems that "uim" could be "SDC_UIM_VBIAS" on DB8074, which looks like MPP in
analog-output mode/function, with additional selection for voltage level (qcom,aout?)

Closest to "uart" function, that I have found, is that one of the SPI buses on the
above board is level translated trough several MPP's, but this still did not make them
act like a UART.

However, I seems that some of the GPIO's could act like clock and PWM sources. 

> > I haven't looked through the dts files for 8974 and 8084, but it's not possible
> > to describe the previous Qualcomm reference formfactor devices (MTP) with only
> > "gpio".

I believe that these DTS files are using qcom,mode(DIG_IN|DIG_OUT|AIN|AOUT..) and 
qcom,src-sel to select desired function. For example:

apq8074-dragonboard:
/* GPIO 1 */
qcom,mode = <0>
qcom,src-sel = <0>

apq8084-cdp:
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <2>;             /* Special Function 1=LPG 3 */

apq8084-mtp:
/* NFC clk request */
qcom,mode = <0>;                /* QPNP_PIN_MODE_DIG_IN */
qcom,src-sel = <2>;             /* QPNP_PIN_SEL_FUNC_1 */

apq8084-sbc:
/* BACKLIGHT2_PWM */
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <2>;             /* Special Function 1=LPG 5 */

apq8084-sbc:
/* DIV_CLK3 SLEEP_CLK */
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <3>;             /* Function 2 */

...

I could just guess what the functions 1 and 2 means.

In which case GPIO functions could be gpio, keypad, clk, pwm/lpg, ...

Rest of the pinctrl parameters prosed looks fine. 

Thanks,
Ivan


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

Stephen Boyd July 23, 2014, 9:46 p.m. UTC | #5

On 07/23/14 09:05, Ivan T. Ivanov wrote:
>
>> both on
>>> pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
>>> MPP. And if you look in your pinconf_set function you will see that they are
>>> very different.
> I bet that the hardware blocks are almost identical, just register
> map is different.

The hardware is different and not "almost identical". If it was the same
then we would have called them all GPIOs or MPPs and not had a distinction.

Stephen Boyd July 23, 2014, 11:47 p.m. UTC | #6

On 07/22/14 14:46, Bjorn Andersson wrote:
> For pm8941 the valid power supply values are:
>  GPIO 1-14
>    0: VPH
>    2: SMPS3
>    3: LDO6
>
>  GPIO 15-18
>   2: SMPS3
>   3: LDO6
>
>  GPIO 19-36
>   0: VPH
>   1: VDD_TORCH
>   2: SPMS3
>   3: LDO6
>
>  MPP 1-8
>   0: VPH
>   1: LDO1
>   2: SPMS3
>   3: LDO6
>
> For pma8084 the valid power supply values are:
>  GPIO 1-22
>   0: VPH
>   2: SPMS4
>   3: LDO6
>
>  MPP 1-8
>   0: VPH
>   1: LDO1
>   2: SMPS4
>   3: LDO6
>
> Please add these constants to the table of valid power-source values and use
> something like I did to translate them to register values - it makes the DT
> much more readable.

The DT could be similarly readable if we had a bunch of #defines for the
different VIN settings that resolved to the final register value for
that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
etc. There would be a lot of them, but then the driver could be really
simple and just jam whatever value is in the DT into the register
without having to bounce through a mapping table in software to figure
out the register value. If we did this for the functions also then I
believe we achieve readability without requiring a bunch of drivers for
each and every single pmic?

>
>>         Value type: <string>
>>         Definition: Specify the alternative function to be configured for the
>> -                   specified pins.  Valid values are:
>> -                       "normal",
>> -                       "paired",
>> -                       "func1",
>> -                       "func2",
>> -                       "dtest1",
>> -                       "dtest2",
>> -                       "dtest3",
>> -                       "dtest4"
>> +                   specified pins.  Valid values is: "gpio"
>>
>>  - bias-disable:
>>         Usage: optional
>> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>>         Value type: <none>
>>         Definition: The specified pins should be configued as pull down.
>>
>> -- bias-pull-up:
>> -       Usage: optional
>> -       Value type: <u32> (optional)
>> -       Definition: The specified pins should be configued as pull up. An
>> -                   optional argument can be used to configure the strength.
>> -                   Valid values are; as defined in
>> -                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
>> -                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
>> -                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
>> -                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
>> -                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
>> -
> As described above, I belive we should make this:
>
> - bias-pull-up:
> 	Usage: optional
> 	Value type: <empty>
> 	Definition: The specified pins should be configured as pull up.
>
> - qcom,pull-up-strength:
> 	Usage: optional
> 	Value type: <u32>
> 	Definition: Specifies the strength to use for pull up, if selected.
>                     Valid values are; as defined in
>                     <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
>                     1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
>                     2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
>                     3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
>                     4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> 		    If this property is ommited 30uA strength will be used if
> 		    pull up is selected.

Why is 30uA special? Just because most drivers use it? I'd prefer we
always be explicit about which pull-up we want so that nothing is left
up to the driver implementation.

Also according to the hw folks the 1.5uA + 30uA boost has never been
used so I say let's drop that feature. If we need it one day we can
always add a qcom,pull-up-boost or something (I highly doubt we'll need
it). Doing that allows us to specify this in actual SI units. Maybe even
allowing us to have a generic pull-up-strength (or
bias-pull-up-strength) specified in SI units of mA?

Linus Walleij July 24, 2014, 3:40 p.m. UTC | #7

On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:

>> Please add these constants to the table of valid power-source values and use
>> something like I did to translate them to register values - it makes the DT
>> much more readable.
>
> The DT could be similarly readable if we had a bunch of #defines for the
> different VIN settings that resolved to the final register value for
> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> etc. There would be a lot of them, but then the driver could be really
> simple and just jam whatever value is in the DT into the register
> without having to bounce through a mapping table in software to figure
> out the register value. If we did this for the functions also then I
> believe we achieve readability without requiring a bunch of drivers for
> each and every single pmic?

Not sure but it sounds like you want to make the device tree a jam table,
(know about individual register offsets, sequences etc). That has been
throrougly NACKed in the past, because DT is not Open Firmware.

The exception is pinctrl-single which is restricted to single register
per pin use cases and is still a point of contention...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd July 25, 2014, 12:23 a.m. UTC | #8

On 07/24/14 08:40, Linus Walleij wrote:
> On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:
>
>>> Please add these constants to the table of valid power-source values and use
>>> something like I did to translate them to register values - it makes the DT
>>> much more readable.
>> The DT could be similarly readable if we had a bunch of #defines for the
>> different VIN settings that resolved to the final register value for
>> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>> etc. There would be a lot of them, but then the driver could be really
>> simple and just jam whatever value is in the DT into the register
>> without having to bounce through a mapping table in software to figure
>> out the register value. If we did this for the functions also then I
>> believe we achieve readability without requiring a bunch of drivers for
>> each and every single pmic?
> Not sure but it sounds like you want to make the device tree a jam table,
> (know about individual register offsets, sequences etc). That has been
> throrougly NACKed in the past, because DT is not Open Firmware.
>
> The exception is pinctrl-single which is restricted to single register
> per pin use cases and is still a point of contention...
>

I'm not proposing a jam table. I'm proposing that we make the
function/source property convenient to the driver by having the actual
function field value encoded there instead of some string that has to be
translated through a table in a driver. There's still going to be
shifting and masking of bits in the driver to put the value in the right
place in the register, but we avoid needing N number of drivers for each
pmic just to translate strings into integers (for functions) and
integers into other integers (for the power source). From what I can
tell there isn't any benefit to having the function property be a string
vs. a #define number besides having a human readable string in pinctrl
debugfs. Is there some other benefit?

Linus Walleij July 25, 2014, 11:29 a.m. UTC | #9

On Fri, Jul 25, 2014 at 2:23 AM, Stephen Boyd <[e-mail]> wrote:
> On 07/24/14 08:40, Linus Walleij wrote:
>> On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:
>>
>>>> Please add these constants to the table of valid power-source values and use
>>>> something like I did to translate them to register values - it makes the DT
>>>> much more readable.
>>> The DT could be similarly readable if we had a bunch of #defines for the
>>> different VIN settings that resolved to the final register value for
>>> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>>> etc. There would be a lot of them, but then the driver could be really
>>> simple and just jam whatever value is in the DT into the register
>>> without having to bounce through a mapping table in software to figure
>>> out the register value. If we did this for the functions also then I
>>> believe we achieve readability without requiring a bunch of drivers for
>>> each and every single pmic?
>> Not sure but it sounds like you want to make the device tree a jam table,
>> (know about individual register offsets, sequences etc). That has been
>> throrougly NACKed in the past, because DT is not Open Firmware.
>>
>> The exception is pinctrl-single which is restricted to single register
>> per pin use cases and is still a point of contention...
>>
>
> I'm not proposing a jam table. I'm proposing that we make the
> function/source property convenient to the driver by having the actual
> function field value encoded there instead of some string that has to be
> translated through a table in a driver. There's still going to be
> shifting and masking of bits in the driver to put the value in the right
> place in the register, but we avoid needing N number of drivers for each
> pmic just to translate strings into integers (for functions) and
> integers into other integers (for the power source). From what I can
> tell there isn't any benefit to having the function property be a string
> vs. a #define number besides having a human readable string in pinctrl
> debugfs. Is there some other benefit?

OK I get it ... I think.

One good reason to use strings is that it apart from debugfs makes
for quite readable debug messages if used the right way.

I feel sort of lukewarm on the issue, so I'd let the driver author
decide the most elegant way to deal with this from an end-user
point of view. If it helps people configure and debug their board
set-ups is a crucial factor to me, and I suspect both Ivan and
Björn has some experience with this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivan T. Ivanov July 25, 2014, 3:15 p.m. UTC | #10

On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
> On 07/24/14 08:40, Linus Walleij wrote:
> > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:
> >
> >>> Please add these constants to the table of valid power-source values and use
> >>> something like I did to translate them to register values - it makes the DT
> >>> much more readable.
> >> The DT could be similarly readable if we had a bunch of #defines for the
> >> different VIN settings that resolved to the final register value for
> >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> >> etc. There would be a lot of them, but then the driver could be really
> >> simple and just jam whatever value is in the DT into the register
> >> without having to bounce through a mapping table in software to figure
> >> out the register value. If we did this for the functions also then I
> >> believe we achieve readability without requiring a bunch of drivers for
> >> each and every single pmic?
> > Not sure but it sounds like you want to make the device tree a jam table,
> > (know about individual register offsets, sequences etc). That has been
> > throrougly NACKed in the past, because DT is not Open Firmware.
> >
> > The exception is pinctrl-single which is restricted to single register
> > per pin use cases and is still a point of contention...
> >
> 
> I'm not proposing a jam table. I'm proposing that we make the
> function/source property convenient to the driver by having the actual
> function field value encoded there instead of some string that has to be
> translated through a table in a driver. There's still going to be
> shifting and masking of bits in the driver to put the value in the right
> place in the register, but we avoid needing N number of drivers for each
> pmic just to translate strings into integers (for functions) and
> integers into other integers (for the power source). 

This sounds good to me. Drawback is that we will have custom 
function parsing code, but I will try and see what that looks like.

Thanks,
Ivan


> From what I can
> tell there isn't any benefit to having the function property be a string
> vs. a #define number besides having a human readable string in pinctrl
> debugfs. Is there some other benefit?
> 


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

Ivan T. Ivanov Aug. 6, 2014, 3:02 p.m. UTC | #11

On Fri, 2014-07-25 at 18:15 +0300, Ivan T. Ivanov wrote:
> On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
> > On 07/24/14 08:40, Linus Walleij wrote:
> > > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:
> > >
> > >>> Please add these constants to the table of valid power-source values and use
> > >>> something like I did to translate them to register values - it makes the DT
> > >>> much more readable.
> > >> The DT could be similarly readable if we had a bunch of #defines for the
> > >> different VIN settings that resolved to the final register value for
> > >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> > >> etc. There would be a lot of them, but then the driver could be really
> > >> simple and just jam whatever value is in the DT into the register
> > >> without having to bounce through a mapping table in software to figure
> > >> out the register value. 

This is ok.

> If we did this for the functions also then I
> > >> believe we achieve readability without requiring a bunch of drivers for
> > >> each and every single pmic?


> > > Not sure but it sounds like you want to make the device tree a jam table,
> > > (know about individual register offsets, sequences etc). That has been
> > > throrougly NACKed in the past, because DT is not Open Firmware.
> > >
> > > The exception is pinctrl-single which is restricted to single register
> > > per pin use cases and is still a point of contention...
> > >
> > 
> > I'm not proposing a jam table. I'm proposing that we make the
> > function/source property convenient to the driver by having the actual
> > function field value encoded there instead of some string that has to be
> > translated through a table in a driver. There's still going to be
> > shifting and masking of bits in the driver to put the value in the right
> > place in the register, but we avoid needing N number of drivers for each
> > pmic just to translate strings into integers (for functions) and
> > integers into other integers (for the power source). 
> 
> This sounds good to me. Drawback is that we will have custom 
> function parsing code, but I will try and see what that looks like.

Hm, I played with this, using numbers for functions. Unfortunately it is
not easy possible to hack existing DT pin-control parsing code to use
numbers instead strings, so I ended coping Samsung parsing code, but unless
this is separated in library we will end with huge code duplication for
little benefit.

Furthermore meaning of number 2 and 3, which represent PMIC GPIO Special
Function 1 and 2 are not consistent across PMIC chips. For example KEYPD
function in PM8038 is encoded with 3, but in PM8058 it is 2.

I tend to agree with Bjorn that "function" property should be "normal", 
"paired", "func1", "func2","dtest1", "dtest2", "dtest3", "dtest4" and we
can add new property "qcom,mode" which will select between digital/analog
input/output.

In DT include file we can still have something like this:

/* To be used with "function = <>" */
#define QCOM_GPIO_FUNC_NORMAL		"normal"
#define QCOM_GPIO_FUNC_PAIRED		"paired"
#define QCOM_GPIO_FUNC_FUNC1		"func1"
#define QCOM_GPIO_FUNC_FUNC2		"func2"
...

#define PM8038_GPIO1_2_LPG_DRV		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO9_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_12_KYPD_DRV	QCOM_GPIO_FUNC_FUNC2

#define PM8058_GPIO7_8_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO7_8_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO9_26_KYPD_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO24_26_LPG_DRV	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO33_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO34_35_MP3_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO36_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UPL_OUT		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UART_M_RX		QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO38_39_CLK_32KHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO39_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO40_EXT_BB_EN		QCOM_GPIO_FUNC_FUNC1

#define PM8917_GPIO9_18_KEYP_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO20_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8917_GPIO25_26_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_MP3_CLK	QCOM_GPIO_FUNC_FUNC2
...


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

Bjorn Andersson Aug. 11, 2014, 5:40 a.m. UTC | #12

On Wed, Aug 6, 2014 at 8:02 AM, Ivan T. Ivanov <[e-mail]> wrote:
> On Fri, 2014-07-25 at 18:15 +0300, Ivan T. Ivanov wrote:
>> On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
>> > On 07/24/14 08:40, Linus Walleij wrote:
>> > > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <[e-mail]> wrote:
>> > >
>> > >>> Please add these constants to the table of valid power-source values and use
>> > >>> something like I did to translate them to register values - it makes the DT
>> > >>> much more readable.
>> > >> The DT could be similarly readable if we had a bunch of #defines for the
>> > >> different VIN settings that resolved to the final register value for
>> > >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>> > >> etc. There would be a lot of them, but then the driver could be really
>> > >> simple and just jam whatever value is in the DT into the register
>> > >> without having to bounce through a mapping table in software to figure
>> > >> out the register value.
>
> This is ok.
>

I'm not sure I think the "optimization" is worth the cluttered names
of these defines, but I will give it a spin and see how it looks!

>> If we did this for the functions also then I
>> > >> believe we achieve readability without requiring a bunch of drivers for
>> > >> each and every single pmic?
>

Either we have a table like the other pinctrl drivers, or we just go
with custom parsing where we shove the register value straight into
devicetree. Although the latter would reduce the number of mapping
tables we need in the kernel, it seems to not follow the general way
of doing things with pinctrl.

[...]
>
> Furthermore meaning of number 2 and 3, which represent PMIC GPIO Special
> Function 1 and 2 are not consistent across PMIC chips. For example KEYPD
> function in PM8038 is encoded with 3, but in PM8058 it is 2.
>
> I tend to agree with Bjorn that "function" property should be "normal",
> "paired", "func1", "func2","dtest1", "dtest2", "dtest3", "dtest4" and we
> can add new property "qcom,mode" which will select between digital/analog
> input/output.
>

Note that for GPIOs we have normal/gpio, paried and a set of functions
(if we ignore the dtest ones). And for MPPs we have digital and analog
as paired and unpaired. Input/output should be controlled with the
separate means already available (gpio api, output-{low,high}.

> In DT include file we can still have something like this:
>
> /* To be used with "function = <>" */
> #define QCOM_GPIO_FUNC_NORMAL           "normal"
> #define QCOM_GPIO_FUNC_PAIRED           "paired"
> #define QCOM_GPIO_FUNC_FUNC1            "func1"
> #define QCOM_GPIO_FUNC_FUNC2            "func2"
> ...
>
> #define PM8038_GPIO1_2_LPG_DRV          QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO3_5V_BOOST_EN        QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO4_SSBI_ALT_CLK       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO5_6_EXT_REG_EN       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO10_11_EXT_REG_EN     QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO6_7_CLK              QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO9_BAT_ALRM_OUT       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO6_12_KYPD_DRV        QCOM_GPIO_FUNC_FUNC2
>
> #define PM8058_GPIO7_8_MP3_CLK          QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO7_8_BCLK_19P2MHZ     QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO9_26_KYPD_DRV        QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO21_23_UART_TX        QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO24_26_LPG_DRV        QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO33_BCLK_19P2MHZ      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO34_35_MP3_CLK        QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO36_BCLK_19P2MHZ      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO37_UPL_OUT           QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO37_UART_M_RX         QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO38_XO_SLEEP_CLK      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO38_39_CLK_32KHZ      QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO39_MP3_CLK           QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO40_EXT_BB_EN         QCOM_GPIO_FUNC_FUNC1
>
> #define PM8917_GPIO9_18_KEYP_DRV        QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO20_BAT_ALRM_OUT      QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO21_23_UART_TX        QCOM_GPIO_FUNC_FUNC2
> #define PM8917_GPIO25_26_EXT_REG_EN     QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO37_38_XO_SLEEP_CLK   QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO37_38_MP3_CLK        QCOM_GPIO_FUNC_FUNC2
> ...

If we're going to maintain this "table" in the kernel then I would
greatly prefer that we just stick with the standard way of
representing it in the pinctrl drivers. My concern is still related to
me lacking the appropriate documentation to create these tables.

I introduced the necessary tables for pm8058 and pm8921 and it looks
reasonable. I'll try to finish it up tomorrow and send out a copy for
you to have a look.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
index 0035dd8..f17580a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
@@ -12,6 +12,8 @@  Qualcomm.
 			"qcom,pm8058-gpio"
 			"qcom,pm8917-gpio"
 			"qcom,pm8921-gpio"
+			"qcom,pm8941-gpio"
+			"qcom,pma8084-gpio"
 
 - reg:
 	Usage: required
@@ -74,20 +76,14 @@  to specify in a pin configuration subnode:
 			gpio1-gpio40 for pm8058
 			gpio1-gpio38 for pm8917
 			gpio1-gpio44 for pm8921
+			gpio1-gpio36 for pm8941
+			gpio1-gpio22 for pma8084
 
 - function:
-	Usage: optional
+	Usage: mandatory
 	Value type: <string>
 	Definition: Specify the alternative function to be configured for the
-		    specified pins.  Valid values are:
-			"normal",
-			"paired",
-			"func1",
-			"func2",
-			"dtest1",
-			"dtest2",
-			"dtest3",
-			"dtest4"
+		    specified pins.  Valid values is: "gpio"
 
 - bias-disable:
 	Usage: optional
@@ -99,18 +95,6 @@  to specify in a pin configuration subnode:
 	Value type: <none>
 	Definition: The specified pins should be configued as pull down.
 
-- bias-pull-up:
-	Usage: optional
-	Value type: <u32> (optional)
-	Definition: The specified pins should be configued as pull up. An
-		    optional argument can be used to configure the strength.
-		    Valid values are; as defined in
-		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
-		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
-		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
-		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
-		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
-
 - bias-high-impedance:
 	Usage: optional
 	Value type: <none>
@@ -139,47 +123,37 @@  to specify in a pin configuration subnode:
 	Definition: Selects the power source for the specified pins. Valid
 		    power sources are, as defined in
 		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
-			0: bb (PM8XXX_GPIO_VIN_BB)
+			0: bb (PM8XXX_GPIO_VIN0)
 				valid for pm8038, pm8058, pm8917, pm8921
-			1: ldo2 (PM8XXX_GPIO_VIN_L2)
+			1: ldo2 (PM8XXX_GPIO_VIN1)
 				valid for pm8018, pm8038, pm8917,pm8921
-			2: ldo3 (PM8XXX_GPIO_VIN_L3)
+			2: ldo3 (PM8XXX_GPIO_VIN2)
 				valid for pm8038, pm8058, pm8917, pm8921
-			3: ldo4 (PM8XXX_GPIO_VIN_L4)
+			3: ldo4 (PM8XXX_GPIO_VIN3)
 				valid for pm8018, pm8917, pm8921
-			4: ldo5 (PM8XXX_GPIO_VIN_L5)
+			4: ldo5 (PM8XXX_GPIO_VIN4)
 				valid for pm8018, pm8058
-			5: ldo6 (PM8XXX_GPIO_VIN_L6)
+			5: ldo6 (PM8XXX_GPIO_VIN5)
 				valid for pm8018, pm8058
-			6: ldo7 (PM8XXX_GPIO_VIN_L7)
+			6: ldo7 (PM8XXX_GPIO_VIN6)
 				valid for pm8058
-			7: ldo8 (PM8XXX_GPIO_VIN_L8)
+			7: ldo8 (PM8XXX_GPIO_VIN7)
 				valid for pm8018
-			8: ldo11 (PM8XXX_GPIO_VIN_L11)
+			8: ldo11 (PM8XXX_GPIO_VIN8)
 				valid for pm8038
-			9: ldo14 (PM8XXX_GPIO_VIN_L14)
+			9: ldo14 (PM8XXX_GPIO_VIN9)
 				valid for pm8018
-			10: ldo15 (PM8XXX_GPIO_VIN_L15)
+			10: ldo15 (PM8XXX_GPIO_VIN10)
 				valid for pm8038, pm8917, pm8921
-			11: ldo17 (PM8XXX_GPIO_VIN_L17)
+			11: ldo17 (PM8XXX_GPIO_VIN11)
 				valid for pm8038, pm8917, pm8921
-			12: smps3 (PM8XXX_GPIO_VIN_S3)
+			12: smps3 (PM8XXX_GPIO_VIN12)
 				valid for pm8018, pm8058
-			13: smps4 (PM8XXX_GPIO_VIN_S4)
+			13: smps4 (PM8XXX_GPIO_VIN13)
 				valid for pm8921
-			14: vph (PM8XXX_GPIO_VIN_VPH)
+			14: vph (PM8XXX_GPIO_VIN14)
 				valid for pm8018, pm8038, pm8058, pm8917 pm8921
 
-- drive-strength:
-	Usage: optional
-	Value type: <u32>
-	Definition: Selects the drive strength for the specified pins. Value
-		    drive strengths are:
-			0: no	(PM8XXX_GPIO_STRENGTH_NO)
-			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
-			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
-			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
-
 - drive-push-pull:
 	Usage: optional
 	Value type: <none>
@@ -190,6 +164,28 @@  to specify in a pin configuration subnode:
 	Value type: <none>
 	Definition: The specified pins are configured in open-drain mode.
 
+- qcom,pull-up:
+	Usage: optional
+	Value type: <u32>
+	Definition: The specified pins should be configued as pull up. An
+		    optional argument can be used to configure the strength.
+		    Valid values are as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
+		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
+		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
+		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
+
+- qcom,strength:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the drive strength for the specified pins.
+		    Valid values are as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+			0: no	(PM8XXX_GPIO_STRENGTH_NO)
+			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
+			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
+			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
 
 Example:
 
@@ -218,13 +214,14 @@  Example:
 		pm8921_gpio_keys: gpio-keys {
 			volume-keys {
 				pins = "gpio20", "gpio21";
-				function = "normal";
+				function = "gpio";
 
 				input-enable;
 				bias-pull-up;
 				drive-push-pull;
-				drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
-				power-source = <PM8XXX_GPIO_VIN_S4>;
+
+				power-source = <PM8XXX_GPIO_VIN13>;
+				qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
 			};
 		};
 	};
diff --git a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
index 5aaf914..68feb2f 100644
--- a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
+++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
@@ -95,9 +95,7 @@  static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
 };
 
 static const char * const pm8xxx_gpio_functions[] = {
-	"normal", "paired",
-	"func1", "func2",
-	"dtest1", "dtest2", "dtest3", "dtest4",
+	"gpio",
 };
 
 static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
@@ -622,9 +620,9 @@  static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
 static const struct pm8xxx_gpio_data pm8018_gpio_data = {
 	.ngpio = 6,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L14, PM8XXX_GPIO_VIN_S3,
-		PM8XXX_GPIO_VIN_L6, PM8XXX_GPIO_VIN_L2, PM8XXX_GPIO_VIN_L5,
-		PM8XXX_GPIO_VIN_L8, PM8XXX_GPIO_VIN_VPH
+		PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN9, PM8XXX_GPIO_VIN12,
+		PM8XXX_GPIO_VIN5, PM8XXX_GPIO_VIN1, PM8XXX_GPIO_VIN4,
+		PM8XXX_GPIO_VIN7, PM8XXX_GPIO_VIN14
 	},
 	.npower_sources = 8,
 };
@@ -632,9 +630,9 @@  static const struct pm8xxx_gpio_data pm8018_gpio_data = {
 static const struct pm8xxx_gpio_data pm8038_gpio_data = {
 	.ngpio = 12,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_L11,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN8,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
@@ -642,18 +640,18 @@  static const struct pm8xxx_gpio_data pm8038_gpio_data = {
 static const struct pm8xxx_gpio_data pm8058_gpio_data = {
 	.ngpio = 40,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S3,
-		PM8XXX_GPIO_VIN_L3, PM8XXX_GPIO_VIN_L7, PM8XXX_GPIO_VIN_L6,
-		PM8XXX_GPIO_VIN_L5, PM8XXX_GPIO_VIN_L2
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN12,
+		PM8XXX_GPIO_VIN2, PM8XXX_GPIO_VIN6, PM8XXX_GPIO_VIN5,
+		PM8XXX_GPIO_VIN4, PM8XXX_GPIO_VIN1
 	},
 	.npower_sources = 8,
 };
 static const struct pm8xxx_gpio_data pm8917_gpio_data = {
 	.ngpio = 38,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
@@ -661,9 +659,9 @@  static const struct pm8xxx_gpio_data pm8917_gpio_data = {
 static const struct pm8xxx_gpio_data pm8921_gpio_data = {
 	.ngpio = 44,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
index 6b66fff..564fd05 100644
--- a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
@@ -5,27 +5,30 @@ 
 #ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
 #define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
 
+/* To be used with "qcom,pull-up = <>" */
 #define PM8XXX_GPIO_PULL_UP_30		1
 #define PM8XXX_GPIO_PULL_UP_1P5		2
 #define PM8XXX_GPIO_PULL_UP_31P5	3
 #define PM8XXX_GPIO_PULL_UP_1P5_30	4
 
-#define PM8XXX_GPIO_VIN_BB		0
-#define PM8XXX_GPIO_VIN_L2		1
-#define PM8XXX_GPIO_VIN_L3		2
-#define PM8XXX_GPIO_VIN_L4		3
-#define PM8XXX_GPIO_VIN_L5		4
-#define PM8XXX_GPIO_VIN_L6		5
-#define PM8XXX_GPIO_VIN_L7		6
-#define PM8XXX_GPIO_VIN_L8		7
-#define PM8XXX_GPIO_VIN_L11		8
-#define PM8XXX_GPIO_VIN_L14		9
-#define PM8XXX_GPIO_VIN_L15		10
-#define PM8XXX_GPIO_VIN_L17		11
-#define PM8XXX_GPIO_VIN_S3		12
-#define PM8XXX_GPIO_VIN_S4		13
-#define PM8XXX_GPIO_VIN_VPH		14
+/* power-source */
+#define PM8XXX_GPIO_VIN0		0
+#define PM8XXX_GPIO_VIN1		1
+#define PM8XXX_GPIO_VIN2		2
+#define PM8XXX_GPIO_VIN3		3
+#define PM8XXX_GPIO_VIN4		4
+#define PM8XXX_GPIO_VIN5		5
+#define PM8XXX_GPIO_VIN6		6
+#define PM8XXX_GPIO_VIN7		7
+#define PM8XXX_GPIO_VIN8		8
+#define PM8XXX_GPIO_VIN9		9
+#define PM8XXX_GPIO_VIN10		10
+#define PM8XXX_GPIO_VIN11		11
+#define PM8XXX_GPIO_VIN12		12
+#define PM8XXX_GPIO_VIN13		13
+#define PM8XXX_GPIO_VIN14		14
 
+/* To be used with "qcom,strength = <>" */
 #define	PM8XXX_GPIO_STRENGTH_NO		0
 #define	PM8XXX_GPIO_STRENGTH_HIGH	1
 #define	PM8XXX_GPIO_STRENGTH_MED	2