gpio: add Intel WhiskeyCove GPIO driver
Subsystems (3)
PATCH 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.
--- drivers/gpio/Kconfig | 13 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-wcove.c | 402 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 416 insertions(+) create mode 100644 drivers/gpio/gpio-wcove.c
drivers/gpio/Kconfig +13 -0
@@ -805,6 +805,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 Whsikey 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 +402 -0
402 lines not on this page · raw message
Signature · 1 line
1.9.1
Quoted 5 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]>
It is always good to let Mika and Mathias look at new Intel GPIO drivers, so added them to the To: line.
drivers/gpio/Kconfig config GPIO_CRYSTAL_COVE · line 815
Quoted 8 lines
drivers/gpio/Kconfig +8
-0+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 Whsikey Cove PMIC
Speling
drivers/gpio/gpio-wcove.c line 20
Quoted 3 lines
drivers/gpio/gpio-wcove.c new file +3
-0+#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/gpio.h>
No use just #include <linux/gpio/driver.h>
Quoted 22 lines
drivers/gpio/gpio-wcove.c new file +22
-0+#include <linux/seq_file.h> +#include <linux/bitops.h> +#include <linux/regmap.h> +#include <linux/mfd/intel_soc_pmic.h> + +#define WCOVE_GPIO_NUM 13 +#define WCOVE_VGPIO_NUM 94 + +#define UPDATE_IRQ_TYPE BIT(0) +#define UPDATE_IRQ_MASK BIT(1) + +#define GPIOIRQ0 0x4e0b +#define GPIOIRQ1 0x4e0c +#define MGPIOIRQ0 0x4e19 +#define MGPIOIRQ1 0x4e1a +#define GPIO0P0CTLO 0x4e44 +#define GPIO0P0CTLI 0x4e51 +#define GPIO1P0CTLO 0x4e4b +#define GPIO1P0CTLI 0x4e58 +#define GPIO2P0CTLO 0x4e4f +#define GPIO2P0CTLI 0x4e5c +
drivers/gpio/gpio-wcove.c line 52
Quoted 10 lines
drivers/gpio/gpio-wcove.c new file +10
-0+#define CTLI_INTCNT_DIS (0) +#define CTLI_INTCNT_NE (1 << 1) +#define CTLI_INTCNT_PE (2 << 1) +#define CTLI_INTCNT_BE (3 << 1) + +#define CTLO_DIR_IN (0) +#define CTLO_DIR_OUT (1 << 5) + +#define CTLO_DRV_CMOS (0) +#define CTLO_DRV_OD (1 << 4)
That is likely what we call push-pull and open drain driving. Implement .set_single_ended() for this driver so you can handle that properly in the driver.
drivers/gpio/gpio-wcove.c line 59
Quoted 6 lines
drivers/gpio/gpio-wcove.c new file +6
-0+#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)
Looks like pull-up settings. That is strictly speaking pin control.
But OK I am trying to find the right way to abstract this without making GPIO too heavyweight.
drivers/gpio/gpio-wcove.c line 91
Quoted 4 lines
drivers/gpio/gpio-wcove.c new file +4
-0+static inline struct wcove_gpio *to_wg (struct gpio_chip *gc) +{ + return container_of(gc, struct wcove_gpio, chip); +}
No don't do that. Use devm_gpiochip_add_data() and just use gpiochip_get_data() to get the pointer out. Look at any other driver in the upstream kernel, I think I converted them all.
Quoted 12 lines
drivers/gpio/gpio-wcove.c new file +12
-0+static inline int to_reg (int gpio,enum ctrl_register reg_type) +{ +int reg; + +if (reg_type ==CTRL_IN ) { +if (gpio <7 ) + reg =GPIO0P0CTLI + gpio; +else if (gpio <11 ) + reg =GPIO1P0CTLI + (gpio %7 ); +else + reg =GPIO2P0CTLI + (gpio %11 ); + }else {
drivers/gpio/gpio-wcove.c line 114
Quoted 10 lines
drivers/gpio/gpio-wcove.c new file +10
-0+if (gpio <7 ) + reg =GPIO0P0CTLO + gpio; +else if (gpio <11 ) + reg =GPIO1P0CTLO + (gpio %7 ); +else + reg =GPIO2P0CTLO + (gpio %11 ); + } + +return reg; +}
You could add a kerneldoc to this function explaining how these GPIO registers are laid out.
drivers/gpio/gpio-wcove.c line 180
Quoted 7 lines
drivers/gpio/gpio-wcove.c new file +7
-0+static void wcove_gpio_set (struct gpio_chip *chip, +unsigned int gpio,int value) +{ +struct wcove_gpio *wg =to_wg (chip); + +if (gpio >WCOVE_VGPIO_NUM ) +return ;
gpiolib already protects against this, you can drop it everywhere.
drivers/gpio/gpio-wcove.c line 268
Quoted 9 lines
drivers/gpio/gpio-wcove.c new file +9
-0+static irqreturn_t wcove_gpio_irq_handler (int irq,void *data) +{ +struct wcove_gpio *wg = data; +unsigned int p0, p1, virq; +int pending, gpio; + +if (regmap_read (wg->regmap,GPIOIRQ0 , &p0) || +regmap_read (wg->regmap,GPIOIRQ1 , &p1)) +return IRQ_NONE ;
What does this mean? If we fail to read regmaps then it was not our IRQ?
Should be an error message or something at least I think?
Apart from that it looks all right.
Yours,
Linus Walleij
Quoted 135 lines · Linus Walleij
Quoted 5 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]>It is always good to let Mika and Mathias look at new Intel GPIO drivers, so added them to the To: line.
drivers/gpio/Kconfig config GPIO_CRYSTAL_COVE · line 815
Quoted 8 lines
drivers/gpio/Kconfig +8
-0+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 Whsikey Cove PMICSpeling
drivers/gpio/gpio-wcove.c line 20
Quoted 3 lines
drivers/gpio/gpio-wcove.c new file +3
-0+#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> No use just #include <
linux/gpio/driver.h>Quoted 22 lines
drivers/gpio/gpio-wcove.c new file +22
-0+#include <linux/seq_file.h> +#include <linux/bitops.h> +#include <linux/regmap.h> +#include <linux/mfd/intel_soc_pmic.h> + +#define WCOVE_GPIO_NUM 13 +#define WCOVE_VGPIO_NUM 94 + +#define UPDATE_IRQ_TYPE BIT(0) +#define UPDATE_IRQ_MASK BIT(1) + +#define GPIOIRQ0 0x4e0b +#define GPIOIRQ1 0x4e0c +#define MGPIOIRQ0 0x4e19 +#define MGPIOIRQ1 0x4e1a +#define GPIO0P0CTLO 0x4e44 +#define GPIO0P0CTLI 0x4e51 +#define GPIO1P0CTLO 0x4e4b +#define GPIO1P0CTLI 0x4e58 +#define GPIO2P0CTLO 0x4e4f +#define GPIO2P0CTLI 0x4e5c +drivers/gpio/gpio-wcove.c line 52
Quoted 10 lines
drivers/gpio/gpio-wcove.c new file +10
-0+#define CTLI_INTCNT_DIS (0) +#define CTLI_INTCNT_NE (1 << 1) +#define CTLI_INTCNT_PE (2 << 1) +#define CTLI_INTCNT_BE (3 << 1) + +#define CTLO_DIR_IN (0) +#define CTLO_DIR_OUT (1 << 5) + +#define CTLO_DRV_CMOS (0) +#define CTLO_DRV_OD (1 << 4)That is likely what we call push-pull and open drain driving. Implement .set_single_ended() for this driver so you can handle that properly in the driver.
drivers/gpio/gpio-wcove.c line 59
Quoted 6 lines
drivers/gpio/gpio-wcove.c new file +6
-0+#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)Looks like pull-up settings. That is strictly speaking pin control.
But OK I am trying to find the right way to abstract this without making GPIO too heavyweight.
drivers/gpio/gpio-wcove.c line 91
Quoted 4 lines
drivers/gpio/gpio-wcove.c new file +4
-0+static inline struct wcove_gpio *to_wg (struct gpio_chip *gc) +{ + return container_of(gc, struct wcove_gpio, chip); +}No don't do that. Use
devm_gpiochip_add_data()and just usegpiochip_get_data()to get the pointer out. Look at any other driver in the upstream kernel, I think I converted them all.Quoted 12 lines
drivers/gpio/gpio-wcove.c new file +12
-0+static inline int to_reg (int gpio,enum ctrl_register reg_type) +{ +int reg; + +if (reg_type ==CTRL_IN ) { +if (gpio <7 ) + reg =GPIO0P0CTLI + gpio; +else if (gpio <11 ) + reg =GPIO1P0CTLI + (gpio %7 ); +else + reg =GPIO2P0CTLI + (gpio %11 ); + }else {drivers/gpio/gpio-wcove.c line 114
Quoted 10 lines
drivers/gpio/gpio-wcove.c new file +10
-0+if (gpio <7 ) + reg =GPIO0P0CTLO + gpio; +else if (gpio <11 ) + reg =GPIO1P0CTLO + (gpio %7 ); +else + reg =GPIO2P0CTLO + (gpio %11 ); + } + +return reg; +}You could add a kerneldoc to this function explaining how these GPIO registers are laid out.
drivers/gpio/gpio-wcove.c line 180
Quoted 7 lines
drivers/gpio/gpio-wcove.c new file +7
-0+static void wcove_gpio_set (struct gpio_chip *chip, +unsigned int gpio,int value) +{ +struct wcove_gpio *wg =to_wg (chip); + +if (gpio >WCOVE_VGPIO_NUM ) +return ;gpiolib already protects against this, you can drop it everywhere.
drivers/gpio/gpio-wcove.c line 268
Quoted 9 lines
drivers/gpio/gpio-wcove.c new file +9
-0+static irqreturn_t wcove_gpio_irq_handler (int irq,void *data) +{ +struct wcove_gpio *wg = data; +unsigned int p0, p1, virq; +int pending, gpio; + +if (regmap_read (wg->regmap,GPIOIRQ0 , &p0) || +regmap_read (wg->regmap,GPIOIRQ1 , &p1)) +return IRQ_NONE ;What does this mean? If we fail to read regmaps then it was not our IRQ?
Quoted 6 lines
Should be an error message or something at least I think?
Apart from that it looks all right.
Yours,
Linus Walleij
Linus,
Thank you for your review. We'll come up with a version 2.
-Bin
drivers/gpio/gpio-wcove.c line 387
Quoted 3 lines · Bin Gao
drivers/gpio/gpio-wcove.c new file +3
-0+static const struct platform_device_id pmic_gpio_id_table[] = { + {"bxt_wcove_gpio" , }, +};
Do you really need this?
drivers/gpio/gpio-wcove.c line 395
Quoted 8 lines
drivers/gpio/gpio-wcove.c new file +8
-0+ +static struct platform_driver wcove_gpio_driver = { + .probe = wcove_gpio_probe, + .remove = wcove_gpio_remove, + .driver = { + .name ="wcove_gpio" , + }, + .id_table = pmic_gpio_id_table,
and this?
drivers/gpio/gpio-wcove.c line 402
Quoted 7 lines
drivers/gpio/gpio-wcove.c new file +7
-0+}; + +module_platform_driver (wcove_gpio_driver); + +MODULE_AUTHOR ("Ajay Thomas <[email protected]>" ); +MODULE_DESCRIPTION ("Intel Whiskey Cove GPIO Driver" ); +MODULE_LICENSE ("GPL v2" );
You can add
MODULE_ALIAS("platform:wcove_gpio");
to get the module loaded automatically.
Quoted 28 lines · Mika Westerberg
drivers/gpio/gpio-wcove.c line 387
Quoted 3 lines · Bin Gao
drivers/gpio/gpio-wcove.c new file +3
-0+static const struct platform_device_id pmic_gpio_id_table[] = { + {"bxt_wcove_gpio" , }, +};Do you really need this?
drivers/gpio/gpio-wcove.c line 395
Quoted 8 lines
drivers/gpio/gpio-wcove.c new file +8
-0+ +static struct platform_driver wcove_gpio_driver = { + .probe = wcove_gpio_probe, + .remove = wcove_gpio_remove, + .driver = { + .name ="wcove_gpio" , + }, + .id_table = pmic_gpio_id_table,and this?
drivers/gpio/gpio-wcove.c line 402
Quoted 7 lines
drivers/gpio/gpio-wcove.c new file +7
-0+}; + +module_platform_driver (wcove_gpio_driver); + +MODULE_AUTHOR ("Ajay Thomas <[email protected]>" ); +MODULE_DESCRIPTION ("Intel Whiskey Cove GPIO Driver" ); +MODULE_LICENSE ("GPL v2" );You can add
Quoted 3 lines
MODULE_ALIAS("platform:wcove_gpio");to get the module loaded automatically.
Mika,
Thanks for your review. I'll address these in patch v2.
-Bin