[PATCH 1/2] Input: Add device tree bindings for input keys

[PATCH 1/2] Input: Add device tree bindings for input keys

[mathieu.poirier]

From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>

This adds a simple device tree binding for the specification of
key sequences.  Definition of the keys found in the sequence are
located in 'include/uapi/linux/input.h'.

A keyset can be applied to a single device or the system as a
whole, depending on the associated driver.  An extention is also
provided for the definition of multiple keysets.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../devicetree/bindings/input/input-keyset.txt     |   78 ++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/input-keyset.txt

diff --git a/Documentation/devicetree/bindings/input/input-keyset.txt b/Documentation/devicetree/bindings/input/input-keyset.txt
new file mode 100644
index 0000000..4f96299
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/input-keyset.txt
@@ -0,0 +1,78 @@
+Input: keyset
+
+A simple binding to represent a set of keys as described in
+include/uapi/linux/input.h.  This is targeted at devices that want
+to react to certain key combination being pressed or need to
+perform some configuration based on a set of input keys.
+
+It can also be used in a scenario where the system has a whole
+needs to respond to a specific sequence of keys.
+
+Required properties:
+ - linux,input-keyset: An array of 1-cell entries representing the
+   values associated with the KEY_xyz #defines found in input.h.
+
+Example1:
+Applicable to a specific device:
+
+        matrix-keypad {
+                compatible = "gpio-matrix-keypad";
+                debounce-delay-ms = <5>;
+                col-scan-delay-us = <2>;
+
+                row-gpios = <&gpio2 25 0
+                             &gpio2 26 0
+                             &gpio2 27 0>;
+
+                col-gpios = <&gpio2 21 0
+                             &gpio2 22 0
+                             &gpio2 23 0>;
+
+                linux,keymap = <0x00000002
+                                0x01000005
+                                0x02000008
+                                0x00010003
+                                0x01010006
+                                0x02010009
+                                0x00020004
+                                0x01020007
+                                0x0202000a>;
+
+                linux,keyset = <0x04
+                                0x05
+                                0x0a>;
+        };
+
+Example2:
+Used as a system-wide parameter:
+
+	sysrq {
+		linux,input-keyset = <0x01
+				      0x0A
+				      0x19>;
+		timout-ms = <3000>;
+	};
+
+Would represent KEY_1, KEY_9 and KEY_P.
+
+Example3:
+Binding used when multiple declarations are needed:
+
+acme_keysets {
+        keyset0 {
+                linux,input-keyset = <0x01
+                                      0x02
+                                      0x03>;
+        };
+        keyset1 {
+                linux,input-keyset = <0x04
+                                      0x05
+                                      0x06>;
+        };
+        keyset2 {
+                linux,input-keyset = <0x07
+                                      0x08
+                                      0x09>;
+        };
+
+};
-- 
1.7.9.5

[PATCH 2/2] Input: Adding DT support for keyreset tuneables

[mathieu.poirier]

From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>

This patch adds the possibility to get the keyreset and timeout
values from the device tree.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index b51c154..91d081c 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -44,6 +44,7 @@
 #include <linux/uaccess.h>
 #include <linux/moduleparam.h>
 #include <linux/jiffies.h>
+#include <linux/of.h>
 
 #include <asm/ptrace.h>
 #include <asm/irq_regs.h>
@@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
 	}
 }
 
+static void sysrq_of_get_keyreset_config(void)
+{
+	unsigned short key;
+	struct device_node *np;
+	const struct property *prop;
+	const __be32 *val;
+	int count, i;
+
+	np = of_find_node_by_path("/sysrq");
+	if (!np) {
+		pr_info("No sysrq node found");
+		goto out;
+	}
+
+	prop = of_find_property(np, "linux,input-keyset", NULL);
+	if (!prop || !prop->value) {
+		pr_err("Invalid input-keyset");
+		goto out;
+	}
+
+	count = prop->length / sizeof(u32);
+	val = prop->value;
+
+	if (count > SYSRQ_KEY_RESET_MAX)
+		count = SYSRQ_KEY_RESET_MAX;
+
+	/* reset in case a __weak definition was present */
+	sysrq_reset_seq_len = 0;
+
+	for (i = 0; i < count; i++) {
+		key = (unsigned short)be32_to_cpup(val++);
+		if (key == KEY_RESERVED || key > KEY_MAX)
+			break;
+
+		sysrq_reset_seq[sysrq_reset_seq_len++] = key;
+	}
+
+	/* get reset timeout if any */
+	of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
+
+ out:
+	;
+}
+
 static void sysrq_reinject_alt_sysrq(struct work_struct *work)
 {
 	struct sysrq_state *sysrq =
@@ -688,6 +733,7 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work)
 		input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1);
 		input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
 
+
 		input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0);
 		input_inject_event(handle, EV_KEY, alt_code, 0);
 		input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
@@ -903,6 +949,7 @@ static inline void sysrq_register_handler(void)
 	int error;
 	int i;
 
+	/* first check if a __weak interface was instantiated */
 	for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) {
 		key = platform_sysrq_reset_seq[i];
 		if (key == KEY_RESERVED || key > KEY_MAX)
@@ -911,6 +958,13 @@ static inline void sysrq_register_handler(void)
 		sysrq_reset_seq[sysrq_reset_seq_len++] = key;
 	}
 
+	/*
+	 * DT configuration takes precedence over anything
+	 * that would have been defined via the __weak
+	 * interface
+	 */
+	sysrq_of_get_keyreset_config();
+
 	error = input_register_handler(&sysrq_handler);
 	if (error)
 		pr_err("Failed to register input handler, error %d", error);
-- 
1.7.9.5

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

Hi Mathieu,

On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> 
> This patch adds the possibility to get the keyreset and timeout
> values from the device tree.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index b51c154..91d081c 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -44,6 +44,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/moduleparam.h>
>  #include <linux/jiffies.h>
> +#include <linux/of.h>
>  
>  #include <asm/ptrace.h>
>  #include <asm/irq_regs.h>
> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
>  	}
>  }
>  
> +static void sysrq_of_get_keyreset_config(void)
> +{
> +	unsigned short key;
> +	struct device_node *np;
> +	const struct property *prop;
> +	const __be32 *val;
> +	int count, i;
> +
> +	np = of_find_node_by_path("/sysrq");
> +	if (!np) {
> +		pr_info("No sysrq node found");

I do not think this should be an info as majority would not have it
defined I think.

> +		goto out;
> +	}
> +
> +	prop = of_find_property(np, "linux,input-keyset", NULL);

Maybe "linux,input-key*re*set"?

> +	if (!prop || !prop->value) {
> +		pr_err("Invalid input-keyset");
> +		goto out;
> +	}
> +
> +	count = prop->length / sizeof(u32);
> +	val = prop->value;
> +
> +	if (count > SYSRQ_KEY_RESET_MAX)
> +		count = SYSRQ_KEY_RESET_MAX;
> +
> +	/* reset in case a __weak definition was present */
> +	sysrq_reset_seq_len = 0;

Hmm, my preference for ordering would be software over firmware, so that
user could override firmware data, if needed.

Please also add the documenation describing the binding.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Thu, Jun 27, 2013 at 09:28:20AM -0700, Dmitry Torokhov wrote:
> Hi Mathieu,
> 
> On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
> > From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> > 
> > This patch adds the possibility to get the keyreset and timeout
> > values from the device tree.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index b51c154..91d081c 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -44,6 +44,7 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/jiffies.h>
> > +#include <linux/of.h>
> >  
> >  #include <asm/ptrace.h>
> >  #include <asm/irq_regs.h>
> > @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> >  	}
> >  }
> >  
> > +static void sysrq_of_get_keyreset_config(void)
> > +{
> > +	unsigned short key;
> > +	struct device_node *np;
> > +	const struct property *prop;
> > +	const __be32 *val;
> > +	int count, i;
> > +
> > +	np = of_find_node_by_path("/sysrq");
> > +	if (!np) {
> > +		pr_info("No sysrq node found");
> 
> I do not think this should be an info as majority would not have it
> defined I think.
> 
> > +		goto out;
> > +	}
> > +
> > +	prop = of_find_property(np, "linux,input-keyset", NULL);
> 
> Maybe "linux,input-key*re*set"?
> 
> > +	if (!prop || !prop->value) {
> > +		pr_err("Invalid input-keyset");
> > +		goto out;
> > +	}
> > +
> > +	count = prop->length / sizeof(u32);
> > +	val = prop->value;
> > +
> > +	if (count > SYSRQ_KEY_RESET_MAX)
> > +		count = SYSRQ_KEY_RESET_MAX;
> > +
> > +	/* reset in case a __weak definition was present */
> > +	sysrq_reset_seq_len = 0;
> 
> Hmm, my preference for ordering would be software over firmware, so that
> user could override firmware data, if needed.
> 
> Please also add the documenation describing the binding.

Ah, I see the other patch now. I'd just combine the two.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Mathieu Poirier]

On 13-06-27 10:28 AM, Dmitry Torokhov wrote:
> Hi Mathieu,
> 
> On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>
>> This patch adds the possibility to get the keyreset and timeout
>> values from the device tree.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
>> index b51c154..91d081c 100644
>> --- a/drivers/tty/sysrq.c
>> +++ b/drivers/tty/sysrq.c
>> @@ -44,6 +44,7 @@
>>  #include <linux/uaccess.h>
>>  #include <linux/moduleparam.h>
>>  #include <linux/jiffies.h>
>> +#include <linux/of.h>
>>  
>>  #include <asm/ptrace.h>
>>  #include <asm/irq_regs.h>
>> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
>>  	}
>>  }
>>  
>> +static void sysrq_of_get_keyreset_config(void)
>> +{
>> +	unsigned short key;
>> +	struct device_node *np;
>> +	const struct property *prop;
>> +	const __be32 *val;
>> +	int count, i;
>> +
>> +	np = of_find_node_by_path("/sysrq");
>> +	if (!np) {
>> +		pr_info("No sysrq node found");
> 
> I do not think this should be an info as majority would not have it
> defined I think.

I fail to understand your point - could you please be more specific ?

> 
>> +		goto out;
>> +	}
>> +
>> +	prop = of_find_property(np, "linux,input-keyset", NULL);
> 
> Maybe "linux,input-key*re*set"?

I do not agree.  We want the binding to be generic and not tied
specifically to the keyreset functionality.  As such 'input-keyset' or
'input-keychord' are more appropriate.

> 
>> +	if (!prop || !prop->value) {
>> +		pr_err("Invalid input-keyset");
>> +		goto out;
>> +	}
>> +
>> +	count = prop->length / sizeof(u32);
>> +	val = prop->value;
>> +
>> +	if (count > SYSRQ_KEY_RESET_MAX)
>> +		count = SYSRQ_KEY_RESET_MAX;
>> +
>> +	/* reset in case a __weak definition was present */
>> +	sysrq_reset_seq_len = 0;
> 
> Hmm, my preference for ordering would be software over firmware, so that
> user could override firmware data, if needed.

The idea is to offer flexibility.  The same kernel can be used on
multiple device.  As such DT information should be prioritised over a
__weak symbol, otherwise it defeats the purpose.

Once the system is loaded user can still configure the keyreset
specifics by using the /sysfs interface.

> 
> Please also add the documenation describing the binding.

The documentation describing the binding is in patch 1/2.  You suggest
that I add the documentation in this patch too ?

> 
> Thanks.
> 

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Thu, Jun 27, 2013 at 11:56:37AM -0600, Mathieu Poirier wrote:
> On 13-06-27 10:28 AM, Dmitry Torokhov wrote:
> > Hi Mathieu,
> > 
> > On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
> >> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> >>
> >> This patch adds the possibility to get the keyreset and timeout
> >> values from the device tree.
> >>
> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> ---
> >>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 54 insertions(+)
> >>
> >> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> >> index b51c154..91d081c 100644
> >> --- a/drivers/tty/sysrq.c
> >> +++ b/drivers/tty/sysrq.c
> >> @@ -44,6 +44,7 @@
> >>  #include <linux/uaccess.h>
> >>  #include <linux/moduleparam.h>
> >>  #include <linux/jiffies.h>
> >> +#include <linux/of.h>
> >>  
> >>  #include <asm/ptrace.h>
> >>  #include <asm/irq_regs.h>
> >> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> >>  	}
> >>  }
> >>  
> >> +static void sysrq_of_get_keyreset_config(void)
> >> +{
> >> +	unsigned short key;
> >> +	struct device_node *np;
> >> +	const struct property *prop;
> >> +	const __be32 *val;
> >> +	int count, i;
> >> +
> >> +	np = of_find_node_by_path("/sysrq");
> >> +	if (!np) {
> >> +		pr_info("No sysrq node found");
> > 
> > I do not think this should be an info as majority would not have it
> > defined I think.
> 
> I fail to understand your point - could you please be more specific ?

pr_info() will clutter everyone's dmesg because I expect majority of
installs will not have this enabled. Please change to pr_debug or just
drop it.

> 
> > 
> >> +		goto out;
> >> +	}
> >> +
> >> +	prop = of_find_property(np, "linux,input-keyset", NULL);
> > 
> > Maybe "linux,input-key*re*set"?
> 
> I do not agree.  We want the binding to be generic and not tied
> specifically to the keyreset functionality.  As such 'input-keyset' or
> 'input-keychord' are more appropriate.

The binding is defined specifically for sysrq and specifically to
perform reset action.

> 
> > 
> >> +	if (!prop || !prop->value) {
> >> +		pr_err("Invalid input-keyset");
> >> +		goto out;
> >> +	}
> >> +
> >> +	count = prop->length / sizeof(u32);
> >> +	val = prop->value;
> >> +
> >> +	if (count > SYSRQ_KEY_RESET_MAX)
> >> +		count = SYSRQ_KEY_RESET_MAX;
> >> +
> >> +	/* reset in case a __weak definition was present */
> >> +	sysrq_reset_seq_len = 0;
> > 
> > Hmm, my preference for ordering would be software over firmware, so that
> > user could override firmware data, if needed.
> 
> The idea is to offer flexibility.  The same kernel can be used on
> multiple device.  As such DT information should be prioritised over a
> __weak symbol, otherwise it defeats the purpose.

The weak symbol should defined in board data, so it won't get picked up
on multiple boards. But I guess I do not care that much as indeed we can
change the sequence from userspace so we won't be stuck with firmware
choices.

> 
> Once the system is loaded user can still configure the keyreset
> specifics by using the /sysfs interface.
> 
> > 
> > Please also add the documenation describing the binding.
> 
> The documentation describing the binding is in patch 1/2.  You suggest
> that I add the documentation in this patch too ?

I simply missed your other patch as it hit my mailbox after this one.
But I would just combine the 2 together.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Mathieu Poirier]

On 13-06-27 12:25 PM, Dmitry Torokhov wrote:
> On Thu, Jun 27, 2013 at 11:56:37AM -0600, Mathieu Poirier wrote:
>> On 13-06-27 10:28 AM, Dmitry Torokhov wrote:
>>> Hi Mathieu,
>>>
>>> On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
>>>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>>>
>>>> This patch adds the possibility to get the keyreset and timeout
>>>> values from the device tree.
>>>>
>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> ---
>>>>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 54 insertions(+)
>>>>
>>>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
>>>> index b51c154..91d081c 100644
>>>> --- a/drivers/tty/sysrq.c
>>>> +++ b/drivers/tty/sysrq.c
>>>> @@ -44,6 +44,7 @@
>>>>  #include <linux/uaccess.h>
>>>>  #include <linux/moduleparam.h>
>>>>  #include <linux/jiffies.h>
>>>> +#include <linux/of.h>
>>>>  
>>>>  #include <asm/ptrace.h>
>>>>  #include <asm/irq_regs.h>
>>>> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
>>>>  	}
>>>>  }
>>>>  
>>>> +static void sysrq_of_get_keyreset_config(void)
>>>> +{
>>>> +	unsigned short key;
>>>> +	struct device_node *np;
>>>> +	const struct property *prop;
>>>> +	const __be32 *val;
>>>> +	int count, i;
>>>> +
>>>> +	np = of_find_node_by_path("/sysrq");
>>>> +	if (!np) {
>>>> +		pr_info("No sysrq node found");
>>>
>>> I do not think this should be an info as majority would not have it
>>> defined I think.
>>
>> I fail to understand your point - could you please be more specific ?
> 
> pr_info() will clutter everyone's dmesg because I expect majority of
> installs will not have this enabled. Please change to pr_debug or just
> drop it.

Ah! Yes certainly.

> 
>>
>>>
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	prop = of_find_property(np, "linux,input-keyset", NULL);
>>>
>>> Maybe "linux,input-key*re*set"?
>>
>> I do not agree.  We want the binding to be generic and not tied
>> specifically to the keyreset functionality.  As such 'input-keyset' or
>> 'input-keychord' are more appropriate.
> 
> The binding is defined specifically for sysrq and specifically to
> perform reset action.

Yes for now but as the examples in the binding show, it is easy to
envision how other drivers could use it.

> 
>>
>>>
>>>> +	if (!prop || !prop->value) {
>>>> +		pr_err("Invalid input-keyset");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	count = prop->length / sizeof(u32);
>>>> +	val = prop->value;
>>>> +
>>>> +	if (count > SYSRQ_KEY_RESET_MAX)
>>>> +		count = SYSRQ_KEY_RESET_MAX;
>>>> +
>>>> +	/* reset in case a __weak definition was present */
>>>> +	sysrq_reset_seq_len = 0;
>>>
>>> Hmm, my preference for ordering would be software over firmware, so that
>>> user could override firmware data, if needed.
>>
>> The idea is to offer flexibility.  The same kernel can be used on
>> multiple device.  As such DT information should be prioritised over a
>> __weak symbol, otherwise it defeats the purpose.
> 
> The weak symbol should defined in board data, so it won't get picked up
> on multiple boards. But I guess I do not care that much as indeed we can
> change the sequence from userspace so we won't be stuck with firmware
> choices.

Humm... My reply wasn't clear enough.  I was thinking about the same
board with different input device, i.e keypad or touchscreen.  In that
case the board file would have been the same but not the keyreset
sequence, which is exactly what the above ordering allows to do.  But
it's ok, we agree.

> 
>>
>> Once the system is loaded user can still configure the keyreset
>> specifics by using the /sysfs interface.
>>
>>>
>>> Please also add the documenation describing the binding.
>>
>> The documentation describing the binding is in patch 1/2.  You suggest
>> that I add the documentation in this patch too ?
> 
> I simply missed your other patch as it hit my mailbox after this one.
> But I would just combine the 2 together.

I will gladly do that.

> 

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Thu, Jun 27, 2013 at 12:42:14PM -0600, Mathieu Poirier wrote:
> On 13-06-27 12:25 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 27, 2013 at 11:56:37AM -0600, Mathieu Poirier wrote:
> >> On 13-06-27 10:28 AM, Dmitry Torokhov wrote:
> >>> Hi Mathieu,
> >>>
> >>> On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
> >>>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> >>>>
> >>>> This patch adds the possibility to get the keyreset and timeout
> >>>> values from the device tree.
> >>>>
> >>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>> ---
> >>>>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 54 insertions(+)
> >>>>
> >>>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> >>>> index b51c154..91d081c 100644
> >>>> --- a/drivers/tty/sysrq.c
> >>>> +++ b/drivers/tty/sysrq.c
> >>>> @@ -44,6 +44,7 @@
> >>>>  #include <linux/uaccess.h>
> >>>>  #include <linux/moduleparam.h>
> >>>>  #include <linux/jiffies.h>
> >>>> +#include <linux/of.h>
> >>>>  
> >>>>  #include <asm/ptrace.h>
> >>>>  #include <asm/irq_regs.h>
> >>>> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> +static void sysrq_of_get_keyreset_config(void)
> >>>> +{
> >>>> +	unsigned short key;
> >>>> +	struct device_node *np;
> >>>> +	const struct property *prop;
> >>>> +	const __be32 *val;
> >>>> +	int count, i;
> >>>> +
> >>>> +	np = of_find_node_by_path("/sysrq");
> >>>> +	if (!np) {
> >>>> +		pr_info("No sysrq node found");
> >>>
> >>> I do not think this should be an info as majority would not have it
> >>> defined I think.
> >>
> >> I fail to understand your point - could you please be more specific ?
> > 
> > pr_info() will clutter everyone's dmesg because I expect majority of
> > installs will not have this enabled. Please change to pr_debug or just
> > drop it.
> 
> Ah! Yes certainly.
> 
> > 
> >>
> >>>
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> +	prop = of_find_property(np, "linux,input-keyset", NULL);
> >>>
> >>> Maybe "linux,input-key*re*set"?
> >>
> >> I do not agree.  We want the binding to be generic and not tied
> >> specifically to the keyreset functionality.  As such 'input-keyset' or
> >> 'input-keychord' are more appropriate.
> > 
> > The binding is defined specifically for sysrq and specifically to
> > perform reset action.
> 
> Yes for now but as the examples in the binding show, it is easy to
> envision how other drivers could use it.

I think you over-complicate things here. Unlike matrix-keypad binding,
where you have a common parsing code, here we have an individual driver.
I really do not see anyone else using such sequences or chords as such
processing should be done in userspace. Sysrq is quite an exception.

> 
> > 
> >>
> >>>
> >>>> +	if (!prop || !prop->value) {
> >>>> +		pr_err("Invalid input-keyset");
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> +	count = prop->length / sizeof(u32);
> >>>> +	val = prop->value;
> >>>> +
> >>>> +	if (count > SYSRQ_KEY_RESET_MAX)
> >>>> +		count = SYSRQ_KEY_RESET_MAX;
> >>>> +
> >>>> +	/* reset in case a __weak definition was present */
> >>>> +	sysrq_reset_seq_len = 0;
> >>>
> >>> Hmm, my preference for ordering would be software over firmware, so that
> >>> user could override firmware data, if needed.
> >>
> >> The idea is to offer flexibility.  The same kernel can be used on
> >> multiple device.  As such DT information should be prioritised over a
> >> __weak symbol, otherwise it defeats the purpose.
> > 
> > The weak symbol should defined in board data, so it won't get picked up
> > on multiple boards. But I guess I do not care that much as indeed we can
> > change the sequence from userspace so we won't be stuck with firmware
> > choices.
> 
> Humm... My reply wasn't clear enough.  I was thinking about the same
> board with different input device, i.e keypad or touchscreen.  In that
> case the board file would have been the same but not the keyreset
> sequence, which is exactly what the above ordering allows to do.

That does not quite make sense as the only sequence we are parsing is
the one in "sysrq" node, which is global.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Mathieu Poirier]

On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> On Thu, Jun 27, 2013 at 12:42:14PM -0600, Mathieu Poirier wrote:
>> On 13-06-27 12:25 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 27, 2013 at 11:56:37AM -0600, Mathieu Poirier wrote:
>>>> On 13-06-27 10:28 AM, Dmitry Torokhov wrote:
>>>>> Hi Mathieu,
>>>>>
>>>>> On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:
>>>>>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>>>>>
>>>>>> This patch adds the possibility to get the keyreset and timeout
>>>>>> values from the device tree.
>>>>>>
>>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>> ---
>>>>>>  drivers/tty/sysrq.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 54 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
>>>>>> index b51c154..91d081c 100644
>>>>>> --- a/drivers/tty/sysrq.c
>>>>>> +++ b/drivers/tty/sysrq.c
>>>>>> @@ -44,6 +44,7 @@
>>>>>>  #include <linux/uaccess.h>
>>>>>>  #include <linux/moduleparam.h>
>>>>>>  #include <linux/jiffies.h>
>>>>>> +#include <linux/of.h>
>>>>>>  
>>>>>>  #include <asm/ptrace.h>
>>>>>>  #include <asm/irq_regs.h>
>>>>>> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> +static void sysrq_of_get_keyreset_config(void)
>>>>>> +{
>>>>>> +	unsigned short key;
>>>>>> +	struct device_node *np;
>>>>>> +	const struct property *prop;
>>>>>> +	const __be32 *val;
>>>>>> +	int count, i;
>>>>>> +
>>>>>> +	np = of_find_node_by_path("/sysrq");
>>>>>> +	if (!np) {
>>>>>> +		pr_info("No sysrq node found");
>>>>>
>>>>> I do not think this should be an info as majority would not have it
>>>>> defined I think.
>>>>
>>>> I fail to understand your point - could you please be more specific ?
>>>
>>> pr_info() will clutter everyone's dmesg because I expect majority of
>>> installs will not have this enabled. Please change to pr_debug or just
>>> drop it.
>>
>> Ah! Yes certainly.
>>
>>>
>>>>
>>>>>
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +
>>>>>> +	prop = of_find_property(np, "linux,input-keyset", NULL);
>>>>>
>>>>> Maybe "linux,input-key*re*set"?
>>>>
>>>> I do not agree.  We want the binding to be generic and not tied
>>>> specifically to the keyreset functionality.  As such 'input-keyset' or
>>>> 'input-keychord' are more appropriate.
>>>
>>> The binding is defined specifically for sysrq and specifically to
>>> perform reset action.
>>
>> Yes for now but as the examples in the binding show, it is easy to
>> envision how other drivers could use it.
> 
> I think you over-complicate things here. Unlike matrix-keypad binding,
> where you have a common parsing code, here we have an individual driver.
> I really do not see anyone else using such sequences or chords as such
> processing should be done in userspace. Sysrq is quite an exception.

To be honest I don't have a very strong opinion on the binding.  I made
it as generic as possible on the guidance of the DT people.  Let's see
what they think of it.

> 
>>
>>>
>>>>
>>>>>
>>>>>> +	if (!prop || !prop->value) {
>>>>>> +		pr_err("Invalid input-keyset");
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +
>>>>>> +	count = prop->length / sizeof(u32);
>>>>>> +	val = prop->value;
>>>>>> +
>>>>>> +	if (count > SYSRQ_KEY_RESET_MAX)
>>>>>> +		count = SYSRQ_KEY_RESET_MAX;
>>>>>> +
>>>>>> +	/* reset in case a __weak definition was present */
>>>>>> +	sysrq_reset_seq_len = 0;
>>>>>
>>>>> Hmm, my preference for ordering would be software over firmware, so that
>>>>> user could override firmware data, if needed.
>>>>
>>>> The idea is to offer flexibility.  The same kernel can be used on
>>>> multiple device.  As such DT information should be prioritised over a
>>>> __weak symbol, otherwise it defeats the purpose.
>>>
>>> The weak symbol should defined in board data, so it won't get picked up
>>> on multiple boards. But I guess I do not care that much as indeed we can
>>> change the sequence from userspace so we won't be stuck with firmware
>>> choices.
>>
>> Humm... My reply wasn't clear enough.  I was thinking about the same
>> board with different input device, i.e keypad or touchscreen.  In that
>> case the board file would have been the same but not the keyreset
>> sequence, which is exactly what the above ordering allows to do.
> 
> That does not quite make sense as the only sequence we are parsing is
> the one in "sysrq" node, which is global.

Once again our opinion diverge.  It is entirely possible to use the same
kernel on different device (stemming from the same board file) with a
device tree to guide the configuration.

> 
> Thanks.
> 

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Grant Likely]

On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> >>>> I do not agree.  We want the binding to be generic and not tied
> >>>> specifically to the keyreset functionality.  As such 'input-keyset' or
> >>>> 'input-keychord' are more appropriate.
> >>>
> >>> The binding is defined specifically for sysrq and specifically to
> >>> perform reset action.
> >>
> >> Yes for now but as the examples in the binding show, it is easy to
> >> envision how other drivers could use it.
> > 
> > I think you over-complicate things here. Unlike matrix-keypad binding,
> > where you have a common parsing code, here we have an individual driver.
> > I really do not see anyone else using such sequences or chords as such
> > processing should be done in userspace. Sysrq is quite an exception.
> 
> To be honest I don't have a very strong opinion on the binding.  I made
> it as generic as possible on the guidance of the DT people.  Let's see
> what they think of it.

Hi Mathieu,

As per our conversation just now at Connect, the binding should probably
look like this:

Sysrq keyset binding:

The /chosen node can contain a linux,input-keyset-sysrq child node to
define a set of keys that will generate a sysrq when pressed together.

Required properties:
keyset: array of keycodes
timeout-ms: duration keys must be pressed together in microseconds
before generating a sysrq

g.

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
> On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> > >>>> I do not agree.  We want the binding to be generic and not tied
> > >>>> specifically to the keyreset functionality.  As such 'input-keyset' or
> > >>>> 'input-keychord' are more appropriate.
> > >>>
> > >>> The binding is defined specifically for sysrq and specifically to
> > >>> perform reset action.
> > >>
> > >> Yes for now but as the examples in the binding show, it is easy to
> > >> envision how other drivers could use it.
> > > 
> > > I think you over-complicate things here. Unlike matrix-keypad binding,
> > > where you have a common parsing code, here we have an individual driver.
> > > I really do not see anyone else using such sequences or chords as such
> > > processing should be done in userspace. Sysrq is quite an exception.
> > 
> > To be honest I don't have a very strong opinion on the binding.  I made
> > it as generic as possible on the guidance of the DT people.  Let's see
> > what they think of it.
> 
> Hi Mathieu,
> 
> As per our conversation just now at Connect, the binding should probably
> look like this:
> 
> Sysrq keyset binding:
> 
> The /chosen node can contain a linux,input-keyset-sysrq child node to
> define a set of keys that will generate a sysrq when pressed together.

Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
whatever. The sysrq setting is system-wide and applicable to all
devices. Given that it is used only on mobile, where there not that
many input devices (a few keys and touchscreen) I do not believe we
should consider adding per-device settings.

> 
> Required properties:
> keyset: array of keycodes

Please, let's call it 'key-reset-seq', because it is exactly the reset
sequence. There won't be any additional sequences or chords as those
should be handled in userspace, sysrq is a special case here.

> timeout-ms: duration keys must be pressed together in microseconds
> before generating a sysrq
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Mathieu Poirier]

[-- Attachment #1.1: Type: text/plain, Size: 2694 bytes --]

On 10 July 2013 10:52, Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
> > On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier <
> mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> > > >>>> I do not agree.  We want the binding to be generic and not tied
> > > >>>> specifically to the keyreset functionality.  As such
> 'input-keyset' or
> > > >>>> 'input-keychord' are more appropriate.
> > > >>>
> > > >>> The binding is defined specifically for sysrq and specifically to
> > > >>> perform reset action.
> > > >>
> > > >> Yes for now but as the examples in the binding show, it is easy to
> > > >> envision how other drivers could use it.
> > > >
> > > > I think you over-complicate things here. Unlike matrix-keypad
> binding,
> > > > where you have a common parsing code, here we have an individual
> driver.
> > > > I really do not see anyone else using such sequences or chords as
> such
> > > > processing should be done in userspace. Sysrq is quite an exception.
> > >
> > > To be honest I don't have a very strong opinion on the binding.  I made
> > > it as generic as possible on the guidance of the DT people.  Let's see
> > > what they think of it.
> >
> > Hi Mathieu,
> >
> > As per our conversation just now at Connect, the binding should probably
> > look like this:
> >
> > Sysrq keyset binding:
> >
> > The /chosen node can contain a linux,input-keyset-sysrq child node to
> > define a set of keys that will generate a sysrq when pressed together.
>
> Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
> whatever. The sysrq setting is system-wide and applicable to all
> devices. Given that it is used only on mobile, where there not that
> many input devices (a few keys and touchscreen) I do not believe we
> should consider adding per-device settings.
>

Putting the binding in the "chosen" node is definitely a system wide
setting.  If I didn't interpret your comment properly, please get back to
me.


>
> >
> > Required properties:
> > keyset: array of keycodes
>
> Please, let's call it 'key-reset-seq', because it is exactly the reset
> sequence. There won't be any additional sequences or chords as those
> should be handled in userspace, sysrq is a special case here.
>


I'm not strongly opinionated on that front but I think it should, at least,
include the some clue about the driver it ties to.  What do you think
about: "linux,sysrq-reset-seq" ?


> > timeout-ms: duration keys must be pressed together in microseconds
> > before generating a sysrq
> >
>
> Thanks.
>
> --
> Dmitry
>

[-- Attachment #1.2: Type: text/html, Size: 4123 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Grant Likely]

On Wed, Jul 10, 2013 at 5:52 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
>> On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>> > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
>> > >>>> I do not agree.  We want the binding to be generic and not tied
>> > >>>> specifically to the keyreset functionality.  As such 'input-keyset' or
>> > >>>> 'input-keychord' are more appropriate.
>> > >>>
>> > >>> The binding is defined specifically for sysrq and specifically to
>> > >>> perform reset action.
>> > >>
>> > >> Yes for now but as the examples in the binding show, it is easy to
>> > >> envision how other drivers could use it.
>> > >
>> > > I think you over-complicate things here. Unlike matrix-keypad binding,
>> > > where you have a common parsing code, here we have an individual driver.
>> > > I really do not see anyone else using such sequences or chords as such
>> > > processing should be done in userspace. Sysrq is quite an exception.
>> >
>> > To be honest I don't have a very strong opinion on the binding.  I made
>> > it as generic as possible on the guidance of the DT people.  Let's see
>> > what they think of it.
>>
>> Hi Mathieu,
>>
>> As per our conversation just now at Connect, the binding should probably
>> look like this:
>>
>> Sysrq keyset binding:
>>
>> The /chosen node can contain a linux,input-keyset-sysrq child node to
>> define a set of keys that will generate a sysrq when pressed together.
>
> Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
> whatever. The sysrq setting is system-wide and applicable to all
> devices. Given that it is used only on mobile, where there not that
> many input devices (a few keys and touchscreen) I do not believe we
> should consider adding per-device settings.

It's in /chosen, that isn't per-device.

>> Required properties:
>> keyset: array of keycodes
>
> Please, let's call it 'key-reset-seq', because it is exactly the reset
> sequence. There won't be any additional sequences or chords as those
> should be handled in userspace, sysrq is a special case here.

This is absolutely a linux-specific binding. It encodes the Linux
keycodes, and generates a linux meaning. I'm usually all about
carrying the OS-independent banner when defining DT bindings, but in
this case the linux prefix and sysrq reference is completely
appropriate.

g.

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Wednesday, July 10, 2013 10:50:26 PM Grant Likely wrote:
> On Wed, Jul 10, 2013 at 5:52 PM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
> >> On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier 
<mathieu.poirier@linaro.org> wrote:
> >> > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> >> > >>>> I do not agree.  We want the binding to be generic and not tied
> >> > >>>> specifically to the keyreset functionality.  As such
> >> > >>>> 'input-keyset' or
> >> > >>>> 'input-keychord' are more appropriate.
> >> > >>> 
> >> > >>> The binding is defined specifically for sysrq and specifically to
> >> > >>> perform reset action.
> >> > >> 
> >> > >> Yes for now but as the examples in the binding show, it is easy to
> >> > >> envision how other drivers could use it.
> >> > > 
> >> > > I think you over-complicate things here. Unlike matrix-keypad
> >> > > binding,
> >> > > where you have a common parsing code, here we have an individual
> >> > > driver.
> >> > > I really do not see anyone else using such sequences or chords as
> >> > > such
> >> > > processing should be done in userspace. Sysrq is quite an exception.
> >> > 
> >> > To be honest I don't have a very strong opinion on the binding.  I made
> >> > it as generic as possible on the guidance of the DT people.  Let's see
> >> > what they think of it.
> >> 
> >> Hi Mathieu,
> >> 
> >> As per our conversation just now at Connect, the binding should probably
> >> look like this:
> >> 
> >> Sysrq keyset binding:
> >> 
> >> The /chosen node can contain a linux,input-keyset-sysrq child node to
> >> define a set of keys that will generate a sysrq when pressed together.
> > 
> > Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
> > whatever. The sysrq setting is system-wide and applicable to all
> > devices. Given that it is used only on mobile, where there not that
> > many input devices (a few keys and touchscreen) I do not believe we
> > should consider adding per-device settings.
> 
> It's in /chosen, that isn't per-device.
> 
> >> Required properties:
> >> keyset: array of keycodes
> > 
> > Please, let's call it 'key-reset-seq', because it is exactly the reset
> > sequence. There won't be any additional sequences or chords as those
> > should be handled in userspace, sysrq is a special case here.
> 
> This is absolutely a linux-specific binding. It encodes the Linux
> keycodes, and generates a linux meaning. I'm usually all about
> carrying the OS-independent banner when defining DT bindings, but in
> this case the linux prefix and sysrq reference is completely
> appropriate.

OK, I have no idea what "/chosen" actually means. What I am trying to say
that there should be either "sysrq" or "linux,sysrq" node and that is what
sysrq driver will be looking for.

The entire node is Linux-specific and therefore there is no point in
marking only one of the properties (the key sequence) Linux-specific while
leaving other ones generic.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Mathieu Poirier]

[-- Attachment #1.1: Type: text/plain, Size: 3541 bytes --]

On 10 July 2013 16:20, Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wednesday, July 10, 2013 10:50:26 PM Grant Likely wrote:
> > On Wed, Jul 10, 2013 at 5:52 PM, Dmitry Torokhov
> >
> > <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
> > >> On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier
> <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > >> > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> > >> > >>>> I do not agree.  We want the binding to be generic and not tied
> > >> > >>>> specifically to the keyreset functionality.  As such
> > >> > >>>> 'input-keyset' or
> > >> > >>>> 'input-keychord' are more appropriate.
> > >> > >>>
> > >> > >>> The binding is defined specifically for sysrq and specifically
> to
> > >> > >>> perform reset action.
> > >> > >>
> > >> > >> Yes for now but as the examples in the binding show, it is easy
> to
> > >> > >> envision how other drivers could use it.
> > >> > >
> > >> > > I think you over-complicate things here. Unlike matrix-keypad
> > >> > > binding,
> > >> > > where you have a common parsing code, here we have an individual
> > >> > > driver.
> > >> > > I really do not see anyone else using such sequences or chords as
> > >> > > such
> > >> > > processing should be done in userspace. Sysrq is quite an
> exception.
> > >> >
> > >> > To be honest I don't have a very strong opinion on the binding.  I
> made
> > >> > it as generic as possible on the guidance of the DT people.  Let's
> see
> > >> > what they think of it.
> > >>
> > >> Hi Mathieu,
> > >>
> > >> As per our conversation just now at Connect, the binding should
> probably
> > >> look like this:
> > >>
> > >> Sysrq keyset binding:
> > >>
> > >> The /chosen node can contain a linux,input-keyset-sysrq child node to
> > >> define a set of keys that will generate a sysrq when pressed together.
> > >
> > > Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
> > > whatever. The sysrq setting is system-wide and applicable to all
> > > devices. Given that it is used only on mobile, where there not that
> > > many input devices (a few keys and touchscreen) I do not believe we
> > > should consider adding per-device settings.
> >
> > It's in /chosen, that isn't per-device.
> >
> > >> Required properties:
> > >> keyset: array of keycodes
> > >
> > > Please, let's call it 'key-reset-seq', because it is exactly the reset
> > > sequence. There won't be any additional sequences or chords as those
> > > should be handled in userspace, sysrq is a special case here.
> >
> > This is absolutely a linux-specific binding. It encodes the Linux
> > keycodes, and generates a linux meaning. I'm usually all about
> > carrying the OS-independent banner when defining DT bindings, but in
> > this case the linux prefix and sysrq reference is completely
> > appropriate.
>
> OK, I have no idea what "/chosen" actually means. What I am trying to say
> that there should be either "sysrq" or "linux,sysrq" node and that is what
> sysrq driver will be looking for.
>


Chosen pertains to system wide parameters that aren't related to hw
specific devices.  Correct, the driver will look exactly for
"linux,sysrq-reset-seq" in the "chosen" node and nowhere else.


>
> The entire node is Linux-specific and therefore there is no point in
> marking only one of the properties (the key sequence) Linux-specific while
> leaving other ones generic.
>
> Thanks.
>
> --
> Dmitry
>

[-- Attachment #1.2: Type: text/html, Size: 5140 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/2] Input: Adding DT support for keyreset tuneables

[Dmitry Torokhov]

On Wednesday, July 10, 2013 04:29:00 PM Mathieu Poirier wrote:
> On 10 July 2013 16:20, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Wednesday, July 10, 2013 10:50:26 PM Grant Likely wrote:
> > > On Wed, Jul 10, 2013 at 5:52 PM, Dmitry Torokhov
> > > 
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > On Wed, Jul 10, 2013 at 04:14:57PM +0100, Grant Likely wrote:
> > > >> On Fri, 28 Jun 2013 07:19:06 -0600, Mathieu Poirier
> > 
> > <mathieu.poirier@linaro.org> wrote:
> > > >> > On 13-06-28 12:09 AM, Dmitry Torokhov wrote:
> > > >> > >>>> I do not agree.  We want the binding to be generic and not
> > > >> > >>>> tied
> > > >> > >>>> specifically to the keyreset functionality.  As such
> > > >> > >>>> 'input-keyset' or
> > > >> > >>>> 'input-keychord' are more appropriate.
> > > >> > >>> 
> > > >> > >>> The binding is defined specifically for sysrq and specifically
> > 
> > to
> > 
> > > >> > >>> perform reset action.
> > > >> > >> 
> > > >> > >> Yes for now but as the examples in the binding show, it is easy
> > 
> > to
> > 
> > > >> > >> envision how other drivers could use it.
> > > >> > > 
> > > >> > > I think you over-complicate things here. Unlike matrix-keypad
> > > >> > > binding,
> > > >> > > where you have a common parsing code, here we have an individual
> > > >> > > driver.
> > > >> > > I really do not see anyone else using such sequences or chords as
> > > >> > > such
> > > >> > > processing should be done in userspace. Sysrq is quite an
> > 
> > exception.
> > 
> > > >> > To be honest I don't have a very strong opinion on the binding.  I
> > 
> > made
> > 
> > > >> > it as generic as possible on the guidance of the DT people.  Let's
> > 
> > see
> > 
> > > >> > what they think of it.
> > > >> 
> > > >> Hi Mathieu,
> > > >> 
> > > >> As per our conversation just now at Connect, the binding should
> > 
> > probably
> > 
> > > >> look like this:
> > > >> 
> > > >> Sysrq keyset binding:
> > > >> 
> > > >> The /chosen node can contain a linux,input-keyset-sysrq child node to
> > > >> define a set of keys that will generate a sysrq when pressed
> > > >> together.
> > > > 
> > > > Hmm, we would have only one such node, /sysrq, or /linux,sysrq,
> > > > whatever. The sysrq setting is system-wide and applicable to all
> > > > devices. Given that it is used only on mobile, where there not that
> > > > many input devices (a few keys and touchscreen) I do not believe we
> > > > should consider adding per-device settings.
> > > 
> > > It's in /chosen, that isn't per-device.
> > > 
> > > >> Required properties:
> > > >> keyset: array of keycodes
> > > > 
> > > > Please, let's call it 'key-reset-seq', because it is exactly the reset
> > > > sequence. There won't be any additional sequences or chords as those
> > > > should be handled in userspace, sysrq is a special case here.
> > > 
> > > This is absolutely a linux-specific binding. It encodes the Linux
> > > keycodes, and generates a linux meaning. I'm usually all about
> > > carrying the OS-independent banner when defining DT bindings, but in
> > > this case the linux prefix and sysrq reference is completely
> > > appropriate.
> > 
> > OK, I have no idea what "/chosen" actually means. What I am trying to say
> > that there should be either "sysrq" or "linux,sysrq" node and that is what
> > sysrq driver will be looking for.
> 
> Chosen pertains to system wide parameters that aren't related to hw
> specific devices.  Correct, the driver will look exactly for
> "linux,sysrq-reset-seq" in the "chosen" node and nowhere else.

OK, it looks like we are talking about the same thing. I seem to have
mis-parsed the original proposal.

Thanks,
Dmitry