[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