Re: [PATCH 1/1] HID: evision: Add preliminary support for EVision keyboards

[Philippe Valembois]

Hello,

Thanks for the review.
I just sent an updated patch taking your comments into account.

Regards,
Philippe Valembois

Le lun. 23 janv. 2023 à 08:57, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> a écrit :
>
> On Sat, Dec 24, 2022 at 1:23 PM Philippe Valembois
> <lephilousophe@xxxxxxxxx> wrote:
> >
> > From: Philippe Valembois <lephilousophe@xxxxxxxxxxxxxxxxxxxxxxxx>
>
> Jiri, I have a doubt. Do we accept emails from users.noreply.github.com?
>
> >
> > For now only supports one model and only filters out bogus reports sent
> > when the keyboard has been configured through hidraw.
> > Without this, as events are not released, soft repeat floods userspace
> > with unknown key events.
> >
> > Signed-off-by: Philippe Valembois <lephilousophe@xxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/hid/Kconfig       |  7 ++++
> >  drivers/hid/Makefile      |  1 +
> >  drivers/hid/hid-evision.c | 79 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 87 insertions(+)
> >  create mode 100644 drivers/hid/hid-evision.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index e2a5d30c8..1320ea75c 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -329,6 +329,13 @@ config HID_ELO
> >         Support for the ELO USB 4000/4500 touchscreens. Note that this is for
> >         different devices than those handled by CONFIG_TOUCHSCREEN_USB_ELO.
> >
> > +config HID_EVISION
> > +       tristate "EVision Keyboards Support"
> > +       depends on USB_HID
>
> AFAICT, the driver only uses pure HID API, so you should be able to
> depend on HID, not just USB_HID.
>
> > +       help
> > +       Support for some EVision keyboards. Note that this is needed only when
> > +       applying customization using userspace programs.
> > +
> >  config HID_EZKEY
> >         tristate "Ezkey BTC 8193 keyboard"
> >         default !EXPERT
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index e8014c1a2..bd01571dd 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -45,6 +45,7 @@ obj-$(CONFIG_HID_EMS_FF)      += hid-emsff.o
> >  obj-$(CONFIG_HID_ELAN)         += hid-elan.o
> >  obj-$(CONFIG_HID_ELECOM)       += hid-elecom.o
> >  obj-$(CONFIG_HID_ELO)          += hid-elo.o
> > +obj-$(CONFIG_HID_EVISION)      += hid-evision.o
> >  obj-$(CONFIG_HID_EZKEY)                += hid-ezkey.o
> >  obj-$(CONFIG_HID_FT260)                += hid-ft260.o
> >  obj-$(CONFIG_HID_GEMBIRD)      += hid-gembird.o
> > diff --git a/drivers/hid/hid-evision.c b/drivers/hid/hid-evision.c
> > new file mode 100644
> > index 000000000..6ea331575
> > --- /dev/null
> > +++ b/drivers/hid/hid-evision.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  HID driver for EVision devices
> > + *  For now, only ignore bogus consumer reports
> > + *  sent after the keyboard has been configured
> > + *
> > + *  Copyright (c) 2022 Philippe Valembois
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/usb.h>
>
> Outside of hid_is_usb(), you are not using anything USB related, so
> this can be dropped
>
> > +
> > +
> > +#define USB_VENDOR_ID_EVISION       0x320f
> > +#define USB_DEVICE_ID_EVISION_ICL01 0x5041
>
> We tend to add those variables in drivers/hid/hid-ids.h
>
> > +
> > +static int evision_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> > +               struct hid_field *field, struct hid_usage *usage,
> > +               unsigned long **bit, int *max)
> > +{
> > +       if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
> > +               return 0;
> > +
> > +       // Ignore key down event
>
> No C++ comments style please, use /* */ instead
>
> > +       if ((usage->hid & HID_USAGE) >> 8 == 0x05)
> > +               return -1;
> > +       // Ignore key up event
>
> Same (and for any other // below).
>
> > +       if ((usage->hid & HID_USAGE) >> 8 == 0x06)
> > +               return -1;
> > +
> > +       switch (usage->hid & HID_USAGE) {
> > +       // Ignore configuration saved event
> > +       case 0x0401: return -1;
> > +       // Ignore reset event
> > +       case 0x0402: return -1;
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int evision_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > +       int ret;
> > +
> > +       if (!hid_is_usb(hdev))
> > +               return -EINVAL;
>
> This can be dropped...
>
> > +
> > +       ret = hid_parse(hdev);
> > +       if (ret) {
> > +               hid_err(hdev, "EVision hid parse failed: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > +       if (ret) {
> > +               hid_err(hdev, "EVision hw start failed: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
>
> ... which means the probe is the default one, meaning it can also be
> dropped from the patch :)
>
> > +}
> > +
> > +static const struct hid_device_id evision_devices[] = {
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_EVISION, USB_DEVICE_ID_EVISION_ICL01) },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(hid, evision_devices);
> > +
> > +static struct hid_driver evision_driver = {
> > +       .name = "evision",
> > +       .id_table = evision_devices,
> > +       .input_mapping = evision_input_mapping,
> > +       .probe = evision_probe,
>
> Just for completeness, remove that .probe line and your driver will
> behave the same and be smaller :)
>
> > +};
> > +module_hid_driver(evision_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > --
> > 2.38.2
> >
>
> Cheers,
> Benjamin
>