diff mbox

[v4,06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

Message ID 20170402204244.14216-7-paul@crapouillou.net (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Cercueil April 2, 2017, 8:42 p.m. UTC
For a description of the pinctrl devicetree node, please read
Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt

For a description of the gpio devicetree nodes, please read
Documentation/devicetree/bindings/gpio/ingenic,gpio.txt

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4740.dtsi | 61 ++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

 v2: Changed the devicetree bindings to match the new driver
 v3: No changes
 v4: Update the bindings for the v4 version of the drivers

Comments

Sergei Shtylyov April 3, 2017, 9:57 a.m. UTC | #1
Hello!

On 4/2/2017 11:42 PM, Paul Cercueil wrote:

> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
[...]

> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index 3e1587f1f77a..9c23c877fc34 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -55,6 +55,67 @@
>  		clock-names = "rtc";
>  	};
>
> +	pinctrl: ingenic-pinctrl@10010000 {

    The node name should be generic, so please rename it to something like 
"pin-controller@..."

> +		compatible = "ingenic,jz4740-pinctrl";
> +		reg = <0x10010000 0x400>;
> +
> +		gpa: gpio-controller@0 {

    The name should be just "gpio@0", according to ePAPR and its successor 
spec. Although, using the <unit-address> without the "reg" prop isn't allowed 
either...

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil April 3, 2017, 10:20 a.m. UTC | #2
Hi,

Le 2017-04-03 11:57, Sergei Shtylyov a écrit :
> Hello!
> 
> On 4/2/2017 11:42 PM, Paul Cercueil wrote:
> 
>> For a description of the pinctrl devicetree node, please read
>> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>> 
>> For a description of the gpio devicetree nodes, please read
>> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> [...]
> 
>> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> index 3e1587f1f77a..9c23c877fc34 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> @@ -55,6 +55,67 @@
>>  		clock-names = "rtc";
>>  	};
>> 
>> +	pinctrl: ingenic-pinctrl@10010000 {
> 
>    The node name should be generic, so please rename it to something
> like "pin-controller@..."

OK.

>> +		compatible = "ingenic,jz4740-pinctrl";
>> +		reg = <0x10010000 0x400>;
>> +
>> +		gpa: gpio-controller@0 {
> 
>    The name should be just "gpio@0", according to ePAPR and its
> successor spec. Although, using the <unit-address> without the "reg"
> prop isn't allowed either...

ePAPR says: "If the node has no reg property, the unit-address may be
omitted if the node name alone differentiates the node from other nodes 
at
the same level in the tree."

I could use 'gpio@bank-a', it is allowed by the spec. Or do you prefer 
'gpio@0'?

> MBR, Sergei

I'll wait from some feedback on the other patches then send a v5.

Thanks,
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov April 3, 2017, 10:32 a.m. UTC | #3
On 4/3/2017 1:20 PM, Paul Cercueil wrote:

>>> For a description of the pinctrl devicetree node, please read
>>> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>>>
>>> For a description of the gpio devicetree nodes, please read
>>> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> [...]
>>
>>> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> index 3e1587f1f77a..9c23c877fc34 100644
>>> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> @@ -55,6 +55,67 @@
>>>          clock-names = "rtc";
>>>      };
>>>
>>> +    pinctrl: ingenic-pinctrl@10010000 {
>>
>>    The node name should be generic, so please rename it to something
>> like "pin-controller@..."
>
> OK.
>
>>> +        compatible = "ingenic,jz4740-pinctrl";
>>> +        reg = <0x10010000 0x400>;
>>> +
>>> +        gpa: gpio-controller@0 {
>>
>>    The name should be just "gpio@0", according to ePAPR and its
>> successor spec. Although, using the <unit-address> without the "reg"
>> prop isn't allowed either...
>
> ePAPR says: "If the node has no reg property, the unit-address may be

    My copy of ePAPR 1.1 says "must be omitted". :-)

> omitted if the node name alone differentiates the node from other nodes at
> the same level in the tree."

> I could use 'gpio@bank-a', it is allowed by the spec. Or do you prefer 'gpio@0'?

    Hm... maybe you should just use the "reg" prop.

> I'll wait from some feedback on the other patches then send a v5.
>
> Thanks,
> -Paul

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 7, 2017, 9:44 a.m. UTC | #4
On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 61 ++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
>  v2: Changed the devicetree bindings to match the new driver
>  v3: No changes
>  v4: Update the bindings for the v4 version of the drivers
(...)

> +       pinctrl: ingenic-pinctrl@10010000 {
> +               compatible = "ingenic,jz4740-pinctrl";
> +               reg = <0x10010000 0x400>;
> +
> +               gpa: gpio-controller@0 {
> +                       compatible = "ingenic,gpio-bank-a", "ingenic,jz4740-gpio";

As Sergei and Rob notes, the bank compatible properties look
a bit strange. Especially if they are all the same essentially.

I like Sergei's idea to simply use the reg property if what you want
is really a unique ID number. What do you think about this?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil April 7, 2017, 1:57 p.m. UTC | #5
Le 2017-04-07 11:44, Linus Walleij a écrit :
> On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
> 
>> For a description of the pinctrl devicetree node, please read
>> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>> 
>> For a description of the gpio devicetree nodes, please read
>> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 61 
>> ++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>> 
>>  v2: Changed the devicetree bindings to match the new driver
>>  v3: No changes
>>  v4: Update the bindings for the v4 version of the drivers
> (...)
> 
>> +       pinctrl: ingenic-pinctrl@10010000 {
>> +               compatible = "ingenic,jz4740-pinctrl";
>> +               reg = <0x10010000 0x400>;
>> +
>> +               gpa: gpio-controller@0 {
>> +                       compatible = "ingenic,gpio-bank-a", 
>> "ingenic,jz4740-gpio";
> 
> As Sergei and Rob notes, the bank compatible properties look
> a bit strange. Especially if they are all the same essentially.
> 
> I like Sergei's idea to simply use the reg property if what you want
> is really a unique ID number. What do you think about this?
> 
> Yours,
> Linus Walleij

I think the 'reg' property makes more sense, yes. I'll fix this in the 
v5, this
week-end. Do you think it can go in 4.12?

Thanks,
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 24, 2017, 12:58 p.m. UTC | #6
On Fri, Apr 7, 2017 at 3:57 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> I think the 'reg' property makes more sense, yes. I'll fix this in the v5,
> this
> week-end. Do you think it can go in 4.12?

Surely, Torvalds just cut an -rc8 giving me more time to queue more
material, and I really like this series.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 3e1587f1f77a..9c23c877fc34 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -55,6 +55,67 @@ 
 		clock-names = "rtc";
 	};
 
+	pinctrl: ingenic-pinctrl@10010000 {
+		compatible = "ingenic,jz4740-pinctrl";
+		reg = <0x10010000 0x400>;
+
+		gpa: gpio-controller@0 {
+			compatible = "ingenic,gpio-bank-a", "ingenic,jz4740-gpio";
+
+			gpio-controller;
+			gpio-ranges = <&pinctrl 0 0 32>;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+
+			interrupt-parent = <&intc>;
+			interrupts = <28>;
+		};
+
+		gpb: gpio-controller@1 {
+			compatible = "ingenic,gpio-bank-b", "ingenic,jz4740-gpio";
+
+			gpio-controller;
+			gpio-ranges = <&pinctrl 0 32 32>;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+
+			interrupt-parent = <&intc>;
+			interrupts = <27>;
+		};
+
+		gpc: gpio-controller@2 {
+			compatible = "ingenic,gpio-bank-c", "ingenic,jz4740-gpio";
+
+			gpio-controller;
+			gpio-ranges = <&pinctrl 0 64 32>;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+
+			interrupt-parent = <&intc>;
+			interrupts = <26>;
+		};
+
+		gpd: gpio-controller@3 {
+			compatible = "ingenic,gpio-bank-d", "ingenic,jz4740-gpio";
+
+			gpio-controller;
+			gpio-ranges = <&pinctrl 0 96 32>;
+			#gpio-cells = <2>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+
+			interrupt-parent = <&intc>;
+			interrupts = <25>;
+		};
+	};
+
 	uart0: serial@10030000 {
 		compatible = "ingenic,jz4740-uart";
 		reg = <0x10030000 0x100>;