Patchwork gpio/mxc: use the edge_sel feature if available

login
register
mail settings
Submitter Benoît Thébaudeau
Date June 18, 2012, 7:57 p.m.
Message ID <375509863.2886324.1340049432848.JavaMail.root@advansee.com>
Download mbox | patch
Permalink /patch/165580/
State New
Headers show

Comments

Benoît Thébaudeau - June 18, 2012, 7:57 p.m.
Some mxc processors have an edge_sel feature, which allows the IRQ to be
triggered by any edge.

This patch makes use of this feature if available, which skips mxc_flip_edge().

Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../drivers/gpio/gpio-mxc.c                        |   44 ++++++++++++++------
 1 file changed, 32 insertions(+), 12 deletions(-)
Shawn Guo - June 19, 2012, 1:24 a.m.
On Mon, Jun 18, 2012 at 09:57:12PM +0200, Benoît Thébaudeau wrote:
> Some mxc processors have an edge_sel feature, which allows the IRQ to be
> triggered by any edge.
> 
> This patch makes use of this feature if available, which skips mxc_flip_edge().
> 
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

NAK.

We have just gone through the pain to clean up those cpu_is_xxx() from
the driver.  Please do not bring them back.

Regards,
Shawn

> ---
>  .../drivers/gpio/gpio-mxc.c                        |   44 ++++++++++++++------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
> index c337143..4c74482 100644
> --- linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c
> +++ linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/module.h>
> +#include <mach/hardware.h>
>  #include <asm-generic/bug.h>
>  #include <asm/mach/irq.h>
>  
> @@ -104,12 +105,13 @@ static struct mxc_gpio_hwdata *mxc_gpio_hwdata;
>  #define GPIO_ICR2		(mxc_gpio_hwdata->icr2_reg)
>  #define GPIO_IMR		(mxc_gpio_hwdata->imr_reg)
>  #define GPIO_ISR		(mxc_gpio_hwdata->isr_reg)
> +#define GPIO_EDGE_SEL		0x1c
>  
>  #define GPIO_INT_LOW_LEV	(mxc_gpio_hwdata->low_level)
>  #define GPIO_INT_HIGH_LEV	(mxc_gpio_hwdata->high_level)
>  #define GPIO_INT_RISE_EDGE	(mxc_gpio_hwdata->rise_edge)
>  #define GPIO_INT_FALL_EDGE	(mxc_gpio_hwdata->fall_edge)
> -#define GPIO_INT_NONE		0x4
> +#define GPIO_INT_BOTH_EDGES	0x4
>  
>  static struct platform_device_id mxc_gpio_devtype[] = {
>  	{
> @@ -150,6 +152,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  	u32 bit, val;
>  	int edge;
>  	void __iomem *reg = port->base;
> +	bool edge_sel = cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51() || cpu_is_mx53();
>  
>  	port->both_edges &= ~(1 << (gpio & 31));
>  	switch (type) {
> @@ -160,15 +163,19 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  		edge = GPIO_INT_FALL_EDGE;
>  		break;
>  	case IRQ_TYPE_EDGE_BOTH:
> -		val = gpio_get_value(gpio);
> -		if (val) {
> -			edge = GPIO_INT_LOW_LEV;
> -			pr_debug("mxc: set GPIO %d to low trigger\n", gpio);
> +		if (edge_sel) {
> +			edge = GPIO_INT_BOTH_EDGES;
>  		} else {
> -			edge = GPIO_INT_HIGH_LEV;
> -			pr_debug("mxc: set GPIO %d to high trigger\n", gpio);
> +			val = gpio_get_value(gpio);
> +			if (val) {
> +				edge = GPIO_INT_LOW_LEV;
> +				pr_debug("mxc: set GPIO %d to low trigger\n", gpio);
> +			} else {
> +				edge = GPIO_INT_HIGH_LEV;
> +				pr_debug("mxc: set GPIO %d to high trigger\n", gpio);
> +			}
> +			port->both_edges |= 1 << (gpio & 31);
>  		}
> -		port->both_edges |= 1 << (gpio & 31);
>  		break;
>  	case IRQ_TYPE_LEVEL_LOW:
>  		edge = GPIO_INT_LOW_LEV;
> @@ -180,10 +187,23 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  		return -EINVAL;
>  	}
>  
> -	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> -	bit = gpio & 0xf;
> -	val = readl(reg) & ~(0x3 << (bit << 1));
> -	writel(val | (edge << (bit << 1)), reg);
> +	if (edge_sel) {
> +		val = readl(port->base + GPIO_EDGE_SEL);
> +		if (edge == GPIO_INT_BOTH_EDGES)
> +			writel(val | (1 << (gpio & 0x1f)),
> +				port->base + GPIO_EDGE_SEL);
> +		else
> +			writel(val & ~(1 << (gpio & 0x1f)),
> +				port->base + GPIO_EDGE_SEL);
> +	}
> +
> +	if (edge != GPIO_INT_BOTH_EDGES) {
> +		reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> +		bit = gpio & 0xf;
> +		val = readl(reg) & ~(0x3 << (bit << 1));
> +		writel(val | (edge << (bit << 1)), reg);
> +	}
> +
>  	writel(1 << (gpio & 0x1f), port->base + GPIO_ISR);
>  
>  	return 0;
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Benoît Thébaudeau - June 19, 2012, noon
On Tue, Jun 19, 2012 at 03:24:22AM +0200, Shawn Guo wrote:
> NAK.
> 
> We have just gone through the pain to clean up those cpu_is_xxx()
> from
> the driver.  Please do not bring them back.

OK. I don't find the changeset you are referring to. Can you give me an example
that tells me by what you replaced those cpu_is_xxx()? By some feature test
macros? E.g., there are still many of those cpu_is_xxx() in
drivers/mtd/nand/mxc_nand.c in linux-next.

Regards,
Benoît
Benoît Thébaudeau - June 19, 2012, 1:18 p.m.
On Tue, Jun 19, 2012 at 02:00:38PM +0200, Benoît Thébaudeau wrote:
> On Tue, Jun 19, 2012 at 03:24:22AM +0200, Shawn Guo wrote:
> > NAK.
> > 
> > We have just gone through the pain to clean up those cpu_is_xxx()
> > from
> > the driver.  Please do not bring them back.
> 
> OK. I don't find the changeset you are referring to. Can you give me
> an example
> that tells me by what you replaced those cpu_is_xxx()? By some
> feature test
> macros? E.g., there are still many of those cpu_is_xxx() in
> drivers/mtd/nand/mxc_nand.c in linux-next.

I think you were talking about commit e7fc6ae0. I'll see what I can do with
that.

Regards,
Benoît
Linus Walleij - June 20, 2012, 8:35 a.m.
On Tue, Jun 19, 2012 at 2:00 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Can you give me an example
> that tells me by what you replaced those cpu_is_xxx()? By some feature test
> macros?

Since this is a DT-enabled driver, add a boolean attribute for
the DT node like "has-edge-select" and add that attribute to
the SoC .dtsi files for the SoCs that has this feature.

Another (better) option is if there are magic numbers in the
silicon register range that you can check for revision of that
hardware in itself. Such auto-detect plug-n-play is always best.

> E.g., there are still many of those cpu_is_xxx() in
> drivers/mtd/nand/mxc_nand.c in linux-next.

Two wrongs does not make one right or how you put it ;-)

Yours,
Linus Walleij
Sascha Hauer - June 20, 2012, 9:19 a.m.
On Mon, Jun 18, 2012 at 09:57:12PM +0200, Benoît Thébaudeau wrote:
> Some mxc processors have an edge_sel feature, which allows the IRQ to be
> triggered by any edge.
> 
> This patch makes use of this feature if available, which skips mxc_flip_edge().
> 
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> ---
>  .../drivers/gpio/gpio-mxc.c                        |   44 ++++++++++++++------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
> index c337143..4c74482 100644
> --- linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c
> +++ linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/module.h>
> +#include <mach/hardware.h>
>  #include <asm-generic/bug.h>
>  #include <asm/mach/irq.h>
>  
> @@ -104,12 +105,13 @@ static struct mxc_gpio_hwdata *mxc_gpio_hwdata;
>  #define GPIO_ICR2		(mxc_gpio_hwdata->icr2_reg)
>  #define GPIO_IMR		(mxc_gpio_hwdata->imr_reg)
>  #define GPIO_ISR		(mxc_gpio_hwdata->isr_reg)
> +#define GPIO_EDGE_SEL		0x1c
>  
>  #define GPIO_INT_LOW_LEV	(mxc_gpio_hwdata->low_level)
>  #define GPIO_INT_HIGH_LEV	(mxc_gpio_hwdata->high_level)
>  #define GPIO_INT_RISE_EDGE	(mxc_gpio_hwdata->rise_edge)
>  #define GPIO_INT_FALL_EDGE	(mxc_gpio_hwdata->fall_edge)
> -#define GPIO_INT_NONE		0x4
> +#define GPIO_INT_BOTH_EDGES	0x4
>  
>  static struct platform_device_id mxc_gpio_devtype[] = {
>  	{
> @@ -150,6 +152,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  	u32 bit, val;
>  	int edge;
>  	void __iomem *reg = port->base;
> +	bool edge_sel = cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51() || cpu_is_mx53();

To make Shawns suggestion a bit more clear:

What you should do here is to add flags describing differences between
SoCs in mxc_gpio_hwdata.

This means that you have to:

- add IMX35_GPIO to enum mxc_gpio_hwtype
- add imx35-gpio to mxc_gpio_devtype[]
- add "fsl,imx35-gpio" to the compatible list.
- fix all devicetree bindings and platform device bindings for
  i.MX25,35,51,53 and probably i.MX6

Sascha
Sascha Hauer - June 20, 2012, 9:21 a.m.
On Wed, Jun 20, 2012 at 10:35:21AM +0200, Linus Walleij wrote:
> On Tue, Jun 19, 2012 at 2:00 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Can you give me an example
> > that tells me by what you replaced those cpu_is_xxx()? By some feature test
> > macros?
> 
> Since this is a DT-enabled driver, add a boolean attribute for
> the DT node like "has-edge-select" and add that attribute to
> the SoC .dtsi files for the SoCs that has this feature.
> 
> Another (better) option is if there are magic numbers in the
> silicon register range that you can check for revision of that
> hardware in itself. Such auto-detect plug-n-play is always best.

If there would be such a register it would almost certainly be on different
register offsets on different SoCs...

Sascha

Patch

diff --git linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
index c337143..4c74482 100644
--- linux-next-HEAD-6c86b58.orig/drivers/gpio/gpio-mxc.c
+++ linux-next-HEAD-6c86b58/drivers/gpio/gpio-mxc.c
@@ -30,6 +30,7 @@ 
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/module.h>
+#include <mach/hardware.h>
 #include <asm-generic/bug.h>
 #include <asm/mach/irq.h>
 
@@ -104,12 +105,13 @@  static struct mxc_gpio_hwdata *mxc_gpio_hwdata;
 #define GPIO_ICR2		(mxc_gpio_hwdata->icr2_reg)
 #define GPIO_IMR		(mxc_gpio_hwdata->imr_reg)
 #define GPIO_ISR		(mxc_gpio_hwdata->isr_reg)
+#define GPIO_EDGE_SEL		0x1c
 
 #define GPIO_INT_LOW_LEV	(mxc_gpio_hwdata->low_level)
 #define GPIO_INT_HIGH_LEV	(mxc_gpio_hwdata->high_level)
 #define GPIO_INT_RISE_EDGE	(mxc_gpio_hwdata->rise_edge)
 #define GPIO_INT_FALL_EDGE	(mxc_gpio_hwdata->fall_edge)
-#define GPIO_INT_NONE		0x4
+#define GPIO_INT_BOTH_EDGES	0x4
 
 static struct platform_device_id mxc_gpio_devtype[] = {
 	{
@@ -150,6 +152,7 @@  static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	u32 bit, val;
 	int edge;
 	void __iomem *reg = port->base;
+	bool edge_sel = cpu_is_mx25() || cpu_is_mx35() || cpu_is_mx51() || cpu_is_mx53();
 
 	port->both_edges &= ~(1 << (gpio & 31));
 	switch (type) {
@@ -160,15 +163,19 @@  static int gpio_set_irq_type(struct irq_data *d, u32 type)
 		edge = GPIO_INT_FALL_EDGE;
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
-		val = gpio_get_value(gpio);
-		if (val) {
-			edge = GPIO_INT_LOW_LEV;
-			pr_debug("mxc: set GPIO %d to low trigger\n", gpio);
+		if (edge_sel) {
+			edge = GPIO_INT_BOTH_EDGES;
 		} else {
-			edge = GPIO_INT_HIGH_LEV;
-			pr_debug("mxc: set GPIO %d to high trigger\n", gpio);
+			val = gpio_get_value(gpio);
+			if (val) {
+				edge = GPIO_INT_LOW_LEV;
+				pr_debug("mxc: set GPIO %d to low trigger\n", gpio);
+			} else {
+				edge = GPIO_INT_HIGH_LEV;
+				pr_debug("mxc: set GPIO %d to high trigger\n", gpio);
+			}
+			port->both_edges |= 1 << (gpio & 31);
 		}
-		port->both_edges |= 1 << (gpio & 31);
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
 		edge = GPIO_INT_LOW_LEV;
@@ -180,10 +187,23 @@  static int gpio_set_irq_type(struct irq_data *d, u32 type)
 		return -EINVAL;
 	}
 
-	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
-	bit = gpio & 0xf;
-	val = readl(reg) & ~(0x3 << (bit << 1));
-	writel(val | (edge << (bit << 1)), reg);
+	if (edge_sel) {
+		val = readl(port->base + GPIO_EDGE_SEL);
+		if (edge == GPIO_INT_BOTH_EDGES)
+			writel(val | (1 << (gpio & 0x1f)),
+				port->base + GPIO_EDGE_SEL);
+		else
+			writel(val & ~(1 << (gpio & 0x1f)),
+				port->base + GPIO_EDGE_SEL);
+	}
+
+	if (edge != GPIO_INT_BOTH_EDGES) {
+		reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
+		bit = gpio & 0xf;
+		val = readl(reg) & ~(0x3 << (bit << 1));
+		writel(val | (edge << (bit << 1)), reg);
+	}
+
 	writel(1 << (gpio & 0x1f), port->base + GPIO_ISR);
 
 	return 0;