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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 13 Nov 2022 14:39:03 +0100
Mitja Špes <mitja@lxnav.com> wrote:

> On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> 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.

Ah I see. The bit that threw me off was the title of this patch.
"allow setting gain ... per channel" which made me think that before this patch
there was one gain for all channels.  I was too lazy to actually check and discover
that it has always been per channel on the write side of things.

Jonathan


> 
> 
> Thanks for in depth look at this and sorry for wasting your time with this
> flawed patch.
> 
> Kind regards,
> Mitja