pinctrl: Add new pinctrl/GPIO driver
Subsystems (3)
v3 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
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 | 114 +++ drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 964 +++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 141 +++ 5 files changed, 1238 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 v3 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 | 141 +++++ 4 files changed, 1124 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 +141 -0
141 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v3 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 | 114 +++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +114 -0
@@ -0,0 +1,114 @@ +# 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: + "^.*@[0-9a-fA-F]+$": + 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: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration. + + bias-pull-down: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration. + + drive-strength: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + slew-rate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + drive-open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration. + + output-enable: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output. + + + required: + - function + - groups + +required: + - compatible + - reg + +examples: + # Pinmux controller node + - | + pinctrl: pinctrl@e2880000 { + compatible = "intel,lgm-pinctrl"; + reg = <0xe2880000 0x100000>; + + # Client device subnode + uart0:uart0 { + pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + function = "CONSOLE_UART0"; + pinmux = <1>, + <1>; + groups = "CONSOLE_UART0"; + }; + }; + +...
Signature · 1 line
2.11.0
Quoted 7 lines · Rahul Tanwar
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
Quoted 6 lines
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.
This version looks perfectly acceptable to me, any remaining nits can surely be fixed-up in-tree.
I give the other reviewers some days to consider it and then I will merge it for v5.5.
Yours,
Linus Walleij
Re: [PATCH v3 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 24
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+static void eqbr_set_val (void __iomem *addr,u32 offset, +u32 mask,u32 set,raw_spinlock_t *lock)
This lock parameter is quite unusual. Can't you supply a pointer to a data structure which has lock along with MMIO address?
drivers/pinctrl/pinctrl-equilibrium.c line 30
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+{ +u32 val; +unsigned long flags; + +raw_spin_lock_irqsave (lock, flags); + mask = mask << offset;
Same Q. Why do you need these...
drivers/pinctrl/pinctrl-equilibrium.c line 32
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+ val =readl (addr); + val = (val & ~mask) | ((set << offset) & mask);
...offset shifts? It's unusual.
drivers/pinctrl/pinctrl-equilibrium.c line 185
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+writel (val, addr); +raw_spin_unlock_irqrestore (lock, flags); +} +static int gpiochip_setup (struct device *dev,struct eqbr_gpio_desc *desc) +{ +struct gpio_irq_chip *girq; +struct gpio_chip *gc; +#if defined(CONFIG_OF_GPIO ) + gc->of_node = desc->node; +#endif
Isn't it what GPIO library does for everybody?
drivers/pinctrl/pinctrl-equilibrium.c line 189
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ +if (!of_property_read_bool (desc->node,"interrupt-controller" )) { +dev_info (dev,"gc %s: doesn't act as interrupt controller!\n" , + desc->name);
Is it fatal or non-fatal?
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 218
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ } +} +static int gpiolib_reg (struct eqbr_pinctrl_drv_data *drvdata) +{ +struct device_node *np; +struct eqbr_gpio_desc *desc;
desc is very confusing here, since GPIO library uses this term for GPIO descriptors.
drivers/pinctrl/pinctrl-equilibrium.c line 249
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+struct device *dev; +int i, ret; +struct resource res; + + ret = bgpio_init(&desc->chip, dev, desc->bank->nr_pins/8,
'nr_pins / 8,'
Quoted 17 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +15
-0+ desc->membase +GPIO_IN , + desc->membase +GPIO_OUTSET , + desc->membase +GPIO_OUTCLR , + desc->membase +GPIO_DIR , +NULL , + 0); +if (ret) { +dev_err (dev,"unable to init generic GPIO\n" ); +return ret; + } +return 0 ; +} +static int eqbr_pinmux_set_mux (struct pinctrl_dev *pctldev, +unsigned int selector,unsigned int group) +{
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct function_desc *func; +struct group_desc *grp; +unsigned int *pinmux; +int i; + pinmux = grp->data; +for (i =0 ; i < grp->num_pins; i++) +eqbr_set_pin_mux (pctl, pinmux[i], grp->pins[i]);
Shouldn't be this part serialized?
Same Q to all similar places. I guess I already mentioned this in previous review.
drivers/pinctrl/pinctrl-equilibrium.c line 574
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+return 0 ; +} +static int is_func_exist(struct eqbr_pmx_func *funcs, const char *name,
Looks like it better to be boolean.
drivers/pinctrl/pinctrl-equilibrium.c line 583
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ unsigned int nr_funcs, unsigned int *idx) +{ + int i; + +if (!funcs || !nr_funcs) +return 0 ; + +for (i =0 ; i < nr_funcs; i++) { +
Redundant blank line.
Quoted 14 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +13
-0+if (funcs[i].name && (strcmp (funcs[i].name, name) ==0 ) ) { + *idx = i; +return 1 ; + } + } + +return 0 ; +} +static int funcs_utils (struct device *dev,struct eqbr_pmx_func *funcs, +unsigned int *nr_funcs,funcs_util_ops op) +{ +struct device_node *node = dev->of_node; +struct device_node *np;
drivers/pinctrl/pinctrl-equilibrium.c line 606
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+struct property *prop; +unsigned int fid; +const char *fn_name; +int i, j; + + i =0 ; +for_each_child_of_node (node, np) { + prop =of_find_property (np,"groups" ,NULL ); +if (prop) {
Why not
if (!prop) continue;
?
drivers/pinctrl/pinctrl-equilibrium.c line 608
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+if (of_property_read_string (np,"function" , + &fn_name)) {
It's perfectly one line. Perhaps you may need to configure your text editor.
drivers/pinctrl/pinctrl-equilibrium.c line 681
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ } + } + } + +return 0 ; +} +for (i=0 ; i < nr_funcs; i++) {
The better style is 'i = 0' and so on.
Simple be consistent. Or do everywhere 'i=0; i<nr_func; i++', etc. But remember that this is for sure will be declined by most of the maintainers.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ } +static int eqbr_build_groups (struct eqbr_pinctrl_drv_data *drvdata) +{ +struct device *dev = drvdata->dev; +struct device_node *node = dev->of_node;
drivers/pinctrl/pinctrl-equilibrium.c line 723
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+struct device_node *np; +struct property *prop; +int j, err; +unsigned int *pinmux, pin_id, pinmux_id; +struct group_desc group; + +for_each_child_of_node (node, np) { + prop =of_find_property (np,"groups" ,NULL ); +if (prop) {
if (!prop) continue; ?
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+ } +memset (&group,0 ,sizeof (group)); + pinmux =NULL ; + } + +return 0 ; +} +static int pinbank_init (struct device_node *np, +struct eqbr_pinctrl_drv_data *drvdata, +struct eqbr_pin_bank *bank,unsigned int id) +{
drivers/pinctrl/pinctrl-equilibrium.c line 847
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+struct device *dev = drvdata->dev; +struct of_phandle_args spec; + + bank->membase = drvdata->membase + id *PAD_REG_OFF ; + +if (of_parse_phandle_with_fixed_args (np,"gpio-ranges" ,3 ,0 , &spec)) { +dev_err (dev,"gpio-range not available!\n" ); +return -EFAULT ;
Shadowing error code with actually unsuitable one.
drivers/pinctrl/pinctrl-equilibrium.c line 869
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ } +return 0 ; +} +int i=0 , nr_gpio=0 ;
Style.
Besides the fact that better to put assignments closer to their usage.
drivers/pinctrl/pinctrl-equilibrium.c line 927
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+static int eqbr_pinctrl_probe (struct platform_device *pdev) +{ +struct eqbr_pinctrl_drv_data *drvdata; +struct device *dev = &pdev->dev; +int ret; + + drvdata =devm_kzalloc (dev,sizeof (*drvdata),GFP_KERNEL ); +if (!drvdata) +return -ENOMEM ; + + drvdata->dev = dev; +platform_set_drvdata (pdev, drvdata);
I think this makes sense to do as last call in the function.
Quoted 18 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +18
-0+ drvdata->membase =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (drvdata->membase)) +return PTR_ERR (drvdata->membase); + + ret =pinbank_probe (drvdata); +if (ret) +return ret; + + ret =pinctrl_reg (drvdata); +if (ret) +return ret; + + ret =gpiolib_reg (drvdata); +if (ret) +return ret; + +return 0 ; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
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 5 lines
drivers/pinctrl/pinctrl-equilibrium.c line 24
Quoted 3 lines
Add a new GPIO & pin control framework based driver for this IP.
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+static void eqbr_set_val (void __iomem *addr,u32 offset, +u32 mask,u32 set,raw_spinlock_t *lock)This lock parameter is quite unusual. Can't you supply a pointer to a data structure which has lock along with MMIO address?
On second thoughts, i realize that this function can be totally avoided since it just has two callers which can be further reduced to one caller. I will remove this function and instead do reg update in the caller function itself.
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 185
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static int gpiochip_setup (struct device *dev,struct eqbr_gpio_desc *desc) +{ +struct gpio_irq_chip *girq; +struct gpio_chip *gc; +#if defined(CONFIG_OF_GPIO ) + gc->of_node = desc->node; +#endif Isn't it what GPIO library does for everybody?
We have 4 different of_node's for 4 different gpio_chips/banks. GPIO library handles like below:
if (chip->parent) {
gdev->dev.parent = chip->parent; gdev->dev.of_node = chip->parent->of_node; }
#ifdef CONFIG_OF_GPIO
/* If the gpiochip has an assigned OF node this takes precedence */ if (chip->of_node)
gdev->dev.of_node = chip->of_node;
else
chip->of_node = gdev->dev.of_node;
#endif
So i think we need to assign 4 of_node's to gpio_chip's in the driver.
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c line 189
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ +if (!of_property_read_bool (desc->node,"interrupt-controller" )) { +dev_info (dev,"gc %s: doesn't act as interrupt controller!\n" , + desc->name);Is it fatal or non-fatal?
It is not fatal. But i am totally missing your point. Is it about dev_info() ? Can you please elaborate more ?
Quoted 11 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 218
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ } +} +static int gpiolib_reg (struct eqbr_pinctrl_drv_data *drvdata) +{ +struct device_node *np; +struct eqbr_gpio_desc *desc;desc is very confusing here, since GPIO library uses this term for GPIO descriptors.
Agree, better to rename it to avoid confusion with GPIO descriptors. I will rename it in v4. Thanks.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c line 249
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+struct device *dev; +int i, ret; +struct resource res; + + ret = bgpio_init(&desc->chip, dev, desc->bank->nr_pins/8,'
nr_pins/ 8,'
Well noted.
Quoted 21 lines
Quoted 20 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +20
-0+ desc->membase +GPIO_IN , + desc->membase +GPIO_OUTSET , + desc->membase +GPIO_OUTCLR , + desc->membase +GPIO_DIR , +NULL , + 0); +if (ret) { +dev_err (dev,"unable to init generic GPIO\n" ); +return ret; + } +return 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;
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ pinmux = grp->data; +for (i =0 ; i < grp->num_pins; i++) +eqbr_set_pin_mux (pctl, pinmux[i], grp->pins[i]);Shouldn't be this part serialized?
Same Q to all similar places. I guess I already mentioned this in previous review.
From serialization, you mean locking..rt ? Yes, there is one writel() statement inthis flow. I will add lock for that statement. Rechecked the code again, i do notfind any other similar places.
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c line 574
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+return 0 ; +} +static int is_func_exist(struct eqbr_pmx_func *funcs, const char *name,Looks like it better to be boolean.
Well noted, will change in v4.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 583
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ unsigned int nr_funcs, unsigned int *idx) +{ + int i; + +if (!funcs || !nr_funcs) +return 0 ; + +for (i =0 ; i < nr_funcs; i++) { +Redundant blank line.
Well noted, will change in v4.
Quoted 23 lines
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+if (funcs[i].name && (strcmp (funcs[i].name, name) ==0 ) ) { + *idx = i; +return 1 ; + } + } + +return 0 ; +} +static int funcs_utils (struct device *dev,struct eqbr_pmx_func *funcs, +unsigned int *nr_funcs,funcs_util_ops op) +{ +struct device_node *node = dev->of_node;drivers/pinctrl/pinctrl-equilibrium.c line 606
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+struct device_node *np; +struct property *prop; +unsigned int fid; +const char *fn_name; +int i, j; + + i =0 ; +for_each_child_of_node (node, np) { + prop =of_find_property (np,"groups" ,NULL ); +if (prop) {Why not
Quoted 3 lines
if (!prop) continue;?
Sure, i can change like that in v4.
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c line 608
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+if (of_property_read_string (np,"function" , + &fn_name)) {It's perfectly one line. Perhaps you may need to configure your text editor.
I am following strict 80 chars limit. This goes on to 81 chars. It's a bit confusing on when to adhere to 80 chars limit and when to bypass it :)
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 681
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ } + } + } + +return 0 ; +} +for (i=0 ; i < nr_funcs; i++) {The better style is 'i = 0' and so on.
Simple be consistent. Or do everywhere 'i=0; i<nr_func; i++', etc. But remember that this is for sure will be declined by most of the maintainers.
Sure, noted.
Quoted 15 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ } +static int pinbank_init(struct device_node *np, + struct eqbr_pinctrl_drv_data *drvdata,drivers/pinctrl/pinctrl-equilibrium.c line 847
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ struct eqbr_pin_bank *bank, unsigned int id) +{ +struct device *dev = drvdata->dev; +struct of_phandle_args spec; + + bank->membase = drvdata->membase + id *PAD_REG_OFF ; + +if (of_parse_phandle_with_fixed_args (np,"gpio-ranges" ,3 ,0 , &spec)) { +dev_err (dev,"gpio-range not available!\n" ); +return -EFAULT ;Shadowing error code with actually unsuitable one.
Will change in v4. Thanks.
Regards,
Rahul
Re: [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
Quoted 8 lines · Rahul
Quoted 6 lines · Andy Shevchenko
drivers/pinctrl/pinctrl-equilibrium.c line 24
Quoted 2 lines · Rahul Tanwar
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+static void eqbr_set_val (void __iomem *addr,u32 offset, +u32 mask,u32 set,raw_spinlock_t *lock)This lock parameter is quite unusual. Can't you supply a pointer to a data structure which has lock along with MMIO address?
Quoted 4 lines
On second thoughts, i realize that this function can be totally avoided since it just has two callers which can be further reduced to one caller. I will remove this function and instead do reg update in the caller function itself.
Good.
Quoted 25 lines
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 185
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static int gpiochip_setup (struct device *dev,struct eqbr_gpio_desc *desc) +{ +struct gpio_irq_chip *girq; +struct gpio_chip *gc; +#if defined(CONFIG_OF_GPIO ) + gc->of_node = desc->node; +#endif Isn't it what GPIO library does for everybody?
We have 4 different of_node's for 4 different gpio_chips/banks. GPIO library handles like below:
if (
chip->parent) {
gdev->dev.parent=chip->parent;gdev->dev.of_node=chip->parent->of_node; }#ifdef
CONFIG_OF_GPIO
/* If the gpiochip has an assigned OF node this takes precedence */ if (chip->of_node)
gdev->dev.of_node=chip->of_node;
else
chip->of_node=gdev->dev.of_node;
#endif
Quoted 1 line
So i think we need to assign 4 of_node's to gpio_chip's in the driver.
OK!
Quoted 5 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c line 189
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+if (!of_property_read_bool (desc->node,"interrupt-controller" )) { +dev_info (dev,"gc %s: doesn't act as interrupt controller!\n" , + desc->name);Is it fatal or non-fatal?
Quoted 8 lines
It is not fatal. But i am totally missing your point. Is it about
dev_info()? Can you please elaborate more ?Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;Ditto.
Review
Quoted 1 line
(quoted diff) +1
-0+ }
If it's fatal, you have to use dev_err() and return an appropriate error code, if it's not fatal, switch to dev_warn() in case user has to know that behaviour may be quite different, while seems to work in general or dev_dbg() if it's only for the developer. dev_info() with return 0 is quite confusing.
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+} +static int eqbr_pinmux_set_mux (struct pinctrl_dev *pctldev, +unsigned int selector,unsigned int group)drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+{ +struct eqbr_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +struct function_desc *func; +struct group_desc *grp; +unsigned int *pinmux; +int i; + pinmux = grp->data; +for (i =0 ; i < grp->num_pins; i++) +eqbr_set_pin_mux (pctl, pinmux[i], grp->pins[i]);Shouldn't be this part serialized?
Quoted 6 lines
Quoted 2 lines
Same Q to all similar places. I guess I already mentioned this in previous review.
From serialization, you mean locking..rt ? Yes, there is one
writel()statement inthis flow. I will add lock for that statement. Rechecked the code again, i do notfind any other similar places.
You need to clarify what exactly you are serializing. When you figure this out, the locking should be done accordingly.
Quoted 10 lines
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c line 608
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+return 0 ; +} +if (of_property_read_string (np,"function" , + &fn_name)) {It's perfectly one line. Perhaps you may need to configure your text editor.
I am following strict 80 chars limit. This goes on to 81 chars. It's a bit confusing on when to adhere to 80 chars limit and when to bypass it :)
I have checked again, it's exactly 80 characters.
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ }
Signature · 2 lines
With Best Regards, Andy Shevchenko
Rob Herring
maintainerRe: [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC
In reply to Rahul TanwarQuoted 35 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 | 114 +++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +20
-0@@ -0,0 +1,114 @@ +# 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 +
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+properties: + compatible: + const: intel,lgm-pinctrl + + reg: + maxItems: 1 + +# Client device subnode's properties +patternProperties: + "^.*@[0-9a-fA-F]+$":
A unit address is wrong here. Please define some pattern we can match on. '-pins$' perhaps.
Quoted 25 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +25
-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. + + 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"
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 65
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ - enum: + - 0 #PINMUX_GPIO + - 1 + - 2 + - 3 + - 4 + + bias-pull-up: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration.
Isn't this boolean?
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 69
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + bias-pull-down: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration.
And this?
Though looks like sometimes it has a value? Pull strength I guess.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 81
Quoted 12 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +12
-0+ + drive-strength: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + slew-rate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + drive-open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration.
boolean?
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 85
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + output-enable: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output.
boolean?
But really, all of these should have a common schema defining the types and only put any additional constraints here.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 94
Quoted 9 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +9
-0+ + + required: + - function + - groups + +required: + - compatible + - reg
additionalProperties: false
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 104
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:uart0 {
space ^
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 v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Hi Rob,
Thanks for the feedback.
Quoted 27 lines · Rob Herring
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 65
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+ bias-pull-up: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration.Isn't this boolean?
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 69
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + bias-pull-down: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration.And this?
Though looks like sometimes it has a value? Pull strength I guess.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 81
Quoted 12 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +12
-0+ + drive-strength: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + slew-rate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + drive-open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration.boolean?
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 85
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + output-enable: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output.boolean?
But really, all of these should have a common schema defining the types and only put any additional constraints here.
Yes, you are right. These are all boolean types.
All these are standard properties & we are using them with no additional constraintsi.e conforming to how they are already documented in pinctrl-bindings.txt. Shall ijust omit documenting these properties here in driver bindings ?
Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 104
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:uart0 {space ^
Just to be sure, you mean space misalignment at below line <65>; /* UART_TX0 */ ?Or is it something else ?
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + function = "CONSOLE_UART0"; + pinmux = <1>, + <1>;
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ groups = "CONSOLE_UART0"; + }; + }; + +...Signature · 1 line
2.11.0
Regards,
Rahul
Re: [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC
Quoted 57 lines · Rahul
Hi Rob,
Thanks for the feedback.
Quoted 27 lines · Rob Herring
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 65
Quoted 3 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +3
-0+ bias-pull-up: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration.Isn't this boolean?
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 69
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + bias-pull-down: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration.And this?
Though looks like sometimes it has a value? Pull strength I guess.
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 81
Quoted 12 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +12
-0+ + drive-strength: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + slew-rate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + drive-open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration.boolean?
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 85
Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +4
-0+ + output-enable: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output.boolean?
But really, all of these should have a common schema defining the types and only put any additional constraints here.
Yes, you are right. These are all boolean types.
All these are standard properties & we are using them with no additional constraintsi.e conforming to how they are already documented in pinctrl-bindings.txt. Shall ijust omit documenting these properties here in driver bindings ?Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 104
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:uart0 {space ^
Quoted 2 lines
Just to be sure, you mean space misalignment at below line <65>; /*
UART_TX0*/ ?Or is it something else ?
Please ignore this query of mine. I now realize that you meant space between alias name & node name i.e. uart0: uart0 {. I will fix it in nextpatch version. Thanks.
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + function = "CONSOLE_UART0"; + pinmux = <1>, + <1>;Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ groups = "CONSOLE_UART0"; + }; + }; + +...Signature · 1 line
2.11.0Regards,
Rahul