[PATCH RFC] pinctrl: at91

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 :

quoted

Signed-off-by: Ludovic Desroches <redacted>
---

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

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:

quoted

 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.

quoted

 					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?

quoted

 		} 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