| Message ID | 20190422031251.11968-2-ronald@innovation.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Apple iBridge support | expand |
On Sun, 21 Apr 2019 20:12:49 -0700 Ronald Tschalär <ronald@innovation.ch> wrote: > 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, since the > functionality for the touch bar and light sensor is exposed via USB HID > interfaces, and the same HID device is used for multiple functions, this > driver provides a multiplexing layer that allows multiple HID drivers to > be registered for a given HID device. This allows the touch bar and ALS > driver to be separated out into their own modules. > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch Hi Ronald, I've only taken a fairly superficial look at this. A few global things to note though. 1. Please either use kernel-doc style for function descriptions, or do not. Right now you are sort of half way there. 2. There is quite a complex nest of separate structures being allocated, so think about whether they can be simplified. In particular use of container_of macros can allow a lot of forwards and backwards pointers to be dropped if you embed the various structures directly. This obviously needs hid and mfd review though as neither is my area of expertise! Jonathan > > --- > drivers/mfd/Kconfig | 15 + > drivers/mfd/Makefile | 1 + > drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ > include/linux/mfd/apple-ibridge.h | 39 ++ > 4 files changed, 938 insertions(+) > create mode 100644 drivers/mfd/apple-ibridge.c > create mode 100644 include/linux/mfd/apple-ibridge.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 76f9909cf396..d55fa77faacf 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1916,5 +1916,20 @@ config RAVE_SP_CORE > Select this to get support for the Supervisory Processor > device found on several devices in RAVE line of hardware. > > +config MFD_APPLE_IBRIDGE > + tristate "Apple iBridge chip" > + depends on ACPI > + depends on USB_HID > + depends on X86 || COMPILE_TEST > + select MFD_CORE > + help > + This MFD provides the core support for the Apple iBridge chip > + found on recent MacBookPro's. The drivers for the Touch Bar > + (apple-ib-tb) and light sensor (apple-ib-als) need to be > + enabled separately. > + > + To compile this driver as a module, choose M here: the > + module will be called apple-ibridge. > + > endmenu > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 12980a4ad460..c364e0e9d313 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -241,4 +241,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > +obj-$(CONFIG_MFD_APPLE_IBRIDGE) += apple-ibridge.o > > diff --git a/drivers/mfd/apple-ibridge.c b/drivers/mfd/apple-ibridge.c > new file mode 100644 > index 000000000000..56d325396961 > --- /dev/null > +++ b/drivers/mfd/apple-ibridge.c > @@ -0,0 +1,883 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Apple iBridge Driver > + * > + * Copyright (c) 2018 Ronald Tschalär > + */ > + > +/** > + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple > + * iBridge chip (also known as T1 chip) which exposes the touch bar, > + * built-in webcam (iSight), ambient light sensor, and Secure Enclave > + * Processor (SEP) for TouchID. It shows up in the system as a USB device > + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge > + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While > + * the second one is used by MacOS to provide the fancy touch bar > + * functionality with custom buttons etc, this driver just uses the first. > + * > + * In the first (default after boot) configuration, 4 usb interfaces are > + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing > + * the touch bar and the ambient light sensor (and possibly the SEP, > + * though at this point in time nothing is known about that). The webcam > + * interfaces are already handled by the uvcvideo driver; furthermore, the > + * handling of the input reports when "keys" on the touch bar are pressed > + * is already handled properly by the generic USB HID core. This leaves > + * the management of the touch bar modes (e.g. switching between function > + * and special keys when the FN key is pressed), the touch bar display > + * (dimming and turning off), the key-remapping when the FN key is > + * pressed, and handling of the light sensor. > + * > + * This driver is implemented as an MFD driver, with the touch bar and ALS > + * functions implemented by appropriate subdrivers (mfd cells). Because > + * both those are basically hid drivers, but the current kernel driver > + * structure does not allow more than one driver per device, this driver > + * implements a demuxer for hid drivers: it registers itself as a hid > + * driver with the core, and in turn it lets the subdrivers register > + * themselves as hid drivers with this driver; the callbacks from the core > + * are then forwarded to the subdrivers. > + * > + * Lastly, this driver also takes care of the power-management for the > + * iBridge when suspending and resuming. > + */ > + > +#include <linux/acpi.h> > +#include <linux/device.h> > +#include <linux/hid.h> > +#include <linux/list.h> > +#include <linux/mfd/apple-ibridge.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/rculist.h> > +#include <linux/slab.h> > +#include <linux/srcu.h> > +#include <linux/usb.h> > + > +#include "../hid/usbhid/usbhid.h" > + > +#define USB_ID_VENDOR_APPLE 0x05ac > +#define USB_ID_PRODUCT_IBRIDGE 0x8600 > + > +#define APPLETB_BASIC_CONFIG 1 > + > +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev) > + > +struct appleib_device { > + struct acpi_device *acpi_dev; > + acpi_handle asoc_socw; > + struct list_head hid_drivers; > + struct list_head hid_devices; > + struct mfd_cell *subdevs; > + struct mutex update_lock; /* protect updates to all lists */ > + struct srcu_struct lists_srcu; > + bool in_hid_probe; > +}; > + > +struct appleib_hid_drv_info { > + struct list_head entry; > + struct hid_driver *driver; > + void *driver_data; > +}; > + > +struct appleib_hid_dev_info { > + struct list_head entry; > + struct list_head drivers; > + struct hid_device *device; > + const struct hid_device_id *device_id; > + bool started; > +}; > + > +static const struct mfd_cell appleib_subdevs[] = { > + { .name = PLAT_NAME_IB_TB }, > + { .name = PLAT_NAME_IB_ALS }, > +}; > + > +static struct appleib_device *appleib_dev; > + > +#define call_void_driver_func(drv_info, fn, ...) \ This sort of macro may seem like a good idea because it saves a few lines of code. However, that comes at the cost of readability, so just put the code inline. > + do { \ > + if ((drv_info)->driver->fn) \ > + (drv_info)->driver->fn(__VA_ARGS__); \ > + } while (0) > + > +#define call_driver_func(drv_info, fn, ret_type, ...) \ > + ({ \ > + ret_type rc = 0; \ > + \ > + if ((drv_info)->driver->fn) \ > + rc = (drv_info)->driver->fn(__VA_ARGS__); \ > + \ > + rc; \ > + }) > + > +static void appleib_remove_driver(struct appleib_device *ib_dev, > + struct appleib_hid_drv_info *drv_info, > + struct appleib_hid_dev_info *dev_info) > +{ > + list_del_rcu(&drv_info->entry); > + synchronize_srcu(&ib_dev->lists_srcu); > + > + call_void_driver_func(drv_info, remove, dev_info->device); > + > + kfree(drv_info); > +} > + > +static int appleib_probe_driver(struct appleib_hid_drv_info *drv_info, > + struct appleib_hid_dev_info *dev_info) > +{ > + struct appleib_hid_drv_info *d; > + int rc = 0; > + > + rc = call_driver_func(drv_info, probe, int, dev_info->device, > + dev_info->device_id); > + if (rc) > + return rc; > + > + d = kmemdup(drv_info, sizeof(*drv_info), GFP_KERNEL); > + if (!d) { > + call_void_driver_func(drv_info, remove, dev_info->device); > + return -ENOMEM; > + } > + > + list_add_tail_rcu(&d->entry, &dev_info->drivers); > + return 0; > +} > + > +static void appleib_remove_driver_attachments(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + struct appleib_hid_drv_info *tmp; > + > + list_for_each_entry_safe(drv_info, tmp, &dev_info->drivers, entry) { > + if (!driver || drv_info->driver == driver) > + appleib_remove_driver(ib_dev, drv_info, dev_info); > + } > +} > + > +/* > + * Find all devices that are attached to this driver and detach them. > + * > + * Note: this must be run with update_lock held. > + */ > +static void appleib_detach_devices(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_dev_info *dev_info; > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) > + appleib_remove_driver_attachments(ib_dev, dev_info, driver); > +} > + > +/* > + * Find all drivers that are attached to this device and detach them. > + * > + * Note: this must be run with update_lock held. > + */ > +static void appleib_detach_drivers(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info) > +{ > + appleib_remove_driver_attachments(ib_dev, dev_info, NULL); > +} > + > +/** > + * Unregister a previously registered HID driver from us. > + * @ib_dev: the appleib_device from which to unregister the driver > + * @driver: the driver to unregister > + */ > +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { > + if (drv_info->driver == driver) { This block does look like it perhaps should be in helper function? Would help with readability. > + appleib_detach_devices(ib_dev, driver); > + list_del_rcu(&drv_info->entry); > + mutex_unlock(&ib_dev->update_lock); > + synchronize_srcu(&ib_dev->lists_srcu); > + kfree(drv_info); > + dev_info(LOG_DEV(ib_dev), "unregistered driver '%s'\n", > + driver->name); > + return 0; > + } > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + return -ENOENT; > +} > +EXPORT_SYMBOL_GPL(appleib_unregister_hid_driver); > + > +static int appleib_start_hid_events(struct appleib_hid_dev_info *dev_info) > +{ > + struct hid_device *hdev = dev_info->device; > + int rc; > + > + rc = hid_connect(hdev, HID_CONNECT_DEFAULT); > + if (rc) { > + hid_err(hdev, "ib: hid connect failed (%d)\n", rc); > + return rc; > + } > + > + rc = hid_hw_open(hdev); > + if (rc) { > + hid_err(hdev, "ib: failed to open hid: %d\n", rc); > + hid_disconnect(hdev); > + } > + > + if (!rc) > + dev_info->started = true; > + > + return rc; > +} > + > +static void appleib_stop_hid_events(struct appleib_hid_dev_info *dev_info) > +{ > + if (dev_info->started) { > + hid_hw_close(dev_info->device); > + hid_disconnect(dev_info->device); > + dev_info->started = false; > + } > +} > + > +/** > + * Register a HID driver with us. > + * @ib_dev: the appleib_device with which to register the driver > + * @driver: the driver to register > + * @data: the driver-data to associate with the driver; this is available > + * from appleib_get_drvdata(...). > + */ > +int appleib_register_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver, void *data) > +{ > + struct appleib_hid_drv_info *drv_info; > + struct appleib_hid_dev_info *dev_info; > + int rc; > + > + if (!driver->probe) > + return -EINVAL; > + > + drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL); > + if (!drv_info) > + return -ENOMEM; > + > + drv_info->driver = driver; > + drv_info->driver_data = data; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_add_tail_rcu(&drv_info->entry, &ib_dev->hid_drivers); > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { > + appleib_stop_hid_events(dev_info); > + > + appleib_probe_driver(drv_info, dev_info); > + > + rc = appleib_start_hid_events(dev_info); > + if (rc) > + appleib_detach_drivers(ib_dev, dev_info); > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + dev_info(LOG_DEV(ib_dev), "registered driver '%s'\n", driver->name); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(appleib_register_hid_driver); > + > +/** > + * Get the driver-specific data associated with the given, previously > + * registered HID driver (provided in the appleib_register_hid_driver() > + * call). > + * @ib_dev: the appleib_device with which the driver was registered > + * @driver: the driver for which to get the data > + */ > +void *appleib_get_drvdata(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + void *drv_data = NULL; > + int idx; > + > + idx = srcu_read_lock(&ib_dev->lists_srcu); > + > + list_for_each_entry_rcu(drv_info, &ib_dev->hid_drivers, entry) { > + if (drv_info->driver == driver) { > + drv_data = drv_info->driver_data; > + break; > + } > + } > + > + srcu_read_unlock(&ib_dev->lists_srcu, idx); > + > + return drv_data; > +} > +EXPORT_SYMBOL_GPL(appleib_get_drvdata); > + > +/* > + * Forward a hid-driver callback to all registered sub-drivers. This is for > + * callbacks that return a status as an int. > + * @hdev the hid-device > + * @forward a function that calls the callback on the given driver > + * @args arguments for the forward function > + */ > +static int appleib_forward_int_op(struct hid_device *hdev, > + int (*forward)(struct appleib_hid_drv_info *, > + struct hid_device *, void *), > + void *args) > +{ > + struct appleib_device *ib_dev = hid_get_drvdata(hdev); > + struct appleib_hid_dev_info *dev_info; > + struct appleib_hid_drv_info *drv_info; > + int idx; > + int rc = 0; > + > + idx = srcu_read_lock(&ib_dev->lists_srcu); > + > + list_for_each_entry_rcu(dev_info, &ib_dev->hid_devices, entry) { > + if (dev_info->device != hdev) > + continue; > + > + list_for_each_entry_rcu(drv_info, &dev_info->drivers, entry) { > + rc = forward(drv_info, hdev, args); > + if (rc) > + break; > + } > + > + break; > + } > + > + srcu_read_unlock(&ib_dev->lists_srcu, idx); > + > + return rc; > +} > + > +struct appleib_hid_event_args { > + struct hid_field *field; > + struct hid_usage *usage; > + __s32 value; > +}; > + > +static int appleib_hid_event_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + struct appleib_hid_event_args *evt_args = args; > + > + return call_driver_func(drv_info, event, int, hdev, evt_args->field, > + evt_args->usage, evt_args->value); > +} > + > +static int appleib_hid_event(struct hid_device *hdev, struct hid_field *field, > + struct hid_usage *usage, __s32 value) > +{ > + struct appleib_hid_event_args args = { > + .field = field, > + .usage = usage, > + .value = value, > + }; > + > + return appleib_forward_int_op(hdev, appleib_hid_event_fwd, &args); > +} > + > +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc, > + unsigned int *rsize) > +{ > + /* Some fields have a size of 64 bits, which according to HID 1.11 > + * Section 8.4 is not valid ("An item field cannot span more than 4 > + * bytes in a report"). Furthermore, hid_field_extract() complains > + * when encountering such a field. So turn them into two 32-bit fields > + * instead. > + */ > + > + if (*rsize == 634 && > + /* Usage Page 0xff12 (vendor defined) */ > + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && > + /* Usage 0x51 */ > + rdesc[416] == 0x09 && rdesc[417] == 0x51 && > + /* report size 64 */ > + rdesc[432] == 0x75 && rdesc[433] == 64 && > + /* report count 1 */ > + rdesc[434] == 0x95 && rdesc[435] == 1) { > + rdesc[433] = 32; > + rdesc[435] = 2; > + hid_dbg(hdev, "Fixed up first 64-bit field\n"); > + } > + > + if (*rsize == 634 && > + /* Usage Page 0xff12 (vendor defined) */ > + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && > + /* Usage 0x51 */ > + rdesc[611] == 0x09 && rdesc[612] == 0x51 && > + /* report size 64 */ > + rdesc[627] == 0x75 && rdesc[628] == 64 && > + /* report count 1 */ > + rdesc[629] == 0x95 && rdesc[630] == 1) { > + rdesc[628] = 32; > + rdesc[630] = 2; > + hid_dbg(hdev, "Fixed up second 64-bit field\n"); > + } > + > + return rdesc; > +} > + > +static int appleib_input_configured_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, input_configured, int, hdev, > + (struct hid_input *)args); > +} > + > +static int appleib_input_configured(struct hid_device *hdev, > + struct hid_input *hidinput) > +{ > + return appleib_forward_int_op(hdev, appleib_input_configured_fwd, > + hidinput); > +} > + > +#ifdef CONFIG_PM > +static int appleib_hid_suspend_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, suspend, int, hdev, > + *(pm_message_t *)args); > +} > + > +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message); > +} > + > +static int appleib_hid_resume_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, resume, int, hdev); > +} > + > +static int appleib_hid_resume(struct hid_device *hdev) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL); > +} > + > +static int appleib_hid_reset_resume_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, reset_resume, int, hdev); > +} > + > +static int appleib_hid_reset_resume(struct hid_device *hdev) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL); > +} > +#endif /* CONFIG_PM */ > + > +/** > + * Find the field in the report with the given usage. > + * @report: the report to search > + * @field_usage: the usage of the field to search for > + */ > +struct hid_field *appleib_find_report_field(struct hid_report *report, > + unsigned int field_usage) > +{ > + int f, u; > + > + for (f = 0; f < report->maxfield; f++) { > + struct hid_field *field = report->field[f]; > + > + if (field->logical == field_usage) > + return field; > + > + for (u = 0; u < field->maxusage; u++) { > + if (field->usage[u].hid == field_usage) > + return field; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(appleib_find_report_field); > + > +/** Please use correct kernel-doc style rather than parts of it. > + * Search all the reports of the device for the field with the given usage. > + * @hdev: the device whose reports to search > + * @application: the usage of application collection that the field must > + * belong to > + * @field_usage: the usage of the field to search for > + */ > +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, > + unsigned int application, > + unsigned int field_usage) > +{ > + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT, > + HID_FEATURE_REPORT }; > + struct hid_report *report; > + struct hid_field *field; > + int t; > + > + for (t = 0; t < ARRAY_SIZE(report_types); t++) { > + struct list_head *report_list = > + &hdev->report_enum[report_types[t]].report_list; > + list_for_each_entry(report, report_list, list) { > + if (report->application != application) > + continue; > + > + field = appleib_find_report_field(report, field_usage); > + if (field) > + return field; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(appleib_find_hid_field); > + > +/** > + * Return whether we're currently inside a hid_device_probe or not. > + * @ib_dev: the appleib_device > + */ > +bool appleib_in_hid_probe(struct appleib_device *ib_dev) > +{ > + return ib_dev->in_hid_probe; > +} > +EXPORT_SYMBOL_GPL(appleib_in_hid_probe); > + > +static struct appleib_hid_dev_info * > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct appleib_hid_dev_info *dev_info; > + struct appleib_hid_drv_info *drv_info; > + > + /* allocate device-info for this device */ > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); > + if (!dev_info) > + return NULL; > + > + INIT_LIST_HEAD(&dev_info->drivers); > + dev_info->device = hdev; > + dev_info->device_id = id; > + > + /* notify all our sub drivers */ > + mutex_lock(&ib_dev->update_lock); > + This is interesting. I'd like to see a comment here on what this flag is going to do. > + ib_dev->in_hid_probe = true; > + > + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { > + appleib_probe_driver(drv_info, dev_info); > + } > + > + ib_dev->in_hid_probe = false; > + > + /* remember this new device */ > + list_add_tail_rcu(&dev_info->entry, &ib_dev->hid_devices); > + > + mutex_unlock(&ib_dev->update_lock); > + > + return dev_info; > +} > + > +static void appleib_remove_device(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info) > +{ > + list_del_rcu(&dev_info->entry); > + synchronize_srcu(&ib_dev->lists_srcu); > + > + appleib_detach_drivers(ib_dev, dev_info); > + > + kfree(dev_info); > +} > + > +static int appleib_hid_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct appleib_device *ib_dev; > + struct appleib_hid_dev_info *dev_info; > + struct usb_device *udev; > + int rc; > + > + /* check usb config first */ > + udev = hid_to_usb_dev(hdev); > + > + if (udev->actconfig->desc.bConfigurationValue != APPLETB_BASIC_CONFIG) { > + rc = usb_driver_set_configuration(udev, APPLETB_BASIC_CONFIG); > + return rc ? rc : -ENODEV; > + } > + > + /* Assign the driver data */ > + ib_dev = appleib_dev; > + hid_set_drvdata(hdev, ib_dev); > + > + /* initialize the report info */ > + rc = hid_parse(hdev); > + if (rc) { > + hid_err(hdev, "ib: hid parse failed (%d)\n", rc); > + goto error; > + } > + > + /* alloc bufs etc so probe's can send requests; but connect later */ > + rc = hid_hw_start(hdev, 0); > + if (rc) { > + hid_err(hdev, "ib: hw start failed (%d)\n", rc); > + goto error; > + } > + > + /* add this hdev to our device list */ > + dev_info = appleib_add_device(ib_dev, hdev, id); > + if (!dev_info) { > + rc = -ENOMEM; > + goto stop_hw; > + } > + > + /* start the hid */ > + rc = appleib_start_hid_events(dev_info); > + if (rc) > + goto remove_dev; > + > + return 0; > + > +remove_dev: > + mutex_lock(&ib_dev->update_lock); > + appleib_remove_device(ib_dev, dev_info); > + mutex_unlock(&ib_dev->update_lock); > +stop_hw: > + hid_hw_stop(hdev); > +error: > + return rc; > +} > + > +static void appleib_hid_remove(struct hid_device *hdev) > +{ > + struct appleib_device *ib_dev = hid_get_drvdata(hdev); > + struct appleib_hid_dev_info *dev_info; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { > + if (dev_info->device == hdev) { > + appleib_stop_hid_events(dev_info); > + appleib_remove_device(ib_dev, dev_info); > + break; > + } > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + hid_hw_stop(hdev); > +} > + > +static const struct hid_device_id appleib_hid_devices[] = { > + { HID_USB_DEVICE(USB_ID_VENDOR_APPLE, USB_ID_PRODUCT_IBRIDGE) }, > + { }, > +}; > + > +static struct hid_driver appleib_hid_driver = { > + .name = "apple-ibridge-hid", > + .id_table = appleib_hid_devices, > + .probe = appleib_hid_probe, > + .remove = appleib_hid_remove, > + .event = appleib_hid_event, > + .report_fixup = appleib_report_fixup, > + .input_configured = appleib_input_configured, > +#ifdef CONFIG_PM > + .suspend = appleib_hid_suspend, > + .resume = appleib_hid_resume, > + .reset_resume = appleib_hid_reset_resume, > +#endif > +}; > + > +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev) > +{ > + struct appleib_device *ib_dev; > + acpi_status sts; > + int rc; > + > + /* allocate */ Drop comments that don't anything a quick glance at the code would tell you. > + ib_dev = kzalloc(sizeof(*ib_dev), GFP_KERNEL); > + if (!ib_dev) > + return ERR_PTR(-ENOMEM); > + > + /* init structures */ > + INIT_LIST_HEAD(&ib_dev->hid_drivers); > + INIT_LIST_HEAD(&ib_dev->hid_devices); > + mutex_init(&ib_dev->update_lock); > + init_srcu_struct(&ib_dev->lists_srcu); > + > + ib_dev->acpi_dev = acpi_dev; > + > + /* get iBridge acpi power control method */ > + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw); > + if (ACPI_FAILURE(sts)) { > + dev_err(LOG_DEV(ib_dev), > + "Error getting handle for ASOC.SOCW method: %s\n", > + acpi_format_exception(sts)); > + rc = -ENXIO; > + goto free_mem; > + } > + > + /* ensure iBridge is powered on */ > + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); > + if (ACPI_FAILURE(sts)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", > + acpi_format_exception(sts)); > + > + return ib_dev; > + > +free_mem: > + kfree(ib_dev); > + return ERR_PTR(rc); > +} > + > +static int appleib_probe(struct acpi_device *acpi) > +{ > + struct appleib_device *ib_dev; > + struct appleib_platform_data *pdata; Platform_data has a lot of historical meaning in Linux. Also you have things in here that are not platform related at all, such as the dev pointer. Hence I would rename it as device_data or private or something like that. > + int i; > + int ret; > + > + if (appleib_dev) This singleton bothers me a bit. I'm really not sure why it is necessary. You can just put a pointer to this in the pdata for the subdevs and I think that covers most of your usecases. It's generally a bad idea to limit things to one instance of a device unless that actually major simplifications. I'm not seeing them here. > + return -EBUSY; > + > + ib_dev = appleib_alloc_device(acpi); > + if (IS_ERR_OR_NULL(ib_dev)) > + return PTR_ERR(ib_dev); > + > + ib_dev->subdevs = kmemdup(appleib_subdevs, sizeof(appleib_subdevs), > + GFP_KERNEL); Given this is fixed sized and always referenced via ib_dev->subdevs, just put the array in there and memcpy into it. That way you have one less allocation and simpler code. > + if (!ib_dev->subdevs) { > + ret = -ENOMEM; > + goto free_dev; > + } > + > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); Might as well embed this in ib_dev as well. That would let you used container_of to avoid having to carry the ib_dev pointer around in side pdata. > + if (!pdata) { > + ret = -ENOMEM; > + goto free_subdevs; > + } > + > + pdata->ib_dev = ib_dev; > + pdata->log_dev = LOG_DEV(ib_dev); > + for (i = 0; i < ARRAY_SIZE(appleib_subdevs); i++) { > + ib_dev->subdevs[i].platform_data = pdata; > + ib_dev->subdevs[i].pdata_size = sizeof(*pdata); > + } > + > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE, > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs), > + NULL, 0, NULL); > + if (ret) { > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret); > + goto free_pdata; > + } > + > + acpi->driver_data = ib_dev; > + appleib_dev = ib_dev; > + > + ret = hid_register_driver(&appleib_hid_driver); > + if (ret) { > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n", > + ret); > + goto rem_mfd_devs; > + } > + > + return 0; > + > +rem_mfd_devs: > + mfd_remove_devices(&acpi->dev); > +free_pdata: > + kfree(pdata); > +free_subdevs: > + kfree(ib_dev->subdevs); > +free_dev: > + appleib_dev = NULL; > + acpi->driver_data = NULL; Why at this point? It's not set to anything until much later in the probe flow. May be worth thinking about devm_ managed allocations to cleanup some of these allocations automatically and simplify the error handling. > + kfree(ib_dev); > + return ret; > +} > + > +static int appleib_remove(struct acpi_device *acpi) > +{ > + struct appleib_device *ib_dev = acpi_driver_data(acpi); > + > + mfd_remove_devices(&acpi->dev); > + hid_unregister_driver(&appleib_hid_driver); > + > + if (appleib_dev == ib_dev) From a general reviewability point of view, it's nice to keep the remove in the same order as the cleanup on error in probe (and hence reverse of probe). That measn this should be a little further down. I'd also like to see a comment on how this condition can be false. > + appleib_dev = NULL; > + > + kfree(ib_dev->subdevs[0].platform_data); > + kfree(ib_dev->subdevs); > + kfree(ib_dev); Is it worth considering devm in here to avoid the need to clean all these up by hand? > + > + return 0; > +} > + > +static int appleib_suspend(struct device *dev) > +{ > + struct acpi_device *adev; > + struct appleib_device *ib_dev; > + int rc; > + > + adev = to_acpi_device(dev); > + ib_dev = acpi_driver_data(adev); Given this appears a few times, probably worth the more compact ib_dev = acpi_driver_data(to_acpi_device(dev)); Allowing you to drop the adev local variable that doesn't add any info. > + > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0); > + if (ACPI_FAILURE(rc)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n", I can sort of see you might want to do the LOG_DEV for consistency but here I'm fairly sure it's just dev which might be clearer. > + acpi_format_exception(rc)); > + > + return 0; > +} > + > +static int appleib_resume(struct device *dev) > +{ > + struct acpi_device *adev; > + struct appleib_device *ib_dev; > + int rc; > + > + adev = to_acpi_device(dev); > + ib_dev = acpi_driver_data(adev); > + > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); > + if (ACPI_FAILURE(rc)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", > + acpi_format_exception(rc)); > + > + return 0; > +} > + > +static const struct dev_pm_ops appleib_pm = { > + .suspend = appleib_suspend, > + .resume = appleib_resume, > + .restore = appleib_resume, > +}; > + > +static const struct acpi_device_id appleib_acpi_match[] = { > + { "APP7777", 0 }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match); > + > +static struct acpi_driver appleib_driver = { > + .name = "apple-ibridge", > + .class = "topcase", /* ? */ > + .owner = THIS_MODULE, > + .ids = appleib_acpi_match, > + .ops = { > + .add = appleib_probe, > + .remove = appleib_remove, > + }, > + .drv = { > + .pm = &appleib_pm, > + }, > +}; > + > +module_acpi_driver(appleib_driver) > + > +MODULE_AUTHOR("Ronald Tschalär"); > +MODULE_DESCRIPTION("Apple iBridge driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/apple-ibridge.h b/include/linux/mfd/apple-ibridge.h > new file mode 100644 > index 000000000000..d321714767f7 > --- /dev/null > +++ b/include/linux/mfd/apple-ibridge.h > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Apple iBridge Driver > + * > + * Copyright (c) 2018 Ronald Tschalär > + */ > + > +#ifndef __LINUX_MFD_APPLE_IBRDIGE_H > +#define __LINUX_MFD_APPLE_IBRDIGE_H > + > +#include <linux/device.h> > +#include <linux/hid.h> > + > +#define PLAT_NAME_IB_TB "apple-ib-tb" > +#define PLAT_NAME_IB_ALS "apple-ib-als" > + > +struct appleib_device; > + > +struct appleib_platform_data { > + struct appleib_device *ib_dev; > + struct device *log_dev; > +}; > + > +int appleib_register_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver, void *data); > +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver); > + > +void *appleib_get_drvdata(struct appleib_device *ib_dev, > + struct hid_driver *driver); > +bool appleib_in_hid_probe(struct appleib_device *ib_dev); > + > +struct hid_field *appleib_find_report_field(struct hid_report *report, > + unsigned int field_usage); > +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, > + unsigned int application, > + unsigned int field_usage); > + > +#endif
Hi Jonathan, On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote: > On Sun, 21 Apr 2019 20:12:49 -0700 > Ronald Tschalär <ronald@innovation.ch> wrote: > > > 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, since the > > functionality for the touch bar and light sensor is exposed via USB HID > > interfaces, and the same HID device is used for multiple functions, this > > driver provides a multiplexing layer that allows multiple HID drivers to > > be registered for a given HID device. This allows the touch bar and ALS > > driver to be separated out into their own modules. > > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch > Hi Ronald, > > I've only taken a fairly superficial look at this. A few global > things to note though. Thanks for this review. > 1. Please either use kernel-doc style for function descriptions, or > do not. Right now you are sort of half way there. Apologies, on re-reading the docs I realize what you mean here. Should be fixed now (next rev). > 2. There is quite a complex nest of separate structures being allocated, > so think about whether they can be simplified. In particular > use of container_of macros can allow a lot of forwards and backwards > pointers to be dropped if you embed the various structures directly. Done (see also below). [snip] > > +#define call_void_driver_func(drv_info, fn, ...) \ > > This sort of macro may seem like a good idea because it saves a few lines > of code. However, that comes at the cost of readability, so just > put the code inline. > > > + do { \ > > + if ((drv_info)->driver->fn) \ > > + (drv_info)->driver->fn(__VA_ARGS__); \ > > + } while (0) > > + > > +#define call_driver_func(drv_info, fn, ret_type, ...) \ > > + ({ \ > > + ret_type rc = 0; \ > > + \ > > + if ((drv_info)->driver->fn) \ > > + rc = (drv_info)->driver->fn(__VA_ARGS__); \ > > + \ > > + rc; \ > > + }) Just to clarify, you're only talking about removing/inlining the call_void_driver_func() macro, not the call_driver_func() macro, right? [snip] > > +static struct appleib_hid_dev_info * > > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev, > > + const struct hid_device_id *id) > > +{ > > + struct appleib_hid_dev_info *dev_info; > > + struct appleib_hid_drv_info *drv_info; > > + > > + /* allocate device-info for this device */ > > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); > > + if (!dev_info) > > + return NULL; > > + > > + INIT_LIST_HEAD(&dev_info->drivers); > > + dev_info->device = hdev; > > + dev_info->device_id = id; > > + > > + /* notify all our sub drivers */ > > + mutex_lock(&ib_dev->update_lock); > > + > This is interesting. I'd like to see a comment here on what > this flag is going to do. I'm not sure I follow: update_lock is simply a mutex protecting all driver and device update (i.e. add/remove) functions. Are you therefore looking for something like: /* protect driver and device lists against concurrent updates */ mutex_lock(&ib_dev->update_lock); [snip] > > +static int appleib_probe(struct acpi_device *acpi) > > +{ > > + struct appleib_device *ib_dev; > > + struct appleib_platform_data *pdata; > Platform_data has a lot of historical meaning in Linux. > Also you have things in here that are not platform related > at all, such as the dev pointer. Hence I would rename it > as device_data or private or something like that. Ok. I guess I called in platform_data because it's stored in the mfd cell's "platform_data" field. Anyway, changed it per your suggestion. > > + int i; > > + int ret; > > + > > + if (appleib_dev) > This singleton bothers me a bit. I'm really not sure why it > is necessary. You can just put a pointer to this in > the pdata for the subdevs and I think that covers most of your > usecases. It's generally a bad idea to limit things to one instance > of a device unless that actually major simplifications. > I'm not seeing them here. Yes, this one is quite ugly. appleib_dev is static so that appleib_hid_probe() can find it. I could not find any other way to pass the appleib_dev instance to that probe function. However, on looking at this again, I realized that hid_device_id has a driver_data field which can be used for this; so if I added the hid_driver and hid_device_id structs to the appleib_device (instead of making them static like now) I could fill in the driver_data and avoid this hack. This looks much cleaner. Thanks for pointing this uglyness out again. [snip] > > + if (!ib_dev->subdevs) { > > + ret = -ENOMEM; > > + goto free_dev; > > + } > > + > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > > Might as well embed this in ib_dev as well. Agreed. > That would let > you used container_of to avoid having to carry the ib_dev pointer > around in side pdata. I see. I guess my main reservation is that the functions exported to the sub-drivers would now take a 'struct appleib_device_data *' argument instead of a 'struct appleib_device *', which just seems a bit unnatural. E.g. int appleib_register_hid_driver(struct appleib_device_data *ib_ddata, struct hid_driver *driver, void *data); instead of (the current) int appleib_register_hid_driver(struct appleib_device *ib_dev, struct hid_driver *driver, void *data); [snip] > > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE, > > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs), > > + NULL, 0, NULL); > > + if (ret) { > > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret); > > + goto free_pdata; > > + } > > + > > + acpi->driver_data = ib_dev; > > + appleib_dev = ib_dev; > > + > > + ret = hid_register_driver(&appleib_hid_driver); > > + if (ret) { > > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n", > > + ret); > > + goto rem_mfd_devs; > > + } > > + > > + return 0; > > + > > +rem_mfd_devs: > > + mfd_remove_devices(&acpi->dev); > > +free_pdata: > > + kfree(pdata); > > +free_subdevs: > > + kfree(ib_dev->subdevs); > > +free_dev: > > + appleib_dev = NULL; > > + acpi->driver_data = NULL; > Why at this point? It's not set to anything until much later in the > probe flow. If the hid_register_driver() call fails, we get here after driver_data has been assigned. However, looking at this again, acpi->driver_data is only used by the remove, suspend, and resume callbacks, and those will not be called until a successful return from probe; therefore I can safely move the setting of driver_data to after the hid_register_driver() call and avoid having to set it to NULL in the error cleanup. > May be worth thinking about devm_ managed allocations > to cleanup some of these allocations automatically and simplify > the error handling. Good point, thanks. [snip] > > + > > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0); > > + if (ACPI_FAILURE(rc)) > > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n", > > I can sort of see you might want to do the LOG_DEV for consistency > but here I'm fairly sure it's just dev which might be clearer. Sorry, you mean rename the macro LOG_DEV() to just DEV()? Cheers, Ronald
On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > 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, since the > functionality for the touch bar and light sensor is exposed via USB HID > interfaces, and the same HID device is used for multiple functions, this > driver provides a multiplexing layer that allows multiple HID drivers to > be registered for a given HID device. This allows the touch bar and ALS > driver to be separated out into their own modules. Sorry for coming late to the party, but IMO this series is far too complex for what you need. As I read this and the first comment of drivers/mfd/apple-ibridge.c, you need to have a HID driver that multiplex 2 other sub drivers through one USB communication. For that, you are using MFD, platform driver and you own sauce instead of creating a bus. So, how about we reuse entirely the HID subsystem which already provides the capability you need (assuming I am correct above). hid-logitech-dj already does the same kind of stuff and you could: - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE - hid-ibridge will then register itself to the hid subsystem with a call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and hid_device_io_start(hdev) to enable the events (so you don't create useless input nodes for it) - then you add your 2 new devices by calling hid_allocate_device() and then hid_add_device(). You can even create a new HID group APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them from the actual USB device. - then you have 2 brand new HID devices you can create their driver as a regular ones. hid-ibridge.c would just need to behave like any other hid transport driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and you can get rid of at least the MFD and the platform part of your drivers. Does it makes sense or am I missing something obvious in the middle? I have one other comment below. Note that I haven't read the whole series as I'd like to first get your feedback with my comment above. > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > --- > drivers/mfd/Kconfig | 15 + > drivers/mfd/Makefile | 1 + > drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ > include/linux/mfd/apple-ibridge.h | 39 ++ > 4 files changed, 938 insertions(+) > create mode 100644 drivers/mfd/apple-ibridge.c > create mode 100644 include/linux/mfd/apple-ibridge.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 76f9909cf396..d55fa77faacf 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1916,5 +1916,20 @@ config RAVE_SP_CORE > Select this to get support for the Supervisory Processor > device found on several devices in RAVE line of hardware. > > +config MFD_APPLE_IBRIDGE > + tristate "Apple iBridge chip" > + depends on ACPI > + depends on USB_HID > + depends on X86 || COMPILE_TEST > + select MFD_CORE > + help > + This MFD provides the core support for the Apple iBridge chip > + found on recent MacBookPro's. The drivers for the Touch Bar > + (apple-ib-tb) and light sensor (apple-ib-als) need to be > + enabled separately. > + > + To compile this driver as a module, choose M here: the > + module will be called apple-ibridge. > + > endmenu > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 12980a4ad460..c364e0e9d313 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -241,4 +241,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > +obj-$(CONFIG_MFD_APPLE_IBRIDGE) += apple-ibridge.o > > diff --git a/drivers/mfd/apple-ibridge.c b/drivers/mfd/apple-ibridge.c > new file mode 100644 > index 000000000000..56d325396961 > --- /dev/null > +++ b/drivers/mfd/apple-ibridge.c > @@ -0,0 +1,883 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Apple iBridge Driver > + * > + * Copyright (c) 2018 Ronald Tschalär > + */ > + > +/** > + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple > + * iBridge chip (also known as T1 chip) which exposes the touch bar, > + * built-in webcam (iSight), ambient light sensor, and Secure Enclave > + * Processor (SEP) for TouchID. It shows up in the system as a USB device > + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge > + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While > + * the second one is used by MacOS to provide the fancy touch bar > + * functionality with custom buttons etc, this driver just uses the first. > + * > + * In the first (default after boot) configuration, 4 usb interfaces are > + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing > + * the touch bar and the ambient light sensor (and possibly the SEP, > + * though at this point in time nothing is known about that). The webcam > + * interfaces are already handled by the uvcvideo driver; furthermore, the > + * handling of the input reports when "keys" on the touch bar are pressed > + * is already handled properly by the generic USB HID core. This leaves > + * the management of the touch bar modes (e.g. switching between function > + * and special keys when the FN key is pressed), the touch bar display > + * (dimming and turning off), the key-remapping when the FN key is > + * pressed, and handling of the light sensor. > + * > + * This driver is implemented as an MFD driver, with the touch bar and ALS > + * functions implemented by appropriate subdrivers (mfd cells). Because > + * both those are basically hid drivers, but the current kernel driver > + * structure does not allow more than one driver per device, this driver > + * implements a demuxer for hid drivers: it registers itself as a hid > + * driver with the core, and in turn it lets the subdrivers register > + * themselves as hid drivers with this driver; the callbacks from the core > + * are then forwarded to the subdrivers. > + * > + * Lastly, this driver also takes care of the power-management for the > + * iBridge when suspending and resuming. > + */ > + > +#include <linux/acpi.h> > +#include <linux/device.h> > +#include <linux/hid.h> > +#include <linux/list.h> > +#include <linux/mfd/apple-ibridge.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/rculist.h> > +#include <linux/slab.h> > +#include <linux/srcu.h> > +#include <linux/usb.h> > + > +#include "../hid/usbhid/usbhid.h" > + > +#define USB_ID_VENDOR_APPLE 0x05ac > +#define USB_ID_PRODUCT_IBRIDGE 0x8600 > + > +#define APPLETB_BASIC_CONFIG 1 > + > +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev) > + > +struct appleib_device { > + struct acpi_device *acpi_dev; > + acpi_handle asoc_socw; > + struct list_head hid_drivers; > + struct list_head hid_devices; > + struct mfd_cell *subdevs; > + struct mutex update_lock; /* protect updates to all lists */ > + struct srcu_struct lists_srcu; > + bool in_hid_probe; > +}; > + > +struct appleib_hid_drv_info { > + struct list_head entry; > + struct hid_driver *driver; > + void *driver_data; > +}; > + > +struct appleib_hid_dev_info { > + struct list_head entry; > + struct list_head drivers; > + struct hid_device *device; > + const struct hid_device_id *device_id; > + bool started; > +}; > + > +static const struct mfd_cell appleib_subdevs[] = { > + { .name = PLAT_NAME_IB_TB }, > + { .name = PLAT_NAME_IB_ALS }, > +}; > + > +static struct appleib_device *appleib_dev; > + > +#define call_void_driver_func(drv_info, fn, ...) \ > + do { \ > + if ((drv_info)->driver->fn) \ > + (drv_info)->driver->fn(__VA_ARGS__); \ > + } while (0) > + > +#define call_driver_func(drv_info, fn, ret_type, ...) \ > + ({ \ > + ret_type rc = 0; \ > + \ > + if ((drv_info)->driver->fn) \ > + rc = (drv_info)->driver->fn(__VA_ARGS__); \ > + \ > + rc; \ > + }) > + > +static void appleib_remove_driver(struct appleib_device *ib_dev, > + struct appleib_hid_drv_info *drv_info, > + struct appleib_hid_dev_info *dev_info) > +{ > + list_del_rcu(&drv_info->entry); > + synchronize_srcu(&ib_dev->lists_srcu); > + > + call_void_driver_func(drv_info, remove, dev_info->device); > + > + kfree(drv_info); > +} > + > +static int appleib_probe_driver(struct appleib_hid_drv_info *drv_info, > + struct appleib_hid_dev_info *dev_info) > +{ > + struct appleib_hid_drv_info *d; > + int rc = 0; > + > + rc = call_driver_func(drv_info, probe, int, dev_info->device, > + dev_info->device_id); > + if (rc) > + return rc; > + > + d = kmemdup(drv_info, sizeof(*drv_info), GFP_KERNEL); > + if (!d) { > + call_void_driver_func(drv_info, remove, dev_info->device); > + return -ENOMEM; > + } > + > + list_add_tail_rcu(&d->entry, &dev_info->drivers); > + return 0; > +} > + > +static void appleib_remove_driver_attachments(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + struct appleib_hid_drv_info *tmp; > + > + list_for_each_entry_safe(drv_info, tmp, &dev_info->drivers, entry) { > + if (!driver || drv_info->driver == driver) > + appleib_remove_driver(ib_dev, drv_info, dev_info); > + } > +} > + > +/* > + * Find all devices that are attached to this driver and detach them. > + * > + * Note: this must be run with update_lock held. > + */ > +static void appleib_detach_devices(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_dev_info *dev_info; > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) > + appleib_remove_driver_attachments(ib_dev, dev_info, driver); > +} > + > +/* > + * Find all drivers that are attached to this device and detach them. > + * > + * Note: this must be run with update_lock held. > + */ > +static void appleib_detach_drivers(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info) > +{ > + appleib_remove_driver_attachments(ib_dev, dev_info, NULL); > +} > + > +/** > + * Unregister a previously registered HID driver from us. > + * @ib_dev: the appleib_device from which to unregister the driver > + * @driver: the driver to unregister > + */ > +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { > + if (drv_info->driver == driver) { > + appleib_detach_devices(ib_dev, driver); > + list_del_rcu(&drv_info->entry); > + mutex_unlock(&ib_dev->update_lock); > + synchronize_srcu(&ib_dev->lists_srcu); > + kfree(drv_info); > + dev_info(LOG_DEV(ib_dev), "unregistered driver '%s'\n", > + driver->name); > + return 0; > + } > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + return -ENOENT; > +} > +EXPORT_SYMBOL_GPL(appleib_unregister_hid_driver); > + > +static int appleib_start_hid_events(struct appleib_hid_dev_info *dev_info) > +{ > + struct hid_device *hdev = dev_info->device; > + int rc; > + > + rc = hid_connect(hdev, HID_CONNECT_DEFAULT); > + if (rc) { > + hid_err(hdev, "ib: hid connect failed (%d)\n", rc); > + return rc; > + } > + > + rc = hid_hw_open(hdev); > + if (rc) { > + hid_err(hdev, "ib: failed to open hid: %d\n", rc); > + hid_disconnect(hdev); > + } > + > + if (!rc) > + dev_info->started = true; > + > + return rc; > +} > + > +static void appleib_stop_hid_events(struct appleib_hid_dev_info *dev_info) > +{ > + if (dev_info->started) { > + hid_hw_close(dev_info->device); > + hid_disconnect(dev_info->device); > + dev_info->started = false; > + } > +} > + > +/** > + * Register a HID driver with us. > + * @ib_dev: the appleib_device with which to register the driver > + * @driver: the driver to register > + * @data: the driver-data to associate with the driver; this is available > + * from appleib_get_drvdata(...). > + */ > +int appleib_register_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver, void *data) > +{ > + struct appleib_hid_drv_info *drv_info; > + struct appleib_hid_dev_info *dev_info; > + int rc; > + > + if (!driver->probe) > + return -EINVAL; > + > + drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL); > + if (!drv_info) > + return -ENOMEM; > + > + drv_info->driver = driver; > + drv_info->driver_data = data; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_add_tail_rcu(&drv_info->entry, &ib_dev->hid_drivers); > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { > + appleib_stop_hid_events(dev_info); > + > + appleib_probe_driver(drv_info, dev_info); > + > + rc = appleib_start_hid_events(dev_info); > + if (rc) > + appleib_detach_drivers(ib_dev, dev_info); > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + dev_info(LOG_DEV(ib_dev), "registered driver '%s'\n", driver->name); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(appleib_register_hid_driver); > + > +/** > + * Get the driver-specific data associated with the given, previously > + * registered HID driver (provided in the appleib_register_hid_driver() > + * call). > + * @ib_dev: the appleib_device with which the driver was registered > + * @driver: the driver for which to get the data > + */ > +void *appleib_get_drvdata(struct appleib_device *ib_dev, > + struct hid_driver *driver) > +{ > + struct appleib_hid_drv_info *drv_info; > + void *drv_data = NULL; > + int idx; > + > + idx = srcu_read_lock(&ib_dev->lists_srcu); > + > + list_for_each_entry_rcu(drv_info, &ib_dev->hid_drivers, entry) { > + if (drv_info->driver == driver) { > + drv_data = drv_info->driver_data; > + break; > + } > + } > + > + srcu_read_unlock(&ib_dev->lists_srcu, idx); > + > + return drv_data; > +} > +EXPORT_SYMBOL_GPL(appleib_get_drvdata); > + > +/* > + * Forward a hid-driver callback to all registered sub-drivers. This is for > + * callbacks that return a status as an int. > + * @hdev the hid-device > + * @forward a function that calls the callback on the given driver > + * @args arguments for the forward function > + */ > +static int appleib_forward_int_op(struct hid_device *hdev, > + int (*forward)(struct appleib_hid_drv_info *, > + struct hid_device *, void *), > + void *args) > +{ > + struct appleib_device *ib_dev = hid_get_drvdata(hdev); > + struct appleib_hid_dev_info *dev_info; > + struct appleib_hid_drv_info *drv_info; > + int idx; > + int rc = 0; > + > + idx = srcu_read_lock(&ib_dev->lists_srcu); > + > + list_for_each_entry_rcu(dev_info, &ib_dev->hid_devices, entry) { > + if (dev_info->device != hdev) > + continue; > + > + list_for_each_entry_rcu(drv_info, &dev_info->drivers, entry) { > + rc = forward(drv_info, hdev, args); > + if (rc) > + break; > + } > + > + break; > + } > + > + srcu_read_unlock(&ib_dev->lists_srcu, idx); > + > + return rc; > +} > + > +struct appleib_hid_event_args { > + struct hid_field *field; > + struct hid_usage *usage; > + __s32 value; > +}; > + > +static int appleib_hid_event_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + struct appleib_hid_event_args *evt_args = args; > + > + return call_driver_func(drv_info, event, int, hdev, evt_args->field, > + evt_args->usage, evt_args->value); > +} > + > +static int appleib_hid_event(struct hid_device *hdev, struct hid_field *field, > + struct hid_usage *usage, __s32 value) > +{ > + struct appleib_hid_event_args args = { > + .field = field, > + .usage = usage, > + .value = value, > + }; > + > + return appleib_forward_int_op(hdev, appleib_hid_event_fwd, &args); > +} > + > +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc, > + unsigned int *rsize) > +{ > + /* Some fields have a size of 64 bits, which according to HID 1.11 > + * Section 8.4 is not valid ("An item field cannot span more than 4 > + * bytes in a report"). Furthermore, hid_field_extract() complains this must have been fixed in 94a9992f7dbdfb28976b565af220e0c4a117144a which is part of v5.1, so not sure you actually need the report descriptor fixup at all. Cheers, Benjamin > + * when encountering such a field. So turn them into two 32-bit fields > + * instead. > + */ > + > + if (*rsize == 634 && > + /* Usage Page 0xff12 (vendor defined) */ > + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && > + /* Usage 0x51 */ > + rdesc[416] == 0x09 && rdesc[417] == 0x51 && > + /* report size 64 */ > + rdesc[432] == 0x75 && rdesc[433] == 64 && > + /* report count 1 */ > + rdesc[434] == 0x95 && rdesc[435] == 1) { > + rdesc[433] = 32; > + rdesc[435] = 2; > + hid_dbg(hdev, "Fixed up first 64-bit field\n"); > + } > + > + if (*rsize == 634 && > + /* Usage Page 0xff12 (vendor defined) */ > + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && > + /* Usage 0x51 */ > + rdesc[611] == 0x09 && rdesc[612] == 0x51 && > + /* report size 64 */ > + rdesc[627] == 0x75 && rdesc[628] == 64 && > + /* report count 1 */ > + rdesc[629] == 0x95 && rdesc[630] == 1) { > + rdesc[628] = 32; > + rdesc[630] = 2; > + hid_dbg(hdev, "Fixed up second 64-bit field\n"); > + } > + > + return rdesc; > +} > + > +static int appleib_input_configured_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, input_configured, int, hdev, > + (struct hid_input *)args); > +} > + > +static int appleib_input_configured(struct hid_device *hdev, > + struct hid_input *hidinput) > +{ > + return appleib_forward_int_op(hdev, appleib_input_configured_fwd, > + hidinput); > +} > + > +#ifdef CONFIG_PM > +static int appleib_hid_suspend_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, suspend, int, hdev, > + *(pm_message_t *)args); > +} > + > +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message); > +} > + > +static int appleib_hid_resume_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, resume, int, hdev); > +} > + > +static int appleib_hid_resume(struct hid_device *hdev) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL); > +} > + > +static int appleib_hid_reset_resume_fwd(struct appleib_hid_drv_info *drv_info, > + struct hid_device *hdev, void *args) > +{ > + return call_driver_func(drv_info, reset_resume, int, hdev); > +} > + > +static int appleib_hid_reset_resume(struct hid_device *hdev) > +{ > + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL); > +} > +#endif /* CONFIG_PM */ > + > +/** > + * Find the field in the report with the given usage. > + * @report: the report to search > + * @field_usage: the usage of the field to search for > + */ > +struct hid_field *appleib_find_report_field(struct hid_report *report, > + unsigned int field_usage) > +{ > + int f, u; > + > + for (f = 0; f < report->maxfield; f++) { > + struct hid_field *field = report->field[f]; > + > + if (field->logical == field_usage) > + return field; > + > + for (u = 0; u < field->maxusage; u++) { > + if (field->usage[u].hid == field_usage) > + return field; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(appleib_find_report_field); > + > +/** > + * Search all the reports of the device for the field with the given usage. > + * @hdev: the device whose reports to search > + * @application: the usage of application collection that the field must > + * belong to > + * @field_usage: the usage of the field to search for > + */ > +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, > + unsigned int application, > + unsigned int field_usage) > +{ > + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT, > + HID_FEATURE_REPORT }; > + struct hid_report *report; > + struct hid_field *field; > + int t; > + > + for (t = 0; t < ARRAY_SIZE(report_types); t++) { > + struct list_head *report_list = > + &hdev->report_enum[report_types[t]].report_list; > + list_for_each_entry(report, report_list, list) { > + if (report->application != application) > + continue; > + > + field = appleib_find_report_field(report, field_usage); > + if (field) > + return field; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(appleib_find_hid_field); > + > +/** > + * Return whether we're currently inside a hid_device_probe or not. > + * @ib_dev: the appleib_device > + */ > +bool appleib_in_hid_probe(struct appleib_device *ib_dev) > +{ > + return ib_dev->in_hid_probe; > +} > +EXPORT_SYMBOL_GPL(appleib_in_hid_probe); > + > +static struct appleib_hid_dev_info * > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct appleib_hid_dev_info *dev_info; > + struct appleib_hid_drv_info *drv_info; > + > + /* allocate device-info for this device */ > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); > + if (!dev_info) > + return NULL; > + > + INIT_LIST_HEAD(&dev_info->drivers); > + dev_info->device = hdev; > + dev_info->device_id = id; > + > + /* notify all our sub drivers */ > + mutex_lock(&ib_dev->update_lock); > + > + ib_dev->in_hid_probe = true; > + > + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { > + appleib_probe_driver(drv_info, dev_info); > + } > + > + ib_dev->in_hid_probe = false; > + > + /* remember this new device */ > + list_add_tail_rcu(&dev_info->entry, &ib_dev->hid_devices); > + > + mutex_unlock(&ib_dev->update_lock); > + > + return dev_info; > +} > + > +static void appleib_remove_device(struct appleib_device *ib_dev, > + struct appleib_hid_dev_info *dev_info) > +{ > + list_del_rcu(&dev_info->entry); > + synchronize_srcu(&ib_dev->lists_srcu); > + > + appleib_detach_drivers(ib_dev, dev_info); > + > + kfree(dev_info); > +} > + > +static int appleib_hid_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct appleib_device *ib_dev; > + struct appleib_hid_dev_info *dev_info; > + struct usb_device *udev; > + int rc; > + > + /* check usb config first */ > + udev = hid_to_usb_dev(hdev); > + > + if (udev->actconfig->desc.bConfigurationValue != APPLETB_BASIC_CONFIG) { > + rc = usb_driver_set_configuration(udev, APPLETB_BASIC_CONFIG); > + return rc ? rc : -ENODEV; > + } > + > + /* Assign the driver data */ > + ib_dev = appleib_dev; > + hid_set_drvdata(hdev, ib_dev); > + > + /* initialize the report info */ > + rc = hid_parse(hdev); > + if (rc) { > + hid_err(hdev, "ib: hid parse failed (%d)\n", rc); > + goto error; > + } > + > + /* alloc bufs etc so probe's can send requests; but connect later */ > + rc = hid_hw_start(hdev, 0); > + if (rc) { > + hid_err(hdev, "ib: hw start failed (%d)\n", rc); > + goto error; > + } > + > + /* add this hdev to our device list */ > + dev_info = appleib_add_device(ib_dev, hdev, id); > + if (!dev_info) { > + rc = -ENOMEM; > + goto stop_hw; > + } > + > + /* start the hid */ > + rc = appleib_start_hid_events(dev_info); > + if (rc) > + goto remove_dev; > + > + return 0; > + > +remove_dev: > + mutex_lock(&ib_dev->update_lock); > + appleib_remove_device(ib_dev, dev_info); > + mutex_unlock(&ib_dev->update_lock); > +stop_hw: > + hid_hw_stop(hdev); > +error: > + return rc; > +} > + > +static void appleib_hid_remove(struct hid_device *hdev) > +{ > + struct appleib_device *ib_dev = hid_get_drvdata(hdev); > + struct appleib_hid_dev_info *dev_info; > + > + mutex_lock(&ib_dev->update_lock); > + > + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { > + if (dev_info->device == hdev) { > + appleib_stop_hid_events(dev_info); > + appleib_remove_device(ib_dev, dev_info); > + break; > + } > + } > + > + mutex_unlock(&ib_dev->update_lock); > + > + hid_hw_stop(hdev); > +} > + > +static const struct hid_device_id appleib_hid_devices[] = { > + { HID_USB_DEVICE(USB_ID_VENDOR_APPLE, USB_ID_PRODUCT_IBRIDGE) }, > + { }, > +}; > + > +static struct hid_driver appleib_hid_driver = { > + .name = "apple-ibridge-hid", > + .id_table = appleib_hid_devices, > + .probe = appleib_hid_probe, > + .remove = appleib_hid_remove, > + .event = appleib_hid_event, > + .report_fixup = appleib_report_fixup, > + .input_configured = appleib_input_configured, > +#ifdef CONFIG_PM > + .suspend = appleib_hid_suspend, > + .resume = appleib_hid_resume, > + .reset_resume = appleib_hid_reset_resume, > +#endif > +}; > + > +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev) > +{ > + struct appleib_device *ib_dev; > + acpi_status sts; > + int rc; > + > + /* allocate */ > + ib_dev = kzalloc(sizeof(*ib_dev), GFP_KERNEL); > + if (!ib_dev) > + return ERR_PTR(-ENOMEM); > + > + /* init structures */ > + INIT_LIST_HEAD(&ib_dev->hid_drivers); > + INIT_LIST_HEAD(&ib_dev->hid_devices); > + mutex_init(&ib_dev->update_lock); > + init_srcu_struct(&ib_dev->lists_srcu); > + > + ib_dev->acpi_dev = acpi_dev; > + > + /* get iBridge acpi power control method */ > + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw); > + if (ACPI_FAILURE(sts)) { > + dev_err(LOG_DEV(ib_dev), > + "Error getting handle for ASOC.SOCW method: %s\n", > + acpi_format_exception(sts)); > + rc = -ENXIO; > + goto free_mem; > + } > + > + /* ensure iBridge is powered on */ > + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); > + if (ACPI_FAILURE(sts)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", > + acpi_format_exception(sts)); > + > + return ib_dev; > + > +free_mem: > + kfree(ib_dev); > + return ERR_PTR(rc); > +} > + > +static int appleib_probe(struct acpi_device *acpi) > +{ > + struct appleib_device *ib_dev; > + struct appleib_platform_data *pdata; > + int i; > + int ret; > + > + if (appleib_dev) > + return -EBUSY; > + > + ib_dev = appleib_alloc_device(acpi); > + if (IS_ERR_OR_NULL(ib_dev)) > + return PTR_ERR(ib_dev); > + > + ib_dev->subdevs = kmemdup(appleib_subdevs, sizeof(appleib_subdevs), > + GFP_KERNEL); > + if (!ib_dev->subdevs) { > + ret = -ENOMEM; > + goto free_dev; > + } > + > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > + if (!pdata) { > + ret = -ENOMEM; > + goto free_subdevs; > + } > + > + pdata->ib_dev = ib_dev; > + pdata->log_dev = LOG_DEV(ib_dev); > + for (i = 0; i < ARRAY_SIZE(appleib_subdevs); i++) { > + ib_dev->subdevs[i].platform_data = pdata; > + ib_dev->subdevs[i].pdata_size = sizeof(*pdata); > + } > + > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE, > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs), > + NULL, 0, NULL); > + if (ret) { > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret); > + goto free_pdata; > + } > + > + acpi->driver_data = ib_dev; > + appleib_dev = ib_dev; > + > + ret = hid_register_driver(&appleib_hid_driver); > + if (ret) { > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n", > + ret); > + goto rem_mfd_devs; > + } > + > + return 0; > + > +rem_mfd_devs: > + mfd_remove_devices(&acpi->dev); > +free_pdata: > + kfree(pdata); > +free_subdevs: > + kfree(ib_dev->subdevs); > +free_dev: > + appleib_dev = NULL; > + acpi->driver_data = NULL; > + kfree(ib_dev); > + return ret; > +} > + > +static int appleib_remove(struct acpi_device *acpi) > +{ > + struct appleib_device *ib_dev = acpi_driver_data(acpi); > + > + mfd_remove_devices(&acpi->dev); > + hid_unregister_driver(&appleib_hid_driver); > + > + if (appleib_dev == ib_dev) > + appleib_dev = NULL; > + > + kfree(ib_dev->subdevs[0].platform_data); > + kfree(ib_dev->subdevs); > + kfree(ib_dev); > + > + return 0; > +} > + > +static int appleib_suspend(struct device *dev) > +{ > + struct acpi_device *adev; > + struct appleib_device *ib_dev; > + int rc; > + > + adev = to_acpi_device(dev); > + ib_dev = acpi_driver_data(adev); > + > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0); > + if (ACPI_FAILURE(rc)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n", > + acpi_format_exception(rc)); > + > + return 0; > +} > + > +static int appleib_resume(struct device *dev) > +{ > + struct acpi_device *adev; > + struct appleib_device *ib_dev; > + int rc; > + > + adev = to_acpi_device(dev); > + ib_dev = acpi_driver_data(adev); > + > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); > + if (ACPI_FAILURE(rc)) > + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", > + acpi_format_exception(rc)); > + > + return 0; > +} > + > +static const struct dev_pm_ops appleib_pm = { > + .suspend = appleib_suspend, > + .resume = appleib_resume, > + .restore = appleib_resume, > +}; > + > +static const struct acpi_device_id appleib_acpi_match[] = { > + { "APP7777", 0 }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match); > + > +static struct acpi_driver appleib_driver = { > + .name = "apple-ibridge", > + .class = "topcase", /* ? */ > + .owner = THIS_MODULE, > + .ids = appleib_acpi_match, > + .ops = { > + .add = appleib_probe, > + .remove = appleib_remove, > + }, > + .drv = { > + .pm = &appleib_pm, > + }, > +}; > + > +module_acpi_driver(appleib_driver) > + > +MODULE_AUTHOR("Ronald Tschalär"); > +MODULE_DESCRIPTION("Apple iBridge driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/apple-ibridge.h b/include/linux/mfd/apple-ibridge.h > new file mode 100644 > index 000000000000..d321714767f7 > --- /dev/null > +++ b/include/linux/mfd/apple-ibridge.h > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Apple iBridge Driver > + * > + * Copyright (c) 2018 Ronald Tschalär > + */ > + > +#ifndef __LINUX_MFD_APPLE_IBRDIGE_H > +#define __LINUX_MFD_APPLE_IBRDIGE_H > + > +#include <linux/device.h> > +#include <linux/hid.h> > + > +#define PLAT_NAME_IB_TB "apple-ib-tb" > +#define PLAT_NAME_IB_ALS "apple-ib-als" > + > +struct appleib_device; > + > +struct appleib_platform_data { > + struct appleib_device *ib_dev; > + struct device *log_dev; > +}; > + > +int appleib_register_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver, void *data); > +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, > + struct hid_driver *driver); > + > +void *appleib_get_drvdata(struct appleib_device *ib_dev, > + struct hid_driver *driver); > +bool appleib_in_hid_probe(struct appleib_device *ib_dev); > + > +struct hid_field *appleib_find_report_field(struct hid_report *report, > + unsigned int field_usage); > +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, > + unsigned int application, > + unsigned int field_usage); > + > +#endif > -- > 2.20.1 >
On Wed, 24 Apr 2019 03:47:18 -0700 "Life is hard, and then you die" <ronald@innovation.ch> wrote: > Hi Jonathan, > > On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote: > > On Sun, 21 Apr 2019 20:12:49 -0700 > > Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > 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, since the > > > functionality for the touch bar and light sensor is exposed via USB HID > > > interfaces, and the same HID device is used for multiple functions, this > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > be registered for a given HID device. This allows the touch bar and ALS > > > driver to be separated out into their own modules. > > > > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch > > Hi Ronald, > > > > I've only taken a fairly superficial look at this. A few global > > things to note though. > > Thanks for this review. > > > 1. Please either use kernel-doc style for function descriptions, or > > do not. Right now you are sort of half way there. > > Apologies, on re-reading the docs I realize what you mean here. Should > be fixed now (next rev). > > > 2. There is quite a complex nest of separate structures being allocated, > > so think about whether they can be simplified. In particular > > use of container_of macros can allow a lot of forwards and backwards > > pointers to be dropped if you embed the various structures directly. > > Done (see also below). > > [snip] > > > +#define call_void_driver_func(drv_info, fn, ...) \ > > > > This sort of macro may seem like a good idea because it saves a few lines > > of code. However, that comes at the cost of readability, so just > > put the code inline. > > > > > + do { \ > > > + if ((drv_info)->driver->fn) \ > > > + (drv_info)->driver->fn(__VA_ARGS__); \ > > > + } while (0) > > > + > > > +#define call_driver_func(drv_info, fn, ret_type, ...) \ > > > + ({ \ > > > + ret_type rc = 0; \ > > > + \ > > > + if ((drv_info)->driver->fn) \ > > > + rc = (drv_info)->driver->fn(__VA_ARGS__); \ > > > + \ > > > + rc; \ > > > + }) > > Just to clarify, you're only talking about removing/inlining the > call_void_driver_func() macro, not the call_driver_func() macro, > right? Both please. Neither adds much. > > [snip] > > > +static struct appleib_hid_dev_info * > > > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev, > > > + const struct hid_device_id *id) > > > +{ > > > + struct appleib_hid_dev_info *dev_info; > > > + struct appleib_hid_drv_info *drv_info; > > > + > > > + /* allocate device-info for this device */ > > > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); > > > + if (!dev_info) > > > + return NULL; > > > + > > > + INIT_LIST_HEAD(&dev_info->drivers); > > > + dev_info->device = hdev; > > > + dev_info->device_id = id; > > > + > > > + /* notify all our sub drivers */ > > > + mutex_lock(&ib_dev->update_lock); > > > + > > This is interesting. I'd like to see a comment here on what > > this flag is going to do. > > I'm not sure I follow: update_lock is simply a mutex protecting all > driver and device update (i.e. add/remove) functions. Are you > therefore looking for something like: That ended up in the wrong place... It was the in_hid_probe just after here that I was referring to. > > /* protect driver and device lists against concurrent updates */ > mutex_lock(&ib_dev->update_lock); > > [snip] > > > +static int appleib_probe(struct acpi_device *acpi) > > > +{ > > > + struct appleib_device *ib_dev; > > > + struct appleib_platform_data *pdata; > > Platform_data has a lot of historical meaning in Linux. > > Also you have things in here that are not platform related > > at all, such as the dev pointer. Hence I would rename it > > as device_data or private or something like that. > > Ok. I guess I called in platform_data because it's stored in the mfd > cell's "platform_data" field. Anyway, changed it per your suggestion. > > > > + int i; > > > + int ret; > > > + > > > + if (appleib_dev) > > This singleton bothers me a bit. I'm really not sure why it > > is necessary. You can just put a pointer to this in > > the pdata for the subdevs and I think that covers most of your > > usecases. It's generally a bad idea to limit things to one instance > > of a device unless that actually major simplifications. > > I'm not seeing them here. > > Yes, this one is quite ugly. appleib_dev is static so that > appleib_hid_probe() can find it. I could not find any other way to > pass the appleib_dev instance to that probe function. > > However, on looking at this again, I realized that hid_device_id has > a driver_data field which can be used for this; so if I added the > hid_driver and hid_device_id structs to the appleib_device (instead of > making them static like now) I could fill in the driver_data and avoid > this hack. This looks much cleaner. > > Thanks for pointing this uglyness out again. > > [snip] > > > + if (!ib_dev->subdevs) { > > > + ret = -ENOMEM; > > > + goto free_dev; > > > + } > > > + > > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > > > > Might as well embed this in ib_dev as well. > > Agreed. > > > That would let > > you used container_of to avoid having to carry the ib_dev pointer > > around in side pdata. > > I see. I guess my main reservation is that the functions exported to > the sub-drivers would now take a 'struct appleib_device_data *' > argument instead of a 'struct appleib_device *', which just seems a > bit unnatural. E.g. > > int appleib_register_hid_driver(struct appleib_device_data *ib_ddata, > struct hid_driver *driver, void *data); > > instead of (the current) > > int appleib_register_hid_driver(struct appleib_device *ib_dev, > struct hid_driver *driver, void *data) I'm not totally sure I can see why. You can go from backwards and forwards from any of the pointers... > > [snip] > > > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE, > > > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs), > > > + NULL, 0, NULL); > > > + if (ret) { > > > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret); > > > + goto free_pdata; > > > + } > > > + > > > + acpi->driver_data = ib_dev; > > > + appleib_dev = ib_dev; > > > + > > > + ret = hid_register_driver(&appleib_hid_driver); > > > + if (ret) { > > > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n", > > > + ret); > > > + goto rem_mfd_devs; > > > + } > > > + > > > + return 0; > > > + > > > +rem_mfd_devs: > > > + mfd_remove_devices(&acpi->dev); > > > +free_pdata: > > > + kfree(pdata); > > > +free_subdevs: > > > + kfree(ib_dev->subdevs); > > > +free_dev: > > > + appleib_dev = NULL; > > > + acpi->driver_data = NULL; > > Why at this point? It's not set to anything until much later in the > > probe flow. > > If the hid_register_driver() call fails, we get here after driver_data > has been assigned. However, looking at this again, acpi->driver_data > is only used by the remove, suspend, and resume callbacks, and those > will not be called until a successful return from probe; therefore I > can safely move the setting of driver_data to after the > hid_register_driver() call and avoid having to set it to NULL in the > error cleanup. > > > May be worth thinking about devm_ managed allocations > > to cleanup some of these allocations automatically and simplify > > the error handling. > > Good point, thanks. > > [snip] > > > + > > > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0); > > > + if (ACPI_FAILURE(rc)) > > > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n", > > > > I can sort of see you might want to do the LOG_DEV for consistency > > but here I'm fairly sure it's just dev which might be clearer. > > Sorry, you mean rename the macro LOG_DEV() to just DEV()? No, just dev_warn(dev,....) It's the same one I think at this particular location. > > > Cheers, > > Ronald >
Hi Benjamin, Thank you for looking at this. On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > 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, since the > > functionality for the touch bar and light sensor is exposed via USB HID > > interfaces, and the same HID device is used for multiple functions, this > > driver provides a multiplexing layer that allows multiple HID drivers to > > be registered for a given HID device. This allows the touch bar and ALS > > driver to be separated out into their own modules. > > Sorry for coming late to the party, but IMO this series is far too > complex for what you need. > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > you need to have a HID driver that multiplex 2 other sub drivers > through one USB communication. > For that, you are using MFD, platform driver and you own sauce instead > of creating a bus. Basically correct. To be a bit more precise, there are currently two hid-devices and two drivers (touchbar and als) involved, with connections as follows (pardon the ugly ascii art): hdev1 --- tb-drv / / / hdev2 --- als-drv i.e. the touchbar driver talks to both hdev's, and hdev2's events (reports) are processed by both drivers (though each handles different reports). > So, how about we reuse entirely the HID subsystem which already > provides the capability you need (assuming I am correct above). > hid-logitech-dj already does the same kind of stuff and you could: > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > - hid-ibridge will then register itself to the hid subsystem with a > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > hid_device_io_start(hdev) to enable the events (so you don't create > useless input nodes for it) > - then you add your 2 new devices by calling hid_allocate_device() and > then hid_add_device(). You can even create a new HID group > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > from the actual USB device. > - then you have 2 brand new HID devices you can create their driver as > a regular ones. > > hid-ibridge.c would just need to behave like any other hid transport > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > you can get rid of at least the MFD and the platform part of your > drivers. > > Does it makes sense or am I missing something obvious in the middle? Yes, I think I understand, and I think this can work. Basically, instead of demux'ing at the hid-driver level as I am doing now (i.e. the iBridge hid-driver forwarding calls to the sub-hid-drivers), we demux at the hid-device level (events forwarded from iBridge hdev to all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to the original hdev via an iBridge ll_driver attached to the sub-hdev's). So I would need to create 3 new "virtual" hid-devices (instances) as follows: hdev1 --- vhdev1 --- tb-drv / -- vhdev2 -- / hdev2 --- vhdev3 --- als-drv (vhdev1 is probably not strictly necessary, but makes things more consistent). > I have one other comment below. > > Note that I haven't read the whole series as I'd like to first get > your feedback with my comment above. Agreed: let's first get the overall strategy stabilized (also so I can avoid having to rewrite the code too many more times ;-) ). [snip] > > +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > + unsigned int *rsize) > > +{ > > + /* Some fields have a size of 64 bits, which according to HID 1.11 > > + * Section 8.4 is not valid ("An item field cannot span more than 4 > > + * bytes in a report"). Furthermore, hid_field_extract() complains > > this must have been fixed in 94a9992f7dbdfb28976b565af220e0c4a117144a > which is part of v5.1, so not sure you actually need the report > descriptor fixup at all. Wasn't aware of this change - thanks. Yes, with that the warning message should be gone and this fixup can be avoided. One thing I find strange, though, is that while 94a9992f7dbd changes the condition at which the warning is emitted, it still truncates the value to 32 bits, albeit completely silently now for lengths between 32 and 256 bits. I.e. I'm somewhat surprised that hid_field_extract() (and __extract() ) weren't updated to actually return the full values for longer fields. Either that, or the callers of hid_field_extract() changed to read longer fields in 32 bit chunks. Cheers, Ronald
On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die <ronald@innovation.ch> wrote: > > > Hi Benjamin, > > Thank you for looking at this. > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > 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, since the > > > functionality for the touch bar and light sensor is exposed via USB HID > > > interfaces, and the same HID device is used for multiple functions, this > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > be registered for a given HID device. This allows the touch bar and ALS > > > driver to be separated out into their own modules. > > > > Sorry for coming late to the party, but IMO this series is far too > > complex for what you need. > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > > you need to have a HID driver that multiplex 2 other sub drivers > > through one USB communication. > > For that, you are using MFD, platform driver and you own sauce instead > > of creating a bus. > > Basically correct. To be a bit more precise, there are currently two > hid-devices and two drivers (touchbar and als) involved, with > connections as follows (pardon the ugly ascii art): > > hdev1 --- tb-drv > / > / > / > hdev2 --- als-drv > > i.e. the touchbar driver talks to both hdev's, and hdev2's events > (reports) are processed by both drivers (though each handles different > reports). > > > So, how about we reuse entirely the HID subsystem which already > > provides the capability you need (assuming I am correct above). > > hid-logitech-dj already does the same kind of stuff and you could: > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > > - hid-ibridge will then register itself to the hid subsystem with a > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > > hid_device_io_start(hdev) to enable the events (so you don't create > > useless input nodes for it) > > - then you add your 2 new devices by calling hid_allocate_device() and > > then hid_add_device(). You can even create a new HID group > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > > from the actual USB device. > > - then you have 2 brand new HID devices you can create their driver as > > a regular ones. > > > > hid-ibridge.c would just need to behave like any other hid transport > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > > you can get rid of at least the MFD and the platform part of your > > drivers. > > > > Does it makes sense or am I missing something obvious in the middle? > > Yes, I think I understand, and I think this can work. Basically, > instead of demux'ing at the hid-driver level as I am doing now (i.e. > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we > demux at the hid-device level (events forwarded from iBridge hdev to > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to > the original hdev via an iBridge ll_driver attached to the > sub-hdev's). > > So I would need to create 3 new "virtual" hid-devices (instances) as > follows: > > hdev1 --- vhdev1 --- tb-drv > / > -- vhdev2 -- > / > hdev2 --- vhdev3 --- als-drv > > (vhdev1 is probably not strictly necessary, but makes things more > consistent). Oh, ok. How about the following: hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then this driver creates 2 virtual hid drivers that are consistent like hdev1---ibridge-drv---vhdev1---tb-drv hdev2--/ \--vhdev2---als-drv > > > I have one other comment below. > > > > Note that I haven't read the whole series as I'd like to first get > > your feedback with my comment above. > > Agreed: let's first get the overall strategy stabilized (also so I > can avoid having to rewrite the code too many more times ;-) ). > > [snip] > > > +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > > + unsigned int *rsize) > > > +{ > > > + /* Some fields have a size of 64 bits, which according to HID 1.11 > > > + * Section 8.4 is not valid ("An item field cannot span more than 4 > > > + * bytes in a report"). Furthermore, hid_field_extract() complains > > > > this must have been fixed in 94a9992f7dbdfb28976b565af220e0c4a117144a > > which is part of v5.1, so not sure you actually need the report > > descriptor fixup at all. > > Wasn't aware of this change - thanks. Yes, with that the warning > message should be gone and this fixup can be avoided. > > One thing I find strange, though, is that while 94a9992f7dbd changes > the condition at which the warning is emitted, it still truncates the > value to 32 bits, albeit completely silently now for lengths between > 32 and 256 bits. I.e. I'm somewhat surprised that hid_field_extract() > (and __extract() ) weren't updated to actually return the full values > for longer fields. Either that, or the callers of hid_field_extract() > changed to read longer fields in 32 bit chunks. facepalm. Yep this commit is just hiding the dust under the carpet. Let's raise that and request a revert :/ Cheers, Benjamin > > > Cheers, > > Ronald >
Hi Jonathan, On Wed, Apr 24, 2019 at 08:13:17PM +0100, Jonathan Cameron wrote: > On Wed, 24 Apr 2019 03:47:18 -0700 > "Life is hard, and then you die" <ronald@innovation.ch> wrote: > > > Hi Jonathan, > > > > On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote: > > > On Sun, 21 Apr 2019 20:12:49 -0700 > > > Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > > 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, since the > > > > functionality for the touch bar and light sensor is exposed via USB HID > > > > interfaces, and the same HID device is used for multiple functions, this > > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > > be registered for a given HID device. This allows the touch bar and ALS > > > > driver to be separated out into their own modules. > > > > > > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch > > > Hi Ronald, > > > > > > I've only taken a fairly superficial look at this. A few global > > > things to note though. > > > > Thanks for this review. [snip] I've applied all your feedback in my tree, but it now looks like this module is going to be redone differently. I'll try to keep all your comments in mind during the rewrite, though, so they're not wasted. Cheers, Ronald
Hi Benjamin, On Thu, Apr 25, 2019 at 11:39:12AM +0200, Benjamin Tissoires wrote: > On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die > <ronald@innovation.ch> wrote: > > > > Hi Benjamin, > > > > Thank you for looking at this. > > > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > > > 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, since the > > > > functionality for the touch bar and light sensor is exposed via USB HID > > > > interfaces, and the same HID device is used for multiple functions, this > > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > > be registered for a given HID device. This allows the touch bar and ALS > > > > driver to be separated out into their own modules. > > > > > > Sorry for coming late to the party, but IMO this series is far too > > > complex for what you need. > > > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > > > you need to have a HID driver that multiplex 2 other sub drivers > > > through one USB communication. > > > For that, you are using MFD, platform driver and you own sauce instead > > > of creating a bus. > > > > Basically correct. To be a bit more precise, there are currently two > > hid-devices and two drivers (touchbar and als) involved, with > > connections as follows (pardon the ugly ascii art): > > > > hdev1 --- tb-drv > > / > > / > > / > > hdev2 --- als-drv > > > > i.e. the touchbar driver talks to both hdev's, and hdev2's events > > (reports) are processed by both drivers (though each handles different > > reports). > > > > > So, how about we reuse entirely the HID subsystem which already > > > provides the capability you need (assuming I am correct above). > > > hid-logitech-dj already does the same kind of stuff and you could: > > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > > > - hid-ibridge will then register itself to the hid subsystem with a > > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > > > hid_device_io_start(hdev) to enable the events (so you don't create > > > useless input nodes for it) > > > - then you add your 2 new devices by calling hid_allocate_device() and > > > then hid_add_device(). You can even create a new HID group > > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > > > from the actual USB device. > > > - then you have 2 brand new HID devices you can create their driver as > > > a regular ones. > > > > > > hid-ibridge.c would just need to behave like any other hid transport > > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > > > you can get rid of at least the MFD and the platform part of your > > > drivers. > > > > > > Does it makes sense or am I missing something obvious in the middle? > > > > Yes, I think I understand, and I think this can work. Basically, > > instead of demux'ing at the hid-driver level as I am doing now (i.e. > > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we > > demux at the hid-device level (events forwarded from iBridge hdev to > > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to > > the original hdev via an iBridge ll_driver attached to the > > sub-hdev's). > > > > So I would need to create 3 new "virtual" hid-devices (instances) as > > follows: > > > > hdev1 --- vhdev1 --- tb-drv > > / > > -- vhdev2 -- > > / > > hdev2 --- vhdev3 --- als-drv > > > > (vhdev1 is probably not strictly necessary, but makes things more > > consistent). > > Oh, ok. > > How about the following: > > hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then > this driver creates 2 virtual hid drivers that are consistent > > like > > hdev1---ibridge-drv---vhdev1---tb-drv > hdev2--/ \--vhdev2---als-drv I don't think this will work. The problem is when the sub-drivers need to send a report or usb-command: how to they specify which hdev the report/command is destined for? While we could store the original hdev in each report (the hid_report's device field), that only works for hid_hw_request(), but not for things like hid_hw_raw_request() or hid_hw_output_report(). Now, currently I don't use the latter two; but I do need to send raw usb control messages in the touchbar driver (some commands are not proper hid reports), so it definitely breaks down there. Or am I missing something? Cheers, Ronald
On Fri, Apr 26, 2019 at 7:56 AM Life is hard, and then you die <ronald@innovation.ch> wrote: > > > Hi Benjamin, > > On Thu, Apr 25, 2019 at 11:39:12AM +0200, Benjamin Tissoires wrote: > > On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die > > <ronald@innovation.ch> wrote: > > > > > > Hi Benjamin, > > > > > > Thank you for looking at this. > > > > > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > > > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > > > > > 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, since the > > > > > functionality for the touch bar and light sensor is exposed via USB HID > > > > > interfaces, and the same HID device is used for multiple functions, this > > > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > > > be registered for a given HID device. This allows the touch bar and ALS > > > > > driver to be separated out into their own modules. > > > > > > > > Sorry for coming late to the party, but IMO this series is far too > > > > complex for what you need. > > > > > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > > > > you need to have a HID driver that multiplex 2 other sub drivers > > > > through one USB communication. > > > > For that, you are using MFD, platform driver and you own sauce instead > > > > of creating a bus. > > > > > > Basically correct. To be a bit more precise, there are currently two > > > hid-devices and two drivers (touchbar and als) involved, with > > > connections as follows (pardon the ugly ascii art): > > > > > > hdev1 --- tb-drv > > > / > > > / > > > / > > > hdev2 --- als-drv > > > > > > i.e. the touchbar driver talks to both hdev's, and hdev2's events > > > (reports) are processed by both drivers (though each handles different > > > reports). > > > > > > > So, how about we reuse entirely the HID subsystem which already > > > > provides the capability you need (assuming I am correct above). > > > > hid-logitech-dj already does the same kind of stuff and you could: > > > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > > > > - hid-ibridge will then register itself to the hid subsystem with a > > > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > > > > hid_device_io_start(hdev) to enable the events (so you don't create > > > > useless input nodes for it) > > > > - then you add your 2 new devices by calling hid_allocate_device() and > > > > then hid_add_device(). You can even create a new HID group > > > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > > > > from the actual USB device. > > > > - then you have 2 brand new HID devices you can create their driver as > > > > a regular ones. > > > > > > > > hid-ibridge.c would just need to behave like any other hid transport > > > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > > > > you can get rid of at least the MFD and the platform part of your > > > > drivers. > > > > > > > > Does it makes sense or am I missing something obvious in the middle? > > > > > > Yes, I think I understand, and I think this can work. Basically, > > > instead of demux'ing at the hid-driver level as I am doing now (i.e. > > > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we > > > demux at the hid-device level (events forwarded from iBridge hdev to > > > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to > > > the original hdev via an iBridge ll_driver attached to the > > > sub-hdev's). > > > > > > So I would need to create 3 new "virtual" hid-devices (instances) as > > > follows: > > > > > > hdev1 --- vhdev1 --- tb-drv > > > / > > > -- vhdev2 -- > > > / > > > hdev2 --- vhdev3 --- als-drv > > > > > > (vhdev1 is probably not strictly necessary, but makes things more > > > consistent). > > > > Oh, ok. > > > > How about the following: > > > > hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then > > this driver creates 2 virtual hid drivers that are consistent > > > > like > > > > hdev1---ibridge-drv---vhdev1---tb-drv > > hdev2--/ \--vhdev2---als-drv > > I don't think this will work. The problem is when the sub-drivers need > to send a report or usb-command: how to they specify which hdev the > report/command is destined for? While we could store the original hdev > in each report (the hid_report's device field), that only works for > hid_hw_request(), but not for things like hid_hw_raw_request() or > hid_hw_output_report(). Now, currently I don't use the latter two; but > I do need to send raw usb control messages in the touchbar driver > (some commands are not proper hid reports), so it definitely breaks > down there. > > Or am I missing something? > I'd need to have a deeper look at the protocol, but you can emulate pure HID devices by having your ibridge handling a translation from set/get features/input to the usb control messages. Likewise, nothing prevents you to slightly rewrite the report descriptors you present to the als and touchbar to have a clear separation with the report ID. For example, if both hdev1 and hdev2 use a report ID of 0x01, you could rewrite the report descriptor so that when you receive a report with an id of 0x01 you send this to hdev1, but you can also translate 0x11 to a report ID 0x01 to hdev2. Likewise, report ID 0x42 could be a raw USB control message to the USB under hdev2. Note that you will have to write 2 report descriptors for your new devices, but you can take what makes sense from the original ones, and just add a new collection with a vendor application with with an opaque meaning (for the USB control messages). Cheers, Benjamin
On Sun, 21 Apr 2019, Ronald Tschalär wrote: > 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, since the > functionality for the touch bar and light sensor is exposed via USB HID > interfaces, and the same HID device is used for multiple functions, this > driver provides a multiplexing layer that allows multiple HID drivers to > be registered for a given HID device. This allows the touch bar and ALS > driver to be separated out into their own modules. > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > --- > drivers/mfd/Kconfig | 15 + > drivers/mfd/Makefile | 1 + > drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ I haven't taken a thorough look through, but I can tell you that the vast majority of what you're trying to do here does not belong in MFD. MFD drivers are used to register child devices. Almost all functionality or 'real work' should be contained in the drivers the MFD registers, not in the MFD parent itself. You will need to move all 'real work' out into the subordinate device drivers for acceptance. > include/linux/mfd/apple-ibridge.h | 39 ++ > 4 files changed, 938 insertions(+) > create mode 100644 drivers/mfd/apple-ibridge.c > create mode 100644 include/linux/mfd/apple-ibridge.h
On Tue, May 07, 2019 at 01:24:15PM +0100, Lee Jones wrote: > On Sun, 21 Apr 2019, Ronald Tschalär wrote: > > > 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, since the > > functionality for the touch bar and light sensor is exposed via USB HID > > interfaces, and the same HID device is used for multiple functions, this > > driver provides a multiplexing layer that allows multiple HID drivers to > > be registered for a given HID device. This allows the touch bar and ALS > > driver to be separated out into their own modules. > > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > > --- > > drivers/mfd/Kconfig | 15 + > > drivers/mfd/Makefile | 1 + > > drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ > > I haven't taken a thorough look through, but I can tell you that the > vast majority of what you're trying to do here does not belong in > MFD. MFD drivers are used to register child devices. Almost all > functionality or 'real work' should be contained in the drivers the > MFD registers, not in the MFD parent itself. You will need to move > all 'real work' out into the subordinate device drivers for > acceptance. Thanks for your feedback. That was/is the idea: the actual Touch Bar and ALS driver code is in separate modules - what is left in the appple-ibridge mfd driver is a fairly generic hid driver demultiplexer. However, that could be moved out into it's own helper/module. Having said that, it looks like the preference is to do all of this as a hid driver with virtual hid devices instead of as an mfd driver. Cheers, Ronald
On Sun, 09 Jun 2019, Life is hard, and then you die wrote: > > On Tue, May 07, 2019 at 01:24:15PM +0100, Lee Jones wrote: > > On Sun, 21 Apr 2019, Ronald Tschalär wrote: > > > > > 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, since the > > > functionality for the touch bar and light sensor is exposed via USB HID > > > interfaces, and the same HID device is used for multiple functions, this > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > be registered for a given HID device. This allows the touch bar and ALS > > > driver to be separated out into their own modules. > > > > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > > > --- > > > drivers/mfd/Kconfig | 15 + > > > drivers/mfd/Makefile | 1 + > > > drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ > > > > I haven't taken a thorough look through, but I can tell you that the > > vast majority of what you're trying to do here does not belong in > > MFD. MFD drivers are used to register child devices. Almost all > > functionality or 'real work' should be contained in the drivers the > > MFD registers, not in the MFD parent itself. You will need to move > > all 'real work' out into the subordinate device drivers for > > acceptance. > > Thanks for your feedback. That was/is the idea: the actual Touch Bar > and ALS driver code is in separate modules - what is left in the > appple-ibridge mfd driver is a fairly generic hid driver > demultiplexer. However, that could be moved out into it's own > helper/module. > > Having said that, it looks like the preference is to do all of this as > a hid driver with virtual hid devices instead of as an mfd driver. Sounds like a better approach.
Hi Benjamin, Sorry for the extremely late reply - RL etc. On Fri, Apr 26, 2019 at 08:26:25AM +0200, Benjamin Tissoires wrote: > On Fri, Apr 26, 2019 at 7:56 AM Life is hard, and then you die > <ronald@innovation.ch> wrote: > > > > On Thu, Apr 25, 2019 at 11:39:12AM +0200, Benjamin Tissoires wrote: > > > On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die > > > <ronald@innovation.ch> wrote: > > > > > > > > Hi Benjamin, > > > > > > > > Thank you for looking at this. > > > > > > > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > > > > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > > > > > > > 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, since the > > > > > > functionality for the touch bar and light sensor is exposed via USB HID > > > > > > interfaces, and the same HID device is used for multiple functions, this > > > > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > > > > be registered for a given HID device. This allows the touch bar and ALS > > > > > > driver to be separated out into their own modules. > > > > > > > > > > Sorry for coming late to the party, but IMO this series is far too > > > > > complex for what you need. > > > > > > > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > > > > > you need to have a HID driver that multiplex 2 other sub drivers > > > > > through one USB communication. > > > > > For that, you are using MFD, platform driver and you own sauce instead > > > > > of creating a bus. > > > > > > > > Basically correct. To be a bit more precise, there are currently two > > > > hid-devices and two drivers (touchbar and als) involved, with > > > > connections as follows (pardon the ugly ascii art): > > > > > > > > hdev1 --- tb-drv > > > > / > > > > / > > > > / > > > > hdev2 --- als-drv > > > > > > > > i.e. the touchbar driver talks to both hdev's, and hdev2's events > > > > (reports) are processed by both drivers (though each handles different > > > > reports). > > > > > > > > > So, how about we reuse entirely the HID subsystem which already > > > > > provides the capability you need (assuming I am correct above). > > > > > hid-logitech-dj already does the same kind of stuff and you could: > > > > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > > > > > - hid-ibridge will then register itself to the hid subsystem with a > > > > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > > > > > hid_device_io_start(hdev) to enable the events (so you don't create > > > > > useless input nodes for it) > > > > > - then you add your 2 new devices by calling hid_allocate_device() and > > > > > then hid_add_device(). You can even create a new HID group > > > > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > > > > > from the actual USB device. > > > > > - then you have 2 brand new HID devices you can create their driver as > > > > > a regular ones. > > > > > > > > > > hid-ibridge.c would just need to behave like any other hid transport > > > > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > > > > > you can get rid of at least the MFD and the platform part of your > > > > > drivers. > > > > > > > > > > Does it makes sense or am I missing something obvious in the middle? > > > > > > > > Yes, I think I understand, and I think this can work. Basically, > > > > instead of demux'ing at the hid-driver level as I am doing now (i.e. > > > > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we > > > > demux at the hid-device level (events forwarded from iBridge hdev to > > > > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to > > > > the original hdev via an iBridge ll_driver attached to the > > > > sub-hdev's). > > > > > > > > So I would need to create 3 new "virtual" hid-devices (instances) as > > > > follows: > > > > > > > > hdev1 --- vhdev1 --- tb-drv > > > > / > > > > -- vhdev2 -- > > > > / > > > > hdev2 --- vhdev3 --- als-drv > > > > > > > > (vhdev1 is probably not strictly necessary, but makes things more > > > > consistent). > > > > > > Oh, ok. > > > > > > How about the following: > > > > > > hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then > > > this driver creates 2 virtual hid drivers that are consistent > > > > > > like > > > > > > hdev1---ibridge-drv---vhdev1---tb-drv > > > hdev2--/ \--vhdev2---als-drv > > > > I don't think this will work. The problem is when the sub-drivers need > > to send a report or usb-command: how to they specify which hdev the > > report/command is destined for? While we could store the original hdev > > in each report (the hid_report's device field), that only works for > > hid_hw_request(), but not for things like hid_hw_raw_request() or > > hid_hw_output_report(). Now, currently I don't use the latter two; but > > I do need to send raw usb control messages in the touchbar driver > > (some commands are not proper hid reports), so it definitely breaks > > down there. > > > > Or am I missing something? > > I'd need to have a deeper look at the protocol, but you can emulate > pure HID devices by having your ibridge handling a translation from > set/get features/input to the usb control messages. Likewise, nothing > prevents you to slightly rewrite the report descriptors you present to > the als and touchbar to have a clear separation with the report ID. > > For example, if both hdev1 and hdev2 use a report ID of 0x01, you > could rewrite the report descriptor so that when you receive a report > with an id of 0x01 you send this to hdev1, but you can also translate > 0x11 to a report ID 0x01 to hdev2. > Likewise, report ID 0x42 could be a raw USB control message to the USB > under hdev2. > > Note that you will have to write 2 report descriptors for your new > devices, but you can take what makes sense from the original ones, and > just add a new collection with a vendor application with with an > opaque meaning (for the USB control messages). A couple things here. First of all, I went and rewrote the mfd driver with the hid-driver demultiplexer as a straight hid driver with 3 (well, 4 actually) virtual hid devices, as first discussed above. This overall led to some simplifications, with only smaller adjustments in the Touch Bar and ALS drivers (the diff stat shows 468 insertions, 825 deletions), so this looks good. Importantly (IMO), this leaves the whole awareness of the fact that the Touch Bar driver is talking to multiple usb interfaces and needs to coordinate appropriately between them (including things like which order they are accessed, sleep times between those accesses, and different power management) clearly in the Touch Bar driver, and the ibridge driver is still fairly generic unaware of any of the details that the sub-drivers need to worry about. Then I started looking more closely at your last suggestion above of creating only 2 virtual hid devices, with report descriptor merging/mangling and the addition of 3 custom reports (for the set-power, io-wait, and usb-control functionality), and I'm having trouble seeing the justification for it. AFAICT, the only advantage of this approach is that there are fewer virtual hid devices. But the disadvantages are significantly more code (especially in the ibridge driver) and more leakage of knowledge from the Touch Bar driver into the ibridge driver. In particular: * this leads to additional work, synchronization, and state management in the ibridge driver to deal with the fact that we have to wait for all (real) hid devices to be probed before we can start creating the virtual hid devices (and visa versa on removal). * merging and mangling those report descriptors requires re-parsing of the descriptors and dealing with various corner cases, which adds a bunch of code. * while the custom set-power and io-wait reports are simple, the custom usb-control report is ugly, because it actually ends up needing to compute some of the parameters and rewrite the data, as both those have values that require knowledge of the real underlying reports and usb interfaces (i.e. it's not a report for a generic usb-control call, but a very Touch Bar specific one, i.e. leaking particular Touch Bar driver knowledge into the ibridge driver). * In addition, while creating especially the custom usb-control report, I started to wonder if it's really worth serializing the parameters for the custom functions/report into an actual report buffer, just to be deserialized again two function calls down the stack. This became obvious when I was adding a helper function to the ibridge driver to serialize the function parameters, and at the same time writing a function right below it to deserialize those parameters again, all so we can call hid_hw_request() to pass them from the Touch Bar driver to the ibridge driver - but since the Touch Bar driver is already calling a custom function in the ibridge driver to serialize the parameters, it seems like that function might just as well make the desired underlying function call directly in the first place. Alternatively, the serializing functionality can be put in the Touch Bar driver instead, with the disadvantage that the implicit knowledge of the structure of the custom report is now spread over two modules. All of this seems somewhat ugly to me. Lastly, as hinted earlier, while this tries to hide the fact that there are actually multiple hid devices (aka usb interfaces) that are being driven by the Touch Bar driver, the Touch Bar driver still needs to be acutely aware of that fact because it cannot treat them equally. So now instead it clearly dealing with two different devices, it now has to do so indirectly by figuring out which reports (in the same virtual hid device) belong to which underlying real hid devices so it can treat those reports accordingly (e.g. when creating the reports to trigger set-power or usb-ctrl, instead of the desired device/interface being targeted directly, that info has to instead be passed somewhat opaquely via a report-id of a report that it happens to know is mapped to the desired device/interface). Sorry for the long-winded response. I hope it isn't too cryptic. But basically it boils down to: going for single virtual hid-device per real device adds a bunch of complexity and knowledge leaking from the Touch Bar driver the ibridge driver with AFAICT only a small advantage (namely fewer virtual devices). Cheers, Ronald
Hi ronald, On Mon, Jun 10, 2019 at 11:20 AM Life is hard, and then you die <ronald@innovation.ch> wrote: > > > Hi Benjamin, > > Sorry for the extremely late reply - RL etc. > > On Fri, Apr 26, 2019 at 08:26:25AM +0200, Benjamin Tissoires wrote: > > On Fri, Apr 26, 2019 at 7:56 AM Life is hard, and then you die > > <ronald@innovation.ch> wrote: > > > > > > On Thu, Apr 25, 2019 at 11:39:12AM +0200, Benjamin Tissoires wrote: > > > > On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die > > > > <ronald@innovation.ch> wrote: > > > > > > > > > > Hi Benjamin, > > > > > > > > > > Thank you for looking at this. > > > > > > > > > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote: > > > > > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote: > > > > > > > > > > > > > > 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, since the > > > > > > > functionality for the touch bar and light sensor is exposed via USB HID > > > > > > > interfaces, and the same HID device is used for multiple functions, this > > > > > > > driver provides a multiplexing layer that allows multiple HID drivers to > > > > > > > be registered for a given HID device. This allows the touch bar and ALS > > > > > > > driver to be separated out into their own modules. > > > > > > > > > > > > Sorry for coming late to the party, but IMO this series is far too > > > > > > complex for what you need. > > > > > > > > > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c, > > > > > > you need to have a HID driver that multiplex 2 other sub drivers > > > > > > through one USB communication. > > > > > > For that, you are using MFD, platform driver and you own sauce instead > > > > > > of creating a bus. > > > > > > > > > > Basically correct. To be a bit more precise, there are currently two > > > > > hid-devices and two drivers (touchbar and als) involved, with > > > > > connections as follows (pardon the ugly ascii art): > > > > > > > > > > hdev1 --- tb-drv > > > > > / > > > > > / > > > > > / > > > > > hdev2 --- als-drv > > > > > > > > > > i.e. the touchbar driver talks to both hdev's, and hdev2's events > > > > > (reports) are processed by both drivers (though each handles different > > > > > reports). > > > > > > > > > > > So, how about we reuse entirely the HID subsystem which already > > > > > > provides the capability you need (assuming I am correct above). > > > > > > hid-logitech-dj already does the same kind of stuff and you could: > > > > > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE > > > > > > - hid-ibridge will then register itself to the hid subsystem with a > > > > > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and > > > > > > hid_device_io_start(hdev) to enable the events (so you don't create > > > > > > useless input nodes for it) > > > > > > - then you add your 2 new devices by calling hid_allocate_device() and > > > > > > then hid_add_device(). You can even create a new HID group > > > > > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them > > > > > > from the actual USB device. > > > > > > - then you have 2 brand new HID devices you can create their driver as > > > > > > a regular ones. > > > > > > > > > > > > hid-ibridge.c would just need to behave like any other hid transport > > > > > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and > > > > > > you can get rid of at least the MFD and the platform part of your > > > > > > drivers. > > > > > > > > > > > > Does it makes sense or am I missing something obvious in the middle? > > > > > > > > > > Yes, I think I understand, and I think this can work. Basically, > > > > > instead of demux'ing at the hid-driver level as I am doing now (i.e. > > > > > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we > > > > > demux at the hid-device level (events forwarded from iBridge hdev to > > > > > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to > > > > > the original hdev via an iBridge ll_driver attached to the > > > > > sub-hdev's). > > > > > > > > > > So I would need to create 3 new "virtual" hid-devices (instances) as > > > > > follows: > > > > > > > > > > hdev1 --- vhdev1 --- tb-drv > > > > > / > > > > > -- vhdev2 -- > > > > > / > > > > > hdev2 --- vhdev3 --- als-drv > > > > > > > > > > (vhdev1 is probably not strictly necessary, but makes things more > > > > > consistent). > > > > > > > > Oh, ok. > > > > > > > > How about the following: > > > > > > > > hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then > > > > this driver creates 2 virtual hid drivers that are consistent > > > > > > > > like > > > > > > > > hdev1---ibridge-drv---vhdev1---tb-drv > > > > hdev2--/ \--vhdev2---als-drv > > > > > > I don't think this will work. The problem is when the sub-drivers need > > > to send a report or usb-command: how to they specify which hdev the > > > report/command is destined for? While we could store the original hdev > > > in each report (the hid_report's device field), that only works for > > > hid_hw_request(), but not for things like hid_hw_raw_request() or > > > hid_hw_output_report(). Now, currently I don't use the latter two; but > > > I do need to send raw usb control messages in the touchbar driver > > > (some commands are not proper hid reports), so it definitely breaks > > > down there. > > > > > > Or am I missing something? > > > > I'd need to have a deeper look at the protocol, but you can emulate > > pure HID devices by having your ibridge handling a translation from > > set/get features/input to the usb control messages. Likewise, nothing > > prevents you to slightly rewrite the report descriptors you present to > > the als and touchbar to have a clear separation with the report ID. > > > > For example, if both hdev1 and hdev2 use a report ID of 0x01, you > > could rewrite the report descriptor so that when you receive a report > > with an id of 0x01 you send this to hdev1, but you can also translate > > 0x11 to a report ID 0x01 to hdev2. > > Likewise, report ID 0x42 could be a raw USB control message to the USB > > under hdev2. > > > > Note that you will have to write 2 report descriptors for your new > > devices, but you can take what makes sense from the original ones, and > > just add a new collection with a vendor application with with an > > opaque meaning (for the USB control messages). > > A couple things here. First of all, I went and rewrote the mfd driver > with the hid-driver demultiplexer as a straight hid driver with 3 > (well, 4 actually) virtual hid devices, as first discussed above. > This overall led to some simplifications, with only smaller > adjustments in the Touch Bar and ALS drivers (the diff stat shows 468 > insertions, 825 deletions), so this looks good. Importantly (IMO), > this leaves the whole awareness of the fact that the Touch Bar driver > is talking to multiple usb interfaces and needs to coordinate > appropriately between them (including things like which order they are > accessed, sleep times between those accesses, and different power > management) clearly in the Touch Bar driver, and the ibridge driver is > still fairly generic unaware of any of the details that the > sub-drivers need to worry about. > > Then I started looking more closely at your last suggestion above of > creating only 2 virtual hid devices, with report descriptor > merging/mangling and the addition of 3 custom reports (for the > set-power, io-wait, and usb-control functionality), and I'm having > trouble seeing the justification for it. AFAICT, the only advantage of > this approach is that there are fewer virtual hid devices. But the > disadvantages are significantly more code (especially in the ibridge > driver) and more leakage of knowledge from the Touch Bar driver into > the ibridge driver. In particular: > > * this leads to additional work, synchronization, and state management > in the ibridge driver to deal with the fact that we have to wait for > all (real) hid devices to be probed before we can start creating the > virtual hid devices (and visa versa on removal). > * merging and mangling those report descriptors requires re-parsing > of the descriptors and dealing with various corner cases, which adds > a bunch of code. > * while the custom set-power and io-wait reports are simple, the > custom usb-control report is ugly, because it actually ends up > needing to compute some of the parameters and rewrite the data, as > both those have values that require knowledge of the real underlying > reports and usb interfaces (i.e. it's not a report for a generic > usb-control call, but a very Touch Bar specific one, i.e. leaking > particular Touch Bar driver knowledge into the ibridge driver). > * In addition, while creating especially the custom usb-control > report, I started to wonder if it's really worth serializing the > parameters for the custom functions/report into an actual report > buffer, just to be deserialized again two function calls down the > stack. This became obvious when I was adding a helper function to > the ibridge driver to serialize the function parameters, and at the > same time writing a function right below it to deserialize those > parameters again, all so we can call hid_hw_request() to pass them > from the Touch Bar driver to the ibridge driver - but since the > Touch Bar driver is already calling a custom function in the ibridge > driver to serialize the parameters, it seems like that function > might just as well make the desired underlying function call > directly in the first place. Alternatively, the serializing > functionality can be put in the Touch Bar driver instead, with the > disadvantage that the implicit knowledge of the structure of the > custom report is now spread over two modules. All of this seems > somewhat ugly to me. > > Lastly, as hinted earlier, while this tries to hide the fact that > there are actually multiple hid devices (aka usb interfaces) that are > being driven by the Touch Bar driver, the Touch Bar driver still needs > to be acutely aware of that fact because it cannot treat them equally. > So now instead it clearly dealing with two different devices, it now > has to do so indirectly by figuring out which reports (in the same > virtual hid device) belong to which underlying real hid devices so it > can treat those reports accordingly (e.g. when creating the reports to > trigger set-power or usb-ctrl, instead of the desired device/interface > being targeted directly, that info has to instead be passed somewhat > opaquely via a report-id of a report that it happens to know is mapped > to the desired device/interface). > > Sorry for the long-winded response. I hope it isn't too cryptic. But > basically it boils down to: going for single virtual hid-device per > real device adds a bunch of complexity and knowledge leaking from the > Touch Bar driver the ibridge driver with AFAICT only a small advantage > (namely fewer virtual devices). > I must confess that understanding all the details above without seeing the code is rather hard. However, if you have a simple and elegant solution right now that doesn't imply the MFD driver, how about posting it now so we can discuss it by looking at the code? I am fine putting the above explanation in a commit message to justify the current approach, but we are already talking about revision 3 when I haven't seen revision 2. Anyway, I can be convinced a design is better than the one I suggested. And sometime it's better to not abstract too much if the overall gets a little bit too complex. So can you post your current WIP? Cheers, Benjamin
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 76f9909cf396..d55fa77faacf 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1916,5 +1916,20 @@ config RAVE_SP_CORE Select this to get support for the Supervisory Processor device found on several devices in RAVE line of hardware. +config MFD_APPLE_IBRIDGE + tristate "Apple iBridge chip" + depends on ACPI + depends on USB_HID + depends on X86 || COMPILE_TEST + select MFD_CORE + help + This MFD provides the core support for the Apple iBridge chip + found on recent MacBookPro's. The drivers for the Touch Bar + (apple-ib-tb) and light sensor (apple-ib-als) need to be + enabled separately. + + To compile this driver as a module, choose M here: the + module will be called apple-ibridge. + endmenu endif diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 12980a4ad460..c364e0e9d313 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -241,4 +241,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o +obj-$(CONFIG_MFD_APPLE_IBRIDGE) += apple-ibridge.o diff --git a/drivers/mfd/apple-ibridge.c b/drivers/mfd/apple-ibridge.c new file mode 100644 index 000000000000..56d325396961 --- /dev/null +++ b/drivers/mfd/apple-ibridge.c @@ -0,0 +1,883 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Apple iBridge Driver + * + * Copyright (c) 2018 Ronald Tschalär + */ + +/** + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple + * iBridge chip (also known as T1 chip) which exposes the touch bar, + * built-in webcam (iSight), ambient light sensor, and Secure Enclave + * Processor (SEP) for TouchID. It shows up in the system as a USB device + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While + * the second one is used by MacOS to provide the fancy touch bar + * functionality with custom buttons etc, this driver just uses the first. + * + * In the first (default after boot) configuration, 4 usb interfaces are + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing + * the touch bar and the ambient light sensor (and possibly the SEP, + * though at this point in time nothing is known about that). The webcam + * interfaces are already handled by the uvcvideo driver; furthermore, the + * handling of the input reports when "keys" on the touch bar are pressed + * is already handled properly by the generic USB HID core. This leaves + * the management of the touch bar modes (e.g. switching between function + * and special keys when the FN key is pressed), the touch bar display + * (dimming and turning off), the key-remapping when the FN key is + * pressed, and handling of the light sensor. + * + * This driver is implemented as an MFD driver, with the touch bar and ALS + * functions implemented by appropriate subdrivers (mfd cells). Because + * both those are basically hid drivers, but the current kernel driver + * structure does not allow more than one driver per device, this driver + * implements a demuxer for hid drivers: it registers itself as a hid + * driver with the core, and in turn it lets the subdrivers register + * themselves as hid drivers with this driver; the callbacks from the core + * are then forwarded to the subdrivers. + * + * Lastly, this driver also takes care of the power-management for the + * iBridge when suspending and resuming. + */ + +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/hid.h> +#include <linux/list.h> +#include <linux/mfd/apple-ibridge.h> +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/rculist.h> +#include <linux/slab.h> +#include <linux/srcu.h> +#include <linux/usb.h> + +#include "../hid/usbhid/usbhid.h" + +#define USB_ID_VENDOR_APPLE 0x05ac +#define USB_ID_PRODUCT_IBRIDGE 0x8600 + +#define APPLETB_BASIC_CONFIG 1 + +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev) + +struct appleib_device { + struct acpi_device *acpi_dev; + acpi_handle asoc_socw; + struct list_head hid_drivers; + struct list_head hid_devices; + struct mfd_cell *subdevs; + struct mutex update_lock; /* protect updates to all lists */ + struct srcu_struct lists_srcu; + bool in_hid_probe; +}; + +struct appleib_hid_drv_info { + struct list_head entry; + struct hid_driver *driver; + void *driver_data; +}; + +struct appleib_hid_dev_info { + struct list_head entry; + struct list_head drivers; + struct hid_device *device; + const struct hid_device_id *device_id; + bool started; +}; + +static const struct mfd_cell appleib_subdevs[] = { + { .name = PLAT_NAME_IB_TB }, + { .name = PLAT_NAME_IB_ALS }, +}; + +static struct appleib_device *appleib_dev; + +#define call_void_driver_func(drv_info, fn, ...) \ + do { \ + if ((drv_info)->driver->fn) \ + (drv_info)->driver->fn(__VA_ARGS__); \ + } while (0) + +#define call_driver_func(drv_info, fn, ret_type, ...) \ + ({ \ + ret_type rc = 0; \ + \ + if ((drv_info)->driver->fn) \ + rc = (drv_info)->driver->fn(__VA_ARGS__); \ + \ + rc; \ + }) + +static void appleib_remove_driver(struct appleib_device *ib_dev, + struct appleib_hid_drv_info *drv_info, + struct appleib_hid_dev_info *dev_info) +{ + list_del_rcu(&drv_info->entry); + synchronize_srcu(&ib_dev->lists_srcu); + + call_void_driver_func(drv_info, remove, dev_info->device); + + kfree(drv_info); +} + +static int appleib_probe_driver(struct appleib_hid_drv_info *drv_info, + struct appleib_hid_dev_info *dev_info) +{ + struct appleib_hid_drv_info *d; + int rc = 0; + + rc = call_driver_func(drv_info, probe, int, dev_info->device, + dev_info->device_id); + if (rc) + return rc; + + d = kmemdup(drv_info, sizeof(*drv_info), GFP_KERNEL); + if (!d) { + call_void_driver_func(drv_info, remove, dev_info->device); + return -ENOMEM; + } + + list_add_tail_rcu(&d->entry, &dev_info->drivers); + return 0; +} + +static void appleib_remove_driver_attachments(struct appleib_device *ib_dev, + struct appleib_hid_dev_info *dev_info, + struct hid_driver *driver) +{ + struct appleib_hid_drv_info *drv_info; + struct appleib_hid_drv_info *tmp; + + list_for_each_entry_safe(drv_info, tmp, &dev_info->drivers, entry) { + if (!driver || drv_info->driver == driver) + appleib_remove_driver(ib_dev, drv_info, dev_info); + } +} + +/* + * Find all devices that are attached to this driver and detach them. + * + * Note: this must be run with update_lock held. + */ +static void appleib_detach_devices(struct appleib_device *ib_dev, + struct hid_driver *driver) +{ + struct appleib_hid_dev_info *dev_info; + + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) + appleib_remove_driver_attachments(ib_dev, dev_info, driver); +} + +/* + * Find all drivers that are attached to this device and detach them. + * + * Note: this must be run with update_lock held. + */ +static void appleib_detach_drivers(struct appleib_device *ib_dev, + struct appleib_hid_dev_info *dev_info) +{ + appleib_remove_driver_attachments(ib_dev, dev_info, NULL); +} + +/** + * Unregister a previously registered HID driver from us. + * @ib_dev: the appleib_device from which to unregister the driver + * @driver: the driver to unregister + */ +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, + struct hid_driver *driver) +{ + struct appleib_hid_drv_info *drv_info; + + mutex_lock(&ib_dev->update_lock); + + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { + if (drv_info->driver == driver) { + appleib_detach_devices(ib_dev, driver); + list_del_rcu(&drv_info->entry); + mutex_unlock(&ib_dev->update_lock); + synchronize_srcu(&ib_dev->lists_srcu); + kfree(drv_info); + dev_info(LOG_DEV(ib_dev), "unregistered driver '%s'\n", + driver->name); + return 0; + } + } + + mutex_unlock(&ib_dev->update_lock); + + return -ENOENT; +} +EXPORT_SYMBOL_GPL(appleib_unregister_hid_driver); + +static int appleib_start_hid_events(struct appleib_hid_dev_info *dev_info) +{ + struct hid_device *hdev = dev_info->device; + int rc; + + rc = hid_connect(hdev, HID_CONNECT_DEFAULT); + if (rc) { + hid_err(hdev, "ib: hid connect failed (%d)\n", rc); + return rc; + } + + rc = hid_hw_open(hdev); + if (rc) { + hid_err(hdev, "ib: failed to open hid: %d\n", rc); + hid_disconnect(hdev); + } + + if (!rc) + dev_info->started = true; + + return rc; +} + +static void appleib_stop_hid_events(struct appleib_hid_dev_info *dev_info) +{ + if (dev_info->started) { + hid_hw_close(dev_info->device); + hid_disconnect(dev_info->device); + dev_info->started = false; + } +} + +/** + * Register a HID driver with us. + * @ib_dev: the appleib_device with which to register the driver + * @driver: the driver to register + * @data: the driver-data to associate with the driver; this is available + * from appleib_get_drvdata(...). + */ +int appleib_register_hid_driver(struct appleib_device *ib_dev, + struct hid_driver *driver, void *data) +{ + struct appleib_hid_drv_info *drv_info; + struct appleib_hid_dev_info *dev_info; + int rc; + + if (!driver->probe) + return -EINVAL; + + drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL); + if (!drv_info) + return -ENOMEM; + + drv_info->driver = driver; + drv_info->driver_data = data; + + mutex_lock(&ib_dev->update_lock); + + list_add_tail_rcu(&drv_info->entry, &ib_dev->hid_drivers); + + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { + appleib_stop_hid_events(dev_info); + + appleib_probe_driver(drv_info, dev_info); + + rc = appleib_start_hid_events(dev_info); + if (rc) + appleib_detach_drivers(ib_dev, dev_info); + } + + mutex_unlock(&ib_dev->update_lock); + + dev_info(LOG_DEV(ib_dev), "registered driver '%s'\n", driver->name); + + return 0; +} +EXPORT_SYMBOL_GPL(appleib_register_hid_driver); + +/** + * Get the driver-specific data associated with the given, previously + * registered HID driver (provided in the appleib_register_hid_driver() + * call). + * @ib_dev: the appleib_device with which the driver was registered + * @driver: the driver for which to get the data + */ +void *appleib_get_drvdata(struct appleib_device *ib_dev, + struct hid_driver *driver) +{ + struct appleib_hid_drv_info *drv_info; + void *drv_data = NULL; + int idx; + + idx = srcu_read_lock(&ib_dev->lists_srcu); + + list_for_each_entry_rcu(drv_info, &ib_dev->hid_drivers, entry) { + if (drv_info->driver == driver) { + drv_data = drv_info->driver_data; + break; + } + } + + srcu_read_unlock(&ib_dev->lists_srcu, idx); + + return drv_data; +} +EXPORT_SYMBOL_GPL(appleib_get_drvdata); + +/* + * Forward a hid-driver callback to all registered sub-drivers. This is for + * callbacks that return a status as an int. + * @hdev the hid-device + * @forward a function that calls the callback on the given driver + * @args arguments for the forward function + */ +static int appleib_forward_int_op(struct hid_device *hdev, + int (*forward)(struct appleib_hid_drv_info *, + struct hid_device *, void *), + void *args) +{ + struct appleib_device *ib_dev = hid_get_drvdata(hdev); + struct appleib_hid_dev_info *dev_info; + struct appleib_hid_drv_info *drv_info; + int idx; + int rc = 0; + + idx = srcu_read_lock(&ib_dev->lists_srcu); + + list_for_each_entry_rcu(dev_info, &ib_dev->hid_devices, entry) { + if (dev_info->device != hdev) + continue; + + list_for_each_entry_rcu(drv_info, &dev_info->drivers, entry) { + rc = forward(drv_info, hdev, args); + if (rc) + break; + } + + break; + } + + srcu_read_unlock(&ib_dev->lists_srcu, idx); + + return rc; +} + +struct appleib_hid_event_args { + struct hid_field *field; + struct hid_usage *usage; + __s32 value; +}; + +static int appleib_hid_event_fwd(struct appleib_hid_drv_info *drv_info, + struct hid_device *hdev, void *args) +{ + struct appleib_hid_event_args *evt_args = args; + + return call_driver_func(drv_info, event, int, hdev, evt_args->field, + evt_args->usage, evt_args->value); +} + +static int appleib_hid_event(struct hid_device *hdev, struct hid_field *field, + struct hid_usage *usage, __s32 value) +{ + struct appleib_hid_event_args args = { + .field = field, + .usage = usage, + .value = value, + }; + + return appleib_forward_int_op(hdev, appleib_hid_event_fwd, &args); +} + +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc, + unsigned int *rsize) +{ + /* Some fields have a size of 64 bits, which according to HID 1.11 + * Section 8.4 is not valid ("An item field cannot span more than 4 + * bytes in a report"). Furthermore, hid_field_extract() complains + * when encountering such a field. So turn them into two 32-bit fields + * instead. + */ + + if (*rsize == 634 && + /* Usage Page 0xff12 (vendor defined) */ + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && + /* Usage 0x51 */ + rdesc[416] == 0x09 && rdesc[417] == 0x51 && + /* report size 64 */ + rdesc[432] == 0x75 && rdesc[433] == 64 && + /* report count 1 */ + rdesc[434] == 0x95 && rdesc[435] == 1) { + rdesc[433] = 32; + rdesc[435] = 2; + hid_dbg(hdev, "Fixed up first 64-bit field\n"); + } + + if (*rsize == 634 && + /* Usage Page 0xff12 (vendor defined) */ + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff && + /* Usage 0x51 */ + rdesc[611] == 0x09 && rdesc[612] == 0x51 && + /* report size 64 */ + rdesc[627] == 0x75 && rdesc[628] == 64 && + /* report count 1 */ + rdesc[629] == 0x95 && rdesc[630] == 1) { + rdesc[628] = 32; + rdesc[630] = 2; + hid_dbg(hdev, "Fixed up second 64-bit field\n"); + } + + return rdesc; +} + +static int appleib_input_configured_fwd(struct appleib_hid_drv_info *drv_info, + struct hid_device *hdev, void *args) +{ + return call_driver_func(drv_info, input_configured, int, hdev, + (struct hid_input *)args); +} + +static int appleib_input_configured(struct hid_device *hdev, + struct hid_input *hidinput) +{ + return appleib_forward_int_op(hdev, appleib_input_configured_fwd, + hidinput); +} + +#ifdef CONFIG_PM +static int appleib_hid_suspend_fwd(struct appleib_hid_drv_info *drv_info, + struct hid_device *hdev, void *args) +{ + return call_driver_func(drv_info, suspend, int, hdev, + *(pm_message_t *)args); +} + +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message) +{ + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message); +} + +static int appleib_hid_resume_fwd(struct appleib_hid_drv_info *drv_info, + struct hid_device *hdev, void *args) +{ + return call_driver_func(drv_info, resume, int, hdev); +} + +static int appleib_hid_resume(struct hid_device *hdev) +{ + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL); +} + +static int appleib_hid_reset_resume_fwd(struct appleib_hid_drv_info *drv_info, + struct hid_device *hdev, void *args) +{ + return call_driver_func(drv_info, reset_resume, int, hdev); +} + +static int appleib_hid_reset_resume(struct hid_device *hdev) +{ + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL); +} +#endif /* CONFIG_PM */ + +/** + * Find the field in the report with the given usage. + * @report: the report to search + * @field_usage: the usage of the field to search for + */ +struct hid_field *appleib_find_report_field(struct hid_report *report, + unsigned int field_usage) +{ + int f, u; + + for (f = 0; f < report->maxfield; f++) { + struct hid_field *field = report->field[f]; + + if (field->logical == field_usage) + return field; + + for (u = 0; u < field->maxusage; u++) { + if (field->usage[u].hid == field_usage) + return field; + } + } + + return NULL; +} +EXPORT_SYMBOL_GPL(appleib_find_report_field); + +/** + * Search all the reports of the device for the field with the given usage. + * @hdev: the device whose reports to search + * @application: the usage of application collection that the field must + * belong to + * @field_usage: the usage of the field to search for + */ +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, + unsigned int application, + unsigned int field_usage) +{ + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT, + HID_FEATURE_REPORT }; + struct hid_report *report; + struct hid_field *field; + int t; + + for (t = 0; t < ARRAY_SIZE(report_types); t++) { + struct list_head *report_list = + &hdev->report_enum[report_types[t]].report_list; + list_for_each_entry(report, report_list, list) { + if (report->application != application) + continue; + + field = appleib_find_report_field(report, field_usage); + if (field) + return field; + } + } + + return NULL; +} +EXPORT_SYMBOL_GPL(appleib_find_hid_field); + +/** + * Return whether we're currently inside a hid_device_probe or not. + * @ib_dev: the appleib_device + */ +bool appleib_in_hid_probe(struct appleib_device *ib_dev) +{ + return ib_dev->in_hid_probe; +} +EXPORT_SYMBOL_GPL(appleib_in_hid_probe); + +static struct appleib_hid_dev_info * +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev, + const struct hid_device_id *id) +{ + struct appleib_hid_dev_info *dev_info; + struct appleib_hid_drv_info *drv_info; + + /* allocate device-info for this device */ + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); + if (!dev_info) + return NULL; + + INIT_LIST_HEAD(&dev_info->drivers); + dev_info->device = hdev; + dev_info->device_id = id; + + /* notify all our sub drivers */ + mutex_lock(&ib_dev->update_lock); + + ib_dev->in_hid_probe = true; + + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) { + appleib_probe_driver(drv_info, dev_info); + } + + ib_dev->in_hid_probe = false; + + /* remember this new device */ + list_add_tail_rcu(&dev_info->entry, &ib_dev->hid_devices); + + mutex_unlock(&ib_dev->update_lock); + + return dev_info; +} + +static void appleib_remove_device(struct appleib_device *ib_dev, + struct appleib_hid_dev_info *dev_info) +{ + list_del_rcu(&dev_info->entry); + synchronize_srcu(&ib_dev->lists_srcu); + + appleib_detach_drivers(ib_dev, dev_info); + + kfree(dev_info); +} + +static int appleib_hid_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + struct appleib_device *ib_dev; + struct appleib_hid_dev_info *dev_info; + struct usb_device *udev; + int rc; + + /* check usb config first */ + udev = hid_to_usb_dev(hdev); + + if (udev->actconfig->desc.bConfigurationValue != APPLETB_BASIC_CONFIG) { + rc = usb_driver_set_configuration(udev, APPLETB_BASIC_CONFIG); + return rc ? rc : -ENODEV; + } + + /* Assign the driver data */ + ib_dev = appleib_dev; + hid_set_drvdata(hdev, ib_dev); + + /* initialize the report info */ + rc = hid_parse(hdev); + if (rc) { + hid_err(hdev, "ib: hid parse failed (%d)\n", rc); + goto error; + } + + /* alloc bufs etc so probe's can send requests; but connect later */ + rc = hid_hw_start(hdev, 0); + if (rc) { + hid_err(hdev, "ib: hw start failed (%d)\n", rc); + goto error; + } + + /* add this hdev to our device list */ + dev_info = appleib_add_device(ib_dev, hdev, id); + if (!dev_info) { + rc = -ENOMEM; + goto stop_hw; + } + + /* start the hid */ + rc = appleib_start_hid_events(dev_info); + if (rc) + goto remove_dev; + + return 0; + +remove_dev: + mutex_lock(&ib_dev->update_lock); + appleib_remove_device(ib_dev, dev_info); + mutex_unlock(&ib_dev->update_lock); +stop_hw: + hid_hw_stop(hdev); +error: + return rc; +} + +static void appleib_hid_remove(struct hid_device *hdev) +{ + struct appleib_device *ib_dev = hid_get_drvdata(hdev); + struct appleib_hid_dev_info *dev_info; + + mutex_lock(&ib_dev->update_lock); + + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) { + if (dev_info->device == hdev) { + appleib_stop_hid_events(dev_info); + appleib_remove_device(ib_dev, dev_info); + break; + } + } + + mutex_unlock(&ib_dev->update_lock); + + hid_hw_stop(hdev); +} + +static const struct hid_device_id appleib_hid_devices[] = { + { HID_USB_DEVICE(USB_ID_VENDOR_APPLE, USB_ID_PRODUCT_IBRIDGE) }, + { }, +}; + +static struct hid_driver appleib_hid_driver = { + .name = "apple-ibridge-hid", + .id_table = appleib_hid_devices, + .probe = appleib_hid_probe, + .remove = appleib_hid_remove, + .event = appleib_hid_event, + .report_fixup = appleib_report_fixup, + .input_configured = appleib_input_configured, +#ifdef CONFIG_PM + .suspend = appleib_hid_suspend, + .resume = appleib_hid_resume, + .reset_resume = appleib_hid_reset_resume, +#endif +}; + +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev) +{ + struct appleib_device *ib_dev; + acpi_status sts; + int rc; + + /* allocate */ + ib_dev = kzalloc(sizeof(*ib_dev), GFP_KERNEL); + if (!ib_dev) + return ERR_PTR(-ENOMEM); + + /* init structures */ + INIT_LIST_HEAD(&ib_dev->hid_drivers); + INIT_LIST_HEAD(&ib_dev->hid_devices); + mutex_init(&ib_dev->update_lock); + init_srcu_struct(&ib_dev->lists_srcu); + + ib_dev->acpi_dev = acpi_dev; + + /* get iBridge acpi power control method */ + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw); + if (ACPI_FAILURE(sts)) { + dev_err(LOG_DEV(ib_dev), + "Error getting handle for ASOC.SOCW method: %s\n", + acpi_format_exception(sts)); + rc = -ENXIO; + goto free_mem; + } + + /* ensure iBridge is powered on */ + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); + if (ACPI_FAILURE(sts)) + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", + acpi_format_exception(sts)); + + return ib_dev; + +free_mem: + kfree(ib_dev); + return ERR_PTR(rc); +} + +static int appleib_probe(struct acpi_device *acpi) +{ + struct appleib_device *ib_dev; + struct appleib_platform_data *pdata; + int i; + int ret; + + if (appleib_dev) + return -EBUSY; + + ib_dev = appleib_alloc_device(acpi); + if (IS_ERR_OR_NULL(ib_dev)) + return PTR_ERR(ib_dev); + + ib_dev->subdevs = kmemdup(appleib_subdevs, sizeof(appleib_subdevs), + GFP_KERNEL); + if (!ib_dev->subdevs) { + ret = -ENOMEM; + goto free_dev; + } + + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + ret = -ENOMEM; + goto free_subdevs; + } + + pdata->ib_dev = ib_dev; + pdata->log_dev = LOG_DEV(ib_dev); + for (i = 0; i < ARRAY_SIZE(appleib_subdevs); i++) { + ib_dev->subdevs[i].platform_data = pdata; + ib_dev->subdevs[i].pdata_size = sizeof(*pdata); + } + + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE, + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs), + NULL, 0, NULL); + if (ret) { + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret); + goto free_pdata; + } + + acpi->driver_data = ib_dev; + appleib_dev = ib_dev; + + ret = hid_register_driver(&appleib_hid_driver); + if (ret) { + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n", + ret); + goto rem_mfd_devs; + } + + return 0; + +rem_mfd_devs: + mfd_remove_devices(&acpi->dev); +free_pdata: + kfree(pdata); +free_subdevs: + kfree(ib_dev->subdevs); +free_dev: + appleib_dev = NULL; + acpi->driver_data = NULL; + kfree(ib_dev); + return ret; +} + +static int appleib_remove(struct acpi_device *acpi) +{ + struct appleib_device *ib_dev = acpi_driver_data(acpi); + + mfd_remove_devices(&acpi->dev); + hid_unregister_driver(&appleib_hid_driver); + + if (appleib_dev == ib_dev) + appleib_dev = NULL; + + kfree(ib_dev->subdevs[0].platform_data); + kfree(ib_dev->subdevs); + kfree(ib_dev); + + return 0; +} + +static int appleib_suspend(struct device *dev) +{ + struct acpi_device *adev; + struct appleib_device *ib_dev; + int rc; + + adev = to_acpi_device(dev); + ib_dev = acpi_driver_data(adev); + + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0); + if (ACPI_FAILURE(rc)) + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n", + acpi_format_exception(rc)); + + return 0; +} + +static int appleib_resume(struct device *dev) +{ + struct acpi_device *adev; + struct appleib_device *ib_dev; + int rc; + + adev = to_acpi_device(dev); + ib_dev = acpi_driver_data(adev); + + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1); + if (ACPI_FAILURE(rc)) + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n", + acpi_format_exception(rc)); + + return 0; +} + +static const struct dev_pm_ops appleib_pm = { + .suspend = appleib_suspend, + .resume = appleib_resume, + .restore = appleib_resume, +}; + +static const struct acpi_device_id appleib_acpi_match[] = { + { "APP7777", 0 }, + { }, +}; + +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match); + +static struct acpi_driver appleib_driver = { + .name = "apple-ibridge", + .class = "topcase", /* ? */ + .owner = THIS_MODULE, + .ids = appleib_acpi_match, + .ops = { + .add = appleib_probe, + .remove = appleib_remove, + }, + .drv = { + .pm = &appleib_pm, + }, +}; + +module_acpi_driver(appleib_driver) + +MODULE_AUTHOR("Ronald Tschalär"); +MODULE_DESCRIPTION("Apple iBridge driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/apple-ibridge.h b/include/linux/mfd/apple-ibridge.h new file mode 100644 index 000000000000..d321714767f7 --- /dev/null +++ b/include/linux/mfd/apple-ibridge.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Apple iBridge Driver + * + * Copyright (c) 2018 Ronald Tschalär + */ + +#ifndef __LINUX_MFD_APPLE_IBRDIGE_H +#define __LINUX_MFD_APPLE_IBRDIGE_H + +#include <linux/device.h> +#include <linux/hid.h> + +#define PLAT_NAME_IB_TB "apple-ib-tb" +#define PLAT_NAME_IB_ALS "apple-ib-als" + +struct appleib_device; + +struct appleib_platform_data { + struct appleib_device *ib_dev; + struct device *log_dev; +}; + +int appleib_register_hid_driver(struct appleib_device *ib_dev, + struct hid_driver *driver, void *data); +int appleib_unregister_hid_driver(struct appleib_device *ib_dev, + struct hid_driver *driver); + +void *appleib_get_drvdata(struct appleib_device *ib_dev, + struct hid_driver *driver); +bool appleib_in_hid_probe(struct appleib_device *ib_dev); + +struct hid_field *appleib_find_report_field(struct hid_report *report, + unsigned int field_usage); +struct hid_field *appleib_find_hid_field(struct hid_device *hdev, + unsigned int application, + unsigned int field_usage); + +#endif
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, since the functionality for the touch bar and light sensor is exposed via USB HID interfaces, and the same HID device is used for multiple functions, this driver provides a multiplexing layer that allows multiple HID drivers to be registered for a given HID device. This allows the touch bar and ALS driver to be separated out into their own modules. Signed-off-by: Ronald Tschalär <ronald@innovation.ch> --- drivers/mfd/Kconfig | 15 + drivers/mfd/Makefile | 1 + drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++ include/linux/mfd/apple-ibridge.h | 39 ++ 4 files changed, 938 insertions(+) create mode 100644 drivers/mfd/apple-ibridge.c create mode 100644 include/linux/mfd/apple-ibridge.h