Message-ID: <20221113120610.6568f9e6@jic23-huawei>
Date:   Sun, 13 Nov 2022 12:06:10 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Mitja Špes <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 2/4] iio: adc: mcp3422: allow setting gain and sampling
 per channel

On Sat, 12 Nov 2022 21:51:36 +0100
Mitja Špes <mitja@...av.com> wrote:

> Hi Jonathan,

Hi Mitja,

For future reference please keep the comments inline with the code and
reply there.  On this occasion I'm coming back this one day alter so it's
not too bad - but if it's a week later as often happens it can be hard
to follow if the code isn't there.

> 
> On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@...nel.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'm not understanding why you should see scales for other sampling rates.
We always allow any IIO attribute to have side effects on others, precisely
to allow for cases where they interact like here.  It's not ideal but there
isn't really a clean solution (as we are seeing here).

> 
> > 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.

> 
> > There are a number of changes in here which are more stylistic cleanup
> > than anything to do with the functional change. Please pull those out
> > to a precursor patch where we can quickly check they make not functional changes
> > rather than having them mixed in here.  
> Will do.
> 
> > I have no particular problem with taking these from hex
> > to decimal, though I'm not really seeing the necessity.
> >
> > However, it is really a style question and should not be in this
> > patch where it mostly adds noise making it slightly harder
> > to spot the functional changes.  
> My styling preference. I think indexes should be decimal so they are not
> confused with flags.

They are also field values as used in the existing code - rather than simply
indexes.  Still field values can be either hex or decimal so given the combined
use I'm fine with the change.  However, precursor patch so we don't have noise
in this patch where closer review is needed.

> 
> > >               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> > > -                             | BIT(IIO_CHAN_INFO_SCALE), \
> > > -             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > > +                             | BIT(IIO_CHAN_INFO_SCALE) \
> > > +                             | BIT(IIO_CHAN_INFO_SAMP_FREQ), \  
> >
> > Hmm. This is an ABI change.  Hopefully no one will notice however.  
> Indeed. I've noted this in the cover letter.
No one reads cover letters :)  Also not in the git log so you want to call attention to
it in the description of this patch as well.

> 
> > > +     for (i = 0; i < MCP3422_SRATE_COUNT; i++) {
> > > +             count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s",
> > > +                     mcp3422_scales[i][0],
> > > +                     mcp3422_scales[i][1],
> > > +                     mcp3422_scales[i][2],
> > > +                     mcp3422_scales[i][3],
> > > +                     (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n"));  
> >
> > What does the output of this now look like?  
> 0.001000000 0.000500000 0.000250000 0.000125000
> 0.000250000 0.000125000 0.000062500 0.000031250
> 0.000062500 0.000031250 0.000015625 0.000007812
> 0.000015625 0.000007812 0.000003906 0.000001953
> All in one line.

Repetition is liable to confuse userspace so if we went this way you would
definitely need to stop doing that.

> 
> > For available attributes we tend to only show the values available assuming
> > just the one thing is changing.  Hence hold sampling frequency static, then
> > show what scales are available
> > It can get complex if there are nasty interactions so we might have a
> > situation where one attribute allows to all the possible values.
> > So maybe we have all scales available and on a write try to find
> > the nearest frequency to the current at which we can deliver the
> > required scale.  
> That's what's being done here:

I don't follow.  This seems to just take the scale and match the first
instance of that it sees. That may not match the option that was available
at the current sampling frequency because of the repetition of possible scales.

> + for (j = 0; j < MCP3422_SRATE_COUNT; j++) {
> + for (i = 0; i < MCP3422_PGA_COUNT; i++) {
> + if (val2 == mcp3422_scales[j][i]) {
> + config &= ~MCP3422_PGA_MASK;
> + config |= MCP3422_PGA_VALUE(i);
> + config &= ~MCP3422_SRATE_MASK;
> + config |= MCP3422_SAMPLE_RATE_VALUE(j);
> + adc->ch_config[req_channel] = config;
> + return 0;
> + }
>   }
>   }
> 
> Before it looked at only one sampling frequency and all gains, now it looks at
> all combinations.
> Looking at this I agree that it would be better to find nearest instead of
> looking for an exact match.
> 
> > My gut feeling for this device is make the sampling frequency the dominant
> > attribute.  So just list the available sampling frequencies not taking
> > scale into account.  For current sampling frequency just list the available
> > scales and only accept those to be written to the scale attr.  
> That way the order in which you set attributes matters. Is that really better?
> This device has a settable sampling rate and gain and they are independent.
> But we could only set gain via scale which values were sampling rate dependent.

This is a very common situation. If you expose all the scales, user who cares about
sampling frequency picks a scale that changes that as well as the pga value.
That is not what they expect to happen. So there is no good answer in cases
like this. Nature of ABI design is that we will always hit corner cases.  The
get of jail free card here was the fact we let changing one attribute affect
others.

Trade off needs to be made and that decision was made in original version of this
driver. I'm not seeing a strong enough reason to change it.

An improvement that does seem logical to me would be to have a change in either
sampling frequency or scale attempt to maintain the nearest value of the other.
In some cases it can be exactly the same.  Any userspace code that already taking
advantage of how the two attributes affect each other will set sampling frequency
first then set the scale.  This optimisation would not affect that so even though
the ABI would change a little it would not be in a way likely to break userspace
code.

Jonathan



> 
> > >       /* meaningful default configuration */
> > > +     for (i = 0; i < 4; i++) {
> > > +             adc->ch_config[i] = (MCP3422_CONT_SAMPLING
> > > +             | MCP3422_CHANNEL_VALUE(i)
> > > +             | MCP3422_PGA_VALUE(MCP3422_PGA_1)
> > > +             | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> > > +     }
> > > +
> > >       config = (MCP3422_CONT_SAMPLING
> > >               | MCP3422_CHANNEL_VALUE(0)
> > >               | MCP3422_PGA_VALUE(MCP3422_PGA_1)  
> >
> > Perhaps use the first channel configuration for this?  
> Will fix.
> 
> Kind regards,
> Mitja

Powered by blists - more mailing lists