Date:   Mon, 22 Apr 2019 13:01:38 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc:     Ronald Tschalär <ronald@...ovation.ch>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Lee Jones <lee.jones@...aro.org>, linux-input@...r.kernel.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: light: apple-ib-als: Add driver for ALS on
 iBridge chip.

On Mon, 22 Apr 2019 11:17:27 +0200 (CEST)
Peter Meerwald-Stadler <pmeerw@...erw.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@...ovation.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");
> >   
> 

Powered by blists - more mailing lists