Messages in this thread | | | Date | Sun, 12 Feb 2023 13:35:55 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip. |
| |
On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote: > From: Ronald Tschalär <ronald@innovation.ch> > > 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@gmail.com>
...
> +#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
| |