Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Mitja Špes]

On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > > Was it possible for these scales to differ before this change?
> > Yes. The difference is that before this change you could only see and set
> > available scales that were available for specified sampling rate. Now you're
> > able to set gain and sampling rate via scale. So before the change you got
> > these (@240sps):
> >
> > 0.001000000 0.000500000 0.000250000 0.000125000
> >
> > Now you get the complete set:
> > /*                 gain x1     gain x2     gain x4     gain x8  */
> > /* 240 sps    */ 0.001000000 0.000500000 0.000250000 0.000125000
> > /*  60 sps    */ 0.000250000 0.000125000 0.000062500 0.000031250
> > /*  15 sps    */ 0.000062500 0.000031250 0.000015625 0.000007812
> > /*   3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953
>
> Ok. That doesn't work as a standard interface because userspace code wants to pick say
> 0.00062500 which appears twice.
I don't know how I missed that. It's clear to me now that this patch is wrong.


> > > If not, then why was the previous patch a fix rather than simply a precursor
> > > to this change (where it now matters).
> > I wanted to separate a bug fix from improvements, if these were rejected for
> > for some reason.
>
> Is it a bug fix?  The way I read it is that, before this patch there is only
> one scale that is applied to all channels.  As such, the current value == the
> value set and the code works as expected.
> So the previous patch is only necessary once this one is applied.  Hence no
> bug, just a rework that is useful to enabling this feature.
I'll post the previous snippet here and write the comments inline:
----
@@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
  struct mcp3422 *adc = iio_priv(iio);
  int err;

+ u8 req_channel = channel->channel;
  u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
- u8 pga = MCP3422_PGA(adc->config);  /* <- this uses the "current" config
      which changes depending on the last read channel */
+ u8 pga = adc->pga[req_channel];          /* this now returns the PGA for the
      selected channel */

  switch (mask) {
  case IIO_CHAN_INFO_RAW:
----
I hope this clarifies the bugfix.


Thanks for in depth look at this and sorry for wasting your time with this
flawed patch.

Kind regards,
Mitja