* [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver
@ 2016-06-20 18:02 Bin Gao
2016-06-20 23:19 ` Andy Shevchenko
0 siblings, 1 reply; 4+ messages in thread
From: Bin Gao @ 2016-06-20 18:02 UTC (permalink / raw)
To: Mika Westerberg, Mathias Nyman, Linus Walleij, Alexandre Courbot,
linux-gpio
Cc: linux-kernel, Ajay Thomas, Yegnesh S Iyer, 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 <ajay.thomas.david.rajamanickam@intel.com>
Signed-off-by: Bin Gao <bin.gao@intel.com>
---
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 | 425 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 439 insertions(+)
create mode 100644 drivers/gpio/gpio-wcove.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index cebcb40..ac74299 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..61f6d77
--- /dev/null
+++ b/drivers/gpio/gpio-wcove.c
@@ -0,0 +1,425 @@
+/*
+ * 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, bit;
+
+ group = gpio < GROUP0_NR_IRQS ? 0 : 1;
+ reg = IRQ_MASK_BASE + 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, 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));
+
+ 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)
+{
+ struct wcove_gpio *wg = data;
+ unsigned int p0, p1, virq;
+ int pending, gpio;
+
+ if (regmap_read(wg->regmap, IRQ_STATUS_BASE, &p0) ||
+ regmap_read(wg->regmap, IRQ_STATUS_BASE + 1, &p1)) {
+ pr_err("%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_BASE, p0);
+ regmap_write(wg->regmap, IRQ_STATUS_BASE + 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_BASE + group, &irq_mask);
+ regmap_read(wg->regmap, IRQ_STATUS_BASE + 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 & 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);
+ return retval;
+}
+
+static int wcove_gpio_remove(struct platform_device *pdev)
+{
+ struct wcove_gpio *wg = platform_get_drvdata(pdev);
+
+ gpiochip_remove(&wg->chip);
+ 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@intel.com>");
+MODULE_AUTHOR("Bin Gao <bin.gao@intel.com>");
+MODULE_DESCRIPTION("Intel Whiskey Cove GPIO Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:bxt_wcove_gpio");
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver
2016-06-20 18:02 [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver Bin Gao
@ 2016-06-20 23:19 ` Andy Shevchenko
2016-06-21 18:54 ` Bin Gao
0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2016-06-20 23:19 UTC (permalink / raw)
To: Bin Gao
Cc: Mika Westerberg, Mathias Nyman, Linus Walleij, Alexandre Courbot,
linux-gpio, linux-kernel, Ajay Thomas, Yegnesh S Iyer, Bin Gao
On Mon, Jun 20, 2016 at 9:02 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
> This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC.
> This driver is based on gpio-crystalcove.c.
My comments below.
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index cebcb40..ac74299 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
What if I have not a tablet with WC PMIC inside?
> + inside.
> +
> + This driver can also be built as a module. If so, the module will be
> + called gpio-wcove.
> +#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>
Alphabetical order?
> +
> +/*
> + * 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
Can you define all your bases as a) offsets by value, and b) with
_OFFSET suffix instead of _BASE, though second one is up to you.
> +#define UPDATE_IRQ_TYPE BIT(0)
> +#define UPDATE_IRQ_MASK BIT(1)
> +
> +#define CTLI_INTCNT_DIS (0)
(0 << 1) or...
> +#define CTLI_INTCNT_NE (1 << 1)
> +#define CTLI_INTCNT_PE (2 << 1)
> +#define CTLI_INTCNT_BE (3 << 1)
...INTCNT(x) ((x) << 1)
...DIS 0
...NE 1
and so on.
Same for all similar blocks of constants.
> +
> +#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)
Wait, are you sure that's is pure gpio and not a full pinctrl (pinconf) + gpio?
> +
> +#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;
It might be better to use a constant struct to iterate through:
struct gpio_banks {
... bankno;
... start;
... len;
};
Though I have no strong opinion here since it will have only 3 records.
> +
> + 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, bit;
> +
> + group = gpio < GROUP0_NR_IRQS ? 0 : 1;
> + reg = IRQ_MASK_BASE + 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, 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;
Reverse xmas tree, please.
> + 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));
> +
> + mutex_lock(&wg->buslock);
I suppose you have to add a hint to static analyzer here and below.
> +}
> +
> +static void wcove_bus_sync_unlock(struct irq_data *data)
> +{
> + struct wcove_gpio *wg = gpiochip_get_data(
> + irq_data_get_irq_chip_data(data));
One line
> + 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,
Is it my mail client or they are not aligned?
> +};
> +
> +static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
> +{
> + struct wcove_gpio *wg = data;
> + unsigned int p0, p1, virq;
> + int pending, gpio;
unsigned int gpio;
> +
> + if (regmap_read(wg->regmap, IRQ_STATUS_BASE, &p0) ||
Better to use + 0 here.
> + regmap_read(wg->regmap, IRQ_STATUS_BASE + 1, &p1)) {
> + pr_err("%s(): regmap_read() failed.\n", __func__);
dev_err();
> + 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_BASE, p0);
Ditto.
> + regmap_write(wg->regmap, IRQ_STATUS_BASE + 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_BASE + group, &irq_mask);
> + regmap_read(wg->regmap, IRQ_STATUS_BASE + 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 & 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);
> +
Put irq assignment here.
> + 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);
Non fatal? Needs an explanation in the comment.
> + goto out_remove_gpio;
> + }
> +
> + return 0;
> +
> +out_remove_gpio:
> + gpiochip_remove(&wg->chip);
> + return retval;
BLUNDER! You are using devm_*() API.
> +}
> +
> +static int wcove_gpio_remove(struct platform_device *pdev)
> +{
> + struct wcove_gpio *wg = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&wg->chip);
Ditto.
> + return 0;
> +}
> +
> +static struct platform_driver wcove_gpio_driver = {
> + .driver = {
> + .name = "bxt_wcove_gpio",
No PM?
> + },
> + .probe = wcove_gpio_probe,
> + .remove = wcove_gpio_remove,
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver
2016-06-20 23:19 ` Andy Shevchenko
@ 2016-06-21 18:54 ` Bin Gao
2016-06-22 8:27 ` Andy Shevchenko
0 siblings, 1 reply; 4+ messages in thread
From: Bin Gao @ 2016-06-21 18:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mika Westerberg, Mathias Nyman, Linus Walleij, Alexandre Courbot,
linux-gpio, linux-kernel, Ajay Thomas, Yegnesh S Iyer, Bin Gao
On Tue, Jun 21, 2016 at 02:19:57AM +0300, Andy Shevchenko wrote:
> My comments below.
Thanks for your review.
> > +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
>
> What if I have not a tablet with WC PMIC inside?
This driver is for the GPIO controller in the WC PMIC. If there is no WC PMIC,
this driver shouldn't be compiled.
You question is more like:
If a device doesn't exist, what will its driver do?
> > +#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>
>
> Alphabetical order?
I checked Documentation/CodingStyle but didn't find any alphabetical order
description. Is this newly enforced?
> > +#define GROUP0_NR_IRQS 7
> > +#define GROUP1_NR_IRQS 6
> > +#define IRQ_MASK_BASE 0x4e19
> > +#define IRQ_STATUS_BASE 0x4e0b
>
> Can you define all your bases as a) offsets by value, and b) with
> _OFFSET suffix instead of _BASE, though second one is up to you.
Strictly speaking, it's called "offset base". I'm really not sure
it will be more readable to replace BASE with OFFSET here.
> > +#define CTLI_INTCNT_DIS (0)
>
> (0 << 1) or...
>
> > +#define CTLI_INTCNT_NE (1 << 1)
> > +#define CTLI_INTCNT_PE (2 << 1)
> > +#define CTLI_INTCNT_BE (3 << 1)
>
> ...INTCNT(x) ((x) << 1)
> ...DIS 0
> ...NE 1
>
> and so on.
>
> Same for all similar blocks of constants.
Makes sense.
>
> > +
> > +#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)
>
> Wait, are you sure that's is pure gpio and not a full pinctrl (pinconf) + gpio?
No, we don't have pinctrl for this driver. Those bits are just for pure GPIO setting.
> > +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;
>
> It might be better to use a constant struct to iterate through:
>
> struct gpio_banks {
> ... bankno;
> ... start;
> ... len;
> };
>
> Though I have no strong opinion here since it will have only 3 records.
As you said, it's only used once in one function, so not worth to do.
> > +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;
>
> Reverse xmas tree, please.
Will do.
> > +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);
>
> I suppose you have to add a hint to static analyzer here and below.
I didn't quite get it. You meant to add some comments for these 2 functions?
>
> > +}
> > +
> > +static void wcove_bus_sync_unlock(struct irq_data *data)
> > +{
> > + struct wcove_gpio *wg = gpiochip_get_data(
> > + irq_data_get_irq_chip_data(data));
>
> One line
Will do.
> > +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,
>
> Is it my mail client or they are not aligned?
I checked my Sent box as well as the source file and they are well aligned.
> > + int pending, gpio;
>
> unsigned int gpio;
Will do.
> > + if (regmap_read(wg->regmap, IRQ_STATUS_BASE, &p0) ||
>
> Better to use + 0 here.
Make sense, will do.
>
> > + regmap_read(wg->regmap, IRQ_STATUS_BASE + 1, &p1)) {
> > + pr_err("%s(): regmap_read() failed.\n", __func__);
>
> dev_err();
Will do.
> > + regmap_write(wg->regmap, IRQ_STATUS_BASE, p0);
>
> Ditto.
Will do.
> > +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);
>
> > +
>
> Put irq assignment here.
Will do.
>
> > + if (irq < 0)
> > + return irq;
> > + dev_warn(&pdev->dev, "request irq failed: %d, virq: %d\n",
> > + retval, virq);
>
> Non fatal? Needs an explanation in the comment.
Should dev_err(), will fix this.
> > +out_remove_gpio:
> > + gpiochip_remove(&wg->chip);
> > + return retval;
>
> BLUNDER! You are using devm_*() API.
Will fix this.
> > +static int wcove_gpio_remove(struct platform_device *pdev)
> > +{
> > + struct wcove_gpio *wg = platform_get_drvdata(pdev);
> > +
> > + gpiochip_remove(&wg->chip);
>
> Ditto.
Will do.
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver wcove_gpio_driver = {
> > + .driver = {
> > + .name = "bxt_wcove_gpio",
>
> No PM?
Right, no PM for this driver.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver
2016-06-21 18:54 ` Bin Gao
@ 2016-06-22 8:27 ` Andy Shevchenko
0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2016-06-22 8:27 UTC (permalink / raw)
To: Bin Gao
Cc: Mika Westerberg, Mathias Nyman, Linus Walleij, Alexandre Courbot,
linux-gpio, linux-kernel, Ajay Thomas, Yegnesh S Iyer, Bin Gao
On Tue, Jun 21, 2016 at 9:54 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
> On Tue, Jun 21, 2016 at 02:19:57AM +0300, Andy Shevchenko wrote:
>> > + help
>> > + Support for GPIO pins on Whiskey Cove PMIC.
>> > +
>> > + Say Yes if you have a Intel SoC based tablet with Whiskey Cove PMIC
>>
>> What if I have not a tablet with WC PMIC inside?
>
> This driver is for the GPIO controller in the WC PMIC. If there is no WC PMIC,
> this driver shouldn't be compiled.
> You question is more like:
> If a device doesn't exist, what will its driver do?
My comment regarding to 'tablet' word used wrongly in my opinion.
>> > +#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>
>>
>> Alphabetical order?
>
> I checked Documentation/CodingStyle but didn't find any alphabetical order
> description. Is this newly enforced?
Not enforced, but it allows you not to duplicate headers and easily
find one in the list.
>
>> > +#define GROUP0_NR_IRQS 7
>> > +#define GROUP1_NR_IRQS 6
>> > +#define IRQ_MASK_BASE 0x4e19
>> > +#define IRQ_STATUS_BASE 0x4e0b
>>
>> Can you define all your bases as a) offsets by value, and b) with
>> _OFFSET suffix instead of _BASE, though second one is up to you.
>
> Strictly speaking, it's called "offset base". I'm really not sure
> it will be more readable to replace BASE with OFFSET here.
OK.
>> > +#define CTLI_INTCNT_DIS (0)
>>
>> (0 << 1) or...
>>
>> > +#define CTLI_INTCNT_NE (1 << 1)
>> > +#define CTLI_INTCNT_PE (2 << 1)
>> > +#define CTLI_INTCNT_BE (3 << 1)
>>
>> ...INTCNT(x) ((x) << 1)
>> ...DIS 0
>> ...NE 1
>>
>> and so on.
>>
>> Same for all similar blocks of constants.
>
> Makes sense.
Please, choose variant that's more suitable and less verbose.
>> > +#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)
>>
>> Wait, are you sure that's is pure gpio and not a full pinctrl (pinconf) + gpio?
>
> No, we don't have pinctrl for this driver. Those bits are just for pure GPIO setting.
I noticed you are not using bias (yet?). For now I have no strong opinion.
I suppose Linus can share his thoughts.
>> > +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);
>>
>> I suppose you have to add a hint to static analyzer here and below.
>
> I didn't quite get it. You meant to add some comments for these 2 functions?
One function takes lock, the other releases.
Is static analyzer happy with it?
>> > +}
>> > +
>> > +static void wcove_bus_sync_unlock(struct irq_data *data)
>> > +{
>> > + struct wcove_gpio *wg = gpiochip_get_data(
>> > + irq_data_get_irq_chip_data(data));
>> > +static struct platform_driver wcove_gpio_driver = {
>> > + .driver = {
>> > + .name = "bxt_wcove_gpio",
>>
>> No PM?
>
> Right, no PM for this driver.
Perhaps couple of words in the commit message that the WC GPIO IP is
a) power gated automaticaly, or b) AON, or c) actual case.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-22 8:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-20 18:02 [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver Bin Gao
2016-06-20 23:19 ` Andy Shevchenko
2016-06-21 18:54 ` Bin Gao
2016-06-22 8:27 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).