Date:	Tue, 19 Jul 2016 10:25:29 +0300
From:	Mika Westerberg <mika.westerberg@...ux.intel.com>
To:	Bin Gao <bin.gao@...ux.intel.com>
Cc:	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	linux-gpio@...r.kernel.org,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	linux-kernel@...r.kernel.org,
	Ajay Thomas <ajay.thomas.david.rajamanickam@...el.com>,
	Yegnesh S Iyer <yegnesh.s.iyer@...el.com>,
	Bin Gao <bin.gao@...el.com>
Subject: Re: [PATCH v5] gpio: add Intel WhiskeyCove GPIO driver

On Mon, Jul 11, 2016 at 05:06:57PM -0700, Bin Gao wrote:
> This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC.
> This driver is based on gpio-crystalcove.c.
> 
> Signed-off-by: Ajay Thomas <ajay.thomas.david.rajamanickam@...el.com>
> Signed-off-by: Bin Gao <bin.gao@...el.com>

I have a couple of comments below. Once you have addressed those you can
add my,

Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>

> ---
> 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 | 444 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 458 insertions(+)
>  create mode 100644 drivers/gpio/gpio-wcove.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 536112f..0f33982 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -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
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 991598e..fff6914 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -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
> diff --git a/drivers/gpio/gpio-wcove.c b/drivers/gpio/gpio-wcove.c
> new file mode 100644
> index 0000000..e29e5a9
> --- /dev/null
> +++ b/drivers/gpio/gpio-wcove.c
> @@ -0,0 +1,444 @@
> +/*
> + * 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/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>
> +
> +/*
> + * 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 GPIO_OUT_CTRL_BASE	0x4e44
> +/* GPIO input control registers(one per pin): 0x4e51 - 0x4e5d */
> +#define GPIO_IN_CTRL_BASE	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_BASE		0x4e19
> +#define IRQ_STATUS_BASE		0x4e0b
> +#define UPDATE_IRQ_TYPE		BIT(0)
> +#define UPDATE_IRQ_MASK		BIT(1)
> +
> +#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_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		(0)
> +#define CTLO_RVAL_2KUP		(1 << 1)
> +#define CTLO_RVAL_50KDW		(2 << 1)
> +#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
> + * @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 = GPIO_IN_CTRL_BASE + bank;
> +	else
> +		reg = GPIO_OUT_CTRL_BASE + bank;
> +
> +	return reg;
> +}
> +
> +static void wcove_update_irq_mask(struct wcove_gpio *wg, int gpio)
> +{
> +	int group;
> +	unsigned int reg, mask;

You forgot to change the ordering of these declarations like

	unsigned int reg, mask;
	int group;

ditto for all other places.

> +
> +	group = gpio < GROUP0_NR_IRQS ? 0 : 1;
> +	reg = IRQ_MASK_BASE + group;
> +	mask = (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, mask, mask);
> +	else
> +		regmap_update_bits(wg->regmap, reg, mask, 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, CTLI_INTCNT_BE, 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)
> +{
> +	struct wcove_gpio *wg = gpiochip_get_data(chip);
> +	int ret;
> +	unsigned int val;
> +
> +	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 = CTLI_INTCNT_DIS;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		wg->intcnt_value = CTLI_INTCNT_BE;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		wg->intcnt_value = CTLI_INTCNT_PE;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		wg->intcnt_value = CTLI_INTCNT_NE;
> +		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));

This was here last time already but you may want to make it look better
by doing this:

	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
	struct wcove_gpio *wg = gpiochip_get_data(chip);

> +
> +	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)
> +{
> +	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");

You have the &wg->chip.dev pointer so use dev_err() here instead. Ditto
for all other places.

> +		return IRQ_NONE;
> +	}
> +
> +	pending = p[0] | (p[1] << 8);
> +	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;
> +			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");
> +			break;
> +		}
> +		pending = p[0] | (p[1] << 8);
> +	}
> +
> +	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, ret = 0;
> +	unsigned int ctlo, ctli, irq_mask, irq_status;
> +
> +	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");
> +			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" : "       ");
> +	}
> +}
> +
> +static int wcove_gpio_probe(struct platform_device *pdev)
> +{
> +	int virq, retval, irq = platform_get_irq(pdev, 0);
> +	struct wcove_gpio *wg;
> +	struct device *dev = pdev->dev.parent;
> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
> +
> +	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_warn(&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 virtual interrupt=%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:
> +	gpiochip_remove(&wg->chip);

I don't think you should call this when you use
devm_gpiochip_add_data().

> +	return retval;
> +}
> +
> +static int wcove_gpio_remove(struct platform_device *pdev)
> +{
> +	struct wcove_gpio *wg = platform_get_drvdata(pdev);
> +
> +	gpiochip_remove(&wg->chip);

And this one is not needed either.

> +	return 0;
> +}
> +
> +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 <ajay.thomas.david.rajamanickam@...el.com>");
> +MODULE_AUTHOR("Bin Gao <bin.gao@...el.com>");
> +MODULE_DESCRIPTION("Intel Whiskey Cove GPIO Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:bxt_wcove_gpio");
> -- 
> 1.9.1

Powered by blists - more mailing lists