Hi Felix,
On 3 February 2017 at 05:29, Felix Brack <f...@ltec.ch> wrote:
> This patch adds a pin controller driver supporting devices
> using a single configuration register per pin.
> Signed-off-by: Felix Brack <f...@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.br...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
