This patch is relative to next-20130419 of linux-next
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 component driver of the DA9058 MFD.
The meaning of the PMIC register 21 bits 1 and 5 has been documented
in the driver source.
Changes relative to V5 of this patch:
- rebased to next-20130419 in git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
- removed redundant #include <linux/mfd/da9058/version.h>
- corrected dates on copyright statements
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(+)
create mode 100644 drivers/gpio/gpio-da9058.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 6899afb..96b8b78 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -103,6 +103,18 @@ config GPIO_DA9055
If driver is built as a module it will be called gpio-da9055.
+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 e005ad5..dc41528 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
+obj-$(CONFIG_GPIO_DA9058) += gpio-da9058.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
diff --git a/drivers/gpio/gpio-da9058.c b/drivers/gpio/gpio-da9058.c
new file mode 100644
index 0000000..7c84400
--- /dev/null
+++ b/drivers/gpio/gpio-da9058.c
@@ -0,0 +1,376 @@
+/*
+ * Copyright (C) 2012, 2013 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/gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/regmap.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.
+*
+*
+ * A note about the control register DA9058_GPIO0001_REG
+ *
+ * Bit 0x02 for GPIO[0] and bit 0x20 for GPIO[1] are the direction bits
+ *
+ * gpio_set() only makes sense for GPIO pins configured to be outputs
+ * and since the meaning of all the other bits is different for INP's
+ * we must not change them if that is the case
+ *
+ * gpio_set_debounce() only makes sense for GPIO pins configured to be
+ * inputs and since the meaning of all the other bits is different for
+ * OUT's we must not change them if that is the case
+ *
+ * However to allow for the case where the GPIO user issues the commands
+ * direction_input(), direction_output(), gpio_set(), gpio_set_debounce()
+ * in any order the driver has to remember those settings since using the
+ * DA9058 PMIC itself will not work and that is the reason for the recorded
+ * inp_config and out_config in the driver's da9058_gpio structure.
+ *
+ */
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ int irq_0;
+ int irq_1;
+ u8 inp_config;
+ u8 out_config;
+};
+
+static inline
+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;
+
+ ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, &gpio_level);
+ 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);
+
+ if (offset) {
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio->out_config &= ~0x80;
+ gpio->out_config |= value ? 0x80 : 0x00;
+
+ if (!(gpio_cntrl & 0x20)) /* an INP pin */
+ goto exit;
+
+ gpio_cntrl &= ~0xF0;
+ gpio_cntrl |= 0xF0 & gpio->out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ } else {
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio->out_config &= ~0x08;
+ gpio->out_config |= value ? 0x08 : 0x00;
+
+ if (!(gpio_cntrl & 0x02)) /* an INP pin */
+ 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;
+ int ret;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_update_bits(da9058, DA9058_GPIO0001_REG,
+ offset ? 0xF0 : 0x0F, gpio->inp_config);
+
+ 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;
+ int ret;
+
+ if (offset > 1)
+ return -EINVAL;
+
+ mutex_lock(&gpio->lock);
+
+ ret = da9058_update_bits(da9058, DA9058_GPIO0001_REG,
+ offset ? 0xF0 : 0x0F, gpio->out_config);
+
+ 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, gpio->irq_1);
+ else
+ return da9058_to_virt_irq_num(da9058, gpio->irq_0);
+}
+
+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);
+
+ if (offset) {
+ u8 debounce_bits = debounce ? 0x80 : 0x00;
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio->inp_config &= ~0x80;
+ gpio->inp_config |= debounce_bits;
+
+ if (gpio_cntrl & 0x20) /* an OUT pin */
+ 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;
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio->inp_config &= ~0x08;
+ gpio->inp_config |= debounce_bits;
+
+ if (gpio_cntrl & 0x02) /* an OUT pin */
+ 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 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->irq_0 = platform_get_irq(pdev, 0);
+ gpio->irq_1 = platform_get_irq(pdev, 1);
+ if (gpio->irq_0 < 0 || gpio->irq_1 < 0) {
+ dev_err(&pdev->dev, "cannot get GPIO IRQ error=%d,%d\n",
+ gpio->irq_0, gpio->irq_1);
+ ret = -ENODEV;
+ goto failed_to_get_irq;
+ }
+
+ 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;
+
+ /*
+ * INP configured pins:
+ * 9 == DebounceOn+ActiceLowPullUp
+ *
+ * OUT configured pins:
+ * 7 == PushPull+ExternalPullUpToVDDIO
+ * 3 == PushPull+InternalPullUpToVDDIO
+ * 2 == OpenDrain+InternalPullUpToVDDIO
+ */
+
+ gpio->inp_config = 0x99;
+ gpio->out_config = 0x77;
+
+ mutex_init(&gpio->lock);
+
+ ret = gpiochip_add(&gpio->gp);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+ goto failed_to_add_gpio_chip;
+ }
+
+ platform_set_drvdata(pdev, gpio);
+
+ 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);
+failed_to_add_gpio_chip:
+failed_to_get_irq:
+exit:
+ return ret;
+}
+
+static int da9058_gpio_remove(struct platform_device *pdev)
+{
+ struct da9058_gpio *gpio = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = gpiochip_remove(&gpio->gp);
+
+ return ret;
+}
+
+static struct platform_driver da9058_gpio_driver = {
+ .probe = da9058_gpio_probe,
+ .remove = 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 V6
On Fri, Apr 19, 2013 at 6:56 PM, Anthony Olech
<[email protected]> wrote:
> This patch is relative to next-20130419 of linux-next
>
> 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 component driver of the DA9058 MFD.
> The meaning of the PMIC register 21 bits 1 and 5 has been documented
> in the driver source.
>
> Changes relative to V5 of this patch:
> - rebased to next-20130419 in git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> - removed redundant #include <linux/mfd/da9058/version.h>
> - corrected dates on copyright statements
>
> Signed-off-by: Anthony Olech <[email protected]>
> Signed-off-by: David Dajun Chen <[email protected]>
Sorry for slow review ...
> +struct da9058_gpio {
> + struct da9058 *da9058;
> + struct platform_device *pdev;
> + struct gpio_chip gp;
> + struct mutex lock;
> + int irq_0;
> + int irq_1;
Why are you keeping thes two numbers here? Use the irqdomain
to contain irq mappings, this is just confusion things. (See review
commens further below.)
> + u8 inp_config;
> + u8 out_config;
> +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 we have only two pins, OK that's said in Kconfig too...
(...)
> +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;
> + }
Here you have an error print, pls insert that on the get function too
then.
(...)
> + if (offset) {
> +
I would put a comment here like
/* pin 1 clause */
Maybe it's more logical to have if (!offset) and handle pin
0 first, then pin 1 in the else clause?
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> + gpio->out_config &= ~0x80;
> + gpio->out_config |= value ? 0x80 : 0x00;
> +
> + if (!(gpio_cntrl & 0x20)) /* an INP pin */
> + goto exit;
Please use #define to define these magic bits.
Something like this:
#include <linux/bitops.h>
#define DA9058_GPIO0_VAL BIT(3)
#define DA9058_GPIO0_INOUT BIT(1)
#define DA9058_GPIO1_VAL BIT(7)
#define DA9058_GPIO1_INOUT BIT(5)
(etc, so we see what all bits are for)
then use these defines in the code...
> +
> + gpio_cntrl &= ~0xF0;
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> +
I would put a comment here like
/* pin 0 clause */
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> + gpio->out_config &= ~0x08;
> + gpio->out_config |= value ? 0x08 : 0x00;
> +
> + if (!(gpio_cntrl & 0x02)) /* an INP pin */
> + goto exit;
Magic bits, see above.
(...)
> +/*
> + * 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, gpio->irq_1);
> + else
> + return da9058_to_virt_irq_num(da9058, gpio->irq_0);
> +}
OK, if the DA9058 core is using irqdomain.
> + if (offset) {
> + u8 debounce_bits = debounce ? 0x80 : 0x00;
Is this really correct??
In earlier code you use bit 7 to set the value!
This is partly why I ask you to use #defines for the bits:
less risk to do things wrong by e.g. copy/paste bugs.
> + gpio->inp_config &= ~0x08;
> + gpio->inp_config |= debounce_bits;
Dito.
(...)
> +static int da9058_gpio_probe(struct platform_device *pdev)
(...)
> + /*
> + * INP configured pins:
> + * 9 == DebounceOn+ActiceLowPullUp
> + *
> + * OUT configured pins:
> + * 7 == PushPull+ExternalPullUpToVDDIO
> + * 3 == PushPull+InternalPullUpToVDDIO
> + * 2 == OpenDrain+InternalPullUpToVDDIO
> + */
> +
> + gpio->inp_config = 0x99;
> + gpio->out_config = 0x77;
Again here you should #define the bits and | or them together
instead of using comments to clarify it.
This is pinctrl basically but since you seem to hardcode it it can
be in the GPIO subsystem anyway.
Yours,
Linus Walleij
> -----Original Message-----
> From: Linus Walleij [mailto:[email protected]]
> Sent: 24 April 2013 14:15
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Lee Jones
> Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
> On Fri, Apr 19, 2013 at 6:56 PM, Anthony Olech
> <[email protected]> wrote:
> > This patch is relative to next-20130419 of linux-next
> >
> > 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 component driver of the DA9058 MFD.
> > The meaning of the PMIC register 21 bits 1 and 5 has been documented
> > in the driver source.
> >
> > Changes relative to V5 of this patch:
> > - rebased to next-20130419 in
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > - removed redundant #include <linux/mfd/da9058/version.h>
> > - corrected dates on copyright statements
> >
> > Signed-off-by: Anthony Olech <[email protected]>
> > Signed-off-by: David Dajun Chen <[email protected]>
>
> Sorry for slow review ...
>
> > +struct da9058_gpio {
> > + struct da9058 *da9058;
> > + struct platform_device *pdev;
> > + struct gpio_chip gp;
> > + struct mutex lock;
> > + int irq_0;
> > + int irq_1;
>
> Why are you keeping thes two numbers here? Use the irqdomain to contain irq
> mappings,
this module does not use the irqdomain directly so it gets the numbers from
the platform data, which I thought did not persist past the probe() function.
Is the platform data permanently available? - if that is the case, I can of course
re-get the values on demand from the platform data - please confirm or suggest
an alternative method of extracting the irq number.
> this is just confusion things. (See review commens further below.)
>
> > + u8 inp_config;
> > + u8 out_config;
>
> > +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 we have only two pins, OK that's said in Kconfig too...
>
> (...)
> > +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;
> > + }
>
> Here you have an error print, pls insert that on the get function too then.
the error print was used here because there is no other way to report an error.
On the get function a negative number can be returned, so therefore there is
no need to report it via the error log. I can put more diagnostics back in, but
is it really necessary to duplicate the error indication in some cases?
> (...)
> > + if (offset) {
> > +
>
> I would put a comment here like
> /* pin 1 clause */
yes I will add such comments
> Maybe it's more logical to have if (!offset) and handle pin
> 0 first, then pin 1 in the else clause?
why is it logical for one state to be preferred over the other?
> > + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG,
> &gpio_cntrl);
> > + if (ret)
> > + goto exit;
> > + gpio->out_config &= ~0x80;
> > + gpio->out_config |= value ? 0x80 : 0x00;
> > +
> > + if (!(gpio_cntrl & 0x20)) /* an INP pin */
> > + goto exit;
>
> Please use #define to define these magic bits.
> Something like this:
>
> #include <linux/bitops.h>
>
> #define DA9058_GPIO0_VAL BIT(3)
> #define DA9058_GPIO0_INOUT BIT(1)
> #define DA9058_GPIO1_VAL BIT(7)
> #define DA9058_GPIO1_INOUT BIT(5)
>
> (etc, so we see what all bits are for)
>
> then use these defines in the code...
the meaning of the bits other than the INP/OUT bits are not independent,
but depend on the direction. Thus each nibble contains 3 dependent bits.
>
> > +
> > + gpio_cntrl &= ~0xF0;
> > + gpio_cntrl |= 0xF0 & gpio->out_config;
> > +
> > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> gpio_cntrl);
> > + } else {
> > +
>
> I would put a comment here like
> /* pin 0 clause */
yes I will add such comments
> > + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG,
> &gpio_cntrl);
> > + if (ret)
> > + goto exit;
> > + gpio->out_config &= ~0x08;
> > + gpio->out_config |= value ? 0x08 : 0x00;
> > +
> > + if (!(gpio_cntrl & 0x02)) /* an INP pin */
> > + goto exit;
>
> Magic bits, see above.
see end of e-mail
> (...)
> > +/*
> > + * 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, gpio->irq_1);
> > + else
> > + return da9058_to_virt_irq_num(da9058, gpio->irq_0); }
>
> OK, if the DA9058 core is using irqdomain.
Yes the core is using irqdomain.
> > + if (offset) {
> > + u8 debounce_bits = debounce ? 0x80 : 0x00;
>
> Is this really correct??
>
> In earlier code you use bit 7 to set the value!
You are confusing input and output operations, the meanings
of each nibble's other 3 bits is depends on the direction. That
is why the INP and OUT configuration needs to be saved in a
control structure.
> This is partly why I ask you to use #defines for the bits:
> less risk to do things wrong by e.g. copy/paste bugs.
The hardware definition is in terms of bit patterns in each nibble,
so introducing a dual name for 3 of the 4 bits means double the
number of points an error can be made.
> > + gpio->inp_config &= ~0x08;
> > + gpio->inp_config |= debounce_bits;
>
> Dito.
>
> (...)
> > +static int da9058_gpio_probe(struct platform_device *pdev)
> (...)
> > + /*
> > + * INP configured pins:
> > + * 9 == DebounceOn+ActiceLowPullUp
> > + *
> > + * OUT configured pins:
> > + * 7 == PushPull+ExternalPullUpToVDDIO
> > + * 3 == PushPull+InternalPullUpToVDDIO
> > + * 2 == OpenDrain+InternalPullUpToVDDIO
> > + */
> > +
> > + gpio->inp_config = 0x99;
> > + gpio->out_config = 0x77;
>
> Again here you should #define the bits and | or them together instead of using
> comments to clarify it.
I think that in this particular case it will be more confusing trying to name the bits
due to the fact that 3 out of 4 bit in each nibble depend on the 4th bit.
> This is pinctrl basically but since you seem to hardcode it it can be in the GPIO
> subsystem anyway.
>
> Yours,
> Linus Walleij
Hi Linus,
I failed previously to imagine any naming scheme that would make the meaning
of the magic bits in each nibble clearer. The best I could come up with was the
comment you referred to.
Tony Olech
On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
<[email protected]> wrote:
> [Me]
>> > +struct da9058_gpio {
>> > + struct da9058 *da9058;
>> > + struct platform_device *pdev;
>> > + struct gpio_chip gp;
>> > + struct mutex lock;
>> > + int irq_0;
>> > + int irq_1;
>>
>> Why are you keeping thes two numbers here? Use the irqdomain to contain irq
>> mappings,
>
> this module does not use the irqdomain directly
This does not compute.
> so it gets the numbers from
> the platform data, which I thought did not persist past the probe() function.
> Is the platform data permanently available? - if that is the case, I can of course
> re-get the values on demand from the platform data - please confirm or suggest
> an alternative method of extracting the irq number.
It is indeed usual practice to cache such things from the platform
data in the local state container, do not worry about that.
I am more after where the IRQs are tracked, in an irqdomain or
as some variables somewhere.
I'm OK with this now, I think...
>> > + if (offset > 1) {
>> > + dev_err(da9058->dev,
>> > + "Failed to set GPIO%d output=%d because illegal GPIO\n",
>> > + offset, value);
>> > + return;
>> > + }
>>
>> Here you have an error print, pls insert that on the get function too then.
>
> the error print was used here because there is no other way to report an error.
> On the get function a negative number can be returned, so therefore there is
> no need to report it via the error log. I can put more diagnostics back in, but
> is it really necessary to duplicate the error indication in some cases?
No not necessary, OK I get it...
>> Maybe it's more logical to have if (!offset) and handle pin
>> 0 first, then pin 1 in the else clause?
>
> why is it logical for one state to be preferred over the other?
Because 0 comes before 1 in the system of numbers?
>> Please use #define to define these magic bits.
>> Something like this:
>>
>> #include <linux/bitops.h>
>>
>> #define DA9058_GPIO0_VAL BIT(3)
>> #define DA9058_GPIO0_INOUT BIT(1)
>> #define DA9058_GPIO1_VAL BIT(7)
>> #define DA9058_GPIO1_INOUT BIT(5)
>>
>> (etc, so we see what all bits are for)
>>
>> then use these defines in the code...
>
> the meaning of the bits other than the INP/OUT bits are not independent,
> but depend on the direction. Thus each nibble contains 3 dependent bits.
(...)
>> > + if (offset) {
>> > + u8 debounce_bits = debounce ? 0x80 : 0x00;
>>
>> Is this really correct??
>>
>> In earlier code you use bit 7 to set the value!
>
> You are confusing input and output operations, the meanings
> of each nibble's other 3 bits is depends on the direction. That
> is why the INP and OUT configuration needs to be saved in a
> control structure.
>
>> This is partly why I ask you to use #defines for the bits:
>> less risk to do things wrong by e.g. copy/paste bugs.
>
> The hardware definition is in terms of bit patterns in each nibble,
> so introducing a dual name for 3 of the 4 bits means double the
> number of points an error can be made.
(...)
>> > + gpio->inp_config = 0x99;
>> > + gpio->out_config = 0x77;
>>
>> Again here you should #define the bits and | or them together instead of using
>> comments to clarify it.
>
> I think that in this particular case it will be more confusing trying to name the bits
> due to the fact that 3 out of 4 bit in each nibble depend on the 4th bit.
(...)
> I failed previously to imagine any naming scheme that would make the meaning
> of the magic bits in each nibble clearer. The best I could come up with was the
> comment you referred to.
So add #defines for all combinations, IN and OUT directions.
Also write a comment explaining that the meaning of some
bits change depending on how other bits are set.
#include <linux/bitops.h>
/* This bit in each nybble detemines if the pin is input or output */
#define DA9058_GPIO0_IN 0
#define DA9058_GPIO1_IN 0
#define DA9058_GPIO0_OUT BIT(1)
#define DA9058_GPIO1_OUT BIT(5)
/* This is the meaning of bits 0, 2, 3 in the input mode */
#define DA9058_GPIO0_IN_PU BIT(0)
#define DA9058_GPIO0_IN_PU BIT(4)
#define DA9058_GPIO0_IN_DEB BIT(3)
#define DA9058_GPIO1_IN_DEB BIT(7)
(what is bit 2 in input mode?)
(...)
/* This is the meaning of bits 0, 2, 3 in the output mode */
#define DA9058_GPIO0_OUT_VAL BIT(3)
#define DA9058_GPIO0_OUT_PP_EXT BIT(2) | BIT(0)
#define DA9058_GPIO0_OUT_PP_INT BIT(0)
#define DA9058_GPIO0_OUT_OD 0
#define DA9058_GPIO1_OUT_VAL BIT(7)
#define DA9058_GPIO1_OUT_PP_EXT BIT(6) | BIT(4)
#define DA9058_GPIO1_OUT_PP_INT BIT(4)
#define DA9058_GPIO1_OUT_OD 0
(...)
Then the init in probe() becomes readable:
gpio->inp_config = DA9058_GPIO0_IN |
DA9058_GPIO0_IN_PU |
DA9058_GPIO0_IN_DEB |
DA9058_GPIO1_IN |
DA9058_GPIO0_IN_PU |
DA9058_GPIO0_IN_DEB;
gpio->out_config = DA9058_GPIO0_OUT |
DA9058_GPIO0_OUT_PP_EXT |
DA9058_GPIO1_OUT |
DA9058_GPIO1_OUT_PP_EXT;
This is way more readable than:
gpio->inp_config = 0x99;
gpio->out_config = 0x77;
After this though, I start to wonder if it's not smarter to just
have:
struct da9058_gpio {
(...)
u8 gpio0_in_config:4;
u8 gpio0_out_config:4;
u8 gpio1_in_config:4;
u8 gpio1_out_config:4;
};
Then only define one set of bits for a single nybble and
use some <<4 to | together the apropriate config at
runtime.
Yours,
Linus Walleij
> -----Original Message-----
> From: Linus Walleij [mailto:[email protected]]
> Sent: 03 May 2013 12:59
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; Lee Jones
> Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
>
> On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
> <[email protected]> wrote:
[...]
> >> Maybe it's more logical to have if (!offset) and handle pin
> >> 0 first, then pin 1 in the else clause?
> >
> > why is it logical for one state to be preferred over the other?
>
> Because 0 comes before 1 in the system of numbers?
OK, good enough reason - I will change for next submission.
> >> Please use #define to define these magic bits.
> >> Something like this:
> >>
> >> #include <linux/bitops.h>
> >>
> >> #define DA9058_GPIO0_VAL BIT(3)
> >> #define DA9058_GPIO0_INOUT BIT(1)
> >> #define DA9058_GPIO1_VAL BIT(7)
> >> #define DA9058_GPIO1_INOUT BIT(5)
> >>
> >> (etc, so we see what all bits are for)
> >>
> >> then use these defines in the code...
> >
> > the meaning of the bits other than the INP/OUT bits are not
> > independent, but depend on the direction. Thus each nibble contains 3
> dependent bits.
>
> (...)
> >> > + if (offset) {
> >> > + u8 debounce_bits = debounce ? 0x80 : 0x00;
> >>
> >> Is this really correct??
> >>
> >> In earlier code you use bit 7 to set the value!
> >
> > You are confusing input and output operations, the meanings of each
> > nibble's other 3 bits is depends on the direction. That is why the INP
> > and OUT configuration needs to be saved in a control structure.
> >
> >> This is partly why I ask you to use #defines for the bits:
> >> less risk to do things wrong by e.g. copy/paste bugs.
> >
> > The hardware definition is in terms of bit patterns in each nibble, so
> > introducing a dual name for 3 of the 4 bits means double the number of
> > points an error can be made.
>
> (...)
> >> > + gpio->inp_config = 0x99;
> >> > + gpio->out_config = 0x77;
> >>
> >> Again here you should #define the bits and | or them together instead
> >> of using comments to clarify it.
> >
> > I think that in this particular case it will be more confusing trying
> > to name the bits due to the fact that 3 out of 4 bit in each nibble depend on
> the 4th bit.
>
> (...)
> > I failed previously to imagine any naming scheme that would make the
> > meaning of the magic bits in each nibble clearer. The best I could
> > come up with was the comment you referred to.
>
> So add #defines for all combinations, IN and OUT directions.
>
> Also write a comment explaining that the meaning of some bits change
> depending on how other bits are set.
>
> #include <linux/bitops.h>
>
>
> /* This bit in each nybble detemines if the pin is input or output */ #define
> DA9058_GPIO0_IN 0 #define DA9058_GPIO1_IN 0 #define
> DA9058_GPIO0_OUT BIT(1) #define DA9058_GPIO1_OUT BIT(5)
> /* This is the meaning of bits 0, 2, 3 in the input mode */ #define
> DA9058_GPIO0_IN_PU BIT(0) #define DA9058_GPIO0_IN_PU BIT(4) #define
> DA9058_GPIO0_IN_DEB BIT(3) #define DA9058_GPIO1_IN_DEB BIT(7) (what
> is bit 2 in input mode?)
> (...)
> /* This is the meaning of bits 0, 2, 3 in the output mode */ #define
> DA9058_GPIO0_OUT_VAL BIT(3) #define DA9058_GPIO0_OUT_PP_EXT BIT(2)
> | BIT(0) #define DA9058_GPIO0_OUT_PP_INT BIT(0) #define
> DA9058_GPIO0_OUT_OD 0 #define DA9058_GPIO1_OUT_VAL BIT(7) #define
> DA9058_GPIO1_OUT_PP_EXT BIT(6) | BIT(4) #define
> DA9058_GPIO1_OUT_PP_INT BIT(4) #define DA9058_GPIO1_OUT_OD 0
> (...)
>
> Then the init in probe() becomes readable:
>
> gpio->inp_config = DA9058_GPIO0_IN |
> DA9058_GPIO0_IN_PU |
> DA9058_GPIO0_IN_DEB |
> DA9058_GPIO1_IN |
> DA9058_GPIO0_IN_PU |
> DA9058_GPIO0_IN_DEB;
> gpio->out_config = DA9058_GPIO0_OUT |
> DA9058_GPIO0_OUT_PP_EXT |
> DA9058_GPIO1_OUT |
> DA9058_GPIO1_OUT_PP_EXT;
>
> This is way more readable than:
>
> gpio->inp_config = 0x99;
> gpio->out_config = 0x77;
>
> After this though, I start to wonder if it's not smarter to just
> have:
>
> struct da9058_gpio {
> (...)
> u8 gpio0_in_config:4;
> u8 gpio0_out_config:4;
> u8 gpio1_in_config:4;
> u8 gpio1_out_config:4;
> };
>
> Then only define one set of bits for a single nybble and use some <<4 to |
> together the apropriate config at runtime.
Thanks for this comment - bit fields look like the best way of managing the bits. I will change the source for the next submission attempt.
Many thanks for taking the time to review this driver.
Tony Olech
Dialog Semiconductor