[PATCH 1/6] ARM: sunxi: Add pinctrl driver for Allwinner SoCs

Linus Walleij linus.walleij at linaro.org
Tue Dec 11 11:28:20 EST 2012


On Mon, Dec 10, 2012 at 11:08 PM, Maxime Ripard
<maxime.ripard at free-electrons.com> wrote:

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

(...)
> +- allwinner,pin-ids: An integer array. Each integer in the array
> +  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.)

> +- allwinner,pull: Integer.
> +    0: No resistor
> +    1: Pull-up resistor
> +    2: Pull-down resistor

This seems legit.

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

(...)
> +       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?

> +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),
> +               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?

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

> +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);
> +       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.

> +       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),
> +                                    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?

(...)
> +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) {
> +               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?

(...)
> +++ b/drivers/pinctrl/pinctrl-sunxi.h

> +#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(                                    \
> +                               (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.

> +#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.

> +#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")
> +#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.

> +#define SUNXI_PINCTRL_PIN_PB0  PINCTRL_PIN(32 + 0, "PB0")
> +#define SUNXI_PINCTRL_PIN_PB1  PINCTRL_PIN(32 + 1, "PB1")
(...)

Dito for C, D, E, F, G...

Yours,
Linus Walleij


More information about the devicetree-discuss mailing list