* [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
@ 2014-05-14 15:44 Zhu, Lejun
2014-05-16 17:33 ` Linus Walleij
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-14 15:44 UTC (permalink / raw)
To: linus.walleij, gnurou; +Cc: linux-kernel, linux-gpio, bin.yang, lejun.zhu
Devices based on Intel SoC products such as Baytrail have a Power
Management IC. In the PMIC there are subsystems for voltage regulation,
A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
Crystal Cove.
This patch adds support for the GPIO function in Crystal Cove.
Signed-off-by: Yang, Bin <bin.yang@intel.com>
Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
---
drivers/gpio/Kconfig | 9 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-crystalcove.c | 323 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 333 insertions(+)
create mode 100644 drivers/gpio/gpio-crystalcove.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a86c49a..95bb5a0 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -440,6 +440,15 @@ config GPIO_ARIZONA
help
Support for GPIOs on Wolfson Arizona class devices.
+config GPIO_INTEL_SOC_PMIC
+ bool "GPIO on Intel SoC PMIC"
+ depends on INTEL_SOC_PMIC
+ help
+ Support for GPIO pins on Intel SoC PMIC, such as Crystal
+ Cove.
+ Say Y if you have a tablet with Intel SoC (e.g. Baytrail)
+ inside.
+
config GPIO_LP3943
tristate "TI/National Semiconductor LP3943 GPIO expander"
depends on MFD_LP3943
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6309aff..5380608 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
+obj-$(CONFIG_GPIO_INTEL_SOC_PMIC) += gpio-crystalcove.o
obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
obj-$(CONFIG_GPIO_IT8761E) += gpio-it8761e.o
obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o
diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
new file mode 100644
index 0000000..974f9b8
--- /dev/null
+++ b/drivers/gpio/gpio-crystalcove.c
@@ -0,0 +1,323 @@
+/*
+ * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
+ *
+ * Copyright (C) 2012, 2014 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.
+ *
+ * Author: Yang, Bin <bin.yang@intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/sched.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/gpio.h>
+
+#define NUM_GPIO 16
+
+#define UPDATE_TYPE (1 << 0)
+#define UPDATE_MASK (1 << 1)
+
+#define GPIO0IRQ 0x0b
+#define GPIO1IRQ 0x0c
+#define MGPIO0IRQS0 0x19
+#define MGPIO1IRQS0 0x1a
+#define MGPIO0IRQSX 0x1b
+#define MGPIO1IRQSX 0x1c
+#define GPIO0P0CTLO 0x2b
+#define GPIO0P0CTLI 0x33
+#define GPIO1P0CTLO 0x3b
+#define GPIO1P0CTLI 0x43
+
+#define CTLI_INTCNT_NE (1 << 1)
+#define CTLI_INTCNT_PE (2 << 1)
+#define CTLI_INTCNT_BE (3 << 1)
+
+#define CTLO_DIR_OUT (1 << 5)
+#define CTLO_DRV_CMOS (0 << 4)
+#define CTLO_DRV_OD (1 << 4)
+#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_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
+#define CTLO_OUTPUT_DEF (CTLO_DIR_OUT | CTLO_INPUT_DEF)
+
+struct crystalcove_gpio {
+ struct mutex buslock; /* irq_bus_lock */
+ struct gpio_chip chip;
+ int irq;
+ int irq_base;
+ int update;
+ int trigger_type;
+ int irq_mask;
+};
+static struct crystalcove_gpio gpio_info;
+
+static void __crystalcove_irq_mask(int gpio, int mask)
+{
+ u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
+ int offset = gpio < 8 ? gpio : gpio - 8;
+
+ if (mask)
+ intel_soc_pmic_setb(mirqs0, 1 << offset);
+ else
+ intel_soc_pmic_clearb(mirqs0, 1 << offset);
+}
+
+static void __crystalcove_irq_type(int gpio, int type)
+{
+ u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
+
+ type &= IRQ_TYPE_EDGE_BOTH;
+ intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
+
+ if (type == IRQ_TYPE_EDGE_BOTH)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
+ else if (type == IRQ_TYPE_EDGE_RISING)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
+ else if (type & IRQ_TYPE_EDGE_FALLING)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
+}
+
+static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
+ unsigned gpio)
+{
+ u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
+
+ intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
+ return 0;
+}
+
+static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
+ unsigned gpio, int value)
+{
+ u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
+
+ intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
+ return 0;
+}
+
+static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+ u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
+
+ return intel_soc_pmic_readb(ctli) & 0x1;
+}
+
+static void crystalcove_gpio_set(struct gpio_chip *chip,
+ unsigned gpio, int value)
+{
+ u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
+
+ if (value)
+ intel_soc_pmic_setb(ctlo, 1);
+ else
+ intel_soc_pmic_clearb(ctlo, 1);
+}
+
+static int crystalcove_irq_type(struct irq_data *data, unsigned type)
+{
+ struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
+
+ cg->trigger_type = type;
+ cg->update |= UPDATE_TYPE;
+
+ return 0;
+}
+
+static int crystalcove_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
+{
+ struct crystalcove_gpio *cg =
+ container_of(chip, struct crystalcove_gpio, chip);
+
+ return cg->irq_base + gpio;
+}
+
+static void crystalcove_bus_lock(struct irq_data *data)
+{
+ struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
+
+ mutex_lock(&cg->buslock);
+}
+
+static void crystalcove_bus_sync_unlock(struct irq_data *data)
+{
+ struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
+ int gpio = data->irq - cg->irq_base;
+
+ if (cg->update & UPDATE_TYPE)
+ __crystalcove_irq_type(gpio, cg->trigger_type);
+ if (cg->update & UPDATE_MASK)
+ __crystalcove_irq_mask(gpio, cg->irq_mask);
+ cg->update = 0;
+
+ mutex_unlock(&cg->buslock);
+}
+
+static void crystalcove_irq_unmask(struct irq_data *data)
+{
+ struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
+
+ cg->irq_mask = 0;
+ cg->update |= UPDATE_MASK;
+}
+
+static void crystalcove_irq_mask(struct irq_data *data)
+{
+ struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
+
+ cg->irq_mask = 1;
+ cg->update |= UPDATE_MASK;
+}
+
+static struct irq_chip crystalcove_irqchip = {
+ .name = "PMIC-GPIO",
+ .irq_mask = crystalcove_irq_mask,
+ .irq_unmask = crystalcove_irq_unmask,
+ .irq_set_type = crystalcove_irq_type,
+ .irq_bus_lock = crystalcove_bus_lock,
+ .irq_bus_sync_unlock = crystalcove_bus_sync_unlock,
+};
+
+static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
+{
+ struct crystalcove_gpio *cg = data;
+ int pending;
+ int gpio;
+
+ pending = intel_soc_pmic_readb(GPIO0IRQ) & 0xff;
+ pending |= (intel_soc_pmic_readb(GPIO1IRQ) & 0xff) << 8;
+ intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
+ intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
+
+ local_irq_disable();
+ for (gpio = 0; gpio < cg->chip.ngpio; gpio++)
+ if (pending & (1 << gpio))
+ generic_handle_irq(cg->irq_base + gpio);
+ local_irq_enable();
+
+ return IRQ_HANDLED;
+}
+
+static void crystalcove_gpio_dbg_show(struct seq_file *s,
+ struct gpio_chip *chip)
+{
+ struct crystalcove_gpio *cg =
+ container_of(chip, struct crystalcove_gpio, chip);
+ int gpio, offset;
+ u8 ctlo, ctli, mirqs0, mirqsx, irq;
+
+ for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
+ offset = gpio < 8 ? gpio : gpio - 8;
+ ctlo = intel_soc_pmic_readb(
+ (gpio < 8 ? GPIO0P0CTLO : GPIO1P0CTLO) + offset);
+ ctli = intel_soc_pmic_readb(
+ (gpio < 8 ? GPIO0P0CTLI : GPIO1P0CTLI) + offset);
+ mirqs0 = intel_soc_pmic_readb(
+ gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
+ mirqsx = intel_soc_pmic_readb(
+ gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
+ irq = intel_soc_pmic_readb(
+ gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
+ seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %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,
+ mirqs0 & (1 << offset) ? "s0 mask " : "s0 unmask",
+ mirqsx & (1 << offset) ? "sx mask " : "sx unmask",
+ irq & (1 << offset) ? "pending" : " ");
+ }
+}
+
+static int crystalcove_gpio_probe(struct platform_device *pdev)
+{
+ int irq = platform_get_irq(pdev, 0);
+ struct crystalcove_gpio *cg = &gpio_info;
+ int retval;
+ int i;
+ struct device *dev = intel_soc_pmic_dev();
+
+ mutex_init(&cg->buslock);
+ cg->chip.label = "intel_crystalcove";
+ cg->chip.direction_input = crystalcove_gpio_direction_input;
+ cg->chip.direction_output = crystalcove_gpio_direction_output;
+ cg->chip.get = crystalcove_gpio_get;
+ cg->chip.set = crystalcove_gpio_set;
+ cg->chip.to_irq = crystalcove_gpio_to_irq;
+ cg->chip.base = -1;
+ cg->chip.ngpio = NUM_GPIO;
+ cg->chip.can_sleep = 1;
+ cg->chip.dev = dev;
+ cg->chip.dbg_show = crystalcove_gpio_dbg_show;
+
+ retval = gpiochip_add(&cg->chip);
+ if (retval) {
+ dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
+ goto out;
+ }
+
+ cg->irq_base = irq_alloc_descs(-1, 0, NUM_GPIO, 0);
+ if (cg->irq_base < 0) {
+ retval = cg->irq_base;
+ cg->irq_base = 0;
+ dev_warn(&pdev->dev, "irq_alloc_descs error: %d\n", retval);
+ goto out_remove_gpio;
+ }
+
+ for (i = 0; i < NUM_GPIO; i++) {
+ irq_set_chip_data(i + cg->irq_base, cg);
+ irq_set_chip_and_handler_name(i + cg->irq_base,
+ &crystalcove_irqchip,
+ handle_simple_irq, "demux");
+ }
+
+ retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
+ IRQF_ONESHOT, "crystalcove_gpio", cg);
+
+ if (retval) {
+ dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
+ goto out_free_desc;
+ }
+
+ return 0;
+
+out_free_desc:
+ irq_free_descs(cg->irq_base, NUM_GPIO);
+out_remove_gpio:
+ gpiochip_remove(&cg->chip);
+out:
+ return retval;
+}
+
+static struct platform_driver crystalcove_gpio_driver = {
+ .probe = crystalcove_gpio_probe,
+ .driver = {
+ .name = "crystal_cove_gpio",
+ },
+};
+
+module_platform_driver(crystalcove_gpio_driver);
+
+MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
+MODULE_DESCRIPTION("Intel Crystal Cove GPIO Driver");
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-14 15:44 [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove) Zhu, Lejun
@ 2014-05-16 17:33 ` Linus Walleij
2014-05-19 0:27 ` Zhu, Lejun
2014-05-16 17:46 ` Daniel Glöckner
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2014-05-16 17:33 UTC (permalink / raw)
To: Zhu, Lejun
Cc: Alexandre Courbot, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, bin.yang, Darren Hart, Holmberg, Hans,
Mika Westerberg, Mathias Nyman
On Wed, May 14, 2014 at 5:44 PM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
> Devices based on Intel SoC products such as Baytrail have a Power
> Management IC. In the PMIC there are subsystems for voltage regulation,
> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
> Crystal Cove.
>
> This patch adds support for the GPIO function in Crystal Cove.
>
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
(...)
> +config GPIO_INTEL_SOC_PMIC
> + bool "GPIO on Intel SoC PMIC"
> + depends on INTEL_SOC_PMIC
select GPIOLIB_IRQCHIP
and use the infrastructure from commit
1425052097b53de841e064dc190a9009480c208c
"gpio: add IRQ chip helpers in gpiolib"
Some fixes for sleeping chips have been merged and are available
on the "devel" branch in the GPIO tree, so you may need to base
your development on that.
Adding some kerneldoc to the below struct will maybe make you
realize whether you actually need it or not.
> +struct crystalcove_gpio {
> + struct mutex buslock; /* irq_bus_lock */
> + struct gpio_chip chip;
> + int irq;
> + int irq_base;
You should not have hardcoded IRQ bases around. Use an irqdomain
to map IRQs, and even better: use the one provided in
chip->irqdomain by GPIOLIB_IRQCHIP. (It's a requirement.)
> + int update;
> + int trigger_type;
> + int irq_mask;
Should this be a bool since you just set it to 0 or 1?
> +};
> +static struct crystalcove_gpio gpio_info;
> +
> +static void __crystalcove_irq_mask(int gpio, int mask)
I don't really like __doubleunderscore specifiers, can you use a
unique name or something instead.
> +{
> + u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
> + int offset = gpio < 8 ? gpio : gpio - 8;
Meh.
unsigned int offset = gpio%8;
> +
> + if (mask)
> + intel_soc_pmic_setb(mirqs0, 1 << offset);
Use
#include <linux/bitops.h>
intel_soc_pmic_setb(mirqs0, BIT(offset));
I really like the BIT() macros.
> + else
> + intel_soc_pmic_clearb(mirqs0, 1 << offset);
Dito.
> +static void __crystalcove_irq_type(int gpio, int type)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
Do it like the first time instead, this is hard to read.
> + type &= IRQ_TYPE_EDGE_BOTH;
> + intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
> +
> + if (type == IRQ_TYPE_EDGE_BOTH)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
> + else if (type & IRQ_TYPE_EDGE_FALLING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
> +}
> +
> +static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
> + unsigned gpio)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
Dito.
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
Dito. (etc for all)
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
> +
> + return intel_soc_pmic_readb(ctli) & 0x1;
> +}
> +
> +static void crystalcove_gpio_set(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + if (value)
> + intel_soc_pmic_setb(ctlo, 1);
> + else
> + intel_soc_pmic_clearb(ctlo, 1);
> +}
> +
> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->trigger_type = type;
> + cg->update |= UPDATE_TYPE;
> +
> + return 0;
> +}
> +
> +static int crystalcove_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> +
> + return cg->irq_base + gpio;
> +}
Nope. Use the irqdomain in chip->irqdomain.
> +static void crystalcove_bus_lock(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
This and all IRQ function callbacks needs to use a construct like
this:
struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
struct crystalcove_gpio *cg = container_of(gc, struct crystalcove_gpio, chip);
> +
> + mutex_lock(&cg->buslock);
> +}
> +
> +static void crystalcove_bus_sync_unlock(struct irq_data *data)
> +{
The same here and for each irq function, as the irqchip helpers pass
struct gpio_chip* as irq_chip_data.
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> + int gpio = data->irq - cg->irq_base;
> +
> + if (cg->update & UPDATE_TYPE)
> + __crystalcove_irq_type(gpio, cg->trigger_type);
> + if (cg->update & UPDATE_MASK)
> + __crystalcove_irq_mask(gpio, cg->irq_mask);
> + cg->update = 0;
> +
> + mutex_unlock(&cg->buslock);
> +}
> +
> +static void crystalcove_irq_unmask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 0;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static void crystalcove_irq_mask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 1;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static struct irq_chip crystalcove_irqchip = {
> + .name = "PMIC-GPIO",
> + .irq_mask = crystalcove_irq_mask,
> + .irq_unmask = crystalcove_irq_unmask,
> + .irq_set_type = crystalcove_irq_type,
> + .irq_bus_lock = crystalcove_bus_lock,
> + .irq_bus_sync_unlock = crystalcove_bus_sync_unlock,
> +};
> +
> +static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
> +{
> + struct crystalcove_gpio *cg = data;
If you also use gpiochip_set_chained_irqchip() you need something like
this here:
struct gpio_chip *gc = data;
then the container_of() construction.
> + int pending;
> + int gpio;
> +
> + pending = intel_soc_pmic_readb(GPIO0IRQ) & 0xff;
> + pending |= (intel_soc_pmic_readb(GPIO1IRQ) & 0xff) << 8;
> + intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
> + intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
> +
> + local_irq_disable();
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++)
> + if (pending & (1 << gpio))
> + generic_handle_irq(cg->irq_base + gpio);
> + local_irq_enable();
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void crystalcove_gpio_dbg_show(struct seq_file *s,
> + struct gpio_chip *chip)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> + int gpio, offset;
> + u8 ctlo, ctli, mirqs0, mirqsx, irq;
> +
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
> + offset = gpio < 8 ? gpio : gpio - 8;
> + ctlo = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLO : GPIO1P0CTLO) + offset);
> + ctli = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLI : GPIO1P0CTLI) + offset);
> + mirqs0 = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
> + mirqsx = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
> + irq = intel_soc_pmic_readb(
> + gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
> + seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %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,
> + mirqs0 & (1 << offset) ? "s0 mask " : "s0 unmask",
> + mirqsx & (1 << offset) ? "sx mask " : "sx unmask",
> + irq & (1 << offset) ? "pending" : " ");
> + }
> +}
Looks helpful!
> +static int crystalcove_gpio_probe(struct platform_device *pdev)
> +{
> + int irq = platform_get_irq(pdev, 0);
> + struct crystalcove_gpio *cg = &gpio_info;
> + int retval;
> + int i;
> + struct device *dev = intel_soc_pmic_dev();
> +
> + mutex_init(&cg->buslock);
> + cg->chip.label = "intel_crystalcove";
> + cg->chip.direction_input = crystalcove_gpio_direction_input;
> + cg->chip.direction_output = crystalcove_gpio_direction_output;
> + cg->chip.get = crystalcove_gpio_get;
> + cg->chip.set = crystalcove_gpio_set;
> + cg->chip.to_irq = crystalcove_gpio_to_irq;
You don't define to_irq when using GPIOLIB_IRQCHIP.
> + cg->chip.base = -1;
> + cg->chip.ngpio = NUM_GPIO;
> + cg->chip.can_sleep = 1;
true. Set it to true. This is a bool.
> + cg->chip.dev = dev;
> + cg->chip.dbg_show = crystalcove_gpio_dbg_show;
> +
> + retval = gpiochip_add(&cg->chip);
> + if (retval) {
> + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> + goto out;
> + }
Skip from here:
> + cg->irq_base = irq_alloc_descs(-1, 0, NUM_GPIO, 0);
> + if (cg->irq_base < 0) {
> + retval = cg->irq_base;
> + cg->irq_base = 0;
> + dev_warn(&pdev->dev, "irq_alloc_descs error: %d\n", retval);
> + goto out_remove_gpio;
> + }
> +
> + for (i = 0; i < NUM_GPIO; i++) {
> + irq_set_chip_data(i + cg->irq_base, cg);
> + irq_set_chip_and_handler_name(i + cg->irq_base,
> + &crystalcove_irqchip,
> + handle_simple_irq, "demux");
> + }
To here, replace with:
gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, cg->irq_base,
handle_simple_irq, IRQ_TYPE_NONE);
> + retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
> + IRQF_ONESHOT, "crystalcove_gpio", cg);
> +
> + if (retval) {
> + dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> + goto out_free_desc;
> + }
Maybe use
gpiochip_set_chained_irqchip() here.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-14 15:44 [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove) Zhu, Lejun
2014-05-16 17:33 ` Linus Walleij
@ 2014-05-16 17:46 ` Daniel Glöckner
2014-05-19 1:46 ` Zhu, Lejun
2014-05-17 14:37 ` [PATCH] " Alexandre Courbot
2014-05-19 10:39 ` Mika Westerberg
3 siblings, 1 reply; 13+ messages in thread
From: Daniel Glöckner @ 2014-05-16 17:46 UTC (permalink / raw)
To: Zhu, Lejun; +Cc: linus.walleij, gnurou, linux-kernel, linux-gpio, bin.yang
Hi Lejun,
On Wed, May 14, 2014 at 11:44:07PM +0800, Zhu, Lejun wrote:
> This patch adds support for the GPIO function in Crystal Cove.
in our device ACPI makes use of "virtual" GPIOs that have numbers from
0x20 to 0x5E to change various bits in the PMIC. Do you know if this
is officially supported by the INT33FD ACPI device or if it is a
vendor hack?
Daniel
--
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Bertha-von-Suttner-Straße 9, 37085 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Dr. Uwe Kracke, Ust-IdNr.: DE 205 198 055
emlix - your embedded linux partner
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-14 15:44 [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove) Zhu, Lejun
2014-05-16 17:33 ` Linus Walleij
2014-05-16 17:46 ` Daniel Glöckner
@ 2014-05-17 14:37 ` Alexandre Courbot
2014-05-19 0:28 ` Zhu, Lejun
2014-05-27 9:01 ` Linus Walleij
2014-05-19 10:39 ` Mika Westerberg
3 siblings, 2 replies; 13+ messages in thread
From: Alexandre Courbot @ 2014-05-17 14:37 UTC (permalink / raw)
To: Zhu, Lejun
Cc: Linus Walleij, Linux Kernel Mailing List,
linux-gpio@vger.kernel.org, bin.yang
On Thu, May 15, 2014 at 12:44 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
> Devices based on Intel SoC products such as Baytrail have a Power
> Management IC. In the PMIC there are subsystems for voltage regulation,
> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
> Crystal Cove.
>
> This patch adds support for the GPIO function in Crystal Cove.
>
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
> drivers/gpio/Kconfig | 9 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-crystalcove.c | 323 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 333 insertions(+)
> create mode 100644 drivers/gpio/gpio-crystalcove.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..95bb5a0 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -440,6 +440,15 @@ config GPIO_ARIZONA
> help
> Support for GPIOs on Wolfson Arizona class devices.
>
> +config GPIO_INTEL_SOC_PMIC
> + bool "GPIO on Intel SoC PMIC"
> + depends on INTEL_SOC_PMIC
> + help
> + Support for GPIO pins on Intel SoC PMIC, such as Crystal
> + Cove.
> + Say Y if you have a tablet with Intel SoC (e.g. Baytrail)
> + inside.
> +
> config GPIO_LP3943
> tristate "TI/National Semiconductor LP3943 GPIO expander"
> depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..5380608 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> +obj-$(CONFIG_GPIO_INTEL_SOC_PMIC) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> obj-$(CONFIG_GPIO_IT8761E) += gpio-it8761e.o
> obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o
> diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
> new file mode 100644
> index 0000000..974f9b8
> --- /dev/null
> +++ b/drivers/gpio/gpio-crystalcove.c
> @@ -0,0 +1,323 @@
> +/*
> + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
> + *
> + * Copyright (C) 2012, 2014 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.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/sched.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/gpio.h>
> +
> +#define NUM_GPIO 16
> +
> +#define UPDATE_TYPE (1 << 0)
> +#define UPDATE_MASK (1 << 1)
> +
> +#define GPIO0IRQ 0x0b
> +#define GPIO1IRQ 0x0c
> +#define MGPIO0IRQS0 0x19
> +#define MGPIO1IRQS0 0x1a
> +#define MGPIO0IRQSX 0x1b
> +#define MGPIO1IRQSX 0x1c
> +#define GPIO0P0CTLO 0x2b
> +#define GPIO0P0CTLI 0x33
> +#define GPIO1P0CTLO 0x3b
> +#define GPIO1P0CTLI 0x43
> +
> +#define CTLI_INTCNT_NE (1 << 1)
> +#define CTLI_INTCNT_PE (2 << 1)
> +#define CTLI_INTCNT_BE (3 << 1)
> +
> +#define CTLO_DIR_OUT (1 << 5)
> +#define CTLO_DRV_CMOS (0 << 4)
> +#define CTLO_DRV_OD (1 << 4)
> +#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_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
> +#define CTLO_OUTPUT_DEF (CTLO_DIR_OUT | CTLO_INPUT_DEF)
> +
> +struct crystalcove_gpio {
> + struct mutex buslock; /* irq_bus_lock */
> + struct gpio_chip chip;
> + int irq;
> + int irq_base;
> + int update;
> + int trigger_type;
> + int irq_mask;
> +};
> +static struct crystalcove_gpio gpio_info;
> +
> +static void __crystalcove_irq_mask(int gpio, int mask)
> +{
> + u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
> + int offset = gpio < 8 ? gpio : gpio - 8;
> +
> + if (mask)
> + intel_soc_pmic_setb(mirqs0, 1 << offset);
> + else
> + intel_soc_pmic_clearb(mirqs0, 1 << offset);
> +}
> +
> +static void __crystalcove_irq_type(int gpio, int type)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
Not a big comment, but wouldn't it be safer to factorize this logic
(which is repeated in many places) into some macro? e.g. something
like:
#define GPIOP0CTL(gpio, dir) ((gpio < 8 ? GPIO0P0CTL ## dir :
GPIO1P0CTL ## dir) + (gpio & ~0x8))
...
u8 ctli = GPIOP0CTL(gpio, I);
Feel free to come with a better macro (or to ignore that comment
altogether if you think it makes the code less readable), but I think
it would be less error-prone if you did not have to copy-paste that
code all over the place.
> +
> + type &= IRQ_TYPE_EDGE_BOTH;
> + intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
> +
> + if (type == IRQ_TYPE_EDGE_BOTH)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
> + else if (type & IRQ_TYPE_EDGE_FALLING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
> +}
> +
> +static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
> + unsigned gpio)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
> +
> + return intel_soc_pmic_readb(ctli) & 0x1;
> +}
> +
> +static void crystalcove_gpio_set(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + if (value)
> + intel_soc_pmic_setb(ctlo, 1);
> + else
> + intel_soc_pmic_clearb(ctlo, 1);
> +}
> +
> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->trigger_type = type;
> + cg->update |= UPDATE_TYPE;
> +
> + return 0;
> +}
> +
> +static int crystalcove_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> +
> + return cg->irq_base + gpio;
> +}
> +
> +static void crystalcove_bus_lock(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + mutex_lock(&cg->buslock);
> +}
> +
> +static void crystalcove_bus_sync_unlock(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> + int gpio = data->irq - cg->irq_base;
> +
> + if (cg->update & UPDATE_TYPE)
> + __crystalcove_irq_type(gpio, cg->trigger_type);
> + if (cg->update & UPDATE_MASK)
> + __crystalcove_irq_mask(gpio, cg->irq_mask);
> + cg->update = 0;
> +
> + mutex_unlock(&cg->buslock);
> +}
> +
> +static void crystalcove_irq_unmask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 0;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static void crystalcove_irq_mask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 1;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static struct irq_chip crystalcove_irqchip = {
> + .name = "PMIC-GPIO",
> + .irq_mask = crystalcove_irq_mask,
> + .irq_unmask = crystalcove_irq_unmask,
> + .irq_set_type = crystalcove_irq_type,
> + .irq_bus_lock = crystalcove_bus_lock,
> + .irq_bus_sync_unlock = crystalcove_bus_sync_unlock,
> +};
> +
> +static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
> +{
> + struct crystalcove_gpio *cg = data;
> + int pending;
> + int gpio;
> +
> + pending = intel_soc_pmic_readb(GPIO0IRQ) & 0xff;
> + pending |= (intel_soc_pmic_readb(GPIO1IRQ) & 0xff) << 8;
> + intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
> + intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
> +
> + local_irq_disable();
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++)
> + if (pending & (1 << gpio))
> + generic_handle_irq(cg->irq_base + gpio);
> + local_irq_enable();
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void crystalcove_gpio_dbg_show(struct seq_file *s,
> + struct gpio_chip *chip)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> + int gpio, offset;
> + u8 ctlo, ctli, mirqs0, mirqsx, irq;
> +
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
> + offset = gpio < 8 ? gpio : gpio - 8;
> + ctlo = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLO : GPIO1P0CTLO) + offset);
> + ctli = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLI : GPIO1P0CTLI) + offset);
> + mirqs0 = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
> + mirqsx = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
> + irq = intel_soc_pmic_readb(
> + gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
> + seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %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,
> + mirqs0 & (1 << offset) ? "s0 mask " : "s0 unmask",
> + mirqsx & (1 << offset) ? "sx mask " : "sx unmask",
> + irq & (1 << offset) ? "pending" : " ");
> + }
> +}
> +
> +static int crystalcove_gpio_probe(struct platform_device *pdev)
> +{
> + int irq = platform_get_irq(pdev, 0);
> + struct crystalcove_gpio *cg = &gpio_info;
> + int retval;
> + int i;
> + struct device *dev = intel_soc_pmic_dev();
> +
> + mutex_init(&cg->buslock);
> + cg->chip.label = "intel_crystalcove";
> + cg->chip.direction_input = crystalcove_gpio_direction_input;
> + cg->chip.direction_output = crystalcove_gpio_direction_output;
> + cg->chip.get = crystalcove_gpio_get;
> + cg->chip.set = crystalcove_gpio_set;
> + cg->chip.to_irq = crystalcove_gpio_to_irq;
> + cg->chip.base = -1;
> + cg->chip.ngpio = NUM_GPIO;
> + cg->chip.can_sleep = 1;
> + cg->chip.dev = dev;
> + cg->chip.dbg_show = crystalcove_gpio_dbg_show;
> +
> + retval = gpiochip_add(&cg->chip);
> + if (retval) {
> + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> + goto out;
> + }
> +
> + cg->irq_base = irq_alloc_descs(-1, 0, NUM_GPIO, 0);
> + if (cg->irq_base < 0) {
> + retval = cg->irq_base;
> + cg->irq_base = 0;
> + dev_warn(&pdev->dev, "irq_alloc_descs error: %d\n", retval);
> + goto out_remove_gpio;
> + }
> +
> + for (i = 0; i < NUM_GPIO; i++) {
> + irq_set_chip_data(i + cg->irq_base, cg);
> + irq_set_chip_and_handler_name(i + cg->irq_base,
> + &crystalcove_irqchip,
> + handle_simple_irq, "demux");
> + }
> +
> + retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
> + IRQF_ONESHOT, "crystalcove_gpio", cg);
> +
> + if (retval) {
> + dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> + goto out_free_desc;
> + }
> +
> + return 0;
> +
> +out_free_desc:
> + irq_free_descs(cg->irq_base, NUM_GPIO);
> +out_remove_gpio:
> + gpiochip_remove(&cg->chip);
> +out:
> + return retval;
> +}
> +
> +static struct platform_driver crystalcove_gpio_driver = {
> + .probe = crystalcove_gpio_probe,
> + .driver = {
> + .name = "crystal_cove_gpio",
> + },
> +};
> +
> +module_platform_driver(crystalcove_gpio_driver);
> +
> +MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
> +MODULE_DESCRIPTION("Intel Crystal Cove GPIO Driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-16 17:33 ` Linus Walleij
@ 2014-05-19 0:27 ` Zhu, Lejun
2014-05-19 14:13 ` Mathias Nyman
0 siblings, 1 reply; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-19 0:27 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, bin.yang, Darren Hart, Holmberg, Hans,
Mika Westerberg, Mathias Nyman
On 5/17/2014 1:33 AM, Linus Walleij wrote:
> On Wed, May 14, 2014 at 5:44 PM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
>>
>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>
> (...)
>
>> +config GPIO_INTEL_SOC_PMIC
>> + bool "GPIO on Intel SoC PMIC"
>> + depends on INTEL_SOC_PMIC
>
> select GPIOLIB_IRQCHIP
>
> and use the infrastructure from commit
> 1425052097b53de841e064dc190a9009480c208c
> "gpio: add IRQ chip helpers in gpiolib"
>
> Some fixes for sleeping chips have been merged and are available
> on the "devel" branch in the GPIO tree, so you may need to base
> your development on that.
>
> Adding some kerneldoc to the below struct will maybe make you
> realize whether you actually need it or not.
>
>> +struct crystalcove_gpio {
>> + struct mutex buslock; /* irq_bus_lock */
>> + struct gpio_chip chip;
>> + int irq;
>> + int irq_base;
>
> You should not have hardcoded IRQ bases around. Use an irqdomain
> to map IRQs, and even better: use the one provided in
> chip->irqdomain by GPIOLIB_IRQCHIP. (It's a requirement.)
>
>> + int update;
>> + int trigger_type;
>> + int irq_mask;
>
> Should this be a bool since you just set it to 0 or 1?
>
>> +};
>> +static struct crystalcove_gpio gpio_info;
>> +
>> +static void __crystalcove_irq_mask(int gpio, int mask)
>
> I don't really like __doubleunderscore specifiers, can you use a
> unique name or something instead.
>
>> +{
>> + u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
>> + int offset = gpio < 8 ? gpio : gpio - 8;
>
> Meh.
> unsigned int offset = gpio%8;
>
>> +
>> + if (mask)
>> + intel_soc_pmic_setb(mirqs0, 1 << offset);
>
> Use
> #include <linux/bitops.h>
>
> intel_soc_pmic_setb(mirqs0, BIT(offset));
>
> I really like the BIT() macros.
>
>> + else
>> + intel_soc_pmic_clearb(mirqs0, 1 << offset);
>
> Dito.
>
>> +static void __crystalcove_irq_type(int gpio, int type)
>> +{
>> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
>
> Do it like the first time instead, this is hard to read.
>
>> + type &= IRQ_TYPE_EDGE_BOTH;
>> + intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
>> +
>> + if (type == IRQ_TYPE_EDGE_BOTH)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
>> + else if (type == IRQ_TYPE_EDGE_RISING)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
>> + else if (type & IRQ_TYPE_EDGE_FALLING)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
>> +}
>> +
>> +static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
>> + unsigned gpio)
>> +{
>> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
>
> Dito.
>
>> +
>> + intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
>> + return 0;
>> +}
>> +
>> +static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
>> + unsigned gpio, int value)
>> +{
>> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
>
> Dito. (etc for all)
>
>> +
>> + intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
>> + return 0;
>> +}
>> +
>> +static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
>> +{
>> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
>> +
>> + return intel_soc_pmic_readb(ctli) & 0x1;
>> +}
>> +
>> +static void crystalcove_gpio_set(struct gpio_chip *chip,
>> + unsigned gpio, int value)
>> +{
>> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
>> +
>> + if (value)
>> + intel_soc_pmic_setb(ctlo, 1);
>> + else
>> + intel_soc_pmic_clearb(ctlo, 1);
>> +}
>> +
>> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
>> +{
>> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
>> +
>> + cg->trigger_type = type;
>> + cg->update |= UPDATE_TYPE;
>> +
>> + return 0;
>> +}
>> +
>> +static int crystalcove_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
>> +{
>> + struct crystalcove_gpio *cg =
>> + container_of(chip, struct crystalcove_gpio, chip);
>> +
>> + return cg->irq_base + gpio;
>> +}
>
> Nope. Use the irqdomain in chip->irqdomain.
>
>> +static void crystalcove_bus_lock(struct irq_data *data)
>> +{
>> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
>
> This and all IRQ function callbacks needs to use a construct like
> this:
>
> struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> struct crystalcove_gpio *cg = container_of(gc, struct crystalcove_gpio, chip);
>
>> +
>> + mutex_lock(&cg->buslock);
>> +}
>> +
>> +static void crystalcove_bus_sync_unlock(struct irq_data *data)
>> +{
>
> The same here and for each irq function, as the irqchip helpers pass
> struct gpio_chip* as irq_chip_data.
>
>> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
>> + int gpio = data->irq - cg->irq_base;
>> +
>> + if (cg->update & UPDATE_TYPE)
>> + __crystalcove_irq_type(gpio, cg->trigger_type);
>> + if (cg->update & UPDATE_MASK)
>> + __crystalcove_irq_mask(gpio, cg->irq_mask);
>> + cg->update = 0;
>> +
>> + mutex_unlock(&cg->buslock);
>> +}
>> +
>> +static void crystalcove_irq_unmask(struct irq_data *data)
>> +{
>> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
>> +
>> + cg->irq_mask = 0;
>> + cg->update |= UPDATE_MASK;
>> +}
>> +
>> +static void crystalcove_irq_mask(struct irq_data *data)
>> +{
>> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
>> +
>> + cg->irq_mask = 1;
>> + cg->update |= UPDATE_MASK;
>> +}
>> +
>> +static struct irq_chip crystalcove_irqchip = {
>> + .name = "PMIC-GPIO",
>> + .irq_mask = crystalcove_irq_mask,
>> + .irq_unmask = crystalcove_irq_unmask,
>> + .irq_set_type = crystalcove_irq_type,
>> + .irq_bus_lock = crystalcove_bus_lock,
>> + .irq_bus_sync_unlock = crystalcove_bus_sync_unlock,
>> +};
>> +
>> +static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
>> +{
>> + struct crystalcove_gpio *cg = data;
>
> If you also use gpiochip_set_chained_irqchip() you need something like
> this here:
>
> struct gpio_chip *gc = data;
> then the container_of() construction.
>
>> + int pending;
>> + int gpio;
>> +
>> + pending = intel_soc_pmic_readb(GPIO0IRQ) & 0xff;
>> + pending |= (intel_soc_pmic_readb(GPIO1IRQ) & 0xff) << 8;
>> + intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
>> + intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
>> +
>> + local_irq_disable();
>> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++)
>> + if (pending & (1 << gpio))
>> + generic_handle_irq(cg->irq_base + gpio);
>> + local_irq_enable();
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void crystalcove_gpio_dbg_show(struct seq_file *s,
>> + struct gpio_chip *chip)
>> +{
>> + struct crystalcove_gpio *cg =
>> + container_of(chip, struct crystalcove_gpio, chip);
>> + int gpio, offset;
>> + u8 ctlo, ctli, mirqs0, mirqsx, irq;
>> +
>> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
>> + offset = gpio < 8 ? gpio : gpio - 8;
>> + ctlo = intel_soc_pmic_readb(
>> + (gpio < 8 ? GPIO0P0CTLO : GPIO1P0CTLO) + offset);
>> + ctli = intel_soc_pmic_readb(
>> + (gpio < 8 ? GPIO0P0CTLI : GPIO1P0CTLI) + offset);
>> + mirqs0 = intel_soc_pmic_readb(
>> + gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
>> + mirqsx = intel_soc_pmic_readb(
>> + gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
>> + irq = intel_soc_pmic_readb(
>> + gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
>> + seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %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,
>> + mirqs0 & (1 << offset) ? "s0 mask " : "s0 unmask",
>> + mirqsx & (1 << offset) ? "sx mask " : "sx unmask",
>> + irq & (1 << offset) ? "pending" : " ");
>> + }
>> +}
>
> Looks helpful!
>
>> +static int crystalcove_gpio_probe(struct platform_device *pdev)
>> +{
>> + int irq = platform_get_irq(pdev, 0);
>> + struct crystalcove_gpio *cg = &gpio_info;
>> + int retval;
>> + int i;
>> + struct device *dev = intel_soc_pmic_dev();
>> +
>> + mutex_init(&cg->buslock);
>> + cg->chip.label = "intel_crystalcove";
>> + cg->chip.direction_input = crystalcove_gpio_direction_input;
>> + cg->chip.direction_output = crystalcove_gpio_direction_output;
>> + cg->chip.get = crystalcove_gpio_get;
>> + cg->chip.set = crystalcove_gpio_set;
>> + cg->chip.to_irq = crystalcove_gpio_to_irq;
>
> You don't define to_irq when using GPIOLIB_IRQCHIP.
>
>> + cg->chip.base = -1;
>> + cg->chip.ngpio = NUM_GPIO;
>> + cg->chip.can_sleep = 1;
>
> true. Set it to true. This is a bool.
>
>> + cg->chip.dev = dev;
>> + cg->chip.dbg_show = crystalcove_gpio_dbg_show;
>> +
>> + retval = gpiochip_add(&cg->chip);
>> + if (retval) {
>> + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
>> + goto out;
>> + }
>
> Skip from here:
>
>> + cg->irq_base = irq_alloc_descs(-1, 0, NUM_GPIO, 0);
>> + if (cg->irq_base < 0) {
>> + retval = cg->irq_base;
>> + cg->irq_base = 0;
>> + dev_warn(&pdev->dev, "irq_alloc_descs error: %d\n", retval);
>> + goto out_remove_gpio;
>> + }
>> +
>> + for (i = 0; i < NUM_GPIO; i++) {
>> + irq_set_chip_data(i + cg->irq_base, cg);
>> + irq_set_chip_and_handler_name(i + cg->irq_base,
>> + &crystalcove_irqchip,
>> + handle_simple_irq, "demux");
>> + }
>
> To here, replace with:
>
> gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, cg->irq_base,
> handle_simple_irq, IRQ_TYPE_NONE);
>
>> + retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
>> + IRQF_ONESHOT, "crystalcove_gpio", cg);
>> +
>> + if (retval) {
>> + dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
>> + goto out_free_desc;
>> + }
>
> Maybe use
> gpiochip_set_chained_irqchip() here.
>
> Yours,
> Linus Walleij
>
Thank you. That's a long list and all of them indeed need to be fixed.
I'll work on them and submit v2 when ready.
Best Regards
Lejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-17 14:37 ` [PATCH] " Alexandre Courbot
@ 2014-05-19 0:28 ` Zhu, Lejun
2014-05-27 9:01 ` Linus Walleij
1 sibling, 0 replies; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-19 0:28 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Linus Walleij, Linux Kernel Mailing List,
linux-gpio@vger.kernel.org, bin.yang
On 5/17/2014 10:37 PM, Alexandre Courbot wrote:
> On Thu, May 15, 2014 at 12:44 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
>>
>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>> ---
>> drivers/gpio/Kconfig | 9 ++
>> drivers/gpio/Makefile | 1 +
>> drivers/gpio/gpio-crystalcove.c | 323 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 333 insertions(+)
>> create mode 100644 drivers/gpio/gpio-crystalcove.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index a86c49a..95bb5a0 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -440,6 +440,15 @@ config GPIO_ARIZONA
>> help
>> Support for GPIOs on Wolfson Arizona class devices.
>>
>> +config GPIO_INTEL_SOC_PMIC
>> + bool "GPIO on Intel SoC PMIC"
>> + depends on INTEL_SOC_PMIC
>> + help
>> + Support for GPIO pins on Intel SoC PMIC, such as Crystal
>> + Cove.
>> + Say Y if you have a tablet with Intel SoC (e.g. Baytrail)
>> + inside.
>> +
>> config GPIO_LP3943
>> tristate "TI/National Semiconductor LP3943 GPIO expander"
>> depends on MFD_LP3943
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 6309aff..5380608 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
>> obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
>> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
>> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
>> +obj-$(CONFIG_GPIO_INTEL_SOC_PMIC) += gpio-crystalcove.o
>> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
>> obj-$(CONFIG_GPIO_IT8761E) += gpio-it8761e.o
>> obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o
>> diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
>> new file mode 100644
>> index 0000000..974f9b8
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-crystalcove.c
>> @@ -0,0 +1,323 @@
>> +/*
>> + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
>> + *
>> + * Copyright (C) 2012, 2014 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.
>> + *
>> + * Author: Yang, Bin <bin.yang@intel.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/sched.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/gpio.h>
>> +
>> +#define NUM_GPIO 16
>> +
>> +#define UPDATE_TYPE (1 << 0)
>> +#define UPDATE_MASK (1 << 1)
>> +
>> +#define GPIO0IRQ 0x0b
>> +#define GPIO1IRQ 0x0c
>> +#define MGPIO0IRQS0 0x19
>> +#define MGPIO1IRQS0 0x1a
>> +#define MGPIO0IRQSX 0x1b
>> +#define MGPIO1IRQSX 0x1c
>> +#define GPIO0P0CTLO 0x2b
>> +#define GPIO0P0CTLI 0x33
>> +#define GPIO1P0CTLO 0x3b
>> +#define GPIO1P0CTLI 0x43
>> +
>> +#define CTLI_INTCNT_NE (1 << 1)
>> +#define CTLI_INTCNT_PE (2 << 1)
>> +#define CTLI_INTCNT_BE (3 << 1)
>> +
>> +#define CTLO_DIR_OUT (1 << 5)
>> +#define CTLO_DRV_CMOS (0 << 4)
>> +#define CTLO_DRV_OD (1 << 4)
>> +#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_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
>> +#define CTLO_OUTPUT_DEF (CTLO_DIR_OUT | CTLO_INPUT_DEF)
>> +
>> +struct crystalcove_gpio {
>> + struct mutex buslock; /* irq_bus_lock */
>> + struct gpio_chip chip;
>> + int irq;
>> + int irq_base;
>> + int update;
>> + int trigger_type;
>> + int irq_mask;
>> +};
>> +static struct crystalcove_gpio gpio_info;
>> +
>> +static void __crystalcove_irq_mask(int gpio, int mask)
>> +{
>> + u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
>> + int offset = gpio < 8 ? gpio : gpio - 8;
>> +
>> + if (mask)
>> + intel_soc_pmic_setb(mirqs0, 1 << offset);
>> + else
>> + intel_soc_pmic_clearb(mirqs0, 1 << offset);
>> +}
>> +
>> +static void __crystalcove_irq_type(int gpio, int type)
>> +{
>> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
>
> Not a big comment, but wouldn't it be safer to factorize this logic
> (which is repeated in many places) into some macro? e.g. something
> like:
>
> #define GPIOP0CTL(gpio, dir) ((gpio < 8 ? GPIO0P0CTL ## dir :
> GPIO1P0CTL ## dir) + (gpio & ~0x8))
>
> ...
>
> u8 ctli = GPIOP0CTL(gpio, I);
>
> Feel free to come with a better macro (or to ignore that comment
> altogether if you think it makes the code less readable), but I think
> it would be less error-prone if you did not have to copy-paste that
> code all over the place.
>
Thank you. I'll fix them in v2 as well.
Best Regards
Lejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-16 17:46 ` Daniel Glöckner
@ 2014-05-19 1:46 ` Zhu, Lejun
0 siblings, 0 replies; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-19 1:46 UTC (permalink / raw)
To: Daniel Glöckner, aaron.lu
Cc: linus.walleij, gnurou, linux-kernel, linux-gpio, bin.yang
On 5/17/2014 1:46 AM, Daniel Glöckner wrote:
> Hi Lejun,
>
> On Wed, May 14, 2014 at 11:44:07PM +0800, Zhu, Lejun wrote:
>> This patch adds support for the GPIO function in Crystal Cove.
>
> in our device ACPI makes use of "virtual" GPIOs that have numbers from
> 0x20 to 0x5E to change various bits in the PMIC. Do you know if this
> is officially supported by the INT33FD ACPI device or if it is a
> vendor hack?
>
> Daniel
>
Hi, sorry I'm quite familiar with ACPI. I know Aaron is looking at the
possibility to use these pins in Linux ACPI, but so far we don't have
any code.
Best Regards
Lejun
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-14 15:44 [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove) Zhu, Lejun
` (2 preceding siblings ...)
2014-05-17 14:37 ` [PATCH] " Alexandre Courbot
@ 2014-05-19 10:39 ` Mika Westerberg
2014-05-20 8:30 ` Mika Westerberg
3 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-05-19 10:39 UTC (permalink / raw)
To: Zhu, Lejun; +Cc: linus.walleij, gnurou, linux-kernel, linux-gpio, bin.yang
On Wed, May 14, 2014 at 11:44:07PM +0800, Zhu, Lejun wrote:
> Devices based on Intel SoC products such as Baytrail have a Power
> Management IC. In the PMIC there are subsystems for voltage regulation,
> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
> Crystal Cove.
>
> This patch adds support for the GPIO function in Crystal Cove.
I have few comments as well in addition to comments from Linus and
Alexandre.
Please see below.
> Signed-off-by: Yang, Bin <bin.yang@intel.com>
> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
> ---
> drivers/gpio/Kconfig | 9 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-crystalcove.c | 323 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 333 insertions(+)
> create mode 100644 drivers/gpio/gpio-crystalcove.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..95bb5a0 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -440,6 +440,15 @@ config GPIO_ARIZONA
> help
> Support for GPIOs on Wolfson Arizona class devices.
>
> +config GPIO_INTEL_SOC_PMIC
> + bool "GPIO on Intel SoC PMIC"
> + depends on INTEL_SOC_PMIC
> + help
> + Support for GPIO pins on Intel SoC PMIC, such as Crystal
> + Cove.
> + Say Y if you have a tablet with Intel SoC (e.g. Baytrail)
> + inside.
I wonder if the Kconfig name sould be GPIO_CRYSTALCOVE or similar so
that it is easier to map the Kconfig option to the actual driver name?
> +
> config GPIO_LP3943
> tristate "TI/National Semiconductor LP3943 GPIO expander"
> depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..5380608 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> +obj-$(CONFIG_GPIO_INTEL_SOC_PMIC) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> obj-$(CONFIG_GPIO_IT8761E) += gpio-it8761e.o
> obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o
> diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
> new file mode 100644
> index 0000000..974f9b8
> --- /dev/null
> +++ b/drivers/gpio/gpio-crystalcove.c
> @@ -0,0 +1,323 @@
> +/*
> + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
> + *
> + * Copyright (C) 2012, 2014 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.
> + *
> + * Author: Yang, Bin <bin.yang@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/sched.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/gpio.h>
> +
> +#define NUM_GPIO 16
> +
> +#define UPDATE_TYPE (1 << 0)
> +#define UPDATE_MASK (1 << 1)
> +
> +#define GPIO0IRQ 0x0b
> +#define GPIO1IRQ 0x0c
> +#define MGPIO0IRQS0 0x19
> +#define MGPIO1IRQS0 0x1a
> +#define MGPIO0IRQSX 0x1b
> +#define MGPIO1IRQSX 0x1c
> +#define GPIO0P0CTLO 0x2b
> +#define GPIO0P0CTLI 0x33
> +#define GPIO1P0CTLO 0x3b
> +#define GPIO1P0CTLI 0x43
> +
> +#define CTLI_INTCNT_NE (1 << 1)
> +#define CTLI_INTCNT_PE (2 << 1)
> +#define CTLI_INTCNT_BE (3 << 1)
> +
> +#define CTLO_DIR_OUT (1 << 5)
> +#define CTLO_DRV_CMOS (0 << 4)
> +#define CTLO_DRV_OD (1 << 4)
> +#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_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
> +#define CTLO_OUTPUT_DEF (CTLO_DIR_OUT | CTLO_INPUT_DEF)
> +
> +struct crystalcove_gpio {
> + struct mutex buslock; /* irq_bus_lock */
> + struct gpio_chip chip;
> + int irq;
> + int irq_base;
> + int update;
> + int trigger_type;
> + int irq_mask;
> +};
> +static struct crystalcove_gpio gpio_info;
Um, why you allocate private driver data like this? Typically you would
do it in driver ->probe() using devm_kzalloc() or similar.
> +
> +static void __crystalcove_irq_mask(int gpio, int mask)
> +{
> + u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
> + int offset = gpio < 8 ? gpio : gpio - 8;
> +
> + if (mask)
> + intel_soc_pmic_setb(mirqs0, 1 << offset);
> + else
> + intel_soc_pmic_clearb(mirqs0, 1 << offset);
> +}
> +
> +static void __crystalcove_irq_type(int gpio, int type)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
> +
> + type &= IRQ_TYPE_EDGE_BOTH;
> + intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
> +
> + if (type == IRQ_TYPE_EDGE_BOTH)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
> + else if (type & IRQ_TYPE_EDGE_FALLING)
> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
> +}
> +
> +static int crystalcove_gpio_direction_input(struct gpio_chip *chip,
> + unsigned gpio)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_INPUT_DEF);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_direction_output(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + intel_soc_pmic_writeb(ctlo, CTLO_OUTPUT_DEF | value);
> + return 0;
> +}
> +
> +static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
> +
> + return intel_soc_pmic_readb(ctli) & 0x1;
> +}
> +
> +static void crystalcove_gpio_set(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
> + u8 ctlo = gpio < 8 ? GPIO0P0CTLO + gpio : GPIO1P0CTLO + (gpio - 8);
> +
> + if (value)
> + intel_soc_pmic_setb(ctlo, 1);
> + else
> + intel_soc_pmic_clearb(ctlo, 1);
> +}
> +
> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->trigger_type = type;
> + cg->update |= UPDATE_TYPE;
> +
> + return 0;
> +}
> +
> +static int crystalcove_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> +
> + return cg->irq_base + gpio;
> +}
> +
> +static void crystalcove_bus_lock(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + mutex_lock(&cg->buslock);
> +}
> +
> +static void crystalcove_bus_sync_unlock(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> + int gpio = data->irq - cg->irq_base;
> +
> + if (cg->update & UPDATE_TYPE)
> + __crystalcove_irq_type(gpio, cg->trigger_type);
> + if (cg->update & UPDATE_MASK)
> + __crystalcove_irq_mask(gpio, cg->irq_mask);
> + cg->update = 0;
> +
> + mutex_unlock(&cg->buslock);
> +}
> +
> +static void crystalcove_irq_unmask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 0;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static void crystalcove_irq_mask(struct irq_data *data)
> +{
> + struct crystalcove_gpio *cg = irq_data_get_irq_chip_data(data);
> +
> + cg->irq_mask = 1;
> + cg->update |= UPDATE_MASK;
> +}
> +
> +static struct irq_chip crystalcove_irqchip = {
> + .name = "PMIC-GPIO",
> + .irq_mask = crystalcove_irq_mask,
> + .irq_unmask = crystalcove_irq_unmask,
> + .irq_set_type = crystalcove_irq_type,
> + .irq_bus_lock = crystalcove_bus_lock,
> + .irq_bus_sync_unlock = crystalcove_bus_sync_unlock,
> +};
> +
> +static irqreturn_t crystalcove_gpio_irq_handler(int irq, void *data)
> +{
> + struct crystalcove_gpio *cg = data;
> + int pending;
> + int gpio;
> +
> + pending = intel_soc_pmic_readb(GPIO0IRQ) & 0xff;
If it is readb() do you still need to apply the 0xff mask here? I would
expect it to return u8.
> + pending |= (intel_soc_pmic_readb(GPIO1IRQ) & 0xff) << 8;
> + intel_soc_pmic_writeb(GPIO0IRQ, pending & 0xff);
> + intel_soc_pmic_writeb(GPIO1IRQ, (pending >> 8) & 0xff);
> +
> + local_irq_disable();
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++)
> + if (pending & (1 << gpio))
> + generic_handle_irq(cg->irq_base + gpio);
> + local_irq_enable();
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void crystalcove_gpio_dbg_show(struct seq_file *s,
> + struct gpio_chip *chip)
> +{
> + struct crystalcove_gpio *cg =
> + container_of(chip, struct crystalcove_gpio, chip);
> + int gpio, offset;
> + u8 ctlo, ctli, mirqs0, mirqsx, irq;
> +
> + for (gpio = 0; gpio < cg->chip.ngpio; gpio++) {
> + offset = gpio < 8 ? gpio : gpio - 8;
> + ctlo = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLO : GPIO1P0CTLO) + offset);
> + ctli = intel_soc_pmic_readb(
> + (gpio < 8 ? GPIO0P0CTLI : GPIO1P0CTLI) + offset);
> + mirqs0 = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0);
> + mirqsx = intel_soc_pmic_readb(
> + gpio < 8 ? MGPIO0IRQSX : MGPIO1IRQSX);
> + irq = intel_soc_pmic_readb(
> + gpio < 8 ? GPIO0IRQ : GPIO1IRQ);
> + seq_printf(s, " gpio-%-2d %s %s %s %s ctlo=%2x,%s %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,
> + mirqs0 & (1 << offset) ? "s0 mask " : "s0 unmask",
> + mirqsx & (1 << offset) ? "sx mask " : "sx unmask",
> + irq & (1 << offset) ? "pending" : " ");
> + }
> +}
> +
> +static int crystalcove_gpio_probe(struct platform_device *pdev)
> +{
> + int irq = platform_get_irq(pdev, 0);
> + struct crystalcove_gpio *cg = &gpio_info;
> + int retval;
> + int i;
> + struct device *dev = intel_soc_pmic_dev();
What does this do? Is it the parent device? Then I suppose you should
use pdev->dev.parent or similar here.
For example in MSIC driver we have pdev_to_intel_msic(pdev) that
returned the parent MFD device to the MFD cell driver.
> +
> + mutex_init(&cg->buslock);
> + cg->chip.label = "intel_crystalcove";
Yet another name for the same thing.
> + cg->chip.direction_input = crystalcove_gpio_direction_input;
> + cg->chip.direction_output = crystalcove_gpio_direction_output;
> + cg->chip.get = crystalcove_gpio_get;
> + cg->chip.set = crystalcove_gpio_set;
> + cg->chip.to_irq = crystalcove_gpio_to_irq;
> + cg->chip.base = -1;
> + cg->chip.ngpio = NUM_GPIO;
> + cg->chip.can_sleep = 1;
> + cg->chip.dev = dev;
> + cg->chip.dbg_show = crystalcove_gpio_dbg_show;
> +
> + retval = gpiochip_add(&cg->chip);
> + if (retval) {
> + dev_warn(&pdev->dev, "add gpio chip error: %d\n", retval);
> + goto out;
> + }
> +
> + cg->irq_base = irq_alloc_descs(-1, 0, NUM_GPIO, 0);
> + if (cg->irq_base < 0) {
> + retval = cg->irq_base;
> + cg->irq_base = 0;
> + dev_warn(&pdev->dev, "irq_alloc_descs error: %d\n", retval);
> + goto out_remove_gpio;
> + }
> +
> + for (i = 0; i < NUM_GPIO; i++) {
> + irq_set_chip_data(i + cg->irq_base, cg);
> + irq_set_chip_and_handler_name(i + cg->irq_base,
> + &crystalcove_irqchip,
> + handle_simple_irq, "demux");
> + }
> +
> + retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
> + IRQF_ONESHOT, "crystalcove_gpio", cg);
> +
> + if (retval) {
> + dev_warn(&pdev->dev, "request irq failed: %d\n", retval);
> + goto out_free_desc;
> + }
> +
> + return 0;
> +
> +out_free_desc:
> + irq_free_descs(cg->irq_base, NUM_GPIO);
> +out_remove_gpio:
> + gpiochip_remove(&cg->chip);
> +out:
> + return retval;
> +}
> +
> +static struct platform_driver crystalcove_gpio_driver = {
> + .probe = crystalcove_gpio_probe,
> + .driver = {
> + .name = "crystal_cove_gpio",
> + },
> +};
Is there anything preventing building this driver as a module?
> +
> +module_platform_driver(crystalcove_gpio_driver);
> +
> +MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
> +MODULE_DESCRIPTION("Intel Crystal Cove GPIO Driver");
> +MODULE_LICENSE("GPL");
This should be "GPL v2" since that's what the license on the top says.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-19 0:27 ` Zhu, Lejun
@ 2014-05-19 14:13 ` Mathias Nyman
2014-05-20 9:16 ` Zhu, Lejun
0 siblings, 1 reply; 13+ messages in thread
From: Mathias Nyman @ 2014-05-19 14:13 UTC (permalink / raw)
To: Zhu, Lejun, Linus Walleij
Cc: Alexandre Courbot, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, bin.yang, Darren Hart, Holmberg, Hans,
Mika Westerberg
On 05/19/2014 03:27 AM, Zhu, Lejun wrote:
>
>
> On 5/17/2014 1:33 AM, Linus Walleij wrote:
>> On Wed, May 14, 2014 at 5:44 PM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>>
>>> Devices based on Intel SoC products such as Baytrail have a Power
>>> Management IC. In the PMIC there are subsystems for voltage regulation,
>>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>>> Crystal Cove.
>>>
>>> This patch adds support for the GPIO function in Crystal Cove.
>>>
>>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>>
>> (...)
>>
>>> +config GPIO_INTEL_SOC_PMIC
>>> + bool "GPIO on Intel SoC PMIC"
>>> + depends on INTEL_SOC_PMIC
>
> Thank you. That's a long list and all of them indeed need to be fixed.
> I'll work on them and submit v2 when ready.
>
Shouldn't there be a .remove function undoing everything probe did?
Freeing interrupts, removing irq domains, calling gpiochip_remove etc.
Or is there something I'm missing?
I see there's no option to compile this as module, but It might be added later so
proper remove function would still be nice.
-Mathias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-19 10:39 ` Mika Westerberg
@ 2014-05-20 8:30 ` Mika Westerberg
2014-05-20 9:15 ` Zhu, Lejun
0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2014-05-20 8:30 UTC (permalink / raw)
To: Zhu, Lejun; +Cc: linus.walleij, gnurou, linux-kernel, linux-gpio, bin.yang
On Mon, May 19, 2014 at 01:39:33PM +0300, Mika Westerberg wrote:
> On Wed, May 14, 2014 at 11:44:07PM +0800, Zhu, Lejun wrote:
> > Devices based on Intel SoC products such as Baytrail have a Power
> > Management IC. In the PMIC there are subsystems for voltage regulation,
> > A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
> > Crystal Cove.
> >
> > This patch adds support for the GPIO function in Crystal Cove.
>
> I have few comments as well in addition to comments from Linus and
> Alexandre.
One more thing, I just remembered. The crystal cove GPIO driver is
supposed to provide ACPI Operation Regions to the ASL code so you need
to make sure you have ACPI handle associated with the device before you
register your driver to the GPIO core.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-20 8:30 ` Mika Westerberg
@ 2014-05-20 9:15 ` Zhu, Lejun
0 siblings, 0 replies; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-20 9:15 UTC (permalink / raw)
To: Mika Westerberg; +Cc: linus.walleij, gnurou, linux-kernel, linux-gpio, bin.yang
On 5/20/2014 4:30 PM, Mika Westerberg wrote:
> On Mon, May 19, 2014 at 01:39:33PM +0300, Mika Westerberg wrote:
>> On Wed, May 14, 2014 at 11:44:07PM +0800, Zhu, Lejun wrote:
>>> Devices based on Intel SoC products such as Baytrail have a Power
>>> Management IC. In the PMIC there are subsystems for voltage regulation,
>>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>>> Crystal Cove.
>>>
>>> This patch adds support for the GPIO function in Crystal Cove.
>>
>> I have few comments as well in addition to comments from Linus and
>> Alexandre.
>
> One more thing, I just remembered. The crystal cove GPIO driver is
> supposed to provide ACPI Operation Regions to the ASL code so you need
> to make sure you have ACPI handle associated with the device before you
> register your driver to the GPIO core.
>
ACPI OpRegions is still under work, and will be submitted separately.
I'll fix the rest in v2. Thank you.
Best Regards
Lejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-19 14:13 ` Mathias Nyman
@ 2014-05-20 9:16 ` Zhu, Lejun
0 siblings, 0 replies; 13+ messages in thread
From: Zhu, Lejun @ 2014-05-20 9:16 UTC (permalink / raw)
To: Mathias Nyman, Linus Walleij
Cc: Alexandre Courbot, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, bin.yang, Darren Hart, Holmberg, Hans,
Mika Westerberg
On 5/19/2014 10:13 PM, Mathias Nyman wrote:
> On 05/19/2014 03:27 AM, Zhu, Lejun wrote:
>>
>>
>> On 5/17/2014 1:33 AM, Linus Walleij wrote:
>>> On Wed, May 14, 2014 at 5:44 PM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>>>
>>>> Devices based on Intel SoC products such as Baytrail have a Power
>>>> Management IC. In the PMIC there are subsystems for voltage regulation,
>>>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>>>> Crystal Cove.
>>>>
>>>> This patch adds support for the GPIO function in Crystal Cove.
>>>>
>>>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>>>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>>>
>>> (...)
>>>
>>>> +config GPIO_INTEL_SOC_PMIC
>>>> + bool "GPIO on Intel SoC PMIC"
>>>> + depends on INTEL_SOC_PMIC
>>
>> Thank you. That's a long list and all of them indeed need to be fixed.
>> I'll work on them and submit v2 when ready.
>>
>
> Shouldn't there be a .remove function undoing everything probe did?
> Freeing interrupts, removing irq domains, calling gpiochip_remove etc.
>
> Or is there something I'm missing?
> I see there's no option to compile this as module, but It might be added later so
> proper remove function would still be nice.
>
> -Mathias
>
Good suggestion. I'll fix it. Thanks.
Best Regards
Lejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)
2014-05-17 14:37 ` [PATCH] " Alexandre Courbot
2014-05-19 0:28 ` Zhu, Lejun
@ 2014-05-27 9:01 ` Linus Walleij
1 sibling, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2014-05-27 9:01 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Zhu, Lejun, Linux Kernel Mailing List, linux-gpio@vger.kernel.org,
bin.yang
On Sat, May 17, 2014 at 4:37 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, May 15, 2014 at 12:44 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>> +static void __crystalcove_irq_type(int gpio, int type)
>> +{
>> + u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
>
> Not a big comment, but wouldn't it be safer to factorize this logic
> (which is repeated in many places) into some macro? e.g. something
> like:
>
> #define GPIOP0CTL(gpio, dir) ((gpio < 8 ? GPIO0P0CTL ## dir :
> GPIO1P0CTL ## dir) + (gpio & ~0x8))
static functions are preferred over macros, please :-)
If you want to be sure they are inlined, just mark them inline (but the
compiler will do it's job for sure).
Just split the function instead so it is more readable:
static void __crystalcove_irq_type(int gpio, int type)
{
u8 ctli;
if (gpio < 8)
ctli = GPIO0P0CTLI + gpio;
else
ctli = GPIO1P0CTLI + (gpio - 8);
But I think this can be simplified with the % operator as stated
elsewhere.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-05-27 9:01 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-14 15:44 [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove) Zhu, Lejun
2014-05-16 17:33 ` Linus Walleij
2014-05-19 0:27 ` Zhu, Lejun
2014-05-19 14:13 ` Mathias Nyman
2014-05-20 9:16 ` Zhu, Lejun
2014-05-16 17:46 ` Daniel Glöckner
2014-05-19 1:46 ` Zhu, Lejun
2014-05-17 14:37 ` [PATCH] " Alexandre Courbot
2014-05-19 0:28 ` Zhu, Lejun
2014-05-27 9:01 ` Linus Walleij
2014-05-19 10:39 ` Mika Westerberg
2014-05-20 8:30 ` Mika Westerberg
2014-05-20 9:15 ` Zhu, Lejun
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).