| Message ID | 20131217050242.727.69737.sendpatchset@w520 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Magnus, Thank you for the patch. 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 > @@ -14,6 +14,22 @@ > #address-cells = <1>; > #size-cells = <1>; > > + aliases { > + gpio0 = &port0; > + gpio1 = &port1; > + gpio2 = &port2; > + gpio3 = &port3; > + gpio4 = &port4; > + gpio5 = &port5; > + gpio6 = &port6; > + gpio7 = &port7; > + gpio8 = &port8; > + gpio9 = &port9; > + gpio10 = &port10; > + gpio11 = &port11; > + gpio12 = &jtagport0; > + }; > + > cpus { > #address-cells = <1>; > #size-cells = <0>; > @@ -33,4 +49,142 @@ > reg = <0xe8201000 0x1000>, > <0xe8202000 0x1000>; > }; > + > + pfc: pfc@fcfe3300 { > + compatible = "renesas,pfc-r7s72100"; > + reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */ > + <0xfcfe3a00 0x100>, /* PFCAE */ > + <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */ > + <0xfcfe7b40 0x04>, /* JPMC */ > + <0xfcfe7b90 0x04>, /* JPMCSR */ > + <0xfcfe7f00 0x04>; /* JPIBC */ > + }; > + > + 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)) > + }; > + > + port2: gpio@fcfe3108 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3108 0x4>, /* PSR */ > + <0xfcfe3208 0x2>, /* PPR */ > + <0xfcfe3808 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 32 16>; > + }; > + > + port3: gpio@fcfe310c { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe310c 0x4>, /* PSR */ > + <0xfcfe320c 0x2>, /* PPR */ > + <0xfcfe380c 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 48 16>; > + }; > + > + port4: gpio@fcfe3110 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3110 0x4>, /* PSR */ > + <0xfcfe3210 0x2>, /* PPR */ > + <0xfcfe3810 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 64 16>; > + }; > + > + port5: gpio@fcfe3114 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3114 0x4>, /* PSR */ > + <0xfcfe3214 0x2>, /* PPR */ > + <0xfcfe3814 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 80 11>; > + }; > + > + port6: gpio@fcfe3118 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3118 0x4>, /* PSR */ > + <0xfcfe3218 0x2>, /* PPR */ > + <0xfcfe3818 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 96 16>; > + }; > + > + port7: gpio@fcfe311c { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe311c 0x4>, /* PSR */ > + <0xfcfe321c 0x2>, /* PPR */ > + <0xfcfe381c 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 112 16>; > + }; > + > + port8: gpio@fcfe3120 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3120 0x4>, /* PSR */ > + <0xfcfe3220 0x2>, /* PPR */ > + <0xfcfe3820 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 128 16>; > + }; > + > + port9: gpio@fcfe3124 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3124 0x4>, /* PSR */ > + <0xfcfe3224 0x2>, /* PPR */ > + <0xfcfe3824 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 144 8>; > + }; > + > + port10: gpio@fcfe3128 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe3128 0x4>, /* PSR */ > + <0xfcfe3228 0x2>, /* PPR */ > + <0xfcfe3828 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 160 16>; > + }; > + > + port11: gpio@fcfe312c { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe312c 0x4>, /* PSR */ > + <0xfcfe322c 0x2>, /* PPR */ > + <0xfcfe382c 0x4>; /* PMSR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 176 16>; > + }; > + > + jtagport0: gpio@fcfe7b20 { > + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; > + reg = <0xfcfe7b20 0x2>; /* JPPR */ > + #gpio-cells = <2>; > + gpio-controller; > + gpio-ranges = <&pfc 0 192 2>; > + }; > };
On Tue, Dec 17, 2013 at 02:02:42PM +0900, 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> Got this when applying the patch: warning: squelched 8 whitespace errors warning: 13 lines add whitespace errors.
Hi Laurent, On Wed, Dec 18, 2013 at 1:29 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > Thank you for the patch. > > 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 >> @@ -14,6 +14,22 @@ >> #address-cells = <1>; >> #size-cells = <1>; >> >> + aliases { >> + gpio0 = &port0; >> + gpio1 = &port1; >> + gpio2 = &port2; >> + gpio3 = &port3; >> + gpio4 = &port4; >> + gpio5 = &port5; >> + gpio6 = &port6; >> + gpio7 = &port7; >> + gpio8 = &port8; >> + gpio9 = &port9; >> + gpio10 = &port10; >> + gpio11 = &port11; >> + gpio12 = &jtagport0; >> + }; >> + >> cpus { >> #address-cells = <1>; >> #size-cells = <0>; >> @@ -33,4 +49,142 @@ >> reg = <0xe8201000 0x1000>, >> <0xe8202000 0x1000>; >> }; >> + >> + pfc: pfc@fcfe3300 { >> + compatible = "renesas,pfc-r7s72100"; >> + reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */ >> + <0xfcfe3a00 0x100>, /* PFCAE */ >> + <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */ >> + <0xfcfe7b40 0x04>, /* JPMC */ >> + <0xfcfe7b90 0x04>, /* JPMCSR */ >> + <0xfcfe7f00 0x04>; /* JPIBC */ >> + }; >> + >> + 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? 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). Please let me know what you think. Cheers, / magnus
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.
Hi Laurent, On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart <laurent.pinchart@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@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 ;-) 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
Hi Magnus (and Linus, as there's a question for your below), On Wednesday 18 December 2013 11:07:15 Magnus Damm wrote: > On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart wrote: > > 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 ;-) > > 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. =) Absolutely, and there's no universal reason why my cleanliness would be better than yours :-) > > 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. You're welcome. > >> 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 agree here, I don't think we need to share RZ_GPIOS_PER_PORT between the two drivers. > 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? I have mixed feelings about this, and understand your concern about complexity. I usually tend to favor correctness over complexity (without reaching the overcomplexity level). In this case I'd like to hear Linus' point of view. If he's fine with your version of the code I'll be fine as well. Is that OK ?
Hi Laurent, On Thu, Dec 19, 2013 at 9:15 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus (and Linus, as there's a question for your below), > > On Wednesday 18 December 2013 11:07:15 Magnus Damm wrote: >> On Wed, Dec 18, 2013 at 9:40 AM, Laurent Pinchart wrote: >> > 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 ;-) >> >> 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. =) > > Absolutely, and there's no universal reason why my cleanliness would be better > than yours :-) I think we should count LOC, just to add a rigged metric to my side. =) >> > 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. > > You're welcome. 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 agree here, I don't think we need to share RZ_GPIOS_PER_PORT between the two > drivers. They have separate name spaces for GPIOS and pins anyway, so yes. >> 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? > > I have mixed feelings about this, and understand your concern about > complexity. I usually tend to favor correctness over complexity (without > reaching the overcomplexity level). In this case I'd like to hear Linus' point > of view. If he's fine with your version of the code I'll be fine as well. Is > that OK ? Sure, that's fine. I may resend the series as-is and see what happens. =) / magnus
--- 0001/arch/arm/boot/dts/r7s72100.dtsi +++ work/arch/arm/boot/dts/r7s72100.dtsi 2013-11-27 16:06:36.000000000 +0900 @@ -14,6 +14,22 @@ #address-cells = <1>; #size-cells = <1>; + aliases { + gpio0 = &port0; + gpio1 = &port1; + gpio2 = &port2; + gpio3 = &port3; + gpio4 = &port4; + gpio5 = &port5; + gpio6 = &port6; + gpio7 = &port7; + gpio8 = &port8; + gpio9 = &port9; + gpio10 = &port10; + gpio11 = &port11; + gpio12 = &jtagport0; + }; + cpus { #address-cells = <1>; #size-cells = <0>; @@ -33,4 +49,142 @@ reg = <0xe8201000 0x1000>, <0xe8202000 0x1000>; }; + + pfc: pfc@fcfe3300 { + compatible = "renesas,pfc-r7s72100"; + reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */ + <0xfcfe3a00 0x100>, /* PFCAE */ + <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */ + <0xfcfe7b40 0x04>, /* JPMC */ + <0xfcfe7b90 0x04>, /* JPMCSR */ + <0xfcfe7f00 0x04>; /* JPIBC */ + }; + + 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>; + }; + + port2: gpio@fcfe3108 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3108 0x4>, /* PSR */ + <0xfcfe3208 0x2>, /* PPR */ + <0xfcfe3808 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 32 16>; + }; + + port3: gpio@fcfe310c { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe310c 0x4>, /* PSR */ + <0xfcfe320c 0x2>, /* PPR */ + <0xfcfe380c 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 48 16>; + }; + + port4: gpio@fcfe3110 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3110 0x4>, /* PSR */ + <0xfcfe3210 0x2>, /* PPR */ + <0xfcfe3810 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 64 16>; + }; + + port5: gpio@fcfe3114 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3114 0x4>, /* PSR */ + <0xfcfe3214 0x2>, /* PPR */ + <0xfcfe3814 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 80 11>; + }; + + port6: gpio@fcfe3118 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3118 0x4>, /* PSR */ + <0xfcfe3218 0x2>, /* PPR */ + <0xfcfe3818 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 96 16>; + }; + + port7: gpio@fcfe311c { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe311c 0x4>, /* PSR */ + <0xfcfe321c 0x2>, /* PPR */ + <0xfcfe381c 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 112 16>; + }; + + port8: gpio@fcfe3120 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3120 0x4>, /* PSR */ + <0xfcfe3220 0x2>, /* PPR */ + <0xfcfe3820 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 128 16>; + }; + + port9: gpio@fcfe3124 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3124 0x4>, /* PSR */ + <0xfcfe3224 0x2>, /* PPR */ + <0xfcfe3824 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 144 8>; + }; + + port10: gpio@fcfe3128 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe3128 0x4>, /* PSR */ + <0xfcfe3228 0x2>, /* PPR */ + <0xfcfe3828 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 160 16>; + }; + + port11: gpio@fcfe312c { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe312c 0x4>, /* PSR */ + <0xfcfe322c 0x2>, /* PPR */ + <0xfcfe382c 0x4>; /* PMSR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 176 16>; + }; + + jtagport0: gpio@fcfe7b20 { + compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz"; + reg = <0xfcfe7b20 0x2>; /* JPPR */ + #gpio-cells = <2>; + gpio-controller; + gpio-ranges = <&pfc 0 192 2>; + }; };