Hi Arjan,
On Fri, Sep 19, 2008 at 09:05:30AM +0200, Arjan Opmeer wrote:
>
> Hi Dmitry,
>
> (Sorry for calling you Dimitry earlier :)
>
> On Fri, Sep 19, 2008 at 12:17:27AM -0400, Dmitry Torokhov wrote:
> > On Sun, Sep 14, 2008 at 05:23:44AM +0200, Arjan Opmeer wrote:
> > >
> > > Update the Elantech touchpad driver to v5
> >
> > Could you tell me if issues with pressure reporting mentioned in bugzilla
> > #8781 have been resolved?
>
> I am not exactly sure what the issue is that is described there.
>
> As far as I know the touchpad does not report pressure (maybe the EeePC
> style touchpad does, but I do not have hands on experience with that).
> However the Xorg Synaptics driver requires pressure events to work. So I
> chose a default pressure value and the driver always toggles between no
> pressure and default pressure for no touch and touch respectively. This
> seems to work well for all my testers as I have not heard any complaints.
>
I see. In this case the driver should not really report ABS_PRESSURE but
only BTN_TOUCH. I understand that this would require synaptics X driver
changes but that should be OK. I wrote a patch to the X driver which
seems to be working if I bastardize my synaptics touchpad but I would
like to have it tested with real Elantech device. I am attaching the
version of Elantech driver that I want to apply (there were some timy
changes) and the patch to teh Synaptics X driver (against recent git
pull from its repository). If you could give it a try that would be
grand.
Thanks!
--
Dmitry
Hi Dmitry,
On Wed, Oct 15, 2008 at 11:05:15PM -0400, Dmitry Torokhov wrote:
> >
> > As far as I know the touchpad does not report pressure
>
> I see. In this case the driver should not really report ABS_PRESSURE but
> only BTN_TOUCH. I understand that this would require synaptics X driver
> changes but that should be OK. I wrote a patch to the X driver which seems
> to be working if I bastardize my synaptics touchpad but I would like to
> have it tested with real Elantech device. I am attaching the version of
> Elantech driver that I want to apply (there were some timy changes) and
> the patch to teh Synaptics X driver (against recent git pull from its
> repository). If you could give it a try that would be grand.
Well, there we have a problem. My laptop with the Elantech touchpad died. So
I no longer have any hardware to try your changes on!
Maybe we should just put the code out there and see what happens?
As I was reading over the patch I found some small things you might want to
change.
> dropped (users wishing to use touchpad in relative mode can use
> standard PS/2 protocol emilation done in hardware). The driver
^^^^^^^^^ emulation
> + case 2:
> + /* We don't now how to check parity in protocol v2 */
^^^ know
> + /*
> + * Read back reg 0x10. The touchpad is probably initalising
> + * and not ready until we read back the value we just wrote.
> + */
Although the fact that we can read back the registers contents means we
didn't jam the touchpad EC by initialising with the wrong register values
earlier, it occured to me that we never actually test the value we read
back.
> + do {
> + rc = elantech_read_reg(psmouse, 0x10, &val);
> + if (rc == 0)
Shouldn't this read
if (rc == 0 && val == etd->reg 10)
?
> + for (i = 1; i < 256; i++)
> + etd->parity[i] = (etd->parity[i & (i - 1)] ^ 1);
^ ^
Why did I put these parentheses there?
> + /*
> + * Assume every version greater than this is new EeePC style
> + * hardware with 6 byte packets
> + */
> + if ((etd->fw_version_maj >= 0x02) && (etd->fw_version_min >= 0x30)) {
In your quest of removing parentheses don't you want to remove these ones too?
:)
> +/*
> + * Try Elantech touchpad.
> + */
> + if (max_proto > PSMOUSE_IMEX &&
> + elantech_detect(psmouse, set_properties) == 0) {
> + if (!set_properties || elantech_init(psmouse) == 0)
> + return PSMOUSE_ELANTECH;
I see you moved the Elantech detection up. I put it after the pmouse_reset
on purpose as a safety measure because when initially developing the driver
I managed to get the touchpad all confused and not responding properly to
its magic knock. However this was using the serio_raw interface, so it might
not be necessary in this case.
Arjan
On Thu, Oct 16, 2008 at 06:29:09AM +0200, Arjan Opmeer wrote:
>
> Hi Dmitry,
>
> On Wed, Oct 15, 2008 at 11:05:15PM -0400, Dmitry Torokhov wrote:
> > >
> > > As far as I know the touchpad does not report pressure
> >
> > I see. In this case the driver should not really report ABS_PRESSURE but
> > only BTN_TOUCH. I understand that this would require synaptics X driver
> > changes but that should be OK. I wrote a patch to the X driver which seems
> > to be working if I bastardize my synaptics touchpad but I would like to
> > have it tested with real Elantech device. I am attaching the version of
> > Elantech driver that I want to apply (there were some timy changes) and
> > the patch to teh Synaptics X driver (against recent git pull from its
> > repository). If you could give it a try that would be grand.
>
> Well, there we have a problem. My laptop with the Elantech touchpad died. So
> I no longer have any hardware to try your changes on!
>
Oops, sorry to hear that. It looks like i wait for too long...
> Maybe we should just put the code out there and see what happens?
>
I suppose so, it was in Andrew's tree for a while.
>
> As I was reading over the patch I found some small things you might want to
> change.
>
> > dropped (users wishing to use touchpad in relative mode can use
> > standard PS/2 protocol emilation done in hardware). The driver
> ^^^^^^^^^ emulation
>
OK.
> > + case 2:
> > + /* We don't now how to check parity in protocol v2 */
> ^^^ know
OK.
>
> > + /*
> > + * Read back reg 0x10. The touchpad is probably initalising
> > + * and not ready until we read back the value we just wrote.
> > + */
>
> Although the fact that we can read back the registers contents means we
> didn't jam the touchpad EC by initialising with the wrong register values
> earlier, it occured to me that we never actually test the value we read
> back.
>
> > + do {
> > + rc = elantech_read_reg(psmouse, 0x10, &val);
> > + if (rc == 0)
>
> Shouldn't this read
>
> if (rc == 0 && val == etd->reg 10)
>
> ?
>
If we see reports of hardware not switching to absolute mode we might
want to do this.
> > + for (i = 1; i < 256; i++)
> > + etd->parity[i] = (etd->parity[i & (i - 1)] ^ 1);
> ^ ^
> Why did I put these parentheses there?
>
:)
> > + /*
> > + * Assume every version greater than this is new EeePC style
> > + * hardware with 6 byte packets
> > + */
> > + if ((etd->fw_version_maj >= 0x02) && (etd->fw_version_min >= 0x30)) {
>
> In your quest of removing parentheses don't you want to remove these ones too?
> :)
>
Done.
> > +/*
> > + * Try Elantech touchpad.
> > + */
> > + if (max_proto > PSMOUSE_IMEX &&
> > + elantech_detect(psmouse, set_properties) == 0) {
> > + if (!set_properties || elantech_init(psmouse) == 0)
> > + return PSMOUSE_ELANTECH;
>
> I see you moved the Elantech detection up. I put it after the pmouse_reset
> on purpose as a safety measure because when initially developing the driver
> I managed to get the touchpad all confused and not responding properly to
> its magic knock. However this was using the serio_raw interface, so it might
> not be necessary in this case.
>
There is psmouse_reset() call in elantech_detect() now. The reason it
was moved is the place it used to be we do "basic" protocol probes when
either user asked us to limit protocols being tested or we know that we
are working with specific hardware but native protocol failed for some
reason.
--
Dmitry