pinctrl: Add new pinctrl/GPIO driver
Subsystems (3)
v6 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
v6:
- Fix code quality/style/readability related review concerns.
- Remove enum usage from pinmux property in dt bindings.
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 | 98 +++ drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 953 +++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 144 ++++ 5 files changed, 1214 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 v6 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 | 953 ++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 144 +++++ 4 files changed, 1116 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 +953 -0
953 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
Re: [PATCH v6 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
Quoted 10 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.
Looking again into this DT parsing and at other drivers, can't you utilize pin control framework better?
I see some drivers are using
pinctrl_utils_add_map_mux()
among other calls.
Some comments below as well.
drivers/pinctrl/pinctrl-equilibrium.c line 306
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+writel (pmx, mem + (offset <<2 ));
offset * 4
looks more naturally here. Applies to other similar cases if any.
drivers/pinctrl/pinctrl-equilibrium.c line 362
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ val =readl (mem +REG_DRCC (idx)); + val =PARSE_DRV_CURRENT (val, pin_offset); + +return val;
Can be
return PARSE_DRV_CURRENT(readl(mem + REG_DRCC(idx)), pin_offset);
but it's up to you.
Quoted 22 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +22
-0+static int eqbr_pinconf_set (struct pinctrl_dev *pctldev,unsigned int pin, +unsigned long *configs,unsigned int num_configs) +{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct eqbr_gpio_ctrl *gctrl; +enum pin_config_param param; +struct eqbr_pin_bank *bank; +unsigned int val, offset; +struct gpio_chip *gc; +unsigned long flags; +void __iomem *mem; +u32 regval, mask; +int i; + +for (i =0 ; i < num_configs; i++) { + param =pinconf_to_config_param (configs[i]); + val =pinconf_to_config_argument (configs[i]); + + bank =find_pinbank_via_pin (pctl, pin); +if (!bank) { +dev_err (pctl->dev, +"Couldn't find pin bank for pin %u\n" , pin);
drivers/pinctrl/pinctrl-equilibrium.c line 473
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+return -ENODEV ; + } + mem = bank->membase; + offset = pin - bank->pin_base; + +switch (param) { +case PIN_CONFIG_BIAS_PULL_UP : + mem +=REG_PUEN ; + val &=0x1 ;
Unneeded if use standard pattern (see below).
drivers/pinctrl/pinctrl-equilibrium.c line 478
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_BIAS_PULL_DOWN : + mem +=REG_PDEN ; + val &=0x1 ;
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 483
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_DRIVE_OPEN_DRAIN : + mem +=REG_OD ; + val &=0x1 ;
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 489
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_DRIVE_STRENGTH : + mem +=REG_DRCC (offset /DRV_CUR_PINS ); + offset = (offset %DRV_CUR_PINS ) <<1 ; + val &=0x3 ;
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 490
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+ mask =GENMASK (offset +1 , offset);
GENMASK() badly works with non-constants. Better
mask = GENMASK(1, 0) << offset;
drivers/pinctrl/pinctrl-equilibrium.c line 494
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+break ; +case PIN_CONFIG_SLEW_RATE : + mem +=REG_SRC ; + val &=0x1 ;
Ditto.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_OUTPUT_ENABLE : + gctrl =get_gpio_ctrls_via_bank (pctl, bank); +if (!gctrl) { +dev_err (pctl->dev,"Failed to find gpio via bank pinbase: %u, pin: %u\n" , + bank->pin_base, pin); +return -ENODEV ; + } + gc = &gctrl->chip;
drivers/pinctrl/pinctrl-equilibrium.c line 513
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ gc->direction_output (gc, offset,0 ); +continue ; +default : +return -ENOTSUPP ; + } + +raw_spin_lock_irqsave (&pctl->lock, flags); + regval =readl (mem); + regval = (regval & ~mask) | (val << offset);
Standard pattern is to apply mask here:
regval = (regval & ~mask) | ((val << offset) & mask);
drivers/pinctrl/pinctrl-equilibrium.c line 610
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+writel (regval, mem); +raw_spin_unlock_irqrestore (&pctl->lock, flags); + } + +return 0 ; +} +dev_dbg (dev,"Group %s: not function binded!\n" , + (char *)prop->value);
Do you need casting here?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v6 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 7 lines
Quoted 1 line
Add a new GPIO & pin control framework based driver for this IP.
Looking again into this DT parsing and at other drivers, can't you utilize pin control framework better?
I see some drivers are using
pinctrl_utils_add_map_mux()
among other calls.
pinctrl_utils_add_map_mux() is already used in the driver via below generic op:
pinctrl_ops.dt_node_to_map = pinconf_generic_dt_node_map_all
Please see call graph of pinconf_generic_dt_node_map_all() where it eventually invokes pinctrl_utils_add_map_mux().
Drivers where you see explicit usage of pinctrl_utils_add_map_mux() are not using GENERIC_PINCONF of core framework.
Since we are using all possible core framework provided generic ops, so i think utilization of pin control framework should already be maximized.
Quoted 5 lines
Some comments below as well.
drivers/pinctrl/pinctrl-equilibrium.c line 306
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+writel (pmx, mem + (offset <<2 ));offset * 4
looks more naturally here. Applies to other similar cases if any.
Noted.
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 362
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ val =readl (mem +REG_DRCC (idx)); + val =PARSE_DRV_CURRENT (val, pin_offset); + +return val;Can be
return PARSE_DRV_CURRENT(readl(mem + REG_DRCC(idx)), pin_offset);but it's up to you.
Agree, will update.
Quoted 55 lines
Quoted 21 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +21
-0+static int eqbr_pinconf_set (struct pinctrl_dev *pctldev,unsigned int pin, +unsigned long *configs,unsigned int num_configs) +{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct eqbr_gpio_ctrl *gctrl; +enum pin_config_param param; +struct eqbr_pin_bank *bank; +unsigned int val, offset; +struct gpio_chip *gc; +unsigned long flags; +void __iomem *mem; +u32 regval, mask; +int i; + +for (i =0 ; i < num_configs; i++) { + param =pinconf_to_config_param (configs[i]); + val =pinconf_to_config_argument (configs[i]); + + bank =find_pinbank_via_pin (pctl, pin); +if (!bank) { + dev_err(pctl->dev,drivers/pinctrl/pinctrl-equilibrium.c line 473
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ "Couldn't find pin bank for pin %u\n", pin); +return -ENODEV ; + } + mem = bank->membase; + offset = pin - bank->pin_base; + +switch (param) { +case PIN_CONFIG_BIAS_PULL_UP : + mem +=REG_PUEN ; + val &=0x1 ;Unneeded if use standard pattern (see below).
drivers/pinctrl/pinctrl-equilibrium.c line 478
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_BIAS_PULL_DOWN : + mem +=REG_PDEN ; + val &=0x1 ;Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 483
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_DRIVE_OPEN_DRAIN : + mem +=REG_OD ; + val &=0x1 ;Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 489
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_DRIVE_STRENGTH : + mem +=REG_DRCC (offset /DRV_CUR_PINS ); + offset = (offset %DRV_CUR_PINS ) <<1 ; + val &=0x3 ;Ditto.
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c line 490
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+ mask =GENMASK (offset +1 , offset);
GENMASK()badly works with non-constants. Bettermask = GENMASK(1, 0) << offset;
Noted.
Quoted 26 lines
drivers/pinctrl/pinctrl-equilibrium.c line 494
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+break ; +case PIN_CONFIG_SLEW_RATE : + mem +=REG_SRC ; + val &=0x1 ;Ditto.
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ mask =BIT (offset); +break ; +case PIN_CONFIG_OUTPUT_ENABLE : + gctrl =get_gpio_ctrls_via_bank (pctl, bank); +if (!gctrl) { +dev_err (pctl->dev,"Failed to find gpio via bank pinbase: %u, pin: %u\n" , + bank->pin_base, pin); +return -ENODEV ; + }drivers/pinctrl/pinctrl-equilibrium.c line 513
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ gc = &gctrl->chip; + gc->direction_output (gc, offset,0 ); +continue ; +default : +return -ENOTSUPP ; + } + +raw_spin_lock_irqsave (&pctl->lock, flags); + regval =readl (mem); + regval = (regval & ~mask) | (val << offset);Standard pattern is to apply mask here:
Quoted 1 line
regval = (regval & ~mask) | ((val << offset) & mask);
Agree. In-fact, i had proposed to you exact same pattern earlier but it was in a different function call so i guess it was not that obvious. Will change, thanks.
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c line 610
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+writel (regval, mem); +raw_spin_unlock_irqrestore (&pctl->lock, flags); + } + +return 0 ; +} +dev_dbg (dev,"Group %s: not function binded!\n" , + (char *)prop->value);Do you need casting here?
I think yes, to avoid compiler warning..
Regards,
Rahul
PATCH v6 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 | 98 ++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +98 -0
@@ -0,0 +1,98 @@ +# 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" + + 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
Rob Herring
maintainerRe: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Quoted 15 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>--- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 98 ++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +0
-0
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 1
Quoted 1 line
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +1
-0@@ -0,0 +1,98 @@ +# SPDX-License-Identifier: GPL-2.0-only
For new bindings:
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
Quoted 29 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +29
-0+%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$':
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 40
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ 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.
Possible strings should be listed out here.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 45
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ + groups: + $ref: /schemas/types.yaml#/definitions/string-array + description: + An array of strings identifying the list of groups.
Possible strings should be listed out here.
Quoted 7 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +7
-0+ + pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + List of pins to select with this function. + + pinmux:
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 62
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ description: The applicable mux group. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32-array" + + bias-pull-up: + type: boolean + bias-pull-down: + type: boolean + drive-strength: + type: boolean
Not a boolean. Need to define possible values.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 64
Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +2
-0+ slew-rate: + type: boolean
Not a boolean. Need to define possible values.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 72
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +8
-0+ drive-open-drain: + type: boolean + output-enable: + type: boolean + + required: + - function + - groups
For the -pins nodes too:
additionalProperties: false
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +6
-0+ +required: + - compatible + - reg + +additionalProperties: false
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 88
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ +examples: + # Pinmux controller node + - | + pinctrl: pinctrl@e2880000 { + compatible = "intel,lgm-pinctrl"; + reg = <0xe2880000 0x100000>; + + # Client device subnode + uart0-pins: uart0 {
This fails 'make dt_binding_check'. Please fix and run that.
Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + function = "CONSOLE_UART0"; + pinmux = <1>, + <1>; + groups = "CONSOLE_UART0"; + }; + }; + +...Signature · 1 line
2.11.0
Re: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Hi Rob,
Thanks for feedback.
Quoted 19 lines · Rob Herring
Quoted 15 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>--- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 98 ++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +0
-0Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 1
Quoted 1 line
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +1
-0@@ -0,0 +1,98 @@ +# SPDX-License-Identifier: GPL-2.0-onlyFor new bindings:
Quoted 1 line
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
Well noted.
Quoted 32 lines
Quoted 32 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +32
-0+%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.
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 40
Quoted 7 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +7
-0+ 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.Possible strings should be listed out here.
Possible number of strings here is a huge number. I agree that it makes sense to list out the possible strings here but when the possible strings are huge, can we just omit specifying all of the strings ? I see many examples here where they only specify the string in examples.
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 45
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ + groups: + $ref: /schemas/types.yaml#/definitions/string-array + description: + An array of strings identifying the list of groups.Possible strings should be listed out here.
Same point for groups. Too many strings to list out here.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ + 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"
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 62
Quoted 7 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +7
-0+ + bias-pull-up: + type: boolean + bias-pull-down: + type: boolean + drive-strength: + type: booleanNot a boolean. Need to define possible values.
Agree. My mistake. Will fix it on v7.
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 64
Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +2
-0+ slew-rate: + type: booleanNot a boolean. Need to define possible values.
In our case, 0 here means slow slew & 1 means fast slew. There are no other possible values. Probably, i can add it in description while keeping data type as boolean.
Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 72
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +8
-0+ drive-open-drain: + type: boolean + output-enable: + type: boolean + + required: + - function + - groupsFor the -pins nodes too:
additionalProperties: false
Well noted.
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +9
-0+ +required: + - compatible + - reg + +additionalProperties: false + +examples: + # Pinmux controller node
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 88
Quoted 7 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +7
-0+ - | + pinctrl: pinctrl@e2880000 { + compatible = "intel,lgm-pinctrl"; + reg = <0xe2880000 0x100000>; + + # Client device subnode + uart0-pins: uart0 {This fails 'make
dt_binding_check'. Please fix and run that.
Will run & fix it in v7. Thanks.
Regards,
Rahul
Re: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
In reply to Rahul TanwarQuoted 4 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>
(...)
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 23
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+properties: + compatible: + const: intel,lgm-pinctrl
Just noted from another review where Rob noted that this name should match the internal name in the datasheet for this hardware block. Is it really called "lgm-pinctrl" inside Intel?
intel,lightning-mountain-io and similar are perfectly fine if that is the name it has in your documentation.
Yours,
Linus Walleij
Re: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Hi Linus,
Quoted 12 lines · Linus Walleij
Quoted 4 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>(...)
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 23
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+properties: + compatible: + const: intel,lgm-pinctrl
Quoted 6 lines
Just noted from another review where Rob noted that this name should match the internal name in the datasheet for this hardware block. Is it really called "lgm-pinctrl" inside Intel?
intel,lightning-mountain-io and similar are perfectly fine if that is the name it has in your documentation.
Our documentation does not have any specific names for these hardware blocks. It names it in a very generic/standard manner like GPIO, pinmux..
To make the name explicit & self explanatory, i should probably change the name as you suggested i.e. intel,lightning-mountain-io.
Regards,
Rahul
Re: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Quoted 21 lines · Rahul
Hi Linus,
Quoted 16 lines · Linus Walleij
Quoted 4 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>(...)
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 23
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+properties: + compatible: + const: intel,lgm-pinctrlJust noted from another review where Rob noted that this name should match the internal name in the datasheet for this hardware block. Is it really called "lgm-pinctrl" inside Intel?
Quoted 8 lines
Quoted 2 lines
intel,lightning-mountain-io and similar are perfectly fine if that is the name it has in your documentation.
Our documentation does not have any specific names for these hardware blocks. It names it in a very generic/standard manner like GPIO, pinmux..
To make the name explicit & self explanatory, i should probably change the name as you suggested i.e. intel,lightning-mountain-io.
You should also be consistent with 'lgm' vs. 'lightning-mountain' use across bindings some of which I think have already been accepted.
Rob
Re: [PATCH v6 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Quoted 24 lines · Rob Herring
Quoted 22 lines · Rahul
Hi Linus,
Quoted 18 lines · Linus Walleij
Quoted 4 lines · Rahul Tanwar
Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>(...)
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 23
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+properties: + compatible: + const: intel,lgm-pinctrlJust noted from another review where Rob noted that this name should match the internal name in the datasheet for this hardware block. Is it really called "lgm-pinctrl" inside Intel?
intel,lightning-mountain-io and similar are perfectly fine if that is the name it has in your documentation.
Quoted 8 lines
Quoted 5 lines
Our documentation does not have any specific names for these hardware blocks. It names it in a very generic/standard manner like GPIO, pinmux..
To make the name explicit & self explanatory, i should probably change the name as you suggested i.e. intel,lightning-mountain-io.
You should also be consistent with 'lgm' vs. 'lightning-mountain' use across bindings some of which I think have already been accepted.
Yes, other accepted drivers/bindings use 'lgm'. I will rename it to 'intel,lgm-io'to be consistent.Thanks.
Regards,
Rahul