Skip to contentSkip to conversation

gpio: add Intel WhiskeyCove GPIO driver

PATCH Superseded Bin Gao 4 participants 7 messages Jun 27, 2016 – Jul 12, 2016

Subsystems (3)

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Bin Gao

author
#0 lore raw

PATCH v4 1/1 gpio: add Intel WhiskeyCove GPIO driver

This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.

---
Changes in v4:
- Converted CTLI_INTCNT_XX macros to less verbose ones INT_DETECT_XX.
- Add comments about why there is no .pm for the driver.
- Header files re-ordered.
- Various coding style change to address Andy's comments.
Changes in v3:
- Fixed the year in copyright line(2015-->2016).
- Removed DRV_NAME macro.
- Added kernel-doc for regmap_irq_chip of the wcove_gpio structure.
- Line length fix.
Changes in v2:
- Typo fix (Whsikey --> Whiskey).
- Included linux/gpio/driver.h instead of linux/gpio.h
- Implemented .set_single_ended().
- Added GPIO register description.
- Replaced container_of() with gpiochip_get_data().
- Removed unnecessary "if (gpio > WCOVE_VGPIO_NUM" check.
- Removed the device id table and added MODULE_ALIAS().

 drivers/gpio/Kconfig      |  13 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-wcove.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 447 insertions(+)
 create mode 100644 drivers/gpio/gpio-wcove.c
drivers/gpio/Kconfig +13 -0
index cebcb40..ac74299 100644
@@ -806,6 +806,19 @@ config GPIO_CRYSTAL_COVE
 	  This driver can also be built as a module. If so, the module will be
 	  called gpio-crystalcove.
 
+config GPIO_WHISKEY_COVE
+	tristate "GPIO support for Whiskey Cove PMIC"
+	depends on INTEL_SOC_PMIC
+	select GPIOLIB_IRQCHIP
+	help
+	  Support for GPIO pins on Whiskey Cove PMIC.
+
+	  Say Yes if you have a Intel SoC based tablet with Whiskey Cove PMIC
+	  inside.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-wcove.
+
 config GPIO_CS5535
 	tristate "AMD CS5535/CS5536 GPIO support"
 	depends on MFD_CS5535
drivers/gpio/Makefile +1 -0
index 991598e..fff6914 100644
@@ -34,6 +34,7 @@ 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_WHISKEY_COVE)	+= gpio-wcove.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
drivers/gpio/gpio-wcove.c new file +433 -0

433 lines not on this page · raw message

Signature · 1 line
1.9.1
Reviewed-by on v4 1/1
#1 lore raw
Quoted 2 lines · Bin Gao

This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.

There are still minors, though they would be fixed later. FWIW:

Quoted 506 lines

---
Changes in v4:
- Converted CTLI_INTCNT_XX macros to less verbose ones INT_DETECT_XX.
- Add comments about why there is no .pm for the driver.
- Header files re-ordered.
- Various coding style change to address Andy's comments.
Changes in v3:
- Fixed the year in copyright line(2015-->2016).
- Removed DRV_NAME macro.
- Added kernel-doc for regmap_irq_chip of the wcove_gpio structure.
- Line length fix.
Changes in v2:
- Typo fix (Whsikey --> Whiskey).
- Included linux/gpio/driver.h instead of linux/gpio.h
- Implemented .set_single_ended().
- Added GPIO register description.
- Replaced container_of() with gpiochip_get_data().
- Removed unnecessary "if (gpio > WCOVE_VGPIO_NUM" check.
- Removed the device id table and added MODULE_ALIAS().

 drivers/gpio/Kconfig      |  13 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-wcove.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 447 insertions(+)
 create mode 100644 drivers/gpio/gpio-wcove.c
drivers/gpio/Kconfig +13 -0
index cebcb40..ac74299 100644
@@ -806,6 +806,19 @@ config GPIO_CRYSTAL_COVE
          This driver can also be built as a module. If so, the module will be
          called gpio-crystalcove.
 
+config GPIO_WHISKEY_COVE
+       tristate "GPIO support for Whiskey Cove PMIC"
+       depends on INTEL_SOC_PMIC
+       select GPIOLIB_IRQCHIP
+       help
+         Support for GPIO pins on Whiskey Cove PMIC.
+
+         Say Yes if you have a Intel SoC based tablet with Whiskey Cove PMIC
+         inside.
+
+         This driver can also be built as a module. If so, the module will be
+         called gpio-wcove.
+
 config GPIO_CS5535
        tristate "AMD CS5535/CS5536 GPIO support"
        depends on MFD_CS5535
drivers/gpio/Makefile +1 -0
index 991598e..fff6914 100644
@@ -34,6 +34,7 @@ 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_WHISKEY_COVE)        += gpio-wcove.o
 obj-$(CONFIG_GPIO_DA9052)      += gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)      += gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)     += gpio-davinci.o
drivers/gpio/gpio-wcove.c new file +433 -1
new file mode 100644
index 0000000..9f1b25a
@@ -0,0 +1,433 @@
+/*
+ * gpio-wcove.c - Intel Whiskey Cove GPIO Driver
+ *
+ * This driver is written based on gpio-crystalcove.c
+ *
+ * Copyright (C) 2016 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.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+/*
+ * Whiskey Cove PMIC has 13 physical GPIO pins divided into 3 banks:
+ * Bank 0: Pin 0 - 6
+ * Bank 1: Pin 7 - 10
+ * Bank 2: Pin 11 -12
+ * Each pin has one output control register and one input control register.
+ */
+#define BANK0_NR_PINS          7
+#define BANK1_NR_PINS          4
+#define BANK2_NR_PINS          2
+#define WCOVE_GPIO_NUM         (BANK0_NR_PINS + BANK1_NR_PINS + BANK2_NR_PINS)
+#define WCOVE_VGPIO_NUM                94
+/* GPIO output control registers(one per pin): 0x4e44 - 0x4e50 */
+#define OUT_CTRL_OFFSET                0x4e44
+/* GPIO input control registers(one per pin): 0x4e51 - 0x4e5d */
+#define IN_CTRL_OFFSET         0x4e51
+
+/*
+ * GPIO interrupts are organized in two groups:
+ * Group 0: Bank 0 pins (Pin 0 - 6)
+ * Group 1: Bank 1 and Bank 2 pins (Pin 7 - 12)
+ * Each group has two registers(one bit per pin): status and mask.
+ */
+#define GROUP0_NR_IRQS         7
+#define GROUP1_NR_IRQS         6
+#define IRQ_MASK_OFFSET                0x4e19
+#define IRQ_STATUS_OFFSET      0x4e0b
+#define UPDATE_IRQ_TYPE                BIT(0)
+#define UPDATE_IRQ_MASK                BIT(1)
+
+#define LSHIFT_ONE(x)          ((x) << 1)
+
+/* GPIO Input Pin Interrupt Dectection */
+#define INT_DETECT_DISABLE     LSHIFT_ONE(0) /* Disable interrupt input */
+#define INT_DETECT_FALLING     LSHIFT_ONE(1) /* Detect falling edge */
+#define INT_DETECT_RISING      LSHIFT_ONE(2) /* Detect rising edge */
+#define INT_DETECT_BOTH                LSHIFT_ONE(3) /* Detect both edges */
+
+#define CTLO_DIR_IN            (0)
+#define CTLO_DIR_OUT           (1 << 5)
+
+#define CTLO_DRV_MASK          (1 << 4)
+#define CTLO_DRV_OD            (0)
+#define CTLO_DRV_CMOS          CTLO_DRV_MASK
+
+#define CTLO_DRV_REN           (1 << 3)
+
+#define CTLO_RVAL_2KDW         LSHIFT_ONE(0)
+#define CTLO_RVAL_2KUP         LSHIFT_ONE(1)
+#define CTLO_RVAL_50KDW                LSHIFT_ONE(2)
+#define CTLO_RVAL_50KUP                LSHIFT_ONE(3)
+
+#define CTLO_INPUT_SET (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
+#define CTLO_OUTPUT_SET        (CTLO_DIR_OUT | CTLO_INPUT_SET)
+
+enum ctrl_register {
+       CTRL_IN,
+       CTRL_OUT,
+};
+
+/*
+ * struct wcove_gpio - Whiskey Cove GPIO controller
+ * @buslock: for bus lock/sync and unlock.
+ * @chip: the abstract gpio_chip structure.
+ * @regmap: the regmap from the parent device.
+ * @regmap_irq_chip: the regmap of the gpio irq chip.
+ * @update: pending IRQ setting update, to be written to the chip upon unlock.
+ * @intcnt_value: the Interrupt Detect value to be written.
+ * @set_irq_mask: true if the IRQ mask needs to be set, false to clear.
+ */
+struct wcove_gpio {
+       struct mutex buslock; /* irq_bus_lock */
+       struct gpio_chip chip;
+       struct regmap *regmap;
+       struct regmap_irq_chip_data *regmap_irq_chip;
+       int update;
+       int intcnt_value;
+       bool set_irq_mask;
+};
+
+static inline unsigned int to_reg(int gpio, enum ctrl_register reg_type)
+{
+       unsigned int reg;
+       int bank;
+
+       if (gpio < BANK0_NR_PINS)
+               bank = 0;
+       else if (gpio < (BANK0_NR_PINS + BANK1_NR_PINS))
+               bank = 1;
+       else
+               bank = 2;
+
+       if (reg_type == CTRL_IN)
+               reg = IN_CTRL_OFFSET + bank;
+       else
+               reg = OUT_CTRL_OFFSET + bank;
+
+       return reg;
+}
+
+static void wcove_update_irq_mask(struct wcove_gpio *wg, int gpio)
+{
+       int group;
+       unsigned int reg, bit;
+
+       group = gpio < GROUP0_NR_IRQS ? 0 : 1;
+       reg = IRQ_MASK_OFFSET + group;
+       bit = (group == 0) ? BIT(gpio % GROUP0_NR_IRQS) :
+               BIT((gpio - GROUP0_NR_IRQS) % GROUP1_NR_IRQS);
+
+       if (wg->set_irq_mask)
+               regmap_update_bits(wg->regmap, reg, bit, 1);
+       else
+               regmap_update_bits(wg->regmap, reg, bit, 0);
+}
+
+static void wcove_update_irq_ctrl(struct wcove_gpio *wg, int gpio)
+{
+       unsigned int reg = to_reg(gpio, CTRL_IN);
+
+       regmap_update_bits(wg->regmap, reg, INT_DETECT_BOTH, wg->intcnt_value);
+}
+
+static int wcove_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+
+       return regmap_write(wg->regmap, to_reg(gpio, CTRL_OUT),
+                           CTLO_INPUT_SET);
+}
+
+static int wcove_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
+                                   int value)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+
+       return regmap_write(wg->regmap, to_reg(gpio, CTRL_OUT),
+                           CTLO_OUTPUT_SET | value);
+}
+
+static int wcove_gpio_get(struct gpio_chip *chip, unsigned int gpio)
+{
+       int ret;
+       unsigned int val;
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+
+       ret = regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &val);
+       if (ret)
+               return ret;
+
+       return val & 0x1;
+}
+
+static void wcove_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+
+       if (value)
+               regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT), 1, 1);
+       else
+               regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT), 1, 0);
+}
+
+static int wcove_gpio_set_single_ended(struct gpio_chip *chip,
+                                       unsigned int gpio,
+                                       enum single_ended_mode mode)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+
+       switch (mode) {
+       case LINE_MODE_OPEN_DRAIN:
+               return regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT),
+                                               CTLO_DRV_MASK, CTLO_DRV_OD);
+       case LINE_MODE_PUSH_PULL:
+               return regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT),
+                                               CTLO_DRV_MASK, CTLO_DRV_CMOS);
+       default:
+               break;
+       }
+
+       return -ENOTSUPP;
+}
+
+static int wcove_irq_type(struct irq_data *data, unsigned int type)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(
+               irq_data_get_irq_chip_data(data));
+
+       switch (type) {
+       case IRQ_TYPE_NONE:
+               wg->intcnt_value = INT_DETECT_DISABLE;
+               break;
+       case IRQ_TYPE_EDGE_BOTH:
+               wg->intcnt_value = INT_DETECT_BOTH;
+               break;
+       case IRQ_TYPE_EDGE_RISING:
+               wg->intcnt_value = INT_DETECT_RISING;
+               break;
+       case IRQ_TYPE_EDGE_FALLING:
+               wg->intcnt_value = INT_DETECT_FALLING;
+               break;
+       default:
+               return -EINVAL;
+       }
+
+       wg->update |= UPDATE_IRQ_TYPE;
+
+       return 0;
+}
+
+static void wcove_bus_lock(struct irq_data *data)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(
+               irq_data_get_irq_chip_data(data));
+
+       mutex_lock(&wg->buslock);
+}
+
+static void wcove_bus_sync_unlock(struct irq_data *data)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(
+               irq_data_get_irq_chip_data(data));
+       int gpio = data->hwirq;
+
+       if (wg->update & UPDATE_IRQ_TYPE)
+               wcove_update_irq_ctrl(wg, gpio);
+       if (wg->update & UPDATE_IRQ_MASK)
+               wcove_update_irq_mask(wg, gpio);
+
+       wg->update = 0;
+
+       mutex_unlock(&wg->buslock);
+}
+
+static void wcove_irq_unmask(struct irq_data *data)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(
+               irq_data_get_irq_chip_data(data));
+
+       wg->set_irq_mask = false;
+       wg->update |= UPDATE_IRQ_MASK;
+}
+
+static void wcove_irq_mask(struct irq_data *data)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(
+               irq_data_get_irq_chip_data(data));
+
+       wg->set_irq_mask = true;
+       wg->update |= UPDATE_IRQ_MASK;
+}
+
+static struct irq_chip wcove_irqchip = {
+       .name                   = "Whiskey Cove",
+       .irq_mask               = wcove_irq_mask,
+       .irq_unmask             = wcove_irq_unmask,
+       .irq_set_type           = wcove_irq_type,
+       .irq_bus_lock           = wcove_bus_lock,
+       .irq_bus_sync_unlock    = wcove_bus_sync_unlock,
+};
+
+static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
+{
+       int pending;
+       unsigned int p0, p1, virq, gpio;
+       struct wcove_gpio *wg = data;
+
+       if (regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 0, &p0) ||
+           regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 1, &p1)) {
+               dev_err(wg->chip.parent, "%s(): regmap_read() failed.\n",
+                                                               __func__);
+               return IRQ_NONE;
+       }
+
+       pending = p0 | (p1 << 8);
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               if (pending & BIT(gpio)) {
+                       virq = irq_find_mapping(wg->chip.irqdomain, gpio);
+                       handle_nested_irq(virq);
+               }
+       }
+
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 0, p0);
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 1, p1);
+
+       return IRQ_HANDLED;
+}
+
+static void wcove_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+       int gpio, offset, group;
+       unsigned int ctlo, ctli, irq_mask, irq_status;
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               group = gpio < GROUP0_NR_IRQS ? 0 : 1;
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &ctlo);
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &ctli);
+               regmap_read(wg->regmap, IRQ_MASK_OFFSET + group, &irq_mask);
+               regmap_read(wg->regmap, IRQ_STATUS_OFFSET + group, &irq_status);
+
+               offset = gpio % 8;
+               seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %s\n",
+                          gpio, ctlo & CTLO_DIR_OUT ? "out" : "in ",
+                          ctli & 0x1 ? "hi" : "lo",
+                          ctli & INT_DETECT_FALLING ? "falling" : "    ",
+                          ctli & INT_DETECT_RISING ? "rising" : "    ",
+                          ctlo,
+                          irq_mask & BIT(offset) ? "mask  " : "unmask",
+                          irq_status & BIT(offset) ? "pending" : "       ");
+       }
+}
+
+static int wcove_gpio_probe(struct platform_device *pdev)
+{
+       int virq, retval, irq;
+       struct wcove_gpio *wg;
+       struct device *dev = pdev->dev.parent;
+       struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0)
+               return irq;
+
+       wg = devm_kzalloc(&pdev->dev, sizeof(*wg), GFP_KERNEL);
+       if (!wg)
+               return -ENOMEM;
+
+       wg->regmap_irq_chip = pmic->irq_chip_data_level2;
+
+       platform_set_drvdata(pdev, wg);
+
+       mutex_init(&wg->buslock);
+       wg->chip.label = KBUILD_MODNAME;
+       wg->chip.direction_input = wcove_gpio_dir_in;
+       wg->chip.direction_output = wcove_gpio_dir_out;
+       wg->chip.get = wcove_gpio_get;
+       wg->chip.set = wcove_gpio_set;
+       wg->chip.set_single_ended = wcove_gpio_set_single_ended,
+       wg->chip.base = -1;
+       wg->chip.ngpio = WCOVE_VGPIO_NUM;
+       wg->chip.can_sleep = true;
+       wg->chip.parent = dev;
+       wg->chip.dbg_show = wcove_gpio_dbg_show;
+       wg->regmap = pmic->regmap;
+
+       retval = devm_gpiochip_add_data(&pdev->dev, &wg->chip, wg);
+       if (retval) {
+               dev_err(&pdev->dev, "add gpio chip error: %d\n", retval);
+               return retval;
+       }
+
+       gpiochip_irqchip_add(&wg->chip, &wcove_irqchip, 0,
+                            handle_simple_irq, IRQ_TYPE_NONE);
+
+       virq = regmap_irq_get_virq(wg->regmap_irq_chip, irq);
+       if (virq < 0) {
+               dev_err(&pdev->dev, "failed to get virq %d\n", irq);
+               retval = virq;
+               goto out_remove_gpio;
+       }
+
+       retval = devm_request_threaded_irq(&pdev->dev, virq,
+                               NULL, wcove_gpio_irq_handler,
+                               IRQF_ONESHOT, pdev->name, wg);
+
+       if (retval) {
+               dev_warn(&pdev->dev, "request irq failed: %d, virq: %d\n",
+                                                       retval, virq);
+               goto out_remove_gpio;
+       }
+
+       return 0;
+
+out_remove_gpio:
+       devm_gpiochip_remove(&pdev->dev, &wg->chip);
+       return retval;
+}
+
+static int wcove_gpio_remove(struct platform_device *pdev)
+{
+       struct wcove_gpio *wg = platform_get_drvdata(pdev);
+
+       devm_gpiochip_remove(&pdev->dev, &wg->chip);
+       return 0;
+}
+
+/*
+ * Whiskey Cove PMIC itself is a analog device(but with digital control
+ * interface) providing power management support for other devices in
+ * the accompanied SoC, so we have no .pm for Whiskey Cove GPIO driver.
+ */
+static struct platform_driver wcove_gpio_driver = {
+       .driver = {
+               .name = "bxt_wcove_gpio",
+       },
+       .probe = wcove_gpio_probe,
+       .remove = wcove_gpio_remove,
+};
+
+module_platform_driver(wcove_gpio_driver);
+
+MODULE_AUTHOR("Ajay Thomas <[email protected]>");
+MODULE_AUTHOR("Bin Gao <[email protected]>");
+MODULE_DESCRIPTION("Intel Whiskey Cove GPIO Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:bxt_wcove_gpio");
--

1.9.1

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#2 lore raw
In reply to Bin Gao
Quoted 11 lines · Bin Gao

This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.

---
Changes in v4:
- Converted CTLI_INTCNT_XX macros to less verbose ones INT_DETECT_XX.
- Add comments about why there is no .pm for the driver.
- Header files re-ordered.
- Various coding style change to address Andy's comments.

Mika can I have your ACK/review tag on this driver so I can merge it? I prefer to have all Intel stuff bearing your seal of approval.

drivers/gpio/gpio-wcove.c line 294

Quoted 8 lines
drivers/gpio/gpio-wcove.c new file +8 -0
+static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
+{
+       int pending;
+       unsigned int p0, p1, virq, gpio;
+       struct wcove_gpio *wg = data;
+
+       if (regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 0, &p0) ||
+           regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 1, &p1)) {

Why can't you use regmap_bulk_read() here?

Quoted 6 lines
drivers/gpio/gpio-wcove.c new file +6 -0
+               dev_err(wg->chip.parent, "%s(): regmap_read() failed.\n",
+                                                               __func__);
+               return IRQ_NONE;
+       }
+
+       pending = p0 | (p1 << 8);

drivers/gpio/gpio-wcove.c line 310

Quoted 10 lines
drivers/gpio/gpio-wcove.c new file +10 -0
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               if (pending & BIT(gpio)) {
+                       virq = irq_find_mapping(wg->chip.irqdomain, gpio);
+                       handle_nested_irq(virq);
+               }
+       }
+
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 0, p0);
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 1, p1);

Use regmap_bulk_write()?

Also you're ignoring the return error code. Check it and dev_err() if it fails.

This loop seems like it could miss interrupts happening while processing. Especially edge interrupts, and thatr will lead to serious bugs later.

Please consider the following construction:

1. read status register
2. Any IRQs active?
2.1 No IRQs active: if this is the FIRST iteration, exit with IRQ_NONE 2.2 No IRQs active If this the second iteration or later, exit with
IRQ_HANDLED
2.3 IRQs active, continue
2. Find first active IRQ
3. Handle first active IRQ
4. ACK the first active IRQ by writing the status register
5. Reiterate from 1

This way, if two IRQs happen at the same time, or if a new IRQ appears while you're inside the interrupt handler, it gets served.

drivers/gpio/gpio-wcove.c line 326

Quoted 12 lines
drivers/gpio/gpio-wcove.c new file +12 -0
+static void wcove_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+       int gpio, offset, group;
+       unsigned int ctlo, ctli, irq_mask, irq_status;
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               group = gpio < GROUP0_NR_IRQS ? 0 : 1;
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &ctlo);
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &ctli);
+               regmap_read(wg->regmap, IRQ_MASK_OFFSET + group, &irq_mask);
+               regmap_read(wg->regmap, IRQ_STATUS_OFFSET + group, &irq_status);

Ignoring error codes. Fix this.

drivers/gpio/gpio-wcove.c line 380

Quoted 2 lines
drivers/gpio/gpio-wcove.c new file +2 -0
+       gpiochip_irqchip_add(&wg->chip, &wcove_irqchip, 0,
+                            handle_simple_irq, IRQ_TYPE_NONE);

Reexamine the use of handle_simple_irq() here. We have two kinds of irq hardware: those with one register for ACKing and reading the status of an IRQ, and those with two registers for it: one where you ACK the IRQ (so it can immediately re-trigger) and one to read the status of whether it happened. Sometimes different handling is needed for levek and edge IRQs even (c.f. gpio-pl061.c).

Only the hardware with just one register for both things should use handle_simple_irq(). This seems to be the case here but I want you to verify.

Yours,
Linus Walleij

#3 lore raw
Quoted 14 lines · Linus Walleij
Quoted 11 lines · Bin Gao

This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.

---
Changes in v4:
- Converted CTLI_INTCNT_XX macros to less verbose ones INT_DETECT_XX.
- Add comments about why there is no .pm for the driver.
- Header files re-ordered.
- Various coding style change to address Andy's comments.

Quoted 2 lines

Mika can I have your ACK/review tag on this driver so I can merge it? I prefer to have all Intel stuff bearing your seal of approval.

Thanks for your trust :)

I don't have much comments in addition to what you already pointed out. I'll just wait for the next revision and give my ack then.

Quoted 5 lines

drivers/gpio/gpio-wcove.c line 291

Quoted 5 lines
drivers/gpio/gpio-wcove.c new file +5 -0
+static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
+{
+       int pending;
+       unsigned int p0, p1, virq, gpio;
+       struct wcove_gpio *wg = data;

Bin,

Since you are going to make another iteration, please arrange the declarations like:

	unsigned int p0, p1, virq, gpio;
	struct wcove_gpio *wg = data;
	int pending;
author
#4 lore raw
Quoted 31 lines · Mika Westerberg
Quoted 14 lines · Linus Walleij
Quoted 11 lines · Bin Gao

This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.

---
Changes in v4:
- Converted CTLI_INTCNT_XX macros to less verbose ones INT_DETECT_XX.
- Add comments about why there is no .pm for the driver.
- Header files re-ordered.
- Various coding style change to address Andy's comments.

Quoted 2 lines

Mika can I have your ACK/review tag on this driver so I can merge it? I prefer to have all Intel stuff bearing your seal of approval.

Thanks for your trust :)

I don't have much comments in addition to what you already pointed out. I'll just wait for the next revision and give my ack then.

Quoted 5 lines

drivers/gpio/gpio-wcove.c line 291

Quoted 5 lines
drivers/gpio/gpio-wcove.c new file +5 -0
+static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
+{
+       int pending;
+       unsigned int p0, p1, virq, gpio;
+       struct wcove_gpio *wg = data;

Bin,

Quoted 6 lines

Since you are going to make another iteration, please arrange the declarations like:

	unsigned int p0, p1, virq, gpio;
	struct wcove_gpio *wg = data;
	int pending;

Yes, will do. Thanks.

-Bin

author
#5 lore raw
In reply to Linus Walleij
Quoted 10 lines · Linus Walleij

drivers/gpio/gpio-wcove.c line 294

Quoted 8 lines
drivers/gpio/gpio-wcove.c new file +8 -0
+static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
+{
+       int pending;
+       unsigned int p0, p1, virq, gpio;
+       struct wcove_gpio *wg = data;
+
+       if (regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 0, &p0) ||
+           regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 1, &p1)) {

Why can't you use regmap_bulk_read() here?

Will fix this in v5.

Quoted 11 lines
Quoted 11 lines
drivers/gpio/gpio-wcove.c new file +10 -0
+               dev_err(wg->chip.parent, "%s(): regmap_read() failed.\n",
+                                                               __func__);
+               return IRQ_NONE;
+       }
+
+       pending = p0 | (p1 << 8);
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               if (pending & BIT(gpio)) {
+                       virq = irq_find_mapping(wg->chip.irqdomain, gpio);
Quoted 8 lines

drivers/gpio/gpio-wcove.c line 310

Quoted 6 lines
drivers/gpio/gpio-wcove.c new file +6 -0
+                       handle_nested_irq(virq);
+               }
+       }
+
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 0, p0);
+       regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 1, p1);

Use regmap_bulk_write()?

Will fix this in v5.

Quoted 3 lines

Also you're ignoring the return error code. Check it and dev_err() if it fails.

Yes, will fix.

Quoted 18 lines

This loop seems like it could miss interrupts happening while processing. Especially edge interrupts, and thatr will lead to serious bugs later.

Please consider the following construction:

1. read status register
2. Any IRQs active?
2.1 No IRQs active: if this is the FIRST iteration, exit with IRQ_NONE 2.2 No IRQs active If this the second iteration or later, exit with
IRQ_HANDLED
2.3 IRQs active, continue
2. Find first active IRQ
3. Handle first active IRQ
4. ACK the first active IRQ by writing the status register
5. Reiterate from 1

Quoted 2 lines

This way, if two IRQs happen at the same time, or if a new IRQ appears while you're inside the interrupt handler, it gets served.

I agree. Writing to status register should be done bit by bit, instead of one write for all bits. Will fix this in v5.

Quoted 15 lines

drivers/gpio/gpio-wcove.c line 326

Quoted 13 lines
drivers/gpio/gpio-wcove.c new file +12 -0
+static void wcove_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+       struct wcove_gpio *wg = gpiochip_get_data(chip);
+       int gpio, offset, group;
+       unsigned int ctlo, ctli, irq_mask, irq_status;
+
+       for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
+               group = gpio < GROUP0_NR_IRQS ? 0 : 1;
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &ctlo);
+               regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &ctli);
+               regmap_read(wg->regmap, IRQ_MASK_OFFSET + group, &irq_mask);
+               regmap_read(wg->regmap, IRQ_STATUS_OFFSET + group, &irq_status);

Ignoring error codes. Fix this.

Will Fix in v5.

Quoted 11 lines

drivers/gpio/gpio-wcove.c line 380

Quoted 3 lines
drivers/gpio/gpio-wcove.c new file +2 -0
+       gpiochip_irqchip_add(&wg->chip, &wcove_irqchip, 0,
+                            handle_simple_irq, IRQ_TYPE_NONE);

Reexamine the use of handle_simple_irq() here. We have two kinds of irq hardware: those with one register for ACKing and reading the status of an IRQ, and those with two registers for it: one where you ACK the IRQ (so it can immediately re-trigger) and one to read the status of whether it happened. Sometimes different handling is needed for levek and edge IRQs even (c.f. gpio-pl061.c).

Quoted 3 lines

Only the hardware with just one register for both things should use handle_simple_irq(). This seems to be the case here but I want you to verify.

I will check and fix if it's needed.

Quoted 3 lines

Yours,
Linus Walleij

Thanks for your review.

author
#6 lore raw
In reply to Linus Walleij
Quoted 10 lines · Linus Walleij

drivers/gpio/gpio-wcove.c line 380

Quoted 2 lines
drivers/gpio/gpio-wcove.c new file +2 -0
+       gpiochip_irqchip_add(&wg->chip, &wcove_irqchip, 0,
+                            handle_simple_irq, IRQ_TYPE_NONE);

Reexamine the use of handle_simple_irq() here. We have two kinds of irq hardware: those with one register for ACKing and reading the status of an IRQ, and those with two registers for it: one where you ACK the IRQ (so it can immediately re-trigger) and one to read the status of whether it happened. Sometimes different handling is needed for levek and edge IRQs even (c.f. gpio-pl061.c).

Quoted 3 lines

Only the hardware with just one register for both things should use handle_simple_irq(). This seems to be the case here but I want you to verify.

Yes, our case is handle_simple_irq(), not handle_edge_irq(), handle_level_irq() or handle_fasteoi_irq(), etc. because there is no ACK mechanism inside the GPIO controller's interrupt logic - all we need to do is read the status register to get the status and write-to-clear the status register so that a new interrupt can be triggered, i.e. there is only one register for both.

Quoted 3 lines

Yours,
Linus Walleij