Thank you for reviewing my patch. Main patch attached, replies inline. Separate ihidev fix sent separately
> 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?
I2C descriptor is the same. HID descriptor however on my model just describes entire packet as "proprietary format". Almost all the command and entire report are very different from ihidev. So much that trying to put them together is likely to result in regular breakages on both sides for at most 10% reduction in line count.
- Looks like this is missing a pmf handler. Does the device require
any action to suspend/resume?
Turn off/on and set absolute mode . Done
may need to be synchronized with any other routines that issue i2c
commands, probably with a mutex at interrupt level IPL_NONE.
Done
> --- 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?
Because ihidev attaches to everything due to a bug. I wasn't sure where to fix it. Fixed and moved to separate patch
> +#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:
Done
Use __BIT instead of shifts here: __BIT(0), __BIT(1), __BIT(2).
Done
> +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.
Done
Use C99 designated initializers here: `.enable = ietp_enable', &c.
Done
> +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).
I followed buggy ihidev
> + return (dpi * 10 /254);
Tiny nit: space on both sides of the operator.
Fixed
> +ietp_attach(device_t parent, device_t self, void *aux)
> +{
> ...
> + ietp_fetch_descriptor(sc);
Check return value here?
Done
> + 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
Done
> + 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.
Dinner
> +ietp_detach(device_t self, int flags)
> +{
This should start with:
error = config_detach_children(self, flags);
if (error)
return error;
Done
it's worth a shot!)
> + return (0);
Tiny nit: `return 0', no parentheses.
Fixed
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.
Done
> +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.
Done
> +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:
Done
> +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.
Removed
> --- /dev/null
> +++ b/sys/dev/i2c/ietp.h
Use include guards:
#ifndef DEV_I2C_IETP_H
#define DEV_I2C_IETP_H
Done
...
#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>
Done