linux-kernel - Re: [PATCH 06/16] iio: adc: at91-sama5d2_adc: add 64 and 256 oversampling ratio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 11 Jun 2022 18:47:31 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Claudiu Beznea <claudiu.beznea@...rochip.com>
Cc:     <eugen.hristev@...rochip.com>, <lars@...afoo.de>,
        <nicolas.ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
        <robh+dt@...nel.org>, <krzk+dt@...nel.org>,
        <ludovic.desroches@...el.com>, <linux-iio@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 06/16] iio: adc: at91-sama5d2_adc: add 64 and 256
 oversampling ratio

On Thu, 9 Jun 2022 11:32:03 +0300
Claudiu Beznea <claudiu.beznea@...rochip.com> wrote:

> Add 64 and 256 oversampling ratio support. It is necessary for temperature
> sensor.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7321a4b519af..b52f1020feaf 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -142,6 +142,8 @@ struct at91_adc_reg_layout {
>  #define AT91_SAMA5D2_EMR_OSR_1SAMPLES		0
>  #define AT91_SAMA5D2_EMR_OSR_4SAMPLES		1
>  #define AT91_SAMA5D2_EMR_OSR_16SAMPLES		2
> +#define AT91_SAMA5D2_EMR_OSR_64SAMPLES		3
> +#define AT91_SAMA5D2_EMR_OSR_256SAMPLES		4
>  
>  /* Extended Mode Register - Averaging on single trigger event */
>  #define AT91_SAMA5D2_EMR_ASTE(V)		((V) << 20)
> @@ -308,6 +310,8 @@ static const struct at91_adc_reg_layout sama7g5_layout = {
>  #define AT91_OSR_1SAMPLES		1
>  #define AT91_OSR_4SAMPLES		4
>  #define AT91_OSR_16SAMPLES		16
> +#define AT91_OSR_64SAMPLES		64
> +#define AT91_OSR_256SAMPLES		256

These defines seems a bit silly.  Better to use the values inline than
to have these.

>  
>  #define AT91_SAMA5D2_CHAN_SINGLE(index, num, addr)			\
>  	{								\
> @@ -640,7 +644,9 @@ static const struct at91_adc_platform sama7g5_platform = {
>  	.osr_mask = GENMASK(18, 16),
>  	.osr_vals = BIT(AT91_SAMA5D2_EMR_OSR_1SAMPLES) |
>  		    BIT(AT91_SAMA5D2_EMR_OSR_4SAMPLES) |
> -		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES),
> +		    BIT(AT91_SAMA5D2_EMR_OSR_16SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES) |
> +		    BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES),
>  	.chan_realbits = 16,
>  };
>  
> @@ -774,6 +780,18 @@ static int at91_adc_config_emr(struct at91_adc_state *st,
>  		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_16SAMPLES,
>  					    osr_mask);
>  		break;
> +	case AT91_OSR_64SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_64SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_64SAMPLES,
> +					    osr_mask);
> +		break;
> +	case AT91_OSR_256SAMPLES:
> +		if (!(osr_vals & BIT(AT91_SAMA5D2_EMR_OSR_256SAMPLES)))
> +			return -EINVAL;
> +		emr |= AT91_SAMA5D2_EMR_OSR(AT91_SAMA5D2_EMR_OSR_256SAMPLES,
> +					    osr_mask);
> +		break;
>  	}
>  
>  	at91_adc_writel(st, EMR, emr);
> @@ -791,6 +809,10 @@ static int at91_adc_adjust_val_osr(struct at91_adc_state *st, int *val)
>  		nbits = 13;
>  	else if (st->oversampling_ratio == AT91_OSR_16SAMPLES)
>  		nbits = 14;
> +	else if (st->oversampling_ratio == AT91_OSR_64SAMPLES)
> +		nbits = 15;
> +	else if (st->oversampling_ratio == AT91_OSR_256SAMPLES)
> +		nbits = 16;
>  
>  	/*
>  	 * We have nbits of real data and channel is registered as
> @@ -1679,7 +1701,8 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if ((val != AT91_OSR_1SAMPLES) && (val != AT91_OSR_4SAMPLES) &&
> -		    (val != AT91_OSR_16SAMPLES))
> +		    (val != AT91_OSR_16SAMPLES) && (val != AT91_OSR_64SAMPLES) &&
> +		    (val != AT91_OSR_256SAMPLES))
Dropping this partial validity check and moving into a default in the switch statement
in config_emr() would be nice cleanup (I also replied to earlier patch based on what
is visible here).

>  			return -EINVAL;
>  		/* if no change, optimize out */
>  		mutex_lock(&st->lock);
> @@ -1897,7 +1920,9 @@ static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
>  static IIO_CONST_ATTR(oversampling_ratio_available,
>  		      __stringify(AT91_OSR_1SAMPLES) " "
>  		      __stringify(AT91_OSR_4SAMPLES) " "
> -		      __stringify(AT91_OSR_16SAMPLES));
> +		      __stringify(AT91_OSR_16SAMPLES) " "
> +		      __stringify(AT91_OSR_64SAMPLES) " "
> +		      __stringify(AT91_OSR_256SAMPLES));

At somepoint it would be good to move this over to the read_avail() callback rather than
hand rolling it.  We are slowly working through doing this for all the IIO drivers
but it will take a long time yet!

>  
>  static struct attribute *at91_adc_attributes[] = {
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,