[PATCH 0/3] ARM: mach-shmobile: lager: support GPIO switche

[PATCH 0/3] ARM: mach-shmobile: lager: support GPIO switche

[Simon Horman]

This short series adds support for accessing the 4 pins of SW2 as
gpio keys 1 - 4.

As the gpio-keys driver requires IRQ_TYPE_EDGE_BOTH support for this
has been added to the R-Car GPIO driver.

Simon Horman (3):
  gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH
  ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH
  ARM: mach-shmobile: lager: support GPIO switches

 arch/arm/mach-shmobile/board-lager.c    | 22 ++++++++++++++++++++++
 arch/arm/mach-shmobile/setup-r8a7790.c  |  1 +
 drivers/gpio/gpio-rcar.c                | 27 ++++++++++++++++++++++-----
 include/linux/platform_data/gpio-rcar.h |  1 +
 4 files changed, 46 insertions(+), 5 deletions(-)

-- 
1.8.2.1

[PATCH 1/3] gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH

[Simon Horman]

As hardware support for this feature is not universal for all SoCs a flag,
has_both_edge_trigger, has been added to the platform data of the driver to
allow this feature to be enabled.

The motivation for this is to allow use of the gpio-keys driver on the
lager board which is based on the r8a7790 SoC. This patch has been lightly
exercised using that driver on that board.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/gpio/gpio-rcar.c                | 27 ++++++++++++++++++++++-----
 include/linux/platform_data/gpio-rcar.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 0f3d647..33d8f6a 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -49,6 +49,7 @@ struct gpio_rcar_priv {
 #define POSNEG 0x20
 #define EDGLEVEL 0x24
 #define FILONOFF 0x28
+#define BOTHEDGE 0x4c
 
 static inline u32 gpio_rcar_read(struct gpio_rcar_priv *p, int offs)
 {
@@ -91,7 +92,8 @@ static void gpio_rcar_irq_enable(struct irq_data *d)
 static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
 						  unsigned int hwirq,
 						  bool active_high_rising_edge,
-						  bool level_trigger)
+						  bool level_trigger,
+						  bool both)
 {
 	unsigned long flags;
 
@@ -111,6 +113,11 @@ static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
 	/* Select "Interrupt Input Mode" in IOINTSEL */
 	gpio_rcar_modify_bit(p, IOINTSEL, hwirq, true);
 
+	if (p->config.has_both_edge_trigger && !level_trigger) {
+		/* Select one edge or both edges in BOTHEDGE */
+		gpio_rcar_modify_bit(p, IOINTSEL, hwirq, both);
+	}
+
 	/* Write INTCLR in case of edge trigger */
 	if (!level_trigger)
 		gpio_rcar_write(p, INTCLR, BIT(hwirq));
@@ -127,16 +134,26 @@ static int gpio_rcar_irq_set_type(struct irq_data *d, unsigned int type)
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_LEVEL_HIGH:
-		gpio_rcar_config_interrupt_input_mode(p, hwirq, true, true);
+		gpio_rcar_config_interrupt_input_mode(p, hwirq, true, true,
+						      false);
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		gpio_rcar_config_interrupt_input_mode(p, hwirq, false, true);
+		gpio_rcar_config_interrupt_input_mode(p, hwirq, false, true,
+						      false);
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		gpio_rcar_config_interrupt_input_mode(p, hwirq, true, false);
+		gpio_rcar_config_interrupt_input_mode(p, hwirq, true, false,
+						      false);
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		gpio_rcar_config_interrupt_input_mode(p, hwirq, false, false);
+		gpio_rcar_config_interrupt_input_mode(p, hwirq, false, false,
+						      false);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		if (!p->config.has_both_edge_trigger)
+			return -EINVAL;
+		gpio_rcar_config_interrupt_input_mode(p, hwirq, true, false,
+						      true);
 		break;
 	default:
 		return -EINVAL;
diff --git a/include/linux/platform_data/gpio-rcar.h b/include/linux/platform_data/gpio-rcar.h
index cc472f6..dfea5c6 100644
--- a/include/linux/platform_data/gpio-rcar.h
+++ b/include/linux/platform_data/gpio-rcar.h
@@ -21,6 +21,7 @@ struct gpio_rcar_config {
 	unsigned int irq_base;
 	unsigned int number_of_pins;
 	const char *pctl_name;
+	unsigned has_both_edge_trigger:1;
 };
 
 #define RCAR_GP_PIN(bank, pin)		(((bank) * 32) + (pin))
-- 
1.8.2.1

[PATCH 2/3] ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH

[Simon Horman]

"gpio-rcar: Support IRQ_TYPE_EDGE_BOTH" adds support to the R-Car GPIO
driver for IRQ_TYPE_EDGE_BOTH. As hardware support for this feature is
not universal for all SoCs a flag, has_both_edge_trigger, has been
added to the platform data of the driver to allow this feature to be
enabled.

As the r8a7790 SoC hardware supports this feature enable it.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---

This patch has a build-time dependency on
"gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH".
---
 arch/arm/mach-shmobile/setup-r8a7790.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-shmobile/setup-r8a7790.c b/arch/arm/mach-shmobile/setup-r8a7790.c
index eeef5f6..b461d93 100644
--- a/arch/arm/mach-shmobile/setup-r8a7790.c
+++ b/arch/arm/mach-shmobile/setup-r8a7790.c
@@ -45,6 +45,7 @@ static struct gpio_rcar_config r8a7790_gpio##idx##_platform_data = {	\
 	.irq_base	= 0,						\
 	.number_of_pins	= 32,						\
 	.pctl_name	= "pfc-r8a7790",				\
+	.has_both_edge_trigger = 1,					\
 };									\
 
 R8A7790_GPIO(0);
-- 
1.8.2.1

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Simon Horman]

The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
This patch allows access to those pins as KEYS 1 - 4 using
gpio-keys.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---

This patch has run time dependencies on
1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH"

Without those dependencies satisfied the gpio-keys driver will
fail to be initialise itself. The boot should be otherwise successful
and the board otherwise functional.
---
 arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
index 6a1ba38..4d1b42b 100644
--- a/arch/arm/mach-shmobile/board-lager.c
+++ b/arch/arm/mach-shmobile/board-lager.c
@@ -18,7 +18,10 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
 #include <linux/interrupt.h>
+#include <linux/input.h>
 #include <linux/irqchip.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
@@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
 	.num_leds	= ARRAY_SIZE(lager_leds),
 };
 
+/* GPIO KEY */
+#define GPIO_KEY(c, g, d, ...) \
+	{ .code = c, .gpio = g, .desc = d, .active_low = 1 }
+
+static struct gpio_keys_button gpio_buttons[] = {
+	GPIO_KEY(KEY_4,		RCAR_GP_PIN(1, 28),	"SW2-pin4"),
+	GPIO_KEY(KEY_3,		RCAR_GP_PIN(1, 26),	"SW2-pin3"),
+	GPIO_KEY(KEY_2,		RCAR_GP_PIN(1, 24),	"SW2-pin2"),
+	GPIO_KEY(KEY_1,		RCAR_GP_PIN(1, 14),	"SW2-pin1"),
+};
+
+static struct gpio_keys_platform_data lager_keys_pdata = {
+	.buttons	= gpio_buttons,
+	.nbuttons	= ARRAY_SIZE(gpio_buttons),
+};
+
 static const struct pinctrl_map lager_pinctrl_map[] = {
 	/* SCIF0 (CN19: DEBUG SERIAL0) */
 	PIN_MAP_MUX_GROUP_DEFAULT("sh-sci.6", "pfc-r8a7790",
@@ -73,6 +92,9 @@ static void __init lager_add_standard_devices(void)
 	platform_device_register_data(&platform_bus, "leds-gpio", -1,
 				      &lager_leds_pdata,
 				      sizeof(lager_leds_pdata));
+	platform_device_register_data(&platform_bus, "gpio-keys", -1,
+				      &lager_keys_pdata,
+				      sizeof(lager_keys_pdata));
 }
 
 static const char *lager_boards_compat_dt[] __initdata = {
-- 
1.8.2.1

[PATCH 1/3] gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH

[Magnus Damm]

On Mon, May 13, 2013 at 5:53 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> As hardware support for this feature is not universal for all SoCs a flag,
> has_both_edge_trigger, has been added to the platform data of the driver to
> allow this feature to be enabled.
>
> The motivation for this is to allow use of the gpio-keys driver on the
> lager board which is based on the r8a7790 SoC. This patch has been lightly
> exercised using that driver on that board.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/gpio/gpio-rcar.c                | 27 ++++++++++++++++++++++-----
>  include/linux/platform_data/gpio-rcar.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)

Thanks for the patch, Simon!

> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index 0f3d647..33d8f6a 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -49,6 +49,7 @@ struct gpio_rcar_priv {
>  #define POSNEG 0x20
>  #define EDGLEVEL 0x24
>  #define FILONOFF 0x28
> +#define BOTHEDGE 0x4c
>
>  static inline u32 gpio_rcar_read(struct gpio_rcar_priv *p, int offs)
>  {
> @@ -111,6 +113,11 @@ static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
>         /* Select "Interrupt Input Mode" in IOINTSEL */
>         gpio_rcar_modify_bit(p, IOINTSEL, hwirq, true);
>
> +       if (p->config.has_both_edge_trigger && !level_trigger) {
> +               /* Select one edge or both edges in BOTHEDGE */
> +               gpio_rcar_modify_bit(p, IOINTSEL, hwirq, both);
> +       }
> +
>         /* Write INTCLR in case of edge trigger */
>         if (!level_trigger)
>                 gpio_rcar_write(p, INTCLR, BIT(hwirq));

In your hunk above I suspect you want to setup the BOTHEDGE register
instead of IOINTSEL.

Also, I wonder how BOTHEDGE should be set in the case of level
trigger. Say that the user first selects edge trigger and then
reconfigures to level, then you probably want to make sure the
BOTHEDGE register is cleared.

So may want to do something like this (untested):

       if (p->config.has_both_edge_trigger) {
               /* Select one edge or both edges in BOTHEDGE */
               gpio_rcar_modify_bit(p, BOTHEDGE, hwirq, both);
       }

Also, please double check that the register access sequences in the
documentation are still kept, please see "6.7.1 Setting Edge-Sensitive
Interrupt Input Mode" in the H2 data sheet. It looks like you want to
move this setting so it happens before IOINTSEL.

Thanks!

/ magnus

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Sergei Shtylyov]

On 13-05-2013 12:53, Simon Horman wrote:

> The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
> This patch allows access to those pins as KEYS 1 - 4 using
> gpio-keys.

> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

[...]

> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index 6a1ba38..4d1b42b 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
[...]
> @@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
>   	.num_leds	= ARRAY_SIZE(lager_leds),
>   };
>
> +/* GPIO KEY */
> +#define GPIO_KEY(c, g, d, ...) \
> +	{ .code = c, .gpio = g, .desc = d, .active_low = 1 }
> +
> +static struct gpio_keys_button gpio_buttons[] = {
> +	GPIO_KEY(KEY_4,		RCAR_GP_PIN(1, 28),	"SW2-pin4"),
> +	GPIO_KEY(KEY_3,		RCAR_GP_PIN(1, 26),	"SW2-pin3"),
> +	GPIO_KEY(KEY_2,		RCAR_GP_PIN(1, 24),	"SW2-pin2"),
> +	GPIO_KEY(KEY_1,		RCAR_GP_PIN(1, 14),	"SW2-pin1"),
> +};
> +
> +static struct gpio_keys_platform_data lager_keys_pdata = {

    You forgot to annotate this with __initdata.

> +	.buttons	= gpio_buttons,
> +	.nbuttons	= ARRAY_SIZE(gpio_buttons),
> +};
> +
>   static const struct pinctrl_map lager_pinctrl_map[] = {
>   	/* SCIF0 (CN19: DEBUG SERIAL0) */
>   	PIN_MAP_MUX_GROUP_DEFAULT("sh-sci.6", "pfc-r8a7790",
> @@ -73,6 +92,9 @@ static void __init lager_add_standard_devices(void)
>   	platform_device_register_data(&platform_bus, "leds-gpio", -1,
>   				      &lager_leds_pdata,
>   				      sizeof(lager_leds_pdata));
> +	platform_device_register_data(&platform_bus, "gpio-keys", -1,
> +				      &lager_keys_pdata,
> +				      sizeof(lager_keys_pdata));
>   }

WBR, Sergei

[PATCH 2/3] ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH

[Laurent Pinchart]

Hi Simon,

On Monday 13 May 2013 17:53:52 Simon Horman wrote:
> "gpio-rcar: Support IRQ_TYPE_EDGE_BOTH" adds support to the R-Car GPIO
> driver for IRQ_TYPE_EDGE_BOTH. As hardware support for this feature is
> not universal for all SoCs a flag, has_both_edge_trigger, has been
> added to the platform data of the driver to allow this feature to be
> enabled.

What about moving this information to a platform ID table in the gpio-rcar 
driver ?

> As the r8a7790 SoC hardware supports this feature enable it.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
> ---
> 
> This patch has a build-time dependency on
> "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH".
> ---
>  arch/arm/mach-shmobile/setup-r8a7790.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mach-shmobile/setup-r8a7790.c
> b/arch/arm/mach-shmobile/setup-r8a7790.c index eeef5f6..b461d93 100644
> --- a/arch/arm/mach-shmobile/setup-r8a7790.c
> +++ b/arch/arm/mach-shmobile/setup-r8a7790.c
> @@ -45,6 +45,7 @@ static struct gpio_rcar_config
> r8a7790_gpio##idx##_platform_data = {	\ .irq_base	= 0,						
\
>  	.number_of_pins	= 32,						\
>  	.pctl_name	= "pfc-r8a7790",				\
> +	.has_both_edge_trigger = 1,					\
>  };									\
> 
>  R8A7790_GPIO(0);
-- 
Regards,

Laurent Pinchart

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Laurent Pinchart]

Hi Simon,

Thank you for the patch.

On Monday 13 May 2013 17:53:53 Simon Horman wrote:
> The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
> This patch allows access to those pins as KEYS 1 - 4 using
> gpio-keys.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
> ---
> 
> This patch has run time dependencies on
> 1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
> 2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH"
> 
> Without those dependencies satisfied the gpio-keys driver will
> fail to be initialise itself. The boot should be otherwise successful
> and the board otherwise functional.
> ---
>  arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm/mach-shmobile/board-lager.c
> b/arch/arm/mach-shmobile/board-lager.c index 6a1ba38..4d1b42b 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
> @@ -18,7 +18,10 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
> USA */
> 
> +#include <linux/gpio.h>
> +#include <linux/gpio_keys.h>
>  #include <linux/interrupt.h>
> +#include <linux/input.h>

My dictionary lists input before interrupt :-)

>  #include <linux/irqchip.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
> @@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
> .num_leds	= ARRAY_SIZE(lager_leds),
>  };
> 
> +/* GPIO KEY */
> +#define GPIO_KEY(c, g, d, ...) \
> +	{ .code = c, .gpio = g, .desc = d, .active_low = 1 }
> +
> +static struct gpio_keys_button gpio_buttons[] = {
> +	GPIO_KEY(KEY_4,		RCAR_GP_PIN(1, 28),	"SW2-pin4"),
> +	GPIO_KEY(KEY_3,		RCAR_GP_PIN(1, 26),	"SW2-pin3"),
> +	GPIO_KEY(KEY_2,		RCAR_GP_PIN(1, 24),	"SW2-pin2"),
> +	GPIO_KEY(KEY_1,		RCAR_GP_PIN(1, 14),	"SW2-pin1"),
> +};
> +
> +static struct gpio_keys_platform_data lager_keys_pdata = {
> +	.buttons	= gpio_buttons,
> +	.nbuttons	= ARRAY_SIZE(gpio_buttons),
> +};
> +

Could you please also update the lager dts file ?

>  static const struct pinctrl_map lager_pinctrl_map[] = {
>  	/* SCIF0 (CN19: DEBUG SERIAL0) */
>  	PIN_MAP_MUX_GROUP_DEFAULT("sh-sci.6", "pfc-r8a7790",
> @@ -73,6 +92,9 @@ static void __init lager_add_standard_devices(void)
>  	platform_device_register_data(&platform_bus, "leds-gpio", -1,
>  				      &lager_leds_pdata,
>  				      sizeof(lager_leds_pdata));
> +	platform_device_register_data(&platform_bus, "gpio-keys", -1,
> +				      &lager_keys_pdata,
> +				      sizeof(lager_keys_pdata));
>  }
> 
>  static const char *lager_boards_compat_dt[] __initdata = {
-- 
Regards,

Laurent Pinchart

[PATCH 2/3] ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH

[Simon Horman]

On Tue, May 14, 2013 at 12:33:42AM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Monday 13 May 2013 17:53:52 Simon Horman wrote:
> > "gpio-rcar: Support IRQ_TYPE_EDGE_BOTH" adds support to the R-Car GPIO
> > driver for IRQ_TYPE_EDGE_BOTH. As hardware support for this feature is
> > not universal for all SoCs a flag, has_both_edge_trigger, has been
> > added to the platform data of the driver to allow this feature to be
> > enabled.
> 
> What about moving this information to a platform ID table in the gpio-rcar 
> driver ?

Magnus, do you have an opinion on this?

> > As the r8a7790 SoC hardware supports this feature enable it.
> > 
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > 
> > ---
> > 
> > This patch has a build-time dependency on
> > "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH".
> > ---
> >  arch/arm/mach-shmobile/setup-r8a7790.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mach-shmobile/setup-r8a7790.c
> > b/arch/arm/mach-shmobile/setup-r8a7790.c index eeef5f6..b461d93 100644
> > --- a/arch/arm/mach-shmobile/setup-r8a7790.c
> > +++ b/arch/arm/mach-shmobile/setup-r8a7790.c
> > @@ -45,6 +45,7 @@ static struct gpio_rcar_config
> > r8a7790_gpio##idx##_platform_data = {	\ .irq_base	= 0,						
> \
> >  	.number_of_pins	= 32,						\
> >  	.pctl_name	= "pfc-r8a7790",				\
> > +	.has_both_edge_trigger = 1,					\
> >  };									\
> > 
> >  R8A7790_GPIO(0);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/3] gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH

[Simon Horman]

On Mon, May 13, 2013 at 07:14:41PM +0900, Magnus Damm wrote:
> On Mon, May 13, 2013 at 5:53 PM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > As hardware support for this feature is not universal for all SoCs a flag,
> > has_both_edge_trigger, has been added to the platform data of the driver to
> > allow this feature to be enabled.
> >
> > The motivation for this is to allow use of the gpio-keys driver on the
> > lager board which is based on the r8a7790 SoC. This patch has been lightly
> > exercised using that driver on that board.
> >
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> >  drivers/gpio/gpio-rcar.c                | 27 ++++++++++++++++++++++-----
> >  include/linux/platform_data/gpio-rcar.h |  1 +
> >  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> Thanks for the patch, Simon!
> 
> > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> > index 0f3d647..33d8f6a 100644
> > --- a/drivers/gpio/gpio-rcar.c
> > +++ b/drivers/gpio/gpio-rcar.c
> > @@ -49,6 +49,7 @@ struct gpio_rcar_priv {
> >  #define POSNEG 0x20
> >  #define EDGLEVEL 0x24
> >  #define FILONOFF 0x28
> > +#define BOTHEDGE 0x4c
> >
> >  static inline u32 gpio_rcar_read(struct gpio_rcar_priv *p, int offs)
> >  {
> > @@ -111,6 +113,11 @@ static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
> >         /* Select "Interrupt Input Mode" in IOINTSEL */
> >         gpio_rcar_modify_bit(p, IOINTSEL, hwirq, true);
> >
> > +       if (p->config.has_both_edge_trigger && !level_trigger) {
> > +               /* Select one edge or both edges in BOTHEDGE */
> > +               gpio_rcar_modify_bit(p, IOINTSEL, hwirq, both);
> > +       }
> > +
> >         /* Write INTCLR in case of edge trigger */
> >         if (!level_trigger)
> >                 gpio_rcar_write(p, INTCLR, BIT(hwirq));
> 
> In your hunk above I suspect you want to setup the BOTHEDGE register
> instead of IOINTSEL.

Thanks, I will fix that.

> Also, I wonder how BOTHEDGE should be set in the case of level
> trigger. Say that the user first selects edge trigger and then
> reconfigures to level, then you probably want to make sure the
> BOTHEDGE register is cleared.
> 
> So may want to do something like this (untested):
> 
>        if (p->config.has_both_edge_trigger) {
>                /* Select one edge or both edges in BOTHEDGE */
>                gpio_rcar_modify_bit(p, BOTHEDGE, hwirq, both);
>        }

Sure, if you think so.

> Also, please double check that the register access sequences in the
> documentation are still kept, please see "6.7.1 Setting Edge-Sensitive
> Interrupt Input Mode" in the H2 data sheet. It looks like you want to
> move this setting so it happens before IOINTSEL.

Thanks, I will fix that.

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Simon Horman]

On Tue, May 14, 2013 at 12:35:20AM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> Thank you for the patch.
> 
> On Monday 13 May 2013 17:53:53 Simon Horman wrote:
> > The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
> > This patch allows access to those pins as KEYS 1 - 4 using
> > gpio-keys.
> > 
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > 
> > ---
> > 
> > This patch has run time dependencies on
> > 1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
> > 2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH"
> > 
> > Without those dependencies satisfied the gpio-keys driver will
> > fail to be initialise itself. The boot should be otherwise successful
> > and the board otherwise functional.
> > ---
> >  arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/arch/arm/mach-shmobile/board-lager.c
> > b/arch/arm/mach-shmobile/board-lager.c index 6a1ba38..4d1b42b 100644
> > --- a/arch/arm/mach-shmobile/board-lager.c
> > +++ b/arch/arm/mach-shmobile/board-lager.c
> > @@ -18,7 +18,10 @@
> >   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
> > USA */
> > 
> > +#include <linux/gpio.h>
> > +#include <linux/gpio_keys.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/input.h>
> 
> My dictionary lists input before interrupt :-)

Thanks, I will fix that.

> >  #include <linux/irqchip.h>
> >  #include <linux/kernel.h>
> >  #include <linux/leds.h>
> > @@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
> > .num_leds	= ARRAY_SIZE(lager_leds),
> >  };
> > 
> > +/* GPIO KEY */
> > +#define GPIO_KEY(c, g, d, ...) \
> > +	{ .code = c, .gpio = g, .desc = d, .active_low = 1 }
> > +
> > +static struct gpio_keys_button gpio_buttons[] = {
> > +	GPIO_KEY(KEY_4,		RCAR_GP_PIN(1, 28),	"SW2-pin4"),
> > +	GPIO_KEY(KEY_3,		RCAR_GP_PIN(1, 26),	"SW2-pin3"),
> > +	GPIO_KEY(KEY_2,		RCAR_GP_PIN(1, 24),	"SW2-pin2"),
> > +	GPIO_KEY(KEY_1,		RCAR_GP_PIN(1, 14),	"SW2-pin1"),
> > +};
> > +
> > +static struct gpio_keys_platform_data lager_keys_pdata = {
> > +	.buttons	= gpio_buttons,
> > +	.nbuttons	= ARRAY_SIZE(gpio_buttons),
> > +};
> > +
> 
> Could you please also update the lager dts file ?

I wonder if in that case we can remove the C code above.
And probably the C code for the GPIO leds too.

Magnus, could you clarify how you would like
C-code and DTS to look on the lager board?

> >  static const struct pinctrl_map lager_pinctrl_map[] = {
> >  	/* SCIF0 (CN19: DEBUG SERIAL0) */
> >  	PIN_MAP_MUX_GROUP_DEFAULT("sh-sci.6", "pfc-r8a7790",
> > @@ -73,6 +92,9 @@ static void __init lager_add_standard_devices(void)
> >  	platform_device_register_data(&platform_bus, "leds-gpio", -1,
> >  				      &lager_leds_pdata,
> >  				      sizeof(lager_leds_pdata));
> > +	platform_device_register_data(&platform_bus, "gpio-keys", -1,
> > +				      &lager_keys_pdata,
> > +				      sizeof(lager_keys_pdata));
> >  }
> > 
> >  static const char *lager_boards_compat_dt[] __initdata = {

[PATCH 2/3] ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH

[Magnus Damm]

Hi Simon, Laurent,

On Tue, May 14, 2013 at 11:40 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, May 14, 2013 at 12:33:42AM +0200, Laurent Pinchart wrote:
>> Hi Simon,
>>
>> On Monday 13 May 2013 17:53:52 Simon Horman wrote:
>> > "gpio-rcar: Support IRQ_TYPE_EDGE_BOTH" adds support to the R-Car GPIO
>> > driver for IRQ_TYPE_EDGE_BOTH. As hardware support for this feature is
>> > not universal for all SoCs a flag, has_both_edge_trigger, has been
>> > added to the platform data of the driver to allow this feature to be
>> > enabled.
>>
>> What about moving this information to a platform ID table in the gpio-rcar
>> driver ?
>
> Magnus, do you have an opinion on this?

I don't mind so much as long as we:
a) can control things freely per driver instance
and
b) don't have to update all drivers for every new SoC

The current approach allows for that, so as an example if we can have
two slightly different GPIO hardware blocks then we can setup two GPIO
controller driver instances where the configuration can be selected
per-instance. Perhaps Laurent's proposal would allow for this too, I'm
not sure.

On top of that I'd like to avoid doing per-SoC string matching in
drivers. Those would force us to update every darn driver for every
new SoC. It is better to try to use a per-hardware device version in
the drivers and then configure each SoC to use the appropriate
version.

Thanks,

/ magnus

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Magnus Damm]

Hi Simon and Laurent,

On Tue, May 14, 2013 at 11:59 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, May 14, 2013 at 12:35:20AM +0200, Laurent Pinchart wrote:
>> Hi Simon,
>>
>> Thank you for the patch.
>>
>> On Monday 13 May 2013 17:53:53 Simon Horman wrote:
>> > The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
>> > This patch allows access to those pins as KEYS 1 - 4 using
>> > gpio-keys.
>> >
>> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>> >
>> > ---
>> >
>> > This patch has run time dependencies on
>> > 1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
>> > 2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH"
>> >
>> > Without those dependencies satisfied the gpio-keys driver will
>> > fail to be initialise itself. The boot should be otherwise successful
>> > and the board otherwise functional.
>> > ---
>> >  arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
>> >  1 file changed, 22 insertions(+)
>> >
>> > diff --git a/arch/arm/mach-shmobile/board-lager.c
>> > b/arch/arm/mach-shmobile/board-lager.c index 6a1ba38..4d1b42b 100644
>> > --- a/arch/arm/mach-shmobile/board-lager.c
>> > +++ b/arch/arm/mach-shmobile/board-lager.c
>> > @@ -18,7 +18,10 @@
>> >   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
>> > USA */
>> >
>> > +#include <linux/gpio.h>
>> > +#include <linux/gpio_keys.h>
>> >  #include <linux/interrupt.h>
>> > +#include <linux/input.h>
>>
>> My dictionary lists input before interrupt :-)
>
> Thanks, I will fix that.
>
>> >  #include <linux/irqchip.h>
>> >  #include <linux/kernel.h>
>> >  #include <linux/leds.h>
>> > @@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
>> > .num_leds   = ARRAY_SIZE(lager_leds),
>> >  };
>> >
>> > +/* GPIO KEY */
>> > +#define GPIO_KEY(c, g, d, ...) \
>> > +   { .code = c, .gpio = g, .desc = d, .active_low = 1 }
>> > +
>> > +static struct gpio_keys_button gpio_buttons[] = {
>> > +   GPIO_KEY(KEY_4,         RCAR_GP_PIN(1, 28),     "SW2-pin4"),
>> > +   GPIO_KEY(KEY_3,         RCAR_GP_PIN(1, 26),     "SW2-pin3"),
>> > +   GPIO_KEY(KEY_2,         RCAR_GP_PIN(1, 24),     "SW2-pin2"),
>> > +   GPIO_KEY(KEY_1,         RCAR_GP_PIN(1, 14),     "SW2-pin1"),
>> > +};
>> > +
>> > +static struct gpio_keys_platform_data lager_keys_pdata = {
>> > +   .buttons        = gpio_buttons,
>> > +   .nbuttons       = ARRAY_SIZE(gpio_buttons),
>> > +};
>> > +
>>
>> Could you please also update the lager dts file ?
>
> I wonder if in that case we can remove the C code above.
> And probably the C code for the GPIO leds too.
>
> Magnus, could you clarify how you would like
> C-code and DTS to look on the lager board?

I'd like C and DTS to coexist in parallel until we can ditch C fully.
I believe there is a need for us to be able to back port the C code.
For the future we want to use DTS.

To support DTS for Lager I think you should create a -reference DTS
file for Lager where you put the DT version of your GPIO LED / switch
code. At the time when we can configure the PFC via PINCTRL DT then I
believe we should be able to replace most of the platform device code
with DT versions. At that we should hopefully be able to ditch all the
C board code, or maybe we need to keep some around until we have the
common clocks going...

Thanks,

/ magnus

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Simon Horman]

On Tue, May 14, 2013 at 03:09:08PM +0900, Magnus Damm wrote:
> Hi Simon and Laurent,
> 
> On Tue, May 14, 2013 at 11:59 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, May 14, 2013 at 12:35:20AM +0200, Laurent Pinchart wrote:
> >> Hi Simon,
> >>
> >> Thank you for the patch.
> >>
> >> On Monday 13 May 2013 17:53:53 Simon Horman wrote:
> >> > The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
> >> > This patch allows access to those pins as KEYS 1 - 4 using
> >> > gpio-keys.
> >> >
> >> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >> >
> >> > ---
> >> >
> >> > This patch has run time dependencies on
> >> > 1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
> >> > 2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH"
> >> >
> >> > Without those dependencies satisfied the gpio-keys driver will
> >> > fail to be initialise itself. The boot should be otherwise successful
> >> > and the board otherwise functional.
> >> > ---
> >> >  arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
> >> >  1 file changed, 22 insertions(+)
> >> >
> >> > diff --git a/arch/arm/mach-shmobile/board-lager.c
> >> > b/arch/arm/mach-shmobile/board-lager.c index 6a1ba38..4d1b42b 100644
> >> > --- a/arch/arm/mach-shmobile/board-lager.c
> >> > +++ b/arch/arm/mach-shmobile/board-lager.c
> >> > @@ -18,7 +18,10 @@
> >> >   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
> >> > USA */
> >> >
> >> > +#include <linux/gpio.h>
> >> > +#include <linux/gpio_keys.h>
> >> >  #include <linux/interrupt.h>
> >> > +#include <linux/input.h>
> >>
> >> My dictionary lists input before interrupt :-)
> >
> > Thanks, I will fix that.
> >
> >> >  #include <linux/irqchip.h>
> >> >  #include <linux/kernel.h>
> >> >  #include <linux/leds.h>
> >> > @@ -52,6 +55,22 @@ static struct gpio_led_platform_data lager_leds_pdata = {
> >> > .num_leds   = ARRAY_SIZE(lager_leds),
> >> >  };
> >> >
> >> > +/* GPIO KEY */
> >> > +#define GPIO_KEY(c, g, d, ...) \
> >> > +   { .code = c, .gpio = g, .desc = d, .active_low = 1 }
> >> > +
> >> > +static struct gpio_keys_button gpio_buttons[] = {
> >> > +   GPIO_KEY(KEY_4,         RCAR_GP_PIN(1, 28),     "SW2-pin4"),
> >> > +   GPIO_KEY(KEY_3,         RCAR_GP_PIN(1, 26),     "SW2-pin3"),
> >> > +   GPIO_KEY(KEY_2,         RCAR_GP_PIN(1, 24),     "SW2-pin2"),
> >> > +   GPIO_KEY(KEY_1,         RCAR_GP_PIN(1, 14),     "SW2-pin1"),
> >> > +};
> >> > +
> >> > +static struct gpio_keys_platform_data lager_keys_pdata = {
> >> > +   .buttons        = gpio_buttons,
> >> > +   .nbuttons       = ARRAY_SIZE(gpio_buttons),
> >> > +};
> >> > +
> >>
> >> Could you please also update the lager dts file ?
> >
> > I wonder if in that case we can remove the C code above.
> > And probably the C code for the GPIO leds too.
> >
> > Magnus, could you clarify how you would like
> > C-code and DTS to look on the lager board?
> 
> I'd like C and DTS to coexist in parallel until we can ditch C fully.
> I believe there is a need for us to be able to back port the C code.
> For the future we want to use DTS.
> 
> To support DTS for Lager I think you should create a -reference DTS
> file for Lager where you put the DT version of your GPIO LED / switch
> code. At the time when we can configure the PFC via PINCTRL DT then I
> believe we should be able to replace most of the platform device code
> with DT versions. At that we should hopefully be able to ditch all the
> C board code, or maybe we need to keep some around until we have the
> common clocks going...

Hi Magnus,

thanks for the clarification. That all sounds reasonable
and I will see about making it so.

[PATCH 3/3] ARM: mach-shmobile: lager: support GPIO switches

[Laurent Pinchart]

Hi Magnus,

On Tuesday 14 May 2013 15:09:08 Magnus Damm wrote:
> On Tue, May 14, 2013 at 11:59 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, May 14, 2013 at 12:35:20AM +0200, Laurent Pinchart wrote:
> >> On Monday 13 May 2013 17:53:53 Simon Horman wrote:
> >> > The lager board has pins 1 - 4 of SW2 wired up to GPIO pins.
> >> > This patch allows access to those pins as KEYS 1 - 4 using
> >> > gpio-keys.
> >> > 
> >> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >> > 
> >> > ---
> >> > 
> >> > This patch has run time dependencies on
> >> > 1. "gpio-rcar: Add support for IRQ_TYPE_EDGE_BOTH"
> >> > 2. "ARM: shmobile: r8a7790: Configure R-Car GPIO for
> >> > IRQ_TYPE_EDGE_BOTH"
> >> > 
> >> > Without those dependencies satisfied the gpio-keys driver will
> >> > fail to be initialise itself. The boot should be otherwise successful
> >> > and the board otherwise functional.
> >> > ---
> >> > 
> >> >  arch/arm/mach-shmobile/board-lager.c | 22 ++++++++++++++++++++++
> >> >  1 file changed, 22 insertions(+)
> >> > 
> >> > diff --git a/arch/arm/mach-shmobile/board-lager.c
> >> > b/arch/arm/mach-shmobile/board-lager.c index 6a1ba38..4d1b42b 100644
> >> > --- a/arch/arm/mach-shmobile/board-lager.c
> >> > +++ b/arch/arm/mach-shmobile/board-lager.c
> >> > @@ -18,7 +18,10 @@
> >> > 
> >> >   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 
> >> >   02110-1301
> >> > 
> >> > USA */
> >> > 
> >> > +#include <linux/gpio.h>
> >> > +#include <linux/gpio_keys.h>
> >> > 
> >> >  #include <linux/interrupt.h>
> >> > 
> >> > +#include <linux/input.h>
> >> 
> >> My dictionary lists input before interrupt :-)
> > 
> > Thanks, I will fix that.
> > 
> >> >  #include <linux/irqchip.h>
> >> >  #include <linux/kernel.h>
> >> >  #include <linux/leds.h>
> >> > 
> >> > @@ -52,6 +55,22 @@ static struct gpio_led_platform_data
> >> > lager_leds_pdata = { .num_leds   = ARRAY_SIZE(lager_leds),
> >> > 
> >> >  };
> >> > 
> >> > +/* GPIO KEY */
> >> > +#define GPIO_KEY(c, g, d, ...) \
> >> > +   { .code = c, .gpio = g, .desc = d, .active_low = 1 }
> >> > +
> >> > +static struct gpio_keys_button gpio_buttons[] = {
> >> > +   GPIO_KEY(KEY_4,         RCAR_GP_PIN(1, 28),     "SW2-pin4"),
> >> > +   GPIO_KEY(KEY_3,         RCAR_GP_PIN(1, 26),     "SW2-pin3"),
> >> > +   GPIO_KEY(KEY_2,         RCAR_GP_PIN(1, 24),     "SW2-pin2"),
> >> > +   GPIO_KEY(KEY_1,         RCAR_GP_PIN(1, 14),     "SW2-pin1"),
> >> > +};
> >> > +
> >> > +static struct gpio_keys_platform_data lager_keys_pdata = {
> >> > +   .buttons        = gpio_buttons,
> >> > +   .nbuttons       = ARRAY_SIZE(gpio_buttons),
> >> > +};
> >> > +
> >> 
> >> Could you please also update the lager dts file ?
> > 
> > I wonder if in that case we can remove the C code above.
> > And probably the C code for the GPIO leds too.
> > 
> > Magnus, could you clarify how you would like
> > C-code and DTS to look on the lager board?
> 
> I'd like C and DTS to coexist in parallel until we can ditch C fully.
> I believe there is a need for us to be able to back port the C code.
> For the future we want to use DTS.
> 
> To support DTS for Lager I think you should create a -reference DTS
> file for Lager where you put the DT version of your GPIO LED / switch
> code. At the time when we can configure the PFC via PINCTRL DT then I
> believe we should be able to replace most of the platform device code
> with DT versions.

I've sent an RFC for gpio-rcar DT bindings, and I'll send one for sh-pfc in 
the next few days. DT support for GPIO and PINCTRL should then hopefully be 
available very soon.

> At that we should hopefully be able to ditch all the C board code,

Will we also ditch non-reference board files ?

> or maybe we need to keep some around until we have the common clocks
> going...

What's the status of CCF support development in sh-mobile ?

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] ARM: shmobile: r8a7790: Configure R-Car GPIO for IRQ_TYPE_EDGE_BOTH

[Laurent Pinchart]

Hi Magnus,

On Tuesday 14 May 2013 15:01:01 Magnus Damm wrote:
> On Tue, May 14, 2013 at 11:40 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, May 14, 2013 at 12:33:42AM +0200, Laurent Pinchart wrote:
> >> Hi Simon,
> >> 
> >> On Monday 13 May 2013 17:53:52 Simon Horman wrote:
> >> > "gpio-rcar: Support IRQ_TYPE_EDGE_BOTH" adds support to the R-Car GPIO
> >> > driver for IRQ_TYPE_EDGE_BOTH. As hardware support for this feature is
> >> > not universal for all SoCs a flag, has_both_edge_trigger, has been
> >> > added to the platform data of the driver to allow this feature to be
> >> > enabled.
> >> 
> >> What about moving this information to a platform ID table in the
> >> gpio-rcar driver ?
> > 
> > Magnus, do you have an opinion on this?
> 
> I don't mind so much as long as we:
> a) can control things freely per driver instance
> and
> b) don't have to update all drivers for every new SoC
> 
> The current approach allows for that, so as an example if we can have
> two slightly different GPIO hardware blocks then we can setup two GPIO
> controller driver instances where the configuration can be selected
> per-instance. Perhaps Laurent's proposal would allow for this too, I'm
> not sure.

I agree that parameters related to the instantiation of a GPIO IP core in a 
SoC should come from DT. Parameters related to the IP core version (usch as 
the ability to trigger on both edges) should in my opinion be computed by the 
GPIO driver based on the IP core version number.

> On top of that I'd like to avoid doing per-SoC string matching in drivers.
> Those would force us to update every darn driver for every new SoC. It is
> better to try to use a per-hardware device version in the drivers and then
> configure each SoC to use the appropriate version.

Agreed. Are the version numbers for the GPIO IP core available ?

-- 
Regards,

Laurent Pinchart