Add pinctrl driver for Allwinner A1X SoCs

This IP has 8 banks of 32 bits, with a number of pins actually useful for each of these banks varying from one to another, and depending on the SoC used on the board.

This driver only implements the pinctrl part, the gpio part will come eventually.

drivers/pinctrl/pinctrl-sunxi.c new file +17 -0

+static int __devinit sunxi_pinctrl_parse_group(struct platform_device *pdev,
+                                              struct device_node *node,
+                                              int idx,
+                                              const char **out_name)
+{
+       struct sunxi_pinctrl *pctl = platform_get_drvdata(pdev);
+       struct sunxi_pinctrl_group *group = pctl->groups + idx;
+       struct property *prop;
+       char *group_name;
+       int i;
+       u32 val, proplen;
+
+       group_name = devm_kzalloc(&pdev->dev, strlen(node->name) + 4,
+                                 GFP_KERNEL);
+       if (!group_name)
+               return -ENOMEM;
+       if (of_property_read_u32(node, "reg", &val))

drivers/pinctrl/pinctrl-sunxi.c new file +8 -0

+       group->pins = devm_kzalloc(&pdev->dev,
+                                  group->npins * sizeof(*group->pins),
+                                  GFP_KERNEL);
+       if (!group->pins)
+               return -ENOMEM;
+
+       group->muxcfg = devm_kzalloc(&pdev->dev,
+                                    group->npins * sizeof(*group->muxcfg),

drivers/pinctrl/pinctrl-sunxi.c new file +7 -0

+static int __devinit sunxi_pinctrl_probe(struct platform_device *pdev)
+{
+       struct sunxi_pinctrl *pctl;
+       int i, ret;
+
+       pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+       if (!pctl)

+++ b/drivers/pinctrl/pinctrl-sunxi.h

drivers/pinctrl/pinctrl-sunxi.h new file +8 -0

+#define PINS_PER_BANK  32
+
+#define CFG_REG(pin)   (round_down(                                    \
+                               (pin / PINS_PER_BANK) * 0x24 +          \
+                               ((pin % PINS_PER_BANK) / 8) * 0x04, 4))
+#define CFG_OFFSET(pin)        ((pin % 8) * 4)
+
+#define DLEVEL_REG(pin)        (round_down(                                    \

drivers/pinctrl/pinctrl-sunxi.h new file +22 -0

+#define SUNXI_PINCTRL_PIN_PA0  PINCTRL_PIN(0 + 0, "PA0")
+#define SUNXI_PINCTRL_PIN_PA1  PINCTRL_PIN(0 + 1, "PA1")
+#define SUNXI_PINCTRL_PIN_PA2  PINCTRL_PIN(0 + 2, "PA2")
+#define SUNXI_PINCTRL_PIN_PA3  PINCTRL_PIN(0 + 3, "PA3")
+#define SUNXI_PINCTRL_PIN_PA4  PINCTRL_PIN(0 + 4, "PA4")
+#define SUNXI_PINCTRL_PIN_PA5  PINCTRL_PIN(0 + 5, "PA5")
+#define SUNXI_PINCTRL_PIN_PA6  PINCTRL_PIN(0 + 6, "PA6")
+#define SUNXI_PINCTRL_PIN_PA7  PINCTRL_PIN(0 + 7, "PA7")
+#define SUNXI_PINCTRL_PIN_PA8  PINCTRL_PIN(0 + 8, "PA8")
+#define SUNXI_PINCTRL_PIN_PA9  PINCTRL_PIN(0 + 9, "PA9")
+#define SUNXI_PINCTRL_PIN_PA10 PINCTRL_PIN(0 + 10, "PA10")
+#define SUNXI_PINCTRL_PIN_PA11 PINCTRL_PIN(0 + 11, "PA11")
+#define SUNXI_PINCTRL_PIN_PA12 PINCTRL_PIN(0 + 12, "PA12")
+#define SUNXI_PINCTRL_PIN_PA13 PINCTRL_PIN(0 + 13, "PA13")
+#define SUNXI_PINCTRL_PIN_PA14 PINCTRL_PIN(0 + 14, "PA14")
+#define SUNXI_PINCTRL_PIN_PA15 PINCTRL_PIN(0 + 15, "PA15")
+#define SUNXI_PINCTRL_PIN_PA16 PINCTRL_PIN(0 + 16, "PA16")
+#define SUNXI_PINCTRL_PIN_PA17 PINCTRL_PIN(0 + 17, "PA17")
+#define SUNXI_PINCTRL_PIN_PA18 PINCTRL_PIN(0 + 18, "PA18")
+#define SUNXI_PINCTRL_PIN_PA19 PINCTRL_PIN(0 + 19, "PA19")
+#define SUNXI_PINCTRL_PIN_PA20 PINCTRL_PIN(0 + 20, "PA20")
+#define SUNXI_PINCTRL_PIN_PA21 PINCTRL_PIN(0 + 21, "PA21")

Quoted 6 lines · Maxime Ripard

This IP has 8 banks of 32 bits, with a number of pins actually useful for each of these banks varying from one to another, and depending on the SoC used on the board.

This driver only implements the pinctrl part, the gpio part will come eventually.

It's looking pretty nice already :-)

Of course I have some comments...

OK will it be a combined driver so the same file implement both pinctrl and gpio?

Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt line 32

Quoted 7 lines

+- allwinner,pin-ids: An integer array. Each integer in the array

Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt new file +6 -0

+  specify a pin with given mux function, with bank, pin and mux packed
+  as below.
+
+    [15..12] : bank number
+    [11..4]  : pin number
+    [3..0]   : mux selection

Why are you using this scheme instead of just open-coding the three things? Well maybe I'm not getting it... Device Trees are usually for reading, not for bitstuffing, that's why I ask.

You should pass all this DT stuff to the devicetree-discuss list because I'm not any good at this (paging Stephen Warren.)

Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt line 54

Quoted 4 lines

+- allwinner,pull: Integer.

Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt new file +3 -0

+    0: No resistor
+    1: Pull-up resistor
+    2: Pull-down resistor

This seems legit.

drivers/pinctrl/Kconfig config PINCTRL_SIRF · line 153

Quoted 4 lines

drivers/pinctrl/Kconfig +4 -0

+config PINCTRL_SUNXI
+       bool
+       select PINMUX
+       select PINCONF

If your SoC is only simple pinconfig like pull-up/pull-down, why are you not using PINCONF_GENERIC and <linux/pinctrl/pinconf-generic.h>?

(...)

drivers/pinctrl/pinctrl-sunxi.c line 80

Quoted 7 lines

drivers/pinctrl/pinctrl-sunxi.c new file +7 -0

+       ret = of_property_read_u32(node, "allwinner,drive", &val);
+       if (!ret)
+               config |= val << DLEVEL_SHIFT | DLEVEL_PRESENT;
+
+       ret = of_property_read_u32(node, "allwinner,pull", &val);
+       if (!ret)
+               config |= val << PULL_SHIFT | PULL_PRESENT;

So looks nice... but can you use generic pinconfig?

Quoted 10 lines

drivers/pinctrl/pinctrl-sunxi.c new file +9 -0

+static void sunxi_pmx_set_config(struct pinctrl_dev *pctldev,
+                                unsigned pin,
+                                u8 config)
+{
+       struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+       u32 val = readl(pctl->membase + CFG_REG(pin));
+       u32 mask = ((1 << CFG_PINS_BITS) - 1) << CFG_OFFSET(pin);
+       writel((val & ~mask) | config << CFG_OFFSET(pin),

Review

Quoted 2 lines

(quoted diff) +2 -0

+               pctl->membase + CFG_REG(pin));
+}

There is something a bit confusing with the naming here, this is configuring the multiplexing (mux) but named config and CFG, which makes for great misunderstandings... can it be changed to eg just pmx_set() and MUX_OFFSET and MUX_REG() for example?

drivers/pinctrl/pinctrl-sunxi.c line 256

Quoted 4 lines

drivers/pinctrl/pinctrl-sunxi.c new file +4 -0

+static struct of_device_id sunxi_pinctrl_match[] __devinitconst = {
+       {}
+};
+MODULE_DEVICE_TABLE(of, sunxi_pinctrl_match);

This table will not match very much :-/

You should put one example in atleast? Something must have been used to test this...

Quoted 21 lines

drivers/pinctrl/pinctrl-sunxi.c new file +21 -0

+static int __devinit sunxi_pinctrl_parse_group(struct platform_device *pdev,
+                                              struct device_node *node,
+                                              int idx,
+                                              const char **out_name)
+{
+       struct sunxi_pinctrl *pctl = platform_get_drvdata(pdev);
+       struct sunxi_pinctrl_group *group = pctl->groups + idx;
+       struct property *prop;
+       char *group_name;
+       int i;
+       u32 val, proplen;
+
+       group_name = devm_kzalloc(&pdev->dev, strlen(node->name) + 4,
+                                 GFP_KERNEL);
+       if (!group_name)
+               return -ENOMEM;
+       if (of_property_read_u32(node, "reg", &val))
+               snprintf(group_name, strlen(node->name), "%s", node->name);
+       else
+               snprintf(group_name, strlen(node->name) + 4,
+                        "%s.%d", node->name, val);

drivers/pinctrl/pinctrl-sunxi.c line 284

Quoted 6 lines

drivers/pinctrl/pinctrl-sunxi.c new file +6 -0

+       group->name = group_name;
+
+       prop = of_find_property(node, "allwinner,pin-ids", &proplen);
+       if (!prop)
+               return -EINVAL;
+       group->npins = proplen / sizeof(u32);

So storing one u32 for every pin I guess.

Quoted 8 lines

drivers/pinctrl/pinctrl-sunxi.c new file +8 -0

+       group->pins = devm_kzalloc(&pdev->dev,
+                                  group->npins * sizeof(*group->pins),
+                                  GFP_KERNEL);
+       if (!group->pins)
+               return -ENOMEM;
+
+       group->muxcfg = devm_kzalloc(&pdev->dev,
+                                    group->npins * sizeof(*group->muxcfg),

drivers/pinctrl/pinctrl-sunxi.c line 303

Quoted 10 lines

drivers/pinctrl/pinctrl-sunxi.c new file +10 -0

+                                    GFP_KERNEL);
+       if (!group->muxcfg)
+               return -ENOMEM;
+
+       of_property_read_u32_array(node, "allwinner,pin-ids", group->pins,
+                                  group->npins);
+       for (i = 0; i < group->npins; i++) {
+               group->muxcfg[i] = MUXID_TO_MUXCFG(group->pins[i]);
+               group->pins[i] = MUXID_TO_PIN(group->pins[i]);
+       }

This loop is rather awkward I mean, instead of bitstuffing muxcfg and pin ID into a single u32 why not just have them as separate attributes.

Then there was somthing about bank ID which I guess is just discarded here?

Quoted 12 lines

drivers/pinctrl/pinctrl-sunxi.c new file +12 -0

+static int __devinit sunxi_pinctrl_probe(struct platform_device *pdev)
+{
+       struct sunxi_pinctrl *pctl;
+       int i, ret;
+
+       pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+       if (!pctl)
+               return -ENOMEM;
+       platform_set_drvdata(pdev, pctl);
+
+       ret = sunxi_pinctrl_probe_dt(pdev, pctl);
+       if (ret) {

drivers/pinctrl/pinctrl-sunxi.c line 404

Quoted 5 lines

drivers/pinctrl/pinctrl-sunxi.c new file +5 -0

+               dev_err(&pdev->dev, "dt probe failed: %d\n", ret);
+               return ret;
+       }
+
+       pctl->data = (struct sunxi_pinctrl_data *)of_match_device(sunxi_pinctrl_match, &pdev->dev)->data;

I can't parse this, what is going on here?
Can you break this statement apart somehow?

Quoted 16 lines

+++ b/drivers/pinctrl/pinctrl-sunxi.h

drivers/pinctrl/pinctrl-sunxi.h new file +14 -0

+#define PINS_PER_BANK  32
+
+#define CFG_REG(pin)   (round_down(                                    \
+                               (pin / PINS_PER_BANK) * 0x24 +          \
+                               ((pin % PINS_PER_BANK) / 8) * 0x04, 4))
+#define CFG_OFFSET(pin)        ((pin % 8) * 4)
+
+#define DLEVEL_REG(pin)        (round_down(                                    \
+                               (pin / PINS_PER_BANK) * 0x24 +          \
+                               ((pin % PINS_PER_BANK) / 16) * 0x04 +   \
+                               0x14, 4))
+#define DLEVEL_OFFSET(pin)     ((pin % 16) * 2)
+
+#define PULL_REG(pin)  (round_down(                                    \

drivers/pinctrl/pinctrl-sunxi.h line 35

Quoted 4 lines

drivers/pinctrl/pinctrl-sunxi.h new file +4 -0

+                               (pin / PINS_PER_BANK) * 0x24 +          \
+                               ((pin % PINS_PER_BANK) / 16) * 0x04 +   \
+                               0x1c, 4))
+#define PULL_OFFSET(pin)       ((pin % 16) * 2)

These are impossible to understand. Please convert these to documented static inline functions instead so the code can be maintained in the future.

drivers/pinctrl/pinctrl-sunxi.h line 45

Quoted 9 lines

drivers/pinctrl/pinctrl-sunxi.h new file +9 -0

+#define CFG_PINS_PER_REG       8
+#define CFG_PINS_BITS          4
+#define DLEVEL_PINS_PER_REG    16
+#define DLEVEL_PINS_BITS       2
+#define PULL_PINS_PER_REG      16
+#define PULL_PINS_BITS         2
+
+#define MUXID_TO_PIN(id)       ((((id) >> 12 & 0xf) * 32) + ((id) >> 4 & 0xff))
+#define MUXID_TO_MUXCFG(id)    ((id) & 0xf)

Same here.

Quoted 22 lines

drivers/pinctrl/pinctrl-sunxi.h new file +22 -0

+#define SUNXI_PINCTRL_PIN_PA0  PINCTRL_PIN(0 + 0, "PA0")
+#define SUNXI_PINCTRL_PIN_PA1  PINCTRL_PIN(0 + 1, "PA1")
+#define SUNXI_PINCTRL_PIN_PA2  PINCTRL_PIN(0 + 2, "PA2")
+#define SUNXI_PINCTRL_PIN_PA3  PINCTRL_PIN(0 + 3, "PA3")
+#define SUNXI_PINCTRL_PIN_PA4  PINCTRL_PIN(0 + 4, "PA4")
+#define SUNXI_PINCTRL_PIN_PA5  PINCTRL_PIN(0 + 5, "PA5")
+#define SUNXI_PINCTRL_PIN_PA6  PINCTRL_PIN(0 + 6, "PA6")
+#define SUNXI_PINCTRL_PIN_PA7  PINCTRL_PIN(0 + 7, "PA7")
+#define SUNXI_PINCTRL_PIN_PA8  PINCTRL_PIN(0 + 8, "PA8")
+#define SUNXI_PINCTRL_PIN_PA9  PINCTRL_PIN(0 + 9, "PA9")
+#define SUNXI_PINCTRL_PIN_PA10 PINCTRL_PIN(0 + 10, "PA10")
+#define SUNXI_PINCTRL_PIN_PA11 PINCTRL_PIN(0 + 11, "PA11")
+#define SUNXI_PINCTRL_PIN_PA12 PINCTRL_PIN(0 + 12, "PA12")
+#define SUNXI_PINCTRL_PIN_PA13 PINCTRL_PIN(0 + 13, "PA13")
+#define SUNXI_PINCTRL_PIN_PA14 PINCTRL_PIN(0 + 14, "PA14")
+#define SUNXI_PINCTRL_PIN_PA15 PINCTRL_PIN(0 + 15, "PA15")
+#define SUNXI_PINCTRL_PIN_PA16 PINCTRL_PIN(0 + 16, "PA16")
+#define SUNXI_PINCTRL_PIN_PA17 PINCTRL_PIN(0 + 17, "PA17")
+#define SUNXI_PINCTRL_PIN_PA18 PINCTRL_PIN(0 + 18, "PA18")
+#define SUNXI_PINCTRL_PIN_PA19 PINCTRL_PIN(0 + 19, "PA19")
+#define SUNXI_PINCTRL_PIN_PA20 PINCTRL_PIN(0 + 20, "PA20")
+#define SUNXI_PINCTRL_PIN_PA21 PINCTRL_PIN(0 + 21, "PA21")

drivers/pinctrl/pinctrl-sunxi.h line 85

Quoted 10 lines

drivers/pinctrl/pinctrl-sunxi.h new file +10 -0

+#define SUNXI_PINCTRL_PIN_PA22 PINCTRL_PIN(0 + 22, "PA22")
+#define SUNXI_PINCTRL_PIN_PA23 PINCTRL_PIN(0 + 23, "PA23")
+#define SUNXI_PINCTRL_PIN_PA24 PINCTRL_PIN(0 + 24, "PA24")
+#define SUNXI_PINCTRL_PIN_PA25 PINCTRL_PIN(0 + 25, "PA25")
+#define SUNXI_PINCTRL_PIN_PA26 PINCTRL_PIN(0 + 26, "PA26")
+#define SUNXI_PINCTRL_PIN_PA27 PINCTRL_PIN(0 + 27, "PA27")
+#define SUNXI_PINCTRL_PIN_PA28 PINCTRL_PIN(0 + 28, "PA28")
+#define SUNXI_PINCTRL_PIN_PA29 PINCTRL_PIN(0 + 29, "PA29")
+#define SUNXI_PINCTRL_PIN_PA30 PINCTRL_PIN(0 + 30, "PA30")
+#define SUNXI_PINCTRL_PIN_PA31 PINCTRL_PIN(0 + 31, "PA31")

0+0, 0+1, 0+2 .... why not just use the scalar? 0, 1, 2, ... 31?

I understand that the zero denotes bank 0 or bank A or somtheing (PA, PB etc) so if you need to keep that, use something like

#define PA_BASE 0

Then

#define SUNXI_PINCTRL_PIN_PA28 PINCTRL_PIN(PA_BASE + 28, "PA28")

which makes more sense.

Hi everyone,

This patch set adds a pinctrl driver for the IP found in the Allwinner A10 and A13, and the SoC-specific bits for the A13. It also adds the documentation for the device tree bindings.

Quoted 7 lines

Hi everyone,

This patch set adds a pinctrl driver for the IP found in the Allwinner A10
and A13, and the SoC-specific bits for the A13. It also adds the documentation
for the device tree bindings.

Hi. I just wanted to comment that there are 3 known sun5i SoCs, not only the A13. The A10s and the A12 are also sun5i. For A10s and A13 there are datasheets available with the pin info. So please don't assume the A13's pins are "sun5i" pins, they are merely aw1626 (aka A13) pins.

Hi Alejandro,

Le 11/12/2012 22:58, Alejandro Mery a ?crit :

Quoted 9 lines · Maxime Ripard

Quoted 7 lines

Hi everyone,

This patch set adds a pinctrl driver for the IP found in the Allwinner A10
and A13, and the SoC-specific bits for the A13. It also adds the documentation
for the device tree bindings.

Quoted 4 lines

Hi. I just wanted to comment that there are 3 known sun5i SoCs, not only the A13. The A10s and the A12 are also sun5i. For A10s and A13 there are datasheets available with the pin info. So please don't assume the A13's pins are "sun5i" pins, they are merely aw1626 (aka A13) pins.

Ok, I will change the naming scheme to something like sun5i-a13. For the other SoC codenames like sun4i, are you aware of several actual SoCs using the same codename, or do we have only sun4i == A10?

Quoted 12 lines

Hi Alejandro,

Le 11/12/2012 22:58, Alejandro Mery a ?crit :

Quoted 9 lines · Maxime Ripard

Quoted 7 lines

Hi everyone,

This patch set adds a pinctrl driver for the IP found in the Allwinner A10
and A13, and the SoC-specific bits for the A13. It also adds the documentation
for the device tree bindings.

Quoted 8 lines

Quoted 4 lines

Hi. I just wanted to comment that there are 3 known sun5i SoCs, not only the A13. The A10s and the A12 are also sun5i. For A10s and A13 there are datasheets available with the pin info. So please don't assume the A13's pins are "sun5i" pins, they are merely aw1626 (aka A13) pins.

Ok, I will change the naming scheme to something like sun5i-a13. For the other SoC codenames like sun4i, are you aware of several actual SoCs using the same codename, or do we have only sun4i == A10?

the A10 is the only sun4i we know of, the A31 is the only sun6i we know of, and the A20 is the only sun7i we know of... they'll soon run out of numbers... :|

Also, to avoid confusions with their poorly chosen marketing name, you could use the internal chip name aw1623 for the A10 and aw1626 for the A13 instead. This value can be read from the BROM and it's commonly used in allwinner sources.

Le 12/12/2012 13:49, Alejandro Mery a ?crit :

Quoted 22 lines · Maxime Ripard

Quoted 12 lines

Hi Alejandro,

Le 11/12/2012 22:58, Alejandro Mery a ?crit :

Quoted 9 lines · Maxime Ripard

Quoted 7 lines

Hi everyone,

This patch set adds a pinctrl driver for the IP found in the Allwinner A10
and A13, and the SoC-specific bits for the A13. It also adds the documentation
for the device tree bindings.

Quoted 8 lines

Quoted 4 lines

Hi. I just wanted to comment that there are 3 known sun5i SoCs, not only the A13. The A10s and the A12 are also sun5i. For A10s and A13 there are datasheets available with the pin info. So please don't assume the A13's pins are "sun5i" pins, they are merely aw1626 (aka A13) pins.

Ok, I will change the naming scheme to something like sun5i-a13. For the other SoC codenames like sun4i, are you aware of several actual SoCs using the same codename, or do we have only sun4i == A10?

Quoted 8 lines

the A10 is the only sun4i we know of, the A31 is the only sun6i we know of, and the A20 is the only sun7i we know of... they'll soon run out of numbers... :|

Also, to avoid confusions with their poorly chosen marketing name, you could use the internal chip name aw1623 for the A10 and aw1626 for the A13 instead. This value can be read from the BROM and it's commonly used in allwinner sources.

In what aspect are they poorly named? I'd really like to keep it as understandable as possible, at least in the device tree.

I guess renaming the sun5i dtsi already there and all that relates to the A13 to sun5i-a13 would remove that collision of names, don't you think? Or am I missing something?