On 13-06-28 12:09 AM, Dmitry Torokhov wrote:

On Thu, Jun 27, 2013 at 12:42:14PM -0600, Mathieu Poirier wrote:

quoted

On 13-06-27 12:25 PM, Dmitry Torokhov wrote:

quoted

On Thu, Jun 27, 2013 at 11:56:37AM -0600, Mathieu Poirier wrote:

quoted

On 13-06-27 10:28 AM, Dmitry Torokhov wrote:

quoted

Hi Mathieu,

On Thu, Jun 27, 2013 at 10:13:25AM -0600, mathieu.poirier@linaro.org wrote:

quoted

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.

quoted

quoted

quoted

quoted

+		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.

quoted

quoted

quoted

quoted

quoted

+	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.