[PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Zhu, Lejun]

Devices based on Intel SoC products such as Baytrail have a Power
Management IC. In the PMIC there are subsystems for voltage regulation,
A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
Crystal Cove.

This patch adds support for the GPIO function in Crystal Cove.

v2:
- Use IRQ chip helper to provide irqdomain.
- Implement .remove and can now build as a module.
- Various fix for unreadable or ugly code pieces.
v3:
- More fix in irq_handler and probe.
v4:
- Minor fix of one return statement.

Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/Kconfig            |  13 ++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-crystalcove.c | 345 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 359 insertions(+)
 create mode 100644 drivers/gpio/gpio-crystalcove.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a86c49a..fed08d9d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -440,6 +440,19 @@ config GPIO_ARIZONA
 	help
 	  Support for GPIOs on Wolfson Arizona class devices.
 
+config GPIO_CRYSTAL_COVE
+	tristate "GPIO support for Crystal Cove PMIC"
+	depends on INTEL_SOC_PMIC
+	select GPIOLIB_IRQCHIP
+	help
+	  Support for GPIO pins on Crystal Cove PMIC.
+
+	  Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC
+	  inside.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-crystalcove.
+
 config GPIO_LP3943
 	tristate "TI/National Semiconductor LP3943 GPIO expander"
 	depends on MFD_LP3943
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6309aff..e6cd935 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
+obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
new file mode 100644
index 0000000..76b6d57
--- /dev/null
+++ b/drivers/gpio/gpio-crystalcove.c
@@ -0,0 +1,345 @@
+/*
+ * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
+ *
+ * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/sched.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/gpio.h>
+#include <linux/bitops.h>
+
+#define NUM_GPIO		16
+
+#define UPDATE_TYPE		BIT(0)
+#define UPDATE_MASK		BIT(1)
+
+#define GPIO0IRQ		0x0b
+#define GPIO1IRQ		0x0c
+#define MGPIO0IRQS0		0x19
+#define MGPIO1IRQS0		0x1a
+#define MGPIO0IRQSX		0x1b
+#define MGPIO1IRQSX		0x1c
+#define GPIO0P0CTLO		0x2b
+#define GPIO0P0CTLI		0x33
+#define GPIO1P0CTLO		0x3b
+#define GPIO1P0CTLI		0x43
+
+#define CTLI_INTCNT_NE		(1 << 1)
+#define CTLI_INTCNT_PE		(2 << 1)
+#define CTLI_INTCNT_BE		(3 << 1)
+
+#define CTLO_DIR_OUT		(1 << 5)
+
+#define CTLO_DRV_CMOS		(0 << 4)
+#define CTLO_DRV_OD		(1 << 4)
+
+#define CTLO_DRV_REN		(1 << 3)
+
+#define CTLO_RVAL_2KDW		(0)
+#define CTLO_RVAL_2KUP		(1 << 1)
+#define CTLO_RVAL_50KDW		(2 << 1)
+#define CTLO_RVAL_50KUP		(3 << 1)
+
+#define CTLO_INPUT_DEF	(CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
+#define CTLO_OUTPUT_DEF	(CTLO_DIR_OUT | CTLO_INPUT_DEF)
+
+#define GPIO_TO_CTL(gpio, dir)		\
+	((gpio < 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8))
+
+/**
+ * struct crystalcove_gpio - Crystal Cove GPIO controller
+ * @buslock: for bus lock/sync and unlock.
+ * @chip: the abstract gpio_chip structure.
+ * @update: pending IRQ setting update, to be written to the chip upon unlock.
+ * @trigger_type: the trigger type of the IRQ.
+ * @set_irq_mask: true if the IRQ mask needs to be set, false to clear.
+ */
+struct crystalcove_gpio {
+	struct mutex		buslock; /* irq_bus_lock */
+	struct gpio_chip	chip;
+	int			update;
+	int			trigger_type;
+	bool			set_irq_mask;
+};
+
+static void crystalcove_update_irq_mask(int gpio, bool mask)
+{
+	u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
+	int offset = gpio % 8;
+
+	if (mask)
+		intel_soc_pmic_setb(mirqs0, BIT(offset));
+	else
+		intel_soc_pmic_clearb(mirqs0, BIT(offset));
+}
+
+static void crystalcove_update_irq_type(int gpio, int type)
+{
+	u8 ctli = GPIO_TO_CTL(gpio, I);
+
+	type &= IRQ_TYPE_EDGE_BOTH;
+	intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
+	else if (type == IRQ_TYPE_EDGE_RISING)
+		intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
+	else if (type & IRQ_TYPE_EDGE_FALLING)
+		intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
+}
+
+static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
+					    unsigned gpio)
+{
+	u8 ctlo = GPIO_TO_CTL(gpio, O);
+
+	intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
+	return 0;
+}
+
+static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
+					     unsigned gpio, int value)
+{
+	u8 ctlo = GPIO_TO_CTL(gpio, O);
+
+	intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
+	return 0;
+}
+
+static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+	u8 ctli = GPIO_TO_CTL(gpio, I);
+
+	return intel_soc_pmic_readb(ctli) & 0x1;
+}
+
+static void crystalcove_gpio_set(struct gpio_chip *chip,
+				 unsigned gpio, int value)
+{
+	u8 ctlo = GPIO_TO_CTL(gpio, O);
+
+	if (value)
+		intel_soc_pmic_setb(ctlo, 1);
+	else
+		intel_soc_pmic_clearb(ctlo, 1);
+}
+
+static int crystalcove_irq_type(struct irq_data *data, unsigned type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct crystalcove_gpio *cg =
+		container_of(gc, struct crystalcove_gpio, chip);
+
+	cg->trigger_type = type;
+	cg->update |= UPDATE_TYPE;
+
+	return 0;
+}
+
+static void crystalcove_bus_lock(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct crystalcove_gpio *cg =
+		container_of(gc, struct crystalcove_gpio, chip);
+
+	mutex_lock(&cg->buslock);
+}
+
+static void crystalcove_bus_sync_unlock(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct crystalcove_gpio *cg =
+		container_of(gc, struct crystalcove_gpio, chip);
+	int gpio = data->hwirq;
+
+	if (cg->update & UPDATE_TYPE)
+		crystalcove_update_irq_type(gpio, cg->trigger_type);
+	if (cg->update & UPDATE_MASK)
+		crystalcove_update_irq_mask(gpio, cg->set_irq_mask);
+	cg->update = 0;
+
+	mutex_unlock(&cg->buslock);
+}
+
+static void crystalcove_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct crystalcove_gpio *cg =
+		container_of(gc, struct crystalcove_gpio, chip);
+
+	cg->set_irq_mask = false;
+	cg->update |= UPDATE_MASK;
+}
+
+static void crystalcove_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct crystalcove_gpio *cg =
+		container_of(gc, struct crystalcove_gpio, chip);
+
+	cg->set_irq_mask = true;
+	cg->update |= UPDATE_MASK;
+}
+
+static struct irq_chip crystalcove_irqchip = {
+	.name			= "PMIC-GPIO",
+	.irq_mask		= crystalcove_irq_mask,
+	.irq_unmask		= crystalcove_irq_unmask,
+	.irq_set_type		= crystalcove_irq_type,
+	.irq_bus_lock		= crystalcove_bus_lock,
+	.irq_bus_sync_unlock	= crystalcove_bus_sync_unlock,
+};
+
+static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
+{
+	struct crystalcove_gpio *cg = data;
+	int pending;
+	int gpio;
+	unsigned int virq;
+
+	pending = intel_soc_pmic_readb(GPIO0IRQ);
+	pending |= intel_soc_pmic_readb(GPIO1IRQ) << 8;
+	intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
+	intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
+
+	for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
+		if (pending & BIT(gpio)) {
+			virq = irq_find_mapping(cg->chip.irqdomain, gpio);
+			if (virq)
+				generic_handle_irq(virq);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void crystalcove_gpio_dbg_show(struct seq_file *s,
+				      struct gpio_chip *chip)
+{
+	struct crystalcove_gpio *cg =
+		container_of(chip, struct crystalcove_gpio, chip);
+	int gpio, offset;
+	u8 ctlo, ctli, mirqs0, mirqsx, irq;
+
+	for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
+		ctlo = intel_soc_pmic_readb(GPIO_TO_CTL(gpio, O));
+		ctli = intel_soc_pmic_readb(GPIO_TO_CTL(gpio, I));
+		mirqs0 = intel_soc_pmic_readb(
+			gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
+		mirqsx = intel_soc_pmic_readb(
+			gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
+		irq = intel_soc_pmic_readb(
+			gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
+
+		offset = gpio % 8;
+		seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %s %s\n",
+			   gpio, ctlo & CTLO_DIR_OUT ? "out" : "in ",
+			   ctli & 0x1 ? "hi" : "lo",
+			   ctli & CTLI_INTCNT_NE ? "fall" : "    ",
+			   ctli & CTLI_INTCNT_PE ? "rise" : "    ",
+			   ctlo,
+			   mirqs0 & BIT(offset) ? "s0 mask  " : "s0 unmask",
+			   mirqsx & BIT(offset) ? "sx mask  " : "sx unmask",
+			   irq & BIT(offset) ? "pending" : "       ");
+	}
+}
+
+static int crystalcove_gpio_probe(struct platform_device *pdev)
+{
+	int irq = platform_get_irq(pdev, 0);
+	struct crystalcove_gpio *cg;
+	int retval;
+	struct device *dev = pdev->dev.parent;
+
+	cg = devm_kzalloc(&pdev->dev, sizeof(*cg), GFP_KERNEL);
+	if (!cg)
+		return -ENOMEM;
+
+	mutex_init(&cg->buslock);
+	cg->chip.label = KBUILD_MODNAME;
+	cg->chip.direction_input = crystalcove_gpio_direction_input;
+	cg->chip.direction_output = crystalcove_gpio_direction_output;
+	cg->chip.get = crystalcove_gpio_get;
+	cg->chip.set = crystalcove_gpio_set;
+	cg->chip.base = -1;
+	cg->chip.ngpio = NUM_GPIO;
+	cg->chip.can_sleep = true;
+	cg->chip.dev = dev;
+	cg->chip.dbg_show = crystalcove_gpio_dbg_show;
+
+	gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
+			     handle_simple_irq, IRQ_TYPE_NONE);
+
+	retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
+				      IRQF_ONESHOT, KBUILD_MODNAME, cg);
+
+	if (retval) {
+		dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
+		goto out;
+	}
+
+	retval = gpiochip_add(&cg->chip);
+	if (retval) {
+		dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
+		goto out_free_irq;
+	}
+
+	platform_set_drvdata(pdev, cg);
+
+	return 0;
+
+out_free_irq:
+	free_irq(irq, cg);
+out:
+	return retval;
+}
+
+static int crystalcove_gpio_remove(struct platform_device *pdev)
+{
+	struct crystalcove_gpio *cg = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+	int err;
+
+	err = gpiochip_remove(&cg->chip);
+
+	if (irq >= 0)
+		free_irq(irq, cg);
+
+	return err;
+}
+
+static struct platform_driver crystalcove_gpio_driver = {
+	.probe = crystalcove_gpio_probe,
+	.remove = crystalcove_gpio_remove,
+	.driver = {
+		.name = "crystal_cove_gpio",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(crystalcove_gpio_driver);
+
+MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
+MODULE_DESCRIPTION("Intel Crystal Cove GPIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.2

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Alexandre Courbot]

On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
> Devices based on Intel SoC products such as Baytrail have a Power
> Management IC. In the PMIC there are subsystems for voltage regulation,
> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
> Crystal Cove.
>
> This patch adds support for the GPIO function in Crystal Cove.

A few minor comments below in case you make another version, but
overall looks pretty good to me.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

>
> v2:
> - Use IRQ chip helper to provide irqdomain.
> - Implement .remove and can now build as a module.
> - Various fix for unreadable or ugly code pieces.
> v3:
> - More fix in irq_handler and probe.
> v4:
> - Minor fix of one return statement.
>
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/Kconfig            |  13 ++
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-crystalcove.c | 345 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 359 insertions(+)
>  create mode 100644 drivers/gpio/gpio-crystalcove.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..fed08d9d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -440,6 +440,19 @@ config GPIO_ARIZONA
>         help
>           Support for GPIOs on Wolfson Arizona class devices.
>
> +config GPIO_CRYSTAL_COVE
> +       tristate "GPIO support for Crystal Cove PMIC"
> +       depends on INTEL_SOC_PMIC
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Support for GPIO pins on Crystal Cove PMIC.
> +
> +         Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC
> +         inside.
> +
> +         This driver can also be built as a module. If so, the module will be
> +         called gpio-crystalcove.
> +
>  config GPIO_LP3943
>         tristate "TI/National Semiconductor LP3943 GPIO expander"
>         depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..e6cd935 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA)   += gpio-bcm-kona.o
>  obj-$(CONFIG_GPIO_BT8XX)       += gpio-bt8xx.o
>  obj-$(CONFIG_GPIO_CLPS711X)    += gpio-clps711x.o
>  obj-$(CONFIG_GPIO_CS5535)      += gpio-cs5535.o
> +obj-$(CONFIG_GPIO_CRYSTAL_COVE)        += gpio-crystalcove.o
>  obj-$(CONFIG_GPIO_DA9052)      += gpio-da9052.o
>  obj-$(CONFIG_GPIO_DA9055)      += gpio-da9055.o
>  obj-$(CONFIG_GPIO_DAVINCI)     += gpio-davinci.o
> diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
> new file mode 100644
> index 0000000..76b6d57
> --- /dev/null
> +++ b/drivers/gpio/gpio-crystalcove.c
> @@ -0,0 +1,345 @@
> +/*
> + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
> + *
> + * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/sched.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/gpio.h>
> +#include <linux/bitops.h>
> +
> +#define NUM_GPIO               16
> +
> +#define UPDATE_TYPE            BIT(0)
> +#define UPDATE_MASK            BIT(1)
> +
> +#define GPIO0IRQ               0x0b
> +#define GPIO1IRQ               0x0c
> +#define MGPIO0IRQS0            0x19
> +#define MGPIO1IRQS0            0x1a
> +#define MGPIO0IRQSX            0x1b
> +#define MGPIO1IRQSX            0x1c
> +#define GPIO0P0CTLO            0x2b
> +#define GPIO0P0CTLI            0x33
> +#define GPIO1P0CTLO            0x3b
> +#define GPIO1P0CTLI            0x43
> +
> +#define CTLI_INTCNT_NE         (1 << 1)
> +#define CTLI_INTCNT_PE         (2 << 1)
> +#define CTLI_INTCNT_BE         (3 << 1)
> +
> +#define CTLO_DIR_OUT           (1 << 5)
> +
> +#define CTLO_DRV_CMOS          (0 << 4)
> +#define CTLO_DRV_OD            (1 << 4)
> +
> +#define CTLO_DRV_REN           (1 << 3)
> +
> +#define CTLO_RVAL_2KDW         (0)
> +#define CTLO_RVAL_2KUP         (1 << 1)
> +#define CTLO_RVAL_50KDW                (2 << 1)
> +#define CTLO_RVAL_50KUP                (3 << 1)
> +
> +#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
> +#define CTLO_OUTPUT_DEF        (CTLO_DIR_OUT | CTLO_INPUT_DEF)
> +
> +#define GPIO_TO_CTL(gpio, dir)         \
> +       ((gpio < 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8))
> +
> +/**
> + * struct crystalcove_gpio - Crystal Cove GPIO controller
> + * @buslock: for bus lock/sync and unlock.
> + * @chip: the abstract gpio_chip structure.
> + * @update: pending IRQ setting update, to be written to the chip upon unlock.
> + * @trigger_type: the trigger type of the IRQ.
> + * @set_irq_mask: true if the IRQ mask needs to be set, false to clear.
> + */
> +struct crystalcove_gpio {
> +       struct mutex            buslock; /* irq_bus_lock */
> +       struct gpio_chip        chip;
> +       int                     update;
> +       int                     trigger_type;
> +       bool                    set_irq_mask;
> +};
> +
> +static void crystalcove_update_irq_mask(int gpio, bool mask)
> +{
> +       u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
> +       int offset = gpio % 8;
> +
> +       if (mask)
> +               intel_soc_pmic_setb(mirqs0, BIT(offset));
> +       else
> +               intel_soc_pmic_clearb(mirqs0, BIT(offset));
> +}
> +
> +static void crystalcove_update_irq_type(int gpio, int type)
> +{
> +       u8 ctli = GPIO_TO_CTL(gpio, I);
> +
> +       type &= IRQ_TYPE_EDGE_BOTH;
> +       intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
> +
> +       if (type == IRQ_TYPE_EDGE_BOTH)
> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
> +       else if (type == IRQ_TYPE_EDGE_RISING)
> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
> +       else if (type & IRQ_TYPE_EDGE_FALLING)
> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);

Maybe a switch would be nicer here to choose the right value? And a
single call to intel_soc_pmic_setb() after the value is picked.

> +}
> +
> +static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
> +                                           unsigned gpio)
> +{
> +       u8 ctlo = GPIO_TO_CTL(gpio, O);
> +
> +       intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
> +       return 0;
> +}
> +
> +static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
> +                                            unsigned gpio, int value)
> +{
> +       u8 ctlo = GPIO_TO_CTL(gpio, O);
> +
> +       intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
> +       return 0;
> +}
> +
> +static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
> +       u8 ctli = GPIO_TO_CTL(gpio, I);
> +
> +       return intel_soc_pmic_readb(ctli) & 0x1;
> +}
> +
> +static void crystalcove_gpio_set(struct gpio_chip *chip,
> +                                unsigned gpio, int value)
> +{
> +       u8 ctlo = GPIO_TO_CTL(gpio, O);
> +
> +       if (value)
> +               intel_soc_pmic_setb(ctlo, 1);
> +       else
> +               intel_soc_pmic_clearb(ctlo, 1);
> +}
> +
> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct crystalcove_gpio *cg =
> +               container_of(gc, struct crystalcove_gpio, chip);
> +
> +       cg->trigger_type = type;
> +       cg->update |= UPDATE_TYPE;
> +
> +       return 0;
> +}
> +
> +static void crystalcove_bus_lock(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct crystalcove_gpio *cg =
> +               container_of(gc, struct crystalcove_gpio, chip);
> +
> +       mutex_lock(&cg->buslock);
> +}
> +
> +static void crystalcove_bus_sync_unlock(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct crystalcove_gpio *cg =
> +               container_of(gc, struct crystalcove_gpio, chip);
> +       int gpio = data->hwirq;
> +
> +       if (cg->update & UPDATE_TYPE)
> +               crystalcove_update_irq_type(gpio, cg->trigger_type);
> +       if (cg->update & UPDATE_MASK)
> +               crystalcove_update_irq_mask(gpio, cg->set_irq_mask);
> +       cg->update = 0;
> +
> +       mutex_unlock(&cg->buslock);
> +}
> +
> +static void crystalcove_irq_unmask(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct crystalcove_gpio *cg =
> +               container_of(gc, struct crystalcove_gpio, chip);
> +
> +       cg->set_irq_mask = false;
> +       cg->update |= UPDATE_MASK;
> +}
> +
> +static void crystalcove_irq_mask(struct irq_data *data)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct crystalcove_gpio *cg =
> +               container_of(gc, struct crystalcove_gpio, chip);
> +
> +       cg->set_irq_mask = true;
> +       cg->update |= UPDATE_MASK;
> +}
> +
> +static struct irq_chip crystalcove_irqchip = {
> +       .name                   = "PMIC-GPIO",
> +       .irq_mask               = crystalcove_irq_mask,
> +       .irq_unmask             = crystalcove_irq_unmask,
> +       .irq_set_type           = crystalcove_irq_type,
> +       .irq_bus_lock           = crystalcove_bus_lock,
> +       .irq_bus_sync_unlock    = crystalcove_bus_sync_unlock,
> +};
> +
> +static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
> +{
> +       struct crystalcove_gpio *cg = data;
> +       int pending;
> +       int gpio;
> +       unsigned int virq;
> +
> +       pending = intel_soc_pmic_readb(GPIO0IRQ);
> +       pending |= intel_soc_pmic_readb(GPIO1IRQ) << 8;
> +       intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
> +       intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
> +
> +       for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
> +               if (pending & BIT(gpio)) {
> +                       virq = irq_find_mapping(cg->chip.irqdomain, gpio);
> +                       if (virq)
> +                               generic_handle_irq(virq);
> +               }
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void crystalcove_gpio_dbg_show(struct seq_file *s,
> +                                     struct gpio_chip *chip)
> +{
> +       struct crystalcove_gpio *cg =
> +               container_of(chip, struct crystalcove_gpio, chip);
> +       int gpio, offset;
> +       u8 ctlo, ctli, mirqs0, mirqsx, irq;
> +
> +       for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
> +               ctlo = intel_soc_pmic_readb(GPIO_TO_CTL(gpio, O));
> +               ctli = intel_soc_pmic_readb(GPIO_TO_CTL(gpio, I));
> +               mirqs0 = intel_soc_pmic_readb(
> +                       gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
> +               mirqsx = intel_soc_pmic_readb(
> +                       gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
> +               irq = intel_soc_pmic_readb(
> +                       gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
> +
> +               offset = gpio % 8;
> +               seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %s %s\n",
> +                          gpio, ctlo & CTLO_DIR_OUT ? "out" : "in ",
> +                          ctli & 0x1 ? "hi" : "lo",
> +                          ctli & CTLI_INTCNT_NE ? "fall" : "    ",
> +                          ctli & CTLI_INTCNT_PE ? "rise" : "    ",
> +                          ctlo,
> +                          mirqs0 & BIT(offset) ? "s0 mask  " : "s0 unmask",
> +                          mirqsx & BIT(offset) ? "sx mask  " : "sx unmask",
> +                          irq & BIT(offset) ? "pending" : "       ");
> +       }
> +}
> +
> +static int crystalcove_gpio_probe(struct platform_device *pdev)
> +{
> +       int irq = platform_get_irq(pdev, 0);
> +       struct crystalcove_gpio *cg;
> +       int retval;
> +       struct device *dev = pdev->dev.parent;
> +
> +       cg = devm_kzalloc(&pdev->dev, sizeof(*cg), GFP_KERNEL);
> +       if (!cg)
> +               return -ENOMEM;
> +
> +       mutex_init(&cg->buslock);
> +       cg->chip.label = KBUILD_MODNAME;
> +       cg->chip.direction_input = crystalcove_gpio_direction_input;
> +       cg->chip.direction_output = crystalcove_gpio_direction_output;
> +       cg->chip.get = crystalcove_gpio_get;
> +       cg->chip.set = crystalcove_gpio_set;
> +       cg->chip.base = -1;
> +       cg->chip.ngpio = NUM_GPIO;
> +       cg->chip.can_sleep = true;
> +       cg->chip.dev = dev;
> +       cg->chip.dbg_show = crystalcove_gpio_dbg_show;
> +
> +       gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
> +                            handle_simple_irq, IRQ_TYPE_NONE);
> +
> +       retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
> +                                     IRQF_ONESHOT, KBUILD_MODNAME, cg);

Can't you use devm_request_threaded_irq() here?

> +
> +       if (retval) {
> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> +               goto out;
> +       }
> +
> +       retval = gpiochip_add(&cg->chip);
> +       if (retval) {
> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> +               goto out_free_irq;
> +       }
> +
> +       platform_set_drvdata(pdev, cg);
> +
> +       return 0;
> +
> +out_free_irq:
> +       free_irq(irq, cg);
> +out:
> +       return retval;
> +}
> +
> +static int crystalcove_gpio_remove(struct platform_device *pdev)
> +{
> +       struct crystalcove_gpio *cg = platform_get_drvdata(pdev);
> +       int irq = platform_get_irq(pdev, 0);
> +       int err;
> +
> +       err = gpiochip_remove(&cg->chip);
> +
> +       if (irq >= 0)
> +               free_irq(irq, cg);
> +
> +       return err;
> +}
> +
> +static struct platform_driver crystalcove_gpio_driver = {
> +       .probe = crystalcove_gpio_probe,
> +       .remove = crystalcove_gpio_remove,
> +       .driver = {
> +               .name = "crystal_cove_gpio",
> +               .owner = THIS_MODULE,
> +       },
> +};
> +
> +module_platform_driver(crystalcove_gpio_driver);
> +
> +MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
> +MODULE_DESCRIPTION("Intel Crystal Cove GPIO Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.8.3.2
>

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Alexandre Courbot]

On Tue, May 27, 2014 at 2:38 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> +       gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
>> +                            handle_simple_irq, IRQ_TYPE_NONE);
>> +
>> +       retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
>> +                                     IRQF_ONESHOT, KBUILD_MODNAME, cg);
>
> Can't you use devm_request_threaded_irq() here?

Ah, I suppose doing so would keep the IRQ handler active longer than
we want - please ignore this comment.

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Zhu, Lejun]

On 5/27/2014 1:38 PM, Alexandre Courbot wrote:
> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>> +static void crystalcove_update_irq_type(int gpio, int type)
>> +{
>> +       u8 ctli = GPIO_TO_CTL(gpio, I);
>> +
>> +       type &= IRQ_TYPE_EDGE_BOTH;
>> +       intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
>> +
>> +       if (type == IRQ_TYPE_EDGE_BOTH)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
>> +       else if (type == IRQ_TYPE_EDGE_RISING)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
>> +       else if (type & IRQ_TYPE_EDGE_FALLING)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
> 
> Maybe a switch would be nicer here to choose the right value? And a
> single call to intel_soc_pmic_setb() after the value is picked.

Thank you. I will fix this in the next version.

Best Regards
Lejun

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Mika Westerberg]

On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
> >> +
> >> +       if (retval) {
> >> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> >> +               goto out;
> >> +       }
> >> +
> >> +       retval = gpiochip_add(&cg->chip);
> >> +       if (retval) {
> >> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> >> +               goto out_free_irq;
> >> +       }
> 
> As to my mind, It'll be better to setup IRQ as last probing step and
> free it as the first step of driver removing.

When gpiochip_add() is called the chip is exported to outside world. At
that point anyone can start requesting GPIOs and setup GPIO based
interrupts. How does that work if you setup the IRQ after you call
gpiochip_add()?

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Zhu, Lejun]

On 5/27/2014 5:24 PM, Grygorii Strashko wrote:
> Hi Lejun,
> 
> On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
>> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>>> +static int crystalcove_gpio_probe(struct platform_device *pdev)
>>> +{
>>> +       int irq = platform_get_irq(pdev, 0);
> 
> Pls note, that platform_get_irq() may return error code.

Thank you. I'll fix it.

> 
> devm_gpiochip_add? ;)
> 

Huh? Can't find the API...

>>> +
>>> +       if (retval) {
>>> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
>>> +               goto out;
>>> +       }
>>> +
>>> +       retval = gpiochip_add(&cg->chip);
>>> +       if (retval) {
>>> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
>>> +               goto out_free_irq;
>>> +       }
> 
> As to my mind, It'll be better to setup IRQ as last probing step and
> free it as the first step of driver removing.

Mika suggested this order. Please see his mail for the reason. Is there
anything bad might happen if I setup IRQ first then do gpiochip_add?

Best Regards
Lejun

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Grygorii Strashko]

Hi Lejun,

On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
> 
> A few minor comments below in case you make another version, but
> overall looks pretty good to me.
> 
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> 
>>
>> v2:
>> - Use IRQ chip helper to provide irqdomain.
>> - Implement .remove and can now build as a module.
>> - Various fix for unreadable or ugly code pieces.
>> v3:
>> - More fix in irq_handler and probe.
>> v4:
>> - Minor fix of one return statement.
>>
>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> ---

[...]

>> +}
>> +
>> +static int crystalcove_gpio_probe(struct platform_device *pdev)
>> +{
>> +       int irq = platform_get_irq(pdev, 0);

Pls note, that platform_get_irq() may return error code.

>> +       struct crystalcove_gpio *cg;
>> +       int retval;
>> +       struct device *dev = pdev->dev.parent;
>> +
>> +       cg = devm_kzalloc(&pdev->dev, sizeof(*cg), GFP_KERNEL);
>> +       if (!cg)
>> +               return -ENOMEM;
>> +
>> +       mutex_init(&cg->buslock);
>> +       cg->chip.label = KBUILD_MODNAME;
>> +       cg->chip.direction_input = crystalcove_gpio_direction_input;
>> +       cg->chip.direction_output = crystalcove_gpio_direction_output;
>> +       cg->chip.get = crystalcove_gpio_get;
>> +       cg->chip.set = crystalcove_gpio_set;
>> +       cg->chip.base = -1;
>> +       cg->chip.ngpio = NUM_GPIO;
>> +       cg->chip.can_sleep = true;
>> +       cg->chip.dev = dev;
>> +       cg->chip.dbg_show = crystalcove_gpio_dbg_show;
>> +
>> +       gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
>> +                            handle_simple_irq, IRQ_TYPE_NONE);
>> +
>> +       retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
>> +                                     IRQF_ONESHOT, KBUILD_MODNAME, cg);
> 
> Can't you use devm_request_threaded_irq() here?

devm_gpiochip_add? ;)

> 
>> +
>> +       if (retval) {
>> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
>> +               goto out;
>> +       }
>> +
>> +       retval = gpiochip_add(&cg->chip);
>> +       if (retval) {
>> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
>> +               goto out_free_irq;
>> +       }

As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.

>> +
>> +       platform_set_drvdata(pdev, cg);
>> +
>> +       return 0;
>> +
>> +out_free_irq:
>> +       free_irq(irq, cg);
>> +out:
>> +       return retval;
>> +}

Best regards,
-grygorii

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Grygorii Strashko]

Hi Mika,

On 05/27/2014 11:46 AM, Mika Westerberg wrote:
> On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
>>>> +
>>>> +       if (retval) {
>>>> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       retval = gpiochip_add(&cg->chip);
>>>> +       if (retval) {
>>>> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
>>>> +               goto out_free_irq;
>>>> +       }
>>
>> As to my mind, It'll be better to setup IRQ as last probing step and
>> free it as the first step of driver removing.
> 
> When gpiochip_add() is called the chip is exported to outside world. At
> that point anyone can start requesting GPIOs and setup GPIO based
> interrupts. How does that work if you setup the IRQ after you call
> gpiochip_add()?
> 

It's difficult for me to imagine case when GPIO will be accessed
until GPIO driver's probe is finished.

Regarding remove()/suspend() routines, It's like an axiom for me:
- always disable irq
- always stop all works/threads created by driver
- do everything else
(It's proved by dozens hours of debugging).

Anyway, above is just my opinion :)
So, It's up to you, because it's your code :)

Also FYI, I did fast analysis of GPIO drivers - funny statistic below:
- 16 drivers setup IRQs BEFORE calling gpiochip_add();
- 22 drivers setup IRQs AFTER calling gpiochip_add();

Best regards,
-grygorii

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Mika Westerberg]

On Tue, May 27, 2014 at 03:04:09PM +0300, Grygorii Strashko wrote:
> Hi Mika,
> 
> On 05/27/2014 11:46 AM, Mika Westerberg wrote:
> > On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
> >>>> +
> >>>> +       if (retval) {
> >>>> +               dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> >>>> +               goto out;
> >>>> +       }
> >>>> +
> >>>> +       retval = gpiochip_add(&cg->chip);
> >>>> +       if (retval) {
> >>>> +               dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> >>>> +               goto out_free_irq;
> >>>> +       }
> >>
> >> As to my mind, It'll be better to setup IRQ as last probing step and
> >> free it as the first step of driver removing.
> > 
> > When gpiochip_add() is called the chip is exported to outside world. At
> > that point anyone can start requesting GPIOs and setup GPIO based
> > interrupts. How does that work if you setup the IRQ after you call
> > gpiochip_add()?
> > 
> 
> It's difficult for me to imagine case when GPIO will be accessed
> until GPIO driver's probe is finished.

Once you call gpiochip_add() your driver gets registered to the GPIO
subsystem and advertised outside. It doesn't matter whether your probe
function is finished or not.

> Regarding remove()/suspend() routines, It's like an axiom for me:
> - always disable irq
> - always stop all works/threads created by driver
> - do everything else
> (It's proved by dozens hours of debugging).

That's true for remove and suspend, yes but I'm not talking about them.

> Anyway, above is just my opinion :)
> So, It's up to you, because it's your code :)

No it's not, it's Lejun's driver :)

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Linus Walleij]

On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 05/27/2014 11:46 AM, Mika Westerberg wrote:

> Regarding remove()/suspend() routines, It's like an axiom for me:
> - always disable irq
> - always stop all works/threads created by driver
> - do everything else
> (It's proved by dozens hours of debugging).
>
> Anyway, above is just my opinion :)

We cannot really let such things be up to someone's
opinion... we need to figure out what is the right way to
do it and do it that way, else we have ambigous semantics
in gpiolib and/or irqchip, which i *hate*.

> So, It's up to you, because it's your code :)

No, let's figure out what is right...

And, FWIW your sequence looks perfectly reasonable.

> Also FYI, I did fast analysis of GPIO drivers - funny statistic below:
> - 16 drivers setup IRQs BEFORE calling gpiochip_add();
> - 22 drivers setup IRQs AFTER calling gpiochip_add();

My idea is that you should call gpiochip_add() *first* and then
add the IRQs to the chip. In succession.

Rationale: with dynamic GPIO numbers, gpio_to_irq()
cannot reasonably be working before the gpiochip is added,
so it should be added first, then the irqchip. Since irq_to_gpio()
is *NOT* to be used (rather obliterated), this is the sequence
we mandate.

This is how the new irqchip helpers work by the way. As I
introduce this to more and more drivers it will look more and
more like this. And attack patches tagged RFT switching the
semantics of drivers are appreciated.

Yours,
Linus Walleij

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Mika Westerberg]

On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote:
> My idea is that you should call gpiochip_add() *first* and then
> add the IRQs to the chip. In succession.
> 
> Rationale: with dynamic GPIO numbers, gpio_to_irq()
> cannot reasonably be working before the gpiochip is added,
> so it should be added first, then the irqchip. Since irq_to_gpio()
> is *NOT* to be used (rather obliterated), this is the sequence
> we mandate.

Thanks for the explanation. Makes sense.

I was wrong again, oh well it happens ;-)

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Linus Walleij]

On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Also, I'd like to note that GPIO IRQs can be accessible not only
> when GPIO chips is added, but also when IRQ domain is registered
> (at least it's valid for DT cases). In these cases gpiod_to_irq()
> might be not used at all.

Yes. We concluded some time back that gpio_chip:s and
irq_chip:s are orthogonal abstractions: you should be able
to use one of them without paying any respect to the other.

We only added the ability to flag GPIO lines as used for
IRQs so they would not be set to output by mistake...
(Straightening up the semantics.)

The only real semantic dependence that really makes sense
is .to_irq() which leads to this semantic registration ordering.

Yours,
Linus Walleij

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Grygorii Strashko]

Hi All,

On 05/29/2014 06:00 PM, Mika Westerberg wrote:
> On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote:
>> My idea is that you should call gpiochip_add() *first* and then
>> add the IRQs to the chip. In succession.
>>
>> Rationale: with dynamic GPIO numbers, gpio_to_irq()
>> cannot reasonably be working before the gpiochip is added,
>> so it should be added first, then the irqchip. Since irq_to_gpio()
>> is *NOT* to be used (rather obliterated), this is the sequence
>> we mandate.
> 
> Thanks for the explanation. Makes sense.
> 

Thanks a lot Linus for your comments here :)

Also, I'd like to note that GPIO IRQs can be accessible not only
when GPIO chips is added, but also when IRQ domain is registered
(at least it's valid for DT cases). In these cases gpiod_to_irq()
might be not used at all.

Regards,
-grygorii

 

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Zhu, Lejun]

On 5/29/2014 9:37 PM, Linus Walleij wrote:
> On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 05/27/2014 11:46 AM, Mika Westerberg wrote:
(...)
> 
> My idea is that you should call gpiochip_add() *first* and then
> add the IRQs to the chip. In succession.
> 
> Rationale: with dynamic GPIO numbers, gpio_to_irq()
> cannot reasonably be working before the gpiochip is added,
> so it should be added first, then the irqchip. Since irq_to_gpio()
> is *NOT* to be used (rather obliterated), this is the sequence
> we mandate.
> 
> This is how the new irqchip helpers work by the way. As I
> introduce this to more and more drivers it will look more and
> more like this. And attack patches tagged RFT switching the
> semantics of drivers are appreciated.
> 
> Yours,
> Linus Walleij
> 

Thanks. I'll use this sequence during probe().

(...)
	cg->regmap = pmic->regmap;

	retval = gpiochip_add(&cg->chip);
	if (retval) {
		dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
		return ret;
	}

	gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
			     handle_simple_irq, IRQ_TYPE_NONE);

	retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
				      IRQF_ONESHOT, KBUILD_MODNAME, cg);

	if (retval) {
		dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
		WARN_ON(gpiochip_remove(&cg->chip));
		return retval;
	}

	return 0;
}

Is the code above OK?

But this code will trigger a crash in gpiolib-acpi. Currently at the end
of gpiochip_add(), it calls:

gpiochip_add() -> acpi_gpiochip_add() -> acpi_gpiochip_request_interrupts()

acpi_gpiochip_request_interrupts() needs ->to_irq to work. Without having
called gpiochip_irqchip_add() already, this will be NULL:

        if (!chip->to_irq)
                return;    <-- It will return here.

        INIT_LIST_HEAD(&acpi_gpio->events);

In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
no longer NULL, then it will walk an uninitialized list.

So, should this be fixed in gpiolib-acpi?

Best Regards
Lejun

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Mika Westerberg]

On Thu, May 29, 2014 at 05:22:05PM +0200, Linus Walleij wrote:
> On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
> > Also, I'd like to note that GPIO IRQs can be accessible not only
> > when GPIO chips is added, but also when IRQ domain is registered
> > (at least it's valid for DT cases). In these cases gpiod_to_irq()
> > might be not used at all.
> 
> Yes. We concluded some time back that gpio_chip:s and
> irq_chip:s are orthogonal abstractions: you should be able
> to use one of them without paying any respect to the other.
> 
> We only added the ability to flag GPIO lines as used for
> IRQs so they would not be set to output by mistake...
> (Straightening up the semantics.)
> 
> The only real semantic dependence that really makes sense
> is .to_irq() which leads to this semantic registration ordering.

acpi_gpiochip_request_interrupts() depends on ->to_irq() to be set
before acpi_gpiochip_add() is called. Since the ordering changes this
won't work anymore.

I'm thinking that could we solve this so that we call
acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add()
and convert both pinctrl-baytrail and gpio-lynxpoint to use
gpiochip_irqchip_add()?

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Linus Walleij]

On Fri, May 30, 2014 at 4:12 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:

>         retval = gpiochip_add(&cg->chip);
>         if (retval) {
>                 dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
>                 return ret;
>         }
>
>         gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
>                              handle_simple_irq, IRQ_TYPE_NONE);
>
>         retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
>                                       IRQF_ONESHOT, KBUILD_MODNAME, cg);

You should request the interrupt before you add the irqchip
I think. But it shouldn't really matter, mainly to avoid tearing
down the irqchip if getting the irq should fail.

> But this code will trigger a crash in gpiolib-acpi. Currently at the end
> of gpiochip_add(), it calls:
>
> gpiochip_add() -> acpi_gpiochip_add() -> acpi_gpiochip_request_interrupts()
>
> acpi_gpiochip_request_interrupts() needs ->to_irq to work. Without having
> called gpiochip_irqchip_add() already, this will be NULL:
>
>         if (!chip->to_irq)
>                 return;    <-- It will return here.
>
>         INIT_LIST_HEAD(&acpi_gpio->events);
>
> In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
> no longer NULL, then it will walk an uninitialized list.
>
> So, should this be fixed in gpiolib-acpi?

Maybe, maybe in the drivers. I think Mika has a proposed solution...

Yours,
Linus Walleij

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Linus Walleij]

On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> I'm thinking that could we solve this so that we call
> acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add()
> and convert both pinctrl-baytrail and gpio-lynxpoint to use
> gpiochip_irqchip_add()?

Yes that seems like a great way to solve it actually.

Is someone able to do this refactoring?

I don't know if you have a case of an ACPI-based GPIO controller
that is *not* supplying interrupts? Because in that case this
would even be required for the thing to work, right?

Yours,
Linus Walleij

Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)

[Mika Westerberg]

On Tue, Jun 03, 2014 at 10:10:13AM +0200, Linus Walleij wrote:
> On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > I'm thinking that could we solve this so that we call
> > acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add()
> > and convert both pinctrl-baytrail and gpio-lynxpoint to use
> > gpiochip_irqchip_add()?
> 
> Yes that seems like a great way to solve it actually.
> 
> Is someone able to do this refactoring?

I have both Haswell and Baytrail hardware here so I can take a look if I
have time.

> I don't know if you have a case of an ACPI-based GPIO controller
> that is *not* supplying interrupts? Because in that case this
> would even be required for the thing to work, right?

Both Haswell and Baytrail support interrupts but only the later provides
ACPI events as far as I can tell.