pinctrl: Add new pinctrl/GPIO driver
Subsystems (3)
v4 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
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 | 964 +++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 142 +++ 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 v4 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 | 964 ++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 142 +++++ 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 +964 -0
964 lines not on this page · raw message
drivers/pinctrl/pinctrl-equilibrium.h new file +142 -0
142 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v4 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" + - 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 v4 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 64
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+static void eqbr_gpio_mask_ack_irq (struct irq_data *d) +{ +eqbr_gpio_disable_irq (d); +eqbr_gpio_ack_irq (d);
Potential race?
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+} +static int eqbr_pinmux_set_mux (struct pinctrl_dev *pctldev, +unsigned int selector,unsigned int group) +{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct function_desc *func; +struct group_desc *grp; +unsigned int *pinmux; +int i; + + func =pinmux_generic_get_function (pctldev, selector); +if (!func)
drivers/pinctrl/pinctrl-equilibrium.c line 331
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+return -EINVAL ; + + grp =pinctrl_generic_get_group (pctldev, group); +if (!grp) +return -EINVAL ; + + pinmux = grp->data; +for (i =0 ; i < grp->num_pins; i++) +eqbr_set_pin_mux (pctl, pinmux[i], grp->pins[i]);
What if in the middle of the loop mux of one of the pins be changed by parallel thread?
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+return 0 ; +} +static int eqbr_pinmux_gpio_request (struct pinctrl_dev *pctldev, +struct pinctrl_gpio_range *range, +unsigned int pin) +{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); + +/* 0 mux is reserved for GPIO */
Perhaps
#define EQBR_GPIO_MODE 0
?
drivers/pinctrl/pinctrl-equilibrium.c line 561
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+return eqbr_set_pin_mux (pctl,0 , pin); +} +for (i =0 ; i < npins; i++) { + ret =eqbr_pinconf_set (pctldev, pins[i], configs, num_configs); +if (ret) +return ret;
What if in the middle of the loop settings of one of the pins be changed by parallel thread?
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+ } +static int eqbr_build_groups (struct eqbr_pinctrl_drv_data *drvdata) +{ +struct device *dev = drvdata->dev; +struct device_node *node = dev->of_node; +unsigned int *pinmux, pin_id, pinmux_id; +struct group_desc group; +struct device_node *np; +struct property *prop; +int j, err; +
drivers/pinctrl/pinctrl-equilibrium.c line 729
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+for_each_child_of_node (node, np) { + prop =of_find_property (np,"groups" ,NULL ); +if (!prop) +continue ; + + group.num_pins =of_property_count_u32_elems (np,"pins" ); +if (group.num_pins <0 ) { +dev_err (dev,"No pins in the group: %s\n" , + prop->name);
It is perfectly one line. The idea of if (!foo) pattern in the loop to make code occupy less LOCs along with increased readability.
drivers/pinctrl/pinctrl-equilibrium.c line 736
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+return -EINVAL ; + } + group.name = prop->value; + group.pins =devm_kcalloc (dev, group.num_pins, +sizeof (*(group.pins)),GFP_KERNEL ); + pinmux =devm_kcalloc (dev, group.num_pins, +sizeof (*pinmux),GFP_KERNEL );
These can be rearranged.
drivers/pinctrl/pinctrl-equilibrium.c line 742
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ +if (!group.pins || !pinmux) +return -ENOMEM ; +for (j =0 ; j < group.num_pins; j++) { +if (of_property_read_u32_index (np,"pins" , + j, &pin_id)) {
One line.
And so on...
Quoted 33 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +33
-0+dev_err (dev,"Group %s: Read intel pins id failed\n" , + group.name); +return -EINVAL ; + } +if (pin_id >= drvdata->pctl_desc.npins) { +dev_err (dev,"Group %s: Invalid pin ID, idx: %d, pin %u\n" , + group.name, j, pin_id); +return -EINVAL ; + } + group.pins[j] = pin_id; +if (of_property_read_u32_index (np,"pinmux" , + j, &pinmux_id)) { +dev_err (dev,"Group %s: Read intel pinmux id failed\n" , + group.name); +return -EINVAL ; + } + pinmux[j] = pinmux_id; + } + + err =pinctrl_generic_add_group (drvdata->pctl_dev, group.name, + group.pins, group.num_pins, + pinmux); +if (err <0 ) { +dev_err (dev,"Failed to register group %s\n" , + group.name); +return err; + } +memset (&group,0 ,sizeof (group)); + pinmux =NULL ; + } + +return 0 ; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
Hi Andy,
Quoted 22 lines · Andy Shevchenko
On Thu, Nov 07, 2019 at 03:36:44PM +0800, Rahul Tanwar wrote:
+static voideqbr_gpio_mask_ack_irq(struct irq_data *d)drivers/pinctrl/pinctrl-equilibrium.c line 64
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+{ +eqbr_gpio_disable_irq (d); +eqbr_gpio_ack_irq (d);Potential race?
Quoted 15 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +15
-0+} +static int eqbr_pinmux_set_mux (struct pinctrl_dev *pctldev, +unsigned int selector,unsigned int group) +{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct function_desc *func; +struct group_desc *grp; +unsigned int *pinmux; +int i; + + func =pinmux_generic_get_function (pctldev, selector); +if (!func) +return -EINVAL ; + + grp =pinctrl_generic_get_group (pctldev, group);
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 331
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+if (!grp) +return -EINVAL ; + + pinmux = grp->data; +for (i =0 ; i < grp->num_pins; i++) +eqbr_set_pin_mux (pctl, pinmux[i], grp->pins[i]);What if in the middle of the loop mux of one of the pins be changed by parallel thread?
These are all ops called back from the core pinctrl framework. My understanding is that multi-threading serialization is provided by the pinctrl framework using mutex's. Drivers don't have to worry about that. Drivers only have to worry about multi-core serialization. I checked many other existing pinctrl drivers & all of them seem to use spin lock only for register accesses (not for serializing the ops itself).
Is this understanding incorrect ?
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+/* 0 mux is reserved for GPIO */ Perhaps
#define
EQBR_GPIO_MODE0?
I had first used #define for this but removed it based on Rob Herring review feedback. I will add it back here but keep it as 0 in dt bindings..
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 561
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+return eqbr_set_pin_mux (pctl,0 , pin); +} +for (i =0 ; i < npins; i++) { + ret =eqbr_pinconf_set (pctldev, pins[i], configs, num_configs); +if (ret) +return ret;What if in the middle of the loop settings of one of the pins be changed by parallel thread?
Same comments as above..
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c line 736
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ group.pins =devm_kcalloc (dev, group.num_pins, +sizeof (*(group.pins)),GFP_KERNEL ); + pinmux =devm_kcalloc (dev, group.num_pins, +sizeof (*pinmux),GFP_KERNEL );These can be rearranged.
Lost you here. Please elaborate more on what you mean by rearranging. Thanks.
Regards,
Rahul