Subject: [PATCH v2 0/3] Apple iBridge support

2016 and 2017 MacBook Pro's have a T1 chip that drives the Touch Bar,
ambient light sensor, webcam, and fingerprint sensor; this shows up
as an iBridge USB device in the system. These patches provide initial
support for the Touch Bar and ALS - the webcam is already handled by
existing drivers, and no information is currently known on how to access
the fingerprint sensor (other than it's apparently via one of the extra
interfaces available in the OS X USB configuration).

One thing of note here is that both the ALS and (some of) the Touch Bar
functionality are exposed via the same USB interface (and hence same
hid_device), so both drivers need to share this device. This is solved
by having the iBridge driver create multiple virtual HID devices for
each real HID device to which the Touch Bar and ALS drivers attach, and
then forwarding the calls between the real and virtual HID devices, so
we end up with a structure like this:

iBridge (HID) driver Sub drivers

-- vhdev0 -- (unbound)
/
hdev1 --- vhdev1 --- tb-drv
/
-- vhdev2 --
/
hdev2 --- vhdev3 --- als-drv


Changes in v2:
- Changed iBridge driver from an MFD driver to a HID driver. Instead
of creating virtual HID drivers and (de)multiplexing their calls,
this now create virtual HID devices and (de)multiplexing the
operations on them. This is from the feedback by Benjamin Tissoires.
- Applied all feedback on ALS driver from Jonathan Cameron
- Applied all feedback on Touch Bar driver from Jonathan Cameron
and Peter Meerwald-Stadler
- various smaller cleanups

Ronald Tschalär (3):
HID: apple-ibridge: Add Apple iBridge HID driver.
HID: apple-ib-tb: Add driver for the Touch Bar on MacBook Pro's.
iio: light: apple-ib-als: Add driver for ALS on iBridge chip.

drivers/hid/Kconfig | 24 +
drivers/hid/Makefile | 2 +
drivers/hid/apple-ib-tb.c | 1389 ++++++++++++++++++++++++++++++
drivers/hid/apple-ibridge.c | 588 +++++++++++++
drivers/hid/hid-ids.h | 1 +
drivers/iio/light/Kconfig | 12 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/apple-ib-als.c | 607 +++++++++++++
include/linux/apple-ibridge.h | 23 +
9 files changed, 2647 insertions(+)
create mode 100644 drivers/hid/apple-ib-tb.c
create mode 100644 drivers/hid/apple-ibridge.c
create mode 100644 drivers/iio/light/apple-ib-als.c
create mode 100644 include/linux/apple-ibridge.h

--
2.21.0


Subject: [PATCH v2 1/3] HID: apple-ibridge: Add Apple iBridge HID driver.

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 creates virtual HID devices, one per real HID device and
sub-driver pair (for a total of 4 virtual HID devices). The sub-drivers
then bind to these virtual HID devices.

This way the Touch Bar and ALS drivers can be kept in their own modules,
while at the same time making them look very much like as if they were
connected to the real HID devices; e.g. in particular the Touch Bar
driver is aware of the fact that it is dealing with two HID devices that
need to managed differently.

Signed-off-by: Ronald Tschalär <[email protected]>
---
drivers/hid/Kconfig | 14 +
drivers/hid/Makefile | 1 +
drivers/hid/apple-ibridge.c | 585 ++++++++++++++++++++++++++++++++++
drivers/hid/hid-ids.h | 1 +
include/linux/apple-ibridge.h | 23 ++
5 files changed, 624 insertions(+)
create mode 100644 drivers/hid/apple-ibridge.c
create mode 100644 include/linux/apple-ibridge.h

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ca0cdfa6b33..545d3691fc1c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -135,6 +135,20 @@ config HID_APPLE
Say Y here if you want support for keyboards of Apple iBooks, PowerBooks,
MacBooks, MacBook Pros and Apple Aluminum.

+config HID_APPLE_IBRIDGE
+ tristate "Apple iBridge"
+ depends on ACPI
+ depends on USB_HID
+ depends on X86 || COMPILE_TEST
+ help
+ This module provides the core support for the Apple T1 chip found
+ on recent MacBookPro's, also known as the iBridge. 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.
+
config HID_APPLEIR
tristate "Apple infrared receiver"
depends on (USB_HID)
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 170163b41303..a4da5663a541 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += hid-accutouch.o
obj-$(CONFIG_HID_ALPS) += hid-alps.o
obj-$(CONFIG_HID_ACRUX) += hid-axff.o
obj-$(CONFIG_HID_APPLE) += hid-apple.o
+obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
obj-$(CONFIG_HID_ASUS) += hid-asus.o
obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
diff --git a/drivers/hid/apple-ibridge.c b/drivers/hid/apple-ibridge.c
new file mode 100644
index 000000000000..565f080a38d6
--- /dev/null
+++ b/drivers/hid/apple-ibridge.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple iBridge Driver
+ *
+ * Copyright (c) 2018 Ronald Tschalär
+ */
+
+/**
+ * DOC: Overview
+ *
+ * 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. 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 a HID driver that creates virtual HID devices
+ * for the ALS and touch bar functionality, and the ALS and touch bar drivers
+ * are HID drivers which in turn attach to these virtual HID devices. This
+ * driver then relays the calls on the real HID devices to the virtual ones,
+ * and visa versa.
+ *
+ * Lastly, this driver also takes care of the power-management for the
+ * iBridge when suspending and resuming.
+ */
+
+#include <linux/acpi.h>
+#include <linux/apple-ibridge.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+#include "../hid/usbhid/usbhid.h"
+
+#define APPLEIB_BASIC_CONFIG 1
+
+#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev)
+
+static struct hid_device_id appleib_sub_hid_ids[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
+ USB_DEVICE_ID_IBRIDGE_TB) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
+ USB_DEVICE_ID_IBRIDGE_ALS) },
+};
+
+struct appleib_device {
+ struct acpi_device *acpi_dev;
+ acpi_handle asoc_socw;
+};
+
+struct appleib_hid_dev_info {
+ struct hid_device *hdev;
+ struct hid_device *sub_hdevs[ARRAY_SIZE(appleib_sub_hid_ids)];
+};
+
+static void appleib_remove_device(struct hid_device *hdev)
+{
+ struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
+ hid_destroy_device(hdev_info->sub_hdevs[i]);
+
+ hid_set_drvdata(hdev, NULL);
+}
+
+/**
+ * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
+ * all virtual HID devices attached to the given real HID device.
+ * @hdev the real hid-device
+ * @forward a function that calls the callback on the given driver
+ * @args arguments for the forward function
+ *
+ * This is for callbacks that return a status as an int.
+ *
+ * Returns: 0 on success, or the first error returned by the @forward function.
+ */
+static int appleib_forward_int_op(struct hid_device *hdev,
+ int (*forward)(struct hid_driver *,
+ struct hid_device *, void *),
+ void *args)
+{
+ struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
+ struct hid_device *sub_hdev;
+ int rc = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
+ sub_hdev = hdev_info->sub_hdevs[i];
+ if (sub_hdev->driver) {
+ rc = forward(sub_hdev->driver, sub_hdev, args);
+ if (rc)
+ break;
+ }
+
+ break;
+ }
+
+ return rc;
+}
+
+static int appleib_hid_raw_event(struct hid_device *hdev,
+ struct hid_report *report, u8 *data, int size)
+{
+ struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
+ hid_input_report(hdev_info->sub_hdevs[i], report->type, data,
+ size, 0);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int appleib_hid_suspend_fwd(struct hid_driver *drv,
+ struct hid_device *hdev, void *args)
+{
+ int rc = 0;
+
+ if (drv->suspend)
+ rc = drv->suspend(hdev, *(pm_message_t *)args);
+
+ return rc;
+}
+
+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 hid_driver *drv,
+ struct hid_device *hdev, void *args)
+{
+ int rc = 0;
+
+ if (drv->resume)
+ rc = drv->resume(hdev);
+
+ return rc;
+}
+
+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 hid_driver *drv,
+ struct hid_device *hdev, void *args)
+{
+ int rc = 0;
+
+ if (drv->reset_resume)
+ rc = drv->reset_resume(hdev);
+
+ return rc;
+}
+
+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 */
+
+/**
+ * appleib_find_report_field() - 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
+ *
+ * Returns: the hid field if found, or NULL if none found.
+ */
+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);
+
+/**
+ * appleib_find_hid_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
+ *
+ * Returns: the hid field if found, or NULL if none found.
+ */
+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);
+
+static int appleib_ll_start(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static void appleib_ll_stop(struct hid_device *hdev)
+{
+}
+
+static int appleib_ll_open(struct hid_device *hdev)
+{
+ // TODO: allow event reporting
+ return 0;
+}
+
+static void appleib_ll_close(struct hid_device *hdev)
+{
+ // TODO: disallow event reporting
+}
+
+static int appleib_ll_power(struct hid_device *hdev, int level)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ return hid_hw_power(hdev_info->hdev, level);
+}
+
+static int appleib_ll_parse(struct hid_device *hdev)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ return hid_parse_report(hdev, hdev_info->hdev->rdesc,
+ hdev_info->hdev->rsize);
+}
+
+static void appleib_ll_request(struct hid_device *hdev,
+ struct hid_report *report, int reqtype)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ hid_hw_request(hdev_info->hdev, report, reqtype);
+}
+
+static int appleib_ll_wait(struct hid_device *hdev)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ hid_hw_wait(hdev_info->hdev);
+ return 0;
+}
+
+static int appleib_ll_raw_request(struct hid_device *hdev,
+ unsigned char reportnum, __u8 *buf,
+ size_t len, unsigned char rtype, int reqtype)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
+ reqtype);
+}
+
+static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
+ size_t len)
+{
+ struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
+
+ return hid_hw_output_report(hdev_info->hdev, buf, len);
+}
+
+static struct hid_ll_driver appleib_ll_driver = {
+ .start = appleib_ll_start,
+ .stop = appleib_ll_stop,
+ .open = appleib_ll_open,
+ .close = appleib_ll_close,
+ .power = appleib_ll_power,
+ .parse = appleib_ll_parse,
+ .request = appleib_ll_request,
+ .wait = appleib_ll_wait,
+ .raw_request = appleib_ll_raw_request,
+ .output_report = appleib_ll_output_report,
+};
+
+static struct hid_device *
+appleib_add_sub_dev(struct appleib_hid_dev_info *hdev_info,
+ struct hid_device_id *dev_id)
+{
+ struct hid_device *sub_hdev;
+ int rc;
+
+ sub_hdev = hid_allocate_device();
+ if (IS_ERR(sub_hdev))
+ return sub_hdev;
+
+ sub_hdev->dev.parent = &hdev_info->hdev->dev;
+
+ sub_hdev->bus = dev_id->bus;
+ sub_hdev->group = dev_id->group;
+ sub_hdev->vendor = dev_id->vendor;
+ sub_hdev->product = dev_id->product;
+
+ sub_hdev->ll_driver = &appleib_ll_driver;
+
+ snprintf(sub_hdev->name, sizeof(sub_hdev->name),
+ "iBridge Virtual HID %s/%04x:%04x",
+ dev_name(sub_hdev->dev.parent), sub_hdev->vendor,
+ sub_hdev->product);
+
+ sub_hdev->driver_data = hdev_info;
+
+ rc = hid_add_device(sub_hdev);
+ if (rc) {
+ hid_destroy_device(sub_hdev);
+ return ERR_PTR(rc);
+ }
+
+ return sub_hdev;
+}
+
+static struct appleib_hid_dev_info *appleib_add_device(struct hid_device *hdev)
+{
+ struct appleib_hid_dev_info *hdev_info;
+ int i;
+
+ hdev_info = devm_kzalloc(&hdev->dev, sizeof(*hdev_info), GFP_KERNEL);
+ if (!hdev_info)
+ return ERR_PTR(-ENOMEM);
+
+ hdev_info->hdev = hdev;
+
+ for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
+ hdev_info->sub_hdevs[i] =
+ appleib_add_sub_dev(hdev_info, &appleib_sub_hid_ids[i]);
+
+ if (IS_ERR(hdev_info->sub_hdevs[i])) {
+ while (i-- > 0)
+ hid_destroy_device(hdev_info->sub_hdevs[i]);
+ return (void *)hdev_info->sub_hdevs[i];
+ }
+ }
+
+ return hdev_info;
+}
+
+static int appleib_hid_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct appleib_hid_dev_info *hdev_info;
+ struct usb_device *udev;
+ int rc;
+
+ /* check and set usb config first */
+ udev = hid_to_usb_dev(hdev);
+
+ if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
+ rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
+ return rc ? rc : -ENODEV;
+ }
+
+ rc = hid_parse(hdev);
+ if (rc) {
+ hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
+ goto error;
+ }
+
+ rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+ if (rc) {
+ hid_err(hdev, "ib: hw start failed (%d)\n", rc);
+ goto error;
+ }
+
+ hdev_info = appleib_add_device(hdev);
+ if (IS_ERR(hdev_info)) {
+ rc = PTR_ERR(hdev_info);
+ goto stop_hw;
+ }
+
+ hid_set_drvdata(hdev, hdev_info);
+
+ rc = hid_hw_open(hdev);
+ if (rc) {
+ hid_err(hdev, "ib: failed to open hid: %d\n", rc);
+ goto remove_dev;
+ }
+
+ return 0;
+
+remove_dev:
+ appleib_remove_device(hdev);
+stop_hw:
+ hid_hw_stop(hdev);
+error:
+ return rc;
+}
+
+static void appleib_hid_remove(struct hid_device *hdev)
+{
+ hid_hw_close(hdev);
+ appleib_remove_device(hdev);
+ hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id appleib_hid_ids[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
+ { },
+};
+
+static struct hid_driver appleib_hid_driver = {
+ .name = "apple-ibridge-hid",
+ .id_table = appleib_hid_ids,
+ .probe = appleib_hid_probe,
+ .remove = appleib_hid_remove,
+ .raw_event = appleib_hid_raw_event,
+#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;
+
+ ib_dev = devm_kzalloc(&acpi_dev->dev, sizeof(*ib_dev), GFP_KERNEL);
+ if (!ib_dev)
+ return ERR_PTR(-ENOMEM);
+
+ ib_dev->acpi_dev = acpi_dev;
+
+ /* get iBridge acpi power control method for suspend/resume */
+ 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));
+ return ERR_PTR(-ENXIO);
+ }
+
+ /* 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;
+}
+
+static int appleib_probe(struct acpi_device *acpi)
+{
+ struct appleib_device *ib_dev;
+ int ret;
+
+ ib_dev = appleib_alloc_device(acpi);
+ if (IS_ERR(ib_dev))
+ return PTR_ERR(ib_dev);
+
+ ret = hid_register_driver(&appleib_hid_driver);
+ if (ret) {
+ dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
+ ret);
+ return ret;
+ }
+
+ acpi->driver_data = ib_dev;
+
+ return 0;
+}
+
+static int appleib_remove(struct acpi_device *acpi)
+{
+ hid_unregister_driver(&appleib_hid_driver);
+
+ return 0;
+}
+
+static int appleib_suspend(struct device *dev)
+{
+ struct appleib_device *ib_dev;
+ int rc;
+
+ ib_dev = acpi_driver_data(to_acpi_device(dev));
+
+ rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
+ if (ACPI_FAILURE(rc))
+ dev_warn(dev, "SOCW(0) failed: %s\n",
+ acpi_format_exception(rc));
+
+ return 0;
+}
+
+static int appleib_resume(struct device *dev)
+{
+ struct appleib_device *ib_dev;
+ int rc;
+
+ ib_dev = acpi_driver_data(to_acpi_device(dev));
+
+ rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
+ if (ACPI_FAILURE(rc))
+ dev_warn(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/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index adce58f24f76..f963bb02c477 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -177,6 +177,7 @@
#define USB_DEVICE_ID_APPLE_IRCONTROL3 0x8241
#define USB_DEVICE_ID_APPLE_IRCONTROL4 0x8242
#define USB_DEVICE_ID_APPLE_IRCONTROL5 0x8243
+#define USB_DEVICE_ID_APPLE_IBRIDGE 0x8600

#define USB_VENDOR_ID_ASUS 0x0486
#define USB_DEVICE_ID_ASUS_T91MT 0x0185
diff --git a/include/linux/apple-ibridge.h b/include/linux/apple-ibridge.h
new file mode 100644
index 000000000000..07ded8c68776
--- /dev/null
+++ b/include/linux/apple-ibridge.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Apple iBridge Driver
+ *
+ * Copyright (c) 2018 Ronald Tschalär
+ */
+
+#ifndef __LINUX_APPLE_IBRDIGE_H
+#define __LINUX_APPLE_IBRDIGE_H
+
+#include <linux/hid.h>
+
+#define USB_VENDOR_ID_LINUX_FOUNDATION 0x1d6b
+#define USB_DEVICE_ID_IBRIDGE_TB 0x0301
+#define USB_DEVICE_ID_IBRIDGE_ALS 0x0302
+
+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.21.0

Subject: [PATCH v2 3/3] iio: light: apple-ib-als: Add driver for ALS on iBridge chip.

On 2016/2017 MacBook Pro's with a Touch Bar the ALS is attached to,
and exposed via the iBridge device. This provides the driver for that
sensor.

Signed-off-by: Ronald Tschalär <[email protected]>
---
drivers/iio/light/Kconfig | 12 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/apple-ib-als.c | 607 +++++++++++++++++++++++++++++++
3 files changed, 620 insertions(+)
create mode 100644 drivers/iio/light/apple-ib-als.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 5190eacfeb0a..b477aa5d2024 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -64,6 +64,18 @@ config APDS9960
To compile this driver as a module, choose M here: the
module will be called apds9960

+config APPLE_IBRIDGE_ALS
+ tristate "Apple iBridge ambient light sensor"
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ depends on HID_APPLE_IBRIDGE
+ help
+ Say Y here to build the driver for the Apple iBridge ALS
+ sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called apple-ib-als.
+
config BH1750
tristate "ROHM BH1750 ambient light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index e40794fbb435..cd6cd5ba6da5 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_APDS9960) += apds9960.o
+obj-$(CONFIG_APPLE_IBRIDGE_ALS) += apple-ib-als.o
obj-$(CONFIG_BH1750) += bh1750.o
obj-$(CONFIG_BH1780) += bh1780.o
obj-$(CONFIG_CM32181) += cm32181.o
diff --git a/drivers/iio/light/apple-ib-als.c b/drivers/iio/light/apple-ib-als.c
new file mode 100644
index 000000000000..b84be0076e0f
--- /dev/null
+++ b/drivers/iio/light/apple-ib-als.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Ambient Light Sensor Driver
+ *
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ */
+
+/*
+ * MacBookPro models with an iBridge chip (13,[23] and 14,[23]) have an
+ * ambient light sensor that is exposed via one of the USB interfaces on
+ * the iBridge as a standard HID light sensor. However, we cannot use the
+ * existing hid-sensor-als driver, for two reasons:
+ *
+ * 1. The hid-sensor-als driver is part of the hid-sensor-hub which in turn
+ * is a hid driver, but you can't have more than one hid driver per hid
+ * device, which is a problem because the touch bar also needs to
+ * register as a driver for this hid device.
+ *
+ * 2. While the hid-sensors-als driver stores sensor readings received via
+ * interrupt in an iio buffer, reads on the sysfs
+ * .../iio:deviceX/in_illuminance_YYY attribute result in a get of the
+ * feature report; however, in the case of this sensor here the
+ * illuminance field of that report is always 0. Instead, the input
+ * report needs to be requested.
+ */
+
+#define dev_fmt(fmt) "als: " fmt
+
+#include <linux/apple-ibridge.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/hid-sensor-ids.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#define APPLEALS_DYN_SENS 0 /* our dynamic sensitivity */
+#define APPLEALS_DEF_CHANGE_SENS APPLEALS_DYN_SENS
+
+struct appleals_device {
+ struct hid_device *hid_dev;
+ struct hid_report *cfg_report;
+ struct hid_field *illum_field;
+ struct iio_dev *iio_dev;
+ int cur_sensitivity;
+ int cur_hysteresis;
+ bool events_enabled;
+};
+
+static struct hid_driver appleals_hid_driver;
+
+/*
+ * This is a primitive way to get a relative sensitivity, one where we get
+ * notified when the value changes by a certain percentage rather than some
+ * absolute value. MacOS somehow manages to configure the sensor to work this
+ * way (with a 15% relative sensitivity), but I haven't been able to figure
+ * out how so far. So until we do, this provides a less-than-perfect
+ * simulation.
+ *
+ * When the brightness value is within one of the ranges, the sensitivity is
+ * set to that range's sensitivity. But in order to reduce flapping when the
+ * brightness is right on the border between two ranges, the ranges overlap
+ * somewhat (by at least one sensitivity), and sensitivity is only changed if
+ * the value leaves the current sensitivity's range.
+ *
+ * The values chosen for the map are somewhat arbitrary: a compromise of not
+ * too many ranges (and hence changing the sensitivity) but not too small or
+ * large of a percentage of the min and max values in the range (currently
+ * from 7.5% to 30%, i.e. within a factor of 2 of 15%), as well as just plain
+ * "this feels reasonable to me".
+ */
+struct appleals_sensitivity_map {
+ int sensitivity;
+ int illum_low;
+ int illum_high;
+};
+
+static const struct appleals_sensitivity_map appleals_sensitivity_map[] = {
+ { 1, 0, 14 },
+ { 3, 10, 40 },
+ { 9, 30, 120 },
+ { 27, 90, 360 },
+ { 81, 270, 1080 },
+ { 243, 810, 3240 },
+ { 729, 2430, 9720 },
+};
+
+static int appleals_compute_sensitivity(int cur_illum, int cur_sens)
+{
+ const struct appleals_sensitivity_map *entry;
+ int i;
+
+ /* see if we're still in current range */
+ for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
+ entry = &appleals_sensitivity_map[i];
+
+ if (entry->sensitivity == cur_sens &&
+ entry->illum_low <= cur_illum &&
+ entry->illum_high >= cur_illum)
+ return cur_sens;
+ else if (entry->sensitivity > cur_sens)
+ break;
+ }
+
+ /* not in current range, so find new sensitivity */
+ for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
+ entry = &appleals_sensitivity_map[i];
+
+ if (entry->illum_low <= cur_illum &&
+ entry->illum_high >= cur_illum)
+ return entry->sensitivity;
+ }
+
+ /* hmm, not in table, so assume we are above highest range */
+ i = ARRAY_SIZE(appleals_sensitivity_map) - 1;
+ return appleals_sensitivity_map[i].sensitivity;
+}
+
+static int appleals_get_field_value_for_usage(struct hid_field *field,
+ unsigned int usage)
+{
+ int u;
+
+ if (!field)
+ return -1;
+
+ for (u = 0; u < field->maxusage; u++) {
+ if (field->usage[u].hid == usage)
+ return u + field->logical_minimum;
+ }
+
+ return -1;
+}
+
+static __s32 appleals_get_field_value(struct appleals_device *als_dev,
+ struct hid_field *field)
+{
+ bool powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
+
+ hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT);
+ hid_hw_wait(als_dev->hid_dev);
+
+ if (powered_on)
+ hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
+
+ return field->value[0];
+}
+
+static void appleals_set_field_value(struct appleals_device *als_dev,
+ struct hid_field *field, __s32 value)
+{
+ hid_set_field(field, 0, value);
+ hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_SET_REPORT);
+}
+
+static int appleals_get_config(struct appleals_device *als_dev,
+ unsigned int field_usage, __s32 *value)
+{
+ struct hid_field *field;
+
+ field = appleib_find_report_field(als_dev->cfg_report, field_usage);
+ if (!field)
+ return -EINVAL;
+
+ *value = appleals_get_field_value(als_dev, field);
+
+ return 0;
+}
+
+static int appleals_set_config(struct appleals_device *als_dev,
+ unsigned int field_usage, __s32 value)
+{
+ struct hid_field *field;
+
+ field = appleib_find_report_field(als_dev->cfg_report, field_usage);
+ if (!field)
+ return -EINVAL;
+
+ appleals_set_field_value(als_dev, field, value);
+
+ return 0;
+}
+
+static int appleals_set_enum_config(struct appleals_device *als_dev,
+ unsigned int field_usage,
+ unsigned int value_usage)
+{
+ struct hid_field *field;
+ int value;
+
+ field = appleib_find_report_field(als_dev->cfg_report, field_usage);
+ if (!field)
+ return -EINVAL;
+
+ value = appleals_get_field_value_for_usage(field, value_usage);
+ if (value >= 0)
+ appleals_set_field_value(als_dev, field, value);
+
+ return 0;
+}
+
+static void appleals_update_dyn_sensitivity(struct appleals_device *als_dev,
+ __s32 value)
+{
+ int new_sens;
+ int rc;
+
+ new_sens = appleals_compute_sensitivity(value,
+ als_dev->cur_sensitivity);
+ if (new_sens != als_dev->cur_sensitivity) {
+ rc = appleals_set_config(als_dev,
+ HID_USAGE_SENSOR_LIGHT_ILLUM |
+ HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
+ new_sens);
+ if (!rc)
+ als_dev->cur_sensitivity = new_sens;
+ }
+}
+
+static void appleals_push_new_value(struct appleals_device *als_dev,
+ __s32 value)
+{
+ __s32 buf[2] = { value, value };
+
+ iio_push_to_buffers(als_dev->iio_dev, buf);
+
+ if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS)
+ appleals_update_dyn_sensitivity(als_dev, value);
+}
+
+static int appleals_hid_event(struct hid_device *hdev, struct hid_field *field,
+ struct hid_usage *usage, __s32 value)
+{
+ struct appleals_device *als_dev = hid_get_drvdata(hdev);
+ int rc = 0;
+
+ if ((usage->hid & HID_USAGE_PAGE) != HID_UP_SENSOR)
+ return 0;
+
+ if (usage->hid == HID_USAGE_SENSOR_LIGHT_ILLUM) {
+ appleals_push_new_value(als_dev, value);
+ rc = 1;
+ }
+
+ return rc;
+}
+
+static int appleals_enable_events(struct iio_trigger *trig, bool enable)
+{
+ struct appleals_device *als_dev = iio_trigger_get_drvdata(trig);
+ int value;
+
+ appleals_set_enum_config(als_dev, HID_USAGE_SENSOR_PROP_REPORT_STATE,
+ enable ? HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
+ HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
+ als_dev->events_enabled = enable;
+
+ /* if the sensor was enabled, push an initial value */
+ if (enable) {
+ value = appleals_get_field_value(als_dev, als_dev->illum_field);
+ appleals_push_new_value(als_dev, value);
+ }
+
+ return 0;
+}
+
+static int appleals_read_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct appleals_device **priv = iio_priv(iio_dev);
+ struct appleals_device *als_dev = *priv;
+ __s32 value;
+ int rc;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ *val = appleals_get_field_value(als_dev, als_dev->illum_field);
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ rc = appleals_get_config(als_dev,
+ HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
+ &value);
+ if (rc)
+ return rc;
+
+ /* interval is in ms; val is in HZ, val2 in µHZ */
+ value = 1000000000 / value;
+ *val = value / 1000000;
+ *val2 = value - (*val * 1000000);
+
+ return IIO_VAL_INT_PLUS_MICRO;
+
+ case IIO_CHAN_INFO_HYSTERESIS:
+ if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS) {
+ *val = als_dev->cur_hysteresis;
+ return IIO_VAL_INT;
+ }
+
+ rc = appleals_get_config(als_dev,
+ HID_USAGE_SENSOR_LIGHT_ILLUM |
+ HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
+ val);
+ if (!rc) {
+ als_dev->cur_sensitivity = *val;
+ als_dev->cur_hysteresis = *val;
+ }
+ return rc ? rc : IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int appleals_write_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct appleals_device **priv = iio_priv(iio_dev);
+ struct appleals_device *als_dev = *priv;
+ __s32 illum;
+ int rc;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ rc = appleals_set_config(als_dev,
+ HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
+ 1000000000 / (val * 1000000 + val2));
+ return rc;
+
+ case IIO_CHAN_INFO_HYSTERESIS:
+ if (val == APPLEALS_DYN_SENS) {
+ if (als_dev->cur_hysteresis != APPLEALS_DYN_SENS) {
+ als_dev->cur_hysteresis = val;
+ illum = appleals_get_field_value(als_dev,
+ als_dev->illum_field);
+ appleals_update_dyn_sensitivity(als_dev, illum);
+ }
+
+ return 0;
+ }
+
+ rc = appleals_set_config(als_dev,
+ HID_USAGE_SENSOR_LIGHT_ILLUM |
+ HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
+ val);
+ if (!rc) {
+ als_dev->cur_sensitivity = val;
+ als_dev->cur_hysteresis = val;
+ }
+
+ return rc;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_chan_spec appleals_channels[] = {
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_BOTH,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+ BIT(IIO_CHAN_INFO_HYSTERESIS),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 32,
+ .storagebits = 32,
+ },
+ .scan_index = 0,
+ },
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+ BIT(IIO_CHAN_INFO_HYSTERESIS),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 32,
+ .storagebits = 32,
+ },
+ .scan_index = 1,
+ }
+};
+
+static const struct iio_trigger_ops appleals_trigger_ops = {
+ .set_trigger_state = &appleals_enable_events,
+};
+
+static const struct iio_info appleals_info = {
+ .read_raw = &appleals_read_raw,
+ .write_raw = &appleals_write_raw,
+};
+
+static void appleals_config_sensor(struct appleals_device *als_dev,
+ bool events_enabled, int sensitivity)
+{
+ struct hid_field *field;
+ bool powered_on;
+ __s32 val;
+
+ powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
+
+ hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
+ HID_REQ_GET_REPORT);
+ hid_hw_wait(als_dev->hid_dev);
+
+ field = appleib_find_report_field(als_dev->cfg_report,
+ HID_USAGE_SENSOR_PROY_POWER_STATE);
+ val = appleals_get_field_value_for_usage(field,
+ HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM);
+ if (val >= 0)
+ hid_set_field(field, 0, val);
+
+ field = appleib_find_report_field(als_dev->cfg_report,
+ HID_USAGE_SENSOR_PROP_REPORT_STATE);
+ val = appleals_get_field_value_for_usage(field,
+ events_enabled ?
+ HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
+ HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
+ if (val >= 0)
+ hid_set_field(field, 0, val);
+
+ field = appleib_find_report_field(als_dev->cfg_report,
+ HID_USAGE_SENSOR_PROP_REPORT_INTERVAL);
+ hid_set_field(field, 0, field->logical_minimum);
+
+ /*
+ * Set initial change sensitivity; if dynamic, enabling trigger will set
+ * it instead.
+ */
+ if (sensitivity != APPLEALS_DYN_SENS) {
+ field = appleib_find_report_field(als_dev->cfg_report,
+ HID_USAGE_SENSOR_LIGHT_ILLUM |
+ HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS);
+
+ hid_set_field(field, 0, sensitivity);
+ }
+
+ hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
+ HID_REQ_SET_REPORT);
+
+ if (powered_on)
+ hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
+}
+
+static int appleals_config_iio(struct appleals_device *als_dev)
+{
+ struct iio_dev *iio_dev;
+ struct iio_trigger *iio_trig;
+ struct appleals_device **priv;
+ struct device *parent = &als_dev->hid_dev->dev;
+ int rc;
+
+ iio_dev = devm_iio_device_alloc(parent, sizeof(als_dev));
+ if (!iio_dev)
+ return -ENOMEM;
+
+ priv = iio_priv(iio_dev);
+ *priv = als_dev;
+
+ iio_dev->channels = appleals_channels;
+ iio_dev->num_channels = ARRAY_SIZE(appleals_channels);
+ iio_dev->dev.parent = parent;
+ iio_dev->info = &appleals_info;
+ iio_dev->name = "als";
+ iio_dev->modes = INDIO_DIRECT_MODE;
+
+ rc = devm_iio_triggered_buffer_setup(parent, iio_dev,
+ &iio_pollfunc_store_time, NULL,
+ NULL);
+ if (rc) {
+ hid_err(als_dev->hid_dev,
+ "Failed to set up iio triggered buffer: %d\n", rc);
+ return rc;
+ }
+
+ iio_trig = devm_iio_trigger_alloc(parent, "%s-dev%d", iio_dev->name,
+ iio_dev->id);
+ if (!iio_trig)
+ return -ENOMEM;
+
+ iio_trig->dev.parent = parent;
+ iio_trig->ops = &appleals_trigger_ops;
+ iio_trigger_set_drvdata(iio_trig, als_dev);
+
+ rc = devm_iio_trigger_register(parent, iio_trig);
+ if (rc) {
+ hid_err(als_dev->hid_dev,
+ "Failed to register iio trigger: %d\n",
+ rc);
+ return rc;
+ }
+
+ rc = devm_iio_device_register(parent, iio_dev);
+ if (rc) {
+ hid_err(als_dev->hid_dev, "Failed to register iio device: %d\n",
+ rc);
+ return rc;
+ }
+
+ als_dev->iio_dev = iio_dev;
+
+ return 0;
+}
+
+static int appleals_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct appleals_device *als_dev;
+ struct hid_field *state_field;
+ struct hid_field *illum_field;
+ int rc;
+
+ /* find als fields and reports */
+ rc = hid_parse(hdev);
+ if (rc) {
+ hid_err(hdev, "als: hid parse failed (%d)\n", rc);
+ return rc;
+ }
+
+ state_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
+ HID_USAGE_SENSOR_PROP_REPORT_STATE);
+ illum_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
+ HID_USAGE_SENSOR_LIGHT_ILLUM);
+ if (!state_field || !illum_field)
+ return -ENODEV;
+
+ hid_dbg(hdev, "Found ambient light sensor\n");
+
+ /* initialize device */
+ als_dev = devm_kzalloc(&hdev->dev, sizeof(*als_dev), GFP_KERNEL);
+ if (!als_dev)
+ return -ENOMEM;
+
+ als_dev->hid_dev = hdev;
+ als_dev->cfg_report = state_field->report;
+ als_dev->illum_field = illum_field;
+
+ als_dev->cur_hysteresis = APPLEALS_DEF_CHANGE_SENS;
+ als_dev->cur_sensitivity = APPLEALS_DEF_CHANGE_SENS;
+
+ hid_set_drvdata(hdev, als_dev);
+
+ rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+ if (rc) {
+ hid_err(hdev, "als: hw start failed (%d)\n", rc);
+ return rc;
+ }
+
+ hid_device_io_start(hdev);
+ appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity);
+ hid_device_io_stop(hdev);
+
+ rc = appleals_config_iio(als_dev);
+ if (rc)
+ return rc;
+
+ return hid_hw_open(hdev);
+}
+
+#ifdef CONFIG_PM
+static int appleals_reset_resume(struct hid_device *hdev)
+{
+ struct appleals_device *als_dev = hid_get_drvdata(hdev);
+ __s32 illum;
+
+ appleals_config_sensor(als_dev, als_dev->events_enabled,
+ als_dev->cur_sensitivity);
+
+ illum = appleals_get_field_value(als_dev, als_dev->illum_field);
+ appleals_push_new_value(als_dev, illum);
+
+ return 0;
+}
+#endif
+
+static const struct hid_device_id appleals_hid_ids[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
+ USB_DEVICE_ID_IBRIDGE_ALS) },
+ { },
+};
+
+MODULE_DEVICE_TABLE(hid, appleals_hid_ids);
+
+static struct hid_driver appleals_hid_driver = {
+ .name = "apple-ib-als",
+ .id_table = appleals_hid_ids,
+ .probe = appleals_probe,
+ .event = appleals_hid_event,
+#ifdef CONFIG_PM
+ .reset_resume = appleals_reset_resume,
+#endif
+};
+
+module_hid_driver(appleals_hid_driver);
+
+MODULE_AUTHOR("Ronald Tschalär");
+MODULE_DESCRIPTION("Apple iBridge ALS driver");
+MODULE_LICENSE("GPL v2");
--
2.21.0

2019-06-16 14:01:45

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] HID: apple-ibridge: Add Apple iBridge HID driver.

On Wed, 12 Jun 2019 01:33:58 -0700
Ronald Tschalär <[email protected]> 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 creates virtual HID devices, one per real HID device and
> sub-driver pair (for a total of 4 virtual HID devices). The sub-drivers
> then bind to these virtual HID devices.
>
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices; e.g. in particular the Touch Bar
> driver is aware of the fact that it is dealing with two HID devices that
> need to managed differently.
>
> Signed-off-by: Ronald Tschalär <[email protected]>
Hi Ronald,

I'm far from a hid expert and was only reading this for background on
the ALS sensor driver... Anyhow, there are some comments in here
that needs some follow up or formatting into kernel comment style.

Nitpicks inline!

Jonathan
> ---
> drivers/hid/Kconfig | 14 +
> drivers/hid/Makefile | 1 +
> drivers/hid/apple-ibridge.c | 585 ++++++++++++++++++++++++++++++++++
> drivers/hid/hid-ids.h | 1 +
> include/linux/apple-ibridge.h | 23 ++
> 5 files changed, 624 insertions(+)
> create mode 100644 drivers/hid/apple-ibridge.c
> create mode 100644 include/linux/apple-ibridge.h
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4ca0cdfa6b33..545d3691fc1c 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -135,6 +135,20 @@ config HID_APPLE
> Say Y here if you want support for keyboards of Apple iBooks, PowerBooks,
> MacBooks, MacBook Pros and Apple Aluminum.
>
> +config HID_APPLE_IBRIDGE
> + tristate "Apple iBridge"
> + depends on ACPI
> + depends on USB_HID
> + depends on X86 || COMPILE_TEST
> + help
> + This module provides the core support for the Apple T1 chip found
> + on recent MacBookPro's, also known as the iBridge. 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.
> +
> config HID_APPLEIR
> tristate "Apple infrared receiver"
> depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 170163b41303..a4da5663a541 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += hid-accutouch.o
> obj-$(CONFIG_HID_ALPS) += hid-alps.o
> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> +obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
> obj-$(CONFIG_HID_ASUS) += hid-asus.o
> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
> diff --git a/drivers/hid/apple-ibridge.c b/drivers/hid/apple-ibridge.c
> new file mode 100644
> index 000000000000..565f080a38d6
> --- /dev/null
> +++ b/drivers/hid/apple-ibridge.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +/**
> + * DOC: Overview
> + *
> + * 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. 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 a HID driver that creates virtual HID devices
> + * for the ALS and touch bar functionality, and the ALS and touch bar drivers
> + * are HID drivers which in turn attach to these virtual HID devices. This
> + * driver then relays the calls on the real HID devices to the virtual ones,
> + * and visa versa.
> + *
> + * Lastly, this driver also takes care of the power-management for the
> + * iBridge when suspending and resuming.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"
> +
> +#define APPLEIB_BASIC_CONFIG 1
> +
> +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev)
> +
> +static struct hid_device_id appleib_sub_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_TB) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +struct appleib_device {
> + struct acpi_device *acpi_dev;
> + acpi_handle asoc_socw;
> +};
> +
> +struct appleib_hid_dev_info {
> + struct hid_device *hdev;
> + struct hid_device *sub_hdevs[ARRAY_SIZE(appleib_sub_hid_ids)];
> +};
> +
> +static void appleib_remove_device(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> +
> + hid_set_drvdata(hdev, NULL);
> +}
> +
> +/**
> + * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
> + * all virtual HID devices attached to the given real HID device.
> + * @hdev the real hid-device
> + * @forward a function that calls the callback on the given driver
> + * @args arguments for the forward function
> + *
> + * This is for callbacks that return a status as an int.
> + *
> + * Returns: 0 on success, or the first error returned by the @forward function.
> + */
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct hid_driver *,
> + struct hid_device *, void *),
> + void *args)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + struct hid_device *sub_hdev;
> + int rc = 0;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + sub_hdev = hdev_info->sub_hdevs[i];
> + if (sub_hdev->driver) {
> + rc = forward(sub_hdev->driver, sub_hdev, args);
> + if (rc)
> + break;
> + }
> +
> + break;
> + }
> +
> + return rc;
> +}
> +
> +static int appleib_hid_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *data, int size)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
> + hid_input_report(hdev_info->sub_hdevs[i], report->type, data,
> + size, 0);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleib_hid_suspend_fwd(struct hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->suspend)
> + rc = drv->suspend(hdev, *(pm_message_t *)args);
> +
> + return rc;
> +}
> +
> +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 hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->resume)
> + rc = drv->resume(hdev);
> +
> + return rc;
> +}
> +
> +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 hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->reset_resume)
> + rc = drv->reset_resume(hdev);
> +
> + return rc;
> +}
> +
> +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 */
> +
> +/**
> + * appleib_find_report_field() - 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
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +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);
> +
> +/**
> + * appleib_find_hid_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
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +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);
> +
> +static int appleib_ll_start(struct hid_device *hdev)
> +{
> + return 0;
> +}
> +
> +static void appleib_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int appleib_ll_open(struct hid_device *hdev)
> +{
> + // TODO: allow event reporting
Comment syntax :)

> + return 0;
> +}
> +
> +static void appleib_ll_close(struct hid_device *hdev)
> +{
> + // TODO: disallow event reporting
> +}
> +
> +static int appleib_ll_power(struct hid_device *hdev, int level)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_power(hdev_info->hdev, level);
> +}
> +
> +static int appleib_ll_parse(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_parse_report(hdev, hdev_info->hdev->rdesc,
> + hdev_info->hdev->rsize);
> +}
> +
> +static void appleib_ll_request(struct hid_device *hdev,
> + struct hid_report *report, int reqtype)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + hid_hw_request(hdev_info->hdev, report, reqtype);
> +}
> +
> +static int appleib_ll_wait(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + hid_hw_wait(hdev_info->hdev);
> + return 0;
> +}
> +
> +static int appleib_ll_raw_request(struct hid_device *hdev,
> + unsigned char reportnum, __u8 *buf,
> + size_t len, unsigned char rtype, int reqtype)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
> + reqtype);
> +}
> +
> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
> + size_t len)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_output_report(hdev_info->hdev, buf, len);
> +}
> +
> +static struct hid_ll_driver appleib_ll_driver = {
> + .start = appleib_ll_start,
> + .stop = appleib_ll_stop,
> + .open = appleib_ll_open,
> + .close = appleib_ll_close,
> + .power = appleib_ll_power,
> + .parse = appleib_ll_parse,
> + .request = appleib_ll_request,
> + .wait = appleib_ll_wait,
> + .raw_request = appleib_ll_raw_request,
> + .output_report = appleib_ll_output_report,
> +};
> +
> +static struct hid_device *
> +appleib_add_sub_dev(struct appleib_hid_dev_info *hdev_info,
> + struct hid_device_id *dev_id)
> +{
> + struct hid_device *sub_hdev;
> + int rc;
> +
> + sub_hdev = hid_allocate_device();
> + if (IS_ERR(sub_hdev))
> + return sub_hdev;
> +
> + sub_hdev->dev.parent = &hdev_info->hdev->dev;
> +
> + sub_hdev->bus = dev_id->bus;
> + sub_hdev->group = dev_id->group;
> + sub_hdev->vendor = dev_id->vendor;
> + sub_hdev->product = dev_id->product;
> +
> + sub_hdev->ll_driver = &appleib_ll_driver;
> +
> + snprintf(sub_hdev->name, sizeof(sub_hdev->name),
> + "iBridge Virtual HID %s/%04x:%04x",
> + dev_name(sub_hdev->dev.parent), sub_hdev->vendor,
> + sub_hdev->product);
> +
> + sub_hdev->driver_data = hdev_info;
> +
> + rc = hid_add_device(sub_hdev);
> + if (rc) {
> + hid_destroy_device(sub_hdev);
> + return ERR_PTR(rc);
> + }
> +
> + return sub_hdev;
> +}
> +
> +static struct appleib_hid_dev_info *appleib_add_device(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info;
> + int i;
> +
> + hdev_info = devm_kzalloc(&hdev->dev, sizeof(*hdev_info), GFP_KERNEL);
> + if (!hdev_info)
> + return ERR_PTR(-ENOMEM);
> +
> + hdev_info->hdev = hdev;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + hdev_info->sub_hdevs[i] =
> + appleib_add_sub_dev(hdev_info, &appleib_sub_hid_ids[i]);
> +
> + if (IS_ERR(hdev_info->sub_hdevs[i])) {
> + while (i-- > 0)
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> + return (void *)hdev_info->sub_hdevs[i];
> + }
> + }
> +
> + return hdev_info;
> +}
> +
> +static int appleib_hid_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_hid_dev_info *hdev_info;
> + struct usb_device *udev;
> + int rc;
> +
> + /* check and set usb config first */
> + udev = hid_to_usb_dev(hdev);
> +
> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
> + return rc ? rc : -ENODEV;
> + }
> +
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> + if (rc) {
> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> + goto error;
> + }
> +
> + hdev_info = appleib_add_device(hdev);
> + if (IS_ERR(hdev_info)) {
> + rc = PTR_ERR(hdev_info);
> + goto stop_hw;
> + }
> +
> + hid_set_drvdata(hdev, hdev_info);
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> + goto remove_dev;
> + }
> +
> + return 0;
> +
> +remove_dev:
> + appleib_remove_device(hdev);
> +stop_hw:
> + hid_hw_stop(hdev);
> +error:
> + return rc;
> +}
> +
> +static void appleib_hid_remove(struct hid_device *hdev)
> +{
> + hid_hw_close(hdev);
> + appleib_remove_device(hdev);
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appleib_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> + { },
> +};
> +
> +static struct hid_driver appleib_hid_driver = {
> + .name = "apple-ibridge-hid",
> + .id_table = appleib_hid_ids,
> + .probe = appleib_hid_probe,
> + .remove = appleib_hid_remove,
> + .raw_event = appleib_hid_raw_event,
> +#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;
> +
> + ib_dev = devm_kzalloc(&acpi_dev->dev, sizeof(*ib_dev), GFP_KERNEL);
> + if (!ib_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + ib_dev->acpi_dev = acpi_dev;
> +
> + /* get iBridge acpi power control method for suspend/resume */
> + 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));
> + return ERR_PTR(-ENXIO);
> + }
> +
> + /* 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;
> +}
> +
> +static int appleib_probe(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev;
> + int ret;
> +
> + ib_dev = appleib_alloc_device(acpi);
> + if (IS_ERR(ib_dev))
> + return PTR_ERR(ib_dev);
> +
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> + ret);
> + return ret;
> + }
> +
> + acpi->driver_data = ib_dev;
> +
> + return 0;
> +}
> +
> +static int appleib_remove(struct acpi_device *acpi)
> +{
> + hid_unregister_driver(&appleib_hid_driver);
> +
> + return 0;
> +}
> +
> +static int appleib_suspend(struct device *dev)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + ib_dev = acpi_driver_data(to_acpi_device(dev));
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(dev, "SOCW(0) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static int appleib_resume(struct device *dev)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + ib_dev = acpi_driver_data(to_acpi_device(dev));
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(rc))
> + dev_warn(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", /* ? */

It's always nice to have a ? but what is the question?

> + .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/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index adce58f24f76..f963bb02c477 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -177,6 +177,7 @@
> #define USB_DEVICE_ID_APPLE_IRCONTROL3 0x8241
> #define USB_DEVICE_ID_APPLE_IRCONTROL4 0x8242
> #define USB_DEVICE_ID_APPLE_IRCONTROL5 0x8243
> +#define USB_DEVICE_ID_APPLE_IBRIDGE 0x8600
>
> #define USB_VENDOR_ID_ASUS 0x0486
> #define USB_DEVICE_ID_ASUS_T91MT 0x0185
> diff --git a/include/linux/apple-ibridge.h b/include/linux/apple-ibridge.h
> new file mode 100644
> index 000000000000..07ded8c68776
> --- /dev/null
> +++ b/include/linux/apple-ibridge.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +#ifndef __LINUX_APPLE_IBRDIGE_H
> +#define __LINUX_APPLE_IBRDIGE_H
> +
> +#include <linux/hid.h>
> +
> +#define USB_VENDOR_ID_LINUX_FOUNDATION 0x1d6b
> +#define USB_DEVICE_ID_IBRIDGE_TB 0x0301
> +#define USB_DEVICE_ID_IBRIDGE_ALS 0x0302
> +
> +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

2019-06-16 14:52:39

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iio: light: apple-ib-als: Add driver for ALS on iBridge chip.

On Wed, 12 Jun 2019 01:34:00 -0700
Ronald Tschalär <[email protected]> wrote:

> On 2016/2017 MacBook Pro's with a Touch Bar the ALS is attached to,
> and exposed via the iBridge device. This provides the driver for that
> sensor.
>
> Signed-off-by: Ronald Tschalär <[email protected]>
Hi Ronald,

One thing that we should perhaps document more clearly in IIO is that
it is acceptable to not have triggers if they don't make any sense.
In this particular case, you have one basically to give a way of saying
to move into a more continuous sampling mode from a polled one (I think).
For that just use the buffer enable callbacks.

It'll be much cleaner without the trigger.

A few other suggestions inline. In particularly I'm not that keen on the
appleals_device having a pointer to the iio device which then has
a pointer back again. I 'think' you can just reorder things a bit and
embed the appleals_device structure in the iio_dev private field directly
and avoid the dance between the different structures.

Thanks,

Jonathan

> ---
> drivers/iio/light/Kconfig | 12 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/apple-ib-als.c | 607 +++++++++++++++++++++++++++++++
> 3 files changed, 620 insertions(+)
> create mode 100644 drivers/iio/light/apple-ib-als.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5190eacfeb0a..b477aa5d2024 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -64,6 +64,18 @@ config APDS9960
> To compile this driver as a module, choose M here: the
> module will be called apds9960
>
> +config APPLE_IBRIDGE_ALS
> + tristate "Apple iBridge ambient light sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on HID_APPLE_IBRIDGE
> + help
> + Say Y here to build the driver for the Apple iBridge ALS
> + sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ib-als.
> +
> config BH1750
> tristate "ROHM BH1750 ambient light sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index e40794fbb435..cd6cd5ba6da5 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> +obj-$(CONFIG_APPLE_IBRIDGE_ALS) += apple-ib-als.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> obj-$(CONFIG_CM32181) += cm32181.o
> diff --git a/drivers/iio/light/apple-ib-als.c b/drivers/iio/light/apple-ib-als.c
> new file mode 100644
> index 000000000000..b84be0076e0f
> --- /dev/null
> +++ b/drivers/iio/light/apple-ib-als.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Ambient Light Sensor Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + */
> +
> +/*
> + * MacBookPro models with an iBridge chip (13,[23] and 14,[23]) have an
> + * ambient light sensor that is exposed via one of the USB interfaces on
> + * the iBridge as a standard HID light sensor. However, we cannot use the
> + * existing hid-sensor-als driver, for two reasons:
> + *
> + * 1. The hid-sensor-als driver is part of the hid-sensor-hub which in turn
> + * is a hid driver, but you can't have more than one hid driver per hid
> + * device, which is a problem because the touch bar also needs to
> + * register as a driver for this hid device.
> + *
> + * 2. While the hid-sensors-als driver stores sensor readings received via
> + * interrupt in an iio buffer, reads on the sysfs
> + * .../iio:deviceX/in_illuminance_YYY attribute result in a get of the
> + * feature report; however, in the case of this sensor here the
> + * illuminance field of that report is always 0. Instead, the input
> + * report needs to be requested.
> + */
> +
> +#define dev_fmt(fmt) "als: " fmt
> +
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/hid-sensor-ids.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#define APPLEALS_DYN_SENS 0 /* our dynamic sensitivity */
> +#define APPLEALS_DEF_CHANGE_SENS APPLEALS_DYN_SENS
> +
> +struct appleals_device {
> + struct hid_device *hid_dev;
> + struct hid_report *cfg_report;
> + struct hid_field *illum_field;
> + struct iio_dev *iio_dev;
> + int cur_sensitivity;
> + int cur_hysteresis;
> + bool events_enabled;
> +};
> +
> +static struct hid_driver appleals_hid_driver;
> +
> +/*
> + * This is a primitive way to get a relative sensitivity, one where we get
> + * notified when the value changes by a certain percentage rather than some
> + * absolute value. MacOS somehow manages to configure the sensor to work this
> + * way (with a 15% relative sensitivity), but I haven't been able to figure
> + * out how so far. So until we do, this provides a less-than-perfect
> + * simulation.
> + *
> + * When the brightness value is within one of the ranges, the sensitivity is
> + * set to that range's sensitivity. But in order to reduce flapping when the
> + * brightness is right on the border between two ranges, the ranges overlap
> + * somewhat (by at least one sensitivity), and sensitivity is only changed if
> + * the value leaves the current sensitivity's range.
> + *
> + * The values chosen for the map are somewhat arbitrary: a compromise of not
> + * too many ranges (and hence changing the sensitivity) but not too small or
> + * large of a percentage of the min and max values in the range (currently
> + * from 7.5% to 30%, i.e. within a factor of 2 of 15%), as well as just plain
> + * "this feels reasonable to me".
> + */
> +struct appleals_sensitivity_map {
> + int sensitivity;
> + int illum_low;
> + int illum_high;
> +};
> +
> +static const struct appleals_sensitivity_map appleals_sensitivity_map[] = {
> + { 1, 0, 14 },
> + { 3, 10, 40 },
> + { 9, 30, 120 },
> + { 27, 90, 360 },
> + { 81, 270, 1080 },
> + { 243, 810, 3240 },
> + { 729, 2430, 9720 },
> +};
> +
> +static int appleals_compute_sensitivity(int cur_illum, int cur_sens)
> +{
> + const struct appleals_sensitivity_map *entry;
> + int i;
> +
> + /* see if we're still in current range */
> + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> + entry = &appleals_sensitivity_map[i];
> +
> + if (entry->sensitivity == cur_sens &&
> + entry->illum_low <= cur_illum &&
> + entry->illum_high >= cur_illum)
> + return cur_sens;
> + else if (entry->sensitivity > cur_sens)
> + break;
> + }
> +
> + /* not in current range, so find new sensitivity */
> + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> + entry = &appleals_sensitivity_map[i];
> +
> + if (entry->illum_low <= cur_illum &&
> + entry->illum_high >= cur_illum)
> + return entry->sensitivity;
> + }
> +
> + /* hmm, not in table, so assume we are above highest range */
> + i = ARRAY_SIZE(appleals_sensitivity_map) - 1;
> + return appleals_sensitivity_map[i].sensitivity;
> +}
> +
> +static int appleals_get_field_value_for_usage(struct hid_field *field,
> + unsigned int usage)
> +{
> + int u;
> +
> + if (!field)
> + return -1;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == usage)
> + return u + field->logical_minimum;
> + }
> +
> + return -1;
> +}
> +
> +static __s32 appleals_get_field_value(struct appleals_device *als_dev,
> + struct hid_field *field)
> +{
> + bool powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
> +
> + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT);
> + hid_hw_wait(als_dev->hid_dev);
> +
> + if (powered_on)
> + hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
> +
> + return field->value[0];
> +}
> +
> +static void appleals_set_field_value(struct appleals_device *als_dev,
> + struct hid_field *field, __s32 value)
> +{
> + hid_set_field(field, 0, value);
> + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_SET_REPORT);
> +}
> +
> +static int appleals_get_config(struct appleals_device *als_dev,
> + unsigned int field_usage, __s32 *value)
> +{
> + struct hid_field *field;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + *value = appleals_get_field_value(als_dev, field);
> +
> + return 0;
> +}
> +
> +static int appleals_set_config(struct appleals_device *als_dev,
> + unsigned int field_usage, __s32 value)
> +{
> + struct hid_field *field;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + appleals_set_field_value(als_dev, field, value);
> +
> + return 0;
> +}
> +
> +static int appleals_set_enum_config(struct appleals_device *als_dev,
> + unsigned int field_usage,
> + unsigned int value_usage)
> +{
> + struct hid_field *field;
> + int value;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + value = appleals_get_field_value_for_usage(field, value_usage);
> + if (value >= 0)
> + appleals_set_field_value(als_dev, field, value);
> +
> + return 0;
> +}
> +
> +static void appleals_update_dyn_sensitivity(struct appleals_device *als_dev,
> + __s32 value)
> +{
> + int new_sens;
> + int rc;
> +
> + new_sens = appleals_compute_sensitivity(value,
> + als_dev->cur_sensitivity);
> + if (new_sens != als_dev->cur_sensitivity) {
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + new_sens);
> + if (!rc)
> + als_dev->cur_sensitivity = new_sens;
> + }
> +}
> +
> +static void appleals_push_new_value(struct appleals_device *als_dev,
> + __s32 value)
> +{
> + __s32 buf[2] = { value, value };
> +
> + iio_push_to_buffers(als_dev->iio_dev, buf);
> +
> + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS)
> + appleals_update_dyn_sensitivity(als_dev, value);
> +}
> +
> +static int appleals_hid_event(struct hid_device *hdev, struct hid_field *field,
> + struct hid_usage *usage, __s32 value)
> +{
> + struct appleals_device *als_dev = hid_get_drvdata(hdev);
> + int rc = 0;
> +
> + if ((usage->hid & HID_USAGE_PAGE) != HID_UP_SENSOR)
> + return 0;
> +
> + if (usage->hid == HID_USAGE_SENSOR_LIGHT_ILLUM) {
> + appleals_push_new_value(als_dev, value);
> + rc = 1;
Direct return here would be more readable, then return 0 below.
> + }
> +
> + return rc;
> +}
> +
> +static int appleals_enable_events(struct iio_trigger *trig, bool enable)
> +{
> + struct appleals_device *als_dev = iio_trigger_get_drvdata(trig);
> + int value;
> +
> + appleals_set_enum_config(als_dev, HID_USAGE_SENSOR_PROP_REPORT_STATE,
> + enable ? HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> + als_dev->events_enabled = enable;
> +
> + /* if the sensor was enabled, push an initial value */
> + if (enable) {
> + value = appleals_get_field_value(als_dev, als_dev->illum_field);
> + appleals_push_new_value(als_dev, value);
> + }
> +
> + return 0;
> +}
> +
> +static int appleals_read_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct appleals_device **priv = iio_priv(iio_dev);
> + struct appleals_device *als_dev = *priv;
> + __s32 value;
> + int rc;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + *val = appleals_get_field_value(als_dev, als_dev->illum_field);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + rc = appleals_get_config(als_dev,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> + &value);
> + if (rc)
> + return rc;
> +
> + /* interval is in ms; val is in HZ, val2 in µHZ */
> + value = 1000000000 / value;
> + *val = value / 1000000;
> + *val2 = value - (*val * 1000000);
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS) {
> + *val = als_dev->cur_hysteresis;
> + return IIO_VAL_INT;
> + }
> +
> + rc = appleals_get_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + val);
> + if (!rc) {
> + als_dev->cur_sensitivity = *val;
> + als_dev->cur_hysteresis = *val;
> + }
> + return rc ? rc : IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int appleals_write_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct appleals_device **priv = iio_priv(iio_dev);
> + struct appleals_device *als_dev = *priv;
> + __s32 illum;
> + int rc;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> + 1000000000 / (val * 1000000 + val2));
> + return rc;
> +
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (val == APPLEALS_DYN_SENS) {

Hysteresis normally takes a value, this looks like a magic number being
pushed through the interface?

> + if (als_dev->cur_hysteresis != APPLEALS_DYN_SENS) {
> + als_dev->cur_hysteresis = val;
> + illum = appleals_get_field_value(als_dev,
> + als_dev->illum_field);
> + appleals_update_dyn_sensitivity(als_dev, illum);
> + }
> +
> + return 0;
> + }
> +
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + val);
> + if (!rc) {
> + als_dev->cur_sensitivity = val;
> + als_dev->cur_hysteresis = val;
> + }
> +
> + return rc;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_chan_spec appleals_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_BOTH,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

This would be unusual. What are the units of this intensity measurement?
Mostly these are the values prior to a proprietary algorithm and have no
particular assigned units that are documented anywhere. As such for
most light sensors this is raw.

> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + .scan_index = 0,
> + },
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + .scan_index = 1,
> + }
> +};
> +
> +static const struct iio_trigger_ops appleals_trigger_ops = {
> + .set_trigger_state = &appleals_enable_events,
> +};
> +
> +static const struct iio_info appleals_info = {
> + .read_raw = &appleals_read_raw,
> + .write_raw = &appleals_write_raw,
> +};
> +
> +static void appleals_config_sensor(struct appleals_device *als_dev,
> + bool events_enabled, int sensitivity)
> +{
> + struct hid_field *field;
> + bool powered_on;
> + __s32 val;
> +
> + powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
> +
> + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
> + HID_REQ_GET_REPORT);
> + hid_hw_wait(als_dev->hid_dev);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROY_POWER_STATE);
> + val = appleals_get_field_value_for_usage(field,
> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM);
> + if (val >= 0)
> + hid_set_field(field, 0, val);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> + val = appleals_get_field_value_for_usage(field,
> + events_enabled ?
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> + if (val >= 0)
> + hid_set_field(field, 0, val);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL);
> + hid_set_field(field, 0, field->logical_minimum);
> +
> + /*
> + * Set initial change sensitivity; if dynamic, enabling trigger will set
> + * it instead.
> + */
> + if (sensitivity != APPLEALS_DYN_SENS) {
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS);
> +
> + hid_set_field(field, 0, sensitivity);
> + }
> +
> + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
> + HID_REQ_SET_REPORT);
> +
> + if (powered_on)
> + hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
> +}
> +
> +static int appleals_config_iio(struct appleals_device *als_dev)
> +{
> + struct iio_dev *iio_dev;
> + struct iio_trigger *iio_trig;
> + struct appleals_device **priv;
> + struct device *parent = &als_dev->hid_dev->dev;
> + int rc;
> +
> + iio_dev = devm_iio_device_alloc(parent, sizeof(als_dev));
> + if (!iio_dev)
> + return -ENOMEM;

Hmm. So we are using the private space of the iio device to just
hold a pointer...
Normally we try to avoid this if at all possible as we end
up with loops of pointers that normally indicate a someone
convoluted driver structure. I've not checked all the paths
here to be sure if we can just embed the actual als_dev
structure in here and always have the iio_dev available in
any callbacks etc.

> +
> + priv = iio_priv(iio_dev);
> + *priv = als_dev;
> +
> + iio_dev->channels = appleals_channels;
> + iio_dev->num_channels = ARRAY_SIZE(appleals_channels);
> + iio_dev->dev.parent = parent;
> + iio_dev->info = &appleals_info;
> + iio_dev->name = "als";
Whilst I suppose not many people will have additional ALS devices connected
to these laptops, we normally try to give a slightly more specific name
than this.

> + iio_dev->modes = INDIO_DIRECT_MODE;
> +
> + rc = devm_iio_triggered_buffer_setup(parent, iio_dev,
> + &iio_pollfunc_store_time, NULL,
This is unusual. You have registered all the infrastructure for a triggered
buffer, but then didn't actually provide a function to put any data in it.

It is perfectly acceptable to just skip the trigger if it doesn't make sense
for a given device. If you are doing that, then register the buffer directly
not using this helper.

> + NULL);
> + if (rc) {
> + hid_err(als_dev->hid_dev,
> + "Failed to set up iio triggered buffer: %d\n", rc);
> + return rc;
> + }
> +
> + iio_trig = devm_iio_trigger_alloc(parent, "%s-dev%d", iio_dev->name,
> + iio_dev->id);
> + if (!iio_trig)
> + return -ENOMEM;
> +
> + iio_trig->dev.parent = parent;
> + iio_trig->ops = &appleals_trigger_ops;
> + iio_trigger_set_drvdata(iio_trig, als_dev);
> +
> + rc = devm_iio_trigger_register(parent, iio_trig);
> + if (rc) {
> + hid_err(als_dev->hid_dev,
> + "Failed to register iio trigger: %d\n",
> + rc);
> + return rc;
> + }

What is the purpose of this trigger? It doesn't seem to 'do' anything. There
is no function calling iio_trigger_poll* so it's not acting as a trigger.

> +
> + rc = devm_iio_device_register(parent, iio_dev);
> + if (rc) {
> + hid_err(als_dev->hid_dev, "Failed to register iio device: %d\n",
> + rc);
> + return rc;
> + }
> +
> + als_dev->iio_dev = iio_dev;
> +
> + return 0;
> +}
> +
> +static int appleals_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleals_device *als_dev;
> + struct hid_field *state_field;
> + struct hid_field *illum_field;
> + int rc;
> +
> + /* find als fields and reports */
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "als: hid parse failed (%d)\n", rc);
> + return rc;
> + }
> +
> + state_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> + illum_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> + HID_USAGE_SENSOR_LIGHT_ILLUM);
> + if (!state_field || !illum_field)
> + return -ENODEV;
> +
> + hid_dbg(hdev, "Found ambient light sensor\n");
> +
> + /* initialize device */
> + als_dev = devm_kzalloc(&hdev->dev, sizeof(*als_dev), GFP_KERNEL);
> + if (!als_dev)
> + return -ENOMEM;
> +
> + als_dev->hid_dev = hdev;
> + als_dev->cfg_report = state_field->report;
> + als_dev->illum_field = illum_field;
> +
> + als_dev->cur_hysteresis = APPLEALS_DEF_CHANGE_SENS;
> + als_dev->cur_sensitivity = APPLEALS_DEF_CHANGE_SENS;
> +
> + hid_set_drvdata(hdev, als_dev);
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> + if (rc) {
> + hid_err(hdev, "als: hw start failed (%d)\n", rc);
> + return rc;
> + }
> +
> + hid_device_io_start(hdev);
> + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity);
> + hid_device_io_stop(hdev);
> +
> + rc = appleals_config_iio(als_dev);
> + if (rc)
> + return rc;
> +
> + return hid_hw_open(hdev);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleals_reset_resume(struct hid_device *hdev)
> +{
> + struct appleals_device *als_dev = hid_get_drvdata(hdev);
> + __s32 illum;
> +
> + appleals_config_sensor(als_dev, als_dev->events_enabled,
> + als_dev->cur_sensitivity);
> +
> + illum = appleals_get_field_value(als_dev, als_dev->illum_field);
> + appleals_push_new_value(als_dev, illum);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct hid_device_id appleals_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(hid, appleals_hid_ids);
> +
> +static struct hid_driver appleals_hid_driver = {
> + .name = "apple-ib-als",
> + .id_table = appleals_hid_ids,
> + .probe = appleals_probe,
> + .event = appleals_hid_event,
> +#ifdef CONFIG_PM
> + .reset_resume = appleals_reset_resume,
> +#endif
> +};
> +
> +module_hid_driver(appleals_hid_driver);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge ALS driver");
> +MODULE_LICENSE("GPL v2");