linux-kernel - Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

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]

Message-ID: <20221112173222.0ca56017@jic23-huawei>
Date:   Sat, 12 Nov 2022 17:32:22 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Mitja Spes <mitja@...av.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Angelo Compagnucci <angelo.compagnucci@...il.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

On Fri, 11 Nov 2022 12:26:55 +0100
Mitja Spes <mitja@...av.com> wrote:

> Allows setting gain separately from scale.

How are the separate?  We normally only use hardwaregain if
changing it has no input on the scale that we need to apply in userspace
to raw channels.  This normally happens for two reasons
1) There is a micro controller on the sensor that is doing a bunch of
   maths so whilst changing the PGA value changes the range measurable it
   doesn't affect the representation when we read from the device.
2) The hardware gain is controlling say the sensitivity of a light sensor
   in a time of flight device - it affects if we can get a measurement, but
   not the measurement itself.

Any of that true here?

If not, we shouldn't be adding a hardwaregain attribute - which is why
almost no ADCs have them...

Jonathan

> 
> Signed-off-by: Mitja Spes <mitja@...av.com>
> ---
>  drivers/iio/adc/mcp3422.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index cfb629b964af..eef35fb2fc22 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -58,7 +58,8 @@
>  		.channel = _index, \
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
>  				| BIT(IIO_CHAN_INFO_SCALE) \
> -				| BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +				| BIT(IIO_CHAN_INFO_SAMP_FREQ) \
> +				| BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
>  	}
>  
>  static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
> @@ -184,6 +185,10 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  		*val1 = mcp3422_sample_rates[sample_rate];
>  		return IIO_VAL_INT;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		*val1 = (1 << pga);
> +		return IIO_VAL_INT;
> +
>  	default:
>  		break;
>  	}
> @@ -245,6 +250,29 @@ static int mcp3422_write_raw(struct iio_dev *iio,
>  		adc->ch_config[req_channel] = config;
>  		return 0;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		switch (val1) {
> +		case 1:
> +			temp = MCP3422_PGA_1;
> +			break;
> +		case 2:
> +			temp = MCP3422_PGA_2;
> +			break;
> +		case 4:
> +			temp = MCP3422_PGA_4;
> +			break;
> +		case 8:
> +			temp = MCP3422_PGA_8;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		config &= ~MCP3422_PGA_MASK;
> +		config |= MCP3422_PGA_VALUE(temp);
> +		adc->ch_config[req_channel] = config;
> +		return 0;
> +
>  	default:
>  		break;
>  	}
> @@ -260,6 +288,8 @@ static int mcp3422_write_raw_get_fmt(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}