linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
@ 2021-01-11 18:29 AngeloGioacchino Del Regno
  2021-01-11 18:29 ` [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
  2021-01-18 13:19 ` [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander Linus Walleij
  0 siblings, 2 replies; 10+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-11 18:29 UTC (permalink / raw)
  To: linus.walleij
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, linux-gpio, devicetree, robh+dt,
	AngeloGioacchino Del Regno

The Awinic AW9523(B) is a multi-function I2C gpio expander in a
TQFN-24L package, featuring PWM (max 37mA per pin, or total max
power 3.2Watts) for LED driving capability.

It has two ports with 8 pins per port (for a total of 16 pins),
configurable as either PWM with 1/256 stepping or GPIO input/output,
1.8V logic input; each GPIO can be configured as input or output
independently from each other.

This IC also has an internal interrupt controller, which is capable
of generating an interrupt for each GPIO, depending on the
configuration, and will raise an interrupt on the INTN pin to
advertise this to an external interrupt controller.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/pinctrl/Kconfig          |   17 +
 drivers/pinctrl/Makefile         |    1 +
 drivers/pinctrl/pinctrl-aw9523.c | 1124 ++++++++++++++++++++++++++++++
 3 files changed, 1142 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-aw9523.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index d4b2f2e2ed75..aaf8cc21fd78 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -111,6 +111,23 @@ config PINCTRL_AMD
 	  Requires ACPI/FDT device enumeration code to set up a platform
 	  device.
 
+config PINCTRL_AW9523
+	bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver"
+	depends on OF && I2C
+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select REGMAP
+	help
+	  The Awinic AW9523/AW9523B is a multi-function I2C GPIO
+	  expander with PWM functionality. This driver bundles a
+	  pinctrl driver to select the function muxing and a GPIO
+	  driver to handle GPIO, when the GPIO function is selected.
+
+	  Say yes to enable pinctrl and GPIO support for the AW9523(B).
+
 config PINCTRL_BM1880
 	bool "Bitmain BM1880 Pinctrl driver"
 	depends on OF && (ARCH_BITMAIN || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 5bb9bb6cc3ce..f641d4974717 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
 obj-$(CONFIG_PINCTRL_AT91)	+= pinctrl-at91.o
 obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
 obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
+obj-$(CONFIG_PINCTRL_AW9523)	+= pinctrl-aw9523.o
 obj-$(CONFIG_PINCTRL_BM1880)	+= pinctrl-bm1880.o
 obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
 obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
diff --git a/drivers/pinctrl/pinctrl-aw9523.c b/drivers/pinctrl/pinctrl-aw9523.c
new file mode 100644
index 000000000000..587f4ac0e96f
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-aw9523.c
@@ -0,0 +1,1124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Awinic AW9523B i2c pin controller driver
+ * Copyright (c) 2020, AngeloGioacchino Del Regno
+ *                     <angelogioacchino.delregno@somainline.org>
+ */
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/regulator/consumer.h>
+
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+#define AW9523_MAX_FUNCS		2
+#define AW9523_NUM_PORTS		2
+#define AW9523_PINS_PER_PORT		8
+
+/*
+ * HW needs at least 20uS for reset and at least 1-2uS to recover from
+ * reset, but we have to account for eventual board quirks, if any:
+ * for this reason, keep reset asserted for 50uS and wait for 20uS
+ * to recover from the reset.
+ */
+#define AW9523_HW_RESET_US		50
+#define AW9523_HW_RESET_RECOVERY_US	20
+
+/* Port 0: P0_0...P0_7 - Port 1: P1_0...P1_7 */
+#define AW9523_PIN_TO_PORT(pin)		(pin >> 3)
+#define AW9523_REG_IN_STATE(pin)	(0x00 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_OUT_STATE(pin)	(0x02 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CONF_STATE(pin)	(0x04 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_INTR_DIS(pin)	(0x06 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CHIPID		0x10
+#define AW9523_VAL_EXPECTED_CHIPID	0x23
+
+#define AW9523_REG_GCR			0x11
+#define AW9523_GCR_ISEL_MASK		GENMASK(0, 1)
+#define AW9523_GCR_GPOMD_MASK		BIT(4)
+
+#define AW9523_REG_PORT_MODE(pin)	(0x12 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_SOFT_RESET		0x7f
+#define AW9523_VAL_RESET		0x00
+
+/*
+ * struct aw9523_irq - Interrupt controller structure
+ * @lock: mutex locking for the irq bus
+ * @irqchip: structure holding irqchip params
+ * @cached_gpio: stores the previous gpio status for bit comparison
+ */
+struct aw9523_irq {
+	struct mutex lock;
+	struct irq_chip *irqchip;
+	u16 cached_gpio;
+};
+
+/*
+ * struct aw9523_pinmux - Pin mux params
+ * @name: Name of the mux
+ * @grps: Groups of the mux
+ * @num_grps: Number of groups (sizeof array grps)
+ */
+struct aw9523_pinmux {
+	const char *name;
+	const char * const *grps;
+	const u8 num_grps;
+};
+
+/*
+ * struct aw9523 - Main driver structure
+ * @dev: device handle
+ * @regmap: regmap handle for current device
+ * @i2c_lock: Mutex lock for i2c operations
+ * @reset_gpio: Hardware reset (RSTN) signal GPIO
+ * @vio_vreg: VCC regulator (Optional)
+ * @pctl: pinctrl handle for current device
+ * @gpio: structure holding gpiochip params
+ * @irq: Interrupt controller structure
+ */
+struct aw9523 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex i2c_lock;
+	struct gpio_desc *reset_gpio;
+	struct regulator *vio_vreg;
+	struct pinctrl_dev *pctl;
+	struct gpio_chip gpio;
+	struct aw9523_irq *irq;
+};
+
+static const struct pinctrl_pin_desc aw9523_pins[] = {
+	/* Port 0 */
+	PINCTRL_PIN(0, "gpio0"),
+	PINCTRL_PIN(1, "gpio1"),
+	PINCTRL_PIN(2, "gpio2"),
+	PINCTRL_PIN(3, "gpio3"),
+	PINCTRL_PIN(4, "gpio4"),
+	PINCTRL_PIN(5, "gpio5"),
+	PINCTRL_PIN(6, "gpio6"),
+	PINCTRL_PIN(7, "gpio7"),
+
+	/* Port 1 */
+	PINCTRL_PIN(8, "gpio8"),
+	PINCTRL_PIN(9, "gpio9"),
+	PINCTRL_PIN(10, "gpio10"),
+	PINCTRL_PIN(11, "gpio11"),
+	PINCTRL_PIN(12, "gpio12"),
+	PINCTRL_PIN(13, "gpio13"),
+	PINCTRL_PIN(14, "gpio14"),
+	PINCTRL_PIN(15, "gpio15"),
+};
+
+static int aw9523_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(aw9523_pins);
+}
+
+static const char *aw9523_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						 unsigned int selector)
+{
+	return aw9523_pins[selector].name;
+}
+
+static int aw9523_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					 unsigned int selector,
+					 const unsigned int **pins,
+					 unsigned int *num_pins)
+{
+	*pins = &aw9523_pins[selector].number;
+	*num_pins = 1;
+	return 0;
+}
+
+static const struct pinctrl_ops aw9523_pinctrl_ops = {
+	.get_groups_count = aw9523_pinctrl_get_groups_count,
+	.get_group_pins = aw9523_pinctrl_get_group_pins,
+	.get_group_name = aw9523_pinctrl_get_group_name,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static const char * const gpio_pwm_groups[] = {
+	"gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5",
+	"gpio6", "gpio7", "gpio8", "gpio9", "gpio10", "gpio11",
+	"gpio12", "gpio13", "gpio14", "gpio15"
+};
+
+/* Warning: Do NOT reorder this array */
+static const struct aw9523_pinmux aw9523_pmx[] = {
+	{
+		.name = "pwm",
+		.grps = gpio_pwm_groups,
+		.num_grps = ARRAY_SIZE(gpio_pwm_groups),
+	},
+	{
+		.name = "gpio",
+		.grps = gpio_pwm_groups,
+		.num_grps = ARRAY_SIZE(gpio_pwm_groups),
+	},
+};
+
+static int aw9523_pmx_get_funcs_count(struct pinctrl_dev *pctl)
+{
+	return ARRAY_SIZE(aw9523_pmx);
+}
+
+static const char *aw9523_pmx_get_fname(struct pinctrl_dev *pctl,
+					unsigned int sel)
+{
+	return aw9523_pmx[sel].name;
+}
+
+static int aw9523_pmx_get_groups(struct pinctrl_dev *pctl, unsigned int sel,
+				 const char * const **groups,
+				 unsigned int * const num_groups)
+{
+	*groups = aw9523_pmx[sel].grps;
+	*num_groups = aw9523_pmx[sel].num_grps;
+	return 0;
+}
+
+static int aw9523_pmx_set_mux(struct pinctrl_dev *pctl, unsigned int fsel,
+			      unsigned int grp)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctl);
+	int ret, pin = aw9523_pins[grp].number % AW9523_PINS_PER_PORT;
+
+	if (fsel >= ARRAY_SIZE(aw9523_pmx))
+		return -EINVAL;
+
+	/*
+	 * This maps directly to the aw9523_pmx array: programming a
+	 * high bit means "gpio" and a low bit means "pwm".
+	 */
+	mutex_lock(&awi->i2c_lock);
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_PORT_MODE(pin),
+				 BIT(pin), (fsel ? BIT(pin) : 0));
+	mutex_unlock(&awi->i2c_lock);
+	return ret;
+}
+
+static const struct pinmux_ops aw9523_pinmux_ops = {
+	.get_functions_count	= aw9523_pmx_get_funcs_count,
+	.get_function_name	= aw9523_pmx_get_fname,
+	.get_function_groups	= aw9523_pmx_get_groups,
+	.set_mux		= aw9523_pmx_set_mux,
+};
+
+static int aw9523_pcfg_param_to_reg(enum pin_config_param pcp, int pin, u8 *r)
+{
+	u8 reg;
+
+	switch (pcp) {
+	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		reg = AW9523_REG_IN_STATE(pin);
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		reg = AW9523_REG_GCR;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		reg = AW9523_REG_CONF_STATE(pin);
+		break;
+	case PIN_CONFIG_OUTPUT:
+		reg = AW9523_REG_OUT_STATE(pin);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+	*r = reg;
+
+	return 0;
+}
+
+static int aw9523_pconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	int regbit = pin % AW9523_PINS_PER_PORT;
+	unsigned int val;
+	u8 reg;
+	int rc;
+
+	rc = aw9523_pcfg_param_to_reg(param, pin, &reg);
+	if (rc)
+		return rc;
+
+	mutex_lock(&awi->i2c_lock);
+	rc = regmap_read(awi->regmap, reg, &val);
+	mutex_unlock(&awi->i2c_lock);
+	if (rc)
+		return rc;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_INPUT_ENABLE:
+	case PIN_CONFIG_OUTPUT:
+		val &= BIT(regbit);
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		val &= BIT(regbit);
+		val = !val;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (pin >= AW9523_PINS_PER_PORT)
+			val = 0;
+		else
+			val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		if (pin >= AW9523_PINS_PER_PORT)
+			val = 1;
+		else
+			val = FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+	if (val < 1)
+		return -EINVAL;
+
+	*config = pinconf_to_config_packed(param, !!val);
+
+	return rc;
+}
+
+static int aw9523_pconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	int regbit = pin % AW9523_PINS_PER_PORT;
+	u32 arg;
+	u8 reg;
+	unsigned int mask, val;
+	int i, rc;
+
+	mutex_lock(&awi->i2c_lock);
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		rc = aw9523_pcfg_param_to_reg(param, pin, &reg);
+		if (rc)
+			goto end;
+
+		switch (param) {
+		case PIN_CONFIG_OUTPUT:
+			/* First, enable pin output */
+			rc = regmap_update_bits(awi->regmap,
+						AW9523_REG_CONF_STATE(pin),
+						BIT(regbit), 0);
+			if (rc)
+				goto end;
+
+			/* Then, fall through to config output level */
+			fallthrough;
+		case PIN_CONFIG_OUTPUT_ENABLE:
+			arg = !arg;
+			fallthrough;
+		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+		case PIN_CONFIG_BIAS_PULL_UP:
+		case PIN_CONFIG_INPUT_ENABLE:
+			mask = BIT(regbit);
+			val = arg ? BIT(regbit) : 0;
+			break;
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			/* Open-Drain is supported only on port 0 */
+			if (pin >= AW9523_PINS_PER_PORT) {
+				rc = -ENOTSUPP;
+				goto end;
+			}
+			mask = AW9523_GCR_GPOMD_MASK;
+			val = 0;
+			break;
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			/* Port 1 is always Push-Pull */
+			if (pin >= AW9523_PINS_PER_PORT) {
+				mask = 0;
+				val = 0;
+				continue;
+			}
+			mask = AW9523_GCR_GPOMD_MASK;
+			val = AW9523_GCR_GPOMD_MASK;
+			break;
+		default:
+			rc = -ENOTSUPP;
+			goto end;
+		}
+
+		rc = regmap_update_bits(awi->regmap, reg, mask, val);
+		if (rc)
+			goto end;
+	}
+end:
+	mutex_unlock(&awi->i2c_lock);
+	return rc;
+}
+
+static const struct pinconf_ops aw9523_pinconf_ops = {
+	.pin_config_get = aw9523_pconf_get,
+	.pin_config_set = aw9523_pconf_set,
+	.is_generic = true,
+};
+
+/*
+ * aw9523_get_pin_direction - Get pin direction
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @n:   pin index in port register
+ *
+ * Return: Pin direction for success or negative number for error
+ */
+static int aw9523_get_pin_direction(struct regmap *regmap, u8 pin, u8 n)
+{
+	int ret;
+
+	ret = regmap_test_bits(regmap, AW9523_REG_CONF_STATE(pin), BIT(n));
+	if (ret < 0)
+		return ret;
+
+	return ret ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+/*
+ * aw9523_get_port_state - Get input or output state for entire port
+ * @regmap: Regmap structure
+ * @pin:    gpiolib pin number
+ * @regbit: hw pin index, used to retrieve port number
+ * @state:  returned port state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int aw9523_get_port_state(struct regmap *regmap, u8 pin,
+				   u8 regbit, unsigned int *state)
+{
+	u8 reg;
+	int dir;
+
+	dir = aw9523_get_pin_direction(regmap, pin, regbit);
+	if (dir < 0)
+		return dir;
+
+	if (dir == GPIO_LINE_DIRECTION_IN)
+		reg = AW9523_REG_IN_STATE(pin);
+	else
+		reg = AW9523_REG_OUT_STATE(pin);
+
+	return regmap_read(regmap, reg, state);
+}
+
+static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+	switch (type) {
+	case IRQ_TYPE_NONE:
+	case IRQ_TYPE_LEVEL_MASK:
+	case IRQ_TYPE_LEVEL_HIGH:
+	case IRQ_TYPE_LEVEL_LOW:
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
+/*
+ * aw9523_irq_mask - Mask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to mask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_mask(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+
+	regmap_update_bits(awi->regmap,
+			   AW9523_REG_INTR_DIS(d->hwirq),
+			   BIT(n), BIT(n));
+}
+
+/*
+ * aw9523_irq_unmask - Unmask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to unmask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_unmask(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+
+	regmap_update_bits(awi->regmap,
+			   AW9523_REG_INTR_DIS(d->hwirq),
+			   BIT(n), 0);
+}
+
+static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id)
+{
+	struct aw9523 *awi = (struct aw9523 *)dev_id;
+	unsigned long n, val = 0;
+	unsigned long changed_gpio;
+	unsigned int tmp, port_pin, i, ret;
+
+	for (i = 0; i < AW9523_NUM_PORTS; i++) {
+		port_pin = i * AW9523_PINS_PER_PORT;
+		ret = regmap_read(awi->regmap,
+				  AW9523_REG_IN_STATE(port_pin),
+				  &tmp);
+		if (ret)
+			return ret;
+		val |= (u8)tmp << (i * 8);
+	}
+
+	/* Handle GPIO input release interrupt as well */
+	changed_gpio = awi->irq->cached_gpio ^ val;
+	awi->irq->cached_gpio = val;
+
+	/*
+	 * To avoid up to four *slow* i2c reads from any driver hooked
+	 * up to our interrupts, just check for the irq_find_mapping
+	 * result: if the interrupt is not mapped, then we don't want
+	 * to care about it.
+	 */
+	for_each_set_bit(n, &changed_gpio, awi->gpio.ngpio) {
+		tmp = irq_find_mapping(awi->gpio.irq.domain, n);
+		if (tmp <= 0)
+			continue;
+		handle_nested_irq(tmp);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * aw9523_irq_bus_lock - Grab lock for interrupt operation
+ * @d: irq data
+ */
+static void aw9523_irq_bus_lock(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+
+	mutex_lock(&awi->irq->lock);
+	regcache_cache_only(awi->regmap, true);
+}
+
+/*
+ * aw9523_irq_bus_sync_unlock - Synchronize state and unlock
+ * @d: irq data
+ *
+ * Writes the interrupt mask bits (found in the bit map) to the
+ * hardware, then unlocks the bus.
+ */
+static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+
+	regcache_cache_only(awi->regmap, false);
+	regcache_sync(awi->regmap);
+	mutex_unlock(&awi->irq->lock);
+}
+
+static int aw9523_gpio_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 regbit = offset % AW9523_PINS_PER_PORT;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+	ret = aw9523_get_pin_direction(awi->regmap, offset, regbit);
+	mutex_unlock(&awi->i2c_lock);
+
+	return ret;
+}
+
+static int aw9523_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 regbit = offset % AW9523_PINS_PER_PORT;
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+	ret = aw9523_get_port_state(awi->regmap, offset, regbit, &val);
+	mutex_unlock(&awi->i2c_lock);
+	if (ret)
+		return ret;
+
+	return !!(val & BIT(regbit));
+}
+
+/**
+ * _aw9523_gpio_get_multiple - Get I/O state for an entire port
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @regbit: hw pin index, used to retrieve port number
+ * @state: returned port I/O state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int _aw9523_gpio_get_multiple(struct aw9523 *awi, u8 regbit,
+				     u8 *state, u8 mask)
+{
+	u32 dir_in, val;
+	u8 m;
+	int ret;
+
+	/* Registers are 8-bits wide */
+	ret = regmap_read(awi->regmap, AW9523_REG_CONF_STATE(regbit), &dir_in);
+	if (ret)
+		return ret;
+	*state = 0;
+
+	m = mask & dir_in;
+	if (m) {
+		ret = regmap_read(awi->regmap, AW9523_REG_IN_STATE(regbit),
+				  &val);
+		if (ret)
+			return ret;
+		*state |= (u8)val & m;
+	}
+
+	m = mask & ~dir_in;
+	if (m) {
+		ret = regmap_read(awi->regmap, AW9523_REG_OUT_STATE(regbit),
+				  &val);
+		if (ret)
+			return ret;
+		*state |= (u8)val & m;
+	}
+
+	return 0;
+}
+
+static int aw9523_gpio_get_multiple(struct gpio_chip *chip,
+				    unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 m, state = 0;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+
+	/* Port 0 (gpio 0-7) */
+	m = *mask & U8_MAX;
+	if (m) {
+		ret = _aw9523_gpio_get_multiple(awi, 0, &state, m);
+		if (ret)
+			goto out;
+	}
+	*bits = state;
+
+	/* Port 1 (gpio 8-15) */
+	m = (*mask >> 8) & U8_MAX;
+	if (m) {
+		ret = _aw9523_gpio_get_multiple(awi, AW9523_PINS_PER_PORT,
+						&state, m);
+		if (ret)
+			goto out;
+
+		*bits |= (state << 8);
+	}
+out:
+	mutex_unlock(&awi->i2c_lock);
+	return ret;
+}
+
+static void aw9523_gpio_set_multiple(struct gpio_chip *chip,
+				    unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 mask_lo, mask_hi, bits_lo, bits_hi;
+	unsigned int reg;
+	int ret = 0;
+
+	mask_lo = *mask & U8_MAX;
+	mask_hi = (*mask >> 8) & U8_MAX;
+	mutex_lock(&awi->i2c_lock);
+	if (mask_hi) {
+		reg = AW9523_REG_OUT_STATE(AW9523_PINS_PER_PORT);
+		bits_hi = (*bits >> 8) & U8_MAX;
+
+		ret = regmap_write_bits(awi->regmap, reg, mask_hi, bits_hi);
+		if (ret) {
+			dev_warn(awi->dev, "Cannot write port1 out level\n");
+			goto out;
+		}
+	}
+	if (mask_lo) {
+		reg = AW9523_REG_OUT_STATE(0);
+		bits_lo = *bits & U8_MAX;
+		ret = regmap_write_bits(awi->regmap, reg, mask_lo, bits_lo);
+		if (ret)
+			dev_warn(awi->dev, "Cannot write port0 out level\n");
+	}
+out:
+	mutex_unlock(&awi->i2c_lock);
+}
+
+static void aw9523_gpio_set(struct gpio_chip *chip,
+			    unsigned int offset, int value)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 regbit = offset % AW9523_PINS_PER_PORT;
+
+	mutex_lock(&awi->i2c_lock);
+	regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+			   BIT(regbit), value ? BIT(regbit) : 0);
+	mutex_unlock(&awi->i2c_lock);
+}
+
+
+static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 regbit = offset % AW9523_PINS_PER_PORT;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_CONF_STATE(offset),
+				 BIT(regbit), BIT(regbit));
+	mutex_unlock(&awi->i2c_lock);
+
+	return ret;
+}
+
+static int aw9523_direction_output(struct gpio_chip *chip,
+				   unsigned int offset, int value)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 regbit = offset % AW9523_PINS_PER_PORT;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+				 BIT(regbit), value ? BIT(regbit) : 0);
+	if (ret)
+		goto end;
+
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_CONF_STATE(offset),
+				 BIT(regbit), 0);
+end:
+	mutex_unlock(&awi->i2c_lock);
+	return ret;
+}
+
+static int aw9523_drive_reset_gpio(struct aw9523 *awi)
+{
+	unsigned int chip_id;
+	int ret;
+
+	/*
+	 * If the chip is already configured for any reason, then we
+	 * will probably succeed in sending the soft reset signal to
+	 * the hardware through I2C: this operation takes less time
+	 * compared to a full HW reset and it gives the same results.
+	 */
+	ret = regmap_write(awi->regmap, AW9523_REG_SOFT_RESET, 0);
+	if (ret == 0)
+		goto done;
+
+	dev_dbg(awi->dev, "Cannot execute soft reset: trying hard reset\n");
+	ret = gpiod_direction_output(awi->reset_gpio, 0);
+	if (ret)
+		return ret;
+
+	/* The reset pulse has to be longer than 20uS due to deglitch */
+	usleep_range(AW9523_HW_RESET_US, AW9523_HW_RESET_US + 1);
+
+	ret = gpiod_direction_output(awi->reset_gpio, 1);
+	if (ret)
+		return ret;
+done:
+	/* The HW needs at least 1uS to reliably recover after reset */
+	usleep_range(AW9523_HW_RESET_RECOVERY_US,
+		     AW9523_HW_RESET_RECOVERY_US + 1);
+
+	/* Check the ChipID */
+	ret = regmap_read(awi->regmap, AW9523_REG_CHIPID, &chip_id);
+	if (ret) {
+		dev_err(awi->dev, "Cannot read Chip ID: %d\n", ret);
+		return ret;
+	}
+	if (chip_id != AW9523_VAL_EXPECTED_CHIPID) {
+		dev_err(awi->dev, "Bad ChipID; read 0x%x, expected 0x%x\n",
+			chip_id, AW9523_VAL_EXPECTED_CHIPID);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int aw9523_hw_reset(struct aw9523 *awi)
+{
+	int ret, max_retries = 2;
+
+	/* Sometimes the chip needs more than one reset cycle */
+	do {
+		ret = aw9523_drive_reset_gpio(awi);
+		if (ret == 0)
+			break;
+		max_retries--;
+	} while (max_retries);
+
+	return ret;
+}
+
+static int aw9523_init_gpiochip(struct aw9523 *awi, unsigned int npins)
+{
+	struct device *dev = awi->dev;
+	struct gpio_chip *gpiochip = &awi->gpio;
+
+	gpiochip->label = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!gpiochip->label)
+		return -ENOMEM;
+
+	gpiochip->base = -1;
+	gpiochip->ngpio = npins;
+	gpiochip->get_direction = aw9523_gpio_get_direction;
+	gpiochip->direction_input = aw9523_direction_input;
+	gpiochip->direction_output = aw9523_direction_output;
+	gpiochip->get = aw9523_gpio_get;
+	gpiochip->get_multiple = aw9523_gpio_get_multiple;
+	gpiochip->set = aw9523_gpio_set;
+	gpiochip->set_multiple = aw9523_gpio_set_multiple;
+	gpiochip->set_config = gpiochip_generic_config;
+	gpiochip->parent = dev;
+	gpiochip->of_node = dev->of_node;
+	gpiochip->owner = THIS_MODULE;
+	gpiochip->can_sleep = false;
+
+	return 0;
+}
+
+static int aw9523_init_irq(struct aw9523 *awi, int irq)
+{
+	struct device *dev = awi->dev;
+	struct gpio_irq_chip *gpioirq;
+	struct irq_chip *irqchip;
+	int ret;
+
+	if (!device_property_read_bool(dev, "interrupt-controller"))
+		return 0;
+
+	irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
+	if (!irqchip)
+		return -ENOMEM;
+
+	awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
+	if (!awi->irq)
+		return -ENOMEM;
+
+	irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!irqchip->name)
+		return -ENOMEM;
+
+	irqchip->irq_mask = aw9523_irq_mask;
+	irqchip->irq_unmask = aw9523_irq_unmask;
+	irqchip->irq_bus_lock = aw9523_irq_bus_lock;
+	irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
+	irqchip->irq_set_type = aw9523_gpio_irq_type;
+	awi->irq->irqchip = irqchip;
+	mutex_init(&awi->irq->lock);
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
+					IRQF_ONESHOT, dev_name(dev), awi);
+	if (ret) {
+		dev_err(dev, "Failed to request irq %d\n", irq);
+		return ret;
+	}
+
+	gpioirq = &awi->gpio.irq;
+	gpioirq->chip = irqchip;
+	gpioirq->parent_handler = NULL;
+	gpioirq->num_parents = 0;
+	gpioirq->parents = NULL;
+	gpioirq->default_type = IRQ_TYPE_LEVEL_MASK;
+	gpioirq->handler = handle_simple_irq;
+	gpioirq->threaded = true;
+	gpioirq->first = 0;
+
+	return 0;
+}
+
+static bool aw9523_is_reg_hole(unsigned int reg)
+{
+	return (reg > AW9523_REG_PORT_MODE(AW9523_PINS_PER_PORT) &&
+		reg < AW9523_REG_SOFT_RESET) ||
+	       (reg > AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT) &&
+		reg < AW9523_REG_CHIPID);
+}
+
+static bool aw9523_readable_reg(struct device *dev, unsigned int reg)
+{
+	/* All available registers (minus holes) can be read */
+	return !aw9523_is_reg_hole(reg);
+}
+
+static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return aw9523_is_reg_hole(reg) ||
+	       reg == AW9523_REG_IN_STATE(0) ||
+	       reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
+	       reg == AW9523_REG_CHIPID ||
+	       reg == AW9523_REG_SOFT_RESET;
+}
+
+static bool aw9523_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return !aw9523_is_reg_hole(reg) && reg != AW9523_REG_CHIPID;
+}
+
+static bool aw9523_precious_reg(struct device *dev, unsigned int reg)
+{
+	/* Reading AW9523_REG_IN_STATE clears interrupt status */
+	return aw9523_is_reg_hole(reg) ||
+	       reg == AW9523_REG_IN_STATE(0) ||
+	       reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT);
+}
+
+static const struct regmap_config aw9523_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+
+	.precious_reg = aw9523_precious_reg,
+	.readable_reg = aw9523_readable_reg,
+	.volatile_reg = aw9523_volatile_reg,
+	.writeable_reg = aw9523_writeable_reg,
+
+	.cache_type = REGCACHE_FLAT,
+	.disable_locking = true,
+
+	.num_reg_defaults_raw = AW9523_REG_SOFT_RESET,
+};
+
+static int aw9523_hw_init(struct aw9523 *awi)
+{
+	u8 p1_pin = AW9523_PINS_PER_PORT;
+	unsigned int val;
+	int ret;
+
+	/* No register caching during initialization */
+	regcache_cache_bypass(awi->regmap, true);
+
+	/* Bring up the chip */
+	ret = aw9523_hw_reset(awi);
+	if (ret) {
+		dev_err(awi->dev, "HW Reset failed: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * This is the expected chip and it is running: it's time to
+	 * set a safe default configuration in case the user doesn't
+	 * configure (all of the available) pins in this chip.
+	 * P.S.: The writes order doesn't matter.
+	 */
+
+	/* Set all pins as GPIO */
+	ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+
+	/* Set Open-Drain mode on Port 0 (Port 1 is always P-P) */
+	ret = regmap_write(awi->regmap, AW9523_REG_GCR, 0);
+	if (ret)
+		return ret;
+
+	/* Set all pins as inputs */
+	ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+
+	/* Disable all interrupts to avoid unreasoned wakeups */
+	ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+
+	/* Clear setup-generated interrupts by performing a port state read */
+	ret = aw9523_get_port_state(awi->regmap, 0, 0, &val);
+	if (ret)
+		return ret;
+	ret = aw9523_get_port_state(awi->regmap, p1_pin, 0, &val);
+	if (ret)
+		return ret;
+
+	/* Everything went fine: activate and reinitialize register cache */
+	regcache_cache_bypass(awi->regmap, false);
+	return regmap_reinit_cache(awi->regmap, &aw9523_regmap);
+}
+
+static int aw9523_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct pinctrl_desc *pdesc;
+	struct aw9523 *awi;
+	int ret;
+
+	awi = devm_kzalloc(dev, sizeof(*awi), GFP_KERNEL);
+	if (!awi)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, awi);
+
+	awi->dev = dev;
+	awi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(awi->reset_gpio))
+		return PTR_ERR(awi->reset_gpio);
+	gpiod_set_consumer_name(awi->reset_gpio, "aw9523 reset");
+
+	awi->regmap = devm_regmap_init_i2c(client, &aw9523_regmap);
+	if (IS_ERR(awi->regmap))
+		return PTR_ERR(awi->regmap);
+
+	awi->vio_vreg = devm_regulator_get_optional(dev, "vio");
+	if (IS_ERR(awi->vio_vreg)) {
+		if (PTR_ERR(awi->vio_vreg) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		awi->vio_vreg = NULL;
+	} else {
+		ret = regulator_enable(awi->vio_vreg);
+		if (ret)
+			return ret;
+	}
+
+	mutex_init(&awi->i2c_lock);
+	lockdep_set_subclass(&awi->i2c_lock,
+			     i2c_adapter_depth(client->adapter));
+
+	pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+
+	ret = aw9523_hw_init(awi);
+	if (ret)
+		goto err_disable_vregs;
+
+	pdesc->name = dev_name(dev);
+	pdesc->owner = THIS_MODULE;
+	pdesc->pctlops = &aw9523_pinctrl_ops;
+	pdesc->pmxops  = &aw9523_pinmux_ops;
+	pdesc->confops = &aw9523_pinconf_ops;
+	pdesc->pins = aw9523_pins;
+	pdesc->npins = ARRAY_SIZE(aw9523_pins);
+
+	ret = aw9523_init_gpiochip(awi, pdesc->npins);
+	if (ret)
+		goto err_disable_vregs;
+
+	if (client->irq) {
+		ret = aw9523_init_irq(awi, client->irq);
+		if (ret)
+			goto err_disable_vregs;
+	}
+
+	awi->pctl = devm_pinctrl_register(dev, pdesc, awi);
+	if (IS_ERR(awi->pctl)) {
+		ret = PTR_ERR(awi->pctl);
+		dev_err(dev, "Cannot register pinctrl: %d", ret);
+		goto err_disable_vregs;
+	}
+
+	ret = devm_gpiochip_add_data(dev, &awi->gpio, awi);
+	if (ret)
+		goto err_disable_vregs;
+
+	return ret;
+
+err_disable_vregs:
+	if (awi->vio_vreg)
+		regulator_disable(awi->vio_vreg);
+	mutex_destroy(&awi->i2c_lock);
+	return ret;
+}
+
+static int aw9523_remove(struct i2c_client *client)
+{
+	struct aw9523 *awi = i2c_get_clientdata(client);
+	int ret;
+
+	if (!awi)
+		return 0;
+
+	/*
+	 * If the chip VIO is connected to a regulator that we can turn
+	 * off, life is easy... otherwise, reinitialize the chip and
+	 * set the pins to hardware defaults before removing the driver
+	 * to leave it in a clean, safe and predictable state.
+	 */
+	if (awi->vio_vreg) {
+		regulator_disable(awi->vio_vreg);
+	} else {
+		mutex_lock(&awi->i2c_lock);
+		ret = aw9523_hw_init(awi);
+		mutex_unlock(&awi->i2c_lock);
+		if (ret)
+			return ret;
+	}
+
+	mutex_destroy(&awi->i2c_lock);
+	return 0;
+}
+
+static const struct i2c_device_id aw9523_i2c_id_table[] = {
+	{ "aw9523_i2c", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, aw9523_i2c_id_table);
+
+static const struct of_device_id of_aw9523_i2c_match[] = {
+	{ .compatible = "awinic,aw9523-pinctrl", },
+};
+MODULE_DEVICE_TABLE(of, of_aw9523_i2c_match);
+
+static struct i2c_driver aw9523_driver = {
+	.driver = {
+		.name = "aw9523-pinctrl",
+		.of_match_table = of_aw9523_i2c_match,
+	},
+	.probe = aw9523_probe,
+	.remove = aw9523_remove,
+	.id_table = aw9523_i2c_id_table,
+};
+module_i2c_driver(aw9523_driver);
+
+MODULE_DESCRIPTION("Awinic AW9523 I2C GPIO Expander driver");
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:aw9523");
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
  2021-01-11 18:29 [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
@ 2021-01-11 18:29 ` AngeloGioacchino Del Regno
  2021-01-12 21:52   ` Rob Herring
  2021-01-13  2:41   ` Rob Herring
  2021-01-18 13:19 ` [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander Linus Walleij
  1 sibling, 2 replies; 10+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-11 18:29 UTC (permalink / raw)
  To: linus.walleij
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, linux-gpio, devicetree, robh+dt,
	AngeloGioacchino Del Regno

Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 .../pinctrl/awinic,aw9523-pinctrl.yaml        | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
new file mode 100644
index 000000000000..a705c05bb5a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/awinic,aw9523-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW9523/AW9523B I2C GPIO Expander
+
+maintainers:
+  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
+
+description: |
+  The Awinic AW9523/AW9523B I2C GPIO Expander featuring 16 multi-function
+  I/O, 256 steps PWM mode and interrupt support.
+
+properties:
+  compatible:
+    const: awinic,aw9523-pinctrl
+
+  reg:
+    maxItems: 1
+
+  '#gpio-cells':
+    description: |
+      Specifying the pin number and flags, as defined in
+      include/dt-bindings/gpio/gpio.h
+    const: 2
+
+  gpio-controller: true
+
+  gpio-ranges:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  interrupts:
+    maxItems: 1
+    description: Specifies the INTN pin IRQ.
+
+  '#interrupt-cells':
+    description:
+      Specifies the PIN numbers and Flags, as defined in defined in
+      include/dt-bindings/interrupt-controller/irq.h
+    const: 2
+
+#PIN CONFIGURATION NODES
+patternProperties:
+  '^.*$':
+    if:
+      type: object
+      $ref: "/schemas/pinctrl/pincfg-node.yaml"
+    then:
+      properties:
+        pins:
+          description:
+            List of gpio pins affected by the properties specified in
+            this subnode.
+          items:
+            pattern: "^gpio([0-9]|1[0-5])$"
+          minItems: 1
+          maxItems: 16
+
+        function:
+          description:
+            Specify the alternative function to be configured for the
+            specified pins.
+
+          enum: [ gpio, pwm ]
+
+        bias-disable: true
+        bias-pull-down: true
+        bias-pull-up: true
+        drive-open-drain: true
+        drive-push-pull: true
+        input-enable: true
+        output-high: true
+        output-low: true
+
+      required:
+        - pins
+        - function
+
+      additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - '#gpio-cells'
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c_node {
+        gpio-expander@58 {
+                compatible = "awinic,aw9523-pinctrl";
+                reg = <0x58>;
+                interrupt-parent = <&tlmm>;
+                interrupts = <50 IRQ_TYPE_EDGE_FALLING>;
+                gpio-controller;
+                #gpio-cells = <2>;
+                gpio-ranges = <&tlmm 0 0 16>;
+                interrupt-controller;
+                #interrupt-cells = <2>;
+                reset-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+        };
+    };
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
  2021-01-11 18:29 ` [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
@ 2021-01-12 21:52   ` Rob Herring
  2021-01-13  2:41   ` Rob Herring
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-01-12 21:52 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: martin.botka, linux-kernel, devicetree, linus.walleij,
	marijn.suijten, linux-gpio, robh+dt, phone-devel, konrad.dybcio

On Mon, 11 Jan 2021 19:29:28 +0100, AngeloGioacchino Del Regno wrote:
> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../pinctrl/awinic,aw9523-pinctrl.yaml        | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dts:25.21-34: Warning (reg_format): /example-0/i2c_node/gpio-expander@58:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dts:23.30-34.15: Warning (avoid_default_addr_size): /example-0/i2c_node/gpio-expander@58: Relying on default #address-cells value
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dts:23.30-34.15: Warning (avoid_default_addr_size): /example-0/i2c_node/gpio-expander@58: Relying on default #size-cells value
Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dt.yaml: Warning (unique_unit_address): Failed prerequisite 'avoid_default_addr_size'

See https://patchwork.ozlabs.org/patch/1424757

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
  2021-01-11 18:29 ` [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
  2021-01-12 21:52   ` Rob Herring
@ 2021-01-13  2:41   ` Rob Herring
  2021-01-13 12:30     ` AngeloGioacchino Del Regno
  1 sibling, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-01-13  2:41 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: linus.walleij, linux-kernel, konrad.dybcio, marijn.suijten,
	martin.botka, phone-devel, linux-gpio, devicetree

On Mon, Jan 11, 2021 at 07:29:28PM +0100, AngeloGioacchino Del Regno wrote:
> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../pinctrl/awinic,aw9523-pinctrl.yaml        | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> new file mode 100644
> index 000000000000..a705c05bb5a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/awinic,aw9523-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Awinic AW9523/AW9523B I2C GPIO Expander
> +
> +maintainers:
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> +
> +description: |
> +  The Awinic AW9523/AW9523B I2C GPIO Expander featuring 16 multi-function
> +  I/O, 256 steps PWM mode and interrupt support.
> +
> +properties:
> +  compatible:
> +    const: awinic,aw9523-pinctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#gpio-cells':
> +    description: |
> +      Specifying the pin number and flags, as defined in
> +      include/dt-bindings/gpio/gpio.h
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  gpio-ranges:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Specifies the INTN pin IRQ.
> +
> +  '#interrupt-cells':
> +    description:
> +      Specifies the PIN numbers and Flags, as defined in defined in
> +      include/dt-bindings/interrupt-controller/irq.h
> +    const: 2
> +
> +#PIN CONFIGURATION NODES
> +patternProperties:
> +  '^.*$':
> +    if:
> +      type: object
> +      $ref: "/schemas/pinctrl/pincfg-node.yaml"
> +    then:

I wish people would stop copying this if/then hack...

For new bindings, just name your nodes something sensible you can match 
on like '-pins$'.

> +      properties:
> +        pins:
> +          description:
> +            List of gpio pins affected by the properties specified in
> +            this subnode.
> +          items:
> +            pattern: "^gpio([0-9]|1[0-5])$"
> +          minItems: 1
> +          maxItems: 16
> +
> +        function:
> +          description:
> +            Specify the alternative function to be configured for the
> +            specified pins.
> +
> +          enum: [ gpio, pwm ]
> +
> +        bias-disable: true
> +        bias-pull-down: true
> +        bias-pull-up: true
> +        drive-open-drain: true
> +        drive-push-pull: true
> +        input-enable: true
> +        output-high: true
> +        output-low: true
> +
> +      required:
> +        - pins
> +        - function
> +
> +      additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c_node {
> +        gpio-expander@58 {
> +                compatible = "awinic,aw9523-pinctrl";
> +                reg = <0x58>;
> +                interrupt-parent = <&tlmm>;
> +                interrupts = <50 IRQ_TYPE_EDGE_FALLING>;
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                gpio-ranges = <&tlmm 0 0 16>;
> +                interrupt-controller;
> +                #interrupt-cells = <2>;
> +                reset-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
> +        };
> +    };
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
  2021-01-13  2:41   ` Rob Herring
@ 2021-01-13 12:30     ` AngeloGioacchino Del Regno
  2021-01-14 22:42       ` Rob Herring
  0 siblings, 1 reply; 10+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-13 12:30 UTC (permalink / raw)
  To: Rob Herring
  Cc: linus.walleij, linux-kernel, konrad.dybcio, marijn.suijten,
	martin.botka, phone-devel, linux-gpio, devicetree

Il 13/01/21 03:41, Rob Herring ha scritto:
> On Mon, Jan 11, 2021 at 07:29:28PM +0100, AngeloGioacchino Del Regno wrote:
>> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   .../pinctrl/awinic,aw9523-pinctrl.yaml        | 112 ++++++++++++++++++
>>   1 file changed, 112 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..a705c05bb5a2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
>> @@ -0,0 +1,112 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/awinic,aw9523-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Awinic AW9523/AW9523B I2C GPIO Expander
>> +
>> +maintainers:
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> +
>> +description: |
>> +  The Awinic AW9523/AW9523B I2C GPIO Expander featuring 16 multi-function
>> +  I/O, 256 steps PWM mode and interrupt support.
>> +
>> +properties:
>> +  compatible:
>> +    const: awinic,aw9523-pinctrl
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#gpio-cells':
>> +    description: |
>> +      Specifying the pin number and flags, as defined in
>> +      include/dt-bindings/gpio/gpio.h
>> +    const: 2
>> +
>> +  gpio-controller: true
>> +
>> +  gpio-ranges:
>> +    maxItems: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Specifies the INTN pin IRQ.
>> +
>> +  '#interrupt-cells':
>> +    description:
>> +      Specifies the PIN numbers and Flags, as defined in defined in
>> +      include/dt-bindings/interrupt-controller/irq.h
>> +    const: 2
>> +
>> +#PIN CONFIGURATION NODES
>> +patternProperties:
>> +  '^.*$':
>> +    if:
>> +      type: object
>> +      $ref: "/schemas/pinctrl/pincfg-node.yaml"
>> +    then:
> 
> I wish people would stop copying this if/then hack...
> 
> For new bindings, just name your nodes something sensible you can match
> on like '-pins$'.
> 
I always check the newest available yaml that I can find in the same 
folder before writing mine... in this case, it was sm8250-pinctrl.yaml
and I thought that this was the accepted way, since.. that's.. the 
newest one.

By the way, I've fixed it now. I'll send V4 in the evening!
Thank you!

>> +      properties:
>> +        pins:
>> +          description:
>> +            List of gpio pins affected by the properties specified in
>> +            this subnode.
>> +          items:
>> +            pattern: "^gpio([0-9]|1[0-5])$"
>> +          minItems: 1
>> +          maxItems: 16
>> +
>> +        function:
>> +          description:
>> +            Specify the alternative function to be configured for the
>> +            specified pins.
>> +
>> +          enum: [ gpio, pwm ]
>> +
>> +        bias-disable: true
>> +        bias-pull-down: true
>> +        bias-pull-up: true
>> +        drive-open-drain: true
>> +        drive-push-pull: true
>> +        input-enable: true
>> +        output-high: true
>> +        output-low: true
>> +
>> +      required:
>> +        - pins
>> +        - function
>> +
>> +      additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - gpio-controller
>> +  - '#gpio-cells'
>> +  - gpio-ranges
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c_node {
>> +        gpio-expander@58 {
>> +                compatible = "awinic,aw9523-pinctrl";
>> +                reg = <0x58>;
>> +                interrupt-parent = <&tlmm>;
>> +                interrupts = <50 IRQ_TYPE_EDGE_FALLING>;
>> +                gpio-controller;
>> +                #gpio-cells = <2>;
>> +                gpio-ranges = <&tlmm 0 0 16>;
>> +                interrupt-controller;
>> +                #interrupt-cells = <2>;
>> +                reset-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
>> +        };
>> +    };
>> -- 
>> 2.29.2
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
  2021-01-13 12:30     ` AngeloGioacchino Del Regno
@ 2021-01-14 22:42       ` Rob Herring
  0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-01-14 22:42 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Linus Walleij, linux-kernel, konrad.dybcio, marijn.suijten,
	martin.botka, phone-devel, open list:GPIO SUBSYSTEM, devicetree

On Wed, Jan 13, 2021 at 6:30 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 13/01/21 03:41, Rob Herring ha scritto:
> > On Mon, Jan 11, 2021 at 07:29:28PM +0100, AngeloGioacchino Del Regno wrote:
> >> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >> ---
> >>   .../pinctrl/awinic,aw9523-pinctrl.yaml        | 112 ++++++++++++++++++
> >>   1 file changed, 112 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> >> new file mode 100644
> >> index 000000000000..a705c05bb5a2
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
> >> @@ -0,0 +1,112 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/pinctrl/awinic,aw9523-pinctrl.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Awinic AW9523/AW9523B I2C GPIO Expander
> >> +
> >> +maintainers:
> >> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >> +
> >> +description: |
> >> +  The Awinic AW9523/AW9523B I2C GPIO Expander featuring 16 multi-function
> >> +  I/O, 256 steps PWM mode and interrupt support.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: awinic,aw9523-pinctrl
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  '#gpio-cells':
> >> +    description: |
> >> +      Specifying the pin number and flags, as defined in
> >> +      include/dt-bindings/gpio/gpio.h
> >> +    const: 2
> >> +
> >> +  gpio-controller: true
> >> +
> >> +  gpio-ranges:
> >> +    maxItems: 1
> >> +
> >> +  interrupt-controller: true
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +    description: Specifies the INTN pin IRQ.
> >> +
> >> +  '#interrupt-cells':
> >> +    description:
> >> +      Specifies the PIN numbers and Flags, as defined in defined in
> >> +      include/dt-bindings/interrupt-controller/irq.h
> >> +    const: 2
> >> +
> >> +#PIN CONFIGURATION NODES
> >> +patternProperties:
> >> +  '^.*$':
> >> +    if:
> >> +      type: object
> >> +      $ref: "/schemas/pinctrl/pincfg-node.yaml"
> >> +    then:
> >
> > I wish people would stop copying this if/then hack...
> >
> > For new bindings, just name your nodes something sensible you can match
> > on like '-pins$'.
> >
> I always check the newest available yaml that I can find in the same
> folder before writing mine... in this case, it was sm8250-pinctrl.yaml
> and I thought that this was the accepted way, since.. that's.. the
> newest one.

Normally, that's a good strategy. Unfortunately, this one went in
without my review. There was supposed to be a follow-up to fix some of
the issues, but seems that never happened.

Rob

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
  2021-01-11 18:29 [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
  2021-01-11 18:29 ` [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
@ 2021-01-18 13:19 ` Linus Walleij
  2021-01-18 14:38   ` AngeloGioacchino Del Regno
  1 sibling, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-01-18 13:19 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring

On Mon, Jan 11, 2021 at 7:29 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:

> The Awinic AW9523(B) is a multi-function I2C gpio expander in a
> TQFN-24L package, featuring PWM (max 37mA per pin, or total max
> power 3.2Watts) for LED driving capability.
>
> It has two ports with 8 pins per port (for a total of 16 pins),
> configurable as either PWM with 1/256 stepping or GPIO input/output,
> 1.8V logic input; each GPIO can be configured as input or output
> independently from each other.
>
> This IC also has an internal interrupt controller, which is capable
> of generating an interrupt for each GPIO, depending on the
> configuration, and will raise an interrupt on the INTN pin to
> advertise this to an external interrupt controller.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

I really like the looks of this new version! :)

Just some minor questions now:

> +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> +       switch (type) {
> +       case IRQ_TYPE_NONE:
> +       case IRQ_TYPE_LEVEL_MASK:
> +       case IRQ_TYPE_LEVEL_HIGH:
> +       case IRQ_TYPE_LEVEL_LOW:
> +               return 0;

This still doesn't make sense, I think you can see why.

If level IRQs can fire both when the line is high and low without
any configuration in any register whatsoever, then IRQs are
*constantly* fireing. Which I suppose they are not.

Something is wrong here. Either you just support one
of them or there is a way to configure whether to act as
high or low level.

I'm also mildly sceptic because GPIO expanders ... do they
really support level IRQs, and does it make sense when you
think about it? I would rather expect edge IRQs as these have
a natural trigger point and level IRQs may very well be gone
by the time you come around to read the status register.
I suppose it can be done but the events must be really slow
then, slower than the 400kHz of an I2C bus.

Can you please look over the interrupt logic in your specs?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
  2021-01-18 13:19 ` [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander Linus Walleij
@ 2021-01-18 14:38   ` AngeloGioacchino Del Regno
  2021-01-22  9:59     ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-18 14:38 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring

Il 18/01/21 14:19, Linus Walleij ha scritto:
> On Mon, Jan 11, 2021 at 7:29 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> 
>> The Awinic AW9523(B) is a multi-function I2C gpio expander in a
>> TQFN-24L package, featuring PWM (max 37mA per pin, or total max
>> power 3.2Watts) for LED driving capability.
>>
>> It has two ports with 8 pins per port (for a total of 16 pins),
>> configurable as either PWM with 1/256 stepping or GPIO input/output,
>> 1.8V logic input; each GPIO can be configured as input or output
>> independently from each other.
>>
>> This IC also has an internal interrupt controller, which is capable
>> of generating an interrupt for each GPIO, depending on the
>> configuration, and will raise an interrupt on the INTN pin to
>> advertise this to an external interrupt controller.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> I really like the looks of this new version! :)
> 
> Just some minor questions now:
> 
>> +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> +       switch (type) {
>> +       case IRQ_TYPE_NONE:
>> +       case IRQ_TYPE_LEVEL_MASK:
>> +       case IRQ_TYPE_LEVEL_HIGH:
>> +       case IRQ_TYPE_LEVEL_LOW:
>> +               return 0;
> 
> This still doesn't make sense, I think you can see why.
> 
> If level IRQs can fire both when the line is high and low without
> any configuration in any register whatsoever, then IRQs are
> *constantly* fireing. Which I suppose they are not.
> 
> Something is wrong here. Either you just support one
> of them or there is a way to configure whether to act as
> high or low level.
> 
> I'm also mildly sceptic because GPIO expanders ... do they
> really support level IRQs, and does it make sense when you
> think about it? I would rather expect edge IRQs as these have
> a natural trigger point and level IRQs may very well be gone
> by the time you come around to read the status register.
> I suppose it can be done but the events must be really slow
> then, slower than the 400kHz of an I2C bus.
> 
> Can you please look over the interrupt logic in your specs?
> 
> Yours,
> Linus Walleij
> 

Have I misunderstood this part of the API?

By the way, this is really LEVEL irq, not EDGE... To avoid any
misunderstanding, I think that the best way to show you what I
am seeing is to just copy-paste the relevant piece from the
datasheet for this hardware (it's not a confidential datasheet
and freely found on the internet).

Check this out:
" External MCU is required acknowledge by INTN pin. INTN is open-drain
out-
put, low-level active, and need external pull-up resistor.

When AW9523B detect port change, any input state from high-level to
low-level or from
  low-level to high-level will generate interrupt after
8us internal deglitch. "

...but since the datasheet is sometimes unclear about "things" (I am
mostly sure that they have translated it to english from chinese), I
have actually checked whether the INTN pin was pushed LOW when one of
the inputs goes from HIGH to LOW.. and.. it does... and as you imagine
yeah.. it's slow.. and yes, as slow as you can imagine. :)

So, in short, this chip is raising an interrupt when any input changes
state, regardless of the change being LOW->HIGH or HIGH->LOW.

Thanks
- Angelo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
  2021-01-18 14:38   ` AngeloGioacchino Del Regno
@ 2021-01-22  9:59     ` Linus Walleij
  2021-01-22 23:39       ` AngeloGioacchino Del Regno
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-01-22  9:59 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring

On Mon, Jan 18, 2021 at 3:38 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:

> By the way, this is really LEVEL irq, not EDGE... To avoid any
> misunderstanding, I think that the best way to show you what I
> am seeing is to just copy-paste the relevant piece from the
> datasheet for this hardware (it's not a confidential datasheet
> and freely found on the internet).
>
> Check this out:
> " External MCU is required acknowledge by INTN pin. INTN is open-drain
> out-
> put, low-level active, and need external pull-up resistor.

This talks about what polarity (active low) the pin from the expander
to the SoC/CPU is. It has nothing to do with the line into the
expander.

> When AW9523B detect port change, any input state from high-level to
> low-level or from
>   low-level to high-level will generate interrupt after
> 8us internal deglitch. "
>
> ...but since the datasheet is sometimes unclear about "things" (I am
> mostly sure that they have translated it to english from chinese), I
> have actually checked whether the INTN pin was pushed LOW when one of
> the inputs goes from HIGH to LOW.. and.. it does... and as you imagine
> yeah.. it's slow.. and yes, as slow as you can imagine. :)
>
> So, in short, this chip is raising an interrupt when any input changes
> state, regardless of the change being LOW->HIGH or HIGH->LOW.

This means that the expander only supports
IRQ_TYPE_EDGE_BOTH and nothing else.

"port change" above means edges.

Augment your driver to only accept this type.

The consumers better request IRQ_TYPE_EDGE_BOTH
(from a device tree for example) and consumers better
handle the fact that they get interrupts on both rising
and falling edge as well, else they may need special
code to handle it. This is not a very nice feature of
the expander, it would be more helpful to users to
get interrupts on only rising or only falling edges, but
as written, it will generate interrupts on both transitions.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
  2021-01-22  9:59     ` Linus Walleij
@ 2021-01-22 23:39       ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 10+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-22 23:39 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, konrad.dybcio, marijn.suijten, martin.botka,
	phone-devel, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring

Il 22/01/21 10:59, Linus Walleij ha scritto:
> On Mon, Jan 18, 2021 at 3:38 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> 
>> By the way, this is really LEVEL irq, not EDGE... To avoid any
>> misunderstanding, I think that the best way to show you what I
>> am seeing is to just copy-paste the relevant piece from the
>> datasheet for this hardware (it's not a confidential datasheet
>> and freely found on the internet).
>>
>> Check this out:
>> " External MCU is required acknowledge by INTN pin. INTN is open-drain
>> out-
>> put, low-level active, and need external pull-up resistor.
> 
> This talks about what polarity (active low) the pin from the expander
> to the SoC/CPU is. It has nothing to do with the line into the
> expander.
> 
>> When AW9523B detect port change, any input state from high-level to
>> low-level or from
>>    low-level to high-level will generate interrupt after
>> 8us internal deglitch. "
>>
>> ...but since the datasheet is sometimes unclear about "things" (I am
>> mostly sure that they have translated it to english from chinese), I
>> have actually checked whether the INTN pin was pushed LOW when one of
>> the inputs goes from HIGH to LOW.. and.. it does... and as you imagine
>> yeah.. it's slow.. and yes, as slow as you can imagine. :)
>>
>> So, in short, this chip is raising an interrupt when any input changes
>> state, regardless of the change being LOW->HIGH or HIGH->LOW.
> 
> This means that the expander only supports
> IRQ_TYPE_EDGE_BOTH and nothing else.
> 
> "port change" above means edges.
> 
> Augment your driver to only accept this type.
> 
> The consumers better request IRQ_TYPE_EDGE_BOTH
> (from a device tree for example) and consumers better
> handle the fact that they get interrupts on both rising
> and falling edge as well, else they may need special
> code to handle it. This is not a very nice feature of
> the expander, it would be more helpful to users to
> get interrupts on only rising or only falling edges, but
> as written, it will generate interrupts on both transitions.
> 
> Yours,
> Linus Walleij
> 

I see the reading mistake now... oh wow, that was... sad, from me.
I will fix this ASAP and will send back a v3.

Thank you!

- Angelo

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-01-22 23:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 18:29 [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
2021-01-11 18:29 ` [PATCH v2 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
2021-01-12 21:52   ` Rob Herring
2021-01-13  2:41   ` Rob Herring
2021-01-13 12:30     ` AngeloGioacchino Del Regno
2021-01-14 22:42       ` Rob Herring
2021-01-18 13:19 ` [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander Linus Walleij
2021-01-18 14:38   ` AngeloGioacchino Del Regno
2021-01-22  9:59     ` Linus Walleij
2021-01-22 23:39       ` AngeloGioacchino Del Regno

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