Re: [PATCH 0/5] Input: elantech - support the Fujitsu H730 laptop

[ulrik.debie-os@e2big.org]

Hi
On Sun, Aug 31, 2014 at 11:54:25AM +0200, Hans de Goede wrote:
> Date:	Sun, 31 Aug 2014 11:54:25 +0200
> From: Hans de Goede <hdegoede@redhat.com>
> To: Ulrik De Bie <ulrik.debie-os@e2big.org>, Dmitry Torokhov
>  <dmitry.torokhov@gmail.com>
> CC: linux-input@vger.kernel.org, David Herrmann <dh.herrmann@gmail.com>
> Subject: Re: [PATCH 0/5] Input: elantech - support the Fujitsu H730 laptop
> X-Mailing-List:	linux-input@vger.kernel.org
> 
> Hi,
> 
> On 08/30/2014 04:10 PM, Ulrik De Bie wrote:
> > This patch set makes the elantech driver work for the Fujitsu H730 laptop. It
> > also adds a sysfs knob to allow other laptops that are facing similar
> > problems as the Fujitsu H730 to have working touchpad.
> > 
> > I'm considering adding a WARN_ONCE when the crc_enabled signature check
> > fails. The message would then point the user to the crc_enabled sysfs knob,
> > and kindly ask the user to report the laptop to linux-input mailinglist ?
> > Any suggestions ?
> 
> WARN_ONCE includes a backtrace, which will just scare users, simply use a
> function static variable to build your own warn_once, which really only does
> warn. Other then that I think having a warning pointing to to the sysfs knob is
> a good idea. Although maybe it should only trigger on 2 consecutive crc errors
> to avoid false positives?

The static variable variant exists as printk_once. But of course, that one
will not allow easy check on 2 consecutive.
 
> > Two users have tested the combination of this patchset and confirmed that
> > it makes the H730 touchpad/trackpoint work.
> > 
> > Here an overview of the patchset:
> > 
> > Patch 1/ : Input: elantech - use elantech_report_trackpoint for hardware v4 too
> > The Fujitsu H730 is the first v4 hardware identified that also sends the
> > trackpoint packets. This patch enables the trackpoint on v4 hardware.
> > With this patch the trackpoint will work.
> > 
> > Patch 2/ : Input: elantech - Fix crc_enabled for Fujitsu H730
> > Uses DMI to detect the H730 and ,if detected, will set crc_enabled to 1. 
> > With this patch the touchpad and left/right button will start to work.
> > 
> > Patch 3/ : Input: elantech - report the middle button of the touchpad
> > The Fujitsu H730 is the first laptop listed in the elantech.c file with
> > 3 touchpad buttons. This patch enables the middle button for all elantech
> > hardware and enables the reporting for v4 hardware.
> > I want to hear feedback here on what preferences exist for the conditions
> > to enable the middle button:
> > - DMI
> > - enable only for v4
> > - enable for all/report for v3+v4
> 
> I assume you've tested this on a v4 model without a middle button ? Assuming
> that that is the case I think that always enabling it on v4 is fine. I see no
> reason to also enable it on v3 as long as we've no reports of v3 models with
> 3 buttons.

No I have only tested myself on L530 (v3 with 2 touchpad buttons) and the two
testers have tested on Fujitsu H730 (v4 with 3 touchpad buttons).

Looking at the list in elantech, that would preferably be a test on Asus
G46VW or G750JX. Since you were able to come up with the list, do you
happen also to know contact details of some with such a laptop ?

> > ...
> > 
> > Patch 4/ : Input: Elantech - provide a sysfs knob for crc_enabled
> > Probably H730 will not remain the only elantech laptop that has this failing
> > detection for the crc_enabled. This provides users with a workaround when
> > the crc_enabled detection fails.
> > 
> > Patch 5/ : Elantech - Update the documentation: trackpoint,v3/v4,crc_enabled
> > This provides an update of the elantech documentation. 
> > 
> > Patch 4 depends on patch 2, and for consistency, patch 5 depends on patch 1-2-4.
> > 
> > Ulrik De Bie (5):
> >   Input: elantech - use elantech_report_trackpoint for hardware v4 too
> >   Input: elantech - Fix crc_enabled for Fujitsu H730
> >   Input: elantech - report the middle button of the touchpad
> >   Input: elantech - provide a sysfs knob for crc_enabled
> >   Input: elantech - Update the documentation:
> >     trackpoint,v3/v4,crc_enabled
> 
> The entire series looks good to me (the adding of the warning can be done in a follow
> up patch), and is:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Regards,
> 
> Hans

Thanks for your feedback !

Regards,
Ulrik