[PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Frank Lee]

From: Yangtao Li <frank@allwinnertech.com>

The sunxi gpio binding defines a few custom cells for its gpio specifier.
Provide bank name for those.

Signed-off-by: Yangtao Li <frank@allwinnertech.com>

---
 include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h

diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
new file mode 100644
index 000000000000..c692b4360da6
--- /dev/null
+++ b/include/dt-bindings/gpio/sunxi-gpio.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GPIO definitions for Allwinner SoCs
+ *
+ * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
+ */
+
+#ifndef _DT_BINDINGS_SUNXI_GPIO_H
+#define _DT_BINDINGS_SUNXI_GPIO_H
+
+#include <dt-bindings/gpio/gpio.h>
+
+/* pio */
+#define PA	0
+#define PB	1
+#define PC	2
+#define PD	3
+#define PE	4
+#define PF	5
+#define PG	6
+#define PH	7
+#define PI	8
+
+/* r-pio */
+#define PL	0
+#define PM	1
+#define PN	2
+
+#endif /* _DT_BINDINGS_SUNXI_GPIO_H */
-- 
2.24.0

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Maxime Ripard]

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

Hi!

On Wed, Jul 15, 2020 at 07:54:12PM +0800, Frank Lee wrote:
> From: Yangtao Li <frank@allwinnertech.com>
> 
> The sunxi gpio binding defines a few custom cells for its gpio specifier.
> Provide bank name for those.
> 
> Signed-off-by: Yangtao Li <frank@allwinnertech.com>

Thanks for working on this, I wanted to do it at some point but it kept
getting pushed further into my todo list.

> ---
>  include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h
> 
> diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
> new file mode 100644
> index 000000000000..c692b4360da6
> --- /dev/null
> +++ b/include/dt-bindings/gpio/sunxi-gpio.h

So generally we've been using the compatible name as the file name. You
should follow that convention too, and since it was added with the A10,
using the A10 compatible.

> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * GPIO definitions for Allwinner SoCs
> + *
> + * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
> + */
> +
> +#ifndef _DT_BINDINGS_SUNXI_GPIO_H
> +#define _DT_BINDINGS_SUNXI_GPIO_H
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/* pio */
> +#define PA	0
> +#define PB	1
> +#define PC	2
> +#define PD	3
> +#define PE	4
> +#define PF	5
> +#define PG	6
> +#define PH	7
> +#define PI	8
> +
> +/* r-pio */
> +#define PL	0
> +#define PM	1
> +#define PN	2
> +
> +#endif /* _DT_BINDINGS_SUNXI_GPIO_H */

Maybe we can go one step further and use a macro to have something like
PIN(A, 12) ?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Samuel Holland]

Maxime,

On 7/17/20 11:07 AM, Maxime Ripard wrote:
> Hi!
> 
> On Wed, Jul 15, 2020 at 07:54:12PM +0800, Frank Lee wrote:
>> From: Yangtao Li <frank@allwinnertech.com>
>>
>> The sunxi gpio binding defines a few custom cells for its gpio specifier.
>> Provide bank name for those.
>>
>> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> 
> Thanks for working on this, I wanted to do it at some point but it kept
> getting pushed further into my todo list.
> 
>> ---
>>  include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>  create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h
>>
>> diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
>> new file mode 100644
>> index 000000000000..c692b4360da6
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/sunxi-gpio.h
> 
> So generally we've been using the compatible name as the file name. You
> should follow that convention too, and since it was added with the A10,
> using the A10 compatible.
> 
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * GPIO definitions for Allwinner SoCs
>> + *
>> + * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_SUNXI_GPIO_H
>> +#define _DT_BINDINGS_SUNXI_GPIO_H
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +/* pio */
>> +#define PA	0
>> +#define PB	1
>> +#define PC	2
>> +#define PD	3
>> +#define PE	4
>> +#define PF	5
>> +#define PG	6
>> +#define PH	7
>> +#define PI	8
>> +
>> +/* r-pio */
>> +#define PL	0
>> +#define PM	1
>> +#define PN	2
>> +
>> +#endif /* _DT_BINDINGS_SUNXI_GPIO_H */
> 
> Maybe we can go one step further and use a macro to have something like
> PIN(A, 12) ?

Since we have separate cells for the bank and pin, I don't think it would be
appropriate to have a single macro generating both. And I'm not sure what the
benefit of the macro would be, if all it does is forward its arguments. Are you
concerned that P[A-M] could conflict with something else in the device tree?

Cheers,
Samuel

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Maxime Ripard]

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

Hi,

On Sat, Jul 25, 2020 at 02:18:39PM -0500, Samuel Holland wrote:
> On 7/17/20 11:07 AM, Maxime Ripard wrote:
> > Hi!
> > 
> > On Wed, Jul 15, 2020 at 07:54:12PM +0800, Frank Lee wrote:
> >> From: Yangtao Li <frank@allwinnertech.com>
> >>
> >> The sunxi gpio binding defines a few custom cells for its gpio specifier.
> >> Provide bank name for those.
> >>
> >> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > 
> > Thanks for working on this, I wanted to do it at some point but it kept
> > getting pushed further into my todo list.
> > 
> >> ---
> >>  include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>  create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h
> >>
> >> diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
> >> new file mode 100644
> >> index 000000000000..c692b4360da6
> >> --- /dev/null
> >> +++ b/include/dt-bindings/gpio/sunxi-gpio.h
> > 
> > So generally we've been using the compatible name as the file name. You
> > should follow that convention too, and since it was added with the A10,
> > using the A10 compatible.
> > 
> >> @@ -0,0 +1,29 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * GPIO definitions for Allwinner SoCs
> >> + *
> >> + * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_SUNXI_GPIO_H
> >> +#define _DT_BINDINGS_SUNXI_GPIO_H
> >> +
> >> +#include <dt-bindings/gpio/gpio.h>
> >> +
> >> +/* pio */
> >> +#define PA	0
> >> +#define PB	1
> >> +#define PC	2
> >> +#define PD	3
> >> +#define PE	4
> >> +#define PF	5
> >> +#define PG	6
> >> +#define PH	7
> >> +#define PI	8
> >> +
> >> +/* r-pio */
> >> +#define PL	0
> >> +#define PM	1
> >> +#define PN	2
> >> +
> >> +#endif /* _DT_BINDINGS_SUNXI_GPIO_H */
> > 
> > Maybe we can go one step further and use a macro to have something like
> > PIN(A, 12) ?
> 
> Since we have separate cells for the bank and pin, I don't think it would be
> appropriate to have a single macro generating both.

Yeah, but it's "just" an encoding issue though, it's not a major concern
if it makes our life easier.

> And I'm not sure what the benefit of the macro would be, if all it
> does is forward its arguments. Are you concerned that P[A-M] could
> conflict with something else in the device tree?

There's indeed a bunch of names that are fairly generic and could be
conflicting with others (PD for power domain is the first one that comes
to my mind). Using a prefix would make the GPIO descriptors pretty long,
so it wasn't ideal either. A macro makes it readable without increasing
too much the risks of conflicts

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Frank Lee]

On Wed, Jul 29, 2020 at 9:06 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Sat, Jul 25, 2020 at 02:18:39PM -0500, Samuel Holland wrote:
> > On 7/17/20 11:07 AM, Maxime Ripard wrote:
> > > Hi!
> > >
> > > On Wed, Jul 15, 2020 at 07:54:12PM +0800, Frank Lee wrote:
> > >> From: Yangtao Li <frank@allwinnertech.com>
> > >>
> > >> The sunxi gpio binding defines a few custom cells for its gpio specifier.
> > >> Provide bank name for those.
> > >>
> > >> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > >
> > > Thanks for working on this, I wanted to do it at some point but it kept
> > > getting pushed further into my todo list.
> > >
> > >> ---
> > >>  include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
> > >>  1 file changed, 29 insertions(+)
> > >>  create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h
> > >>
> > >> diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
> > >> new file mode 100644
> > >> index 000000000000..c692b4360da6
> > >> --- /dev/null
> > >> +++ b/include/dt-bindings/gpio/sunxi-gpio.h
> > >
> > > So generally we've been using the compatible name as the file name. You
> > > should follow that convention too, and since it was added with the A10,
> > > using the A10 compatible.
> > >
> > >> @@ -0,0 +1,29 @@
> > >> +/* SPDX-License-Identifier: GPL-2.0 */
> > >> +/*
> > >> + * GPIO definitions for Allwinner SoCs
> > >> + *
> > >> + * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
> > >> + */
> > >> +
> > >> +#ifndef _DT_BINDINGS_SUNXI_GPIO_H
> > >> +#define _DT_BINDINGS_SUNXI_GPIO_H
> > >> +
> > >> +#include <dt-bindings/gpio/gpio.h>
> > >> +
> > >> +/* pio */
> > >> +#define PA        0
> > >> +#define PB        1
> > >> +#define PC        2
> > >> +#define PD        3
> > >> +#define PE        4
> > >> +#define PF        5
> > >> +#define PG        6
> > >> +#define PH        7
> > >> +#define PI        8
> > >> +
> > >> +/* r-pio */
> > >> +#define PL        0
> > >> +#define PM        1
> > >> +#define PN        2
> > >> +
> > >> +#endif /* _DT_BINDINGS_SUNXI_GPIO_H */
> > >
> > > Maybe we can go one step further and use a macro to have something like
> > > PIN(A, 12) ?
> >
> > Since we have separate cells for the bank and pin, I don't think it would be
> > appropriate to have a single macro generating both.
>
> Yeah, but it's "just" an encoding issue though, it's not a major concern
> if it makes our life easier.
>
> > And I'm not sure what the benefit of the macro would be, if all it
> > does is forward its arguments. Are you concerned that P[A-M] could
> > conflict with something else in the device tree?
>
> There's indeed a bunch of names that are fairly generic and could be
> conflicting with others (PD for power domain is the first one that comes
> to my mind). Using a prefix would make the GPIO descriptors pretty long,
> so it wasn't ideal either. A macro makes it readable without increasing
> too much the risks of conflicts

I tried to use macros, but failed.

I have a look at some other GPIO macros, which have a prefix in front of them.
Maybe we can do the same? It's all numbers. It's not intuitive.

Yangtao

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Frank Lee]

HI Maxime,

Maybe let's convert to use something like SUN4I_PA?

Yangtao

Re: [PATCH 1/3] dt-bindings: gpio: sunxi：create a DT header for Allwinner pin controller

[Maxime Ripard]

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

On Wed, Oct 28, 2020 at 08:15:45PM +0800, Frank Lee wrote:
> On Wed, Jul 29, 2020 at 9:06 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Sat, Jul 25, 2020 at 02:18:39PM -0500, Samuel Holland wrote:
> > > On 7/17/20 11:07 AM, Maxime Ripard wrote:
> > > > Hi!
> > > >
> > > > On Wed, Jul 15, 2020 at 07:54:12PM +0800, Frank Lee wrote:
> > > >> From: Yangtao Li <frank@allwinnertech.com>
> > > >>
> > > >> The sunxi gpio binding defines a few custom cells for its gpio specifier.
> > > >> Provide bank name for those.
> > > >>
> > > >> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > > >
> > > > Thanks for working on this, I wanted to do it at some point but it kept
> > > > getting pushed further into my todo list.
> > > >
> > > >> ---
> > > >>  include/dt-bindings/gpio/sunxi-gpio.h | 29 +++++++++++++++++++++++++++
> > > >>  1 file changed, 29 insertions(+)
> > > >>  create mode 100644 include/dt-bindings/gpio/sunxi-gpio.h
> > > >>
> > > >> diff --git a/include/dt-bindings/gpio/sunxi-gpio.h b/include/dt-bindings/gpio/sunxi-gpio.h
> > > >> new file mode 100644
> > > >> index 000000000000..c692b4360da6
> > > >> --- /dev/null
> > > >> +++ b/include/dt-bindings/gpio/sunxi-gpio.h
> > > >
> > > > So generally we've been using the compatible name as the file name. You
> > > > should follow that convention too, and since it was added with the A10,
> > > > using the A10 compatible.
> > > >
> > > >> @@ -0,0 +1,29 @@
> > > >> +/* SPDX-License-Identifier: GPL-2.0 */
> > > >> +/*
> > > >> + * GPIO definitions for Allwinner SoCs
> > > >> + *
> > > >> + * Copyright (C) 2020 Yangtao Li <frank@allwinnertech.com>
> > > >> + */
> > > >> +
> > > >> +#ifndef _DT_BINDINGS_SUNXI_GPIO_H
> > > >> +#define _DT_BINDINGS_SUNXI_GPIO_H
> > > >> +
> > > >> +#include <dt-bindings/gpio/gpio.h>
> > > >> +
> > > >> +/* pio */
> > > >> +#define PA        0
> > > >> +#define PB        1
> > > >> +#define PC        2
> > > >> +#define PD        3
> > > >> +#define PE        4
> > > >> +#define PF        5
> > > >> +#define PG        6
> > > >> +#define PH        7
> > > >> +#define PI        8
> > > >> +
> > > >> +/* r-pio */
> > > >> +#define PL        0
> > > >> +#define PM        1
> > > >> +#define PN        2
> > > >> +
> > > >> +#endif /* _DT_BINDINGS_SUNXI_GPIO_H */
> > > >
> > > > Maybe we can go one step further and use a macro to have something like
> > > > PIN(A, 12) ?
> > >
> > > Since we have separate cells for the bank and pin, I don't think it would be
> > > appropriate to have a single macro generating both.
> >
> > Yeah, but it's "just" an encoding issue though, it's not a major concern
> > if it makes our life easier.
> >
> > > And I'm not sure what the benefit of the macro would be, if all it
> > > does is forward its arguments. Are you concerned that P[A-M] could
> > > conflict with something else in the device tree?
> >
> > There's indeed a bunch of names that are fairly generic and could be
> > conflicting with others (PD for power domain is the first one that comes
> > to my mind). Using a prefix would make the GPIO descriptors pretty long,
> > so it wasn't ideal either. A macro makes it readable without increasing
> > too much the risks of conflicts
> 
> I tried to use macros, but failed.
> 
> I have a look at some other GPIO macros, which have a prefix in front of them.
> Maybe we can do the same? It's all numbers. It's not intuitive.

This works

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
index f3f8e177ab61..3a66d1102a78 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
@@ -8,6 +8,9 @@
 
 #include <dt-bindings/gpio/gpio.h>
 
+#define SUNXI_PE 4
+#define SUNXI_PIN(bank, pin)    SUNXI_##bank pin
+
 / {
 	model = "Olimex A64-Olinuxino";
 	compatible = "olimex,a64-olinuxino", "allwinner,sun50i-a64";
@@ -37,7 +40,7 @@ leds {
 
 		led-0 {
 			label = "a64-olinuxino:red:user";
-			gpios = <&pio 4 17 GPIO_ACTIVE_HIGH>; /* PE17 */
+			gpios = <&pio SUNXI_PIN(PE, 17) GPIO_ACTIVE_HIGH>;
 		};
 	};
 

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]