Re: [PATCH 3/3] OLPC: touchpad driver (take 2)

[Andres Salomon <dilinger@queued.net>]

On Thu, 14 Aug 2008 23:14:35 -0400
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Andres,
> 
> On Wed, Aug 13, 2008 at 03:24:59PM -0400, Andres Salomon wrote:
[...]
> 
> > +
> > +	retval = serio_pin_driver(serio);
> > +	if (retval)
> > +		return retval;
> > +
> > +	psmouse = serio_get_drvdata(serio);
> > +	priv = psmouse->private;
> > +
> > +	if (val == priv->powered)
> > +		goto done;
> > +
> > +	/* hgpk_toggle_power will deal w/ state so we're not
> > racing w/ irq */
> > +	retval = hgpk_toggle_power(psmouse, val);
> > +	if (!retval)
> > +		priv->powered = val;
> > +
> > +done:
> > +	serio_unpin_driver(serio);
> > +	return retval ? retval : count;
> > +}
> > +
> > +static DEVICE_ATTR(powered, S_IWUSR | S_IRUGO, hgpk_show_powered,
> > +		hgpk_set_powered);
> > 
> 
> Any particular reason you didn't use PSMOUSE_DEFINE_ATTR? Then you
> would not need to bother with pinning the driver and provode mutual
> exclusion with other things. Btw, what do we do if device is powered
> down an user tries to change some settings via generic attributes
> defined in psmouse-base?
> 

Ok, the problem is this - hgpk_set_powered disables power by sending a
special command, and power is re-enabled with the following code:

                /*
                 * Sending a byte will drive MS-DAT low; this will wake
up
                 * the controller.  Once we get an ACK back from it, it
                 * means we can continue with the touchpad re-init. ALPS
                 * tells us that 1s should be long enough, so set that
as
                 * the upper bound.
                 */
                for (timeo = 20; timeo > 0; timeo--) {
                        if (!ps2_sendbyte(&psmouse->ps2dev,
                                        PSMOUSE_CMD_DISABLE, 20))
                                break;
                        msleep(50);
                }

This means that after telling the ALPS device to turn off power, we
shouldn't be sending any ps2 commands until we want to turn power
back on.  However, psmouse_attr_set_helper code has the following:

        psmouse_deactivate(psmouse);
        retval = attr->set(psmouse, attr->data, buf, count);
        if (retval != -ENODEV)
                psmouse_activate(psmouse);

If we're disabling power, psmouse_activate will proceed to turn
power back on.

There's also the following check in psmouse_attr_set_helper:

        if (psmouse->state == PSMOUSE_IGNORE) {
                retval = -ENODEV;
                goto out_unlock;
        }

That's not at all what we want; when we disable power, we also
do a psmouse_set_state(psmouse, PSMOUSE_IGNORE).  That check
means we'd never be able to turn power back on.


What do you think about either changing PSMOUSE_DEFINE_ATTR
to take an additional 'raw' argument (meaning psmouse->state is
not checked, and psmouse_deactivate/psmouse_activate are not
called), or alternatively adding new helper functions that just
handle the locking (__PSMOUSE_DEFINE_ATTR() and
__psmouse_attr_{set,show}_helper())?  I'd prefer the raw
argument.