pinctrl: Add new pinctrl/GPIO driver
Subsystems (3)
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 dt bindings document & include file.
Patches are against Linux 5.3-rc5 at below Git tree:git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
Rahul Tanwar (2):
pinctrl: Add pinmux & GPIO controller driver for new SoC
dt-bindings: pinctrl: intel: Add for new SoC
.../bindings/pinctrl/intel,lgm-pinctrl.yaml | 131 ++ drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 1377 ++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 194 +++ include/dt-bindings/pinctrl/intel,equilibrium.h | 23 + 6 files changed, 1739 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 create mode 100644 include/dt-bindings/pinctrl/intel,equilibrium.h
Signature · 1 line
2.11.0
PATCH 1/2 pinctrl: Add pinmux & GPIO controller driver for 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. Add GPIO & pin control framework based driver for this IP.
--- drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-equilibrium.c | 1377 +++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-equilibrium.h | 194 +++++ 4 files changed, 1585 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-equilibrium.c create mode 100644 drivers/pinctrl/pinctrl-equilibrium.h
drivers/pinctrl/Kconfig +13 -0
@@ -420,4 +420,17 @@ 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 GPIOLIB_IRQCHIP + 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 +1377 -0
1,377 lines not on this page · raw message
drivers/pinctrl/pinctrl-equilibrium.h new file +194 -0
194 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH 2/2 dt-bindings: pinctrl: intel: Add for new SoC
In reply to Rahul TanwarAdd dt bindings document & include file for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
--- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 131 +++++++++++++++++++++ include/dt-bindings/pinctrl/intel,equilibrium.h | 23 ++++ 2 files changed, 154 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml create mode 100644 include/dt-bindings/pinctrl/intel,equilibrium.h
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +131 -0
@@ -0,0 +1,131 @@ +# 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 defined properties. + + properties: + intel,function: + $ref: /schemas/types.yaml#/definitions/string + description: + A string containing the name of the function to mux to the group. + + intel,groups: + $ref: /schemas/types.yaml#/definitions/string-array + description: + An array of strings identifying the list of groups. + + intel,pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + List of pins to select with this function. + + intel,mux: + description: The applicable mux group. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32" + - enum: + # Refer include/dt-bindings/pinctrl/intel,equilibrium.h + - PINMUX_0 # 0 PINMUX_GPIO + - PINMUX_1 # 1 + - PINMUX_2 # 2 + - PINMUX_3 # 3 + - PINMUX_4 # 4 + + intel,pullup: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration. + + intel,pulldown: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration. + + intel,drive-current: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + intel,slew-rate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + intel,open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration. + + intel,output: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output. + + + required: + - intel,function + - intel,groups + +required: + - compatible + - reg + +examples: + # Pinmux controller node + - | + pinctrl: pinctrl@e2880000 { + compatible = "intel,lgm-pinctrl"; + reg = <0xe2880000 0x100000>; + }; + + # Client device node + - | + asc0: serial@e0a00000 { + compatible = "intel,lgm-asc"; + reg = <0xe0a00000 0x1000>; + interrupt-parent = <&ioapic1>; + interrupts = <128 1>; + interrupt-names = "asc_irq"; + clocks = <&cgu0 31>, <&cgu0 98>; + clock-names = "freq", "asc"; + pinctrl-names = "default"; + pinctrl-0 = <&uart0>; + }; + + # Client device subnode + - | + uart0:uart0 { + intel,pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + intel,function = "CONSOLE_UART0"; + intel,mux = <1>, + <1>; + intel,groups = "CONSOLE_UART0"; + }; + + +...
include/dt-bindings/pinctrl/intel,equilibrium.h new file +23 -0
@@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ +#define __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ + +#define PINCTRL_DRCC_2_MA 0 +#define PINCTRL_DRCC_4_MA 1 +#define PINCTRL_DRCC_8_MA 2 +#define PINCTRL_DRCC_12_MA 3 + +#define PINMUX_0 0 +#define PINMUX_1 1 +#define PINMUX_2 2 +#define PINMUX_3 3 +#define PINMUX_4 4 +#define PINMUX_GPIO PINMUX_0 + +#define PINCTRL_GROUP "intel,groups" +#define PINCTRL_FUNCTION "intel,function" +#define PINCTRL_PINS "intel,pins" +#define PINCTRL_MUX "intel,mux" + +#endif /* __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ */
Signature · 1 line
2.11.0
Hi Rahul,
thanks for your patches!
Quoted 6 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 dt bindings document & include file.Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
OK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Yours,
Linus Walleij
Quoted 11 lines · Linus Walleij
Hi Rahul,
thanks for your patches!
Quoted 4 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 dt bindings document & include file.
Quoted 7 lines
Quoted 2 lines
Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.gitOK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Linus,
nevertheless I guess you may give your comments WRT device tree use (bindings, helpers, etc) along with some basics, (like devm_*() [ab]use I just noticed).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
In reply to Rahul TanwarQuoted 4 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. Add GPIO & pin control framework based driver for this IP.
Can you put few more words to explain how new this IP, and why it requires a complete separate driver?
drivers/pinctrl/pinctrl-equilibrium.c line 30
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+#define PIN_NAME_FMT "io-%d" +#define PIN_NAME_LEN 10 +#define PAD_REG_OFF 0x100 + +static const struct pin_config pin_cfg_type[] = { + {"intel,pullup" ,PINCONF_TYPE_PULL_UP }, + {"intel,pulldown" ,PINCONF_TYPE_PULL_DOWN }, + {"intel,drive-current" ,PINCONF_TYPE_DRIVE_CURRENT }, + {"intel,slew-rate" ,PINCONF_TYPE_SLEW_RATE }, + {"intel,open-drain" ,PINCONF_TYPE_OPEN_DRAIN }, + {"intel,output" ,PINCONF_TYPE_OUTPUT }, +};
Doesn't DT provide a generic naming scheme for these?
drivers/pinctrl/pinctrl-equilibrium.c line 40
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+ val =readl (addr) & ~(mask << offset); +writel (val | ((set & mask) << offset), addr);
More traditional pattern is
val = readl(...); val = (val & ~mask) | (newval & mask); writel(val, ...);
drivers/pinctrl/pinctrl-equilibrium.c line 127
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ virq =irq_find_mapping (desc->irq_domain, offset); +if (virq) +return virq; +else
Redundant.
drivers/pinctrl/pinctrl-equilibrium.c line 128
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+return irq_create_mapping (desc->irq_domain, offset);
Don't we have more clever helper for this? AFAIR something like this is done in IRQ framework when you get a mapping from certain domain.
drivers/pinctrl/pinctrl-equilibrium.c line 145
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+ gc->base =-1 ;/* desc->bank->pin_base; */
Useless comment.
drivers/pinctrl/pinctrl-equilibrium.c line 157
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ ret =devm_gpiochip_add_data (dev, gc, desc); +if (ret) +dev_err (dev,"failed to register gpiochip: %s, err: %d\n" , + gc->label, ret); + +return ret;
Simple
return devm_gpiochip_add_data(...);
would be enough.
drivers/pinctrl/pinctrl-equilibrium.c line 194
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+writel (readl (addr) & (~BIT (offset)), addr);
Too many parentheses.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static int eqbr_gpio_irq_req_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset; +int ret; +
drivers/pinctrl/pinctrl-equilibrium.c line 284
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ offset =irqd_to_hwirq (d); + +/* gpio must be set as input */ +intel_eqbr_gpio_dir_input (&desc->chip, offset); + ret =gpiochip_lock_as_irq (&desc->chip, offset); +if (ret) { +pr_err ("%s: Failed to lock gpio %u as irq!\n" , + desc->name, offset); +return ret; + }
Above is pretty much generic one...
drivers/pinctrl/pinctrl-equilibrium.c line 285
Quoted 1 line
drivers/pinctrl/pinctrl-equilibrium.c new file +1
-0+eqbr_gpio_enable_irq (d);
...and this may go to unmask() / enable() callback.
No?
drivers/pinctrl/pinctrl-equilibrium.c line 297
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+return 0 ; +} +static void eqbr_gpio_irq_rel_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset =irqd_to_hwirq (d); + +eqbr_gpio_disable_irq (d); +gpiochip_unlock_as_irq (&desc->chip, offset); +}
Ditto.
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static void eqbr_irq_handler (struct irq_desc *desc) +{ +struct intel_gpio_desc *gc; +struct irq_chip *ic; +u32 pins, offset; +unsigned int virq; + + gc =irq_desc_get_handler_data (desc); + ic =irq_desc_get_chip (desc);
drivers/pinctrl/pinctrl-equilibrium.c line 327
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ +chained_irq_enter (ic, desc); + pins =readl (gc->membase +GPIO_IRNCR ); + +for_each_set_bit (offset, (unsigned long *)&pins, gc->bank->nr_pins) { + virq =irq_linear_revmap (gc->irq_domain, offset); +if (!virq) +pr_err ("gc[%s]:pin:%d irq not registered!\n" , + gc->name, offset);
dev_err() ? But Why is it needed? Shouldn't be registered as a spurious IRQ for later debugging?
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+else +generic_handle_irq (virq); + } +chained_irq_exit (ic, desc); +} +static int gpiolib_reg (struct intel_pinctrl_drv_data *drvdata) +{ +struct device_node *np; +struct intel_gpio_desc *desc; +struct device *dev; +int i, ret; +char name[32 ];
drivers/pinctrl/pinctrl-equilibrium.c line 373
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+struct resource res; + + dev = drvdata->dev; +for (i =0 ; i < drvdata->nr_gpio_descs; i++) { + desc = drvdata->gpio_desc + i; + np = desc->node; +sprintf (name,"gpiochip%d" , i); + desc->name =devm_kmemdup (dev, name, +strlen (name) +1 ,GFP_KERNEL );
Have you run coccinelle scripts against this code? Above is copy'n'paste of devm_kasprintf().
drivers/pinctrl/pinctrl-equilibrium.c line 384
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+if (!desc->name) +return -ENOMEM ; +if (of_address_to_resource (np,0 , &res)) { +dev_err (dev,"Failed to get GPIO register addrss\n" ); +return -ENXIO ; + } + desc->membase =devm_ioremap_resource (dev, &res); +if (IS_ERR (desc->membase)) { +dev_err (dev,"ioremap fail\n" ); +return PTR_ERR (desc->membase); + }
Can't you simple use devm_platform_ioremap_resource()?
drivers/pinctrl/pinctrl-equilibrium.c line 386
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+dev_dbg (dev,"gpio resource: %pr\n" , &res); +dev_dbg (dev,"gpiochip membase: %px\n" , desc->membase);
Is it anyhow useful?
Quoted 28 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +27
-0+ desc->virq =irq_of_parse_and_map (np,0 ); +if (!desc->virq) { +dev_err (dev,"%s: failed to parse and map irq\n" , + name); +return -ENXIO ; + } +raw_spin_lock_init (&desc->lock); + + ret =gpiochip_setup (dev, desc); +if (ret) +return ret; + ret =irqchip_setup (dev, desc); +if (ret) +return ret; + } + +return 0 ; +} +static int add_config (struct intel_pinctrl_drv_data *drvdata, +unsigned long **confs,unsigned int *nr_conf, +unsigned long pinconf) +{ +unsigned long *configs; +struct device *dev = drvdata->dev; +unsigned int num_conf = *nr_conf +1 ; + +if (!(*nr_conf)) {
drivers/pinctrl/pinctrl-equilibrium.c line 467
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ configs =devm_kcalloc (dev,1 ,sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; + }else { + configs =devm_kmemdup (dev, *confs, + num_conf *sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; +devm_kfree (dev, *confs);
This a red flag for using devm_*().
Either a sign of bad design or misplacement of devm_*().
drivers/pinctrl/pinctrl-equilibrium.c line 537
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ } + + configs[num_conf -1 ] = pinconf; + *confs = configs; + *nr_conf = num_conf; + +return 0 ; +} +/**
Are you sure about this?
Have you run kernel-doc script? Does it complain?
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+* Create pinctrl_map for each groups, per group per entry. +* Create pinctrl_map for pin config, per group per entry. +*/ +if (nr_config) + map_cnt++;
drivers/pinctrl/pinctrl-equilibrium.c line 610
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+static void eqbr_dt_free_map (struct pinctrl_dev *pctldev, +struct pinctrl_map *map,unsigned int num_maps) +{ +struct intel_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +int i; + +for (i =0 ; i < num_maps; i++) +if (map[i].type ==PIN_MAP_TYPE_CONFIGS_GROUP ) +devm_kfree (pctl->dev, map[i].data.configs.configs); +devm_kfree (pctl->dev, map);
Yeah, no devm_*() here.
drivers/pinctrl/pinctrl-equilibrium.c line 705
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+} +static const struct pinmux_ops eqbr_pinmux_ops = { + .get_functions_count = eqbr_pinmux_get_func_count, + .get_function_name = eqbr_pinmux_get_fname, + .get_function_groups = eqbr_pinmux_get_groups, + .set_mux = eqbr_pinmux_set_mux, + .gpio_request_enable = eqbr_pinmux_gpio_request, + .strict =1 ,
Wrong type.
drivers/pinctrl/pinctrl-equilibrium.c line 714
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+}; + +static void set_drv_cur (void __iomem *mem,unsigned int offset, +unsigned int set,raw_spinlock_t *lock) +{ +unsigned int idx = offset;/* 16 pin per register*/ +unsigned int reg; + + idx = idx /DRV_CUR_PINS ;
It can be done in the first place in the definition block.
drivers/pinctrl/pinctrl-equilibrium.c line 718
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+ offset %=DRV_CUR_PINS ; + reg =REG_DRCC (idx); +eqbr_set_val (mem +REG_DRCC (idx), offset *2 , +0x3 , set, lock);
Quite enough space in previous line for arguments.
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+} +static int get_drv_cur (void __iomem *mem,unsigned int offset) +{
drivers/pinctrl/pinctrl-equilibrium.c line 732
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+unsigned int idx = offset;/* 0-15, 16-31 per register*/ +unsigned int val; + + idx = idx /DRV_CUR_PINS ; + val =readl (mem +REG_DRCC (idx)); + offset %=DRV_CUR_PINS ; + val =PARSE_DRV_CURRENT (val, offset); + +return val; +}
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 838
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+case PINCONF_TYPE_PULL_UP : +eqbr_set_val (mem +REG_PUEN , offset, +1 , val, &pctl->lock);
Please, configure you editor better. The recommended limit is 80, not 60.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+break ; +case PINCONF_TYPE_PULL_DOWN : +eqbr_set_val (mem +REG_PDEN , offset, +1 , val, &pctl->lock); +break ; +case PINCONF_TYPE_OPEN_DRAIN :
drivers/pinctrl/pinctrl-equilibrium.c line 854
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+eqbr_set_val (mem +REG_OD , offset, +1 , val, &pctl->lock); +break ; +case PINCONF_TYPE_DRIVE_CURRENT : +set_drv_cur (mem, offset, val, &pctl->lock); +break ; +case PINCONF_TYPE_SLEW_RATE : +eqbr_set_val (mem +REG_SRC , offset, +1 , val, &pctl->lock); +break ;
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 932
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +13
-0+ val = !!(readl (mem +REG_PUEN ) &BIT (offset)); +seq_printf (s,"PULL UP: %u\n" , val); + val = !!(readl (mem +REG_PDEN ) &BIT (offset)); +seq_printf (s,"PULL DOWN: %u\n" , val); + val = !!(readl (mem +REG_OD ) &BIT (offset)); +seq_printf (s,"OPEN DRAIN: %u\n" , val); + val =get_drv_cur (mem, offset); +seq_printf (s,"DRIVE CURRENT: %u\n" , val); + val = !!(readl (mem +REG_SRC ) &BIT (offset)); +seq_printf (s,"SLEW RATE: %u\n" , val); + gpio =get_gpio_desc_via_bank (pctl, bank); + val =intel_eqbr_gpio_get_dir (&gpio->chip, offset); +seq_printf (s,"OUTPUT: %u\n" , !val);
I think GPIO library does it for you.
drivers/pinctrl/pinctrl-equilibrium.c line 987
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+static int add_group_to_func (struct device *dev,struct intel_pmx_func *funcs, +unsigned int nr_funcs,unsigned int idx, +const char *grp_name) +{ +unsigned int nr_grps = funcs[idx].nr_groups +1 ; +const char **grps; + + grps =devm_kmemdup (dev, funcs[idx].groups, + nr_grps *sizeof (*grps),GFP_KERNEL ); +if (!grps) +return -ENOMEM ; +devm_kfree (dev, funcs[idx].groups);
Possible misplacement of devm_*().
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+ grps[nr_grps -1 ] = grp_name; + funcs[idx].groups = grps; + funcs[idx].nr_groups = nr_grps; + +return 0 ; +} +static int is_func_exist (struct intel_pmx_func *funcs,const char *name, +unsigned int nr_funcs,unsigned int *idx) +{ +int i; + +if (!funcs || !nr_funcs)
drivers/pinctrl/pinctrl-equilibrium.c line 1010
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+return 0 ; + +for (i =0 ; i < nr_funcs; i++) { +if (strcmp (funcs[i].name, name) ==0 ) { + *idx = i; +return 1 ; + } + } + +return 0 ;
NIH match_string(). But I think pin control core does it for drivers.
Quoted 20 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +19
-0+} +static void dump_pinctrl_group_func (struct intel_pinctrl_drv_data *drvdata) +{ +struct device *dev = drvdata->dev; +const struct intel_pin_group *group; +const struct intel_pmx_func *func; +int i, j; + +dev_info (dev,"Total %u groups, %u functions\n" , + drvdata->nr_grps, drvdata->nr_funcs); + +for (i =0 ; i < drvdata->nr_grps; i++) { + group = &drvdata->pin_grps[i]; + +dev_dbg (dev,"group name: %s, pin num: %u, pmx: %u\n" , + group->name, group->nr_pins, group->pmx); +for (j =0 ; j < group->nr_pins; j++) +dev_dbg (dev,"pin[%d]: %u\n" , j, group->pins[j]); + }
drivers/pinctrl/pinctrl-equilibrium.c line 1040
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ +for (i =0 ; i < drvdata->nr_funcs; i++) { + func = &drvdata->pmx_funcs[i]; + +dev_dbg (dev,"function name: %s, group num: %u\n" , + func->name, func->nr_groups); +for (j =0 ; j < func->nr_groups; j++) +dev_dbg (dev,"group[%d]: %s\n" , j, func->groups[j]); + } +}
Ditto.
drivers/pinctrl/pinctrl-equilibrium.c line 1044
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+static int pinctrl_setup_from_dt (struct device *dev, +struct intel_pinctrl_drv_data *drvdata) +{
It's a very big function with potential of refactoring. Also check what core provides.
drivers/pinctrl/pinctrl-equilibrium.c line 1191
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+} +static int pinctrl_reg (struct intel_pinctrl_drv_data *drvdata) +{ + drvdata->pctl_dev =devm_pinctrl_register (dev, pctl_desc, drvdata); +if (IS_ERR (drvdata->pctl_dev)) { +dev_err (dev,"Register pinctrl failed!\n" );
Useless.
drivers/pinctrl/pinctrl-equilibrium.c line 1195
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+return PTR_ERR (drvdata->pctl_dev); + } + +return 0 ;
PTR_ERR_OR_ZERO()
drivers/pinctrl/pinctrl-equilibrium.c line 1283
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+} +/** +* reset all pins to DEFAULT state, including below registers +* PINMUX set to GPIO +* DIR set to INPUT +* Clear PULLUP/PULLDOWN/SLEW RATE/DRIVE CURRENT/OPEN DRAIN +*/
Please, read kernel doc documentation.
drivers/pinctrl/pinctrl-equilibrium.c line 1294
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+static void pinctrl_pin_reset (struct intel_pinctrl_drv_data *drvdata) +{ +for (i =0 ; i < drvdata->nr_gpio_descs; i++) { + gdesc = &drvdata->gpio_desc[i]; +for (pin =0 ; pin < gdesc->bank->nr_pins; pin++) { +if (!(BIT (pin) & gdesc->bank->aval_pinmap)) +continue ;
for_each_set_bit()
drivers/pinctrl/pinctrl-equilibrium.c line 1340
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ } + } +} +static int intel_pinctrl_probe (struct platform_device *pdev) +{ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + drvdata->membase =devm_ioremap_resource (dev, res);
devm_platform_ioremap_resource()
Review
Quoted 7 lines
(quoted diff) +5
-0+ if (IS_ERR(drvdata->membase)) + return PTR_ERR(drvdata->membase); +} +#define PINCONF_SET_MASK (BIT(PINCONF_SET_SIZE) - 1) +#define PINCONF_TYPE_MASK (BIT(PINCONF_TYPE_SIZE) - 1)
GENMASK()
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 20 lines · Andriy Shevchenko
Quoted 11 lines · Linus Walleij
Hi Rahul,
thanks for your patches!
Quoted 4 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 dt bindings document & include file.Quoted 7 lines
Quoted 2 lines
Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.gitOK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Quoted 4 lines
Linus,
nevertheless I guess you may give your comments WRT device tree use (bindings, helpers, etc) along with some basics, (like devm_*() [ab]use I just noticed).
I plan to look at the patches per se but right now I don't have much time because soon there is merge window and kernel summit, the patches just need to age a little bit like a good wine ;)
Yours,
Linus Walleij
Quoted 22 lines · Linus Walleij
Quoted 20 lines · Andriy Shevchenko
Quoted 18 lines · Linus Walleij
Hi Rahul,
thanks for your patches!
Quoted 6 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 dt bindings document & include file.Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.gitOK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Quoted 8 lines
Quoted 4 lines
Linus,
nevertheless I guess you may give your comments WRT device tree use (bindings, helpers, etc) along with some basics, (like devm_*() [ab]use I just noticed).I plan to look at the patches per se but right now I don't have much time because soon there is merge window and kernel summit, the patches just need to age a little bit like a good wine ;)
I meant in general, I know about merge window, so, I guess this is not material for v5.4.
I reviewed it a bit, hope this helps.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 11 lines · Linus Walleij
Hi Rahul,
thanks for your patches!
Quoted 4 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 dt bindings document & include file.
Quoted 7 lines
Quoted 2 lines
Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.gitOK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Thanks Linus for looping me in.
Even if this is not directly based on the stuff we have under drivers/pinctrl/intel/*, I have a couple of comments. I don't have this patch series in my inbox so I'm commenting here.
Since the driver name is equilibrium I suggest you to name intel_pinctrl_driver and the like (probe, remove) to follow that convention to avoid confusing this with the Intel pinctrl drivers under drivers/pinctrl/intel/*.
Maybe use eqbr prefix so then intel_pinctrl_driver becomes eqbr_pinctrl_driver and so on. Also all the structures like intel_pinctrl_drv_data should be changed accordingly.
Ditto for:
MODULE_DESCRIPTION("Intel Pinctrl Driver for LGM SoC");
I think better would be:
MODULE_DESCRIPTION("Pinctrl Driver for LGM SoC (Equilibrium)");
Anyway you get the idea :)
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
In reply to Andy ShevchenkoHi Andy,
Thanks for your comments. I agree & will address all your review concerns in v2 except below mentioned points where i need more clarification.
Quoted 9 lines · Andy Shevchenko
drivers/pinctrl/pinctrl-equilibrium.c line 30
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+static const struct pin_config pin_cfg_type[] = { + {"intel,pullup" ,PINCONF_TYPE_PULL_UP }, + {"intel,pulldown" ,PINCONF_TYPE_PULL_DOWN }, + {"intel,drive-current" ,PINCONF_TYPE_DRIVE_CURRENT }, + {"intel,slew-rate" ,PINCONF_TYPE_SLEW_RATE }, + {"intel,open-drain" ,PINCONF_TYPE_OPEN_DRAIN }, + {"intel,output" ,PINCONF_TYPE_OUTPUT }, +};Doesn't DT provide a generic naming scheme for these?
For pinctrl multiplexing/configuration nodes, DT does provide generic names for some properties but it does not seem to mandate it. It states that the content of pin mux/conf nodes is defined entirely by the binding of the pin controller device. There are many other examples of pinctrl drivers which use different prop names than generic ones. For e.g. Samsung. Our understanding
is: if the node is generic i.e. handled by framework, then it should use generic name. But if the node is private to driver, then it is better to prefix it with driver name to avoid any conflicts.
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c line 128
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ virq =irq_find_mapping (desc->irq_domain, offset); +if (virq) +return virq; +else +return irq_create_mapping (desc->irq_domain, offset);Don't we have more clever helper for this? AFAIR something like this is done in IRQ framework when you get a mapping from certain domain.
I guess, you mean irq_domain_add_simple(). This function does optionally map the IRQs but only statically assigned IRQs. We need dynamic gpio_to_irq mappings which is why the gpio_chip->to_irq() is optionally provided so the drivers requiring dynamic IRQ mappings can override this function.
But i can definitely get rid of redundant irq_find_mapping() because irq_create_mapping() anyways invokes irq_find_mapping() first. I will remove irq_find_mapping() and just use irq_create_mapping() here.
Quoted 12 lines
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+static void eqbr_irq_handler (struct irq_desc *desc) +{ +struct intel_gpio_desc *gc; +struct irq_chip *ic; +u32 pins, offset; +unsigned int virq; + + gc =irq_desc_get_handler_data (desc); + ic =irq_desc_get_chip (desc); + +chained_irq_enter (ic, desc); + pins =readl (gc->membase +GPIO_IRNCR );
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 327
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ +for_each_set_bit (offset, (unsigned long *)&pins, gc->bank->nr_pins) { + virq =irq_linear_revmap (gc->irq_domain, offset); +if (!virq) +pr_err ("gc[%s]:pin:%d irq not registered!\n" , + gc->name, offset);
dev_err()? But Why is it needed? Shouldn't be registered as a spurious IRQ for later debugging?
IMHO, spurious IRQ can only be registered if none of the pins are valid. Please see below alternative way of handling it & let me know if this makes more sense.
@@ -313,6 +313,7 @@ static void eqbr_irq_handler(struct irq_desc *desc) struct irq_chip *ic;
u32 pins, offset;
unsigned int virq;
+ int handled = 0;
gc = irq_desc_get_handler_data(desc);
ic = irq_desc_get_chip(desc);
@@ -322,12 +323,16 @@ static void eqbr_irq_handler(struct irq_desc *desc)
for_each_set_bit(offset, (unsigned long *)&pins, gc->bank->nr_pins) { virq = irq_linear_revmap(gc->irq_domain, offset); - if (!virq)
- pr_err("gc[%s]:pin:%d irq not registered!\n", - gc->name, offset);
- else
+ if (virq) {
generic_handle_irq(virq);
+ handled++;
+ }
}
+
+ /* Spurious interrupt */
+ if (handled == 0)
+ handle_bad_irq(desc);
+
chained_irq_exit(ic, desc);
}
Quoted 12 lines
Quoted 12 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +12
-0+static int add_config (struct intel_pinctrl_drv_data *drvdata, +unsigned long **confs,unsigned int *nr_conf, +unsigned long pinconf) +{ +unsigned long *configs; +struct device *dev = drvdata->dev; +unsigned int num_conf = *nr_conf +1 ; + +if (!(*nr_conf)) { + configs =devm_kcalloc (dev,1 ,sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ;
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 467
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ }else { + configs =devm_kmemdup (dev, *confs, + num_conf *sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; +devm_kfree (dev, *confs);This a red flag for using devm_*().
Either a sign of bad design or misplacement of devm_*().
I can switch to non devm versions i.e. kmalloc/kfree for these buffer allocations. But this leaves me with a fundamental question. As i understand, devm*() variants are to allocate device specific resources so you don't have to worry about releasing those resources when device is unloaded. Does that mean that we should only use devm*() for resources which have a lifetime from driver load until the driver is unloaded ? For all other allocations which requires freeing up & reallocation during the operational state of driver, we should use non devm variants i.e. kmalloc/kfree ? If yes, then what's the purpose of devm_kfree() & other similar APIs ?
Quoted 15 lines
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ val = !!(readl (mem +REG_PUEN ) &BIT (offset)); +seq_printf (s,"PULL UP: %u\n" , val); + val = !!(readl (mem +REG_PDEN ) &BIT (offset));drivers/pinctrl/pinctrl-equilibrium.c line 932
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+seq_printf (s,"PULL DOWN: %u\n" , val); + val = !!(readl (mem +REG_OD ) &BIT (offset)); +seq_printf (s,"OPEN DRAIN: %u\n" , val); + val =get_drv_cur (mem, offset); +seq_printf (s,"DRIVE CURRENT: %u\n" , val); + val = !!(readl (mem +REG_SRC ) &BIT (offset)); +seq_printf (s,"SLEW RATE: %u\n" , val); + gpio =get_gpio_desc_via_bank (pctl, bank); + val =intel_eqbr_gpio_get_dir (&gpio->chip, offset); +seq_printf (s,"OUTPUT: %u\n" , !val);I think GPIO library does it for you.
Sorry, could not figure out what you mean. Do you mean no need to override pinconf_ops->pin_config_dbg_show() ?
Quoted 10 lines
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static int is_func_exist (struct intel_pmx_func *funcs,const char *name, +unsigned int nr_funcs,unsigned int *idx) +{ +int i; + +if (!funcs || !nr_funcs) +return 0 ; + +for (i =0 ; i < nr_funcs; i++) {
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 1010
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+if (strcmp (funcs[i].name, name) ==0 ) { + *idx = i; +return 1 ; + } + } + +return 0 ;NIH
match_string(). But I think pin control core does it for drivers.
match_string() searches a string within a array of strings. Here, we are searching an array of structures, each struct having a unique string, to match that unique string & return index of containing structure. Could not find any helper for this purpose. Checked pin control core code as well but could not find any routine to use for this purpose.
Regards,
Rahul
Hi Mika,
Quoted 35 lines · Mika Westerberg
Quoted 11 lines · Linus Walleij
Hi Rahul,
thanks for your patches!
Quoted 4 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 dt bindings document & include file.Quoted 6 lines
Quoted 2 lines
Patches are against Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.gitOK nice, I think you need to include Mika Westerberg on this review as well, because I think he likes to stay on top of all things intel in pin control. (Also included two other Intel folks in Finland who usually take an interest in these things.)
Thanks Linus for looping me in.
Even if this is not directly based on the stuff we have under
drivers/pinctrl/intel/*, I have a couple of comments. I don't have this patch series in my inbox so I'm commenting here.Since the driver name is equilibrium I suggest you to name
intel_pinctrl_driverand the like (probe, remove) to follow that convention to avoid confusing this with the Intel pinctrl drivers underdrivers/pinctrl/intel/*.Maybe use eqbr prefix so then
intel_pinctrl_driverbecomeseqbr_pinctrl_driverand so on. Also all the structures likeintel_pinctrl_drv_datashould be changed accordingly.Ditto for:
Quoted 7 lines
MODULE_DESCRIPTION("Intel Pinctrl Driver for LGM SoC");I think better would be:
MODULE_DESCRIPTION("Pinctrl Driver for LGM SoC (Equilibrium)");Anyway you get the idea :)
Yes, i understand your point. Will update in v2. Thanks.
Regards,
Rahul
Rob Herring
maintainerRe: [PATCH v1 2/2] dt-bindings: pinctrl: intel: Add for new SoC
In reply to Rahul TanwarQuoted 93 lines · Rahul Tanwar
Add dt bindings document & include file for pinmux & GPIO controller driver of Intel Lightning Mountain SoC.
Signed-off-by Rahul Tanwar <[email protected]>--- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 131 +++++++++++++++++++++ include/dt-bindings/pinctrl/intel,equilibrium.h | 23 ++++ 2 files changed, 154 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml create mode 100644 include/dt-bindings/pinctrl/intel,equilibrium.hDocumentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +76
-0@@ -0,0 +1,131 @@ +# 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 defined properties. + + properties: + intel,function: + $ref: /schemas/types.yaml#/definitions/string + description: + A string containing the name of the function to mux to the group. + + intel,groups: + $ref: /schemas/types.yaml#/definitions/string-array + description: + An array of strings identifying the list of groups. + + intel,pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + List of pins to select with this function. + + intel,mux: + description: The applicable mux group. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32" + - enum: + # Refer include/dt-bindings/pinctrl/intel,equilibrium.h + - PINMUX_0 # 0 PINMUX_GPIO + - PINMUX_1 # 1 + - PINMUX_2 # 2 + - PINMUX_3 # 3 + - PINMUX_4 # 4 + + intel,pullup: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-up configuration. + + intel,pulldown: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies pull-down configuration. + + intel,drive-current: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables driver-current. + + intel,slew-rate:
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 86
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + description: Enables slew-rate. + + intel,open-drain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies open-drain configuration. + + intel,output: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specifies if the pin is to be configured as output.
We have a whole slew of standard properties for pinctrl and you aren't using any of them.
Rob
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ + + required: + - intel,function + - intel,groups + +required: + - compatible + - reg +
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 106
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 node + - |
These are not really separate examples.
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +5
-0+ asc0: serial@e0a00000 { + compatible = "intel,lgm-asc"; + reg = <0xe0a00000 0x1000>; + interrupt-parent = <&ioapic1>; + interrupts = <128 1>;
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml line 121
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ interrupt-names = "asc_irq"; + clocks = <&cgu0 31>, <&cgu0 98>; + clock-names = "freq", "asc"; + pinctrl-names = "default"; + pinctrl-0 = <&uart0>; + }; + + # Client device subnode + - | + uart0:uart0 {
This should be a child of pinctrl node.
Quoted 16 lines
Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file +10
-0+ intel,pins = <64>, /* UART_RX0 */ + <65>; /* UART_TX0 */ + intel,function = "CONSOLE_UART0"; + intel,mux = <1>, + <1>; + intel,groups = "CONSOLE_UART0"; + }; + + +...include/dt-bindings/pinctrl/intel,equilibrium.h new file +0
-0
include/dt-bindings/pinctrl/intel,equilibrium.h line 9
Quoted 9 lines
include/dt-bindings/pinctrl/intel,equilibrium.h new file +9
-0@@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ +#define __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ + +#define PINCTRL_DRCC_2_MA 0 +#define PINCTRL_DRCC_4_MA 1 +#define PINCTRL_DRCC_8_MA 2 +#define PINCTRL_DRCC_12_MA 3
Use the property that defines drive strength in terms of microamps and convert to register values in the driver.
include/dt-bindings/pinctrl/intel,equilibrium.h line 15
Quoted 6 lines
include/dt-bindings/pinctrl/intel,equilibrium.h new file +6
-0+ +#define PINMUX_0 0 +#define PINMUX_1 1 +#define PINMUX_2 2 +#define PINMUX_3 3 +#define PINMUX_4 4
Not useful defines. Just use the numbers directly.
include/dt-bindings/pinctrl/intel,equilibrium.h line 21
Quoted 6 lines
include/dt-bindings/pinctrl/intel,equilibrium.h new file +6
-0+#define PINMUX_GPIO PINMUX_0 + +#define PINCTRL_GROUP "intel,groups" +#define PINCTRL_FUNCTION "intel,function" +#define PINCTRL_PINS "intel,pins" +#define PINCTRL_MUX "intel,mux"
We don't create defines for property names (really for any strings).
Quoted 5 lines
include/dt-bindings/pinctrl/intel,equilibrium.h new file +2
-0+ +#endif /* __DT_BINDINGS_PINCTRL_INTEL_EQUILIBRIUM_H_ */ Signature · 1 line
2.11.0
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
In reply to Rahul TanwarHi Rahul,
this is an interesting patch!
Let's dive in and fix the things not already pointed out in review.
It will need some work but I am sure you can get there.
Quoted 6 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. Add GPIO & pin control framework based driver for this IP.
Signed-off-by Rahul Tanwar <[email protected]>
(...)
drivers/pinctrl/Kconfig config PINCTRL_TB10X · line 428
Quoted 6 lines
drivers/pinctrl/Kconfig +6
-0+config PINCTRL_EQUILIBRIUM + tristate "Generic pinctrl and GPIO driver for Intel Lightning Mountain SoC" + select PINMUX + select PINCONF + select GPIOLIB + select GPIOLIB_IRQCHIP
Nice use of the GPIOLIB_IRQCHIP.
Are you sure you can't just use GPIO_GENERIC as well? This is almost always usable when you have a register with n consecutive bits representing GPIO lines.
Look how we use bgpio_init() in e.g. drivers/gpio/gpio-ftgpio010.c to cut down on boilerplate code, and we also get a spinlock protection and .get/.set_multiple() implementation for free.
drivers/pinctrl/pinctrl-equilibrium.c line 13
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+#include <linux/pinctrl/consumer.h> +#include <linux/pinctrl/machine.h>
Why do you need these two includes?
drivers/pinctrl/pinctrl-equilibrium.c line 30
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+static const struct pin_config pin_cfg_type[] = { + {"intel,pullup" ,PINCONF_TYPE_PULL_UP }, + {"intel,pulldown" ,PINCONF_TYPE_PULL_DOWN }, + {"intel,drive-current" ,PINCONF_TYPE_DRIVE_CURRENT }, + {"intel,slew-rate" ,PINCONF_TYPE_SLEW_RATE }, + {"intel,open-drain" ,PINCONF_TYPE_OPEN_DRAIN }, + {"intel,output" ,PINCONF_TYPE_OUTPUT }, +};
So... if we are adding a new driver with a new DT binding, why use the made-up "intel," prefixed flags when we have the standard DT flags from Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt already handled by the core?
drivers/pinctrl/pinctrl-equilibrium.c line 42
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+static inline void eqbr_set_val (void __iomem *addr,u32 offset, +u32 mask,u32 set,raw_spinlock_t *lock) +{ +u32 val; +unsigned long flags; + +raw_spin_lock_irqsave (lock, flags); + val =readl (addr) & ~(mask << offset); +writel (val | ((set & mask) << offset), addr); +raw_spin_unlock_irqrestore (lock, flags); +}
Mask-and-set is usually achieved with regmap-mmio if you dont use GPIO_GENERIC, but I think you can just use GPIO_GENERIC. All of these:
drivers/pinctrl/pinctrl-equilibrium.c line 100
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static int intel_eqbr_gpio_get_dir(struct gpio_chip *gc, unsigned int offset) +static int intel_eqbr_gpio_dir_input(struct gpio_chip *gc, unsigned int offset) +static int intel_eqbr_gpio_dir_output(struct gpio_chip *gc, unsigned int offset, +static void intel_eqbr_gpio_set(struct gpio_chip *gc, + unsigned int offset, int dir) +static int intel_eqbr_gpio_get(struct gpio_chip *gc, unsigned int offset)
Look very bit-per-bit mapped.
Quoted 15 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +15
-0+static int eqbr_irq_map (struct irq_domain *d, +unsigned int virq,irq_hw_number_t hw) +{ +struct intel_gpio_desc *desc = d->host_data; + +irq_set_chip_data (virq, desc); +irq_set_chip_and_handler (virq, desc->ic, handle_level_irq); +return 0 ; +} + +static const struct irq_domain_ops gc_irqdomain_ops = { + .map = eqbr_irq_map, + .xlate = irq_domain_xlate_twocell, +}; +
(...)
drivers/pinctrl/pinctrl-equilibrium.c line 118
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+static int intel_eqbr_gpio_to_irq (struct gpio_chip *gc,unsigned int offset) +{ +struct intel_gpio_desc *desc =gpiochip_get_data (gc);
Since struct gpio_desc means a per-line state container and struct intel_gpio_desc refers to the whole chip, I think this struct should be renamed something like struct eqbr_gpio.
Quoted 14 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +14
-0+unsigned int virq; + +if (!desc->irq_domain) +return -ENODEV ; + + virq =irq_find_mapping (desc->irq_domain, offset); +if (virq) +return virq; +else +return irq_create_mapping (desc->irq_domain, offset); +} + +static int gpiochip_setup (struct device *dev,struct intel_gpio_desc *desc) +{
(...)
Quoted 1 line
+
gc->to_irq=intel_eqbr_gpio_to_irq;
You don't need any of this funku stuff. The GPIOLIB_IRQCHIP provides default implementations to do all this for you. Just look in drivers/gpio/gpio-ftgpio010.c and follow the pattern (look how I set up struct gpio_irq_chip using *girq etc). If you need anything custom we need some special motivation here.
drivers/pinctrl/pinctrl-equilibrium.c line 150
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+ gc->of_node = desc->node; + gc->parent = dev;
I would allocate a dynamic irqchip as part of the struct intel_gpio_desc and populate it dynamically with function pointers.
struct gpio_irq_chip *girq;
girq = &gc->irq;girq->chip = ...
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ ret =devm_gpiochip_add_data (dev, gc, desc); +if (ret) +dev_err (dev,"failed to register gpiochip: %s, err: %d\n" , + gc->label, ret); + +return ret; +}
(...)
drivers/pinctrl/pinctrl-equilibrium.c line 278
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+static int eqbr_gpio_irq_req_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset; +int ret; + + offset =irqd_to_hwirq (d); + +/* gpio must be set as input */ +intel_eqbr_gpio_dir_input (&desc->chip, offset);
Please move this to the .irq_enable() callback instead.
drivers/pinctrl/pinctrl-equilibrium.c line 285
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ ret =gpiochip_lock_as_irq (&desc->chip, offset); +if (ret) { +pr_err ("%s: Failed to lock gpio %u as irq!\n" , + desc->name, offset); +return ret; + } +eqbr_gpio_enable_irq (d);
Why are you calling this here? It is premature I think, isn't the call in .unmask() soon enough? The latter is what we rely upon.
drivers/pinctrl/pinctrl-equilibrium.c line 295
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static void eqbr_gpio_irq_rel_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset =irqd_to_hwirq (d); + +eqbr_gpio_disable_irq (d);
No need to do this, .irq_mask() has already done it at this point.
Review
Quoted 2 lines
(quoted diff) +2
-0+ gpiochip_unlock_as_irq(&desc->chip, offset); +}
I think the core default implementations should be fine for both reqres and relres.
drivers/pinctrl/pinctrl-equilibrium.c line 308
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+static struct irq_chip eqbr_irq_chip = { + .name ="gpio_irq" , + .irq_mask = eqbr_gpio_disable_irq, + .irq_unmask = eqbr_gpio_enable_irq, + .irq_ack = eqbr_gpio_ack_irq, + .irq_mask_ack = eqbr_gpio_mask_ack_irq, + .irq_set_type = eqbr_gpio_set_irq_type, + .irq_request_resources = eqbr_gpio_irq_req_res, + .irq_release_resources = eqbr_gpio_irq_rel_res, +};
So please add a struct irq_chip to the state container (struct intel_gpio_desc) and assign these functions directly in probe (again look at gpio-ftgpio010.c).
drivers/pinctrl/pinctrl-equilibrium.c line 318
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static void eqbr_irq_handler (struct irq_desc *desc) +{ +struct intel_gpio_desc *gc; +struct irq_chip *ic; +u32 pins, offset; +unsigned int virq; + + gc =irq_desc_get_handler_data (desc); + ic =irq_desc_get_chip (desc);
When using the GPIOLIB_IRQCHIP follow the pattern from other drivers and assume the handler data is the struct gpio_chip instead.
struct gpio_chip *gc = irq_desc_get_handler_data(desc); struct intel_gpio_desc *i = gpiochip_get_data(gc); (...)
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static int irqchip_setup (struct device *dev,struct intel_gpio_desc *desc) +{ +struct device_node *np = desc->node; + +if (!of_property_read_bool (np,"interrupt-controller" )) { +dev_info (dev,"gc %s: doesn't act as interrupt controller!\n" , + desc->name); +return 0 ; + }
OK just skip assigning *girq with the chip etc for this case.
drivers/pinctrl/pinctrl-equilibrium.c line 352
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ desc->irq_domain =irq_domain_add_linear (desc->node, + desc->bank->nr_pins, + &gc_irqdomain_ops, desc); +if (!desc->irq_domain) { +dev_err (dev,"%s: failed to create gpio irq domain!\n" , + desc->name); +return -ENODEV ; + } +irq_set_chained_handler_and_data (desc->virq, eqbr_irq_handler, desc);
Let GPIOLIB_IRQCHIP handle these things for you instead of making your own domain etc.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+static int gpiolib_reg (struct intel_pinctrl_drv_data *drvdata) +{ +struct device_node *np; +struct intel_gpio_desc *desc; +struct device *dev; +int i, ret; +char name[32 ]; +struct resource res; + + dev = drvdata->dev;
drivers/pinctrl/pinctrl-equilibrium.c line 377
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+for (i =0 ; i < drvdata->nr_gpio_descs; i++) { + desc = drvdata->gpio_desc + i; + np = desc->node; +sprintf (name,"gpiochip%d" , i); + desc->name =devm_kmemdup (dev, name, +strlen (name) +1 ,GFP_KERNEL ); +if (!desc->name) +return -ENOMEM ; +if (of_address_to_resource (np,0 , &res)) { +dev_err (dev,"Failed to get GPIO register addrss\n" );
Speling
Quoted 14 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +14
-0+return -ENXIO ; + } + desc->membase =devm_ioremap_resource (dev, &res); +if (IS_ERR (desc->membase)) { +dev_err (dev,"ioremap fail\n" ); +return PTR_ERR (desc->membase); + } +dev_dbg (dev,"gpio resource: %pr\n" , &res); +dev_dbg (dev,"gpiochip membase: %px\n" , desc->membase); + + desc->virq =irq_of_parse_and_map (np,0 ); +if (!desc->virq) { +dev_err (dev,"%s: failed to parse and map irq\n" , + name);
drivers/pinctrl/pinctrl-equilibrium.c line 401
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+return -ENXIO ; + } +raw_spin_lock_init (&desc->lock); + + ret =gpiochip_setup (dev, desc); +if (ret) +return ret; + ret =irqchip_setup (dev, desc); +if (ret) +return ret;
Bake these two into a single function setting up gpio_chip and irq_chip. With proper use of GPIOLIB_IRQCHIP it will be so much simpler anyway.
Quoted 4 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +4
-0+static int parse_mux_info (struct device_node *np) +{ +int ret; +const char *str;
drivers/pinctrl/pinctrl-equilibrium.c line 448
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ + ret =of_property_read_string (np,"intel,function" , &str); +if (ret) +return -ENODEV ; + ret =of_property_read_string (np,"intel,groups" , &str); +if (ret) +return -ENODEV ; + +return ret; +}
Again these are intel,foo-specific properties for things we already have standard bindings for, so use those.
Quoted 98 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +98
-0+static int add_config (struct intel_pinctrl_drv_data *drvdata, +unsigned long **confs,unsigned int *nr_conf, +unsigned long pinconf) +{ +unsigned long *configs; +struct device *dev = drvdata->dev; +unsigned int num_conf = *nr_conf +1 ; + +if (!(*nr_conf)) { + configs =devm_kcalloc (dev,1 ,sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; + }else { + configs =devm_kmemdup (dev, *confs, + num_conf *sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; +devm_kfree (dev, *confs); + } + + configs[num_conf -1 ] = pinconf; + *confs = configs; + *nr_conf = num_conf; + +return 0 ; +} + +static void eqbr_add_map_mux (struct device_node *np,struct pinctrl_map **map, +int *index) +{ +int idx = *index; +const char *function, *group; + +of_property_read_string (np,"intel,function" , &function); +of_property_read_string (np,"intel,groups" , &group); + + (*map)[idx].type =PIN_MAP_TYPE_MUX_GROUP ; + (*map)[idx].data.mux.group = group; + (*map)[idx].data.mux.function = function; + *index = idx +1 ; +} + +static void eqbr_add_map_configs (struct device_node *np, +struct pinctrl_map **map,int *index, +unsigned long *configs,unsigned int nr_config) +{ +int idx = *index; +const char *group; + +of_property_read_string (np,"intel,groups" , &group); + (*map)[idx].type =PIN_MAP_TYPE_CONFIGS_GROUP ; + (*map)[idx].data.configs.group_or_pin = group; + (*map)[idx].data.configs.configs = configs; + (*map)[idx].data.configs.num_configs = nr_config; + *index = idx +1 ; +} + +static int eqbr_dt_node_to_map (struct pinctrl_dev *pctldev, +struct device_node *np, +struct pinctrl_map **map,unsigned int *num_maps) +{ +struct intel_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +unsigned int map_cnt, nr_config; +unsigned long pin_conf, *configs =NULL ; +int i, ret; +unsigned int val; +bool func = false; + + *map =NULL ; + *num_maps = map_cnt = nr_config =0 ; + + ret =parse_mux_info (np); +if (!ret) { + map_cnt++; + func = true; + } + +for (i =0 ; i <ARRAY_SIZE (pin_cfg_type); i++) { + ret =of_property_read_u32 (np, pin_cfg_type[i].property, &val); +if (!ret) { + pin_conf =PINCONF_PACK (pin_cfg_type[i].type, val); + ret =add_config (pctl, &configs, &nr_config, pin_conf); +if (ret) +return ret; + } + } + +/** +* Create pinctrl_map for each groups, per group per entry. +* Create pinctrl_map for pin config, per group per entry. +*/ +if (nr_config) + map_cnt++; + + *map =devm_kcalloc (pctl->dev, map_cnt,sizeof (**map),GFP_KERNEL ); +if (!*map) +return -ENOMEM ; +
drivers/pinctrl/pinctrl-equilibrium.c line 557
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+ i =0 ; +if (func) +eqbr_add_map_mux (np, map, &i); +if (nr_config) +eqbr_add_map_configs (np, map, &i, configs, nr_config); + + *num_maps = map_cnt; + +return 0 ; +}
With the library code for the standard bindings select GENERIC_PINMUX_FUNCTIONS and select GENERIC_PINCONF most of the above goes away as well.
drivers/pinctrl/pinctrl-equilibrium.c line 611
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +11
-0+static void eqbr_dt_free_map (struct pinctrl_dev *pctldev, +struct pinctrl_map *map,unsigned int num_maps) +{ +struct intel_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +int i; + +for (i =0 ; i < num_maps; i++) +if (map[i].type ==PIN_MAP_TYPE_CONFIGS_GROUP ) +devm_kfree (pctl->dev, map[i].data.configs.configs); +devm_kfree (pctl->dev, map); +}
In this case I think you can use the library function pinctrl_utils_free_map() just as is.
Now I ran out of time, but the generic advice is to use library code and standard bindings as much as you can and all will shrink down considerably. Start with the above pointers and I will look closer after that!
Yours,
Linus Walleij
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
Hi Linus,
Thanks for taking time out to review.
Quoted 8 lines · Linus Walleij
drivers/pinctrl/Kconfig config PINCTRL_TB10X · line 428
Quoted 6 lines
drivers/pinctrl/Kconfig +6
-0+config PINCTRL_EQUILIBRIUM + tristate "Generic pinctrl and GPIO driver for Intel Lightning Mountain SoC" + select PINMUX + select PINCONF + select GPIOLIB + select GPIOLIB_IRQCHIPNice use of the
GPIOLIB_IRQCHIP.
Quoted 7 lines
Are you sure you can't just use
GPIO_GENERICas well? This is almost always usable when you have a register with n consecutive bits representing GPIO lines.Look how we use
bgpio_init()in e.g.drivers/gpio/gpio-ftgpio010.cto cut down on boilerplate code, and we also get a spinlock protection and .get/.set_multiple() implementation for free.
I went through gpio-mmio.c & gpio-ftgpio010.c code. My understanding is that GPIO_GENERIC can support a max of 64 consecutive bits representing GPIO lines. We have more than 100 GPIO's and they are spread out across 4 different banks with non consecutive registers i.e. DATA_IN_0~31@offset0x0, DATA_IN_32~63@offset0x100 and so on. In other words, i think we can not support memory mapped GPIO controller.
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c line 13
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+#include <linux/pinctrl/consumer.h> +#include <linux/pinctrl/machine.h> Why do you need these two includes?
Yes, these are redundant. I will remove them. Thanks.
Quoted 5 lines
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+static const struct pin_config pin_cfg_type[] = { + {"intel,pullup" ,PINCONF_TYPE_PULL_UP }, + {"intel,pulldown" ,PINCONF_TYPE_PULL_DOWN }, + {"intel,drive-current" ,PINCONF_TYPE_DRIVE_CURRENT }, + {"intel,slew-rate" ,PINCONF_TYPE_SLEW_RATE },
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 30
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+ {"intel,open-drain", PINCONF_TYPE_OPEN_DRAIN}, + {"intel,output", PINCONF_TYPE_OUTPUT}, +};So... if we are adding a new driver with a new DT binding, why use the made-up "intel," prefixed flags when we have the standard DT flags from
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txtalready handled by the core?
Yes, Andy & Rob have also raised same concerns. I will switch to using standard DT properties & generic pinconf and remove redundant code. Thanks.
Quoted 6 lines
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static inline void eqbr_set_val (void __iomem *addr,u32 offset, +u32 mask,u32 set,raw_spinlock_t *lock) +{ +u32 val; +unsigned long flags; +
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 42
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+raw_spin_lock_irqsave (lock, flags); + val =readl (addr) & ~(mask << offset); +writel (val | ((set & mask) << offset), addr); +raw_spin_unlock_irqrestore (lock, flags); +}Mask-and-set is usually achieved with regmap-mmio if you dont use
GPIO_GENERIC, but I think you can just useGPIO_GENERIC. All of these:
As mentioned above, we cannot use GPIO_GENERIC. And there was no specific reason/motivation for us to use regmap-mmio because most of the driver's that we referenced were using readl()/write(). Do you recommend us to remove readl()/writel() and switch to regmap-mmio based design ?
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 100
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static int intel_eqbr_gpio_get_dir(struct gpio_chip *gc, unsigned int offset) +static int intel_eqbr_gpio_dir_input(struct gpio_chip *gc, unsigned int offset) +static int intel_eqbr_gpio_dir_output(struct gpio_chip *gc, unsigned int offset, +static void intel_eqbr_gpio_set(struct gpio_chip *gc, + unsigned int offset, int dir) +static int intel_eqbr_gpio_get(struct gpio_chip *gc, unsigned int offset)Look very bit-per-bit mapped.
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c line 118
Quoted 3 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +3
-0+static int intel_eqbr_gpio_to_irq (struct gpio_chip *gc,unsigned int offset) +{ +struct intel_gpio_desc *desc =gpiochip_get_data (gc);Since struct
gpio_descmeans a per-line state container and structintel_gpio_descrefers to the whole chip, I think this struct should be renamed something like structeqbr_gpio.
Just to clarify, we have 4 different GPIO banks and each GPIO bank is implemented as a separate gpio_chip. So we have 4 instances of gpio_desc each one having its own gpio_chip. What i mean to say is that gpio_desc is actually not a per-line state container, its a per gpio_chip container.
Quoted 15 lines
Quoted 14 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +14
-0+unsigned int virq; + +if (!desc->irq_domain) +return -ENODEV ; + + virq =irq_find_mapping (desc->irq_domain, offset); +if (virq) +return virq; +else +return irq_create_mapping (desc->irq_domain, offset); +} + +static int gpiochip_setup (struct device *dev,struct intel_gpio_desc *desc) +{(...)
Quoted 7 lines
Quoted 1 line
+
gc->to_irq=intel_eqbr_gpio_to_irq;You don't need any of this funku stuff. The
GPIOLIB_IRQCHIPprovides default implementations to do all this for you. Just look indrivers/gpio/gpio-ftgpio010.cand follow the pattern (look how I set up structgpio_irq_chipusing *girq etc). If you need anything custom we need some special motivation here.
Yes, i checked gpio-ftgpio010.c and agree that this is already handled under GPIOLIB_IRQCHIP. I will make these changes in V2. Thanks.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 150
Quoted 2 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +2
-0+ gc->of_node = desc->node; + gc->parent = dev;I would allocate a dynamic irqchip as part of the struct
intel_gpio_descand populate it dynamically with function pointers.struct
gpio_irq_chip*girq;girq =
&gc->irq;girq->chip= ...
Agree, i will follow this approach as part of switching to reusing GPIOLIB_IRQCHIP default implementations. Thanks.
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c line 278
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +10
-0+static int eqbr_gpio_irq_req_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset; +int ret; + + offset =irqd_to_hwirq (d); + +/* gpio must be set as input */ +intel_eqbr_gpio_dir_input (&desc->chip, offset);Please move this to the .irq_enable() callback instead.
Well noted.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 285
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+ ret =gpiochip_lock_as_irq (&desc->chip, offset); +if (ret) { +pr_err ("%s: Failed to lock gpio %u as irq!\n" , + desc->name, offset); +return ret; + } +eqbr_gpio_enable_irq (d);Why are you calling this here? It is premature I think, isn't the call in .unmask() soon enough? The latter is what we rely upon.
Agree, have already changed it.
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 295
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+static void eqbr_gpio_irq_rel_res (struct irq_data *d) +{ +struct intel_gpio_desc *desc =irq_data_get_irq_chip_data (d); +unsigned int offset =irqd_to_hwirq (d); + +eqbr_gpio_disable_irq (d);No need to do this, .irq_mask() has already done it at this point.
Agree, have already changed it.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ gpiochip_unlock_as_irq(&desc->chip, offset); +}I think the core default implementations should be fine for both reqres and relres.
I also observed this when referring to gpio-ftgpio010.c. Will change to default implementations.
Quoted 5 lines
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+static struct irq_chip eqbr_irq_chip = { + .name ="gpio_irq" , + .irq_mask = eqbr_gpio_disable_irq, + .irq_unmask = eqbr_gpio_enable_irq, + .irq_ack = eqbr_gpio_ack_irq,
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 308
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ .irq_mask_ack = eqbr_gpio_mask_ack_irq, + .irq_set_type = eqbr_gpio_set_irq_type, + .irq_request_resources = eqbr_gpio_irq_req_res, + .irq_release_resources = eqbr_gpio_irq_rel_res, +};So please add a struct
irq_chipto the state container (structintel_gpio_desc) and assign these functions directly in probe (again look atgpio-ftgpio010.c).
Yup, agree. Will change. Thanks.
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c line 318
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static void eqbr_irq_handler (struct irq_desc *desc) +{ +struct intel_gpio_desc *gc; +struct irq_chip *ic; +u32 pins, offset; +unsigned int virq; + + gc =irq_desc_get_handler_data (desc); + ic =irq_desc_get_chip (desc);When using the
GPIOLIB_IRQCHIPfollow the pattern from other drivers and assume the handler data is the structgpio_chipinstead.
Quoted 3 lines
struct
gpio_chip*gc =irq_desc_get_handler_data(desc); structintel_gpio_desc*i =gpiochip_get_data(gc); (...)
Well noted.
Quoted 10 lines
drivers/pinctrl/pinctrl-equilibrium.c line 342
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+static int irqchip_setup (struct device *dev,struct intel_gpio_desc *desc) +{ +struct device_node *np = desc->node; + +if (!of_property_read_bool (np,"interrupt-controller" )) { +dev_info (dev,"gc %s: doesn't act as interrupt controller!\n" , + desc->name); +return 0 ; + }OK just skip assigning *girq with the chip etc for this case.
I understand. BTW, this call is in the probe path. I am planning to do girq setup on the lines of gpio-ftgpio010.c in this function instead of probe. So yes, i can skip assigning chip to girq for this case in this function.
Quoted 11 lines
drivers/pinctrl/pinctrl-equilibrium.c line 352
Quoted 9 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +9
-0+ desc->irq_domain =irq_domain_add_linear (desc->node, + desc->bank->nr_pins, + &gc_irqdomain_ops, desc); +if (!desc->irq_domain) { +dev_err (dev,"%s: failed to create gpio irq domain!\n" , + desc->name); +return -ENODEV ; + } +irq_set_chained_handler_and_data (desc->virq, eqbr_irq_handler, desc);Let
GPIOLIB_IRQCHIPhandle these things for you instead of making your own domain etc.
Yes, got it now. Thanks.
Quoted 13 lines
Quoted 13 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +13
-0+static int gpiolib_reg (struct intel_pinctrl_drv_data *drvdata) +{ +struct device_node *np; +struct intel_gpio_desc *desc; +struct device *dev; +int i, ret; +char name[32 ]; +struct resource res; + + dev = drvdata->dev; +for (i =0 ; i < drvdata->nr_gpio_descs; i++) { + desc = drvdata->gpio_desc + i; + np = desc->node;
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 377
Quoted 7 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +7
-0+sprintf (name,"gpiochip%d" , i); + desc->name =devm_kmemdup (dev, name, +strlen (name) +1 ,GFP_KERNEL ); +if (!desc->name) +return -ENOMEM ; +if (of_address_to_resource (np,0 , &res)) { +dev_err (dev,"Failed to get GPIO register addrss\n" );Speling
Well noted.
Quoted 19 lines
Quoted 19 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +19
-0+return -ENXIO ; + } + desc->membase =devm_ioremap_resource (dev, &res); +if (IS_ERR (desc->membase)) { +dev_err (dev,"ioremap fail\n" ); +return PTR_ERR (desc->membase); + } +dev_dbg (dev,"gpio resource: %pr\n" , &res); +dev_dbg (dev,"gpiochip membase: %px\n" , desc->membase); + + desc->virq =irq_of_parse_and_map (np,0 ); +if (!desc->virq) { +dev_err (dev,"%s: failed to parse and map irq\n" , + name); +return -ENXIO ; + } +raw_spin_lock_init (&desc->lock); + + ret =gpiochip_setup (dev, desc);
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 401
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+if (ret) +return ret; + ret =irqchip_setup (dev, desc); +if (ret) +return ret;Bake these two into a single function setting up
gpio_chipandirq_chip. With proper use ofGPIOLIB_IRQCHIPit will be so much simpler anyway.
Agree. Will change accordingly. Thanks.
Quoted 8 lines
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +8
-0+static int parse_mux_info (struct device_node *np) +{ +int ret; +const char *str; + + ret =of_property_read_string (np,"intel,function" , &str); +if (ret) +return -ENODEV ;
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 448
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ ret =of_property_read_string (np,"intel,groups" , &str); +if (ret) +return -ENODEV ; + +return ret; +}Again these are intel,foo-specific properties for things we already have standard bindings for, so use those.
Yes, this will most likely go away because pinctrl_ops->dt_node_to_map() uses this function to count group map entries. And based on your comments, i will switch to using pinconf generic with standard properties which already handles dt_node_to_map() & dt_free_map().
Quoted 103 lines
Quoted 103 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +103
-0+static int add_config (struct intel_pinctrl_drv_data *drvdata, +unsigned long **confs,unsigned int *nr_conf, +unsigned long pinconf) +{ +unsigned long *configs; +struct device *dev = drvdata->dev; +unsigned int num_conf = *nr_conf +1 ; + +if (!(*nr_conf)) { + configs =devm_kcalloc (dev,1 ,sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; + }else { + configs =devm_kmemdup (dev, *confs, + num_conf *sizeof (pinconf),GFP_KERNEL ); +if (!configs) +return -ENOMEM ; +devm_kfree (dev, *confs); + } + + configs[num_conf -1 ] = pinconf; + *confs = configs; + *nr_conf = num_conf; + +return 0 ; +} + +static void eqbr_add_map_mux (struct device_node *np,struct pinctrl_map **map, +int *index) +{ +int idx = *index; +const char *function, *group; + +of_property_read_string (np,"intel,function" , &function); +of_property_read_string (np,"intel,groups" , &group); + + (*map)[idx].type =PIN_MAP_TYPE_MUX_GROUP ; + (*map)[idx].data.mux.group = group; + (*map)[idx].data.mux.function = function; + *index = idx +1 ; +} + +static void eqbr_add_map_configs (struct device_node *np, +struct pinctrl_map **map,int *index, +unsigned long *configs,unsigned int nr_config) +{ +int idx = *index; +const char *group; + +of_property_read_string (np,"intel,groups" , &group); + (*map)[idx].type =PIN_MAP_TYPE_CONFIGS_GROUP ; + (*map)[idx].data.configs.group_or_pin = group; + (*map)[idx].data.configs.configs = configs; + (*map)[idx].data.configs.num_configs = nr_config; + *index = idx +1 ; +} + +static int eqbr_dt_node_to_map (struct pinctrl_dev *pctldev, +struct device_node *np, +struct pinctrl_map **map,unsigned int *num_maps) +{ +struct intel_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +unsigned int map_cnt, nr_config; +unsigned long pin_conf, *configs =NULL ; +int i, ret; +unsigned int val; +bool func = false; + + *map =NULL ; + *num_maps = map_cnt = nr_config =0 ; + + ret =parse_mux_info (np); +if (!ret) { + map_cnt++; + func = true; + } + +for (i =0 ; i <ARRAY_SIZE (pin_cfg_type); i++) { + ret =of_property_read_u32 (np, pin_cfg_type[i].property, &val); +if (!ret) { + pin_conf =PINCONF_PACK (pin_cfg_type[i].type, val); + ret =add_config (pctl, &configs, &nr_config, pin_conf); +if (ret) +return ret; + } + } + +/** +* Create pinctrl_map for each groups, per group per entry. +* Create pinctrl_map for pin config, per group per entry. +*/ +if (nr_config) + map_cnt++; + + *map =devm_kcalloc (pctl->dev, map_cnt,sizeof (**map),GFP_KERNEL ); +if (!*map) +return -ENOMEM ; + + i =0 ; +if (func) +eqbr_add_map_mux (np, map, &i); +if (nr_config) +eqbr_add_map_configs (np, map, &i, configs, nr_config);
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 557
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+ + *num_maps = map_cnt; + +return 0 ; +}With the library code for the standard bindings select
GENERIC_PINMUX_FUNCTIONSand selectGENERIC_PINCONFmost of the above goes away as well.
Agree, clear about it now. All this goes away with GENERIC_PINCONF. Not yet sure about GENERIC_PINMUX_FUNCTIONS. Need to test if generic pinmux functions are ok to use for us. Seems not many driveruse generic pinmux presently.
Quoted 5 lines
Quoted 5 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +5
-0+static void eqbr_dt_free_map (struct pinctrl_dev *pctldev, +struct pinctrl_map *map,unsigned int num_maps) +{ +struct intel_pinctrl_drv_data *pctl =pinctrl_dev_get_drvdata (pctldev); +int i;
Quoted 8 lines
drivers/pinctrl/pinctrl-equilibrium.c line 611
Quoted 6 lines
drivers/pinctrl/pinctrl-equilibrium.c new file +6
-0+ +for (i =0 ; i < num_maps; i++) +if (map[i].type ==PIN_MAP_TYPE_CONFIGS_GROUP ) +devm_kfree (pctl->dev, map[i].data.configs.configs); +devm_kfree (pctl->dev, map); +}In this case I think you can use the library function
pinctrl_utils_free_map()just as is.
Yup, thanks.
Quoted 7 lines
Now I ran out of time, but the generic advice is to use library code and standard bindings as much as you can and all will shrink down considerably. Start with the above pointers and I will look closer after that!
Yours,
Linus Walleij
Regards,
Rahul
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
Quoted 3 lines · Rahul
My understanding is
thatGPIO_GENERICcan support a max of 64 consecutive bits representing GPIO lines.
Correct, it also demand that all GPIOs are accessed in a single register of 8, 16, 32 or 64 bits.
Quoted 4 lines
We have more than 100 GPIO's and they are spread out across
4 different banks with non consecutive registers i.e. DATA_IN_0~31@offset0x0, DATA_IN_32~63@offset0x100 and so on. In other words, i think we can not support memory mapped GPIO controller.
But why can't you just create one device and thus one gpio_chip per bank?
This is what most drivers do, it also often makes things easier.
The main reason for not doing this is usually something like that some registers are shared inbetween the banks, or their registers are mingled in the same MMIO region or so.
If they are just in different memory chunks then create one device per bank, and all gets much simpler.
Quoted 4 lines
As mentioned above, we cannot use
GPIO_GENERIC. And there was no specific reason/motivation for us to use regmap-mmio because most of the driver's that we referenced were using readl()/write(). Do you recommend us to remove readl()/writel() and switch to regmap-mmio based design ?
I don't really know, usually whatever makes for the simplest code. But check if you can use GPIO_GENERIC first.
Quoted 4 lines
Just to clarify, we have 4 different GPIO banks and each GPIO bank is implemented as a separate
gpio_chip. So we have 4 instances ofgpio_desceach one having its owngpio_chip. What i mean to say is thatgpio_descis actually not a per-line state container, its a pergpio_chipcontainer.
As you're already creating one gpio_chip per bank using GPIO_GENERIC should be simple.
Just reference the memory cells directly when calling bgpio_init() for each mmio address, you're already half done.
Yours,
Linus Walleij