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;
Date:   Tue, 22 Mar 2022 20:38:44 +0000
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Stephen Boyd <swboyd@chromium.org>
CC:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        <linux-kernel@vger.kernel.org>, <linux-iio@vger.kernel.org>,
        Gwendal Grignou <gwendal@chromium.org>
Subject: Re: [PATCH] iio:proximity:sx9324: Fix hardware gain read/write
Message-ID: <20220322203844.0000466f@Huawei.com>
In-Reply-To: <CAE-0n52jDZz0qKhfg8OWVDmDg5+xXo-qSL3jNka82QHwA2-xsw@mail.gmail.com>
References: <20220318204808.3404542-1-swboyd@chromium.org>
        <20220319152641.49d8b3e1@jic23-huawei>
        <CAE-0n52jDZz0qKhfg8OWVDmDg5+xXo-qSL3jNka82QHwA2-xsw@mail.gmail.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 21 Mar 2022 19:36:33 +0100
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2022-03-19 08:26:41)
> > On Fri, 18 Mar 2022 13:48:08 -0700
> > Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Hi Stephen,
> >
> >  
> > > There are four possible gain values according to sx9324_gain_vals[]: 1,
> > > 2, 4, and 8. When writing and reading the register the values are off by
> > > one.
> > > The bits should be set according to this equation:
> > >
> > >       ilog2(<gain>) + 1
> > >
> > > so that a gain of 8 is 0x3 in the register field and a gain of 4 is 0x2
> > > in the register field, etc. Fix up the functions.  
> >
> > So is the 0 value reserved?  I can't find an sx9324 datasheet but he
> > 9320 is online and that seems to be the case there.  If so please state
> > that in this description as well.  
> 
> Yes 0 is reserved. The top of this driver's C file has the datasheet
> link[1]
Ah. Thanks ;)

> 
> >  
> > >
> > > Fixes: 4c18a890dff8 ("iio:proximity:sx9324: Add SX9324 support")
> > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  drivers/iio/proximity/sx9324.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> > > index 0d9bbbb50cb4..a3c8e02f5a56 100644
> > > --- a/drivers/iio/proximity/sx9324.c
> > > +++ b/drivers/iio/proximity/sx9324.c
> > > @@ -379,7 +379,10 @@ static int sx9324_read_gain(struct sx_common_data *data,
> > >       if (ret)
> > >               return ret;
> > >
> > > -     *val = 1 << FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
> > > +     regval = FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
> > > +     if (regval)  
> >
> > If 0 is reserved then I'd return and error code here to indicate
> > we don't know what the gain is rather than carrying on regardless.
> > Or is this going to cause problems as it will be an ABI change (error
> > return possible when it wasn't really before)?
> >  
> 
> That sounds OK to me. The driver is only being introduced now so we can
> still fix it to reject a gain of 0. Unless 0 should mean "off", i.e.
> hardware gain of 1?
No.  I don't think we want to add that sort of fiddly definition.
So error is the way to go - I'd forgotten we only just introduced this
so no ABI breakage risk.


Jonathan

> 
> [1] https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf