gpio: add Intel WhiskeyCove PMIC GPIO driver
Subsystems (3)
PATCH v6 1/1 gpio: add Intel WhiskeyCove PMIC GPIO driver
This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on gpio-crystalcove.c.
---
Changes in v6:
- Removed unnecessary wcove_gpio_remove()
- Used devm_gpiochip_remove() instead of gpiochip_remove()
- Various coding style changes per Mika's comment
Changes in v5:
- Revisited the interrupt handler code to iterate until all pending interrupts are handled. This change is to avoid missing interrupt when we're inside the interrupt handler.
- Used regmap_bulk_read() to read address adjacent registers.
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 | 439 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 453 insertions(+) create mode 100644 drivers/gpio/gpio-wcove.c
drivers/gpio/Kconfig +13 -0
@@ -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
@@ -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 +439 -0
439 lines not on this page · raw message
Signature · 1 line
1.9.1
Quoted 7 lines · Bin Gao
This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC. This driver is based on
gpio-crystalcove.c.Signed-off-by Ajay Thomas <[email protected]>Signed-off-by Bin Gao <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>Reviewed-by Mika Westerberg <[email protected]>
There are still (minor) issues, since the patch most likely will not make v4.8-rc1, my comments below.
drivers/gpio/Kconfig config GPIO_CRYSTAL_COVE · line 809
Quoted 9 lines
drivers/gpio/Kconfig +1
-0@@ -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
I think it should follow alphabetical order.
drivers/gpio/Makefile obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o · line 37
Quoted 7 lines
drivers/gpio/Makefile +1
-0@@ -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
Ditto.
drivers/gpio/Makefile line 2
Quoted 4 lines
drivers/gpio/Makefile +3
-0+++ b/drivers/gpio/gpio-wcove.c@@ -0,0 +1,439 @@ +/* + * gpio-wcove.c - Intel Whiskey Cove GPIO Driver
No file name.
drivers/gpio/Makefile line 11
Quoted 9 lines
drivers/gpio/Makefile +9
-0+ */ + +#include <linux/seq_file.h> +#include <linux/bitops.h> +#include <linux/regmap.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/gpio/driver.h> +#include <linux/mfd/intel_soc_pmic.h>
Perhaps alphabetical order.
drivers/gpio/Makefile line 12
Quoted 1 line
drivers/gpio/Makefile +1
-0+/* GPIO output control registers(one per pin): 0x4e44 - 0x4e50 */
Space before (.
drivers/gpio/Makefile line 14
Quoted 2 lines
drivers/gpio/Makefile +2
-0+#define GPIO_OUT_CTRL_BASE 0x4e44 +/* GPIO input control registers(one per pin): 0x4e51 - 0x4e5d */
Ditto.
Needs to be changed in a whole file.
drivers/gpio/Makefile line 15
Quoted 1 line
drivers/gpio/Makefile +1
-0+#define CTLI_INTCNT_DIS (0)
It seems I commented this.
Please, use
(0 << 1)
drivers/gpio/Makefile line 20
Quoted 5 lines
drivers/gpio/Makefile +5
-0+#define CTLI_INTCNT_NE (1 << 1) +#define CTLI_INTCNT_PE (2 << 1) +#define CTLI_INTCNT_BE (3 << 1) + +#define CTLO_DIR_IN (0)
(0 << 5)
drivers/gpio/Makefile
Quoted 4 lines
drivers/gpio/Makefile +3
-0+#define CTLO_DIR_OUT (1 << 5) + +#define CTLO_DRV_MASK (1 << 4)
I'm not sure it makes sense for 1 bit, perhaps you may use _SHIFT macro instead.
Quoted 1 line
+#define
CTLO_DRV_OD(0)
(0 << 4)
Quoted 1 line
+#define
CTLO_DRV_CMOSCTLO_DRV_MASK
Perhaps
(1 << 4)
drivers/gpio/Makefile
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ +#define CTLO_DRV_REN (1 << 3) + +#define CTLO_RVAL_2KDW (0)
(0 << 1)
drivers/gpio/Makefile
Quoted 2 lines
drivers/gpio/Makefile +2
-0+#define CTLO_RVAL_2KUP (1 << 1) +#define CTLO_RVAL_50KDW (2 << 1)
Usually DN is used as opposite abbreviation to UP, until else is mentioned in datasheet.
Quoted 12 lines
drivers/gpio/Makefile +12
-0+#define CTLO_RVAL_50KUP (3 << 1) + +#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
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ * @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 */
Useless comment, use above kernel doc lines.
drivers/gpio/Makefile
Quoted 5 lines
drivers/gpio/Makefile +5
-0+ struct gpio_chip chip; + struct regmap *regmaop; + struct regmap_irq_chip_data *regmap_irq_chip; + int update; + int intcnt_value;
Does value suffix has a value?
drivers/gpio/Makefile
Quoted 11 lines
drivers/gpio/Makefile +11
-0+ 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))
Internal parens are redundant.
Quoted 8 lines
drivers/gpio/Makefile +8
-0+ bank = 1; + else + bank = 2; + + if (reg_type == CTRL_IN) + reg = GPIO_IN_CTRL_BASE + bank; + else + reg = GPIO_OUT_CTRL_BASE + bank;
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ + return reg; +} + +static void wcove_update_irq_mask(struct wcove_gpio *wg, int gpio) +{ + unsigned int reg, mask; + int group; + + group = gpio < GROUP0_NR_IRQS ? 0 : 1;
Entire line is useless.
drivers/gpio/Makefile
Quoted 3 lines
drivers/gpio/Makefile +3
-0+ reg = IRQ_MASK_BASE + group; + mask = (group == 0) ? BIT(gpio % GROUP0_NR_IRQS) : + BIT((gpio - GROUP0_NR_IRQS) % GROUP1_NR_IRQS);
#define IRQ_MASK_BASE(x) (... + (x))
#define IRQ_MASK_BASE0 ...
#define IRQ_MASK_BASE1 ...
if (gpio < ...) {
reg = IRQ_MASK_BASE0;
mask = BIT(...);
} else {
reg = IRQ_MASK_BASE1;
mask = BIT(...);
}
Above will be more readable.
Quoted 5 lines
drivers/gpio/Makefile +5
-0+ + if (wg->set_irq_mask) + regmap_update_bits(wg->regmap, reg, mask, mask); + else + regmap_update_bits(wg->regmap, reg, mask, 0);
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +9
-0+} +static irqreturn_t wcove_gpio_irq_handler(int irq, void *data) +{ + unsigned int pending, virq, gpio, mask, offset; + struct wcove_gpio *wg = data; + u8 p[2]; + + if (regmap_bulk_read(wg->regmap, IRQ_STATUS_BASE, p, 2)) { + pr_err("Failed to read irq status register\n");
dev_err() ?
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ return IRQ_NONE; + } + + pending = p[0] | (p[1] << 8);
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +9
-0+ if (!pending) + return IRQ_NONE; + + /* Iterate until no interrupt is pending */ + while (pending) { + /* One iteration is for all pending bits */ + for (gpio = 0; gpio < GROUP0_NR_IRQS; gpio++) { + if (!(pending & BIT(gpio))) + continue;
for_each_bit() ?
drivers/gpio/Makefile
Quoted 11 lines
drivers/gpio/Makefile +11
-0+ offset = (gpio > GROUP0_NR_IRQS) ? 1 : 0; + mask = (offset == 1) ? BIT(gpio - GROUP0_NR_IRQS) : + BIT(gpio); + virq = irq_find_mapping(wg->chip.irqdomain, gpio); + handle_nested_irq(virq); + regmap_update_bits(wg->regmap, IRQ_STATUS_BASE + offset, + mask, mask); + } + /* Next iteration */ + if (regmap_bulk_read(wg->regmap, IRQ_STATUS_BASE, p, 2)) { + pr_err("Failed to read irq status register\n");
Ditto.
Quoted 15 lines
drivers/gpio/Makefile +15
-0+ break; + } + pending = p[0] | (p[1] << 8); + } + + return IRQ_HANDLED; +} + +static void wcove_gpio_dbg_show(struct seq_file *s, + struct gpio_chip *chip) +{ + unsigned int ctlo, ctli, irq_mask, irq_status; + struct wcove_gpio *wg = gpiochip_get_data(chip); + int gpio, offset, group, ret = 0; +
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) { + group = gpio < GROUP0_NR_IRQS ? 0 : 1; + ret += regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &ctlo); + ret += regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &ctli); + ret += regmap_read(wg->regmap, IRQ_MASK_BASE + group, + &irq_mask); + ret += regmap_read(wg->regmap, IRQ_STATUS_BASE + group, + &irq_status); + if (ret) { + pr_err("Failed to read registers: ctrl out/in or irq status/mask\n");
dev_err() ?
Quoted 13 lines
drivers/gpio/Makefile +13
-0+ break; + } + + 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 & CTLI_INTCNT_NE ? "fall" : " ", + ctli & CTLI_INTCNT_PE ? "rise" : " ", + ctlo, + irq_mask & BIT(offset) ? "mask " : "unmask", + irq_status & BIT(offset) ? "pending" : " "); + }
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+} + +static int wcove_gpio_probe(struct platform_device *pdev) +{ + struct intel_soc_pmic *pmic; + struct wcove_gpio *wg; + int virq, retval, irq; + struct device *dev; + + pmic = dev_get_drvdata(pdev->dev.parent);
I think this line needs a comment to answer to the question "why parent?"
Quoted 42 lines
drivers/gpio/Makefile +42
-0+ if (!pmic) + return -ENODEV; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + dev = &pdev->dev; + + wg = devm_kzalloc(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 = pdev->dev.parent; + wg->chip.dbg_show = wcove_gpio_dbg_show; + wg->regmap = pmic->regmap; + + retval = devm_gpiochip_add_data(dev, &wg->chip, wg); + if (retval) { + dev_warn(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) {
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ dev_err(dev, "failed to get virtual irq by irq %d\n", irq); + retval = virq; + goto out_remove_gpio; + } + + retval = devm_request_threaded_irq(dev, virq, NULL, + wcove_gpio_irq_handler, IRQF_ONESHOT, pdev->name, wg); + + if (retval) { + dev_warn(dev, "request irq(%d) failed: %d\n", retval, virq);
It's not a warning as I can see.
Also question if it's useful at all to print anything here?
drivers/gpio/Makefile
Quoted 9 lines
drivers/gpio/Makefile +8
-0+ goto out_remove_gpio; + } + + return 0; + +out_remove_gpio: + devm_gpiochip_remove(dev, &wg->chip); + return retval;
Useless lines.
Signature · 2 lines
With Best Regards, Andy Shevchenko