[PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[Baolin Wang]

This patch adds the device tree bindings for the Spreadtrum EIC
controller. The EIC can be recognized as one special type of GPIO,
which can only be used as input.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 .../devicetree/bindings/gpio/gpio-eic-sprd.txt     |   51 ++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
new file mode 100644
index 0000000..34f194f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
@@ -0,0 +1,51 @@
+Spreadtrum EIC controller bindings
+
+The EIC is the abbreviation of external interrupt controller, which
+is only can be used as input mode. The EIC controller includes 4
+sub-modules: EIC-Debounce, EIC-Latch, EIC-Async, EIC-Sync.
+
+The EIC-debounce sub-module provides up to 8 source input signal
+connection. A debounce machanism is used to capture input signal's
+stable status (ms grade) and a single-trigger mechanism is introduced
+into this sub-module to enhance the input event detection reliability.
+In addition, this sub-module's clock can be shut-off automatically to
+reduce power dissipation. The debounce range is from 1ms to 4s with
+the step of 1ms. If the input signal is shorter than 1ms, it will be
+omitted as this sub-module.
+
+The EIC-latch sub-module is used to latch some special input signal
+and send interrupts to MCU core, and it can provide up to 8 latch
+source input signal connection.
+
+The EIC-async sub-module uses 32k clock to capture short signal (us
+grade) to generate interrupt to MCU by level or edge trigger.
+
+The EIC-sync is similar with GPIO's input function.
+
+Required properties:
+- compatible: Should be one of the following:
+  "sprd,sc9860-eic-debounce",
+  "sprd,sc9860-eic-latch",
+  "sprd,sc9860-eic-async",
+  "sprd,sc9860-eic-sync",
+  "sprd,sc27xx-eic-debounce".
+- reg: Define the base and range of the I/O address space containing
+  the GPIO controller registers.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be <2>. The first cell is the gpio number and
+  the second cell is used to specify optional parameters.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Should be <2>. Specifies the number of cells needed
+  to encode interrupt source.
+- interrupts: Should be the port interrupt shared by all the gpios.
+
+Example:
+	eic_debounce: eic@40210000 {
+		compatible = "sprd,sc9860-eic-debounce";
+		reg = <0 0x40210000 0 0x80>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
1.7.9.5

[PATCH 2/2] gpio: Add Spreadtrum EIC driver support

[Baolin Wang]

The Spreadtrum platform has 2 EIC controllers, one is in digital chip and
another one is in PMIC. The digital-chip EIC controller has 4 sub-modules:
debounce EIC, latch EIC, async EIC and sync EIC, each sub-module can has
multiple groups and each group contains 8 EICs. The PMIC EIC controller has
only one debounce EIC sub-module.

Each EIC can only be used as input mode, and has the capability to trigger
interrupts when detecting input signals.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/gpio/Kconfig         |    7 +
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-eic-sprd.c |  782 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 790 insertions(+)
 create mode 100644 drivers/gpio/gpio-eic-sprd.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 184b581..2414391 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -159,6 +159,13 @@ config GPIO_DWAPB
 	  Say Y or M here to build support for the Synopsys DesignWare APB
 	  GPIO block.
 
+config GPIO_EIC_SPRD
+	tristate "Spreadtrum EIC support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support Spreadtrum EIC device.
+
 config GPIO_EM
 	tristate "Emma Mobile GPIO"
 	depends on (ARCH_EMEV2 || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 2e3d320..66eaa9e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
 obj-$(CONFIG_GPIO_DLN2)		+= gpio-dln2.o
 obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
+obj-$(CONFIG_GPIO_EIC_SPRD)	+= gpio-eic-sprd.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_ETRAXFS)	+= gpio-etraxfs.o
diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
new file mode 100644
index 0000000..ccc12a8
--- /dev/null
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -0,0 +1,782 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Spreadtrum Communications Inc.
+ * Copyright (c) 2018 Linaro Ltd.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+
+/* EIC registers definition */
+#define SPRD_EIC_DBNC_DATA		0x0
+#define SPRD_EIC_DBNC_DMSK		0x4
+#define SPRD_EIC_DBNC_IEV		0x14
+#define SPRD_EIC_DBNC_IE		0x18
+#define SPRD_EIC_DBNC_RIS		0x1c
+#define SPRD_EIC_DBNC_MIS		0x20
+#define SPRD_EIC_DBNC_IC		0x24
+#define SPRD_EIC_DBNC_TRIG		0x28
+#define SPRD_EIC_DBNC_CTRL0		0x40
+#define SPRD_EIC_DBNC_CTRL1		0x44
+#define SPRD_EIC_DBNC_CTRL2		0x48
+#define SPRD_EIC_DBNC_CTRL3		0x4c
+#define SPRD_EIC_DBNC_CTRL4		0x50
+#define SPRD_EIC_DBNC_CTRL5		0x54
+#define SPRD_EIC_DBNC_CTRL6		0x58
+#define SPRD_EIC_DBNC_CTRL7		0x5c
+
+#define SPRD_EIC_LATCH_INTEN		0x0
+#define SPRD_EIC_LATCH_INTRAW		0x4
+#define SPRD_EIC_LATCH_INTMSK		0x8
+#define SPRD_EIC_LATCH_INTCLR		0xc
+#define SPRD_EIC_LATCH_INTPOL		0x10
+#define SPRD_EIC_LATCH_INTMODE		0x14
+
+#define SPRD_EIC_ASYNC_INTIE		0x0
+#define SPRD_EIC_ASYNC_INTRAW		0x4
+#define SPRD_EIC_ASYNC_INTMSK		0x8
+#define SPRD_EIC_ASYNC_INTCLR		0xc
+#define SPRD_EIC_ASYNC_INTMODE		0x10
+#define SPRD_EIC_ASYNC_INTBOTH		0x14
+#define SPRD_EIC_ASYNC_INTPOL		0x18
+#define SPRD_EIC_ASYNC_DATA		0x1c
+
+#define SPRD_EIC_SYNC_INTIE		0x0
+#define SPRD_EIC_SYNC_INTRAW		0x4
+#define SPRD_EIC_SYNC_INTMSK		0x8
+#define SPRD_EIC_SYNC_INTCLR		0xc
+#define SPRD_EIC_SYNC_INTMODE		0x10
+#define SPRD_EIC_SYNC_INTBOTH		0x14
+#define SPRD_EIC_SYNC_INTPOL		0x18
+#define SPRD_EIC_SYNC_DATA		0x1c
+
+/*
+ * The digital-chip EIC controller can support maximum 3 groups, and each group
+ * contains 8 EICs.
+ */
+#define SPRD_EIC_MAX_GROUP		3
+#define SPRD_EIC_PER_GROUP_NR		8
+#define SPRD_EIC_DATA_MASK		GENMASK(7, 0)
+#define SPRD_EIC_BIT(x)			((x) & (SPRD_EIC_PER_GROUP_NR - 1))
+
+/*
+ * The PMIC EIC controller only has one group, and each group now can contain
+ * 16 EICs.
+ */
+#define SPRD_PMIC_EIC_PER_GROUP_NR	16
+#define SPRD_PMIC_EIC_DATA_MASK		GENMASK(15, 0)
+#define SPRD_PMIC_EIC_BIT(x)		((x) & (SPRD_PMIC_EIC_PER_GROUP_NR - 1))
+#define SPRD_PMIC_EIC_CACHE_NR_REGS	(SPRD_EIC_DBNC_TRIG + 0x4)
+
+#define SPRD_EIC_DBNC_MASK		GENMASK(11, 0)
+
+/*
+ * The Spreadtrum EIC (external interrupt controller) can only be used
+ * as input mode to generate interrupts if detecting input signals.
+ *
+ * The Spreadtrum platform has two EIC controllers, one EIC controller
+ * is in digital-chip, another one is in PMIC. There are some differences
+ * between this two EIC controllers. The digital-chip EIC controller
+ * includes 4 sub-modules: debounce EIC, latch EIC, async EIC and sync EIC,
+ * but the PMIC EIC controller only has one debounce EIC sub-module.
+ *
+ * The debounce EIC is used to capture input signal's stable status
+ * (ms grade) and a single-trigger mechanism is introduced into this
+ * sub-module to enhance the input event detection reliability. The
+ * debounce range is from 1ms to 4s with the step of 1ms.
+ *
+ * The latch EIC is used to latch some special input signal and send
+ * interrupts to MCU core.
+ *
+ * The async EIC uses 32k clock to capture short signal (us grade)
+ * to generate interrupt to MCU by level or edge trigger.
+ *
+ * The sync EIC is similar with GPIO's input function.
+ */
+enum sprd_eic_type {
+	SPRD_EIC_DEBOUNCE,
+	SPRD_EIC_LATCH,
+	SPRD_EIC_ASYNC,
+	SPRD_EIC_SYNC,
+	SPRD_EIC_PMIC_DEBOUNCE,
+	SPRD_EIC_MAX,
+};
+
+struct sprd_eic {
+	struct gpio_chip chip;
+	struct irq_chip intc;
+	void __iomem *base[SPRD_EIC_MAX_GROUP];
+	struct regmap *map;
+	u32 map_base;
+	u8 reg[SPRD_PMIC_EIC_CACHE_NR_REGS];
+	enum sprd_eic_type type;
+	spinlock_t lock;
+	struct mutex buslock;
+	int irq;
+};
+
+struct sprd_eic_data {
+	enum sprd_eic_type type;
+	u32 num_eics;
+};
+
+static const char *sprd_eic_label_name[SPRD_EIC_MAX] = {
+	"eic-debounce", "eic-latch", "eic-async",
+	"eic-sync", "eic-pmic-debounce",
+};
+
+static const struct sprd_eic_data sc9860_eic_dbnc_data = {
+	.type = SPRD_EIC_DEBOUNCE,
+	.num_eics = 8,
+};
+
+static const struct sprd_eic_data sc9860_eic_latch_data = {
+	.type = SPRD_EIC_LATCH,
+	.num_eics = 8,
+};
+
+static const struct sprd_eic_data sc9860_eic_async_data = {
+	.type = SPRD_EIC_ASYNC,
+	.num_eics = 8,
+};
+
+static const struct sprd_eic_data sc9860_eic_sync_data = {
+	.type = SPRD_EIC_SYNC,
+	.num_eics = 8,
+};
+
+static const struct sprd_eic_data pmic_eic_dbnc_data = {
+	.type = SPRD_EIC_PMIC_DEBOUNCE,
+	.num_eics = 16,
+};
+
+static inline void __iomem *sprd_eic_group_base(struct sprd_eic *sprd_eic,
+						unsigned int group)
+{
+	if (group >= SPRD_EIC_MAX_GROUP)
+		return NULL;
+
+	return sprd_eic->base[group];
+}
+
+static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
+			    unsigned int reg, unsigned int val)
+{
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 shift;
+
+	if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
+		shift = SPRD_PMIC_EIC_BIT(offset);
+
+		regmap_update_bits(sprd_eic->map, sprd_eic->map_base + reg,
+				   BIT(shift), val << shift);
+	} else {
+		void __iomem *base = sprd_eic_group_base(sprd_eic,
+						offset / SPRD_EIC_PER_GROUP_NR);
+		unsigned long flags;
+		u32 orig, tmp;
+
+		shift = SPRD_EIC_BIT(offset);
+
+		spin_lock_irqsave(&sprd_eic->lock, flags);
+		orig = readl_relaxed(base + reg);
+
+		tmp = (orig & ~BIT(shift)) | (val << shift);
+		writel_relaxed(tmp, base + reg);
+		spin_unlock_irqrestore(&sprd_eic->lock, flags);
+	}
+}
+
+static int sprd_eic_read(struct gpio_chip *chip, unsigned int offset,
+			 unsigned int reg)
+{
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 shift;
+	u32 value;
+
+	if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
+		int ret;
+
+		shift = SPRD_PMIC_EIC_BIT(offset);
+		ret = regmap_read(sprd_eic->map, sprd_eic->map_base + reg,
+				  &value);
+		if (ret)
+			return ret;
+
+		value &= SPRD_PMIC_EIC_DATA_MASK;
+	} else {
+		void __iomem *base = sprd_eic_group_base(sprd_eic,
+						offset / SPRD_EIC_PER_GROUP_NR);
+
+		shift = SPRD_EIC_BIT(offset);
+		value = readl_relaxed(base + reg) & SPRD_EIC_DATA_MASK;
+	}
+
+	return !!(value & BIT(shift));
+}
+
+static int sprd_eic_request(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 1);
+	return 0;
+}
+
+static void sprd_eic_free(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 0);
+}
+
+static int sprd_eic_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return sprd_eic_read(chip, offset, SPRD_EIC_DBNC_DATA);
+}
+
+static int sprd_eic_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	/* EICs are always input, nothing need to do here. */
+	return 0;
+}
+
+static void sprd_eic_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	/* EICs are always input, nothing need to do here. */
+}
+
+static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
+				 unsigned int debounce)
+{
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 shift, reg, value;
+
+	if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
+		int ret;
+
+		shift = SPRD_PMIC_EIC_BIT(offset);
+		reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
+		ret = regmap_read(sprd_eic->map, sprd_eic->map_base + reg,
+				  &value);
+		if (ret)
+			return ret;
+
+		value &= ~SPRD_EIC_DBNC_MASK;
+		value |= debounce / 1000;
+		ret = regmap_write(sprd_eic->map, sprd_eic->map_base + reg,
+				   value);
+		if (ret)
+			return ret;
+	} else {
+		void __iomem *base = sprd_eic_group_base(sprd_eic,
+						offset / SPRD_EIC_PER_GROUP_NR);
+
+		shift = SPRD_EIC_BIT(offset);
+		reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
+		value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
+
+		value |= debounce / 1000;
+		writel_relaxed(value, base + reg);
+	}
+
+	return 0;
+}
+
+static int sprd_eic_set_config(struct gpio_chip *chip, unsigned int offset,
+			       unsigned long config)
+{
+	unsigned long param = pinconf_to_config_param(config);
+	u32 arg = pinconf_to_config_argument(config);
+
+	if (param == PIN_CONFIG_INPUT_DEBOUNCE)
+		return sprd_eic_set_debounce(chip, offset, arg);
+
+	return -ENOTSUPP;
+}
+
+static void sprd_eic_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	switch (sprd_eic->type) {
+	case SPRD_EIC_PMIC_DEBOUNCE:
+		sprd_eic->reg[SPRD_EIC_DBNC_IE] = 0;
+		sprd_eic->reg[SPRD_EIC_DBNC_TRIG] = 0;
+		break;
+	case SPRD_EIC_DEBOUNCE:
+		sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IE, 0);
+		sprd_eic_update(chip, offset, SPRD_EIC_DBNC_TRIG, 0);
+		break;
+	case SPRD_EIC_LATCH:
+		sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTEN, 0);
+		break;
+	case SPRD_EIC_ASYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTIE, 0);
+		break;
+	case SPRD_EIC_SYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTIE, 0);
+		break;
+	default:
+		dev_err(chip->parent, "Unsupported EIC type.\n");
+	}
+}
+
+static void sprd_eic_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	switch (sprd_eic->type) {
+	case SPRD_EIC_PMIC_DEBOUNCE:
+		sprd_eic->reg[SPRD_EIC_DBNC_IE] = 1;
+		sprd_eic->reg[SPRD_EIC_DBNC_TRIG] = 1;
+		break;
+	case SPRD_EIC_DEBOUNCE:
+		sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IE, 1);
+		sprd_eic_update(chip, offset, SPRD_EIC_DBNC_TRIG, 1);
+		break;
+	case SPRD_EIC_LATCH:
+		sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTEN, 1);
+		break;
+	case SPRD_EIC_ASYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTIE, 1);
+		break;
+	case SPRD_EIC_SYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTIE, 1);
+		break;
+	default:
+		dev_err(chip->parent, "Unsupported EIC type.\n");
+	}
+}
+
+static void sprd_eic_irq_ack(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	switch (sprd_eic->type) {
+	case SPRD_EIC_PMIC_DEBOUNCE:
+		break;
+	case SPRD_EIC_DEBOUNCE:
+		sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IC, 1);
+		break;
+	case SPRD_EIC_LATCH:
+		sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTCLR, 1);
+		break;
+	case SPRD_EIC_ASYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTCLR, 1);
+		break;
+	case SPRD_EIC_SYNC:
+		sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTCLR, 1);
+		break;
+	default:
+		dev_err(chip->parent, "Unsupported EIC type.\n");
+	}
+}
+
+static int sprd_eic_irq_set_type(struct irq_data *data,
+				 unsigned int flow_type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	switch (sprd_eic->type) {
+	case SPRD_EIC_PMIC_DEBOUNCE:
+		switch (flow_type) {
+		case IRQ_TYPE_LEVEL_HIGH:
+			sprd_eic->reg[SPRD_EIC_DBNC_IEV] = 1;
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			sprd_eic->reg[SPRD_EIC_DBNC_IEV] = 0;
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+
+		irq_set_handler_locked(data, handle_level_irq);
+		break;
+	case SPRD_EIC_DEBOUNCE:
+		switch (flow_type) {
+		case IRQ_TYPE_LEVEL_HIGH:
+			sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IEV, 1);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IEV, 0);
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+
+		irq_set_handler_locked(data, handle_level_irq);
+		break;
+	case SPRD_EIC_LATCH:
+		switch (flow_type) {
+		case IRQ_TYPE_LEVEL_HIGH:
+			sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 0);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 1);
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+
+		irq_set_handler_locked(data, handle_level_irq);
+		break;
+	case SPRD_EIC_ASYNC:
+		switch (flow_type) {
+		case IRQ_TYPE_EDGE_RISING:
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 1);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_EDGE_FALLING:
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 0);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_EDGE_BOTH:
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 1);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 1);
+			irq_set_handler_locked(data, handle_level_irq);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTMODE, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_ASYNC_INTPOL, 0);
+			irq_set_handler_locked(data, handle_level_irq);
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+		break;
+	case SPRD_EIC_SYNC:
+		switch (flow_type) {
+		case IRQ_TYPE_EDGE_RISING:
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 1);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_EDGE_FALLING:
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 0);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_EDGE_BOTH:
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 1);
+			irq_set_handler_locked(data, handle_edge_irq);
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 1);
+			irq_set_handler_locked(data, handle_level_irq);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTBOTH, 0);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTMODE, 1);
+			sprd_eic_update(chip, offset, SPRD_EIC_SYNC_INTPOL, 0);
+			irq_set_handler_locked(data, handle_level_irq);
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+	default:
+		dev_err(chip->parent, "Unsupported EIC type.\n");
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static void sprd_eic_bus_lock(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+
+	mutex_lock(&sprd_eic->buslock);
+}
+
+static void sprd_eic_bus_sync_unlock(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	/* Set irq type */
+	sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IEV,
+			sprd_eic->reg[SPRD_EIC_DBNC_IEV]);
+	/* Set irq unmask */
+	sprd_eic_update(chip, offset, SPRD_EIC_DBNC_IE,
+			sprd_eic->reg[SPRD_EIC_DBNC_IE]);
+	/* Generate trigger start pulse for debounce EIC */
+	sprd_eic_update(chip, offset, SPRD_EIC_DBNC_TRIG,
+			sprd_eic->reg[SPRD_EIC_DBNC_TRIG]);
+
+	mutex_unlock(&sprd_eic->buslock);
+}
+
+static int sprd_eic_match_chip_by_type(struct gpio_chip *chip, void *data)
+{
+	enum sprd_eic_type type = *(enum sprd_eic_type *)data;
+
+	return !strcmp(chip->label, sprd_eic_label_name[type]);
+}
+
+static void sprd_eic_handle_one_type(struct gpio_chip *chip)
+{
+	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
+	u32 group, n, girq;
+
+	for (group = 0; group * SPRD_EIC_PER_GROUP_NR < chip->ngpio; group++) {
+		void __iomem *base = sprd_eic_group_base(sprd_eic, group);
+		unsigned long reg;
+
+		switch (sprd_eic->type) {
+		case SPRD_EIC_DEBOUNCE:
+			reg = readl_relaxed(base + SPRD_EIC_DBNC_MIS) &
+				SPRD_EIC_DATA_MASK;
+			break;
+		case SPRD_EIC_LATCH:
+			reg = readl_relaxed(base + SPRD_EIC_LATCH_INTMSK) &
+				SPRD_EIC_DATA_MASK;
+			break;
+		case SPRD_EIC_ASYNC:
+			reg = readl_relaxed(base + SPRD_EIC_ASYNC_INTMSK) &
+				SPRD_EIC_DATA_MASK;
+			break;
+		case SPRD_EIC_SYNC:
+			reg = readl_relaxed(base + SPRD_EIC_SYNC_INTMSK) &
+				SPRD_EIC_DATA_MASK;
+			break;
+		default:
+			dev_err(chip->parent, "Unsupported EIC type.\n");
+			return;
+		}
+
+		for_each_set_bit(n, &reg, SPRD_EIC_PER_GROUP_NR) {
+			girq = irq_find_mapping(chip->irq.domain,
+					group * SPRD_EIC_PER_GROUP_NR + n);
+
+			generic_handle_irq(girq);
+		}
+	}
+}
+
+static void sprd_eic_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *ic = irq_desc_get_chip(desc);
+	struct gpio_chip *chip;
+	enum sprd_eic_type type;
+
+	chained_irq_enter(ic, desc);
+
+	/*
+	 * Since the digital-chip EIC 4 sub-modules (debounce, latch, async and
+	 * sync) share one same interrupt line, we should iterate each EIC
+	 * module to check if there are EIC interrupts were triggered.
+	 */
+	for (type = SPRD_EIC_DEBOUNCE; type < SPRD_EIC_PMIC_DEBOUNCE; type++) {
+		chip = gpiochip_find(&type, sprd_eic_match_chip_by_type);
+		if (!chip)
+			continue;
+
+		sprd_eic_handle_one_type(chip);
+	}
+
+	chained_irq_exit(ic, desc);
+}
+
+static irqreturn_t sprd_pmic_eic_irq_handler(int irq, void *data)
+{
+	struct sprd_eic *sprd_eic = data;
+	struct gpio_chip *chip = &sprd_eic->chip;
+	u32 n, girq, val;
+	int ret;
+
+	ret = regmap_read(sprd_eic->map, sprd_eic->map_base + SPRD_EIC_DBNC_MIS,
+			  &val);
+	if (ret)
+		return IRQ_RETVAL(ret);
+
+	val &= SPRD_PMIC_EIC_DATA_MASK;
+
+	for (n = 0; n < chip->ngpio; n++) {
+		if (!(BIT(n) & val))
+			continue;
+
+		sprd_eic_update(chip, n, SPRD_EIC_DBNC_IC, 1);
+
+		girq = irq_find_mapping(chip->irq.domain, n);
+		handle_nested_irq(girq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sprd_eic_probe(struct platform_device *pdev)
+{
+	const struct sprd_eic_data *pdata;
+	struct gpio_irq_chip *irq;
+	struct sprd_eic *sprd_eic;
+	struct resource *res;
+	int ret, i;
+
+	pdata = of_device_get_match_data(&pdev->dev);
+	if (!pdata) {
+		dev_err(&pdev->dev, "No matching driver data found.\n");
+		return -EINVAL;
+	}
+
+	sprd_eic = devm_kzalloc(&pdev->dev, sizeof(*sprd_eic), GFP_KERNEL);
+	if (!sprd_eic)
+		return -ENOMEM;
+
+	mutex_init(&sprd_eic->buslock);
+	spin_lock_init(&sprd_eic->lock);
+	sprd_eic->type = pdata->type;
+
+	sprd_eic->irq = platform_get_irq(pdev, 0);
+	if (sprd_eic->irq < 0) {
+		dev_err(&pdev->dev, "Failed to get EIC interrupt.\n");
+		return sprd_eic->irq;
+	}
+
+	if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
+		sprd_eic->map = dev_get_regmap(pdev->dev.parent, NULL);
+		if (!sprd_eic->map)
+			return -ENODEV;
+
+		ret = of_property_read_u32(pdev->dev.of_node, "reg",
+					   &sprd_eic->map_base);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to get EIC base address\n");
+			return ret;
+		}
+
+		ret = devm_request_threaded_irq(&pdev->dev, sprd_eic->irq, NULL,
+						sprd_pmic_eic_irq_handler,
+						IRQF_TRIGGER_LOW |
+						IRQF_ONESHOT | IRQF_NO_SUSPEND,
+						dev_name(&pdev->dev), sprd_eic);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request EIC IRQ.\n");
+			return ret;
+		}
+	} else {
+		for (i = 0; i < SPRD_EIC_MAX_GROUP; i++) {
+			/*
+			 * We can have maximum 3 groups EICs, and each EIC has
+			 * its own base address. But some platform maybe only
+			 * have one group EIC, thus base[1] and base[2] can be
+			 * optional.
+			 */
+			res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+			if (!res)
+				continue;
+
+			sprd_eic->base[i] = devm_ioremap_resource(&pdev->dev,
+								  res);
+			if (IS_ERR(sprd_eic->base[i]))
+				return PTR_ERR(sprd_eic->base[i]);
+		}
+	}
+
+	sprd_eic->chip.label = sprd_eic_label_name[sprd_eic->type];
+	sprd_eic->chip.ngpio = pdata->num_eics;
+	sprd_eic->chip.base = -1;
+	sprd_eic->chip.parent = &pdev->dev;
+	sprd_eic->chip.of_node = pdev->dev.of_node;
+	sprd_eic->chip.direction_input = sprd_eic_direction_input;
+	switch (sprd_eic->type) {
+	case SPRD_EIC_PMIC_DEBOUNCE:
+		/* fall-through */
+	case SPRD_EIC_DEBOUNCE:
+		sprd_eic->chip.request = sprd_eic_request;
+		sprd_eic->chip.free = sprd_eic_free;
+		sprd_eic->chip.set_config = sprd_eic_set_config;
+		sprd_eic->chip.set = sprd_eic_set;
+		/* fall-through */
+	case SPRD_EIC_ASYNC:
+		/* fall-through */
+	case SPRD_EIC_SYNC:
+		sprd_eic->chip.get = sprd_eic_get;
+		break;
+	case SPRD_EIC_LATCH:
+		/* fall-through */
+	default:
+		break;
+	}
+
+	sprd_eic->intc.name = dev_name(&pdev->dev);
+	sprd_eic->intc.irq_ack = sprd_eic_irq_ack;
+	sprd_eic->intc.irq_mask = sprd_eic_irq_mask;
+	sprd_eic->intc.irq_unmask = sprd_eic_irq_unmask;
+	sprd_eic->intc.irq_set_type = sprd_eic_irq_set_type;
+	sprd_eic->intc.flags = IRQCHIP_SKIP_SET_WAKE;
+
+	irq = &sprd_eic->chip.irq;
+	irq->chip = &sprd_eic->intc;
+	if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
+		sprd_eic->intc.irq_bus_lock = sprd_eic_bus_lock;
+		sprd_eic->intc.irq_bus_sync_unlock = sprd_eic_bus_sync_unlock;
+		irq->threaded = true;
+	} else {
+		irq->handler = handle_simple_irq;
+		irq->default_type = IRQ_TYPE_NONE;
+		irq->parent_handler = sprd_eic_irq_handler;
+		irq->parent_handler_data = sprd_eic;
+		irq->num_parents = 1;
+		irq->parents = &sprd_eic->irq;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &sprd_eic->chip, sprd_eic);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip %d.\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, sprd_eic);
+	return 0;
+}
+
+static const struct of_device_id sprd_eic_of_match[] = {
+	{ .compatible = "sprd,sc9860-eic-debounce", .data = &sc9860_eic_dbnc_data },
+	{ .compatible = "sprd,sc9860-eic-latch", .data = &sc9860_eic_latch_data },
+	{ .compatible = "sprd,sc9860-eic-async", .data = &sc9860_eic_async_data },
+	{ .compatible = "sprd,sc9860-eic-sync", .data = &sc9860_eic_sync_data },
+	{ .compatible = "sprd,sc27xx-eic-debounce", .data = &pmic_eic_dbnc_data },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, sprd_eic_of_match);
+
+static struct platform_driver sprd_eic_driver = {
+	.probe = sprd_eic_probe,
+	.driver = {
+		.name = "sprd-eic",
+		.of_match_table	= sprd_eic_of_match,
+	},
+};
+
+module_platform_driver(sprd_eic_driver);
+
+MODULE_DESCRIPTION("Spreadtrum EIC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[Linus Walleij]

Hi Baolin!

Thank you for your patch.

On Thu, Feb 8, 2018 at 9:01 AM, Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> This patch adds the device tree bindings for the Spreadtrum EIC
> controller. The EIC can be recognized as one special type of GPIO,

s/recognized as one/seen as a/g

> which can only be used as input.
>
> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> +The EIC is the abbreviation of external interrupt controller, which
> +is only can be used as input mode. The EIC controller includes 4

can be used only in input mode.

> +sub-modules: EIC-Debounce, EIC-Latch, EIC-Async, EIC-Sync.

Are they four sub-modules that are always synthesized into the
silicon at the same time, or do you mean that when producing the
hardware, the designer will choose one of these four types (it looks
like that from the example).

> +
> +The EIC-debounce sub-module provides up to 8 source input signal
> +connection.

connections.

> A debounce machanism is used to

mechanism

> capture input signal's

capture the input signals'

(note plural signals genitive)

> +stable status (ms grade)

is that millisecond resolution you mean?

> and a single-trigger mechanism is introduced
> +into this sub-module to enhance the input event detection reliability.
> +In addition, this sub-module's clock can be shut-off automatically to

no dash in "shut off"

> +reduce power dissipation. The debounce range is from 1ms to 4s with
> +the step of 1ms.

a step size of

> If the input signal is shorter than 1ms, it will be
> +omitted as this sub-module.

I don't understand the last part, do you mean the signal will be ignored
if it is asserted for less than 1 ms?

> +The EIC-latch sub-module is used to latch some special input signal

signals (plural)

What is special about them?

> +and send interrupts to MCU core, and it can provide up to 8 latch
> +source input signal connection.

connections (plural)

> +The EIC-async sub-module uses 32k clock

a 32kHz clock

> to capture short signal

to capture the short signal

> +(us grade)

Do you mean "microsecond granularity"?

> to generate interrupt to MCU by level or edge trigger.

What is MCU? I think you can just omit it, it could be integrated
elsewhere.

> +The EIC-sync is similar with GPIO's input function.

Do you mean that the EIC-sync module is a synchronized signal input
register? Please write that.

> +Required properties:
> +- compatible: Should be one of the following:
> +  "sprd,sc9860-eic-debounce",
> +  "sprd,sc9860-eic-latch",
> +  "sprd,sc9860-eic-async",
> +  "sprd,sc9860-eic-sync",
> +  "sprd,sc27xx-eic-debounce".

So it looks like there is one at the time, so in the SC9860
all four modules exist, but at different addresses?

(...)
> +Example:
> +       eic_debounce: eic@40210000 {
> +               compatible = "sprd,sc9860-eic-debounce";
> +               reg = <0 0x40210000 0 0x80>;

So does this mean that this is a debounced-only EIC?

There are latch, async and sync versions somewhere else in
memory? Or there could be? And they are never say debounce
and latch at the same time? Etc?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] gpio: Add Spreadtrum EIC driver support

[Linus Walleij]

Hi Baolin,

thank you for your patch!

On Thu, Feb 8, 2018 at 9:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> The Spreadtrum platform has 2 EIC controllers, one is in digital chip and
> another one is in PMIC. The digital-chip EIC controller has 4 sub-modules:
> debounce EIC, latch EIC, async EIC and sync EIC, each sub-module can has
> multiple groups and each group contains 8 EICs. The PMIC EIC controller has
> only one debounce EIC sub-module.

OK it seems they are at different memory addresses and totally
separate instances then. I start to understand!

> Each EIC can only be used as input mode, and has the capability to trigger
> interrupts when detecting input signals.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> +config GPIO_EIC_SPRD
> +       tristate "Spreadtrum EIC support"
> +       depends on ARCH_SPRD || COMPILE_TEST

depend on OF_GPIO

> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say yes here to support Spreadtrum EIC device.

> +/* EIC registers definition */

Aha I see so the register map is different depending on which
type is synthesized.

> +#define SPRD_EIC_DBNC_DATA             0x0
> +#define SPRD_EIC_DBNC_DMSK             0x4
> +#define SPRD_EIC_DBNC_IEV              0x14
> +#define SPRD_EIC_DBNC_IE               0x18
> +#define SPRD_EIC_DBNC_RIS              0x1c
> +#define SPRD_EIC_DBNC_MIS              0x20
> +#define SPRD_EIC_DBNC_IC               0x24
> +#define SPRD_EIC_DBNC_TRIG             0x28
> +#define SPRD_EIC_DBNC_CTRL0            0x40
> +#define SPRD_EIC_DBNC_CTRL1            0x44
> +#define SPRD_EIC_DBNC_CTRL2            0x48
> +#define SPRD_EIC_DBNC_CTRL3            0x4c
> +#define SPRD_EIC_DBNC_CTRL4            0x50
> +#define SPRD_EIC_DBNC_CTRL5            0x54
> +#define SPRD_EIC_DBNC_CTRL6            0x58
> +#define SPRD_EIC_DBNC_CTRL7            0x5c

It looks from the code below that register 0x40 thru 0x5c
are actually present on all the variants, not just the
debounce DBNC variant.

This is strange, then they should not be named
with the *_DBNC_* infix but instead something
neutral like SPRD_CTRL0 0x40 etc.

> +#define SPRD_EIC_LATCH_INTEN           0x0
> +#define SPRD_EIC_LATCH_INTRAW          0x4
> +#define SPRD_EIC_LATCH_INTMSK          0x8
> +#define SPRD_EIC_LATCH_INTCLR          0xc
> +#define SPRD_EIC_LATCH_INTPOL          0x10
> +#define SPRD_EIC_LATCH_INTMODE         0x14
> +
> +#define SPRD_EIC_ASYNC_INTIE           0x0
> +#define SPRD_EIC_ASYNC_INTRAW          0x4
> +#define SPRD_EIC_ASYNC_INTMSK          0x8
> +#define SPRD_EIC_ASYNC_INTCLR          0xc
> +#define SPRD_EIC_ASYNC_INTMODE         0x10
> +#define SPRD_EIC_ASYNC_INTBOTH         0x14
> +#define SPRD_EIC_ASYNC_INTPOL          0x18
> +#define SPRD_EIC_ASYNC_DATA            0x1c
> +
> +#define SPRD_EIC_SYNC_INTIE            0x0
> +#define SPRD_EIC_SYNC_INTRAW           0x4
> +#define SPRD_EIC_SYNC_INTMSK           0x8
> +#define SPRD_EIC_SYNC_INTCLR           0xc
> +#define SPRD_EIC_SYNC_INTMODE          0x10
> +#define SPRD_EIC_SYNC_INTBOTH          0x14
> +#define SPRD_EIC_SYNC_INTPOL           0x18
> +#define SPRD_EIC_SYNC_DATA             0x1c
> +
> +/*
> + * The digital-chip EIC controller can support maximum 3 groups, and each group
> + * contains 8 EICs.
> + */
> +#define SPRD_EIC_MAX_GROUP             3
> +#define SPRD_EIC_PER_GROUP_NR          8
> +#define SPRD_EIC_DATA_MASK             GENMASK(7, 0)
> +#define SPRD_EIC_BIT(x)                        ((x) & (SPRD_EIC_PER_GROUP_NR - 1))

Here the comments from the other spreadtrum driver
applied. (Banks instead of groups)

> +/*
> + * The PMIC EIC controller only has one group, and each group now can contain
> + * 16 EICs.
> + */
> +#define SPRD_PMIC_EIC_PER_GROUP_NR     16

How can it have 16 groups if it has only one group? I don't understand...

> +#define SPRD_PMIC_EIC_DATA_MASK                GENMASK(15, 0)
> +#define SPRD_PMIC_EIC_BIT(x)           ((x) & (SPRD_PMIC_EIC_PER_GROUP_NR - 1))
> +#define SPRD_PMIC_EIC_CACHE_NR_REGS    (SPRD_EIC_DBNC_TRIG + 0x4)
> +
> +#define SPRD_EIC_DBNC_MASK             GENMASK(11, 0)
> +
> +/*
> + * The Spreadtrum EIC (external interrupt controller) can only be used
> + * as input mode to generate interrupts if detecting input signals.
> + *
> + * The Spreadtrum platform has two EIC controllers, one EIC controller
> + * is in digital-chip, another one is in PMIC. There are some differences
> + * between this two EIC controllers. The digital-chip EIC controller
> + * includes 4 sub-modules: debounce EIC, latch EIC, async EIC and sync EIC,
> + * but the PMIC EIC controller only has one debounce EIC sub-module.
> + *
> + * The debounce EIC is used to capture input signal's stable status
> + * (ms grade) and a single-trigger mechanism is introduced into this
> + * sub-module to enhance the input event detection reliability. The
> + * debounce range is from 1ms to 4s with the step of 1ms.
> + *
> + * The latch EIC is used to latch some special input signal and send
> + * interrupts to MCU core.
> + *
> + * The async EIC uses 32k clock to capture short signal (us grade)
> + * to generate interrupt to MCU by level or edge trigger.
> + *
> + * The sync EIC is similar with GPIO's input function.

Same text as in the DT binding, so since I had a lot of comments on
spelling and grammar, re-copy-and-paste after fixing up the binding
please.

> +enum sprd_eic_type {
> +       SPRD_EIC_DEBOUNCE,
> +       SPRD_EIC_LATCH,
> +       SPRD_EIC_ASYNC,
> +       SPRD_EIC_SYNC,
> +       SPRD_EIC_PMIC_DEBOUNCE,
> +       SPRD_EIC_MAX,
> +};

OK good

> +struct sprd_eic {
> +       struct gpio_chip chip;
> +       struct irq_chip intc;
> +       void __iomem *base[SPRD_EIC_MAX_GROUP];
> +       struct regmap *map;
> +       u32 map_base;
> +       u8 reg[SPRD_PMIC_EIC_CACHE_NR_REGS];
> +       enum sprd_eic_type type;
> +       spinlock_t lock;
> +       struct mutex buslock;
> +       int irq;
> +};

This might need some kerneldoc. The regmap is not evident.

> +struct sprd_eic_data {
> +       enum sprd_eic_type type;
> +       u32 num_eics;
> +};

Maybe name it sprd_eic_variant_data so it is clear that this is a
data container for the variants. Maybe add some kerneldoc to make
it clear?

> +static const char *sprd_eic_label_name[SPRD_EIC_MAX] = {
> +       "eic-debounce", "eic-latch", "eic-async",
> +       "eic-sync", "eic-pmic-debounce",
> +};
> +
> +static const struct sprd_eic_data sc9860_eic_dbnc_data = {
> +       .type = SPRD_EIC_DEBOUNCE,
> +       .num_eics = 8,
> +};
> +
> +static const struct sprd_eic_data sc9860_eic_latch_data = {
> +       .type = SPRD_EIC_LATCH,
> +       .num_eics = 8,
> +};
> +
> +static const struct sprd_eic_data sc9860_eic_async_data = {
> +       .type = SPRD_EIC_ASYNC,
> +       .num_eics = 8,
> +};
> +
> +static const struct sprd_eic_data sc9860_eic_sync_data = {
> +       .type = SPRD_EIC_SYNC,
> +       .num_eics = 8,
> +};
> +
> +static const struct sprd_eic_data pmic_eic_dbnc_data = {
> +       .type = SPRD_EIC_PMIC_DEBOUNCE,
> +       .num_eics = 16,
> +};

Makes sense.

> +static inline void __iomem *sprd_eic_group_base(struct sprd_eic *sprd_eic,
> +                                               unsigned int group)
> +{
> +       if (group >= SPRD_EIC_MAX_GROUP)
> +               return NULL;
> +
> +       return sprd_eic->base[group];
> +}

Same comment as the other driver, maybe just use offset as parameter
to this instead of group?

Maybe rename sprd_eic_offset_base()?

> +static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
> +                           unsigned int reg, unsigned int val)
> +{
> +       struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
> +       u32 shift;
> +
> +       if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
> +               shift = SPRD_PMIC_EIC_BIT(offset);
> +
> +               regmap_update_bits(sprd_eic->map, sprd_eic->map_base + reg,
> +                                  BIT(shift), val << shift);

So since everything about the PMIC debounce is handled separately
since it goes through regmap, I think maybe you should
simply make that its own driver to make this driver cleaner?

> +       } else {
> +               void __iomem *base = sprd_eic_group_base(sprd_eic,
> +                                               offset / SPRD_EIC_PER_GROUP_NR);
> +               unsigned long flags;
> +               u32 orig, tmp;
> +
> +               shift = SPRD_EIC_BIT(offset);
> +
> +               spin_lock_irqsave(&sprd_eic->lock, flags);
> +               orig = readl_relaxed(base + reg);
> +
> +               tmp = (orig & ~BIT(shift)) | (val << shift);
> +               writel_relaxed(tmp, base + reg);
> +               spin_unlock_irqrestore(&sprd_eic->lock, flags);
> +       }

On this read-modify-write algoritm I have the same comments
as on the other driver.

> +static int sprd_eic_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 1);
> +       return 0;
> +}
> +
> +static void sprd_eic_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 0);
> +}
> +
> +static int sprd_eic_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       return sprd_eic_read(chip, offset, SPRD_EIC_DBNC_DATA);
> +}
> +
> +static int sprd_eic_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       /* EICs are always input, nothing need to do here. */
> +       return 0;
> +}

Implement sprd_eic_direction_output() as well and make it
return -EIO every time. So setting as output fails.

> +
> +static void sprd_eic_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       /* EICs are always input, nothing need to do here. */
> +}

So only the debounce version really supports GPIO read.

And for the other versions, all you really do is use gpiolib to
get an irqchip in a convenient way?

I'm not so sure about this.

I think it is more appropriate to just have a GPIO driver for
the first variant in that case, and then put the support
for the remaining variants into drivers/irqchip/* since
they can only do IRQ.

If they are supposed to be present in drivers/gpio/* they should
at least be able to read a value, SPRD_EIC_LATCH_INTRAW
etc if nothing else. Else I think it is a real bad fit for those.

But then comes the fact that it seems all of them have
debounce. So I'm a bit confused. The irqchips don't have
debounce, so in that case it makes a bit of sense.

But again: you can't really use the debounce properties from
the irqchip side, and then I think the right thing to do is
to use drivers/irqchip, and add debouncing support to
the irqchip infrastructure so you can use it directly on the
irqchip.

> +static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +                                unsigned int debounce)
> +{
> +       struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
> +       u32 shift, reg, value;
> +
> +       if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
> +               int ret;
> +
> +               shift = SPRD_PMIC_EIC_BIT(offset);
> +               reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
> +               ret = regmap_read(sprd_eic->map, sprd_eic->map_base + reg,
> +                                 &value);
> +               if (ret)
> +                       return ret;
> +
> +               value &= ~SPRD_EIC_DBNC_MASK;
> +               value |= debounce / 1000;
> +               ret = regmap_write(sprd_eic->map, sprd_eic->map_base + reg,
> +                                  value);
> +               if (ret)
> +                       return ret;

This illustrated I think again why the PMIC should have its
own driver.

> +       } else {
> +               void __iomem *base = sprd_eic_group_base(sprd_eic,
> +                                               offset / SPRD_EIC_PER_GROUP_NR);
> +
> +               shift = SPRD_EIC_BIT(offset);
> +               reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
> +               value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
> +
> +               value |= debounce / 1000;
> +               writel_relaxed(value, base + reg);
> +       }

So these variants, also those which are not named *DEBOUNCE
still has a debounce register?

That is pretty confusing.

> +static int sprd_eic_set_config(struct gpio_chip *chip, unsigned int offset,
> +                              unsigned long config)
> +{
> +       unsigned long param = pinconf_to_config_param(config);
> +       u32 arg = pinconf_to_config_argument(config);
> +
> +       if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> +               return sprd_eic_set_debounce(chip, offset, arg);

So this is supported on *all* variants?

Does this mean that you have a number of "gpios" which
can only handle interrupts, have debounce but you can never
read the value from them?

Apart from these design questions it looks all right,
but we really need to think about if this should rather be,
at least partially, inside drivers/irqchip.

I understand it is convenient to use the infrastructure inside
drivers/gpio/* but if it means things that are not even GPIO
start to be implemented as "GPIO" then we are a victim of
our own success.

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[Rob Herring]

On Thu, Feb 08, 2018 at 04:01:42PM +0800, Baolin Wang wrote:
> This patch adds the device tree bindings for the Spreadtrum EIC
> controller. The EIC can be recognized as one special type of GPIO,
> which can only be used as input.
> 
> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/gpio/gpio-eic-sprd.txt     |   51 ++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
> new file mode 100644
> index 0000000..34f194f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
> @@ -0,0 +1,51 @@
> +Spreadtrum EIC controller bindings
> +
> +The EIC is the abbreviation of external interrupt controller, which
> +is only can be used as input mode. The EIC controller includes 4
> +sub-modules: EIC-Debounce, EIC-Latch, EIC-Async, EIC-Sync.
> +
> +The EIC-debounce sub-module provides up to 8 source input signal
> +connection. A debounce machanism is used to capture input signal's
> +stable status (ms grade) and a single-trigger mechanism is introduced
> +into this sub-module to enhance the input event detection reliability.
> +In addition, this sub-module's clock can be shut-off automatically to
> +reduce power dissipation. The debounce range is from 1ms to 4s with
> +the step of 1ms. If the input signal is shorter than 1ms, it will be
> +omitted as this sub-module.
> +
> +The EIC-latch sub-module is used to latch some special input signal
> +and send interrupts to MCU core, and it can provide up to 8 latch
> +source input signal connection.
> +
> +The EIC-async sub-module uses 32k clock to capture short signal (us
> +grade) to generate interrupt to MCU by level or edge trigger.
> +
> +The EIC-sync is similar with GPIO's input function.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> +  "sprd,sc9860-eic-debounce",
> +  "sprd,sc9860-eic-latch",
> +  "sprd,sc9860-eic-async",
> +  "sprd,sc9860-eic-sync",
> +  "sprd,sc27xx-eic-debounce".
> +- reg: Define the base and range of the I/O address space containing
> +  the GPIO controller registers.
> +- gpio-controller: Marks the device node as a GPIO controller.

Why a gpio controller? Do you read the input pin state?

> +- #gpio-cells: Should be <2>. The first cell is the gpio number and
> +  the second cell is used to specify optional parameters.
> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- #interrupt-cells: Should be <2>. Specifies the number of cells needed
> +  to encode interrupt source.
> +- interrupts: Should be the port interrupt shared by all the gpios.
> +
> +Example:
> +	eic_debounce: eic@40210000 {

interrupt-controller@...

Or (if this remains a gpio controller)

gpio@...

> +		compatible = "sprd,sc9860-eic-debounce";
> +		reg = <0 0x40210000 0 0x80>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;

The example doesn't seem to match your description of the block or 
blocks. You talk about a bunch of sub modules and then there's just this 
one node?

> +	};
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[Baolin Wang]

Hi Linus,

On 13 February 2018 at 16:28, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Baolin!
>
> Thank you for your patch.
>
> On Thu, Feb 8, 2018 at 9:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> This patch adds the device tree bindings for the Spreadtrum EIC
>> controller. The EIC can be recognized as one special type of GPIO,
>
> s/recognized as one/seen as a/g

Sorry for the typos.

>
>> which can only be used as input.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
>> +The EIC is the abbreviation of external interrupt controller, which
>> +is only can be used as input mode. The EIC controller includes 4
>
> can be used only in input mode.

OK.

>
>> +sub-modules: EIC-Debounce, EIC-Latch, EIC-Async, EIC-Sync.
>
> Are they four sub-modules that are always synthesized into the
> silicon at the same time, or do you mean that when producing the
> hardware, the designer will choose one of these four types (it looks
> like that from the example).

Usually they are all synthesized into the silicon, but not always. For
our PMIC EIC, we only have one debounce EIC.

>
>> +
>> +The EIC-debounce sub-module provides up to 8 source input signal
>> +connection.
>
> connections.
>
>> A debounce machanism is used to
>
> mechanism
>
>> capture input signal's
>
> capture the input signals'

Sure.

>
> (note plural signals genitive)
>
>> +stable status (ms grade)
>
> is that millisecond resolution you mean?

Yes.

>
>> and a single-trigger mechanism is introduced
>> +into this sub-module to enhance the input event detection reliability.
>> +In addition, this sub-module's clock can be shut-off automatically to
>
> no dash in "shut off"

OK.

>
>> +reduce power dissipation. The debounce range is from 1ms to 4s with
>> +the step of 1ms.
>
> a step size of

OK.

>
>> If the input signal is shorter than 1ms, it will be
>> +omitted as this sub-module.
>
> I don't understand the last part, do you mean the signal will be ignored
> if it is asserted for less than 1 ms?

Yes, sorry for confusing, and I will modify this part.

>
>> +The EIC-latch sub-module is used to latch some special input signal
>
> signals (plural)
>
> What is special about them?

Ah, I will describe them in next version after making sure with my colleagues.

>
>> +and send interrupts to MCU core, and it can provide up to 8 latch
>> +source input signal connection.
>
> connections (plural)
>
>> +The EIC-async sub-module uses 32k clock
>
> a 32kHz clock
>
>> to capture short signal
>
> to capture the short signal

OK.

>
>> +(us grade)
>
> Do you mean "microsecond granularity"?

Yes.

>
>> to generate interrupt to MCU by level or edge trigger.
>
> What is MCU? I think you can just omit it, it could be integrated
> elsewhere.

Sure, I will remove it.

>
>> +The EIC-sync is similar with GPIO's input function.
>
> Do you mean that the EIC-sync module is a synchronized signal input
> register? Please write that.

Yes. OK.

>
>> +Required properties:
>> +- compatible: Should be one of the following:
>> +  "sprd,sc9860-eic-debounce",
>> +  "sprd,sc9860-eic-latch",
>> +  "sprd,sc9860-eic-async",
>> +  "sprd,sc9860-eic-sync",
>> +  "sprd,sc27xx-eic-debounce".
>
> So it looks like there is one at the time, so in the SC9860
> all four modules exist, but at different addresses?

Yes.

>
> (...)
>> +Example:
>> +       eic_debounce: eic@40210000 {
>> +               compatible = "sprd,sc9860-eic-debounce";
>> +               reg = <0 0x40210000 0 0x80>;
>
> So does this mean that this is a debounced-only EIC?

Yes.

>
> There are latch, async and sync versions somewhere else in
> memory? Or there could be? And they are never say debounce
> and latch at the same time? Etc?

I did not list latch, async, and sync EIC. They are different
sub-modules and they can be listed at the same time. I will add other
device nodes in next version. Thanks for your comments.

>
> Yours,
> Linus Walleij



-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] dt-bindings: gpio: Add Spreadtrum EIC controller documentation

[Baolin Wang]

Hi Rob,

On 14 February 2018 at 07:39, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Feb 08, 2018 at 04:01:42PM +0800, Baolin Wang wrote:
>> This patch adds the device tree bindings for the Spreadtrum EIC
>> controller. The EIC can be recognized as one special type of GPIO,
>> which can only be used as input.
>>
>> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  .../devicetree/bindings/gpio/gpio-eic-sprd.txt     |   51 ++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
>> new file mode 100644
>> index 0000000..34f194f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-eic-sprd.txt
>> @@ -0,0 +1,51 @@
>> +Spreadtrum EIC controller bindings
>> +
>> +The EIC is the abbreviation of external interrupt controller, which
>> +is only can be used as input mode. The EIC controller includes 4
>> +sub-modules: EIC-Debounce, EIC-Latch, EIC-Async, EIC-Sync.
>> +
>> +The EIC-debounce sub-module provides up to 8 source input signal
>> +connection. A debounce machanism is used to capture input signal's
>> +stable status (ms grade) and a single-trigger mechanism is introduced
>> +into this sub-module to enhance the input event detection reliability.
>> +In addition, this sub-module's clock can be shut-off automatically to
>> +reduce power dissipation. The debounce range is from 1ms to 4s with
>> +the step of 1ms. If the input signal is shorter than 1ms, it will be
>> +omitted as this sub-module.
>> +
>> +The EIC-latch sub-module is used to latch some special input signal
>> +and send interrupts to MCU core, and it can provide up to 8 latch
>> +source input signal connection.
>> +
>> +The EIC-async sub-module uses 32k clock to capture short signal (us
>> +grade) to generate interrupt to MCU by level or edge trigger.
>> +
>> +The EIC-sync is similar with GPIO's input function.
>> +
>> +Required properties:
>> +- compatible: Should be one of the following:
>> +  "sprd,sc9860-eic-debounce",
>> +  "sprd,sc9860-eic-latch",
>> +  "sprd,sc9860-eic-async",
>> +  "sprd,sc9860-eic-sync",
>> +  "sprd,sc27xx-eic-debounce".
>> +- reg: Define the base and range of the I/O address space containing
>> +  the GPIO controller registers.
>> +- gpio-controller: Marks the device node as a GPIO controller.
>
> Why a gpio controller? Do you read the input pin state?

We always regard it as GPIO except it only can be used as input mode.
Like debounce eic, we still need request it before using it.

>
>> +- #gpio-cells: Should be <2>. The first cell is the gpio number and
>> +  the second cell is used to specify optional parameters.
>> +- interrupt-controller: Marks the device node as an interrupt controller.
>> +- #interrupt-cells: Should be <2>. Specifies the number of cells needed
>> +  to encode interrupt source.
>> +- interrupts: Should be the port interrupt shared by all the gpios.
>> +
>> +Example:
>> +     eic_debounce: eic@40210000 {
>
> interrupt-controller@...
>
> Or (if this remains a gpio controller)
>
> gpio@...

OK.

>
>> +             compatible = "sprd,sc9860-eic-debounce";
>> +             reg = <0 0x40210000 0 0x80>;
>> +             gpio-controller;
>> +             #gpio-cells = <2>;
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>;
>> +             interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
>
> The example doesn't seem to match your description of the block or
> blocks. You talk about a bunch of sub modules and then there's just this
> one node?

I just list one as one example, and I will list all of them in next
version. Thanks.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] gpio: Add Spreadtrum EIC driver support

[Baolin Wang]

Hi Linus,

On 13 February 2018 at 17:18, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Baolin,
>
> thank you for your patch!
>
> On Thu, Feb 8, 2018 at 9:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> The Spreadtrum platform has 2 EIC controllers, one is in digital chip and
>> another one is in PMIC. The digital-chip EIC controller has 4 sub-modules:
>> debounce EIC, latch EIC, async EIC and sync EIC, each sub-module can has
>> multiple groups and each group contains 8 EICs. The PMIC EIC controller has
>> only one debounce EIC sub-module.
>
> OK it seems they are at different memory addresses and totally
> separate instances then. I start to understand!

Yes.

>
>> Each EIC can only be used as input mode, and has the capability to trigger
>> interrupts when detecting input signals.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> +config GPIO_EIC_SPRD
>> +       tristate "Spreadtrum EIC support"
>> +       depends on ARCH_SPRD || COMPILE_TEST
>
> depend on OF_GPIO

Sure.

>
>> +       select GPIOLIB_IRQCHIP
>> +       help
>> +         Say yes here to support Spreadtrum EIC device.
>
>> +/* EIC registers definition */
>
> Aha I see so the register map is different depending on which
> type is synthesized.

Yes.

>
>> +#define SPRD_EIC_DBNC_DATA             0x0
>> +#define SPRD_EIC_DBNC_DMSK             0x4
>> +#define SPRD_EIC_DBNC_IEV              0x14
>> +#define SPRD_EIC_DBNC_IE               0x18
>> +#define SPRD_EIC_DBNC_RIS              0x1c
>> +#define SPRD_EIC_DBNC_MIS              0x20
>> +#define SPRD_EIC_DBNC_IC               0x24
>> +#define SPRD_EIC_DBNC_TRIG             0x28
>> +#define SPRD_EIC_DBNC_CTRL0            0x40
>> +#define SPRD_EIC_DBNC_CTRL1            0x44
>> +#define SPRD_EIC_DBNC_CTRL2            0x48
>> +#define SPRD_EIC_DBNC_CTRL3            0x4c
>> +#define SPRD_EIC_DBNC_CTRL4            0x50
>> +#define SPRD_EIC_DBNC_CTRL5            0x54
>> +#define SPRD_EIC_DBNC_CTRL6            0x58
>> +#define SPRD_EIC_DBNC_CTRL7            0x5c
>
> It looks from the code below that register 0x40 thru 0x5c
> are actually present on all the variants, not just the
> debounce DBNC variant.

No. these control registers are only used for debounce eic.

>
> This is strange, then they should not be named
> with the *_DBNC_* infix but instead something
> neutral like SPRD_CTRL0 0x40 etc.
>
>> +#define SPRD_EIC_LATCH_INTEN           0x0
>> +#define SPRD_EIC_LATCH_INTRAW          0x4
>> +#define SPRD_EIC_LATCH_INTMSK          0x8
>> +#define SPRD_EIC_LATCH_INTCLR          0xc
>> +#define SPRD_EIC_LATCH_INTPOL          0x10
>> +#define SPRD_EIC_LATCH_INTMODE         0x14
>> +
>> +#define SPRD_EIC_ASYNC_INTIE           0x0
>> +#define SPRD_EIC_ASYNC_INTRAW          0x4
>> +#define SPRD_EIC_ASYNC_INTMSK          0x8
>> +#define SPRD_EIC_ASYNC_INTCLR          0xc
>> +#define SPRD_EIC_ASYNC_INTMODE         0x10
>> +#define SPRD_EIC_ASYNC_INTBOTH         0x14
>> +#define SPRD_EIC_ASYNC_INTPOL          0x18
>> +#define SPRD_EIC_ASYNC_DATA            0x1c
>> +
>> +#define SPRD_EIC_SYNC_INTIE            0x0
>> +#define SPRD_EIC_SYNC_INTRAW           0x4
>> +#define SPRD_EIC_SYNC_INTMSK           0x8
>> +#define SPRD_EIC_SYNC_INTCLR           0xc
>> +#define SPRD_EIC_SYNC_INTMODE          0x10
>> +#define SPRD_EIC_SYNC_INTBOTH          0x14
>> +#define SPRD_EIC_SYNC_INTPOL           0x18
>> +#define SPRD_EIC_SYNC_DATA             0x1c
>> +
>> +/*
>> + * The digital-chip EIC controller can support maximum 3 groups, and each group
>> + * contains 8 EICs.
>> + */
>> +#define SPRD_EIC_MAX_GROUP             3
>> +#define SPRD_EIC_PER_GROUP_NR          8
>> +#define SPRD_EIC_DATA_MASK             GENMASK(7, 0)
>> +#define SPRD_EIC_BIT(x)                        ((x) & (SPRD_EIC_PER_GROUP_NR - 1))
>
> Here the comments from the other spreadtrum driver
> applied. (Banks instead of groups)

Sure.

>
>> +/*
>> + * The PMIC EIC controller only has one group, and each group now can contain
>> + * 16 EICs.
>> + */
>> +#define SPRD_PMIC_EIC_PER_GROUP_NR     16
>
> How can it have 16 groups if it has only one group? I don't understand...

PMIC EIC is designed differently, it only has one group and it can
support maximum 16 EICs. I can not say why, but that is what we
design.

>
>> +#define SPRD_PMIC_EIC_DATA_MASK                GENMASK(15, 0)
>> +#define SPRD_PMIC_EIC_BIT(x)           ((x) & (SPRD_PMIC_EIC_PER_GROUP_NR - 1))
>> +#define SPRD_PMIC_EIC_CACHE_NR_REGS    (SPRD_EIC_DBNC_TRIG + 0x4)
>> +
>> +#define SPRD_EIC_DBNC_MASK             GENMASK(11, 0)
>> +
>> +/*
>> + * The Spreadtrum EIC (external interrupt controller) can only be used
>> + * as input mode to generate interrupts if detecting input signals.
>> + *
>> + * The Spreadtrum platform has two EIC controllers, one EIC controller
>> + * is in digital-chip, another one is in PMIC. There are some differences
>> + * between this two EIC controllers. The digital-chip EIC controller
>> + * includes 4 sub-modules: debounce EIC, latch EIC, async EIC and sync EIC,
>> + * but the PMIC EIC controller only has one debounce EIC sub-module.
>> + *
>> + * The debounce EIC is used to capture input signal's stable status
>> + * (ms grade) and a single-trigger mechanism is introduced into this
>> + * sub-module to enhance the input event detection reliability. The
>> + * debounce range is from 1ms to 4s with the step of 1ms.
>> + *
>> + * The latch EIC is used to latch some special input signal and send
>> + * interrupts to MCU core.
>> + *
>> + * The async EIC uses 32k clock to capture short signal (us grade)
>> + * to generate interrupt to MCU by level or edge trigger.
>> + *
>> + * The sync EIC is similar with GPIO's input function.
>
> Same text as in the DT binding, so since I had a lot of comments on
> spelling and grammar, re-copy-and-paste after fixing up the binding
> please.

Sure.

>
>> +enum sprd_eic_type {
>> +       SPRD_EIC_DEBOUNCE,
>> +       SPRD_EIC_LATCH,
>> +       SPRD_EIC_ASYNC,
>> +       SPRD_EIC_SYNC,
>> +       SPRD_EIC_PMIC_DEBOUNCE,
>> +       SPRD_EIC_MAX,
>> +};
>
> OK good
>
>> +struct sprd_eic {
>> +       struct gpio_chip chip;
>> +       struct irq_chip intc;
>> +       void __iomem *base[SPRD_EIC_MAX_GROUP];
>> +       struct regmap *map;
>> +       u32 map_base;
>> +       u8 reg[SPRD_PMIC_EIC_CACHE_NR_REGS];
>> +       enum sprd_eic_type type;
>> +       spinlock_t lock;
>> +       struct mutex buslock;
>> +       int irq;
>> +};
>
> This might need some kerneldoc. The regmap is not evident.

OK, will add some documentation.

>
>> +struct sprd_eic_data {
>> +       enum sprd_eic_type type;
>> +       u32 num_eics;
>> +};
>
> Maybe name it sprd_eic_variant_data so it is clear that this is a
> data container for the variants. Maybe add some kerneldoc to make
> it clear?

Sure, change to sprd_eic_variant_data and doc them.

>
>> +static const char *sprd_eic_label_name[SPRD_EIC_MAX] = {
>> +       "eic-debounce", "eic-latch", "eic-async",
>> +       "eic-sync", "eic-pmic-debounce",
>> +};
>> +
>> +static const struct sprd_eic_data sc9860_eic_dbnc_data = {
>> +       .type = SPRD_EIC_DEBOUNCE,
>> +       .num_eics = 8,
>> +};
>> +
>> +static const struct sprd_eic_data sc9860_eic_latch_data = {
>> +       .type = SPRD_EIC_LATCH,
>> +       .num_eics = 8,
>> +};
>> +
>> +static const struct sprd_eic_data sc9860_eic_async_data = {
>> +       .type = SPRD_EIC_ASYNC,
>> +       .num_eics = 8,
>> +};
>> +
>> +static const struct sprd_eic_data sc9860_eic_sync_data = {
>> +       .type = SPRD_EIC_SYNC,
>> +       .num_eics = 8,
>> +};
>> +
>> +static const struct sprd_eic_data pmic_eic_dbnc_data = {
>> +       .type = SPRD_EIC_PMIC_DEBOUNCE,
>> +       .num_eics = 16,
>> +};
>
> Makes sense.
>
>> +static inline void __iomem *sprd_eic_group_base(struct sprd_eic *sprd_eic,
>> +                                               unsigned int group)
>> +{
>> +       if (group >= SPRD_EIC_MAX_GROUP)
>> +               return NULL;
>> +
>> +       return sprd_eic->base[group];
>> +}
>
> Same comment as the other driver, maybe just use offset as parameter
> to this instead of group?
>
> Maybe rename sprd_eic_offset_base()?

Like I said, in some situation, we can only pass the group number as parameter.

>
>> +static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
>> +                           unsigned int reg, unsigned int val)
>> +{
>> +       struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
>> +       u32 shift;
>> +
>> +       if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
>> +               shift = SPRD_PMIC_EIC_BIT(offset);
>> +
>> +               regmap_update_bits(sprd_eic->map, sprd_eic->map_base + reg,
>> +                                  BIT(shift), val << shift);
>
> So since everything about the PMIC debounce is handled separately
> since it goes through regmap, I think maybe you should
> simply make that its own driver to make this driver cleaner?

I also thought about one new driver, but they are all named EIC and
have the same registers description, so ...

But if you have strong opinion to split the PMIC debounce EIC into one
new driver, I will do that.

>
>> +       } else {
>> +               void __iomem *base = sprd_eic_group_base(sprd_eic,
>> +                                               offset / SPRD_EIC_PER_GROUP_NR);
>> +               unsigned long flags;
>> +               u32 orig, tmp;
>> +
>> +               shift = SPRD_EIC_BIT(offset);
>> +
>> +               spin_lock_irqsave(&sprd_eic->lock, flags);
>> +               orig = readl_relaxed(base + reg);
>> +
>> +               tmp = (orig & ~BIT(shift)) | (val << shift);
>> +               writel_relaxed(tmp, base + reg);
>> +               spin_unlock_irqrestore(&sprd_eic->lock, flags);
>> +       }
>
> On this read-modify-write algoritm I have the same comments
> as on the other driver.

Make sense.

>
>> +static int sprd_eic_request(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 1);
>> +       return 0;
>> +}
>> +
>> +static void sprd_eic_free(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       sprd_eic_update(chip, offset, SPRD_EIC_DBNC_DMSK, 0);
>> +}
>> +
>> +static int sprd_eic_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       return sprd_eic_read(chip, offset, SPRD_EIC_DBNC_DATA);
>> +}
>> +
>> +static int sprd_eic_direction_input(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       /* EICs are always input, nothing need to do here. */
>> +       return 0;
>> +}
>
> Implement sprd_eic_direction_output() as well and make it
> return -EIO every time. So setting as output fails.

Yes, will do.

>
>> +
>> +static void sprd_eic_set(struct gpio_chip *chip, unsigned int offset, int value)
>> +{
>> +       /* EICs are always input, nothing need to do here. */
>> +}
>
> So only the debounce version really supports GPIO read.
>
> And for the other versions, all you really do is use gpiolib to
> get an irqchip in a convenient way?
>
> I'm not so sure about this.
>
> I think it is more appropriate to just have a GPIO driver for
> the first variant in that case, and then put the support
> for the remaining variants into drivers/irqchip/* since
> they can only do IRQ.
>
> If they are supposed to be present in drivers/gpio/* they should
> at least be able to read a value, SPRD_EIC_LATCH_INTRAW
> etc if nothing else. Else I think it is a real bad fit for those.
>
> But then comes the fact that it seems all of them have
> debounce. So I'm a bit confused. The irqchips don't have
> debounce, so in that case it makes a bit of sense.
>
> But again: you can't really use the debounce properties from
> the irqchip side, and then I think the right thing to do is
> to use drivers/irqchip, and add debouncing support to
> the irqchip infrastructure so you can use it directly on the
> irqchip.

Yes, we have debounce EIC, this type EIC need be requested firstly and
can be read. Only latch EIC can not be read. So I still think put them
in drivers/gpio/* is more suitable, they have most of GPIO features
except they can only be used as input mode.

>
>> +static int sprd_eic_set_debounce(struct gpio_chip *chip, unsigned int offset,
>> +                                unsigned int debounce)
>> +{
>> +       struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
>> +       u32 shift, reg, value;
>> +
>> +       if (sprd_eic->type == SPRD_EIC_PMIC_DEBOUNCE) {
>> +               int ret;
>> +
>> +               shift = SPRD_PMIC_EIC_BIT(offset);
>> +               reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
>> +               ret = regmap_read(sprd_eic->map, sprd_eic->map_base + reg,
>> +                                 &value);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               value &= ~SPRD_EIC_DBNC_MASK;
>> +               value |= debounce / 1000;
>> +               ret = regmap_write(sprd_eic->map, sprd_eic->map_base + reg,
>> +                                  value);
>> +               if (ret)
>> +                       return ret;
>
> This illustrated I think again why the PMIC should have its
> own driver.
>
>> +       } else {
>> +               void __iomem *base = sprd_eic_group_base(sprd_eic,
>> +                                               offset / SPRD_EIC_PER_GROUP_NR);
>> +
>> +               shift = SPRD_EIC_BIT(offset);
>> +               reg = SPRD_EIC_DBNC_CTRL0 + shift * 0x4;
>> +               value = readl_relaxed(base + reg) & ~SPRD_EIC_DBNC_MASK;
>> +
>> +               value |= debounce / 1000;
>> +               writel_relaxed(value, base + reg);
>> +       }
>
> So these variants, also those which are not named *DEBOUNCE
> still has a debounce register?
>
> That is pretty confusing.

Only debounce EICs are implemented the set_config() intrface.

>
>> +static int sprd_eic_set_config(struct gpio_chip *chip, unsigned int offset,
>> +                              unsigned long config)
>> +{
>> +       unsigned long param = pinconf_to_config_param(config);
>> +       u32 arg = pinconf_to_config_argument(config);
>> +
>> +       if (param == PIN_CONFIG_INPUT_DEBOUNCE)
>> +               return sprd_eic_set_debounce(chip, offset, arg);
>
> So this is supported on *all* variants?

No, only for debounce EIC.

>
> Does this mean that you have a number of "gpios" which
> can only handle interrupts, have debounce but you can never
> read the value from them?
>
> Apart from these design questions it looks all right,
> but we really need to think about if this should rather be,
> at least partially, inside drivers/irqchip.
>
> I understand it is convenient to use the infrastructure inside
> drivers/gpio/* but if it means things that are not even GPIO
> start to be implemented as "GPIO" then we are a victim of
> our own success.

But we have lots of GPIO features, and we need request debounce EIC
firstly, and we can read EIC except latch EIC. Moreover we use EIC to
be gpio-keys, if we implement them in drivers/irqchip, it will be hard
to use for other drivers. Please think about our special GPIOs (EIC).
Thanks.

-- 
Baolin.wang
Best Regards