[RFC,4/5] arm: dts: r7s1000: Add pincontroller node
diff mbox

Message ID 1485367787-8109-5-git-send-email-jacopo+renesas@jmondi.org
State New
Headers show

Commit Message

Jacopo Mondi Jan. 25, 2017, 6:09 p.m. UTC
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(+)

Comments

Sergei Shtylyov Jan. 26, 2017, 9:49 a.m. UTC | #1
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
Geert Uytterhoeven Jan. 26, 2017, 7:54 p.m. UTC | #2
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
Laurent Pinchart Jan. 30, 2017, 6:29 p.m. UTC | #3
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>;
Chris Brandt Jan. 30, 2017, 7:39 p.m. UTC | #4
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
Jacopo Mondi Jan. 31, 2017, 10:24 a.m. UTC | #5
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
Geert Uytterhoeven Jan. 31, 2017, 10:34 a.m. UTC | #6
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
Laurent Pinchart Jan. 31, 2017, 10:35 a.m. UTC | #7
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.
Chris Brandt Jan. 31, 2017, 2:08 p.m. UTC | #8
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

Patch
diff mbox

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