Re: [PATCH] HID: ayaneo-flip-kb: Add driver for keyboard backlight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] HID: ayaneo-flip-kb: spurious error log in flip_kb_probe()
- [Low] HID: ayaneo-flip-kb: missing HID_CONNECT_DRIVER in
  flip_kb_probe()
--

commit 9c851b9f20ca56279eed558bac4b9d796257c299
Author: Hijae Song <hijae99@gmail.com>

HID: ayaneo-flip-kb: Add driver for keyboard backlight

This commit adds a HID driver exposing the RGB keyboard backlight of the
AYANEO Flip KB as a multicolor LED class device.

> diff --git a/drivers/hid/hid-ayaneo-flip-kb.c b/drivers/hid/hid-ayaneo-flip-kb.c
> new file mode 100644
> index 0000000000000..8068b7d44eae2
> --- /dev/null
> +++ b/drivers/hid/hid-ayaneo-flip-kb.c

[ ... ]

> +static int flip_kb_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct flip_kb *kb;
> +	int ret, i;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Both interfaces match this driver, which keeps hid-generic from
> +	 * binding to them, so the boot keyboard interface must be started
> +	 * normally and left alone.
> +	 */
> +	if (!hid_validate_values(hdev, HID_FEATURE_REPORT, FLIP_KB_REPORT_ID,
> +				 0, 1))
> +		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);

[Severity: Low]
Does calling hid_validate_values() here log a spurious error every time the
boot keyboard interface is probed?

hid_validate_values() logs hid_err(hid, "missing %s %u\n", ...) whenever the
requested report ID is absent, so probing the boot keyboard interface always
emits a false error message.  Also, if the vendor interface has a malformed
report 0x41 descriptor, hid_validate_values() returns NULL and causes
flip_kb_probe() to treat it as the boot keyboard interface instead of
failing probe.

> +
> +	kb = devm_kzalloc(&hdev->dev, sizeof(*kb), GFP_KERNEL);
> +	if (!kb)
> +		return -ENOMEM;
> +
> +	kb->hdev = hdev;
> +	hid_set_drvdata(hdev, kb);
> +
> +	kb->subled[0].color_index = LED_COLOR_ID_RED;
> +	kb->subled[1].color_index = LED_COLOR_ID_GREEN;
> +	kb->subled[2].color_index = LED_COLOR_ID_BLUE;
> +	for (i = 0; i < 3; i++)
> +		kb->subled[i].intensity = 255;
> +
> +	kb->mc_led.subled_info = kb->subled;
> +	kb->mc_led.num_colors = 3;
> +	kb->mc_led.led_cdev.name = "ayaneo:rgb:" LED_FUNCTION_KBD_BACKLIGHT;
> +	kb->mc_led.led_cdev.max_brightness = 255;
> +	/* Keep the backlight lit across driver unbind instead of blanking it. */
> +	kb->mc_led.led_cdev.flags = LED_RETAIN_AT_SHUTDOWN;
> +	kb->mc_led.led_cdev.brightness_set_blocking = flip_kb_brightness_set;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		return ret;

[Severity: Low]
Will hid_hw_start() fail with -ENODEV on the backlight interface when both
CONFIG_HIDRAW and CONFIG_USB_HIDDEV are disabled?

Because the vendor-defined interface has no input application usages and
flip_kb_driver has no .raw_event callback, hid_connect() finds hdev->claimed
== 0 when hidraw and hiddev are disabled and returns -ENODEV ("device has no
listeners, quitting"), preventing the LED device from registering.  Passing
HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER (or HID_CONNECT_DRIVER) marks the
interface claimed by the driver.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108110757.41368-1-hijae99@gmail.com?part=1