linux-sh.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
@ 2013-12-19 15:28 Laurent Pinchart
  2013-12-20  7:18 ` Magnus Damm
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Laurent Pinchart @ 2013-12-19 15:28 UTC (permalink / raw)
  To: linux-sh

The Koelsh reference device tree is going away, copy the missing GPIO
keys device node to the Koeslch device tree file.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/boot/dts/r8a7791-koelsch.dts | 54 +++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

Hi Simon,

This patch contains the differences between v4 (which you have queued up) and
v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference DTS". Feel free
to apply it on top of your dt branch, or alternatively rebase the branch to
replace v4 with v5.

diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
index fd556c3..c6f5de3 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -31,6 +31,60 @@
 		#size-cells = <1>;
 	};
 
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		key-a {
+			gpios = <&gpio7 0 GPIO_ACTIVE_LOW>;
+			linux,code = <30>;
+			label = "SW30";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-b {
+			gpios = <&gpio7 1 GPIO_ACTIVE_LOW>;
+			linux,code = <48>;
+			label = "SW31";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-c {
+			gpios = <&gpio7 2 GPIO_ACTIVE_LOW>;
+			linux,code = <46>;
+			label = "SW32";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-d {
+			gpios = <&gpio7 3 GPIO_ACTIVE_LOW>;
+			linux,code = <32>;
+			label = "SW33";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-e {
+			gpios = <&gpio7 4 GPIO_ACTIVE_LOW>;
+			linux,code = <18>;
+			label = "SW34";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-f {
+			gpios = <&gpio7 5 GPIO_ACTIVE_LOW>;
+			linux,code = <33>;
+			label = "SW35";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+		key-g {
+			gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
+			linux,code = <34>;
+			label = "SW36";
+			gpio-key,wakeup;
+			debounce-interval = <20>;
+		};
+	};
+
 	leds {
 		compatible = "gpio-leds";
 		led6 {
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
@ 2013-12-20  7:18 ` Magnus Damm
  2013-12-20 14:24 ` Laurent Pinchart
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Magnus Damm @ 2013-12-20  7:18 UTC (permalink / raw)
  To: linux-sh

On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> The Koelsh reference device tree is going away, copy the missing GPIO
> keys device node to the Koeslch device tree file.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 +++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> Hi Simon,
>
> This patch contains the differences between v4 (which you have queued up) and
> v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference DTS". Feel free
> to apply it on top of your dt branch, or alternatively rebase the branch to
> replace v4 with v5.

Hi Laurent,

Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
then you can see that both have a DIP switch and Koelsch also has a
set of buttons. I may recall wrong, but I think the legacy code
supports both the DIP switch and buttons. This DT reference patch
seems to only add buttons, not the DIP switch. Any plans to include
the DIP switch?

Cheers,

/ magnus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
  2013-12-20  7:18 ` Magnus Damm
@ 2013-12-20 14:24 ` Laurent Pinchart
  2013-12-20 14:40 ` Magnus Damm
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2013-12-20 14:24 UTC (permalink / raw)
  To: linux-sh

Hi Magnus,

On Friday 20 December 2013 16:18:39 Magnus Damm wrote:
> On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart
> 
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
> > The Koelsh reference device tree is going away, copy the missing GPIO
> > keys device node to the Koeslch device tree file.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 ++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > Hi Simon,
> > 
> > This patch contains the differences between v4 (which you have queued up)
> > and v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference DTS".
> > Feel free to apply it on top of your dt branch, or alternatively rebase
> > the branch to replace v4 with v5.
> 
> Hi Laurent,
> 
> Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
> then you can see that both have a DIP switch and Koelsch also has a
> set of buttons. I may recall wrong, but I think the legacy code
> supports both the DIP switch and buttons. This DT reference patch
> seems to only add buttons, not the DIP switch. Any plans to include
> the DIP switch?

I've seen that when implementing the original Koelsh GPIO keys DT support and 
poundered how to handle it properly. It basically boils down the how we define 
a GPIO key. A GPIO is a hardware concept, but a key is defined by its usage 
intent. As the buttons and DIP switches you refer to are just buttons and 
switches without a defined purpose, how to handle them is I believe a policy 
decision.

In the end I've decided to handle the buttons with the GPIO keys driver as I 
believe it makes sense to have keys hooked up to the input subsystem on a 
development board. As the DIP switches are not momentary buttons from a 
hardware point of view I've decided to omit them and let them be used for 
other purposes.

I'm fine with revisiting this decision, but please note that this patch only 
synchronizes the non-reference dts with the reference dts.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
  2013-12-20  7:18 ` Magnus Damm
  2013-12-20 14:24 ` Laurent Pinchart
@ 2013-12-20 14:40 ` Magnus Damm
  2013-12-20 15:18 ` Geert Uytterhoeven
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Magnus Damm @ 2013-12-20 14:40 UTC (permalink / raw)
  To: linux-sh

Hi Laurent,

On Fri, Dec 20, 2013 at 11:24 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Friday 20 December 2013 16:18:39 Magnus Damm wrote:
>> On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart
>>
>> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> > The Koelsh reference device tree is going away, copy the missing GPIO
>> > keys device node to the Koeslch device tree file.
>> >
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---
>> >
>> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 ++++++++++++++++++++++++++++++
>> >  1 file changed, 54 insertions(+)
>> >
>> > Hi Simon,
>> >
>> > This patch contains the differences between v4 (which you have queued up)
>> > and v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference DTS".
>> > Feel free to apply it on top of your dt branch, or alternatively rebase
>> > the branch to replace v4 with v5.
>>
>> Hi Laurent,
>>
>> Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
>> then you can see that both have a DIP switch and Koelsch also has a
>> set of buttons. I may recall wrong, but I think the legacy code
>> supports both the DIP switch and buttons. This DT reference patch
>> seems to only add buttons, not the DIP switch. Any plans to include
>> the DIP switch?
>
> I've seen that when implementing the original Koelsh GPIO keys DT support and
> poundered how to handle it properly. It basically boils down the how we define
> a GPIO key. A GPIO is a hardware concept, but a key is defined by its usage
> intent. As the buttons and DIP switches you refer to are just buttons and
> switches without a defined purpose, how to handle them is I believe a policy
> decision.

I agree. This is IMO the same as for the platform device case, but I
guess there it is more common to mix software policy and hardware
description there. There are also no letters printed on the buttons on
the actual hardware, so assigning something dummy in DT seems kind of
odd too.

> In the end I've decided to handle the buttons with the GPIO keys driver as I
> believe it makes sense to have keys hooked up to the input subsystem on a
> development board. As the DIP switches are not momentary buttons from a
> hardware point of view I've decided to omit them and let them be used for
> other purposes.

What are "other purposes" here if I may ask? =)

> I'm fine with revisiting this decision, but please note that this patch only
> synchronizes the non-reference dts with the reference dts.

How do you mean by synchronize? It seems to half-synchronize to me, see below!

I want to keep the legacy board code and the DT reference stuff in
full sync. I also would like to keep software support for Lager and
Koelsch on a similar level.

As you know, I submitted GPIO code to the Koelsch legacy code earlier,
and I was about to send the DT bits too, but someone else got there
before me. =)

Unless I'm mistaken the legacy Koelsch code contains this:

static struct gpio_keys_button gpio_buttons[] = {
GPIO_KEY(KEY_4, RCAR_GP_PIN(5, 3), "SW2-pin4"),
GPIO_KEY(KEY_3, RCAR_GP_PIN(5, 2), "SW2-pin3"),
GPIO_KEY(KEY_2, RCAR_GP_PIN(5, 1), "SW2-pin2"),
GPIO_KEY(KEY_1, RCAR_GP_PIN(5, 0), "SW2-pin1"),

So if you want to use SW2 for something else, then I think it's
something we should do across both Lager and Koelsch and both legacy
and DT reference code. Just being special in one place seems odd.

Please note that in the Lager case we only have DIP switches, so
waking up from Suspend-to-RAM via GPIO can only be done via the DIP
switch. Because of that I prefer to allow wakeup via DIP switch in a
consistent way for both Lager and Koelsch. Did you have any special
idea how to interface to the DIP switches so we can wake up from
Suspend-to-RAM? =)

Cheers,

/ magnus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
                   ` (2 preceding siblings ...)
  2013-12-20 14:40 ` Magnus Damm
@ 2013-12-20 15:18 ` Geert Uytterhoeven
  2013-12-20 22:46 ` Laurent Pinchart
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2013-12-20 15:18 UTC (permalink / raw)
  To: linux-sh

On Fri, Dec 20, 2013 at 3:40 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> I've seen that when implementing the original Koelsh GPIO keys DT support and
>> poundered how to handle it properly. It basically boils down the how we define
>> a GPIO key. A GPIO is a hardware concept, but a key is defined by its usage
>> intent. As the buttons and DIP switches you refer to are just buttons and
>> switches without a defined purpose, how to handle them is I believe a policy
>> decision.

> Please note that in the Lager case we only have DIP switches, so
> waking up from Suspend-to-RAM via GPIO can only be done via the DIP
> switch. Because of that I prefer to allow wakeup via DIP switch in a
> consistent way for both Lager and Koelsch. Did you have any special
> idea how to interface to the DIP switches so we can wake up from
> Suspend-to-RAM? =)

Can't you handle the wakeup key mapping from the DT, as this is a usage
intent? E.g. through a board-specific alias, or through chosen.
Documentation/devicetree/bindings/input/input-reset.txt shows an example
through chosen.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
                   ` (3 preceding siblings ...)
  2013-12-20 15:18 ` Geert Uytterhoeven
@ 2013-12-20 22:46 ` Laurent Pinchart
  2013-12-23  0:45 ` Simon Horman
  2014-01-06  7:56 ` Simon Horman
  6 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2013-12-20 22:46 UTC (permalink / raw)
  To: linux-sh

Hi Magnus,

On Friday 20 December 2013 23:40:19 Magnus Damm wrote:
> On Fri, Dec 20, 2013 at 11:24 PM, Laurent Pinchart wrote:
> > On Friday 20 December 2013 16:18:39 Magnus Damm wrote:
> >> On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart wrote:
> >> > The Koelsh reference device tree is going away, copy the missing GPIO
> >> > keys device node to the Koeslch device tree file.
> >> > 
> >> > Signed-off-by: Laurent Pinchart
> >> > <laurent.pinchart+renesas@ideasonboard.com>
> >> > ---
> >> > 
> >> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 +++++++++++++++++++++++++++
> >> >  1 file changed, 54 insertions(+)
> >> > 
> >> > Hi Simon,
> >> > 
> >> > This patch contains the differences between v4 (which you have queued
> >> > up) and v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference
> >> > DTS".> Feel free to apply it on top of your dt branch, or alternatively
> >> > rebase the branch to replace v4 with v5.
> >> 
> >> Hi Laurent,
> >> 
> >> Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
> >> then you can see that both have a DIP switch and Koelsch also has a
> >> set of buttons. I may recall wrong, but I think the legacy code
> >> supports both the DIP switch and buttons. This DT reference patch
> >> seems to only add buttons, not the DIP switch. Any plans to include
> >> the DIP switch?
> > 
> > I've seen that when implementing the original Koelsh GPIO keys DT support
> > and poundered how to handle it properly. It basically boils down the how
> > we define a GPIO key. A GPIO is a hardware concept, but a key is defined
> > by its usage intent. As the buttons and DIP switches you refer to are
> > just buttons and switches without a defined purpose, how to handle them
> > is I believe a policy decision.
> 
> I agree. This is IMO the same as for the platform device case, but I guess
> there it is more common to mix software policy and hardware description
> there. There are also no letters printed on the buttons on the actual
> hardware, so assigning something dummy in DT seems kind of odd too.
> 
> > In the end I've decided to handle the buttons with the GPIO keys driver as
> > I believe it makes sense to have keys hooked up to the input subsystem on
> > a development board. As the DIP switches are not momentary buttons from a
> > hardware point of view I've decided to omit them and let them be used for
> > other purposes.
> 
> What are "other purposes" here if I may ask? =)

Boot configuration is one possibility. Basically anything that the GPIO API 
can expose to kernelspace and/or userspace. Exposing GPIOs as GPIOs is 
probably useful on a development board the same way exposing them as keys is.

> > I'm fine with revisiting this decision, but please note that this patch
> > only synchronizes the non-reference dts with the reference dts.
> 
> How do you mean by synchronize? It seems to half-synchronize to me, see
> below!

This patch is about synchronizing r8a7791-koelsch.dts with r8a7791-koelsch-
reference.dts, as r8a7791-koelsch-reference.dts will be removed. I'm not 
opposed to adding keys for the DIP switch to r8a7791-koelsch.dts as a second 
step.

> I want to keep the legacy board code and the DT reference stuff in full
> sync. I also would like to keep software support for Lager and Koelsch on a
> similar level.
> 
> As you know, I submitted GPIO code to the Koelsch legacy code earlier, and I
> was about to send the DT bits too, but someone else got there before me. =)
> 
> Unless I'm mistaken the legacy Koelsch code contains this:
> 
> static struct gpio_keys_button gpio_buttons[] = {
> GPIO_KEY(KEY_4, RCAR_GP_PIN(5, 3), "SW2-pin4"),
> GPIO_KEY(KEY_3, RCAR_GP_PIN(5, 2), "SW2-pin3"),
> GPIO_KEY(KEY_2, RCAR_GP_PIN(5, 1), "SW2-pin2"),
> GPIO_KEY(KEY_1, RCAR_GP_PIN(5, 0), "SW2-pin1"),
> 
> So if you want to use SW2 for something else, then I think it's something we
> should do across both Lager and Koelsch and both legacy and DT reference
> code. Just being special in one place seems odd.

Lager and Koelsch are two different boards so I don't think they should have 
the same features set just for the sake of it, but that doesn't mean I'm 
opposed to adding keys for the DIP switch on Koelsch.

> Please note that in the Lager case we only have DIP switches, so waking up
> from Suspend-to-RAM via GPIO can only be done via the DIP switch.

Doesn't the reset button wake up the CPU ? :-p

> Because of that I prefer to allow wakeup via DIP switch in a consistent way
> for both Lager and Koelsch. Did you have any special idea how to interface
> to the DIP switches so we can wake up from Suspend-to-RAM? =)

That's a good question. Aren't there use cases where we want a button to wake 
up the system, but without exposing it as a key ?

Anyway, I'm not opposed to adding keys for the DIP switch, but I'd rather do 
it on top of this patch series.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
                   ` (4 preceding siblings ...)
  2013-12-20 22:46 ` Laurent Pinchart
@ 2013-12-23  0:45 ` Simon Horman
  2014-01-06  7:56 ` Simon Horman
  6 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2013-12-23  0:45 UTC (permalink / raw)
  To: linux-sh

On Fri, Dec 20, 2013 at 11:46:10PM +0100, Laurent Pinchart wrote:
> Hi Magnus,
> 
> On Friday 20 December 2013 23:40:19 Magnus Damm wrote:
> > On Fri, Dec 20, 2013 at 11:24 PM, Laurent Pinchart wrote:
> > > On Friday 20 December 2013 16:18:39 Magnus Damm wrote:
> > >> On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart wrote:
> > >> > The Koelsh reference device tree is going away, copy the missing GPIO
> > >> > keys device node to the Koeslch device tree file.
> > >> > 
> > >> > Signed-off-by: Laurent Pinchart
> > >> > <laurent.pinchart+renesas@ideasonboard.com>
> > >> > ---
> > >> > 
> > >> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 +++++++++++++++++++++++++++
> > >> >  1 file changed, 54 insertions(+)
> > >> > 
> > >> > Hi Simon,
> > >> > 
> > >> > This patch contains the differences between v4 (which you have queued
> > >> > up) and v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference
> > >> > DTS".> Feel free to apply it on top of your dt branch, or alternatively
> > >> > rebase the branch to replace v4 with v5.
> > >> 
> > >> Hi Laurent,
> > >> 
> > >> Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
> > >> then you can see that both have a DIP switch and Koelsch also has a
> > >> set of buttons. I may recall wrong, but I think the legacy code
> > >> supports both the DIP switch and buttons. This DT reference patch
> > >> seems to only add buttons, not the DIP switch. Any plans to include
> > >> the DIP switch?
> > > 
> > > I've seen that when implementing the original Koelsh GPIO keys DT support
> > > and poundered how to handle it properly. It basically boils down the how
> > > we define a GPIO key. A GPIO is a hardware concept, but a key is defined
> > > by its usage intent. As the buttons and DIP switches you refer to are
> > > just buttons and switches without a defined purpose, how to handle them
> > > is I believe a policy decision.
> > 
> > I agree. This is IMO the same as for the platform device case, but I guess
> > there it is more common to mix software policy and hardware description
> > there. There are also no letters printed on the buttons on the actual
> > hardware, so assigning something dummy in DT seems kind of odd too.
> > 
> > > In the end I've decided to handle the buttons with the GPIO keys driver as
> > > I believe it makes sense to have keys hooked up to the input subsystem on
> > > a development board. As the DIP switches are not momentary buttons from a
> > > hardware point of view I've decided to omit them and let them be used for
> > > other purposes.
> > 
> > What are "other purposes" here if I may ask? =)
> 
> Boot configuration is one possibility. Basically anything that the GPIO API 
> can expose to kernelspace and/or userspace. Exposing GPIOs as GPIOs is 
> probably useful on a development board the same way exposing them as keys is.
> 
> > > I'm fine with revisiting this decision, but please note that this patch
> > > only synchronizes the non-reference dts with the reference dts.
> > 
> > How do you mean by synchronize? It seems to half-synchronize to me, see
> > below!
> 
> This patch is about synchronizing r8a7791-koelsch.dts with r8a7791-koelsch-
> reference.dts, as r8a7791-koelsch-reference.dts will be removed. I'm not 
> opposed to adding keys for the DIP switch to r8a7791-koelsch.dts as a second 
> step.
> 
> > I want to keep the legacy board code and the DT reference stuff in full
> > sync. I also would like to keep software support for Lager and Koelsch on a
> > similar level.
> > 
> > As you know, I submitted GPIO code to the Koelsch legacy code earlier, and I
> > was about to send the DT bits too, but someone else got there before me. =)
> > 
> > Unless I'm mistaken the legacy Koelsch code contains this:
> > 
> > static struct gpio_keys_button gpio_buttons[] = {
> > GPIO_KEY(KEY_4, RCAR_GP_PIN(5, 3), "SW2-pin4"),
> > GPIO_KEY(KEY_3, RCAR_GP_PIN(5, 2), "SW2-pin3"),
> > GPIO_KEY(KEY_2, RCAR_GP_PIN(5, 1), "SW2-pin2"),
> > GPIO_KEY(KEY_1, RCAR_GP_PIN(5, 0), "SW2-pin1"),
> > 
> > So if you want to use SW2 for something else, then I think it's something we
> > should do across both Lager and Koelsch and both legacy and DT reference
> > code. Just being special in one place seems odd.
> 
> Lager and Koelsch are two different boards so I don't think they should have 
> the same features set just for the sake of it, but that doesn't mean I'm 
> opposed to adding keys for the DIP switch on Koelsch.
> 
> > Please note that in the Lager case we only have DIP switches, so waking up
> > from Suspend-to-RAM via GPIO can only be done via the DIP switch.
> 
> Doesn't the reset button wake up the CPU ? :-p
> 
> > Because of that I prefer to allow wakeup via DIP switch in a consistent way
> > for both Lager and Koelsch. Did you have any special idea how to interface
> > to the DIP switches so we can wake up from Suspend-to-RAM? =)
> 
> That's a good question. Aren't there use cases where we want a button to wake 
> up the system, but without exposing it as a key ?
> 
> Anyway, I'm not opposed to adding keys for the DIP switch, but I'd rather do 
> it on top of this patch series.

My understanding is that this patch merely moves code that
was (the file has now been removed by the subsequent patch,
which I have queued up) from r8a7791-koelsch-reference.dts to
r8a7791-koelsch.dts.

With this in mind I think that this patch is entirely reasonable in
its current form and that further enablement can be added on top.

Are there any objections to me queuing up this patch?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS
  2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
                   ` (5 preceding siblings ...)
  2013-12-23  0:45 ` Simon Horman
@ 2014-01-06  7:56 ` Simon Horman
  6 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2014-01-06  7:56 UTC (permalink / raw)
  To: linux-sh

On Mon, Dec 23, 2013 at 09:45:17AM +0900, Simon Horman wrote:
> On Fri, Dec 20, 2013 at 11:46:10PM +0100, Laurent Pinchart wrote:
> > Hi Magnus,
> > 
> > On Friday 20 December 2013 23:40:19 Magnus Damm wrote:
> > > On Fri, Dec 20, 2013 at 11:24 PM, Laurent Pinchart wrote:
> > > > On Friday 20 December 2013 16:18:39 Magnus Damm wrote:
> > > >> On Fri, Dec 20, 2013 at 12:28 AM, Laurent Pinchart wrote:
> > > >> > The Koelsh reference device tree is going away, copy the missing GPIO
> > > >> > keys device node to the Koeslch device tree file.
> > > >> > 
> > > >> > Signed-off-by: Laurent Pinchart
> > > >> > <laurent.pinchart+renesas@ideasonboard.com>
> > > >> > ---
> > > >> > 
> > > >> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 54 +++++++++++++++++++++++++++
> > > >> >  1 file changed, 54 insertions(+)
> > > >> > 
> > > >> > Hi Simon,
> > > >> > 
> > > >> > This patch contains the differences between v4 (which you have queued
> > > >> > up) and v5 of "ARM: shmobile: Sync Koelsch DTS with Koelsch reference
> > > >> > DTS".> Feel free to apply it on top of your dt branch, or alternatively
> > > >> > rebase the branch to replace v4 with v5.
> > > >> 
> > > >> Hi Laurent,
> > > >> 
> > > >> Thanks for this patch. If you compare GPIO-keys on Koelsch and Lager
> > > >> then you can see that both have a DIP switch and Koelsch also has a
> > > >> set of buttons. I may recall wrong, but I think the legacy code
> > > >> supports both the DIP switch and buttons. This DT reference patch
> > > >> seems to only add buttons, not the DIP switch. Any plans to include
> > > >> the DIP switch?
> > > > 
> > > > I've seen that when implementing the original Koelsh GPIO keys DT support
> > > > and poundered how to handle it properly. It basically boils down the how
> > > > we define a GPIO key. A GPIO is a hardware concept, but a key is defined
> > > > by its usage intent. As the buttons and DIP switches you refer to are
> > > > just buttons and switches without a defined purpose, how to handle them
> > > > is I believe a policy decision.
> > > 
> > > I agree. This is IMO the same as for the platform device case, but I guess
> > > there it is more common to mix software policy and hardware description
> > > there. There are also no letters printed on the buttons on the actual
> > > hardware, so assigning something dummy in DT seems kind of odd too.
> > > 
> > > > In the end I've decided to handle the buttons with the GPIO keys driver as
> > > > I believe it makes sense to have keys hooked up to the input subsystem on
> > > > a development board. As the DIP switches are not momentary buttons from a
> > > > hardware point of view I've decided to omit them and let them be used for
> > > > other purposes.
> > > 
> > > What are "other purposes" here if I may ask? =)
> > 
> > Boot configuration is one possibility. Basically anything that the GPIO API 
> > can expose to kernelspace and/or userspace. Exposing GPIOs as GPIOs is 
> > probably useful on a development board the same way exposing them as keys is.
> > 
> > > > I'm fine with revisiting this decision, but please note that this patch
> > > > only synchronizes the non-reference dts with the reference dts.
> > > 
> > > How do you mean by synchronize? It seems to half-synchronize to me, see
> > > below!
> > 
> > This patch is about synchronizing r8a7791-koelsch.dts with r8a7791-koelsch-
> > reference.dts, as r8a7791-koelsch-reference.dts will be removed. I'm not 
> > opposed to adding keys for the DIP switch to r8a7791-koelsch.dts as a second 
> > step.
> > 
> > > I want to keep the legacy board code and the DT reference stuff in full
> > > sync. I also would like to keep software support for Lager and Koelsch on a
> > > similar level.
> > > 
> > > As you know, I submitted GPIO code to the Koelsch legacy code earlier, and I
> > > was about to send the DT bits too, but someone else got there before me. =)
> > > 
> > > Unless I'm mistaken the legacy Koelsch code contains this:
> > > 
> > > static struct gpio_keys_button gpio_buttons[] = {
> > > GPIO_KEY(KEY_4, RCAR_GP_PIN(5, 3), "SW2-pin4"),
> > > GPIO_KEY(KEY_3, RCAR_GP_PIN(5, 2), "SW2-pin3"),
> > > GPIO_KEY(KEY_2, RCAR_GP_PIN(5, 1), "SW2-pin2"),
> > > GPIO_KEY(KEY_1, RCAR_GP_PIN(5, 0), "SW2-pin1"),
> > > 
> > > So if you want to use SW2 for something else, then I think it's something we
> > > should do across both Lager and Koelsch and both legacy and DT reference
> > > code. Just being special in one place seems odd.
> > 
> > Lager and Koelsch are two different boards so I don't think they should have 
> > the same features set just for the sake of it, but that doesn't mean I'm 
> > opposed to adding keys for the DIP switch on Koelsch.
> > 
> > > Please note that in the Lager case we only have DIP switches, so waking up
> > > from Suspend-to-RAM via GPIO can only be done via the DIP switch.
> > 
> > Doesn't the reset button wake up the CPU ? :-p
> > 
> > > Because of that I prefer to allow wakeup via DIP switch in a consistent way
> > > for both Lager and Koelsch. Did you have any special idea how to interface
> > > to the DIP switches so we can wake up from Suspend-to-RAM? =)
> > 
> > That's a good question. Aren't there use cases where we want a button to wake 
> > up the system, but without exposing it as a key ?
> > 
> > Anyway, I'm not opposed to adding keys for the DIP switch, but I'd rather do 
> > it on top of this patch series.
> 
> My understanding is that this patch merely moves code that
> was (the file has now been removed by the subsequent patch,
> which I have queued up) from r8a7791-koelsch-reference.dts to
> r8a7791-koelsch.dts.
> 
> With this in mind I think that this patch is entirely reasonable in
> its current form and that further enablement can be added on top.
> 
> Are there any objections to me queuing up this patch?

I will queue this up.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-01-06  7:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-19 15:28 [PATCH v6 15.1/17] ARM: shmobile: Add GPIO keys to Koelsch DTS Laurent Pinchart
2013-12-20  7:18 ` Magnus Damm
2013-12-20 14:24 ` Laurent Pinchart
2013-12-20 14:40 ` Magnus Damm
2013-12-20 15:18 ` Geert Uytterhoeven
2013-12-20 22:46 ` Laurent Pinchart
2013-12-23  0:45 ` Simon Horman
2014-01-06  7:56 ` Simon Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).