Re: [PATCH v2] HID: add driver for EWEADN G5 mice

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] HID: hid-eweadn: missing hid-ids.h include breaks build
- [Low] HID: hid-eweadn: redundant eweadn_probe() duplicates default probe
--

commit 4edaba02eb50bfbcf89b68ad006b2e16efd6acc9
Author: Liang Haowen <nbg2974@gmail.com>

HID: add driver for EWEADN G5 mice

This commit adds a HID driver for EWEADN G5 wireless and wired mice that
replaces the broken firmware report descriptor on USB interface 0 with a
standard mouse report descriptor.

> diff --git a/drivers/hid/hid-eweadn.c b/drivers/hid/hid-eweadn.c
> new file mode 100644
> index 0000000000000..79a9a1545b395
> --- /dev/null
> +++ b/drivers/hid/hid-eweadn.c
> @@ -0,0 +1,129 @@

[ ... ]

> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +
> +static __u8 eweadn_rdesc[] = {

[ ... ]

> +static int eweadn_probe(struct hid_device *hdev,
> +			const struct hid_device_id *id)
> +{
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "parse failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
This isn't a bug, but is eweadn_probe() needed here?

In drivers/hid/hid-core.c, __hid_device_probe() already runs the default
probe sequence when hdrv->probe is not set:

drivers/hid/hid-core.c:__hid_device_probe() {
	if (hdrv->probe) {
		ret = hdrv->probe(hdev, id);
	} else { /* default probe */
		ret = hid_open_report(hdev);
		if (!ret)
			ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
	}
}

Since hid_parse() is just a wrapper around hid_open_report(), could
eweadn_probe() and the .probe assignment in eweadn_driver be dropped to
rely on the default HID core probe like other report-fixup-only drivers?

> +
> +static const struct hid_device_id eweadn_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_YJXCHIP, USB_DEVICE_ID_YJXCHIP_MOUSE) },
> +	{ HID_USB_DEVICE(0xa8a4, 0x2255) },	/* replacement unit, wired mouse */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, eweadn_devices);
> +
> +static struct hid_driver eweadn_driver = {
> +	.name = "eweadn",
> +	.id_table = eweadn_devices,
> +	.report_fixup = eweadn_report_fixup,
> +	.probe = eweadn_probe,
> +};

[Severity: Low]
As reported by Sashiko on v1 [1], does this fail to build when
CONFIG_HID_EWEADN is enabled because drivers/hid/hid-eweadn.c uses
USB_VENDOR_ID_YJXCHIP and USB_DEVICE_ID_YJXCHIP_MOUSE in eweadn_devices[]
without including "hid-ids.h"?

Those macros are defined in drivers/hid/hid-ids.h:

#define USB_VENDOR_ID_YJXCHIP		0xa8a5
#define USB_DEVICE_ID_YJXCHIP_MOUSE	0x2255

Should drivers/hid/hid-eweadn.c include "hid-ids.h", define symbolic
constants in drivers/hid/hid-ids.h for the 0xa8a4:0x2255 device ID entry in
eweadn_devices[] as well, and mark eweadn_rdesc[] as const?

[1] https://lore.kernel.org/r/6ae7529d.10abdf86.7545b.3e9a@mx.google.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6ae759e9.4b0417f2.1062f7.b196@mx.google.com?part=1