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

[Magnus Damm]

Hi Laurent,

On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
> 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 at opensource.se>
>> >>
>> >> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
>> >> and jtagport0.
>> >>
>> >> Signed-off-by: Magnus Damm <damm at 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 at 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 at 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 ;-)

Yes, you are correct about pins vs GPIOs. Regarding how to implement
RZ_PORT_PIN(), I believe the only way not to shoot yourself in the
foot is to keep things simple. I also think that some level of
redundancy is an acceptable tradeoff if it keeps things simpler. So I
suppose cleanliness is a matter of taste. =)

> 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.

Incremental patches to improve the state is always nice, thanks.

>> 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 ?

Well, I thought I needed it but it turns out that I'm wrong. =)

Initially I had the following two in the header file:
+#define RZ_GPIOS_PER_PORT 16
+#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))

RZ_GPIOS_PER_PORT was used in both the GPIO driver and
RZ_PORT_PIN() was used in the PFC driver

On a second though, I don't mind duplicating them.

I do however think your version of the RZ_PORT_PIN() is overly
complex. And that needs to be matched with updated gpio-ranges that
together seem quite error prone to me.

How would you like me to proceed?

Cheers,

/ magnus