iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample

PATCH Superseded Heiner Kallweit 2 participants 3 messages Feb 14, 2017 – Feb 15, 2017 1 message not in the archive

Subsystems (2)

Arm/Amlogic Meson SoC support IIO

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Heiner Kallweit

author

2017-02-14 21:59 UTC #1 lore raw

PATCH 2/2 iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample

After sampling there should always be only one value in the FIFO. This also applies to averaging mode as the averaging is done chip-internally. So we don't have to loop and let the driver complain if there's not exactly one value in the FIFO.

If the value belongs to a different channel then don't silently swallow the value but complain.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/iio/adc/meson_saradc.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

drivers/iio/adc/meson_saradc.c +17 ~~-19~~

index dbd56bcc..d39711c0 100644
@@ -278,33 +278,31 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
 					 int *val)
 {
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-	int regval, fifo_chan, fifo_val, sum = 0, count = 0;
+	int regval, fifo_chan, fifo_val, count;
 
 	if(!wait_for_completion_timeout(&priv->done,
 				msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
 		return -ETIMEDOUT;
 
-	while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
-	       count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
-		regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
-
-		fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK,
-				      regval);
-		if (fifo_chan != chan->channel)
-			continue;
-
-		fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK,
-				     regval);
-		fifo_val &= (BIT(priv->data->resolution) - 1);
-
-		sum += fifo_val;
-		count++;
+	count = meson_sar_adc_get_fifo_count(indio_dev);
+	if (count != 1) {
+		dev_err(&indio_dev->dev,
+			"ADC FIFO has %d elements instead of one\n", count);
+		return -EINVAL;
 	}
 
-	if (!count)
-		return -ENOENT;
+	regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
+	fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval);
+	if (fifo_chan != chan->channel) {
+		dev_err(&indio_dev->dev,
+			"ADC FIFO entry belongs to channel %d instead of %d\n",
+			fifo_chan, chan->channel);
+		return -EINVAL;
+	}
 
-	*val = sum / count;
+	fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK, regval);
+	fifo_val &= GENMASK(priv->data->resolution - 1, 0);
+	*val = fifo_val;
 
 	return 0;
 }

Signature · 1 line

2.11.1

Martin Blumenstingl

reviewer Acked-by on v1 2/2

2017-02-14 23:03 UTC #2 lore raw

Quoted 9 lines · Heiner Kallweit

After sampling there should always be only one value in the FIFO. This also applies to averaging mode as the averaging is done chip-internally. So we don't have to loop and let the driver complain if there's not exactly one value in the FIFO.
If the value belongs to a different channel then don't silently swallow the value but complain.
Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Martin Blumenstingl <[email protected]>

Quoted 9 lines

---
 drivers/iio/adc/meson_saradc.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

drivers/iio/adc/meson_saradc.c +0 -0

index dbd56bcc..d39711c0 100644

drivers/iio/adc/meson_saradc.c static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, · line 286

Quoted 10 lines

drivers/iio/adc/meson_saradc.c +1 -1

@@ -278,33 +278,31 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
                                         int *val)
 {
        struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-       int regval, fifo_chan, fifo_val, sum = 0, count = 0;
+       int regval, fifo_chan, fifo_val, count;
 
        if(!wait_for_completion_timeout(&priv->done,
                                msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
                return -ETIMEDOUT;

I thought I had changed the following loop when I introduced meson_sar_adc_clear_fifo() but it seems that I didn't do it - so thanks for cleaning this up!

Quoted 26 lines

drivers/iio/adc/meson_saradc.c +7 ~~-17~~

-       while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
-              count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
-               regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
-
-               fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK,
-                                     regval);
-               if (fifo_chan != chan->channel)
-                       continue;
-
-               fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK,
-                                    regval);
-               fifo_val &= (BIT(priv->data->resolution) - 1);
-
-               sum += fifo_val;
-               count++;
+       count = meson_sar_adc_get_fifo_count(indio_dev);
+       if (count != 1) {
+               dev_err(&indio_dev->dev,
+                       "ADC FIFO has %d elements instead of one\n", count);
+               return -EINVAL;
        }
 
-       if (!count)
-               return -ENOENT;
+       regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
+       fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval);

drivers/iio/adc/meson_saradc.c static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, · line 304

Quoted 10 lines

drivers/iio/adc/meson_saradc.c +8 -1

+       if (fifo_chan != chan->channel) {
+               dev_err(&indio_dev->dev,
+                       "ADC FIFO entry belongs to channel %d instead of %d\n",
+                       fifo_chan, chan->channel);
+               return -EINVAL;
+       }
 
-       *val = sum / count;
+       fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK, regval);
+       fifo_val &= GENMASK(priv->data->resolution - 1, 0);

my code used "BIT(priv->data->resolution) - 1" instead of GENMASK. it shouldn't make a difference so I don't see an issue with that change

Quoted 8 lines

drivers/iio/adc/meson_saradc.c +1 -1
+       *val = fifo_val;
 
        return 0;
 }
--
2.11.1

Heiner Kallweit

author

2017-02-15 19:30 UTC #3 lore raw

Am 15.02.2017 um 00:03 schrieb Martin Blumenstingl:

Quoted 68 lines · Heiner Kallweit

Quoted 9 lines

After sampling there should always be only one value in the FIFO. This also applies to averaging mode as the averaging is done chip-internally. So we don't have to loop and let the driver complain if there's not exactly one value in the FIFO.
If the value belongs to a different channel then don't silently swallow the value but complain.
Signed-off-by Heiner Kallweit <[email protected]>

Acked-by Martin Blumenstingl <[email protected]>

Quoted 9 lines

---
 drivers/iio/adc/meson_saradc.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

drivers/iio/adc/meson_saradc.c +0 -0

index dbd56bcc..d39711c0 100644

drivers/iio/adc/meson_saradc.c static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, · line 286

Quoted 10 lines

drivers/iio/adc/meson_saradc.c +1 -1

@@ -278,33 +278,31 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev,
                                         int *val)
 {
        struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-       int regval, fifo_chan, fifo_val, sum = 0, count = 0;
+       int regval, fifo_chan, fifo_val, count;
 
        if(!wait_for_completion_timeout(&priv->done,
                                msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT)))
                return -ETIMEDOUT;

I thought I had changed the following loop when I introduced meson_sar_adc_clear_fifo() but it seems that I didn't do it - so thanks for cleaning this up!

Quoted 33 lines

drivers/iio/adc/meson_saradc.c +13 ~~-17~~

-       while (meson_sar_adc_get_fifo_count(indio_dev) > 0 &&
-              count < MESON_SAR_ADC_MAX_FIFO_SIZE) {
-               regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
-
-               fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK,
-                                     regval);
-               if (fifo_chan != chan->channel)
-                       continue;
-
-               fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK,
-                                    regval);
-               fifo_val &= (BIT(priv->data->resolution) - 1);
-
-               sum += fifo_val;
-               count++;
+       count = meson_sar_adc_get_fifo_count(indio_dev);
+       if (count != 1) {
+               dev_err(&indio_dev->dev,
+                       "ADC FIFO has %d elements instead of one\n", count);
+               return -EINVAL;
        }
 
-       if (!count)
-               return -ENOENT;
+       regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, &regval);
+       fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval);
+       if (fifo_chan != chan->channel) {
+               dev_err(&indio_dev->dev,
+                       "ADC FIFO entry belongs to channel %d instead of %d\n",
+                       fifo_chan, chan->channel);
+               return -EINVAL;
+       }

Quoted 6 lines

drivers/iio/adc/meson_saradc.c static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, · line 304
Quoted 3 lines
drivers/iio/adc/meson_saradc.c +2 -1
-       *val = sum / count;
+       fifo_val = FIELD_GET(MESON_SAR_ADC_FIFO_RD_SAMPLE_VALUE_MASK, regval);
+       fifo_val &= GENMASK(priv->data->resolution - 1, 0);
my code used "BIT(priv->data->resolution) - 1" instead of GENMASK. it shouldn't make a difference so I don't see an issue with that change

Functionally it's the same. I just think that using GENMASK is more intuitive here as the statement is about applying a bitmask to a value.

Quoted 9 lines

Quoted 8 lines
drivers/iio/adc/meson_saradc.c +1 -1
+       *val = fifo_val;
 
        return 0;
 }
--
2.11.1