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

Felix Brack fb at ltec.ch
Tue Feb 7 10:00:56 UTC 2017


Hello Simon,

Thank you for the review! I will submit an improved version of the patch.

On 06.02.2017 16:34, Simon Glass wrote:
> 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.
> 

Yes, I will.

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

Right, my bad.

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

Agreed. Seems I tried to reinvent the wheel.

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

Okay.

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

This is relict. I will use 'single_pinctrl' instead.

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

Regards, Felix


More information about the U-Boot mailing list