Re: [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

On Tue, Feb 14, 2017 at 10:58 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 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 <hkallweit1@gmail.com>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

> ---
>  drivers/iio/adc/meson_saradc.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index dbd56bcc..d39711c0 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -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!
> -       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);
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

> +       *val = fifo_val;
>
>         return 0;
>  }
> --
> 2.11.1
>
>