* [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver @ 2019-11-05 6:49 Rahul Tanwar 2019-11-05 6:49 ` [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Rahul Tanwar @ 2019-11-05 6:49 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland Cc: linux-gpio, linux-kernel, devicetree, robh, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim, Rahul Tanwar 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 -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC 2019-11-05 6:49 [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Rahul Tanwar @ 2019-11-05 6:49 ` Rahul Tanwar 2019-11-05 9:49 ` Andy Shevchenko 2019-11-05 6:49 ` [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for " Rahul Tanwar 2019-11-05 9:46 ` [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Linus Walleij 2 siblings, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2019-11-05 6:49 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland Cc: linux-gpio, linux-kernel, devicetree, robh, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim, 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. Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> --- 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 diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index b372419d61f2..7809e33c7762 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -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 diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index ac537fdbc998..879f312bfb75 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -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/ diff --git a/drivers/pinctrl/pinctrl-equilibrium.c b/drivers/pinctrl/pinctrl-equilibrium.c new file mode 100644 index 000000000000..bafb1074bc06 --- /dev/null +++ b/drivers/pinctrl/pinctrl-equilibrium.c @@ -0,0 +1,964 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2019 Intel Corporation */ +#include <linux/gpio/driver.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinconf.h> +#include <linux/pinctrl/pinconf-generic.h> +#include <linux/pinctrl/pinmux.h> +#include <linux/platform_device.h> + +#include "core.h" +#include "pinconf.h" +#include "pinmux.h" +#include "pinctrl-equilibrium.h" + +#define PIN_NAME_FMT "io-%d" +#define PIN_NAME_LEN 10 +#define PAD_REG_OFF 0x100 + +static 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); + mask = mask << offset; + val = readl(addr); + val = (val & ~mask) | ((set << offset) & mask); + writel(val, addr); + raw_spin_unlock_irqrestore(lock, flags); +} + +static void eqbr_gpio_disable_irq(struct irq_data *d) +{ + unsigned int offset = irqd_to_hwirq(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct eqbr_gpio_desc *desc = gpiochip_get_data(gc); + unsigned long flags; + + raw_spin_lock_irqsave(&desc->lock, flags); + writel(BIT(offset), desc->membase + GPIO_IRNENCLR); + raw_spin_unlock_irqrestore(&desc->lock, flags); +} + +static void eqbr_gpio_enable_irq(struct irq_data *d) +{ + unsigned int offset = irqd_to_hwirq(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct eqbr_gpio_desc *desc = gpiochip_get_data(gc); + unsigned long flags; + + gc->direction_input(gc, offset); + raw_spin_lock_irqsave(&desc->lock, flags); + writel(BIT(offset), desc->membase + GPIO_IRNRNSET); + raw_spin_unlock_irqrestore(&desc->lock, flags); +} + +static void eqbr_gpio_ack_irq(struct irq_data *d) +{ + unsigned int offset = irqd_to_hwirq(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct eqbr_gpio_desc *desc = gpiochip_get_data(gc); + unsigned long flags; + + raw_spin_lock_irqsave(&desc->lock, flags); + writel(BIT(offset), desc->membase + GPIO_IRNCR); + raw_spin_unlock_irqrestore(&desc->lock, flags); +} + +static void eqbr_gpio_mask_ack_irq(struct irq_data *d) +{ + eqbr_gpio_disable_irq(d); + eqbr_gpio_ack_irq(d); +} + +static inline void eqbr_cfg_bit(void __iomem *addr, + unsigned int offset, unsigned int set) +{ + if (set) + writel(readl(addr) | BIT(offset), addr); + else + writel(readl(addr) & ~BIT(offset), addr); +} + +static int eqbr_irq_type_cfg(struct gpio_irq_type *type, + struct eqbr_gpio_desc *desc, + unsigned int offset) +{ + unsigned long flags; + + raw_spin_lock_irqsave(&desc->lock, flags); + eqbr_cfg_bit(desc->membase + GPIO_IRNCFG, offset, type->trig_type); + eqbr_cfg_bit(desc->membase + GPIO_EXINTCR1, offset, type->trig_type); + eqbr_cfg_bit(desc->membase + GPIO_EXINTCR0, offset, type->logic_type); + raw_spin_unlock_irqrestore(&desc->lock, flags); + + return 0; +} + +static int eqbr_gpio_set_irq_type(struct irq_data *d, unsigned int type) +{ + unsigned int offset = irqd_to_hwirq(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct eqbr_gpio_desc *desc = gpiochip_get_data(gc); + struct gpio_irq_type it; + + memset(&it, 0, sizeof(it)); + + if ((type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_NONE) + return 0; + + switch (type) { + case IRQ_TYPE_EDGE_RISING: + it.trig_type = GPIO_EDGE_TRIG; + it.edge_type = GPIO_SINGLE_EDGE; + it.logic_type = GPIO_POSITIVE_TRIG; + break; + + case IRQ_TYPE_EDGE_FALLING: + it.trig_type = GPIO_EDGE_TRIG; + it.edge_type = GPIO_SINGLE_EDGE; + it.logic_type = GPIO_NEGATIVE_TRIG; + break; + + case IRQ_TYPE_EDGE_BOTH: + it.trig_type = GPIO_EDGE_TRIG; + it.edge_type = GPIO_BOTH_EDGE; + it.logic_type = GPIO_POSITIVE_TRIG; + break; + + case IRQ_TYPE_LEVEL_HIGH: + it.trig_type = GPIO_LEVEL_TRIG; + it.edge_type = GPIO_SINGLE_EDGE; + it.logic_type = GPIO_POSITIVE_TRIG; + break; + + case IRQ_TYPE_LEVEL_LOW: + it.trig_type = GPIO_LEVEL_TRIG; + it.edge_type = GPIO_SINGLE_EDGE; + it.logic_type = GPIO_NEGATIVE_TRIG; + break; + + default: + return -EINVAL; + } + + eqbr_irq_type_cfg(&it, desc, offset); + if (it.trig_type == GPIO_EDGE_TRIG) + irq_set_handler_locked(d, handle_edge_irq); + else + irq_set_handler_locked(d, handle_level_irq); + + return 0; +} + +static void eqbr_irq_handler(struct irq_desc *desc) +{ + struct gpio_chip *gc = irq_desc_get_handler_data(desc); + struct eqbr_gpio_desc *gpio_desc = gpiochip_get_data(gc); + struct irq_chip *ic = irq_desc_get_chip(desc); + unsigned long pins, offset; + + chained_irq_enter(ic, desc); + pins = readl(gpio_desc->membase + GPIO_IRNCR); + + for_each_set_bit(offset, &pins, gc->ngpio) + generic_handle_irq(irq_find_mapping(gc->irq.domain, offset)); + + chained_irq_exit(ic, desc); +} + +static int gpiochip_setup(struct device *dev, struct eqbr_gpio_desc *desc) +{ + struct gpio_irq_chip *girq; + struct gpio_chip *gc; + + gc = &desc->chip; + gc->label = desc->name; +#if defined(CONFIG_OF_GPIO) + gc->of_node = desc->node; +#endif + + if (!of_property_read_bool(desc->node, "interrupt-controller")) { + dev_info(dev, "gc %s: doesn't act as interrupt controller!\n", + desc->name); + return 0; + } + + desc->ic.name = "gpio_irq"; + desc->ic.irq_mask = eqbr_gpio_disable_irq; + desc->ic.irq_unmask = eqbr_gpio_enable_irq; + desc->ic.irq_ack = eqbr_gpio_ack_irq; + desc->ic.irq_mask_ack = eqbr_gpio_mask_ack_irq; + desc->ic.irq_set_type = eqbr_gpio_set_irq_type; + + girq = &desc->chip.irq; + girq->chip = &desc->ic; + girq->parent_handler = eqbr_irq_handler; + girq->num_parents = 1; + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), GFP_KERNEL); + if (!girq->parents) + return -ENOMEM; + + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_bad_irq; + girq->parents[0] = desc->virq; + + return 0; +} + +static int gpiolib_reg(struct eqbr_pinctrl_drv_data *drvdata) +{ + struct device_node *np; + struct eqbr_gpio_desc *desc; + struct device *dev; + int i, ret; + struct resource res; + + dev = drvdata->dev; + for (i = 0; i < drvdata->nr_gpio_descs; i++) { + desc = drvdata->gpio_desc + i; + np = desc->node; + + desc->name = devm_kasprintf(dev, GFP_KERNEL, "gpiochip%d", i); + if (!desc->name) + return -ENOMEM; + + if (of_address_to_resource(np, 0, &res)) { + dev_err(dev, "Failed to get GPIO register address\n"); + return -ENXIO; + } + + desc->membase = devm_ioremap_resource(dev, &res); + if (IS_ERR(desc->membase)) + return PTR_ERR(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", + desc->name); + return -ENXIO; + } + raw_spin_lock_init(&desc->lock); + + ret = bgpio_init(&desc->chip, dev, desc->bank->nr_pins/8, + 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; + } + + ret = gpiochip_setup(dev, desc); + if (ret) + return ret; + + ret = devm_gpiochip_add_data(dev, &desc->chip, desc); + if (ret) + return ret; + } + + return 0; +} + +static inline struct eqbr_pin_bank +*find_pinbank_via_pin(struct eqbr_pinctrl_drv_data *pctl, unsigned int pin) +{ + int i; + struct eqbr_pin_bank *bank; + + for (i = 0; i < pctl->nr_banks; i++) { + bank = &pctl->pin_banks[i]; + if (pin >= bank->pin_base && + (pin - bank->pin_base) < bank->nr_pins) + return bank; + } + + return NULL; +} + +static const struct pinctrl_ops eqbr_pctl_ops = { + .get_groups_count = pinctrl_generic_get_group_count, + .get_group_name = pinctrl_generic_get_group_name, + .get_group_pins = pinctrl_generic_get_group_pins, + .dt_node_to_map = pinconf_generic_dt_node_to_map_all, + .dt_free_map = pinconf_generic_dt_free_map, +}; + +static int eqbr_set_pin_mux(struct eqbr_pinctrl_drv_data *pctl, + unsigned int pmx, unsigned int pin) +{ + void __iomem *mem; + struct eqbr_pin_bank *bank; + unsigned int offset; + + bank = find_pinbank_via_pin(pctl, pin); + if (!bank) { + dev_err(pctl->dev, "Couldn't find pin bank for pin %u\n", pin); + return -ENODEV; + } + mem = bank->membase; + offset = pin - bank->pin_base; + + if (!(bank->aval_pinmap & BIT(offset))) { + dev_err(pctl->dev, + "PIN: %u is not valid, pinbase: %u, bitmap: %u\n", + pin, bank->pin_base, bank->aval_pinmap); + return -ENODEV; + } + + writel(pmx, mem + (offset << 2)); + 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; + + func = pinmux_generic_get_function(pctldev, selector); + if (!func) + return -EINVAL; + + grp = pinctrl_generic_get_group(pctldev, group); + if (!grp) + return -EINVAL; + + pinmux = grp->data; + for (i = 0; i < grp->num_pins; i++) + eqbr_set_pin_mux(pctl, pinmux[i], grp->pins[i]); + + return 0; +} + +static int eqbr_pinmux_gpio_request(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned int pin) +{ + struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev); + + /* 0 mux is reserved for GPIO */ + return eqbr_set_pin_mux(pctl, 0, pin); +} + +static const struct pinmux_ops eqbr_pinmux_ops = { + .get_functions_count = pinmux_generic_get_function_count, + .get_function_name = pinmux_generic_get_function_name, + .get_function_groups = pinmux_generic_get_function_groups, + .set_mux = eqbr_pinmux_set_mux, + .gpio_request_enable = eqbr_pinmux_gpio_request, + .strict = true, +}; + +static void set_drv_cur(void __iomem *mem, unsigned int offset, + unsigned int set, raw_spinlock_t *lock) +{ + unsigned int idx = offset / DRV_CUR_PINS; /* 16 pin per register*/ + unsigned int reg; + + offset %= DRV_CUR_PINS; + reg = REG_DRCC(idx); + eqbr_set_val(mem + REG_DRCC(idx), offset * 2, 0x3, set, lock); +} + +static int get_drv_cur(void __iomem *mem, unsigned int offset) +{ + unsigned int idx = offset / DRV_CUR_PINS; /* 0-15, 16-31 per register*/ + unsigned int val; + + val = readl(mem + REG_DRCC(idx)); + offset %= DRV_CUR_PINS; + val = PARSE_DRV_CURRENT(val, offset); + + return val; +} + +static struct eqbr_gpio_desc +*get_gpio_desc_via_bank(struct eqbr_pinctrl_drv_data *pctl, + struct eqbr_pin_bank *bank) +{ + int i; + + for (i = 0; i < pctl->nr_gpio_descs; i++) { + if (pctl->gpio_desc[i].bank == bank) + return &pctl->gpio_desc[i]; + } + + return NULL; +} + +static int eqbr_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, + unsigned long *config) +{ + struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev); + enum pin_config_param param = pinconf_to_config_param(*config); + struct eqbr_pin_bank *bank; + unsigned int offset; + void __iomem *mem; + struct eqbr_gpio_desc *gpio; + u32 val; + + bank = find_pinbank_via_pin(pctl, pin); + if (!bank) { + dev_err(pctl->dev, "Couldn't find pin bank for pin %u\n", pin); + return -ENODEV; + } + mem = bank->membase; + offset = pin - bank->pin_base; + + if (!(bank->aval_pinmap & BIT(offset))) { + dev_err(pctl->dev, + "PIN: %u is not valid, pinbase: %u, bitmap: %u\n", + pin, bank->pin_base, bank->aval_pinmap); + return -ENODEV; + } + + switch (param) { + case PIN_CONFIG_BIAS_PULL_UP: + val = !!(readl(mem + REG_PUEN) & BIT(offset)); + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + val = !!(readl(mem + REG_PDEN) & BIT(offset)); + break; + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + val = !!(readl(mem + REG_OD) & BIT(offset)); + break; + case PIN_CONFIG_DRIVE_STRENGTH: + val = get_drv_cur(mem, offset); + break; + case PIN_CONFIG_SLEW_RATE: + val = !!(readl(mem + REG_SRC) & BIT(offset)); + break; + case PIN_CONFIG_OUTPUT_ENABLE: + gpio = get_gpio_desc_via_bank(pctl, bank); + if (!gpio) { + dev_err(pctl->dev, "Failed to find gpio via bank pinbase: %u, pin: %u\n", + bank->pin_base, pin); + return -ENODEV; + } + val = !!(readl(gpio->membase + GPIO_DIR) & BIT(offset)); + break; + default: + return -ENOTSUPP; + } + + *config = pinconf_to_config_packed(param, val); +; + return 0; +} + +static int eqbr_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, + unsigned long *configs, unsigned int num_configs) +{ + struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev); + enum pin_config_param param; + unsigned int val, offset; + struct eqbr_pin_bank *bank; + struct eqbr_gpio_desc *gpio; + struct gpio_chip *gc; + void __iomem *mem; + int i; + + for (i = 0; i < num_configs; i++) { + param = pinconf_to_config_param(configs[i]); + val = pinconf_to_config_argument(configs[i]); + + bank = find_pinbank_via_pin(pctl, pin); + if (!bank) { + dev_err(pctl->dev, + "Couldn't find pin bank for pin %u\n", pin); + return -ENODEV; + } + mem = bank->membase; + offset = pin - bank->pin_base; + + switch (param) { + case PIN_CONFIG_BIAS_PULL_UP: + eqbr_set_val(mem + REG_PUEN, offset, 1, val, &pctl->lock); + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + eqbr_set_val(mem + REG_PDEN, offset, 1, val, &pctl->lock); + break; + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + eqbr_set_val(mem + REG_OD, offset, 1, val, &pctl->lock); + break; + case PIN_CONFIG_DRIVE_STRENGTH: + set_drv_cur(mem, offset, val, &pctl->lock); + break; + case PIN_CONFIG_SLEW_RATE: + eqbr_set_val(mem + REG_SRC, offset, 1, val, &pctl->lock); + break; + case PIN_CONFIG_OUTPUT_ENABLE: + gpio = get_gpio_desc_via_bank(pctl, bank); + if (!gpio) { + dev_err(pctl->dev, "Failed to find gpio via bank pinbase: %u, pin: %u\n", + bank->pin_base, pin); + return -ENODEV; + } + gc = &gpio->chip; + gc->direction_output(gc, offset, 0); + break; + default: + return -ENOTSUPP; + } + } + + return 0; +} + +static int eqbr_pinconf_group_get(struct pinctrl_dev *pctldev, + unsigned int group, unsigned long *config) +{ + const unsigned int *pins; + unsigned int i, npins, old = 0; + int ret; + + ret = pinctrl_generic_get_group_pins(pctldev, group, &pins, &npins); + if (ret) + return ret; + + for (i = 0; i < npins; i++) { + if (eqbr_pinconf_get(pctldev, pins[i], config)) + return -ENOTSUPP; + + if (i && old != *config) + return -ENOTSUPP; + + old = *config; + } + return 0; +} + +static int eqbr_pinconf_group_set(struct pinctrl_dev *pctldev, + unsigned int group, unsigned long *configs, + unsigned int num_configs) +{ + const unsigned int *pins; + unsigned int i, npins; + int ret; + + ret = pinctrl_generic_get_group_pins(pctldev, group, &pins, &npins); + if (ret) + return ret; + + for (i = 0; i < npins; i++) { + ret = eqbr_pinconf_set(pctldev, pins[i], configs, num_configs); + if (ret) + return ret; + } + return 0; +} + +static const struct pinconf_ops eqbr_pinconf_ops = { + .is_generic = true, + .pin_config_get = eqbr_pinconf_get, + .pin_config_set = eqbr_pinconf_set, + .pin_config_group_get = eqbr_pinconf_group_get, + .pin_config_group_set = eqbr_pinconf_group_set, + .pin_config_config_dbg_show = pinconf_generic_dump_config, +}; + +static int is_func_exist(struct eqbr_pmx_func *funcs, const char *name, + unsigned int nr_funcs, unsigned int *idx) +{ + int i; + + if (!funcs || !nr_funcs) + return 0; + + for (i = 0; i < nr_funcs; i++) { + + 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; + 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) { + if (of_property_read_string(np, "function", + &fn_name)) { + /* some groups may not have function, it's OK */ + dev_dbg(dev, "Group %s: not function binded!\n", + (char *)prop->value); + continue; + } + + switch (op) { + case OP_COUNT_NR_FUNCS: + if (!is_func_exist(funcs, fn_name, + *nr_funcs, &fid)) + *nr_funcs = *nr_funcs + 1; + break; + + case OP_ADD_FUNCS: + if (!is_func_exist(funcs, fn_name, + *nr_funcs, &fid)) + funcs[i].name = fn_name; + break; + + case OP_COUNT_NR_FUNC_GRPS: + if (is_func_exist(funcs, fn_name, + *nr_funcs, &fid)) + funcs[fid].nr_groups++; + break; + + case OP_ADD_FUNC_GRPS: + if (is_func_exist(funcs, fn_name, + *nr_funcs, &fid)) { + for(j=0; + j < funcs[fid].nr_groups; + j++) { + if (!funcs[fid].groups[j]) + break; + } + funcs[fid].groups[j] = prop->value; + } + break; + + default: + return -EINVAL; + + } + i++; + } + } + + return 0; +} + +static int eqbr_build_functions(struct eqbr_pinctrl_drv_data *drvdata) +{ + struct device *dev = drvdata->dev; + unsigned int nr_funcs = 0; + struct eqbr_pmx_func *funcs = NULL; + int i, ret; + + ret = funcs_utils(dev, funcs, &nr_funcs, OP_COUNT_NR_FUNCS); + if (ret) + return ret; + + funcs = devm_kcalloc(dev, nr_funcs, sizeof(*funcs), GFP_KERNEL); + if (!funcs) + return -ENOMEM; + + ret = funcs_utils(dev, funcs, &nr_funcs, OP_ADD_FUNCS); + if (ret) + return ret; + + ret = funcs_utils(dev, funcs, &nr_funcs, OP_COUNT_NR_FUNC_GRPS); + if (ret) + return ret; + + for (i=0; i < nr_funcs; i++) { + if (funcs[i].nr_groups) { + funcs[i].groups = devm_kcalloc(dev, funcs[i].nr_groups, + sizeof(*(funcs[i].groups)), + GFP_KERNEL); + if (!funcs[i].groups) + return -ENOMEM; + } + } + + ret = funcs_utils(dev, funcs, &nr_funcs, OP_ADD_FUNC_GRPS); + if (ret) + return ret; + + for (i=0; i < nr_funcs; i++) { + ret = pinmux_generic_add_function(drvdata->pctl_dev, + funcs[i].name, + funcs[i].groups, + funcs[i].nr_groups, + drvdata); + if (ret < 0) { + dev_err(dev, "Failed to register function %s\n", + funcs[i].name); + return ret; + } + } + + return 0; +} + +static int eqbr_build_groups(struct eqbr_pinctrl_drv_data *drvdata) +{ + struct device *dev = drvdata->dev; + struct device_node *node = dev->of_node; + 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) { + group.num_pins = of_property_count_u32_elems(np, "pins"); + if (group.num_pins < 0) { + dev_err(dev, "No pins in the group: %s\n", + prop->name); + return -EINVAL; + } + group.name = prop->value; + group.pins = devm_kcalloc(dev, group.num_pins, + sizeof(*(group.pins)), + GFP_KERNEL); + pinmux = devm_kcalloc(dev, group.num_pins, + sizeof(*pinmux), GFP_KERNEL); + + if (!group.pins || !pinmux) + return -ENOMEM; + for (j = 0; j < group.num_pins; j++) { + if (of_property_read_u32_index(np, "pins", + j, &pin_id)) { + dev_err(dev, "Group %s: Read intel pins id failed\n", + group.name); + return -EINVAL; + } + if (pin_id >= drvdata->pctl_desc.npins) { + dev_err(dev, "Group %s: Invalid pin ID, idx: %d, pin %u\n", + group.name, j, pin_id); + return -EINVAL; + } + group.pins[j] = pin_id; + if (of_property_read_u32_index(np, "pinmux", + j, &pinmux_id)) { + dev_err(dev, "Group %s: Read intel pinmux id failed\n", + group.name); + return -EINVAL; + } + pinmux[j] = pinmux_id; + } + + err = pinctrl_generic_add_group(drvdata->pctl_dev, + group.name, group.pins, + group.num_pins, + pinmux); + if (err < 0) { + dev_err(dev, "Failed to register group %s\n", + group.name); + return err; + } + } + memset(&group, 0, sizeof(group)); + pinmux = NULL; + } + + return 0; +} + +static int pinctrl_reg(struct eqbr_pinctrl_drv_data *drvdata) +{ + struct pinctrl_desc *pctl_desc; + struct pinctrl_pin_desc *pdesc; + struct device *dev; + unsigned int nr_pins; + char *pin_names; + int i, ret; + + dev = drvdata->dev; + pctl_desc = &drvdata->pctl_desc; + pctl_desc->name = "eqbr-pinctrl"; + pctl_desc->owner = THIS_MODULE; + pctl_desc->pctlops = &eqbr_pctl_ops; + pctl_desc->pmxops = &eqbr_pinmux_ops; + pctl_desc->confops = &eqbr_pinconf_ops; + raw_spin_lock_init(&drvdata->lock); + + for (i = 0, nr_pins = 0; i < drvdata->nr_banks; i++) + nr_pins += drvdata->pin_banks[i].nr_pins; + + pdesc = devm_kcalloc(dev, nr_pins, sizeof(*pdesc), GFP_KERNEL); + if (!pdesc) + return -ENOMEM; + pin_names = devm_kcalloc(dev, nr_pins, PIN_NAME_LEN, GFP_KERNEL); + if (!pin_names) + return -ENOMEM; + + for (i = 0; i < nr_pins; i++) { + sprintf(pin_names, PIN_NAME_FMT, i); + pdesc[i].number = i; + pdesc[i].name = pin_names; + pin_names += PIN_NAME_LEN; + } + pctl_desc->pins = pdesc; + pctl_desc->npins = nr_pins; + dev_dbg(dev, "pinctrl total pin number: %u\n", nr_pins); + + ret = devm_pinctrl_register_and_init(dev, pctl_desc, drvdata, + &drvdata->pctl_dev); + if (ret) + return ret; + + ret = eqbr_build_groups(drvdata); + if (ret) { + dev_err(dev, "Failed to build groups\n"); + return ret; + } + + ret = eqbr_build_functions(drvdata); + if (ret) { + dev_err(dev, "Failed to build groups\n"); + return ret; + } + + return pinctrl_enable(drvdata->pctl_dev); +} + +static int pinbank_init(struct device_node *np, + struct eqbr_pinctrl_drv_data *drvdata, + 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; + } + + bank->pin_base = spec.args[1]; + bank->nr_pins = spec.args[2]; + + bank->aval_pinmap = readl(bank->membase + REG_AVAIL); + bank->id = id; + + dev_dbg(dev, "pinbank id: %d, reg: %px, pinbase: %u, pin number: %u, pinmap: 0x%x\n", + id, bank->membase, bank->pin_base, + bank->nr_pins, bank->aval_pinmap); + + return 0; +} + +static int pinbank_probe(struct eqbr_pinctrl_drv_data *drvdata) +{ + struct device_node *node, *np_gpio; + struct eqbr_pin_bank *banks; + struct eqbr_gpio_desc *gpio_desc; + struct device *dev; + int i=0, nr_gpio=0; + + dev = drvdata->dev; + node = dev->of_node; + + /* Count gpio bank number */ + for_each_node_by_name(np_gpio, "gpio") { + if (of_device_is_available(np_gpio)) + nr_gpio++; + } + + if (!nr_gpio) { + dev_err(dev, "NO pin bank available!\n"); + return -ENODEV; + } + + /* Count pin bank number and gpio controller number */ + banks = devm_kcalloc(dev, nr_gpio, sizeof(*banks), GFP_KERNEL); + if (!banks) + return -ENOMEM; + + gpio_desc = devm_kcalloc(dev, nr_gpio, sizeof(*gpio_desc), GFP_KERNEL); + if (!gpio_desc) + return -ENOMEM; + + dev_dbg(dev, "found %d gpio controller!\n", nr_gpio); + + /* Initialize Pin bank */ + for_each_node_by_name(np_gpio, "gpio") { + if (!of_device_is_available(np_gpio)) + continue; + + pinbank_init(np_gpio, drvdata, banks + i, i); + + gpio_desc[i].node = np_gpio; + gpio_desc[i].bank = banks + i; + i++; + } + + drvdata->pin_banks = banks; + drvdata->nr_banks = nr_gpio; + drvdata->gpio_desc = gpio_desc; + drvdata->nr_gpio_descs = nr_gpio; + + return 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); + + 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; +} + +static const struct of_device_id eqbr_pinctrl_dt_match[] = { + { .compatible = "intel,lgm-pinctrl" }, + {} +}; + +static struct platform_driver eqbr_pinctrl_driver = { + .probe = eqbr_pinctrl_probe, + .driver = { + .name = "eqbr-pinctrl", + .of_match_table = eqbr_pinctrl_dt_match, + }, +}; + +module_platform_driver(eqbr_pinctrl_driver); + +MODULE_AUTHOR("Zhu Yixin <yixin.zhu@intel.com>, Rahul Tanwar <rahul.tanwar@intel.com>"); +MODULE_DESCRIPTION("Pinctrl Driver for LGM SoC (Equilibrium)"); diff --git a/drivers/pinctrl/pinctrl-equilibrium.h b/drivers/pinctrl/pinctrl-equilibrium.h new file mode 100644 index 000000000000..1bb92229a186 --- /dev/null +++ b/drivers/pinctrl/pinctrl-equilibrium.h @@ -0,0 +1,141 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright(c) 2019 Intel Corporation. + */ +#ifndef __PINCTRL_EQUILIBRIUM_H +#define __PINCTRL_EQUILIBRIUM_H + +/* PINPAD register offset */ +#define REG_PMX_BASE 0x0 /* Port Multiplexer Control Register */ +#define REG_PUEN 0x80 /* PULL UP Enable Register */ +#define REG_PDEN 0x84 /* PULL DOWN Enable Register */ +#define REG_SRC 0x88 /* Slew Rate Control Register */ +#define REG_DCC0 0x8C /* Drive Current Control Register 0 */ +#define REG_DCC1 0x90 /* Drive Current Control Register 1 */ +#define REG_OD 0x94 /* Open Drain Enable Register */ +#define REG_AVAIL 0x98 /* Pad Control Availability Register */ +#define DRV_CUR_PINS 16 /* Drive Current pin number per register */ +#define REG_DRCC(x) (REG_DCC0 + (x) * 4) /* Driver current macro */ + +/* GPIO register offset */ +#define GPIO_OUT 0x0 /* Data Output Register */ +#define GPIO_IN 0x4 /* Data Input Register */ +#define GPIO_DIR 0x8 /* Direction Register */ +#define GPIO_EXINTCR0 0x18 /* External Interrupt Control Register 0 */ +#define GPIO_EXINTCR1 0x1C /* External Interrupt Control Register 1 */ +#define GPIO_IRNCR 0x20 /* IRN Capture Register */ +#define GPIO_IRNICR 0x24 /* IRN Interrupt Control Register */ +#define GPIO_IRNEN 0x28 /* IRN Interrupt Enable Register */ +#define GPIO_IRNCFG 0x2C /* IRN Interrupt Configuration Register */ +#define GPIO_IRNRNSET 0x30 /* IRN Interrupt Enable Set Register */ +#define GPIO_IRNENCLR 0x34 /* IRN Interrupt Enable Clear Register */ +#define GPIO_OUTSET 0x40 /* Output Set Register */ +#define GPIO_OUTCLR 0x44 /* Output Clear Register */ +#define GPIO_DIRSET 0x48 /* Direction Set Register */ +#define GPIO_DIRCLR 0x4C /* Direction Clear Register */ + +/* parse given pin's driver current value */ +#define PARSE_DRV_CURRENT(val, pin) (((val) >> ((pin) << 1)) & 0x3) + +#define GPIO_EDGE_TRIG 0 +#define GPIO_LEVEL_TRIG 1 +#define GPIO_SINGLE_EDGE 0 +#define GPIO_BOTH_EDGE 1 +#define GPIO_POSITIVE_TRIG 0 +#define GPIO_NEGATIVE_TRIG 1 + +typedef enum { + OP_COUNT_NR_FUNCS, + OP_ADD_FUNCS, + OP_COUNT_NR_FUNC_GRPS, + OP_ADD_FUNC_GRPS, + OP_NONE, +} funcs_util_ops; + +/** + * struct gpio_irq_type: gpio irq configuration + * @trig_type: level trigger or edge trigger + * @edge_type: sigle edge or both edge + * @logic_type: positive trigger or negative trigger + */ +struct gpio_irq_type { + unsigned int trig_type; + unsigned int edge_type; + unsigned int logic_type; +}; + +/** + * struct eqbr_pmx_func: represent a pin function. + * @name: name of the pin function, used to lookup the function. + * @groups: one or more names of pin groups that provide this function. + * @nr_groups: number of groups included in @groups. + */ +struct eqbr_pmx_func { + const char *name; + const char **groups; + unsigned int nr_groups; +}; + +/** + * struct eqbr_pin_bank: represent a pin bank. + * @membase: base address of the pin bank register. + * @id: bank id, to idenify the unique bank. + * @pin_base: starting pin number of the pin bank. + * @nr_pins: number of the pins of the pin bank. + * @aval_pinmap: available pin bitmap of the pin bank. + */ +struct eqbr_pin_bank { + void __iomem *membase; + unsigned int id; + unsigned int pin_base; + unsigned int nr_pins; + u32 aval_pinmap; +}; + +/** + * struct eqbr_gpio_desc: represent a gpio controller. + * @node: device node of gpio controller. + * @bank: pointer to corresponding pin bank. + * @membase: base address of the gpio controller. + * @chip: gpio chip. + * @ic: irq chip. + * @name: gpio chip name. + * @virq: irq number of the gpio chip to parent's irq domain. + * @lock: spin lock to protect gpio register write. + */ +struct eqbr_gpio_desc { + struct device_node *node; + struct eqbr_pin_bank *bank; + void __iomem *membase; + struct gpio_chip chip; + struct irq_chip ic; + const char *name; + unsigned int virq; + raw_spinlock_t lock; /* protect gpio register */ +}; + +/** + * struct eqbr_pinctrl_drv_data: + * @dev: device instance representing the controller. + * @pctl_desc: pin controller descriptor. + * @pctl_dev: pin control class device + * @membase: base address of pin controller + * @pin_banks: list of pin banks of the driver. + * @nr_banks: number of pin banks. + * @gpio_desc: list of gpio controller descriptor. + * @nr_gpio_descs: number of gpio descriptors. + * @lock: protect pinctrl register write + */ +struct eqbr_pinctrl_drv_data { + struct device *dev; + struct pinctrl_desc pctl_desc; + struct pinctrl_dev *pctl_dev; + void __iomem *membase; + struct eqbr_pin_bank *pin_banks; + unsigned int nr_banks; + struct eqbr_gpio_desc *gpio_desc; + unsigned int nr_gpio_descs; + raw_spinlock_t lock; /* protect pinpad register */ +}; + +#endif /* __PINCTRL_EQUILIBRIUM_H */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC 2019-11-05 6:49 ` [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar @ 2019-11-05 9:49 ` Andy Shevchenko 2019-11-05 10:52 ` Tanwar, Rahul 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2019-11-05 9:49 UTC (permalink / raw) To: Rahul Tanwar Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, linux-kernel, devicetree, robh, qi-ming.wu, yixin.zhu, cheol.yong.kim On Tue, Nov 05, 2019 at 02:49:42PM +0800, Rahul Tanwar wrote: > 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. > +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? > +{ > + u32 val; > + unsigned long flags; > + > + raw_spin_lock_irqsave(lock, flags); > + mask = mask << offset; Same Q. Why do you need these... > + val = readl(addr); > + val = (val & ~mask) | ((set << offset) & mask); ...offset shifts? It's unusual. > + 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? > + > + 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? > + return 0; Ditto. > + } > +} > +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. > + struct device *dev; > + int i, ret; > + struct resource res; > + > + ret = bgpio_init(&desc->chip, dev, desc->bank->nr_pins/8, 'nr_pins / 8,' > + 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; > + 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. > + return 0; > +} > +static int is_func_exist(struct eqbr_pmx_func *funcs, const char *name, Looks like it better to be boolean. > + 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. > + 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; > + 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; ? > + if (of_property_read_string(np, "function", > + &fn_name)) { It's perfectly one line. Perhaps you may need to configure your text editor. > + } > + } > + } > + > + 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. > + } > +static int eqbr_build_groups(struct eqbr_pinctrl_drv_data *drvdata) > +{ > + struct device *dev = drvdata->dev; > + struct device_node *node = dev->of_node; > + 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; ? > + } > + 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) > +{ > + 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. > + } > + return 0; > +} > + int i=0, nr_gpio=0; Style. Besides the fact that better to put assignments closer to their usage. > +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. > + 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; > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC 2019-11-05 9:49 ` Andy Shevchenko @ 2019-11-05 10:52 ` Tanwar, Rahul 2019-11-05 12:05 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: Tanwar, Rahul @ 2019-11-05 10:52 UTC (permalink / raw) To: Andy Shevchenko Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, linux-kernel, devicetree, robh, qi-ming.wu, yixin.zhu, cheol.yong.kim On 5/11/2019 5:49 PM, Andy Shevchenko wrote: > On Tue, Nov 05, 2019 at 02:49:42PM +0800, Rahul Tanwar wrote: >> 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. >> +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. >> +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. >> + >> + 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 ? >> + return 0; > Ditto. > >> + } >> +} >> +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. >> + 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. >> + 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; > >> + 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. >> + 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. >> + 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. >> + 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; >> + 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; > ? Sure, i can change like that in v4. >> + 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 :) >> + } >> + } >> + } >> + >> + 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. >> + } >> >> +static int pinbank_init(struct device_node *np, >> + struct eqbr_pinctrl_drv_data *drvdata, >> + 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC 2019-11-05 10:52 ` Tanwar, Rahul @ 2019-11-05 12:05 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2019-11-05 12:05 UTC (permalink / raw) To: Tanwar, Rahul Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, linux-kernel, devicetree, robh, qi-ming.wu, yixin.zhu, cheol.yong.kim On Tue, Nov 05, 2019 at 06:52:52PM +0800, Tanwar, Rahul wrote: > On 5/11/2019 5:49 PM, Andy Shevchenko wrote: > > On Tue, Nov 05, 2019 at 02:49:42PM +0800, Rahul Tanwar wrote: > >> +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. Good. > >> +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. OK! > >> + 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 ? > > > >> + return 0; > > Ditto. > >> + } 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. > >> +} > >> +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; > > > >> + 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. You need to clarify what exactly you are serializing. When you figure this out, the locking should be done accordingly. > >> + 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. > >> + } -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC 2019-11-05 6:49 [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Rahul Tanwar 2019-11-05 6:49 ` [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar @ 2019-11-05 6:49 ` Rahul Tanwar 2019-11-05 21:29 ` Rob Herring 2019-11-05 9:46 ` [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Linus Walleij 2 siblings, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2019-11-05 6:49 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland Cc: linux-gpio, linux-kernel, devicetree, robh, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim, Rahul Tanwar Add dt bindings document for pinmux & GPIO controller driver of Intel Lightning Mountain SoC. Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> --- .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 114 +++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml diff --git a/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml new file mode 100644 index 000000000000..961ac877a962 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml @@ -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 <rahul.tanwar@linux.intel.com> + +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"; + }; + }; + +... -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC 2019-11-05 6:49 ` [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for " Rahul Tanwar @ 2019-11-05 21:29 ` Rob Herring 2019-11-06 10:24 ` Tanwar, Rahul 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2019-11-05 21:29 UTC (permalink / raw) To: Rahul Tanwar Cc: linus.walleij, mark.rutland, linux-gpio, linux-kernel, devicetree, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim On Tue, Nov 05, 2019 at 02:49:43PM +0800, Rahul Tanwar wrote: > Add dt bindings document for pinmux & GPIO controller driver of > Intel Lightning Mountain SoC. > > Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> > --- > .../bindings/pinctrl/intel,lgm-pinctrl.yaml | 114 +++++++++++++++++++++ > 1 file changed, 114 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml > new file mode 100644 > index 000000000000..961ac877a962 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml > @@ -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 <rahul.tanwar@linux.intel.com> > + > +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]+$": A unit address is wrong here. Please define some pattern we can match on. '-pins$' perhaps. > + 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. Isn't this boolean? > + > + 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. > + > + 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? > + > + 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. > + > + > + required: > + - function > + - groups > + > +required: > + - compatible > + - reg additionalProperties: false > + > +examples: > + # Pinmux controller node > + - | > + pinctrl: pinctrl@e2880000 { > + compatible = "intel,lgm-pinctrl"; > + reg = <0xe2880000 0x100000>; > + > + # Client device subnode > + uart0:uart0 { space ^ > + pins = <64>, /* UART_RX0 */ > + <65>; /* UART_TX0 */ > + function = "CONSOLE_UART0"; > + pinmux = <1>, > + <1>; > + groups = "CONSOLE_UART0"; > + }; > + }; > + > +... > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC 2019-11-05 21:29 ` Rob Herring @ 2019-11-06 10:24 ` Tanwar, Rahul 2019-11-06 10:29 ` Tanwar, Rahul 0 siblings, 1 reply; 10+ messages in thread From: Tanwar, Rahul @ 2019-11-06 10:24 UTC (permalink / raw) To: Rob Herring Cc: linus.walleij, mark.rutland, linux-gpio, linux-kernel, devicetree, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim Hi Rob, Thanks for the feedback. On 6/11/2019 5:29 AM, Rob Herring wrote: >> + bias-pull-up: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Specifies pull-up configuration. > Isn't this boolean? > >> + >> + 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. > >> + >> + 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? > >> + >> + 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 ? >> + >> +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 ? >> + pins = <64>, /* UART_RX0 */ >> + <65>; /* UART_TX0 */ >> + function = "CONSOLE_UART0"; >> + pinmux = <1>, >> + <1>; >> + groups = "CONSOLE_UART0"; >> + }; >> + }; >> + >> +... >> -- >> 2.11.0 >> Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for new SoC 2019-11-06 10:24 ` Tanwar, Rahul @ 2019-11-06 10:29 ` Tanwar, Rahul 0 siblings, 0 replies; 10+ messages in thread From: Tanwar, Rahul @ 2019-11-06 10:29 UTC (permalink / raw) To: Rob Herring Cc: linus.walleij, mark.rutland, linux-gpio, linux-kernel, devicetree, andriy.shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim On 6/11/2019 6:24 PM, Tanwar, Rahul wrote: > Hi Rob, > > Thanks for the feedback. > > On 6/11/2019 5:29 AM, Rob Herring wrote: >>> + bias-pull-up: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: Specifies pull-up configuration. >> Isn't this boolean? >> >>> + >>> + 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. >> >>> + >>> + 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? >> >>> + >>> + 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 ? > >>> + >>> +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 ? 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. >>> + pins = <64>, /* UART_RX0 */ >>> + <65>; /* UART_TX0 */ >>> + function = "CONSOLE_UART0"; >>> + pinmux = <1>, >>> + <1>; >>> + groups = "CONSOLE_UART0"; >>> + }; >>> + }; >>> + >>> +... >>> -- >>> 2.11.0 >>> > Regards, > Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver 2019-11-05 6:49 [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Rahul Tanwar 2019-11-05 6:49 ` [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar 2019-11-05 6:49 ` [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for " Rahul Tanwar @ 2019-11-05 9:46 ` Linus Walleij 2 siblings, 0 replies; 10+ messages in thread From: Linus Walleij @ 2019-11-05 9:46 UTC (permalink / raw) To: Rahul Tanwar Cc: Rob Herring, Mark Rutland, open list:GPIO SUBSYSTEM, linux-kernel, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rob Herring, Andriy Shevchenko, qi-ming.wu, yixin.zhu, cheol.yong.kim On Tue, Nov 5, 2019 at 7:49 AM Rahul Tanwar <rahul.tanwar@linux.intel.com> wrote: > 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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-06 10:29 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-05 6:49 [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Rahul Tanwar 2019-11-05 6:49 ` [PATCH v3 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar 2019-11-05 9:49 ` Andy Shevchenko 2019-11-05 10:52 ` Tanwar, Rahul 2019-11-05 12:05 ` Andy Shevchenko 2019-11-05 6:49 ` [PATCH v3 2/2] dt-bindings: pinctrl: intel: Add for " Rahul Tanwar 2019-11-05 21:29 ` Rob Herring 2019-11-06 10:24 ` Tanwar, Rahul 2019-11-06 10:29 ` Tanwar, Rahul 2019-11-05 9:46 ` [PATCH v3 0/2] pinctrl: Add new pinctrl/GPIO driver Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).