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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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