[PATCH 0/4] One fix and some improvements for RK3288 Popmetal board

[PATCH 0/4] One fix and some improvements for RK3288 Popmetal board

[Andy Yan]

This series fix one sensor i2c device address on Popmetal
board, and change some pinctrl description to make it more
clearly, also enable the usb otg function on this board.


Andy Yan (4):
  ARM: dts: rockchip: enable usbotg for Popemtal-rk3288 board
  ARM: dts: rockchip: fix i2c address L3G4200D on PopMetal-RK3288 board
  include: dt-bindings: Add more GPIO bank and index definition for
    rockchip pinctrl
  ARM: dts: use definition in rockchip pinctrl header to describe gpios
    on Popmetal-RK3288

 arch/arm/boot/dts/rk3288-popmetal.dts  | 36 +++++++++++++++++++++-------------
 include/dt-bindings/pinctrl/rockchip.h | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH 1/4] ARM: dts: rockchip: enable usbotg for Popemtal-rk3288 board

[Andy Yan]

Enable USB OTG port on RK3288 Popmetal board, So we can run
some usb gadget functions like Android adb on this board.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

 arch/arm/boot/dts/rk3288-popmetal.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-popmetal.dts b/arch/arm/boot/dts/rk3288-popmetal.dts
index bd35392..802216f 100644
--- a/arch/arm/boot/dts/rk3288-popmetal.dts
+++ b/arch/arm/boot/dts/rk3288-popmetal.dts
@@ -529,3 +529,7 @@
 &usbphy {
 	status = "okay";
 };
+
+&usb_otg {
+	status = "okay";
+};
-- 
2.7.4

[PATCH 2/4] ARM: dts: rockchip: fix i2c address L3G4200D on PopMetal-RK3288 board

[Andy Yan]

The i2c address of the three-axis digital gyroscope L3G4200D should be
0x69 according to hardware design.
Also add power supply reference for L3G3200D and the 3-axis Electronic
Compass AK8963.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

 arch/arm/boot/dts/rk3288-popmetal.dts | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288-popmetal.dts b/arch/arm/boot/dts/rk3288-popmetal.dts
index 802216f..706c67c 100644
--- a/arch/arm/boot/dts/rk3288-popmetal.dts
+++ b/arch/arm/boot/dts/rk3288-popmetal.dts
@@ -391,12 +391,16 @@
 		interrupts = <1 IRQ_TYPE_EDGE_RISING>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&comp_int>;
+		vdd-supply = <&vcc_io>;
+		vid-supply = <&vcc_io>;
 	};
 
-	l3g4200d: l3g4200d@68 {
+	l3g4200d: l3g4200d@69 {
 		compatible = "st,l3g4200d-gyro";
 		st,drdy-int-pin = <2>;
-		reg = <0x6b>;
+		reg = <0x69>;
+		vdd-supply = <&vcc_io>;
+		vddio-supply = <&vcc_io>;
 	};
 
 	mma8452: mma8452@1d {
-- 
2.7.4

[PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl

[Andy Yan]

There are 8 gpio banks on RK3288, so add the missing
RK_GPIO7 and RK_GPIO8. Also add gpio index definition
to make it easier to description GPIO in dts.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

 include/dt-bindings/pinctrl/rockchip.h | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/dt-bindings/pinctrl/rockchip.h b/include/dt-bindings/pinctrl/rockchip.h
index 743e66a..fd35350 100644
--- a/include/dt-bindings/pinctrl/rockchip.h
+++ b/include/dt-bindings/pinctrl/rockchip.h
@@ -24,6 +24,41 @@
 #define RK_GPIO3	3
 #define RK_GPIO4	4
 #define RK_GPIO6	6
+#define RK_GPIO7	7
+#define RK_GPIO8	8
+
+#define GPIO_A0		0
+#define GPIO_A1		1
+#define GPIO_A2		2
+#define GPIO_A3		3
+#define GPIO_A4		4
+#define GPIO_A5		5
+#define GPIO_A6		6
+#define GPIO_A7		7
+#define GPIO_B0		8
+#define GPIO_B1		9
+#define GPIO_B2		10
+#define GPIO_B3		11
+#define GPIO_B4		12
+#define GPIO_B5		13
+#define GPIO_B6		14
+#define GPIO_B7		15
+#define GPIO_C0		16
+#define GPIO_C1		17
+#define GPIO_C2		18
+#define GPIO_C3		19
+#define GPIO_C4		20
+#define GPIO_C5		21
+#define GPIO_C6		22
+#define GPIO_C7		23
+#define GPIO_D0		24
+#define GPIO_D1		25
+#define GPIO_D2		26
+#define GPIO_D3		27
+#define GPIO_D4		28
+#define GPIO_D5		29
+#define GPIO_D6		30
+#define GPIO_D7		31
 
 #define RK_FUNC_GPIO	0
 #define RK_FUNC_1	1
-- 
2.7.4

[PATCH 4/4] ARM: dts: use definition in rockchip pinctrl header to describe gpios on Popmetal-RK3288

[Andy Yan]

Use definition in rockchip pinctrl header to describe
gpios, this will make it more clearer.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

 arch/arm/boot/dts/rk3288-popmetal.dts | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288-popmetal.dts b/arch/arm/boot/dts/rk3288-popmetal.dts
index 706c67c..88f4ad0 100644
--- a/arch/arm/boot/dts/rk3288-popmetal.dts
+++ b/arch/arm/boot/dts/rk3288-popmetal.dts
@@ -72,7 +72,7 @@
 		pinctrl-0 = <&pwrbtn>;
 
 		power {
-			gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
+			gpios = <&gpio0 GPIO_A5 GPIO_ACTIVE_LOW>;
 			linux,code = <KEY_POWER>;
 			label = "GPIO Key Power";
 			linux,input-type = <1>;
@@ -83,7 +83,7 @@
 
 	ir: ir-receiver {
 		compatible = "gpio-ir-receiver";
-		gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
+		gpios = <&gpio0 GPIO_A6 GPIO_ACTIVE_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&ir_int>;
 	};
@@ -98,7 +98,7 @@
 
 	vcc_sd: sdmmc-regulator {
 		compatible = "regulator-fixed";
-		gpio = <&gpio7 11 GPIO_ACTIVE_LOW>;
+		gpio = <&gpio7 GPIO_B3 GPIO_ACTIVE_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdmmc_pwr>;
 		regulator-name = "vcc_sd";
@@ -132,7 +132,7 @@
 	vcc28_dvp: vcc28-dvp-regulator {
 		compatible = "regulator-fixed";
 		enable-active-high;
-		gpio = <&gpio0 17 GPIO_ACTIVE_HIGH>;
+		gpio = <&gpio0 GPIO0_C1 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&dvp_pwr>;
 		regulator-name = "vcc28_dvp";
@@ -178,7 +178,7 @@
 	phy-supply = <&vcc_lan>;
 	phy-mode = "rgmii";
 	clock_in_out = "input";
-	snps,reset-gpio = <&gpio4 7 0>;
+	snps,reset-gpio = <&gpio4 GPIO_B0 0>;
 	snps,reset-active-low;
 	snps,reset-delays-us = <0 10000 1000000>;
 	assigned-clocks = <&cru SCLK_MAC>;
@@ -447,43 +447,43 @@
 &pinctrl {
 	ak8963 {
 		comp_int: comp-int {
-			rockchip,pins = <8 1 RK_FUNC_GPIO &pcfg_pull_up>;
+			rockchip,pins = <RK_GPIO8 GPIO_A1 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
 	buttons {
 		pwrbtn: pwrbtn {
-			rockchip,pins = <0 5 RK_FUNC_GPIO &pcfg_pull_up>;
+			rockchip,pins = <RK_GPIO0 GPIO_A5 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
 	dvp {
 		dvp_pwr: dvp-pwr {
-			rockchip,pins = <0 17 RK_FUNC_GPIO &pcfg_pull_none>;
+			rockchip,pins = <RK_GPIO0 GPIO_C1 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 	};
 
 	ir {
 		ir_int: ir-int {
-			rockchip,pins = <0 6 RK_FUNC_GPIO &pcfg_pull_up>;
+			rockchip,pins = <RK_GPIO0 GPIO_A6 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
 	mma8452 {
 		gsensor_int: gsensor-int {
-			rockchip,pins = <8 0 RK_FUNC_GPIO &pcfg_pull_up>;
+			rockchip,pins = <RK_GPIO8 GPIO_A0 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
 	pmic {
 		pmic_int: pmic-int {
-			rockchip,pins = <RK_GPIO0 4 RK_FUNC_GPIO &pcfg_pull_up>;
+			rockchip,pins = <RK_GPIO0 GPIO_A4 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
 	sdmmc {
 		sdmmc_pwr: sdmmc-pwr {
-			rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
+			rockchip,pins = <RK_GPIO7 GPIO_B3 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 	};
 };
-- 
2.7.4

Re: [PATCH 4/4] ARM: dts: use definition in rockchip pinctrl header to describe gpios on Popmetal-RK3288

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Hi Andy,

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.8-rc4 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Andy-Yan/One-fix-and-some-improvements-for-RK3288-Popmetal-board/20160904-164007
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: arm-aspeed_g4_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

>> Error: arch/arm/boot/dts/rk3288-popmetal.dts:131.18-19 syntax error
>> FATAL ERROR: Unable to parse input tree

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 7602 bytes --]

Re: [PATCH 1/4] ARM: dts: rockchip: enable usbotg for Popemtal-rk3288 board

[Heiko Stuebner]

Am Sonntag, 4. September 2016, 16:33:10 CEST schrieb Andy Yan:
> Enable USB OTG port on RK3288 Popmetal board, So we can run
> some usb gadget functions like Android adb on this board.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

applied to my dts32 branch for 4.9


Thanks
Heiko

Re: [PATCH 2/4] ARM: dts: rockchip: fix i2c address L3G4200D on PopMetal-RK3288 board

[Heiko Stuebner]

Am Sonntag, 4. September 2016, 16:34:27 CEST schrieb Andy Yan:
> The i2c address of the three-axis digital gyroscope L3G4200D should be
> 0x69 according to hardware design.
> Also add power supply reference for L3G3200D and the 3-axis Electronic
> Compass AK8963.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

I've split this into two patches:
ARM: dts: rockchip: fix L3G4200D i2c address on PopMetal-RK3288 board
ARM: dts: rockchip: Add sensor-supplies on PopMetal-RK3288 board
and applied them to my dts32 branch for 4.9

Please don't mix multiple changes into one patch in the future :-)


Thanks
Heiko

Re: [PATCH 2/4] ARM: dts: rockchip: fix i2c address L3G4200D on PopMetal-RK3288 board

[Andy Yan]

Hi Heiko:



On 2016年09月05日 17:12, Heiko Stuebner wrote:
> Am Sonntag, 4. September 2016, 16:34:27 CEST schrieb Andy Yan:
>> The i2c address of the three-axis digital gyroscope L3G4200D should be
>> 0x69 according to hardware design.
>> Also add power supply reference for L3G3200D and the 3-axis Electronic
>> Compass AK8963.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> I've split this into two patches:
> ARM: dts: rockchip: fix L3G4200D i2c address on PopMetal-RK3288 board
> ARM: dts: rockchip: Add sensor-supplies on PopMetal-RK3288 board
> and applied them to my dts32 branch for 4.9
>
> Please don't mix multiple changes into one patch in the future :-)
>
     Thanks, I'll keep this in mind.
> Thanks
> Heiko
>
>
>

Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl

[Heiko Stuebner]

Hi Andy,

Am Sonntag, 4. September 2016, 16:35:30 CEST schrieb Andy Yan:
> There are 8 gpio banks on RK3288, so add the missing
> RK_GPIO7 and RK_GPIO8. Also add gpio index definition
> to make it easier to description GPIO in dts.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

I tend to disagree here.

I consider the bank defines RK_GPIOx to be deprecated and highly discourage 
them being used in new boards. They only encode the same number again (2 -> 
RK_GPIO2 etc) and therefore don't bring any useful addition over using the 
bank number directly.

Slightly similar argument for the per-pin defines. While the external pins are 
described in the A/B/C/Dx notation for pinmux purposes, the gpio controllers 
on top use a regular 0-31 numbering. Also you cannot name constants generic 
GPIO_x, simply because that may conflict with other overly generic constant 
names.

So I'm not yet convinced that these improve readability, but they would 
definitly need a RK_* prefix to make them specific.


Heiko

> ---
> 
>  include/dt-bindings/pinctrl/rockchip.h | 35
> ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
> 
> diff --git a/include/dt-bindings/pinctrl/rockchip.h
> b/include/dt-bindings/pinctrl/rockchip.h index 743e66a..fd35350 100644
> --- a/include/dt-bindings/pinctrl/rockchip.h
> +++ b/include/dt-bindings/pinctrl/rockchip.h
> @@ -24,6 +24,41 @@
>  #define RK_GPIO3	3
>  #define RK_GPIO4	4
>  #define RK_GPIO6	6
> +#define RK_GPIO7	7
> +#define RK_GPIO8	8
> +
> +#define GPIO_A0		0
> +#define GPIO_A1		1
> +#define GPIO_A2		2
> +#define GPIO_A3		3
> +#define GPIO_A4		4
> +#define GPIO_A5		5
> +#define GPIO_A6		6
> +#define GPIO_A7		7
> +#define GPIO_B0		8
> +#define GPIO_B1		9
> +#define GPIO_B2		10
> +#define GPIO_B3		11
> +#define GPIO_B4		12
> +#define GPIO_B5		13
> +#define GPIO_B6		14
> +#define GPIO_B7		15
> +#define GPIO_C0		16
> +#define GPIO_C1		17
> +#define GPIO_C2		18
> +#define GPIO_C3		19
> +#define GPIO_C4		20
> +#define GPIO_C5		21
> +#define GPIO_C6		22
> +#define GPIO_C7		23
> +#define GPIO_D0		24
> +#define GPIO_D1		25
> +#define GPIO_D2		26
> +#define GPIO_D3		27
> +#define GPIO_D4		28
> +#define GPIO_D5		29
> +#define GPIO_D6		30
> +#define GPIO_D7		31
> 
>  #define RK_FUNC_GPIO	0
>  #define RK_FUNC_1	1

Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl

[Andy Yan]

Hi Heiko:


On 2016年09月05日 17:33, Heiko Stuebner wrote:
> Hi Andy,
>
> Am Sonntag, 4. September 2016, 16:35:30 CEST schrieb Andy Yan:
>> There are 8 gpio banks on RK3288, so add the missing
>> RK_GPIO7 and RK_GPIO8. Also add gpio index definition
>> to make it easier to description GPIO in dts.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> I tend to disagree here.
>
> I consider the bank defines RK_GPIOx to be deprecated and highly discourage
> them being used in new boards. They only encode the same number again (2 ->
> RK_GPIO2 etc) and therefore don't bring any useful addition over using the
> bank number directly.
>
> Slightly similar argument for the per-pin defines. While the external pins are
> described in the A/B/C/Dx notation for pinmux purposes, the gpio controllers
> on top use a regular 0-31 numbering. Also you cannot name constants generic
> GPIO_x, simply because that may conflict with other overly generic constant
> names.
>
> So I'm not yet convinced that these improve readability, but they would
> definitly need a RK_* prefix to make them specific.
>
>
> Heiko

         I consider for a people who doesn't familiar with rockchip 
pinctrl, He
  may don't know what does these number 2/4/17....stands for in the dts  
like
  rockchip,pins = <2 17 RK_FUNC_GPIO &pcfg_pull_up>.
         But if we use meaningful macro here like : rockchip,pins = 
<RK_GPIO2 RK_GPIO_C0 RK_FUNC_GPIO &pcfg_pull_up>, I think it easier to 
tell people this GPIO is GPIO_C0 of BNAK2.
Especially all the GPIOs in rockchip based schematic are described as 
GPIOn_A/B/C/Dx. And it is also easier for people to directly use a macro 
stands for 0~32 than translating A/B/C/Dx from the schematic to 0~32.
>> ---
>>
>>   include/dt-bindings/pinctrl/rockchip.h | 35
>> ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
>>
>> diff --git a/include/dt-bindings/pinctrl/rockchip.h
>> b/include/dt-bindings/pinctrl/rockchip.h index 743e66a..fd35350 100644
>> --- a/include/dt-bindings/pinctrl/rockchip.h
>> +++ b/include/dt-bindings/pinctrl/rockchip.h
>> @@ -24,6 +24,41 @@
>>   #define RK_GPIO3	3
>>   #define RK_GPIO4	4
>>   #define RK_GPIO6	6
>> +#define RK_GPIO7	7
>> +#define RK_GPIO8	8
>> +
>> +#define GPIO_A0		0
>> +#define GPIO_A1		1
>> +#define GPIO_A2		2
>> +#define GPIO_A3		3
>> +#define GPIO_A4		4
>> +#define GPIO_A5		5
>> +#define GPIO_A6		6
>> +#define GPIO_A7		7
>> +#define GPIO_B0		8
>> +#define GPIO_B1		9
>> +#define GPIO_B2		10
>> +#define GPIO_B3		11
>> +#define GPIO_B4		12
>> +#define GPIO_B5		13
>> +#define GPIO_B6		14
>> +#define GPIO_B7		15
>> +#define GPIO_C0		16
>> +#define GPIO_C1		17
>> +#define GPIO_C2		18
>> +#define GPIO_C3		19
>> +#define GPIO_C4		20
>> +#define GPIO_C5		21
>> +#define GPIO_C6		22
>> +#define GPIO_C7		23
>> +#define GPIO_D0		24
>> +#define GPIO_D1		25
>> +#define GPIO_D2		26
>> +#define GPIO_D3		27
>> +#define GPIO_D4		28
>> +#define GPIO_D5		29
>> +#define GPIO_D6		30
>> +#define GPIO_D7		31
>>
>>   #define RK_FUNC_GPIO	0
>>   #define RK_FUNC_1	1
>
>
>
>

Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl

[Heiko Stuebner]

Am Montag, 5. September 2016, 18:22:46 CEST schrieb Andy Yan:
> Hi Heiko:
> 
> On 2016年09月05日 17:33, Heiko Stuebner wrote:
> > Hi Andy,
> > 
> > Am Sonntag, 4. September 2016, 16:35:30 CEST schrieb Andy Yan:
> >> There are 8 gpio banks on RK3288, so add the missing
> >> RK_GPIO7 and RK_GPIO8. Also add gpio index definition
> >> to make it easier to description GPIO in dts.
> >> 
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> > 
> > I tend to disagree here.
> > 
> > I consider the bank defines RK_GPIOx to be deprecated and highly
> > discourage
> > them being used in new boards. They only encode the same number again (2
> > ->
> > RK_GPIO2 etc) and therefore don't bring any useful addition over using the
> > bank number directly.
> > 
> > Slightly similar argument for the per-pin defines. While the external pins
> > are described in the A/B/C/Dx notation for pinmux purposes, the gpio
> > controllers on top use a regular 0-31 numbering. Also you cannot name
> > constants generic GPIO_x, simply because that may conflict with other
> > overly generic constant names.
> > 
> > So I'm not yet convinced that these improve readability, but they would
> > definitly need a RK_* prefix to make them specific.
> > 
> > 
> > Heiko
> 
>          I consider for a people who doesn't familiar with rockchip
> pinctrl, He
>   may don't know what does these number 2/4/17....stands for in the dts
> like
>   rockchip,pins = <2 17 RK_FUNC_GPIO &pcfg_pull_up>.

That person shouldn't work on devicetrees anyway :-) ... or simply read the 
dt-binding for the pinctrl at first ... which should normally really be the 
first step, otherwise we wouldn't need the binding documentation.

>          But if we use meaningful macro here like : rockchip,pins =
> <RK_GPIO2 RK_GPIO_C0 RK_FUNC_GPIO &pcfg_pull_up>, I think it easier to
> tell people this GPIO is GPIO_C0 of BNAK2.

I definitly don't agree for the gpio-bank number, see above .

> Especially all the GPIOs in rockchip based schematic are described as
> GPIOn_A/B/C/Dx. And it is also easier for people to directly use a macro
> stands for 0~32 than translating A/B/C/Dx from the schematic to 0~32.

But the pins might in fact really be helpful, but definitly need a prefix and 
maybe even distinguish between pin and gpio, aka RK_PIN_A0 etc?


Heiko

> 
> >> ---
> >> 
> >>   include/dt-bindings/pinctrl/rockchip.h | 35
> >> 
> >> ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
> >> 
> >> diff --git a/include/dt-bindings/pinctrl/rockchip.h
> >> b/include/dt-bindings/pinctrl/rockchip.h index 743e66a..fd35350 100644
> >> --- a/include/dt-bindings/pinctrl/rockchip.h
> >> +++ b/include/dt-bindings/pinctrl/rockchip.h
> >> @@ -24,6 +24,41 @@
> >> 
> >>   #define RK_GPIO3	3
> >>   #define RK_GPIO4	4
> >>   #define RK_GPIO6	6
> >> 
> >> +#define RK_GPIO7	7
> >> +#define RK_GPIO8	8
> >> +
> >> +#define GPIO_A0		0
> >> +#define GPIO_A1		1
> >> +#define GPIO_A2		2
> >> +#define GPIO_A3		3
> >> +#define GPIO_A4		4
> >> +#define GPIO_A5		5
> >> +#define GPIO_A6		6
> >> +#define GPIO_A7		7
> >> +#define GPIO_B0		8
> >> +#define GPIO_B1		9
> >> +#define GPIO_B2		10
> >> +#define GPIO_B3		11
> >> +#define GPIO_B4		12
> >> +#define GPIO_B5		13
> >> +#define GPIO_B6		14
> >> +#define GPIO_B7		15
> >> +#define GPIO_C0		16
> >> +#define GPIO_C1		17
> >> +#define GPIO_C2		18
> >> +#define GPIO_C3		19
> >> +#define GPIO_C4		20
> >> +#define GPIO_C5		21
> >> +#define GPIO_C6		22
> >> +#define GPIO_C7		23
> >> +#define GPIO_D0		24
> >> +#define GPIO_D1		25
> >> +#define GPIO_D2		26
> >> +#define GPIO_D3		27
> >> +#define GPIO_D4		28
> >> +#define GPIO_D5		29
> >> +#define GPIO_D6		30
> >> +#define GPIO_D7		31
> >> 
> >>   #define RK_FUNC_GPIO	0
> >>   #define RK_FUNC_1	1

Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl

[Andy Yan]

Hi Heiko:


On 2016年09月06日 06:10, Heiko Stuebner wrote:
> Am Montag, 5. September 2016, 18:22:46 CEST schrieb Andy Yan:
>> Hi Heiko:
>>
>> On 2016年09月05日 17:33, Heiko Stuebner wrote:
>>> Hi Andy,
>>>
>>> Am Sonntag, 4. September 2016, 16:35:30 CEST schrieb Andy Yan:
>>>> There are 8 gpio banks on RK3288, so add the missing
>>>> RK_GPIO7 and RK_GPIO8. Also add gpio index definition
>>>> to make it easier to description GPIO in dts.
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>> I tend to disagree here.
>>>
>>> I consider the bank defines RK_GPIOx to be deprecated and highly
>>> discourage
>>> them being used in new boards. They only encode the same number again (2
>>> ->
>>> RK_GPIO2 etc) and therefore don't bring any useful addition over using the
>>> bank number directly.
>>>
>>> Slightly similar argument for the per-pin defines. While the external pins
>>> are described in the A/B/C/Dx notation for pinmux purposes, the gpio
>>> controllers on top use a regular 0-31 numbering. Also you cannot name
>>> constants generic GPIO_x, simply because that may conflict with other
>>> overly generic constant names.
>>>
>>> So I'm not yet convinced that these improve readability, but they would
>>> definitly need a RK_* prefix to make them specific.
>>>
>>>
>>> Heiko
>>           I consider for a people who doesn't familiar with rockchip
>> pinctrl, He
>>    may don't know what does these number 2/4/17....stands for in the dts
>> like
>>    rockchip,pins = <2 17 RK_FUNC_GPIO &pcfg_pull_up>.
> That person shouldn't work on devicetrees anyway :-) ... or simply read the
> dt-binding for the pinctrl at first ... which should normally really be the
> first step, otherwise we wouldn't need the binding documentation.

     It's true that people who wants to develop in the kernel should 
know the technical detail  as much as possible.
But I still thinks it's a good thing if we can write code more clearly 
to  let people know what it stands for at first sight.
>>           But if we use meaningful macro here like : rockchip,pins =
>> <RK_GPIO2 RK_GPIO_C0 RK_FUNC_GPIO &pcfg_pull_up>, I think it easier to
>> tell people this GPIO is GPIO_C0 of BNAK2.
> I definitly don't agree for the gpio-bank number, see above .
>
>> Especially all the GPIOs in rockchip based schematic are described as
>> GPIOn_A/B/C/Dx. And it is also easier for people to directly use a macro
>> stands for 0~32 than translating A/B/C/Dx from the schematic to 0~32.
> But the pins might in fact really be helpful, but definitly need a prefix and
> maybe even distinguish between pin and gpio, aka RK_PIN_A0 etc?
>

     Anyway, very glad to see you think the pin definition is helpful, 
so let's move on.
I'll send a new version with the prefix as your suggested  like 
RK_PA/B/C/Dx.

     Thanks.
> Heiko
>
>>>> ---
>>>>
>>>>    include/dt-bindings/pinctrl/rockchip.h | 35
>>>>
>>>> ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
>>>>
>>>> diff --git a/include/dt-bindings/pinctrl/rockchip.h
>>>> b/include/dt-bindings/pinctrl/rockchip.h index 743e66a..fd35350 100644
>>>> --- a/include/dt-bindings/pinctrl/rockchip.h
>>>> +++ b/include/dt-bindings/pinctrl/rockchip.h
>>>> @@ -24,6 +24,41 @@
>>>>
>>>>    #define RK_GPIO3	3
>>>>    #define RK_GPIO4	4
>>>>    #define RK_GPIO6	6
>>>>
>>>> +#define RK_GPIO7	7
>>>> +#define RK_GPIO8	8
>>>> +
>>>> +#define GPIO_A0		0
>>>> +#define GPIO_A1		1
>>>> +#define GPIO_A2		2
>>>> +#define GPIO_A3		3
>>>> +#define GPIO_A4		4
>>>> +#define GPIO_A5		5
>>>> +#define GPIO_A6		6
>>>> +#define GPIO_A7		7
>>>> +#define GPIO_B0		8
>>>> +#define GPIO_B1		9
>>>> +#define GPIO_B2		10
>>>> +#define GPIO_B3		11
>>>> +#define GPIO_B4		12
>>>> +#define GPIO_B5		13
>>>> +#define GPIO_B6		14
>>>> +#define GPIO_B7		15
>>>> +#define GPIO_C0		16
>>>> +#define GPIO_C1		17
>>>> +#define GPIO_C2		18
>>>> +#define GPIO_C3		19
>>>> +#define GPIO_C4		20
>>>> +#define GPIO_C5		21
>>>> +#define GPIO_C6		22
>>>> +#define GPIO_C7		23
>>>> +#define GPIO_D0		24
>>>> +#define GPIO_D1		25
>>>> +#define GPIO_D2		26
>>>> +#define GPIO_D3		27
>>>> +#define GPIO_D4		28
>>>> +#define GPIO_D5		29
>>>> +#define GPIO_D6		30
>>>> +#define GPIO_D7		31
>>>>
>>>>    #define RK_FUNC_GPIO	0
>>>>    #define RK_FUNC_1	1
>
>
>
>