[v1,3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
diff mbox

Message ID ed9716c7-5e15-461a-985c-eea93257928c@rwthex-w2-a.rwth-ad.de
State New
Headers show

Commit Message

Stefan Brüns Dec. 8, 2017, 5:41 p.m. UTC
Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.

The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

 drivers/iio/adc/ina2xx-adc.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Jonathan Cameron Dec. 10, 2017, 5:27 p.m. UTC | #1
On Fri, 8 Dec 2017 18:41:48 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Although the datasheet states the CNVR flag is cleared by reading the
> BUS_VOLTAGE register, it is actually cleared by reading any of the
> voltage/current/power registers.
> 
> The behaviour has been confirmed by TI support:
> http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

I haven't checked the code thoroughly so there may well be something stopping it
but have you checked the case where the only channel enabled is the timestamp?

Obviously it makes little sense, but IIRC there is nothing in the core preventing
that happening.

Jonathan
> ---
> 
>  drivers/iio/adc/ina2xx-adc.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 8c8120406f52..b027d485398b 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	int bit, ret, i = 0;
>  	s64 time_a, time_b;
>  	unsigned int alert;
> -	int cnvr_need_clear = 0;
>  
>  	time_a = iio_get_time_ns(indio_dev);
>  
> @@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  				ret = regmap_read(chip->regmap,
>  						  INA2XX_BUS_VOLTAGE, &alert);
>  				alert &= INA219_CNVR;
> -				cnvr_need_clear = alert;
>  			}
>  
>  			if (ret < 0)
> @@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  			return ret;
>  
>  		data[i++] = val;
> -
> -		if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
> -			cnvr_need_clear = 0;
> -	}
> -
> -	/* Dummy read on INA219 power register to clear CNVR flag */
> -	if (cnvr_need_clear && chip->config->chip_id == ina219) {
> -		unsigned int val;
> -
> -		ret = regmap_read(chip->regmap, INA2XX_POWER, &val);
> -		if (ret < 0)
> -			return ret;
>  	}
>  
>  	time_b = iio_get_time_ns(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
Stefan Brüns Dec. 10, 2017, 8:53 p.m. UTC | #2
On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:48 +0100
> 
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > Although the datasheet states the CNVR flag is cleared by reading the
> > BUS_VOLTAGE register, it is actually cleared by reading any of the
> > voltage/current/power registers.
> > 
> > The behaviour has been confirmed by TI support:
> > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053
> > /2378282
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 
> I haven't checked the code thoroughly so there may well be something
> stopping it but have you checked the case where the only channel enabled is
> the timestamp?
> 
> Obviously it makes little sense, but IIRC there is nothing in the core
> preventing that happening.

The timestamp is completely unrelated to the status register, so I fail to 
understand your question. Can you please clarify?

This only removes a redundant read.

All channel combinations (w/ and w/o timestamp) work, but combinations not 
including the power register use less bus time now.

Kind regards,

Stefan
Jonathan Cameron Dec. 12, 2017, 8:15 p.m. UTC | #3
On Sun, 10 Dec 2017 21:53:42 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> > On Fri, 8 Dec 2017 18:41:48 +0100
> > 
> > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > Although the datasheet states the CNVR flag is cleared by reading the
> > > BUS_VOLTAGE register, it is actually cleared by reading any of the
> > > voltage/current/power registers.
> > > 
> > > The behaviour has been confirmed by TI support:
> > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053
> > > /2378282
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>  
> > 
> > I haven't checked the code thoroughly so there may well be something
> > stopping it but have you checked the case where the only channel enabled is
> > the timestamp?
> > 
> > Obviously it makes little sense, but IIRC there is nothing in the core
> > preventing that happening.  
> 
> The timestamp is completely unrelated to the status register, so I fail to 
> understand your question. Can you please clarify?

If you only have a timestamp, the trigger will still fire (I think)
but you'll do no reading at all from the device.  If configured in this,
admittedly odd, way you should just get a stream of timestamps with no
data.

> 
> This only removes a redundant read.
The question is whether it is redundant if we have no non timestamp
registers enabled.

I'll be honest, whilst I can't immediately spot any protection against
this in the core (and it definitely used to be possible), I'm not totally
sure it now is and don't have a system to hand to test against.

We had some debate a long time back on whether it made sense to have
only timestamps and I think we concluded it did as you might in theory
only care about the timing and not the data in some obscure cases.

Jonathan
 
> 
> All channel combinations (w/ and w/o timestamp) work, but combinations not 
> including the power register use less bus time now.
> 
> Kind regards,
> 
> Stefan
> 

--
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
Stefan Brüns Dec. 12, 2017, 11:48 p.m. UTC | #4
On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote:
> On Sun, 10 Dec 2017 21:53:42 +0100
> 
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> > > On Fri, 8 Dec 2017 18:41:48 +0100
> > > 
> > > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > > > Although the datasheet states the CNVR flag is cleared by reading the
> > > > BUS_VOLTAGE register, it is actually cleared by reading any of the
> > > > voltage/current/power registers.
> > > > 
> > > > The behaviour has been confirmed by TI support:
> > > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/64
> > > > 7053
> > > > /2378282
> > > > 
> > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > 
> > > I haven't checked the code thoroughly so there may well be something
> > > stopping it but have you checked the case where the only channel enabled
> > > is
> > > the timestamp?
> > > 
> > > Obviously it makes little sense, but IIRC there is nothing in the core
> > > preventing that happening.
> > 
> > The timestamp is completely unrelated to the status register, so I fail to
> > understand your question. Can you please clarify?
> 
> If you only have a timestamp, the trigger will still fire (I think)
> but you'll do no reading at all from the device.  If configured in this,
> admittedly odd, way you should just get a stream of timestamps with no
> data.

If there are reads depends on the mode - if running asynchronously, it will 
just stream out 64 bits of timestamp each interval. In synchronous mode, the 
driver will read the status register (low bits of bus voltage register for 
INA219, msk register for INA226), which implicitly clears the CNVR flag.
 
> > This only removes a redundant read.
> 
> The question is whether it is redundant if we have no non timestamp
> registers enabled.

According to the documentation, INA219 and 226 had to be treated differently. 
As it turned out, both actually behave the same way regarding the CNVR flag, 
so we just poll the status register, which for both devices clears the flag.

Regards,

Stefan
Jonathan Cameron Dec. 17, 2017, 10:09 a.m. UTC | #5
On Wed, 13 Dec 2017 00:48:40 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote:
> > On Sun, 10 Dec 2017 21:53:42 +0100
> > 
> > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:  
> > > > On Fri, 8 Dec 2017 18:41:48 +0100
> > > > 
> > > > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > > > Although the datasheet states the CNVR flag is cleared by reading the
> > > > > BUS_VOLTAGE register, it is actually cleared by reading any of the
> > > > > voltage/current/power registers.
> > > > > 
> > > > > The behaviour has been confirmed by TI support:
> > > > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/64
> > > > > 7053
> > > > > /2378282
> > > > > 
> > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>  
> > > > 
> > > > I haven't checked the code thoroughly so there may well be something
> > > > stopping it but have you checked the case where the only channel enabled
> > > > is
> > > > the timestamp?
> > > > 
> > > > Obviously it makes little sense, but IIRC there is nothing in the core
> > > > preventing that happening.  
> > > 
> > > The timestamp is completely unrelated to the status register, so I fail to
> > > understand your question. Can you please clarify?  
> > 
> > If you only have a timestamp, the trigger will still fire (I think)
> > but you'll do no reading at all from the device.  If configured in this,
> > admittedly odd, way you should just get a stream of timestamps with no
> > data.  
> 
> If there are reads depends on the mode - if running asynchronously, it will 
> just stream out 64 bits of timestamp each interval. In synchronous mode, the 
> driver will read the status register (low bits of bus voltage register for 
> INA219, msk register for INA226), which implicitly clears the CNVR flag.
>  
> > > This only removes a redundant read.  
> > 
> > The question is whether it is redundant if we have no non timestamp
> > registers enabled.  
> 
> According to the documentation, INA219 and 226 had to be treated differently. 
> As it turned out, both actually behave the same way regarding the CNVR flag, 
> so we just poll the status register, which for both devices clears the flag.
Ah, fine then. I thought we were talking about having to read a channel
not just the register we are anyway polling for status.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> 
> Regards,
> 
> Stefan
> 
> 

--
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
diff mbox

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@  static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	int bit, ret, i = 0;
 	s64 time_a, time_b;
 	unsigned int alert;
-	int cnvr_need_clear = 0;
 
 	time_a = iio_get_time_ns(indio_dev);
 
@@ -730,7 +729,6 @@  static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 				ret = regmap_read(chip->regmap,
 						  INA2XX_BUS_VOLTAGE, &alert);
 				alert &= INA219_CNVR;
-				cnvr_need_clear = alert;
 			}
 
 			if (ret < 0)
@@ -752,18 +750,6 @@  static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 			return ret;
 
 		data[i++] = val;
-
-		if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
-			cnvr_need_clear = 0;
-	}
-
-	/* Dummy read on INA219 power register to clear CNVR flag */
-	if (cnvr_need_clear && chip->config->chip_id == ina219) {
-		unsigned int val;
-
-		ret = regmap_read(chip->regmap, INA2XX_POWER, &val);
-		if (ret < 0)
-			return ret;
 	}
 
 	time_b = iio_get_time_ns(indio_dev);