| Message ID | ed9716c7-5e15-461a-985c-eea93257928c@rwthex-w2-a.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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);
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(-)