Le dim. 30 juil. 2023, 23:35, Vladimir 'phcoder' Serbinenko <
phco...@gmail.com> a écrit :
>
> Thank you for reviewing my patch. Main patch attached, replies inline.
> Separate ihidev fix sent separately
>
> Le dim. 30 juil. 2023, 13:48, Taylor R Campbell <riastr...@netbsd.org> a
> écrit :
>
>> > Date: Sat, 15 Jul 2023 03:48:54 +0200
>> > From: "Vladimir 'phcoder' Serbinenko" <phco...@gmail.com>
>> >
>> > 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
>
>>
