* [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample
[not found] <eae2ac11-867f-d471-7430-963906746d29@gmail.com>
@ 2017-02-14 21:58 ` Heiner Kallweit
2017-02-14 23:03 ` Martin Blumenstingl
0 siblings, 1 reply; 3+ messages in thread
From: Heiner Kallweit @ 2017-02-14 21:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Martin Blumenstingl, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
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>
---
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;
- 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, ®val);
-
- 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, ®val);
+ 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;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample
2017-02-14 21:58 ` [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample Heiner Kallweit
@ 2017-02-14 23:03 ` Martin Blumenstingl
2017-02-15 19:30 ` Heiner Kallweit
0 siblings, 1 reply; 3+ messages in thread
From: Martin Blumenstingl @ 2017-02-14 23:03 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
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, ®val);
> -
> - 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, ®val);
> + 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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample
2017-02-14 23:03 ` Martin Blumenstingl
@ 2017-02-15 19:30 ` Heiner Kallweit
0 siblings, 0 replies; 3+ messages in thread
From: Heiner Kallweit @ 2017-02-15 19:30 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio
Am 15.02.2017 um 00:03 schrieb Martin Blumenstingl:
> 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, ®val);
>> -
>> - 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, ®val);
>> + 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
>
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.
>> + *val = fifo_val;
>>
>> return 0;
>> }
>> --
>> 2.11.1
>>
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-15 19:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <eae2ac11-867f-d471-7430-963906746d29@gmail.com>
2017-02-14 21:58 ` [PATCH 2/2] iio: adc: meson-saradc: improve meson_sar_adc_read_raw_sample Heiner Kallweit
2017-02-14 23:03 ` Martin Blumenstingl
2017-02-15 19:30 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).