Re: [PATCH v5 5/8] HID: apple: Add support for DockChannel HID keyboards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] HID: apple: redundant apple_match() duplicates
  hid_check_device_match()
--

commit 9add0dddf8a7b243671bc6303c493d3097cdd737
Author: Michael Reeves <michael.reeves077@gmail.com>

HID: apple: Add support for DockChannel HID keyboards

This commit adds HID_GROUP_APPLE_DOCKCHANNEL and matches host-bus Apple
DockChannel keyboards in hid-apple using the modern Magic Keyboard
function-key translation table.

> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 5b32fb8c77f3f..d0059a3c0ac7e 100644
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c

[ ... ]

> @@ -1025,6 +1035,18 @@ static void apple_remove(struct hid_device *hdev)
>  	hid_hw_stop(hdev);
>  }
>  
> +static bool apple_match(struct hid_device *hdev, bool ignore_special_driver)
> +{
> +	if (ignore_special_driver ||
> +	    (hdev->quirks & HID_QUIRK_IGNORE_SPECIAL_DRIVER))
> +		return false;
> +
> +	if (hdev->group == HID_GROUP_APPLE_DOCKCHANNEL)
> +		return apple_is_dockchannel_keyboard(hdev);
> +
> +	return true;
> +}
> +
>  static const struct hid_device_id apple_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE),
>  		.driver_data = APPLE_MIGHTYMOUSE | APPLE_INVERT_HWHEEL },
> @@ -1250,6 +1272,9 @@ static const struct hid_device_id apple_devices[] = {
>  		.driver_data = APPLE_HAS_FN | APPLE_ISO_TILDE_QUIRK },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT),
>  		.driver_data = APPLE_MAGIC_BACKLIGHT },
> +	{ HID_DEVICE(BUS_HOST, HID_GROUP_APPLE_DOCKCHANNEL,
> +		     HID_ANY_ID, HID_ANY_ID),
> +		.driver_data = APPLE_HAS_FN | APPLE_ISO_TILDE_QUIRK },
>  
>  	{ }
>  };
> @@ -1258,6 +1283,7 @@ MODULE_DEVICE_TABLE(hid, apple_devices);
>  static struct hid_driver apple_driver = {
>  	.name = "apple",
>  	.id_table = apple_devices,
> +	.match = apple_match,

[Severity: Low]
Is apple_match() needed here?

hid_check_device_match() first matches against apple_devices[], where the
HID_DEVICE(BUS_HOST, HID_GROUP_APPLE_DOCKCHANNEL, ...) entry already
requires hdev->bus == BUS_HOST and hdev->group ==
HID_GROUP_APPLE_DOCKCHANNEL.  When hdrv->match is NULL,
hid_check_device_match() also enforces !hid_ignore_special_drivers and
!(hdev->quirks & HID_QUIRK_IGNORE_SPECIAL_DRIVER) itself:

drivers/hid/hid-core.c:hid_check_device_match() {
	*id = hid_match_device(hdev, hdrv);
	if (!*id)
		return false;

	if (hdrv->match)
		return hdrv->match(hdev, hid_ignore_special_drivers);

	/*
	 * hid-generic implements .match(), so we must be dealing with a
	 * different HID driver here, and can simply check if
	 * hid_ignore_special_drivers or HID_QUIRK_IGNORE_SPECIAL_DRIVER
	 * are set or not.
	 */
	return !hid_ignore_special_drivers && !(hdev->quirks & HID_QUIRK_IGNORE_SPECIAL_DRIVER);
}

Setting .match on apple_driver duplicates those checks and causes
__hid_bus_driver_added() to run __hid_bus_reprobe_drivers() across all HID
devices whenever any HID driver is registered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-b4-apple-mtp-keyboard-v5-0-e2c97b82669a@gmail.com?part=5