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

[Jonathan Cameron]

On Fri, 11 Nov 2022 12:26:55 +0100
Mitja Spes <mitja@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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;
>  	}