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

[Frank Lee <tiny.windzz@gmail.com>]

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