pinctrl: Add new pinctrl/GPIO driver
Subsystems (3)
v5 cover pinctrl: Add new pinctrl/GPIO driver
Hi,
This series is to add pinctrl & GPIO controller driver for a new SoC. Patch 1 adds pinmux & GPIO controller driver.
Patch 2 adds the corresponding dt bindings YAML document.
Patches are against Linux 5.4-rc1 at below Git tree:git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
v5:
- Fix code style/readability related review concerns.
- Change data type of groups property to uint32-array in dt bindings.
v4:
- Correct data types for some pin config properties in dt bindings.
- Improve pattern regex as per review feedback in dt bindings.
- Remove eqbr_set_val() & set_drv_cur() reg update routines & instead do reg updates in eqbr_pinconf_set() routine itself.
- Add locking in few ops where it was missing.
- Rename eqbr_gpio_desc struct to eqbr_gpio_ctrl and avoid using desc in variable namings so as not to confuse with GPIO descriptors.
- Address code quality/convention/style related review concerns.
v3:
- Add locking for GPIO IRQ ops.
- Fix property naming mistake in dt bindings document.
- Address other code quality related review concerns.
- Fix a build error reported by kbuild test robot.
- Remove deleted structure fields from comments.
v2:
- Enable GENERIC_PINMUX_FUNCTIONS & GENERIC_PINCTRL_GROUPS and use core provided code for pinmux_ops & pinctrl_ops. Remove related code from the driver.
- Enable GENERIC_PINCONF & use core provided pinconf code. Remove related code from the driver.
- Use GPIOLIB_IRQCHIP framework core code instead of implementing separtely in the driver.
- Enable GPIO_GENERIC and switch to core provided memory mapped GPIO banks design.
- Use standard pinctrl DT properties instead of custom made properties.
- Address review concerns for dt bindings document.
- Address code quality related review concerns.
v1:
- Initial version.
Rahul Tanwar (2):
pinctrl: Add pinmux & GPIO controller driver for a new SoC
dt-bindings: pinctrl: intel: Add for new SoC
.../bindings/pinctrl/intel,lgm-pinctrl.yaml | 104 +++ drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 962 +++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 144 +++ 5 files changed, 1229 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml create mode 100644 drivers/pinctrl/pinctrl-equilibrium.c create mode 100644 drivers/pinctrl/pinctrl-equilibrium.h
Signature · 1 line
2.11.0
PATCH v5 1/2 pinctrl: Add pinmux & GPIO controller driver for a new SoC
Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which controls pin multiplexing & configuration including GPIO functions selection & GPIO attributes configuration.
This IP is not based on & does not have anything in common with Chassis specification. The pinctrl drivers under pinctrl/intel/* are all based upon Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use pinctrl framework under pinctrl/intel/* and it requires a separate new driver.
Add a new GPIO & pin control framework based driver for this IP.
--- drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 962 ++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 144 +++++ 4 files changed, 1125 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-equilibrium.c create mode 100644 drivers/pinctrl/pinctrl-equilibrium.h
drivers/pinctrl/Kconfig +18 -0
@@ -420,4 +420,22 @@ config PINCTRL_TB10X depends on OF && ARC_PLAT_TB10X select GPIOLIB +config PINCTRL_EQUILIBRIUM + tristate "Generic pinctrl and GPIO driver for Intel Lightning Mountain SoC" + select PINMUX + select PINCONF + select GPIOLIB + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + select GENERIC_PINCONF + select GENERIC_PINCTRL_GROUPS + select GENERIC_PINMUX_FUNCTIONS + + help + Equilibrium pinctrl driver is a pinctrl & GPIO driver for Intel Lightning + Mountain network processor SoC that supports both the linux GPIO and pin + control frameworks. It provides interfaces to setup pinmux, assign desired + pin functions, configure GPIO attributes for LGM SoC pins. Pinmux and + pinconf settings are retrieved from device tree. + endif
drivers/pinctrl/Makefile +1 -0
@@ -46,6 +46,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o +obj-$(CONFIG_PINCTRL_EQUILIBRIUM) += pinctrl-equilibrium.o obj-y += actions/ obj-$(CONFIG_ARCH_ASPEED) += aspeed/
drivers/pinctrl/pinctrl-equilibrium.c new file +962 -0
962 lines not on this page · raw message
drivers/pinctrl/pinctrl-equilibrium.h new file +144 -0
144 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v5 2/2 dt-bindings: pinctrl: intel: Add for new SoC
In reply to Rahul TanwarAdd dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
--- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 104 +++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +104 -0
@@ -0,0 +1,104 @@ +# SPDX-License-Identifier: GPL-2.0-only +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/bindings/pinctrl/intel,lgm-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightning Mountain SoC pinmux & GPIO controller binding + +maintainers: + - Rahul Tanwar <[email protected]> + +description: | + Pinmux & GPIO controller controls pin multiplexing & configuration including + GPIO function selection & GPIO attributes configuration. + + Please refer to [1] for details of the common pinctrl bindings used by the + client devices. + + [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt + +properties: + compatible: + const: intel,lgm-pinctrl + + reg: + maxItems: 1 + +# Client device subnode's properties +patternProperties: + '-pins$': + type: object + description: + Pinctrl node's client devices use subnodes for desired pin configuration. + Client device subnodes use below standard properties. + + properties: + function: + $ref: /schemas/types.yaml#/definitions/string + description: + A string containing the name of the function to mux to the group. + + groups: + $ref: /schemas/types.yaml#/definitions/string-array + description: + An array of strings identifying the list of groups. + + pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + List of pins to select with this function. + + pinmux: + description: The applicable mux group. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32-array" + - enum: + - 0 #PINMUX_GPIO + - 1 + - 2 + - 3 + - 4 + + bias-pull-up: + type: boolean + bias-pull-down: + type: boolean + drive-strength: + type: boolean + slew-rate: + type: boolean + drive-open-drain: + type: boolean + output-enable: + type: boolean + + required: + - function + - groups + +required: + - compatible + - reg + +additionalProperties: false + +examples: + # Pinmux controller node + - | + pinctrl: pinctrl@e2880000 { + compatible = "intel,lgm-pinctrl"; + reg = <0xe2880000 0x100000>; + + # Client device subnode + uart0-pins: uart0 { + pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + function = "CONSOLE_UART0"; + pinmux = <1>, + <1>; + groups = "CONSOLE_UART0"; + }; + }; + +...
Signature · 1 line
2.11.0
Re: [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
In reply to Rahul TanwarQuoted 11 lines · Rahul Tanwar
Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which controls pin multiplexing & configuration including GPIO functions selection & GPIO attributes configuration.
This IP is not based on & does not have anything in common with Chassis specification. The pinctrl drivers under pinctrl/intel/* are all based upon Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use pinctrl framework under pinctrl/intel/* and it requires a separate new driver.
Add a new GPIO & pin control framework based driver for this IP.
drivers/pinctrl/pinctrl-equilibrium.c line 43
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+static void eqbr_gpio_enable_irq (struct irq_data *d) +{ +struct gpio_chip *gc =irq_data_get_irq_chip_data (d); +struct eqbr_gpio_ctrl *gctrl =gpiochip_get_data (gc); +unsigned int offset =irqd_to_hwirq (d); +unsigned long flags; + + gc->direction_input (gc, offset);
Does this any IO?
If so, between above and below a window of possible race. Ditto for all other functions that do something similar.
drivers/pinctrl/pinctrl-equilibrium.c line 241
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+raw_spin_lock_irqsave (&gctrl->lock, flags); +writel (BIT (offset), gctrl->membase +GPIO_IRNRNSET ); +raw_spin_unlock_irqrestore (&gctrl->lock, flags); +} + ret =bgpio_init (&gctrl->chip, dev, gctrl->bank->nr_pins /8 , + gctrl->membase +GPIO_IN , + gctrl->membase +GPIO_OUTSET , + gctrl->membase +GPIO_OUTCLR , + gctrl->membase +GPIO_DIR , +NULL , +0 );
One line?
drivers/pinctrl/pinctrl-equilibrium.c line 360
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static int get_drv_cur (void __iomem *mem,unsigned int offset) +{ +unsigned int idx = offset /DRV_CUR_PINS ;/* 0-15, 16-31 per register*/ +unsigned int val; + + val =readl (mem +REG_DRCC (idx)); + offset %=DRV_CUR_PINS ;
From style point of view is better to have
... foo = offset / X; ... bar = offset % X;
directly in definition block. Moreover, for example, on x86 it might be converted by compiler to single idiv call in assembly that returns in (eax, edx) both values at once.
drivers/pinctrl/pinctrl-equilibrium.c line 405
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ val =PARSE_DRV_CURRENT (val, offset); + +return val; +} +if (!(bank->aval_pinmap &BIT (offset))) { +dev_err (pctl->dev, +"PIN: %u is not valid, pinbase: %u, bitmap: %u\n" , + pin, bank->pin_base, bank->aval_pinmap); +return -ENODEV ; + }
Looks like aval_pinmap is NIH of valid_mask bitmap in GPIO library. Can you check if it suits your purposes?
drivers/pinctrl/pinctrl-equilibrium.c line 580
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static bool is_func_exist (struct eqbr_pmx_func *funcs,const char *name, +unsigned int nr_funcs,unsigned int *idx) +{ +int i; + +if (!funcs || !nr_funcs) +return false;
nr_funcs check is a dup of the one in for loop.
drivers/pinctrl/pinctrl-equilibrium.c line 583
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+for (i =0 ; i < nr_funcs; i++) { +if (funcs[i].name && (strcmp (funcs[i].name, name) ==0 ) ) {
An extra space, but you may use !strcmp() and make it shorter without redundant parentheses.
drivers/pinctrl/pinctrl-equilibrium.c line 616
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ *idx = i; +return true; + } + } + +return false; +} +switch (op) { +case OP_COUNT_NR_FUNCS :
case goes usually on the same column as switch.
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+if (!is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) + *nr_funcs = *nr_funcs +1 ; +break ; + +case OP_ADD_FUNCS : +if (!is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) + funcs[i].name = fn_name; +break ; + +case OP_COUNT_NR_FUNC_GRPS :
drivers/pinctrl/pinctrl-equilibrium.c line 637
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+if (is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) + funcs[fid].nr_groups++; +break ; + +case OP_ADD_FUNC_GRPS : +if (is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) { + for(j=0;
Other style issueS.
drivers/pinctrl/pinctrl-equilibrium.c line 649
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+ j < funcs[fid].nr_groups; + j++) { +if (!funcs[fid].groups[j]) +break ; + } + funcs[fid].groups[j] = prop->value; + } +break ; + +default : +return -EINVAL ; +
Redundant blank line.
drivers/pinctrl/pinctrl-equilibrium.c line 681
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ } +for (i =0 ; i < nr_funcs; i++) { +if (funcs[i].nr_groups) {
if (!foo) continue;
?
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ funcs[i].groups =devm_kcalloc (dev, funcs[i].nr_groups, +sizeof (*(funcs[i].groups)), +GFP_KERNEL ); +if (!funcs[i].groups) +return -ENOMEM ; + } + }
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
Hi Andy,
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Rahul Tanwar
Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which controls pin multiplexing & configuration including GPIO functions selection & GPIO attributes configuration.
This IP is not based on & does not have anything in common with Chassis specification. The pinctrl drivers under pinctrl/intel/* are all based upon Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use pinctrl framework under pinctrl/intel/* and it requires a separate new driver.
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c line 43
Quoted 9 lines
Add a new GPIO & pin control framework based driver for this IP.
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+static void eqbr_gpio_enable_irq (struct irq_data *d) +{ +struct gpio_chip *gc =irq_data_get_irq_chip_data (d); +struct eqbr_gpio_ctrl *gctrl =gpiochip_get_data (gc); +unsigned int offset =irqd_to_hwirq (d); +unsigned long flags; + + gc->direction_input (gc, offset);Does this any IO?
If so, between above and below a window of possible race. Ditto for all other functions that do something similar.
gpio-mmio lib uses its own spin lock when it does IO in gc->direction_input() And that would set pin direction as input (hw ensures that the pin is not driven high when set direction) while below will enable interrupton that pin. I do not see any possible race condition in that..
Quoted 4 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+raw_spin_lock_irqsave (&gctrl->lock, flags); +writel (BIT (offset), gctrl->membase +GPIO_IRNRNSET ); +raw_spin_unlock_irqrestore (&gctrl->lock, flags); +}
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 241
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ ret =bgpio_init (&gctrl->chip, dev, gctrl->bank->nr_pins /8 , + gctrl->membase +GPIO_IN , + gctrl->membase +GPIO_OUTSET , + gctrl->membase +GPIO_OUTCLR , + gctrl->membase +GPIO_DIR , +NULL , +0 );One line?
Sure, missed it.
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 360
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static int get_drv_cur (void __iomem *mem,unsigned int offset) +{ +unsigned int idx = offset /DRV_CUR_PINS ;/* 0-15, 16-31 per register*/ +unsigned int val; + + val =readl (mem +REG_DRCC (idx)); + offset %=DRV_CUR_PINS ;From style point of view is better to have
Quoted 6 lines
... foo = offset / X; ... bar = offset % X;directly in definition block. Moreover, for example, on x86 it might be converted by compiler to single idiv call in assembly that returns in (eax, edx) both values at once.
Ok, i will change it like that but will have to introduce one more variable.
Quoted 4 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ val =PARSE_DRV_CURRENT (val, offset); + +return val; +}
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 304
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+if (!(bank->aval_pinmap &BIT (offset))) { +dev_err (pctl->dev, +"PIN: %u is not valid, pinbase: %u, bitmap: %u\n" , + pin, bank->pin_base, bank->aval_pinmap); +return -ENODEV ; + }Looks like
aval_pinmapis NIH ofvalid_maskbitmap in GPIO library. Can you check if it suits your purposes?
I did check about it earlier & now as well. It doesn't suit our purpose. aval_pinmapin the driver indicates whether pad control of this pad is available or not. It's for all pins irrespective of if it is used as a gpio or not. Whereas, valid_mask bitmap of GPIO library is just to indicate if that gpio line is valid or not. valid_mask would have been useful if this driver was purely a GPIO driver.
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 580
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static bool is_func_exist (struct eqbr_pmx_func *funcs,const char *name, +unsigned int nr_funcs,unsigned int *idx) +{ +int i; + +if (!funcs || !nr_funcs) +return false;
nr_funcscheck is a dup of the one in for loop.
Sure, noted.
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c line 583
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+for (i =0 ; i < nr_funcs; i++) { +if (funcs[i].name && (strcmp (funcs[i].name, name) ==0 ) ) {An extra space, but you may use !strcmp() and make it shorter without redundant parentheses.
Well noted, thanks.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 616
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ *idx = i; +return true; + } + } + +return false; +} +switch (op) { +case OP_COUNT_NR_FUNCS :case goes usually on the same column as switch.
Sure, will change, didn't know about it. Thanks.
Quoted 14 lines
Quoted 14 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +14
-0+if (!is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) + *nr_funcs = *nr_funcs +1 ; +break ; + +case OP_ADD_FUNCS : +if (!is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) + funcs[i].name = fn_name; +break ; + +case OP_COUNT_NR_FUNC_GRPS : +if (is_func_exist (funcs, fn_name, + *nr_funcs, &fid))
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 637
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ funcs[fid].nr_groups++; +break ; + +case OP_ADD_FUNC_GRPS : +if (is_func_exist (funcs, fn_name, + *nr_funcs, &fid)) { + for(j=0;Other style issueS.
Will fix in v6, thanks.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c line 681
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ } +for (i =0 ; i < nr_funcs; i++) { +if (funcs[i].nr_groups) {if (!foo) continue;?
Sure, will change. Thanks.
Regards,
Rahul