Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
> 
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
> 
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and on devices with the T1 chip one of the HID devices is
> used for both functions. So this driver creates virtual HID devices, one
> per top-level report collection on each HID device (for a total of 3
> virtual HID devices). The sub-drivers then bind to these virtual HID
> devices.
> 
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices. And those drivers then work (mostly)
> without further changes on MacBooks with the T2 chip that don't need
> this driver.

...

> [Kerem Karabay: convert to a platform driver]
> [Kerem Karabay: fix appleib_forward_int_op]
> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]

If somebody is going to update this (and update seems required for upstreaming)
the list of changes will grow. I suggest to consider Co-developed-by and move
these lines to cover-letter changelog.

> Signed-off-by: Kerem Karabay <kekrby@xxxxxxxxx>

...

> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>

Can we keep it sorted?

> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"

+ Blank line?

> +#include "apple-ibridge.h"

...

> +static struct hid_device_id appleib_sub_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_TB) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +static struct {
> +	unsigned int usage;
> +	struct hid_device_id *dev_id;
> +} appleib_usage_map[] = {
> +	/* Default iBridge configuration, key inputs and mode settings */
> +	{ 0x00010006, &appleib_sub_hid_ids[0] },
> +	/* OS X iBridge configuration, digitizer inputs */
> +	{ 0x000D0005, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, display/DFR settings */
> +	{ 0xFF120001, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, ALS */
> +	{ 0x00200041, &appleib_sub_hid_ids[1] },
> +};

Shouldn't be other way around, i.e. via driver_data?

...

> +struct appleib_device {
> +	acpi_handle asoc_socw;
> +};

What's the point of having struct out of a single member? Can you use it directly?
(you can try and see if it's not ugly, in some cases struct can be justified)

...

> +	bool			sub_open[ARRAY_SIZE(appleib_sub_hid_ids)];

Why not using bitmap?

	DECLARE_BITMAP(sub_open, ARRAY_SIZE(...));

...

> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> +				  unsigned int *rsize)

Why __ types are in use? Is it part of ABI?

...

> +static int appleib_forward_int_op(struct hid_device *hdev,

> +				  int (*forward)(struct hid_driver *,
> +						 struct hid_device *, void *),

This can be on one line

> +				  void *args)

...

> +	if (drv->suspend)
> +		rc = drv->suspend(hdev, *(pm_message_t *)args);

This looks like a hack. What's going on here and why the pm_message_t is in
use? All new PM callbacks do not use it.

...

> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		/*
> +		 * hid_hw_open(), and hence appleib_ll_open(), is called
> +		 * from the driver's probe function, which in turn is called
> +		 * while adding the sub-hdev; but at this point we haven't yet
> +		 * added the sub-hdev to our list. So if we don't find the
> +		 * sub-hdev in our list assume it's in the process of being
> +		 * added and set the flag on the first unset sub-hdev.
> +		 */
> +		if (hdev_info->sub_hdevs[i] == hdev ||
> +		    !hdev_info->sub_hdevs[i]) {

Unusual order of || operator arguments.

This will have a side effect, i.e. if hdev is equal to NULL it will go to the
true branch. Is it by design?

> +			WRITE_ONCE(hdev_info->sub_open[i], open);
> +			return 0;
> +		}
> +	}

...

> +				while (i-- > 0)

while (i--) ?

> +					hid_destroy_device(hdev_info->sub_hdevs[i]);

> +				return (void *)hdev_info->sub_hdevs[i];

This casting is strange. And entire code piece. You will always return 0
element as a pointer here, why 'i'? Needs a lot of explanation.

...

> +static const struct hid_device_id appleib_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },

> +	{ },

No comma for the terminator entry.

> +};

...

> +#ifdef CONFIG_PM
> +	.suspend = appleib_hid_suspend,
> +	.resume = appleib_hid_resume,
> +	.reset_resume = appleib_hid_reset_resume,
> +#endif

Why not using

	.driver = {
		.pm = ...;
	},

?

...

> +	ret = hid_register_driver(&appleib_hid_driver);
> +	if (ret) {

> +		dev_err(&pdev->dev, "Error registering hid driver: %d\n",
> +			ret);
> +		return ret;

	return dev_err_probe(...);

> +	}

...

> +static int appleib_suspend(struct platform_device *pdev, pm_message_t message)
> +{
> +	struct appleib_device *ib_dev;
> +	int rc;

> +	ib_dev = platform_get_drvdata(pdev);

Just unite it with the definition above.
Ditto for the similar cases here and there.

> +	rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> +	if (ACPI_FAILURE(rc))
> +		dev_warn(&pdev->dev, "SOCW(0) failed: %s\n",
> +			 acpi_format_exception(rc));
> +
> +	return 0;
> +}

...

> +static const struct acpi_device_id appleib_acpi_match[] = {
> +	{ "APP7777", 0 },
> +	{ },

No comma for terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko