Re: [PATCH v1 1/1] Input: elantech - fix absolute mode setting on some Asus touchpads

[ulrik.debie-os@e2big.org]

On Wed, Mar 25, 2015 at 03:52:22PM -0700, Dmitry Torokhov wrote:
> Hi Ulrik,
> 
> On Wed, Mar 25, 2015 at 10:27:55PM +0100, Ulrik De Bie wrote:
> > On Asus TP500LN and X750JN, the touchpad absolute mode is reset each
> > time set_rate is done.
> > 
> > In order to fix this, we will verify the firmware version, and if it
> > matches the one in those laptops, the set_rate function is overloaded
> > with a function elantech_set_rate_restore_reg_07 that perfoms the
> > set_rate with the original function, followed by a restore of reg_07
> > (the register that sets the absolute mode on elantech v4 hardware).
> > 
> > Also the asus TP500LN and X750JN firmware version, capabilities, and
> > button constellation is added to elantech.c
> > 
> > Reported-and-tested-by: George Moutsopoulos <gmoutso@yahoo.co.uk>
> > Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
> > ---
> >  drivers/input/mouse/elantech.c | 22 ++++++++++++++++++++++
> >  drivers/input/mouse/elantech.h |  1 +
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> > index 6e22682..991dc6b 100644
> > --- a/drivers/input/mouse/elantech.c
> > +++ b/drivers/input/mouse/elantech.c
> > @@ -893,6 +893,21 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
> >  }
> >  
> >  /*
> > + * This writes the reg_07 value again to the hardware at the end of every
> > + * set_rate call because the register loses its value. reg_07 allows setting
> > + * absolute mode on v4 hardware
> > + */
> > +static void elantech_set_rate_restore_reg_07(struct psmouse *psmouse,
> > +		unsigned int rate)
> > +{
> > +	struct elantech_data *etd = psmouse->private;
> > +
> > +	etd->original_set_rate(psmouse, rate);
> > +	if (elantech_write_reg(psmouse, 0x07, etd->reg_07))
> > +		psmouse_err(psmouse, "restoring reg_07 failed\n");
> 
> I wonder if set rate is useful for Elantech and if it would not be
> simpler to just have an empty stub for set_rate, maybe even for all
> Elantech devices?

1) all Elantech devices ?
I've considered it for all elantech devices, but I've no idea how this
might effect all elantech devices out there.
I did a simple test on my work laptop with elantech. Setting the rate to
a value lower than 10 does disable the mouse, setting it to a value higher
than 10 enables it again. I would not describe that as 'useful' but
not performing a set_rate 100 could give issues on touchpads that do work now.
=> I think best not all elantech devices because of backwards compatibility
issues

2) selectivity of Elantech device
In this patch I used the firmware version, which will probably match with
all (and no more than the ) elantech chips out there that have this issue.
I could go for dmi table code, but then we will probably have to extend this 
for a couple of years with more and more rows in the table.
=> preference for firmare version above dmi

3) empty stub instead of set_rate+fixup
Could work, but needs verification because currently set_rate is called with
100 and I don't know what the default is. Of course, ideally the sysfs would
then report back that the rate reading/writing is not supported, or the
rate sysfs file would not exist at all.
=> small preference for actually performing the set_rate




Kind regards,
Ulrik