Add single register pin controller driver
Subsystems (2)
PATCH 1/1 Add single register pin controller driver
This patch adds a pin controller driver supporting devices using a single configuration register per pin.
Signed-off-by Felix Brack <[email protected]>
---
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
drivers/pinctrl/Kconfig +10 -0
@@ -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"
drivers/pinctrl/Makefile +1 -0
@@ -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
drivers/pinctrl/pinctrl-single.c new file +138 -0
138 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH 1/1 Add single register pin controller driver
Hi Felix,
Quoted 66 lines · Felix Brack
This patch adds a pin controller driver supporting devices using a single configuration register per pin.
Signed-off-by Felix Brack <[email protected]>---
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.cdrivers/pinctrl/Kconfig +10
-0@@ -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"drivers/pinctrl/Makefile +1
-0@@ -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.odrivers/pinctrl/pinctrl-single.c new file +19
-0@@ -0,0 +1,138 @@ +/* +* Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]> +* +* 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 */
drivers/pinctrl/pinctrl-single.c line 29
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +10
-0+}; + +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.
Quoted 8 lines
drivers/pinctrl/pinctrl-single.c new file +8
-0+{ +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);
drivers/pinctrl/pinctrl-single.c line 45
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +8
-0+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
Quoted 31 lines
drivers/pinctrl/pinctrl-single.c new file +31
-0+ 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)
drivers/pinctrl/pinctrl-single.c line 86
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +10
-0+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()?
Quoted 14 lines
drivers/pinctrl/pinctrl-single.c new file +14
-0+ +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 ); +
drivers/pinctrl/pinctrl-single.c line 110
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +10
-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)
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +10
-0+ } + 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 = {
drivers/pinctrl/pinctrl-single.c line 130
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +10
-0+ .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?
Quoted 11 lines
drivers/pinctrl/pinctrl-single.c new file +8
-1+ .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
PATCH 1/1 Add single register pin controller driver
Hello Simon,
Thank you for the review! I will submit an improved version of the patch.
Quoted 74 lines · Simon Glass
Hi Felix,
Quoted 71 lines · Felix Brack
This patch adds a pin controller driver supporting devices using a single configuration register per pin.
Signed-off-by Felix Brack <[email protected]>---
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.cdrivers/pinctrl/Kconfig +10
-0@@ -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"drivers/pinctrl/Makefile +1
-0@@ -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.odrivers/pinctrl/pinctrl-single.c new file +24
-0@@ -0,0 +1,138 @@ +/* +* Copyright (C) EETS GmbH, 2017, Felix Brack <[email protected]> +* +* 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;
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c line 29
Quoted 5 lines
drivers/pinctrl/pinctrl-single.c new file +5
-0+}; + +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.
Quoted 19 lines
Quoted 8 lines
drivers/pinctrl/pinctrl-single.c new file +8
-0+{ +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);drivers/pinctrl/pinctrl-single.c line 45
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c new file +8
-0+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 : {
Quoted 2 lines
case should be at same level as switch
Right, my bad.
Quoted 36 lines
Quoted 36 lines
drivers/pinctrl/pinctrl-single.c new file +36
-0+ 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 )) {
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c line 86
Quoted 5 lines
drivers/pinctrl/pinctrl-single.c new file +5
-0+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.
Quoted 18 lines
Quoted 18 lines
drivers/pinctrl/pinctrl-single.c new file +18
-0+ +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;
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c line 110
Quoted 6 lines
drivers/pinctrl/pinctrl-single.c new file +6
-0+ 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.
Quoted 14 lines
Quoted 14 lines
drivers/pinctrl/pinctrl-single.c new file +14
-0+ } + 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, +}; +
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c line 130
Quoted 6 lines
drivers/pinctrl/pinctrl-single.c new file +6
-0+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.
Quoted 12 lines
Quoted 11 lines
drivers/pinctrl/pinctrl-single.c new file +8
-1+ .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
Quoted 3 lines
Regards,
Simon
Regards, Felix