Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221111112657.1521307-1-mitja@lxnav.com> <20221111112657.1521307-3-mitja@lxnav.com>
 <20221112172806.6db090eb@jic23-huawei> <CACbQKWfEa64Fv4CmW8BDp2rXw504YyL_s2TWiA_SwH-zCKKvCA@mail.gmail.com>
 <20221113120610.6568f9e6@jic23-huawei>
In-Reply-To: <20221113120610.6568f9e6@jic23-huawei>
From:   =?UTF-8?Q?Mitja_=C5=A0pes?= <mitja@lxnav.com>
Date:   Sun, 13 Nov 2022 14:39:03 +0100
Message-ID: <CACbQKWfzbxS2SKzd3v=h8-3oQw3hRhZJr_fJMaiTKaFwLn-jJg@mail.gmail.com>
Subject: Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling
 per channel
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Angelo Compagnucci <angelo.compagnucci@gmail.com>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.


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

Kind regards,
Mitja