Right amount of info in the DT

Right amount of info in the DT

[Yves Lefloch]

Hello everybody,

I'm a kernel newbie trying to write a GPIO driver for my platform (ARM tango),
and I'd need some advice regarding the oddity of my hardware.

I have a bunch of GPIO groups all over the place (dedicated pins, UART pins,
ethernet pins, etc.), and even though they mainly behave similarly (same way of
handling direction and value), there are some differences between them:
- IRQs, because some groups can generate interrupts and others can't;
- Alternate functions (for instance GPIO mode vs UART mode), because obviously
  dedicated pins don't have an alternate function, and for the others which have
  it, the mode-changing register is sometimes before, sometimes after the other
  regs.

My question is: where to encode this information?

I've noticed that in other device trees, there's an irq-controller property that
could probably handle the first case, but for the second, I'm not sure what I
should do. I've thought of:
1) Making up a property that would tell the driver the offset of the mode
   register in the resource, but I'm afraid this is encoding too much HW
   info in the DT.
2) Making up a property or perhaps different compatible strings that would tell
   the driver which kind of GPIO group it's dealing with, but that would maybe
   make the driver quite ad-hoc.
3) Put in another resource to deal with the mode register, but the thing is,
   this register must not be written the same way for all groups.
4) Encoding all the information (offsets, number of GPIOs, etc.) in a const
   array within the driver, but that would defeat the purpose of a DT, would
   it not?

Any help appreciated!

Yves Lefloch

Re: Right amount of info in the DT

[Linus Walleij]

On Thu, Apr 20, 2017 at 11:54 AM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> I have a bunch of GPIO groups all over the place (dedicated pins, UART pins,
> ethernet pins, etc.), and even though they mainly behave similarly (same way of
> handling direction and value), there are some differences between them:
> - IRQs, because some groups can generate interrupts and others can't;
> - Alternate functions (for instance GPIO mode vs UART mode), because obviously
>   dedicated pins don't have an alternate function, and for the others which have
>   it, the mode-changing register is sometimes before, sometimes after the other
>   regs.

The basic problem is that you conceptualize of these things as GPIO
and not the more abstract concept of pins under pin control.
For example, there are not GPIO groups, there are pin control pin
multiplexing groups.

Please read Documentation/pinctrl.txt
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/pinctrl.txt

Also read the generic pin control device tree bindings:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Further in the GPIO subsystem you will find references back to how it
interacts with pin control.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

Hello Linus, thanks for reply.

OK I see now that I have to do this kind of stuff in a pinctrl/pinmux driver.

I've read the documentation you pointed to and browsed a few existing drivers,
but I'm still a bit confused by how my device tree should look like. Indeed in
my SoC there is no central hw pin controller, and pin control registers are
scattered all over the address space because they're in different IPs. How am I
supposed to deal with so many different addresses? I have considered hardcoding
them in the driver but that seems counter-productive, so I thought I could do
something like this, with the address of the function-select register as a
resource (it's the only pin property I can change besides direction and data):


pincontroller {
        compatible = "foo-pinctrl";
        #address-cells = <1>;
        #size-cells = <0>;

        uart1 {
                reg = <0x10001>;

                state_0 {
                        groups = "uart1";
                        function = "uart1";
                };
                state_1 {
                        groups = "uart1";
                        function = "gpio";
                };

        ... other IPs (SPI, ethernet, etc.) ...

};


Furthermore, I was wondering how it's possible for pinctrl drivers to map registers
possibly used by other drivers. For example, the "GPIO pad mode" register of my
ethernet pins is right in the middle of the address space requested by the ethernet
driver. Meaning that if I try to "devm_ioremap_resource" when probing the GPIO or
pinctrl drivers, I will get an error saying that the memory region is already taken.
Do you have any insight on that?

Regards,
Yves.


On 04/24/2017 10:23 AM, Linus Walleij wrote:
> On Thu, Apr 20, 2017 at 11:54 AM, Yves Lefloch
> <YvesMarie_Lefloch@sigmadesigns.com> wrote:
> 
>> I have a bunch of GPIO groups all over the place (dedicated pins, UART pins,
>> ethernet pins, etc.), and even though they mainly behave similarly (same way of
>> handling direction and value), there are some differences between them:
>> - IRQs, because some groups can generate interrupts and others can't;
>> - Alternate functions (for instance GPIO mode vs UART mode), because obviously
>>   dedicated pins don't have an alternate function, and for the others which have
>>   it, the mode-changing register is sometimes before, sometimes after the other
>>   regs.
> 
> The basic problem is that you conceptualize of these things as GPIO
> and not the more abstract concept of pins under pin control.
> For example, there are not GPIO groups, there are pin control pin
> multiplexing groups.
> 
> Please read Documentation/pinctrl.txt
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/pinctrl.txt
> 
> Also read the generic pin control device tree bindings:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> 
> Further in the GPIO subsystem you will find references back to how it
> interacts with pin control.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt
> 
> Yours,
> Linus Walleij
> 

Re: Right amount of info in the DT

[Linus Walleij]

On Mon, Apr 24, 2017 at 3:03 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> OK I see now that I have to do this kind of stuff in a pinctrl/pinmux driver.

Great!

> in
> my SoC there is no central hw pin controller, and pin control registers are
> scattered all over the address space because they're in different IPs. How am I
> supposed to deal with so many different addresses?

I've herd about this happening sometimes... I haven't seen a perfect solution
for it.

> I have considered hardcoding
> them in the driver but that seems counter-productive, so I thought I could do
> something like this, with the address of the function-select register as a
> resource (it's the only pin property I can change besides direction and data):
>
>
> pincontroller {
>         compatible = "foo-pinctrl";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         uart1 {
>                 reg = <0x10001>;
>
>                 state_0 {
>                         groups = "uart1";
>                         function = "uart1";
>                 };
>                 state_1 {
>                         groups = "uart1";
>                         function = "gpio";
>                 };
>
>         ... other IPs (SPI, ethernet, etc.) ...
>
> };

Maybe, I would have to check the final result. It will need some serious
documentation so that we can establish this way of dealing with this type of
hardware.

> Furthermore, I was wondering how it's possible for pinctrl drivers to map registers
> possibly used by other drivers.

We usually use syscon approaches for that. Especially when it considers
individual bits in a single register.

If you just have one single 32bit register  in each hardware blocks,
then certainly
it can be remapped by several drivers, or you can leave a "hole" there in the
main driver register definitions, but that seems very awkward.

> For example, the "GPIO pad mode" register of my
> ethernet pins is right in the middle of the address space requested by the ethernet
> driver. Meaning that if I try to "devm_ioremap_resource" when probing the GPIO or
> pinctrl drivers, I will get an error saying that the memory region is already taken.
> Do you have any insight on that?

Isn't the best approach to add a pin controller as a subnode of the ethernet
controller in the device tree, so that the probe() of you main driver looks
for it and instantiates a pin controller that is then referenced by the
driver itself?

We have similar constructions with e.g. PCI irq controllers inside of PCI
bridge nodes.

Then, if the same type of pin control is
sprinkled all over your drivers, maybe turn it into a library in drivers/pinctrl
that you can reuse from each driver so as to centralize the code?

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 04/24/2017 06:45 PM, Linus Walleij wrote:
> Isn't the best approach to add a pin controller as a subnode of the ethernet
> controller in the device tree, so that the probe() of you main driver looks
> for it and instantiates a pin controller that is then referenced by the
> driver itself?

I guess that could do it, but I'm not confident enough in my kernel abilities
to modify such a big driver just yet (drivers/net/ethernet/aurora).
I've decided not to support ethernet pins, at least for the first iteration
of the driver.

I've put together a first draft, would you mind taking a look at it?
I haven't documented the DT binding yet, though.

Regards,
Yves.



From: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
Date: Tue, 25 Apr 2017 14:06:46 +0200
Subject: [PATCH] drivers: pinctrl: Add a pinctrl for mach-tango

Signed-off-by: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-common.dtsi |  98 ++++++++
 arch/arm/configs/tango4_defconfig    |   2 +
 arch/arm/mach-tango/Kconfig          |   1 +
 drivers/pinctrl/Kconfig              |   7 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-tango.c      | 421 +++++++++++++++++++++++++++++++++++
 6 files changed, 530 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tango.c

diff --git a/arch/arm/boot/dts/tango4-common.dtsi b/arch/arm/boot/dts/tango4-common.dtsi
index a3e81ab..5e89af9 100644
--- a/arch/arm/boot/dts/tango4-common.dtsi
+++ b/arch/arm/boot/dts/tango4-common.dtsi
@@ -18,6 +18,104 @@
 	#address-cells = <1>;
 	#size-cells = <1>;

+	pins {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		pinctrl@10500 {
+			reg = <0x10500 0x8>;
+			compatible = "sigma,smp8758-pinctrl";
+			label = "sys";
+			tango,pins = <0 16>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			gpio {
+				gpio-controller;
+				#gpio-cells = <1>;
+			};
+		};
+
+		pinctrl@15b8e0 {
+			compatible = "sigma,smp8758-pinctrl";
+			label = "tdmux";
+			reg = <0x15b8e0 0xc>;
+			tango,pins = <16 2>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			gpio {
+				gpio-controller;
+				#gpio-cells = <1>;
+			};
+
+			alt_function {
+				function = "tdmux";
+				groups = "tdmux";
+			};
+		};
+
+		pinctrl@6c130 {
+			compatible = "sigma,smp8758-pinctrl";
+			label = "spi";
+			reg = <0x6c130 0xc>;
+			tango,pins = <18 8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			gpio {
+				gpio-controller;
+				#gpio-cells = <1>;
+			};
+
+			alt_function {
+				function = "spi";
+				groups = "spi";
+			};
+		};
+
+		pinctrl@6c230 {
+			compatible = "sigma,smp8758-pinctrl";
+			label = "uart1";
+			reg = <0x6c230 0xc>;
+			tango,pins = <26 7>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			gpio {
+				gpio-controller;
+				#gpio-cells = <1>;
+			};
+
+			alt_function {
+				function = "uart1";
+				groups = "uart1";
+			};
+
+		};
+
+		pinctrl@6c358 {
+			compatible = "sigma,smp8758-pinctrl";
+			label = "smcard";
+			reg = <0x6c368 0xc>;
+			tango,pins = <71 8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			gpio {
+				gpio-controller;
+				#gpio-cells = <1>;
+			};
+
+			alt_function {
+				function = "smcard";
+				groups = "smcard";
+			};
+		};
+	};
+
 	periph_clk: periph_clk {
 		compatible = "fixed-factor-clock";
 		clocks = <&clkgen CPU_CLK>;
diff --git a/arch/arm/configs/tango4_defconfig b/arch/arm/configs/tango4_defconfig
index b26bb4e..c973758 100644
--- a/arch/arm/configs/tango4_defconfig
+++ b/arch/arm/configs/tango4_defconfig
@@ -68,7 +68,9 @@ CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C=y
 CONFIG_I2C_XLR=y
+CONFIG_PINCTRL_TANGO=m
 CONFIG_GPIOLIB=y
+CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_CPU_THERMAL=y
 CONFIG_TANGO_THERMAL=y
diff --git a/arch/arm/mach-tango/Kconfig b/arch/arm/mach-tango/Kconfig
index ebe15b9..479c9aa 100644
--- a/arch/arm/mach-tango/Kconfig
+++ b/arch/arm/mach-tango/Kconfig
@@ -11,3 +11,4 @@ config ARCH_TANGO
 	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD
 	select TANGO_IRQ
+	select PINCTRL
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0e75d94..5648e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -179,6 +179,13 @@ config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP

+config PINCTRL_TANGO
+	tristate "Tango pin control driver"
+	depends on OF
+	select PINMUX
+	help
+	  Say yes to activate the pinctrl/gpio driver for Tango chips.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 11bad37..d5563e49 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
+obj-$(CONFIG_PINCTRL_TANGO)	+= pinctrl-tango.o
 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
diff --git a/drivers/pinctrl/pinctrl-tango.c b/drivers/pinctrl/pinctrl-tango.c
new file mode 100644
index 0000000..1112047
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tango.c
@@ -0,0 +1,421 @@
+/*
+ * Copyright (C) 2017 Sigma Designs
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+/* Pin regs are laid out this way:
+ * ______________________________
+ * | bitwise mask | bitwise val |
+ * 32             16            0
+ *
+ * To set bit 1, one must write (1 << (16+1) | 1).
+ *
+ * Because of this layout, there are no more than 16 pins in a reg.
+ */
+
+/*
+ * Offsets from `addr' in `struct tango_pinctrl'.
+ * The `mod' register isn't there for pins dedicated to GPIO.
+ */
+#define GPIO_OFF_DIR 0x0
+#define GPIO_OFF_VAL 0x4
+#define GPIO_OFF_MOD 0x8
+
+struct tango_pinctrl {
+	const char *label;
+	u8 id;
+	u8 len;
+	bool dedicated;
+	void __iomem *addr;
+
+	struct pinctrl_desc pd;
+	struct pinctrl_gpio_range gr;
+};
+
+static int pinreg_get_bit(void __iomem *addr, unsigned offset)
+{
+	unsigned long val = readl(addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(val & GENMASK(31, 16));
+#endif
+
+	return test_bit(offset, &val);
+}
+
+static void pinreg_set_bit(void __iomem *addr, unsigned offset, int value)
+{
+	unsigned long val = 0;
+
+	/* Enable change by writing upper half-word too. */
+	set_bit(offset + 16, &val);
+	if (value)
+		set_bit(offset, &val);
+
+	writel(val, addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(pinreg_get_bit(addr, offset) != value);
+#endif
+}
+
+static void pinreg_set_multiple(void __iomem *addr, u16 mask, u16 bits)
+{
+	unsigned long val = (mask << 16) | bits;
+
+	writel(val, addr);
+}
+
+static int gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+
+	if (!data)
+		return -EINVAL;
+
+	return pinreg_get_bit(data->addr + GPIO_OFF_VAL, offset);
+}
+
+static void gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+
+	if (!data)
+		return;
+
+	pinreg_set_bit(data->addr + GPIO_OFF_VAL, offset, value);
+}
+
+static int gpio_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+
+	if (!data)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	return !pinreg_get_bit(data->addr + GPIO_OFF_DIR, offset);
+}
+
+static int gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+
+	if (!data)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	pinreg_set_bit(data->addr + GPIO_OFF_DIR, offset, 0);
+
+	return 0;
+}
+
+static int gpio_direction_output(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+
+	if (!data)
+		return -EINVAL;
+
+	/* The output value can be changed before the direction is. */
+	pinreg_set_bit(data->addr + GPIO_OFF_VAL, offset, value);
+
+	/* Bit set means output. */
+	pinreg_set_bit(data->addr + GPIO_OFF_DIR, offset, 1);
+
+	return 0;
+}
+
+static void gpio_set_multiple(struct gpio_chip *gc,
+		unsigned long *mask, unsigned long *bits)
+{
+	u16 _mask = (u16) *mask;
+	u16 _bits = (u16) *bits;
+	struct tango_pinctrl *data = gpiochip_get_data(gc);
+	if (!data)
+		return;
+
+	pinreg_set_multiple(data->addr + GPIO_OFF_VAL, _mask, _bits);
+}
+
+static const unsigned int all_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
+	10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+	20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+	30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
+	40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
+	50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
+	60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
+	70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
+	80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
+	90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
+};
+
+static int get_group_count(struct pinctrl_dev *pctldev)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return -EINVAL;
+
+	return data->dedicated ? 0 : 1;
+}
+
+static const char *get_group_name(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return ERR_PTR(-EINVAL);
+
+	return data->label;
+}
+
+static int get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
+		const unsigned **pins, unsigned *num_pins)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return -EINVAL;
+
+	*pins = all_pins + data->id;
+	*num_pins = data->len;
+
+	return 0;
+}
+
+static const struct pinctrl_ops pctlops = {
+	.get_groups_count = get_group_count,
+	.get_group_name = get_group_name,
+	.get_group_pins = get_group_pins,
+};
+
+int get_function_groups(struct pinctrl_dev *pctldev, unsigned selector,
+		const char * const **groups, unsigned *num_groups)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return -EINVAL;
+
+	*groups = &data->label;
+	*num_groups = 1;
+
+	return 0;
+}
+
+int set_mux(struct pinctrl_dev *pctldev, unsigned func_selector,
+		unsigned group_selector)
+{
+	/* With only one possible alt function, muxing is only
+	 * necessary for GPIO request/free.
+	 */
+	return 0;
+}
+
+int gpio_request_enable(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return -EINVAL;
+
+	if (offset >= data->len)
+		return -EINVAL;
+
+	if (!data->dedicated)
+		pinreg_set_bit(data->addr + GPIO_OFF_MOD, offset, 1);
+
+	return 0;
+}
+
+void gpio_disable_free(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct tango_pinctrl *data;
+
+	data = pinctrl_dev_get_drvdata(pctldev);
+	if (!data)
+		return;
+
+	if (offset >= data->len)
+		return;
+
+	if (!data->dedicated)
+		pinreg_set_bit(data->addr + GPIO_OFF_MOD, offset, 0);
+}
+
+const static struct pinmux_ops pmxops = {
+	.get_functions_count = get_group_count,
+	.get_function_name = get_group_name,
+	.get_function_groups = get_function_groups,
+	.set_mux = set_mux,
+	.gpio_request_enable = gpio_request_enable,
+	.gpio_disable_free = gpio_disable_free,
+	.strict = true,
+};
+
+static int get_node_info(struct device_node *np, struct tango_pinctrl *data)
+{
+	/* Extract info from the pinctrl node:
+	 * - the human-friendly name;
+	 * - the base num and the num of pins;
+	 * - whether pins are dedicated to GPIO (no alt state).
+	 */
+	int ret;
+	u32 tango_pins[2];
+
+	ret = of_property_read_string(np, "label", &data->label);
+	if (ret)
+		return ret;
+	ret = of_property_read_u32_array(np, "tango,pins", tango_pins, 2);
+	if (ret)
+		return ret;
+	data->id = tango_pins[0];
+	data->len = tango_pins[1];
+	data->dedicated = !of_get_property(np, "alt_function", NULL);
+
+	return 0;
+}
+
+static int init_pd(struct device *dev, struct tango_pinctrl *data)
+{
+	int i;
+	struct pinctrl_pin_desc *pdesc;
+
+	pdesc = devm_kzalloc(dev, data->len * sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+	for (i = 0; i < data->len; i++)
+		pdesc[i].number = data->id + i;
+
+	data->pd.name = data->label;
+	data->pd.pins = pdesc;
+	data->pd.npins = data->len;
+	data->pd.pctlops = &pctlops;
+	data->pd.pmxops = &pmxops;
+	data->pd.owner = THIS_MODULE;
+
+	return 0;
+}
+
+static int init_gr(struct device *dev, struct tango_pinctrl *data)
+{
+	struct gpio_chip *gc;
+
+	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	data->gr.name = data->label;
+	data->gr.id = data->id;
+	data->gr.base = data->id;
+	data->gr.pin_base = data->id;
+	data->gr.npins = data->len;
+	data->gr.gc = gc;
+
+	gc->label = data->label;
+	gc->parent = dev;
+	gc->owner = THIS_MODULE;
+	gc->get_direction = gpio_get_direction;
+	gc->direction_input = gpio_direction_input;
+	gc->direction_output = gpio_direction_output;
+	gc->get = gpio_get;
+	gc->set = gpio_set;
+	gc->set_multiple = gpio_set_multiple;
+	gc->base = data->id;
+	gc->ngpio = data->len;
+
+	return devm_gpiochip_add_data(dev, gc, data);
+}
+
+static int tango_pinctrl_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *res;
+	struct tango_pinctrl *data;
+	struct pinctrl_dev *pindev;
+	struct device_node *np = pdev->dev.of_node;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, data);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+	data->addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->addr))
+		return PTR_ERR(data->addr);
+
+	ret = get_node_info(np, data);
+	if (ret)
+		return ret;
+	dev_dbg(&pdev->dev, "%8s id=%d len=%d\n",
+			data->label, data->id, data->len);
+
+	ret = init_pd(&pdev->dev, data);
+	if (ret)
+		return ret;
+
+	pindev = devm_pinctrl_register(&pdev->dev, &data->pd, data);
+	if (IS_ERR(pindev))
+		return PTR_ERR(pindev);
+
+	ret = init_gr(&pdev->dev, data);
+	if (ret)
+		return ret;
+	pinctrl_add_gpio_range(pindev, &data->gr);
+
+	return 0;
+}
+
+static const struct of_device_id tango_pinctrl_of_match[] = {
+	{ .compatible =  "sigma,smp8758-pinctrl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tango_pinctrl_of_match);
+
+static struct platform_driver tango_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-tango",
+		.of_match_table = of_match_ptr(tango_pinctrl_of_match),
+	},
+	.probe = tango_pinctrl_probe,
+};
+
+static int __init tango_pinctrl_init(void)
+{
+	return platform_driver_register(&tango_pinctrl_driver);
+}
+module_init(tango_pinctrl_init);
+
+static void __exit tango_pinctrl_exit(void)
+{
+	platform_driver_unregister(&tango_pinctrl_driver);
+}
+module_exit(tango_pinctrl_exit);
+
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango pinctrl driver");
+MODULE_LICENSE("GPL");
--
2.10.1

Re: Right amount of info in the DT

[Linus Walleij]

On Wed, Apr 26, 2017 at 5:47 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> I've put together a first draft, would you mind taking a look at it?
> I haven't documented the DT binding yet, though.

OK

> +       pins {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               pinctrl@10500 {
> +                       reg = <0x10500 0x8>;
> +                       compatible = "sigma,smp8758-pinctrl";
> +                       label = "sys";
> +                       tango,pins = <0 16>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       gpio {
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                       };
> +               };
> +
> +               pinctrl@15b8e0 {
> +                       compatible = "sigma,smp8758-pinctrl";
> +                       label = "tdmux";
> +                       reg = <0x15b8e0 0xc>;
> +                       tango,pins = <16 2>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       gpio {
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                       };
> +
> +                       alt_function {
> +                               function = "tdmux";
> +                               groups = "tdmux";
> +                       };
> +               };
> +
> +               pinctrl@6c130 {
> +                       compatible = "sigma,smp8758-pinctrl";
> +                       label = "spi";
> +                       reg = <0x6c130 0xc>;
> +                       tango,pins = <18 8>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       gpio {
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                       };
> +
> +                       alt_function {
> +                               function = "spi";
> +                               groups = "spi";
> +                       };
> +               };
> +
> +               pinctrl@6c230 {
> +                       compatible = "sigma,smp8758-pinctrl";
> +                       label = "uart1";
> +                       reg = <0x6c230 0xc>;
> +                       tango,pins = <26 7>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       gpio {
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                       };
> +
> +                       alt_function {
> +                               function = "uart1";
> +                               groups = "uart1";
> +                       };
> +
> +               };
> +
> +               pinctrl@6c358 {
> +                       compatible = "sigma,smp8758-pinctrl";
> +                       label = "smcard";
> +                       reg = <0x6c368 0xc>;
> +                       tango,pins = <71 8>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       gpio {
> +                               gpio-controller;
> +                               #gpio-cells = <1>;
> +                       };
> +
> +                       alt_function {
> +                               function = "smcard";
> +                               groups = "smcard";
> +                       };
> +               };
> +       };


Does all of these really have gpio controller functionality?
You do not have to compulsively add that. Just make it
a pin controller.

But I was not meaning you should collect them all under a
"pins" node like this, but inside each peripheral.

So for example:

mmc@80114000 {
        compatible = "mmc-sd-card";
        reg = <0x80114000 0x1000>;
        interrupts = <....>;

        pinctrl {
                compatible = "sigma,smp8758-pinctrl";
                label = "smcard";
                (....)
        };
};

So it lives inside the node of the peripheral. It can then
access the register range by simply looking at the parent,
and maybe when making the Linux driver, the parent simply
calls a library function to set up the pin controller for itself
providing the child node as reference. (Child nodes are not
automatically spawning devices, you have to do that with code.)

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 05/07/2017 12:26 PM, Linus Walleij wrote:
> Does all of these really have gpio controller functionality?
> You do not have to compulsively add that. Just make it
> a pin controller.

Most of them have pin controller functionalities, because the pins
can be muxed between GPIO and another function. Only the first group
is GPIO only (dedicated pins). But apart from the choosing of the
current function, the other properties (direction, value) are handled
the same way.
Regarding that, I was wondering what's the recommended approach to
mutualize code:
1. Having a driver under drivers/gpio and another on drivers/pinctrl
   which depends on the first one.
2. Having both drivers in the same file under drivers/pinctrl.
3. Having only one pinctrl driver.
For now I've taken approach 3, but isn't it strange to have pins
dedicated to GPIO like this first group I mentioned controlled
for a pinctrl driver?
What is your opinion on this?

> But I was not meaning you should collect them all under a
> "pins" node like this, but inside each peripheral.

OK, I will do that. Although not all alternate functions currently
have a driver, nor a node in the device tree. I guess the best way
to handle that is to declare the pins as GPIO only and not to worry
about the alternate function; which brings me back to the question
above. Do they need a separate GPIO driver?

Re: Right amount of info in the DT

[Linus Walleij]

On Tue, May 9, 2017 at 12:49 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> I was wondering what's the recommended approach to
> mutualize code:
> 1. Having a driver under drivers/gpio and another on drivers/pinctrl
>    which depends on the first one.
> 2. Having both drivers in the same file under drivers/pinctrl.
> 3. Having only one pinctrl driver.

All approaches exist. I have no strong opinion, but when possible
try to make separation of concerns.

> For now I've taken approach 3, but isn't it strange to have pins
> dedicated to GPIO like this first group I mentioned controlled
> for a pinctrl driver?
> What is your opinion on this?

Just make sure your driver also provides a gpio_chip and
write good looking code.

>> But I was not meaning you should collect them all under a
>> "pins" node like this, but inside each peripheral.
>
> OK, I will do that. Although not all alternate functions currently
> have a driver, nor a node in the device tree. I guess the best way
> to handle that is to declare the pins as GPIO only and not to worry
> about the alternate function; which brings me back to the question
> above. Do they need a separate GPIO driver?

Implement everything you have a data sheet for even if you
do not use it right now. It is good to have.

Have you read the section "GPIO mode pitfalls" in the documentation?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/pinctrl.txt

It is very important that you know for sure whether it is actual
GPIO or just named that way by some hardware engineer.

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 05/11/2017 05:22 PM, Linus Walleij wrote:
> Just make sure your driver also provides a gpio_chip and
> write good looking code.

OK, IMHO the code I attached earlier would do that.

> Implement everything you have a data sheet for even if you
> do not use it right now. It is good to have.

Do you mean that I should submit a driver for every single device
before I can declare its pins in the DT? That will surely take
quite a long time, and I'm required to get GPIO functionality before
that.

Furthermore, even if I had a driver for a device whose pins can also
be GPIOs, say smartcard. If the driver is built as a module, and has
not been yet loaded in, then, since you want the pinctrl node has a
child node, no gpiochip struct could have been registered, and the pins
can't be used as GPIOs.
Does this mean that to use the smartcard's pin as GPIOs, the smartcard
driver must absolutely be loaded, even though smartcards are never used?

> Have you read the section "GPIO mode pitfalls" in the documentation?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/pinctrl.txt
> 
> It is very important that you know for sure whether it is actual
> GPIO or just named that way by some hardware engineer.

I did read the documentation, and I'm pretty sure those pins are all
actual GPIOs: once a pin's mode flag has been cleared, it can be
arbitrarily set high or low, or listened to by kernel code.

Regards,
Yves.

Re: Right amount of info in the DT

[Linus Walleij]

On Fri, May 12, 2017 at 5:31 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:
> On 05/11/2017 05:22 PM, Linus Walleij wrote:

>> Implement everything you have a data sheet for even if you
>> do not use it right now. It is good to have.
>
> Do you mean that I should submit a driver for every single device
> before I can declare its pins in the DT? That will surely take
> quite a long time, and I'm required to get GPIO functionality before
> that.

I mean that when you write a GPIO driver, i.e. ONE driver for
ONE GPIO controlling hardware, you should implement all the
functionality for THAT GPIO block.

> Furthermore, even if I had a driver for a device whose pins can also
> be GPIOs, say smartcard. If the driver is built as a module, and has
> not been yet loaded in, then, since you want the pinctrl node has a
> child node, no gpiochip struct could have been registered, and the pins
> can't be used as GPIOs.
> Does this mean that to use the smartcard's pin as GPIOs, the smartcard
> driver must absolutely be loaded, even though smartcards are never used?

I do not understand this remark.

If the GPIOs are provided by the smartcard IP block inside your
chip, yes then it makes sense to spawn the GPIO driver as part
of the smartcard driver.

Usually the GPIO hardware is a separate IP block from any others
(such as smartcard) so this situation seldom occurs.

Is you system such that there is an IO range for something like
a smart card, and then that IO range also contains registers for
reusing some or all pins as GPIO?

Then yes, we usually write a driver for this whole IP block and let
it expose a gpiochip as part of its main function.

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Thibaud Cornic]

On 22/05/2017 17:12, Linus Walleij wrote:
> On Fri, May 12, 2017 at 5:31 PM, Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com> wrote:
>> On 05/11/2017 05:22 PM, Linus Walleij wrote: 
>>> Implement everything you have a data sheet for even if you do not use it right now. It is good to have. 
>> Do you mean that I should submit a driver for every single device before I can declare its pins in the DT? That will surely take quite a long time, and I'm required to get GPIO functionality before that. 
> I mean that when you write a GPIO driver, i.e. ONE driver for ONE GPIO controlling hardware, you should implement all the functionality for THAT GPIO block.
Right now, Yves works on the GPIOs, and he'd like to be able to support all the GPIOs of the chip, even those
which are muxed with the UART/smart card/other IPs pins.
For example, we have a UART2 IP, which pins can be used as GPIOs or for the UART functionality.
The registers which toggle the GPIO/UART "mode" of these pins are in the address range of the UART block.
Does your answer mean that you won't allow any other code than a UART driver to touch those registers?
Which would mean that to be able to use those pins as GPIOs, Yves must first submit the UART driver and DT node,
then modify it to somehow expose a GPIO functionality?

>> Furthermore, even if I had a driver for a device whose pins can also be GPIOs, say smartcard. If the driver is built as a module, and has not been yet loaded in, then, since you want the pinctrl node has a child node, no gpiochip struct could have been registered, and the pins can't be used as GPIOs. Does this mean that to use the smartcard's pin as GPIOs, the smartcard driver must absolutely be loaded, even though smartcards are never used? 
> I do not understand this remark. If the GPIOs are provided by the smartcard IP block inside your chip, yes then it makes sense to spawn the GPIO driver as part of the smartcard driver.
Let's talk about UART instead (as there's no real use case for a dynamic switch of pin usage on the board between
the smart card and something else).
The remark is: when the UART driver is loaded, it will start using the pins in the UART mode. And this driver is not
designed to serve GPIO requests. It seems like a big modification to add this (like a wrapper completely disabling the
function for which that driver has been written).
Do we need to do this for all the drivers which control pins which can be used as GPIOs?

I had imagined that the pin controller driver would be the one writing in the "pin mode" registers, wherever they might
be located, and we would add "pin request" calls in the other drivers (UART, smart card) which would tell the pin
controller to put those pins in the UART/smart card mode, and not allow gpio requests to them.

> Usually the GPIO hardware is a separate IP block from any others (such as smartcard) so this situation seldom occurs.
Really? I imagine we're not the only company adding muxes to use certain IP pins as GPIOs. And those muxes a necessarily
placed in those IP blocks, which (depending on the interconnect) will force the mux register address to be in the IP address range.

> Is you system such that there is an IO range for something like a smart card, and then that IO range also contains registers for reusing some or all pins as GPIO?
Yes exactly. It contains the register to reuse the smart card pins as GPIOs.
And let's say the ethernet IO range contains the regs to reuse the ethernet pins.

> Then yes, we usually write a driver for this whole IP block and let it expose a gpiochip as part of its main function.
On top of my other remarks above, what if the driver is completely standard?

Regards,
Thibaud Cornic

Re: Right amount of info in the DT

[Linus Walleij]

On Wed, May 24, 2017 at 11:41 AM, Thibaud Cornic
<thibaud_cornic@sigmadesigns.com> wrote:

> Right now, Yves works on the GPIOs, and he'd like to be able to support all the GPIOs of the chip, even those
> which are muxed with the UART/smart card/other IPs pins.
> For example, we have a UART2 IP, which pins can be used as GPIOs or for the UART functionality.
> The registers which toggle the GPIO/UART "mode" of these pins are in the address range of the UART block.

OK I see.

> Does your answer mean that you won't allow any other code than a UART driver to touch those registers?

No.

Your answer seems to reflect an organizational problem though:
https://en.wikipedia.org/wiki/Stovepipe_(organisation)

> Which would mean that to be able to use those pins as GPIOs, Yves must first submit the UART driver and DT node,
> then modify it to somehow expose a GPIO functionality?

What is needed is first a proper DT represenation and second a driver.

I think this:

uart {
    compatible = "...";
    ...
    pinctrl-gpio {
        compatible = "...";
        ...
    };
};

Is going to be the pattern you want to solidify first. Then you can think
about how to engineer the driver(s).

>>> Furthermore, even if I had a driver for a device whose pins can also be GPIOs, say smartcard.
>>> If the driver is built as a module, and has not been yet loaded in, then, since you want the pinctrl
>>> node has a child node, no gpiochip struct could have been registered, and the pins can't be used
>>> as GPIOs. Does this mean that to use the smartcard's pin as GPIOs, the smartcard driver must
>>> absolutely be loaded, even though smartcards are never used?

>> I do not understand this remark. If the GPIOs are provided by the smartcard IP block inside your
>> chip, yes then it makes sense to spawn the GPIO driver as part of the smartcard driver.

> Let's talk about UART instead (as there's no real use case for a dynamic switch of pin usage on the board between
> the smart card and something else).
> The remark is: when the UART driver is loaded, it will start using the pins in the UART mode. And this driver is not
> designed to serve GPIO requests. It seems like a big modification to add this (like a wrapper completely disabling the
> function for which that driver has been written).
> Do we need to do this for all the drivers which control pins which can be used as GPIOs?

There are tons of UART drivers out there asking separate pin control
drivers to provide
them the pins they use.

What you need is to abstract out the pin control portions and let the
UART driver
grab these.

Linux already grabs the "default" pin control setting for any driver
right before
it calls probe() on the driver, see drivers/base/pinctrl.c

The pin control resources need to be created from an orthogonal driver,
whether the addresses used by this driver are in the same range as the
UART is not important, it however seems that it needs to be instantiated
before the UART driver and they can hardly be one and the same. This
is a Linux shortcoming augmented by deferred probe and other mechanisms
such as initcall reordering (shudder).

> I had imagined that the pin controller driver would be the one writing in the "pin mode" registers, wherever they might
> be located,

That is one way to do it. But another way to do it is to instantiate one
pin controller per pin mode register range. Do whatever you think becomes
most elegant.

> and we would add "pin request" calls in the other drivers (UART, smart card) which would tell the pin
> controller to put those pins in the UART/smart card mode, and not allow gpio requests to them.

The "default" mode will be requested automatically. If you need more pin modes,
you need to request them using explicit code.

>> Usually the GPIO hardware is a separate IP block from any others (such as smartcard)
>> so this situation seldom occurs.
>
> Really? I imagine we're not the only company adding muxes to use certain IP pins as GPIOs.

That is usually controlled by a register range specifically dedicated
to pins and GPIOs,
and not intermingled with the register range for the other IP as seems
to be your case.

So they see the "pad ring" as a device. Essentially it is usually constructed by
stacking together control lines to the I/O cells into a register range
just for that
purpose.

Much like so:
https://dflund.se/~triad/papers/pincontrol.pdf

> And those muxes a necessarily placed in those IP blocks, which (depending on the interconnect)
> will force the mux register address to be in the IP address range.

I've heard about this before but noone ever submitted a driver for that.
(I'm sorry you are the first, being first always sucks.)

>> Is you system such that there is an IO range for something like a smart card, and
>> then that IO range also contains registers for reusing some or all pins as GPIO?
>
> Yes exactly. It contains the register to reuse the smart card pins as GPIOs.
> And let's say the ethernet IO range contains the regs to reuse the ethernet pins.

Interesting. That is a first, or a second I heard, or something.

>> Then yes, we usually write a driver for this whole IP block and let it expose a
>>  gpiochip as part of its main function.
>
> On top of my other remarks above, what if the driver is completely standard?

Better to have the pin control driver separate. Sorry if I was wrong about that.

But I don't know whether to:

(A) Try to collect the address ranges from all over the place into one driver.

(B) Try to create one instance of a pin controller per IP

I'd like to see some code in the direction you choose I guess. Either should
work, I just don't know how to do (A) in an elegant manner, nor how to
instantiate (B) in a nice way, so some challenges there.

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 05/29/2017 12:02 PM, Linus Walleij wrote:
> But I don't know whether to:
> 
> (A) Try to collect the address ranges from all over the place into one driver.
> 
> (B) Try to create one instance of a pin controller per IP
> 
> I'd like to see some code in the direction you choose I guess. Either should
> work, I just don't know how to do (A) in an elegant manner, nor how to
> instantiate (B) in a nice way, so some challenges there.

I've put together a draft to try and solve this problem using approach (A).
Each IP node gets a GPIO child node, since this is what the hardware looks like,
and they are all tied together using phandles to a central pin controller node.

The draft only demonstrates the solution for the pins dedicated to GPIO,
namely `gpio_sys', and for the smartcard pins, namely `gpio_smcard'.

As before, bindings are not yet commented.

BTW, I'm sure there is a far better way than this `all_pins' array, let me know
if you have feedback on that.

Regards,
Yves.



>From f4e7e9314a3201a3a2ca00ad2381114af4ff6fd9 Mon Sep 17 00:00:00 2001
From: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
Date: Tue, 25 Apr 2017 14:06:46 +0200
Subject: [PATCH] drivers: pinctrl: Add a pinctrl for mach-tango

Change-Id: Id5b5872b2a7d116829ddc75577f26115eb6e1c98
Signed-off-by: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-common.dtsi |  28 ++
 arch/arm/configs/tango4_defconfig    |   2 +
 arch/arm/mach-tango/Kconfig          |   1 +
 drivers/pinctrl/Kconfig              |   7 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-tango.c      | 566 +++++++++++++++++++++++++++++++++++
 6 files changed, 605 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tango.c

diff --git a/arch/arm/boot/dts/tango4-common.dtsi b/arch/arm/boot/dts/tango4-common.dtsi
index a3e81ab..671f9fc 100644
--- a/arch/arm/boot/dts/tango4-common.dtsi
+++ b/arch/arm/boot/dts/tango4-common.dtsi
@@ -18,6 +18,34 @@
 	#address-cells = <1>;
 	#size-cells = <1>;

+	pinctrl {
+		compatible = "sigma,smp8758-pinctrl";
+		groups = <&gpio_sys>, <&gpio_smcard>;
+	};
+
+	gpio_sys: gpio@10500 {
+		reg = <0x10500 0x8>;
+		label = "sys";
+		ngpios = <16>;
+		tango,mux = "none";
+		gpio-controller;
+		#gpio-cells = <1>;
+	};
+
+	smcard: smcard@6c300 {
+		/* Driver in the works. */
+		reg = <0x6c300 0x6c>;
+
+		gpio_smcard: gpio@6c358 {
+			reg = <0x6c358 0xc>;
+			label = "smcard";
+			ngpios = <8>;
+			tango,mux = "pin";
+			gpio-controller;
+			#gpio-cells = <1>;
+		};
+	};
+
 	periph_clk: periph_clk {
 		compatible = "fixed-factor-clock";
 		clocks = <&clkgen CPU_CLK>;
diff --git a/arch/arm/configs/tango4_defconfig b/arch/arm/configs/tango4_defconfig
index b26bb4e..b58b01b 100644
--- a/arch/arm/configs/tango4_defconfig
+++ b/arch/arm/configs/tango4_defconfig
@@ -68,7 +68,9 @@ CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C=y
 CONFIG_I2C_XLR=y
+CONFIG_PINCTRL_TANGO=y
 CONFIG_GPIOLIB=y
+CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_CPU_THERMAL=y
 CONFIG_TANGO_THERMAL=y
diff --git a/arch/arm/mach-tango/Kconfig b/arch/arm/mach-tango/Kconfig
index ebe15b9..479c9aa 100644
--- a/arch/arm/mach-tango/Kconfig
+++ b/arch/arm/mach-tango/Kconfig
@@ -11,3 +11,4 @@ config ARCH_TANGO
 	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD
 	select TANGO_IRQ
+	select PINCTRL
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0e75d94..5648e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -179,6 +179,13 @@ config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP

+config PINCTRL_TANGO
+	tristate "Tango pin control driver"
+	depends on OF
+	select PINMUX
+	help
+	  Say yes to activate the pinctrl/gpio driver for Tango chips.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 11bad37..d5563e49 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
+obj-$(CONFIG_PINCTRL_TANGO)	+= pinctrl-tango.o
 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
diff --git a/drivers/pinctrl/pinctrl-tango.c b/drivers/pinctrl/pinctrl-tango.c
new file mode 100644
index 0000000..33b2ad9
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tango.c
@@ -0,0 +1,566 @@
+/*
+ * Copyright (C) 2017 Sigma Designs
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+/* Pin regs are laid out this way:
+ * ______________________________
+ * | bitwise mask | bitwise val |
+ * 32             16            0
+ *
+ * To set bit n, one must write (1 << (16+n) | n) to the reg.
+ * Consequently, one reg can't handle more than 16 pins, so n < 16.
+ *
+ * There are 3 kind of registers that follow this layout: value registers,
+ * direction registers, and gpio-mode registers.
+ * The latter kind allows muxing between the primary function (eg UART), which
+ * is the pin's direction and value entirely controlled by hardware, and the
+ * "GPIO mode", which is the pin's direction and value entirely controlled by
+ * software. This gpio-mode register only exists for some GPIO groups.
+ *
+ * Additionally, for some pin groups, there is no gpio-mode register that muxes
+ * with the granularity of a pin, but rather with the granularity of the whole
+ * group. In that case, such mux register does *not* follow the layout above.
+ *
+ * Finally, it is possible for some groups to have neither type of muxing. In
+ * that case, the pins are dedicated GPIOs.
+ *
+ * Registers are organized this way for each group:
+ * group_mux? (direction value pin_mux?)+
+ */
+
+enum mux_type {
+	MUX_NONE,
+	MUX_PIN,
+	MUX_GROUP,
+};
+
+/* Which magic values to write in the mux group registers. */
+#define MUX_GROUP_GPIO 0x6
+#define MUX_GROUP_ALTF 0x1
+
+/* Conversion table for char* <-> enum mux_type */
+static const struct {
+	const char *str;
+	enum mux_type type;
+} mux_types[] = {
+	{ .str = "none",  .type = MUX_NONE,  },
+	{ .str = "pin",   .type = MUX_PIN,   },
+	{ .str = "group", .type = MUX_GROUP, },
+};
+
+/* Pinctrl device data */
+struct pingroup {
+	struct list_head list;
+	const char *label;
+	unsigned base;
+	unsigned ngpio;
+	size_t sz;
+	phys_addr_t pa;
+	void __iomem *va;
+	enum mux_type mux_type;
+};
+
+/* Platform device data */
+struct pinctrl_data {
+	struct list_head group_list;
+};
+
+#define __reg_offset_from_pin(pg, pin) ( \
+		((pg)->mux_type == MUX_GROUP ? sizeof(u32) : 0) + \
+		((pg)->mux_type == MUX_PIN ? 3 : 2) * sizeof(u32) * ((pin)/16) \
+		)
+#define group_mux_reg_va(pg)    ((pg)->va)
+#define dir_reg_va(pg, pin)     (group_mux_reg_va(pg) + __reg_offset_from_pin(pg, pin))
+#define val_reg_va(pg, pin)     (dir_reg_va(pg, pin)  + sizeof(u32))
+#define pin_mux_reg_va(pg, pin) (val_reg_va(pg, pin)  + sizeof(u32))
+
+static int pinreg_get_bit(void __iomem *addr, unsigned offset)
+{
+	unsigned long val = readl(addr);
+
+	return test_bit(offset, &val);
+}
+
+static void pinreg_set_bit(void __iomem *addr, unsigned offset, int value)
+{
+	unsigned long val = 0;
+
+	/* Enable change by writing upper half-word too. */
+	set_bit(offset + 16, &val);
+	if (value)
+		set_bit(offset, &val);
+
+	writel(val, addr);
+}
+
+static void pinreg_set_multiple(void __iomem *addr, u16 mask, u16 bits)
+{
+	unsigned long val = (mask << 16) | bits;
+
+	writel(val, addr);
+}
+
+static int gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	return pinreg_get_bit(val_reg_va(group, offset), offset % 16);
+}
+
+static void gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+}
+
+static int gpio_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	return !pinreg_get_bit(dir_reg_va(group, offset), offset % 16);
+}
+
+static int gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 0);
+
+	return 0;
+}
+
+static int gpio_direction_output(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* The output value can be changed before the direction is. */
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 1);
+
+	return 0;
+}
+
+static void gpio_set_multiple(struct gpio_chip *gc,
+		unsigned long *mask, unsigned long *bits)
+{
+	unsigned i, j;
+	unsigned num_of_ulongs;
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group)
+		return;
+
+	num_of_ulongs = (group->ngpio / 32) + 1;
+	for (i = 0; i < num_of_ulongs; i++)
+		for (j = 0; j < 1; j++) {
+			u16 _mask = (u16)(*(mask + i) >> (16 * j));
+			u16 _bits = (u16)(*(bits + i) >> (16 * j));
+			pinreg_set_multiple(val_reg_va(group, 2*i + j), _mask, _bits);
+		}
+}
+
+static const unsigned int all_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
+	10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+	20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+	30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
+	40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
+	50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
+	60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
+	70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
+	80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
+	90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
+};
+
+static int get_group_count(struct pinctrl_dev *pctldev)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	return group->mux_type == MUX_NONE ? 0 : 1;
+}
+
+static const char *get_group_name(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return ERR_PTR(-EINVAL);
+
+	return group->label;
+}
+
+static int get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
+		const unsigned **pins, unsigned *num_pins)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	*pins = all_pins + group->base;
+	*num_pins = group->ngpio;
+
+	return 0;
+}
+
+static const struct pinctrl_ops pctlops = {
+	.get_groups_count = get_group_count,
+	.get_group_name = get_group_name,
+	.get_group_pins = get_group_pins,
+};
+
+int get_function_groups(struct pinctrl_dev *pctldev, unsigned selector,
+		const char * const **groups, unsigned *num_groups)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	if (group->mux_type == MUX_NONE) {
+		*groups = NULL;
+		*num_groups = 0;
+	} else {
+		*groups = &group->label;
+		*num_groups = 1;
+	}
+
+	return 0;
+}
+
+int set_mux(struct pinctrl_dev *pctldev, unsigned func_selector,
+		unsigned group_selector)
+{
+	/* With only one possible alt function, muxing is only
+	 * necessary for GPIO request/free.
+	 */
+	return 0;
+}
+
+int gpio_request_enable(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 1);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_GPIO, group_mux_reg_va(group));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void gpio_disable_free(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group;
+
+	group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 0);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_ALTF, group_mux_reg_va(group));
+		break;
+	}
+}
+
+const static struct pinmux_ops pmxops = {
+	.get_functions_count = get_group_count,
+	.get_function_name = get_group_name,
+	.get_function_groups = get_function_groups,
+	.set_mux = set_mux,
+	.gpio_request_enable = gpio_request_enable,
+	.gpio_disable_free = gpio_disable_free,
+	.strict = true,
+};
+
+static int get_mux_type_from_str(const char *str, enum mux_type *type)
+{
+	unsigned i;
+	for (i = 0; i < ARRAY_SIZE(mux_types); i++)
+		if (!strcmp(mux_types[i].str, str)) {
+			*type = mux_types[i].type;
+			return 0;
+		}
+	return -EINVAL;
+}
+
+static int get_node_info(struct device_node *np, struct pingroup *group)
+{
+	/* Extract info from the pinctrl node:
+	 * - the label;
+	 * - the number of pins;
+	 * - the address and size of the registers;
+	 * - the mux type.
+	 */
+	int ret;
+	u32 ngpio;
+	u32 reg[2];
+	const char *label;
+	const char *tango_mux;
+
+	ret = of_property_read_u32(np, "ngpios", &ngpio);
+	if (ret)
+		return ret;
+	ret = of_property_read_u32_array(np, "reg", reg, 2);
+	if (ret)
+		return ret;
+	ret = of_property_read_string(np, "label", &label);
+	if (ret)
+		return ret;
+	ret = of_property_read_string(np, "tango,mux", &tango_mux);
+	if (ret)
+		return ret;
+
+	ret = get_mux_type_from_str(tango_mux, &group->mux_type);
+	if (ret)
+		return ret;
+	group->label = label;
+	group->ngpio = ngpio;
+	group->pa = reg[0];
+	group->sz = reg[1];
+
+	return 0;
+}
+
+static int register_group_gpios(struct device *dev,
+		struct pingroup *group,
+		struct pinctrl_dev *pindev)
+{
+	int ret;
+	struct gpio_chip *gchip;
+	struct pinctrl_gpio_range *prange;
+
+	gchip = devm_kzalloc(dev, sizeof(*gchip), GFP_KERNEL);
+	if (!gchip)
+		return -ENOMEM;
+	prange = devm_kzalloc(dev, sizeof(*prange), GFP_KERNEL);
+	if (!prange)
+		return -ENOMEM;
+
+	gchip->label = group->label;
+	gchip->parent = dev;
+	gchip->owner = THIS_MODULE;
+	gchip->get_direction = gpio_get_direction;
+	gchip->direction_input = gpio_direction_input;
+	gchip->direction_output = gpio_direction_output;
+	gchip->get = gpio_get;
+	gchip->set = gpio_set;
+	gchip->set_multiple = gpio_set_multiple;
+	gchip->base = group->base;
+	gchip->ngpio = group->ngpio;
+	ret = devm_gpiochip_add_data(dev, gchip, group);
+	if (ret)
+		return ret;
+
+	prange->name = group->label;
+	prange->id = group->base;
+	prange->base = group->base;
+	prange->pin_base = group->base;
+	prange->npins = group->ngpio;
+	prange->gc = gchip;
+	pinctrl_add_gpio_range(pindev, prange);
+
+	return 0;
+}
+
+static int register_group_pins(struct device *dev,
+		struct pingroup *group)
+{
+	unsigned i;
+	struct pinctrl_desc *pdesc;
+	struct pinctrl_pin_desc *ppdesc;
+	struct pinctrl_dev *pindev;
+
+	pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+	ppdesc = devm_kzalloc(dev, group->ngpio * sizeof(*ppdesc), GFP_KERNEL);
+	if (!ppdesc)
+		return -ENOMEM;
+
+	for (i = 0; i < group->ngpio; i++)
+		ppdesc[i].number = group->base + i;
+
+	pdesc->name = group->label;
+	pdesc->pins = ppdesc;
+	pdesc->npins = group->ngpio;
+	pdesc->pctlops = &pctlops;
+	pdesc->pmxops = &pmxops;
+	pdesc->owner = THIS_MODULE;
+
+	pindev = devm_pinctrl_register(dev, pdesc, group);
+	if (IS_ERR(pindev))
+		return PTR_ERR(pindev);
+
+	return register_group_gpios(dev, group, pindev);
+}
+
+static int register_group(struct platform_device *pdev, struct device_node *np)
+{
+	int ret;
+	struct pingroup *group;
+	struct pinctrl_data *pdata;
+
+	group = devm_kzalloc(&pdev->dev, sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return -ENOMEM;
+
+	/* Extract info from the device node. */
+	ret = get_node_info(np, group);
+	if (ret)
+		return ret;
+
+	/* Get the base number of this group by looking at the previous one. */
+	pdata = platform_get_drvdata(pdev);
+	if (!pdata)
+		return -EINVAL;
+	if (list_empty(&pdata->group_list)) {
+		group->base = 0;
+	} else {
+		struct pingroup *prev;
+		prev = list_last_entry(&pdata->group_list, typeof(*prev), list);
+		group->base = prev->base + prev->ngpio;
+	}
+
+	/* Ioremap the group registers. */
+	group->va = devm_ioremap_nocache(&pdev->dev, group->pa, group->sz);
+	if (!group->va)
+		return -ENOMEM;
+
+	/* Register the group pins. */
+	ret = register_group_pins(&pdev->dev, group);
+	if (ret)
+		return ret;
+
+	/* Add this new group to the list. */
+	list_add_tail(&group->list, &pdata->group_list);
+
+	dev_dbg(&pdev->dev, "%8s base=%d ngpio=%d\n",
+			group->label, group->base, group->ngpio);
+	return 0;
+}
+
+static int tango_pinctrl_probe(struct platform_device *pdev)
+{
+	unsigned i = 0;
+	unsigned groups_num;
+	struct pinctrl_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&pdata->group_list);
+	platform_set_drvdata(pdev, pdata);
+
+	groups_num = of_count_phandle_with_args(np, "groups", NULL);
+	if (!groups_num)
+		return -EINVAL;
+
+	for (i = 0; i < groups_num; i++) {
+		int ret;
+		struct of_phandle_args args;
+		ret = of_parse_phandle_with_args(np, "groups", NULL, i, &args);
+		if (ret)
+			return ret;
+		ret = register_group(pdev, args.np);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id tango_pinctrl_of_match[] = {
+	{ .compatible =  "sigma,smp8758-pinctrl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tango_pinctrl_of_match);
+
+static struct platform_driver tango_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-tango",
+		.of_match_table = of_match_ptr(tango_pinctrl_of_match),
+	},
+	.probe = tango_pinctrl_probe,
+};
+
+static int __init tango_pinctrl_init(void)
+{
+	return platform_driver_register(&tango_pinctrl_driver);
+}
+module_init(tango_pinctrl_init);
+
+static void __exit tango_pinctrl_exit(void)
+{
+	platform_driver_unregister(&tango_pinctrl_driver);
+}
+module_exit(tango_pinctrl_exit);
+
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango pinctrl driver");
+MODULE_LICENSE("GPL");
--
2.10.1

Re: Right amount of info in the DT

[Linus Walleij]

On Tue, May 30, 2017 at 3:59 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> I've put together a draft to try and solve this problem using approach (A).
> Each IP node gets a GPIO child node, since this is what the hardware looks like,
> and they are all tied together using phandles to a central pin controller node.

I like the looks of this!

> The draft only demonstrates the solution for the pins dedicated to GPIO,
> namely `gpio_sys', and for the smartcard pins, namely `gpio_smcard'.

It is elegant and it will scale.

> As before, bindings are not yet commented.
>
> BTW, I'm sure there is a far better way than this `all_pins' array, let me know
> if you have feedback on that.

So that is this:

> +static const unsigned int all_pins[] = {
> +       0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
> +       10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
> +       20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
> +       30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
> +       40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
> +       50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
> +       60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
> +       70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
> +       80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
> +       90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
> +};

That looks a bit like what we usually put in pin descriptors.

(...)

This:

> +       struct pinctrl_gpio_range *prange;
> +
(...)
> +       prange->name = group->label;
> +       prange->id = group->base;
> +       prange->base = group->base;
> +       prange->pin_base = group->base;
> +       prange->npins = group->ngpio;
> +       prange->gc = gchip;
> +       pinctrl_add_gpio_range(pindev, prange);

Can't you just add that to the device tree using the gpio-ranges
property? It is handled in the core and should "just work".

The documentation for that is a bit terse in
Documentation/devicetree/bindings/gpio/gpio.txt
but if you just grep for gpio-ranges you will get the hang of it.

It is handled from the gpiochip side which is advisible, even if
you hardcode it in the driver.

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 05/31/2017 02:24 AM, Linus Walleij wrote:
>> BTW, I'm sure there is a far better way than this `all_pins' array, let me know
>> if you have feedback on that.
> 
> So that is this:
> 
>> +static const unsigned int all_pins[] = {
>> +       0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
>> +       10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
>> +       20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
>> +       30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
>> +       40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
>> +       50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
>> +       60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
>> +       70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
>> +       80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
>> +       90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
>> +};
> 
> That looks a bit like what we usually put in pin descriptors.

This array is here because of my implementation of get_group_pins() in
`struct pinctrl_ops'. It seems the function is expecting such an array.
I mean I probably could have done something like:

static const unsigned int smcard_pins[] = { 35, 36, 37, ... };

and returned `smcard_pins' instead of `&all_pins[35]'
But the thing is, this `35' is not a constant number because it depends
on the number of pins that got registered before the smartcard's own.
I know it should reflect the number of the pin on my board's spec, and it
will once all pin groups are put in the device tree in the right order.

That being said, we might want to add a `base' property to the GPIO nodes
which are parsed by the pinctrl driver to make sure this `35' never moves.

> This:
> 
>> +       struct pinctrl_gpio_range *prange;
>> +
> (...)
>> +       prange->name = group->label;
>> +       prange->id = group->base;
>> +       prange->base = group->base;
>> +       prange->pin_base = group->base;
>> +       prange->npins = group->ngpio;
>> +       prange->gc = gchip;
>> +       pinctrl_add_gpio_range(pindev, prange);
> 
> Can't you just add that to the device tree using the gpio-ranges
> property? It is handled in the core and should "just work".
> 
> The documentation for that is a bit terse in
> Documentation/devicetree/bindings/gpio/gpio.txt
> but if you just grep for gpio-ranges you will get the hang of it.
> 
> It is handled from the gpiochip side which is advisible, even if
> you hardcode it in the driver.

OK got it. I missed the deprecation warning in the doc, my bad.
I've updated the patch to reflect this change, and got rid of the
`label' property in the GPIO nodes to use `gpio-ranges-group-names' instead.


I tried to carry on and add the other IPs' pins, and I've got a question
regarding pin requesting.
See, adding the smartcard's pins was easy because we don't have a driver for
it yet, so the pins are just straightforward GPIOs, which the user-space can
request using /sys/class/gpio/export.
But that's not the case for the ethernet device, whose pins are already in
use, although not requested explicitly by its driver
(drivers/net/ethernet/aurora/nb8800.c). Which means user-space can also
request/set them within the sysfs, and that is wrong. So I wondered:

- Is there a mechanism in the device tree to automatically request some pins
for a driver once it gets loaded? I have looked at the `stateN_nodeM' thing
in the documentation, but that doesn't seem to do the job without changes in
the ethernet driver's code.

- Should the pins be declared at all if we know they are not going to be used
for anything else than, for instance, ethernet? What about some removable
device then, for instance UART as Thibaud pointed out previously?


Thanks and regards,
Yves.



>From 0c0d9bd5e55203b4a450e44097e1e50fa1d525aa Mon Sep 17 00:00:00 2001
From: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
Date: Tue, 25 Apr 2017 14:06:46 +0200
Subject: [PATCH] drivers: pinctrl: Add a pinctrl for mach-tango

Change-Id: Id5b5872b2a7d116829ddc75577f26115eb6e1c98
Signed-off-by: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-common.dtsi |  46 +++
 arch/arm/configs/tango4_defconfig    |   2 +
 arch/arm/mach-tango/Kconfig          |   1 +
 drivers/pinctrl/Kconfig              |   7 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-tango.c      | 552 +++++++++++++++++++++++++++++++++++
 6 files changed, 609 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tango.c

diff --git a/arch/arm/boot/dts/tango4-common.dtsi b/arch/arm/boot/dts/tango4-common.dtsi
index a3e81ab..4eee27d 100644
--- a/arch/arm/boot/dts/tango4-common.dtsi
+++ b/arch/arm/boot/dts/tango4-common.dtsi
@@ -18,6 +18,42 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	pinctrl: pinctrl {
+		compatible = "sigma,smp8758-pinctrl";
+		groups = <&gpio_sys>, <&gpio_eth0>, <&gpio_smcard>;
+
+		pin_state_eth0: state_eth0 {
+			function = "eth0";
+			groups = "eth0";
+		};
+	};
+
+	gpio_sys: gpio@10500 {
+		reg = <0x10500 0x8>;
+		ngpios = <16>;
+		tango,mux = "none";
+		gpio-controller;
+		#gpio-cells = <1>;
+		gpio-ranges = <&pinctrl 0 0 0>;
+		gpio-ranges-group-names = "sys";
+	};
+
+	smcard: smcard@6c300 {
+		/* Driver in the works. */
+		reg = <0x6c300 0x6c>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		gpio_smcard: gpio@6c358 {
+			reg = <0x6c358 0xc>;
+			ngpios = <8>;
+			tango,mux = "pin";
+			gpio-controller;
+			#gpio-cells = <1>;
+			gpio-ranges-group-names = "smcard";
+		};
+	};
+
 	periph_clk: periph_clk {
 		compatible = "fixed-factor-clock";
 		clocks = <&clkgen CPU_CLK>;
@@ -151,6 +187,16 @@
 			reg = <0x26000 0x800>;
 			interrupts = <38 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clkgen SYS_CLK>;
+			pinctrl-0 = <&pin_state_eth0>;
+
+			gpio_eth0: gpio@26404 {
+				reg = <0x26404 0x10>;
+				ngpios = <19>;
+				tango,mux = "group";
+				gpio-controller;
+				#gpio-cells = <1>;
+				gpio-ranges-group-names = "eth0";
+			};
 		};
 
 		dma: dma@290a0 {
diff --git a/arch/arm/configs/tango4_defconfig b/arch/arm/configs/tango4_defconfig
index b26bb4e..b58b01b 100644
--- a/arch/arm/configs/tango4_defconfig
+++ b/arch/arm/configs/tango4_defconfig
@@ -68,7 +68,9 @@ CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C=y
 CONFIG_I2C_XLR=y
+CONFIG_PINCTRL_TANGO=y
 CONFIG_GPIOLIB=y
+CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_CPU_THERMAL=y
 CONFIG_TANGO_THERMAL=y
diff --git a/arch/arm/mach-tango/Kconfig b/arch/arm/mach-tango/Kconfig
index ebe15b9..479c9aa 100644
--- a/arch/arm/mach-tango/Kconfig
+++ b/arch/arm/mach-tango/Kconfig
@@ -11,3 +11,4 @@ config ARCH_TANGO
 	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD
 	select TANGO_IRQ
+	select PINCTRL
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0e75d94..5648e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -179,6 +179,13 @@ config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP
 
+config PINCTRL_TANGO
+	tristate "Tango pin control driver"
+	depends on OF
+	select PINMUX
+	help
+	  Say yes to activate the pinctrl/gpio driver for Tango chips.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 11bad37..d5563e49 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
+obj-$(CONFIG_PINCTRL_TANGO)	+= pinctrl-tango.o
 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
diff --git a/drivers/pinctrl/pinctrl-tango.c b/drivers/pinctrl/pinctrl-tango.c
new file mode 100644
index 0000000..0a50238
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tango.c
@@ -0,0 +1,552 @@
+/*
+ * Copyright (C) 2017 Sigma Designs
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+/* Pin regs are laid out this way:
+ * ______________________________
+ * | bitwise mask | bitwise val |
+ * 32             16            0
+ *
+ * To set bit n, one must write (1 << (16+n) | n) to the reg.
+ * Consequently, one reg can't handle more than 16 pins, so n < 16.
+ *
+ * There are 3 kind of registers that follow this layout: value registers,
+ * direction registers, and gpio-mode registers.
+ * The latter kind allows muxing between the primary function (eg UART), which
+ * is the pin's direction and value entirely controlled by hardware, and the
+ * "GPIO mode", which is the pin's direction and value entirely controlled by
+ * software. This gpio-mode register only exists for some GPIO groups.
+ *
+ * Additionally, for some pin groups, there is no gpio-mode register that muxes
+ * with the granularity of a pin, but rather with the granularity of the whole
+ * group. In that case, such mux register does *not* follow the layout above.
+ *
+ * Finally, it is possible for some groups to have neither type of muxing. In
+ * that case, the pins are dedicated GPIOs.
+ *
+ * Registers are organized this way for each group:
+ * group_mux? (direction value pin_mux?)+
+ */
+
+enum mux_type {
+	MUX_NONE,
+	MUX_PIN,
+	MUX_GROUP,
+};
+
+/* Which magic values to write in the mux group registers. */
+#define MUX_GROUP_GPIO 0x6
+#define MUX_GROUP_ALTF 0x1
+
+/* Conversion table for char* <-> enum mux_type */
+static const struct {
+	const char *str;
+	enum mux_type type;
+} mux_types[] = {
+	{ .str = "none",  .type = MUX_NONE,  },
+	{ .str = "pin",   .type = MUX_PIN,   },
+	{ .str = "group", .type = MUX_GROUP, },
+};
+
+/* Pinctrl device data */
+struct pingroup {
+	struct list_head list;
+	const char *label;
+	unsigned base;
+	unsigned ngpio;
+	size_t sz;
+	phys_addr_t pa;
+	void __iomem *va;
+	enum mux_type mux_type;
+};
+
+/* Platform device data */
+struct pinctrl_data {
+	struct list_head group_list;
+};
+
+#define __reg_offset_from_pin(pg, pin) ( \
+		((pg)->mux_type == MUX_GROUP ? sizeof(u32) : 0) + \
+		((pg)->mux_type == MUX_PIN ? 3 : 2) * sizeof(u32) * ((pin)/16) \
+		)
+#define group_mux_reg_va(pg)    ((pg)->va)
+#define dir_reg_va(pg, pin)     (group_mux_reg_va(pg) + __reg_offset_from_pin(pg, pin))
+#define val_reg_va(pg, pin)     (dir_reg_va(pg, pin)  + sizeof(u32))
+#define pin_mux_reg_va(pg, pin) (val_reg_va(pg, pin)  + sizeof(u32))
+
+static int pinreg_get_bit(void __iomem *addr, unsigned offset)
+{
+	unsigned long val = readl(addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(val & GENMASK(31, 16));
+#endif
+
+	return test_bit(offset, &val);
+}
+
+static void pinreg_set_bit(void __iomem *addr, unsigned offset, int value)
+{
+	unsigned long val = 0;
+
+	/* Enable change by writing upper half-word too. */
+	set_bit(offset + 16, &val);
+	if (value)
+		set_bit(offset, &val);
+
+	writel(val, addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(pinreg_get_bit(addr, offset) != value);
+#endif
+}
+
+static void pinreg_set_multiple(void __iomem *addr, u16 mask, u16 bits)
+{
+	unsigned long val = (mask << 16) | bits;
+
+	writel(val, addr);
+}
+
+static int gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	return pinreg_get_bit(val_reg_va(group, offset), offset % 16);
+}
+
+static void gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+}
+
+static int gpio_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	return !pinreg_get_bit(dir_reg_va(group, offset), offset % 16);
+}
+
+static int gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 0);
+
+	return 0;
+}
+
+static int gpio_direction_output(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* The output value can be changed before the direction is. */
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 1);
+
+	return 0;
+}
+
+static void gpio_set_multiple(struct gpio_chip *gc,
+		unsigned long *mask, unsigned long *bits)
+{
+	unsigned i, j;
+	unsigned num_of_ulongs;
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group)
+		return;
+
+	num_of_ulongs = (group->ngpio / 32) + 1;
+	for (i = 0; i < num_of_ulongs; i++)
+		for (j = 0; j < 1; j++) {
+			u16 _mask = (u16)(*(mask + i) >> (16 * j));
+			u16 _bits = (u16)(*(bits + i) >> (16 * j));
+			pinreg_set_multiple(val_reg_va(group, 2*i + j), _mask, _bits);
+		}
+}
+
+static const unsigned int all_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
+	10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+	20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+	30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
+	40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
+	50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
+	60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
+	70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
+	80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
+	90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
+};
+
+static int get_group_count(struct pinctrl_dev *pctldev)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	return group->mux_type == MUX_NONE ? 0 : 1;
+}
+
+static const char *get_group_name(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return ERR_PTR(-EINVAL);
+
+	return group->label;
+}
+
+static int get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
+		const unsigned **pins, unsigned *num_pins)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	*pins = all_pins + group->base;
+	*num_pins = group->ngpio;
+
+	return 0;
+}
+
+static const struct pinctrl_ops pctlops = {
+	.get_groups_count = get_group_count,
+	.get_group_name = get_group_name,
+	.get_group_pins = get_group_pins,
+};
+
+int get_function_groups(struct pinctrl_dev *pctldev, unsigned selector,
+		const char * const **groups, unsigned *num_groups)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	if (group->mux_type == MUX_NONE) {
+		*groups = NULL;
+		*num_groups = 0;
+	} else {
+		*groups = &group->label;
+		*num_groups = 1;
+	}
+
+	return 0;
+}
+
+int set_mux(struct pinctrl_dev *pctldev, unsigned func_selector,
+		unsigned group_selector)
+{
+	/* With only one possible alt function, muxing is only
+	 * necessary for GPIO request/free.
+	 */
+	return 0;
+}
+
+int gpio_request_enable(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 1);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_GPIO, group_mux_reg_va(group));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void gpio_disable_free(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group;
+
+	group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 0);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_ALTF, group_mux_reg_va(group));
+		break;
+	}
+}
+
+const static struct pinmux_ops pmxops = {
+	.get_functions_count = get_group_count,
+	.get_function_name = get_group_name,
+	.get_function_groups = get_function_groups,
+	.set_mux = set_mux,
+	.gpio_request_enable = gpio_request_enable,
+	.gpio_disable_free = gpio_disable_free,
+	.strict = true,
+};
+
+static int get_mux_type_from_str(const char *str, enum mux_type *type)
+{
+	unsigned i;
+	for (i = 0; i < ARRAY_SIZE(mux_types); i++)
+		if (!strcmp(mux_types[i].str, str)) {
+			*type = mux_types[i].type;
+			return 0;
+		}
+	return -EINVAL;
+}
+
+static int get_node_info(struct device_node *np, struct pingroup *group)
+{
+	/* Extract info from the pinctrl node:
+	 * - the label;
+	 * - the number of pins;
+	 * - the address and size of the registers;
+	 * - the mux type.
+	 */
+	int ret;
+	u32 ngpio;
+	u32 reg[2];
+	const char *label;
+	const char *tango_mux;
+
+	ret = of_property_read_u32(np, "ngpios", &ngpio);
+	if (ret)
+		return ret;
+	ret = of_property_read_u32_array(np, "reg", reg, 2);
+	if (ret)
+		return ret;
+	ret = of_property_read_string(np, "gpio-ranges-group-names", &label);
+	if (ret)
+		return ret;
+	ret = of_property_read_string(np, "tango,mux", &tango_mux);
+	if (ret)
+		return ret;
+
+	ret = get_mux_type_from_str(tango_mux, &group->mux_type);
+	if (ret)
+		return ret;
+	group->label = label;
+	group->ngpio = ngpio;
+	group->pa = reg[0];
+	group->sz = reg[1];
+
+	return 0;
+}
+
+static int register_group_gpios(struct device *dev, struct pingroup *group)
+{
+	struct gpio_chip *gchip;
+
+	gchip = devm_kzalloc(dev, sizeof(*gchip), GFP_KERNEL);
+	if (!gchip)
+		return -ENOMEM;
+
+	gchip->label = group->label;
+	gchip->parent = dev;
+	gchip->owner = THIS_MODULE;
+	gchip->get_direction = gpio_get_direction;
+	gchip->direction_input = gpio_direction_input;
+	gchip->direction_output = gpio_direction_output;
+	gchip->get = gpio_get;
+	gchip->set = gpio_set;
+	gchip->set_multiple = gpio_set_multiple;
+	gchip->base = group->base;
+	gchip->ngpio = group->ngpio;
+
+	return devm_gpiochip_add_data(dev, gchip, group);
+}
+
+static int register_group_pins(struct device *dev, struct pingroup *group)
+{
+	unsigned i;
+	struct pinctrl_desc *pdesc;
+	struct pinctrl_pin_desc *ppdesc;
+	struct pinctrl_dev *pindev;
+
+	pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+	ppdesc = devm_kzalloc(dev, group->ngpio * sizeof(*ppdesc), GFP_KERNEL);
+	if (!ppdesc)
+		return -ENOMEM;
+
+	for (i = 0; i < group->ngpio; i++)
+		ppdesc[i].number = group->base + i;
+
+	pdesc->name = group->label;
+	pdesc->pins = ppdesc;
+	pdesc->npins = group->ngpio;
+	pdesc->pctlops = &pctlops;
+	pdesc->pmxops = &pmxops;
+	pdesc->owner = THIS_MODULE;
+
+	pindev = devm_pinctrl_register(dev, pdesc, group);
+	if (IS_ERR(pindev))
+		return PTR_ERR(pindev);
+
+	return 0;
+}
+
+static int register_group(struct platform_device *pdev, struct device_node *np)
+{
+	int ret;
+	struct pingroup *group;
+	struct pinctrl_data *pdata;
+
+	group = devm_kzalloc(&pdev->dev, sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return -ENOMEM;
+
+	/* Extract info from the device node. */
+	ret = get_node_info(np, group);
+	if (ret)
+		return ret;
+
+	/* Get the base number of this group by looking at the previous one. */
+	pdata = platform_get_drvdata(pdev);
+	if (!pdata)
+		return -EINVAL;
+	if (list_empty(&pdata->group_list)) {
+		group->base = 0;
+	} else {
+		struct pingroup *prev;
+		prev = list_last_entry(&pdata->group_list, typeof(*prev), list);
+		group->base = prev->base + prev->ngpio;
+	}
+
+	/* Ioremap the group registers. */
+	group->va = devm_ioremap_nocache(&pdev->dev, group->pa, group->sz);
+	if (!group->va)
+		return -ENOMEM;
+
+	/* Register the group pins. */
+	ret = register_group_pins(&pdev->dev, group);
+	if (ret)
+		return ret;
+
+	/* Register the group GPIOs. */
+	ret = register_group_gpios(&pdev->dev, group);
+	if (ret)
+		return ret;
+
+	/* Add this new group to the list. */
+	list_add_tail(&group->list, &pdata->group_list);
+
+	dev_dbg(&pdev->dev, "%-8s base=%-2d ngpio=%-2d OK\n",
+			group->label, group->base, group->ngpio);
+	return 0;
+}
+
+static int tango_pinctrl_probe(struct platform_device *pdev)
+{
+	unsigned i = 0;
+	unsigned groups_num;
+	struct pinctrl_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&pdata->group_list);
+	platform_set_drvdata(pdev, pdata);
+
+	groups_num = of_count_phandle_with_args(np, "groups", NULL);
+	if (!groups_num)
+		return -EINVAL;
+
+	for (i = 0; i < groups_num; i++) {
+		int ret;
+		struct of_phandle_args args;
+		ret = of_parse_phandle_with_args(np, "groups", NULL, i, &args);
+		if (ret)
+			return ret;
+		ret = register_group(pdev, args.np);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id tango_pinctrl_of_match[] = {
+	{ .compatible =  "sigma,smp8758-pinctrl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tango_pinctrl_of_match);
+
+static struct platform_driver tango_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-tango",
+		.of_match_table = of_match_ptr(tango_pinctrl_of_match),
+	},
+	.probe = tango_pinctrl_probe,
+};
+
+static int __init tango_pinctrl_init(void)
+{
+	return platform_driver_register(&tango_pinctrl_driver);
+}
+module_init(tango_pinctrl_init);
+
+static void __exit tango_pinctrl_exit(void)
+{
+	platform_driver_unregister(&tango_pinctrl_driver);
+}
+module_exit(tango_pinctrl_exit);
+
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.10.1

Re: Right amount of info in the DT

[Linus Walleij]

On Wed, May 31, 2017 at 6:36 PM, Yves Lefloch
<YvesMarie_Lefloch@sigmadesigns.com> wrote:

> I tried to carry on and add the other IPs' pins, and I've got a question
> regarding pin requesting.
> See, adding the smartcard's pins was easy because we don't have a driver for
> it yet, so the pins are just straightforward GPIOs, which the user-space can
> request using /sys/class/gpio/export.

Please do not use sysfs for userspace tests, familiarize yourself with
tools/gpio in the kernel and use these for testing using the ioctl()s.

> But that's not the case for the ethernet device, whose pins are already in
> use, although not requested explicitly by its driver
> (drivers/net/ethernet/aurora/nb8800.c). Which means user-space can also
> request/set them within the sysfs, and that is wrong.

It is common that a device allows simultaneous use of a pin for
GPIO and a certain device. (E.g. so that GPIO can "spy" on the pin
or similar.)

If on a certain system, this is not allowed, one shall set
.strict = true on the struct pinmux_ops, see
include/linux/pinctrl/pinmux.h

> - Is there a mechanism in the device tree to automatically request some pins
> for a driver once it gets loaded? I have looked at the `stateN_nodeM' thing
> in the documentation, but that doesn't seem to do the job without changes in
> the ethernet driver's code.

Yes those are called pin control hogs. Then the pins get associated with the
pin controller itself. Try first to associate the pins with the device actually
using them if possible.

> - Should the pins be declared at all if we know they are not going to be used
> for anything else than, for instance, ethernet? What about some removable
> device then, for instance UART as Thibaud pointed out previously?

I guess it depends on how much control of things you need in your system.

Also you might not need it now, but later on it turns out that in order to
properly deal with things like suspend/resume, you all of a sudden need
to switch pin control states when going to sleep and then it is nice if
the driver can handle these pins.

When in doubt, implement a driver for everything you have hardware
registers for, anything that is software controlled, I guess.

Yours,
Linus Walleij

Re: Right amount of info in the DT

[Yves Lefloch]

On 06/09/2017 10:14 AM, Linus Walleij wrote:
> Please do not use sysfs for userspace tests, familiarize yourself with
> tools/gpio in the kernel and use these for testing using the ioctl()s.
OK I will. What is the reasoning behind using ioctl()s instead of sysfs?

>> But that's not the case for the ethernet device, whose pins are already in
>> use, although not requested explicitly by its driver
>> (drivers/net/ethernet/aurora/nb8800.c). Which means user-space can also
>> request/set them within the sysfs, and that is wrong.
> 
> It is common that a device allows simultaneous use of a pin for
> GPIO and a certain device. (E.g. so that GPIO can "spy" on the pin
> or similar.)
In my case, this is not allowed, writing the registers won't do anything.

> If on a certain system, this is not allowed, one shall set
> .strict = true on the struct pinmux_ops, see
> include/linux/pinctrl/pinmux.h
Yes I was already using this property, but it didn't give me the exclusivity.
I think that this was because I never made the call to pinctrl_request_gpio().
Looking at "drivers/pinctrl/meson/pinctrl-meson.c", I tried to add a call to
that function in the gpiochip.request() op.

And then something strange happened: I'm only able to export to user-space the
GPIOs from the `gpio_sys' group, but not from the `gpio_smcard' group (see DT
below).

Here's how I'm seeing that (yes I know I should switch to tools/gpio!):


# ls /sys/class/gpio
export      gpiochip0   gpiochip71  unexport
# echo 0 > /sys/class/gpio/export
[  543.733511] pinctrl-tango pinctrl: request pin 0 (PIN0) for sys:0
[  543.739696] device: 'gpio0': device_add
# echo 71 > /sys/class/gpio/export
[  626.460176] pinctrl-tango pinctrl: pin 71 is not registered so it cannot be requested
[  626.468092] pinctrl-tango pinctrl: pin-71 (smcard:71) status -22
[  626.474148] gpio-71 (?): gpiod_request: status -22
[  626.479121] export_store: status -22
bash: echo: write error: Invalid argument


Eventually, I managed to find the root cause of this issue:
For every "group" in the pinctrl node of my DT, there is a different pinctrl_dev,
because I make for every one of them a call to `devm_pinctrl_register'.
When the gpiolib parses the gpio-controller nodes of the DT and creates the GPIO
ranges with gpiochip_add_pin_range(), it tries to retrieve a pinctrl_dev with
of_pinctrl_get(), and attaches the GPIO range to it.
But the thing is, of_pinctrl_get() just returns the first pinctrl_dev that
corresponds to the pinctrl node, and in my case, that's always the "sys" one.

So instead of having:
- pinctrl:
	- pinctrl_dev: sys
		- gpio_range: sys
	- pinctrl_dev: smcard
		- gpio_range: smcard

I have:
- pinctrl:
	- pinctrl_dev: sys
		- gpio_range: sys
		- gpio_range: smcard
	- pinctrl_dev: smcard

However, pinctrl_request_gpio() which, as I said, is now called by my code, calls
pinctrl_get_device_gpio_range() and therefore does check the GPIO range of the
pinctrl_dev before returning it.
When exporting GPIO-0, the "sys" GPIO range does exist in the "sys" pinctrl_dev,
so that works. But when exporting GPIO-71, the "smcard" GPIO range does not for
the "smcard" pinctrl_dev, hence the error.

I think of_pinctrl_get() should check the pin range as well, do you concur?

Best regards,
Yves.


>From 62cfce78da5bc3d4a8d8870bad8396747f7b5806 Mon Sep 17 00:00:00 2001
From: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
Date: Tue, 25 Apr 2017 14:06:46 +0200
Subject: [PATCH] drivers: pinctrl: Add a pinctrl for mach-tango 

Change-Id: Id5b5872b2a7d116829ddc75577f26115eb6e1c98
Signed-off-by: Yves Lefloch <YvesMarie_Lefloch@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-common.dtsi |  31 ++
 arch/arm/configs/tango4_defconfig    |   2 +
 arch/arm/mach-tango/Kconfig          |   1 +
 drivers/pinctrl/Kconfig              |   7 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-tango.c      | 559 +++++++++++++++++++++++++++++++++++
 6 files changed, 601 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tango.c

diff --git a/arch/arm/boot/dts/tango4-common.dtsi b/arch/arm/boot/dts/tango4-common.dtsi
index a3e81ab..7cf698a 100644
--- a/arch/arm/boot/dts/tango4-common.dtsi
+++ b/arch/arm/boot/dts/tango4-common.dtsi
@@ -18,6 +18,37 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	pinctrl: pinctrl {
+		compatible = "sigma,smp8758-pinctrl";
+		groups = <&gpio_sys>, <&gpio_smcard>;
+	};
+
+	gpio_sys: gpio@10500 {
+		reg = <0x10500 0x8>;
+		tango,mux = "none";
+		tango,label = "sys";
+		gpio-controller;
+		#gpio-cells = <1>;
+		gpio-ranges = <&pinctrl 0 0 16>;
+	};
+
+	smcard: smcard@6c300 {
+		/* Driver in the works. */
+		reg = <0x6c300 0x6c>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		gpio_smcard: gpio@6c358 {
+			reg = <0x6c358 0xc>;
+			ngpios = <8>;
+			tango,mux = "pin";
+			tango,label = "smcard";
+			gpio-controller;
+			#gpio-cells = <1>;
+			gpio-ranges = <&pinctrl 0 71 8>;
+		};
+	};
+
 	periph_clk: periph_clk {
 		compatible = "fixed-factor-clock";
 		clocks = <&clkgen CPU_CLK>;
diff --git a/arch/arm/configs/tango4_defconfig b/arch/arm/configs/tango4_defconfig
index b26bb4e..b58b01b 100644
--- a/arch/arm/configs/tango4_defconfig
+++ b/arch/arm/configs/tango4_defconfig
@@ -68,7 +68,9 @@ CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C=y
 CONFIG_I2C_XLR=y
+CONFIG_PINCTRL_TANGO=y
 CONFIG_GPIOLIB=y
+CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_CPU_THERMAL=y
 CONFIG_TANGO_THERMAL=y
diff --git a/arch/arm/mach-tango/Kconfig b/arch/arm/mach-tango/Kconfig
index ebe15b9..479c9aa 100644
--- a/arch/arm/mach-tango/Kconfig
+++ b/arch/arm/mach-tango/Kconfig
@@ -11,3 +11,4 @@ config ARCH_TANGO
 	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD
 	select TANGO_IRQ
+	select PINCTRL
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0e75d94..5648e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -179,6 +179,13 @@ config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP
 
+config PINCTRL_TANGO
+	tristate "Tango pin control driver"
+	depends on OF
+	select PINMUX
+	help
+	  Say yes to activate the pinctrl/gpio driver for Tango chips.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 11bad37..d5563e49 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
+obj-$(CONFIG_PINCTRL_TANGO)	+= pinctrl-tango.o
 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
diff --git a/drivers/pinctrl/pinctrl-tango.c b/drivers/pinctrl/pinctrl-tango.c
new file mode 100644
index 0000000..3170d8f
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tango.c
@@ -0,0 +1,559 @@
+/*
+ * Copyright (C) 2017 Sigma Designs
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+/* Pin regs are laid out this way:
+ * ______________________________
+ * | bitwise mask | bitwise val |
+ * 32             16            0
+ *
+ * To set bit n, one must write (1 << (16+n) | n) to the reg.
+ * Consequently, one reg can't handle more than 16 pins, so n < 16.
+ *
+ * There are 3 kind of registers that follow this layout: value registers,
+ * direction registers, and gpio-mode registers.
+ * The latter kind allows muxing between the primary function (eg UART), which
+ * is the pin's direction and value entirely controlled by hardware, and the
+ * "GPIO mode", which is the pin's direction and value entirely controlled by
+ * software. This gpio-mode register only exists for some GPIO groups.
+ *
+ * Additionally, for some pin groups, there is no gpio-mode register that muxes
+ * with the granularity of a pin, but rather with the granularity of the whole
+ * group. In that case, such mux register does *not* follow the layout above.
+ *
+ * Finally, it is possible for some groups to have neither type of muxing. In
+ * that case, the pins are dedicated GPIOs.
+ *
+ * Registers are organized this way for each group:
+ * group_mux? (direction value pin_mux?)+
+ */
+
+enum mux_type {
+	MUX_NONE,
+	MUX_PIN,
+	MUX_GROUP,
+};
+
+/* Which magic values to write in the mux group registers. */
+#define MUX_GROUP_GPIO 0x6
+#define MUX_GROUP_ALTF 0x1
+
+/* Conversion table for char* <-> enum mux_type */
+static const struct {
+	const char *str;
+	enum mux_type type;
+} mux_types[] = {
+	{ .str = "none",  .type = MUX_NONE,  },
+	{ .str = "pin",   .type = MUX_PIN,   },
+	{ .str = "group", .type = MUX_GROUP, },
+};
+
+/* Pinctrl device data */
+struct pingroup {
+	struct list_head list;
+	struct device_node *np;
+	const char *label;
+	unsigned base;
+	unsigned ngpio;
+	size_t sz;
+	phys_addr_t pa;
+	void __iomem *va;
+	enum mux_type mux_type;
+};
+
+/* Platform device data */
+struct pinctrl_data {
+	struct list_head group_list;
+};
+
+#define __reg_offset_from_pin(pg, pin) ( \
+		((pg)->mux_type == MUX_GROUP ? sizeof(u32) : 0) + \
+		((pg)->mux_type == MUX_PIN ? 3 : 2) * sizeof(u32) * ((pin)/16) \
+		)
+#define group_mux_reg_va(pg)    ((pg)->va)
+#define dir_reg_va(pg, pin)     (group_mux_reg_va(pg) + __reg_offset_from_pin(pg, pin))
+#define val_reg_va(pg, pin)     (dir_reg_va(pg, pin)  + sizeof(u32))
+#define pin_mux_reg_va(pg, pin) (val_reg_va(pg, pin)  + sizeof(u32))
+
+static int pinreg_get_bit(void __iomem *addr, unsigned offset)
+{
+	unsigned long val = readl(addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(val & GENMASK(31, 16));
+#endif
+
+	return test_bit(offset, &val);
+}
+
+static void pinreg_set_bit(void __iomem *addr, unsigned offset, int value)
+{
+	unsigned long val = 0;
+
+	/* Enable change by writing upper half-word too. */
+	set_bit(offset + 16, &val);
+	if (value)
+		set_bit(offset, &val);
+
+	writel(val, addr);
+
+	/* TODO remove me later */
+#if IS_ENABLED(CONFIG_DEBUG)
+	BUG_ON(pinreg_get_bit(addr, offset) != value);
+#endif
+}
+
+static void pinreg_set_multiple(void __iomem *addr, u16 mask, u16 bits)
+{
+	unsigned long val = (mask << 16) | bits;
+
+	writel(val, addr);
+}
+
+static int gpio_request(struct gpio_chip *gc, unsigned offset)
+{
+	return pinctrl_request_gpio(gc->base + offset);
+}
+
+static void gpio_free(struct gpio_chip *gc, unsigned offset)
+{
+	pinctrl_free_gpio(gc->base + offset);
+}
+
+static int gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	return pinreg_get_bit(val_reg_va(group, offset), offset % 16);
+}
+
+static void gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+}
+
+static int gpio_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	return !pinreg_get_bit(dir_reg_va(group, offset), offset % 16);
+}
+
+static int gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 0);
+
+	return 0;
+}
+
+static int gpio_direction_output(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	/* The output value can be changed before the direction is. */
+	pinreg_set_bit(val_reg_va(group, offset), offset % 16, value);
+
+	/* Bit set means output. */
+	pinreg_set_bit(dir_reg_va(group, offset), offset % 16, 1);
+
+	return 0;
+}
+
+static void gpio_set_multiple(struct gpio_chip *gc,
+		unsigned long *mask, unsigned long *bits)
+{
+	unsigned i, j;
+	unsigned num_of_ulongs;
+	struct pingroup *group = gpiochip_get_data(gc);
+
+	if (!group)
+		return;
+
+	num_of_ulongs = (group->ngpio / 32) + 1;
+	for (i = 0; i < num_of_ulongs; i++)
+		for (j = 0; j < 1; j++) {
+			u16 _mask = (u16)(*(mask + i) >> (16 * j));
+			u16 _bits = (u16)(*(bits + i) >> (16 * j));
+			pinreg_set_multiple(val_reg_va(group, 2*i + j), _mask, _bits);
+		}
+}
+
+static const unsigned int all_pins[] = {
+	0,  1,  2,  3,  4,  5,  6,  7,  8,  9,
+	10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+	20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+	30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
+	40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
+	50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
+	60, 61, 62, 63, 64, 65, 66, 67, 68, 69,
+	70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
+	80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
+	90, 91, 92, 93, 94, 95, 96, 97, 98, 99,
+};
+
+static int get_group_count(struct pinctrl_dev *pctldev)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	return group->mux_type == MUX_NONE ? 0 : 1;
+}
+
+static const char *get_group_name(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return ERR_PTR(-EINVAL);
+
+	return group->label;
+}
+
+static int get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
+		const unsigned **pins, unsigned *num_pins)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	*pins = all_pins + group->base;
+	*num_pins = group->ngpio;
+
+	return 0;
+}
+
+static const struct pinctrl_ops pctlops = {
+	.get_groups_count = get_group_count,
+	.get_group_name = get_group_name,
+	.get_group_pins = get_group_pins,
+};
+
+int get_function_groups(struct pinctrl_dev *pctldev, unsigned selector,
+		const char * const **groups, unsigned *num_groups)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group)
+		return -EINVAL;
+
+	if (group->mux_type == MUX_NONE) {
+		*groups = NULL;
+		*num_groups = 0;
+	} else {
+		*groups = &group->label;
+		*num_groups = 1;
+	}
+
+	return 0;
+}
+
+int set_mux(struct pinctrl_dev *pctldev, unsigned func_selector,
+		unsigned group_selector)
+{
+	/* With only one possible alt function, muxing is only
+	 * necessary for GPIO request/free.
+	 */
+	return 0;
+}
+
+int gpio_request_enable(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return -EINVAL;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 1);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_GPIO, group_mux_reg_va(group));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void gpio_disable_free(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pingroup *group;
+
+	group = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!group || offset >= group->ngpio)
+		return;
+
+	switch (group->mux_type) {
+	case MUX_NONE:
+		break;
+	case MUX_PIN:
+		pinreg_set_bit(pin_mux_reg_va(group, offset), offset % 16, 0);
+		break;
+	case MUX_GROUP:
+		writel(MUX_GROUP_ALTF, group_mux_reg_va(group));
+		break;
+	}
+}
+
+const static struct pinmux_ops pmxops = {
+	.get_functions_count = get_group_count,
+	.get_function_name = get_group_name,
+	.get_function_groups = get_function_groups,
+	.set_mux = set_mux,
+	.gpio_request_enable = gpio_request_enable,
+	.gpio_disable_free = gpio_disable_free,
+	.strict = true,
+};
+
+static int get_mux_type_from_str(const char *str, enum mux_type *type)
+{
+	unsigned i;
+	for (i = 0; i < ARRAY_SIZE(mux_types); i++)
+		if (!strcmp(mux_types[i].str, str)) {
+			*type = mux_types[i].type;
+			return 0;
+		}
+	return -EINVAL;
+}
+
+static int get_node_info(struct device_node *np, struct pingroup *group)
+{
+	/* Extract info from the pinctrl node:
+	 * - the label;
+	 * - the number of pins;
+	 * - the address and size of the registers;
+	 * - the mux type.
+	 */
+	int ret;
+	u32 reg[2];
+	const char *label;
+	const char *tango_mux;
+	struct of_phandle_args ranges;
+
+	ret = of_property_read_string(np, "tango,mux", &tango_mux);
+	if (ret)
+		return ret;
+	ret = of_property_read_string(np, "tango,label", &label);
+	if (ret)
+		return ret;
+	ret = of_property_read_u32_array(np, "reg", reg, 2);
+	if (ret)
+		return ret;
+	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges",
+			3, 0, &ranges);
+	if (ret)
+		return ret;
+
+	ret = get_mux_type_from_str(tango_mux, &group->mux_type);
+	if (ret)
+		return ret;
+	group->label = label;
+	group->pa = reg[0];
+	group->sz = reg[1];
+	group->base = ranges.args[1];
+	group->ngpio = ranges.args[2];
+
+	group->np = np;
+
+	return 0;
+}
+
+static int register_group_gpios(struct device *dev, struct pingroup *group)
+{
+	struct gpio_chip *gchip;
+
+	gchip = devm_kzalloc(dev, sizeof(*gchip), GFP_KERNEL);
+	if (!gchip)
+		return -ENOMEM;
+
+	gchip->label = group->label;
+	gchip->parent = dev;
+	gchip->owner = THIS_MODULE;
+	gchip->request = gpio_request;
+	gchip->free = gpio_free;
+	gchip->get_direction = gpio_get_direction;
+	gchip->direction_input = gpio_direction_input;
+	gchip->direction_output = gpio_direction_output;
+	gchip->get = gpio_get;
+	gchip->set = gpio_set;
+	gchip->set_multiple = gpio_set_multiple;
+	gchip->base = group->base;
+	gchip->ngpio = group->ngpio;
+	gchip->of_node = group->np;
+
+	return devm_gpiochip_add_data(dev, gchip, group);
+}
+
+static int register_group_pins(struct device *dev, struct pingroup *group)
+{
+	unsigned i;
+	struct pinctrl_desc *pdesc;
+	struct pinctrl_pin_desc *ppdesc;
+	struct pinctrl_dev *pindev;
+
+	pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+	ppdesc = devm_kzalloc(dev, group->ngpio * sizeof(*ppdesc), GFP_KERNEL);
+	if (!ppdesc)
+		return -ENOMEM;
+
+	for (i = 0; i < group->ngpio; i++)
+		ppdesc[i].number = group->base + i;
+
+	pdesc->name = group->label;
+	pdesc->pins = ppdesc;
+	pdesc->npins = group->ngpio;
+	pdesc->pctlops = &pctlops;
+	pdesc->pmxops = &pmxops;
+	pdesc->owner = THIS_MODULE;
+
+	pindev = devm_pinctrl_register(dev, pdesc, group);
+	if (IS_ERR(pindev))
+		return PTR_ERR(pindev);
+
+	return 0;
+}
+
+static struct pingroup *create_group(struct device *dev, struct device_node *np)
+{
+	int ret;
+	struct pingroup *group;
+
+	group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return ERR_PTR(-ENOMEM);
+
+	/* Extract info from the device node. */
+	ret = get_node_info(np, group);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/* Ioremap the group registers. */
+	group->va = devm_ioremap_nocache(dev, group->pa, group->sz);
+	if (!group->va)
+		return ERR_PTR(-ENOMEM);
+
+	/* Register the group pins. */
+	ret = register_group_pins(dev, group);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/* Register the group GPIOs. */
+	ret = register_group_gpios(dev, group);
+	if (ret)
+		return ERR_PTR(ret);
+
+	dev_dbg(dev, "%-8s base=%-2d ngpio=%-2d OK\n",
+			group->label, group->base, group->ngpio);
+	return group;
+}
+
+static int tango_pinctrl_probe(struct platform_device *pdev)
+{
+	unsigned i = 0;
+	unsigned groups_num;
+	struct pinctrl_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&pdata->group_list);
+	platform_set_drvdata(pdev, pdata);
+
+	groups_num = of_count_phandle_with_args(np, "groups", NULL);
+	if (!groups_num)
+		return -EINVAL;
+
+	for (i = 0; i < groups_num; i++) {
+		int ret;
+		struct pingroup *group;
+		struct of_phandle_args args;
+
+		ret = of_parse_phandle_with_args(np, "groups", NULL, i, &args);
+		if (ret)
+			return ret;
+
+		group = create_group(&pdev->dev, args.np);
+		if (IS_ERR(group))
+			return PTR_ERR(group);
+		else
+			list_add_tail(&group->list, &pdata->group_list);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id tango_pinctrl_of_match[] = {
+	{ .compatible =  "sigma,smp8758-pinctrl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tango_pinctrl_of_match);
+
+static struct platform_driver tango_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-tango",
+		.of_match_table = of_match_ptr(tango_pinctrl_of_match),
+	},
+	.probe = tango_pinctrl_probe,
+};
+
+static int __init tango_pinctrl_init(void)
+{
+	return platform_driver_register(&tango_pinctrl_driver);
+}
+arch_initcall(tango_pinctrl_init);
+
+static void __exit tango_pinctrl_exit(void)
+{
+	platform_driver_unregister(&tango_pinctrl_driver);
+}
+module_exit(tango_pinctrl_exit);
+
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.10.1