Re: [PATCH 4/6] iio: light: rpr0521 proximity offset read/write added

Re: [PATCH 4/6] iio: light: rpr0521 proximity offset read/write added

[Koivunen, Mikko]

On 5.4.2017 15:03, Peter Meerwald-Stadler wrote:
> nOn Wed, 5 Apr 2017, Mikko Koivunen wrote:
>
>> Added sysfs read/write proximity offset feature. Offset is read/write from
>> sensor registers. Values are proximity raw 10-bit values. After applying
>> offset value, output values will be (measured_raw - offset_value). Output
>> values are unsigned so offset value doesn't make output negative.
> comments below

Fixing to patchset v2.

>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
>>  drivers/iio/light/rpr0521.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 13e2f9a..579bf6d 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -30,6 +30,9 @@
>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>> +#define RPR0521_PS_OFFSET_LSB		0x53
>> +#define RPR0521_PS_OFFSET_MSB		0x54
>> +
>>  #define RPR0521_REG_ID			0x92
>>  
>>  #define RPR0521_MODE_ALS_MASK		BIT(7)
>> @@ -214,7 +217,9 @@ struct rpr0521_data {
>>  		.type = IIO_PROXIMITY,
>>  		.address = RPR0521_CHAN_PXS,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_OFFSET) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	}
>>  };
>> @@ -400,6 +405,42 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>>  		i);
>>  }
>>  
>> +int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
> static
>
>> +{
>> +	int ret;
>> +	u8 buffer[2];
> __le16
>
>> +
>> +	ret = regmap_bulk_read(data->regmap,
>> +		RPR0521_PS_OFFSET_LSB, &buffer, 2);
> sizeof(buffer)
>
>> +
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
>> +		return ret;
>> +	}
>> +	*offset = buffer[0] + (buffer[1] << 8);
> le16_to_cpu()
>
>> +
>> +	return ret;
>> +}
>> +
>> +int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
> static
>
>> +{
>> +	int ret;
>> +	u8 buffer[2];
> __le16
>
>> +
>> +	buffer[0] = offset & 0xff;
>> +	buffer[1] = (offset >> 8) & (BIT(0) | BIT(1));
>> +
>> +	ret = regmap_raw_write(data->regmap,
>> +		RPR0521_PS_OFFSET_LSB, &buffer, 2);
> sizeof(buffer)
>
>> +
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *chan, int *val,
>>  			    int *val2, long mask)
>> @@ -456,6 +497,14 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			return ret;
>>  		return IIO_VAL_INT_PLUS_MICRO;
>>  
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_read_ps_offset(data, val);
>> +		mutex_unlock(&data->lock);
>> +		if (ret < 0)
>> +			return ret;
>> +		return IIO_VAL_INT;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -483,6 +532,12 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  		mutex_unlock(&data->lock);
>>  		return ret;
>>  
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_write_ps_offset(data, val);
>> +		mutex_unlock(&data->lock);
>> +		return ret;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>>

Re: [PATCH 6/6] iio: light: rpr0521 triggered buffer added

[Koivunen, Mikko]

On 5.4.2017 15:14, Peter Meerwald-Stadler wrote:
> On Wed, 5 Apr 2017, Mikko Koivunen wrote:
>
>> Driver sets up and uses triggered buffer if there is irq defined for device in
>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
> actually, there is a nasty #define which shouldn't be needed
> comments below

Ack.

>> Trigger consumer reads rpr0521 data to scan buffer.
>> Depends on previous commits of _scale and _offset.
>>
>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>> Builds OK with torvalds/linux 4.9
>> checkpatch.pl OK.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> ---
>>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 311 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index fa642b7..22cb097 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,9 +9,11 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, PM support, buffer
>> + * TODO: illuminance channel, PM support
>>   */
>>  
>> +#define RPR0521_TRIGGERS
> no, don't

Ack.

>> +
>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/i2c.h>
>> @@ -20,6 +22,12 @@
>>  #include <linux/acpi.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#ifdef RPR0521_TRIGGERS
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#endif
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/pm_runtime.h>
>>  
>> @@ -30,6 +38,7 @@
>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>> +#define RPR0521_REG_INTERRUPT		0x4A
>>  #define RPR0521_PS_OFFSET_LSB		0x53
>>  #define RPR0521_PS_OFFSET_MSB		0x54
>>  
>> @@ -44,16 +53,33 @@
>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>  #define RPR0521_PXS_GAIN_SHIFT		4
>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>>  
>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
>> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>>  
>>  #define RPR0521_MANUFACT_ID		0xE0
>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>  
>>  #define RPR0521_DRV_NAME		"RPR0521"
>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>  
>>  #define RPR0521_SLEEP_DELAY_MS	2000
>> @@ -169,6 +195,11 @@ struct rpr0521_data {
>>  	bool als_dev_en;
>>  	bool pxs_dev_en;
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +	struct iio_trigger *drdy_trigger0;
>> +	bool drdy_trigger_on;
>> +	u16 *scan_buffer;
>> +#endif
>>  	/* optimize runtime pm ops - enable device only if needed */
>>  	bool als_ps_need_en;
>>  	bool pxs_ps_need_en;
>> @@ -194,6 +225,13 @@ struct rpr0521_data {
>>  	.attrs = rpr0521_attributes,
>>  };
>>  
>> +/* Order of the channel data in buffer */
>> +enum rpr0521_scan_index_order {
>> +	RPR0521_CHAN_INDEX_PXS,
>> +	RPR0521_CHAN_INDEX_BOTH,
>> +	RPR0521_CHAN_INDEX_IR,
>> +};
>> +
>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>  	{
>>  		.type = IIO_PROXIMITY,
>> @@ -202,6 +240,13 @@ struct rpr0521_data {
>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
> the data is little endian, needs to be specified here

Ack.

>
>> +			.shift = 0,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -211,6 +256,13 @@ struct rpr0521_data {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.shift = 0,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -220,9 +272,155 @@ struct rpr0521_data {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.shift = 0,
>> +		},
>>  	},
>>  };
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>> +				   u8 device_mask);
>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
>> +
>> +/* IRQ to trigger -handler */
>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>> +{
>> +	struct iio_dev *indio_dev = private;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
>> +	/* Notify trigger0 consumers/subscribers */
>> +	if (data->drdy_trigger_on)
>> +		iio_trigger_poll(data->drdy_trigger0);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>> +
>> +	ret = regmap_bulk_read(data->regmap,
>> +		RPR0521_REG_PXS_DATA,
>> +		&buffer,
>> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
>> +	if (ret < 0)
>> +		goto done;
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +		buffer,
>> +		pf->timestamp);
>> +done:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>> +{
>> +	int err;
>> +
>> +	/* Interrupt after each measurement */
>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>> +		RPR0521_PXS_PERSISTENCE_MASK,
>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
>> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>> +		);
>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> +		RPR0521_INTERRUPT_INT_MODE_MASK,
>> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
>> +	if (err) {
>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>> +		return -EBUSY;
>> +		}
>> +	return 0;
>> +}
>> +
>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>> +{
>> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>> +				);
>> +}
>> +
>> +/* Trigger -enable/disable */
>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>> +	bool enable_drdy)
>> +{
>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int err;
>> +
>> +	if (enable_drdy) {
>> +		/* ENABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, true,
>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_write_int_enable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	} else {
>> +		/* DISABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, false,
>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +		err = rpr0521_write_int_disable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	}
>> +	data->drdy_trigger_on = enable_drdy;
>> +	return 0;
>> +
>> +rpr0521_set_state_err:
>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>> +	return err;
>> +}
>> +
>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>> +	.owner = THIS_MODULE,
>> +	};
>> +#endif
>> +
>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>  {
>>  	int ret;
>> @@ -454,6 +652,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>  			return -EINVAL;
>>  
>> +		if (iio_buffer_enabled(indio_dev))
>> +			return -EBUSY;
>> +
>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>  
>>  		mutex_lock(&data->lock);
>> @@ -542,11 +743,28 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static int rpr0521_update_scan_mode(struct iio_dev *indio_dev,
>> +	const unsigned long *scan_mask)
>> +{
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
> this is super-ugly; probably just allocate the maximum possible allocation 
> size?

Actually this is not needed at all. Receiving buffer is already in
rpr0521_trigger_consumer_handler() and this buffer is not even used.
Removing this.
There is probably need to recheck other drivers too for this pattern.

>> +	mutex_lock(&data->lock);
>> +	kfree(data->scan_buffer);
>> +	data->scan_buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> +	mutex_unlock(&data->lock);
>> +
>> +	if (data->scan_buffer == NULL)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct iio_info rpr0521_info = {
>>  	.driver_module	= THIS_MODULE,
>>  	.read_raw	= rpr0521_read_raw,
>>  	.write_raw	= rpr0521_write_raw,
>>  	.attrs		= &rpr0521_attribute_group,
>> +	.update_scan_mode = &rpr0521_update_scan_mode,
> no & for functions

Ack. + removing the function.


>>  };
>>  
>>  static int rpr0521_init(struct rpr0521_data *data)
>> @@ -664,20 +882,95 @@ static int rpr0521_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  
>> -	ret = pm_runtime_set_active(&client->dev);
>> -	if (ret < 0)
>> -		return ret;
>> +#ifdef RPR0521_TRIGGERS
>> +	/* IRQ to trigger setup */
>> +	if (client->irq) {
>> +		/* Trigger0 producer setup */
>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>> +			indio_dev->dev.parent,
>> +			"%s-dev%d",
>> +			indio_dev->name,
>> +			indio_dev->id);
>> +		if (!data->drdy_trigger0) {
>> +			ret = -ENOMEM;
>> +			return ret;
>> +		}
>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>>  
>> +		ret = iio_trigger_register(data->drdy_trigger0);
> devm_?

Can't, since no devm_ on kernel 4.4.
I could make another commit to change to devm_, but I don't have target
to test it.

>> +		if (ret) {
>> +			dev_err(&client->dev, "iio trigger register failed\n");
>> +			return ret;
>> +			/* since devm, we can bail out with ret */
> no need to document that...

Ack.

>> +		}
>> +
>> +		/* Set trigger0 as default trigger (and inc refcount) */
>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>> +
>> +		/* Ties irq to trigger producer handler. */
>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>> +			rpr0521_drdy_irq_handler,
>> +			NULL,
>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +			RPR0521_IRQ_NAME,
>> +			indio_dev);
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>> +				client->irq);
>> +			goto err_iio_trigger_put_unregister;
>> +			}
>> +		/*
>> +		 * Now whole pipe from physical interrupt (irq defined by
>> +		 * devicetree to device) to trigger0 output is set up.
>> +		 */
>> +
>> +		/* Trigger consumer setup */
>> +		ret = iio_triggered_buffer_setup(indio_dev,
> devm_?

Can't, since no devm_ on kernel 4.4.
I could make another commit to change to devm_, but I don't have target
to test it.

>> +			&iio_pollfunc_store_time,
>> +			rpr0521_trigger_consumer_handler,
>> +			NULL);		//Use default _ops
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>> +			/* Count on devm to free_irq */
>> +			goto err_iio_trigger_put_unregister;
>> +		}
>> +	}
>> +#endif
>> +
>> +	ret = pm_runtime_set_active(&client->dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "set_active failed\n");
>> +		goto err_iio_unregister_trigger_consumer;
>> +		}
>>  	pm_runtime_enable(&client->dev);
>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>  	pm_runtime_use_autosuspend(&client->dev);
>>  
>>  	return iio_device_register(indio_dev);
>> +
>> +
>> +err_iio_unregister_trigger_consumer:
>> +#ifdef RPR0521_TRIGGERS
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +err_iio_trigger_put_unregister:
>> +	if (indio_dev->trig) {
>> +		iio_trigger_put(indio_dev->trig);
>> +		indio_dev->trig = NULL;
>> +		}
>> +	if (data->drdy_trigger0) {
>> +		iio_trigger_unregister(data->drdy_trigger0);
>> +		data->drdy_trigger0 = NULL;
>> +		}
>> +#endif
>> +	return ret;
>>  }
>>  
>>  static int rpr0521_remove(struct i2c_client *client)
>>  {
>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>  
>>  	iio_device_unregister(indio_dev);
>>  
>> @@ -685,8 +978,21 @@ static int rpr0521_remove(struct i2c_client *client)
>>  	pm_runtime_set_suspended(&client->dev);
>>  	pm_runtime_put_noidle(&client->dev);
>>  
>> -	rpr0521_poweroff(iio_priv(indio_dev));
>> +	rpr0521_poweroff(data);
>>  
>> +#ifdef RPR0521_TRIGGERS
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +	kfree(data->scan_buffer);
>> +	if (indio_dev->trig) {
>> +		iio_trigger_put(indio_dev->trig);
>> +		indio_dev->trig = NULL;
>> +		}
>> +	if (data->drdy_trigger0) {
>> +		iio_trigger_unregister(data->drdy_trigger0);
>> +		data->drdy_trigger0 = NULL;
>> +		}
>> +#endif
>> +	/* Rest of the cleanup is done by devm. */
>>  	return 0;
>>  }
>>  
>>

Re: [PATCH 6/6] iio: light: rpr0521 triggered buffer added

[Jonathan Cameron]

On 07/04/17 12:54, Koivunen, Mikko wrote:
> On 5.4.2017 15:14, Peter Meerwald-Stadler wrote:
>> On Wed, 5 Apr 2017, Mikko Koivunen wrote:
>>
>>> Driver sets up and uses triggered buffer if there is irq defined for device in
>>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
>> actually, there is a nasty #define which shouldn't be needed
>> comments below
> 
> Ack.
> 
>>> Trigger consumer reads rpr0521 data to scan buffer.
>>> Depends on previous commits of _scale and _offset.
>>>
>>> Tested on LeMaker HiKey with AOSP7.1, IIO_HAL and kernel 4.4.
>>> Builds OK with torvalds/linux 4.9
>>> checkpatch.pl OK.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> ---
>>>  drivers/iio/light/rpr0521.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 311 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index fa642b7..22cb097 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -9,9 +9,11 @@
>>>   *
>>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>   *
>>> - * TODO: illuminance channel, PM support, buffer
>>> + * TODO: illuminance channel, PM support
>>>   */
>>>  
>>> +#define RPR0521_TRIGGERS
>> no, don't
> 
> Ack.
> 
>>> +
>>>  #include <linux/module.h>
>>>  #include <linux/init.h>
>>>  #include <linux/i2c.h>
>>> @@ -20,6 +22,12 @@
>>>  #include <linux/acpi.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>> +#ifdef RPR0521_TRIGGERS
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +#endif
>>>  #include <linux/iio/sysfs.h>
>>>  #include <linux/pm_runtime.h>
>>>  
>>> @@ -30,6 +38,7 @@
>>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>>> +#define RPR0521_REG_INTERRUPT		0x4A
>>>  #define RPR0521_PS_OFFSET_LSB		0x53
>>>  #define RPR0521_PS_OFFSET_MSB		0x54
>>>  
>>> @@ -44,16 +53,33 @@
>>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>>  #define RPR0521_PXS_GAIN_SHIFT		4
>>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>>> +#define RPR0521_INTERRUPT_INT_LATCH_MASK	BIT(2)
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>>> +#define RPR0521_INTERRUPT_INT_MODE_MASK		GENMASK(5, 4)
>>>  
>>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_LATCH_DISABLE	BIT(2)
>>> +#define RPR0521_INTERRUPT_INT_LATCH_ENABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>>> +#define RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT	BIT(5)
>>>  
>>>  #define RPR0521_MANUFACT_ID		0xE0
>>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>>  
>>>  #define RPR0521_DRV_NAME		"RPR0521"
>>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>>  
>>>  #define RPR0521_SLEEP_DELAY_MS	2000
>>> @@ -169,6 +195,11 @@ struct rpr0521_data {
>>>  	bool als_dev_en;
>>>  	bool pxs_dev_en;
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +	struct iio_trigger *drdy_trigger0;
>>> +	bool drdy_trigger_on;
>>> +	u16 *scan_buffer;
>>> +#endif
>>>  	/* optimize runtime pm ops - enable device only if needed */
>>>  	bool als_ps_need_en;
>>>  	bool pxs_ps_need_en;
>>> @@ -194,6 +225,13 @@ struct rpr0521_data {
>>>  	.attrs = rpr0521_attributes,
>>>  };
>>>  
>>> +/* Order of the channel data in buffer */
>>> +enum rpr0521_scan_index_order {
>>> +	RPR0521_CHAN_INDEX_PXS,
>>> +	RPR0521_CHAN_INDEX_BOTH,
>>> +	RPR0521_CHAN_INDEX_IR,
>>> +};
>>> +
>>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>>  	{
>>>  		.type = IIO_PROXIMITY,
>>> @@ -202,6 +240,13 @@ struct rpr0521_data {
>>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>> the data is little endian, needs to be specified here
> 
> Ack.
> 
>>
>>> +			.shift = 0,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -211,6 +256,13 @@ struct rpr0521_data {
>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.shift = 0,
>>> +		},
>>>  	},
>>>  	{
>>>  		.type = IIO_INTENSITY,
>>> @@ -220,9 +272,155 @@ struct rpr0521_data {
>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>  			BIT(IIO_CHAN_INFO_SCALE),
>>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.shift = 0,
>>> +		},
>>>  	},
>>>  };
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>> +				   u8 device_mask);
>>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status);
>>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status);
>>> +
>>> +/* IRQ to trigger -handler */
>>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>>> +{
>>> +	struct iio_dev *indio_dev = private;
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +
>>> +	/* Notify trigger0 consumers/subscribers */
>>> +	if (data->drdy_trigger_on)
>>> +		iio_trigger_poll(data->drdy_trigger0);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>>> +
>>> +	ret = regmap_bulk_read(data->regmap,
>>> +		RPR0521_REG_PXS_DATA,
>>> +		&buffer,
>>> +		(3*2)+1);	/* 3 * 16-bit + (discarded) int clear reg. */
>>> +	if (ret < 0)
>>> +		goto done;
>>> +
>>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>>> +		buffer,
>>> +		pf->timestamp);
>>> +done:
>>> +	iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>>> +{
>>> +	int err;
>>> +
>>> +	/* Interrupt after each measurement */
>>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>>> +		RPR0521_PXS_PERSISTENCE_MASK,
>>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> +		RPR0521_INTERRUPT_INT_REASSERT_MASK |
>>> +		RPR0521_INTERRUPT_INT_LATCH_MASK |
>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>>> +		RPR0521_INTERRUPT_INT_LATCH_ENABLE |
>>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>>> +		);
>>> +	err = err || regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> +		RPR0521_INTERRUPT_INT_MODE_MASK,
>>> +		RPR0521_INTERRUPT_INT_PS_OUTSIDE_DETECT);
>>> +	if (err) {
>>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>>> +		return -EBUSY;
>>> +		}
>>> +	return 0;
>>> +}
>>> +
>>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>>> +{
>>> +	return regmap_update_bits(data->regmap, RPR0521_REG_INTERRUPT,
>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_MASK |
>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_MASK,
>>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>>> +				);
>>> +}
>>> +
>>> +/* Trigger -enable/disable */
>>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>>> +	bool enable_drdy)
>>> +{
>>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +	int err;
>>> +
>>> +	if (enable_drdy) {
>>> +		/* ENABLE DRDY */
>>> +		mutex_lock(&data->lock);
>>> +		err = rpr0521_set_power_state(data, true,
>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>> +		mutex_unlock(&data->lock);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_write_int_enable(data);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +	} else {
>>> +		/* DISABLE DRDY */
>>> +		mutex_lock(&data->lock);
>>> +		err = rpr0521_set_power_state(data, false,
>>> +			(RPR0521_MODE_PXS_MASK & RPR0521_MODE_ALS_MASK));
>>> +		mutex_unlock(&data->lock);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_als_enable(data, RPR0521_MODE_ALS_DISABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_DISABLE);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +		err = rpr0521_write_int_disable(data);
>>> +		if (err)
>>> +			goto rpr0521_set_state_err;
>>> +	}
>>> +	data->drdy_trigger_on = enable_drdy;
>>> +	return 0;
>>> +
>>> +rpr0521_set_state_err:
>>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>>> +	return err;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>>> +	.owner = THIS_MODULE,
>>> +	};
>>> +#endif
>>> +
>>>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>>  {
>>>  	int ret;
>>> @@ -454,6 +652,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>>  			return -EINVAL;
>>>  
>>> +		if (iio_buffer_enabled(indio_dev))
>>> +			return -EBUSY;
>>> +
>>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>>  
>>>  		mutex_lock(&data->lock);
>>> @@ -542,11 +743,28 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>>  	}
>>>  }
>>>  
>>> +static int rpr0521_update_scan_mode(struct iio_dev *indio_dev,
>>> +	const unsigned long *scan_mask)
>>> +{
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>> +
>> this is super-ugly; probably just allocate the maximum possible allocation 
>> size?
> 
> Actually this is not needed at all. Receiving buffer is already in
> rpr0521_trigger_consumer_handler() and this buffer is not even used.
> Removing this.
> There is probably need to recheck other drivers too for this pattern.
> 
>>> +	mutex_lock(&data->lock);
>>> +	kfree(data->scan_buffer);
>>> +	data->scan_buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>>> +	mutex_unlock(&data->lock);
>>> +
>>> +	if (data->scan_buffer == NULL)
>>> +		return -ENOMEM;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct iio_info rpr0521_info = {
>>>  	.driver_module	= THIS_MODULE,
>>>  	.read_raw	= rpr0521_read_raw,
>>>  	.write_raw	= rpr0521_write_raw,
>>>  	.attrs		= &rpr0521_attribute_group,
>>> +	.update_scan_mode = &rpr0521_update_scan_mode,
>> no & for functions
> 
> Ack. + removing the function.
> 
> 
>>>  };
>>>  
>>>  static int rpr0521_init(struct rpr0521_data *data)
>>> @@ -664,20 +882,95 @@ static int rpr0521_probe(struct i2c_client *client,
>>>  		return ret;
>>>  	}
>>>  
>>> -	ret = pm_runtime_set_active(&client->dev);
>>> -	if (ret < 0)
>>> -		return ret;
>>> +#ifdef RPR0521_TRIGGERS
>>> +	/* IRQ to trigger setup */
>>> +	if (client->irq) {
>>> +		/* Trigger0 producer setup */
>>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>>> +			indio_dev->dev.parent,
>>> +			"%s-dev%d",
>>> +			indio_dev->name,
>>> +			indio_dev->id);
>>> +		if (!data->drdy_trigger0) {
>>> +			ret = -ENOMEM;
>>> +			return ret;
>>> +		}
>>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>>>  
>>> +		ret = iio_trigger_register(data->drdy_trigger0);
>> devm_?
> 
> Can't, since no devm_ on kernel 4.4.
> I could make another commit to change to devm_, but I don't have target
> to test it.
> 
>>> +		if (ret) {
>>> +			dev_err(&client->dev, "iio trigger register failed\n");
>>> +			return ret;
>>> +			/* since devm, we can bail out with ret */
>> no need to document that...
> 
> Ack.
> 
>>> +		}
>>> +
>>> +		/* Set trigger0 as default trigger (and inc refcount) */
>>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>>> +
>>> +		/* Ties irq to trigger producer handler. */
>>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> +			rpr0521_drdy_irq_handler,
>>> +			NULL,
>>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> +			RPR0521_IRQ_NAME,
>>> +			indio_dev);
>>> +		if (ret < 0) {
>>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>>> +				client->irq);
>>> +			goto err_iio_trigger_put_unregister;
>>> +			}
>>> +		/*
>>> +		 * Now whole pipe from physical interrupt (irq defined by
>>> +		 * devicetree to device) to trigger0 output is set up.
>>> +		 */
>>> +
>>> +		/* Trigger consumer setup */
>>> +		ret = iio_triggered_buffer_setup(indio_dev,
>> devm_?
> 
> Can't, since no devm_ on kernel 4.4.
> I could make another commit to change to devm_, but I don't have target
> to test it.
OK. We can leave it for someone else to clean up in future.
> 
>>> +			&iio_pollfunc_store_time,
>>> +			rpr0521_trigger_consumer_handler,
>>> +			NULL);		//Use default _ops
>>> +		if (ret < 0) {
>>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>> +			/* Count on devm to free_irq */
>>> +			goto err_iio_trigger_put_unregister;
>>> +		}
>>> +	}
>>> +#endif
>>> +
>>> +	ret = pm_runtime_set_active(&client->dev);
>>> +	if (ret < 0) {
>>> +		dev_err(&client->dev, "set_active failed\n");
>>> +		goto err_iio_unregister_trigger_consumer;
>>> +		}
>>>  	pm_runtime_enable(&client->dev);
>>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>>  	pm_runtime_use_autosuspend(&client->dev);
>>>  
>>>  	return iio_device_register(indio_dev);
>>> +
>>> +
>>> +err_iio_unregister_trigger_consumer:
>>> +#ifdef RPR0521_TRIGGERS
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>> +err_iio_trigger_put_unregister:
>>> +	if (indio_dev->trig) {
>>> +		iio_trigger_put(indio_dev->trig);
>>> +		indio_dev->trig = NULL;
>>> +		}
>>> +	if (data->drdy_trigger0) {
>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>> +		data->drdy_trigger0 = NULL;
>>> +		}
>>> +#endif
>>> +	return ret;
>>>  }
>>>  
>>>  static int rpr0521_remove(struct i2c_client *client)
>>>  {
>>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>>>  
>>>  	iio_device_unregister(indio_dev);
>>>  
>>> @@ -685,8 +978,21 @@ static int rpr0521_remove(struct i2c_client *client)
>>>  	pm_runtime_set_suspended(&client->dev);
>>>  	pm_runtime_put_noidle(&client->dev);
>>>  
>>> -	rpr0521_poweroff(iio_priv(indio_dev));
>>> +	rpr0521_poweroff(data);
>>>  
>>> +#ifdef RPR0521_TRIGGERS
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>> +	kfree(data->scan_buffer);
>>> +	if (indio_dev->trig) {
>>> +		iio_trigger_put(indio_dev->trig);
>>> +		indio_dev->trig = NULL;
>>> +		}
>>> +	if (data->drdy_trigger0) {
>>> +		iio_trigger_unregister(data->drdy_trigger0);
>>> +		data->drdy_trigger0 = NULL;
>>> +		}
>>> +#endif
>>> +	/* Rest of the cleanup is done by devm. */
>>>  	return 0;
>>>  }
>>>  
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/6] iio: light: rpr0521 disable sensor -bugfix

[Mikko Koivunen]

Sensor was marked enabled on each call even if the call was for disabling
sensor.
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 7de0f39..c15529b 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -197,8 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->als_dev_en = true;
-
+	if (status & RPR0521_MODE_ALS_MASK)
+		data->als_dev_en = true;
+	else
+		data->als_dev_en = false;
 	return 0;
 }
 
@@ -212,7 +214,10 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->pxs_dev_en = true;
+	if (status & RPR0521_MODE_PXS_MASK)
+		data->pxs_dev_en = true;
+	else
+		data->pxs_dev_en = false;
 
 	return 0;
 }
-- 
1.9.1

[PATCH 4/6] iio: light: rpr0521 proximity offset read/write added

[Mikko Koivunen]

Added sysfs read/write proximity offset feature. Offset is read/write from
sensor registers. Values are proximity raw 10-bit values. After applying
offset value, output values will be (measured_raw - offset_value). Output
values are unsigned so offset value doesn't make output negative.

Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds OK with torvalds/linux 4.9
checkpatch.pl OK.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 13e2f9a..579bf6d 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -30,6 +30,9 @@
 #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
+#define RPR0521_PS_OFFSET_LSB		0x53
+#define RPR0521_PS_OFFSET_MSB		0x54
+
 #define RPR0521_REG_ID			0x92
 
 #define RPR0521_MODE_ALS_MASK		BIT(7)
@@ -214,7 +217,9 @@ struct rpr0521_data {
 		.type = IIO_PROXIMITY,
 		.address = RPR0521_CHAN_PXS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
 			BIT(IIO_CHAN_INFO_SCALE),
+
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
@@ -400,6 +405,42 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
 		i);
 }
 
+int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
+{
+	int ret;
+	u8 buffer[2];
+
+	ret = regmap_bulk_read(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, 2);
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
+		return ret;
+	}
+	*offset = buffer[0] + (buffer[1] << 8);
+
+	return ret;
+}
+
+int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
+{
+	int ret;
+	u8 buffer[2];
+
+	buffer[0] = offset & 0xff;
+	buffer[1] = (offset >> 8) & (BIT(0) | BIT(1));
+
+	ret = regmap_raw_write(data->regmap,
+		RPR0521_PS_OFFSET_LSB, &buffer, 2);
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -456,6 +497,14 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_read_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -483,6 +532,12 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&data->lock);
 		return ret;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_write_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.9.1