gpio: add Intel WhiskeyCove GPIO driver
Subsystems (3)
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
@@ -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 +433 -0
433 lines not on this page · raw message
Signature · 1 line
1.9.1
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
Signed-off-by Ajay Thomas <[email protected]>Signed-off-by Bin Gao <[email protected]>---
Changes in v4:
- ConvertedCTLI_INTCNT_XXmacros to less verbose onesINT_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 copyrightline(2015-->2016).
- RemovedDRV_NAMEmacro.
- Added kernel-doc forregmap_irq_chipof thewcove_gpiostructure.
- Line length fix.
Changes in v2:
- Typo fix (Whsikey --> Whiskey).
- Includedlinux/gpio/driver.hinstead oflinux/gpio.h
- Implemented .set_single_ended().
- Added GPIO register description.
- Replacedcontainer_of()withgpiochip_get_data().
- Removed unnecessary "if (gpio >WCOVE_VGPIO_NUM" check.
- Removed the device id table and addedMODULE_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.cdrivers/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_CS5535drivers/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.odrivers/gpio/gpio-wcove.c new file +433
-1@@ -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
Quoted 11 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]>---
Changes in v4:
- ConvertedCTLI_INTCNT_XXmacros to less verbose onesINT_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 withIRQ_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
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.Signed-off-by Ajay Thomas <[email protected]>Signed-off-by Bin Gao <[email protected]>---
Changes in v4:
- ConvertedCTLI_INTCNT_XXmacros to less verbose onesINT_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;
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.Signed-off-by Ajay Thomas <[email protected]>Signed-off-by Bin Gao <[email protected]>---
Changes in v4:
- ConvertedCTLI_INTCNT_XXmacros to less verbose onesINT_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
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 withIRQ_NONE2.2 No IRQs active If this the second iteration or later, exit withIRQ_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.
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