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

Benjamin Tissoires
2023-01-23T07:58:44Z

On Sat, Dec 24, 2022 at 1:23 PM Philippe Valembois
<lephilousophe@gmail.com> wrote:
>
> From: Philippe Valembois <lephilousophe@users.noreply.github.com>

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@users.noreply.github.com>
> ---
> 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