[PATCH] pinctrl: dt: at91: new binding

[PATCH] pinctrl: dt: at91: new binding

[Jean-Christophe PLAGNIOL-VILLARD]

Today if we want to disable a pio bank we may will siliently break pinctrl
configuration in the DT. This will be detected only at runtime.

So move the pinctrl configuration to the bank instead of the bus.
This allow to detect pinctrl issue at DT compiling time when disable a bank.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
index b7a93e8..78355ee 100644
--- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
@@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
 	pinctrl-0 = <&pinctrl_dbgu>;
 	status = "disabled";
 };
+
+II) New Bindings per PIO Block
+
+This allow to detect pinctrl issue at DT compiling time when disable a bank
+
+Required properties for iomux controller:
+- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl"
+		or "atmel,sama5d3-pio-pinctrl"
+- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
+  configured in this periph mode. All the periph and bank need to be describe.
+
+How to create such array:
+
+Each column will represent the possible peripheral of the pinctrl for the bank
+
+Take an example on the 9260
+Peripheral: 2 ( A and B)
+=>
+
+  /*    A         B     */
+  0xffffffff 0xffc00c3b  /* pioA */
+
+For each peripheral/bank we will descibe in a u32 if a pin can be
+configured in it by putting 1 to the pin bit (1 << pin)
+
+Required properties for pin configuration node:
+- atmel,pins: 3 integers array, represents a group of pins mux and config
+  setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>.
+  The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
+
+Bits used for CONFIG:
+cf atmel,at91-pinctrl
+
+pioB: gpio@fffff600 {
+	compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl";
+	reg = <0xfffff600 0x200>;
+	interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
+	#gpio-cells = <2>;
+	gpio-controller;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	clocks = <&pioB_clk>;
+
+			 /*    A         B     */
+	atmel,mux-mask = <0xffffffff 0x7fff3ccf>;
+
+	dbgu {
+		pinctrl_dbgu: dbgu-0 {
+			atmel,pins =
+				<14 0x1 0x0	/* PB14 periph A */
+				 15 0x1 0x1>;	/* PB15 periph A with pullup */
+		};
+	};
+};
+
+dbgu: serial@fffff200 {
+	compatible = "atmel,at91sam9260-usart";
+	reg = <0xfffff200 0x200>;
+	interrupts = <1 4 7>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_dbgu>;
+	status = "disabled";
+};
+
+if you have to use multiple bank
+	pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>;
-- 
2.1.4

Re: [PATCH] pinctrl: dt: at91: new binding

[Ludovic Desroches]

Hi Jean-Christophe,

On Thu, Feb 26, 2015 at 10:34:54AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Today if we want to disable a pio bank we may will siliently break pinctrl
> configuration in the DT. This will be detected only at runtime.

Do you have a case where it breaks pinctrl? I can do more tests but with
the latest patch "pinctrl: at91: allow to have disabled gpio bank", I
had no issue.

> 
> So move the pinctrl configuration to the bank instead of the bus.
> This allow to detect pinctrl issue at DT compiling time when disable a bank.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> index b7a93e8..78355ee 100644
> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
>  	pinctrl-0 = <&pinctrl_dbgu>;
>  	status = "disabled";
>  };
> +
> +II) New Bindings per PIO Block
> +
> +This allow to detect pinctrl issue at DT compiling time when disable a bank
> +
> +Required properties for iomux controller:
> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl"
> +		or "atmel,sama5d3-pio-pinctrl"
> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
> +  configured in this periph mode. All the periph and bank need to be describe.
> +
> +How to create such array:
> +
> +Each column will represent the possible peripheral of the pinctrl for the bank
> +
> +Take an example on the 9260
> +Peripheral: 2 ( A and B)
> +=>
> +
> +  /*    A         B     */
> +  0xffffffff 0xffc00c3b  /* pioA */
> +
> +For each peripheral/bank we will descibe in a u32 if a pin can be
> +configured in it by putting 1 to the pin bit (1 << pin)
> +
> +Required properties for pin configuration node:
> +- atmel,pins: 3 integers array, represents a group of pins mux and config
> +  setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>.
> +  The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
> +
> +Bits used for CONFIG:
> +cf atmel,at91-pinctrl
> +
> +pioB: gpio@fffff600 {
> +	compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl";
> +	reg = <0xfffff600 0x200>;
> +	interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
> +	#gpio-cells = <2>;
> +	gpio-controller;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	clocks = <&pioB_clk>;
> +
> +			 /*    A         B     */
> +	atmel,mux-mask = <0xffffffff 0x7fff3ccf>;
> +
> +	dbgu {
> +		pinctrl_dbgu: dbgu-0 {
> +			atmel,pins =
> +				<14 0x1 0x0	/* PB14 periph A */
> +				 15 0x1 0x1>;	/* PB15 periph A with pullup */
> +		};
> +	};

So we have to update all device tree files, that's a lot of work...
Moreover it adds complexity if we have a device using pins from
different pio controllers.

> +};
> +
> +dbgu: serial@fffff200 {
> +	compatible = "atmel,at91sam9260-usart";
> +	reg = <0xfffff200 0x200>;
> +	interrupts = <1 4 7>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_dbgu>;
> +	status = "disabled";
> +};
> +
> +if you have to use multiple bank
> +	pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>;

You mean pio I think.


Ludovic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: dt: at91: new binding

[Jean-Christophe PLAGNIOL-VILLARD]

> On Feb 26, 2015, at 6:43 PM, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
> 
> Hi Jean-Christophe,
> 
> On Thu, Feb 26, 2015 at 10:34:54AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> Today if we want to disable a pio bank we may will siliently break pinctrl
>> configuration in the DT. This will be detected only at runtime.
> 
> Do you have a case where it breaks pinctrl? I can do more tests but with
> the latest patch "pinctrl: at91: allow to have disabled gpio bank", I
> had no issue.

simple disable the PIO bank and if one device use it as pinctrl and no gpio is used in the DT
then you get a broken platform but dtb is generated

as all the node are enabled for pinctrl

> 
>> 
>> So move the pinctrl configuration to the bank instead of the bus.
>> This allow to detect pinctrl issue at DT compiling time when disable a bank.
>> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
>> 1 file changed, 66 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> index b7a93e8..78355ee 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
>> 	pinctrl-0 = <&pinctrl_dbgu>;
>> 	status = "disabled";
>> };
>> +
>> +II) New Bindings per PIO Block
>> +
>> +This allow to detect pinctrl issue at DT compiling time when disable a bank
>> +
>> +Required properties for iomux controller:
>> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl"
>> +		or "atmel,sama5d3-pio-pinctrl"
>> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
>> +  configured in this periph mode. All the periph and bank need to be describe.
>> +
>> +How to create such array:
>> +
>> +Each column will represent the possible peripheral of the pinctrl for the bank
>> +
>> +Take an example on the 9260
>> +Peripheral: 2 ( A and B)
>> +=>
>> +
>> +  /*    A         B     */
>> +  0xffffffff 0xffc00c3b  /* pioA */
>> +
>> +For each peripheral/bank we will descibe in a u32 if a pin can be
>> +configured in it by putting 1 to the pin bit (1 << pin)
>> +
>> +Required properties for pin configuration node:
>> +- atmel,pins: 3 integers array, represents a group of pins mux and config
>> +  setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>.
>> +  The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
>> +
>> +Bits used for CONFIG:
>> +cf atmel,at91-pinctrl
>> +
>> +pioB: gpio@fffff600 {
>> +	compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl";
>> +	reg = <0xfffff600 0x200>;
>> +	interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
>> +	#gpio-cells = <2>;
>> +	gpio-controller;
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	clocks = <&pioB_clk>;
>> +
>> +			 /*    A         B     */
>> +	atmel,mux-mask = <0xffffffff 0x7fff3ccf>;
>> +
>> +	dbgu {
>> +		pinctrl_dbgu: dbgu-0 {
>> +			atmel,pins =
>> +				<14 0x1 0x0	/* PB14 periph A */
>> +				 15 0x1 0x1>;	/* PB15 periph A with pullup */
>> +		};
>> +	};
> 
> So we have to update all device tree files, that's a lot of work…
break nothing both binding support will be in the kernel

but only one enable at a time, so basically mainline will drop the old one

and as today 99% of the pinctrl are at SoC level so it’s not a big work at all
> Moreover it adds complexity if we have a device using pins from
> different pio controllers.

put 2 phandle means complexity please

and it’s the key point of the new binding

Best Regards,
J.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pinctrl: dt: at91: new binding

[Nicolas Ferre]

Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> Today if we want to disable a pio bank we may will siliently break pinctrl
> configuration in the DT. This will be detected only at runtime.
> 
> So move the pinctrl configuration to the bank instead of the bus.
> This allow to detect pinctrl issue at DT compiling time when disable a bank.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTDQFizaE/u3fw@public.gmane.orgm>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> index b7a93e8..78355ee 100644
> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
>  	pinctrl-0 = <&pinctrl_dbgu>;
>  	status = "disabled";
>  };
> +
> +II) New Bindings per PIO Block

Sorry but NACK.

I don't want to manage another flavor of the pinmux biding with no real
benefit. I would have been good if we had it from day-1. Now it's too late.

Moreover, splitting a binding definition if you have a function given by
multiple banks can be weird and not well understood in regard to our
current group+function definition scheme (Cf. your last example).


> +This allow to detect pinctrl issue at DT compiling time when disable a bank
> +
> +Required properties for iomux controller:
> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl"
> +		or "atmel,sama5d3-pio-pinctrl"
> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
> +  configured in this periph mode. All the periph and bank need to be describe.
> +
> +How to create such array:
> +
> +Each column will represent the possible peripheral of the pinctrl for the bank
> +
> +Take an example on the 9260
> +Peripheral: 2 ( A and B)
> +=>
> +
> +  /*    A         B     */
> +  0xffffffff 0xffc00c3b  /* pioA */
> +
> +For each peripheral/bank we will descibe in a u32 if a pin can be
> +configured in it by putting 1 to the pin bit (1 << pin)
> +
> +Required properties for pin configuration node:
> +- atmel,pins: 3 integers array, represents a group of pins mux and config
> +  setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>.
> +  The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
> +
> +Bits used for CONFIG:
> +cf atmel,at91-pinctrl
> +
> +pioB: gpio@fffff600 {
> +	compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl";
> +	reg = <0xfffff600 0x200>;
> +	interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
> +	#gpio-cells = <2>;
> +	gpio-controller;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	clocks = <&pioB_clk>;
> +
> +			 /*    A         B     */
> +	atmel,mux-mask = <0xffffffff 0x7fff3ccf>;
> +
> +	dbgu {
> +		pinctrl_dbgu: dbgu-0 {
> +			atmel,pins =
> +				<14 0x1 0x0	/* PB14 periph A */
> +				 15 0x1 0x1>;	/* PB15 periph A with pullup */
> +		};
> +	};
> +};
> +
> +dbgu: serial@fffff200 {
> +	compatible = "atmel,at91sam9260-usart";
> +	reg = <0xfffff200 0x200>;
> +	interrupts = <1 4 7>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_dbgu>;
> +	status = "disabled";
> +};
> +
> +if you have to use multiple bank
> +	pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>;
> 

Best regards,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: dt: at91: new binding

[Jean-Christophe PLAGNIOL-VILLARD]

> On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
> Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit :
>> Today if we want to disable a pio bank we may will siliently break pinctrl
>> configuration in the DT. This will be detected only at runtime.
>> 
>> So move the pinctrl configuration to the bank instead of the bus.
>> This allow to detect pinctrl issue at DT compiling time when disable a bank.
>> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
>> 1 file changed, 66 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> index b7a93e8..78355ee 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
>> 	pinctrl-0 = <&pinctrl_dbgu>;
>> 	status = "disabled";
>> };
>> +
>> +II) New Bindings per PIO Block
> 
> Sorry but NACK.
> 
> I don't want to manage another flavor of the pinmux biding with no real
> benefit. I would have been good if we had it from day-1. Now it's too late.

yes we do, we catch but a compiling time instead of RUNTIME which is critical

so I’ll pass on the NACK

> 
> Moreover, splitting a binding definition if you have a function given by
> multiple banks can be weird and not well understood in regard to our
> current group+function definition scheme (Cf. your last example).
> 

Others already do so and this is not complex at all

Best Regards,
J.

> 
>> +This allow to detect pinctrl issue at DT compiling time when disable a bank
>> +
>> +Required properties for iomux controller:
>> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl"
>> +		or "atmel,sama5d3-pio-pinctrl"
>> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
>> +  configured in this periph mode. All the periph and bank need to be describe.
>> +
>> +How to create such array:
>> +
>> +Each column will represent the possible peripheral of the pinctrl for the bank
>> +
>> +Take an example on the 9260
>> +Peripheral: 2 ( A and B)
>> +=>
>> +
>> +  /*    A         B     */
>> +  0xffffffff 0xffc00c3b  /* pioA */
>> +
>> +For each peripheral/bank we will descibe in a u32 if a pin can be
>> +configured in it by putting 1 to the pin bit (1 << pin)
>> +
>> +Required properties for pin configuration node:
>> +- atmel,pins: 3 integers array, represents a group of pins mux and config
>> +  setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>.
>> +  The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
>> +
>> +Bits used for CONFIG:
>> +cf atmel,at91-pinctrl
>> +
>> +pioB: gpio@fffff600 {
>> +	compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl";
>> +	reg = <0xfffff600 0x200>;
>> +	interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
>> +	#gpio-cells = <2>;
>> +	gpio-controller;
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	clocks = <&pioB_clk>;
>> +
>> +			 /*    A         B     */
>> +	atmel,mux-mask = <0xffffffff 0x7fff3ccf>;
>> +
>> +	dbgu {
>> +		pinctrl_dbgu: dbgu-0 {
>> +			atmel,pins =
>> +				<14 0x1 0x0	/* PB14 periph A */
>> +				 15 0x1 0x1>;	/* PB15 periph A with pullup */
>> +		};
>> +	};
>> +};
>> +
>> +dbgu: serial@fffff200 {
>> +	compatible = "atmel,at91sam9260-usart";
>> +	reg = <0xfffff200 0x200>;
>> +	interrupts = <1 4 7>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_dbgu>;
>> +	status = "disabled";
>> +};
>> +
>> +if you have to use multiple bank
>> +	pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>;
>> 
> 
> Best regards,
> -- 
> Nicolas Ferre
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pinctrl: dt: at91: new binding

[Alexandre Belloni]

On 07/03/2015 at 00:49:55 +0800, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > 
> > Sorry but NACK.
> > 
> > I don't want to manage another flavor of the pinmux biding with no real
> > benefit. I would have been good if we had it from day-1. Now it's too late.
> 
> yes we do, we catch but a compiling time instead of RUNTIME which is critical
> 
> so I’ll pass on the NACK
> 

If you are changing the binding, how about doing it right this time and
completely drop the current mess?

> > 
> > Moreover, splitting a binding definition if you have a function given by
> > multiple banks can be weird and not well understood in regard to our
> > current group+function definition scheme (Cf. your last example).
> > 
> 
> Others already do so and this is not complex at all
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: dt: at91: new binding

[Boris Brezillon]

Hi Jean-Christophe,

On Sat, 7 Mar 2015 00:49:55 +0800
Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:

> 
> > On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> >> Today if we want to disable a pio bank we may will siliently break pinctrl
> >> configuration in the DT. This will be detected only at runtime.
> >> 
> >> So move the pinctrl configuration to the bank instead of the bus.
> >> This allow to detect pinctrl issue at DT compiling time when disable a bank.
> >> 
> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> >> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> ---
> >> .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
> >> 1 file changed, 66 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >> index b7a93e8..78355ee 100644
> >> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
> >> 	pinctrl-0 = <&pinctrl_dbgu>;
> >> 	status = "disabled";
> >> };
> >> +
> >> +II) New Bindings per PIO Block
> > 
> > Sorry but NACK.
> > 
> > I don't want to manage another flavor of the pinmux biding with no real
> > benefit. I would have been good if we had it from day-1. Now it's too late.
> 
> yes we do, we catch but a compiling time instead of RUNTIME which is critical
> 
> so I’ll pass on the NACK

Tell me, how can you pass on a NACK coming from the at91 maintainer
(which is also your co-maintainer) when you modify bindings of an at91
driver ?
Please let's try to be constructive here, so that we can find an
acceptable solution.

> 
> > 
> > Moreover, splitting a binding definition if you have a function given by
> > multiple banks can be weird and not well understood in regard to our
> > current group+function definition scheme (Cf. your last example).
> > 

I don't think it's a good idea either: you'll have to split pinconf
definitions and that definitely doesn't improve readability.

> 
> Others already do so and this is not complex at all

Could you point out these bindings (and real examples please).

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: dt: at91: new binding

[Jean-Christophe PLAGNIOL-VILLARD]

> On Mar 7, 2015, at 2:23 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> Hi Jean-Christophe,
> 
> On Sat, 7 Mar 2015 00:49:55 +0800
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> 
>> 
>>> On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>>> 
>>> Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit :
>>>> Today if we want to disable a pio bank we may will siliently break pinctrl
>>>> configuration in the DT. This will be detected only at runtime.
>>>> 
>>>> So move the pinctrl configuration to the bank instead of the bus.
>>>> This allow to detect pinctrl issue at DT compiling time when disable a bank.
>>>> 
>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: devicetree@vger.kernel.org
>>>> ---
>>>> .../bindings/pinctrl/atmel,at91-pinctrl.txt        | 66 ++++++++++++++++++++++
>>>> 1 file changed, 66 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>>>> index b7a93e8..78355ee 100644
>>>> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>>>> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 {
>>>> 	pinctrl-0 = <&pinctrl_dbgu>;
>>>> 	status = "disabled";
>>>> };
>>>> +
>>>> +II) New Bindings per PIO Block
>>> 
>>> Sorry but NACK.
>>> 
>>> I don't want to manage another flavor of the pinmux biding with no real
>>> benefit. I would have been good if we had it from day-1. Now it's too late.
>> 
>> yes we do, we catch but a compiling time instead of RUNTIME which is critical
>> 
>> so I’ll pass on the NACK
> 
> Tell me, how can you pass on a NACK coming from the at91 maintainer
> (which is also your co-maintainer) when you modify bindings of an at91
> driver ?
> Please let's try to be constructive here, so that we can find an
> acceptable solution.

I do pass on it which means, I do not accept to stop here the discussion because of this
as Nicolas did for dropping the soc detection when I NACK
> 
>> 
>>> 
>>> Moreover, splitting a binding definition if you have a function given by
>>> multiple banks can be weird and not well understood in regard to our
>>> current group+function definition scheme (Cf. your last example).
>>> 
> 
> I don't think it's a good idea either: you'll have to split pinconf
> definitions and that definitely doesn't improve readability.

in HW it’s already the CASE. And today disable a bank you BROKE a board without
even known it. This is NOT acceptable when we can detect it at compiling TIME.
> 
>> 
>> Others already do so and this is not complex at all
> 
> Could you point out these bindings (and real examples please).

look at ST-E 9500 pinctrl they DO use it


> 
> Best Regards,
> 
> Boris
> 
> 
> -- 
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pinctrl: dt: at91: new binding

[Linus Walleij]

On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:

> +For each peripheral/bank we will descibe in a u32 if a pin can be
> +configured in it by putting 1 to the pin bit (1 << pin)

This seems to be describing driver intrinsics in the device tree, like
how the hardware is routed on the inside and what it can do.

IMO that is driver territory, the driver should know these limitations
and protest if you try to do something illegal.

Anyway as the AT91 maintainers seem to disagree I will allow some
more time for discussion before merging the patch.

I can't really have one AT91 maintainer NACKing another, it doesn't
matter that this is a separate driver, in my book the MAINTAINERS
entry for AT91 as a whole overrides that so can you please find an
agreement on how to handle this or I will stall the patch until
you're in agreement.

ARM SoC maintainers input would be welcomed.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: dt: at91: new binding

[Olof Johansson]

On Tue, Mar 17, 2015 at 5:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
>
>> +For each peripheral/bank we will descibe in a u32 if a pin can be
>> +configured in it by putting 1 to the pin bit (1 << pin)
>
> This seems to be describing driver intrinsics in the device tree, like
> how the hardware is routed on the inside and what it can do.
>
> IMO that is driver territory, the driver should know these limitations
> and protest if you try to do something illegal.
>
> Anyway as the AT91 maintainers seem to disagree I will allow some
> more time for discussion before merging the patch.
>
> I can't really have one AT91 maintainer NACKing another, it doesn't
> matter that this is a separate driver, in my book the MAINTAINERS
> entry for AT91 as a whole overrides that so can you please find an
> agreement on how to handle this or I will stall the patch until
> you're in agreement.

Nicolas has been the de-facto maintainer of AT91 for quite a while
now, even though more of them are listed on the maintainers entry. It
would be inappropriate to merge something that he disagreed with on
that platform.

> ARM SoC maintainers input would be welcomed.

It seems appropriate to ask the at91 folks to come back with a
solution that everybody is OK with, and until then hold off merging
this.


-Olof

Re: [PATCH] pinctrl: dt: at91: new binding

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:55 Sun 29 Mar     , Olof Johansson wrote:
> On Tue, Mar 17, 2015 at 5:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD
> > <plagnioj@jcrosoft.com> wrote:
> >
> >> +For each peripheral/bank we will descibe in a u32 if a pin can be
> >> +configured in it by putting 1 to the pin bit (1 << pin)
> >
> > This seems to be describing driver intrinsics in the device tree, like
> > how the hardware is routed on the inside and what it can do.
> >
> > IMO that is driver territory, the driver should know these limitations
> > and protest if you try to do something illegal.
> >
> > Anyway as the AT91 maintainers seem to disagree I will allow some
> > more time for discussion before merging the patch.
> >
> > I can't really have one AT91 maintainer NACKing another, it doesn't
> > matter that this is a separate driver, in my book the MAINTAINERS
> > entry for AT91 as a whole overrides that so can you please find an
> > agreement on how to handle this or I will stall the patch until
> > you're in agreement.
> 
> Nicolas has been the de-facto maintainer of AT91 for quite a while
> now, even though more of them are listed on the maintainers entry. It
> would be inappropriate to merge something that he disagreed with on
> that platform.
I'm still following it
> 
> > ARM SoC maintainers input would be welcomed.
> 
> It seems appropriate to ask the at91 folks to come back with a
> solution that everybody is OK with, and until then hold off merging
> this.
when the nack is motivated not just I don't like it

Best Regards,
J.