| Message ID | 1372349605-4500-2-git-send-email-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);