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

  Allows setting gain separately from scale.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/adc/mcp3422.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)
  

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

Hi Jonathan,

On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 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?
No. I see I misunderstood the hardwaregain attribute. For me this is a user
friendly way of setting the gain and subsequently scale.

What I don't understand is why set scale at all? It's a result of sampling
rate and gain settings. Using it as a setting, for which input value range also
changes depending on another attribute is quite cumbersome.
To use a sensor the program has to do this:
1. set the sampling rate
2. read available scales for this sampling rate
3. set the scale according to desired gain
or know the scales for each sampling rate in advance...which makes available
scales attribute quite useless.

Kind regards,
Mitja
  

On Sat, 12 Nov 2022 22:19:07 +0100
Mitja Špes <mitja@lxnav.com> wrote:

> Hi Jonathan,
> 
> On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > 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?  
> No. I see I misunderstood the hardwaregain attribute. For me this is a user
> friendly way of setting the gain and subsequently scale.
> 
> What I don't understand is why set scale at all? 

Key issue here is the ABI is not designed against one part. It is designed against
many. Also it is inherently biased in favour of the parts that were around when
we originally created it - I'll note that at that time the trade off of resolution
against sampling period (oversampling or cutting off the measurement) was not common.

That means userspace code has been written that assumes a certain set of attributes.
The cost of starting to use new attributes is high because no userspace code knows
about them.  Hence we put a lot of effort into avoiding doing so.  I agree that your
argument makes sense for your particular device - it doesn't for many other ADCs.

Many ADCs (I'd go as far as to say most though I could be wrong on that) do not
couple scale and sampling frequency at all. 

> It's a result of sampling
> rate and gain settings. Using it as a setting, for which input value range also
> changes depending on another attribute is quite cumbersome.
> To use a sensor the program has to do this:
> 1. set the sampling rate
> 2. read available scales for this sampling rate
> 3. set the scale according to desired gain
> or know the scales for each sampling rate in advance...which makes available
> scales attribute quite useless.

For this last point, I think trying to match against previous scale makes a lot of
sense as there is considerable overlap available here between the different rates.
I think that would be an improvement.  Another improvement would be to at least
expose the current resolution - that can be done by providing and _available
for the raw reading.  Not an idea way to undestand what is going on but it would
make more data available to userspace.  (_raw_available(max) * scale would give
the range of the device and allow an adjustment of the scale to achieve what the
user wants).

ABI design is hard.  We don't always get it right and often there is no right answer.
Reality is that sometimes userspace code needs to search the space if it is trying
to get as close as possible to a particular set of constraints.  However lets assume
in most cases the code has limits of:

1) A minimum data rate with which it is happy (controls
the sampling frequency - higher is normally fine, but wastes power etc).
To get best possible power usage (and in case of this device resolution) it will pick
the lowest sampling frequency to meet this constraint.

2) A range of values it is interested in (here related to the PGA settings but not
   the sampling frequency).  Adding _raw_avail would help it to have visibility of
   what the range is.

3) A resolution it cares about getting data at (scale)

We have to present 'scale' because that's necessary to allow userspace to calculate the
actual voltage.  That adds a constraint to the ABI design.  We also don't want to provide
more overlapping controls than absolutely necessary as that makes it hard for userspace
to know which one to use.

So upshot is that userspace has to search to find a value that works for it.

In this particular case the set of ranges at all sampling frequencies are the same.
So if we assume a constraint on how often data is wanted is more important than the
resolution (have to pick one or the other to be more important) then we set that
first to the minimum value to meet the requirement.  Then scale is tweaked to set
the PGA to match the range desired.  Sure, not super clean but consistent with the
ABI as it stands (and we can't change that other than very very carefully).

As a fun side note, if the device (or driver) had justified the lower resolutions the other way
(so the zeros ended up in least significant bits) a common solution would have been
to just present that to userspace as is, thus the scale would have been decoupled from
the sampling frequency.  Not this is what oversampling devices normally do...
We obviously could fake that now, but the issue would then be that it was a major
driver ABI change. So we can't.

Jonathan






> 
> Kind regards,
> Mitja
  

Thank you for the explanations.

Kind regards,
Mitja

On Sun, Nov 13, 2022 at 1:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 12 Nov 2022 22:19:07 +0100
> Mitja Špes <mitja@lxnav.com> wrote:
>
> > Hi Jonathan,
> >
> > On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > 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?
> > No. I see I misunderstood the hardwaregain attribute. For me this is a user
> > friendly way of setting the gain and subsequently scale.
> >
> > What I don't understand is why set scale at all?
>
> Key issue here is the ABI is not designed against one part. It is designed against
> many. Also it is inherently biased in favour of the parts that were around when
> we originally created it - I'll note that at that time the trade off of resolution
> against sampling period (oversampling or cutting off the measurement) was not common.
>
> That means userspace code has been written that assumes a certain set of attributes.
> The cost of starting to use new attributes is high because no userspace code knows
> about them.  Hence we put a lot of effort into avoiding doing so.  I agree that your
> argument makes sense for your particular device - it doesn't for many other ADCs.
>
> Many ADCs (I'd go as far as to say most though I could be wrong on that) do not
> couple scale and sampling frequency at all.
>
> > It's a result of sampling
> > rate and gain settings. Using it as a setting, for which input value range also
> > changes depending on another attribute is quite cumbersome.
> > To use a sensor the program has to do this:
> > 1. set the sampling rate
> > 2. read available scales for this sampling rate
> > 3. set the scale according to desired gain
> > or know the scales for each sampling rate in advance...which makes available
> > scales attribute quite useless.
>
> For this last point, I think trying to match against previous scale makes a lot of
> sense as there is considerable overlap available here between the different rates.
> I think that would be an improvement.  Another improvement would be to at least
> expose the current resolution - that can be done by providing and _available
> for the raw reading.  Not an idea way to undestand what is going on but it would
> make more data available to userspace.  (_raw_available(max) * scale would give
> the range of the device and allow an adjustment of the scale to achieve what the
> user wants).
>
> ABI design is hard.  We don't always get it right and often there is no right answer.
> Reality is that sometimes userspace code needs to search the space if it is trying
> to get as close as possible to a particular set of constraints.  However lets assume
> in most cases the code has limits of:
>
> 1) A minimum data rate with which it is happy (controls
> the sampling frequency - higher is normally fine, but wastes power etc).
> To get best possible power usage (and in case of this device resolution) it will pick
> the lowest sampling frequency to meet this constraint.
>
> 2) A range of values it is interested in (here related to the PGA settings but not
>    the sampling frequency).  Adding _raw_avail would help it to have visibility of
>    what the range is.
>
> 3) A resolution it cares about getting data at (scale)
>
> We have to present 'scale' because that's necessary to allow userspace to calculate the
> actual voltage.  That adds a constraint to the ABI design.  We also don't want to provide
> more overlapping controls than absolutely necessary as that makes it hard for userspace
> to know which one to use.
>
> So upshot is that userspace has to search to find a value that works for it.
>
> In this particular case the set of ranges at all sampling frequencies are the same.
> So if we assume a constraint on how often data is wanted is more important than the
> resolution (have to pick one or the other to be more important) then we set that
> first to the minimum value to meet the requirement.  Then scale is tweaked to set
> the PGA to match the range desired.  Sure, not super clean but consistent with the
> ABI as it stands (and we can't change that other than very very carefully).
>
> As a fun side note, if the device (or driver) had justified the lower resolutions the other way
> (so the zeros ended up in least significant bits) a common solution would have been
> to just present that to userspace as is, thus the scale would have been decoupled from
> the sampling frequency.  Not this is what oversampling devices normally do...
> We obviously could fake that now, but the issue would then be that it was a major
> driver ABI change. So we can't.
>
> Jonathan
>
>
>
>
>
>
> >
> > Kind regards,
> > Mitja
>