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

Hi Felix,

On 3 February 2017 at 05:29, Felix Brack fb@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@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@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