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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

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