* [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
* [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 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
* 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
* 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
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).