[PATCH 0/8] iio: adc: ti-ads1015: fixes, cleanups, and threshold event support

[PATCH 0/8] iio: adc: ti-ads1015: fixes, cleanups, and threshold event support

[Akinobu Mita]

This series contains some fixes and cleanups that I found, and a threshold
event support for ti-ads1015 driver.

The first five patches are classified as bug fixes (one of which is a
borderline between bugfix and cleanup).  The next two are cleanups that
can be considered as preparation for the last threshold event support.

Akinobu Mita (8):
  iio: adc: ti-ads1015: fix incorrect data rate setting update
  iio: adc: ti-ads1015: remove unnecessary config register update
  iio: adc: ti-ads1015: fix scale information for ADS1115
  iio: adc: ti-ads1015: enable conversion when CONFIG_PM is not set
  iio: adc: ti-ads1015: avoid getting stale result after runtime resume
  iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup
  iio: adc: ti-ads1015: use iio_device_claim_direct_mode()
  iio: adc: ti-ads1015: add threshold event support

 drivers/iio/adc/ti-ads1015.c | 534 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 442 insertions(+), 92 deletions(-)

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
-- 
2.7.4

[PATCH 1/8] iio: adc: ti-ads1015: fix incorrect data rate setting update

[Akinobu Mita]

The ti-ads1015 driver has eight iio voltage channels and each iio channel
can hold own sampling frequency information.

The ADS1015 device only have a single config register which contains an
input multiplexer selection, PGA and data rate settings.  So the driver
should load the correct settings when the input multiplexer selection is
changed.

However, regardless of which channlel is currently selected, changing any
iio channel's sampling frequency information immediately overwrites the
current data rate setting in the config register.

It breaks the current data rate setting if the different channel's sampling
frequency information is changed because the data rate setting is not
reloaded when the input multiplexer is switched.

This removes the unexpected config register update and correctly load the
data rate setting before getting adc result.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index f76d979..7e21151 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -252,9 +252,11 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 
 	ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
 				       ADS1015_CFG_MUX_MASK |
-				       ADS1015_CFG_PGA_MASK,
+				       ADS1015_CFG_PGA_MASK |
+				       ADS1015_CFG_DR_MASK,
 				       chan << ADS1015_CFG_MUX_SHIFT |
-				       pga << ADS1015_CFG_PGA_SHIFT,
+				       pga << ADS1015_CFG_PGA_SHIFT |
+				       dr << ADS1015_CFG_DR_SHIFT,
 				       &change);
 	if (ret < 0)
 		return ret;
@@ -325,25 +327,16 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
 
 static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)
 {
-	int i, ret, rindex = -1;
+	int i;
 
-	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++)
+	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++) {
 		if (data->data_rate[i] == rate) {
-			rindex = i;
-			break;
+			data->channel_data[chan].data_rate = i;
+			return 0;
 		}
-	if (rindex < 0)
-		return -EINVAL;
-
-	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				 ADS1015_CFG_DR_MASK,
-				 rindex << ADS1015_CFG_DR_SHIFT);
-	if (ret < 0)
-		return ret;
-
-	data->channel_data[chan].data_rate = rindex;
+	}
 
-	return 0;
+	return -EINVAL;
 }
 
 static int ads1015_read_raw(struct iio_dev *indio_dev,
-- 
2.7.4

Re: [PATCH 1/8] iio: adc: ti-ads1015: fix incorrect data rate setting update

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:31:57 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The ti-ads1015 driver has eight iio voltage channels and each iio channel
> can hold own sampling frequency information.
> 
> The ADS1015 device only have a single config register which contains an
> input multiplexer selection, PGA and data rate settings.  So the driver
> should load the correct settings when the input multiplexer selection is
> changed.
> 
> However, regardless of which channlel is currently selected, changing any
> iio channel's sampling frequency information immediately overwrites the
> current data rate setting in the config register.
> 
> It breaks the current data rate setting if the different channel's sampling
> frequency information is changed because the data rate setting is not
> reloaded when the input multiplexer is switched.
> 
> This removes the unexpected config register update and correctly load the
> data rate setting before getting adc result.
> 
Updated Daniel's email in the cc as he is no longer at Intel.

Given I'd like to push these out as fixes, I'm particularly keen to
get some additional input on these.

This behaviour in the existing driver would be fine if the sampling
frequency was a shared attribute.  Technically it's still fine under
the ABI for any attribute to change any other.  

However, here we have an odd mixed setup that is doing local
caching to allow the channels to have different sampling frequencies
but then isn't implementing it actually happening right.

So I think this patch is right, but other input welcome!

Akinobu, if I seem to have forgotten this in a week or so and
we have no other comments please give me a bump!

Thanks,

Jonathan
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/iio/adc/ti-ads1015.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index f76d979..7e21151 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -252,9 +252,11 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  
>  	ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
>  				       ADS1015_CFG_MUX_MASK |
> -				       ADS1015_CFG_PGA_MASK,
> +				       ADS1015_CFG_PGA_MASK |
> +				       ADS1015_CFG_DR_MASK,
>  				       chan << ADS1015_CFG_MUX_SHIFT |
> -				       pga << ADS1015_CFG_PGA_SHIFT,
> +				       pga << ADS1015_CFG_PGA_SHIFT |
> +				       dr << ADS1015_CFG_DR_SHIFT,
>  				       &change);
>  	if (ret < 0)
>  		return ret;
> @@ -325,25 +327,16 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
>  
>  static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)
>  {
> -	int i, ret, rindex = -1;
> +	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++)
> +	for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++) {
>  		if (data->data_rate[i] == rate) {
> -			rindex = i;
> -			break;
> +			data->channel_data[chan].data_rate = i;
> +			return 0;
>  		}
> -	if (rindex < 0)
> -		return -EINVAL;
> -
> -	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				 ADS1015_CFG_DR_MASK,
> -				 rindex << ADS1015_CFG_DR_SHIFT);
> -	if (ret < 0)
> -		return ret;
> -
> -	data->channel_data[chan].data_rate = rindex;
> +	}
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  static int ads1015_read_raw(struct iio_dev *indio_dev,

[PATCH 2/8] iio: adc: ti-ads1015: remove unnecessary config register update

[Akinobu Mita]

The ti-ads1015 driver has eight iio voltage channels and each iio channel
can hold own scale information.

The ADS1015 device only have a single config register which contains an
input multiplexer selection, PGA and data rate settings.  So the driver
should load the correct settings when the input multiplexer selection is
changed.

However, regardless of which channlel is currently selected, changing any
iio channel's scale information immediately overwrites the current PGA
setting in the config register.

It is harmless because the correct PGA settings are reloaded just before
getting adc result anyway.  But it is unnecessary register update and
should be removed.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 7e21151..94c2221 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -303,26 +303,17 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
 static int ads1015_set_scale(struct ads1015_data *data, int chan,
 			     int scale, int uscale)
 {
-	int i, ret, rindex = -1;
+	int i;
 
-	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++)
+	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++) {
 		if (ads1015_scale[i].scale == scale &&
 		    ads1015_scale[i].uscale == uscale) {
-			rindex = i;
-			break;
+			data->channel_data[chan].pga = i;
+			return 0;
 		}
-	if (rindex < 0)
-		return -EINVAL;
-
-	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				 ADS1015_CFG_PGA_MASK,
-				 rindex << ADS1015_CFG_PGA_SHIFT);
-	if (ret < 0)
-		return ret;
-
-	data->channel_data[chan].pga = rindex;
+	}
 
-	return 0;
+	return -EINVAL;
 }
 
 static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)
-- 
2.7.4

Re: [PATCH 2/8] iio: adc: ti-ads1015: remove unnecessary config register update

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:31:58 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The ti-ads1015 driver has eight iio voltage channels and each iio channel
> can hold own scale information.
> 
> The ADS1015 device only have a single config register which contains an
> input multiplexer selection, PGA and data rate settings.  So the driver
> should load the correct settings when the input multiplexer selection is
> changed.
> 
> However, regardless of which channlel is currently selected, changing any
> iio channel's scale information immediately overwrites the current PGA
> setting in the config register.
> 
> It is harmless because the correct PGA settings are reloaded just before
> getting adc result anyway.  But it is unnecessary register update and
> should be removed.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Make sense but this one is as you say harmless so I'm 
not tempted to send it as a fix for stable.   The first
one 'might' go as such.

Input from Daniel still welcome though.
Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 7e21151..94c2221 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -303,26 +303,17 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  static int ads1015_set_scale(struct ads1015_data *data, int chan,
>  			     int scale, int uscale)
>  {
> -	int i, ret, rindex = -1;
> +	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++)
> +	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++) {
>  		if (ads1015_scale[i].scale == scale &&
>  		    ads1015_scale[i].uscale == uscale) {
> -			rindex = i;
> -			break;
> +			data->channel_data[chan].pga = i;
> +			return 0;
>  		}
> -	if (rindex < 0)
> -		return -EINVAL;
> -
> -	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				 ADS1015_CFG_PGA_MASK,
> -				 rindex << ADS1015_CFG_PGA_SHIFT);
> -	if (ret < 0)
> -		return ret;
> -
> -	data->channel_data[chan].pga = rindex;
> +	}
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)

[PATCH 3/8] iio: adc: ti-ads1015: fix scale information for ADS1115

[Akinobu Mita]

The ti-ads1015 driver supports ADS1015 and ADS1115 devices.  The same
scale information is used for both devices in this driver, however they
have actually different values and the ADS1115's one is not correct.

These devices have the same full-scale input voltage range for each PGA
selection.  So instead of adding another hardcoded scale information,
compute a correct scale on demand from each device's resolution.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 47 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 94c2221..1edf323 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -81,18 +81,12 @@ static const unsigned int ads1115_data_rate[] = {
 	8, 16, 32, 64, 128, 250, 475, 860
 };
 
-static const struct {
-	int scale;
-	int uscale;
-} ads1015_scale[] = {
-	{3, 0},
-	{2, 0},
-	{1, 0},
-	{0, 500000},
-	{0, 250000},
-	{0, 125000},
-	{0, 125000},
-	{0, 125000},
+/*
+ * Translation from PGA bits to full-scale positive and negative input voltage
+ * range in mV
+ */
+static int ads1015_fullscale_range[] = {
+	6144, 4096, 2048, 1024, 512, 256, 256, 256
 };
 
 #define ADS1015_V_CHAN(_chan, _addr) {				\
@@ -300,15 +294,17 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-static int ads1015_set_scale(struct ads1015_data *data, int chan,
+static int ads1015_set_scale(struct ads1015_data *data,
+			     struct iio_chan_spec const *chan,
 			     int scale, int uscale)
 {
 	int i;
+	int fullscale = div_s64((scale * 1000000LL + uscale) <<
+				(chan->scan_type.realbits - 1), 1000000);
 
-	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++) {
-		if (ads1015_scale[i].scale == scale &&
-		    ads1015_scale[i].uscale == uscale) {
-			data->channel_data[chan].pga = i;
+	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
+		if (ads1015_fullscale_range[i] == fullscale) {
+			data->channel_data[chan->address].pga = i;
 			return 0;
 		}
 	}
@@ -369,9 +365,9 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 	}
 	case IIO_CHAN_INFO_SCALE:
 		idx = data->channel_data[chan->address].pga;
-		*val = ads1015_scale[idx].scale;
-		*val2 = ads1015_scale[idx].uscale;
-		ret = IIO_VAL_INT_PLUS_MICRO;
+		*val = ads1015_fullscale_range[idx];
+		*val2 = chan->scan_type.realbits - 1;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
 		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		idx = data->channel_data[chan->address].data_rate;
@@ -398,7 +394,7 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
 	mutex_lock(&data->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		ret = ads1015_set_scale(data, chan->address, val, val2);
+		ret = ads1015_set_scale(data, chan, val, val2);
 		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = ads1015_set_data_rate(data, chan->address, val);
@@ -430,7 +426,10 @@ static const struct iio_buffer_setup_ops ads1015_buffer_setup_ops = {
 	.validate_scan_mask = &iio_validate_scan_mask_onehot,
 };
 
-static IIO_CONST_ATTR(scale_available, "3 2 1 0.5 0.25 0.125");
+static IIO_CONST_ATTR_NAMED(ads1015_scale_available, scale_available,
+	"3 2 1 0.5 0.25 0.125");
+static IIO_CONST_ATTR_NAMED(ads1115_scale_available, scale_available,
+	"0.1875 0.125 0.0625 0.03125 0.015625 0.007813");
 
 static IIO_CONST_ATTR_NAMED(ads1015_sampling_frequency_available,
 	sampling_frequency_available, "128 250 490 920 1600 2400 3300");
@@ -438,7 +437,7 @@ static IIO_CONST_ATTR_NAMED(ads1115_sampling_frequency_available,
 	sampling_frequency_available, "8 16 32 64 128 250 475 860");
 
 static struct attribute *ads1015_attributes[] = {
-	&iio_const_attr_scale_available.dev_attr.attr,
+	&iio_const_attr_ads1015_scale_available.dev_attr.attr,
 	&iio_const_attr_ads1015_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
@@ -448,7 +447,7 @@ static const struct attribute_group ads1015_attribute_group = {
 };
 
 static struct attribute *ads1115_attributes[] = {
-	&iio_const_attr_scale_available.dev_attr.attr,
+	&iio_const_attr_ads1115_scale_available.dev_attr.attr,
 	&iio_const_attr_ads1115_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
-- 
2.7.4

Re: [PATCH 3/8] iio: adc: ti-ads1015: fix scale information for ADS1115

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:31:59 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The ti-ads1015 driver supports ADS1015 and ADS1115 devices.  The same
> scale information is used for both devices in this driver, however they
> have actually different values and the ADS1115's one is not correct.
> 
> These devices have the same full-scale input voltage range for each PGA
> selection.  So instead of adding another hardcoded scale information,
> compute a correct scale on demand from each device's resolution.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Looks good to me.  Cc'd to Daniel's current address.
> ---
>  drivers/iio/adc/ti-ads1015.c | 47 ++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 94c2221..1edf323 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -81,18 +81,12 @@ static const unsigned int ads1115_data_rate[] = {
>  	8, 16, 32, 64, 128, 250, 475, 860
>  };
>  
> -static const struct {
> -	int scale;
> -	int uscale;
> -} ads1015_scale[] = {
> -	{3, 0},
> -	{2, 0},
> -	{1, 0},
> -	{0, 500000},
> -	{0, 250000},
> -	{0, 125000},
> -	{0, 125000},
> -	{0, 125000},
> +/*
> + * Translation from PGA bits to full-scale positive and negative input voltage
> + * range in mV
> + */
> +static int ads1015_fullscale_range[] = {
> +	6144, 4096, 2048, 1024, 512, 256, 256, 256
>  };
>  
>  #define ADS1015_V_CHAN(_chan, _addr) {				\
> @@ -300,15 +294,17 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> -static int ads1015_set_scale(struct ads1015_data *data, int chan,
> +static int ads1015_set_scale(struct ads1015_data *data,
> +			     struct iio_chan_spec const *chan,
>  			     int scale, int uscale)
>  {
>  	int i;
> +	int fullscale = div_s64((scale * 1000000LL + uscale) <<
> +				(chan->scan_type.realbits - 1), 1000000);
>  
> -	for (i = 0; i < ARRAY_SIZE(ads1015_scale); i++) {
> -		if (ads1015_scale[i].scale == scale &&
> -		    ads1015_scale[i].uscale == uscale) {
> -			data->channel_data[chan].pga = i;
> +	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
> +		if (ads1015_fullscale_range[i] == fullscale) {
> +			data->channel_data[chan->address].pga = i;
>  			return 0;
>  		}
>  	}
> @@ -369,9 +365,9 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  	}
>  	case IIO_CHAN_INFO_SCALE:
>  		idx = data->channel_data[chan->address].pga;
> -		*val = ads1015_scale[idx].scale;
> -		*val2 = ads1015_scale[idx].uscale;
> -		ret = IIO_VAL_INT_PLUS_MICRO;
> +		*val = ads1015_fullscale_range[idx];
> +		*val2 = chan->scan_type.realbits - 1;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;
>  		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		idx = data->channel_data[chan->address].data_rate;
> @@ -398,7 +394,7 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
>  	mutex_lock(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = ads1015_set_scale(data, chan->address, val, val2);
> +		ret = ads1015_set_scale(data, chan, val, val2);
>  		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = ads1015_set_data_rate(data, chan->address, val);
> @@ -430,7 +426,10 @@ static const struct iio_buffer_setup_ops ads1015_buffer_setup_ops = {
>  	.validate_scan_mask = &iio_validate_scan_mask_onehot,
>  };
>  
> -static IIO_CONST_ATTR(scale_available, "3 2 1 0.5 0.25 0.125");
> +static IIO_CONST_ATTR_NAMED(ads1015_scale_available, scale_available,
> +	"3 2 1 0.5 0.25 0.125");
> +static IIO_CONST_ATTR_NAMED(ads1115_scale_available, scale_available,
> +	"0.1875 0.125 0.0625 0.03125 0.015625 0.007813");
>  
>  static IIO_CONST_ATTR_NAMED(ads1015_sampling_frequency_available,
>  	sampling_frequency_available, "128 250 490 920 1600 2400 3300");
> @@ -438,7 +437,7 @@ static IIO_CONST_ATTR_NAMED(ads1115_sampling_frequency_available,
>  	sampling_frequency_available, "8 16 32 64 128 250 475 860");
>  
>  static struct attribute *ads1015_attributes[] = {
> -	&iio_const_attr_scale_available.dev_attr.attr,
> +	&iio_const_attr_ads1015_scale_available.dev_attr.attr,
>  	&iio_const_attr_ads1015_sampling_frequency_available.dev_attr.attr,
>  	NULL,
>  };
> @@ -448,7 +447,7 @@ static const struct attribute_group ads1015_attribute_group = {
>  };
>  
>  static struct attribute *ads1115_attributes[] = {
> -	&iio_const_attr_scale_available.dev_attr.attr,
> +	&iio_const_attr_ads1115_scale_available.dev_attr.attr,
>  	&iio_const_attr_ads1115_sampling_frequency_available.dev_attr.attr,
>  	NULL,
>  };

[PATCH 4/8] iio: adc: ti-ads1015: enable conversion when CONFIG_PM is not set

[Akinobu Mita]

The ADS1015 device have two operating modes, continuous conversion mode
and single-shot mode.  This driver assumes that the continuous conversion
mode is selected by runtime resume callback when the ADC result is
requested.

If CONFIG_PM is disabled, the device is always in the default single-shot
mode and no one begins a single conversion.  So the conversion register
doesn't contain valid ADC result.  Fix it by changing the continuous mode
in probe function.  This also adds a helper function to set conversion
mode as there are a fair number of users.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 1edf323..984d4bb 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -556,6 +556,13 @@ static void ads1015_get_channels_config(struct i2c_client *client)
 	}
 }
 
+static int ads1015_set_conv_mode(struct ads1015_data *data, int mode)
+{
+	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
+				  ADS1015_CFG_MOD_MASK,
+				  mode << ADS1015_CFG_MOD_SHIFT);
+}
+
 static int ads1015_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -613,6 +620,11 @@ static int ads1015_probe(struct i2c_client *client,
 		dev_err(&client->dev, "iio triggered buffer setup failed\n");
 		return ret;
 	}
+
+	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
+	if (ret)
+		return ret;
+
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret)
 		goto err_buffer_cleanup;
@@ -648,9 +660,7 @@ static int ads1015_remove(struct i2c_client *client)
 	iio_triggered_buffer_cleanup(indio_dev);
 
 	/* power down single shot mode */
-	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				  ADS1015_CFG_MOD_MASK,
-				  ADS1015_SINGLESHOT << ADS1015_CFG_MOD_SHIFT);
+	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
 }
 
 #ifdef CONFIG_PM
@@ -659,9 +669,7 @@ static int ads1015_runtime_suspend(struct device *dev)
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct ads1015_data *data = iio_priv(indio_dev);
 
-	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				  ADS1015_CFG_MOD_MASK,
-				  ADS1015_SINGLESHOT << ADS1015_CFG_MOD_SHIFT);
+	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
 }
 
 static int ads1015_runtime_resume(struct device *dev)
@@ -669,9 +677,7 @@ static int ads1015_runtime_resume(struct device *dev)
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct ads1015_data *data = iio_priv(indio_dev);
 
-	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				  ADS1015_CFG_MOD_MASK,
-				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
+	return ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
 }
 #endif
 
-- 
2.7.4

Re: [PATCH 4/8] iio: adc: ti-ads1015: enable conversion when CONFIG_PM is not set

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:32:00 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The ADS1015 device have two operating modes, continuous conversion mode
> and single-shot mode.  This driver assumes that the continuous conversion
> mode is selected by runtime resume callback when the ADC result is
> requested.
> 
> If CONFIG_PM is disabled, the device is always in the default single-shot
> mode and no one begins a single conversion.  So the conversion register
> doesn't contain valid ADC result.  Fix it by changing the continuous mode
> in probe function.  This also adds a helper function to set conversion
> mode as there are a fair number of users.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Two patches please.  First one is a fix to make it work without config_pm
second can do the refactor (and can be applied after the first one
has gone to stable and worked it's way back to our upstream).

Good catch,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 1edf323..984d4bb 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -556,6 +556,13 @@ static void ads1015_get_channels_config(struct i2c_client *client)
>  	}
>  }
>  
> +static int ads1015_set_conv_mode(struct ads1015_data *data, int mode)
> +{
> +	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> +				  ADS1015_CFG_MOD_MASK,
> +				  mode << ADS1015_CFG_MOD_SHIFT);
> +}
> +
>  static int ads1015_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -613,6 +620,11 @@ static int ads1015_probe(struct i2c_client *client,
>  		dev_err(&client->dev, "iio triggered buffer setup failed\n");
>  		return ret;
>  	}
> +
> +	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
> +	if (ret)
> +		return ret;
> +
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret)
>  		goto err_buffer_cleanup;
> @@ -648,9 +660,7 @@ static int ads1015_remove(struct i2c_client *client)
>  	iio_triggered_buffer_cleanup(indio_dev);
>  
>  	/* power down single shot mode */
> -	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				  ADS1015_CFG_MOD_MASK,
> -				  ADS1015_SINGLESHOT << ADS1015_CFG_MOD_SHIFT);
> +	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -659,9 +669,7 @@ static int ads1015_runtime_suspend(struct device *dev)
>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>  	struct ads1015_data *data = iio_priv(indio_dev);
>  
> -	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				  ADS1015_CFG_MOD_MASK,
> -				  ADS1015_SINGLESHOT << ADS1015_CFG_MOD_SHIFT);
> +	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
>  }
>  
>  static int ads1015_runtime_resume(struct device *dev)
> @@ -669,9 +677,7 @@ static int ads1015_runtime_resume(struct device *dev)
>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>  	struct ads1015_data *data = iio_priv(indio_dev);
>  
> -	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				  ADS1015_CFG_MOD_MASK,
> -				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
> +	return ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
>  }
>  #endif
>  

[PATCH 5/8] iio: adc: ti-ads1015: avoid getting stale result after runtime resume

[Akinobu Mita]

This driver assumes that the device is operating in the continuous
conversion mode which performs the conversion continuously.  So this driver
doesn't insert a wait time before reading the conversion register if the
configuration is not changed from a previous request.

This assumption is broken if the device is runtime suspended and entered
a power-down state.  The forthcoming request causes reading a stale result
from the conversion register as the device is runtime resumed just before.

Fix it by adding a flag to detect that condition and insert a necessary
wait time.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 984d4bb..f5f256a 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -177,6 +177,12 @@ struct ads1015_data {
 	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 
 	unsigned int *data_rate;
+	/*
+	 * Set to true when the ADC is switched to the continuous-conversion
+	 * mode and exits from a power-down state.  This flag is used to avoid
+	 * getting the stale result from the conversion register.
+	 */
+	bool conv_invalid;
 };
 
 static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
@@ -255,9 +261,10 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 	if (ret < 0)
 		return ret;
 
-	if (change) {
+	if (change || data->conv_invalid) {
 		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
 		usleep_range(conv_time, conv_time + 1);
+		data->conv_invalid = false;
 	}
 
 	return regmap_read(data->regmap, ADS1015_CONV_REG, val);
@@ -625,6 +632,8 @@ static int ads1015_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	data->conv_invalid = true;
+
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret)
 		goto err_buffer_cleanup;
@@ -676,8 +685,13 @@ static int ads1015_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct ads1015_data *data = iio_priv(indio_dev);
+	int ret;
 
-	return ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
+	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
+	if (!ret)
+		data->conv_invalid = true;
+
+	return ret;
 }
 #endif
 
-- 
2.7.4

Re: [PATCH 5/8] iio: adc: ti-ads1015: avoid getting stale result after runtime resume

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:32:01 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> This driver assumes that the device is operating in the continuous
> conversion mode which performs the conversion continuously.  So this driver
> doesn't insert a wait time before reading the conversion register if the
> configuration is not changed from a previous request.
> 
> This assumption is broken if the device is runtime suspended and entered
> a power-down state.  The forthcoming request causes reading a stale result
> from the conversion register as the device is runtime resumed just before.
> 
> Fix it by adding a flag to detect that condition and insert a necessary
> wait time.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Another one that wants to be near the head of the series as a fix
that we want to get into stable.

Good catch.

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 984d4bb..f5f256a 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -177,6 +177,12 @@ struct ads1015_data {
>  	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>  
>  	unsigned int *data_rate;
> +	/*
> +	 * Set to true when the ADC is switched to the continuous-conversion
> +	 * mode and exits from a power-down state.  This flag is used to avoid
> +	 * getting the stale result from the conversion register.
> +	 */
> +	bool conv_invalid;
>  };
>  
>  static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
> @@ -255,9 +261,10 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (change) {
> +	if (change || data->conv_invalid) {
>  		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
>  		usleep_range(conv_time, conv_time + 1);
> +		data->conv_invalid = false;
>  	}
>  
>  	return regmap_read(data->regmap, ADS1015_CONV_REG, val);
> @@ -625,6 +632,8 @@ static int ads1015_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	data->conv_invalid = true;
> +
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret)
>  		goto err_buffer_cleanup;
> @@ -676,8 +685,13 @@ static int ads1015_runtime_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>  	struct ads1015_data *data = iio_priv(indio_dev);
> +	int ret;
>  
> -	return ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
> +	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
> +	if (!ret)
> +		data->conv_invalid = true;
> +
> +	return ret;
>  }
>  #endif
>  

[PATCH 6/8] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup

[Akinobu Mita]

Use devm_iio_triggered_buffer_setup to simplify the error path in the
probe() and remove() function.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index f5f256a..821e649 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -620,9 +620,9 @@ static int ads1015_probe(struct i2c_client *client,
 		return PTR_ERR(data->regmap);
 	}
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-					 ads1015_trigger_handler,
-					 &ads1015_buffer_setup_ops);
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+					      ads1015_trigger_handler,
+					      &ads1015_buffer_setup_ops);
 	if (ret < 0) {
 		dev_err(&client->dev, "iio triggered buffer setup failed\n");
 		return ret;
@@ -636,7 +636,7 @@ static int ads1015_probe(struct i2c_client *client,
 
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret)
-		goto err_buffer_cleanup;
+		return ret;
 	pm_runtime_set_autosuspend_delay(&client->dev, ADS1015_SLEEP_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 	pm_runtime_enable(&client->dev);
@@ -644,15 +644,10 @@ static int ads1015_probe(struct i2c_client *client,
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to register IIO device\n");
-		goto err_buffer_cleanup;
+		return ret;
 	}
 
 	return 0;
-
-err_buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-
-	return ret;
 }
 
 static int ads1015_remove(struct i2c_client *client)
@@ -666,8 +661,6 @@ static int ads1015_remove(struct i2c_client *client)
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
 
-	iio_triggered_buffer_cleanup(indio_dev);
-
 	/* power down single shot mode */
 	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
 }
-- 
2.7.4

Re: [PATCH 6/8] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:32:02 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> Use devm_iio_triggered_buffer_setup to simplify the error path in the
> probe() and remove() function.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/iio/adc/ti-ads1015.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index f5f256a..821e649 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -620,9 +620,9 @@ static int ads1015_probe(struct i2c_client *client,
>  		return PTR_ERR(data->regmap);
>  	}
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> -					 ads1015_trigger_handler,
> -					 &ads1015_buffer_setup_ops);
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      ads1015_trigger_handler,
> +					      &ads1015_buffer_setup_ops);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "iio triggered buffer setup failed\n");
>  		return ret;
> @@ -636,7 +636,7 @@ static int ads1015_probe(struct i2c_client *client,
>  
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret)
> -		goto err_buffer_cleanup;
> +		return ret;
>  	pm_runtime_set_autosuspend_delay(&client->dev, ADS1015_SLEEP_DELAY_MS);
>  	pm_runtime_use_autosuspend(&client->dev);
>  	pm_runtime_enable(&client->dev);
> @@ -644,15 +644,10 @@ static int ads1015_probe(struct i2c_client *client,
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "Failed to register IIO device\n");
> -		goto err_buffer_cleanup;
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -err_buffer_cleanup:
> -	iio_triggered_buffer_cleanup(indio_dev);
> -
> -	return ret;
>  }
>  
>  static int ads1015_remove(struct i2c_client *client)
> @@ -666,8 +661,6 @@ static int ads1015_remove(struct i2c_client *client)
>  	pm_runtime_set_suspended(&client->dev);
>  	pm_runtime_put_noidle(&client->dev);
>  
> -	iio_triggered_buffer_cleanup(indio_dev);
> -
>  	/* power down single shot mode */
>  	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
Hmm. This does technically change the remove order to not quite be
the reverse of probe.. So I'm not keen from the 'obviously
right' angle rather than because I actually think there is any
problem with doing this.

Jonathan
>  }

Re: [PATCH 6/8] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup

[Akinobu Mita]

2017-07-15 21:28 GMT+09:00 Jonathan Cameron <jic23@kernel.org>:
> On Thu, 13 Jul 2017 01:32:02 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> Use devm_iio_triggered_buffer_setup to simplify the error path in the
>> probe() and remove() function.
>>
>> Cc: Daniel Baluta <daniel.baluta@intel.com>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  drivers/iio/adc/ti-ads1015.c | 17 +++++------------
>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
>> index f5f256a..821e649 100644
>> --- a/drivers/iio/adc/ti-ads1015.c
>> +++ b/drivers/iio/adc/ti-ads1015.c
>> @@ -620,9 +620,9 @@ static int ads1015_probe(struct i2c_client *client,
>>               return PTR_ERR(data->regmap);
>>       }
>>
>> -     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> -                                      ads1015_trigger_handler,
>> -                                      &ads1015_buffer_setup_ops);
>> +     ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>> +                                           ads1015_trigger_handler,
>> +                                           &ads1015_buffer_setup_ops);
>>       if (ret < 0) {
>>               dev_err(&client->dev, "iio triggered buffer setup failed\n");
>>               return ret;
>> @@ -636,7 +636,7 @@ static int ads1015_probe(struct i2c_client *client,
>>
>>       ret = pm_runtime_set_active(&client->dev);
>>       if (ret)
>> -             goto err_buffer_cleanup;
>> +             return ret;
>>       pm_runtime_set_autosuspend_delay(&client->dev, ADS1015_SLEEP_DELAY_MS);
>>       pm_runtime_use_autosuspend(&client->dev);
>>       pm_runtime_enable(&client->dev);
>> @@ -644,15 +644,10 @@ static int ads1015_probe(struct i2c_client *client,
>>       ret = iio_device_register(indio_dev);
>>       if (ret < 0) {
>>               dev_err(&client->dev, "Failed to register IIO device\n");
>> -             goto err_buffer_cleanup;
>> +             return ret;
>>       }
>>
>>       return 0;
>> -
>> -err_buffer_cleanup:
>> -     iio_triggered_buffer_cleanup(indio_dev);
>> -
>> -     return ret;
>>  }
>>
>>  static int ads1015_remove(struct i2c_client *client)
>> @@ -666,8 +661,6 @@ static int ads1015_remove(struct i2c_client *client)
>>       pm_runtime_set_suspended(&client->dev);
>>       pm_runtime_put_noidle(&client->dev);
>>
>> -     iio_triggered_buffer_cleanup(indio_dev);
>> -
>>       /* power down single shot mode */
>>       return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
> Hmm. This does technically change the remove order to not quite be
> the reverse of probe.. So I'm not keen from the 'obviously
> right' angle rather than because I actually think there is any
> problem with doing this.

Yes, this patch changes the remove order.  But the end result of this
change does the reverse of probe, doesn't it?  or am I missing something?

ads1015_probe
- ...
- devm_regmap_init_i2c
- devm_iio_triggered_buffer_setup
- ads1015_set_conv_mode(CONTINUOUS)
- pm_runtime_*
- iio_device_register

ads1015_remove
- iio_device_unregister
- pm_runtime_*
- ads1015_set_conv_mode(SINGLESHOT)

Re: [PATCH 6/8] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup

[Jonathan Cameron]

On Mon, 17 Jul 2017 01:02:45 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> 2017-07-15 21:28 GMT+09:00 Jonathan Cameron <jic23@kernel.org>:
> > On Thu, 13 Jul 2017 01:32:02 +0900
> > Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >  
> >> Use devm_iio_triggered_buffer_setup to simplify the error path in the
> >> probe() and remove() function.
> >>
> >> Cc: Daniel Baluta <daniel.baluta@intel.com>
> >> Cc: Jonathan Cameron <jic23@kernel.org>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> ---
> >>  drivers/iio/adc/ti-ads1015.c | 17 +++++------------
> >>  1 file changed, 5 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> index f5f256a..821e649 100644
> >> --- a/drivers/iio/adc/ti-ads1015.c
> >> +++ b/drivers/iio/adc/ti-ads1015.c
> >> @@ -620,9 +620,9 @@ static int ads1015_probe(struct i2c_client *client,
> >>               return PTR_ERR(data->regmap);
> >>       }
> >>
> >> -     ret = iio_triggered_buffer_setup(indio_dev, NULL,
> >> -                                      ads1015_trigger_handler,
> >> -                                      &ads1015_buffer_setup_ops);
> >> +     ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> >> +                                           ads1015_trigger_handler,
> >> +                                           &ads1015_buffer_setup_ops);
> >>       if (ret < 0) {
> >>               dev_err(&client->dev, "iio triggered buffer setup failed\n");
> >>               return ret;
> >> @@ -636,7 +636,7 @@ static int ads1015_probe(struct i2c_client *client,
> >>
> >>       ret = pm_runtime_set_active(&client->dev);
> >>       if (ret)
> >> -             goto err_buffer_cleanup;
> >> +             return ret;
> >>       pm_runtime_set_autosuspend_delay(&client->dev, ADS1015_SLEEP_DELAY_MS);
> >>       pm_runtime_use_autosuspend(&client->dev);
> >>       pm_runtime_enable(&client->dev);
> >> @@ -644,15 +644,10 @@ static int ads1015_probe(struct i2c_client *client,
> >>       ret = iio_device_register(indio_dev);
> >>       if (ret < 0) {
> >>               dev_err(&client->dev, "Failed to register IIO device\n");
> >> -             goto err_buffer_cleanup;
> >> +             return ret;
> >>       }
> >>
> >>       return 0;
> >> -
> >> -err_buffer_cleanup:
> >> -     iio_triggered_buffer_cleanup(indio_dev);
> >> -
> >> -     return ret;
> >>  }
> >>
> >>  static int ads1015_remove(struct i2c_client *client)
> >> @@ -666,8 +661,6 @@ static int ads1015_remove(struct i2c_client *client)
> >>       pm_runtime_set_suspended(&client->dev);
> >>       pm_runtime_put_noidle(&client->dev);
> >>
> >> -     iio_triggered_buffer_cleanup(indio_dev);
> >> -
> >>       /* power down single shot mode */
> >>       return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);  
> > Hmm. This does technically change the remove order to not quite be
> > the reverse of probe.. So I'm not keen from the 'obviously
> > right' angle rather than because I actually think there is any
> > problem with doing this.  
> 
> Yes, this patch changes the remove order.  But the end result of this
> change does the reverse of probe, doesn't it?  or am I missing something?
ah. I failed to note the ordering was wrong previously!

This is fine then.

Jonathan
> 
> ads1015_probe
> - ...
> - devm_regmap_init_i2c
> - devm_iio_triggered_buffer_setup
> - ads1015_set_conv_mode(CONTINUOUS)
> - pm_runtime_*
> - iio_device_register
> 
> ads1015_remove
> - iio_device_unregister
> - pm_runtime_*
> - ads1015_set_conv_mode(SINGLESHOT)
> --
> 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 7/8] iio: adc: ti-ads1015: use iio_device_claim_direct_mode()

[Akinobu Mita]

While the iio buffer for the ti-ads1015 driver is enabled, reading the
raw ADC channel data is restricted.  We usually use the
iio_device_claim_direct_mode()/iio_device_release_direct_mode() pair for
that.

This change consequently reverses the locking order for the driver's
private lock and indio_dev->mlock which acquired by
iio_device_claim_direct_mode() internally. But it's safe because there is
no other dependency between these locks.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 821e649..dfeb3af 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -340,34 +340,34 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 	int ret, idx;
 	struct ads1015_data *data = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	mutex_lock(&data->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW: {
 		int shift = chan->scan_type.shift;
 
-		if (iio_buffer_enabled(indio_dev)) {
-			ret = -EBUSY;
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
 			break;
-		}
 
 		ret = ads1015_set_power_state(data, true);
 		if (ret < 0)
-			break;
+			goto release_direct;
 
 		ret = ads1015_get_adc_result(data, chan->address, val);
 		if (ret < 0) {
 			ads1015_set_power_state(data, false);
-			break;
+			goto release_direct;
 		}
 
 		*val = sign_extend32(*val >> shift, 15 - shift);
 
 		ret = ads1015_set_power_state(data, false);
 		if (ret < 0)
-			break;
+			goto release_direct;
 
 		ret = IIO_VAL_INT;
+release_direct:
+		iio_device_release_direct_mode(indio_dev);
 		break;
 	}
 	case IIO_CHAN_INFO_SCALE:
@@ -386,7 +386,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 		break;
 	}
 	mutex_unlock(&data->lock);
-	mutex_unlock(&indio_dev->mlock);
 
 	return ret;
 }
-- 
2.7.4

Re: [PATCH 7/8] iio: adc: ti-ads1015: use iio_device_claim_direct_mode()

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:32:03 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> While the iio buffer for the ti-ads1015 driver is enabled, reading the
> raw ADC channel data is restricted.  We usually use the
> iio_device_claim_direct_mode()/iio_device_release_direct_mode() pair for
> that.
> 
> This change consequently reverses the locking order for the driver's
> private lock and indio_dev->mlock which acquired by
> iio_device_claim_direct_mode() internally. But it's safe because there is
> no other dependency between these locks.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Sensible cleanup.  Will pick up when rest are sorted.

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 821e649..dfeb3af 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -340,34 +340,34 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  	int ret, idx;
>  	struct ads1015_data *data = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
>  	mutex_lock(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW: {
>  		int shift = chan->scan_type.shift;
>  
> -		if (iio_buffer_enabled(indio_dev)) {
> -			ret = -EBUSY;
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
>  			break;
> -		}
>  
>  		ret = ads1015_set_power_state(data, true);
>  		if (ret < 0)
> -			break;
> +			goto release_direct;
>  
>  		ret = ads1015_get_adc_result(data, chan->address, val);
>  		if (ret < 0) {
>  			ads1015_set_power_state(data, false);
> -			break;
> +			goto release_direct;
>  		}
>  
>  		*val = sign_extend32(*val >> shift, 15 - shift);
>  
>  		ret = ads1015_set_power_state(data, false);
>  		if (ret < 0)
> -			break;
> +			goto release_direct;
>  
>  		ret = IIO_VAL_INT;
> +release_direct:
> +		iio_device_release_direct_mode(indio_dev);
>  		break;
>  	}
>  	case IIO_CHAN_INFO_SCALE:
> @@ -386,7 +386,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  		break;
>  	}
>  	mutex_unlock(&data->lock);
> -	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret;
>  }

[PATCH 8/8] iio: adc: ti-ads1015: add threshold event support

[Akinobu Mita]

The ADS1015 device provides programmable comparator that can issue an
interrupt on the ALERT pin.  This change adds the iio threshold event
support for that feature.

The ADS1015 device only have a single config register which contains an
input multiplexer selection, comparator settings, and etc.  So only a
single event channel can be enabled at a time.  Also enabling both buffer
and event are prohibited for simplicity.

Cc: Daniel Baluta <daniel.baluta@intel.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 379 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 367 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index dfeb3af..d735e855 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/init.h>
+#include <linux/irq.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/pm_runtime.h>
@@ -28,6 +29,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
@@ -36,17 +38,38 @@
 
 #define ADS1015_CONV_REG	0x00
 #define ADS1015_CFG_REG		0x01
+#define ADS1015_LO_THRESH_REG	0x02
+#define ADS1015_HI_THRESH_REG	0x03
 
+#define ADS1015_CFG_COMP_QUE_SHIFT	0
+#define ADS1015_CFG_COMP_LAT_SHIFT	2
+#define ADS1015_CFG_COMP_POL_SHIFT	3
+#define ADS1015_CFG_COMP_MODE_SHIFT	4
 #define ADS1015_CFG_DR_SHIFT	5
 #define ADS1015_CFG_MOD_SHIFT	8
 #define ADS1015_CFG_PGA_SHIFT	9
 #define ADS1015_CFG_MUX_SHIFT	12
 
+#define ADS1015_CFG_COMP_QUE_MASK	GENMASK(1, 0)
+#define ADS1015_CFG_COMP_LAT_MASK	BIT(2)
+#define ADS1015_CFG_COMP_POL_MASK	BIT(2)
+#define ADS1015_CFG_COMP_MODE_MASK	BIT(4)
 #define ADS1015_CFG_DR_MASK	GENMASK(7, 5)
 #define ADS1015_CFG_MOD_MASK	BIT(8)
 #define ADS1015_CFG_PGA_MASK	GENMASK(11, 9)
 #define ADS1015_CFG_MUX_MASK	GENMASK(14, 12)
 
+/* Comparator queue and disable field */
+#define ADS1015_CFG_COMP_DISABLE	3
+
+/* Comparator polarity field */
+#define ADS1015_CFG_COMP_POL_LOW	0
+#define ADS1015_CFG_COMP_POL_HIGH	1
+
+/* Comparator mode field */
+#define ADS1015_CFG_COMP_MODE_TRAD	0
+#define ADS1015_CFG_COMP_MODE_WINDOW	1
+
 /* device operating modes */
 #define ADS1015_CONTINUOUS	0
 #define ADS1015_SINGLESHOT	1
@@ -89,6 +112,29 @@ static int ads1015_fullscale_range[] = {
 	6144, 4096, 2048, 1024, 512, 256, 256, 256
 };
 
+/*
+ * Translation from COMP_QUE field value to the number of successive readings
+ * exceed the threshold values before an interrupt is generated
+ */
+static const int ads1015_comp_queue[] = { 1, 2, 4 };
+
+static const struct iio_event_spec ads1015_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				BIT(IIO_EV_INFO_PERIOD),
+	},
+};
+
 #define ADS1015_V_CHAN(_chan, _addr) {				\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
@@ -105,6 +151,8 @@ static int ads1015_fullscale_range[] = {
 		.shift = 4,					\
 		.endianness = IIO_CPU,				\
 	},							\
+	.event_spec = ads1015_events,				\
+	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
 	.datasheet_name = "AIN"#_chan,				\
 }
 
@@ -126,6 +174,8 @@ static int ads1015_fullscale_range[] = {
 		.shift = 4,					\
 		.endianness = IIO_CPU,				\
 	},							\
+	.event_spec = ads1015_events,				\
+	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
 	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
 }
 
@@ -144,6 +194,8 @@ static int ads1015_fullscale_range[] = {
 		.storagebits = 16,				\
 		.endianness = IIO_CPU,				\
 	},							\
+	.event_spec = ads1015_events,				\
+	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
 	.datasheet_name = "AIN"#_chan,				\
 }
 
@@ -164,9 +216,17 @@ static int ads1015_fullscale_range[] = {
 		.storagebits = 16,				\
 		.endianness = IIO_CPU,				\
 	},							\
+	.event_spec = ads1015_events,				\
+	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
 	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
 }
 
+struct ads1015_thresh_data {
+	unsigned int comp_queue;
+	unsigned int high_thresh;
+	unsigned int low_thresh;
+};
+
 struct ads1015_data {
 	struct regmap *regmap;
 	/*
@@ -176,6 +236,9 @@ struct ads1015_data {
 	struct mutex lock;
 	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 
+	unsigned int event_channel;
+	struct ads1015_thresh_data thresh_data[ADS1015_CHANNELS];
+
 	unsigned int *data_rate;
 	/*
 	 * Set to true when the ADC is switched to the continuous-conversion
@@ -185,15 +248,39 @@ struct ads1015_data {
 	bool conv_invalid;
 };
 
+static bool ads1015_event_channel_enabled(struct ads1015_data *data)
+{
+	return (data->event_channel != ADS1015_CHANNELS);
+}
+
+static void ads1015_event_channel_enable(struct ads1015_data *data, int chan)
+{
+	WARN_ON(ads1015_event_channel_enabled(data));
+
+	data->event_channel = chan;
+}
+
+static void ads1015_event_channel_disable(struct ads1015_data *data, int chan)
+{
+	data->event_channel = ADS1015_CHANNELS;
+}
+
 static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
 {
-	return (reg == ADS1015_CFG_REG);
+	switch (reg) {
+	case ADS1015_CFG_REG:
+	case ADS1015_LO_THRESH_REG:
+	case ADS1015_HI_THRESH_REG:
+		return true;
+	default:
+		return false;
+	}
 }
 
 static const struct regmap_config ads1015_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
-	.max_register = ADS1015_CFG_REG,
+	.max_register = ADS1015_HI_THRESH_REG,
 	.writeable_reg = ads1015_is_writeable_reg,
 };
 
@@ -238,26 +325,38 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 	return ret;
 }
 
+int ads1015_config(struct ads1015_data *data, int chan, bool *change)
+{
+	unsigned int cfg_mask, cfg_value;
+
+	cfg_mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
+		   ADS1015_CFG_DR_MASK;
+	cfg_value = chan << ADS1015_CFG_MUX_SHIFT |
+		    data->channel_data[chan].pga << ADS1015_CFG_PGA_SHIFT |
+		    data->channel_data[chan].data_rate << ADS1015_CFG_DR_SHIFT;
+
+	if (ads1015_event_channel_enabled(data)) {
+		cfg_mask |= ADS1015_CFG_COMP_QUE_MASK;
+		cfg_value |= data->thresh_data[chan].comp_queue <<
+				ADS1015_CFG_COMP_QUE_SHIFT;
+	}
+
+	return regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
+					cfg_mask, cfg_value, change);
+}
+
 static
 int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 {
-	int ret, pga, dr, conv_time;
+	int ret, dr, conv_time;
 	bool change;
 
 	if (chan < 0 || chan >= ADS1015_CHANNELS)
 		return -EINVAL;
 
-	pga = data->channel_data[chan].pga;
 	dr = data->channel_data[chan].data_rate;
 
-	ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
-				       ADS1015_CFG_MUX_MASK |
-				       ADS1015_CFG_PGA_MASK |
-				       ADS1015_CFG_DR_MASK,
-				       chan << ADS1015_CFG_MUX_SHIFT |
-				       pga << ADS1015_CFG_PGA_SHIFT |
-				       dr << ADS1015_CFG_DR_SHIFT,
-				       &change);
+	ret = ads1015_config(data, chan, &change);
 	if (ret < 0)
 		return ret;
 
@@ -349,6 +448,12 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			break;
 
+		if (ads1015_event_channel_enabled(data) &&
+				data->event_channel != chan->address) {
+			ret = -EBUSY;
+			goto release_direct;
+		}
+
 		ret = ads1015_set_power_state(data, true);
 		if (ret < 0)
 			goto release_direct;
@@ -414,8 +519,207 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int ads1015_read_event(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int *val,
+	int *val2)
+{
+	struct ads1015_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int comp_queue;
+	int period;
+	int dr;
+
+	mutex_lock(&data->lock);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		*val = (dir == IIO_EV_DIR_RISING) ?
+			data->thresh_data[chan->address].high_thresh :
+			data->thresh_data[chan->address].low_thresh;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_EV_INFO_PERIOD:
+		dr = data->channel_data[chan->address].data_rate;
+		comp_queue = data->thresh_data[chan->address].comp_queue;
+		period = ads1015_comp_queue[comp_queue] *
+			USEC_PER_SEC / data->data_rate[dr];
+
+		*val = period / USEC_PER_SEC;
+		*val2 = period % USEC_PER_SEC;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int ads1015_write_event(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int val,
+	int val2)
+{
+	struct ads1015_data *data = iio_priv(indio_dev);
+	int realbits = chan->scan_type.realbits;
+	int ret = 0;
+	long long period;
+	int i;
+	int dr;
+
+	mutex_lock(&data->lock);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (val >= 1 << (realbits - 1) || val < -1 << (realbits - 1)) {
+			ret = -EINVAL;
+			break;
+		}
+		if (dir == IIO_EV_DIR_RISING)
+			data->thresh_data[chan->address].high_thresh = val;
+		else
+			data->thresh_data[chan->address].low_thresh = val;
+		break;
+	case IIO_EV_INFO_PERIOD:
+		dr = data->channel_data[chan->address].data_rate;
+		period = val * USEC_PER_SEC + val2;
+
+		for (i = 0; i < ARRAY_SIZE(ads1015_comp_queue) - 1; i++) {
+			if (period <= ads1015_comp_queue[i] *
+					USEC_PER_SEC / data->data_rate[dr])
+				break;
+		}
+		data->thresh_data[chan->address].comp_queue = i;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int ads1015_read_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir)
+{
+	struct ads1015_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = (data->event_channel == chan->address);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int ads1015_write_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, int state)
+{
+	struct ads1015_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	/* Prevent from enabling both buffer and event at a time */
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret) {
+		mutex_unlock(&data->lock);
+		return ret;
+	}
+
+	if (ads1015_event_channel_enabled(data) == state) {
+		ret = -EBUSY;
+		goto error;
+	}
+
+	if (ads1015_event_channel_enabled(data) &&
+			data->event_channel != chan->address) {
+		ret = -EBUSY;
+		goto error;
+	}
+
+	if (state) {
+		bool change;
+
+		ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
+				data->thresh_data[chan->address].low_thresh <<
+				chan->scan_type.shift);
+		if (ret)
+			goto error;
+
+		ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
+				data->thresh_data[chan->address].high_thresh <<
+				chan->scan_type.shift);
+		if (ret)
+			goto error;
+
+		ret = ads1015_set_power_state(data, true);
+		if (ret < 0)
+			goto error;
+
+		ads1015_event_channel_enable(data, chan->address);
+
+		ret = ads1015_config(data, chan->address, &change);
+		if (ret) {
+			ads1015_event_channel_disable(data, chan->address);
+			ads1015_set_power_state(data, false);
+		}
+	} else {
+		ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
+					ADS1015_CFG_COMP_QUE_MASK,
+					ADS1015_CFG_COMP_DISABLE <<
+						ADS1015_CFG_COMP_QUE_SHIFT);
+		if (ret)
+			goto error;
+
+		ads1015_event_channel_disable(data, chan->address);
+
+		ret = ads1015_set_power_state(data, false);
+	}
+error:
+	iio_device_release_direct_mode(indio_dev);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static irqreturn_t ads1015_event_handler(int irq, void *priv)
+{
+	struct iio_dev *indio_dev = priv;
+	struct ads1015_data *data = iio_priv(indio_dev);
+	int val;
+
+	/* Clear the latched ALERT/RDY pin */
+	regmap_read(data->regmap, ADS1015_CONV_REG, &val);
+
+	if (ads1015_event_channel_enabled(data)) {
+		u64 code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
+						data->event_channel,
+						IIO_EV_TYPE_THRESH,
+						IIO_EV_DIR_EITHER);
+
+		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int ads1015_buffer_preenable(struct iio_dev *indio_dev)
 {
+	struct ads1015_data *data = iio_priv(indio_dev);
+
+	/* Prevent from enabling both buffer and event at a time */
+	if (ads1015_event_channel_enabled(data))
+		return -EBUSY;
+
 	return ads1015_set_power_state(iio_priv(indio_dev), true);
 }
 
@@ -466,6 +770,10 @@ static const struct iio_info ads1015_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw	= ads1015_read_raw,
 	.write_raw	= ads1015_write_raw,
+	.read_event_value = ads1015_read_event,
+	.write_event_value = ads1015_write_event,
+	.read_event_config = ads1015_read_event_config,
+	.write_event_config = ads1015_write_event_config,
 	.attrs          = &ads1015_attribute_group,
 };
 
@@ -473,6 +781,10 @@ static const struct iio_info ads1115_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw	= ads1015_read_raw,
 	.write_raw	= ads1015_write_raw,
+	.read_event_value = ads1015_read_event,
+	.write_event_value = ads1015_write_event,
+	.read_event_config = ads1015_read_event_config,
+	.write_event_config = ads1015_write_event_config,
 	.attrs          = &ads1115_attribute_group,
 };
 
@@ -576,6 +888,7 @@ static int ads1015_probe(struct i2c_client *client,
 	struct ads1015_data *data;
 	int ret;
 	enum chip_ids chip;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -610,6 +923,12 @@ static int ads1015_probe(struct i2c_client *client,
 		break;
 	}
 
+	data->event_channel = ADS1015_CHANNELS;
+	for (i = 0; i < ADS1015_CHANNELS; i++) {
+		data->thresh_data[i].high_thresh =
+		(1 << (indio_dev->channels[i].scan_type.realbits - 1)) - 1;
+	}
+
 	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
 	ads1015_get_channels_config(client);
 
@@ -627,6 +946,42 @@ static int ads1015_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (client->irq) {
+		unsigned long irq_trig =
+			irqd_get_trigger_type(irq_get_irq_data(client->irq));
+		unsigned int cfg_comp_mask = ADS1015_CFG_COMP_QUE_MASK |
+			ADS1015_CFG_COMP_LAT_MASK | ADS1015_CFG_COMP_POL_MASK |
+			ADS1015_CFG_COMP_MODE_MASK;
+		unsigned int cfg_comp =
+			ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
+			1 << ADS1015_CFG_COMP_LAT_SHIFT |
+			ADS1015_CFG_COMP_MODE_WINDOW <<
+				ADS1015_CFG_COMP_MODE_SHIFT;
+
+		switch (irq_trig) {
+		case IRQF_TRIGGER_LOW:
+			cfg_comp |= ADS1015_CFG_COMP_POL_LOW;
+			break;
+		case IRQF_TRIGGER_HIGH:
+			cfg_comp |= ADS1015_CFG_COMP_POL_HIGH;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
+					cfg_comp_mask, cfg_comp);
+		if (ret)
+			return ret;
+
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, ads1015_event_handler,
+						irq_trig | IRQF_ONESHOT,
+						client->name, indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
 	if (ret)
 		return ret;
-- 
2.7.4

Re: [PATCH 8/8] iio: adc: ti-ads1015: add threshold event support

[Jonathan Cameron]

On Thu, 13 Jul 2017 01:32:04 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> The ADS1015 device provides programmable comparator that can issue an
> interrupt on the ALERT pin.  This change adds the iio threshold event
> support for that feature.
> 
> The ADS1015 device only have a single config register which contains an
> input multiplexer selection, comparator settings, and etc.  So only a
> single event channel can be enabled at a time.  Also enabling both buffer
> and event are prohibited for simplicity.
> 
> Cc: Daniel Baluta <daniel.baluta@intel.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Few comments inline, but looks pretty good in general.

THanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 379 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 367 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index dfeb3af..d735e855 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/init.h>
> +#include <linux/irq.h>
>  #include <linux/i2c.h>
>  #include <linux/regmap.h>
>  #include <linux/pm_runtime.h>
> @@ -28,6 +29,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
> @@ -36,17 +38,38 @@
>  
>  #define ADS1015_CONV_REG	0x00
>  #define ADS1015_CFG_REG		0x01
> +#define ADS1015_LO_THRESH_REG	0x02
> +#define ADS1015_HI_THRESH_REG	0x03
>  
> +#define ADS1015_CFG_COMP_QUE_SHIFT	0
> +#define ADS1015_CFG_COMP_LAT_SHIFT	2
> +#define ADS1015_CFG_COMP_POL_SHIFT	3
> +#define ADS1015_CFG_COMP_MODE_SHIFT	4
>  #define ADS1015_CFG_DR_SHIFT	5
>  #define ADS1015_CFG_MOD_SHIFT	8
>  #define ADS1015_CFG_PGA_SHIFT	9
>  #define ADS1015_CFG_MUX_SHIFT	12
>  
> +#define ADS1015_CFG_COMP_QUE_MASK	GENMASK(1, 0)
> +#define ADS1015_CFG_COMP_LAT_MASK	BIT(2)
> +#define ADS1015_CFG_COMP_POL_MASK	BIT(2)
> +#define ADS1015_CFG_COMP_MODE_MASK	BIT(4)
>  #define ADS1015_CFG_DR_MASK	GENMASK(7, 5)
>  #define ADS1015_CFG_MOD_MASK	BIT(8)
>  #define ADS1015_CFG_PGA_MASK	GENMASK(11, 9)
>  #define ADS1015_CFG_MUX_MASK	GENMASK(14, 12)
>  
> +/* Comparator queue and disable field */
> +#define ADS1015_CFG_COMP_DISABLE	3
> +
> +/* Comparator polarity field */
> +#define ADS1015_CFG_COMP_POL_LOW	0
> +#define ADS1015_CFG_COMP_POL_HIGH	1
> +
> +/* Comparator mode field */
> +#define ADS1015_CFG_COMP_MODE_TRAD	0
Not used..
> +#define ADS1015_CFG_COMP_MODE_WINDOW	1
> +
>  /* device operating modes */
>  #define ADS1015_CONTINUOUS	0
>  #define ADS1015_SINGLESHOT	1
> @@ -89,6 +112,29 @@ static int ads1015_fullscale_range[] = {
>  	6144, 4096, 2048, 1024, 512, 256, 256, 256
>  };
>  
> +/*
> + * Translation from COMP_QUE field value to the number of successive readings
> + * exceed the threshold values before an interrupt is generated
> + */
> +static const int ads1015_comp_queue[] = { 1, 2, 4 };
> +
> +static const struct iio_event_spec ads1015_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				BIT(IIO_EV_INFO_PERIOD),
> +	},
> +};
> +
>  #define ADS1015_V_CHAN(_chan, _addr) {				\
>  	.type = IIO_VOLTAGE,					\
>  	.indexed = 1,						\
> @@ -105,6 +151,8 @@ static int ads1015_fullscale_range[] = {
>  		.shift = 4,					\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.event_spec = ads1015_events,				\
> +	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
>  	.datasheet_name = "AIN"#_chan,				\
>  }
>  
> @@ -126,6 +174,8 @@ static int ads1015_fullscale_range[] = {
>  		.shift = 4,					\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.event_spec = ads1015_events,				\
> +	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
>  	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
>  }
>  
> @@ -144,6 +194,8 @@ static int ads1015_fullscale_range[] = {
>  		.storagebits = 16,				\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.event_spec = ads1015_events,				\
> +	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
>  	.datasheet_name = "AIN"#_chan,				\
>  }
>  
> @@ -164,9 +216,17 @@ static int ads1015_fullscale_range[] = {
>  		.storagebits = 16,				\
>  		.endianness = IIO_CPU,				\
>  	},							\
> +	.event_spec = ads1015_events,				\
> +	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
>  	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
>  }
>  
> +struct ads1015_thresh_data {
> +	unsigned int comp_queue;
> +	unsigned int high_thresh;
> +	unsigned int low_thresh;
> +};
> +
>  struct ads1015_data {
>  	struct regmap *regmap;
>  	/*
> @@ -176,6 +236,9 @@ struct ads1015_data {
>  	struct mutex lock;
>  	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>  
> +	unsigned int event_channel;
> +	struct ads1015_thresh_data thresh_data[ADS1015_CHANNELS];
> +
>  	unsigned int *data_rate;
>  	/*
>  	 * Set to true when the ADC is switched to the continuous-conversion
> @@ -185,15 +248,39 @@ struct ads1015_data {
>  	bool conv_invalid;
>  };
>  
> +static bool ads1015_event_channel_enabled(struct ads1015_data *data)
> +{
> +	return (data->event_channel != ADS1015_CHANNELS);
> +}
> +
> +static void ads1015_event_channel_enable(struct ads1015_data *data, int chan)
> +{
> +	WARN_ON(ads1015_event_channel_enabled(data));
> +
> +	data->event_channel = chan;
> +}
> +
> +static void ads1015_event_channel_disable(struct ads1015_data *data, int chan)
> +{
> +	data->event_channel = ADS1015_CHANNELS;
> +}
> +
>  static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
>  {
> -	return (reg == ADS1015_CFG_REG);
> +	switch (reg) {
> +	case ADS1015_CFG_REG:
> +	case ADS1015_LO_THRESH_REG:
> +	case ADS1015_HI_THRESH_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }
>  
>  static const struct regmap_config ads1015_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 16,
> -	.max_register = ADS1015_CFG_REG,
> +	.max_register = ADS1015_HI_THRESH_REG,
>  	.writeable_reg = ads1015_is_writeable_reg,
>  };
>  
> @@ -238,26 +325,38 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
>  	return ret;
>  }
>  
> +int ads1015_config(struct ads1015_data *data, int chan, bool *change)
> +{
> +	unsigned int cfg_mask, cfg_value;
> +
> +	cfg_mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> +		   ADS1015_CFG_DR_MASK;
> +	cfg_value = chan << ADS1015_CFG_MUX_SHIFT |
> +		    data->channel_data[chan].pga << ADS1015_CFG_PGA_SHIFT |
> +		    data->channel_data[chan].data_rate << ADS1015_CFG_DR_SHIFT;
> +
> +	if (ads1015_event_channel_enabled(data)) {
> +		cfg_mask |= ADS1015_CFG_COMP_QUE_MASK;
> +		cfg_value |= data->thresh_data[chan].comp_queue <<
> +				ADS1015_CFG_COMP_QUE_SHIFT;
> +	}
> +
> +	return regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
> +					cfg_mask, cfg_value, change);
> +}
> +
>  static
>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  {
> -	int ret, pga, dr, conv_time;
> +	int ret, dr, conv_time;
>  	bool change;
>  
>  	if (chan < 0 || chan >= ADS1015_CHANNELS)
>  		return -EINVAL;
>  
> -	pga = data->channel_data[chan].pga;
>  	dr = data->channel_data[chan].data_rate;
>  
> -	ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
> -				       ADS1015_CFG_MUX_MASK |
> -				       ADS1015_CFG_PGA_MASK |
> -				       ADS1015_CFG_DR_MASK,
> -				       chan << ADS1015_CFG_MUX_SHIFT |
> -				       pga << ADS1015_CFG_PGA_SHIFT |
> -				       dr << ADS1015_CFG_DR_SHIFT,
> -				       &change);
> +	ret = ads1015_config(data, chan, &change);
This refactoring should be a separate patch.
>  	if (ret < 0)
>  		return ret;
>  
> @@ -349,6 +448,12 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			break;
>  
> +		if (ads1015_event_channel_enabled(data) &&
> +				data->event_channel != chan->address) {
> +			ret = -EBUSY;
> +			goto release_direct;
> +		}
> +
>  		ret = ads1015_set_power_state(data, true);
>  		if (ret < 0)
>  			goto release_direct;
> @@ -414,8 +519,207 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int ads1015_read_event(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
> +	int *val2)
> +{
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int comp_queue;
> +	int period;
> +	int dr;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = (dir == IIO_EV_DIR_RISING) ?
> +			data->thresh_data[chan->address].high_thresh :
> +			data->thresh_data[chan->address].low_thresh;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_EV_INFO_PERIOD:
> +		dr = data->channel_data[chan->address].data_rate;
> +		comp_queue = data->thresh_data[chan->address].comp_queue;
> +		period = ads1015_comp_queue[comp_queue] *
> +			USEC_PER_SEC / data->data_rate[dr];
> +
> +		*val = period / USEC_PER_SEC;
> +		*val2 = period % USEC_PER_SEC;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int ads1015_write_event(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int val,
> +	int val2)
> +{
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +	int realbits = chan->scan_type.realbits;
> +	int ret = 0;
> +	long long period;
> +	int i;
> +	int dr;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val >= 1 << (realbits - 1) || val < -1 << (realbits - 1)) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		if (dir == IIO_EV_DIR_RISING)
> +			data->thresh_data[chan->address].high_thresh = val;
> +		else
> +			data->thresh_data[chan->address].low_thresh = val;
> +		break;
> +	case IIO_EV_INFO_PERIOD:
> +		dr = data->channel_data[chan->address].data_rate;
> +		period = val * USEC_PER_SEC + val2;
> +
> +		for (i = 0; i < ARRAY_SIZE(ads1015_comp_queue) - 1; i++) {
> +			if (period <= ads1015_comp_queue[i] *
> +					USEC_PER_SEC / data->data_rate[dr])
> +				break;
> +		}
> +		data->thresh_data[chan->address].comp_queue = i;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int ads1015_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = (data->event_channel == chan->address);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int ads1015_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	/* Prevent from enabling both buffer and event at a time */
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret) {
> +		mutex_unlock(&data->lock);
Ah, has to be this way around to avoid lock inversion I guess.
Ugly, but such is life!
> +		return ret;
> +	}
> +
> +	if (ads1015_event_channel_enabled(data) == state) {
> +		ret = -EBUSY;
Why is this an error?  Surely just a drop out with success as
we are enabling something that is already set.  Should be idempotent.
Next condition deals with the different channel option.  Perhaps
reverse the order and have this set ret to 0.
> +		goto error;
> +	}
> +
> +	if (ads1015_event_channel_enabled(data) &&
> +			data->event_channel != chan->address) {
> +		ret = -EBUSY;
> +		goto error;
> +	}
> +
> +	if (state) {
> +		bool change;
> +
> +		ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> +				data->thresh_data[chan->address].low_thresh <<
> +				chan->scan_type.shift);
> +		if (ret)
> +			goto error;
> +
> +		ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> +				data->thresh_data[chan->address].high_thresh <<
> +				chan->scan_type.shift);
> +		if (ret)
> +			goto error;
> +
> +		ret = ads1015_set_power_state(data, true);
> +		if (ret < 0)
> +			goto error;
> +
> +		ads1015_event_channel_enable(data, chan->address);
> +
> +		ret = ads1015_config(data, chan->address, &change);
> +		if (ret) {
> +			ads1015_event_channel_disable(data, chan->address);
> +			ads1015_set_power_state(data, false);
> +		}
> +	} else {
> +		ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> +					ADS1015_CFG_COMP_QUE_MASK,
> +					ADS1015_CFG_COMP_DISABLE <<
> +						ADS1015_CFG_COMP_QUE_SHIFT);
> +		if (ret)
> +			goto error;
> +
> +		ads1015_event_channel_disable(data, chan->address);
> +
> +		ret = ads1015_set_power_state(data, false);
> +	}
> +error:
> +	iio_device_release_direct_mode(indio_dev);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t ads1015_event_handler(int irq, void *priv)
> +{
> +	struct iio_dev *indio_dev = priv;
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +	int val;
> +
> +	/* Clear the latched ALERT/RDY pin */
> +	regmap_read(data->regmap, ADS1015_CONV_REG, &val);
> +
> +	if (ads1015_event_channel_enabled(data)) {
> +		u64 code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> +						data->event_channel,
> +						IIO_EV_TYPE_THRESH,
> +						IIO_EV_DIR_EITHER);
> +
> +		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
The type surprised me.  I guess this is true if we are windowed mode.
Any thought on supporting the other options?  
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ads1015_buffer_preenable(struct iio_dev *indio_dev)
>  {
> +	struct ads1015_data *data = iio_priv(indio_dev);
> +
> +	/* Prevent from enabling both buffer and event at a time */
> +	if (ads1015_event_channel_enabled(data))
> +		return -EBUSY;
> +
>  	return ads1015_set_power_state(iio_priv(indio_dev), true);
>  }
>  
> @@ -466,6 +770,10 @@ static const struct iio_info ads1015_info = {
>  	.driver_module	= THIS_MODULE,
>  	.read_raw	= ads1015_read_raw,
>  	.write_raw	= ads1015_write_raw,
> +	.read_event_value = ads1015_read_event,
> +	.write_event_value = ads1015_write_event,
> +	.read_event_config = ads1015_read_event_config,
> +	.write_event_config = ads1015_write_event_config,
>  	.attrs          = &ads1015_attribute_group,
>  };
>  
> @@ -473,6 +781,10 @@ static const struct iio_info ads1115_info = {
>  	.driver_module	= THIS_MODULE,
>  	.read_raw	= ads1015_read_raw,
>  	.write_raw	= ads1015_write_raw,
> +	.read_event_value = ads1015_read_event,
> +	.write_event_value = ads1015_write_event,
> +	.read_event_config = ads1015_read_event_config,
> +	.write_event_config = ads1015_write_event_config,
>  	.attrs          = &ads1115_attribute_group,
>  };
>  
> @@ -576,6 +888,7 @@ static int ads1015_probe(struct i2c_client *client,
>  	struct ads1015_data *data;
>  	int ret;
>  	enum chip_ids chip;
> +	int i;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -610,6 +923,12 @@ static int ads1015_probe(struct i2c_client *client,
>  		break;
>  	}
>  
> +	data->event_channel = ADS1015_CHANNELS;
> +	for (i = 0; i < ADS1015_CHANNELS; i++) {
> +		data->thresh_data[i].high_thresh =
> +		(1 << (indio_dev->channels[i].scan_type.realbits - 1)) - 1;
> +	}
> +
This block wants a comment on what it is doing. It's fairly obvious but
nothing wrong with a bit more clarity ;)
>  	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
>  	ads1015_get_channels_config(client);
>  
> @@ -627,6 +946,42 @@ static int ads1015_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	if (client->irq) {
> +		unsigned long irq_trig =
> +			irqd_get_trigger_type(irq_get_irq_data(client->irq));
> +		unsigned int cfg_comp_mask = ADS1015_CFG_COMP_QUE_MASK |
> +			ADS1015_CFG_COMP_LAT_MASK | ADS1015_CFG_COMP_POL_MASK |
> +			ADS1015_CFG_COMP_MODE_MASK;
> +		unsigned int cfg_comp =
> +			ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> +			1 << ADS1015_CFG_COMP_LAT_SHIFT |
> +			ADS1015_CFG_COMP_MODE_WINDOW <<
> +				ADS1015_CFG_COMP_MODE_SHIFT;
> +
> +		switch (irq_trig) {
> +		case IRQF_TRIGGER_LOW:
> +			cfg_comp |= ADS1015_CFG_COMP_POL_LOW;
> +			break;
> +		case IRQF_TRIGGER_HIGH:
> +			cfg_comp |= ADS1015_CFG_COMP_POL_HIGH;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> +					cfg_comp_mask, cfg_comp);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, ads1015_event_handler,
> +						irq_trig | IRQF_ONESHOT,
> +						client->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
>  	if (ret)
>  		return ret;

Re: [PATCH 8/8] iio: adc: ti-ads1015: add threshold event support

[Akinobu Mita]

2017-07-15 21:45 GMT+09:00 Jonathan Cameron <jic23@kernel.org>:
> On Thu, 13 Jul 2017 01:32:04 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> The ADS1015 device provides programmable comparator that can issue an
>> interrupt on the ALERT pin.  This change adds the iio threshold event
>> support for that feature.
>>
>> The ADS1015 device only have a single config register which contains an
>> input multiplexer selection, comparator settings, and etc.  So only a
>> single event channel can be enabled at a time.  Also enabling both buffer
>> and event are prohibited for simplicity.
>>
>> Cc: Daniel Baluta <daniel.baluta@intel.com>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Few comments inline, but looks pretty good in general.
>
> THanks,
>
> Jonathan
>> ---
>>  drivers/iio/adc/ti-ads1015.c | 379 +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 367 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
>> index dfeb3af..d735e855 100644
>> --- a/drivers/iio/adc/ti-ads1015.c
>> +++ b/drivers/iio/adc/ti-ads1015.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/module.h>
>>  #include <linux/of_device.h>
>>  #include <linux/init.h>
>> +#include <linux/irq.h>
>>  #include <linux/i2c.h>
>>  #include <linux/regmap.h>
>>  #include <linux/pm_runtime.h>
>> @@ -28,6 +29,7 @@
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/types.h>
>>  #include <linux/iio/sysfs.h>
>> +#include <linux/iio/events.h>
>>  #include <linux/iio/buffer.h>
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/trigger_consumer.h>
>> @@ -36,17 +38,38 @@
>>
>>  #define ADS1015_CONV_REG     0x00
>>  #define ADS1015_CFG_REG              0x01
>> +#define ADS1015_LO_THRESH_REG        0x02
>> +#define ADS1015_HI_THRESH_REG        0x03
>>
>> +#define ADS1015_CFG_COMP_QUE_SHIFT   0
>> +#define ADS1015_CFG_COMP_LAT_SHIFT   2
>> +#define ADS1015_CFG_COMP_POL_SHIFT   3
>> +#define ADS1015_CFG_COMP_MODE_SHIFT  4
>>  #define ADS1015_CFG_DR_SHIFT 5
>>  #define ADS1015_CFG_MOD_SHIFT        8
>>  #define ADS1015_CFG_PGA_SHIFT        9
>>  #define ADS1015_CFG_MUX_SHIFT        12
>>
>> +#define ADS1015_CFG_COMP_QUE_MASK    GENMASK(1, 0)
>> +#define ADS1015_CFG_COMP_LAT_MASK    BIT(2)
>> +#define ADS1015_CFG_COMP_POL_MASK    BIT(2)
>> +#define ADS1015_CFG_COMP_MODE_MASK   BIT(4)
>>  #define ADS1015_CFG_DR_MASK  GENMASK(7, 5)
>>  #define ADS1015_CFG_MOD_MASK BIT(8)
>>  #define ADS1015_CFG_PGA_MASK GENMASK(11, 9)
>>  #define ADS1015_CFG_MUX_MASK GENMASK(14, 12)
>>
>> +/* Comparator queue and disable field */
>> +#define ADS1015_CFG_COMP_DISABLE     3
>> +
>> +/* Comparator polarity field */
>> +#define ADS1015_CFG_COMP_POL_LOW     0
>> +#define ADS1015_CFG_COMP_POL_HIGH    1
>> +
>> +/* Comparator mode field */
>> +#define ADS1015_CFG_COMP_MODE_TRAD   0
> Not used..
>> +#define ADS1015_CFG_COMP_MODE_WINDOW 1
>> +
>>  /* device operating modes */
>>  #define ADS1015_CONTINUOUS   0
>>  #define ADS1015_SINGLESHOT   1
>> @@ -89,6 +112,29 @@ static int ads1015_fullscale_range[] = {
>>       6144, 4096, 2048, 1024, 512, 256, 256, 256
>>  };
>>
>> +/*
>> + * Translation from COMP_QUE field value to the number of successive readings
>> + * exceed the threshold values before an interrupt is generated
>> + */
>> +static const int ads1015_comp_queue[] = { 1, 2, 4 };
>> +
>> +static const struct iio_event_spec ads1015_events[] = {
>> +     {
>> +             .type = IIO_EV_TYPE_THRESH,
>> +             .dir = IIO_EV_DIR_RISING,
>> +             .mask_separate = BIT(IIO_EV_INFO_VALUE),
>> +     }, {
>> +             .type = IIO_EV_TYPE_THRESH,
>> +             .dir = IIO_EV_DIR_FALLING,
>> +             .mask_separate = BIT(IIO_EV_INFO_VALUE),
>> +     }, {
>> +             .type = IIO_EV_TYPE_THRESH,
>> +             .dir = IIO_EV_DIR_EITHER,
>> +             .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
>> +                             BIT(IIO_EV_INFO_PERIOD),
>> +     },
>> +};
>> +
>>  #define ADS1015_V_CHAN(_chan, _addr) {                               \
>>       .type = IIO_VOLTAGE,                                    \
>>       .indexed = 1,                                           \
>> @@ -105,6 +151,8 @@ static int ads1015_fullscale_range[] = {
>>               .shift = 4,                                     \
>>               .endianness = IIO_CPU,                          \
>>       },                                                      \
>> +     .event_spec = ads1015_events,                           \
>> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
>>       .datasheet_name = "AIN"#_chan,                          \
>>  }
>>
>> @@ -126,6 +174,8 @@ static int ads1015_fullscale_range[] = {
>>               .shift = 4,                                     \
>>               .endianness = IIO_CPU,                          \
>>       },                                                      \
>> +     .event_spec = ads1015_events,                           \
>> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
>>       .datasheet_name = "AIN"#_chan"-AIN"#_chan2,             \
>>  }
>>
>> @@ -144,6 +194,8 @@ static int ads1015_fullscale_range[] = {
>>               .storagebits = 16,                              \
>>               .endianness = IIO_CPU,                          \
>>       },                                                      \
>> +     .event_spec = ads1015_events,                           \
>> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
>>       .datasheet_name = "AIN"#_chan,                          \
>>  }
>>
>> @@ -164,9 +216,17 @@ static int ads1015_fullscale_range[] = {
>>               .storagebits = 16,                              \
>>               .endianness = IIO_CPU,                          \
>>       },                                                      \
>> +     .event_spec = ads1015_events,                           \
>> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
>>       .datasheet_name = "AIN"#_chan"-AIN"#_chan2,             \
>>  }
>>
>> +struct ads1015_thresh_data {
>> +     unsigned int comp_queue;
>> +     unsigned int high_thresh;
>> +     unsigned int low_thresh;
>> +};
>> +
>>  struct ads1015_data {
>>       struct regmap *regmap;
>>       /*
>> @@ -176,6 +236,9 @@ struct ads1015_data {
>>       struct mutex lock;
>>       struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>>
>> +     unsigned int event_channel;
>> +     struct ads1015_thresh_data thresh_data[ADS1015_CHANNELS];
>> +
>>       unsigned int *data_rate;
>>       /*
>>        * Set to true when the ADC is switched to the continuous-conversion
>> @@ -185,15 +248,39 @@ struct ads1015_data {
>>       bool conv_invalid;
>>  };
>>
>> +static bool ads1015_event_channel_enabled(struct ads1015_data *data)
>> +{
>> +     return (data->event_channel != ADS1015_CHANNELS);
>> +}
>> +
>> +static void ads1015_event_channel_enable(struct ads1015_data *data, int chan)
>> +{
>> +     WARN_ON(ads1015_event_channel_enabled(data));
>> +
>> +     data->event_channel = chan;
>> +}
>> +
>> +static void ads1015_event_channel_disable(struct ads1015_data *data, int chan)
>> +{
>> +     data->event_channel = ADS1015_CHANNELS;
>> +}
>> +
>>  static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
>>  {
>> -     return (reg == ADS1015_CFG_REG);
>> +     switch (reg) {
>> +     case ADS1015_CFG_REG:
>> +     case ADS1015_LO_THRESH_REG:
>> +     case ADS1015_HI_THRESH_REG:
>> +             return true;
>> +     default:
>> +             return false;
>> +     }
>>  }
>>
>>  static const struct regmap_config ads1015_regmap_config = {
>>       .reg_bits = 8,
>>       .val_bits = 16,
>> -     .max_register = ADS1015_CFG_REG,
>> +     .max_register = ADS1015_HI_THRESH_REG,
>>       .writeable_reg = ads1015_is_writeable_reg,
>>  };
>>
>> @@ -238,26 +325,38 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
>>       return ret;
>>  }
>>
>> +int ads1015_config(struct ads1015_data *data, int chan, bool *change)
>> +{
>> +     unsigned int cfg_mask, cfg_value;
>> +
>> +     cfg_mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
>> +                ADS1015_CFG_DR_MASK;
>> +     cfg_value = chan << ADS1015_CFG_MUX_SHIFT |
>> +                 data->channel_data[chan].pga << ADS1015_CFG_PGA_SHIFT |
>> +                 data->channel_data[chan].data_rate << ADS1015_CFG_DR_SHIFT;
>> +
>> +     if (ads1015_event_channel_enabled(data)) {
>> +             cfg_mask |= ADS1015_CFG_COMP_QUE_MASK;
>> +             cfg_value |= data->thresh_data[chan].comp_queue <<
>> +                             ADS1015_CFG_COMP_QUE_SHIFT;
>> +     }
>> +
>> +     return regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
>> +                                     cfg_mask, cfg_value, change);
>> +}
>> +
>>  static
>>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>>  {
>> -     int ret, pga, dr, conv_time;
>> +     int ret, dr, conv_time;
>>       bool change;
>>
>>       if (chan < 0 || chan >= ADS1015_CHANNELS)
>>               return -EINVAL;
>>
>> -     pga = data->channel_data[chan].pga;
>>       dr = data->channel_data[chan].data_rate;
>>
>> -     ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
>> -                                    ADS1015_CFG_MUX_MASK |
>> -                                    ADS1015_CFG_PGA_MASK |
>> -                                    ADS1015_CFG_DR_MASK,
>> -                                    chan << ADS1015_CFG_MUX_SHIFT |
>> -                                    pga << ADS1015_CFG_PGA_SHIFT |
>> -                                    dr << ADS1015_CFG_DR_SHIFT,
>> -                                    &change);
>> +     ret = ads1015_config(data, chan, &change);
> This refactoring should be a separate patch.
>>       if (ret < 0)
>>               return ret;
>>
>> @@ -349,6 +448,12 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>>               if (ret)
>>                       break;
>>
>> +             if (ads1015_event_channel_enabled(data) &&
>> +                             data->event_channel != chan->address) {
>> +                     ret = -EBUSY;
>> +                     goto release_direct;
>> +             }
>> +
>>               ret = ads1015_set_power_state(data, true);
>>               if (ret < 0)
>>                       goto release_direct;
>> @@ -414,8 +519,207 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
>>       return ret;
>>  }
>>
>> +static int ads1015_read_event(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan, enum iio_event_type type,
>> +     enum iio_event_direction dir, enum iio_event_info info, int *val,
>> +     int *val2)
>> +{
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +     unsigned int comp_queue;
>> +     int period;
>> +     int dr;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     switch (info) {
>> +     case IIO_EV_INFO_VALUE:
>> +             *val = (dir == IIO_EV_DIR_RISING) ?
>> +                     data->thresh_data[chan->address].high_thresh :
>> +                     data->thresh_data[chan->address].low_thresh;
>> +             ret = IIO_VAL_INT;
>> +             break;
>> +     case IIO_EV_INFO_PERIOD:
>> +             dr = data->channel_data[chan->address].data_rate;
>> +             comp_queue = data->thresh_data[chan->address].comp_queue;
>> +             period = ads1015_comp_queue[comp_queue] *
>> +                     USEC_PER_SEC / data->data_rate[dr];
>> +
>> +             *val = period / USEC_PER_SEC;
>> +             *val2 = period % USEC_PER_SEC;
>> +             ret = IIO_VAL_INT_PLUS_MICRO;
>> +             break;
>> +     default:
>> +             ret = -EINVAL;
>> +             break;
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ads1015_write_event(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan, enum iio_event_type type,
>> +     enum iio_event_direction dir, enum iio_event_info info, int val,
>> +     int val2)
>> +{
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +     int realbits = chan->scan_type.realbits;
>> +     int ret = 0;
>> +     long long period;
>> +     int i;
>> +     int dr;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     switch (info) {
>> +     case IIO_EV_INFO_VALUE:
>> +             if (val >= 1 << (realbits - 1) || val < -1 << (realbits - 1)) {
>> +                     ret = -EINVAL;
>> +                     break;
>> +             }
>> +             if (dir == IIO_EV_DIR_RISING)
>> +                     data->thresh_data[chan->address].high_thresh = val;
>> +             else
>> +                     data->thresh_data[chan->address].low_thresh = val;
>> +             break;
>> +     case IIO_EV_INFO_PERIOD:
>> +             dr = data->channel_data[chan->address].data_rate;
>> +             period = val * USEC_PER_SEC + val2;
>> +
>> +             for (i = 0; i < ARRAY_SIZE(ads1015_comp_queue) - 1; i++) {
>> +                     if (period <= ads1015_comp_queue[i] *
>> +                                     USEC_PER_SEC / data->data_rate[dr])
>> +                             break;
>> +             }
>> +             data->thresh_data[chan->address].comp_queue = i;
>> +             break;
>> +     default:
>> +             ret = -EINVAL;
>> +             break;
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ads1015_read_event_config(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan, enum iio_event_type type,
>> +     enum iio_event_direction dir)
>> +{
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     mutex_lock(&data->lock);
>> +     ret = (data->event_channel == chan->address);
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ads1015_write_event_config(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan, enum iio_event_type type,
>> +     enum iio_event_direction dir, int state)
>> +{
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     /* Prevent from enabling both buffer and event at a time */
>> +     ret = iio_device_claim_direct_mode(indio_dev);
>> +     if (ret) {
>> +             mutex_unlock(&data->lock);
> Ah, has to be this way around to avoid lock inversion I guess.
> Ugly, but such is life!
>> +             return ret;
>> +     }
>> +
>> +     if (ads1015_event_channel_enabled(data) == state) {
>> +             ret = -EBUSY;
> Why is this an error?  Surely just a drop out with success as
> we are enabling something that is already set.  Should be idempotent.
> Next condition deals with the different channel option.  Perhaps
> reverse the order and have this set ret to 0.
>> +             goto error;
>> +     }
>> +
>> +     if (ads1015_event_channel_enabled(data) &&
>> +                     data->event_channel != chan->address) {
>> +             ret = -EBUSY;
>> +             goto error;
>> +     }
>> +
>> +     if (state) {
>> +             bool change;
>> +
>> +             ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
>> +                             data->thresh_data[chan->address].low_thresh <<
>> +                             chan->scan_type.shift);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
>> +                             data->thresh_data[chan->address].high_thresh <<
>> +                             chan->scan_type.shift);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             ret = ads1015_set_power_state(data, true);
>> +             if (ret < 0)
>> +                     goto error;
>> +
>> +             ads1015_event_channel_enable(data, chan->address);
>> +
>> +             ret = ads1015_config(data, chan->address, &change);
>> +             if (ret) {
>> +                     ads1015_event_channel_disable(data, chan->address);
>> +                     ads1015_set_power_state(data, false);
>> +             }
>> +     } else {
>> +             ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
>> +                                     ADS1015_CFG_COMP_QUE_MASK,
>> +                                     ADS1015_CFG_COMP_DISABLE <<
>> +                                             ADS1015_CFG_COMP_QUE_SHIFT);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             ads1015_event_channel_disable(data, chan->address);
>> +
>> +             ret = ads1015_set_power_state(data, false);
>> +     }
>> +error:
>> +     iio_device_release_direct_mode(indio_dev);
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static irqreturn_t ads1015_event_handler(int irq, void *priv)
>> +{
>> +     struct iio_dev *indio_dev = priv;
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +     int val;
>> +
>> +     /* Clear the latched ALERT/RDY pin */
>> +     regmap_read(data->regmap, ADS1015_CONV_REG, &val);
>> +
>> +     if (ads1015_event_channel_enabled(data)) {
>> +             u64 code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
>> +                                             data->event_channel,
>> +                                             IIO_EV_TYPE_THRESH,
>> +                                             IIO_EV_DIR_EITHER);
>> +
>> +             iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
> The type surprised me.  I guess this is true if we are windowed mode.
> Any thought on supporting the other options?

Yes.  This threshold event support is implemented with windowed mode.
The interrupt is asserted when the conversion data exceed the upper
threshold or fall below the lower threshold.  So I thought that event
code fits well.

On the other hand, in the traditional comparator mode which ADS1015/1115
supports, the interrupt pin is asserted only when the conversion exceed
the upper threshold.  So I choose not to use the traditional comparator
mode as the window comparator mode is more flexible.

>> +     }
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>>  static int ads1015_buffer_preenable(struct iio_dev *indio_dev)
>>  {
>> +     struct ads1015_data *data = iio_priv(indio_dev);
>> +
>> +     /* Prevent from enabling both buffer and event at a time */
>> +     if (ads1015_event_channel_enabled(data))
>> +             return -EBUSY;
>> +
>>       return ads1015_set_power_state(iio_priv(indio_dev), true);
>>  }
>>
>> @@ -466,6 +770,10 @@ static const struct iio_info ads1015_info = {
>>       .driver_module  = THIS_MODULE,
>>       .read_raw       = ads1015_read_raw,
>>       .write_raw      = ads1015_write_raw,
>> +     .read_event_value = ads1015_read_event,
>> +     .write_event_value = ads1015_write_event,
>> +     .read_event_config = ads1015_read_event_config,
>> +     .write_event_config = ads1015_write_event_config,
>>       .attrs          = &ads1015_attribute_group,
>>  };
>>
>> @@ -473,6 +781,10 @@ static const struct iio_info ads1115_info = {
>>       .driver_module  = THIS_MODULE,
>>       .read_raw       = ads1015_read_raw,
>>       .write_raw      = ads1015_write_raw,
>> +     .read_event_value = ads1015_read_event,
>> +     .write_event_value = ads1015_write_event,
>> +     .read_event_config = ads1015_read_event_config,
>> +     .write_event_config = ads1015_write_event_config,
>>       .attrs          = &ads1115_attribute_group,
>>  };
>>
>> @@ -576,6 +888,7 @@ static int ads1015_probe(struct i2c_client *client,
>>       struct ads1015_data *data;
>>       int ret;
>>       enum chip_ids chip;
>> +     int i;
>>
>>       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>       if (!indio_dev)
>> @@ -610,6 +923,12 @@ static int ads1015_probe(struct i2c_client *client,
>>               break;
>>       }
>>
>> +     data->event_channel = ADS1015_CHANNELS;
>> +     for (i = 0; i < ADS1015_CHANNELS; i++) {
>> +             data->thresh_data[i].high_thresh =
>> +             (1 << (indio_dev->channels[i].scan_type.realbits - 1)) - 1;
>> +     }
>> +
> This block wants a comment on what it is doing. It's fairly obvious but
> nothing wrong with a bit more clarity ;)
>>       /* we need to keep this ABI the same as used by hwmon ADS1015 driver */
>>       ads1015_get_channels_config(client);
>>
>> @@ -627,6 +946,42 @@ static int ads1015_probe(struct i2c_client *client,
>>               return ret;
>>       }
>>
>> +     if (client->irq) {
>> +             unsigned long irq_trig =
>> +                     irqd_get_trigger_type(irq_get_irq_data(client->irq));
>> +             unsigned int cfg_comp_mask = ADS1015_CFG_COMP_QUE_MASK |
>> +                     ADS1015_CFG_COMP_LAT_MASK | ADS1015_CFG_COMP_POL_MASK |
>> +                     ADS1015_CFG_COMP_MODE_MASK;
>> +             unsigned int cfg_comp =
>> +                     ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
>> +                     1 << ADS1015_CFG_COMP_LAT_SHIFT |
>> +                     ADS1015_CFG_COMP_MODE_WINDOW <<
>> +                             ADS1015_CFG_COMP_MODE_SHIFT;
>> +
>> +             switch (irq_trig) {
>> +             case IRQF_TRIGGER_LOW:
>> +                     cfg_comp |= ADS1015_CFG_COMP_POL_LOW;
>> +                     break;
>> +             case IRQF_TRIGGER_HIGH:
>> +                     cfg_comp |= ADS1015_CFG_COMP_POL_HIGH;
>> +                     break;
>> +             default:
>> +                     return -EINVAL;
>> +             }
>> +
>> +             ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
>> +                                     cfg_comp_mask, cfg_comp);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             ret = devm_request_threaded_irq(&client->dev, client->irq,
>> +                                             NULL, ads1015_event_handler,
>> +                                             irq_trig | IRQF_ONESHOT,
>> +                                             client->name, indio_dev);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>>       ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
>>       if (ret)
>>               return ret;
>

Re: [PATCH 8/8] iio: adc: ti-ads1015: add threshold event support

[Jonathan Cameron]

On Tue, 18 Jul 2017 01:45:21 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> 2017-07-15 21:45 GMT+09:00 Jonathan Cameron <jic23@kernel.org>:
> > On Thu, 13 Jul 2017 01:32:04 +0900
> > Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >  
> >> The ADS1015 device provides programmable comparator that can issue an
> >> interrupt on the ALERT pin.  This change adds the iio threshold event
> >> support for that feature.
> >>
> >> The ADS1015 device only have a single config register which contains an
> >> input multiplexer selection, comparator settings, and etc.  So only a
> >> single event channel can be enabled at a time.  Also enabling both buffer
> >> and event are prohibited for simplicity.
> >>
> >> Cc: Daniel Baluta <daniel.baluta@intel.com>
> >> Cc: Jonathan Cameron <jic23@kernel.org>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> > Few comments inline, but looks pretty good in general.
> >
> > THanks,
> >
> > Jonathan  
> >> ---
> >>  drivers/iio/adc/ti-ads1015.c | 379 +++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 367 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> index dfeb3af..d735e855 100644
> >> --- a/drivers/iio/adc/ti-ads1015.c
> >> +++ b/drivers/iio/adc/ti-ads1015.c
> >> @@ -17,6 +17,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/of_device.h>
> >>  #include <linux/init.h>
> >> +#include <linux/irq.h>
> >>  #include <linux/i2c.h>
> >>  #include <linux/regmap.h>
> >>  #include <linux/pm_runtime.h>
> >> @@ -28,6 +29,7 @@
> >>  #include <linux/iio/iio.h>
> >>  #include <linux/iio/types.h>
> >>  #include <linux/iio/sysfs.h>
> >> +#include <linux/iio/events.h>
> >>  #include <linux/iio/buffer.h>
> >>  #include <linux/iio/triggered_buffer.h>
> >>  #include <linux/iio/trigger_consumer.h>
> >> @@ -36,17 +38,38 @@
> >>
> >>  #define ADS1015_CONV_REG     0x00
> >>  #define ADS1015_CFG_REG              0x01
> >> +#define ADS1015_LO_THRESH_REG        0x02
> >> +#define ADS1015_HI_THRESH_REG        0x03
> >>
> >> +#define ADS1015_CFG_COMP_QUE_SHIFT   0
> >> +#define ADS1015_CFG_COMP_LAT_SHIFT   2
> >> +#define ADS1015_CFG_COMP_POL_SHIFT   3
> >> +#define ADS1015_CFG_COMP_MODE_SHIFT  4
> >>  #define ADS1015_CFG_DR_SHIFT 5
> >>  #define ADS1015_CFG_MOD_SHIFT        8
> >>  #define ADS1015_CFG_PGA_SHIFT        9
> >>  #define ADS1015_CFG_MUX_SHIFT        12
> >>
> >> +#define ADS1015_CFG_COMP_QUE_MASK    GENMASK(1, 0)
> >> +#define ADS1015_CFG_COMP_LAT_MASK    BIT(2)
> >> +#define ADS1015_CFG_COMP_POL_MASK    BIT(2)
> >> +#define ADS1015_CFG_COMP_MODE_MASK   BIT(4)
> >>  #define ADS1015_CFG_DR_MASK  GENMASK(7, 5)
> >>  #define ADS1015_CFG_MOD_MASK BIT(8)
> >>  #define ADS1015_CFG_PGA_MASK GENMASK(11, 9)
> >>  #define ADS1015_CFG_MUX_MASK GENMASK(14, 12)
> >>
> >> +/* Comparator queue and disable field */
> >> +#define ADS1015_CFG_COMP_DISABLE     3
> >> +
> >> +/* Comparator polarity field */
> >> +#define ADS1015_CFG_COMP_POL_LOW     0
> >> +#define ADS1015_CFG_COMP_POL_HIGH    1
> >> +
> >> +/* Comparator mode field */
> >> +#define ADS1015_CFG_COMP_MODE_TRAD   0  
> > Not used..  
> >> +#define ADS1015_CFG_COMP_MODE_WINDOW 1
> >> +
> >>  /* device operating modes */
> >>  #define ADS1015_CONTINUOUS   0
> >>  #define ADS1015_SINGLESHOT   1
> >> @@ -89,6 +112,29 @@ static int ads1015_fullscale_range[] = {
> >>       6144, 4096, 2048, 1024, 512, 256, 256, 256
> >>  };
> >>
> >> +/*
> >> + * Translation from COMP_QUE field value to the number of successive readings
> >> + * exceed the threshold values before an interrupt is generated
> >> + */
> >> +static const int ads1015_comp_queue[] = { 1, 2, 4 };
> >> +
> >> +static const struct iio_event_spec ads1015_events[] = {
> >> +     {
> >> +             .type = IIO_EV_TYPE_THRESH,
> >> +             .dir = IIO_EV_DIR_RISING,
> >> +             .mask_separate = BIT(IIO_EV_INFO_VALUE),
> >> +     }, {
> >> +             .type = IIO_EV_TYPE_THRESH,
> >> +             .dir = IIO_EV_DIR_FALLING,
> >> +             .mask_separate = BIT(IIO_EV_INFO_VALUE),
> >> +     }, {
> >> +             .type = IIO_EV_TYPE_THRESH,
> >> +             .dir = IIO_EV_DIR_EITHER,
> >> +             .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> >> +                             BIT(IIO_EV_INFO_PERIOD),
> >> +     },
> >> +};
> >> +
> >>  #define ADS1015_V_CHAN(_chan, _addr) {                               \
> >>       .type = IIO_VOLTAGE,                                    \
> >>       .indexed = 1,                                           \
> >> @@ -105,6 +151,8 @@ static int ads1015_fullscale_range[] = {
> >>               .shift = 4,                                     \
> >>               .endianness = IIO_CPU,                          \
> >>       },                                                      \
> >> +     .event_spec = ads1015_events,                           \
> >> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
> >>       .datasheet_name = "AIN"#_chan,                          \
> >>  }
> >>
> >> @@ -126,6 +174,8 @@ static int ads1015_fullscale_range[] = {
> >>               .shift = 4,                                     \
> >>               .endianness = IIO_CPU,                          \
> >>       },                                                      \
> >> +     .event_spec = ads1015_events,                           \
> >> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
> >>       .datasheet_name = "AIN"#_chan"-AIN"#_chan2,             \
> >>  }
> >>
> >> @@ -144,6 +194,8 @@ static int ads1015_fullscale_range[] = {
> >>               .storagebits = 16,                              \
> >>               .endianness = IIO_CPU,                          \
> >>       },                                                      \
> >> +     .event_spec = ads1015_events,                           \
> >> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
> >>       .datasheet_name = "AIN"#_chan,                          \
> >>  }
> >>
> >> @@ -164,9 +216,17 @@ static int ads1015_fullscale_range[] = {
> >>               .storagebits = 16,                              \
> >>               .endianness = IIO_CPU,                          \
> >>       },                                                      \
> >> +     .event_spec = ads1015_events,                           \
> >> +     .num_event_specs = ARRAY_SIZE(ads1015_events),          \
> >>       .datasheet_name = "AIN"#_chan"-AIN"#_chan2,             \
> >>  }
> >>
> >> +struct ads1015_thresh_data {
> >> +     unsigned int comp_queue;
> >> +     unsigned int high_thresh;
> >> +     unsigned int low_thresh;
> >> +};
> >> +
> >>  struct ads1015_data {
> >>       struct regmap *regmap;
> >>       /*
> >> @@ -176,6 +236,9 @@ struct ads1015_data {
> >>       struct mutex lock;
> >>       struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
> >>
> >> +     unsigned int event_channel;
> >> +     struct ads1015_thresh_data thresh_data[ADS1015_CHANNELS];
> >> +
> >>       unsigned int *data_rate;
> >>       /*
> >>        * Set to true when the ADC is switched to the continuous-conversion
> >> @@ -185,15 +248,39 @@ struct ads1015_data {
> >>       bool conv_invalid;
> >>  };
> >>
> >> +static bool ads1015_event_channel_enabled(struct ads1015_data *data)
> >> +{
> >> +     return (data->event_channel != ADS1015_CHANNELS);
> >> +}
> >> +
> >> +static void ads1015_event_channel_enable(struct ads1015_data *data, int chan)
> >> +{
> >> +     WARN_ON(ads1015_event_channel_enabled(data));
> >> +
> >> +     data->event_channel = chan;
> >> +}
> >> +
> >> +static void ads1015_event_channel_disable(struct ads1015_data *data, int chan)
> >> +{
> >> +     data->event_channel = ADS1015_CHANNELS;
> >> +}
> >> +
> >>  static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
> >>  {
> >> -     return (reg == ADS1015_CFG_REG);
> >> +     switch (reg) {
> >> +     case ADS1015_CFG_REG:
> >> +     case ADS1015_LO_THRESH_REG:
> >> +     case ADS1015_HI_THRESH_REG:
> >> +             return true;
> >> +     default:
> >> +             return false;
> >> +     }
> >>  }
> >>
> >>  static const struct regmap_config ads1015_regmap_config = {
> >>       .reg_bits = 8,
> >>       .val_bits = 16,
> >> -     .max_register = ADS1015_CFG_REG,
> >> +     .max_register = ADS1015_HI_THRESH_REG,
> >>       .writeable_reg = ads1015_is_writeable_reg,
> >>  };
> >>
> >> @@ -238,26 +325,38 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
> >>       return ret;
> >>  }
> >>
> >> +int ads1015_config(struct ads1015_data *data, int chan, bool *change)
> >> +{
> >> +     unsigned int cfg_mask, cfg_value;
> >> +
> >> +     cfg_mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> >> +                ADS1015_CFG_DR_MASK;
> >> +     cfg_value = chan << ADS1015_CFG_MUX_SHIFT |
> >> +                 data->channel_data[chan].pga << ADS1015_CFG_PGA_SHIFT |
> >> +                 data->channel_data[chan].data_rate << ADS1015_CFG_DR_SHIFT;
> >> +
> >> +     if (ads1015_event_channel_enabled(data)) {
> >> +             cfg_mask |= ADS1015_CFG_COMP_QUE_MASK;
> >> +             cfg_value |= data->thresh_data[chan].comp_queue <<
> >> +                             ADS1015_CFG_COMP_QUE_SHIFT;
> >> +     }
> >> +
> >> +     return regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
> >> +                                     cfg_mask, cfg_value, change);
> >> +}
> >> +
> >>  static
> >>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >>  {
> >> -     int ret, pga, dr, conv_time;
> >> +     int ret, dr, conv_time;
> >>       bool change;
> >>
> >>       if (chan < 0 || chan >= ADS1015_CHANNELS)
> >>               return -EINVAL;
> >>
> >> -     pga = data->channel_data[chan].pga;
> >>       dr = data->channel_data[chan].data_rate;
> >>
> >> -     ret = regmap_update_bits_check(data->regmap, ADS1015_CFG_REG,
> >> -                                    ADS1015_CFG_MUX_MASK |
> >> -                                    ADS1015_CFG_PGA_MASK |
> >> -                                    ADS1015_CFG_DR_MASK,
> >> -                                    chan << ADS1015_CFG_MUX_SHIFT |
> >> -                                    pga << ADS1015_CFG_PGA_SHIFT |
> >> -                                    dr << ADS1015_CFG_DR_SHIFT,
> >> -                                    &change);
> >> +     ret = ads1015_config(data, chan, &change);  
> > This refactoring should be a separate patch.  
> >>       if (ret < 0)
> >>               return ret;
> >>
> >> @@ -349,6 +448,12 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
> >>               if (ret)
> >>                       break;
> >>
> >> +             if (ads1015_event_channel_enabled(data) &&
> >> +                             data->event_channel != chan->address) {
> >> +                     ret = -EBUSY;
> >> +                     goto release_direct;
> >> +             }
> >> +
> >>               ret = ads1015_set_power_state(data, true);
> >>               if (ret < 0)
> >>                       goto release_direct;
> >> @@ -414,8 +519,207 @@ static int ads1015_write_raw(struct iio_dev *indio_dev,
> >>       return ret;
> >>  }
> >>
> >> +static int ads1015_read_event(struct iio_dev *indio_dev,
> >> +     const struct iio_chan_spec *chan, enum iio_event_type type,
> >> +     enum iio_event_direction dir, enum iio_event_info info, int *val,
> >> +     int *val2)
> >> +{
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +     int ret;
> >> +     unsigned int comp_queue;
> >> +     int period;
> >> +     int dr;
> >> +
> >> +     mutex_lock(&data->lock);
> >> +
> >> +     switch (info) {
> >> +     case IIO_EV_INFO_VALUE:
> >> +             *val = (dir == IIO_EV_DIR_RISING) ?
> >> +                     data->thresh_data[chan->address].high_thresh :
> >> +                     data->thresh_data[chan->address].low_thresh;
> >> +             ret = IIO_VAL_INT;
> >> +             break;
> >> +     case IIO_EV_INFO_PERIOD:
> >> +             dr = data->channel_data[chan->address].data_rate;
> >> +             comp_queue = data->thresh_data[chan->address].comp_queue;
> >> +             period = ads1015_comp_queue[comp_queue] *
> >> +                     USEC_PER_SEC / data->data_rate[dr];
> >> +
> >> +             *val = period / USEC_PER_SEC;
> >> +             *val2 = period % USEC_PER_SEC;
> >> +             ret = IIO_VAL_INT_PLUS_MICRO;
> >> +             break;
> >> +     default:
> >> +             ret = -EINVAL;
> >> +             break;
> >> +     }
> >> +
> >> +     mutex_unlock(&data->lock);
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +static int ads1015_write_event(struct iio_dev *indio_dev,
> >> +     const struct iio_chan_spec *chan, enum iio_event_type type,
> >> +     enum iio_event_direction dir, enum iio_event_info info, int val,
> >> +     int val2)
> >> +{
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +     int realbits = chan->scan_type.realbits;
> >> +     int ret = 0;
> >> +     long long period;
> >> +     int i;
> >> +     int dr;
> >> +
> >> +     mutex_lock(&data->lock);
> >> +
> >> +     switch (info) {
> >> +     case IIO_EV_INFO_VALUE:
> >> +             if (val >= 1 << (realbits - 1) || val < -1 << (realbits - 1)) {
> >> +                     ret = -EINVAL;
> >> +                     break;
> >> +             }
> >> +             if (dir == IIO_EV_DIR_RISING)
> >> +                     data->thresh_data[chan->address].high_thresh = val;
> >> +             else
> >> +                     data->thresh_data[chan->address].low_thresh = val;
> >> +             break;
> >> +     case IIO_EV_INFO_PERIOD:
> >> +             dr = data->channel_data[chan->address].data_rate;
> >> +             period = val * USEC_PER_SEC + val2;
> >> +
> >> +             for (i = 0; i < ARRAY_SIZE(ads1015_comp_queue) - 1; i++) {
> >> +                     if (period <= ads1015_comp_queue[i] *
> >> +                                     USEC_PER_SEC / data->data_rate[dr])
> >> +                             break;
> >> +             }
> >> +             data->thresh_data[chan->address].comp_queue = i;
> >> +             break;
> >> +     default:
> >> +             ret = -EINVAL;
> >> +             break;
> >> +     }
> >> +
> >> +     mutex_unlock(&data->lock);
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +static int ads1015_read_event_config(struct iio_dev *indio_dev,
> >> +     const struct iio_chan_spec *chan, enum iio_event_type type,
> >> +     enum iio_event_direction dir)
> >> +{
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +     int ret;
> >> +
> >> +     mutex_lock(&data->lock);
> >> +     ret = (data->event_channel == chan->address);
> >> +     mutex_unlock(&data->lock);
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +static int ads1015_write_event_config(struct iio_dev *indio_dev,
> >> +     const struct iio_chan_spec *chan, enum iio_event_type type,
> >> +     enum iio_event_direction dir, int state)
> >> +{
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +     int ret;
> >> +
> >> +     mutex_lock(&data->lock);
> >> +
> >> +     /* Prevent from enabling both buffer and event at a time */
> >> +     ret = iio_device_claim_direct_mode(indio_dev);
> >> +     if (ret) {
> >> +             mutex_unlock(&data->lock);  
> > Ah, has to be this way around to avoid lock inversion I guess.
> > Ugly, but such is life!  
> >> +             return ret;
> >> +     }
> >> +
> >> +     if (ads1015_event_channel_enabled(data) == state) {
> >> +             ret = -EBUSY;  
> > Why is this an error?  Surely just a drop out with success as
> > we are enabling something that is already set.  Should be idempotent.
> > Next condition deals with the different channel option.  Perhaps
> > reverse the order and have this set ret to 0.  
> >> +             goto error;
> >> +     }
> >> +
> >> +     if (ads1015_event_channel_enabled(data) &&
> >> +                     data->event_channel != chan->address) {
> >> +             ret = -EBUSY;
> >> +             goto error;
> >> +     }
> >> +
> >> +     if (state) {
> >> +             bool change;
> >> +
> >> +             ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> >> +                             data->thresh_data[chan->address].low_thresh <<
> >> +                             chan->scan_type.shift);
> >> +             if (ret)
> >> +                     goto error;
> >> +
> >> +             ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> >> +                             data->thresh_data[chan->address].high_thresh <<
> >> +                             chan->scan_type.shift);
> >> +             if (ret)
> >> +                     goto error;
> >> +
> >> +             ret = ads1015_set_power_state(data, true);
> >> +             if (ret < 0)
> >> +                     goto error;
> >> +
> >> +             ads1015_event_channel_enable(data, chan->address);
> >> +
> >> +             ret = ads1015_config(data, chan->address, &change);
> >> +             if (ret) {
> >> +                     ads1015_event_channel_disable(data, chan->address);
> >> +                     ads1015_set_power_state(data, false);
> >> +             }
> >> +     } else {
> >> +             ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> >> +                                     ADS1015_CFG_COMP_QUE_MASK,
> >> +                                     ADS1015_CFG_COMP_DISABLE <<
> >> +                                             ADS1015_CFG_COMP_QUE_SHIFT);
> >> +             if (ret)
> >> +                     goto error;
> >> +
> >> +             ads1015_event_channel_disable(data, chan->address);
> >> +
> >> +             ret = ads1015_set_power_state(data, false);
> >> +     }
> >> +error:
> >> +     iio_device_release_direct_mode(indio_dev);
> >> +     mutex_unlock(&data->lock);
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +static irqreturn_t ads1015_event_handler(int irq, void *priv)
> >> +{
> >> +     struct iio_dev *indio_dev = priv;
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +     int val;
> >> +
> >> +     /* Clear the latched ALERT/RDY pin */
> >> +     regmap_read(data->regmap, ADS1015_CONV_REG, &val);
> >> +
> >> +     if (ads1015_event_channel_enabled(data)) {
> >> +             u64 code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> >> +                                             data->event_channel,
> >> +                                             IIO_EV_TYPE_THRESH,
> >> +                                             IIO_EV_DIR_EITHER);
> >> +
> >> +             iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));  
> > The type surprised me.  I guess this is true if we are windowed mode.
> > Any thought on supporting the other options?  
> 
> Yes.  This threshold event support is implemented with windowed mode.
> The interrupt is asserted when the conversion data exceed the upper
> threshold or fall below the lower threshold.  So I thought that event
> code fits well.
> 
> On the other hand, in the traditional comparator mode which ADS1015/1115
> supports, the interrupt pin is asserted only when the conversion exceed
> the upper threshold.  So I choose not to use the traditional comparator
> mode as the window comparator mode is more flexible.
I don't think it would be hard to support both options, though I can
see some confusion around the case where only the lower one is
enabled. That will have to turn the upper on as well which makes
for an odd (if allowed under the ABI docs) interface.
Rule is that any attribute can result in any other changing so
it's up to userspace to check anything it cares about after making a change.

Perhaps best left with the simpler option you have here.
> 
> >> +     }
> >> +
> >> +     return IRQ_HANDLED;
> >> +}
> >> +
> >>  static int ads1015_buffer_preenable(struct iio_dev *indio_dev)
> >>  {
> >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> +
> >> +     /* Prevent from enabling both buffer and event at a time */
> >> +     if (ads1015_event_channel_enabled(data))
> >> +             return -EBUSY;
> >> +
> >>       return ads1015_set_power_state(iio_priv(indio_dev), true);
> >>  }
> >>
> >> @@ -466,6 +770,10 @@ static const struct iio_info ads1015_info = {
> >>       .driver_module  = THIS_MODULE,
> >>       .read_raw       = ads1015_read_raw,
> >>       .write_raw      = ads1015_write_raw,
> >> +     .read_event_value = ads1015_read_event,
> >> +     .write_event_value = ads1015_write_event,
> >> +     .read_event_config = ads1015_read_event_config,
> >> +     .write_event_config = ads1015_write_event_config,
> >>       .attrs          = &ads1015_attribute_group,
> >>  };
> >>
> >> @@ -473,6 +781,10 @@ static const struct iio_info ads1115_info = {
> >>       .driver_module  = THIS_MODULE,
> >>       .read_raw       = ads1015_read_raw,
> >>       .write_raw      = ads1015_write_raw,
> >> +     .read_event_value = ads1015_read_event,
> >> +     .write_event_value = ads1015_write_event,
> >> +     .read_event_config = ads1015_read_event_config,
> >> +     .write_event_config = ads1015_write_event_config,
> >>       .attrs          = &ads1115_attribute_group,
> >>  };
> >>
> >> @@ -576,6 +888,7 @@ static int ads1015_probe(struct i2c_client *client,
> >>       struct ads1015_data *data;
> >>       int ret;
> >>       enum chip_ids chip;
> >> +     int i;
> >>
> >>       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >>       if (!indio_dev)
> >> @@ -610,6 +923,12 @@ static int ads1015_probe(struct i2c_client *client,
> >>               break;
> >>       }
> >>
> >> +     data->event_channel = ADS1015_CHANNELS;
> >> +     for (i = 0; i < ADS1015_CHANNELS; i++) {
> >> +             data->thresh_data[i].high_thresh =
> >> +             (1 << (indio_dev->channels[i].scan_type.realbits - 1)) - 1;
> >> +     }
> >> +  
> > This block wants a comment on what it is doing. It's fairly obvious but
> > nothing wrong with a bit more clarity ;)  
> >>       /* we need to keep this ABI the same as used by hwmon ADS1015 driver */
> >>       ads1015_get_channels_config(client);
> >>
> >> @@ -627,6 +946,42 @@ static int ads1015_probe(struct i2c_client *client,
> >>               return ret;
> >>       }
> >>
> >> +     if (client->irq) {
> >> +             unsigned long irq_trig =
> >> +                     irqd_get_trigger_type(irq_get_irq_data(client->irq));
> >> +             unsigned int cfg_comp_mask = ADS1015_CFG_COMP_QUE_MASK |
> >> +                     ADS1015_CFG_COMP_LAT_MASK | ADS1015_CFG_COMP_POL_MASK |
> >> +                     ADS1015_CFG_COMP_MODE_MASK;
> >> +             unsigned int cfg_comp =
> >> +                     ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> >> +                     1 << ADS1015_CFG_COMP_LAT_SHIFT |
> >> +                     ADS1015_CFG_COMP_MODE_WINDOW <<
> >> +                             ADS1015_CFG_COMP_MODE_SHIFT;
> >> +
> >> +             switch (irq_trig) {
> >> +             case IRQF_TRIGGER_LOW:
> >> +                     cfg_comp |= ADS1015_CFG_COMP_POL_LOW;
> >> +                     break;
> >> +             case IRQF_TRIGGER_HIGH:
> >> +                     cfg_comp |= ADS1015_CFG_COMP_POL_HIGH;
> >> +                     break;
> >> +             default:
> >> +                     return -EINVAL;
> >> +             }
> >> +
> >> +             ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> >> +                                     cfg_comp_mask, cfg_comp);
> >> +             if (ret)
> >> +                     return ret;
> >> +
> >> +             ret = devm_request_threaded_irq(&client->dev, client->irq,
> >> +                                             NULL, ads1015_event_handler,
> >> +                                             irq_trig | IRQF_ONESHOT,
> >> +                                             client->name, indio_dev);
> >> +             if (ret)
> >> +                     return ret;
> >> +     }
> >> +
> >>       ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
> >>       if (ret)
> >>               return ret;  
> >