[PATCH 0/6] iio: sx9500: power and GPIO fixes

[PATCH 0/6] iio: sx9500: power and GPIO fixes

[Vlad Dogaru]

This series addresses a few separate issues of the driver, the main one
being power usage.

Also featured: GPIO pin renaming to something less silly than
"sx9500_gpio-gpios", using a reset pin if available, and finally a minor
formatting nit that kept bugging me.

Vlad Dogaru (6):
  iio: sx9500: add power management
  iio: sx9500: optimize power usage
  iio: sx9500: rename GPIO interrupt pin
  iio: sx9500: refactor GPIO interrupt code
  iio: sx9500: add GPIO reset pin
  iio: sx9500: fix formatting

 drivers/iio/proximity/sx9500.c | 479 ++++++++++++++++++++++++++++++++---------
 1 file changed, 381 insertions(+), 98 deletions(-)

-- 
1.9.1

[PATCH 1/6] iio: sx9500: add power management

[Vlad Dogaru]

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 47 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 74dff4e..f62d0b6 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/regmap.h>
+#include <linux/pm.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -89,6 +90,8 @@ struct sx9500_data {
 	bool event_enabled[SX9500_NUM_CHANNELS];
 	bool trigger_enabled;
 	u16 *buffer;
+	/* Remember enabled channels and sample rate during suspend. */
+	unsigned int suspend_ctrl0;
 };
 
 static const struct iio_event_spec sx9500_events[] = {
@@ -724,6 +727,49 @@ static int sx9500_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int sx9500_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, SX9500_REG_PROX_CTRL0,
+			  &data->suspend_ctrl0);
+	if (ret < 0)
+		goto out;
+
+	/*
+	 * Scan period doesn't matter because when all the sensors are
+	 * deactivated the device is in sleep mode.
+	 */
+	ret = regmap_write(data->regmap, SX9500_REG_PROX_CTRL0, 0);
+
+out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int sx9500_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_write(data->regmap, SX9500_REG_PROX_CTRL0,
+			   data->suspend_ctrl0);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops sx9500_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sx9500_suspend, sx9500_resume)
+};
+
 static const struct acpi_device_id sx9500_acpi_match[] = {
 	{"SSX9500", 0},
 	{ },
@@ -740,6 +786,7 @@ static struct i2c_driver sx9500_driver = {
 	.driver = {
 		.name	= SX9500_DRIVER_NAME,
 		.acpi_match_table = ACPI_PTR(sx9500_acpi_match),
+		.pm = &sx9500_pm_ops,
 	},
 	.probe		= sx9500_probe,
 	.remove		= sx9500_remove,
-- 
1.9.1

Re: [PATCH 1/6] iio: sx9500: add power management

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
Applied to the togreg branch of iio.git - initially pushed out as testing for the autobuilders to play.

J
> ---
>  drivers/iio/proximity/sx9500.c | 47 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 74dff4e..f62d0b6 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -18,6 +18,7 @@
>  #include <linux/acpi.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/regmap.h>
> +#include <linux/pm.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
> @@ -89,6 +90,8 @@ struct sx9500_data {
>  	bool event_enabled[SX9500_NUM_CHANNELS];
>  	bool trigger_enabled;
>  	u16 *buffer;
> +	/* Remember enabled channels and sample rate during suspend. */
> +	unsigned int suspend_ctrl0;
>  };
>  
>  static const struct iio_event_spec sx9500_events[] = {
> @@ -724,6 +727,49 @@ static int sx9500_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int sx9500_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	ret = regmap_read(data->regmap, SX9500_REG_PROX_CTRL0,
> +			  &data->suspend_ctrl0);
> +	if (ret < 0)
> +		goto out;
> +
> +	/*
> +	 * Scan period doesn't matter because when all the sensors are
> +	 * deactivated the device is in sleep mode.
> +	 */
> +	ret = regmap_write(data->regmap, SX9500_REG_PROX_CTRL0, 0);
> +
> +out:
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}
> +
> +static int sx9500_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	ret = regmap_write(data->regmap, SX9500_REG_PROX_CTRL0,
> +			   data->suspend_ctrl0);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops sx9500_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(sx9500_suspend, sx9500_resume)
> +};
> +
>  static const struct acpi_device_id sx9500_acpi_match[] = {
>  	{"SSX9500", 0},
>  	{ },
> @@ -740,6 +786,7 @@ static struct i2c_driver sx9500_driver = {
>  	.driver = {
>  		.name	= SX9500_DRIVER_NAME,
>  		.acpi_match_table = ACPI_PTR(sx9500_acpi_match),
> +		.pm = &sx9500_pm_ops,
>  	},
>  	.probe		= sx9500_probe,
>  	.remove		= sx9500_remove,
> 

[PATCH 2/6] iio: sx9500: optimize power usage

[Vlad Dogaru]

In the interest of lowering power usage, we only activate the proximity
channels and interrupts that we are currently using.

For raw reads, we activate the corresponding channel and the data ready
interrupt and wait for the interrupt to trigger.  Thus, it is no longer
optional to register an irq.

The following types of usage patterns may overlap:

* raw proximity reads (need a single data ready interrupt)
* trigger usage (needs data ready interrupts as long as active)
* proximity events (need near/far interrupts)
* triggered buffer reads (don't need any interrupts, but are usually
coupled with our own trigger.

To mitigate all possible patterns, we implement usage counting for all
the resources used: data ready interrupts, near/far interrupts and
individual channels.

The device enters sleep mode as documented in the data sheet when its
buffer, trigger and events are disabled, and no raw reads are currently
running.

Because of this new usage pattern, it is important that we give the
device a chance to perform an initial compensation for all its channels
at probe time.

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 376 +++++++++++++++++++++++++++++++++--------
 1 file changed, 302 insertions(+), 74 deletions(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index f62d0b6..94c56f3 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -19,6 +19,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/regmap.h>
 #include <linux/pm.h>
+#include <linux/delay.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -74,6 +75,7 @@
 #define SX9500_CONVDONE_IRQ		BIT(3)
 
 #define SX9500_PROXSTAT_SHIFT		4
+#define SX9500_COMPSTAT_MASK		GENMASK(3, 0)
 
 #define SX9500_NUM_CHANNELS		4
 
@@ -92,6 +94,9 @@ struct sx9500_data {
 	u16 *buffer;
 	/* Remember enabled channels and sample rate during suspend. */
 	unsigned int suspend_ctrl0;
+	struct completion completion;
+	int data_rdy_users, close_far_users;
+	int channel_users[SX9500_NUM_CHANNELS];
 };
 
 static const struct iio_event_spec sx9500_events[] = {
@@ -194,7 +199,67 @@ static const struct regmap_config sx9500_regmap_config = {
 	.volatile_table = &sx9500_volatile_regs,
 };
 
-static int sx9500_read_proximity(struct sx9500_data *data,
+static int sx9500_inc_users(struct sx9500_data *data, int *counter,
+			    unsigned int reg, unsigned int bitmask)
+{
+	(*counter)++;
+	if (*counter != 1)
+		/* Bit is already active, nothing to do. */
+		return 0;
+
+	return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
+}
+
+static int sx9500_dec_users(struct sx9500_data *data, int *counter,
+			    unsigned int reg, unsigned int bitmask)
+{
+	(*counter)--;
+	if (*counter != 0)
+		/* There are more users, do not deactivate. */
+		return 0;
+
+	return regmap_update_bits(data->regmap, reg, bitmask, 0);
+}
+
+static int sx9500_inc_chan_users(struct sx9500_data *data, int chan)
+{
+	return sx9500_inc_users(data, &data->channel_users[chan],
+				SX9500_REG_PROX_CTRL0, BIT(chan));
+}
+
+static int sx9500_dec_chan_users(struct sx9500_data *data, int chan)
+{
+	return sx9500_dec_users(data, &data->channel_users[chan],
+				SX9500_REG_PROX_CTRL0, BIT(chan));
+}
+
+static int sx9500_inc_data_rdy_users(struct sx9500_data *data)
+{
+	return sx9500_inc_users(data, &data->data_rdy_users,
+				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
+}
+
+static int sx9500_dec_data_rdy_users(struct sx9500_data *data)
+{
+	return sx9500_dec_users(data, &data->data_rdy_users,
+				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
+}
+
+static int sx9500_inc_close_far_users(struct sx9500_data *data)
+{
+	return sx9500_inc_users(data, &data->close_far_users,
+				SX9500_REG_IRQ_MSK,
+				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
+}
+
+static int sx9500_dec_close_far_users(struct sx9500_data *data)
+{
+	return sx9500_dec_users(data, &data->close_far_users,
+				SX9500_REG_IRQ_MSK,
+				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
+}
+
+static int sx9500_read_prox_data(struct sx9500_data *data,
 				 const struct iio_chan_spec *chan,
 				 int *val)
 {
@@ -203,15 +268,66 @@ static int sx9500_read_proximity(struct sx9500_data *data,
 
 	ret = regmap_write(data->regmap, SX9500_REG_SENSOR_SEL, chan->channel);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = regmap_bulk_read(data->regmap, SX9500_REG_USE_MSB, &regval, 2);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	*val = 32767 - (s16)be16_to_cpu(regval);
+	ret = IIO_VAL_INT;
+
+out:
+	return ret;
+}
+
+static int sx9500_read_proximity(struct sx9500_data *data,
+				 const struct iio_chan_spec *chan,
+				 int *val)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	ret = sx9500_inc_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = sx9500_inc_data_rdy_users(data);
+	if (ret < 0)
+		goto out_dec_chan;
+
+	mutex_unlock(&data->mutex);
+
+	ret = wait_for_completion_interruptible(&data->completion);
+	if (ret < 0)
+		return ret;
 
-	return IIO_VAL_INT;
+	mutex_lock(&data->mutex);
+
+	ret = sx9500_read_prox_data(data, chan, val);
+	if (ret < 0)
+		goto out;
+
+	ret = sx9500_dec_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = sx9500_dec_data_rdy_users(data);
+	if (ret < 0)
+		goto out;
+
+	ret = IIO_VAL_INT;
+
+	goto out;
+
+out_dec_chan:
+	sx9500_dec_chan_users(data, chan->channel);
+out:
+	mutex_unlock(&data->mutex);
+	reinit_completion(&data->completion);
+
+	return ret;
 }
 
 static int sx9500_read_samp_freq(struct sx9500_data *data,
@@ -239,7 +355,6 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct sx9500_data *data = iio_priv(indio_dev);
-	int ret;
 
 	switch (chan->type) {
 	case IIO_PROXIMITY:
@@ -247,10 +362,7 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
 		case IIO_CHAN_INFO_RAW:
 			if (iio_buffer_enabled(indio_dev))
 				return -EBUSY;
-			mutex_lock(&data->mutex);
-			ret = sx9500_read_proximity(data, chan, val);
-			mutex_unlock(&data->mutex);
-			return ret;
+			return sx9500_read_proximity(data, chan, val);
 		case IIO_CHAN_INFO_SAMP_FREQ:
 			return sx9500_read_samp_freq(data, val, val2);
 		default:
@@ -321,28 +433,16 @@ static irqreturn_t sx9500_irq_handler(int irq, void *private)
 	return IRQ_WAKE_THREAD;
 }
 
-static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
+static void sx9500_push_events(struct iio_dev *indio_dev)
 {
-	struct iio_dev *indio_dev = private;
-	struct sx9500_data *data = iio_priv(indio_dev);
 	int ret;
 	unsigned int val, chan;
-
-	mutex_lock(&data->mutex);
-
-	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "i2c transfer error in irq\n");
-		goto out;
-	}
-
-	if (!(val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ)))
-		goto out;
+	struct sx9500_data *data = iio_priv(indio_dev);
 
 	ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
 	if (ret < 0) {
 		dev_err(&data->client->dev, "i2c transfer error in irq\n");
-		goto out;
+		return;
 	}
 
 	val >>= SX9500_PROXSTAT_SHIFT;
@@ -357,15 +457,34 @@ static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
 			/* No change on this channel. */
 			continue;
 
-		dir = new_prox ? IIO_EV_DIR_FALLING :
-			IIO_EV_DIR_RISING;
-		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
-					  chan,
-					  IIO_EV_TYPE_THRESH,
-					  dir);
+		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
+					  IIO_EV_TYPE_THRESH, dir);
 		iio_push_event(indio_dev, ev, iio_get_time_ns());
 		data->prox_stat[chan] = new_prox;
 	}
+}
+
+static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int val;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "i2c transfer error in irq\n");
+		goto out;
+	}
+
+	if (val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ))
+		sx9500_push_events(indio_dev);
+
+	if (val & SX9500_CONVDONE_IRQ)
+		complete_all(&data->completion);
 
 out:
 	mutex_unlock(&data->mutex);
@@ -394,9 +513,7 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
 				     int state)
 {
 	struct sx9500_data *data = iio_priv(indio_dev);
-	int ret, i;
-	bool any_active = false;
-	unsigned int irqmask;
+	int ret;
 
 	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
 	    dir != IIO_EV_DIR_EITHER)
@@ -404,24 +521,32 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
 
 	mutex_lock(&data->mutex);
 
-	data->event_enabled[chan->channel] = state;
+	if (state == 1) {
+		ret = sx9500_inc_chan_users(data, chan->channel);
+		if (ret < 0)
+			goto out_unlock;
+		ret = sx9500_inc_close_far_users(data);
+		if (ret < 0)
+			goto out_undo_chan;
+	} else {
+		ret = sx9500_dec_chan_users(data, chan->channel);
+		if (ret < 0)
+			goto out_unlock;
+		ret = sx9500_dec_close_far_users(data);
+		if (ret < 0)
+			goto out_undo_chan;
+	}
 
-	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
-		if (data->event_enabled[i]) {
-			any_active = true;
-			break;
-		}
+	data->event_enabled[chan->channel] = state;
+	goto out_unlock;
 
-	irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ;
-	if (any_active)
-		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
-					 irqmask, irqmask);
+out_undo_chan:
+	if (state == 1)
+		sx9500_dec_chan_users(data, chan->channel);
 	else
-		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
-					 irqmask, 0);
-
+		sx9500_inc_chan_users(data, chan->channel);
+out_unlock:
 	mutex_unlock(&data->mutex);
-
 	return ret;
 }
 
@@ -472,12 +597,16 @@ static int sx9500_set_trigger_state(struct iio_trigger *trig,
 
 	mutex_lock(&data->mutex);
 
-	ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
-				 SX9500_CONVDONE_IRQ,
-				 state ? SX9500_CONVDONE_IRQ : 0);
-	if (ret == 0)
-		data->trigger_enabled = state;
+	if (state)
+		ret = sx9500_inc_data_rdy_users(data);
+	else
+		ret = sx9500_dec_data_rdy_users(data);
+	if (ret < 0)
+		goto out;
+
+	data->trigger_enabled = state;
 
+out:
 	mutex_unlock(&data->mutex);
 
 	return ret;
@@ -499,7 +628,7 @@ static irqreturn_t sx9500_trigger_handler(int irq, void *private)
 
 	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
 			 indio_dev->masklength) {
-		ret = sx9500_read_proximity(data, &indio_dev->channels[bit],
+		ret = sx9500_read_prox_data(data, &indio_dev->channels[bit],
 					    &val);
 		if (ret < 0)
 			goto out;
@@ -518,6 +647,62 @@ out:
 	return IRQ_HANDLED;
 }
 
+static int sx9500_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int ret, i;
+
+	mutex_lock(&data->mutex);
+
+	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
+		if (test_bit(i, indio_dev->active_scan_mask)) {
+			ret = sx9500_inc_chan_users(data, i);
+			if (ret)
+				break;
+		}
+
+	if (ret)
+		for (i = i - 1; i >= 0; i--)
+			if (test_bit(i, indio_dev->active_scan_mask))
+				sx9500_dec_chan_users(data, i);
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int sx9500_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int ret, i;
+
+	iio_triggered_buffer_predisable(indio_dev);
+
+	mutex_lock(&data->mutex);
+
+	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
+		if (test_bit(i, indio_dev->active_scan_mask)) {
+			ret = sx9500_dec_chan_users(data, i);
+			if (ret)
+				break;
+		}
+
+	if (ret)
+		for (i = i - 1; i >= 0; i--)
+			if (test_bit(i, indio_dev->active_scan_mask))
+				sx9500_inc_chan_users(data, i);
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops sx9500_buffer_setup_ops = {
+	.preenable = sx9500_buffer_preenable,
+	.postenable = iio_triggered_buffer_postenable,
+	.predisable = sx9500_buffer_predisable,
+};
+
 struct sx9500_reg_default {
 	u8 reg;
 	u8 def;
@@ -573,11 +758,44 @@ static const struct sx9500_reg_default sx9500_default_regs[] = {
 	},
 	{
 		.reg = SX9500_REG_PROX_CTRL0,
-		/* Scan period: 30ms, all sensors enabled. */
-		.def = 0x0f,
+		/* Scan period: 30ms, all sensors disabled. */
+		.def = 0x00,
 	},
 };
 
+/* Activate all channels and perform an initial compensation. */
+static int sx9500_init_compensation(struct iio_dev *indio_dev)
+{
+	struct sx9500_data *data = iio_priv(indio_dev);
+	int i, ret;
+	unsigned int val;
+
+	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
+				 GENMASK(SX9500_NUM_CHANNELS, 0),
+				 GENMASK(SX9500_NUM_CHANNELS, 0));
+	if (ret < 0)
+		return ret;
+
+	for (i = 10; i >= 0; i--) {
+		usleep_range(10000, 20000);
+		ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
+		if (ret < 0)
+			goto out;
+		if (!(val & SX9500_COMPSTAT_MASK))
+			break;
+	}
+
+	if (i < 0) {
+		dev_err(&data->client->dev, "initial compensation timed out");
+		ret = -ETIMEDOUT;
+	}
+
+out:
+	regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
+			   GENMASK(SX9500_NUM_CHANNELS, 0), 0);
+	return ret;
+}
+
 static int sx9500_init_device(struct iio_dev *indio_dev)
 {
 	struct sx9500_data *data = iio_priv(indio_dev);
@@ -605,6 +823,10 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
 			return ret;
 	}
 
+	ret = sx9500_init_compensation(indio_dev);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
@@ -652,6 +874,7 @@ static int sx9500_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	data->client = client;
 	mutex_init(&data->mutex);
+	init_completion(&data->completion);
 	data->trigger_enabled = false;
 
 	data->regmap = devm_regmap_init_i2c(client, &sx9500_regmap_config);
@@ -671,30 +894,35 @@ static int sx9500_probe(struct i2c_client *client,
 	if (client->irq <= 0)
 		client->irq = sx9500_gpio_probe(client, data);
 
-	if (client->irq > 0) {
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
-				sx9500_irq_handler, sx9500_irq_thread_handler,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				SX9500_IRQ_NAME, indio_dev);
-		if (ret < 0)
-			return ret;
+	if (client->irq <= 0) {
+		dev_err(&client->dev, "no valid irq found\n");
+		return -EINVAL;
+	}
+
+	ret = devm_request_threaded_irq(&client->dev, client->irq,
+					sx9500_irq_handler,
+					sx9500_irq_thread_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					SX9500_IRQ_NAME, indio_dev);
+	if (ret < 0)
+		return ret;
 
-		data->trig = devm_iio_trigger_alloc(&client->dev,
-				"%s-dev%d", indio_dev->name, indio_dev->id);
-		if (!data->trig)
-			return -ENOMEM;
+	data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+					    indio_dev->name, indio_dev->id);
+	if (!data->trig)
+		return -ENOMEM;
 
-		data->trig->dev.parent = &client->dev;
-		data->trig->ops = &sx9500_trigger_ops;
-		iio_trigger_set_drvdata(data->trig, indio_dev);
+	data->trig->dev.parent = &client->dev;
+	data->trig->ops = &sx9500_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, indio_dev);
 
-		ret = iio_trigger_register(data->trig);
-		if (ret)
-			return ret;
-	}
+	ret = iio_trigger_register(data->trig);
+	if (ret)
+		return ret;
 
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-					 sx9500_trigger_handler, NULL);
+					 sx9500_trigger_handler,
+					 &sx9500_buffer_setup_ops);
 	if (ret < 0)
 		goto out_trigger_unregister;
 
-- 
1.9.1

Re: [PATCH 2/6] iio: sx9500: optimize power usage

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> In the interest of lowering power usage, we only activate the proximity
> channels and interrupts that we are currently using.
> 
> For raw reads, we activate the corresponding channel and the data ready
> interrupt and wait for the interrupt to trigger.  Thus, it is no longer
> optional to register an irq.
This change bothers me.  We may well have people with the device that
don't have the interrupt wired and if they are already using the driver
then we have a resulting regression.

Any chance we could have an alternative of a registering polling or similar
to identify when the interrupt would have occured?

> 
> The following types of usage patterns may overlap:
> 
> * raw proximity reads (need a single data ready interrupt)
We often avoid this complexity by just failing raw reads when
the buffer is running but the solution you have here is pretty
elegant.
> * trigger usage (needs data ready interrupts as long as active)
> * proximity events (need near/far interrupts)
> * triggered buffer reads (don't need any interrupts, but are usually
> coupled with our own trigger.

An alternative, perhaps more standard method, would be to handle this via
an interrupt chip and let that infrastructure be responisble for the
underlying real interrupts. I'm not sure which approach would be more
complex though so perhaps, though I'd have gone that way, what you have
here is the best way...

> 
> To mitigate all possible patterns, we implement usage counting for all
> the resources used: data ready interrupts, near/far interrupts and
> individual channels.
> 
> The device enters sleep mode as documented in the data sheet when its
> buffer, trigger and events are disabled, and no raw reads are currently
> running.
> 
> Because of this new usage pattern, it is important that we give the
> device a chance to perform an initial compensation for all its channels
> at probe time.
> 
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> ---
>  drivers/iio/proximity/sx9500.c | 376 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 302 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index f62d0b6..94c56f3 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -19,6 +19,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/regmap.h>
>  #include <linux/pm.h>
> +#include <linux/delay.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
> @@ -74,6 +75,7 @@
>  #define SX9500_CONVDONE_IRQ		BIT(3)
>  
>  #define SX9500_PROXSTAT_SHIFT		4
> +#define SX9500_COMPSTAT_MASK		GENMASK(3, 0)
>  
>  #define SX9500_NUM_CHANNELS		4
>  
> @@ -92,6 +94,9 @@ struct sx9500_data {
>  	u16 *buffer;
>  	/* Remember enabled channels and sample rate during suspend. */
>  	unsigned int suspend_ctrl0;
> +	struct completion completion;
> +	int data_rdy_users, close_far_users;
> +	int channel_users[SX9500_NUM_CHANNELS];
>  };
>  
>  static const struct iio_event_spec sx9500_events[] = {
> @@ -194,7 +199,67 @@ static const struct regmap_config sx9500_regmap_config = {
>  	.volatile_table = &sx9500_volatile_regs,
>  };
>  
> -static int sx9500_read_proximity(struct sx9500_data *data,
> +static int sx9500_inc_users(struct sx9500_data *data, int *counter,
> +			    unsigned int reg, unsigned int bitmask)
> +{
> +	(*counter)++;
> +	if (*counter != 1)
> +		/* Bit is already active, nothing to do. */
> +		return 0;
> +
> +	return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
> +}
> +
> +static int sx9500_dec_users(struct sx9500_data *data, int *counter,
> +			    unsigned int reg, unsigned int bitmask)
> +{
> +	(*counter)--;
> +	if (*counter != 0)
> +		/* There are more users, do not deactivate. */
> +		return 0;
> +
> +	return regmap_update_bits(data->regmap, reg, bitmask, 0);
> +}
> +
> +static int sx9500_inc_chan_users(struct sx9500_data *data, int chan)
> +{
> +	return sx9500_inc_users(data, &data->channel_users[chan],
> +				SX9500_REG_PROX_CTRL0, BIT(chan));
> +}
> +
> +static int sx9500_dec_chan_users(struct sx9500_data *data, int chan)
> +{
> +	return sx9500_dec_users(data, &data->channel_users[chan],
> +				SX9500_REG_PROX_CTRL0, BIT(chan));
> +}
> +
> +static int sx9500_inc_data_rdy_users(struct sx9500_data *data)
> +{
> +	return sx9500_inc_users(data, &data->data_rdy_users,
> +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
> +}
> +
> +static int sx9500_dec_data_rdy_users(struct sx9500_data *data)
> +{
> +	return sx9500_dec_users(data, &data->data_rdy_users,
> +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
> +}
> +
> +static int sx9500_inc_close_far_users(struct sx9500_data *data)
> +{
> +	return sx9500_inc_users(data, &data->close_far_users,
> +				SX9500_REG_IRQ_MSK,
> +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
> +}
> +
> +static int sx9500_dec_close_far_users(struct sx9500_data *data)
> +{
> +	return sx9500_dec_users(data, &data->close_far_users,
> +				SX9500_REG_IRQ_MSK,
> +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
> +}
> +
> +static int sx9500_read_prox_data(struct sx9500_data *data,
>  				 const struct iio_chan_spec *chan,
>  				 int *val)
>  {
> @@ -203,15 +268,66 @@ static int sx9500_read_proximity(struct sx9500_data *data,
>  
>  	ret = regmap_write(data->regmap, SX9500_REG_SENSOR_SEL, chan->channel);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
>  	ret = regmap_bulk_read(data->regmap, SX9500_REG_USE_MSB, &regval, 2);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
>  	*val = 32767 - (s16)be16_to_cpu(regval);
> +	ret = IIO_VAL_INT;
> +
> +out:
> +	return ret;
> +}
> +
> +static int sx9500_read_proximity(struct sx9500_data *data,
> +				 const struct iio_chan_spec *chan,
> +				 int *val)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +
> +	ret = sx9500_inc_chan_users(data, chan->channel);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = sx9500_inc_data_rdy_users(data);
> +	if (ret < 0)
> +		goto out_dec_chan;
> +
> +	mutex_unlock(&data->mutex);
> +
> +	ret = wait_for_completion_interruptible(&data->completion);
> +	if (ret < 0)
> +		return ret;
>  
> -	return IIO_VAL_INT;
> +	mutex_lock(&data->mutex);
> +
> +	ret = sx9500_read_prox_data(data, chan, val);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = sx9500_dec_chan_users(data, chan->channel);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = sx9500_dec_data_rdy_users(data);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = IIO_VAL_INT;
> +
> +	goto out;
> +
> +out_dec_chan:
> +	sx9500_dec_chan_users(data, chan->channel);
> +out:
> +	mutex_unlock(&data->mutex);
> +	reinit_completion(&data->completion);
> +
> +	return ret;
>  }
>  
>  static int sx9500_read_samp_freq(struct sx9500_data *data,
> @@ -239,7 +355,6 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
>  			   int *val, int *val2, long mask)
>  {
>  	struct sx9500_data *data = iio_priv(indio_dev);
> -	int ret;
>  
>  	switch (chan->type) {
>  	case IIO_PROXIMITY:
> @@ -247,10 +362,7 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
>  		case IIO_CHAN_INFO_RAW:
>  			if (iio_buffer_enabled(indio_dev))
>  				return -EBUSY;
> -			mutex_lock(&data->mutex);
> -			ret = sx9500_read_proximity(data, chan, val);
> -			mutex_unlock(&data->mutex);
> -			return ret;
> +			return sx9500_read_proximity(data, chan, val);
>  		case IIO_CHAN_INFO_SAMP_FREQ:
>  			return sx9500_read_samp_freq(data, val, val2);
>  		default:
> @@ -321,28 +433,16 @@ static irqreturn_t sx9500_irq_handler(int irq, void *private)
>  	return IRQ_WAKE_THREAD;
>  }
>  
> -static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
> +static void sx9500_push_events(struct iio_dev *indio_dev)
>  {
> -	struct iio_dev *indio_dev = private;
> -	struct sx9500_data *data = iio_priv(indio_dev);
>  	int ret;
>  	unsigned int val, chan;
> -
> -	mutex_lock(&data->mutex);
> -
> -	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> -		goto out;
> -	}
> -
> -	if (!(val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ)))
> -		goto out;
> +	struct sx9500_data *data = iio_priv(indio_dev);
>  
>  	ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> -		goto out;
> +		return;
>  	}
>  
>  	val >>= SX9500_PROXSTAT_SHIFT;
> @@ -357,15 +457,34 @@ static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
>  			/* No change on this channel. */
>  			continue;
>  
> -		dir = new_prox ? IIO_EV_DIR_FALLING :
> -			IIO_EV_DIR_RISING;
> -		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
> -					  chan,
> -					  IIO_EV_TYPE_THRESH,
> -					  dir);
> +		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
> +					  IIO_EV_TYPE_THRESH, dir);
>  		iio_push_event(indio_dev, ev, iio_get_time_ns());
>  		data->prox_stat[chan] = new_prox;
>  	}
> +}
> +
> +static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int val;
> +
> +	mutex_lock(&data->mutex);
> +
> +	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> +		goto out;
> +	}
> +
> +	if (val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ))
> +		sx9500_push_events(indio_dev);
> +
> +	if (val & SX9500_CONVDONE_IRQ)
> +		complete_all(&data->completion);
>  
>  out:
>  	mutex_unlock(&data->mutex);
> @@ -394,9 +513,7 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
>  				     int state)
>  {
>  	struct sx9500_data *data = iio_priv(indio_dev);
> -	int ret, i;
> -	bool any_active = false;
> -	unsigned int irqmask;
> +	int ret;
>  
>  	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
>  	    dir != IIO_EV_DIR_EITHER)
> @@ -404,24 +521,32 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&data->mutex);
>  
> -	data->event_enabled[chan->channel] = state;
> +	if (state == 1) {
Is if (state) not sufficient?
> +		ret = sx9500_inc_chan_users(data, chan->channel);
> +		if (ret < 0)
> +			goto out_unlock;
> +		ret = sx9500_inc_close_far_users(data);
> +		if (ret < 0)
> +			goto out_undo_chan;
> +	} else {
> +		ret = sx9500_dec_chan_users(data, chan->channel);
> +		if (ret < 0)
> +			goto out_unlock;
> +		ret = sx9500_dec_close_far_users(data);
> +		if (ret < 0)
> +			goto out_undo_chan;
> +	}
>  
> -	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> -		if (data->event_enabled[i]) {
> -			any_active = true;
> -			break;
> -		}
> +	data->event_enabled[chan->channel] = state;
> +	goto out_unlock;
>  
> -	irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ;
> -	if (any_active)
> -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> -					 irqmask, irqmask);
> +out_undo_chan:
> +	if (state == 1)
> +		sx9500_dec_chan_users(data, chan->channel);
>  	else
> -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> -					 irqmask, 0);
> -
> +		sx9500_inc_chan_users(data, chan->channel);
> +out_unlock:
>  	mutex_unlock(&data->mutex);
> -
>  	return ret;
>  }
>  
> @@ -472,12 +597,16 @@ static int sx9500_set_trigger_state(struct iio_trigger *trig,
>  
>  	mutex_lock(&data->mutex);
>  
> -	ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> -				 SX9500_CONVDONE_IRQ,
> -				 state ? SX9500_CONVDONE_IRQ : 0);
> -	if (ret == 0)
> -		data->trigger_enabled = state;
> +	if (state)
> +		ret = sx9500_inc_data_rdy_users(data);
> +	else
> +		ret = sx9500_dec_data_rdy_users(data);
> +	if (ret < 0)
> +		goto out;
> +
> +	data->trigger_enabled = state;
>  
> +out:
>  	mutex_unlock(&data->mutex);
>  
>  	return ret;
> @@ -499,7 +628,7 @@ static irqreturn_t sx9500_trigger_handler(int irq, void *private)
>  
>  	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
>  			 indio_dev->masklength) {
> -		ret = sx9500_read_proximity(data, &indio_dev->channels[bit],
> +		ret = sx9500_read_prox_data(data, &indio_dev->channels[bit],
>  					    &val);
>  		if (ret < 0)
>  			goto out;
> @@ -518,6 +647,62 @@ out:
>  	return IRQ_HANDLED;
>  }
>  
> +static int sx9500_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +
> +	mutex_lock(&data->mutex);
> +
> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> +		if (test_bit(i, indio_dev->active_scan_mask)) {
> +			ret = sx9500_inc_chan_users(data, i);
> +			if (ret)
> +				break;
> +		}
> +
> +	if (ret)
> +		for (i = i - 1; i >= 0; i--)
> +			if (test_bit(i, indio_dev->active_scan_mask))
> +				sx9500_dec_chan_users(data, i);
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int sx9500_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +
> +	iio_triggered_buffer_predisable(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> +		if (test_bit(i, indio_dev->active_scan_mask)) {
> +			ret = sx9500_dec_chan_users(data, i);
> +			if (ret)
> +				break;
> +		}
> +
> +	if (ret)
> +		for (i = i - 1; i >= 0; i--)
> +			if (test_bit(i, indio_dev->active_scan_mask))
> +				sx9500_inc_chan_users(data, i);
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops sx9500_buffer_setup_ops = {
> +	.preenable = sx9500_buffer_preenable,
> +	.postenable = iio_triggered_buffer_postenable,
> +	.predisable = sx9500_buffer_predisable,
> +};
> +
>  struct sx9500_reg_default {
>  	u8 reg;
>  	u8 def;
> @@ -573,11 +758,44 @@ static const struct sx9500_reg_default sx9500_default_regs[] = {
>  	},
>  	{
>  		.reg = SX9500_REG_PROX_CTRL0,
> -		/* Scan period: 30ms, all sensors enabled. */
> -		.def = 0x0f,
> +		/* Scan period: 30ms, all sensors disabled. */
> +		.def = 0x00,
>  	},
>  };
>  
> +/* Activate all channels and perform an initial compensation. */
> +static int sx9500_init_compensation(struct iio_dev *indio_dev)
> +{
> +	struct sx9500_data *data = iio_priv(indio_dev);
> +	int i, ret;
> +	unsigned int val;
> +
> +	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
> +				 GENMASK(SX9500_NUM_CHANNELS, 0),
> +				 GENMASK(SX9500_NUM_CHANNELS, 0));
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 10; i >= 0; i--) {
> +		usleep_range(10000, 20000);
> +		ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
> +		if (ret < 0)
> +			goto out;
> +		if (!(val & SX9500_COMPSTAT_MASK))
> +			break;
> +	}
> +
> +	if (i < 0) {
> +		dev_err(&data->client->dev, "initial compensation timed out");
> +		ret = -ETIMEDOUT;
> +	}
> +
> +out:
> +	regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
> +			   GENMASK(SX9500_NUM_CHANNELS, 0), 0);
> +	return ret;
> +}
> +
>  static int sx9500_init_device(struct iio_dev *indio_dev)
>  {
>  	struct sx9500_data *data = iio_priv(indio_dev);
> @@ -605,6 +823,10 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> +	ret = sx9500_init_compensation(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return 0;
>  }
>  
> @@ -652,6 +874,7 @@ static int sx9500_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  	mutex_init(&data->mutex);
> +	init_completion(&data->completion);
>  	data->trigger_enabled = false;
>  
>  	data->regmap = devm_regmap_init_i2c(client, &sx9500_regmap_config);
> @@ -671,30 +894,35 @@ static int sx9500_probe(struct i2c_client *client,
>  	if (client->irq <= 0)
>  		client->irq = sx9500_gpio_probe(client, data);
>  
> -	if (client->irq > 0) {
> -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> -				sx9500_irq_handler, sx9500_irq_thread_handler,
> -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				SX9500_IRQ_NAME, indio_dev);
> -		if (ret < 0)
> -			return ret;
> +	if (client->irq <= 0) {
> +		dev_err(&client->dev, "no valid irq found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq,
> +					sx9500_irq_handler,
> +					sx9500_irq_thread_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					SX9500_IRQ_NAME, indio_dev);
> +	if (ret < 0)
> +		return ret;
>  
> -		data->trig = devm_iio_trigger_alloc(&client->dev,
> -				"%s-dev%d", indio_dev->name, indio_dev->id);
> -		if (!data->trig)
> -			return -ENOMEM;
> +	data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> +					    indio_dev->name, indio_dev->id);
> +	if (!data->trig)
> +		return -ENOMEM;
>  
> -		data->trig->dev.parent = &client->dev;
> -		data->trig->ops = &sx9500_trigger_ops;
> -		iio_trigger_set_drvdata(data->trig, indio_dev);
> +	data->trig->dev.parent = &client->dev;
> +	data->trig->ops = &sx9500_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, indio_dev);
>  
> -		ret = iio_trigger_register(data->trig);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = iio_trigger_register(data->trig);
> +	if (ret)
> +		return ret;
>  
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> -					 sx9500_trigger_handler, NULL);
> +					 sx9500_trigger_handler,
> +					 &sx9500_buffer_setup_ops);
>  	if (ret < 0)
>  		goto out_trigger_unregister;
>  
> 

Re: [PATCH 2/6] iio: sx9500: optimize power usage

[Vlad Dogaru]

On Thu, Apr 09, 2015 at 05:27:34PM +0100, Jonathan Cameron wrote:
> On 03/04/15 13:47, Vlad Dogaru wrote:
> > In the interest of lowering power usage, we only activate the proximity
> > channels and interrupts that we are currently using.
> > 
> > For raw reads, we activate the corresponding channel and the data ready
> > interrupt and wait for the interrupt to trigger.  Thus, it is no longer
> > optional to register an irq.
> This change bothers me.  We may well have people with the device that
> don't have the interrupt wired and if they are already using the driver
> then we have a resulting regression.
> 
> Any chance we could have an alternative of a registering polling or similar
> to identify when the interrupt would have occured?

Sure, I have a patch queued up that calls msleep instead of waiting for
an interrupt if one is not available.

But I'm not exactly sure which patches I should resend.  You mentioned
you applied some of the series, but I can't find anything in testing or
togreg.

Or am i confused and you would rather I resent the whole series?

> 
> > 
> > The following types of usage patterns may overlap:
> > 
> > * raw proximity reads (need a single data ready interrupt)
> We often avoid this complexity by just failing raw reads when
> the buffer is running but the solution you have here is pretty
> elegant.
> > * trigger usage (needs data ready interrupts as long as active)
> > * proximity events (need near/far interrupts)
> > * triggered buffer reads (don't need any interrupts, but are usually
> > coupled with our own trigger.
> 
> An alternative, perhaps more standard method, would be to handle this via
> an interrupt chip and let that infrastructure be responisble for the
> underlying real interrupts. I'm not sure which approach would be more
> complex though so perhaps, though I'd have gone that way, what you have
> here is the best way...
> 
> > 
> > To mitigate all possible patterns, we implement usage counting for all
> > the resources used: data ready interrupts, near/far interrupts and
> > individual channels.
> > 
> > The device enters sleep mode as documented in the data sheet when its
> > buffer, trigger and events are disabled, and no raw reads are currently
> > running.
> > 
> > Because of this new usage pattern, it is important that we give the
> > device a chance to perform an initial compensation for all its channels
> > at probe time.
> > 
> > Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> > ---
> >  drivers/iio/proximity/sx9500.c | 376 +++++++++++++++++++++++++++++++++--------
> >  1 file changed, 302 insertions(+), 74 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> > index f62d0b6..94c56f3 100644
> > --- a/drivers/iio/proximity/sx9500.c
> > +++ b/drivers/iio/proximity/sx9500.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm.h>
> > +#include <linux/delay.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/buffer.h>
> > @@ -74,6 +75,7 @@
> >  #define SX9500_CONVDONE_IRQ		BIT(3)
> >  
> >  #define SX9500_PROXSTAT_SHIFT		4
> > +#define SX9500_COMPSTAT_MASK		GENMASK(3, 0)
> >  
> >  #define SX9500_NUM_CHANNELS		4
> >  
> > @@ -92,6 +94,9 @@ struct sx9500_data {
> >  	u16 *buffer;
> >  	/* Remember enabled channels and sample rate during suspend. */
> >  	unsigned int suspend_ctrl0;
> > +	struct completion completion;
> > +	int data_rdy_users, close_far_users;
> > +	int channel_users[SX9500_NUM_CHANNELS];
> >  };
> >  
> >  static const struct iio_event_spec sx9500_events[] = {
> > @@ -194,7 +199,67 @@ static const struct regmap_config sx9500_regmap_config = {
> >  	.volatile_table = &sx9500_volatile_regs,
> >  };
> >  
> > -static int sx9500_read_proximity(struct sx9500_data *data,
> > +static int sx9500_inc_users(struct sx9500_data *data, int *counter,
> > +			    unsigned int reg, unsigned int bitmask)
> > +{
> > +	(*counter)++;
> > +	if (*counter != 1)
> > +		/* Bit is already active, nothing to do. */
> > +		return 0;
> > +
> > +	return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
> > +}
> > +
> > +static int sx9500_dec_users(struct sx9500_data *data, int *counter,
> > +			    unsigned int reg, unsigned int bitmask)
> > +{
> > +	(*counter)--;
> > +	if (*counter != 0)
> > +		/* There are more users, do not deactivate. */
> > +		return 0;
> > +
> > +	return regmap_update_bits(data->regmap, reg, bitmask, 0);
> > +}
> > +
> > +static int sx9500_inc_chan_users(struct sx9500_data *data, int chan)
> > +{
> > +	return sx9500_inc_users(data, &data->channel_users[chan],
> > +				SX9500_REG_PROX_CTRL0, BIT(chan));
> > +}
> > +
> > +static int sx9500_dec_chan_users(struct sx9500_data *data, int chan)
> > +{
> > +	return sx9500_dec_users(data, &data->channel_users[chan],
> > +				SX9500_REG_PROX_CTRL0, BIT(chan));
> > +}
> > +
> > +static int sx9500_inc_data_rdy_users(struct sx9500_data *data)
> > +{
> > +	return sx9500_inc_users(data, &data->data_rdy_users,
> > +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
> > +}
> > +
> > +static int sx9500_dec_data_rdy_users(struct sx9500_data *data)
> > +{
> > +	return sx9500_dec_users(data, &data->data_rdy_users,
> > +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
> > +}
> > +
> > +static int sx9500_inc_close_far_users(struct sx9500_data *data)
> > +{
> > +	return sx9500_inc_users(data, &data->close_far_users,
> > +				SX9500_REG_IRQ_MSK,
> > +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
> > +}
> > +
> > +static int sx9500_dec_close_far_users(struct sx9500_data *data)
> > +{
> > +	return sx9500_dec_users(data, &data->close_far_users,
> > +				SX9500_REG_IRQ_MSK,
> > +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
> > +}
> > +
> > +static int sx9500_read_prox_data(struct sx9500_data *data,
> >  				 const struct iio_chan_spec *chan,
> >  				 int *val)
> >  {
> > @@ -203,15 +268,66 @@ static int sx9500_read_proximity(struct sx9500_data *data,
> >  
> >  	ret = regmap_write(data->regmap, SX9500_REG_SENSOR_SEL, chan->channel);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto out;
> >  
> >  	ret = regmap_bulk_read(data->regmap, SX9500_REG_USE_MSB, &regval, 2);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto out;
> >  
> >  	*val = 32767 - (s16)be16_to_cpu(regval);
> > +	ret = IIO_VAL_INT;
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int sx9500_read_proximity(struct sx9500_data *data,
> > +				 const struct iio_chan_spec *chan,
> > +				 int *val)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	ret = sx9500_inc_chan_users(data, chan->channel);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = sx9500_inc_data_rdy_users(data);
> > +	if (ret < 0)
> > +		goto out_dec_chan;
> > +
> > +	mutex_unlock(&data->mutex);
> > +
> > +	ret = wait_for_completion_interruptible(&data->completion);
> > +	if (ret < 0)
> > +		return ret;
> >  
> > -	return IIO_VAL_INT;
> > +	mutex_lock(&data->mutex);
> > +
> > +	ret = sx9500_read_prox_data(data, chan, val);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = sx9500_dec_chan_users(data, chan->channel);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = sx9500_dec_data_rdy_users(data);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = IIO_VAL_INT;
> > +
> > +	goto out;
> > +
> > +out_dec_chan:
> > +	sx9500_dec_chan_users(data, chan->channel);
> > +out:
> > +	mutex_unlock(&data->mutex);
> > +	reinit_completion(&data->completion);
> > +
> > +	return ret;
> >  }
> >  
> >  static int sx9500_read_samp_freq(struct sx9500_data *data,
> > @@ -239,7 +355,6 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
> >  			   int *val, int *val2, long mask)
> >  {
> >  	struct sx9500_data *data = iio_priv(indio_dev);
> > -	int ret;
> >  
> >  	switch (chan->type) {
> >  	case IIO_PROXIMITY:
> > @@ -247,10 +362,7 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
> >  		case IIO_CHAN_INFO_RAW:
> >  			if (iio_buffer_enabled(indio_dev))
> >  				return -EBUSY;
> > -			mutex_lock(&data->mutex);
> > -			ret = sx9500_read_proximity(data, chan, val);
> > -			mutex_unlock(&data->mutex);
> > -			return ret;
> > +			return sx9500_read_proximity(data, chan, val);
> >  		case IIO_CHAN_INFO_SAMP_FREQ:
> >  			return sx9500_read_samp_freq(data, val, val2);
> >  		default:
> > @@ -321,28 +433,16 @@ static irqreturn_t sx9500_irq_handler(int irq, void *private)
> >  	return IRQ_WAKE_THREAD;
> >  }
> >  
> > -static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
> > +static void sx9500_push_events(struct iio_dev *indio_dev)
> >  {
> > -	struct iio_dev *indio_dev = private;
> > -	struct sx9500_data *data = iio_priv(indio_dev);
> >  	int ret;
> >  	unsigned int val, chan;
> > -
> > -	mutex_lock(&data->mutex);
> > -
> > -	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
> > -	if (ret < 0) {
> > -		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> > -		goto out;
> > -	}
> > -
> > -	if (!(val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ)))
> > -		goto out;
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> >  
> >  	ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
> >  	if (ret < 0) {
> >  		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> > -		goto out;
> > +		return;
> >  	}
> >  
> >  	val >>= SX9500_PROXSTAT_SHIFT;
> > @@ -357,15 +457,34 @@ static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
> >  			/* No change on this channel. */
> >  			continue;
> >  
> > -		dir = new_prox ? IIO_EV_DIR_FALLING :
> > -			IIO_EV_DIR_RISING;
> > -		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
> > -					  chan,
> > -					  IIO_EV_TYPE_THRESH,
> > -					  dir);
> > +		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
> > +					  IIO_EV_TYPE_THRESH, dir);
> >  		iio_push_event(indio_dev, ev, iio_get_time_ns());
> >  		data->prox_stat[chan] = new_prox;
> >  	}
> > +}
> > +
> > +static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "i2c transfer error in irq\n");
> > +		goto out;
> > +	}
> > +
> > +	if (val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ))
> > +		sx9500_push_events(indio_dev);
> > +
> > +	if (val & SX9500_CONVDONE_IRQ)
> > +		complete_all(&data->completion);
> >  
> >  out:
> >  	mutex_unlock(&data->mutex);
> > @@ -394,9 +513,7 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
> >  				     int state)
> >  {
> >  	struct sx9500_data *data = iio_priv(indio_dev);
> > -	int ret, i;
> > -	bool any_active = false;
> > -	unsigned int irqmask;
> > +	int ret;
> >  
> >  	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
> >  	    dir != IIO_EV_DIR_EITHER)
> > @@ -404,24 +521,32 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
> >  
> >  	mutex_lock(&data->mutex);
> >  
> > -	data->event_enabled[chan->channel] = state;
> > +	if (state == 1) {
> Is if (state) not sufficient?
> > +		ret = sx9500_inc_chan_users(data, chan->channel);
> > +		if (ret < 0)
> > +			goto out_unlock;
> > +		ret = sx9500_inc_close_far_users(data);
> > +		if (ret < 0)
> > +			goto out_undo_chan;
> > +	} else {
> > +		ret = sx9500_dec_chan_users(data, chan->channel);
> > +		if (ret < 0)
> > +			goto out_unlock;
> > +		ret = sx9500_dec_close_far_users(data);
> > +		if (ret < 0)
> > +			goto out_undo_chan;
> > +	}
> >  
> > -	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> > -		if (data->event_enabled[i]) {
> > -			any_active = true;
> > -			break;
> > -		}
> > +	data->event_enabled[chan->channel] = state;
> > +	goto out_unlock;
> >  
> > -	irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ;
> > -	if (any_active)
> > -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> > -					 irqmask, irqmask);
> > +out_undo_chan:
> > +	if (state == 1)
> > +		sx9500_dec_chan_users(data, chan->channel);
> >  	else
> > -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> > -					 irqmask, 0);
> > -
> > +		sx9500_inc_chan_users(data, chan->channel);
> > +out_unlock:
> >  	mutex_unlock(&data->mutex);
> > -
> >  	return ret;
> >  }
> >  
> > @@ -472,12 +597,16 @@ static int sx9500_set_trigger_state(struct iio_trigger *trig,
> >  
> >  	mutex_lock(&data->mutex);
> >  
> > -	ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
> > -				 SX9500_CONVDONE_IRQ,
> > -				 state ? SX9500_CONVDONE_IRQ : 0);
> > -	if (ret == 0)
> > -		data->trigger_enabled = state;
> > +	if (state)
> > +		ret = sx9500_inc_data_rdy_users(data);
> > +	else
> > +		ret = sx9500_dec_data_rdy_users(data);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	data->trigger_enabled = state;
> >  
> > +out:
> >  	mutex_unlock(&data->mutex);
> >  
> >  	return ret;
> > @@ -499,7 +628,7 @@ static irqreturn_t sx9500_trigger_handler(int irq, void *private)
> >  
> >  	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
> >  			 indio_dev->masklength) {
> > -		ret = sx9500_read_proximity(data, &indio_dev->channels[bit],
> > +		ret = sx9500_read_prox_data(data, &indio_dev->channels[bit],
> >  					    &val);
> >  		if (ret < 0)
> >  			goto out;
> > @@ -518,6 +647,62 @@ out:
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static int sx9500_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> > +	int ret, i;
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> > +		if (test_bit(i, indio_dev->active_scan_mask)) {
> > +			ret = sx9500_inc_chan_users(data, i);
> > +			if (ret)
> > +				break;
> > +		}
> > +
> > +	if (ret)
> > +		for (i = i - 1; i >= 0; i--)
> > +			if (test_bit(i, indio_dev->active_scan_mask))
> > +				sx9500_dec_chan_users(data, i);
> > +
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int sx9500_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> > +	int ret, i;
> > +
> > +	iio_triggered_buffer_predisable(indio_dev);
> > +
> > +	mutex_lock(&data->mutex);
> > +
> > +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> > +		if (test_bit(i, indio_dev->active_scan_mask)) {
> > +			ret = sx9500_dec_chan_users(data, i);
> > +			if (ret)
> > +				break;
> > +		}
> > +
> > +	if (ret)
> > +		for (i = i - 1; i >= 0; i--)
> > +			if (test_bit(i, indio_dev->active_scan_mask))
> > +				sx9500_inc_chan_users(data, i);
> > +
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_buffer_setup_ops sx9500_buffer_setup_ops = {
> > +	.preenable = sx9500_buffer_preenable,
> > +	.postenable = iio_triggered_buffer_postenable,
> > +	.predisable = sx9500_buffer_predisable,
> > +};
> > +
> >  struct sx9500_reg_default {
> >  	u8 reg;
> >  	u8 def;
> > @@ -573,11 +758,44 @@ static const struct sx9500_reg_default sx9500_default_regs[] = {
> >  	},
> >  	{
> >  		.reg = SX9500_REG_PROX_CTRL0,
> > -		/* Scan period: 30ms, all sensors enabled. */
> > -		.def = 0x0f,
> > +		/* Scan period: 30ms, all sensors disabled. */
> > +		.def = 0x00,
> >  	},
> >  };
> >  
> > +/* Activate all channels and perform an initial compensation. */
> > +static int sx9500_init_compensation(struct iio_dev *indio_dev)
> > +{
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> > +	int i, ret;
> > +	unsigned int val;
> > +
> > +	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
> > +				 GENMASK(SX9500_NUM_CHANNELS, 0),
> > +				 GENMASK(SX9500_NUM_CHANNELS, 0));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (i = 10; i >= 0; i--) {
> > +		usleep_range(10000, 20000);
> > +		ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
> > +		if (ret < 0)
> > +			goto out;
> > +		if (!(val & SX9500_COMPSTAT_MASK))
> > +			break;
> > +	}
> > +
> > +	if (i < 0) {
> > +		dev_err(&data->client->dev, "initial compensation timed out");
> > +		ret = -ETIMEDOUT;
> > +	}
> > +
> > +out:
> > +	regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
> > +			   GENMASK(SX9500_NUM_CHANNELS, 0), 0);
> > +	return ret;
> > +}
> > +
> >  static int sx9500_init_device(struct iio_dev *indio_dev)
> >  {
> >  	struct sx9500_data *data = iio_priv(indio_dev);
> > @@ -605,6 +823,10 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
> >  			return ret;
> >  	}
> >  
> > +	ret = sx9500_init_compensation(indio_dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -652,6 +874,7 @@ static int sx9500_probe(struct i2c_client *client,
> >  	data = iio_priv(indio_dev);
> >  	data->client = client;
> >  	mutex_init(&data->mutex);
> > +	init_completion(&data->completion);
> >  	data->trigger_enabled = false;
> >  
> >  	data->regmap = devm_regmap_init_i2c(client, &sx9500_regmap_config);
> > @@ -671,30 +894,35 @@ static int sx9500_probe(struct i2c_client *client,
> >  	if (client->irq <= 0)
> >  		client->irq = sx9500_gpio_probe(client, data);
> >  
> > -	if (client->irq > 0) {
> > -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > -				sx9500_irq_handler, sx9500_irq_thread_handler,
> > -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > -				SX9500_IRQ_NAME, indio_dev);
> > -		if (ret < 0)
> > -			return ret;
> > +	if (client->irq <= 0) {
> > +		dev_err(&client->dev, "no valid irq found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +					sx9500_irq_handler,
> > +					sx9500_irq_thread_handler,
> > +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +					SX9500_IRQ_NAME, indio_dev);
> > +	if (ret < 0)
> > +		return ret;
> >  
> > -		data->trig = devm_iio_trigger_alloc(&client->dev,
> > -				"%s-dev%d", indio_dev->name, indio_dev->id);
> > -		if (!data->trig)
> > -			return -ENOMEM;
> > +	data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> > +					    indio_dev->name, indio_dev->id);
> > +	if (!data->trig)
> > +		return -ENOMEM;
> >  
> > -		data->trig->dev.parent = &client->dev;
> > -		data->trig->ops = &sx9500_trigger_ops;
> > -		iio_trigger_set_drvdata(data->trig, indio_dev);
> > +	data->trig->dev.parent = &client->dev;
> > +	data->trig->ops = &sx9500_trigger_ops;
> > +	iio_trigger_set_drvdata(data->trig, indio_dev);
> >  
> > -		ret = iio_trigger_register(data->trig);
> > -		if (ret)
> > -			return ret;
> > -	}
> > +	ret = iio_trigger_register(data->trig);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > -					 sx9500_trigger_handler, NULL);
> > +					 sx9500_trigger_handler,
> > +					 &sx9500_buffer_setup_ops);
> >  	if (ret < 0)
> >  		goto out_trigger_unregister;
> >  
> > 
> 

Re: [PATCH 2/6] iio: sx9500: optimize power usage

[Jonathan Cameron]

On 10/04/15 11:36, Vlad Dogaru wrote:
> On Thu, Apr 09, 2015 at 05:27:34PM +0100, Jonathan Cameron wrote:
>> On 03/04/15 13:47, Vlad Dogaru wrote:
>>> In the interest of lowering power usage, we only activate the proximity
>>> channels and interrupts that we are currently using.
>>>
>>> For raw reads, we activate the corresponding channel and the data ready
>>> interrupt and wait for the interrupt to trigger.  Thus, it is no longer
>>> optional to register an irq.
>> This change bothers me.  We may well have people with the device that
>> don't have the interrupt wired and if they are already using the driver
>> then we have a resulting regression.
>>
>> Any chance we could have an alternative of a registering polling or similar
>> to identify when the interrupt would have occured?
> 
> Sure, I have a patch queued up that calls msleep instead of waiting for
> an interrupt if one is not available.
Cool.
> 
> But I'm not exactly sure which patches I should resend.  You mentioned
> you applied some of the series, but I can't find anything in testing or
> togreg.
Sorry, had to run out and forgot to push out when I got back later.
Testing should now be up to date.
> 
> Or am i confused and you would rather I resent the whole series?
> 
>>
>>>
>>> The following types of usage patterns may overlap:
>>>
>>> * raw proximity reads (need a single data ready interrupt)
>> We often avoid this complexity by just failing raw reads when
>> the buffer is running but the solution you have here is pretty
>> elegant.
>>> * trigger usage (needs data ready interrupts as long as active)
>>> * proximity events (need near/far interrupts)
>>> * triggered buffer reads (don't need any interrupts, but are usually
>>> coupled with our own trigger.
>>
>> An alternative, perhaps more standard method, would be to handle this via
>> an interrupt chip and let that infrastructure be responisble for the
>> underlying real interrupts. I'm not sure which approach would be more
>> complex though so perhaps, though I'd have gone that way, what you have
>> here is the best way...
>>
>>>
>>> To mitigate all possible patterns, we implement usage counting for all
>>> the resources used: data ready interrupts, near/far interrupts and
>>> individual channels.
>>>
>>> The device enters sleep mode as documented in the data sheet when its
>>> buffer, trigger and events are disabled, and no raw reads are currently
>>> running.
>>>
>>> Because of this new usage pattern, it is important that we give the
>>> device a chance to perform an initial compensation for all its channels
>>> at probe time.
>>>
>>> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
>>> ---
>>>  drivers/iio/proximity/sx9500.c | 376 +++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 302 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
>>> index f62d0b6..94c56f3 100644
>>> --- a/drivers/iio/proximity/sx9500.c
>>> +++ b/drivers/iio/proximity/sx9500.c
>>> @@ -19,6 +19,7 @@
>>>  #include <linux/gpio/consumer.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/pm.h>
>>> +#include <linux/delay.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>>  #include <linux/iio/buffer.h>
>>> @@ -74,6 +75,7 @@
>>>  #define SX9500_CONVDONE_IRQ		BIT(3)
>>>  
>>>  #define SX9500_PROXSTAT_SHIFT		4
>>> +#define SX9500_COMPSTAT_MASK		GENMASK(3, 0)
>>>  
>>>  #define SX9500_NUM_CHANNELS		4
>>>  
>>> @@ -92,6 +94,9 @@ struct sx9500_data {
>>>  	u16 *buffer;
>>>  	/* Remember enabled channels and sample rate during suspend. */
>>>  	unsigned int suspend_ctrl0;
>>> +	struct completion completion;
>>> +	int data_rdy_users, close_far_users;
>>> +	int channel_users[SX9500_NUM_CHANNELS];
>>>  };
>>>  
>>>  static const struct iio_event_spec sx9500_events[] = {
>>> @@ -194,7 +199,67 @@ static const struct regmap_config sx9500_regmap_config = {
>>>  	.volatile_table = &sx9500_volatile_regs,
>>>  };
>>>  
>>> -static int sx9500_read_proximity(struct sx9500_data *data,
>>> +static int sx9500_inc_users(struct sx9500_data *data, int *counter,
>>> +			    unsigned int reg, unsigned int bitmask)
>>> +{
>>> +	(*counter)++;
>>> +	if (*counter != 1)
>>> +		/* Bit is already active, nothing to do. */
>>> +		return 0;
>>> +
>>> +	return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
>>> +}
>>> +
>>> +static int sx9500_dec_users(struct sx9500_data *data, int *counter,
>>> +			    unsigned int reg, unsigned int bitmask)
>>> +{
>>> +	(*counter)--;
>>> +	if (*counter != 0)
>>> +		/* There are more users, do not deactivate. */
>>> +		return 0;
>>> +
>>> +	return regmap_update_bits(data->regmap, reg, bitmask, 0);
>>> +}
>>> +
>>> +static int sx9500_inc_chan_users(struct sx9500_data *data, int chan)
>>> +{
>>> +	return sx9500_inc_users(data, &data->channel_users[chan],
>>> +				SX9500_REG_PROX_CTRL0, BIT(chan));
>>> +}
>>> +
>>> +static int sx9500_dec_chan_users(struct sx9500_data *data, int chan)
>>> +{
>>> +	return sx9500_dec_users(data, &data->channel_users[chan],
>>> +				SX9500_REG_PROX_CTRL0, BIT(chan));
>>> +}
>>> +
>>> +static int sx9500_inc_data_rdy_users(struct sx9500_data *data)
>>> +{
>>> +	return sx9500_inc_users(data, &data->data_rdy_users,
>>> +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
>>> +}
>>> +
>>> +static int sx9500_dec_data_rdy_users(struct sx9500_data *data)
>>> +{
>>> +	return sx9500_dec_users(data, &data->data_rdy_users,
>>> +				SX9500_REG_IRQ_MSK, SX9500_CONVDONE_IRQ);
>>> +}
>>> +
>>> +static int sx9500_inc_close_far_users(struct sx9500_data *data)
>>> +{
>>> +	return sx9500_inc_users(data, &data->close_far_users,
>>> +				SX9500_REG_IRQ_MSK,
>>> +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
>>> +}
>>> +
>>> +static int sx9500_dec_close_far_users(struct sx9500_data *data)
>>> +{
>>> +	return sx9500_dec_users(data, &data->close_far_users,
>>> +				SX9500_REG_IRQ_MSK,
>>> +				SX9500_CLOSE_IRQ | SX9500_FAR_IRQ);
>>> +}
>>> +
>>> +static int sx9500_read_prox_data(struct sx9500_data *data,
>>>  				 const struct iio_chan_spec *chan,
>>>  				 int *val)
>>>  {
>>> @@ -203,15 +268,66 @@ static int sx9500_read_proximity(struct sx9500_data *data,
>>>  
>>>  	ret = regmap_write(data->regmap, SX9500_REG_SENSOR_SEL, chan->channel);
>>>  	if (ret < 0)
>>> -		return ret;
>>> +		goto out;
>>>  
>>>  	ret = regmap_bulk_read(data->regmap, SX9500_REG_USE_MSB, &regval, 2);
>>>  	if (ret < 0)
>>> -		return ret;
>>> +		goto out;
>>>  
>>>  	*val = 32767 - (s16)be16_to_cpu(regval);
>>> +	ret = IIO_VAL_INT;
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static int sx9500_read_proximity(struct sx9500_data *data,
>>> +				 const struct iio_chan_spec *chan,
>>> +				 int *val)
>>> +{
>>> +	int ret;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	ret = sx9500_inc_chan_users(data, chan->channel);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = sx9500_inc_data_rdy_users(data);
>>> +	if (ret < 0)
>>> +		goto out_dec_chan;
>>> +
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	ret = wait_for_completion_interruptible(&data->completion);
>>> +	if (ret < 0)
>>> +		return ret;
>>>  
>>> -	return IIO_VAL_INT;
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	ret = sx9500_read_prox_data(data, chan, val);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = sx9500_dec_chan_users(data, chan->channel);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = sx9500_dec_data_rdy_users(data);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = IIO_VAL_INT;
>>> +
>>> +	goto out;
>>> +
>>> +out_dec_chan:
>>> +	sx9500_dec_chan_users(data, chan->channel);
>>> +out:
>>> +	mutex_unlock(&data->mutex);
>>> +	reinit_completion(&data->completion);
>>> +
>>> +	return ret;
>>>  }
>>>  
>>>  static int sx9500_read_samp_freq(struct sx9500_data *data,
>>> @@ -239,7 +355,6 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
>>>  			   int *val, int *val2, long mask)
>>>  {
>>>  	struct sx9500_data *data = iio_priv(indio_dev);
>>> -	int ret;
>>>  
>>>  	switch (chan->type) {
>>>  	case IIO_PROXIMITY:
>>> @@ -247,10 +362,7 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
>>>  		case IIO_CHAN_INFO_RAW:
>>>  			if (iio_buffer_enabled(indio_dev))
>>>  				return -EBUSY;
>>> -			mutex_lock(&data->mutex);
>>> -			ret = sx9500_read_proximity(data, chan, val);
>>> -			mutex_unlock(&data->mutex);
>>> -			return ret;
>>> +			return sx9500_read_proximity(data, chan, val);
>>>  		case IIO_CHAN_INFO_SAMP_FREQ:
>>>  			return sx9500_read_samp_freq(data, val, val2);
>>>  		default:
>>> @@ -321,28 +433,16 @@ static irqreturn_t sx9500_irq_handler(int irq, void *private)
>>>  	return IRQ_WAKE_THREAD;
>>>  }
>>>  
>>> -static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
>>> +static void sx9500_push_events(struct iio_dev *indio_dev)
>>>  {
>>> -	struct iio_dev *indio_dev = private;
>>> -	struct sx9500_data *data = iio_priv(indio_dev);
>>>  	int ret;
>>>  	unsigned int val, chan;
>>> -
>>> -	mutex_lock(&data->mutex);
>>> -
>>> -	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
>>> -	if (ret < 0) {
>>> -		dev_err(&data->client->dev, "i2c transfer error in irq\n");
>>> -		goto out;
>>> -	}
>>> -
>>> -	if (!(val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ)))
>>> -		goto out;
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>>  
>>>  	ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
>>>  	if (ret < 0) {
>>>  		dev_err(&data->client->dev, "i2c transfer error in irq\n");
>>> -		goto out;
>>> +		return;
>>>  	}
>>>  
>>>  	val >>= SX9500_PROXSTAT_SHIFT;
>>> @@ -357,15 +457,34 @@ static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
>>>  			/* No change on this channel. */
>>>  			continue;
>>>  
>>> -		dir = new_prox ? IIO_EV_DIR_FALLING :
>>> -			IIO_EV_DIR_RISING;
>>> -		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
>>> -					  chan,
>>> -					  IIO_EV_TYPE_THRESH,
>>> -					  dir);
>>> +		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
>>> +		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
>>> +					  IIO_EV_TYPE_THRESH, dir);
>>>  		iio_push_event(indio_dev, ev, iio_get_time_ns());
>>>  		data->prox_stat[chan] = new_prox;
>>>  	}
>>> +}
>>> +
>>> +static irqreturn_t sx9500_irq_thread_handler(int irq, void *private)
>>> +{
>>> +	struct iio_dev *indio_dev = private;
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>> +	int ret;
>>> +	unsigned int val;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val);
>>> +	if (ret < 0) {
>>> +		dev_err(&data->client->dev, "i2c transfer error in irq\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ))
>>> +		sx9500_push_events(indio_dev);
>>> +
>>> +	if (val & SX9500_CONVDONE_IRQ)
>>> +		complete_all(&data->completion);
>>>  
>>>  out:
>>>  	mutex_unlock(&data->mutex);
>>> @@ -394,9 +513,7 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
>>>  				     int state)
>>>  {
>>>  	struct sx9500_data *data = iio_priv(indio_dev);
>>> -	int ret, i;
>>> -	bool any_active = false;
>>> -	unsigned int irqmask;
>>> +	int ret;
>>>  
>>>  	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
>>>  	    dir != IIO_EV_DIR_EITHER)
>>> @@ -404,24 +521,32 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
>>>  
>>>  	mutex_lock(&data->mutex);
>>>  
>>> -	data->event_enabled[chan->channel] = state;
>>> +	if (state == 1) {
>> Is if (state) not sufficient?
>>> +		ret = sx9500_inc_chan_users(data, chan->channel);
>>> +		if (ret < 0)
>>> +			goto out_unlock;
>>> +		ret = sx9500_inc_close_far_users(data);
>>> +		if (ret < 0)
>>> +			goto out_undo_chan;
>>> +	} else {
>>> +		ret = sx9500_dec_chan_users(data, chan->channel);
>>> +		if (ret < 0)
>>> +			goto out_unlock;
>>> +		ret = sx9500_dec_close_far_users(data);
>>> +		if (ret < 0)
>>> +			goto out_undo_chan;
>>> +	}
>>>  
>>> -	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
>>> -		if (data->event_enabled[i]) {
>>> -			any_active = true;
>>> -			break;
>>> -		}
>>> +	data->event_enabled[chan->channel] = state;
>>> +	goto out_unlock;
>>>  
>>> -	irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ;
>>> -	if (any_active)
>>> -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
>>> -					 irqmask, irqmask);
>>> +out_undo_chan:
>>> +	if (state == 1)
>>> +		sx9500_dec_chan_users(data, chan->channel);
>>>  	else
>>> -		ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
>>> -					 irqmask, 0);
>>> -
>>> +		sx9500_inc_chan_users(data, chan->channel);
>>> +out_unlock:
>>>  	mutex_unlock(&data->mutex);
>>> -
>>>  	return ret;
>>>  }
>>>  
>>> @@ -472,12 +597,16 @@ static int sx9500_set_trigger_state(struct iio_trigger *trig,
>>>  
>>>  	mutex_lock(&data->mutex);
>>>  
>>> -	ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK,
>>> -				 SX9500_CONVDONE_IRQ,
>>> -				 state ? SX9500_CONVDONE_IRQ : 0);
>>> -	if (ret == 0)
>>> -		data->trigger_enabled = state;
>>> +	if (state)
>>> +		ret = sx9500_inc_data_rdy_users(data);
>>> +	else
>>> +		ret = sx9500_dec_data_rdy_users(data);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	data->trigger_enabled = state;
>>>  
>>> +out:
>>>  	mutex_unlock(&data->mutex);
>>>  
>>>  	return ret;
>>> @@ -499,7 +628,7 @@ static irqreturn_t sx9500_trigger_handler(int irq, void *private)
>>>  
>>>  	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
>>>  			 indio_dev->masklength) {
>>> -		ret = sx9500_read_proximity(data, &indio_dev->channels[bit],
>>> +		ret = sx9500_read_prox_data(data, &indio_dev->channels[bit],
>>>  					    &val);
>>>  		if (ret < 0)
>>>  			goto out;
>>> @@ -518,6 +647,62 @@ out:
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static int sx9500_buffer_preenable(struct iio_dev *indio_dev)
>>> +{
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>> +	int ret, i;
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
>>> +		if (test_bit(i, indio_dev->active_scan_mask)) {
>>> +			ret = sx9500_inc_chan_users(data, i);
>>> +			if (ret)
>>> +				break;
>>> +		}
>>> +
>>> +	if (ret)
>>> +		for (i = i - 1; i >= 0; i--)
>>> +			if (test_bit(i, indio_dev->active_scan_mask))
>>> +				sx9500_dec_chan_users(data, i);
>>> +
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int sx9500_buffer_predisable(struct iio_dev *indio_dev)
>>> +{
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>> +	int ret, i;
>>> +
>>> +	iio_triggered_buffer_predisable(indio_dev);
>>> +
>>> +	mutex_lock(&data->mutex);
>>> +
>>> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
>>> +		if (test_bit(i, indio_dev->active_scan_mask)) {
>>> +			ret = sx9500_dec_chan_users(data, i);
>>> +			if (ret)
>>> +				break;
>>> +		}
>>> +
>>> +	if (ret)
>>> +		for (i = i - 1; i >= 0; i--)
>>> +			if (test_bit(i, indio_dev->active_scan_mask))
>>> +				sx9500_inc_chan_users(data, i);
>>> +
>>> +	mutex_unlock(&data->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct iio_buffer_setup_ops sx9500_buffer_setup_ops = {
>>> +	.preenable = sx9500_buffer_preenable,
>>> +	.postenable = iio_triggered_buffer_postenable,
>>> +	.predisable = sx9500_buffer_predisable,
>>> +};
>>> +
>>>  struct sx9500_reg_default {
>>>  	u8 reg;
>>>  	u8 def;
>>> @@ -573,11 +758,44 @@ static const struct sx9500_reg_default sx9500_default_regs[] = {
>>>  	},
>>>  	{
>>>  		.reg = SX9500_REG_PROX_CTRL0,
>>> -		/* Scan period: 30ms, all sensors enabled. */
>>> -		.def = 0x0f,
>>> +		/* Scan period: 30ms, all sensors disabled. */
>>> +		.def = 0x00,
>>>  	},
>>>  };
>>>  
>>> +/* Activate all channels and perform an initial compensation. */
>>> +static int sx9500_init_compensation(struct iio_dev *indio_dev)
>>> +{
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>> +	int i, ret;
>>> +	unsigned int val;
>>> +
>>> +	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
>>> +				 GENMASK(SX9500_NUM_CHANNELS, 0),
>>> +				 GENMASK(SX9500_NUM_CHANNELS, 0));
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	for (i = 10; i >= 0; i--) {
>>> +		usleep_range(10000, 20000);
>>> +		ret = regmap_read(data->regmap, SX9500_REG_STAT, &val);
>>> +		if (ret < 0)
>>> +			goto out;
>>> +		if (!(val & SX9500_COMPSTAT_MASK))
>>> +			break;
>>> +	}
>>> +
>>> +	if (i < 0) {
>>> +		dev_err(&data->client->dev, "initial compensation timed out");
>>> +		ret = -ETIMEDOUT;
>>> +	}
>>> +
>>> +out:
>>> +	regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
>>> +			   GENMASK(SX9500_NUM_CHANNELS, 0), 0);
>>> +	return ret;
>>> +}
>>> +
>>>  static int sx9500_init_device(struct iio_dev *indio_dev)
>>>  {
>>>  	struct sx9500_data *data = iio_priv(indio_dev);
>>> @@ -605,6 +823,10 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
>>>  			return ret;
>>>  	}
>>>  
>>> +	ret = sx9500_init_compensation(indio_dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -652,6 +874,7 @@ static int sx9500_probe(struct i2c_client *client,
>>>  	data = iio_priv(indio_dev);
>>>  	data->client = client;
>>>  	mutex_init(&data->mutex);
>>> +	init_completion(&data->completion);
>>>  	data->trigger_enabled = false;
>>>  
>>>  	data->regmap = devm_regmap_init_i2c(client, &sx9500_regmap_config);
>>> @@ -671,30 +894,35 @@ static int sx9500_probe(struct i2c_client *client,
>>>  	if (client->irq <= 0)
>>>  		client->irq = sx9500_gpio_probe(client, data);
>>>  
>>> -	if (client->irq > 0) {
>>> -		ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> -				sx9500_irq_handler, sx9500_irq_thread_handler,
>>> -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> -				SX9500_IRQ_NAME, indio_dev);
>>> -		if (ret < 0)
>>> -			return ret;
>>> +	if (client->irq <= 0) {
>>> +		dev_err(&client->dev, "no valid irq found\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> +					sx9500_irq_handler,
>>> +					sx9500_irq_thread_handler,
>>> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> +					SX9500_IRQ_NAME, indio_dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>>  
>>> -		data->trig = devm_iio_trigger_alloc(&client->dev,
>>> -				"%s-dev%d", indio_dev->name, indio_dev->id);
>>> -		if (!data->trig)
>>> -			return -ENOMEM;
>>> +	data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
>>> +					    indio_dev->name, indio_dev->id);
>>> +	if (!data->trig)
>>> +		return -ENOMEM;
>>>  
>>> -		data->trig->dev.parent = &client->dev;
>>> -		data->trig->ops = &sx9500_trigger_ops;
>>> -		iio_trigger_set_drvdata(data->trig, indio_dev);
>>> +	data->trig->dev.parent = &client->dev;
>>> +	data->trig->ops = &sx9500_trigger_ops;
>>> +	iio_trigger_set_drvdata(data->trig, indio_dev);
>>>  
>>> -		ret = iio_trigger_register(data->trig);
>>> -		if (ret)
>>> -			return ret;
>>> -	}
>>> +	ret = iio_trigger_register(data->trig);
>>> +	if (ret)
>>> +		return ret;
>>>  
>>>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> -					 sx9500_trigger_handler, NULL);
>>> +					 sx9500_trigger_handler,
>>> +					 &sx9500_buffer_setup_ops);
>>>  	if (ret < 0)
>>>  		goto out_trigger_unregister;
>>>  
>>>
>>
> --
> 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 3/6] iio: sx9500: rename GPIO interrupt pin

[Vlad Dogaru]

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 94c56f3..13b174c 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -31,7 +31,8 @@
 
 #define SX9500_DRIVER_NAME		"sx9500"
 #define SX9500_IRQ_NAME			"sx9500_event"
-#define SX9500_GPIO_NAME		"sx9500_gpio"
+
+#define SX9500_GPIO_INT			"interrupt"
 
 /* Register definitions. */
 #define SX9500_REG_IRQ_SRC		0x00
-- 
1.9.1

Re: [PATCH 3/6] iio: sx9500: rename GPIO interrupt pin

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
silly indeed.

Applied this one to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play.

Thanks,

Jonathan
> ---
>  drivers/iio/proximity/sx9500.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 94c56f3..13b174c 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -31,7 +31,8 @@
>  
>  #define SX9500_DRIVER_NAME		"sx9500"
>  #define SX9500_IRQ_NAME			"sx9500_event"
> -#define SX9500_GPIO_NAME		"sx9500_gpio"
> +
> +#define SX9500_GPIO_INT			"interrupt"
>  
>  /* Register definitions. */
>  #define SX9500_REG_IRQ_SRC		0x00
> 

Re: [PATCH 3/6] iio: sx9500: rename GPIO interrupt pin

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
For some reason I missed this before... 
> ---
>  drivers/iio/proximity/sx9500.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 94c56f3..13b174c 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -31,7 +31,8 @@
>  
>  #define SX9500_DRIVER_NAME		"sx9500"
>  #define SX9500_IRQ_NAME			"sx9500_event"
> -#define SX9500_GPIO_NAME		"sx9500_gpio"
> +
> +#define SX9500_GPIO_INT			"interrupt"
You've changed the define as well as the name that broke the build so
I've changed it back again as a fix-up for this patch.

Your v3 patch 2 will uses the new name... I might fix it up
to change the name of the define there if I'm happy with the
rest of that patch.

J
>  
>  /* Register definitions. */
>  #define SX9500_REG_IRQ_SRC		0x00
> 

[PATCH 4/6] iio: sx9500: refactor GPIO interrupt code

[Vlad Dogaru]

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 13b174c..d1e886c 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -831,34 +831,25 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static int sx9500_gpio_probe(struct i2c_client *client,
-			     struct sx9500_data *data)
+static void sx9500_gpio_probe(struct i2c_client *client,
+			      struct sx9500_data *data)
 {
 	struct device *dev;
 	struct gpio_desc *gpio;
-	int ret;
 
 	if (!client)
-		return -EINVAL;
+		return;
 
 	dev = &client->dev;
 
-	/* data ready gpio interrupt pin */
-	gpio = devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0);
-	if (IS_ERR(gpio)) {
-		dev_err(dev, "acpi gpio get index failed\n");
-		return PTR_ERR(gpio);
+	if (client->irq <= 0) {
+		gpio = devm_gpiod_get_index(dev, SX9500_GPIO_INT, 0, GPIOD_IN);
+		if (IS_ERR(gpio))
+			dev_err(dev, "gpio get irq failed\n");
+		else
+			client->irq = gpiod_to_irq(gpio);
 	}
 
-	ret = gpiod_direction_input(gpio);
-	if (ret)
-		return ret;
-
-	ret = gpiod_to_irq(gpio);
-
-	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
-
-	return ret;
 }
 
 static int sx9500_probe(struct i2c_client *client,
@@ -892,8 +883,7 @@ static int sx9500_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	i2c_set_clientdata(client, indio_dev);
 
-	if (client->irq <= 0)
-		client->irq = sx9500_gpio_probe(client, data);
+	sx9500_gpio_probe(client, data);
 
 	if (client->irq <= 0) {
 		dev_err(&client->dev, "no valid irq found\n");
-- 
1.9.1

Re: [PATCH 4/6] iio: sx9500: refactor GPIO interrupt code

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
This one is fine, though won't apply until we've sorted the earlier patch out.
> ---
>  drivers/iio/proximity/sx9500.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 13b174c..d1e886c 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -831,34 +831,25 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> -static int sx9500_gpio_probe(struct i2c_client *client,
> -			     struct sx9500_data *data)
> +static void sx9500_gpio_probe(struct i2c_client *client,
> +			      struct sx9500_data *data)
>  {
>  	struct device *dev;
>  	struct gpio_desc *gpio;
> -	int ret;
>  
>  	if (!client)
> -		return -EINVAL;
> +		return;
>  
>  	dev = &client->dev;
>  
> -	/* data ready gpio interrupt pin */
> -	gpio = devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0);
> -	if (IS_ERR(gpio)) {
> -		dev_err(dev, "acpi gpio get index failed\n");
> -		return PTR_ERR(gpio);
> +	if (client->irq <= 0) {
> +		gpio = devm_gpiod_get_index(dev, SX9500_GPIO_INT, 0, GPIOD_IN);
> +		if (IS_ERR(gpio))
> +			dev_err(dev, "gpio get irq failed\n");
> +		else
> +			client->irq = gpiod_to_irq(gpio);
>  	}
>  
> -	ret = gpiod_direction_input(gpio);
> -	if (ret)
> -		return ret;
> -
> -	ret = gpiod_to_irq(gpio);
> -
> -	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> -
> -	return ret;
>  }
>  
>  static int sx9500_probe(struct i2c_client *client,
> @@ -892,8 +883,7 @@ static int sx9500_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	i2c_set_clientdata(client, indio_dev);
>  
> -	if (client->irq <= 0)
> -		client->irq = sx9500_gpio_probe(client, data);
> +	sx9500_gpio_probe(client, data);
>  
>  	if (client->irq <= 0) {
>  		dev_err(&client->dev, "no valid irq found\n");
> 

[PATCH 5/6] iio: sx9500: add GPIO reset pin

[Vlad Dogaru]

If a GPIO reset pin is listed in ACPI or Device Tree, use it to reset
the device on initialization.

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index d1e886c..9738df1 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -33,6 +33,7 @@
 #define SX9500_IRQ_NAME			"sx9500_event"
 
 #define SX9500_GPIO_INT			"interrupt"
+#define SX9500_GPIO_RESET		"reset"
 
 /* Register definitions. */
 #define SX9500_REG_IRQ_SRC		0x00
@@ -85,6 +86,7 @@ struct sx9500_data {
 	struct i2c_client *client;
 	struct iio_trigger *trig;
 	struct regmap *regmap;
+	struct gpio_desc *gpiod_rst;
 	/*
 	 * Last reading of the proximity status for each channel.  We
 	 * only send an event to user space when this changes.
@@ -803,6 +805,13 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
 	int ret, i;
 	unsigned int val;
 
+	if (data->gpiod_rst) {
+		gpiod_set_value_cansleep(data->gpiod_rst, 0);
+		usleep_range(1000, 2000);
+		gpiod_set_value_cansleep(data->gpiod_rst, 1);
+		usleep_range(1000, 2000);
+	}
+
 	ret = regmap_write(data->regmap, SX9500_REG_IRQ_MSK, 0);
 	if (ret < 0)
 		return ret;
@@ -850,6 +859,12 @@ static void sx9500_gpio_probe(struct i2c_client *client,
 			client->irq = gpiod_to_irq(gpio);
 	}
 
+	data->gpiod_rst = devm_gpiod_get_index(dev, SX9500_GPIO_RESET,
+					       0, GPIOD_OUT_HIGH);
+	if (IS_ERR(data->gpiod_rst)) {
+		dev_warn(dev, "gpio get reset pin failed\n");
+		data->gpiod_rst = NULL;
+	}
 }
 
 static int sx9500_probe(struct i2c_client *client,
@@ -873,8 +888,6 @@ static int sx9500_probe(struct i2c_client *client,
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
-	sx9500_init_device(indio_dev);
-
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = SX9500_DRIVER_NAME;
 	indio_dev->channels = sx9500_channels;
@@ -890,6 +903,10 @@ static int sx9500_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	ret = sx9500_init_device(indio_dev);
+	if (ret < 0)
+		return ret;
+
 	ret = devm_request_threaded_irq(&client->dev, client->irq,
 					sx9500_irq_handler,
 					sx9500_irq_thread_handler,
-- 
1.9.1

Re: [PATCH 5/6] iio: sx9500: add GPIO reset pin

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> If a GPIO reset pin is listed in ACPI or Device Tree, use it to reset
> the device on initialization.
> 
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
Again this one is fine.
> ---
>  drivers/iio/proximity/sx9500.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index d1e886c..9738df1 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -33,6 +33,7 @@
>  #define SX9500_IRQ_NAME			"sx9500_event"
>  
>  #define SX9500_GPIO_INT			"interrupt"
> +#define SX9500_GPIO_RESET		"reset"
>  
>  /* Register definitions. */
>  #define SX9500_REG_IRQ_SRC		0x00
> @@ -85,6 +86,7 @@ struct sx9500_data {
>  	struct i2c_client *client;
>  	struct iio_trigger *trig;
>  	struct regmap *regmap;
> +	struct gpio_desc *gpiod_rst;
>  	/*
>  	 * Last reading of the proximity status for each channel.  We
>  	 * only send an event to user space when this changes.
> @@ -803,6 +805,13 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
>  	int ret, i;
>  	unsigned int val;
>  
> +	if (data->gpiod_rst) {
> +		gpiod_set_value_cansleep(data->gpiod_rst, 0);
> +		usleep_range(1000, 2000);
> +		gpiod_set_value_cansleep(data->gpiod_rst, 1);
> +		usleep_range(1000, 2000);
> +	}
> +
>  	ret = regmap_write(data->regmap, SX9500_REG_IRQ_MSK, 0);
>  	if (ret < 0)
>  		return ret;
> @@ -850,6 +859,12 @@ static void sx9500_gpio_probe(struct i2c_client *client,
>  			client->irq = gpiod_to_irq(gpio);
>  	}
>  
> +	data->gpiod_rst = devm_gpiod_get_index(dev, SX9500_GPIO_RESET,
> +					       0, GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->gpiod_rst)) {
> +		dev_warn(dev, "gpio get reset pin failed\n");
> +		data->gpiod_rst = NULL;
> +	}
>  }
>  
>  static int sx9500_probe(struct i2c_client *client,
> @@ -873,8 +888,6 @@ static int sx9500_probe(struct i2c_client *client,
>  	if (IS_ERR(data->regmap))
>  		return PTR_ERR(data->regmap);
>  
> -	sx9500_init_device(indio_dev);
> -
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = SX9500_DRIVER_NAME;
>  	indio_dev->channels = sx9500_channels;
> @@ -890,6 +903,10 @@ static int sx9500_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>  
> +	ret = sx9500_init_device(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = devm_request_threaded_irq(&client->dev, client->irq,
>  					sx9500_irq_handler,
>  					sx9500_irq_thread_handler,
> 

[PATCH 6/6] iio: sx9500: fix formatting

[Vlad Dogaru]

Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
 drivers/iio/proximity/sx9500.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 9738df1..2156037 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -1014,7 +1014,7 @@ MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);
 
 static const struct i2c_device_id sx9500_id[] = {
 	{"sx9500", 0},
-	{}
+	{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx9500_id);
 
-- 
1.9.1

Re: [PATCH 6/6] iio: sx9500: fix formatting

[Jonathan Cameron]

On 03/04/15 13:47, Vlad Dogaru wrote:
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
Trivial and applies fine so applied to the togreg branch of iio.git
so we can both forget about it ;)
> ---
>  drivers/iio/proximity/sx9500.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 9738df1..2156037 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -1014,7 +1014,7 @@ MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);
>  
>  static const struct i2c_device_id sx9500_id[] = {
>  	{"sx9500", 0},
> -	{}
> +	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, sx9500_id);
>  
>