Re: [PATCH] Driver for Elantech I2C touchpad

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Sat, 15 Jul 2023 03:48:54 +0200
> From: "Vladimir 'phcoder' Serbinenko" <phcoder%gmail.com@localhost>
> 
> I've submitted a similar patch for OpenBSD recently and it got merged. It
> adds support for Elantech I2C touchpad used on many laptops. Tested on my
> Chromebook Elemi

Cool!  This looks great.  I don't have any hardware to test, but I can
review the code -- nothing major.  Some higher-level questions first:

- Is this device interface substantively different from the ihidev(4)
  interface?  I notice it uses the same struct i2c_hid_desc; is that a
  red herring?

- Looks like this is missing a pmf handler.  Does the device require
  any action to suspend/resume?  If so, your attach routine should do:

	pmf_device_register(self, ietp_suspend, ietp_resume);

  (Technically pmf_device_register returns a boolean success status,
  but in reality it always returns true, and I plan to eliminate the
  return value at some point soon; doing it this way will reduce the
  churn.)

  And your detach routine should do:

	pmf_device_deregister(self);

  The ietp_suspend and ietp_resume functions are guaranteed not to be
  called concurrently with attach or detach.  Other than that, they
  may need to be synchronized with any other routines that issue i2c
  commands, probably with a mutex at interrupt level IPL_NONE.

  You can test suspend/resume of just this driver with drvctl(8): use
  `drvctl -S ietp0' to suspend, `drvctl -Q ietp0' to resume.

  If no action is needed, your attach routine should still do:

	pmf_device_register(self, NULL, NULL);

> --- a/sys/dev/i2c/i2c.c
> +++ b/sys/dev/i2c/i2c.c
> @@ -646,7 +646,7 @@ iic_use_direct_match(const struct i2c_attach_args *ia, const cfdata_t cf,
>  
>  	if (ia->ia_ncompat > 0 && ia->ia_compat != NULL) {
>  		*match_resultp = iic_compatible_match(ia, compats);
> -		return true;
> +		return *match_resultp != 0;
>  	}

Why did you make this change?

I don't think this can be right -- and it needs to be considered
separately from one driver import, since the function is used by
zillions of i2c drivers throughout the tree.

> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/device.h>
> +#include <sys/malloc.h>
> +#include <sys/stdint.h>
> +#include <sys/kmem.h>

Sort includes like this:

#include <sys/param.h>	// special, always goes first

#include <sys/device.h>	// lexicographic
#include <sys/kmem.h>
#include <sys/malloc.h>
#include <sys/stdint.h>
#include <sys/systm.h>

> +#define	IETP_TOUCH_LMB		(1 << 0)
> +#define	IETP_TOUCH_RMB		(1 << 1)
> +#define	IETP_TOUCH_MMB		(1 << 2)

Use __BIT instead of shifts here: __BIT(0), __BIT(1), __BIT(2).

> +static int ietp_ioctl(void *dev, u_long cmd, void *data, int flag,
> +	   struct lwp *l);

Tiny nit: four-space continuation lines, not tab + 3space.

> +static const struct wsmouse_accessops ietp_mouse_access = {
> +	ietp_enable,
> +	ietp_ioctl,
> +	ietp_disable
> +};

Use C99 designated initializers here: `.enable = ietp_enable', &c.

> +ietp_match(device_t parent, cfdata_t match, void *aux)
> +{
> ...
> +	if (iic_use_direct_match(ia, match, compat_data, &match_result)) {
> +		return I2C_MATCH_DIRECT_COMPATIBLE;
> +	}

Why does this return I2C_MATCH_DIRECT_COMPATIBLE rather than
match_result?

The usual idea of iic_use_direct_match is that it returns true if it
has an answer, and match_result is the answer (except that you seem to
have patched that logic away, but I'm not clear on why).

> +	return (dpi * 10 /254);

Tiny nit: space on both sides of the operator.

> +ietp_attach(device_t parent, device_t self, void *aux)
> +{
> ...
> +	ietp_fetch_descriptor(sc);

Check return value here?

> +	sc->sc_ih = acpi_intr_establish(sc->sc_dev, sc->sc_phandle, IPL_TTY, false,
> +					ietp_intr, sc, device_xname(sc->sc_dev));

Tiny nits:
- break line before 80 columns
- maybe write `/*mpsafe*/false' for clarity (not your fault, why is
  this a boolean and not a named flag?)
- four-space continuation lines

> +		printf("%s: failed reading product ID\n", device_xname(sc->sc_dev));

Use aprint_error_dev(sc->sc_dev, "failed to read product ID\n")
instead of printf with device_xname here and everywhere in the attach
routine for this kind of error message.

However, after attach, use device_printf rather than aprint_*_dev.

> +ietp_detach(device_t self, int flags)
> +{

This should start with:

	error = config_detach_children(self, flags);
	if (error)
		return error;

Otherwise, there's nothing to disconnect wsmouse(4) when the device is
detached.  You can test the detach path with drvctl(4):

- `drvctl -d ietp0' to detach ietp0
- `drvctl -r iic0' to rescan iic0 (not sure if this will work, but
  it's worth a shot!)

> +	return (0);

Tiny nit: `return 0', no parentheses.

> +ietp_activate(device_t self, enum devact act)
> +{
> +	struct ietp_softc *sc = device_private(self);
> +
> +	DPRINTF(("%s(%d)\n", __func__, act));
> +
> +	switch (act) {
> +	case DVACT_DEACTIVATE:
> +		sc->sc_dying = 1;
> +		if (ietp_set_power(sc, I2C_HID_POWER_OFF))
> +			printf("%s: failed to power down\n",
> +			    device_xname(sc->sc_dev));

The ietp_set_power call should go in ietp_detach (after
config_detach_children), not in ietp_activate.  I don't think
sc->sc_dying is actually needed; more on that in a bit.  So I think
the ietp_activate callback can go away.

(The activate callback is a bit of a legacy relic that is really only
relevant in the pcmcia/cardbus driver API, but it remains confusing
for everyone; I think we should just get rid of it.)

> +ietp_iic_set_absolute_mode(struct ietp_softc *sc, bool enable)
> +{
> ...
> +		printf("%s: failed writing poweron command\n", device_xname(sc->sc_dev));

device_printf(sc->sc_dev, ...)

> +ietp_iic_read_reg(struct ietp_softc *sc, uint16_t reg, size_t len, void *val)
> +{
> +	uint8_t cmd[] = {
> +		reg & 0xff,
> +		reg >> 8,
> +	};
> +
> +	return iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
> +		 &cmd, 2, val, len, 0);

Avoid magic constants -- write __arraycount(cmd) instead of 2 here,
and in ietp_iic_write_reg.

> +parse_input(struct ietp_softc *sc, u_char *report, int len)
> +{
> ...
> +	s = spltty();
> ...
> +		wsmouse_input(sc->sc_wsmousedev, buttons,
> ...
> +	splx(s);

New drivers generally shouldn't use spltty/splx for synchronization.

Instead, you should:

1. Put `kmutex_t sc_intr_lock;' in struct ietp_softc, initialized with
   mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_TTY) on attach and
   destroyed with mutex_destroy(&sc->sc_intr_lock) on detach.

2. Create a childdetached function for the driver that does this:

	mutex_enter(&sc->sc_intr_lock);
	if (child == sc->sc_wsmousedev)
		sc->sc_wsmousedev = NULL;
	mutex_exit(&sc->sc_intr_lock);

3. In the interrupt handler, use the lock and wsmousedev like so:


	mutex_enter(&sc->sc_intr_lock);
	if (sc->sc_wsmousedev == NULL)
		goto out;
	...
		wsmouse_input(sc->sc_wsmousedev ...);
	...
out:	mutex_exit(&sc->sc_intr_lock);

   With this structure, there is no need for a separate sc->sc_dying
   variable.  And it can be marked MP-safe as soon as wsmouse(4) is.

   ...Actually, let's just make wsmouse(4) work safely even if not
   called with the kernel lock held so you can mark it MP-safe now.

> +ietp_enable(void *dev)
> +{
> ...
> +	if (sc->sc_refcnt++ || sc->sc_isize == 0)
> +		return (0);

No need for reference-counting here.  wsmouse will not call the
struct wsmouse_accessops::enable function more than once before
disable.

> --- /dev/null
> +++ b/sys/dev/i2c/ietp.h

Use include guards:

#ifndef	DEV_I2C_IETP_H
#define	DEV_I2C_IETP_H

...

#endif	/* DEV_I2C_IETP_H */

> +#include "ihidev.h" // For i2c_hid_desc

#include <dev/i2c/ihidev.h>

You'll also need, for uintN_t, device_t, and i2c_tag_t:

#include <sys/types.h>	// (comes first, right after sys/param.h if both)

#include <sys/device_if.h>

#include <dev/i2c/i2cvar.h>