diff mbox series

[linux,dev-4.18] ASPEED: dts: Define the gpio-line-names property for aspeed-g5

Message ID 959CAFA1E282D14FB901BE9A7BF4E77249E0F1FA@shsmsx102.ccr.corp.intel.com
State Rejected, archived
Headers show
Series [linux,dev-4.18] ASPEED: dts: Define the gpio-line-names property for aspeed-g5 | expand

Commit Message

Wang, Kuiying Sept. 10, 2018, 7:27 a.m. UTC 
Gpio-line-names property of aspeed-g5 is not defined in the upstream submission.
It is required by new gpio API "linux/gpio.h"

commit 9dfd72ff5f628d7aaa10728756ce42bd5f768d4f (HEAD -> dev-4.18)
Author: Kuiying Wang <kuiying.wang@intel.com>
Date:   Fri Sep 7 18:01:41 2018 +0800

    Define the gpio-line-names property for aspeed-g5

    Based on aspeed AST-2500 Datasheet spec, there are 232 gpios.
    So defines the gpio line name from "A0" to "AC7".

    Signed-off-by: Kuiying Wang <kuiying.wang@intel.com>

Kuiying.

Comments

Andrew Jeffery Sept. 10, 2018, 7:36 a.m. UTC | #1 
Hi Kuiying,

On Mon, 10 Sep 2018, at 16:57, Wang, Kuiying wrote:
> Gpio-line-names property of aspeed-g5 is not defined in the upstream submission.
> It is required by new gpio API "linux/gpio.h"
> 
> commit 9dfd72ff5f628d7aaa10728756ce42bd5f768d4f (HEAD -> dev-4.18)
> Author: Kuiying Wang <kuiying.wang@intel.com>
> Date:   Fri Sep 7 18:01:41 2018 +0800
> 
>     Define the gpio-line-names property for aspeed-g5
> 
>     Based on aspeed AST-2500 Datasheet spec, there are 232 gpios.
>     So defines the gpio line name from "A0" to "AC7".
> 
>     Signed-off-by: Kuiying Wang <kuiying.wang@intel.com>
> 
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/
> aspeed-g5.dtsi
> index d92f047907de..6d664f4f1621 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -265,6 +265,36 @@
>                                 gpio-ranges = <&pinctrl 0 0 220>;
>                                 clocks = <&syscon ASPEED_CLK_APB>;
>                                 interrupt-controller;
> +                             gpio-line-names = 
> "A0","A1","A2","A3","A4","A5","A6","A7",
> +                                   
> "B0","B1","B2","B3","B4","B5","B6","B7",
> +                                   
> "C0","C1","C2","C3","C4","C5","C6","C7",
> +                                   
> "D0","D1","D2","D3","D4","D5","D6","D7",
> +                                   
> "E0","E1","E2","E3","E4","E5","E6","E7",
> +                                   
> "F0","F1","F2","F3","F4","F5","F6","F7",
> +                                   
> "G0","G1","G2","G3","G4","G5","G6","G7",
> +                                   
> "H0","H1","H2","H3","H4","H5","H6","H7",
> +                                   
> "I0","I1","I2","I3","I4","I5","I6","I7",
> +                                   
> "J0","J1","J2","J3","J4","J5","J6","J7",
> +                                   
> "K0","E1","E2","E3","E4","E5","E6","E7",
> +                                   
> "L0","L1","L2","L3","L4","L5","L6","L7",
> +                                   
> "M0","M1","M2","M3","M4","M5","M6","M7",
> +                                   
> "N0","N1","N2","N3","N4","N5","N6","N7",
> +                                   
> "O0","O1","O2","O3","O4","O5","O6","O7",
> +                                   
> "P0","P1","P2","P3","P4","P5","P6","P7",
> +                                   
> "Q0","Q1","Q2","Q3","Q4","Q5","Q6","Q7",
> +                                   
> "R0","R1","R2","R3","R4","R5","R6","R7",
> +                                   
> "S0","S1","S2","S3","S4","S5","S6","S7",
> +                                   
> "T0","T1","T2","T3","T4","T5","T6","T7",
> +                                   
> "U0","U1","U2","U3","U4","U5","U6","U7",
> +                                   
> "V0","V1","V2","V3","V4","V5","V6","V7",
> +                                   
> "W0","W1","W2","W3","W4","W5","W6","W7",
> +                                   
> "X0","X1","X2","X3","X4","X5","X6","X7",
> +                                   
> "Y0","Y1","Y2","Y3","Y4","Y5","Y6","Y7",
> +                                   
> "Z0","Z1","Z2","Z3","Z4","Z5","Z6","Z7",
> +                                   
> "AA0","AA1","AA2","AA3","AA4","AA5","AA6","AA7",
> +                                   
> "AB0","AB1","AB2","AB3","AB4","AB5","AB6","AB7",
> +                                   
> "AC0","AC1","AC2","AC3","AC4","AC5","AC6","AC7"
> +                             ;
>                         };

I have to say I don't find this terribly useful. It's much better if the platform DTS adds a name that captures the semantic meaning of the line rather than simply indicating its position in a bank. It's pretty straight forward to convert between the raw index and the "bank name" (e.g. GPIOV3). If it helps Joel wrote a little python script for it:

https://github.com/shenki/aspeed-kernel-tests/blob/master/num-to-gpio

Cheers,

Andrew
Wang, Kuiying Sept. 10, 2018, 7:56 a.m. UTC | #2 
Hi Andrew J,
This is just common definition for aspeed-g5.
There will be different configuration for specified platform just like following:
https://gerrit.openbmc-project.xyz/#/c/openbmc/meta-intel/+/12178/1/meta-intel/meta-s2600wf/recipes-phosphor/skeleton/obmc-libobmc-intf/gpio_defs.json 


Thanks,
Kuiying.


-----Original Message-----
From: Andrew Jeffery [mailto:andrew@aj.id.au] 
Sent: Monday, September 10, 2018 3:37 PM
To: Wang, Kuiying <kuiying.wang@intel.com>; Andrew Geissler <geissonator@gmail.com>; Joel Stanley <joel@jms.id.au>
Cc: Brad Bishop <bradleyb@fuzziesquirrel.com>; Andrew Geissler <geissonator@yahoo.com>; chunhui.jia@linux.intel.com; kunyi731@gmail.com; Mihm, James <james.mihm@intel.com>; Tanous, Ed <ed.tanous@intel.com>; Feist, James <james.feist@intel.com>; Jia, Chunhui <chunhui.jia@intel.com>; Patrick Venture <venture@google.com>; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Li, Yong B <yong.b.li@intel.com>; Yang, Cheng C <cheng.c.yang@intel.com>; Xu, Qiang <qiang.xu@intel.com>; Nguyen, Hai V <hai.v.nguyen@intel.com>
Subject: Re: [PATCH linux dev-4.18] ASPEED: dts: Define the gpio-line-names property for aspeed-g5

Hi Kuiying,

On Mon, 10 Sep 2018, at 16:57, Wang, Kuiying wrote:
> Gpio-line-names property of aspeed-g5 is not defined in the upstream submission.
> It is required by new gpio API "linux/gpio.h"
> 
> commit 9dfd72ff5f628d7aaa10728756ce42bd5f768d4f (HEAD -> dev-4.18)
> Author: Kuiying Wang <kuiying.wang@intel.com>
> Date:   Fri Sep 7 18:01:41 2018 +0800
> 
>     Define the gpio-line-names property for aspeed-g5
> 
>     Based on aspeed AST-2500 Datasheet spec, there are 232 gpios.
>     So defines the gpio line name from "A0" to "AC7".
> 
>     Signed-off-by: Kuiying Wang <kuiying.wang@intel.com>
> 
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/ 
> aspeed-g5.dtsi index d92f047907de..6d664f4f1621 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -265,6 +265,36 @@
>                                 gpio-ranges = <&pinctrl 0 0 220>;
>                                 clocks = <&syscon ASPEED_CLK_APB>;
>                                 interrupt-controller;
> +                             gpio-line-names =
> "A0","A1","A2","A3","A4","A5","A6","A7",
> +                                   
> "B0","B1","B2","B3","B4","B5","B6","B7",
> +                                   
> "C0","C1","C2","C3","C4","C5","C6","C7",
> +                                   
> "D0","D1","D2","D3","D4","D5","D6","D7",
> +                                   
> "E0","E1","E2","E3","E4","E5","E6","E7",
> +                                   
> "F0","F1","F2","F3","F4","F5","F6","F7",
> +                                   
> "G0","G1","G2","G3","G4","G5","G6","G7",
> +                                   
> "H0","H1","H2","H3","H4","H5","H6","H7",
> +                                   
> "I0","I1","I2","I3","I4","I5","I6","I7",
> +                                   
> "J0","J1","J2","J3","J4","J5","J6","J7",
> +                                   
> "K0","E1","E2","E3","E4","E5","E6","E7",
> +                                   
> "L0","L1","L2","L3","L4","L5","L6","L7",
> +                                   
> "M0","M1","M2","M3","M4","M5","M6","M7",
> +                                   
> "N0","N1","N2","N3","N4","N5","N6","N7",
> +                                   
> "O0","O1","O2","O3","O4","O5","O6","O7",
> +                                   
> "P0","P1","P2","P3","P4","P5","P6","P7",
> +                                   
> "Q0","Q1","Q2","Q3","Q4","Q5","Q6","Q7",
> +                                   
> "R0","R1","R2","R3","R4","R5","R6","R7",
> +                                   
> "S0","S1","S2","S3","S4","S5","S6","S7",
> +                                   
> "T0","T1","T2","T3","T4","T5","T6","T7",
> +                                   
> "U0","U1","U2","U3","U4","U5","U6","U7",
> +                                   
> "V0","V1","V2","V3","V4","V5","V6","V7",
> +                                   
> "W0","W1","W2","W3","W4","W5","W6","W7",
> +                                   
> "X0","X1","X2","X3","X4","X5","X6","X7",
> +                                   
> "Y0","Y1","Y2","Y3","Y4","Y5","Y6","Y7",
> +                                   
> "Z0","Z1","Z2","Z3","Z4","Z5","Z6","Z7",
> +                                   
> "AA0","AA1","AA2","AA3","AA4","AA5","AA6","AA7",
> +                                   
> "AB0","AB1","AB2","AB3","AB4","AB5","AB6","AB7",
> +                                   
> "AC0","AC1","AC2","AC3","AC4","AC5","AC6","AC7"
> +                             ;
>                         };

I have to say I don't find this terribly useful. It's much better if the platform DTS adds a name that captures the semantic meaning of the line rather than simply indicating its position in a bank. It's pretty straight forward to convert between the raw index and the "bank name" (e.g. GPIOV3). If it helps Joel wrote a little python script for it:

https://github.com/shenki/aspeed-kernel-tests/blob/master/num-to-gpio

Cheers,

Andrew
Andrew Jeffery Sept. 11, 2018, 1:28 a.m. UTC | #3 
On Mon, 10 Sep 2018, at 17:26, Wang, Kuiying wrote:
> Hi Andrew J,
> This is just common definition for aspeed-g5.
> There will be different configuration for specified platform just like 
> following:
> https://gerrit.openbmc-project.xyz/#/c/openbmc/meta-intel/+/12178/1/meta-intel/meta-s2600wf/recipes-phosphor/skeleton/obmc-libobmc-intf/gpio_defs.json 

Are you doing a string match on the pin name from the structures returned by ioctls on the gpiochip's chardev? This won't work if someone decides to specify the semantic name of the line in the devicetree like I linked to in my previous reply. Your best bet is to map the pin name from the JSON to the pin index on the gpiochip. Doing it any other way is fragile.

Andrew
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d92f047907de..6d664f4f1621 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -265,6 +265,36 @@ 
                                gpio-ranges = <&pinctrl 0 0 220>;
                                clocks = <&syscon ASPEED_CLK_APB>;
                                interrupt-controller;
+                             gpio-line-names = "A0","A1","A2","A3","A4","A5","A6","A7",
+                                   "B0","B1","B2","B3","B4","B5","B6","B7",
+                                   "C0","C1","C2","C3","C4","C5","C6","C7",
+                                   "D0","D1","D2","D3","D4","D5","D6","D7",
+                                   "E0","E1","E2","E3","E4","E5","E6","E7",
+                                   "F0","F1","F2","F3","F4","F5","F6","F7",
+                                   "G0","G1","G2","G3","G4","G5","G6","G7",
+                                   "H0","H1","H2","H3","H4","H5","H6","H7",
+                                   "I0","I1","I2","I3","I4","I5","I6","I7",
+                                   "J0","J1","J2","J3","J4","J5","J6","J7",
+                                   "K0","E1","E2","E3","E4","E5","E6","E7",
+                                   "L0","L1","L2","L3","L4","L5","L6","L7",
+                                   "M0","M1","M2","M3","M4","M5","M6","M7",
+                                   "N0","N1","N2","N3","N4","N5","N6","N7",
+                                   "O0","O1","O2","O3","O4","O5","O6","O7",
+                                   "P0","P1","P2","P3","P4","P5","P6","P7",
+                                   "Q0","Q1","Q2","Q3","Q4","Q5","Q6","Q7",
+                                   "R0","R1","R2","R3","R4","R5","R6","R7",
+                                   "S0","S1","S2","S3","S4","S5","S6","S7",
+                                   "T0","T1","T2","T3","T4","T5","T6","T7",
+                                   "U0","U1","U2","U3","U4","U5","U6","U7",
+                                   "V0","V1","V2","V3","V4","V5","V6","V7",
+                                   "W0","W1","W2","W3","W4","W5","W6","W7",
+                                   "X0","X1","X2","X3","X4","X5","X6","X7",
+                                   "Y0","Y1","Y2","Y3","Y4","Y5","Y6","Y7",
+                                   "Z0","Z1","Z2","Z3","Z4","Z5","Z6","Z7",
+                                   "AA0","AA1","AA2","AA3","AA4","AA5","AA6","AA7",
+                                   "AB0","AB1","AB2","AB3","AB4","AB5","AB6","AB7",
+                                   "AC0","AC1","AC2","AC3","AC4","AC5","AC6","AC7"
+                             ;
                        };

Thanks,