[U-Boot] [PATCH v1] Add single register pin controller driver

Simon Glass sjg at chromium.org
Mon Feb 6 15:34:50 UTC 2017


Hi Felix,

On 3 February 2017 at 05:29, Felix Brack <fb at ltec.ch> wrote:
> This patch adds a pin controller driver supporting devices
> using a single configuration register per pin.
> Signed-off-by: Felix Brack <fb at ltec.ch>
> ---
>
>  drivers/pinctrl/Kconfig          |  10 +++
>  drivers/pinctrl/Makefile         |   1 +
>  drivers/pinctrl/pinctrl-single.c | 138 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-single.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index efcb4c0..32bda65 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -175,6 +175,16 @@ config PIC32_PINCTRL
>           by a device tree node which contains both GPIO defintion and pin control
>           functions.
>
> +config PINCTRL_SINGLE
> +       bool "Single register pin-control and pin-multiplex driver"
> +       depends on DM
> +       help
> +         This enables pinctrl driver for systems using a single register for
> +         pin configuration and multiplexing. TI's AM335X SoCs are examples of
> +         such systems.
> +         Depending on the platform make sure to also enable OF_TRANSLATE and
> +         eventually SPL_OF_TRANSLATE to get correct address translations.
> +
>  endif
>
>  source "drivers/pinctrl/meson/Kconfig"
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 512112a..f148f94 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_PIC32_PINCTRL)   += pinctrl_pic32.o
>  obj-$(CONFIG_PINCTRL_EXYNOS)   += exynos/
>  obj-$(CONFIG_PINCTRL_MESON)    += meson/
>  obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
> +obj-$(CONFIG_PINCTRL_SINGLE)   += pinctrl-single.o
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> new file mode 100644
> index 0000000..f9e04f0
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright (C) EETS GmbH, 2017, Felix Brack <f.brack at eets.ch>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm/device.h>
> +#include <dm/pinctrl.h>
> +#include <libfdt.h>
> +#include <asm/io.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct single_pdata {
> +       fdt_addr_t base;        /* first configuration register */
> +       int offset;             /* index of last configuration register */
> +       u32 mask;               /* configuration-value mask bits */
> +       int width;              /* configuration register bit width */
> +};
> +
> +struct single_fdt_pin_cfg {
> +       fdt32_t reg;
> +       fdt32_t val;
> +};
> +
> +static int single_configure_pins(struct udevice *dev,
> +                                const struct single_fdt_pin_cfg *pins,
> +                                int size)

Can you add a comment about this function? In particular, mention that
pins is a pointer to a device-tree property.

> +{
> +       struct single_pdata *pdata = dev->platdata;
> +       int count = size / sizeof(struct single_fdt_pin_cfg);
> +       int n, reg;
> +       u32 val;
> +
> +       for (n = 0; n < count; n++) {
> +               reg = fdt32_to_cpu(pins->reg);


> +               if ((reg < 0) || (reg > pdata->offset)) {
> +                       dev_dbg(dev, "  invalid register offset 0x%08x\n", reg);
> +                       pins++;
> +                       continue;
> +               }
> +               reg += pdata->base;
> +               switch (pdata->width) {
> +                       case 32: {

case should be at same level as switch

> +                               val = readl(reg) & ~pdata->mask;
> +                               val |= fdt32_to_cpu(pins->val) & pdata->mask;
> +                               writel(val, reg);
> +                               dev_dbg(dev, "  reg/val 0x%08x/0x%08x\n",
> +                                       reg, val);
> +                               break;
> +                       }
> +                       default: {
> +                               dev_warn(dev, "unsupported register width %i\n",
> +                                        pdata->width);
> +                       }
> +               }
> +               pins++;
> +       }
> +       return 0;
> +}
> +
> +static int single_set_state_simple(struct udevice *dev,
> +                                  struct udevice *periph)
> +{
> +       const void *fdt = gd->fdt_blob;
> +       const struct single_fdt_pin_cfg *prop;
> +       int len;
> +       int offset;
> +       const char *name;
> +
> +       offset = fdt_first_property_offset(fdt, periph->of_offset);
> +       if (offset < 0)
> +               return offset;
> +       prop = fdt_getprop_by_offset(fdt, offset, &name, &len);
> +       if (!prop)
> +               return len;
> +
> +       if (!strcmp(name, "pinctrl-single,pins")) {
> +               dev_dbg(dev, "configuring pins for %s\n", periph->name);
> +               if (len % sizeof(struct single_fdt_pin_cfg)) {
> +                       dev_dbg(dev, "  invalid pin configuration in fdt\n");
> +                       return -FDT_ERR_BADSTRUCTURE;
> +               }
> +               single_configure_pins(dev, prop, len);
> +       }

This seems odd in that it looks at the first property and processes it
if it is pinctrl-single,pins. Why not use fdt_getprop()?

> +
> +       return 0;
> +}
> +
> +static int single_ofdata_to_platdata(struct udevice *dev)
> +{
> +       fdt_addr_t addr;
> +       u32 of_reg[2];
> +       int res;
> +       struct single_pdata *pdata = dev->platdata;
> +
> +       pdata->width = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> +                                     "pinctrl-single,register-width", 0);
> +
> +       res = fdtdec_get_int_array(gd->fdt_blob, dev->of_offset,
> +                                  "reg", of_reg, 2);
> +       if (res)
> +               return res;
> +       pdata->offset = of_reg[1] - pdata->width / 8;
> +
> +       addr = dev_get_addr(dev);
> +       if (addr == FDT_ADDR_T_NONE) {
> +               dev_dbg(dev, "no valid base register address\n");
> +               return -FDT_ERR_BADSTRUCTURE;

-EINVAL (we can't return FDT error numbers to the driver-model system)

> +       }
> +       pdata->base = addr;
> +
> +       pdata->mask = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> +                                    "pinctrl-single,function-mask",
> +                                    0xffffffff);
> +       return 0;
> +}
> +
> +const struct pinctrl_ops single_pinctrl_ops = {
> +       .set_state_simple = single_set_state_simple,
> +       .set_state = pinctrl_generic_set_state,
> +};
> +
> +static const struct udevice_id single_pinctrl_match[] = {
> +       { .compatible = "pinctrl-single" },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(pcs_pinctrl) = {

Can you use a generic name here?

> +       .name = "single-pinctrl",
> +       .id = UCLASS_PINCTRL,
> +       .of_match = single_pinctrl_match,
> +       .ops = &single_pinctrl_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +       .platdata_auto_alloc_size = sizeof(struct single_pdata),
> +       .ofdata_to_platdata = single_ofdata_to_platdata,
> +};
> --
> 2.7.4
>

Regards,
Simon


More information about the U-Boot mailing list