| Message ID | 20190422031251.11968-4-ronald@innovation.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple iBridge support | expand |
On Sun, 21 Apr 2019, Ronald Tschalär 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. some comments below inline > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > --- > drivers/iio/light/Kconfig | 12 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/apple-ib-als.c | 694 +++++++++++++++++++++++++++++++ > 3 files changed, 707 insertions(+) > create mode 100644 drivers/iio/light/apple-ib-als.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 36f458433480..49159fab1c0e 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 MFD_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 286bf3975372..144d918917f7 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..1718fcbe304f > --- /dev/null > +++ b/drivers/iio/light/apple-ib-als.c > @@ -0,0 +1,694 @@ > +// 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/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/mfd/apple-ibridge.h> > +#include <linux/module.h> > +#include <linux/platform_device.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 appleib_device *ib_dev; > + struct device *log_dev; > + struct hid_device *hid_dev; > + struct hid_report *cfg_report; > + struct hid_field *illum_field; > + struct iio_dev *iio_dev; > + struct iio_trigger *iio_trig; > + 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 struct appleals_sensitivity_map appleals_sensitivity_map[] = { const? > + { 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) > +{ > + 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) > +{ > + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT); > + hid_hw_wait(als_dev->hid_dev); > + > + 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); can return -1, not checked > + > + 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 = > + appleib_get_drvdata(hid_get_drvdata(hdev), > + &appleals_hid_driver); > + 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; > + > + /* set the sensor's reporting state */ > + 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 *als_dev = > + *(struct appleals_device **)iio_priv(iio_dev); > + __s32 value; > + int rc; > + > + *val = 0; > + *val2 = 0; no need to set these here > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + 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 *als_dev = > + *(struct appleals_device **)iio_priv(iio_dev); > + __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)); > + break; maybe return directly instead of at the end (matter of taste); here and in the other cases below > + > + 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); > + } > + rc = 0; > + break; > + } > + > + 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; > + } > + break; > + > + default: > + rc = -EINVAL; > + } > + > + return rc; > +} > + > +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) | > + BIT(IIO_CHAN_INFO_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) | > + BIT(IIO_CHAN_INFO_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 = 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; > + __s32 val; > + > + /* > + * We're (often) in a probe here, so need to enable input processing > + * in that case, but only in that case. > + */ > + if (appleib_in_hid_probe(als_dev->ib_dev)) > + hid_device_io_start(als_dev->hid_dev); > + > + /* power on the sensor */ > + 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); what if -1? > + hid_set_field(field, 0, val); > + > + /* configure reporting of change events */ > + 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); > + hid_set_field(field, 0, val); > + > + /* report change events asap */ > + 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); > + } > + > + /* write the new config to the sensor */ > + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report, > + HID_REQ_SET_REPORT); > + > + if (appleib_in_hid_probe(als_dev->ib_dev)) > + hid_device_io_stop(als_dev->hid_dev); > +}; no semicolon at the end of a function please > + > +static int appleals_config_iio(struct appleals_device *als_dev) > +{ > + struct iio_dev *iio_dev; > + struct iio_trigger *iio_trig; > + int rc; > + > + /* create and register iio device */ > + iio_dev = iio_device_alloc(sizeof(als_dev)); how about using the devm_ variants? > + if (!iio_dev) > + return -ENOMEM; > + > + *(struct appleals_device **)iio_priv(iio_dev) = als_dev; > + > + iio_dev->channels = appleals_channels; > + iio_dev->num_channels = ARRAY_SIZE(appleals_channels); > + iio_dev->dev.parent = &als_dev->hid_dev->dev; > + iio_dev->info = &appleals_info; > + iio_dev->name = "als"; > + iio_dev->modes = INDIO_DIRECT_MODE; > + > + rc = iio_triggered_buffer_setup(iio_dev, &iio_pollfunc_store_time, NULL, > + NULL); > + if (rc) { > + dev_err(als_dev->log_dev, "failed to set up iio triggers: %d\n", just one trigger? > + rc); > + goto free_iio_dev; > + } > + > + iio_trig = iio_trigger_alloc("%s-dev%d", iio_dev->name, iio_dev->id); > + if (!iio_trig) { > + rc = -ENOMEM; > + goto clean_trig_buf; > + } > + > + iio_trig->dev.parent = &als_dev->hid_dev->dev; > + iio_trig->ops = &appleals_trigger_ops; > + iio_trigger_set_drvdata(iio_trig, als_dev); > + > + rc = iio_trigger_register(iio_trig); > + if (rc) { > + dev_err(als_dev->log_dev, "failed to register iio trigger: %d\n", some messages start lowercase, some uppercase (nitpicking) > + rc); > + goto free_iio_trig; > + } > + > + als_dev->iio_trig = iio_trig; > + > + rc = iio_device_register(iio_dev); > + if (rc) { > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n", > + rc); > + goto unreg_iio_trig; > + } > + > + als_dev->iio_dev = iio_dev; > + > + return 0; > + > +unreg_iio_trig: > + iio_trigger_unregister(iio_trig); > +free_iio_trig: > + iio_trigger_free(iio_trig); > + als_dev->iio_trig = NULL; > +clean_trig_buf: > + iio_triggered_buffer_cleanup(iio_dev); > +free_iio_dev: > + iio_device_free(iio_dev); > + > + return rc; > +} > + > +static int appleals_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct appleals_device *als_dev = > + appleib_get_drvdata(hid_get_drvdata(hdev), > + &appleals_hid_driver); > + struct hid_field *state_field; > + struct hid_field *illum_field; > + int rc; > + > + /* find als fields and reports */ > + 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; > + > + if (als_dev->hid_dev) { > + dev_warn(als_dev->log_dev, > + "Found duplicate ambient light sensor - ignoring\n"); > + return -EBUSY; > + } > + > + dev_info(als_dev->log_dev, "Found ambient light sensor\n"); in general avoid logging for the OK case, it just clutters the log > + > + /* initialize device */ > + 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; > + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity); > + > + rc = appleals_config_iio(als_dev); > + if (rc) > + return rc; > + > + return 0; > +} > + > +static void appleals_remove(struct hid_device *hdev) > +{ > + struct appleals_device *als_dev = > + appleib_get_drvdata(hid_get_drvdata(hdev), > + &appleals_hid_driver); > + could be a lot less if devm_ were used? > + if (als_dev->iio_dev) { > + iio_device_unregister(als_dev->iio_dev); > + > + iio_trigger_unregister(als_dev->iio_trig); > + iio_trigger_free(als_dev->iio_trig); > + als_dev->iio_trig = NULL; > + > + iio_triggered_buffer_cleanup(als_dev->iio_dev); > + iio_device_free(als_dev->iio_dev); > + als_dev->iio_dev = NULL; > + } > + > + als_dev->hid_dev = NULL; > +} > + > +#ifdef CONFIG_PM > +static int appleals_reset_resume(struct hid_device *hdev) > +{ > + struct appleals_device *als_dev = > + appleib_get_drvdata(hid_get_drvdata(hdev), > + &appleals_hid_driver); > + > + appleals_config_sensor(als_dev, als_dev->events_enabled, > + als_dev->cur_sensitivity); > + > + return 0; > +} > +#endif > + > +static struct hid_driver appleals_hid_driver = { > + .name = "apple-ib-als", > + .probe = appleals_probe, > + .remove = appleals_remove, > + .event = appleals_hid_event, > +#ifdef CONFIG_PM > + .reset_resume = appleals_reset_resume, > +#endif > +}; > + > +static int appleals_platform_probe(struct platform_device *pdev) > +{ > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > + struct appleib_device *ib_dev = pdata->ib_dev; > + struct appleals_device *als_dev; > + int rc; > + > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL); > + if (!als_dev) > + return -ENOMEM; > + > + als_dev->ib_dev = ib_dev; > + als_dev->log_dev = pdata->log_dev; > + > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev); > + if (rc) { > + dev_err(als_dev->log_dev, "Error registering hid driver: %d\n", > + rc); > + goto error; > + } > + > + platform_set_drvdata(pdev, als_dev); > + > + return 0; > + > +error: > + kfree(als_dev); > + return rc; > +} > + > +static int appleals_platform_remove(struct platform_device *pdev) > +{ > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > + struct appleib_device *ib_dev = pdata->ib_dev; > + struct appleals_device *als_dev = platform_get_drvdata(pdev); > + int rc; > + > + rc = appleib_unregister_hid_driver(ib_dev, &appleals_hid_driver); > + if (rc) { > + dev_err(als_dev->log_dev, > + "Error unregistering hid driver: %d\n", rc); > + goto error; > + } > + > + kfree(als_dev); > + > + return 0; > + > +error: > + return rc; > +} > + > +static const struct platform_device_id appleals_platform_ids[] = { > + { .name = PLAT_NAME_IB_ALS }, > + { } > +}; > +MODULE_DEVICE_TABLE(platform, appleals_platform_ids); > + > +static struct platform_driver appleals_platform_driver = { > + .id_table = appleals_platform_ids, > + .driver = { > + .name = "apple-ib-als", > + }, > + .probe = appleals_platform_probe, > + .remove = appleals_platform_remove, > +}; > + > +module_platform_driver(appleals_platform_driver); > + > +MODULE_AUTHOR("Ronald Tschalär"); > +MODULE_DESCRIPTION("Apple iBridge ALS driver"); > +MODULE_LICENSE("GPL v2"); >
On Mon, 22 Apr 2019 11:17:27 +0200 (CEST) Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote: > On Sun, 21 Apr 2019, Ronald Tschalär 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. > > some comments below inline I'll 'nest' on Peter's review to avoid repetition. A few additional comments inline. Thanks, Jonathan > > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch> > > --- > > drivers/iio/light/Kconfig | 12 + > > drivers/iio/light/Makefile | 1 + > > drivers/iio/light/apple-ib-als.c | 694 +++++++++++++++++++++++++++++++ > > 3 files changed, 707 insertions(+) > > create mode 100644 drivers/iio/light/apple-ib-als.c > > > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > > index 36f458433480..49159fab1c0e 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 MFD_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 286bf3975372..144d918917f7 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..1718fcbe304f > > --- /dev/null > > +++ b/drivers/iio/light/apple-ib-als.c > > @@ -0,0 +1,694 @@ > > +// 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/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/mfd/apple-ibridge.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.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 appleib_device *ib_dev; > > + struct device *log_dev; > > + struct hid_device *hid_dev; > > + struct hid_report *cfg_report; > > + struct hid_field *illum_field; > > + struct iio_dev *iio_dev; > > + struct iio_trigger *iio_trig; > > + 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 struct appleals_sensitivity_map appleals_sensitivity_map[] = { > > const? > > > + { 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) > > +{ > > + 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) > > +{ > > + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT); > > + hid_hw_wait(als_dev->hid_dev); > > + > > + 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); > > can return -1, not checked > > > + > > + 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 = > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > + &appleals_hid_driver); > > + 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; > > + > > + /* set the sensor's reporting state */ > > + 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 *als_dev = > > + *(struct appleals_device **)iio_priv(iio_dev); > > + __s32 value; > > + int rc; > > + > > + *val = 0; > > + *val2 = 0; > > no need to set these here > > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + case IIO_CHAN_INFO_PROCESSED: > > + *val = appleals_get_field_value(als_dev, als_dev->illum_field); How can one read by both processed and raw? > > + 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 *als_dev = > > + *(struct appleals_device **)iio_priv(iio_dev); > > + __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)); > > + break; > > maybe return directly instead of at the end (matter of taste); > here and in the other cases below > > > + > > + 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); There is some debate in another thread on whether dynamic sensitivity can be mapped to hysteresis or not... > > + } > > + rc = 0; > > + break; > > + } > > + > > + 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; > > + } > > + break; > > + > > + default: > > + rc = -EINVAL; > > + } > > + > > + return rc; > > +} > > + > > +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) | > > + BIT(IIO_CHAN_INFO_RAW), Why return both processed and raw? We don't generally allow that in IIO (there are a few historical exceptions due to us getting it wrong the first time). > > + .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) | > > + BIT(IIO_CHAN_INFO_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 = 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; > > + __s32 val; > > + > > + /* > > + * We're (often) in a probe here, so need to enable input processing > > + * in that case, but only in that case. > > + */ I'd be a little happier if this wasn't termed 'in_hid_probe' but rather something like _needs_io_start. That way we are reflecting what needs to happen, not where we are in the code flow that makes it true. > > + if (appleib_in_hid_probe(als_dev->ib_dev)) > > + hid_device_io_start(als_dev->hid_dev); > > + > > + /* power on the sensor */ > > + 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); > > what if -1? > > > + hid_set_field(field, 0, val); > > + > > + /* configure reporting of change events */ > > + 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); > > + hid_set_field(field, 0, val); > > + > > + /* report change events asap */ > > + 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); > > + } > > + > > + /* write the new config to the sensor */ > > + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report, > > + HID_REQ_SET_REPORT); > > + > > + if (appleib_in_hid_probe(als_dev->ib_dev)) > > + hid_device_io_stop(als_dev->hid_dev); > > +}; > > no semicolon at the end of a function please > > > + > > +static int appleals_config_iio(struct appleals_device *als_dev) > > +{ > > + struct iio_dev *iio_dev; > > + struct iio_trigger *iio_trig; > > + int rc; > > + > > + /* create and register iio device */ A very good reason for not adding superficial comments. This one is wrong or at least misleading. It doesn't register the iio device. So get rid of all these comments that are just saying what the code next to them is doing. Keep the ones that add extra information. > > + iio_dev = iio_device_alloc(sizeof(als_dev)); > > how about using the devm_ variants? > > > + if (!iio_dev) > > + return -ENOMEM; > > + > > + *(struct appleals_device **)iio_priv(iio_dev) = als_dev; That is nasty. Add a local variable to make to clear that iio_priv(iio_dev) is actually some space for this pointer. Complex type casting is not terribly readable. > > + > > + iio_dev->channels = appleals_channels; > > + iio_dev->num_channels = ARRAY_SIZE(appleals_channels); > > + iio_dev->dev.parent = &als_dev->hid_dev->dev; > > + iio_dev->info = &appleals_info; > > + iio_dev->name = "als"; > > + iio_dev->modes = INDIO_DIRECT_MODE; > > + > > + rc = iio_triggered_buffer_setup(iio_dev, &iio_pollfunc_store_time, NULL, > > + NULL); > > + if (rc) { > > + dev_err(als_dev->log_dev, "failed to set up iio triggers: %d\n", > > just one trigger? Also wrong as it's not actually setting up the trigger at all ;) It's setting up the handling for what to do when a trigger event occurs. > > > + rc); > > + goto free_iio_dev; > > + } > > + > > + iio_trig = iio_trigger_alloc("%s-dev%d", iio_dev->name, iio_dev->id); > > + if (!iio_trig) { > > + rc = -ENOMEM; > > + goto clean_trig_buf; > > + } > > + > > + iio_trig->dev.parent = &als_dev->hid_dev->dev; > > + iio_trig->ops = &appleals_trigger_ops; > > + iio_trigger_set_drvdata(iio_trig, als_dev); > > + > > + rc = iio_trigger_register(iio_trig); > > + if (rc) { > > + dev_err(als_dev->log_dev, "failed to register iio trigger: %d\n", > > some messages start lowercase, some uppercase (nitpicking) > > > + rc); > > + goto free_iio_trig; > > + } > > + > > + als_dev->iio_trig = iio_trig; > > + > > + rc = iio_device_register(iio_dev); > > + if (rc) { > > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n", > > + rc); > > + goto unreg_iio_trig; > > + } > > + > > + als_dev->iio_dev = iio_dev; I really don't like nest of pointers going on in here. I haven't dug down to check if any of them can be remove, but they are definitely ugly to deal with. > > + > > + return 0; > > + > > +unreg_iio_trig: > > + iio_trigger_unregister(iio_trig); > > +free_iio_trig: > > + iio_trigger_free(iio_trig); > > + als_dev->iio_trig = NULL; > > +clean_trig_buf: > > + iio_triggered_buffer_cleanup(iio_dev); > > +free_iio_dev: > > + iio_device_free(iio_dev); > > + > > + return rc; > > +} > > + > > +static int appleals_probe(struct hid_device *hdev, > > + const struct hid_device_id *id) > > +{ > > + struct appleals_device *als_dev = > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > + &appleals_hid_driver); > > + struct hid_field *state_field; > > + struct hid_field *illum_field; > > + int rc; > > + > > + /* find als fields and reports */ > > + 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; > > + > > + if (als_dev->hid_dev) { > > + dev_warn(als_dev->log_dev, > > + "Found duplicate ambient light sensor - ignoring\n"); I'll bite. So how can this actually happen? What fundamentally breaks in your model if there really are two? Can you fix whatever that is so as to drop this handling? > > + return -EBUSY; > > + } > > + > > + dev_info(als_dev->log_dev, "Found ambient light sensor\n"); > > in general avoid logging for the OK case, it just clutters the log > > > + > > + /* initialize device */ > > + 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; > > + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity); > > + > > + rc = appleals_config_iio(als_dev); > > + if (rc) > > + return rc; > > + > > + return 0; return appleals_config_iio(als_dev); > > +} > > + > > +static void appleals_remove(struct hid_device *hdev) > > +{ > > + struct appleals_device *als_dev = > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > + &appleals_hid_driver); > > + > > could be a lot less if devm_ were used? > > > + if (als_dev->iio_dev) { > > + iio_device_unregister(als_dev->iio_dev); > > + > > + iio_trigger_unregister(als_dev->iio_trig); > > + iio_trigger_free(als_dev->iio_trig); > > + als_dev->iio_trig = NULL; I would be decidedly worried if you actually have any paths where setting these to NULL do anything useful. By the time we get here we should absolutely 'know' nothing will touch these pointers. It is these that are blocking Peter's suggestion of using devm to clean all of this up automatically for you. > > + > > + iio_triggered_buffer_cleanup(als_dev->iio_dev); > > + iio_device_free(als_dev->iio_dev); > > + als_dev->iio_dev = NULL; > > + } > > + > > + als_dev->hid_dev = NULL; > > +} > > + > > +#ifdef CONFIG_PM > > +static int appleals_reset_resume(struct hid_device *hdev) > > +{ > > + struct appleals_device *als_dev = > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > + &appleals_hid_driver); > > + > > + appleals_config_sensor(als_dev, als_dev->events_enabled, > > + als_dev->cur_sensitivity); > > + > > + return 0; > > +} > > +#endif > > + > > +static struct hid_driver appleals_hid_driver = { > > + .name = "apple-ib-als", > > + .probe = appleals_probe, > > + .remove = appleals_remove, > > + .event = appleals_hid_event, > > +#ifdef CONFIG_PM > > + .reset_resume = appleals_reset_resume, > > +#endif > > +}; > > + > > +static int appleals_platform_probe(struct platform_device *pdev) > > +{ > > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > > + struct appleib_device *ib_dev = pdata->ib_dev; > > + struct appleals_device *als_dev; > > + int rc; > > + > > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL); > > + if (!als_dev) > > + return -ENOMEM; > > + > > + als_dev->ib_dev = ib_dev; > > + als_dev->log_dev = pdata->log_dev; > > + > > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev); Hmm. This is all a bit odd. We register a platform device, to call it's driver so that it can then register another driver? I'll let Lee comment on that but it does seem overly complex. > > + if (rc) { > > + dev_err(als_dev->log_dev, "Error registering hid driver: %d\n", > > + rc); > > + goto error; > > + } > > + > > + platform_set_drvdata(pdev, als_dev); > > + > > + return 0; > > + > > +error: > > + kfree(als_dev); > > + return rc; > > +} > > + > > +static int appleals_platform_remove(struct platform_device *pdev) > > +{ > > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > > + struct appleib_device *ib_dev = pdata->ib_dev; > > + struct appleals_device *als_dev = platform_get_drvdata(pdev); > > + int rc; > > + > > + rc = appleib_unregister_hid_driver(ib_dev, &appleals_hid_driver); > > + if (rc) { > > + dev_err(als_dev->log_dev, > > + "Error unregistering hid driver: %d\n", rc); Given you are going to have this error printing in every sub driver and the text is totally generic, can you just move it into the core? Same applies for similar cases above. > > + goto error; > > + } > > + > > + kfree(als_dev); Use managed allocation to avoid needing to clean this up? > > + > > + return 0; > > + > > +error: > > + return rc; > > +} > > + > > +static const struct platform_device_id appleals_platform_ids[] = { > > + { .name = PLAT_NAME_IB_ALS }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(platform, appleals_platform_ids); > > + > > +static struct platform_driver appleals_platform_driver = { > > + .id_table = appleals_platform_ids, > > + .driver = { > > + .name = "apple-ib-als", > > + }, > > + .probe = appleals_platform_probe, > > + .remove = appleals_platform_remove, > > +}; > > + > > +module_platform_driver(appleals_platform_driver); > > + > > +MODULE_AUTHOR("Ronald Tschalär"); > > +MODULE_DESCRIPTION("Apple iBridge ALS driver"); > > +MODULE_LICENSE("GPL v2"); > > >
Hi Jonathan, Peter, On Mon, Apr 22, 2019 at 01:01:38PM +0100, Jonathan Cameron wrote: > On Mon, 22 Apr 2019 11:17:27 +0200 (CEST) > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote: > > > On Sun, 21 Apr 2019, Ronald Tschalär 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. > > > > some comments below inline > I'll 'nest' on Peter's review to avoid repetition. > > A few additional comments inline. Thank you both for your reviews. I've applied most of your suggestions, so I'm limiting my responses below to just those places where I need further clarification or can provide some answers. [snip] > > > +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 *als_dev = > > > + *(struct appleals_device **)iio_priv(iio_dev); > > > + __s32 value; > > > + int rc; > > > + > > > + *val = 0; > > > + *val2 = 0; > > > > no need to set these here > > > > > + > > > + switch (mask) { > > > + case IIO_CHAN_INFO_RAW: > > > + case IIO_CHAN_INFO_PROCESSED: > > > + *val = appleals_get_field_value(als_dev, als_dev->illum_field); > > How can one read by both processed and raw? From my understanding, processed is a converted and normalized value of raw? But since the raw value is already in Lux the processed value is then the same. Furthermore, looking at userspace apps that read these values (e.g. iio-sensor-proxy, lightsd) they seem to be all over the map in terms of what sysfs entries they read: in_illuminance_input, in_intensity_both_raw, etc. So I figured providing both would provide maximal compatibility. What then is currently the preferred approach here? > > > + 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); > > There is some debate in another thread on whether dynamic sensitivity can be > mapped to hysteresis or not... Is that the "drivers: iio: Add more data field for iio driver hysteresis parsing" thread? In any case, I'm using the value 0 here to indicate that the pseudo-percent-relative change sensitivity should be used. Better suggestions certainly welcome. [snip] > > > +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) | > > > + BIT(IIO_CHAN_INFO_RAW), > Why return both processed and raw? We don't generally allow that in IIO > (there are a few historical exceptions due to us getting it wrong the > first time). Ok. But which should be used then, especially in view of the fact that different user-space apps/daemons appear to use different values? [snip] > > > +static int appleals_config_iio(struct appleals_device *als_dev) [snip] > > > + iio_dev = iio_device_alloc(sizeof(als_dev)); > > > > how about using the devm_ variants? The problem is that there is no device with the proper lifetime here. In particular we can't use hid_device (the only struct device available here) because the instances of those can (and do) outlive the module. This is a consequence of the hid-driver demuxing: e.g. when this als module is unloaded, the hid_device is still in use by the touchbar driver, and if this als module is then loaded again it will get associated with that same hid_device again. [snip] > > > + rc = iio_device_register(iio_dev); > > > + if (rc) { > > > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n", > > > + rc); > > > + goto unreg_iio_trig; > > > + } > > > + > > > + als_dev->iio_dev = iio_dev; > > I really don't like nest of pointers going on in here. I haven't dug > down to check if any of them can be remove, but they are definitely > ugly to deal with. I'm not sure how this can be avoided: *iio_dev is allocated by iio_device_alloc(), and this is just storing that pointer in our data structure for this als device. [snip] > > > +static int appleals_probe(struct hid_device *hdev, > > > + const struct hid_device_id *id) > > > +{ > > > + struct appleals_device *als_dev = > > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > > + &appleals_hid_driver); > > > + struct hid_field *state_field; > > > + struct hid_field *illum_field; > > > + int rc; > > > + > > > + /* find als fields and reports */ > > > + 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; > > > + > > > + if (als_dev->hid_dev) { > > > + dev_warn(als_dev->log_dev, > > > + "Found duplicate ambient light sensor - ignoring\n"); > > I'll bite. So how can this actually happen? What fundamentally breaks in > your model if there really are two? Can you fix whatever that is so > as to drop this handling? This should indeed never happen - the check is just defensive programming, in case either some new device in some new model appears, or due to some bug somewhere. To actually handle this I'd need to split up appleals_device into a per iBridge-device part and a per ALS-device part, and allow multiple ALS-device parts. This is certainly doable, but seemed a bit overkill for something that is unlikely to ever be needed. [snip] > > > +static void appleals_remove(struct hid_device *hdev) > > > +{ > > > + struct appleals_device *als_dev = > > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > > + &appleals_hid_driver); > > > + > > > > could be a lot less if devm_ were used? Agreed, but see explanation above of why this can't be used. > > > + if (als_dev->iio_dev) { > > > + iio_device_unregister(als_dev->iio_dev); > > > + > > > + iio_trigger_unregister(als_dev->iio_trig); > > > + iio_trigger_free(als_dev->iio_trig); > > > + als_dev->iio_trig = NULL; > > I would be decidedly worried if you actually have any paths > where setting these to NULL do anything useful. By the time > we get here we should absolutely 'know' nothing will touch > these pointers. I guess I was being overly paranoid here. I've removed these now. > It is these that are blocking Peter's suggestion of using > devm to clean all of this up automatically for you. No, that is for a different reason - see above. [snip] > > > +static int appleals_platform_probe(struct platform_device *pdev) > > > +{ > > > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > > > + struct appleib_device *ib_dev = pdata->ib_dev; > > > + struct appleals_device *als_dev; > > > + int rc; > > > + > > > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL); > > > + if (!als_dev) > > > + return -ENOMEM; > > > + > > > + als_dev->ib_dev = ib_dev; > > > + als_dev->log_dev = pdata->log_dev; > > > + > > > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev); > > Hmm. This is all a bit odd. We register a platform device, to call it's driver > so that it can then register another driver? I'll let Lee comment on that but > it does seem overly complex. "Normally" this call would be to hid_register_driver(). However, as I tried to explain in the cover letter and module comments, at least one of the hid_device's needs to be shared by both this als driver and the touchbar driver. But the linux device/driver architecture does not allow multiple drivers to be attached to a single device. Hence the mfd driver implements demuxing hid_driver that allows multiple hid_drivers to be registered for a single hid_device. And this is why we register our hid_driver with this demuxing driver here instead of directly via hid_register_driver(). Does this make sense? Cheers, Ronald
On Tue, 23 Apr 2019 03:38:55 -0700 "Life is hard, and then you die" <ronald@innovation.ch> wrote: > Hi Jonathan, Peter, > > On Mon, Apr 22, 2019 at 01:01:38PM +0100, Jonathan Cameron wrote: > > On Mon, 22 Apr 2019 11:17:27 +0200 (CEST) > > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote: > > > > > On Sun, 21 Apr 2019, Ronald Tschalär 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. > > > > > > some comments below inline > > I'll 'nest' on Peter's review to avoid repetition. > > > > A few additional comments inline. > > Thank you both for your reviews. I've applied most of your > suggestions, so I'm limiting my responses below to just those places > where I need further clarification or can provide some answers. > > [snip] > > > > +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 *als_dev = > > > > + *(struct appleals_device **)iio_priv(iio_dev); > > > > + __s32 value; > > > > + int rc; > > > > + > > > > + *val = 0; > > > > + *val2 = 0; > > > > > > no need to set these here > > > > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_RAW: > > > > + case IIO_CHAN_INFO_PROCESSED: > > > > + *val = appleals_get_field_value(als_dev, als_dev->illum_field); > > > > How can one read by both processed and raw? > > From my understanding, processed is a converted and normalized value > of raw? But since the raw value is already in Lux the processed value > is then the same. Furthermore, looking at userspace apps that read > these values (e.g. iio-sensor-proxy, lightsd) they seem to be all over > the map in terms of what sysfs entries they read: > in_illuminance_input, in_intensity_both_raw, etc. So I figured > providing both would provide maximal compatibility. > > What then is currently the preferred approach here? Just provide the processed value. We may have to fix any userspace apps that aren't checking both. For the raw version they should be applying the scale in userspace. > > > > > + 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); > > > > There is some debate in another thread on whether dynamic sensitivity can be > > mapped to hysteresis or not... > > Is that the "drivers: iio: Add more data field for iio driver > hysteresis parsing" thread? That's the one. > > In any case, I'm using the value 0 here to indicate that the > pseudo-percent-relative change sensitivity should be used. Better > suggestions certainly welcome. It seems we are converging on a new IIO_CHAN_INFO type for sensitivity. > > [snip] > > > > +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) | > > > > + BIT(IIO_CHAN_INFO_RAW), > > Why return both processed and raw? We don't generally allow that in IIO > > (there are a few historical exceptions due to us getting it wrong the > > first time). > > Ok. But which should be used then, especially in view of the fact that > different user-space apps/daemons appear to use different values? forwards. > > [snip] > > > > +static int appleals_config_iio(struct appleals_device *als_dev) > [snip] > > > > + iio_dev = iio_device_alloc(sizeof(als_dev)); > > > > > > how about using the devm_ variants? > > The problem is that there is no device with the proper lifetime here. > In particular we can't use hid_device (the only struct device > available here) because the instances of those can (and do) outlive > the module. This is a consequence of the hid-driver demuxing: e.g. > when this als module is unloaded, the hid_device is still in use by > the touchbar driver, and if this als module is then loaded again it > will get associated with that same hid_device again. Fair enough. > > [snip] > > > > + rc = iio_device_register(iio_dev); > > > > + if (rc) { > > > > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n", > > > > + rc); > > > > + goto unreg_iio_trig; > > > > + } > > > > + > > > > + als_dev->iio_dev = iio_dev; > > > > I really don't like nest of pointers going on in here. I haven't dug > > down to check if any of them can be remove, but they are definitely > > ugly to deal with. > > I'm not sure how this can be avoided: *iio_dev is allocated by > iio_device_alloc(), and this is just storing that pointer in our data > structure for this als device. You are probably correct. Ugly will have to stay :( This tends to happen when we end up merging various different driver subsystems together like this. > > [snip] > > > > +static int appleals_probe(struct hid_device *hdev, > > > > + const struct hid_device_id *id) > > > > +{ > > > > + struct appleals_device *als_dev = > > > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > > > + &appleals_hid_driver); > > > > + struct hid_field *state_field; > > > > + struct hid_field *illum_field; > > > > + int rc; > > > > + > > > > + /* find als fields and reports */ > > > > + 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; > > > > + > > > > + if (als_dev->hid_dev) { > > > > + dev_warn(als_dev->log_dev, > > > > + "Found duplicate ambient light sensor - ignoring\n"); > > > > I'll bite. So how can this actually happen? What fundamentally breaks in > > your model if there really are two? Can you fix whatever that is so > > as to drop this handling? > > This should indeed never happen - the check is just defensive > programming, in case either some new device in some new model appears, > or due to some bug somewhere. > > To actually handle this I'd need to split up appleals_device into a > per iBridge-device part and a per ALS-device part, and allow multiple > ALS-device parts. This is certainly doable, but seemed a bit overkill > for something that is unlikely to ever be needed. Fair enough I guess. > > [snip] > > > > +static void appleals_remove(struct hid_device *hdev) > > > > +{ > > > > + struct appleals_device *als_dev = > > > > + appleib_get_drvdata(hid_get_drvdata(hdev), > > > > + &appleals_hid_driver); > > > > + > > > > > > could be a lot less if devm_ were used? > > Agreed, but see explanation above of why this can't be used. > > > > > + if (als_dev->iio_dev) { > > > > + iio_device_unregister(als_dev->iio_dev); > > > > + > > > > + iio_trigger_unregister(als_dev->iio_trig); > > > > + iio_trigger_free(als_dev->iio_trig); > > > > + als_dev->iio_trig = NULL; > > > > I would be decidedly worried if you actually have any paths > > where setting these to NULL do anything useful. By the time > > we get here we should absolutely 'know' nothing will touch > > these pointers. > > I guess I was being overly paranoid here. I've removed these now. > > > It is these that are blocking Peter's suggestion of using > > devm to clean all of this up automatically for you. > > No, that is for a different reason - see above. > > [snip] > > > > +static int appleals_platform_probe(struct platform_device *pdev) > > > > +{ > > > > + struct appleib_platform_data *pdata = pdev->dev.platform_data; > > > > + struct appleib_device *ib_dev = pdata->ib_dev; > > > > + struct appleals_device *als_dev; > > > > + int rc; > > > > + > > > > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL); > > > > + if (!als_dev) > > > > + return -ENOMEM; > > > > + > > > > + als_dev->ib_dev = ib_dev; > > > > + als_dev->log_dev = pdata->log_dev; > > > > + > > > > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev); > > > > Hmm. This is all a bit odd. We register a platform device, to call it's driver > > so that it can then register another driver? I'll let Lee comment on that but > > it does seem overly complex. > > "Normally" this call would be to hid_register_driver(). > However, as I > tried to explain in the cover letter and module comments, at least one > of the hid_device's needs to be shared by both this als driver and the > touchbar driver. But the linux device/driver architecture does not > allow multiple drivers to be attached to a single device. Hence the > mfd driver implements demuxing hid_driver that allows multiple > hid_drivers to be registered for a single hid_device. And this is why > we register our hid_driver with this demuxing driver here instead of > directly via hid_register_driver(). I'd expect a probe to create a device managed by the driver. The drive would normally be registered on module load, hence _init, rather than probe. I can't find any instances of hid_register_driver being called from a probe function. Jonathan > > Does this make sense? > > > Cheers, > > Ronald >
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 36f458433480..49159fab1c0e 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 MFD_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 286bf3975372..144d918917f7 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..1718fcbe304f --- /dev/null +++ b/drivers/iio/light/apple-ib-als.c @@ -0,0 +1,694 @@ +// 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/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/mfd/apple-ibridge.h> +#include <linux/module.h> +#include <linux/platform_device.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 appleib_device *ib_dev; + struct device *log_dev; + struct hid_device *hid_dev; + struct hid_report *cfg_report; + struct hid_field *illum_field; + struct iio_dev *iio_dev; + struct iio_trigger *iio_trig; + 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 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) +{ + 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) +{ + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT); + hid_hw_wait(als_dev->hid_dev); + + 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); + + 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 = + appleib_get_drvdata(hid_get_drvdata(hdev), + &appleals_hid_driver); + 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; + + /* set the sensor's reporting state */ + 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 *als_dev = + *(struct appleals_device **)iio_priv(iio_dev); + __s32 value; + int rc; + + *val = 0; + *val2 = 0; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + 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 *als_dev = + *(struct appleals_device **)iio_priv(iio_dev); + __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)); + break; + + 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); + } + rc = 0; + break; + } + + 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; + } + break; + + default: + rc = -EINVAL; + } + + return rc; +} + +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) | + BIT(IIO_CHAN_INFO_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) | + BIT(IIO_CHAN_INFO_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 = 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; + __s32 val; + + /* + * We're (often) in a probe here, so need to enable input processing + * in that case, but only in that case. + */ + if (appleib_in_hid_probe(als_dev->ib_dev)) + hid_device_io_start(als_dev->hid_dev); + + /* power on the sensor */ + 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); + hid_set_field(field, 0, val); + + /* configure reporting of change events */ + 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); + hid_set_field(field, 0, val); + + /* report change events asap */ + 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); + } + + /* write the new config to the sensor */ + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report, + HID_REQ_SET_REPORT); + + if (appleib_in_hid_probe(als_dev->ib_dev)) + hid_device_io_stop(als_dev->hid_dev); +}; + +static int appleals_config_iio(struct appleals_device *als_dev) +{ + struct iio_dev *iio_dev; + struct iio_trigger *iio_trig; + int rc; + + /* create and register iio device */ + iio_dev = iio_device_alloc(sizeof(als_dev)); + if (!iio_dev) + return -ENOMEM; + + *(struct appleals_device **)iio_priv(iio_dev) = als_dev; + + iio_dev->channels = appleals_channels; + iio_dev->num_channels = ARRAY_SIZE(appleals_channels); + iio_dev->dev.parent = &als_dev->hid_dev->dev; + iio_dev->info = &appleals_info; + iio_dev->name = "als"; + iio_dev->modes = INDIO_DIRECT_MODE; + + rc = iio_triggered_buffer_setup(iio_dev, &iio_pollfunc_store_time, NULL, + NULL); + if (rc) { + dev_err(als_dev->log_dev, "failed to set up iio triggers: %d\n", + rc); + goto free_iio_dev; + } + + iio_trig = iio_trigger_alloc("%s-dev%d", iio_dev->name, iio_dev->id); + if (!iio_trig) { + rc = -ENOMEM; + goto clean_trig_buf; + } + + iio_trig->dev.parent = &als_dev->hid_dev->dev; + iio_trig->ops = &appleals_trigger_ops; + iio_trigger_set_drvdata(iio_trig, als_dev); + + rc = iio_trigger_register(iio_trig); + if (rc) { + dev_err(als_dev->log_dev, "failed to register iio trigger: %d\n", + rc); + goto free_iio_trig; + } + + als_dev->iio_trig = iio_trig; + + rc = iio_device_register(iio_dev); + if (rc) { + dev_err(als_dev->log_dev, "failed to register iio device: %d\n", + rc); + goto unreg_iio_trig; + } + + als_dev->iio_dev = iio_dev; + + return 0; + +unreg_iio_trig: + iio_trigger_unregister(iio_trig); +free_iio_trig: + iio_trigger_free(iio_trig); + als_dev->iio_trig = NULL; +clean_trig_buf: + iio_triggered_buffer_cleanup(iio_dev); +free_iio_dev: + iio_device_free(iio_dev); + + return rc; +} + +static int appleals_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + struct appleals_device *als_dev = + appleib_get_drvdata(hid_get_drvdata(hdev), + &appleals_hid_driver); + struct hid_field *state_field; + struct hid_field *illum_field; + int rc; + + /* find als fields and reports */ + 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; + + if (als_dev->hid_dev) { + dev_warn(als_dev->log_dev, + "Found duplicate ambient light sensor - ignoring\n"); + return -EBUSY; + } + + dev_info(als_dev->log_dev, "Found ambient light sensor\n"); + + /* initialize device */ + 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; + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity); + + rc = appleals_config_iio(als_dev); + if (rc) + return rc; + + return 0; +} + +static void appleals_remove(struct hid_device *hdev) +{ + struct appleals_device *als_dev = + appleib_get_drvdata(hid_get_drvdata(hdev), + &appleals_hid_driver); + + if (als_dev->iio_dev) { + iio_device_unregister(als_dev->iio_dev); + + iio_trigger_unregister(als_dev->iio_trig); + iio_trigger_free(als_dev->iio_trig); + als_dev->iio_trig = NULL; + + iio_triggered_buffer_cleanup(als_dev->iio_dev); + iio_device_free(als_dev->iio_dev); + als_dev->iio_dev = NULL; + } + + als_dev->hid_dev = NULL; +} + +#ifdef CONFIG_PM +static int appleals_reset_resume(struct hid_device *hdev) +{ + struct appleals_device *als_dev = + appleib_get_drvdata(hid_get_drvdata(hdev), + &appleals_hid_driver); + + appleals_config_sensor(als_dev, als_dev->events_enabled, + als_dev->cur_sensitivity); + + return 0; +} +#endif + +static struct hid_driver appleals_hid_driver = { + .name = "apple-ib-als", + .probe = appleals_probe, + .remove = appleals_remove, + .event = appleals_hid_event, +#ifdef CONFIG_PM + .reset_resume = appleals_reset_resume, +#endif +}; + +static int appleals_platform_probe(struct platform_device *pdev) +{ + struct appleib_platform_data *pdata = pdev->dev.platform_data; + struct appleib_device *ib_dev = pdata->ib_dev; + struct appleals_device *als_dev; + int rc; + + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL); + if (!als_dev) + return -ENOMEM; + + als_dev->ib_dev = ib_dev; + als_dev->log_dev = pdata->log_dev; + + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev); + if (rc) { + dev_err(als_dev->log_dev, "Error registering hid driver: %d\n", + rc); + goto error; + } + + platform_set_drvdata(pdev, als_dev); + + return 0; + +error: + kfree(als_dev); + return rc; +} + +static int appleals_platform_remove(struct platform_device *pdev) +{ + struct appleib_platform_data *pdata = pdev->dev.platform_data; + struct appleib_device *ib_dev = pdata->ib_dev; + struct appleals_device *als_dev = platform_get_drvdata(pdev); + int rc; + + rc = appleib_unregister_hid_driver(ib_dev, &appleals_hid_driver); + if (rc) { + dev_err(als_dev->log_dev, + "Error unregistering hid driver: %d\n", rc); + goto error; + } + + kfree(als_dev); + + return 0; + +error: + return rc; +} + +static const struct platform_device_id appleals_platform_ids[] = { + { .name = PLAT_NAME_IB_ALS }, + { } +}; +MODULE_DEVICE_TABLE(platform, appleals_platform_ids); + +static struct platform_driver appleals_platform_driver = { + .id_table = appleals_platform_ids, + .driver = { + .name = "apple-ib-als", + }, + .probe = appleals_platform_probe, + .remove = appleals_platform_remove, +}; + +module_platform_driver(appleals_platform_driver); + +MODULE_AUTHOR("Ronald Tschalär"); +MODULE_DESCRIPTION("Apple iBridge ALS driver"); +MODULE_LICENSE("GPL v2");
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 <ronald@innovation.ch> --- drivers/iio/light/Kconfig | 12 + drivers/iio/light/Makefile | 1 + drivers/iio/light/apple-ib-als.c | 694 +++++++++++++++++++++++++++++++ 3 files changed, 707 insertions(+) create mode 100644 drivers/iio/light/apple-ib-als.c