[PATCH 09/16] gpio: add driver for Atheros AR5312 SoC GPIO controller

[PATCH 09/16] gpio: add driver for Atheros AR5312 SoC GPIO controller

[Sergey Ryazanov]

Atheros AR5312 SoC have a builtin GPIO controller, which could be accessed
via memory mapped registers. This patch adds new driver for them.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
---

Changes since RFC:
  - move device registration to separate patch

 drivers/gpio/Kconfig       |   7 +++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-ar5312.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/gpio/gpio-ar5312.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..7ce411b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -112,6 +112,13 @@ config GPIO_MAX730X
 
 comment "Memory mapped GPIO drivers:"
 
+config GPIO_AR5312
+	bool "AR5312 SoC GPIO support"
+	default y if SOC_AR5312
+	depends on SOC_AR5312
+	help
+	  Say yes here to enable GPIO support for Atheros AR5312/AR2312+ SoCs.
+
 config GPIO_CLPS711X
 	tristate "CLPS711X GPIO support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..fae00f4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_ADNP)		+= gpio-adnp.o
 obj-$(CONFIG_GPIO_ADP5520)	+= gpio-adp5520.o
 obj-$(CONFIG_GPIO_ADP5588)	+= gpio-adp5588.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
+obj-$(CONFIG_GPIO_AR5312)	+= gpio-ar5312.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-ar5312.c b/drivers/gpio/gpio-ar5312.c
new file mode 100644
index 0000000..27adb61
--- /dev/null
+++ b/drivers/gpio/gpio-ar5312.c
@@ -0,0 +1,121 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2003 Atheros Communications, Inc.,  All Rights Reserved.
+ * Copyright (C) 2006 FON Technology, SL.
+ * Copyright (C) 2006 Imre Kaloz <kaloz@openwrt.org>
+ * Copyright (C) 2006-2009 Felix Fietkau <nbd@openwrt.org>
+ * Copyright (C) 2012 Alexandros C. Couloumbis <alex@ozo.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+
+#define DRIVER_NAME	"ar5312-gpio"
+
+#define AR5312_GPIO_DO		0x00		/* output register */
+#define AR5312_GPIO_DI		0x04		/* intput register */
+#define AR5312_GPIO_CR		0x08		/* control register */
+
+#define AR5312_GPIO_CR_M(x)	(1 << (x))	/* mask for i/o */
+#define AR5312_GPIO_CR_O(x)	(0 << (x))	/* mask for output */
+#define AR5312_GPIO_CR_I(x)	(1 << (x))	/* mask for input */
+#define AR5312_GPIO_CR_INT(x)	(1 << ((x)+8))	/* mask for interrupt */
+#define AR5312_GPIO_CR_UART(x)	(1 << ((x)+16))	/* uart multiplex */
+
+#define AR5312_GPIO_NUM		8
+
+static void __iomem *ar5312_mem;
+
+static inline u32 ar5312_gpio_reg_read(unsigned reg)
+{
+	return __raw_readl(ar5312_mem + reg);
+}
+
+static inline void ar5312_gpio_reg_write(unsigned reg, u32 val)
+{
+	__raw_writel(val, ar5312_mem + reg);
+}
+
+static inline void ar5312_gpio_reg_mask(unsigned reg, u32 mask, u32 val)
+{
+	ar5312_gpio_reg_write(reg, (ar5312_gpio_reg_read(reg) & ~mask) | val);
+}
+
+static int ar5312_gpio_get_val(struct gpio_chip *chip, unsigned gpio)
+{
+	return (ar5312_gpio_reg_read(AR5312_GPIO_DI) >> gpio) & 1;
+}
+
+static void ar5312_gpio_set_val(struct gpio_chip *chip, unsigned gpio, int val)
+{
+	u32 reg = ar5312_gpio_reg_read(AR5312_GPIO_DO);
+
+	reg = val ? reg | (1 << gpio) : reg & ~(1 << gpio);
+	ar5312_gpio_reg_write(AR5312_GPIO_DO, reg);
+}
+
+static int ar5312_gpio_dir_in(struct gpio_chip *chip, unsigned gpio)
+{
+	ar5312_gpio_reg_mask(AR5312_GPIO_CR, 0, 1 << gpio);
+	return 0;
+}
+
+static int ar5312_gpio_dir_out(struct gpio_chip *chip, unsigned gpio, int val)
+{
+	ar5312_gpio_reg_mask(AR5312_GPIO_CR, 1 << gpio, 0);
+	ar5312_gpio_set_val(chip, gpio, val);
+	return 0;
+}
+
+static struct gpio_chip ar5312_gpio_chip = {
+	.label			= DRIVER_NAME,
+	.direction_input	= ar5312_gpio_dir_in,
+	.direction_output	= ar5312_gpio_dir_out,
+	.set			= ar5312_gpio_set_val,
+	.get			= ar5312_gpio_get_val,
+	.base			= 0,
+	.ngpio			= AR5312_GPIO_NUM,
+};
+
+static int ar5312_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	if (ar5312_mem)
+		return -EBUSY;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ar5312_mem = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ar5312_mem))
+		return PTR_ERR(ar5312_mem);
+
+	ar5312_gpio_chip.dev = dev;
+	ret = gpiochip_add(&ar5312_gpio_chip);
+	if (ret) {
+		dev_err(dev, "failed to add gpiochip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver ar5312_gpio_driver = {
+	.probe = ar5312_gpio_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	}
+};
+
+static int __init ar5312_gpio_init(void)
+{
+	return platform_driver_register(&ar5312_gpio_driver);
+}
+subsys_initcall(ar5312_gpio_init);
-- 
1.8.5.5

Re: [PATCH 09/16] gpio: add driver for Atheros AR5312 SoC GPIO controller

[Linus Walleij]

On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:

> Atheros AR5312 SoC have a builtin GPIO controller, which could be accessed
> via memory mapped registers. This patch adds new driver for them.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org

This driver is extremely simple. You should be able to use
drivers/gpio/gpio-generic.c for this.

NAK for the time being.

> +#define AR5312_GPIO_CR_INT(x)  (1 << ((x)+8))  /* mask for interrupt */

Seems to be unused.
For a MMIO gpiochip using interrupts it's still possible
to use gpio-generic.c as a library-

> +#define AR5312_GPIO_CR_UART(x) (1 << ((x)+16)) /* uart multiplex */

That sounds like pin control business.

Yours,
Linus Walleij

[PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Sergey Ryazanov]

Atheros AR2315 SoC have a builtin GPIO controller, which could be
accessed via memory mapped registers. This patch adds new driver
for them.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
---

Changes since RFC:
  - fix chip name, this patch adds AR2315 GPIO controller driver
  - use dynamic IRQ numbers allocation
  - move device registration to separate patch

 drivers/gpio/Kconfig       |   7 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-ar2315.c | 232 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/gpio/gpio-ar2315.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 7ce411b..0ceb4ba 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -112,6 +112,13 @@ config GPIO_MAX730X
 
 comment "Memory mapped GPIO drivers:"
 
+config GPIO_AR2315
+	bool "AR2315 SoC GPIO support"
+	default y if SOC_AR2315
+	depends on SOC_AR2315
+	help
+	  Say yes here to enable GPIO support for Atheros AR2315+ SoCs.
+
 config GPIO_AR5312
 	bool "AR5312 SoC GPIO support"
 	default y if SOC_AR5312
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index fae00f4..9a3a136 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_ADNP)		+= gpio-adnp.o
 obj-$(CONFIG_GPIO_ADP5520)	+= gpio-adp5520.o
 obj-$(CONFIG_GPIO_ADP5588)	+= gpio-adp5588.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
+obj-$(CONFIG_GPIO_AR2315)	+= gpio-ar2315.o
 obj-$(CONFIG_GPIO_AR5312)	+= gpio-ar5312.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
diff --git a/drivers/gpio/gpio-ar2315.c b/drivers/gpio/gpio-ar2315.c
new file mode 100644
index 0000000..2a6caaf
--- /dev/null
+++ b/drivers/gpio/gpio-ar2315.c
@@ -0,0 +1,232 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2003 Atheros Communications, Inc.,  All Rights Reserved.
+ * Copyright (C) 2006 FON Technology, SL.
+ * Copyright (C) 2006 Imre Kaloz <kaloz@openwrt.org>
+ * Copyright (C) 2006 Felix Fietkau <nbd@openwrt.org>
+ * Copyright (C) 2012 Alexandros C. Couloumbis <alex@ozo.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/irq.h>
+
+#define DRIVER_NAME	"ar2315-gpio"
+
+#define AR2315_GPIO_DI			0x0000
+#define AR2315_GPIO_DO			0x0008
+#define AR2315_GPIO_DIR			0x0010
+#define AR2315_GPIO_INT			0x0018
+
+#define AR2315_GPIO_DIR_M(x)		(1 << (x))	/* mask for i/o */
+#define AR2315_GPIO_DIR_O(x)		(1 << (x))	/* output */
+#define AR2315_GPIO_DIR_I(x)		(0)		/* input */
+
+#define AR2315_GPIO_INT_NUM_M		0x3F		/* mask for GPIO num */
+#define AR2315_GPIO_INT_TRIG(x)		((x) << 6)	/* interrupt trigger */
+#define AR2315_GPIO_INT_TRIG_M		(0x3 << 6)	/* mask for int trig */
+
+#define AR2315_GPIO_INT_TRIG_OFF	0	/* Triggerring off */
+#define AR2315_GPIO_INT_TRIG_LOW	1	/* Low Level Triggered */
+#define AR2315_GPIO_INT_TRIG_HIGH	2	/* High Level Triggered */
+#define AR2315_GPIO_INT_TRIG_EDGE	3	/* Edge Triggered */
+
+#define AR2315_GPIO_NUM		22
+
+static u32 ar2315_gpio_intmask;
+static u32 ar2315_gpio_intval;
+static unsigned ar2315_gpio_irq_base;
+static void __iomem *ar2315_mem;
+
+static inline u32 ar2315_gpio_reg_read(unsigned reg)
+{
+	return __raw_readl(ar2315_mem + reg);
+}
+
+static inline void ar2315_gpio_reg_write(unsigned reg, u32 val)
+{
+	__raw_writel(val, ar2315_mem + reg);
+}
+
+static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val)
+{
+	ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) | val);
+}
+
+static void ar2315_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	u32 pend;
+	int bit = -1;
+
+	/* only do one gpio interrupt at a time */
+	pend = ar2315_gpio_reg_read(AR2315_GPIO_DI);
+	pend ^= ar2315_gpio_intval;
+	pend &= ar2315_gpio_intmask;
+
+	if (pend) {
+		bit = fls(pend) - 1;
+		pend &= ~(1 << bit);
+		ar2315_gpio_intval ^= (1 << bit);
+	}
+
+	/* Enable interrupt with edge detection */
+	if ((ar2315_gpio_reg_read(AR2315_GPIO_DIR) & AR2315_GPIO_DIR_M(bit)) !=
+	    AR2315_GPIO_DIR_I(bit))
+		return;
+
+	if (bit >= 0)
+		generic_handle_irq(ar2315_gpio_irq_base + bit);
+}
+
+static void ar2315_gpio_int_setup(unsigned gpio, int trig)
+{
+	u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT);
+
+	reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M);
+	reg |= gpio | AR2315_GPIO_INT_TRIG(trig);
+	ar2315_gpio_reg_write(AR2315_GPIO_INT, reg);
+}
+
+static void ar2315_gpio_irq_unmask(struct irq_data *d)
+{
+	unsigned gpio = d->irq - ar2315_gpio_irq_base;
+	u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR);
+
+	/* Enable interrupt with edge detection */
+	if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio))
+		return;
+
+	ar2315_gpio_intmask |= (1 << gpio);
+	ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE);
+}
+
+static void ar2315_gpio_irq_mask(struct irq_data *d)
+{
+	unsigned gpio = d->irq - ar2315_gpio_irq_base;
+
+	/* Disable interrupt */
+	ar2315_gpio_intmask &= ~(1 << gpio);
+	ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_OFF);
+}
+
+static struct irq_chip ar2315_gpio_irq_chip = {
+	.name		= DRIVER_NAME,
+	.irq_unmask	= ar2315_gpio_irq_unmask,
+	.irq_mask	= ar2315_gpio_irq_mask,
+};
+
+static void ar2315_gpio_irq_init(unsigned irq)
+{
+	unsigned i;
+
+	ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI);
+	for (i = 0; i < AR2315_GPIO_NUM; i++) {
+		unsigned _irq = ar2315_gpio_irq_base + i;
+
+		irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip,
+					 handle_level_irq);
+	}
+	irq_set_chained_handler(irq, ar2315_gpio_irq_handler);
+}
+
+static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio)
+{
+	return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1;
+}
+
+static void ar2315_gpio_set_val(struct gpio_chip *chip, unsigned gpio, int val)
+{
+	u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_DO);
+
+	reg = val ? reg | (1 << gpio) : reg & ~(1 << gpio);
+	ar2315_gpio_reg_write(AR2315_GPIO_DO, reg);
+}
+
+static int ar2315_gpio_dir_in(struct gpio_chip *chip, unsigned gpio)
+{
+	ar2315_gpio_reg_mask(AR2315_GPIO_DIR, 1 << gpio, 0);
+	return 0;
+}
+
+static int ar2315_gpio_dir_out(struct gpio_chip *chip, unsigned gpio, int val)
+{
+	ar2315_gpio_reg_mask(AR2315_GPIO_DIR, 0, 1 << gpio);
+	ar2315_gpio_set_val(chip, gpio, val);
+	return 0;
+}
+
+static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
+{
+	return ar2315_gpio_irq_base + gpio;
+}
+
+static struct gpio_chip ar2315_gpio_chip = {
+	.label			= DRIVER_NAME,
+	.direction_input	= ar2315_gpio_dir_in,
+	.direction_output	= ar2315_gpio_dir_out,
+	.set			= ar2315_gpio_set_val,
+	.get			= ar2315_gpio_get_val,
+	.to_irq			= ar2315_gpio_to_irq,
+	.base			= 0,
+	.ngpio			= AR2315_GPIO_NUM,
+};
+
+static int ar2315_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	unsigned irq;
+	int ret;
+
+	if (ar2315_mem)
+		return -EBUSY;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, DRIVER_NAME);
+	if (!res) {
+		dev_err(dev, "not found IRQ number\n");
+		return -ENXIO;
+	}
+	irq = res->start;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, DRIVER_NAME);
+	ar2315_mem = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ar2315_mem))
+		return PTR_ERR(ar2315_mem);
+
+	ar2315_gpio_chip.dev = dev;
+	ret = gpiochip_add(&ar2315_gpio_chip);
+	if (ret) {
+		dev_err(dev, "failed to add gpiochip\n");
+		return ret;
+	}
+
+	ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to allocate IRQ numbers\n");
+		return ret;
+	}
+	ar2315_gpio_irq_base = ret;
+
+	ar2315_gpio_irq_init(irq);
+
+	return 0;
+}
+
+static struct platform_driver ar2315_gpio_driver = {
+	.probe = ar2315_gpio_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	}
+};
+
+static int __init ar2315_gpio_init(void)
+{
+	return platform_driver_register(&ar2315_gpio_driver);
+}
+subsys_initcall(ar2315_gpio_init);
-- 
1.8.5.5

Re: [PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Linus Walleij]

On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:

> Atheros AR2315 SoC have a builtin GPIO controller, which could be
> accessed via memory mapped registers. This patch adds new driver
> for them.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org

> +config GPIO_AR2315
> +       bool "AR2315 SoC GPIO support"
> +       default y if SOC_AR2315
> +       depends on SOC_AR2315
> +       help
> +         Say yes here to enable GPIO support for Atheros AR2315+ SoCs.

select GPIOLIB_IRQCHIP

As far as I can see this driver should use the gpiolib irqchip helpers
and create a cascading irqchip. The code uses some copy/pasting
from earlier drivers which is not a good idea. Look at one of the drivers
using GPIOLIB_IRQCHIP and be inspired, check e.g. gpio-pl061.c
or gpio-zynq.c

> +static u32 ar2315_gpio_intmask;
> +static u32 ar2315_gpio_intval;
> +static unsigned ar2315_gpio_irq_base;
> +static void __iomem *ar2315_mem;

No static locals. Allocate and use a state container, see
Documentation/driver-model/design-patterns.txt

> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
> +{
> +       return __raw_readl(ar2315_mem + reg);
> +}

Use readl_relaxed() instead.

> +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val)
> +{
> +       __raw_writel(val, ar2315_mem + reg);
> +}

Use writel_relaxed() instead.

When you use the state container, you need to do a
state dereference like that:

mystate->base + reg

So I don't think these inlines buy you anything. Just use
readl/writel_relaxed directly in the code.

> +static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val)
> +{
> +       ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) | val);
> +}

Too simple helper IMO, if you wanna do this in some organized fashion,
use regmap.

> +static void ar2315_gpio_int_setup(unsigned gpio, int trig)
> +{
> +       u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT);
> +
> +       reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M);
> +       reg |= gpio | AR2315_GPIO_INT_TRIG(trig);
> +       ar2315_gpio_reg_write(AR2315_GPIO_INT, reg);
> +}

Trig? Isn't this supposed to be done in the .set_type()
callback?

> +static void ar2315_gpio_irq_unmask(struct irq_data *d)
> +{
> +       unsigned gpio = d->irq - ar2315_gpio_irq_base;

This kind of weird calculations go away with irqdomain and that
is in turn used by GPIOLIB_IRQCHIP.

After that you will just use d->hwirq. And name the variable
"offset" while you're at it.

> +       u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR);
> +
> +       /* Enable interrupt with edge detection */
> +       if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio))
> +               return;
> +
> +       ar2315_gpio_intmask |= (1 << gpio);

#include <linux/bitops.h>

ar2315_gpio_intmask |= BIT(gpio);

> +       ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE);
> +}

> +static struct irq_chip ar2315_gpio_irq_chip = {
> +       .name           = DRIVER_NAME,
> +       .irq_unmask     = ar2315_gpio_irq_unmask,
> +       .irq_mask       = ar2315_gpio_irq_mask,
> +};

So why is .set_type() not implemented and instead hard-coded into
the unmask function? Please fix this. It will be called by the
core eventually no matter what.

> +static void ar2315_gpio_irq_init(unsigned irq)
> +{
> +       unsigned i;
> +
> +       ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI);
> +       for (i = 0; i < AR2315_GPIO_NUM; i++) {
> +               unsigned _irq = ar2315_gpio_irq_base + i;
> +
> +               irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip,
> +                                        handle_level_irq);
> +       }
> +       irq_set_chained_handler(irq, ar2315_gpio_irq_handler);
> +}

No, use the gpiolib irqchip helpers.

> +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio)
> +{
> +       return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1;
> +}

Do this instead:

return !!(ar2315_gpio_reg_read(AR2315_GPIO_DI) & BIT(offset));

> +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> +       return ar2315_gpio_irq_base + gpio;
> +}

You do not implement this at all when using the gpiolib irqchip helpers.

> +static int ar2315_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       unsigned irq;
> +       int ret;
> +
> +       if (ar2315_mem)
> +               return -EBUSY;
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, DRIVER_NAME);
> +       if (!res) {
> +               dev_err(dev, "not found IRQ number\n");
> +               return -ENXIO;
> +       }
> +       irq = res->start;

Use

irq = platform_get_irq_byname(pdev, DRIVER_NAME);
if (irq < 0)...

> +       ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to allocate IRQ numbers\n");
> +               return ret;
> +       }
> +       ar2315_gpio_irq_base = ret;
> +
> +       ar2315_gpio_irq_init(irq);

No, let GPIOLIB_IRQCHIP handle this.

> +static int __init ar2315_gpio_init(void)
> +{
> +       return platform_driver_register(&ar2315_gpio_driver);
> +}
> +subsys_initcall(ar2315_gpio_init);

Why are you using subsys_initcall()?

This should not be necessary.

Yours,
Linus Walleij

Re: [PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Sergey Ryazanov]

Hi Linus,

thank you for so detailed review!

I have one more generic question: could you merge driver without
GPIOLIB_IRQCHIP usage? Currently no one driver for the AR231x SoCs
uses irq_domain and I do not like to enable IRQ_DOMAIN just for one
driver. I plan to convert drivers to make them irq_domain aware a bit
later.

Please, find my comments below.

2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>:
> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com>
> wrote:
>
>> Atheros AR2315 SoC have a builtin GPIO controller, which could be
>> accessed via memory mapped registers. This patch adds new driver
>> for them.
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Alexandre Courbot <gnurou@gmail.com>
>> Cc: linux-gpio@vger.kernel.org
>
>> +config GPIO_AR2315
>> +       bool "AR2315 SoC GPIO support"
>> +       default y if SOC_AR2315
>> +       depends on SOC_AR2315
>> +       help
>> +         Say yes here to enable GPIO support for Atheros AR2315+ SoCs.
>
> select GPIOLIB_IRQCHIP
>
> As far as I can see this driver should use the gpiolib irqchip helpers
> and create a cascading irqchip. The code uses some copy/pasting
> from earlier drivers which is not a good idea. Look at one of the drivers
> using GPIOLIB_IRQCHIP and be inspired, check e.g. gpio-pl061.c
> or gpio-zynq.c
>
Yes, this driver is pretty old, I updated it with newer API except
IRQ_DOMAIN, which I left for stage 2. Thank you for your hint about
reference realization.

>> +static u32 ar2315_gpio_intmask;
>> +static u32 ar2315_gpio_intval;
>> +static unsigned ar2315_gpio_irq_base;
>> +static void __iomem *ar2315_mem;
>
> No static locals. Allocate and use a state container, see
> Documentation/driver-model/design-patterns.txt
>
Is that rule mandatory for drivers, which serve only one device?

>> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
>> +{
>> +       return __raw_readl(ar2315_mem + reg);
>> +}
>
> Use readl_relaxed() instead.
>
readl_relaxed() converts the bit ordering and seems inapplicable in this case.

>> +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val)
>> +{
>> +       __raw_writel(val, ar2315_mem + reg);
>> +}
>
> Use writel_relaxed() instead.
>
> When you use the state container, you need to do a
> state dereference like that:
>
> mystate->base + reg
>
> So I don't think these inlines buy you anything. Just use
> readl/writel_relaxed directly in the code.
>
These helpers make code shorter and clearer. I can use macros if you
do preferred.

>> +static inline void ar2315_gpio_reg_mask(unsigned reg, u32 mask, u32 val)
>> +{
>> +       ar2315_gpio_reg_write(reg, (ar2315_gpio_reg_read(reg) & ~mask) |
>> val);
>> +}
>
> Too simple helper IMO, if you wanna do this in some organized fashion,
> use regmap.
>
>> +static void ar2315_gpio_int_setup(unsigned gpio, int trig)
>> +{
>> +       u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_INT);
>> +
>> +       reg &= ~(AR2315_GPIO_INT_NUM_M | AR2315_GPIO_INT_TRIG_M);
>> +       reg |= gpio | AR2315_GPIO_INT_TRIG(trig);
>> +       ar2315_gpio_reg_write(AR2315_GPIO_INT, reg);
>> +}
>
> Trig? Isn't this supposed to be done in the .set_type()
> callback?
>
Yep. I tried to cheat kernel and made as if driver does not supports
any other trigger types :)

>> +static void ar2315_gpio_irq_unmask(struct irq_data *d)
>> +{
>> +       unsigned gpio = d->irq - ar2315_gpio_irq_base;
>
> This kind of weird calculations go away with irqdomain and that
> is in turn used by GPIOLIB_IRQCHIP.
>
> After that you will just use d->hwirq. And name the variable
> "offset" while you're at it.
>
>> +       u32 dir = ar2315_gpio_reg_read(AR2315_GPIO_DIR);
>> +
>> +       /* Enable interrupt with edge detection */
>> +       if ((dir & AR2315_GPIO_DIR_M(gpio)) != AR2315_GPIO_DIR_I(gpio))
>> +               return;
>> +
>> +       ar2315_gpio_intmask |= (1 << gpio);
>
> #include <linux/bitops.h>
>
> ar2315_gpio_intmask |= BIT(gpio);
>
>> +       ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE);
>> +}
>
>> +static struct irq_chip ar2315_gpio_irq_chip = {
>> +       .name           = DRIVER_NAME,
>> +       .irq_unmask     = ar2315_gpio_irq_unmask,
>> +       .irq_mask       = ar2315_gpio_irq_mask,
>> +};
>
> So why is .set_type() not implemented and instead hard-coded into
> the unmask function? Please fix this. It will be called by the
> core eventually no matter what.
>
The interrupt configuration is a bit complex. This controller could be
configured to generate interrupts only for two lines at once. Or in
other words: user could select any two lines to generate interrupt.

>> +static void ar2315_gpio_irq_init(unsigned irq)
>> +{
>> +       unsigned i;
>> +
>> +       ar2315_gpio_intval = ar2315_gpio_reg_read(AR2315_GPIO_DI);
>> +       for (i = 0; i < AR2315_GPIO_NUM; i++) {
>> +               unsigned _irq = ar2315_gpio_irq_base + i;
>> +
>> +               irq_set_chip_and_handler(_irq, &ar2315_gpio_irq_chip,
>> +                                        handle_level_irq);
>> +       }
>> +       irq_set_chained_handler(irq, ar2315_gpio_irq_handler);
>> +}
>
> No, use the gpiolib irqchip helpers.
>
>> +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +       return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1;
>> +}
>
> Do this instead:
>
> return !!(ar2315_gpio_reg_read(AR2315_GPIO_DI) & BIT(offset));
>
>> +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +       return ar2315_gpio_irq_base + gpio;
>> +}
>
> You do not implement this at all when using the gpiolib irqchip helpers.
>
>> +static int ar2315_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct resource *res;
>> +       unsigned irq;
>> +       int ret;
>> +
>> +       if (ar2315_mem)
>> +               return -EBUSY;
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
>> DRIVER_NAME);
>> +       if (!res) {
>> +               dev_err(dev, "not found IRQ number\n");
>> +               return -ENXIO;
>> +       }
>> +       irq = res->start;
>
> Use
>
> irq = platform_get_irq_byname(pdev, DRIVER_NAME);
> if (irq < 0)...
>
>> +       ret = irq_alloc_descs(-1, 0, AR2315_GPIO_NUM, 0);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to allocate IRQ numbers\n");
>> +               return ret;
>> +       }
>> +       ar2315_gpio_irq_base = ret;
>> +
>> +       ar2315_gpio_irq_init(irq);
>
> No, let GPIOLIB_IRQCHIP handle this.
>
>> +static int __init ar2315_gpio_init(void)
>> +{
>> +       return platform_driver_register(&ar2315_gpio_driver);
>> +}
>> +subsys_initcall(ar2315_gpio_init);
>
> Why are you using subsys_initcall()?
>
> This should not be necessary.
>
I have users of GPIO in arch code, what called earlier than the
devices initcall.

> Yours,
> Linus Walleij
>

-- 
BR,
Sergey

Re: [PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Linus Walleij]

On Wed, Oct 15, 2014 at 1:12 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>:
>> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com>

> I have one more generic question: could you merge driver without
> GPIOLIB_IRQCHIP usage?

No.

> Currently no one driver for the AR231x SoCs
> uses irq_domain and I do not like to enable IRQ_DOMAIN just for one
> driver. I plan to convert drivers to make them irq_domain aware a bit
> later.

I don't believe any such promises. It's nothing personal, just I've
been burned too many times by people promising to "fix later".

>>> +static u32 ar2315_gpio_intmask;
>>> +static u32 ar2315_gpio_intval;
>>> +static unsigned ar2315_gpio_irq_base;
>>> +static void __iomem *ar2315_mem;
>>
>> No static locals. Allocate and use a state container, see
>> Documentation/driver-model/design-patterns.txt
>>
> Is that rule mandatory for drivers, which serve only one device?

There is no central authority which decides what is mandatory
or not. It is mandatory to get a driver past the GPIO maintainer.

>>> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
>>> +{
>>> +       return __raw_readl(ar2315_mem + reg);
>>> +}
>>
>> Use readl_relaxed() instead.
>>
> readl_relaxed() converts the bit ordering and seems inapplicable in this case.

It assumes the peripherals IO memory is little endian.

If the IO memory for this device is little endian, please stay with
[readl|writel]_relaxed so it looks familiar.

Or is this machine really using big endian hardware registers?
In that case I understand your comment...

>> When you use the state container, you need to do a
>> state dereference like that:
>>
>> mystate->base + reg
>>
>> So I don't think these inlines buy you anything. Just use
>> readl/writel_relaxed directly in the code.
>>
> These helpers make code shorter and clearer. I can use macros if you
> do preferred.

No big deal. Keep it if you like it this way.

>> So why is .set_type() not implemented and instead hard-coded into
>> the unmask function? Please fix this. It will be called by the
>> core eventually no matter what.
>>
> The interrupt configuration is a bit complex. This controller could be
> configured to generate interrupts only for two lines at once. Or in
> other words: user could select any two lines to generate interrupt.

Oh well, better just handle it I guess...

>>> +static int __init ar2315_gpio_init(void)
>>> +{
>>> +       return platform_driver_register(&ar2315_gpio_driver);
>>> +}
>>> +subsys_initcall(ar2315_gpio_init);
>>
>> Why are you using subsys_initcall()?
>>
>> This should not be necessary.
>>
> I have users of GPIO in arch code, what called earlier than the
> devices initcall.

OK? Why are there such users that early and what do they
use the GPIOs for? Any reason they cannot be device_initcall()s?

Yours,
Linus Walleij

Re: [PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Sergey Ryazanov]

2014-10-28 17:37 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Oct 15, 2014 at 1:12 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@linaro.org>:
>>> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@gmail.com>
>
>> I have one more generic question: could you merge driver without
>> GPIOLIB_IRQCHIP usage?
>
> No.
>
Ok.

>> Currently no one driver for the AR231x SoCs
>> uses irq_domain and I do not like to enable IRQ_DOMAIN just for one
>> driver. I plan to convert drivers to make them irq_domain aware a bit
>> later.
>
> I don't believe any such promises. It's nothing personal, just I've
> been burned too many times by people promising to "fix later".
>
Now I drop the driver from the series and return to the development a
bit later, when finished the basic code for the MIPS architecture. In
that case I will have a time to write the driver that does not require
further fixes.


>>>> +static u32 ar2315_gpio_intmask;
>>>> +static u32 ar2315_gpio_intval;
>>>> +static unsigned ar2315_gpio_irq_base;
>>>> +static void __iomem *ar2315_mem;
>>>
>>> No static locals. Allocate and use a state container, see
>>> Documentation/driver-model/design-patterns.txt
>>>
>> Is that rule mandatory for drivers, which serve only one device?
>
> There is no central authority which decides what is mandatory
> or not. It is mandatory to get a driver past the GPIO maintainer.
>
Nice point :)

>>>> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
>>>> +{
>>>> +       return __raw_readl(ar2315_mem + reg);
>>>> +}
>>>
>>> Use readl_relaxed() instead.
>>>
>> readl_relaxed() converts the bit ordering and seems inapplicable in this case.
>
> It assumes the peripherals IO memory is little endian.
>
> If the IO memory for this device is little endian, please stay with
> [readl|writel]_relaxed so it looks familiar.
>
> Or is this machine really using big endian hardware registers?
> In that case I understand your comment...
>
Yes, AR5312 and AR2315 SoCs are big endian machines with big endian registers.


>>> When you use the state container, you need to do a
>>> state dereference like that:
>>>
>>> mystate->base + reg
>>>
>>> So I don't think these inlines buy you anything. Just use
>>> readl/writel_relaxed directly in the code.
>>>
>> These helpers make code shorter and clearer. I can use macros if you
>> do preferred.
>
> No big deal. Keep it if you like it this way.
>
>>> So why is .set_type() not implemented and instead hard-coded into
>>> the unmask function? Please fix this. It will be called by the
>>> core eventually no matter what.
>>>
>> The interrupt configuration is a bit complex. This controller could be
>> configured to generate interrupts only for two lines at once. Or in
>> other words: user could select any two lines to generate interrupt.
>
> Oh well, better just handle it I guess...
>
Will do in v2.

>>>> +static int __init ar2315_gpio_init(void)
>>>> +{
>>>> +       return platform_driver_register(&ar2315_gpio_driver);
>>>> +}
>>>> +subsys_initcall(ar2315_gpio_init);
>>>
>>> Why are you using subsys_initcall()?
>>>
>>> This should not be necessary.
>>>
>> I have users of GPIO in arch code, what called earlier than the
>> devices initcall.
>
> OK? Why are there such users that early and what do they
> use the GPIOs for? Any reason they cannot be device_initcall()s?
>
One GPIO line is used in reset handler to be able to reliably reset
the chip. This is a workaround from vendor's reference design to
eliminate a hw bug in the reset circuit of the AR2315 SoC. So I prefer
to have GPIO controller in ready state as soon as possible.

> Yours,
> Linus Walleij

-- 
BR,
Sergey