This is the GPIO component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the core DA9058 MFD driver.
Signed-off-by: Anthony Olech <[email protected]>
Signed-off-by: David Dajun Chen <[email protected]>
---
drivers/gpio/Kconfig | 12 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-da9058.c | 376 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 389 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/gpio-da9058.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 542f0c0..63b574a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -86,6 +86,18 @@ config GPIO_DA9052
help
Say yes here to enable the GPIO driver for the DA9052 chip.
+config GPIO_DA9058
+ tristate "Dialog DA9058 GPIO"
+ depends on MFD_DA9058
+ help
+ Say yes here to enable the GPIO driver for the DA9058 chip.
+
+ The Dialog DA9058 PMIC chip has 2 GPIO pins that can be
+ be controller by this driver.
+
+ If driver is built as a module it will be called da9058-gpio.
+
+
config GPIO_MAX730X
tristate
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0f55662..209224a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
+obj-$(CONFIG_GPIO_DA9058) += gpio-da9058.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
obj-$(CONFIG_GPIO_EM) += gpio-em.o
diff --git a/drivers/gpio/gpio-da9058.c b/drivers/gpio/gpio-da9058.c
new file mode 100644
index 0000000..ed464ff
--- /dev/null
+++ b/drivers/gpio/gpio-da9058.c
@@ -0,0 +1,376 @@
+/*
+ * Copyright (C) 2012 Dialog Semiconductor Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/syscalls.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/da9058/version.h>
+#include <linux/mfd/da9058/registers.h>
+#include <linux/mfd/da9058/core.h>
+#include <linux/mfd/da9058/gpio.h>
+#include <linux/mfd/da9058/irq.h>
+#include <linux/mfd/da9058/pdata.h>
+
+/*
+ * There are only 2 available GPIO pins on the DA9058 PMIC
+ *
+ * Thus this driver distinguishes them by the offset number
+ * being zero or non-zero for simplicity
+ */
+
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ u8 inp_config;
+ u8 out_config;
+};
+
+static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip *chip)
+{
+ return container_of(chip, struct da9058_gpio, gp);
+}
+
+static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+ unsigned int gpio_level;
+ int ret;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+ ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, &gpio_level);
+ mutex_unlock(&gpio->lock);
+ if (ret < 0)
+ return ret;
+
+ if (offset) {
+ if (gpio_level & DA9058_STATUSC_GPI1)
+ return 1;
+ else
+ return 0;
+ } else {
+ if (gpio_level & DA9058_STATUSC_GPI0)
+ return 1;
+ else
+ return 0;
+ }
+}
+
+static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+ unsigned int gpio_cntrl;
+ int ret;
+
+ if (offset > 1) {
+ dev_err(da9058->dev,
+ "Failed to set GPIO%d output=%d because illegal GPIO\n",
+ offset, value);
+ return;
+ }
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
+ u8 value_bits = value ? 0x80 : 0x00;
+
+ gpio->out_config &= ~0x80;
+ gpio->out_config |= value_bits;
+
+ if (!(gpio_cntrl & 0x20))
+ goto exit;
+
+ gpio_cntrl &= ~0xF0;
+ gpio_cntrl |= 0xF0 & gpio->out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ } else {
+ u8 value_bits = value ? 0x08 : 0x00;
+
+ gpio->out_config &= ~0x08;
+ gpio->out_config |= value_bits;
+
+ if (!(gpio_cntrl & 0x02))
+ goto exit;
+
+ gpio_cntrl &= ~0x0F;
+ gpio_cntrl |= 0x0F & gpio->out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ }
+exit:
+ mutex_unlock(&gpio->lock);
+ if (ret)
+ dev_err(da9058->dev,
+ "Failed to set GPIO%d output=%d error=%d\n",
+ offset, value, ret);
+ return;
+}
+
+static int da9058_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+ unsigned int gpio_cntrl;
+ int ret;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
+ gpio_cntrl &= ~0xF0;
+ gpio_cntrl |= 0xF0 & gpio->inp_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ } else {
+ gpio_cntrl &= ~0x0F;
+ gpio_cntrl |= 0x0F & gpio->inp_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ }
+exit:
+ mutex_unlock(&gpio->lock);
+ if (ret)
+ dev_err(da9058->dev,
+ "Failed to set GPIO%d to output error=%d\n",
+ offset, ret);
+ return ret;
+}
+
+static int da9058_gpio_direction_output(struct gpio_chip *gc,
+ unsigned offset, int value)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+ unsigned int gpio_cntrl;
+ int ret;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
+ gpio_cntrl &= ~0xF0;
+ gpio_cntrl |= 0xF0 & gpio->out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ } else {
+ gpio_cntrl &= ~0x0F;
+ gpio_cntrl |= 0x0F & gpio->out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ }
+exit:
+ mutex_unlock(&gpio->lock);
+ if (ret)
+ dev_err(da9058->dev,
+ "Failed to set GPIO%d to input error=%d\n",
+ offset, ret);
+ return ret;
+}
+/*
+ * da9058_gpio_to_irq is an implementation of the GPIO Hook
+ * @to_irq: supporting non-static gpio_to_irq() mappings
+ * whose implementation may not sleep. This hook is called
+ * when setting up the threaded GPIO irq handler.
+ */
+static int da9058_gpio_to_irq(struct gpio_chip *gc, u32 offset)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ if (offset)
+ return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1);
+ else
+ return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0);
+}
+
+static int da9058_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
+ unsigned debounce)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio->da9058;
+ int ret;
+ unsigned int gpio_cntrl;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
+ u8 debounce_bits = debounce ? 0x80 : 0x00;
+
+ gpio->inp_config &= ~0x80;
+ gpio->inp_config |= debounce_bits;
+
+ if (gpio_cntrl & 0x20)
+ goto exit;
+
+ gpio_cntrl &= ~0xF0;
+ gpio_cntrl |= 0xF0 & gpio->inp_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ } else {
+ u8 debounce_bits = debounce ? 0x08 : 0x00;
+
+ gpio->inp_config &= ~0x08;
+ gpio->inp_config |= debounce_bits;
+
+ if (gpio_cntrl & 0x02)
+ goto exit;
+
+ gpio_cntrl &= ~0x0F;
+ gpio_cntrl |= 0x0F & gpio->inp_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ }
+exit:
+ mutex_unlock(&gpio->lock);
+ if (ret)
+ dev_err(da9058->dev,
+ "Failed to set GPIO%d bounce=%d error=%d\n",
+ offset, debounce, ret);
+ return ret;
+}
+
+static int __devinit da9058_gpio_probe(struct platform_device *pdev)
+{
+ struct da9058 *da9058 = dev_get_drvdata(pdev->dev.parent);
+ const struct mfd_cell *cell = mfd_get_cell(pdev);
+ struct da9058_gpio_pdata *gpio_pdata;
+ struct da9058_gpio *gpio;
+ int ret;
+
+ if (cell == NULL) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ gpio_pdata = cell->platform_data;
+
+ if (!gpio_pdata) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(struct da9058_gpio), GFP_KERNEL);
+ if (!gpio) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+ gpio->da9058 = da9058;
+ gpio->pdev = pdev;
+
+ gpio->gp.label = "da9058-gpio";
+ gpio->gp.owner = THIS_MODULE;
+ gpio->gp.get = da9058_gpio_get;
+ gpio->gp.set = da9058_gpio_set;
+ gpio->gp.direction_input = da9058_gpio_direction_input;
+ gpio->gp.direction_output = da9058_gpio_direction_output;
+ gpio->gp.to_irq = da9058_gpio_to_irq;
+ gpio->gp.set_debounce = da9058_gpio_set_debounce;
+ gpio->gp.can_sleep = 1;
+ gpio->gp.ngpio = gpio_pdata->ngpio;
+ gpio->gp.base = gpio_pdata->gpio_base;
+
+ gpio->inp_config = 0x99;
+ gpio->out_config = 0x66;
+
+ mutex_init(&gpio->lock);
+
+ ret = gpiochip_add(&gpio->gp);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+ goto err;
+ }
+
+ platform_set_drvdata(pdev, gpio);
+
+ dev_info(&pdev->dev, "%d GPIO pins at base %d\n", gpio_pdata->ngpio,
+ gpio->gp.base);
+
+ mutex_lock(&gpio->lock);
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio->inp_config);
+ mutex_unlock(&gpio->lock);
+ if (ret)
+ dev_err(da9058->dev,
+ "Failed to set GPIO0 as an output error = %d\n",
+ ret);
+err:
+exit:
+ return ret;
+}
+
+static int __devexit da9058_gpio_remove(struct platform_device *pdev)
+{
+ struct da9058_gpio *gpio = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = gpiochip_remove(&gpio->gp);
+ if (ret == 0)
+ kfree(gpio);
+
+ return ret;
+}
+
+static struct platform_driver da9058_gpio_driver = {
+ .probe = da9058_gpio_probe,
+ .remove = __devexit_p(da9058_gpio_remove),
+ .driver = {
+ .name = "da9058-gpio",
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(da9058_gpio_driver);
+
+MODULE_DESCRIPTION("Dialog DA9058 PMIC General Purpose IO Driver");
+MODULE_AUTHOR("Anthony Olech <[email protected]>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:da9058-gpio");
--
end-of-patch for NEW DRIVER V2
Hi Anthony, sorry for delayed reply...
On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
<[email protected]> wrote:
> This is the GPIO component driver of the Dialog DA9058 PMIC.
> This driver is just one component of the whole DA9058 PMIC driver.
> It depends on the core DA9058 MFD driver.
OK
> +config GPIO_DA9058
> + tristate "Dialog DA9058 GPIO"
> + depends on MFD_DA9058
select IRQ_DOMAIN, you're going to want to use it...
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0f55662..209224a 100644
(...)
> +#include <linux/module.h>
> +#include <linux/fs.h>
Really?
> +#include <linux/uaccess.h>
Really?
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/syscalls.h>
Really?
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/regmap.h>
If you're using regmap you better select it in Kconfig
too, but it appears you don't. You should be using regmap in the
main MFD driver in this case (I haven't looked at it though.)
This header set just looks like it was copied from some other
file and never really proofread, so please go over it in detail.
> +#include <linux/mfd/da9058/version.h>
> +#include <linux/mfd/da9058/registers.h>
> +#include <linux/mfd/da9058/core.h>
> +#include <linux/mfd/da9058/gpio.h>
> +#include <linux/mfd/da9058/irq.h>
> +#include <linux/mfd/da9058/pdata.h>
Samuel will have to comment on this organization of headers, it seems a
little much. DO you really need all of them?
> +struct da9058_gpio {
> + struct da9058 *da9058;
> + struct platform_device *pdev;
> + struct gpio_chip gp;
> + struct mutex lock;
> + u8 inp_config;
> + u8 out_config;
> +};
> +
> +static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct da9058_gpio, gp);
> +}
static inline, or a #define, but the compile will probably optimize-inline
it anyway.
> +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_level;
> + int ret;
> +
> + if (offset > 1)
> + return -EINVAL;
So there are two GPIO pins, 0 and 1? That seems odd, but OK.
> +
> + mutex_lock(&gpio->lock);
> + ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, &gpio_level);
> + mutex_unlock(&gpio->lock);
> + if (ret < 0)
> + return ret;
> +
> + if (offset) {
> + if (gpio_level & DA9058_STATUSC_GPI1)
> + return 1;
> + else
> + return 0;
> + } else {
> + if (gpio_level & DA9058_STATUSC_GPI0)
> + return 1;
> + else
> + return 0;
> + }
> +}
> +
> +static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_cntrl;
> + int ret;
> +
> + if (offset > 1) {
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d output=%d because illegal GPIO\n",
> + offset, value);
> + return;
> + }
> +
> + mutex_lock(&gpio->lock);
> +
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
So this is for GPIO 1
> + u8 value_bits = value ? 0x80 : 0x00;
These "value_bits" are just confusing. Just delete this and use the
direct value below.
> +
> + gpio->out_config &= ~0x80;
A better way of writing &= ~0x80; is &= 0x7F
> + gpio->out_config |= value_bits;
gpio->out_config = value ? 0x80 : 0x00;
So, less confusing.
> + if (!(gpio_cntrl & 0x20))
> + goto exit;
Please insert a comment explaining what this bit is doing and
why you're just exiting if it's not set. I don't understand one thing.
Maybe this would be better if you didn't use so many magic values,
what about:
#include <linux/bitops.h>
#define FOO_FLAG BIT(3) /* This is a flag for foo */
> +
> + gpio_cntrl &= ~0xF0;
A better way to write &= ~F0 is to write &= 0x0F;
If you don't #define the constants this way of negating numbers
just get confusing.
So this is OK:
foo &= ~FOO_FLAG;
foo |= set ? FOO_FLAG : 0;
This is just hard to read:
foo &= ~0x55;
foo |= set ? 0x55 : 0;
And is better off
foo &= 0xAA;
foo |= set ? 0x55 : 0;
I prefer that you #define the registers but it's your pick.
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> + u8 value_bits = value ? 0x08 : 0x00;
Same here, delete this.
> + gpio->out_config &= ~0x08;
&= 0xF7;
or use some <linux/bitops.h> and #defines ...
> + gpio->out_config |= value_bits;
Just
gpio->out_config |= value ? 0x08 : 0x00;
> + if (!(gpio_cntrl & 0x02))
> + goto exit;
Same thing, explain that flag.
> +
> + gpio_cntrl &= ~0x0F;
Just &= 0xF0;
> + gpio_cntrl |= 0x0F & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
Further, if you're checking that flag just in order to avoid doing this
write if it's not necessary, it's the wrong solution. The right solution
is to implement regmap in the MFD driver so it quickly sees that
the right value is already in the register and bounces off.
> + }
> +exit:
> + mutex_unlock(&gpio->lock);
> + if (ret)
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d output=%d error=%d\n",
> + offset, value, ret);
> + return;
> +}
> +
> +static int da9058_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_cntrl;
> + int ret;
> +
> + if (offset > 1)
> + return -EINVAL;
> +
> + mutex_lock(&gpio->lock);
> +
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
> + gpio_cntrl &= ~0xF0;
&= 0x0F; or #define the flag... FOO_MASK.
> + gpio_cntrl |= 0xF0 & gpio->inp_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> + gpio_cntrl &= ~0x0F;
&= 0xF0;
> + gpio_cntrl |= 0x0F & gpio->inp_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + }
> +exit:
> + mutex_unlock(&gpio->lock);
> + if (ret)
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d to output error=%d\n",
> + offset, ret);
> + return ret;
> +}
> +
> +static int da9058_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_cntrl;
> + int ret;
> +
> + if (offset > 1)
> + return -EINVAL;
> +
> + mutex_lock(&gpio->lock);
> +
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
> + gpio_cntrl &= ~0xF0;
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> + gpio_cntrl &= ~0x0F;
> + gpio_cntrl |= 0x0F & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + }
> +exit:
> + mutex_unlock(&gpio->lock);
> + if (ret)
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d to input error=%d\n",
> + offset, ret);
> + return ret;
> +}
Same comments as above, and you need a blank line here.
> +/*
> + * da9058_gpio_to_irq is an implementation of the GPIO Hook
> + * @to_irq: supporting non-static gpio_to_irq() mappings
> + * whose implementation may not sleep. This hook is called
> + * when setting up the threaded GPIO irq handler.
> + */
> +static int da9058_gpio_to_irq(struct gpio_chip *gc, u32 offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> +
> + if (offset > 1)
> + return -EINVAL;
> +
> + if (offset)
> + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1);
> + else
> + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0);
> +}
Lee Jones and Mark Brown discussed these virtual IRQ mapping functions
recently, and I think the outcome was to patch irqdomain to do the work
and not sprinkle these custom interfaces to fetch virtual IRQs all over the
place.
> +static int da9058_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
> + unsigned debounce)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + int ret;
> + unsigned int gpio_cntrl;
> +
> + if (offset > 1)
> + return -EINVAL;
> +
> + mutex_lock(&gpio->lock);
> +
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
> + u8 debounce_bits = debounce ? 0x80 : 0x00;
> +
> + gpio->inp_config &= ~0x80;
> + gpio->inp_config |= debounce_bits;
> +
> + if (gpio_cntrl & 0x20)
> + goto exit;
> +
> + gpio_cntrl &= ~0xF0;
> + gpio_cntrl |= 0xF0 & gpio->inp_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> + u8 debounce_bits = debounce ? 0x08 : 0x00;
> +
> + gpio->inp_config &= ~0x08;
> + gpio->inp_config |= debounce_bits;
> +
> + if (gpio_cntrl & 0x02)
> + goto exit;
> +
> + gpio_cntrl &= ~0x0F;
> + gpio_cntrl |= 0x0F & gpio->inp_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + }
> +exit:
> + mutex_unlock(&gpio->lock);
> + if (ret)
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d bounce=%d error=%d\n",
> + offset, debounce, ret);
> + return ret;
> + }
Apart from the same comments as above, debounce is actually pin control
territory and not part of <linux/gpio.h>. Now this is a very small interface,
but I still want you to consider using pin control for these portions.
The good thing about doing it with pin control is that you can have it
set up on boot using hogs and need to think anymor about the pin
config business.
The rest looks nice!
Yours,
Linus Walleij
On Mon, Aug 13, 2012 at 03:09:31PM +0200, Linus Walleij wrote:
> On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
> > +#include <linux/regmap.h>
> If you're using regmap you better select it in Kconfig
> too, but it appears you don't. You should be using regmap in the
> main MFD driver in this case (I haven't looked at it though.)
For MFDs the MFD core should already be ensuring that regmap is
selected.
> > + gpio_cntrl |= 0x0F & gpio->out_config;
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> Further, if you're checking that flag just in order to avoid doing this
> write if it's not necessary, it's the wrong solution. The right solution
> is to implement regmap in the MFD driver so it quickly sees that
> the right value is already in the register and bounces off.
Just using regmap_update_bits() will do the right thing.
> > + if (offset)
> > + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1);
> > + else
> > + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0);
> > +}
> Lee Jones and Mark Brown discussed these virtual IRQ mapping functions
> recently, and I think the outcome was to patch irqdomain to do the work
> and not sprinkle these custom interfaces to fetch virtual IRQs all over the
> place.
The easiest thing to do would be to pick the VIRQ numbers so that you
can just do
da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0 + offset);
> -----Original Message-----
> From: Linus Walleij [mailto:[email protected]]
> Sent: 13 August 2012 14:10
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Samuel
> Ortiz; Lee Jones
> Subject: Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
> Hi Anthony, sorry for delayed reply...
> On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
> <[email protected]> wrote:
> > This is the GPIO component driver of the Dialog DA9058 PMIC.
> > This driver is just one component of the whole DA9058 PMIC driver.
> > It depends on the core DA9058 MFD driver.
> OK
> > +config GPIO_DA9058
> > + tristate "Dialog DA9058 GPIO"
> > + depends on MFD_DA9058
> select IRQ_DOMAIN, you're going to want to use it...
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
> > 0f55662..209224a 100644
> (...)
> > +#include <linux/module.h>
> > +#include <linux/fs.h>
> Really?
> > +#include <linux/uaccess.h>
> Really?
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio.h>
> > +#include <linux/syscalls.h>
> Really?
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/regmap.h>
> If you're using regmap you better select it in Kconfig too, but it appears you
> don't. You should be using regmap in the main MFD driver in this case (I
> haven't looked at it though.)
> This header set just looks like it was copied from some other file and never
> really proofread, so please go over it in detail.
for some reason against 2.6.2x some of those were required, I just totally
forgot to prune out the unwanted ones when rebasing forwards to 3.5.
Good that you spotted it! Sorry I will try to prune the includes in the future.
> > +#include <linux/mfd/da9058/version.h> #include
> > +<linux/mfd/da9058/registers.h> #include <linux/mfd/da9058/core.h>
> > +#include <linux/mfd/da9058/gpio.h> #include <linux/mfd/da9058/irq.h>
> > +#include <linux/mfd/da9058/pdata.h>
> Samuel will have to comment on this organization of headers, it seems a little
> much. DO you really need all of them?
One of them should have been stripped out by my submit script, but as for the
others you must bear in mind that the DA9058 PMIC is a multifunction device,
and thus some header files are common and some are specific to various
component drivers. The very reason that you picked up on non-relevant include
files surely has implications on the structure of the header files for the DA9058,
in particular struct's and define's that only apply to one component driver should
be in separate header files.
> > +struct da9058_gpio {
> > + struct da9058 *da9058;
> > + struct platform_device *pdev;
> > + struct gpio_chip gp;
> > + struct mutex lock;
> > + u8 inp_config;
> > + u8 out_config;
> > +};
> > +
> > +static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip
> > +*chip) {
> > + return container_of(chip, struct da9058_gpio, gp); }
> static inline, or a #define, but the compile will probably optimize-inline it
> anyway.
The compiler should optimize it to in-line, but I will change it anyway.
> > +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + unsigned int gpio_level;
> > + int ret;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
> So there are two GPIO pins, 0 and 1? That seems odd, but OK.
That is a feature of the hardware. I believe that calling them "0" and
"1" is the correct thing to do. Correct me if they should have been
called "1" and "2", or something else.
> > + if (offset) {
> So this is for GPIO 1
Yes, it seemed the obvious thing to do.
> > + u8 value_bits = value ? 0x80 : 0x00;
>
> These "value_bits" are just confusing. Just delete this and use the direct value
> below.
Will do. It was done for diagnostics that have since been stripped out.
> > +
> > + gpio->out_config &= ~0x80;
> A better way of writing &= ~0x80; is &= 0x7F
> > + gpio->out_config |= value_bits;
> gpio->out_config = value ? 0x80 : 0x00;
> So, less confusing.
see HANDLING NIBBLES below
> > + if (!(gpio_cntrl & 0x20))
> > + goto exit;
> Please insert a comment explaining what this bit is doing and why you're just
> exiting if it's not set. I don't understand one thing.
I have explained why in the driver source in the next submission attempt
> Maybe this would be better if you didn't use so many magic values, what about:
> #include <linux/bitops.h>
> #define FOO_FLAG BIT(3) /* This is a flag for foo */
> > +
> > + gpio_cntrl &= ~0xF0;
> A better way to write &= ~F0 is to write &= 0x0F;
> If you don't #define the constants this way of negating numbers just get
> confusing.
> So this is OK:
> foo &= ~FOO_FLAG;
> foo |= set ? FOO_FLAG : 0;
> This is just hard to read:
> foo &= ~0x55;
> foo |= set ? 0x55 : 0;
> And is better off
> foo &= 0xAA;
> foo |= set ? 0x55 : 0;
> I prefer that you #define the registers but it's your pick.
> > + gpio_cntrl |= 0xF0 & gpio->out_config;
> > +
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> gpio_cntrl);
> > + } else {
> > + u8 value_bits = value ? 0x08 : 0x00;
> Same here, delete this.
> > + gpio->out_config &= ~0x08;
> &= 0xF7;
> or use some <linux/bitops.h> and #defines ...
see HANDLING NIBBLES below
> > + gpio->out_config |= value_bits;
> Just
> gpio->out_config |= value ? 0x08 : 0x00;
done in next submission attempt
> > + if (!(gpio_cntrl & 0x02))
> > + goto exit;
> Same thing, explain that flag.
added explanation next submission attempt
> > +
> > + gpio_cntrl &= ~0x0F;
> Just &= 0xF0;
see HANDLING NIBBLES below
> > + gpio_cntrl |= 0x0F & gpio->out_config;
> > +
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> > + gpio_cntrl);
>
> Further, if you're checking that flag just in order to avoid doing this write if it's
> not necessary, it's the wrong solution. The right solution is to implement
> regmap in the MFD driver so it quickly sees that the right value is already in the
> register and bounces off.
added explanation next submission attempt
[...]
> > +/*
> > + * da9058_gpio_to_irq is an implementation of the GPIO Hook
> > + * @to_irq: supporting non-static gpio_to_irq() mappings
> > + * whose implementation may not sleep. This hook is called
> > + * when setting up the threaded GPIO irq handler.
> > + */
> > +static int da9058_gpio_to_irq(struct gpio_chip *gc, u32 offset) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
> > +
> > + if (offset)
> > + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1);
> > + else
> > + return da9058_to_virt_irq_num(da9058,
> > +DA9058_IRQ_EGPI0); }
> Lee Jones and Mark Brown discussed these virtual IRQ mapping functions
> recently, and I think the outcome was to patch irqdomain to do the work and
> not sprinkle these custom interfaces to fetch virtual IRQs all over the place.
The DA9058 driver does use the virtual IRQ domains
> > +static int da9058_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
> > + unsigned debounce) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + int ret;
> > + unsigned int gpio_cntrl;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
> > +
> > + mutex_lock(&gpio->lock);
> > +
> > + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> > + if (ret)
> > + goto exit;
> > +
> > + if (offset) {
> > + u8 debounce_bits = debounce ? 0x80 : 0x00;
> > +
> > + gpio->inp_config &= ~0x80;
> > + gpio->inp_config |= debounce_bits;
> > +
> > + if (gpio_cntrl & 0x20)
> > + goto exit;
> > +
> > + gpio_cntrl &= ~0xF0;
> > + gpio_cntrl |= 0xF0 & gpio->inp_config;
> > +
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> gpio_cntrl);
> > + } else {
> > + u8 debounce_bits = debounce ? 0x08 : 0x00;
> > +
> > + gpio->inp_config &= ~0x08;
> > + gpio->inp_config |= debounce_bits;
> > +
> > + if (gpio_cntrl & 0x02)
> > + goto exit;
> > +
> > + gpio_cntrl &= ~0x0F;
> > + gpio_cntrl |= 0x0F & gpio->inp_config;
> > +
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> gpio_cntrl);
> > + }
> > +exit:
> > + mutex_unlock(&gpio->lock);
> > + if (ret)
> > + dev_err(da9058->dev,
> > + "Failed to set GPIO%d bounce=%d error=%d\n",
> > + offset, debounce, ret);
> > + return ret;
> > + }
> Apart from the same comments as above, debounce is actually pin control
> territory and not part of <linux/gpio.h>. Now this is a very small interface, but I
> still want you to consider using pin control for these portions.
> The good thing about doing it with pin control is that you can have it set up on
> boot using hogs and need to think anymor about the pin config business.
PIN CONTROL ???, are there any example drivers that I could/should base it on?
> The rest looks nice!
> Yours,
> Linus Walleij
Thanks for reviewing this DA9058 component driver, that just leaves the
contentious point:
HANDLING NIBBLES
================
The handling of nibbles within a byte follows the rule that constants
for the nibble NOT being operated on have those bits set to zero,
and thus only bits being operated on may be non-zero. Thus to set,
for example, the value 0xB into the MSH the operation is:
byte &= ~0xF0
byte |= 0xB0
it being obvious that it is the upper nibble being operated on.
It seems that you are following a different rule for handling nibbles,
and I can't find any standard for doing so in the kernel, so could
you send me your reference documents?
Tony Olech
On Wed, Aug 15, 2012 at 12:08 PM, Opensource [Anthony Olech]
<[email protected]> wrote:
> [Me]
>> > + if (offset > 1)
>> > + return -EINVAL;
>> So there are two GPIO pins, 0 and 1? That seems odd, but OK.
>
> That is a feature of the hardware. I believe that calling them "0" and
> "1" is the correct thing to do. Correct me if they should have been
> called "1" and "2", or something else.
It's correct, what I thought was odd was the fact that there were only
two GPIO pins on this device. But some have only one even, just wanted
to verify...
> HANDLING NIBBLES
> ================
>
> The handling of nibbles within a byte follows the rule that constants
> for the nibble NOT being operated on have those bits set to zero,
> and thus only bits being operated on may be non-zero. Thus to set,
> for example, the value 0xB into the MSH the operation is:
> byte &= ~0xF0
> byte |= 0xB0
> it being obvious that it is the upper nibble being operated on.
> It seems that you are following a different rule for handling nibbles,
> and I can't find any standard for doing so in the kernel, so could
> you send me your reference documents?
In this case as stated elsewhere, I'm happy that you do things
this way, if you #define the magic values you're using
in your bytes and nibbles, because else it's just hard to read.
#define FOO_MASK 0xF0
#define BAR_FEATURE 0xB0
byte &= ~FOO_MASK;
byte |= BAR_FEATURE;
It's more readble.
Yours,
Linus Walleij