[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

This patch allows to have gpio controller with status set to disabled.

gpio_banks represents all the gpio banks available on the device whereas
nbanks represents the gpio banks used. Having a disabled gpio controller
involves that nbanks value is lower than gpio_banks and that some
pointers in the gpio_chips array are NULL. This patch deals with these
specific cases.

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---

Hi,

Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
support unused gpio bank.


 drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 94643bb..95ae122 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 
 	for_each_child_of_node(np, child) {
 		if (of_device_is_compatible(child, gpio_compat)) {
-			info->nbanks++;
+			if (of_device_is_available(child))
+				info->nbanks++;
 		} else {
 			info->nfunctions++;
 			info->ngroups += of_get_child_count(child);
@@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
 	}
 
 	size /= sizeof(*list);
-	if (!size || size % info->nbanks) {
+	if (!size || size % gpio_banks) {
 		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
 		return -EINVAL;
 	}
-	info->nmux = size / info->nbanks;
+	info->nmux = size / gpio_banks;
 
 	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
 	if (!info->mux_mask) {
@@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 {
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
-	int ret, i, j, k;
+	int ret, i, j, k, ngpio_chips_enabled = 0;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	 * to obtain references to the struct gpio_chip * for them, and we
 	 * need this to proceed.
 	 */
-	for (i = 0; i < info->nbanks; i++) {
-		if (!gpio_chips[i]) {
-			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
-			devm_kfree(&pdev->dev, info);
-			return -EPROBE_DEFER;
-		}
+	for (i = 0; i < gpio_banks; i++) {
+		if (gpio_chips[i])
+			ngpio_chips_enabled++;
+	}
+	if (ngpio_chips_enabled < info->nbanks) {
+		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
+		devm_kfree(&pdev->dev, info);
+		return -EPROBE_DEFER;
 	}
 
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
@@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	/* We will handle a range of GPIO pins */
-	for (i = 0; i < info->nbanks; i++)
-		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
+	for (i = 0; i < gpio_banks; i++)
+		if (gpio_chips[i])
+			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
 
 	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
 
@@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
 
 	for (i = 0; i < gpio_banks; i++) {
 		at91_gpio = gpio_chips[i];
+		if (!at91_gpio)
+			continue;
 
 		/*
 		 * GPIO controller are grouped on some SoC:
-- 
2.0.3

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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

This patch allows to have gpio controller with status set to disabled.

gpio_banks represents all the gpio banks available on the device whereas
nbanks represents the gpio banks used. Having a disabled gpio controller
involves that nbanks value is lower than gpio_banks and that some
pointers in the gpio_chips array are NULL. This patch deals with these
specific cases.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---

Hi,

Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
support unused gpio bank.


 drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 94643bb..95ae122 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 
 	for_each_child_of_node(np, child) {
 		if (of_device_is_compatible(child, gpio_compat)) {
-			info->nbanks++;
+			if (of_device_is_available(child))
+				info->nbanks++;
 		} else {
 			info->nfunctions++;
 			info->ngroups += of_get_child_count(child);
@@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
 	}
 
 	size /= sizeof(*list);
-	if (!size || size % info->nbanks) {
+	if (!size || size % gpio_banks) {
 		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
 		return -EINVAL;
 	}
-	info->nmux = size / info->nbanks;
+	info->nmux = size / gpio_banks;
 
 	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
 	if (!info->mux_mask) {
@@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 {
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
-	int ret, i, j, k;
+	int ret, i, j, k, ngpio_chips_enabled = 0;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	 * to obtain references to the struct gpio_chip * for them, and we
 	 * need this to proceed.
 	 */
-	for (i = 0; i < info->nbanks; i++) {
-		if (!gpio_chips[i]) {
-			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
-			devm_kfree(&pdev->dev, info);
-			return -EPROBE_DEFER;
-		}
+	for (i = 0; i < gpio_banks; i++) {
+		if (gpio_chips[i])
+			ngpio_chips_enabled++;
+	}
+	if (ngpio_chips_enabled < info->nbanks) {
+		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
+		devm_kfree(&pdev->dev, info);
+		return -EPROBE_DEFER;
 	}
 
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
@@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	/* We will handle a range of GPIO pins */
-	for (i = 0; i < info->nbanks; i++)
-		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
+	for (i = 0; i < gpio_banks; i++)
+		if (gpio_chips[i])
+			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
 
 	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
 
@@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
 
 	for (i = 0; i < gpio_banks; i++) {
 		at91_gpio = gpio_chips[i];
+		if (!at91_gpio)
+			continue;
 
 		/*
 		 * GPIO controller are grouped on some SoC:
-- 
2.0.3

[PATCH 2/2] ARM: at91/dt: sama5d4: add pioD controller

[Ludovic Desroches]

PioD controller was not described in the device tree since we don't use
it. As pinctrl-at91 allows disabled gpio controllers in the device
tree, we can add it to complete the device description.

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sama5d4.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index e0157b0..2cc3cfe 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -61,6 +61,7 @@
 		gpio0 = &pioA;
 		gpio1 = &pioB;
 		gpio2 = &pioC;
+		gpio3 = &pioD;
 		gpio4 = &pioE;
 		tcb0 = &tcb0;
 		tcb1 = &tcb1;
@@ -1042,6 +1043,18 @@
 					clocks = <&pioC_clk>;
 				};
 
+				pioD: gpio@fc068000 {
+					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
+					reg = <0xfc068000 0x100>;
+					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
+					#gpio-cells = <2>;
+					gpio-controller;
+					interrupt-controller;
+					#interrupt-cells = <2>;
+					clocks = <&pioD_clk>;
+					status = "disabled";
+				};
+
 				pioE: gpio@fc06d000 {
 					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
 					reg = <0xfc06d000 0x100>;
-- 
2.0.3

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

[PATCH 2/2] ARM: at91/dt: sama5d4: add pioD controller

[Ludovic Desroches]

PioD controller was not described in the device tree since we don't use
it. As pinctrl-at91 allows disabled gpio controllers in the device
tree, we can add it to complete the device description.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 arch/arm/boot/dts/sama5d4.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index e0157b0..2cc3cfe 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -61,6 +61,7 @@
 		gpio0 = &pioA;
 		gpio1 = &pioB;
 		gpio2 = &pioC;
+		gpio3 = &pioD;
 		gpio4 = &pioE;
 		tcb0 = &tcb0;
 		tcb1 = &tcb1;
@@ -1042,6 +1043,18 @@
 					clocks = <&pioC_clk>;
 				};
 
+				pioD: gpio at fc068000 {
+					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
+					reg = <0xfc068000 0x100>;
+					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
+					#gpio-cells = <2>;
+					gpio-controller;
+					interrupt-controller;
+					#interrupt-cells = <2>;
+					clocks = <&pioD_clk>;
+					status = "disabled";
+				};
+
 				pioE: gpio at fc06d000 {
 					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
 					reg = <0xfc06d000 0x100>;
-- 
2.0.3

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> This patch allows to have gpio controller with status set to disabled.
>
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

(...)
>         /* We will handle a range of GPIO pins */
> -       for (i = 0; i < info->nbanks; i++)
> -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +       for (i = 0; i < gpio_banks; i++)
> +               if (gpio_chips[i])
> +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);

I highly suspect the real solution to this problem is to get rid
of the pinctrl_add_gpio_range() call from the driver.

Remobe these calls, and instead in at91_gpio_probe() in the same
file, call gpiochip_add_pingroup_range() for each GPIO chip.

That way the GPIO ranges are inserted from the GPIO side instead
of the pinctrl side, which is way better, since it is more relative,
and make you only add ranges for the gpio chips actually there.

Yours,
Linus Walleij
--
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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:

> This patch allows to have gpio controller with status set to disabled.
>
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>

(...)
>         /* We will handle a range of GPIO pins */
> -       for (i = 0; i < info->nbanks; i++)
> -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +       for (i = 0; i < gpio_banks; i++)
> +               if (gpio_chips[i])
> +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);

I highly suspect the real solution to this problem is to get rid
of the pinctrl_add_gpio_range() call from the driver.

Remobe these calls, and instead in at91_gpio_probe() in the same
file, call gpiochip_add_pingroup_range() for each GPIO chip.

That way the GPIO ranges are inserted from the GPIO side instead
of the pinctrl side, which is way better, since it is more relative,
and make you only add ranges for the gpio chips actually there.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Nicolas Ferre]

Le 28/11/2014 17:49, Ludovic Desroches a écrit :
> This patch allows to have gpio controller with status set to disabled.
> 
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Yes:

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Linus, is it too late for 3.18? Otherwise, we can add this tag:

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.18+

Bye,

> ---
> 
> Hi,
> 
> Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
> support unused gpio bank.
> 
> 
>  drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 94643bb..95ae122 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
> -			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nbanks++;
>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>  	}
>  
>  	size /= sizeof(*list);
> -	if (!size || size % info->nbanks) {
> +	if (!size || size % gpio_banks) {
>  		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
>  		return -EINVAL;
>  	}
> -	info->nmux = size / info->nbanks;
> +	info->nmux = size / gpio_banks;
>  
>  	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
>  	if (!info->mux_mask) {
> @@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * to obtain references to the struct gpio_chip * for them, and we
>  	 * need this to proceed.
>  	 */
> -	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +	for (i = 0; i < gpio_banks; i++) {
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nbanks) {
> +		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	/* We will handle a range of GPIO pins */
> -	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +	for (i = 0; i < gpio_banks; i++)
> +		if (gpio_chips[i])
> +			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> 


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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Nicolas Ferre]

Le 28/11/2014 17:49, Ludovic Desroches a ?crit :
> This patch allows to have gpio controller with status set to disabled.
> 
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Yes:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Linus, is it too late for 3.18? Otherwise, we can add this tag:

Cc: <stable@vger.kernel.org> # v3.18+

Bye,

> ---
> 
> Hi,
> 
> Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
> support unused gpio bank.
> 
> 
>  drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 94643bb..95ae122 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
> -			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nbanks++;
>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>  	}
>  
>  	size /= sizeof(*list);
> -	if (!size || size % info->nbanks) {
> +	if (!size || size % gpio_banks) {
>  		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
>  		return -EINVAL;
>  	}
> -	info->nmux = size / info->nbanks;
> +	info->nmux = size / gpio_banks;
>  
>  	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
>  	if (!info->mux_mask) {
> @@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * to obtain references to the struct gpio_chip * for them, and we
>  	 * need this to proceed.
>  	 */
> -	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +	for (i = 0; i < gpio_banks; i++) {
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nbanks) {
> +		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	/* We will handle a range of GPIO pins */
> -	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +	for (i = 0; i < gpio_banks; i++)
> +		if (gpio_chips[i])
> +			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> 


-- 
Nicolas Ferre

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:
> On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > This patch allows to have gpio controller with status set to disabled.
> >
> > gpio_banks represents all the gpio banks available on the device whereas
> > nbanks represents the gpio banks used. Having a disabled gpio controller
> > involves that nbanks value is lower than gpio_banks and that some
> > pointers in the gpio_chips array are NULL. This patch deals with these
> > specific cases.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> 
> (...)
> >         /* We will handle a range of GPIO pins */
> > -       for (i = 0; i < info->nbanks; i++)
> > -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +       for (i = 0; i < gpio_banks; i++)
> > +               if (gpio_chips[i])
> > +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> 
> I highly suspect the real solution to this problem is to get rid
> of the pinctrl_add_gpio_range() call from the driver.
> 
> Remobe these calls, and instead in at91_gpio_probe() in the same
> file, call gpiochip_add_pingroup_range() for each GPIO chip.
> 
> That way the GPIO ranges are inserted from the GPIO side instead
> of the pinctrl side, which is way better, since it is more relative,
> and make you only add ranges for the gpio chips actually there.

Thanks for your advices, I'll try to do it the way you recommend.

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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:
> On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> 
> > This patch allows to have gpio controller with status set to disabled.
> >
> > gpio_banks represents all the gpio banks available on the device whereas
> > nbanks represents the gpio banks used. Having a disabled gpio controller
> > involves that nbanks value is lower than gpio_banks and that some
> > pointers in the gpio_chips array are NULL. This patch deals with these
> > specific cases.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> 
> (...)
> >         /* We will handle a range of GPIO pins */
> > -       for (i = 0; i < info->nbanks; i++)
> > -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +       for (i = 0; i < gpio_banks; i++)
> > +               if (gpio_chips[i])
> > +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> 
> I highly suspect the real solution to this problem is to get rid
> of the pinctrl_add_gpio_range() call from the driver.
> 
> Remobe these calls, and instead in at91_gpio_probe() in the same
> file, call gpiochip_add_pingroup_range() for each GPIO chip.
> 
> That way the GPIO ranges are inserted from the GPIO side instead
> of the pinctrl side, which is way better, since it is more relative,
> and make you only add ranges for the gpio chips actually there.

Thanks for your advices, I'll try to do it the way you recommend.

Ludovic

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Mon, Dec 1, 2014 at 2:59 PM, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> Le 28/11/2014 17:49, Ludovic Desroches a écrit :
>> This patch allows to have gpio controller with status set to disabled.
>>
>> gpio_banks represents all the gpio banks available on the device whereas
>> nbanks represents the gpio banks used. Having a disabled gpio controller
>> involves that nbanks value is lower than gpio_banks and that some
>> pointers in the gpio_chips array are NULL. This patch deals with these
>> specific cases.
>>
>> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>
> Yes:
>
> Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>
> Linus, is it too late for 3.18?

I think it's too late for any kernel as I think it's the wrong fix ;)

Yours,
Linus Walleij
--
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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Mon, Dec 1, 2014 at 2:59 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Le 28/11/2014 17:49, Ludovic Desroches a ?crit :
>> This patch allows to have gpio controller with status set to disabled.
>>
>> gpio_banks represents all the gpio banks available on the device whereas
>> nbanks represents the gpio banks used. Having a disabled gpio controller
>> involves that nbanks value is lower than gpio_banks and that some
>> pointers in the gpio_chips array are NULL. This patch deals with these
>> specific cases.
>>
>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>
> Yes:
>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Linus, is it too late for 3.18?

I think it's too late for any kernel as I think it's the wrong fix ;)

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:
> On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > This patch allows to have gpio controller with status set to disabled.
> >
> > gpio_banks represents all the gpio banks available on the device whereas
> > nbanks represents the gpio banks used. Having a disabled gpio controller
> > involves that nbanks value is lower than gpio_banks and that some
> > pointers in the gpio_chips array are NULL. This patch deals with these
> > specific cases.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> 
> (...)
> >         /* We will handle a range of GPIO pins */
> > -       for (i = 0; i < info->nbanks; i++)
> > -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +       for (i = 0; i < gpio_banks; i++)
> > +               if (gpio_chips[i])
> > +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> 
> I highly suspect the real solution to this problem is to get rid
> of the pinctrl_add_gpio_range() call from the driver.
> 
> Remobe these calls, and instead in at91_gpio_probe() in the same
> file, call gpiochip_add_pingroup_range() for each GPIO chip.
> 
> That way the GPIO ranges are inserted from the GPIO side instead
> of the pinctrl side, which is way better, since it is more relative,
> and make you only add ranges for the gpio chips actually there.

I had a quick look to Documentation about that stuff, I totally agree
that it is a better approach. Before going further I was wondering if it
would not cause backward compatibility issue with old dtb (it seems I'll
have to add some properties to gpio controllers).

Regards

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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:
> On Fri, Nov 28, 2014 at 5:49 PM, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> 
> > This patch allows to have gpio controller with status set to disabled.
> >
> > gpio_banks represents all the gpio banks available on the device whereas
> > nbanks represents the gpio banks used. Having a disabled gpio controller
> > involves that nbanks value is lower than gpio_banks and that some
> > pointers in the gpio_chips array are NULL. This patch deals with these
> > specific cases.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> 
> (...)
> >         /* We will handle a range of GPIO pins */
> > -       for (i = 0; i < info->nbanks; i++)
> > -               pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +       for (i = 0; i < gpio_banks; i++)
> > +               if (gpio_chips[i])
> > +                       pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> 
> I highly suspect the real solution to this problem is to get rid
> of the pinctrl_add_gpio_range() call from the driver.
> 
> Remobe these calls, and instead in at91_gpio_probe() in the same
> file, call gpiochip_add_pingroup_range() for each GPIO chip.
> 
> That way the GPIO ranges are inserted from the GPIO side instead
> of the pinctrl side, which is way better, since it is more relative,
> and make you only add ranges for the gpio chips actually there.

I had a quick look to Documentation about that stuff, I totally agree
that it is a better approach. Before going further I was wondering if it
would not cause backward compatibility issue with old dtb (it seems I'll
have to add some properties to gpio controllers).

Regards

Ludovic

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Wed, Dec 3, 2014 at 4:08 PM, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:

>> Remobe these calls, and instead in at91_gpio_probe() in the same
>> file, call gpiochip_add_pingroup_range() for each GPIO chip.
>>
>> That way the GPIO ranges are inserted from the GPIO side instead
>> of the pinctrl side, which is way better, since it is more relative,
>> and make you only add ranges for the gpio chips actually there.
>
> I had a quick look to Documentation about that stuff, I totally agree
> that it is a better approach. Before going further I was wondering if it
> would not cause backward compatibility issue with old dtb (it seems I'll
> have to add some properties to gpio controllers).

The gpiolib core code parses and add ranges when using device tree,
and that is part of the core gpio bindings.

I think you should just try to add in the ranges in the dts nodes for
the gpiochips, remove the  pinctrl_add_gpio_range() calls and see
what happens.

Yours,
Linus Walleij
--
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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Wed, Dec 3, 2014 at 4:08 PM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> On Mon, Dec 01, 2014 at 02:56:22PM +0100, Linus Walleij wrote:

>> Remobe these calls, and instead in at91_gpio_probe() in the same
>> file, call gpiochip_add_pingroup_range() for each GPIO chip.
>>
>> That way the GPIO ranges are inserted from the GPIO side instead
>> of the pinctrl side, which is way better, since it is more relative,
>> and make you only add ranges for the gpio chips actually there.
>
> I had a quick look to Documentation about that stuff, I totally agree
> that it is a better approach. Before going further I was wondering if it
> would not cause backward compatibility issue with old dtb (it seems I'll
> have to add some properties to gpio controllers).

The gpiolib core code parses and add ranges when using device tree,
and that is part of the core gpio bindings.

I think you should just try to add in the ranges in the dts nodes for
the gpiochips, remove the  pinctrl_add_gpio_range() calls and see
what happens.

Yours,
Linus Walleij

[PATCH RFC] pinctrl: at91

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---

Hi Linus,

I have reworked my patch (of course it will be split for submission) trying to
follow your advices.

I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
tests but it seems to work. Maybe I've missed something but I still need to fix
the case when there is a gpio controller not used.

A lot of things rely on the gpio controller id (taken from the alias):
index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
rid of this constraint.

Regards

Ludovic

 arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
 drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 1b0f30c..c1c01a3 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -62,6 +62,7 @@
 		gpio0 = &pioA;
 		gpio1 = &pioB;
 		gpio2 = &pioC;
+		gpio3 = &pioD;
 		gpio4 = &pioE;
 		tcb0 = &tcb0;
 		tcb1 = &tcb1;
@@ -1063,7 +1064,7 @@
 			};
 
 
-			pinctrl@fc06a000 {
+			pinctrl: pinctrl@fc06a000 {
 				#address-cells = <1>;
 				#size-cells = <1>;
 				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
@@ -1084,6 +1085,7 @@
 					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 0 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioA_clk>;
@@ -1095,6 +1097,7 @@
 					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 32 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioB_clk>;
@@ -1106,17 +1109,31 @@
 					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 64 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioC_clk>;
 				};
 
+				pioD: gpio@fc068000 {
+					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
+					reg = <0xfc068000 0x100>;
+					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
+					#gpio-cells = <2>;
+					gpio-controller;
+					interrupt-controller;
+					#interrupt-cells = <2>;
+					clocks = <&pioD_clk>;
+					status = "disabled";
+				};
+
 				pioE: gpio@fc06d000 {
 					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
 					reg = <0xfc06d000 0x100>;
 					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 128 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioE_clk>;
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index dfd021e..f5d4aea 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
 
 struct at91_gpio_chip {
 	struct gpio_chip	chip;
-	struct pinctrl_gpio_range range;
 	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
 	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
 	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
@@ -178,6 +178,7 @@ struct at91_pinctrl {
 	struct pinctrl_dev	*pctl;
 
 	int			nbanks;
+	int			nactive_banks;
 
 	uint32_t		*mux_mask;
 	int			nmux;
@@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 	for_each_child_of_node(np, child) {
 		if (of_device_is_compatible(child, gpio_compat)) {
 			info->nbanks++;
+			if (of_device_is_available(child))
+				info->nactive_banks;
 		} else {
 			info->nfunctions++;
 			info->ngroups += of_get_child_count(child);
@@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "mux-mask\n");
 	tmp = info->mux_mask;
 	for (i = 0; i < info->nbanks; i++) {
+		if (!gpio_chips[i]) {
+			tmp += info->nmux;
+			continue;
+		}
 		for (j = 0; j < info->nmux; j++, tmp++) {
-			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
+			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
 		}
 	}
 
@@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 {
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
-	int ret, i, j, k;
+	int ret, i, j, k, ngpio_chips_enabled = 0;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	 * need this to proceed.
 	 */
 	for (i = 0; i < info->nbanks; i++) {
-		if (!gpio_chips[i]) {
-			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
-			devm_kfree(&pdev->dev, info);
-			return -EPROBE_DEFER;
-		}
+		if (gpio_chips[i])
+			ngpio_chips_enabled++;
+	}
+	if (ngpio_chips_enabled < info->nactive_banks) {
+		dev_warn(&pdev->dev,
+			 "All GPIO chips are not registered yet (%d/%d)\n",
+			 ngpio_chips_enabled, info->nactive_banks);
+		devm_kfree(&pdev->dev, info);
+		return -EPROBE_DEFER;
 	}
 
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
@@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	/* We will handle a range of GPIO pins */
 	for (i = 0; i < info->nbanks; i++)
-		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
+		if (gpio_chips[i])
+			of_gpiochip_add(&gpio_chips[i]->chip);
 
 	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
 
@@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
 
 	for (i = 0; i < gpio_banks; i++) {
 		at91_gpio = gpio_chips[i];
+		if (!at91_gpio)
+			continue;
 
 		/*
 		 * GPIO controller are grouped on some SoC:
@@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;
-	struct pinctrl_gpio_range *range;
 	int ret = 0;
 	int irq, i;
 	int alias_idx = of_alias_get_id(np, "gpio");
@@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	chip->names = (const char *const *)names;
 
-	range = &at91_chip->range;
-	range->name = chip->label;
-	range->id = alias_idx;
-	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
-
-	range->npins = chip->ngpio;
-	range->gc = chip;
-
 	ret = gpiochip_add(chip);
 	if (ret)
 		goto gpiochip_add_err;
-- 
2.0.3

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

[PATCH RFC] pinctrl: at91

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---

Hi Linus,

I have reworked my patch (of course it will be split for submission) trying to
follow your advices.

I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
tests but it seems to work. Maybe I've missed something but I still need to fix
the case when there is a gpio controller not used.

A lot of things rely on the gpio controller id (taken from the alias):
index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
rid of this constraint.

Regards

Ludovic

 arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
 drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 1b0f30c..c1c01a3 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -62,6 +62,7 @@
 		gpio0 = &pioA;
 		gpio1 = &pioB;
 		gpio2 = &pioC;
+		gpio3 = &pioD;
 		gpio4 = &pioE;
 		tcb0 = &tcb0;
 		tcb1 = &tcb1;
@@ -1063,7 +1064,7 @@
 			};
 
 
-			pinctrl at fc06a000 {
+			pinctrl: pinctrl at fc06a000 {
 				#address-cells = <1>;
 				#size-cells = <1>;
 				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
@@ -1084,6 +1085,7 @@
 					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 0 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioA_clk>;
@@ -1095,6 +1097,7 @@
 					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 32 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioB_clk>;
@@ -1106,17 +1109,31 @@
 					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 64 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioC_clk>;
 				};
 
+				pioD: gpio at fc068000 {
+					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
+					reg = <0xfc068000 0x100>;
+					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
+					#gpio-cells = <2>;
+					gpio-controller;
+					interrupt-controller;
+					#interrupt-cells = <2>;
+					clocks = <&pioD_clk>;
+					status = "disabled";
+				};
+
 				pioE: gpio at fc06d000 {
 					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
 					reg = <0xfc06d000 0x100>;
 					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
 					#gpio-cells = <2>;
 					gpio-controller;
+					gpio-ranges = <&pinctrl 0 128 32>;
 					interrupt-controller;
 					#interrupt-cells = <2>;
 					clocks = <&pioE_clk>;
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index dfd021e..f5d4aea 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
 
 struct at91_gpio_chip {
 	struct gpio_chip	chip;
-	struct pinctrl_gpio_range range;
 	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
 	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
 	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
@@ -178,6 +178,7 @@ struct at91_pinctrl {
 	struct pinctrl_dev	*pctl;
 
 	int			nbanks;
+	int			nactive_banks;
 
 	uint32_t		*mux_mask;
 	int			nmux;
@@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 	for_each_child_of_node(np, child) {
 		if (of_device_is_compatible(child, gpio_compat)) {
 			info->nbanks++;
+			if (of_device_is_available(child))
+				info->nactive_banks;
 		} else {
 			info->nfunctions++;
 			info->ngroups += of_get_child_count(child);
@@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "mux-mask\n");
 	tmp = info->mux_mask;
 	for (i = 0; i < info->nbanks; i++) {
+		if (!gpio_chips[i]) {
+			tmp += info->nmux;
+			continue;
+		}
 		for (j = 0; j < info->nmux; j++, tmp++) {
-			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
+			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
 		}
 	}
 
@@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 {
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
-	int ret, i, j, k;
+	int ret, i, j, k, ngpio_chips_enabled = 0;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	 * need this to proceed.
 	 */
 	for (i = 0; i < info->nbanks; i++) {
-		if (!gpio_chips[i]) {
-			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
-			devm_kfree(&pdev->dev, info);
-			return -EPROBE_DEFER;
-		}
+		if (gpio_chips[i])
+			ngpio_chips_enabled++;
+	}
+	if (ngpio_chips_enabled < info->nactive_banks) {
+		dev_warn(&pdev->dev,
+			 "All GPIO chips are not registered yet (%d/%d)\n",
+			 ngpio_chips_enabled, info->nactive_banks);
+		devm_kfree(&pdev->dev, info);
+		return -EPROBE_DEFER;
 	}
 
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
@@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	/* We will handle a range of GPIO pins */
 	for (i = 0; i < info->nbanks; i++)
-		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
+		if (gpio_chips[i])
+			of_gpiochip_add(&gpio_chips[i]->chip);
 
 	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
 
@@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
 
 	for (i = 0; i < gpio_banks; i++) {
 		at91_gpio = gpio_chips[i];
+		if (!at91_gpio)
+			continue;
 
 		/*
 		 * GPIO controller are grouped on some SoC:
@@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;
-	struct pinctrl_gpio_range *range;
 	int ret = 0;
 	int irq, i;
 	int alias_idx = of_alias_get_id(np, "gpio");
@@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	chip->names = (const char *const *)names;
 
-	range = &at91_chip->range;
-	range->name = chip->label;
-	range->id = alias_idx;
-	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
-
-	range->npins = chip->ngpio;
-	range->gc = chip;
-
 	ret = gpiochip_add(chip);
 	if (ret)
 		goto gpiochip_add_err;
-- 
2.0.3

Re: [PATCH RFC] pinctrl: at91

[Nicolas Ferre]

Le 15/12/2014 10:57, Ludovic Desroches a écrit :
> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> 
> Hi Linus,
> 
> I have reworked my patch (of course it will be split for submission) trying to
> follow your advices.
> 
> I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
> tests but it seems to work. Maybe I've missed something but I still need to fix
> the case when there is a gpio controller not used.
> 
> A lot of things rely on the gpio controller id (taken from the alias):
> index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
> rid of this constraint.

Fair enough, I'm personally okay with those changes. When you rework
this RFC into real patches and when you correct the little nitpicking
hereafter, you can add my:

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

On your side Linus, does it sound good?


BTW, once split, you'll have to add a commit message with explanation to
each patch ;-)

Otherwise, see below:


>  arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
>  drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
>  2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
> index 1b0f30c..c1c01a3 100644
> --- a/arch/arm/boot/dts/sama5d4.dtsi
> +++ b/arch/arm/boot/dts/sama5d4.dtsi
> @@ -62,6 +62,7 @@
>  		gpio0 = &pioA;
>  		gpio1 = &pioB;
>  		gpio2 = &pioC;
> +		gpio3 = &pioD;
>  		gpio4 = &pioE;
>  		tcb0 = &tcb0;
>  		tcb1 = &tcb1;
> @@ -1063,7 +1064,7 @@
>  			};
>  
>  
> -			pinctrl@fc06a000 {
> +			pinctrl: pinctrl@fc06a000 {
>  				#address-cells = <1>;
>  				#size-cells = <1>;
>  				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
> @@ -1084,6 +1085,7 @@
>  					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 0 32>;

You may need to modify our pinctrl binding documentation as well.


>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioA_clk>;
> @@ -1095,6 +1097,7 @@
>  					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 32 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioB_clk>;
> @@ -1106,17 +1109,31 @@
>  					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 64 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioC_clk>;
>  				};
>  
> +				pioD: gpio@fc068000 {
> +					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfc068000 0x100>;
> +					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pioD_clk>;
> +					status = "disabled";
> +				};
> +
>  				pioE: gpio@fc06d000 {
>  					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>  					reg = <0xfc06d000 0x100>;
>  					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 128 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioE_clk>;
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index dfd021e..f5d4aea 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
> @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
>  
>  struct at91_gpio_chip {
>  	struct gpio_chip	chip;
> -	struct pinctrl_gpio_range range;
>  	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
>  	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
>  	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
> @@ -178,6 +178,7 @@ struct at91_pinctrl {
>  	struct pinctrl_dev	*pctl;
>  
>  	int			nbanks;
> +	int			nactive_banks;
>  
>  	uint32_t		*mux_mask;
>  	int			nmux;
> @@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
>  			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nactive_banks;

What is the purpose of the line just above?


>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  	dev_dbg(&pdev->dev, "mux-mask\n");
>  	tmp = info->mux_mask;
>  	for (i = 0; i < info->nbanks; i++) {
> +		if (!gpio_chips[i]) {
> +			tmp += info->nmux;
> +			continue;
> +		}
>  		for (j = 0; j < info->nmux; j++, tmp++) {
> -			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
> +			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
>  		}
>  	}
>  
> @@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * need this to proceed.
>  	 */
>  	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nactive_banks) {
> +		dev_warn(&pdev->dev,
> +			 "All GPIO chips are not registered yet (%d/%d)\n",
> +			 ngpio_chips_enabled, info->nactive_banks);
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	/* We will handle a range of GPIO pins */
>  	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +		if (gpio_chips[i])
> +			of_gpiochip_add(&gpio_chips[i]->chip);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct at91_gpio_chip *at91_chip = NULL;
>  	struct gpio_chip *chip;
> -	struct pinctrl_gpio_range *range;
>  	int ret = 0;
>  	int irq, i;
>  	int alias_idx = of_alias_get_id(np, "gpio");
> @@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  
>  	chip->names = (const char *const *)names;
>  
> -	range = &at91_chip->range;
> -	range->name = chip->label;
> -	range->id = alias_idx;
> -	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
> -
> -	range->npins = chip->ngpio;
> -	range->gc = chip;
> -
>  	ret = gpiochip_add(chip);
>  	if (ret)
>  		goto gpiochip_add_err;
> 

Thanks Ludovic, bye,
-- 
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

[PATCH RFC] pinctrl: at91

[Nicolas Ferre]

Le 15/12/2014 10:57, Ludovic Desroches a ?crit :
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> 
> Hi Linus,
> 
> I have reworked my patch (of course it will be split for submission) trying to
> follow your advices.
> 
> I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
> tests but it seems to work. Maybe I've missed something but I still need to fix
> the case when there is a gpio controller not used.
> 
> A lot of things rely on the gpio controller id (taken from the alias):
> index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
> rid of this constraint.

Fair enough, I'm personally okay with those changes. When you rework
this RFC into real patches and when you correct the little nitpicking
hereafter, you can add my:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

On your side Linus, does it sound good?


BTW, once split, you'll have to add a commit message with explanation to
each patch ;-)

Otherwise, see below:


>  arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
>  drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
>  2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
> index 1b0f30c..c1c01a3 100644
> --- a/arch/arm/boot/dts/sama5d4.dtsi
> +++ b/arch/arm/boot/dts/sama5d4.dtsi
> @@ -62,6 +62,7 @@
>  		gpio0 = &pioA;
>  		gpio1 = &pioB;
>  		gpio2 = &pioC;
> +		gpio3 = &pioD;
>  		gpio4 = &pioE;
>  		tcb0 = &tcb0;
>  		tcb1 = &tcb1;
> @@ -1063,7 +1064,7 @@
>  			};
>  
>  
> -			pinctrl at fc06a000 {
> +			pinctrl: pinctrl at fc06a000 {
>  				#address-cells = <1>;
>  				#size-cells = <1>;
>  				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
> @@ -1084,6 +1085,7 @@
>  					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 0 32>;

You may need to modify our pinctrl binding documentation as well.


>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioA_clk>;
> @@ -1095,6 +1097,7 @@
>  					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 32 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioB_clk>;
> @@ -1106,17 +1109,31 @@
>  					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 64 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioC_clk>;
>  				};
>  
> +				pioD: gpio at fc068000 {
> +					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfc068000 0x100>;
> +					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pioD_clk>;
> +					status = "disabled";
> +				};
> +
>  				pioE: gpio at fc06d000 {
>  					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>  					reg = <0xfc06d000 0x100>;
>  					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
>  					#gpio-cells = <2>;
>  					gpio-controller;
> +					gpio-ranges = <&pinctrl 0 128 32>;
>  					interrupt-controller;
>  					#interrupt-cells = <2>;
>  					clocks = <&pioE_clk>;
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index dfd021e..f5d4aea 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
> @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
>  
>  struct at91_gpio_chip {
>  	struct gpio_chip	chip;
> -	struct pinctrl_gpio_range range;
>  	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
>  	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
>  	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
> @@ -178,6 +178,7 @@ struct at91_pinctrl {
>  	struct pinctrl_dev	*pctl;
>  
>  	int			nbanks;
> +	int			nactive_banks;
>  
>  	uint32_t		*mux_mask;
>  	int			nmux;
> @@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
>  			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nactive_banks;

What is the purpose of the line just above?


>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  	dev_dbg(&pdev->dev, "mux-mask\n");
>  	tmp = info->mux_mask;
>  	for (i = 0; i < info->nbanks; i++) {
> +		if (!gpio_chips[i]) {
> +			tmp += info->nmux;
> +			continue;
> +		}
>  		for (j = 0; j < info->nmux; j++, tmp++) {
> -			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
> +			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
>  		}
>  	}
>  
> @@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * need this to proceed.
>  	 */
>  	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nactive_banks) {
> +		dev_warn(&pdev->dev,
> +			 "All GPIO chips are not registered yet (%d/%d)\n",
> +			 ngpio_chips_enabled, info->nactive_banks);
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	/* We will handle a range of GPIO pins */
>  	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +		if (gpio_chips[i])
> +			of_gpiochip_add(&gpio_chips[i]->chip);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct at91_gpio_chip *at91_chip = NULL;
>  	struct gpio_chip *chip;
> -	struct pinctrl_gpio_range *range;
>  	int ret = 0;
>  	int irq, i;
>  	int alias_idx = of_alias_get_id(np, "gpio");
> @@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  
>  	chip->names = (const char *const *)names;
>  
> -	range = &at91_chip->range;
> -	range->name = chip->label;
> -	range->id = alias_idx;
> -	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
> -
> -	range->npins = chip->ngpio;
> -	range->gc = chip;
> -
>  	ret = gpiochip_add(chip);
>  	if (ret)
>  		goto gpiochip_add_err;
> 

Thanks Ludovic, bye,
-- 
Nicolas Ferre

Re: [PATCH RFC] pinctrl: at91

[Ludovic Desroches]

Hi Nicolas, Linus,

On Fri, Dec 19, 2014 at 03:41:55PM +0100, Nicolas Ferre wrote:
> Le 15/12/2014 10:57, Ludovic Desroches a écrit :
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> > 
> > Hi Linus,
> > 
> > I have reworked my patch (of course it will be split for submission) trying to
> > follow your advices.
> > 
> > I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
> > tests but it seems to work. Maybe I've missed something but I still need to fix
> > the case when there is a gpio controller not used.
> > 
> > A lot of things rely on the gpio controller id (taken from the alias):
> > index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
> > rid of this constraint.
> 
> Fair enough, I'm personally okay with those changes. When you rework
> this RFC into real patches and when you correct the little nitpicking
> hereafter, you can add my:
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> On your side Linus, does it sound good?
> 

After testing more these changes, it breaks GPIOs if gpio-ranges
property is not added to all our SOCs (about 10 dtsi to update).

Usage of of_gpiochip_add() only solves my issue about gpio but not about
pinctrl stuff, I still need a patch to manage the case when we have a gap if
a gpio controller is not enabled to not break the pin naming, etc.

Maybe I am missing something, I am still discovering how pinctrl subsystem
works in order to maintain our pinctrl driver. So I would be pleased to
have some advices to find the proper way to fix this issue.


Regards

Ludovic

> 
> BTW, once split, you'll have to add a commit message with explanation to
> each patch ;-)
> 
> Otherwise, see below:
> 
> 
> >  arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
> >  drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
> >  2 files changed, 41 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
> > index 1b0f30c..c1c01a3 100644
> > --- a/arch/arm/boot/dts/sama5d4.dtsi
> > +++ b/arch/arm/boot/dts/sama5d4.dtsi
> > @@ -62,6 +62,7 @@
> >  		gpio0 = &pioA;
> >  		gpio1 = &pioB;
> >  		gpio2 = &pioC;
> > +		gpio3 = &pioD;
> >  		gpio4 = &pioE;
> >  		tcb0 = &tcb0;
> >  		tcb1 = &tcb1;
> > @@ -1063,7 +1064,7 @@
> >  			};
> >  
> >  
> > -			pinctrl@fc06a000 {
> > +			pinctrl: pinctrl@fc06a000 {
> >  				#address-cells = <1>;
> >  				#size-cells = <1>;
> >  				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
> > @@ -1084,6 +1085,7 @@
> >  					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 0 32>;
> 
> You may need to modify our pinctrl binding documentation as well.
> 
> 
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioA_clk>;
> > @@ -1095,6 +1097,7 @@
> >  					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 32 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioB_clk>;
> > @@ -1106,17 +1109,31 @@
> >  					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 64 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioC_clk>;
> >  				};
> >  
> > +				pioD: gpio@fc068000 {
> > +					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> > +					reg = <0xfc068000 0x100>;
> > +					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
> > +					#gpio-cells = <2>;
> > +					gpio-controller;
> > +					interrupt-controller;
> > +					#interrupt-cells = <2>;
> > +					clocks = <&pioD_clk>;
> > +					status = "disabled";
> > +				};
> > +
> >  				pioE: gpio@fc06d000 {
> >  					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> >  					reg = <0xfc06d000 0x100>;
> >  					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 128 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioE_clk>;
> > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> > index dfd021e..f5d4aea 100644
> > --- a/drivers/pinctrl/pinctrl-at91.c
> > +++ b/drivers/pinctrl/pinctrl-at91.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
> >  
> >  struct at91_gpio_chip {
> >  	struct gpio_chip	chip;
> > -	struct pinctrl_gpio_range range;
> >  	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
> >  	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
> >  	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
> > @@ -178,6 +178,7 @@ struct at91_pinctrl {
> >  	struct pinctrl_dev	*pctl;
> >  
> >  	int			nbanks;
> > +	int			nactive_banks;
> >  
> >  	uint32_t		*mux_mask;
> >  	int			nmux;
> > @@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
> >  	for_each_child_of_node(np, child) {
> >  		if (of_device_is_compatible(child, gpio_compat)) {
> >  			info->nbanks++;
> > +			if (of_device_is_available(child))
> > +				info->nactive_banks;
> 
> What is the purpose of the line just above?
> 
> 
> >  		} else {
> >  			info->nfunctions++;
> >  			info->ngroups += of_get_child_count(child);
> > @@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
> >  	dev_dbg(&pdev->dev, "mux-mask\n");
> >  	tmp = info->mux_mask;
> >  	for (i = 0; i < info->nbanks; i++) {
> > +		if (!gpio_chips[i]) {
> > +			tmp += info->nmux;
> > +			continue;
> > +		}
> >  		for (j = 0; j < info->nmux; j++, tmp++) {
> > -			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
> > +			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
> >  		}
> >  	}
> >  
> > @@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  {
> >  	struct at91_pinctrl *info;
> >  	struct pinctrl_pin_desc *pdesc;
> > -	int ret, i, j, k;
> > +	int ret, i, j, k, ngpio_chips_enabled = 0;
> >  
> >  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> >  	if (!info)
> > @@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  	 * need this to proceed.
> >  	 */
> >  	for (i = 0; i < info->nbanks; i++) {
> > -		if (!gpio_chips[i]) {
> > -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> > -			devm_kfree(&pdev->dev, info);
> > -			return -EPROBE_DEFER;
> > -		}
> > +		if (gpio_chips[i])
> > +			ngpio_chips_enabled++;
> > +	}
> > +	if (ngpio_chips_enabled < info->nactive_banks) {
> > +		dev_warn(&pdev->dev,
> > +			 "All GPIO chips are not registered yet (%d/%d)\n",
> > +			 ngpio_chips_enabled, info->nactive_banks);
> > +		devm_kfree(&pdev->dev, info);
> > +		return -EPROBE_DEFER;
> >  	}
> >  
> >  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> > @@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  		goto err;
> >  	}
> >  
> > -	/* We will handle a range of GPIO pins */
> >  	for (i = 0; i < info->nbanks; i++)
> > -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +		if (gpio_chips[i])
> > +			of_gpiochip_add(&gpio_chips[i]->chip);
> >  
> >  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
> >  
> > @@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
> >  
> >  	for (i = 0; i < gpio_banks; i++) {
> >  		at91_gpio = gpio_chips[i];
> > +		if (!at91_gpio)
> > +			continue;
> >  
> >  		/*
> >  		 * GPIO controller are grouped on some SoC:
> > @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
> >  	struct resource *res;
> >  	struct at91_gpio_chip *at91_chip = NULL;
> >  	struct gpio_chip *chip;
> > -	struct pinctrl_gpio_range *range;
> >  	int ret = 0;
> >  	int irq, i;
> >  	int alias_idx = of_alias_get_id(np, "gpio");
> > @@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
> >  
> >  	chip->names = (const char *const *)names;
> >  
> > -	range = &at91_chip->range;
> > -	range->name = chip->label;
> > -	range->id = alias_idx;
> > -	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
> > -
> > -	range->npins = chip->ngpio;
> > -	range->gc = chip;
> > -
> >  	ret = gpiochip_add(chip);
> >  	if (ret)
> >  		goto gpiochip_add_err;
> > 
> 
> Thanks Ludovic, bye,
> -- 
> Nicolas Ferre

[PATCH RFC] pinctrl: at91

[Ludovic Desroches]

Hi Nicolas, Linus,

On Fri, Dec 19, 2014 at 03:41:55PM +0100, Nicolas Ferre wrote:
> Le 15/12/2014 10:57, Ludovic Desroches a ?crit :
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> > 
> > Hi Linus,
> > 
> > I have reworked my patch (of course it will be split for submission) trying to
> > follow your advices.
> > 
> > I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more
> > tests but it seems to work. Maybe I've missed something but I still need to fix
> > the case when there is a gpio controller not used.
> > 
> > A lot of things rely on the gpio controller id (taken from the alias):
> > index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get
> > rid of this constraint.
> 
> Fair enough, I'm personally okay with those changes. When you rework
> this RFC into real patches and when you correct the little nitpicking
> hereafter, you can add my:
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> On your side Linus, does it sound good?
> 

After testing more these changes, it breaks GPIOs if gpio-ranges
property is not added to all our SOCs (about 10 dtsi to update).

Usage of of_gpiochip_add() only solves my issue about gpio but not about
pinctrl stuff, I still need a patch to manage the case when we have a gap if
a gpio controller is not enabled to not break the pin naming, etc.

Maybe I am missing something, I am still discovering how pinctrl subsystem
works in order to maintain our pinctrl driver. So I would be pleased to
have some advices to find the proper way to fix this issue.


Regards

Ludovic

> 
> BTW, once split, you'll have to add a commit message with explanation to
> each patch ;-)
> 
> Otherwise, see below:
> 
> 
> >  arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++-
> >  drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++-------------------
> >  2 files changed, 41 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
> > index 1b0f30c..c1c01a3 100644
> > --- a/arch/arm/boot/dts/sama5d4.dtsi
> > +++ b/arch/arm/boot/dts/sama5d4.dtsi
> > @@ -62,6 +62,7 @@
> >  		gpio0 = &pioA;
> >  		gpio1 = &pioB;
> >  		gpio2 = &pioC;
> > +		gpio3 = &pioD;
> >  		gpio4 = &pioE;
> >  		tcb0 = &tcb0;
> >  		tcb1 = &tcb1;
> > @@ -1063,7 +1064,7 @@
> >  			};
> >  
> >  
> > -			pinctrl at fc06a000 {
> > +			pinctrl: pinctrl at fc06a000 {
> >  				#address-cells = <1>;
> >  				#size-cells = <1>;
> >  				compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
> > @@ -1084,6 +1085,7 @@
> >  					interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 0 32>;
> 
> You may need to modify our pinctrl binding documentation as well.
> 
> 
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioA_clk>;
> > @@ -1095,6 +1097,7 @@
> >  					interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 32 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioB_clk>;
> > @@ -1106,17 +1109,31 @@
> >  					interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 64 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioC_clk>;
> >  				};
> >  
> > +				pioD: gpio at fc068000 {
> > +					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> > +					reg = <0xfc068000 0x100>;
> > +					interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>;
> > +					#gpio-cells = <2>;
> > +					gpio-controller;
> > +					interrupt-controller;
> > +					#interrupt-cells = <2>;
> > +					clocks = <&pioD_clk>;
> > +					status = "disabled";
> > +				};
> > +
> >  				pioE: gpio at fc06d000 {
> >  					compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> >  					reg = <0xfc06d000 0x100>;
> >  					interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>;
> >  					#gpio-cells = <2>;
> >  					gpio-controller;
> > +					gpio-ranges = <&pinctrl 0 128 32>;
> >  					interrupt-controller;
> >  					#interrupt-cells = <2>;
> >  					clocks = <&pioE_clk>;
> > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> > index dfd021e..f5d4aea 100644
> > --- a/drivers/pinctrl/pinctrl-at91.c
> > +++ b/drivers/pinctrl/pinctrl-at91.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;
> >  
> >  struct at91_gpio_chip {
> >  	struct gpio_chip	chip;
> > -	struct pinctrl_gpio_range range;
> >  	struct at91_gpio_chip	*next;		/* Bank sharing same clock */
> >  	int			pioc_hwirq;	/* PIO bank interrupt identifier on AIC */
> >  	int			pioc_virq;	/* PIO bank Linux virtual interrupt */
> > @@ -178,6 +178,7 @@ struct at91_pinctrl {
> >  	struct pinctrl_dev	*pctl;
> >  
> >  	int			nbanks;
> > +	int			nactive_banks;
> >  
> >  	uint32_t		*mux_mask;
> >  	int			nmux;
> > @@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
> >  	for_each_child_of_node(np, child) {
> >  		if (of_device_is_compatible(child, gpio_compat)) {
> >  			info->nbanks++;
> > +			if (of_device_is_available(child))
> > +				info->nactive_banks;
> 
> What is the purpose of the line just above?
> 
> 
> >  		} else {
> >  			info->nfunctions++;
> >  			info->ngroups += of_get_child_count(child);
> > @@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
> >  	dev_dbg(&pdev->dev, "mux-mask\n");
> >  	tmp = info->mux_mask;
> >  	for (i = 0; i < info->nbanks; i++) {
> > +		if (!gpio_chips[i]) {
> > +			tmp += info->nmux;
> > +			continue;
> > +		}
> >  		for (j = 0; j < info->nmux; j++, tmp++) {
> > -			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
> > +			dev_dbg(&pdev->dev, "pio%c:periphal %c\t0x%x\n", 'A' + i, 'A' + j, tmp[0]);
> >  		}
> >  	}
> >  
> > @@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  {
> >  	struct at91_pinctrl *info;
> >  	struct pinctrl_pin_desc *pdesc;
> > -	int ret, i, j, k;
> > +	int ret, i, j, k, ngpio_chips_enabled = 0;
> >  
> >  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> >  	if (!info)
> > @@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  	 * need this to proceed.
> >  	 */
> >  	for (i = 0; i < info->nbanks; i++) {
> > -		if (!gpio_chips[i]) {
> > -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> > -			devm_kfree(&pdev->dev, info);
> > -			return -EPROBE_DEFER;
> > -		}
> > +		if (gpio_chips[i])
> > +			ngpio_chips_enabled++;
> > +	}
> > +	if (ngpio_chips_enabled < info->nactive_banks) {
> > +		dev_warn(&pdev->dev,
> > +			 "All GPIO chips are not registered yet (%d/%d)\n",
> > +			 ngpio_chips_enabled, info->nactive_banks);
> > +		devm_kfree(&pdev->dev, info);
> > +		return -EPROBE_DEFER;
> >  	}
> >  
> >  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> > @@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
> >  		goto err;
> >  	}
> >  
> > -	/* We will handle a range of GPIO pins */
> >  	for (i = 0; i < info->nbanks; i++)
> > -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> > +		if (gpio_chips[i])
> > +			of_gpiochip_add(&gpio_chips[i]->chip);
> >  
> >  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
> >  
> > @@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)
> >  
> >  	for (i = 0; i < gpio_banks; i++) {
> >  		at91_gpio = gpio_chips[i];
> > +		if (!at91_gpio)
> > +			continue;
> >  
> >  		/*
> >  		 * GPIO controller are grouped on some SoC:
> > @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
> >  	struct resource *res;
> >  	struct at91_gpio_chip *at91_chip = NULL;
> >  	struct gpio_chip *chip;
> > -	struct pinctrl_gpio_range *range;
> >  	int ret = 0;
> >  	int irq, i;
> >  	int alias_idx = of_alias_get_id(np, "gpio");
> > @@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
> >  
> >  	chip->names = (const char *const *)names;
> >  
> > -	range = &at91_chip->range;
> > -	range->name = chip->label;
> > -	range->id = alias_idx;
> > -	range->pin_base = range->base = range->id * MAX_NB_GPIO_PER_BANK;
> > -
> > -	range->npins = chip->ngpio;
> > -	range->gc = chip;
> > -
> >  	ret = gpiochip_add(chip);
> >  	if (ret)
> >  		goto gpiochip_add_err;
> > 
> 
> Thanks Ludovic, bye,
> -- 
> Nicolas Ferre

Re: [PATCH RFC] pinctrl: at91

[Linus Walleij]

On Tue, Jan 6, 2015 at 10:37 AM, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> Usage of of_gpiochip_add() only solves my issue about gpio but not about
> pinctrl stuff, I still need a patch to manage the case when we have a gap if
> a gpio controller is not enabled to not break the pin naming, etc.

This has been the topic of many threads today.
I assume you are talking about keeping GPIO numbers
consistent.

- My suggestion is to add alias handling of the GPIO chips
  to the core so they can be probed in the right order.

- For consistency in sysfs use the "names" array in
  struct gpio_chip so you can search for a symbolic
  name in sysfs and don't have to rely on fragile stuff
  like GPIO numbers.

- Partake in the development of a new GPIO ABI
  that does not use the global GPIO numberspace.

Yours,
Linus Walleij
--
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

[PATCH RFC] pinctrl: at91

[Linus Walleij]

On Tue, Jan 6, 2015 at 10:37 AM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:

> Usage of of_gpiochip_add() only solves my issue about gpio but not about
> pinctrl stuff, I still need a patch to manage the case when we have a gap if
> a gpio controller is not enabled to not break the pin naming, etc.

This has been the topic of many threads today.
I assume you are talking about keeping GPIO numbers
consistent.

- My suggestion is to add alias handling of the GPIO chips
  to the core so they can be probed in the right order.

- For consistency in sysfs use the "names" array in
  struct gpio_chip so you can search for a symbolic
  name in sysfs and don't have to rely on fragile stuff
  like GPIO numbers.

- Partake in the development of a new GPIO ABI
  that does not use the global GPIO numberspace.

Yours,
Linus Walleij

Re: [PATCH RFC] pinctrl: at91

[Ludovic Desroches]

On Wed, Jan 14, 2015 at 01:26:16PM +0100, Linus Walleij wrote:
> On Tue, Jan 6, 2015 at 10:37 AM, Ludovic Desroches
> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Usage of of_gpiochip_add() only solves my issue about gpio but not about
> > pinctrl stuff, I still need a patch to manage the case when we have a gap if
> > a gpio controller is not enabled to not break the pin naming, etc.
> 
> This has been the topic of many threads today.
> I assume you are talking about keeping GPIO numbers
> consistent.
> 
> - My suggestion is to add alias handling of the GPIO chips
>   to the core so they can be probed in the right order.

We are already using aliases but it seems to not be the perfect
solution. For example, at the probe time, we wait for all gpio
controllers to be probed. We fill a gpio_chips table whose position in
this table is the alias id of the gpio controller. The at91 pinctrl driver
is waiting for 'maximum alias id' gpio controllers. What happens if
don't want to use a gpio controller and don't declare it or set it as
disabled?

> 
> - For consistency in sysfs use the "names" array in
>   struct gpio_chip so you can search for a symbolic
>   name in sysfs and don't have to rely on fragile stuff
>   like GPIO numbers.
> 
> - Partake in the development of a new GPIO ABI
>   that does not use the global GPIO numberspace.

I will have a look and bring my humble contribution if I can but I think
this topic is far away from the fix I am sending.

As you notice we can improve the at91 pinctrl driver, removing
pinctrl_add_gpio_range() and the range computation in the driver but it
won't solve my issue and it involves to add the gpio-range property to
all our devices so it breaks backward compatibility with old dtb.

>From my point of view, it is two distinct topics. One is a very
important fix because our SAMA5D4 device is not booting without it. The
other one is a proper way to manage gpio ranges but alone I don't think
it can solve my issue.


Regards

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

[PATCH RFC] pinctrl: at91

[Ludovic Desroches]

On Wed, Jan 14, 2015 at 01:26:16PM +0100, Linus Walleij wrote:
> On Tue, Jan 6, 2015 at 10:37 AM, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> 
> > Usage of of_gpiochip_add() only solves my issue about gpio but not about
> > pinctrl stuff, I still need a patch to manage the case when we have a gap if
> > a gpio controller is not enabled to not break the pin naming, etc.
> 
> This has been the topic of many threads today.
> I assume you are talking about keeping GPIO numbers
> consistent.
> 
> - My suggestion is to add alias handling of the GPIO chips
>   to the core so they can be probed in the right order.

We are already using aliases but it seems to not be the perfect
solution. For example, at the probe time, we wait for all gpio
controllers to be probed. We fill a gpio_chips table whose position in
this table is the alias id of the gpio controller. The at91 pinctrl driver
is waiting for 'maximum alias id' gpio controllers. What happens if
don't want to use a gpio controller and don't declare it or set it as
disabled?

> 
> - For consistency in sysfs use the "names" array in
>   struct gpio_chip so you can search for a symbolic
>   name in sysfs and don't have to rely on fragile stuff
>   like GPIO numbers.
> 
> - Partake in the development of a new GPIO ABI
>   that does not use the global GPIO numberspace.

I will have a look and bring my humble contribution if I can but I think
this topic is far away from the fix I am sending.

As you notice we can improve the at91 pinctrl driver, removing
pinctrl_add_gpio_range() and the range computation in the driver but it
won't solve my issue and it involves to add the gpio-range property to
all our devices so it breaks backward compatibility with old dtb.

>From my point of view, it is two distinct topics. One is a very
important fix because our SAMA5D4 device is not booting without it. The
other one is a proper way to manage gpio ranges but alone I don't think
it can solve my issue.


Regards

Ludovic

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Jean-Christophe PLAGNIOL-VILLARD]

[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]

On 17:49 Fri 28 Nov     , Ludovic Desroches wrote:
> This patch allows to have gpio controller with status set to disabled.
> 
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> 
> Hi,
> 
> Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
> support unused gpio bank.

the problem is different

The current code does support this partially

but need to have few by using gpio_banks for the pinctrl part instead of
info->banks
as we already known the last enabled bank

> 
> 
>  drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 94643bb..95ae122 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
> -			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nbanks++;
>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>  	}
>  
>  	size /= sizeof(*list);
> -	if (!size || size % info->nbanks) {
> +	if (!size || size % gpio_banks) {
>  		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
>  		return -EINVAL;
>  	}
> -	info->nmux = size / info->nbanks;
> +	info->nmux = size / gpio_banks;
>  
>  	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
>  	if (!info->mux_mask) {
> @@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * to obtain references to the struct gpio_chip * for them, and we
>  	 * need this to proceed.
>  	 */
> -	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +	for (i = 0; i < gpio_banks; i++) {
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nbanks) {
> +		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	/* We will handle a range of GPIO pins */
> -	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +	for (i = 0; i < gpio_banks; i++)
> +		if (gpio_chips[i])
> +			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;

here Nack

as if you disable ANY of the bank that use pioc you break the IRQ support

attached a patch we the idea how to handle the current issue
only compiled not tested (too late here)

Best Regards,
J.
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> -- 
> 2.0.3
> 

[-- Attachment #2: 0001-pinctrl-at91-allow-to-have-disable-gpio-bank.patch --]
[-- Type: text/x-diff, Size: 7649 bytes --]

>From c94cb27d4d4feb2d3af8d7eeffa34f8ea9484db7 Mon Sep 17 00:00:00 2001
From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Date: Thu, 15 Jan 2015 04:00:46 +0800
Subject: [PATCH 1/1] pinctrl: at91: allow to have disable gpio bank

Today we expect that all the bank are enabled, and count the number of banks
used by the pinctrl based on it instead of using the last bank id enabled.

So switch to it

And set the chained IRQ at runtime based on enabled banks

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/pinctrl/pinctrl-at91.c | 94 ++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 50 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 354a81d..170e67f 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -655,12 +655,18 @@ static int pin_check_config(struct at91_pinctrl *info, const char *name,
 	int mux;
 
 	/* check if it's a valid config */
-	if (pin->bank >= info->nbanks) {
+	if (pin->bank >= gpio_banks) {
 		dev_err(info->dev, "%s: pin conf %d bank_id %d >= nbanks %d\n",
-			name, index, pin->bank, info->nbanks);
+			name, index, pin->bank, gpio_banks);
 		return -EINVAL;
 	}
 
+	if (!gpio_chips[pin->bank]) {
+		dev_err(info->dev, "%s: pin conf %d bank_id %d not enabled\n",
+			name, index, pin->bank);
+		return -ENXIO;
+	}
+
 	if (pin->pin >= MAX_NB_GPIO_PER_BANK) {
 		dev_err(info->dev, "%s: pin conf %d pin_bank_id %d >= %d\n",
 			name, index, pin->pin, MAX_NB_GPIO_PER_BANK);
@@ -982,12 +988,10 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 	struct device_node *child;
 
 	for_each_child_of_node(np, child) {
-		if (of_device_is_compatible(child, gpio_compat)) {
-			info->nbanks++;
-		} else {
-			info->nfunctions++;
-			info->ngroups += of_get_child_count(child);
-		}
+		if (of_device_is_compatible(child, gpio_compat))
+			continue;
+		info->nfunctions++;
+		info->ngroups += of_get_child_count(child);
 	}
 }
 
@@ -1005,11 +1009,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
 	}
 
 	size /= sizeof(*list);
-	if (!size || size % info->nbanks) {
-		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
+	if (!size || size % gpio_banks) {
+		dev_err(info->dev, "wrong mux mask array should be by %d\n", gpio_banks);
 		return -EINVAL;
 	}
-	info->nmux = size / info->nbanks;
+	info->nmux = size / gpio_banks;
 
 	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
 	if (!info->mux_mask) {
@@ -1133,7 +1137,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
 	at91_pinctrl_child_count(info, np);
 
-	if (info->nbanks < 1) {
+	if (gpio_banks < 1) {
 		dev_err(&pdev->dev, "you need to specify at least one gpio-controller\n");
 		return -EINVAL;
 	}
@@ -1146,7 +1150,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 
 	dev_dbg(&pdev->dev, "mux-mask\n");
 	tmp = info->mux_mask;
-	for (i = 0; i < info->nbanks; i++) {
+	for (i = 0; i < gpio_banks; i++) {
 		for (j = 0; j < info->nmux; j++, tmp++) {
 			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
 		}
@@ -1164,7 +1168,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	if (!info->groups)
 		return -ENOMEM;
 
-	dev_dbg(&pdev->dev, "nbanks = %d\n", info->nbanks);
+	dev_dbg(&pdev->dev, "nbanks = %d\n", gpio_banks);
 	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
 	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
 
@@ -1202,7 +1206,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	 * to obtain references to the struct gpio_chip * for them, and we
 	 * need this to proceed.
 	 */
-	for (i = 0; i < info->nbanks; i++) {
+	for (i = 0; i < gpio_banks; i++) {
 		if (!gpio_chips[i]) {
 			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
 			devm_kfree(&pdev->dev, info);
@@ -1211,14 +1215,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
-	at91_pinctrl_desc.npins = info->nbanks * MAX_NB_GPIO_PER_BANK;
+	at91_pinctrl_desc.npins = gpio_banks * MAX_NB_GPIO_PER_BANK;
 	at91_pinctrl_desc.pins = pdesc =
 		devm_kzalloc(&pdev->dev, sizeof(*pdesc) * at91_pinctrl_desc.npins, GFP_KERNEL);
 
 	if (!at91_pinctrl_desc.pins)
 		return -ENOMEM;
 
-	for (i = 0 , k = 0; i < info->nbanks; i++) {
+	for (i = 0 , k = 0; i < gpio_banks; i++) {
 		for (j = 0; j < MAX_NB_GPIO_PER_BANK; j++, k++) {
 			pdesc->number = k;
 			pdesc->name = kasprintf(GFP_KERNEL, "pio%c%d", i + 'A', j);
@@ -1236,7 +1240,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	/* We will handle a range of GPIO pins */
-	for (i = 0; i < info->nbanks; i++)
+	for (i = 0; i < gpio_banks; i++)
 		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
 
 	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
@@ -1614,9 +1618,10 @@ static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
 static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 				  struct at91_gpio_chip *at91_gpio)
 {
+	struct gpio_chip	*gpiochip_prev = NULL;
 	struct at91_gpio_chip   *prev = NULL;
 	struct irq_data		*d = irq_get_irq_data(at91_gpio->pioc_virq);
-	int ret;
+	int ret, i;
 
 	at91_gpio->pioc_hwirq = irqd_to_hwirq(d);
 
@@ -1642,24 +1647,33 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 		return ret;
 	}
 
-	/* Setup chained handler */
-	if (at91_gpio->pioc_idx)
-		prev = gpio_chips[at91_gpio->pioc_idx - 1];
-
 	/* The top level handler handles one bank of GPIOs, except
 	 * on some SoC it can handle up to three...
 	 * We only set up the handler for the first of the list.
 	 */
-	if (prev && prev->next == at91_gpio)
+	gpiochip_prev = irq_get_handler_data(at91_gpio->pioc_virq);
+	if (!gpiochip_prev) {
+		/* Then register the chain on the parent IRQ */
+		gpiochip_set_chained_irqchip(&at91_gpio->chip,
+					     &gpio_irqchip,
+					     at91_gpio->pioc_virq,
+					     gpio_irq_handler);
 		return 0;
+	}
 
-	/* Then register the chain on the parent IRQ */
-	gpiochip_set_chained_irqchip(&at91_gpio->chip,
-				     &gpio_irqchip,
-				     at91_gpio->pioc_virq,
-				     gpio_irq_handler);
+	prev = container_of(gpiochip_prev, struct at91_gpio_chip, chip);
 
-	return 0;
+	/* we can only have 2 banks before */
+	for (i = 0; i < 2; i++) {
+		if (prev->next) {
+			prev = prev->next;
+		} else {
+			prev->next = at91_gpio;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
 }
 
 /* This structure is replicated for each GPIO block allocated at probe time */
@@ -1676,24 +1690,6 @@ static struct gpio_chip at91_gpio_template = {
 	.ngpio			= MAX_NB_GPIO_PER_BANK,
 };
 
-static void at91_gpio_probe_fixup(void)
-{
-	unsigned i;
-	struct at91_gpio_chip *at91_gpio, *last = NULL;
-
-	for (i = 0; i < gpio_banks; i++) {
-		at91_gpio = gpio_chips[i];
-
-		/*
-		 * GPIO controller are grouped on some SoC:
-		 * PIOC, PIOD and PIOE can share the same IRQ line
-		 */
-		if (last && last->pioc_virq == at91_gpio->pioc_virq)
-			last->next = at91_gpio;
-		last = at91_gpio;
-	}
-}
-
 static struct of_device_id at91_gpio_of_match[] = {
 	{ .compatible = "atmel,at91sam9x5-gpio", .data = &at91sam9x5_ops, },
 	{ .compatible = "atmel,at91rm9200-gpio", .data = &at91rm9200_ops },
@@ -1806,8 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	gpio_chips[alias_idx] = at91_chip;
 	gpio_banks = max(gpio_banks, alias_idx + 1);
 
-	at91_gpio_probe_fixup();
-
 	ret = at91_gpio_of_irq_setup(pdev, at91_chip);
 	if (ret)
 		goto irq_setup_err;
-- 
2.1.3


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:49 Fri 28 Nov     , Ludovic Desroches wrote:
> This patch allows to have gpio controller with status set to disabled.
> 
> gpio_banks represents all the gpio banks available on the device whereas
> nbanks represents the gpio banks used. Having a disabled gpio controller
> involves that nbanks value is lower than gpio_banks and that some
> pointers in the gpio_chips array are NULL. This patch deals with these
> specific cases.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> 
> Hi,
> 
> Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't
> support unused gpio bank.

the problem is different

The current code does support this partially

but need to have few by using gpio_banks for the pinctrl part instead of
info->banks
as we already known the last enabled bank

> 
> 
>  drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 94643bb..95ae122 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  
>  	for_each_child_of_node(np, child) {
>  		if (of_device_is_compatible(child, gpio_compat)) {
> -			info->nbanks++;
> +			if (of_device_is_available(child))
> +				info->nbanks++;
>  		} else {
>  			info->nfunctions++;
>  			info->ngroups += of_get_child_count(child);
> @@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>  	}
>  
>  	size /= sizeof(*list);
> -	if (!size || size % info->nbanks) {
> +	if (!size || size % gpio_banks) {
>  		dev_err(info->dev, "wrong mux mask array should be by %d\n", info->nbanks);
>  		return -EINVAL;
>  	}
> -	info->nmux = size / info->nbanks;
> +	info->nmux = size / gpio_banks;
>  
>  	info->mux_mask = devm_kzalloc(info->dev, sizeof(u32) * size, GFP_KERNEL);
>  	if (!info->mux_mask) {
> @@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k;
> +	int ret, i, j, k, ngpio_chips_enabled = 0;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	 * to obtain references to the struct gpio_chip * for them, and we
>  	 * need this to proceed.
>  	 */
> -	for (i = 0; i < info->nbanks; i++) {
> -		if (!gpio_chips[i]) {
> -			dev_warn(&pdev->dev, "GPIO chip %d not registered yet\n", i);
> -			devm_kfree(&pdev->dev, info);
> -			return -EPROBE_DEFER;
> -		}
> +	for (i = 0; i < gpio_banks; i++) {
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +	}
> +	if (ngpio_chips_enabled < info->nbanks) {
> +		dev_warn(&pdev->dev, "All GPIO chips are not registered yet\n");
> +		devm_kfree(&pdev->dev, info);
> +		return -EPROBE_DEFER;
>  	}
>  
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
> @@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	/* We will handle a range of GPIO pins */
> -	for (i = 0; i < info->nbanks; i++)
> -		pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
> +	for (i = 0; i < gpio_banks; i++)
> +		if (gpio_chips[i])
> +			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
>  
>  	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
>  
> @@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)
>  
>  	for (i = 0; i < gpio_banks; i++) {
>  		at91_gpio = gpio_chips[i];
> +		if (!at91_gpio)
> +			continue;

here Nack

as if you disable ANY of the bank that use pioc you break the IRQ support

attached a patch we the idea how to handle the current issue
only compiled not tested (too late here)

Best Regards,
J.
>  
>  		/*
>  		 * GPIO controller are grouped on some SoC:
> -- 
> 2.0.3
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pinctrl-at91-allow-to-have-disable-gpio-bank.patch
Type: text/x-diff
Size: 7649 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150114/b2a3e16e/attachment-0001.bin>

Re: [PATCH RFC] pinctrl: at91

[Linus Walleij]

On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> From my point of view, it is two distinct topics. One is a very
> important fix because our SAMA5D4 device is not booting without it. The
> other one is a proper way to manage gpio ranges but alone I don't think
> it can solve my issue.

Can you make me a minimal, sane patch that fixes the boot regression?

I hate regressions so these need to be fixed first...

The rest we can discuss I guess.

Or is it not possible to solve the boot regression without the larger
fix?

Yours,
Linus Walleij
--
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

[PATCH RFC] pinctrl: at91

[Linus Walleij]

On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:

> From my point of view, it is two distinct topics. One is a very
> important fix because our SAMA5D4 device is not booting without it. The
> other one is a proper way to manage gpio ranges but alone I don't think
> it can solve my issue.

Can you make me a minimal, sane patch that fixes the boot regression?

I hate regressions so these need to be fixed first...

The rest we can discuss I guess.

Or is it not possible to solve the boot regression without the larger
fix?

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Wed, Jan 14, 2015 at 9:45 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:

> attached a patch we the idea how to handle the current issue
> only compiled not tested (too late here)

Ludovic have you tested Jean-Christophe's patch?

Does it solve the boot issue?

Yours,
Linus Walleij
--
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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Linus Walleij]

On Wed, Jan 14, 2015 at 9:45 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:

> attached a patch we the idea how to handle the current issue
> only compiled not tested (too late here)

Ludovic have you tested Jean-Christophe's patch?

Does it solve the boot issue?

Yours,
Linus Walleij

Re: [PATCH RFC] pinctrl: at91

[Ludovic Desroches]

On Mon, Jan 19, 2015 at 11:12:45AM +0100, Linus Walleij wrote:
> On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
> > From my point of view, it is two distinct topics. One is a very
> > important fix because our SAMA5D4 device is not booting without it. The
> > other one is a proper way to manage gpio ranges but alone I don't think
> > it can solve my issue.
> 
> Can you make me a minimal, sane patch that fixes the boot regression?
> 
> I hate regressions so these need to be fixed first...
> 
> The rest we can discuss I guess.
> 
> Or is it not possible to solve the boot regression without the larger
> fix?

No I don't think we can have a smaller patch than this one to fix it.
Next step is to decide if we go further as you suggested, knowing that we
would have to modify the device tree. Moreover we will have to rework
our pinctrl driver (or write a new one) since we will have new pio
controller on future devices.

Regards

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

[PATCH RFC] pinctrl: at91

[Ludovic Desroches]

On Mon, Jan 19, 2015 at 11:12:45AM +0100, Linus Walleij wrote:
> On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> 
> > From my point of view, it is two distinct topics. One is a very
> > important fix because our SAMA5D4 device is not booting without it. The
> > other one is a proper way to manage gpio ranges but alone I don't think
> > it can solve my issue.
> 
> Can you make me a minimal, sane patch that fixes the boot regression?
> 
> I hate regressions so these need to be fixed first...
> 
> The rest we can discuss I guess.
> 
> Or is it not possible to solve the boot regression without the larger
> fix?

No I don't think we can have a smaller patch than this one to fix it.
Next step is to decide if we go further as you suggested, knowing that we
would have to modify the device tree. Moreover we will have to rework
our pinctrl driver (or write a new one) since we will have new pio
controller on future devices.

Regards

Ludovic

Re: [PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Jan 19, 2015 at 11:16:30AM +0100, Linus Walleij wrote:
> On Wed, Jan 14, 2015 at 9:45 PM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> 
> > attached a patch we the idea how to handle the current issue
> > only compiled not tested (too late here)
> 
> Ludovic have you tested Jean-Christophe's patch?
> 
> Does it solve the boot issue?

Yes I have tested it. I have send a new version since it doesn't totally
solved the boot issue. It corrects some potential problems I have missed
but there is still an error in the pinctrl probe.

Regards

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

[PATCH 1/2] pinctrl: at91: allow disabled gpio controllers

[Ludovic Desroches]

On Mon, Jan 19, 2015 at 11:16:30AM +0100, Linus Walleij wrote:
> On Wed, Jan 14, 2015 at 9:45 PM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
> 
> > attached a patch we the idea how to handle the current issue
> > only compiled not tested (too late here)
> 
> Ludovic have you tested Jean-Christophe's patch?
> 
> Does it solve the boot issue?

Yes I have tested it. I have send a new version since it doesn't totally
solved the boot issue. It corrects some potential problems I have missed
but there is still an error in the pinctrl probe.

Regards

Ludovic

Re: [PATCH RFC] pinctrl: at91

[Nicolas Ferre]

Le 19/01/2015 11:30, Ludovic Desroches a écrit :
> On Mon, Jan 19, 2015 at 11:12:45AM +0100, Linus Walleij wrote:
>> On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
>> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> From my point of view, it is two distinct topics. One is a very
>>> important fix because our SAMA5D4 device is not booting without it. The
>>> other one is a proper way to manage gpio ranges but alone I don't think
>>> it can solve my issue.
>>
>> Can you make me a minimal, sane patch that fixes the boot regression?
>>
>> I hate regressions so these need to be fixed first...
>>
>> The rest we can discuss I guess.
>>
>> Or is it not possible to solve the boot regression without the larger
>> fix?
> 
> No I don't think we can have a smaller patch than this one to fix it.
> Next step is to decide if we go further as you suggested, knowing that we
> would have to modify the device tree. Moreover we will have to rework
> our pinctrl driver (or write a new one) since we will have new pio
> controller on future devices.

Yes, all the upcoming AT91 SoCs will embed a totally new pinctrl/pinmux
IP. We will write a new driver for it and we will be able to use the
up-to-date standards of the framework.

In the meantime, this fix allows us to use the SAMA5D4 without too much
changes to the original version of our current driver and without
modification to all the current SoCs device trees.

Bye,
-- 
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

[PATCH RFC] pinctrl: at91

[Nicolas Ferre]

Le 19/01/2015 11:30, Ludovic Desroches a ?crit :
> On Mon, Jan 19, 2015 at 11:12:45AM +0100, Linus Walleij wrote:
>> On Wed, Jan 14, 2015 at 4:03 PM, Ludovic Desroches
>> <ludovic.desroches@atmel.com> wrote:
>>
>>> From my point of view, it is two distinct topics. One is a very
>>> important fix because our SAMA5D4 device is not booting without it. The
>>> other one is a proper way to manage gpio ranges but alone I don't think
>>> it can solve my issue.
>>
>> Can you make me a minimal, sane patch that fixes the boot regression?
>>
>> I hate regressions so these need to be fixed first...
>>
>> The rest we can discuss I guess.
>>
>> Or is it not possible to solve the boot regression without the larger
>> fix?
> 
> No I don't think we can have a smaller patch than this one to fix it.
> Next step is to decide if we go further as you suggested, knowing that we
> would have to modify the device tree. Moreover we will have to rework
> our pinctrl driver (or write a new one) since we will have new pio
> controller on future devices.

Yes, all the upcoming AT91 SoCs will embed a totally new pinctrl/pinmux
IP. We will write a new driver for it and we will be able to use the
up-to-date standards of the framework.

In the meantime, this fix allows us to use the SAMA5D4 without too much
changes to the original version of our current driver and without
modification to all the current SoCs device trees.

Bye,
-- 
Nicolas Ferre