| Message ID | 1485367787-8109-5-git-send-email-jacopo+renesas@jmondi.org |
|---|---|
| State | New |
| Headers | show |
Hello!
s/r7s1000/r7s72100/ in the subject.
On 1/25/2017 9:09 PM, Jacopo Mondi wrote:
> Add pincontroller node compatible with the new Renesas RZ/A1
> pincontroller driver.
Pin controller.
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
> index 3dd427d..764006d 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -171,6 +171,18 @@
> };
> };
>
> + pinctrl: pinctrl@fcfe3000 {
I'd call the node "pin-controller@...".
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add pincontroller node compatible with the new Renesas RZ/A1 > pincontroller driver. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index 3dd427d..764006d 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -171,6 +171,18 @@ > }; > }; > > + pinctrl: pinctrl@fcfe3000 { > + compatible = "renesas,rza1-pinctrl"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + #pinctrl-cells = <2>; Souldn't that be <3>? E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacopo, Thank you for the patch. On Wednesday 25 Jan 2017 19:09:46 Jacopo Mondi wrote: > Add pincontroller node compatible with the new Renesas RZ/A1 > pincontroller driver. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi > b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..764006d 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -171,6 +171,18 @@ > }; > }; > > + pinctrl: pinctrl@fcfe3000 { > + compatible = "renesas,rza1-pinctrl"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + #pinctrl-cells = <2>; > + > + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ What's the reason for splitting those registers in two sets ? Maybe you can explain that in the DT bindings documentation that this patch series is missing ;-) > + <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */ s/B/b/ > + }; > + > scif0: serial@e8007000 { > compatible = "renesas,scif-r7s72100", "renesas,scif"; > reg = <0xe8007000 64>;
Hi Jacopo, On Monday, January 30, 2017, Laurent Pinchart wrote: > > + pinctrl: pinctrl@fcfe3000 { > > + compatible = "renesas,rza1-pinctrl"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + #pinctrl-cells = <2>; > > + > > + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > > + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > What's the reason for splitting those registers in two sets ? Maybe you > can explain that in the DT bindings documentation that this patch series > is missing ;-) I left this out of my review comments, but even though the chip designers left a BIG HOLE in the memory map of the PFC controller, I don't think it will 'cost' you anything by just mapping the whole area (dead space and all) and getting rid of the "high and low" memory indexing thing that you are doing in the driver. There is nothing mapped in that dead area anyway. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 26/01/2017 20:54, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: >> Add pincontroller node compatible with the new Renesas RZ/A1 >> pincontroller driver. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> --- >> arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi >> index 3dd427d..764006d 100644 >> --- a/arch/arm/boot/dts/r7s72100.dtsi >> +++ b/arch/arm/boot/dts/r7s72100.dtsi >> @@ -171,6 +171,18 @@ >> }; >> }; >> >> + pinctrl: pinctrl@fcfe3000 { >> + compatible = "renesas,rza1-pinctrl"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + #pinctrl-cells = <2>; > > Souldn't that be <3>? > E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. > From pinctrl-bindings.txt: #pinctrl-cells: Number of pin control cells in addition to the index within the pin controller device instance So here it's (2 + 1) as at least the pin index (first parameter) is mandatory. We're twisting the assumption of having "index" as first, single, parameter, as we have <bank pin mode> and the <bank, pin> pair identifies a pin. Hope this is ok, and we can re-use existing bindings even if our semantic is a bit different. > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacopo, On Tue, Jan 31, 2017 at 11:24 AM, jacopo mondi <jacopo@jmondi.org> wrote: > On 26/01/2017 20:54, Geert Uytterhoeven wrote: >> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> >> wrote: >>> >>> Add pincontroller node compatible with the new Renesas RZ/A1 >>> pincontroller driver. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/r7s72100.dtsi >>> b/arch/arm/boot/dts/r7s72100.dtsi >>> index 3dd427d..764006d 100644 >>> --- a/arch/arm/boot/dts/r7s72100.dtsi >>> +++ b/arch/arm/boot/dts/r7s72100.dtsi >>> @@ -171,6 +171,18 @@ >>> }; >>> }; >>> >>> + pinctrl: pinctrl@fcfe3000 { >>> + compatible = "renesas,rza1-pinctrl"; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + #pinctrl-cells = <2>; >> >> >> Souldn't that be <3>? >> E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. >> > > From pinctrl-bindings.txt: > > #pinctrl-cells: Number of pin control cells in addition to the index within > the pin controller device instance IC. I incorrectly assumed it would be the number of cells after a pinctrl phandle, which is superfluous here as these are subnodes of the pfc node. > So here it's (2 + 1) as at least the pin index (first parameter) is > mandatory. > > We're twisting the assumption of having "index" as first, single, parameter, > as we have <bank pin mode> and the <bank, pin> pair identifies a pin. > > Hope this is ok, and we can re-use existing bindings even if our semantic is > a bit different. That's indeed a bit of twisting ;-) You can fix that by keeping the RZ_PIN() macro, and changing it to e.g. #define RZ_PIN(bank, pin) ((bank) << 16 | (pin)) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chris, On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote: > On Monday, January 30, 2017, Laurent Pinchart wrote: > >> + pinctrl: pinctrl@fcfe3000 { > >> + compatible = "renesas,rza1-pinctrl"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + #pinctrl-cells = <2>; > >> + > >> + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > >> + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > > > What's the reason for splitting those registers in two sets ? Maybe you > > can explain that in the DT bindings documentation that this patch series > > is missing ;-) > > I left this out of my review comments, but even though the chip designers > left a BIG HOLE in the memory map of the PFC controller, I don't think it > will 'cost' you anything by just mapping the whole area (dead space and > all) and getting rid of the "high and low" memory indexing thing that you > are doing in the driver. > There is nothing mapped in that dead area anyway. For the first two areas, I agree. The third area is a separate pin controller for the JTAG port, not multiplexed with the GPIO ports. I even wonder whether it should be split in a separate DT node.
Hi Laurent (and Jacopo) On Tuesday, January 31, 2017, Laurent Pinchart wrote: > On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote: > > On Monday, January 30, 2017, Laurent Pinchart wrote: > > >> + pinctrl: pinctrl@fcfe3000 { > > >> + compatible = "renesas,rza1-pinctrl"; > > >> + #address-cells = <1>; > > >> + #size-cells = <0>; > > >> + > > >> + #pinctrl-cells = <2>; > > >> + > > >> + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > > >> + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > > > > > What's the reason for splitting those registers in two sets ? Maybe > > > you can explain that in the DT bindings documentation that this > > > patch series is missing ;-) > > > > I left this out of my review comments, but even though the chip > > designers left a BIG HOLE in the memory map of the PFC controller, I > > don't think it will 'cost' you anything by just mapping the whole area > > (dead space and > > all) and getting rid of the "high and low" memory indexing thing that > > you are doing in the driver. > > There is nothing mapped in that dead area anyway. > > For the first two areas, I agree. The third area is a separate pin > controller for the JTAG port, not multiplexed with the GPIO ports. I even > wonder whether it should be split in a separate DT node. I think we should just forget about the JTAG pins completely. Honestly, they are 2 pins that can only be configured as JTAG or a GPIO input (not even output). No one is using those pins for anything other than JTAG. IMHO, adding extra support just for those 2 pins is basically a waste of code. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..764006d 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -171,6 +171,18 @@ }; }; + pinctrl: pinctrl@fcfe3000 { + compatible = "renesas,rza1-pinctrl"; + #address-cells = <1>; + #size-cells = <0>; + + #pinctrl-cells = <2>; + + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ + <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */ + }; + scif0: serial@e8007000 { compatible = "renesas,scif-r7s72100", "renesas,scif"; reg = <0xe8007000 64>;
Add pincontroller node compatible with the new Renesas RZ/A1 pincontroller driver. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)