diff mbox series

[v2,3/3] gpio: Add Spreadtrum PMIC EIC driver support

Message ID f4a2fefb4c3179e7598dbc8141a661833200b922.1519468782.git.baolin.wang@linaro.org
State Superseded
Headers show
Series None | expand

Commit Message

(Exiting) Baolin Wang Feb. 24, 2018, 10:44 a.m. UTC
The Spreadtrum PMIC EIC controller contains only one bank of debounce EIC,
and this bank contains 16 EICs. Each EIC can only be used as input mode,
as well as supporting the debounce and the capability to trigger interrupts
when detecting input signals.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
Changes since v1:
 - New patch in v2.
---
 drivers/gpio/Kconfig              |    8 +
 drivers/gpio/Makefile             |    1 +
 drivers/gpio/gpio-pmic-eic-sprd.c |  328 +++++++++++++++++++++++++++++++++++++
 3 files changed, 337 insertions(+)
 create mode 100644 drivers/gpio/gpio-pmic-eic-sprd.c

-- 
1.7.9.5

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

Comments

(Exiting) Baolin Wang Feb. 26, 2018, 3:01 a.m. UTC | #1
Hi Andy,

On 25 February 2018 at 20:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sat, Feb 24, 2018 at 12:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:

>> The Spreadtrum PMIC EIC controller contains only one bank of debounce EIC,

>> and this bank contains 16 EICs. Each EIC can only be used as input mode,

>> as well as supporting the debounce and the capability to trigger interrupts

>> when detecting input signals.

>

>> +/*

>> + * These registers are modified under the irq bus lock and cached to avoid

>> + * unnecessary writes in bus_sync_unlock.

>> + */

>> +enum { REG_IEV, REG_IE, REG_TRIG, CACHE_NR_REGS };

>

> One item per line.


Sure.

>

>> +static int sprd_pmic_eic_direction_input(struct gpio_chip *chip,

>> +                                        unsigned int offset)

>> +{

>> +       /* EICs are always input, nothing need to do here. */

>> +       return 0;

>> +}

>> +

>> +static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset,

>> +                             int value)

>> +{

>> +       /* EICs are always input, nothing need to do here. */

>> +}

>

> Remove both.

>

> Look at what GPIO core does.


I've checked the GPIO core, we need the
sprd_pmic_eic_direction_input() returns 0, since user can set GPIOD_IN
flag when requesting one GPIO, otherwise it will return errors.
We also need one dummy sprd_pmic_eic_set() when setting debounce for
one GPIO, otherwise it will return errors.

>

>> +       value |= debounce / 1000;

>

> Possible overflow.


OK. I should & SPRD_PMIC_EIC_DBC_MASK.

>

>> +       for (n = 0; n < chip->ngpio; n++) {

>> +               if (!(BIT(n) & val))

>

> for_each_set_bit().

>

> At some point you may need just to go across lib/ in the kernel and

> see what we have there.


I've considered the for_each_set_bit(), it need one 'unsigned long'
type parameter, but we get the value from regmap is 'u32' type. So we
need one extra conversion from 'u32' to 'unsigned long' like:

unsigned long reg = val;

for_each_set_bit(n, &reg, chip->ngpio) {
        .......
}

If you like this conversion, then I can change to use
for_each_set_bit(). Thanks.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Exiting) Baolin Wang Feb. 27, 2018, 2:35 a.m. UTC | #2
On 26 February 2018 at 20:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 5:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

>> On 25 February 2018 at 20:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

>>> On Sat, Feb 24, 2018 at 12:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:

>

>>>> +static int sprd_pmic_eic_direction_input(struct gpio_chip *chip,

>>>> +                                        unsigned int offset)

>>>> +{

>>>> +       /* EICs are always input, nothing need to do here. */

>>>> +       return 0;

>>>> +}

>>>> +

>>>> +static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset,

>>>> +                             int value)

>>>> +{

>>>> +       /* EICs are always input, nothing need to do here. */

>>>> +}

>>>

>>> Remove both.

>>>

>>> Look at what GPIO core does.

>>

>> I've checked the GPIO core, we need the

>> sprd_pmic_eic_direction_input() returns 0, since user can set GPIOD_IN

>> flag when requesting one GPIO, otherwise it will return errors.

>

> Right. I thought it depends on presence of direction_output().

>

>> We also need one dummy sprd_pmic_eic_set() when setting debounce for

>> one GPIO, otherwise it will return errors.

>

> This is pretty much a "feature" of GPIO framework. It shouldn't

> require ->set() by logic if there is no output facility.

> OK.

>

>>>> +       for (n = 0; n < chip->ngpio; n++) {

>>>> +               if (!(BIT(n) & val))

>>>

>>> for_each_set_bit().

>>>

>>> At some point you may need just to go across lib/ in the kernel and

>>> see what we have there.

>>

>> I've considered the for_each_set_bit(), it need one 'unsigned long'

>> type parameter, but we get the value from regmap is 'u32' type. So we

>> need one extra conversion from 'u32' to 'unsigned long' like:

>>

>> unsigned long reg = val;

>>

>> for_each_set_bit(n, &reg, chip->ngpio) {

>>         .......

>> }

>>

>> If you like this conversion, then I can change to use

>> for_each_set_bit(). Thanks.

>

> Wouldn't it work like

>

> unsigned long val;

>

> ...regmap_read(..., &val);

>

> ?


It can not work, regmap_read() expects 'unsigned int *'. But I can
convert it like this:

for_each_set_bit(n, (unsigned long *)&val, chip->ngpio) {
         .......
}

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Feb. 27, 2018, 3:54 p.m. UTC | #3
On Tue, Feb 27, 2018 at 4:35 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 26 February 2018 at 20:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

>> On Mon, Feb 26, 2018 at 5:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

>>> On 25 February 2018 at 20:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:


>>>>> +       for (n = 0; n < chip->ngpio; n++) {

>>>>> +               if (!(BIT(n) & val))

>>>>

>>>> for_each_set_bit().

>>>>

>>>> At some point you may need just to go across lib/ in the kernel and

>>>> see what we have there.

>>>

>>> I've considered the for_each_set_bit(), it need one 'unsigned long'

>>> type parameter, but we get the value from regmap is 'u32' type. So we

>>> need one extra conversion from 'u32' to 'unsigned long' like:

>>>

>>> unsigned long reg = val;

>>>

>>> for_each_set_bit(n, &reg, chip->ngpio) {

>>>         .......

>>> }

>>>

>>> If you like this conversion, then I can change to use

>>> for_each_set_bit(). Thanks.

>>

>> Wouldn't it work like

>>

>> unsigned long val;

>>

>> ...regmap_read(..., &val);

>>

>> ?

>

> It can not work, regmap_read() expects 'unsigned int *'.


Ah, OK, than the temporary variable is a left approach.

> But I can

> convert it like this:

>

> for_each_set_bit(n, (unsigned long *)&val, chip->ngpio) {

>          .......

> }


No, this is a boilerplate for static analyzers and definitely UB.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c937407..f642314 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -380,6 +380,14 @@  config GPIO_PL061
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device
 
+config GPIO_PMIC_EIC_SPRD
+	tristate "Spreadtrum PMIC EIC support"
+	depends on MFD_SC27XX_PMIC || COMPILE_TEST
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support Spreadtrum PMIC EIC device.
+
 config GPIO_PXA
 	bool "PXA GPIO support"
 	depends on ARCH_PXA || ARCH_MMP
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 66eaa9e..50ba64b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -99,6 +99,7 @@  obj-$(CONFIG_GPIO_PCI_IDIO_16)	+= gpio-pci-idio-16.o
 obj-$(CONFIG_GPIO_PCIE_IDIO_24)	+= gpio-pcie-idio-24.o
 obj-$(CONFIG_GPIO_PISOSR)	+= gpio-pisosr.o
 obj-$(CONFIG_GPIO_PL061)	+= gpio-pl061.o
+obj-$(CONFIG_GPIO_PMIC_EIC_SPRD)	+= gpio-pmic-eic-sprd.o
 obj-$(CONFIG_GPIO_PXA)		+= gpio-pxa.o
 obj-$(CONFIG_GPIO_RC5T583)	+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RDC321X)	+= gpio-rdc321x.o
diff --git a/drivers/gpio/gpio-pmic-eic-sprd.c b/drivers/gpio/gpio-pmic-eic-sprd.c
new file mode 100644
index 0000000..ed5d6b2
--- /dev/null
+++ b/drivers/gpio/gpio-pmic-eic-sprd.c
@@ -0,0 +1,328 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Spreadtrum Communications Inc.
+ * Copyright (C) 2018 Linaro Ltd.
+ */
+
+#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>
+
+/* EIC registers definition */
+#define SPRD_PMIC_EIC_DATA		0x0
+#define SPRD_PMIC_EIC_DMSK		0x4
+#define SPRD_PMIC_EIC_IEV		0x14
+#define SPRD_PMIC_EIC_IE		0x18
+#define SPRD_PMIC_EIC_RIS		0x1c
+#define SPRD_PMIC_EIC_MIS		0x20
+#define SPRD_PMIC_EIC_IC		0x24
+#define SPRD_PMIC_EIC_TRIG		0x28
+#define SPRD_PMIC_EIC_CTRL0		0x40
+
+/*
+ * The PMIC EIC controller only has one bank, and each bank now can contain
+ * 16 EICs.
+ */
+#define SPRD_PMIC_EIC_PER_BANK_NR	16
+#define SPRD_PMIC_EIC_NR		SPRD_PMIC_EIC_PER_BANK_NR
+#define SPRD_PMIC_EIC_DATA_MASK		GENMASK(15, 0)
+#define SPRD_PMIC_EIC_BIT(x)		((x) & (SPRD_PMIC_EIC_PER_BANK_NR - 1))
+#define SPRD_PMIC_EIC_DBNC_MASK		GENMASK(11, 0)
+
+/*
+ * These registers are modified under the irq bus lock and cached to avoid
+ * unnecessary writes in bus_sync_unlock.
+ */
+enum { REG_IEV, REG_IE, REG_TRIG, CACHE_NR_REGS };
+
+/**
+ * struct sprd_pmic_eic - PMIC EIC controller
+ * @chip: the gpio_chip structure.
+ * @intc: the irq_chip structure.
+ * @regmap: the regmap from the parent device.
+ * @offset: the EIC controller's offset address of the PMIC.
+ * @reg: the array to cache the EIC registers.
+ * @buslock: for bus lock/sync and unlock.
+ * @irq: the interrupt number of the PMIC EIC conteroller.
+ */
+struct sprd_pmic_eic {
+	struct gpio_chip chip;
+	struct irq_chip intc;
+	struct regmap *map;
+	u32 offset;
+	u8 reg[CACHE_NR_REGS];
+	struct mutex buslock;
+	int irq;
+};
+
+static void sprd_pmic_eic_update(struct gpio_chip *chip, unsigned int offset,
+				 u16 reg, unsigned int val)
+{
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+	u32 shift = SPRD_PMIC_EIC_BIT(offset);
+
+	regmap_update_bits(pmic_eic->map, pmic_eic->offset + reg,
+			   BIT(shift), val << shift);
+}
+
+static int sprd_pmic_eic_read(struct gpio_chip *chip, unsigned int offset,
+			      u16 reg)
+{
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+	u32 value;
+	int ret;
+
+	ret = regmap_read(pmic_eic->map, pmic_eic->offset + reg, &value);
+	if (ret)
+		return ret;
+
+	return !!(value & BIT(SPRD_PMIC_EIC_BIT(offset)));
+}
+
+static int sprd_pmic_eic_request(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_DMSK, 1);
+	return 0;
+}
+
+static void sprd_pmic_eic_free(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_DMSK, 0);
+}
+
+static int sprd_pmic_eic_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return sprd_pmic_eic_read(chip, offset, SPRD_PMIC_EIC_DATA);
+}
+
+static int sprd_pmic_eic_direction_input(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	/* EICs are always input, nothing need to do here. */
+	return 0;
+}
+
+static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset,
+			      int value)
+{
+	/* EICs are always input, nothing need to do here. */
+}
+
+static int sprd_pmic_eic_set_debounce(struct gpio_chip *chip,
+				      unsigned int offset,
+				      unsigned int debounce)
+{
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+	u32 reg, value;
+	int ret;
+
+	reg = SPRD_PMIC_EIC_CTRL0 + SPRD_PMIC_EIC_BIT(offset) * 0x4;
+	ret = regmap_read(pmic_eic->map, pmic_eic->offset + reg, &value);
+	if (ret)
+		return ret;
+
+	value &= ~SPRD_PMIC_EIC_DBNC_MASK;
+	value |= debounce / 1000;
+	return regmap_write(pmic_eic->map, pmic_eic->offset + reg, value);
+}
+
+static int sprd_pmic_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_pmic_eic_set_debounce(chip, offset, arg);
+
+	return -ENOTSUPP;
+}
+
+static void sprd_pmic_eic_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+
+	pmic_eic->reg[REG_IE] = 0;
+	pmic_eic->reg[REG_TRIG] = 0;
+}
+
+static void sprd_pmic_eic_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+
+	pmic_eic->reg[REG_IE] = 1;
+	pmic_eic->reg[REG_TRIG] = 1;
+}
+
+static int sprd_pmic_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_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+
+	switch (flow_type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		pmic_eic->reg[REG_IEV] = 1;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		pmic_eic->reg[REG_IEV] = 0;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	irq_set_handler_locked(data, handle_level_irq);
+	return 0;
+}
+
+static void sprd_pmic_eic_bus_lock(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+
+	mutex_lock(&pmic_eic->buslock);
+}
+
+static void sprd_pmic_eic_bus_sync_unlock(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sprd_pmic_eic *pmic_eic = gpiochip_get_data(chip);
+	u32 offset = irqd_to_hwirq(data);
+
+	/* Set irq type */
+	sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_IEV,
+			     pmic_eic->reg[REG_IEV]);
+	/* Set irq unmask */
+	sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_IE,
+			     pmic_eic->reg[REG_IE]);
+	/* Generate trigger start pulse for debounce EIC */
+	sprd_pmic_eic_update(chip, offset, SPRD_PMIC_EIC_TRIG,
+			     pmic_eic->reg[REG_TRIG]);
+
+	mutex_unlock(&pmic_eic->buslock);
+}
+
+static irqreturn_t sprd_pmic_eic_irq_handler(int irq, void *data)
+{
+	struct sprd_pmic_eic *pmic_eic = data;
+	struct gpio_chip *chip = &pmic_eic->chip;
+	u32 n, girq, val;
+	int ret;
+
+	ret = regmap_read(pmic_eic->map, pmic_eic->offset + SPRD_PMIC_EIC_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;
+
+		/* Clear the interrupt */
+		sprd_pmic_eic_update(chip, n, SPRD_PMIC_EIC_IC, 1);
+
+		girq = irq_find_mapping(chip->irq.domain, n);
+		handle_nested_irq(girq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sprd_pmic_eic_probe(struct platform_device *pdev)
+{
+	struct gpio_irq_chip *irq;
+	struct sprd_pmic_eic *pmic_eic;
+	int ret;
+
+	pmic_eic = devm_kzalloc(&pdev->dev, sizeof(*pmic_eic), GFP_KERNEL);
+	if (!pmic_eic)
+		return -ENOMEM;
+
+	mutex_init(&pmic_eic->buslock);
+
+	pmic_eic->irq = platform_get_irq(pdev, 0);
+	if (pmic_eic->irq < 0) {
+		dev_err(&pdev->dev, "Failed to get PMIC EIC interrupt.\n");
+		return pmic_eic->irq;
+	}
+
+	pmic_eic->map = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!pmic_eic->map)
+		return -ENODEV;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "reg", &pmic_eic->offset);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get PMIC EIC base address.\n");
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, pmic_eic->irq, NULL,
+					sprd_pmic_eic_irq_handler,
+					IRQF_TRIGGER_LOW |
+					IRQF_ONESHOT | IRQF_NO_SUSPEND,
+					dev_name(&pdev->dev), pmic_eic);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request PMIC EIC IRQ.\n");
+		return ret;
+	}
+
+	pmic_eic->chip.label = dev_name(&pdev->dev);
+	pmic_eic->chip.ngpio = SPRD_PMIC_EIC_NR;
+	pmic_eic->chip.base = -1;
+	pmic_eic->chip.parent = &pdev->dev;
+	pmic_eic->chip.of_node = pdev->dev.of_node;
+	pmic_eic->chip.direction_input = sprd_pmic_eic_direction_input;
+	pmic_eic->chip.request = sprd_pmic_eic_request;
+	pmic_eic->chip.free = sprd_pmic_eic_free;
+	pmic_eic->chip.set_config = sprd_pmic_eic_set_config;
+	pmic_eic->chip.set = sprd_pmic_eic_set;
+	pmic_eic->chip.get = sprd_pmic_eic_get;
+
+	pmic_eic->intc.name = dev_name(&pdev->dev);
+	pmic_eic->intc.irq_mask = sprd_pmic_eic_irq_mask;
+	pmic_eic->intc.irq_unmask = sprd_pmic_eic_irq_unmask;
+	pmic_eic->intc.irq_set_type = sprd_pmic_eic_irq_set_type;
+	pmic_eic->intc.irq_bus_lock = sprd_pmic_eic_bus_lock;
+	pmic_eic->intc.irq_bus_sync_unlock = sprd_pmic_eic_bus_sync_unlock;
+	pmic_eic->intc.flags = IRQCHIP_SKIP_SET_WAKE;
+
+	irq = &pmic_eic->chip.irq;
+	irq->chip = &pmic_eic->intc;
+	irq->threaded = true;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &pmic_eic->chip, pmic_eic);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip %d.\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pmic_eic);
+	return 0;
+}
+
+static const struct of_device_id sprd_pmic_eic_of_match[] = {
+	{ .compatible = "sprd,sc27xx-eic", },
+	{ /* end of list */ }
+};
+MODULE_DEVICE_TABLE(of, sprd_pmic_eic_of_match);
+
+static struct platform_driver sprd_pmic_eic_driver = {
+	.probe = sprd_pmic_eic_probe,
+	.driver = {
+		.name = "sprd-pmic-eic",
+		.of_match_table	= sprd_pmic_eic_of_match,
+	},
+};
+
+module_platform_driver(sprd_pmic_eic_driver);
+
+MODULE_DESCRIPTION("Spreadtrum PMIC EIC driver");
+MODULE_LICENSE("GPL v2");