Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset

2013-04-17 Thread Shawn Nematbakhsh 
Hi Dmitry,

On Tue, Apr 16, 2013 at 4:09 PM, Dmitry Torokhov
 wrote:
> Hi Shawn,
>
> On Tue, Apr 09, 2013 at 02:53:44PM -0700, Shawn Nematbakhsh wrote:
>> Hi Dmitry,
>>
>> Thanks for the review. Comments in-line.
>>
>> On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
>>  wrote:
>> > Hi Shawn,
>> >
>> > On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
>> >> The trackpoint driver sets various parameter default values, all of
>> >> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
>> >> Specification, Version 4.0. Also confirmed by empirical data).
>> >>
>> >> By sending the power-on reset command to reset all parameters to
>> >> power-on state, we can skip the lengthy process of programming all
>> >> parameters. In testing, ~2.5 secs of time writing parameters was reduced
>> >> to .35 seconds waiting for power-on reset to complete.
>> >>
>> >> Signed-off-by: Shawn Nematbakhsh 
>> >> ---
>> >>  drivers/input/mouse/trackpoint.c | 223 
>> >> +--
>> >>  drivers/input/mouse/trackpoint.h |   7 +-
>> >>  2 files changed, 149 insertions(+), 81 deletions(-)
>> >>
>> >> diff --git a/drivers/input/mouse/trackpoint.c 
>> >> b/drivers/input/mouse/trackpoint.c
>> >> index f310249..17e9b36 100644
>> >> --- a/drivers/input/mouse/trackpoint.c
>> >> +++ b/drivers/input/mouse/trackpoint.c
>> >> @@ -20,6 +20,26 @@
>> >>  #include "trackpoint.h"
>> >>
>> >>  /*
>> >> + * Power-on Reset: Resets all trackpoint parameters, including RAM 
>> >> values,
>> >> + * to defaults.
>> >> + * Returns zero on success, non-zero on failure.
>> >> + */
>> >> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
>> >> +{
>> >> + unsigned char results[2];
>> >> +
>> >> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
>> >> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
>> >> + return -1;
>> >> + }
>> >> +
>> >> + /* POR response should be 0xAA00 or 0xFC00 */
>> >> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 
>> >> 0x00)
>> >> + return -1;
>> >
>> > I am a bit concerned about this. 0xfc00 indicates POR failure. In this
>> > case shouldn't we try to reset the device, issue another POR and bail if
>> > it fails again?
>> >
>>
>> Yes, you are correct. I just now posted v2 to fix this. Now, on
>> 0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
>> attempt to set parameters manually.
>>
>> >>
>> >>  static struct attribute *trackpoint_attrs[] = {
>> >>   &psmouse_attr_sensitivity.dattr.attr,
>> >> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
>> >>   &psmouse_attr_press_to_select.dattr.attr,
>> >>   &psmouse_attr_skipback.dattr.attr,
>> >>   &psmouse_attr_ext_dev.dattr.attr,
>> >> + &psmouse_attr_twohand.dattr.attr,
>> >> + &psmouse_attr_source_tag.dattr.attr,
>> >> + &psmouse_attr_mb.dattr.attr,
>> >
>> > What is the benefit of adding these 3 new attributes?
>> >
>>
>> Previously, these attributes were handled in a special way -- the
>> corresponding TP values were set to default, but the attributes were
>> not exported. Now, they are set to default and exported. It makes for
>> cleaner code.
>>
>> I see no good use for these attributes other than driver hacking. I
>> can remove them if you think it's best.
>
>
> Yes, I think that even though having them as attributes makes the code
> look nice we should not be exposing them as they do break driver
> behavior.
>
> Does the version of the patch below work for you?
>

This version looks + tests good, with one small change: Since we no
longer export the three extra parameters, I removed the corresponding
trackpoint_data struct members. Ex.

< @@ -136,10 +138,13 @@ struct trackpoint_data
---
> @@ -136,9 +138,9 @@ struct trackpoint_data
386,388d385
< + unsigned char twohand;
< + unsigned char source_tag;
< + unsigned char mb;

See complete revised patch below.


Input: trackpoint - Optimize trackpoint init to use power-on reset

From: Shawn Nematbakhsh 

The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).

By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.

Signed-off-by: Shawn Nematbakhsh 
Signed-off-by: Dmitry Torokhov 
---
 drivers/input/mouse/trackpoint.c | 249 +--
 drivers/input/mouse/trackpoint.h |   4 +-
 2 files changed, 166 insertions(+), 87 deletions(-)

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..ca843b6 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,9 +2

Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset

2013-04-16 Thread Dmitry Torokhov 
Hi Shawn,

On Tue, Apr 09, 2013 at 02:53:44PM -0700, Shawn Nematbakhsh wrote:
> Hi Dmitry,
> 
> Thanks for the review. Comments in-line.
> 
> On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
>  wrote:
> > Hi Shawn,
> >
> > On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
> >> The trackpoint driver sets various parameter default values, all of
> >> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
> >> Specification, Version 4.0. Also confirmed by empirical data).
> >>
> >> By sending the power-on reset command to reset all parameters to
> >> power-on state, we can skip the lengthy process of programming all
> >> parameters. In testing, ~2.5 secs of time writing parameters was reduced
> >> to .35 seconds waiting for power-on reset to complete.
> >>
> >> Signed-off-by: Shawn Nematbakhsh 
> >> ---
> >>  drivers/input/mouse/trackpoint.c | 223 
> >> +--
> >>  drivers/input/mouse/trackpoint.h |   7 +-
> >>  2 files changed, 149 insertions(+), 81 deletions(-)
> >>
> >> diff --git a/drivers/input/mouse/trackpoint.c 
> >> b/drivers/input/mouse/trackpoint.c
> >> index f310249..17e9b36 100644
> >> --- a/drivers/input/mouse/trackpoint.c
> >> +++ b/drivers/input/mouse/trackpoint.c
> >> @@ -20,6 +20,26 @@
> >>  #include "trackpoint.h"
> >>
> >>  /*
> >> + * Power-on Reset: Resets all trackpoint parameters, including RAM values,
> >> + * to defaults.
> >> + * Returns zero on success, non-zero on failure.
> >> + */
> >> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> >> +{
> >> + unsigned char results[2];
> >> +
> >> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> >> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
> >> + return -1;
> >> + }
> >> +
> >> + /* POR response should be 0xAA00 or 0xFC00 */
> >> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
> >> + return -1;
> >
> > I am a bit concerned about this. 0xfc00 indicates POR failure. In this
> > case shouldn't we try to reset the device, issue another POR and bail if
> > it fails again?
> >
> 
> Yes, you are correct. I just now posted v2 to fix this. Now, on
> 0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
> attempt to set parameters manually.
> 
> >>
> >>  static struct attribute *trackpoint_attrs[] = {
> >>   &psmouse_attr_sensitivity.dattr.attr,
> >> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
> >>   &psmouse_attr_press_to_select.dattr.attr,
> >>   &psmouse_attr_skipback.dattr.attr,
> >>   &psmouse_attr_ext_dev.dattr.attr,
> >> + &psmouse_attr_twohand.dattr.attr,
> >> + &psmouse_attr_source_tag.dattr.attr,
> >> + &psmouse_attr_mb.dattr.attr,
> >
> > What is the benefit of adding these 3 new attributes?
> >
> 
> Previously, these attributes were handled in a special way -- the
> corresponding TP values were set to default, but the attributes were
> not exported. Now, they are set to default and exported. It makes for
> cleaner code.
> 
> I see no good use for these attributes other than driver hacking. I
> can remove them if you think it's best.


Yes, I think that even though having them as attributes makes the code
look nice we should not be exposing them as they do break driver
behavior.

Does the version of the patch below work for you?

Thanks.

-- 
Dmitry

Input: trackpoint - Optimize trackpoint init to use power-on reset

From: Shawn Nematbakhsh 

The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).

By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.

Signed-off-by: Shawn Nematbakhsh 
Signed-off-by: Dmitry Torokhov 
---
 drivers/input/mouse/trackpoint.c |  249 +-
 drivers/input/mouse/trackpoint.h |7 +
 2 files changed, 169 insertions(+), 87 deletions(-)

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..ca843b6 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,9 +20,34 @@
 #include "trackpoint.h"
 
 /*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+   unsigned char results[2];
+   int tries = 0;
+
+   /* Issue POR command, and repeat up to once if 0xFC00 received */
+   do {
+   if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+   ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))

Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset

2013-04-09 Thread Shawn Nematbakhsh 
Hi Dmitry,

Thanks for the review. Comments in-line.

On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
 wrote:
> Hi Shawn,
>
> On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
>> The trackpoint driver sets various parameter default values, all of
>> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
>> Specification, Version 4.0. Also confirmed by empirical data).
>>
>> By sending the power-on reset command to reset all parameters to
>> power-on state, we can skip the lengthy process of programming all
>> parameters. In testing, ~2.5 secs of time writing parameters was reduced
>> to .35 seconds waiting for power-on reset to complete.
>>
>> Signed-off-by: Shawn Nematbakhsh 
>> ---
>>  drivers/input/mouse/trackpoint.c | 223 
>> +--
>>  drivers/input/mouse/trackpoint.h |   7 +-
>>  2 files changed, 149 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/input/mouse/trackpoint.c 
>> b/drivers/input/mouse/trackpoint.c
>> index f310249..17e9b36 100644
>> --- a/drivers/input/mouse/trackpoint.c
>> +++ b/drivers/input/mouse/trackpoint.c
>> @@ -20,6 +20,26 @@
>>  #include "trackpoint.h"
>>
>>  /*
>> + * Power-on Reset: Resets all trackpoint parameters, including RAM values,
>> + * to defaults.
>> + * Returns zero on success, non-zero on failure.
>> + */
>> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
>> +{
>> + unsigned char results[2];
>> +
>> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
>> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
>> + return -1;
>> + }
>> +
>> + /* POR response should be 0xAA00 or 0xFC00 */
>> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
>> + return -1;
>
> I am a bit concerned about this. 0xfc00 indicates POR failure. In this
> case shouldn't we try to reset the device, issue another POR and bail if
> it fails again?
>

Yes, you are correct. I just now posted v2 to fix this. Now, on
0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
attempt to set parameters manually.

>>
>>  static struct attribute *trackpoint_attrs[] = {
>>   &psmouse_attr_sensitivity.dattr.attr,
>> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
>>   &psmouse_attr_press_to_select.dattr.attr,
>>   &psmouse_attr_skipback.dattr.attr,
>>   &psmouse_attr_ext_dev.dattr.attr,
>> + &psmouse_attr_twohand.dattr.attr,
>> + &psmouse_attr_source_tag.dattr.attr,
>> + &psmouse_attr_mb.dattr.attr,
>
> What is the benefit of adding these 3 new attributes?
>

Previously, these attributes were handled in a special way -- the
corresponding TP values were set to default, but the attributes were
not exported. Now, they are set to default and exported. It makes for
cleaner code.

I see no good use for these attributes other than driver hacking. I
can remove them if you think it's best.

> Thanks.
>
> --
> Dmitry

Thanks,

Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset

2013-03-27 Thread Dmitry Torokhov 
Hi Shawn,

On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
> The trackpoint driver sets various parameter default values, all of
> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
> Specification, Version 4.0. Also confirmed by empirical data).
> 
> By sending the power-on reset command to reset all parameters to
> power-on state, we can skip the lengthy process of programming all
> parameters. In testing, ~2.5 secs of time writing parameters was reduced
> to .35 seconds waiting for power-on reset to complete.
> 
> Signed-off-by: Shawn Nematbakhsh 
> ---
>  drivers/input/mouse/trackpoint.c | 223 
> +--
>  drivers/input/mouse/trackpoint.h |   7 +-
>  2 files changed, 149 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/input/mouse/trackpoint.c 
> b/drivers/input/mouse/trackpoint.c
> index f310249..17e9b36 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -20,6 +20,26 @@
>  #include "trackpoint.h"
>  
>  /*
> + * Power-on Reset: Resets all trackpoint parameters, including RAM values,
> + * to defaults.
> + * Returns zero on success, non-zero on failure.
> + */
> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> +{
> + unsigned char results[2];
> +
> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
> + return -1;
> + }
> +
> + /* POR response should be 0xAA00 or 0xFC00 */
> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
> + return -1;

I am a bit concerned about this. 0xfc00 indicates POR failure. In this
case shouldn't we try to reset the device, issue another POR and bail if
it fails again?

>  
>  static struct attribute *trackpoint_attrs[] = {
>   &psmouse_attr_sensitivity.dattr.attr,
> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
>   &psmouse_attr_press_to_select.dattr.attr,
>   &psmouse_attr_skipback.dattr.attr,
>   &psmouse_attr_ext_dev.dattr.attr,
> + &psmouse_attr_twohand.dattr.attr,
> + &psmouse_attr_source_tag.dattr.attr,
> + &psmouse_attr_mb.dattr.attr,

What is the benefit of adding these 3 new attributes?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset

2013-03-26 Thread Shawn Nematbakhsh 
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).

By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.

Signed-off-by: Shawn Nematbakhsh 
---
 drivers/input/mouse/trackpoint.c | 223 +--
 drivers/input/mouse/trackpoint.h |   7 +-
 2 files changed, 149 insertions(+), 81 deletions(-)

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..17e9b36 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,26 @@
 #include "trackpoint.h"
 
 /*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+   unsigned char results[2];
+
+   if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+   ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
+   return -1;
+   }
+
+   /* POR response should be 0xAA00 or 0xFC00 */
+   if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
+   return -1;
+   return 0;
+}
+
+/*
  * Device IO: read, write and toggle bit
  */
 static int trackpoint_read(struct ps2dev *ps2dev, unsigned char loc, unsigned 
char *results)
@@ -69,6 +89,7 @@ struct trackpoint_attr_data {
unsigned char command;
unsigned char mask;
unsigned char inverted;
+   unsigned char power_on_default;
 };
 
 static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data, 
char *buf)
@@ -102,10 +123,11 @@ static ssize_t trackpoint_set_int_attr(struct psmouse 
*psmouse, void *data,
return count;
 }
 
-#define TRACKPOINT_INT_ATTR(_name, _command)   
\
+#define TRACKPOINT_INT_ATTR(_name, _command, _default) 
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {  
\
.field_offset = offsetof(struct trackpoint_data, _name),
\
.command = _command,
\
+   .power_on_default = _default,   
\
};  
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,   
\
&trackpoint_attr_##_name,   
\
@@ -139,31 +161,71 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse 
*psmouse, void *data,
 }
 
 
-#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv)  
\
+#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {  
\
-   .field_offset   = offsetof(struct trackpoint_data, _name),  
\
-   .command= _command, 
\
-   .mask   = _mask,
\
-   .inverted   = _inv, 
\
+   .field_offset   = offsetof(struct trackpoint_data,  
\
+  _name),  
\
+   .command= _command, 
\
+   .mask   = _mask,
\
+   .inverted   = _inv, 
\
+   .power_on_default   = _default, 
\
};  
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,   
\
&trackpoint_attr_##_name,   
\
trackpoint_show_int_attr, trackpoint_set_bit_attr)
 
-TRACKPOINT_INT_ATTR(sensitivity, TP_SENS);
-TRACKPOINT_INT_ATTR(speed, TP_SPEED);
-TRACKPOINT_INT_ATTR(inertia, TP_INERTIA);
-TRACKPOINT_INT_ATTR(reach, TP_REACH);
-TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS);
-TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG);
-TRACKPOINT_INT_ATTR(thresh, TP_THRESH);
-TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH);
-TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME);
-TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV);
-
-TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0);
-TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0);
-TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1);
+#defin