[PATCH v2 00/11] iio: adc: ti-ads1015: fixes, cleanups, and threshold event support

[PATCH v2 00/11] iio: adc: ti-ads1015: fixes, cleanups, and threshold event support

[Akinobu Mita]

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

The first six patches are bug fixes, next four are cleanups, and the last
one adds support threshold event.

Changes from v1
- Split the patch "enable conversion when CONFIG_PM is not set" into a pure
  fix and a cleanup
- Add new patch "don't return invalid value from buffer setup callbacks"
- Add new patch "add adequate wait time to get correct conversion"
- Support traditional comparator mode in addition to window comparator mode,
  add a comment for default threshold settings, and allow setting the state
  that has already been set in threshold event support patch

Akinobu Mita (11):
  iio: adc: ti-ads1015: fix incorrect data rate setting 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: don't return invalid value from buffer setup
    callbacks
  iio: adc: ti-ads1015: add adequate wait time to get correct conversion
  iio: adc: ti-ads1015: remove unnecessary config register update
  iio: adc: ti-ads1015: add helper to set conversion mode
  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 | 592 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 498 insertions(+), 94 deletions(-)

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

[PATCH v2 01/11] 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@gmail.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 884b8e46..443444c 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 v2 01/11] iio: adc: ti-ads1015: fix incorrect data rate setting update

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:17 +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.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Looks good to me - ideally would have a fixes tag.

However, I'm a little nervous about this series in general
so would like to leave a little more time for Daniel to
have a chance to look.

Daniel, feel free to say you are too busy if you are and
I'll go with my own judgement.

Thanks,

Jonathan
> ---
>  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 884b8e46..443444c 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,

Re: [PATCH v2 01/11] iio: adc: ti-ads1015: fix incorrect data rate setting update

[Jonathan Cameron]

On Sun, 23 Jul 2017 12:32:39 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Jul 2017 00:24:17 +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.
> > 
> > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> Looks good to me - ideally would have a fixes tag.
> 
> However, I'm a little nervous about this series in general
> so would like to leave a little more time for Daniel to
> have a chance to look.
> 
> Daniel, feel free to say you are too busy if you are and
> I'll go with my own judgement.

Guessing Daniel is busy.

I'm going to take these via the togreg branch of iio.git given the late
point we are now in the cycle, but mark the fixes for stable so they'll
get picked up after the merge window.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> 
> Thanks,
> 
> Jonathan
> > ---
> >  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 884b8e46..443444c 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,  
> 
> --
> 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 v2 02/11] 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@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 48 ++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 443444c..f32d046 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,17 +294,20 @@ 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, ret, rindex = -1;
+	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) {
+	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
+		if (ads1015_fullscale_range[i] == fullscale) {
 			rindex = i;
 			break;
 		}
+	}
 	if (rindex < 0)
 		return -EINVAL;
 
@@ -320,7 +317,7 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
 	if (ret < 0)
 		return ret;
 
-	data->channel_data[chan].pga = rindex;
+	data->channel_data[chan->address].pga = rindex;
 
 	return 0;
 }
@@ -378,9 +375,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;
@@ -407,7 +404,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);
@@ -439,7 +436,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");
@@ -447,7 +447,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,
 };
@@ -457,7 +457,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 v2 02/11] iio: adc: ti-ads1015: fix scale information for ADS1115

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:18 +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@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied to the togreg branch of iio.git and marked for stable.

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 48 ++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 443444c..f32d046 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,17 +294,20 @@ 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, ret, rindex = -1;
> +	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) {
> +	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
> +		if (ads1015_fullscale_range[i] == fullscale) {
>  			rindex = i;
>  			break;
>  		}
> +	}
>  	if (rindex < 0)
>  		return -EINVAL;
>  
> @@ -320,7 +317,7 @@ static int ads1015_set_scale(struct ads1015_data *data, int chan,
>  	if (ret < 0)
>  		return ret;
>  
> -	data->channel_data[chan].pga = rindex;
> +	data->channel_data[chan->address].pga = rindex;
>  
>  	return 0;
>  }
> @@ -378,9 +375,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;
> @@ -407,7 +404,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);
> @@ -439,7 +436,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");
> @@ -447,7 +447,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,
>  };
> @@ -457,7 +457,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 v2 03/11] 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.

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

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index f32d046..fcfa570 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -623,6 +623,13 @@ static int ads1015_probe(struct i2c_client *client,
 		dev_err(&client->dev, "iio triggered buffer setup failed\n");
 		return ret;
 	}
+
+	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
+				ADS1015_CFG_MOD_MASK,
+				ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
+	if (ret)
+		return ret;
+
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret)
 		goto err_buffer_cleanup;
-- 
2.7.4

Re: [PATCH v2 03/11] iio: adc: ti-ads1015: enable conversion when CONFIG_PM is not set

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:19 +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.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied to the togreg branch of iio.git and marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index f32d046..fcfa570 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -623,6 +623,13 @@ static int ads1015_probe(struct i2c_client *client,
>  		dev_err(&client->dev, "iio triggered buffer setup failed\n");
>  		return ret;
>  	}
> +
> +	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> +				ADS1015_CFG_MOD_MASK,
> +				ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
> +	if (ret)
> +		return ret;
> +
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret)
>  		goto err_buffer_cleanup;

[PATCH v2 04/11] 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@gmail.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 fcfa570..8905f0d 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);
@@ -630,6 +637,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;
@@ -685,10 +694,15 @@ 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 regmap_update_bits(data->regmap, ADS1015_CFG_REG,
+	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
 				  ADS1015_CFG_MOD_MASK,
 				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
+	if (!ret)
+		data->conv_invalid = true;
+
+	return ret;
 }
 #endif
 
-- 
2.7.4

Re: [PATCH v2 04/11] iio: adc: ti-ads1015: avoid getting stale result after runtime resume

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:20 +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@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied and marked for stable.

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 fcfa570..8905f0d 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);
> @@ -630,6 +637,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;
> @@ -685,10 +694,15 @@ 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 regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> +	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
>  				  ADS1015_CFG_MOD_MASK,
>  				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
> +	if (!ret)
> +		data->conv_invalid = true;
> +
> +	return ret;
>  }
>  #endif
>  

[PATCH v2 05/11] iio: adc: ti-ads1015: don't return invalid value from buffer setup callbacks

[Akinobu Mita]

pm_runtime_get_sync() and pm_runtime_put_autosuspend() return 0 on
success, 1 if the device's runtime PM status was already requested status
or error code on failure.  So a positive return value doesn't indicate an
error condition.

However, any non-zero return values from buffer preenable and postdisable
callbacks are recognized as an error and this driver reuses the return
value from pm_runtime_get_sync() and pm_runtime_put_autosuspend() in
these callbacks.  This change fixes the false error detections.

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

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 8905f0d..1c475e2 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -235,7 +235,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 		ret = pm_runtime_put_autosuspend(dev);
 	}
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static
-- 
2.7.4

Re: [PATCH v2 05/11] iio: adc: ti-ads1015: don't return invalid value from buffer setup callbacks

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:21 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> pm_runtime_get_sync() and pm_runtime_put_autosuspend() return 0 on
> success, 1 if the device's runtime PM status was already requested status
> or error code on failure.  So a positive return value doesn't indicate an
> error condition.
> 
> However, any non-zero return values from buffer preenable and postdisable
> callbacks are recognized as an error and this driver reuses the return
> value from pm_runtime_get_sync() and pm_runtime_put_autosuspend() in
> these callbacks.  This change fixes the false error detections.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied to the togreg branch of iio.git and marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 8905f0d..1c475e2 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -235,7 +235,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
>  		ret = pm_runtime_put_autosuspend(dev);
>  	}
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static

[PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Akinobu Mita]

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

Currently, the wait time is only the period required for a single
conversion that is calculated as the reciprocal of the sampling frequency.
However we also need to wait for the the previous conversion to complete.
Otherwise we probably get the conversion result for the previous
configuration when the sampling frequency is lower.

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

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 1c475e2..9c501e5 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -242,27 +242,34 @@ static
 int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 {
 	int ret, pga, dr, conv_time;
-	bool change;
+	unsigned int old, mask, cfg;
 
 	if (chan < 0 || chan >= ADS1015_CHANNELS)
 		return -EINVAL;
 
+	ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
+	if (ret)
+		return ret;
+
 	pga = data->channel_data[chan].pga;
 	dr = data->channel_data[chan].data_rate;
+	mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
+		ADS1015_CFG_DR_MASK;
+	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
+		dr << ADS1015_CFG_DR_SHIFT;
 
-	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);
-	if (ret < 0)
+	cfg = (old & ~mask) | (cfg & mask);
+
+	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
+	if (ret)
 		return ret;
 
-	if (change || data->conv_invalid) {
-		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
+	if (old != cfg || data->conv_invalid) {
+		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
+				ADS1015_CFG_DR_SHIFT;
+
+		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
+		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
 		usleep_range(conv_time, conv_time + 1);
 		data->conv_invalid = false;
 	}
-- 
2.7.4

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:22 +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
> inserts a wait time before reading the conversion register if the
> configuration is changed from a previous request.
> 
> Currently, the wait time is only the period required for a single
> conversion that is calculated as the reciprocal of the sampling frequency.
> However we also need to wait for the the previous conversion to complete.
> Otherwise we probably get the conversion result for the previous
> configuration when the sampling frequency is lower.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Assuming Daniel is happy with these, I propose to take these
first 6 through the fixes-togreg branch and mark them all for stable.

The rest may well have to wait on those patches coming back
around and into the togreg branch of iio.git.

Hence it may be at least a few weeks.

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 1c475e2..9c501e5 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -242,27 +242,34 @@ static
>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  {
>  	int ret, pga, dr, conv_time;
> -	bool change;
> +	unsigned int old, mask, cfg;
>  
>  	if (chan < 0 || chan >= ADS1015_CHANNELS)
>  		return -EINVAL;
>  
> +	ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
> +	if (ret)
> +		return ret;
> +
>  	pga = data->channel_data[chan].pga;
>  	dr = data->channel_data[chan].data_rate;
> +	mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> +		ADS1015_CFG_DR_MASK;
> +	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> +		dr << ADS1015_CFG_DR_SHIFT;
>  
> -	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);
> -	if (ret < 0)
> +	cfg = (old & ~mask) | (cfg & mask);
> +
> +	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> +	if (ret)
>  		return ret;
>  
> -	if (change || data->conv_invalid) {
> -		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> +	if (old != cfg || data->conv_invalid) {
> +		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> +				ADS1015_CFG_DR_SHIFT;
> +
> +		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> +		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
>  		usleep_range(conv_time, conv_time + 1);
>  		data->conv_invalid = false;
>  	}

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Jonathan Cameron]

On Sun, 23 Jul 2017 12:36:18 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Jul 2017 00:24:22 +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
> > inserts a wait time before reading the conversion register if the
> > configuration is changed from a previous request.
> > 
> > Currently, the wait time is only the period required for a single
> > conversion that is calculated as the reciprocal of the sampling frequency.
> > However we also need to wait for the the previous conversion to complete.
> > Otherwise we probably get the conversion result for the previous
> > configuration when the sampling frequency is lower.
> > 
> > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> Assuming Daniel is happy with these, I propose to take these
> first 6 through the fixes-togreg branch and mark them all for stable.

I changed my mind on this given the late staging in the cycle and
am pushing them all through the togreg branch.  The fixes can then
be picked up by stable post merge window which may be the quickest
route at the moment!

Thanks,

Jonathan
> 
> The rest may well have to wait on those patches coming back
> around and into the togreg branch of iio.git.
> 
> Hence it may be at least a few weeks.
> 
> Jonathan
> > ---
> >  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > index 1c475e2..9c501e5 100644
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -242,27 +242,34 @@ static
> >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >  {
> >  	int ret, pga, dr, conv_time;
> > -	bool change;
> > +	unsigned int old, mask, cfg;
> >  
> >  	if (chan < 0 || chan >= ADS1015_CHANNELS)
> >  		return -EINVAL;
> >  
> > +	ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
> > +	if (ret)
> > +		return ret;
> > +
> >  	pga = data->channel_data[chan].pga;
> >  	dr = data->channel_data[chan].data_rate;
> > +	mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> > +		ADS1015_CFG_DR_MASK;
> > +	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> > +		dr << ADS1015_CFG_DR_SHIFT;
> >  
> > -	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);
> > -	if (ret < 0)
> > +	cfg = (old & ~mask) | (cfg & mask);
> > +
> > +	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > +	if (ret)
> >  		return ret;
> >  
> > -	if (change || data->conv_invalid) {
> > -		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > +	if (old != cfg || data->conv_invalid) {
> > +		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> > +				ADS1015_CFG_DR_SHIFT;
> > +
> > +		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> > +		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >  		usleep_range(conv_time, conv_time + 1);
> >  		data->conv_invalid = false;
> >  	}  
> 
> --
> 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

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Ladislav Michl]

On Sun, Aug 20, 2017 at 11:57:30AM +0100, Jonathan Cameron wrote:
> On Sun, 23 Jul 2017 12:36:18 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 21 Jul 2017 00:24:22 +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
> > > inserts a wait time before reading the conversion register if the
> > > configuration is changed from a previous request.
> > > 
> > > Currently, the wait time is only the period required for a single
> > > conversion that is calculated as the reciprocal of the sampling frequency.
> > > However we also need to wait for the the previous conversion to complete.
> > > Otherwise we probably get the conversion result for the previous
> > > configuration when the sampling frequency is lower.
> > > 
> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> > Assuming Daniel is happy with these, I propose to take these
> > first 6 through the fixes-togreg branch and mark them all for stable.
> 
> I changed my mind on this given the late staging in the cycle and
> am pushing them all through the togreg branch.  The fixes can then
> be picked up by stable post merge window which may be the quickest
> route at the moment!

Tested this patch serie and something is still odd, see bellow...
Once sorted out, proper patches will be generated.

> 
> Thanks,
> 
> Jonathan
> > 
> > The rest may well have to wait on those patches coming back
> > around and into the togreg branch of iio.git.
> > 
> > Hence it may be at least a few weeks.
> > 
> > Jonathan
> > > ---
> > >  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
> > >  1 file changed, 19 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > > index 1c475e2..9c501e5 100644
> > > --- a/drivers/iio/adc/ti-ads1015.c
> > > +++ b/drivers/iio/adc/ti-ads1015.c
> > > @@ -242,27 +242,34 @@ static
> > >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> > >  {
> > >  	int ret, pga, dr, conv_time;
> > > -	bool change;
> > > +	unsigned int old, mask, cfg;
> > >  
> > >  	if (chan < 0 || chan >= ADS1015_CHANNELS)
> > >  		return -EINVAL;
> > >  
> > > +	ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	pga = data->channel_data[chan].pga;
> > >  	dr = data->channel_data[chan].data_rate;
> > > +	mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> > > +		ADS1015_CFG_DR_MASK;
> > > +	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> > > +		dr << ADS1015_CFG_DR_SHIFT;
> > >  
> > > -	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);
> > > -	if (ret < 0)
> > > +	cfg = (old & ~mask) | (cfg & mask);
> > > +
> > > +	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > > +	if (ret)
> > >  		return ret;
> > >  
> > > -	if (change || data->conv_invalid) {
> > > -		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > > +	if (old != cfg || data->conv_invalid) {
> > > +		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> > > +				ADS1015_CFG_DR_SHIFT;
> > > +
> > > +		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> > > +		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > >  		usleep_range(conv_time, conv_time + 1);
> > >  		data->conv_invalid = false;
> > >  	}  

Btw, this could be optimized futher this way:
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 static
 int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 {
-	int ret, pga, dr, conv_time;
+	int ret, pga, dr, dr_old, conv_time;
 	unsigned int old, mask, cfg;
 
 	if (chan < 0 || chan >= ADS1015_CHANNELS)
@@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 		ADS1015_CFG_DR_MASK;
 	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
 		dr << ADS1015_CFG_DR_SHIFT;
-
 	cfg = (old & ~mask) | (cfg & mask);
 
-	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
-	if (ret)
-		return ret;
-
 	if (old != cfg || data->conv_invalid) {
-		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
-				ADS1015_CFG_DR_SHIFT;
+		ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
+		if (ret)
+			return ret;
 
+		dr_old = (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_SHIFT;
 		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
 		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
 		usleep_range(conv_time, conv_time + 1);


Now that conv_time is too picky, at first "ADS1015EVM, ADS1115EVM,
ADS1015EVM-PDK, ADS1115EVM-PDK User Guide (Rev. B)" [*] states at page 16:
"Note that both the ADS1115 and ADS1015 have internal clocks with a ±10%
accuracy. If performing FFT tests, frequencies may appear to be incorrect
as a result of this tolerance range.", so we should add at least those 10%
and at second conv_time + 1 is too precise, increasing number of undesired
interrupts, so adding min(dr_period, dr_old_period) instead of 1 should do
the trick.

But the real showstopper is increasing probability of reading stale result
with lowering sample frequency, to debug this following dirty modification
was made to driver:
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -269,6 +269,11 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 
 		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
 		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
+		for (ret = 0; ret < 20; ret++) {
+			regmap_read(data->regmap, ADS1015_CONV_REG, val);
+			printk("%d -> %d\n", ret, *val);
+			usleep_range(conv_time, conv_time + 1);
+		}
 		usleep_range(conv_time, conv_time + 1);
 		data->conv_invalid = false;
 	}

With ADS1015 and sampling frequency set to 128 SPS this leads to:
$ cat in_voltage0_raw && cat in_voltage1_raw
[285276.998382] mode 0
[285277.005096] cfg 4003
[285277.015380] 0 -> 13713
[285277.037109] 1 -> 13713
[285277.058898] 2 -> 13713
[285277.080291] 3 -> 13713
[285277.101715] 4 -> 13713
[285277.123291] 5 -> 13713
[285277.144836] 6 -> 13704
[285277.166503] 7 -> 13704
[285277.188262] 8 -> 13704
[285277.210083] 9 -> 13704
[285277.231811] 10 -> 13704
[285277.253570] 11 -> 13704
[285277.275390] 12 -> 3083
[285277.296936] 13 -> 3083
[285277.318023] 14 -> 3083
[285277.339172] 15 -> 3083
[285277.360076] 16 -> 3083
[285277.381164] 17 -> 3083
[285277.402252] 18 -> 3076
[285277.423339] 19 -> 3076
192
[285277.463623] cfg 5003
[285277.468933] 0 -> 3076
[285277.489746] 1 -> 3076
[285277.510955] 2 -> 3074
[285277.531585] 3 -> 3074
[285277.552612] 4 -> 3074
[285277.573272] 5 -> 3074
[285277.594207] 6 -> 3074
[285277.615173] 7 -> 3074
[285277.636016] 8 -> 13702
[285277.657073] 9 -> 13702
[285277.678131] 10 -> 13702
[285277.699493] 11 -> 13702
[285277.721374] 12 -> 13702
[285277.742492] 13 -> 13702
[285277.763702] 14 -> 13701
[285277.784820] 15 -> 13701
[285277.806030] 16 -> 13701
[285277.827178] 17 -> 13701
[285277.848480] 18 -> 13701
[285277.869689] 19 -> 13701
856

As you can see, it took way longer to switch channel than sampling preriod.
Anyone has a clue what is going on here?

Thank you,
	ladis

[*] http://www.ti.com/lit/ug/sbau157b/sbau157b.pdf

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Akinobu Mita]

2017-08-22 6:00 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
> On Sun, Aug 20, 2017 at 11:57:30AM +0100, Jonathan Cameron wrote:
>> On Sun, 23 Jul 2017 12:36:18 +0100
>> Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> > On Fri, 21 Jul 2017 00:24:22 +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
>> > > inserts a wait time before reading the conversion register if the
>> > > configuration is changed from a previous request.
>> > >
>> > > Currently, the wait time is only the period required for a single
>> > > conversion that is calculated as the reciprocal of the sampling freq=
uency.
>> > > However we also need to wait for the the previous conversion to comp=
lete.
>> > > Otherwise we probably get the conversion result for the previous
>> > > configuration when the sampling frequency is lower.
>> > >
>> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
>> > > Cc: Jonathan Cameron <jic23@kernel.org>
>> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> > Assuming Daniel is happy with these, I propose to take these
>> > first 6 through the fixes-togreg branch and mark them all for stable.
>>
>> I changed my mind on this given the late staging in the cycle and
>> am pushing them all through the togreg branch.  The fixes can then
>> be picked up by stable post merge window which may be the quickest
>> route at the moment!
>
> Tested this patch serie and something is still odd, see bellow...
> Once sorted out, proper patches will be generated.
>
>>
>> Thanks,
>>
>> Jonathan
>> >
>> > The rest may well have to wait on those patches coming back
>> > around and into the togreg branch of iio.git.
>> >
>> > Hence it may be at least a few weeks.
>> >
>> > Jonathan
>> > > ---
>> > >  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
>> > >  1 file changed, 19 insertions(+), 12 deletions(-)
>> > >
>> > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads10=
15.c
>> > > index 1c475e2..9c501e5 100644
>> > > --- a/drivers/iio/adc/ti-ads1015.c
>> > > +++ b/drivers/iio/adc/ti-ads1015.c
>> > > @@ -242,27 +242,34 @@ static
>> > >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int=
 *val)
>> > >  {
>> > >   int ret, pga, dr, conv_time;
>> > > - bool change;
>> > > + unsigned int old, mask, cfg;
>> > >
>> > >   if (chan < 0 || chan >=3D ADS1015_CHANNELS)
>> > >           return -EINVAL;
>> > >
>> > > + ret =3D regmap_read(data->regmap, ADS1015_CFG_REG, &old);
>> > > + if (ret)
>> > > +         return ret;
>> > > +
>> > >   pga =3D data->channel_data[chan].pga;
>> > >   dr =3D data->channel_data[chan].data_rate;
>> > > + mask =3D ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
>> > > +         ADS1015_CFG_DR_MASK;
>> > > + cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHI=
FT |
>> > > +         dr << ADS1015_CFG_DR_SHIFT;
>> > >
>> > > - ret =3D 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);
>> > > - if (ret < 0)
>> > > + cfg =3D (old & ~mask) | (cfg & mask);
>> > > +
>> > > + ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> > > + if (ret)
>> > >           return ret;
>> > >
>> > > - if (change || data->conv_invalid) {
>> > > -         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[d=
r]);
>> > > + if (old !=3D cfg || data->conv_invalid) {
>> > > +         int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
>> > > +                         ADS1015_CFG_DR_SHIFT;
>> > > +
>> > > +         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[d=
r_old]);
>> > > +         conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[=
dr]);
>> > >           usleep_range(conv_time, conv_time + 1);
>> > >           data->conv_invalid =3D false;
>> > >   }
>
> Btw, this could be optimized futher this way:
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_dat=
a *data, bool on)
>  static
>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val=
)
>  {
> -       int ret, pga, dr, conv_time;
> +       int ret, pga, dr, dr_old, conv_time;
>         unsigned int old, mask, cfg;
>
>         if (chan < 0 || chan >=3D ADS1015_CHANNELS)
> @@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *dat=
a, int chan, int *val)
>                 ADS1015_CFG_DR_MASK;
>         cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SH=
IFT |
>                 dr << ADS1015_CFG_DR_SHIFT;
> -
>         cfg =3D (old & ~mask) | (cfg & mask);
>
> -       ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> -       if (ret)
> -               return ret;
> -
>         if (old !=3D cfg || data->conv_invalid) {
> -               int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
> -                               ADS1015_CFG_DR_SHIFT;
> +               ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> +               if (ret)
> +                       return ret;

You can also skip config register write in the case old =3D=3D cfg &&
data->conv_invalid.

> +               dr_old =3D (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_=
SHIFT;
>                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[=
dr_old]);
>                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate=
[dr]);
>                 usleep_range(conv_time, conv_time + 1);
>
>
> Now that conv_time is too picky, at first "ADS1015EVM, ADS1115EVM,
> ADS1015EVM-PDK, ADS1115EVM-PDK User Guide (Rev. B)" [*] states at page 16=
:
> "Note that both the ADS1115 and ADS1015 have internal clocks with a =C2=
=B110%
> accuracy. If performing FFT tests, frequencies may appear to be incorrect
> as a result of this tolerance range.", so we should add at least those 10=
%
> and at second conv_time + 1 is too precise, increasing number of undesire=
d
> interrupts, so adding min(dr_period, dr_old_period) instead of 1 should d=
o
> the trick.

Can we just simply do like below?

        conv_time =3D conv_time * 11 / 10

or

        conv_time +=3D conv_time / 10;

> But the real showstopper is increasing probability of reading stale resul=
t
> with lowering sample frequency, to debug this following dirty modificatio=
n
> was made to driver:
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -269,6 +269,11 @@ int ads1015_get_adc_result(struct ads1015_data *data=
, int chan, int *val)
>
>                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[=
dr_old]);
>                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate=
[dr]);
> +               for (ret =3D 0; ret < 20; ret++) {
> +                       regmap_read(data->regmap, ADS1015_CONV_REG, val);
> +                       printk("%d -> %d\n", ret, *val);
> +                       usleep_range(conv_time, conv_time + 1);
> +               }
>                 usleep_range(conv_time, conv_time + 1);
>                 data->conv_invalid =3D false;
>         }
>
> With ADS1015 and sampling frequency set to 128 SPS this leads to:
> $ cat in_voltage0_raw && cat in_voltage1_raw
> [285276.998382] mode 0
> [285277.005096] cfg 4003
> [285277.015380] 0 -> 13713
> [285277.037109] 1 -> 13713
> [285277.058898] 2 -> 13713
> [285277.080291] 3 -> 13713
> [285277.101715] 4 -> 13713
> [285277.123291] 5 -> 13713
> [285277.144836] 6 -> 13704
> [285277.166503] 7 -> 13704
> [285277.188262] 8 -> 13704
> [285277.210083] 9 -> 13704
> [285277.231811] 10 -> 13704
> [285277.253570] 11 -> 13704
> [285277.275390] 12 -> 3083
> [285277.296936] 13 -> 3083
> [285277.318023] 14 -> 3083
> [285277.339172] 15 -> 3083
> [285277.360076] 16 -> 3083
> [285277.381164] 17 -> 3083
> [285277.402252] 18 -> 3076
> [285277.423339] 19 -> 3076
> 192
> [285277.463623] cfg 5003
> [285277.468933] 0 -> 3076
> [285277.489746] 1 -> 3076
> [285277.510955] 2 -> 3074
> [285277.531585] 3 -> 3074
> [285277.552612] 4 -> 3074
> [285277.573272] 5 -> 3074
> [285277.594207] 6 -> 3074
> [285277.615173] 7 -> 3074
> [285277.636016] 8 -> 13702
> [285277.657073] 9 -> 13702
> [285277.678131] 10 -> 13702
> [285277.699493] 11 -> 13702
> [285277.721374] 12 -> 13702
> [285277.742492] 13 -> 13702
> [285277.763702] 14 -> 13701
> [285277.784820] 15 -> 13701
> [285277.806030] 16 -> 13701
> [285277.827178] 17 -> 13701
> [285277.848480] 18 -> 13701
> [285277.869689] 19 -> 13701
> 856
>
> As you can see, it took way longer to switch channel than sampling prerio=
d.
> Anyone has a clue what is going on here?

Looks like your device is ADS1115 but you use it as ADS1015.

The slowest sampling rate is 128 SPS for ads1015 and 8 SPS for ads1115
when DR field in config register is set to zero.  According to the kernel
debug log message that you have added, the device is working with 8 SPS.

> Thank you,
>         ladis
>
> [*] http://www.ti.com/lit/ug/sbau157b/sbau157b.pdf

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Ladislav Michl]

On Tue, Aug 22, 2017 at 07:03:12PM +0900, Akinobu Mita wrote:
> 2017-08-22 6:00 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
> > On Sun, Aug 20, 2017 at 11:57:30AM +0100, Jonathan Cameron wrote:
> >> On Sun, 23 Jul 2017 12:36:18 +0100
> >> Jonathan Cameron <jic23@kernel.org> wrote:
> >>
> >> > On Fri, 21 Jul 2017 00:24:22 +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
> >> > > inserts a wait time before reading the conversion register if the
> >> > > configuration is changed from a previous request.
> >> > >
> >> > > Currently, the wait time is only the period required for a single
> >> > > conversion that is calculated as the reciprocal of the sampling frequency.
> >> > > However we also need to wait for the the previous conversion to complete.
> >> > > Otherwise we probably get the conversion result for the previous
> >> > > configuration when the sampling frequency is lower.
> >> > >
> >> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> >> > > Cc: Jonathan Cameron <jic23@kernel.org>
> >> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> > Assuming Daniel is happy with these, I propose to take these
> >> > first 6 through the fixes-togreg branch and mark them all for stable.
> >>
> >> I changed my mind on this given the late staging in the cycle and
> >> am pushing them all through the togreg branch.  The fixes can then
> >> be picked up by stable post merge window which may be the quickest
> >> route at the moment!
> >
> > Tested this patch serie and something is still odd, see bellow...
> > Once sorted out, proper patches will be generated.
> >
> >>
> >> Thanks,
> >>
> >> Jonathan
> >> >
> >> > The rest may well have to wait on those patches coming back
> >> > around and into the togreg branch of iio.git.
> >> >
> >> > Hence it may be at least a few weeks.
> >> >
> >> > Jonathan
> >> > > ---
> >> > >  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++------------
> >> > >  1 file changed, 19 insertions(+), 12 deletions(-)
> >> > >
> >> > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> > > index 1c475e2..9c501e5 100644
> >> > > --- a/drivers/iio/adc/ti-ads1015.c
> >> > > +++ b/drivers/iio/adc/ti-ads1015.c
> >> > > @@ -242,27 +242,34 @@ static
> >> > >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >> > >  {
> >> > >   int ret, pga, dr, conv_time;
> >> > > - bool change;
> >> > > + unsigned int old, mask, cfg;
> >> > >
> >> > >   if (chan < 0 || chan >= ADS1015_CHANNELS)
> >> > >           return -EINVAL;
> >> > >
> >> > > + ret = regmap_read(data->regmap, ADS1015_CFG_REG, &old);
> >> > > + if (ret)
> >> > > +         return ret;
> >> > > +
> >> > >   pga = data->channel_data[chan].pga;
> >> > >   dr = data->channel_data[chan].data_rate;
> >> > > + mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> >> > > +         ADS1015_CFG_DR_MASK;
> >> > > + cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >> > > +         dr << ADS1015_CFG_DR_SHIFT;
> >> > >
> >> > > - 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);
> >> > > - if (ret < 0)
> >> > > + cfg = (old & ~mask) | (cfg & mask);
> >> > > +
> >> > > + ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> >> > > + if (ret)
> >> > >           return ret;
> >> > >
> >> > > - if (change || data->conv_invalid) {
> >> > > -         conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >> > > + if (old != cfg || data->conv_invalid) {
> >> > > +         int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> >> > > +                         ADS1015_CFG_DR_SHIFT;
> >> > > +
> >> > > +         conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> >> > > +         conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >> > >           usleep_range(conv_time, conv_time + 1);
> >> > >           data->conv_invalid = false;
> >> > >   }
> >
> > Btw, this could be optimized futher this way:
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
> >  static
> >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >  {
> > -       int ret, pga, dr, conv_time;
> > +       int ret, pga, dr, dr_old, conv_time;
> >         unsigned int old, mask, cfg;
> >
> >         if (chan < 0 || chan >= ADS1015_CHANNELS)
> > @@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >                 ADS1015_CFG_DR_MASK;
> >         cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >                 dr << ADS1015_CFG_DR_SHIFT;
> > -
> >         cfg = (old & ~mask) | (cfg & mask);
> >
> > -       ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > -       if (ret)
> > -               return ret;
> > -
> >         if (old != cfg || data->conv_invalid) {
> > -               int dr_old = (old & ADS1015_CFG_DR_MASK) >>
> > -                               ADS1015_CFG_DR_SHIFT;
> > +               ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > +               if (ret)
> > +                       return ret;
> 
> You can also skip config register write in the case old == cfg &&
> data->conv_invalid.

Yes, something like this:
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 static
 int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 {
-	int ret, pga, dr, conv_time;
+	int ret, pga, dr, dr_old, conv_time;
 	unsigned int old, mask, cfg;
 
 	if (chan < 0 || chan >= ADS1015_CHANNELS)
@@ -256,17 +256,16 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 		ADS1015_CFG_DR_MASK;
 	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
 		dr << ADS1015_CFG_DR_SHIFT;
-
 	cfg = (old & ~mask) | (cfg & mask);
 
-	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
-	if (ret)
-		return ret;
-
-	if (old != cfg || data->conv_invalid) {
-		int dr_old = (old & ADS1015_CFG_DR_MASK) >>
-				ADS1015_CFG_DR_SHIFT;
-
+	if (old != cfg) {
+		ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
+		if (ret)
+			return ret;
+		data->conv_invalid = true;
+	}
+	if (data->conv_invalid) {
+		dr_old = (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_SHIFT;
 		conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
 		conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
 		usleep_range(conv_time, conv_time + 1);

I was temped to use goto, but compiler will probably optimize anyway.

> > +               dr_old = (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_SHIFT;
> >                 conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> >                 conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> >                 usleep_range(conv_time, conv_time + 1);
> >
> >
> > Now that conv_time is too picky, at first "ADS1015EVM, ADS1115EVM,
> > ADS1015EVM-PDK, ADS1115EVM-PDK User Guide (Rev. B)" [*] states at page 16:
> > "Note that both the ADS1115 and ADS1015 have internal clocks with a ±10%
> > accuracy. If performing FFT tests, frequencies may appear to be incorrect
> > as a result of this tolerance range.", so we should add at least those 10%
> > and at second conv_time + 1 is too precise, increasing number of undesired
> > interrupts, so adding min(dr_period, dr_old_period) instead of 1 should do
> > the trick.
> 
> Can we just simply do like below?
> 
>         conv_time = conv_time * 11 / 10
> 
> or
> 
>         conv_time += conv_time / 10;

That's definitely enough. Question is whenever we should care about usleep_range
being only 1us wide. See Documentation/timers/timers-howto.txt

> > But the real showstopper is increasing probability of reading stale result
> > with lowering sample frequency, to debug this following dirty modification
> > was made to driver:
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -269,6 +269,11 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >
> >                 conv_time = DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr_old]);
> >                 conv_time += DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[dr]);
> > +               for (ret = 0; ret < 20; ret++) {
> > +                       regmap_read(data->regmap, ADS1015_CONV_REG, val);
> > +                       printk("%d -> %d\n", ret, *val);
> > +                       usleep_range(conv_time, conv_time + 1);
> > +               }
> >                 usleep_range(conv_time, conv_time + 1);
> >                 data->conv_invalid = false;
> >         }
> >
> > With ADS1015 and sampling frequency set to 128 SPS this leads to:
> > $ cat in_voltage0_raw && cat in_voltage1_raw
> > [285276.998382] mode 0
> > [285277.005096] cfg 4003
> > [285277.015380] 0 -> 13713
> > [285277.037109] 1 -> 13713
> > [285277.058898] 2 -> 13713
> > [285277.080291] 3 -> 13713
> > [285277.101715] 4 -> 13713
> > [285277.123291] 5 -> 13713
> > [285277.144836] 6 -> 13704
> > [285277.166503] 7 -> 13704
> > [285277.188262] 8 -> 13704
> > [285277.210083] 9 -> 13704
> > [285277.231811] 10 -> 13704
> > [285277.253570] 11 -> 13704
> > [285277.275390] 12 -> 3083
> > [285277.296936] 13 -> 3083
> > [285277.318023] 14 -> 3083
> > [285277.339172] 15 -> 3083
> > [285277.360076] 16 -> 3083
> > [285277.381164] 17 -> 3083
> > [285277.402252] 18 -> 3076
> > [285277.423339] 19 -> 3076
> > 192
> > [285277.463623] cfg 5003
> > [285277.468933] 0 -> 3076
> > [285277.489746] 1 -> 3076
> > [285277.510955] 2 -> 3074
> > [285277.531585] 3 -> 3074
> > [285277.552612] 4 -> 3074
> > [285277.573272] 5 -> 3074
> > [285277.594207] 6 -> 3074
> > [285277.615173] 7 -> 3074
> > [285277.636016] 8 -> 13702
> > [285277.657073] 9 -> 13702
> > [285277.678131] 10 -> 13702
> > [285277.699493] 11 -> 13702
> > [285277.721374] 12 -> 13702
> > [285277.742492] 13 -> 13702
> > [285277.763702] 14 -> 13701
> > [285277.784820] 15 -> 13701
> > [285277.806030] 16 -> 13701
> > [285277.827178] 17 -> 13701
> > [285277.848480] 18 -> 13701
> > [285277.869689] 19 -> 13701
> > 856
> >
> > As you can see, it took way longer to switch channel than sampling preriod.
> > Anyone has a clue what is going on here?
> 
> Looks like your device is ADS1115 but you use it as ADS1015.

Package mark reads 03 BOGI, which is indeed ADS1115. That's a shame and I
have no excuse for it. Previous samples come with BRPI and I didn't check
this time. Sorry for the noise.

> The slowest sampling rate is 128 SPS for ads1015 and 8 SPS for ads1115
> when DR field in config register is set to zero.  According to the kernel
> debug log message that you have added, the device is working with 8 SPS.
> 
> > Thank you,
> >         ladis
> >
> > [*] http://www.ti.com/lit/ug/sbau157b/sbau157b.pdf

Re: [PATCH v2 06/11] iio: adc: ti-ads1015: add adequate wait time to get correct conversion

[Akinobu Mita]

2017-08-22 23:36 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
> On Tue, Aug 22, 2017 at 07:03:12PM +0900, Akinobu Mita wrote:
>> 2017-08-22 6:00 GMT+09:00 Ladislav Michl <ladis@linux-mips.org>:
>> > On Sun, Aug 20, 2017 at 11:57:30AM +0100, Jonathan Cameron wrote:
>> >> On Sun, 23 Jul 2017 12:36:18 +0100
>> >> Jonathan Cameron <jic23@kernel.org> wrote:
>> >>
>> >> > On Fri, 21 Jul 2017 00:24:22 +0900
>> >> > Akinobu Mita <akinobu.mita@gmail.com> wrote:
>> >> >
>> >> > > This driver assumes that the device is operating in the continuou=
s
>> >> > > conversion mode which performs the conversion continuously.  So t=
his driver
>> >> > > inserts a wait time before reading the conversion register if the
>> >> > > configuration is changed from a previous request.
>> >> > >
>> >> > > Currently, the wait time is only the period required for a single
>> >> > > conversion that is calculated as the reciprocal of the sampling f=
requency.
>> >> > > However we also need to wait for the the previous conversion to c=
omplete.
>> >> > > Otherwise we probably get the conversion result for the previous
>> >> > > configuration when the sampling frequency is lower.
>> >> > >
>> >> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
>> >> > > Cc: Jonathan Cameron <jic23@kernel.org>
>> >> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> >> > Assuming Daniel is happy with these, I propose to take these
>> >> > first 6 through the fixes-togreg branch and mark them all for stabl=
e.
>> >>
>> >> I changed my mind on this given the late staging in the cycle and
>> >> am pushing them all through the togreg branch.  The fixes can then
>> >> be picked up by stable post merge window which may be the quickest
>> >> route at the moment!
>> >
>> > Tested this patch serie and something is still odd, see bellow...
>> > Once sorted out, proper patches will be generated.
>> >
>> >>
>> >> Thanks,
>> >>
>> >> Jonathan
>> >> >
>> >> > The rest may well have to wait on those patches coming back
>> >> > around and into the togreg branch of iio.git.
>> >> >
>> >> > Hence it may be at least a few weeks.
>> >> >
>> >> > Jonathan
>> >> > > ---
>> >> > >  drivers/iio/adc/ti-ads1015.c | 31 +++++++++++++++++++-----------=
-
>> >> > >  1 file changed, 19 insertions(+), 12 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ad=
s1015.c
>> >> > > index 1c475e2..9c501e5 100644
>> >> > > --- a/drivers/iio/adc/ti-ads1015.c
>> >> > > +++ b/drivers/iio/adc/ti-ads1015.c
>> >> > > @@ -242,27 +242,34 @@ static
>> >> > >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, =
int *val)
>> >> > >  {
>> >> > >   int ret, pga, dr, conv_time;
>> >> > > - bool change;
>> >> > > + unsigned int old, mask, cfg;
>> >> > >
>> >> > >   if (chan < 0 || chan >=3D ADS1015_CHANNELS)
>> >> > >           return -EINVAL;
>> >> > >
>> >> > > + ret =3D regmap_read(data->regmap, ADS1015_CFG_REG, &old);
>> >> > > + if (ret)
>> >> > > +         return ret;
>> >> > > +
>> >> > >   pga =3D data->channel_data[chan].pga;
>> >> > >   dr =3D data->channel_data[chan].data_rate;
>> >> > > + mask =3D ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
>> >> > > +         ADS1015_CFG_DR_MASK;
>> >> > > + cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_=
SHIFT |
>> >> > > +         dr << ADS1015_CFG_DR_SHIFT;
>> >> > >
>> >> > > - ret =3D 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);
>> >> > > - if (ret < 0)
>> >> > > + cfg =3D (old & ~mask) | (cfg & mask);
>> >> > > +
>> >> > > + ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> >> > > + if (ret)
>> >> > >           return ret;
>> >> > >
>> >> > > - if (change || data->conv_invalid) {
>> >> > > -         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rat=
e[dr]);
>> >> > > + if (old !=3D cfg || data->conv_invalid) {
>> >> > > +         int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
>> >> > > +                         ADS1015_CFG_DR_SHIFT;
>> >> > > +
>> >> > > +         conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rat=
e[dr_old]);
>> >> > > +         conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr]);
>> >> > >           usleep_range(conv_time, conv_time + 1);
>> >> > >           data->conv_invalid =3D false;
>> >> > >   }
>> >
>> > Btw, this could be optimized futher this way:
>> > --- a/drivers/iio/adc/ti-ads1015.c
>> > +++ b/drivers/iio/adc/ti-ads1015.c
>> > @@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_=
data *data, bool on)
>> >  static
>> >  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *=
val)
>> >  {
>> > -       int ret, pga, dr, conv_time;
>> > +       int ret, pga, dr, dr_old, conv_time;
>> >         unsigned int old, mask, cfg;
>> >
>> >         if (chan < 0 || chan >=3D ADS1015_CHANNELS)
>> > @@ -256,17 +256,14 @@ int ads1015_get_adc_result(struct ads1015_data *=
data, int chan, int *val)
>> >                 ADS1015_CFG_DR_MASK;
>> >         cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA=
_SHIFT |
>> >                 dr << ADS1015_CFG_DR_SHIFT;
>> > -
>> >         cfg =3D (old & ~mask) | (cfg & mask);
>> >
>> > -       ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> > -       if (ret)
>> > -               return ret;
>> > -
>> >         if (old !=3D cfg || data->conv_invalid) {
>> > -               int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
>> > -                               ADS1015_CFG_DR_SHIFT;
>> > +               ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cf=
g);
>> > +               if (ret)
>> > +                       return ret;
>>
>> You can also skip config register write in the case old =3D=3D cfg &&
>> data->conv_invalid.
>
> Yes, something like this:
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -240,7 +240,7 @@ static int ads1015_set_power_state(struct ads1015_dat=
a *data, bool on)
>  static
>  int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val=
)
>  {
> -       int ret, pga, dr, conv_time;
> +       int ret, pga, dr, dr_old, conv_time;
>         unsigned int old, mask, cfg;
>
>         if (chan < 0 || chan >=3D ADS1015_CHANNELS)
> @@ -256,17 +256,16 @@ int ads1015_get_adc_result(struct ads1015_data *dat=
a, int chan, int *val)
>                 ADS1015_CFG_DR_MASK;
>         cfg =3D chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SH=
IFT |
>                 dr << ADS1015_CFG_DR_SHIFT;
> -
>         cfg =3D (old & ~mask) | (cfg & mask);
>
> -       ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> -       if (ret)
> -               return ret;
> -
> -       if (old !=3D cfg || data->conv_invalid) {
> -               int dr_old =3D (old & ADS1015_CFG_DR_MASK) >>
> -                               ADS1015_CFG_DR_SHIFT;
> -
> +       if (old !=3D cfg) {
> +               ret =3D regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> +               if (ret)
> +                       return ret;
> +               data->conv_invalid =3D true;
> +       }
> +       if (data->conv_invalid) {
> +               dr_old =3D (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_DR_=
SHIFT;
>                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate[=
dr_old]);
>                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_rate=
[dr]);
>                 usleep_range(conv_time, conv_time + 1);
>
> I was temped to use goto, but compiler will probably optimize anyway.

Looks good to me.

>> > +               dr_old =3D (old & ADS1015_CFG_DR_MASK) >> ADS1015_CFG_=
DR_SHIFT;
>> >                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr_old]);
>> >                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_r=
ate[dr]);
>> >                 usleep_range(conv_time, conv_time + 1);
>> >
>> >
>> > Now that conv_time is too picky, at first "ADS1015EVM, ADS1115EVM,
>> > ADS1015EVM-PDK, ADS1115EVM-PDK User Guide (Rev. B)" [*] states at page=
 16:
>> > "Note that both the ADS1115 and ADS1015 have internal clocks with a =
=C2=B110%
>> > accuracy. If performing FFT tests, frequencies may appear to be incorr=
ect
>> > as a result of this tolerance range.", so we should add at least those=
 10%
>> > and at second conv_time + 1 is too precise, increasing number of undes=
ired
>> > interrupts, so adding min(dr_period, dr_old_period) instead of 1 shoul=
d do
>> > the trick.
>>
>> Can we just simply do like below?
>>
>>         conv_time =3D conv_time * 11 / 10
>>
>> or
>>
>>         conv_time +=3D conv_time / 10;
>
> That's definitely enough. Question is whenever we should care about uslee=
p_range
> being only 1us wide. See Documentation/timers/timers-howto.txt

I have no particular opinion on how large range we should choose for
the usleep_range call.  The range has been 1us since this driver has
been committed at first time.

If there is a reasonable range other than 1us, it should be prepared as
a separate patch.  Because it is separate issue from increasing the
minimum conv_time by 10%.

>> > But the real showstopper is increasing probability of reading stale re=
sult
>> > with lowering sample frequency, to debug this following dirty modifica=
tion
>> > was made to driver:
>> > --- a/drivers/iio/adc/ti-ads1015.c
>> > +++ b/drivers/iio/adc/ti-ads1015.c
>> > @@ -269,6 +269,11 @@ int ads1015_get_adc_result(struct ads1015_data *d=
ata, int chan, int *val)
>> >
>> >                 conv_time =3D DIV_ROUND_UP(USEC_PER_SEC, data->data_ra=
te[dr_old]);
>> >                 conv_time +=3D DIV_ROUND_UP(USEC_PER_SEC, data->data_r=
ate[dr]);
>> > +               for (ret =3D 0; ret < 20; ret++) {
>> > +                       regmap_read(data->regmap, ADS1015_CONV_REG, va=
l);
>> > +                       printk("%d -> %d\n", ret, *val);
>> > +                       usleep_range(conv_time, conv_time + 1);
>> > +               }
>> >                 usleep_range(conv_time, conv_time + 1);
>> >                 data->conv_invalid =3D false;
>> >         }
>> >
>> > With ADS1015 and sampling frequency set to 128 SPS this leads to:
>> > $ cat in_voltage0_raw && cat in_voltage1_raw
>> > [285276.998382] mode 0
>> > [285277.005096] cfg 4003
>> > [285277.015380] 0 -> 13713
>> > [285277.037109] 1 -> 13713
>> > [285277.058898] 2 -> 13713
>> > [285277.080291] 3 -> 13713
>> > [285277.101715] 4 -> 13713
>> > [285277.123291] 5 -> 13713
>> > [285277.144836] 6 -> 13704
>> > [285277.166503] 7 -> 13704
>> > [285277.188262] 8 -> 13704
>> > [285277.210083] 9 -> 13704
>> > [285277.231811] 10 -> 13704
>> > [285277.253570] 11 -> 13704
>> > [285277.275390] 12 -> 3083
>> > [285277.296936] 13 -> 3083
>> > [285277.318023] 14 -> 3083
>> > [285277.339172] 15 -> 3083
>> > [285277.360076] 16 -> 3083
>> > [285277.381164] 17 -> 3083
>> > [285277.402252] 18 -> 3076
>> > [285277.423339] 19 -> 3076
>> > 192
>> > [285277.463623] cfg 5003
>> > [285277.468933] 0 -> 3076
>> > [285277.489746] 1 -> 3076
>> > [285277.510955] 2 -> 3074
>> > [285277.531585] 3 -> 3074
>> > [285277.552612] 4 -> 3074
>> > [285277.573272] 5 -> 3074
>> > [285277.594207] 6 -> 3074
>> > [285277.615173] 7 -> 3074
>> > [285277.636016] 8 -> 13702
>> > [285277.657073] 9 -> 13702
>> > [285277.678131] 10 -> 13702
>> > [285277.699493] 11 -> 13702
>> > [285277.721374] 12 -> 13702
>> > [285277.742492] 13 -> 13702
>> > [285277.763702] 14 -> 13701
>> > [285277.784820] 15 -> 13701
>> > [285277.806030] 16 -> 13701
>> > [285277.827178] 17 -> 13701
>> > [285277.848480] 18 -> 13701
>> > [285277.869689] 19 -> 13701
>> > 856
>> >
>> > As you can see, it took way longer to switch channel than sampling pre=
riod.
>> > Anyone has a clue what is going on here?
>>
>> Looks like your device is ADS1115 but you use it as ADS1015.
>
> Package mark reads 03 BOGI, which is indeed ADS1115. That's a shame and I
> have no excuse for it. Previous samples come with BRPI and I didn't check
> this time. Sorry for the noise.

No problem. Thanks for testing.

>> The slowest sampling rate is 128 SPS for ads1015 and 8 SPS for ads1115
>> when DR field in config register is set to zero.  According to the kerne=
l
>> debug log message that you have added, the device is working with 8 SPS.
>>
>> > Thank you,
>> >         ladis
>> >
>> > [*] http://www.ti.com/lit/ug/sbau157b/sbau157b.pdf

[PATCH v2 07/11] 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@gmail.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, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 9c501e5..e83cebc 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -312,28 +312,18 @@ static int ads1015_set_scale(struct ads1015_data *data,
 			     struct iio_chan_spec const *chan,
 			     int scale, int uscale)
 {
-	int i, ret, rindex = -1;
+	int i;
 	int fullscale = div_s64((scale * 1000000LL + uscale) <<
 				(chan->scan_type.realbits - 1), 1000000);
 
 	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
 		if (ads1015_fullscale_range[i] == fullscale) {
-			rindex = i;
-			break;
+			data->channel_data[chan->address].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->address].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 v2 07/11] iio: adc: ti-ads1015: remove unnecessary config register update

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:23 +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@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 9c501e5..e83cebc 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -312,28 +312,18 @@ static int ads1015_set_scale(struct ads1015_data *data,
>  			     struct iio_chan_spec const *chan,
>  			     int scale, int uscale)
>  {
> -	int i, ret, rindex = -1;
> +	int i;
>  	int fullscale = div_s64((scale * 1000000LL + uscale) <<
>  				(chan->scan_type.realbits - 1), 1000000);
>  
>  	for (i = 0; i < ARRAY_SIZE(ads1015_fullscale_range); i++) {
>  		if (ads1015_fullscale_range[i] == fullscale) {
> -			rindex = i;
> -			break;
> +			data->channel_data[chan->address].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->address].pga = rindex;
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate)

[PATCH v2 08/11] iio: adc: ti-ads1015: add helper to set conversion mode

[Akinobu Mita]

This adds a helper function to set conversion mode as there are a fair
number of users.

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

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index e83cebc..9f3f0c9 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -570,6 +570,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)
 {
@@ -628,9 +635,7 @@ static int ads1015_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				ADS1015_CFG_MOD_MASK,
-				ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
+	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
 	if (ret)
 		return ret;
 
@@ -671,9 +676,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
@@ -682,9 +685,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)
@@ -693,9 +694,7 @@ static int ads1015_runtime_resume(struct device *dev)
 	struct ads1015_data *data = iio_priv(indio_dev);
 	int ret;
 
-	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
-				  ADS1015_CFG_MOD_MASK,
-				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
+	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
 	if (!ret)
 		data->conv_invalid = true;
 
-- 
2.7.4

Re: [PATCH v2 08/11] iio: adc: ti-ads1015: add helper to set conversion mode

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:24 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> This adds a helper function to set conversion mode as there are a fair
> number of users.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index e83cebc..9f3f0c9 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -570,6 +570,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)
>  {
> @@ -628,9 +635,7 @@ static int ads1015_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				ADS1015_CFG_MOD_MASK,
> -				ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
> +	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
>  	if (ret)
>  		return ret;
>  
> @@ -671,9 +676,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
> @@ -682,9 +685,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)
> @@ -693,9 +694,7 @@ static int ads1015_runtime_resume(struct device *dev)
>  	struct ads1015_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> -				  ADS1015_CFG_MOD_MASK,
> -				  ADS1015_CONTINUOUS << ADS1015_CFG_MOD_SHIFT);
> +	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
>  	if (!ret)
>  		data->conv_invalid = true;
>  

[PATCH v2 09/11] 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.

This changes the remove order, but the end result of remove function
actually does the reverse of probe.

Cc: Daniel Baluta <daniel.baluta@gmail.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 9f3f0c9..da25780 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -627,9 +627,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;
@@ -643,7 +643,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);
@@ -651,15 +651,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)
@@ -673,8 +668,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 v2 09/11] iio: adc: ti-ads1015: use devm_iio_triggered_buffer_setup

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:25 +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.
> 
> This changes the remove order, but the end result of remove function
> actually does the reverse of probe.
> 
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied.

Thanks,

Joanthan
> ---
>  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 9f3f0c9..da25780 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -627,9 +627,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;
> @@ -643,7 +643,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);
> @@ -651,15 +651,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)
> @@ -673,8 +668,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);
>  }

[PATCH v2 10/11] 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@gmail.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 da25780..5568be4 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -347,34 +347,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:
@@ -393,7 +393,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 v2 10/11] iio: adc: ti-ads1015: use iio_device_claim_direct_mode()

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:26 +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@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Applied.

Thanks,

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 da25780..5568be4 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -347,34 +347,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:
> @@ -393,7 +393,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  		break;
>  	}
>  	mutex_unlock(&data->lock);
> -	mutex_unlock(&indio_dev->mlock);
>  
>  	return ret;
>  }

[PATCH v2 11/11] 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@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 404 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 5568be4..54547a2 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,30 @@ 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) |
+				BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.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 +152,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 +175,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 +195,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 +217,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;
+	int high_thresh;
+	int low_thresh;
+};
+
 struct ads1015_data {
 	struct regmap *regmap;
 	/*
@@ -176,6 +237,10 @@ struct ads1015_data {
 	struct mutex lock;
 	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 
+	unsigned int event_channel;
+	unsigned int comp_mode;
+	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 +250,41 @@ 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,
+					 int comp_mode)
+{
+	WARN_ON(ads1015_event_channel_enabled(data));
+
+	data->event_channel = chan;
+	data->comp_mode = comp_mode;
+}
+
+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,
 };
 
@@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
 	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
 		dr << ADS1015_CFG_DR_SHIFT;
 
+	if (ads1015_event_channel_enabled(data)) {
+		mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
+		cfg |= data->thresh_data[chan].comp_queue <<
+				ADS1015_CFG_COMP_QUE_SHIFT |
+			data->comp_mode <<
+				ADS1015_CFG_COMP_MODE_SHIFT;
+	}
+
 	cfg = (old & ~mask) | (cfg & mask);
 
 	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
@@ -356,6 +455,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;
@@ -421,8 +526,251 @@ 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 = 0;
+
+	mutex_lock(&data->lock);
+	if (data->event_channel == chan->address) {
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = 1;
+			break;
+		case IIO_EV_DIR_EITHER:
+			ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+	}
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int ads1015_enable_event_config(struct ads1015_data *data,
+	const struct iio_chan_spec *chan, int comp_mode)
+{
+	int low_thresh = data->thresh_data[chan->address].low_thresh;
+	int high_thresh = data->thresh_data[chan->address].high_thresh;
+	int ret;
+	unsigned int val;
+
+	if (ads1015_event_channel_enabled(data)) {
+		if (data->event_channel != chan->address ||
+			(data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
+				comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
+			return -EBUSY;
+
+		return 0;
+	}
+
+	if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
+		low_thresh = max(-1 << (chan->scan_type.realbits - 1),
+				high_thresh - 1);
+	}
+	ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
+			low_thresh << chan->scan_type.shift);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
+			high_thresh << chan->scan_type.shift);
+	if (ret)
+		return ret;
+
+	ret = ads1015_set_power_state(data, true);
+	if (ret < 0)
+		return ret;
+
+	ads1015_event_channel_enable(data, chan->address, comp_mode);
+
+	ret = ads1015_get_adc_result(data, chan->address, &val);
+	if (ret) {
+		ads1015_event_channel_disable(data, chan->address);
+		ads1015_set_power_state(data, false);
+	}
+
+	return ret;
+}
+
+static int ads1015_disable_event_config(struct ads1015_data *data,
+	const struct iio_chan_spec *chan, int comp_mode)
+{
+	int ret;
+
+	if (!ads1015_event_channel_enabled(data))
+		return 0;
+
+	if (data->event_channel != chan->address)
+		return 0;
+
+	if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
+			comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
+		return 0;
+
+	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)
+		return ret;
+
+	ads1015_event_channel_disable(data, chan->address);
+
+	return ads1015_set_power_state(data, false);
+}
+
+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;
+	int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
+		ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
+
+	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 (state)
+		ret = ads1015_enable_event_config(data, chan, comp_mode);
+	else
+		ret = ads1015_disable_event_config(data, chan, comp_mode);
+
+	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)) {
+		enum iio_event_direction dir;
+		u64 code;
+
+		dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
+					IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
+		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
+					IIO_EV_TYPE_THRESH, dir);
+		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);
 }
 
@@ -473,6 +821,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,
 };
 
@@ -480,6 +832,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,
 };
 
@@ -583,6 +939,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)
@@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
 		break;
 	}
 
+	data->event_channel = ADS1015_CHANNELS;
+	/*
+	 * Set default lower and upper threshold to min and max value
+	 * respectively.
+	 */
+	for (i = 0; i < ADS1015_CHANNELS; i++) {
+		int realbits = indio_dev->channels[i].scan_type.realbits;
+
+		data->thresh_data[i].low_thresh = -1 << (realbits - 1);
+		data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
+	}
+
 	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
 	ads1015_get_channels_config(client);
 
@@ -634,6 +1003,39 @@ 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;
+		unsigned int cfg_comp =
+			ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
+			1 << ADS1015_CFG_COMP_LAT_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 v2 11/11] iio: adc: ti-ads1015: add threshold event support

[Jonathan Cameron]

On Fri, 21 Jul 2017 00:24:27 +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@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
one really minor point inline. 

Otherwise I'm happy with this set.  Just waiting to see if
Daniel has time to take a look (or anyone else!)

Jonathan
> ---
>  drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 404 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 5568be4..54547a2 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,30 @@ 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) |
> +				BIT(IIO_EV_INFO_ENABLE),
> +	}, {
> +		.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 +152,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 +175,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 +195,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 +217,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;
> +	int high_thresh;
> +	int low_thresh;
> +};
> +
>  struct ads1015_data {
>  	struct regmap *regmap;
>  	/*
> @@ -176,6 +237,10 @@ struct ads1015_data {
>  	struct mutex lock;
>  	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>  
> +	unsigned int event_channel;
> +	unsigned int comp_mode;
> +	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 +250,41 @@ 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,
> +					 int comp_mode)
> +{
> +	WARN_ON(ads1015_event_channel_enabled(data));
> +
> +	data->event_channel = chan;
> +	data->comp_mode = comp_mode;
> +}
> +
> +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,
>  };
>  
> @@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
>  		dr << ADS1015_CFG_DR_SHIFT;
>  
> +	if (ads1015_event_channel_enabled(data)) {
> +		mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
> +		cfg |= data->thresh_data[chan].comp_queue <<
> +				ADS1015_CFG_COMP_QUE_SHIFT |
> +			data->comp_mode <<
> +				ADS1015_CFG_COMP_MODE_SHIFT;
> +	}
> +
>  	cfg = (old & ~mask) | (cfg & mask);
>  
>  	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> @@ -356,6 +455,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;
> @@ -421,8 +526,251 @@ 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 = 0;
> +
> +	mutex_lock(&data->lock);
> +	if (data->event_channel == chan->address) {
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = 1;
> +			break;
> +		case IIO_EV_DIR_EITHER:
> +			ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int ads1015_enable_event_config(struct ads1015_data *data,
> +	const struct iio_chan_spec *chan, int comp_mode)
> +{
> +	int low_thresh = data->thresh_data[chan->address].low_thresh;
> +	int high_thresh = data->thresh_data[chan->address].high_thresh;
> +	int ret;
> +	unsigned int val;
> +
> +	if (ads1015_event_channel_enabled(data)) {
> +		if (data->event_channel != chan->address ||
> +			(data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> +				comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
> +			return -EBUSY;
> +
> +		return 0;
> +	}
> +
> +	if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
> +		low_thresh = max(-1 << (chan->scan_type.realbits - 1),
> +				high_thresh - 1);
> +	}
> +	ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> +			low_thresh << chan->scan_type.shift);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> +			high_thresh << chan->scan_type.shift);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1015_set_power_state(data, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ads1015_event_channel_enable(data, chan->address, comp_mode);
> +
> +	ret = ads1015_get_adc_result(data, chan->address, &val);
> +	if (ret) {
> +		ads1015_event_channel_disable(data, chan->address);
> +		ads1015_set_power_state(data, false);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ads1015_disable_event_config(struct ads1015_data *data,
> +	const struct iio_chan_spec *chan, int comp_mode)
> +{
> +	int ret;
> +
> +	if (!ads1015_event_channel_enabled(data))
> +		return 0;
> +
> +	if (data->event_channel != chan->address)
> +		return 0;
> +
> +	if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> +			comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
> +		return 0;
> +
> +	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)
> +		return ret;
> +
> +	ads1015_event_channel_disable(data, chan->address);
> +
> +	return ads1015_set_power_state(data, false);
> +}
> +
> +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;
> +	int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
> +		ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
> +
> +	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 (state)
> +		ret = ads1015_enable_event_config(data, chan, comp_mode);
> +	else
> +		ret = ads1015_disable_event_config(data, chan, comp_mode);
> +
> +	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);
error checking?  Obviously you can't do anything much with
the error, but you don't want to muddle through the rest
with it.
> +
> +	if (ads1015_event_channel_enabled(data)) {
> +		enum iio_event_direction dir;
> +		u64 code;
> +
> +		dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
> +					IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
> +		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
> +					IIO_EV_TYPE_THRESH, dir);
> +		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);
>  }
>  
> @@ -473,6 +821,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,
>  };
>  
> @@ -480,6 +832,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,
>  };
>  
> @@ -583,6 +939,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)
> @@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
>  		break;
>  	}
>  
> +	data->event_channel = ADS1015_CHANNELS;
> +	/*
> +	 * Set default lower and upper threshold to min and max value
> +	 * respectively.
> +	 */
> +	for (i = 0; i < ADS1015_CHANNELS; i++) {
> +		int realbits = indio_dev->channels[i].scan_type.realbits;
> +
> +		data->thresh_data[i].low_thresh = -1 << (realbits - 1);
> +		data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
> +	}
> +
>  	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
>  	ads1015_get_channels_config(client);
>  
> @@ -634,6 +1003,39 @@ 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;
> +		unsigned int cfg_comp =
> +			ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> +			1 << ADS1015_CFG_COMP_LAT_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 v2 11/11] iio: adc: ti-ads1015: add threshold event support

[Jonathan Cameron]

On Sun, 23 Jul 2017 13:01:43 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Jul 2017 00:24:27 +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@gmail.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> one really minor point inline. 
> 
> Otherwise I'm happy with this set.  Just waiting to see if
> Daniel has time to take a look (or anyone else!)
> 

I added a really trivial check on the error as mentioned inline and
applied to the togreg branch of iio.git and pushed out as testing.

If you could sanity check my change that would be great.

Thanks,

Jonathan

> Jonathan
> > ---
> >  drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 404 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > index 5568be4..54547a2 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,30 @@ 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) |
> > +				BIT(IIO_EV_INFO_ENABLE),
> > +	}, {
> > +		.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 +152,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 +175,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 +195,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 +217,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;
> > +	int high_thresh;
> > +	int low_thresh;
> > +};
> > +
> >  struct ads1015_data {
> >  	struct regmap *regmap;
> >  	/*
> > @@ -176,6 +237,10 @@ struct ads1015_data {
> >  	struct mutex lock;
> >  	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
> >  
> > +	unsigned int event_channel;
> > +	unsigned int comp_mode;
> > +	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 +250,41 @@ 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,
> > +					 int comp_mode)
> > +{
> > +	WARN_ON(ads1015_event_channel_enabled(data));
> > +
> > +	data->event_channel = chan;
> > +	data->comp_mode = comp_mode;
> > +}
> > +
> > +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,
> >  };
> >  
> > @@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >  	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >  		dr << ADS1015_CFG_DR_SHIFT;
> >  
> > +	if (ads1015_event_channel_enabled(data)) {
> > +		mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
> > +		cfg |= data->thresh_data[chan].comp_queue <<
> > +				ADS1015_CFG_COMP_QUE_SHIFT |
> > +			data->comp_mode <<
> > +				ADS1015_CFG_COMP_MODE_SHIFT;
> > +	}
> > +
> >  	cfg = (old & ~mask) | (cfg & mask);
> >  
> >  	ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> > @@ -356,6 +455,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;
> > @@ -421,8 +526,251 @@ 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 = 0;
> > +
> > +	mutex_lock(&data->lock);
> > +	if (data->event_channel == chan->address) {
> > +		switch (dir) {
> > +		case IIO_EV_DIR_RISING:
> > +			ret = 1;
> > +			break;
> > +		case IIO_EV_DIR_EITHER:
> > +			ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
> > +			break;
> > +		default:
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ads1015_enable_event_config(struct ads1015_data *data,
> > +	const struct iio_chan_spec *chan, int comp_mode)
> > +{
> > +	int low_thresh = data->thresh_data[chan->address].low_thresh;
> > +	int high_thresh = data->thresh_data[chan->address].high_thresh;
> > +	int ret;
> > +	unsigned int val;
> > +
> > +	if (ads1015_event_channel_enabled(data)) {
> > +		if (data->event_channel != chan->address ||
> > +			(data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> > +				comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
> > +			return -EBUSY;
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
> > +		low_thresh = max(-1 << (chan->scan_type.realbits - 1),
> > +				high_thresh - 1);
> > +	}
> > +	ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> > +			low_thresh << chan->scan_type.shift);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> > +			high_thresh << chan->scan_type.shift);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ads1015_set_power_state(data, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ads1015_event_channel_enable(data, chan->address, comp_mode);
> > +
> > +	ret = ads1015_get_adc_result(data, chan->address, &val);
> > +	if (ret) {
> > +		ads1015_event_channel_disable(data, chan->address);
> > +		ads1015_set_power_state(data, false);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int ads1015_disable_event_config(struct ads1015_data *data,
> > +	const struct iio_chan_spec *chan, int comp_mode)
> > +{
> > +	int ret;
> > +
> > +	if (!ads1015_event_channel_enabled(data))
> > +		return 0;
> > +
> > +	if (data->event_channel != chan->address)
> > +		return 0;
> > +
> > +	if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> > +			comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
> > +		return 0;
> > +
> > +	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)
> > +		return ret;
> > +
> > +	ads1015_event_channel_disable(data, chan->address);
> > +
> > +	return ads1015_set_power_state(data, false);
> > +}
> > +
> > +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;
> > +	int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
> > +		ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
> > +
> > +	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 (state)
> > +		ret = ads1015_enable_event_config(data, chan, comp_mode);
> > +	else
> > +		ret = ads1015_disable_event_config(data, chan, comp_mode);
> > +
> > +	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);  
> error checking?  Obviously you can't do anything much with
> the error, but you don't want to muddle through the rest
> with it.
> > +
> > +	if (ads1015_event_channel_enabled(data)) {
> > +		enum iio_event_direction dir;
> > +		u64 code;
> > +
> > +		dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
> > +					IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
> > +		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
> > +					IIO_EV_TYPE_THRESH, dir);
> > +		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);
> >  }
> >  
> > @@ -473,6 +821,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,
> >  };
> >  
> > @@ -480,6 +832,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,
> >  };
> >  
> > @@ -583,6 +939,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)
> > @@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
> >  		break;
> >  	}
> >  
> > +	data->event_channel = ADS1015_CHANNELS;
> > +	/*
> > +	 * Set default lower and upper threshold to min and max value
> > +	 * respectively.
> > +	 */
> > +	for (i = 0; i < ADS1015_CHANNELS; i++) {
> > +		int realbits = indio_dev->channels[i].scan_type.realbits;
> > +
> > +		data->thresh_data[i].low_thresh = -1 << (realbits - 1);
> > +		data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
> > +	}
> > +
> >  	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
> >  	ads1015_get_channels_config(client);
> >  
> > @@ -634,6 +1003,39 @@ 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;
> > +		unsigned int cfg_comp =
> > +			ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> > +			1 << ADS1015_CFG_COMP_LAT_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;  
> 
> --
> 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

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

[Akinobu Mita]

2017-08-20 20:05 GMT+09:00 Jonathan Cameron <jic23@jic23.retrosnub.co.uk>:
> On Sun, 23 Jul 2017 13:01:43 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
>> On Fri, 21 Jul 2017 00:24:27 +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@gmail.com>
>> > Cc: Jonathan Cameron <jic23@kernel.org>
>> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> one really minor point inline.
>>
>> Otherwise I'm happy with this set.  Just waiting to see if
>> Daniel has time to take a look (or anyone else!)
>>
>
> I added a really trivial check on the error as mentioned inline and
> applied to the togreg branch of iio.git and pushed out as testing.
>
> If you could sanity check my change that would be great.

I have sent version 3 of this series on 2017-07-31 that included
a fix for the comparator polarity field setting in config register.

Should I send a incremental patch for the missing part to the togreg
branch?

> Thanks,
>
> Jonathan
>
>> Jonathan
>> > ---
>> >  drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 404 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
>> > index 5568be4..54547a2 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,30 @@ 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) |
>> > +                           BIT(IIO_EV_INFO_ENABLE),
>> > +   }, {
>> > +           .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 +152,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 +175,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 +195,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 +217,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;
>> > +   int high_thresh;
>> > +   int low_thresh;
>> > +};
>> > +
>> >  struct ads1015_data {
>> >     struct regmap *regmap;
>> >     /*
>> > @@ -176,6 +237,10 @@ struct ads1015_data {
>> >     struct mutex lock;
>> >     struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
>> >
>> > +   unsigned int event_channel;
>> > +   unsigned int comp_mode;
>> > +   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 +250,41 @@ 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,
>> > +                                    int comp_mode)
>> > +{
>> > +   WARN_ON(ads1015_event_channel_enabled(data));
>> > +
>> > +   data->event_channel = chan;
>> > +   data->comp_mode = comp_mode;
>> > +}
>> > +
>> > +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,
>> >  };
>> >
>> > @@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>> >     cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
>> >             dr << ADS1015_CFG_DR_SHIFT;
>> >
>> > +   if (ads1015_event_channel_enabled(data)) {
>> > +           mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
>> > +           cfg |= data->thresh_data[chan].comp_queue <<
>> > +                           ADS1015_CFG_COMP_QUE_SHIFT |
>> > +                   data->comp_mode <<
>> > +                           ADS1015_CFG_COMP_MODE_SHIFT;
>> > +   }
>> > +
>> >     cfg = (old & ~mask) | (cfg & mask);
>> >
>> >     ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
>> > @@ -356,6 +455,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;
>> > @@ -421,8 +526,251 @@ 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 = 0;
>> > +
>> > +   mutex_lock(&data->lock);
>> > +   if (data->event_channel == chan->address) {
>> > +           switch (dir) {
>> > +           case IIO_EV_DIR_RISING:
>> > +                   ret = 1;
>> > +                   break;
>> > +           case IIO_EV_DIR_EITHER:
>> > +                   ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
>> > +                   break;
>> > +           default:
>> > +                   ret = -EINVAL;
>> > +                   break;
>> > +           }
>> > +   }
>> > +   mutex_unlock(&data->lock);
>> > +
>> > +   return ret;
>> > +}
>> > +
>> > +static int ads1015_enable_event_config(struct ads1015_data *data,
>> > +   const struct iio_chan_spec *chan, int comp_mode)
>> > +{
>> > +   int low_thresh = data->thresh_data[chan->address].low_thresh;
>> > +   int high_thresh = data->thresh_data[chan->address].high_thresh;
>> > +   int ret;
>> > +   unsigned int val;
>> > +
>> > +   if (ads1015_event_channel_enabled(data)) {
>> > +           if (data->event_channel != chan->address ||
>> > +                   (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
>> > +                           comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
>> > +                   return -EBUSY;
>> > +
>> > +           return 0;
>> > +   }
>> > +
>> > +   if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
>> > +           low_thresh = max(-1 << (chan->scan_type.realbits - 1),
>> > +                           high_thresh - 1);
>> > +   }
>> > +   ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
>> > +                   low_thresh << chan->scan_type.shift);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
>> > +                   high_thresh << chan->scan_type.shift);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   ret = ads1015_set_power_state(data, true);
>> > +   if (ret < 0)
>> > +           return ret;
>> > +
>> > +   ads1015_event_channel_enable(data, chan->address, comp_mode);
>> > +
>> > +   ret = ads1015_get_adc_result(data, chan->address, &val);
>> > +   if (ret) {
>> > +           ads1015_event_channel_disable(data, chan->address);
>> > +           ads1015_set_power_state(data, false);
>> > +   }
>> > +
>> > +   return ret;
>> > +}
>> > +
>> > +static int ads1015_disable_event_config(struct ads1015_data *data,
>> > +   const struct iio_chan_spec *chan, int comp_mode)
>> > +{
>> > +   int ret;
>> > +
>> > +   if (!ads1015_event_channel_enabled(data))
>> > +           return 0;
>> > +
>> > +   if (data->event_channel != chan->address)
>> > +           return 0;
>> > +
>> > +   if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
>> > +                   comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
>> > +           return 0;
>> > +
>> > +   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)
>> > +           return ret;
>> > +
>> > +   ads1015_event_channel_disable(data, chan->address);
>> > +
>> > +   return ads1015_set_power_state(data, false);
>> > +}
>> > +
>> > +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;
>> > +   int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
>> > +           ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
>> > +
>> > +   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 (state)
>> > +           ret = ads1015_enable_event_config(data, chan, comp_mode);
>> > +   else
>> > +           ret = ads1015_disable_event_config(data, chan, comp_mode);
>> > +
>> > +   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);
>> error checking?  Obviously you can't do anything much with
>> the error, but you don't want to muddle through the rest
>> with it.
>> > +
>> > +   if (ads1015_event_channel_enabled(data)) {
>> > +           enum iio_event_direction dir;
>> > +           u64 code;
>> > +
>> > +           dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
>> > +                                   IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
>> > +           code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
>> > +                                   IIO_EV_TYPE_THRESH, dir);
>> > +           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);
>> >  }
>> >
>> > @@ -473,6 +821,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,
>> >  };
>> >
>> > @@ -480,6 +832,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,
>> >  };
>> >
>> > @@ -583,6 +939,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)
>> > @@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
>> >             break;
>> >     }
>> >
>> > +   data->event_channel = ADS1015_CHANNELS;
>> > +   /*
>> > +    * Set default lower and upper threshold to min and max value
>> > +    * respectively.
>> > +    */
>> > +   for (i = 0; i < ADS1015_CHANNELS; i++) {
>> > +           int realbits = indio_dev->channels[i].scan_type.realbits;
>> > +
>> > +           data->thresh_data[i].low_thresh = -1 << (realbits - 1);
>> > +           data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
>> > +   }
>> > +
>> >     /* we need to keep this ABI the same as used by hwmon ADS1015 driver */
>> >     ads1015_get_channels_config(client);
>> >
>> > @@ -634,6 +1003,39 @@ 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;
>> > +           unsigned int cfg_comp =
>> > +                   ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
>> > +                   1 << ADS1015_CFG_COMP_LAT_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;
>>
>> --
>> 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
>

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

[Jonathan Cameron]

On Tue, 22 Aug 2017 19:20:20 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> 2017-08-20 20:05 GMT+09:00 Jonathan Cameron <jic23@jic23.retrosnub.co.uk>:
> > On Sun, 23 Jul 2017 13:01:43 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> >> On Fri, 21 Jul 2017 00:24:27 +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@gmail.com>
> >> > Cc: Jonathan Cameron <jic23@kernel.org>
> >> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> >> one really minor point inline.
> >>
> >> Otherwise I'm happy with this set.  Just waiting to see if
> >> Daniel has time to take a look (or anyone else!)
> >>  
> >
> > I added a really trivial check on the error as mentioned inline and
> > applied to the togreg branch of iio.git and pushed out as testing.
> >
> > If you could sanity check my change that would be great.  
> 
> I have sent version 3 of this series on 2017-07-31 that included
> a fix for the comparator polarity field setting in config register.

Sorry, I missed that for some reason when picking these up.

> 
> Should I send a incremental patch for the missing part to the togreg
> branch?

Please do.

Jonathan

> 
> > Thanks,
> >
> > Jonathan
> >  
> >> Jonathan  
> >> > ---
> >> >  drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 404 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> > index 5568be4..54547a2 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,30 @@ 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) |
> >> > +                           BIT(IIO_EV_INFO_ENABLE),
> >> > +   }, {
> >> > +           .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 +152,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 +175,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 +195,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 +217,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;
> >> > +   int high_thresh;
> >> > +   int low_thresh;
> >> > +};
> >> > +
> >> >  struct ads1015_data {
> >> >     struct regmap *regmap;
> >> >     /*
> >> > @@ -176,6 +237,10 @@ struct ads1015_data {
> >> >     struct mutex lock;
> >> >     struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
> >> >
> >> > +   unsigned int event_channel;
> >> > +   unsigned int comp_mode;
> >> > +   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 +250,41 @@ 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,
> >> > +                                    int comp_mode)
> >> > +{
> >> > +   WARN_ON(ads1015_event_channel_enabled(data));
> >> > +
> >> > +   data->event_channel = chan;
> >> > +   data->comp_mode = comp_mode;
> >> > +}
> >> > +
> >> > +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,
> >> >  };
> >> >
> >> > @@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >> >     cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >> >             dr << ADS1015_CFG_DR_SHIFT;
> >> >
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
> >> > +           cfg |= data->thresh_data[chan].comp_queue <<
> >> > +                           ADS1015_CFG_COMP_QUE_SHIFT |
> >> > +                   data->comp_mode <<
> >> > +                           ADS1015_CFG_COMP_MODE_SHIFT;
> >> > +   }
> >> > +
> >> >     cfg = (old & ~mask) | (cfg & mask);
> >> >
> >> >     ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> >> > @@ -356,6 +455,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;
> >> > @@ -421,8 +526,251 @@ 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 = 0;
> >> > +
> >> > +   mutex_lock(&data->lock);
> >> > +   if (data->event_channel == chan->address) {
> >> > +           switch (dir) {
> >> > +           case IIO_EV_DIR_RISING:
> >> > +                   ret = 1;
> >> > +                   break;
> >> > +           case IIO_EV_DIR_EITHER:
> >> > +                   ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
> >> > +                   break;
> >> > +           default:
> >> > +                   ret = -EINVAL;
> >> > +                   break;
> >> > +           }
> >> > +   }
> >> > +   mutex_unlock(&data->lock);
> >> > +
> >> > +   return ret;
> >> > +}
> >> > +
> >> > +static int ads1015_enable_event_config(struct ads1015_data *data,
> >> > +   const struct iio_chan_spec *chan, int comp_mode)
> >> > +{
> >> > +   int low_thresh = data->thresh_data[chan->address].low_thresh;
> >> > +   int high_thresh = data->thresh_data[chan->address].high_thresh;
> >> > +   int ret;
> >> > +   unsigned int val;
> >> > +
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           if (data->event_channel != chan->address ||
> >> > +                   (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> >> > +                           comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
> >> > +                   return -EBUSY;
> >> > +
> >> > +           return 0;
> >> > +   }
> >> > +
> >> > +   if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
> >> > +           low_thresh = max(-1 << (chan->scan_type.realbits - 1),
> >> > +                           high_thresh - 1);
> >> > +   }
> >> > +   ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> >> > +                   low_thresh << chan->scan_type.shift);
> >> > +   if (ret)
> >> > +           return ret;
> >> > +
> >> > +   ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> >> > +                   high_thresh << chan->scan_type.shift);
> >> > +   if (ret)
> >> > +           return ret;
> >> > +
> >> > +   ret = ads1015_set_power_state(data, true);
> >> > +   if (ret < 0)
> >> > +           return ret;
> >> > +
> >> > +   ads1015_event_channel_enable(data, chan->address, comp_mode);
> >> > +
> >> > +   ret = ads1015_get_adc_result(data, chan->address, &val);
> >> > +   if (ret) {
> >> > +           ads1015_event_channel_disable(data, chan->address);
> >> > +           ads1015_set_power_state(data, false);
> >> > +   }
> >> > +
> >> > +   return ret;
> >> > +}
> >> > +
> >> > +static int ads1015_disable_event_config(struct ads1015_data *data,
> >> > +   const struct iio_chan_spec *chan, int comp_mode)
> >> > +{
> >> > +   int ret;
> >> > +
> >> > +   if (!ads1015_event_channel_enabled(data))
> >> > +           return 0;
> >> > +
> >> > +   if (data->event_channel != chan->address)
> >> > +           return 0;
> >> > +
> >> > +   if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> >> > +                   comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
> >> > +           return 0;
> >> > +
> >> > +   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)
> >> > +           return ret;
> >> > +
> >> > +   ads1015_event_channel_disable(data, chan->address);
> >> > +
> >> > +   return ads1015_set_power_state(data, false);
> >> > +}
> >> > +
> >> > +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;
> >> > +   int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
> >> > +           ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
> >> > +
> >> > +   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 (state)
> >> > +           ret = ads1015_enable_event_config(data, chan, comp_mode);
> >> > +   else
> >> > +           ret = ads1015_disable_event_config(data, chan, comp_mode);
> >> > +
> >> > +   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);  
> >> error checking?  Obviously you can't do anything much with
> >> the error, but you don't want to muddle through the rest
> >> with it.  
> >> > +
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           enum iio_event_direction dir;
> >> > +           u64 code;
> >> > +
> >> > +           dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
> >> > +                                   IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
> >> > +           code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
> >> > +                                   IIO_EV_TYPE_THRESH, dir);
> >> > +           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);
> >> >  }
> >> >
> >> > @@ -473,6 +821,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,
> >> >  };
> >> >
> >> > @@ -480,6 +832,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,
> >> >  };
> >> >
> >> > @@ -583,6 +939,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)
> >> > @@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
> >> >             break;
> >> >     }
> >> >
> >> > +   data->event_channel = ADS1015_CHANNELS;
> >> > +   /*
> >> > +    * Set default lower and upper threshold to min and max value
> >> > +    * respectively.
> >> > +    */
> >> > +   for (i = 0; i < ADS1015_CHANNELS; i++) {
> >> > +           int realbits = indio_dev->channels[i].scan_type.realbits;
> >> > +
> >> > +           data->thresh_data[i].low_thresh = -1 << (realbits - 1);
> >> > +           data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
> >> > +   }
> >> > +
> >> >     /* we need to keep this ABI the same as used by hwmon ADS1015 driver */
> >> >     ads1015_get_channels_config(client);
> >> >
> >> > @@ -634,6 +1003,39 @@ 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;
> >> > +           unsigned int cfg_comp =
> >> > +                   ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> >> > +                   1 << ADS1015_CFG_COMP_LAT_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;  
> >>
> >> --
> >> 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  
> >  
> --
> 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

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

[Jonathan Cameron]

On Tue, 22 Aug 2017 19:20:20 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> 2017-08-20 20:05 GMT+09:00 Jonathan Cameron <jic23@jic23.retrosnub.co.uk>:
> > On Sun, 23 Jul 2017 13:01:43 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> >> On Fri, 21 Jul 2017 00:24:27 +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@gmail.com>
> >> > Cc: Jonathan Cameron <jic23@kernel.org>
> >> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>  
> >> one really minor point inline.
> >>
> >> Otherwise I'm happy with this set.  Just waiting to see if
> >> Daniel has time to take a look (or anyone else!)
> >>  
> >
> > I added a really trivial check on the error as mentioned inline and
> > applied to the togreg branch of iio.git and pushed out as testing.
> >
> > If you could sanity check my change that would be great.  
> 
> I have sent version 3 of this series on 2017-07-31 that included
> a fix for the comparator polarity field setting in config register.

Sorry, I missed that for some reason when picking these 

> 
> Should I send a incremental patch for the missing part to the togreg
> branch?

Please do.

Jonathan

> 
> > Thanks,
> >
> > Jonathan
> >  
> >> Jonathan  
> >> > ---
> >> >  drivers/iio/adc/ti-ads1015.c | 406 ++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 404 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> >> > index 5568be4..54547a2 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,30 @@ 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) |
> >> > +                           BIT(IIO_EV_INFO_ENABLE),
> >> > +   }, {
> >> > +           .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 +152,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 +175,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 +195,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 +217,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;
> >> > +   int high_thresh;
> >> > +   int low_thresh;
> >> > +};
> >> > +
> >> >  struct ads1015_data {
> >> >     struct regmap *regmap;
> >> >     /*
> >> > @@ -176,6 +237,10 @@ struct ads1015_data {
> >> >     struct mutex lock;
> >> >     struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
> >> >
> >> > +   unsigned int event_channel;
> >> > +   unsigned int comp_mode;
> >> > +   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 +250,41 @@ 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,
> >> > +                                    int comp_mode)
> >> > +{
> >> > +   WARN_ON(ads1015_event_channel_enabled(data));
> >> > +
> >> > +   data->event_channel = chan;
> >> > +   data->comp_mode = comp_mode;
> >> > +}
> >> > +
> >> > +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,
> >> >  };
> >> >
> >> > @@ -258,6 +349,14 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
> >> >     cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> >> >             dr << ADS1015_CFG_DR_SHIFT;
> >> >
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
> >> > +           cfg |= data->thresh_data[chan].comp_queue <<
> >> > +                           ADS1015_CFG_COMP_QUE_SHIFT |
> >> > +                   data->comp_mode <<
> >> > +                           ADS1015_CFG_COMP_MODE_SHIFT;
> >> > +   }
> >> > +
> >> >     cfg = (old & ~mask) | (cfg & mask);
> >> >
> >> >     ret = regmap_write(data->regmap, ADS1015_CFG_REG, cfg);
> >> > @@ -356,6 +455,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;
> >> > @@ -421,8 +526,251 @@ 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 = 0;
> >> > +
> >> > +   mutex_lock(&data->lock);
> >> > +   if (data->event_channel == chan->address) {
> >> > +           switch (dir) {
> >> > +           case IIO_EV_DIR_RISING:
> >> > +                   ret = 1;
> >> > +                   break;
> >> > +           case IIO_EV_DIR_EITHER:
> >> > +                   ret = (data->comp_mode == ADS1015_CFG_COMP_MODE_WINDOW);
> >> > +                   break;
> >> > +           default:
> >> > +                   ret = -EINVAL;
> >> > +                   break;
> >> > +           }
> >> > +   }
> >> > +   mutex_unlock(&data->lock);
> >> > +
> >> > +   return ret;
> >> > +}
> >> > +
> >> > +static int ads1015_enable_event_config(struct ads1015_data *data,
> >> > +   const struct iio_chan_spec *chan, int comp_mode)
> >> > +{
> >> > +   int low_thresh = data->thresh_data[chan->address].low_thresh;
> >> > +   int high_thresh = data->thresh_data[chan->address].high_thresh;
> >> > +   int ret;
> >> > +   unsigned int val;
> >> > +
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           if (data->event_channel != chan->address ||
> >> > +                   (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> >> > +                           comp_mode == ADS1015_CFG_COMP_MODE_WINDOW))
> >> > +                   return -EBUSY;
> >> > +
> >> > +           return 0;
> >> > +   }
> >> > +
> >> > +   if (comp_mode == ADS1015_CFG_COMP_MODE_TRAD) {
> >> > +           low_thresh = max(-1 << (chan->scan_type.realbits - 1),
> >> > +                           high_thresh - 1);
> >> > +   }
> >> > +   ret = regmap_write(data->regmap, ADS1015_LO_THRESH_REG,
> >> > +                   low_thresh << chan->scan_type.shift);
> >> > +   if (ret)
> >> > +           return ret;
> >> > +
> >> > +   ret = regmap_write(data->regmap, ADS1015_HI_THRESH_REG,
> >> > +                   high_thresh << chan->scan_type.shift);
> >> > +   if (ret)
> >> > +           return ret;
> >> > +
> >> > +   ret = ads1015_set_power_state(data, true);
> >> > +   if (ret < 0)
> >> > +           return ret;
> >> > +
> >> > +   ads1015_event_channel_enable(data, chan->address, comp_mode);
> >> > +
> >> > +   ret = ads1015_get_adc_result(data, chan->address, &val);
> >> > +   if (ret) {
> >> > +           ads1015_event_channel_disable(data, chan->address);
> >> > +           ads1015_set_power_state(data, false);
> >> > +   }
> >> > +
> >> > +   return ret;
> >> > +}
> >> > +
> >> > +static int ads1015_disable_event_config(struct ads1015_data *data,
> >> > +   const struct iio_chan_spec *chan, int comp_mode)
> >> > +{
> >> > +   int ret;
> >> > +
> >> > +   if (!ads1015_event_channel_enabled(data))
> >> > +           return 0;
> >> > +
> >> > +   if (data->event_channel != chan->address)
> >> > +           return 0;
> >> > +
> >> > +   if (data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD &&
> >> > +                   comp_mode == ADS1015_CFG_COMP_MODE_WINDOW)
> >> > +           return 0;
> >> > +
> >> > +   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)
> >> > +           return ret;
> >> > +
> >> > +   ads1015_event_channel_disable(data, chan->address);
> >> > +
> >> > +   return ads1015_set_power_state(data, false);
> >> > +}
> >> > +
> >> > +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;
> >> > +   int comp_mode = (dir == IIO_EV_DIR_EITHER) ?
> >> > +           ADS1015_CFG_COMP_MODE_WINDOW : ADS1015_CFG_COMP_MODE_TRAD;
> >> > +
> >> > +   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 (state)
> >> > +           ret = ads1015_enable_event_config(data, chan, comp_mode);
> >> > +   else
> >> > +           ret = ads1015_disable_event_config(data, chan, comp_mode);
> >> > +
> >> > +   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);  
> >> error checking?  Obviously you can't do anything much with
> >> the error, but you don't want to muddle through the rest
> >> with it.  
> >> > +
> >> > +   if (ads1015_event_channel_enabled(data)) {
> >> > +           enum iio_event_direction dir;
> >> > +           u64 code;
> >> > +
> >> > +           dir = data->comp_mode == ADS1015_CFG_COMP_MODE_TRAD ?
> >> > +                                   IIO_EV_DIR_RISING : IIO_EV_DIR_EITHER;
> >> > +           code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, data->event_channel,
> >> > +                                   IIO_EV_TYPE_THRESH, dir);
> >> > +           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);
> >> >  }
> >> >
> >> > @@ -473,6 +821,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,
> >> >  };
> >> >
> >> > @@ -480,6 +832,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,
> >> >  };
> >> >
> >> > @@ -583,6 +939,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)
> >> > @@ -617,6 +974,18 @@ static int ads1015_probe(struct i2c_client *client,
> >> >             break;
> >> >     }
> >> >
> >> > +   data->event_channel = ADS1015_CHANNELS;
> >> > +   /*
> >> > +    * Set default lower and upper threshold to min and max value
> >> > +    * respectively.
> >> > +    */
> >> > +   for (i = 0; i < ADS1015_CHANNELS; i++) {
> >> > +           int realbits = indio_dev->channels[i].scan_type.realbits;
> >> > +
> >> > +           data->thresh_data[i].low_thresh = -1 << (realbits - 1);
> >> > +           data->thresh_data[i].high_thresh = (1 << (realbits - 1)) - 1;
> >> > +   }
> >> > +
> >> >     /* we need to keep this ABI the same as used by hwmon ADS1015 driver */
> >> >     ads1015_get_channels_config(client);
> >> >
> >> > @@ -634,6 +1003,39 @@ 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;
> >> > +           unsigned int cfg_comp =
> >> > +                   ADS1015_CFG_COMP_DISABLE << ADS1015_CFG_COMP_QUE_SHIFT |
> >> > +                   1 << ADS1015_CFG_COMP_LAT_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;  
> >>
> >> --
> >> 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  
> >  
> --
> 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