Re: [PATCH 01/04] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Magnus,

On Wednesday 18 December 2013 07:41:57 Magnus Damm wrote:
> On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart wrote:
> > On Tuesday 17 December 2013 14:02:42 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> >> and jtagport0.
> >> 
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >> 
> >>  arch/arm/boot/dts/r7s72100.dtsi |  154 +++++++++++++++++++++++++++++++++
> >>  1 file changed, 154 insertions(+)
> >> 
> >> --- 0001/arch/arm/boot/dts/r7s72100.dtsi
> >> +++ work/arch/arm/boot/dts/r7s72100.dtsi      2013-11-27
> >> 16:06:36.000000000 +0900

[snip]

> >> +
> >> +     port0: gpio@fcfe3100 {
> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> +             reg = <0xfcfe3100 0x4>, /* PSR */
> >> +                       <0xfcfe3200 0x2>, /* PPR */
> >> +                       <0xfcfe3800 0x4>; /* PMSR */
> >> +             #gpio-cells = <2>;
> >> +             gpio-controller;
> >> +             gpio-ranges = <&pfc 0 0 6>;
> >> +     };
> >> +
> >> +     port1: gpio@fcfe3104 {
> >> +             compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> >> +             reg = <0xfcfe3104 0x4>, /* PSR */
> >> +                       <0xfcfe3204 0x2>, /* PPR */
> >> +                       <0xfcfe3804 0x4>; /* PMSR */
> >> +             #gpio-cells = <2>;
> >> +             gpio-controller;
> >> +             gpio-ranges = <&pfc 0 16 16>;
> > 
> > As P0 has 6 pins only this should ideally be
> > 
> >                 gpio-ranges = <&pfc 0 6 16>;
> > 
> > Otherwise the PFC driver will expose pins that don't exist. However, that
> > would require computing the pin numbers in the PFC driver differently, as
> > we currently just use the bank * 16 + index formula. Given that we only
> > have three ports with less than 16 pins we could come up with a not
> > overly complex formula that can be evaluated at compile time. Something
> > like this should do.
> > 
> > #define RZ_PORT_PIN(bank, pin) \
> > 
> >         (bank) < 1 ? (pin) : \
> >         (bank) < 6 ? 6 + (((bank) - 1) * 16) + (pin)) : \
> >         (bank) < 10 ? 6 + 11 + 4 * 16 + (((bank) - 6) * 16) + (pin)) : \
> >         6 + 11 + 8 + 7 * 16 + (((bank) - 10) * 16) + (pin))
> 
> Uhm, well, you can make the mapping more compact yes, but I'm not sure
> if I agree that it becomes any better. Isn't it better to simply
> follow the per-port setup that the manual defines? Is there an actual
> problem with having unused GPIOs?

If I'm not mistaken it's unused pins, not unused GPIOs. They waste memory in 
data tables, although by a relatively small amount. Oh, and of course, it's 
not clean ;-)

Speaking of data tables, I'm thinking about simplifying them. The RZ/A1H is a 
good candidate for that, as each pin is handled individually, and several 
registers could be handled to with a small amount of code instead of large 
data tables. It's just a thought for now, I have more urgent tasks to work on.

> Actually, I prefer going in the opposite direction so I would like to
> share the simple version of RZ_PORT_PIN() in a header file like we do
> with RCAR_GP_PIN() in <linux/platform_data/gpio-rcar.h>. This because
> we would like to use the same macro in the GPIO driver and in the
> current PFC code (and potentially more PFCs using the same GPIO
> driver).

What do you need it for in the GPIO driver ?

> Please let me know what you think.

-- 
Regards,

Laurent Pinchart