linux-kernel - Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKUZ0z+AsM_UUtSDAZyBRMOt2CiqghBfoV1-s4N-Q9zQHC1++w@mail.gmail.com>
Date: Fri, 18 Apr 2025 14:11:59 -0400
From: Gabriel Shahrouzi <gshahrouzi@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: gregkh@...uxfoundation.org, lars@...afoo.de, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev, 
	Michael.Hennerich@...log.com, skhan@...uxfoundation.org, 
	kernelmentees@...ts.linuxfoundation.org, stable@...r.kernel.org
Subject: Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet

On Fri, Apr 18, 2025 at 11:37 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Wed, 16 Apr 2025 10:22:19 -0400
> Gabriel Shahrouzi <gshahrouzi@...il.com> wrote:
>
> > Implement the settling cycles encoding as specified in the AD5933
> > datasheet, Table 13 ("Number of Settling Times Cycles Register"). The
> > previous logic did not correctly translate the user-requested effective
> > cycle count into the required 9-bit base + 2-bit multiplier format
> > (D10..D0) for values exceeding 511.
> >
> > Clamp the user input for out_altvoltage0_settling_cycles to the
> > maximum effective value of 2044 cycles (511 * 4x multiplier).
> >
> > Fixes: f94aa354d676 ("iio: impedance-analyzer: New driver for AD5933/4 Impedance Converter, Network Analyzer")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> > ---
> >  .../staging/iio/impedance-analyzer/ad5933.c   | 21 ++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index d5544fc2fe989..5a8c5039bb159 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -28,7 +28,7 @@
> >  #define AD5933_REG_FREQ_START                0x82    /* R/W, 3 bytes */
> >  #define AD5933_REG_FREQ_INC          0x85    /* R/W, 3 bytes */
> >  #define AD5933_REG_INC_NUM           0x88    /* R/W, 2 bytes, 9 bit */
> > -#define AD5933_REG_SETTLING_CYCLES   0x8A    /* R/W, 2 bytes */
> > +#define AD5933_REG_SETTLING_CYCLES   0x8A    /* R/W, 2 bytes, 11+2 bit */
>
> Probably spaces around the +  It's not code, but lets keep to consistent code style.
Got it.
>
> >  #define AD5933_REG_STATUS            0x8F    /* R, 1 byte */
> >  #define AD5933_REG_TEMP_DATA         0x92    /* R, 2 bytes*/
> >  #define AD5933_REG_REAL_DATA         0x94    /* R, 2 bytes*/
> > @@ -71,6 +71,8 @@
> >  #define AD5933_INT_OSC_FREQ_Hz               16776000
> >  #define AD5933_MAX_OUTPUT_FREQ_Hz    100000
> >  #define AD5933_MAX_RETRIES           100
> > +#define AD5933_MAX_FREQ_POINTS               511
> > +#define AD5933_MAX_SETTLING_CYCLES   2044 /* 511 * 4 */
>
> >
> >  #define AD5933_OUT_RANGE             1
> >  #define AD5933_OUT_RANGE_AVAIL               2
> > @@ -82,6 +84,10 @@
> >  #define AD5933_POLL_TIME_ms          10
> >  #define AD5933_INIT_EXCITATION_TIME_ms       100
> >
> > +/* Settling cycles multiplier bits D10, D9 */
> > +#define AD5933_SETTLE_MUL_2X         BIT(9)
> > +#define AD5933_SETTLE_MUL_4X         (BIT(9) | BIT(10))
>
> That looks like  a number, not a pair of separate bits.
> I would expect a mask for this field then some defines for teh
> values it can take.
Got it. Defined the mask and then the values. Change added to v2.
>
> > +
> >  struct ad5933_state {
> >       struct i2c_client               *client;
> >       struct clk                      *mclk;
> > @@ -411,14 +417,15 @@ static ssize_t ad5933_store(struct device *dev,
> >               ret = ad5933_cmd(st, 0);
> >               break;
> >       case AD5933_OUT_SETTLING_CYCLES:
> > -             val = clamp(val, (u16)0, (u16)0x7FF);
> > +             val = clamp(val, (u16)0, (u16)AD5933_MAX_SETTLING_CYCLES);
> >               st->settling_cycles = val;
> >
> > -             /* 2x, 4x handling, see datasheet */
> > +             /* Encode value for register: D10..D0 */
> > +             /* Datasheet Table 13: If cycles > 1022 -> val/4, set bits D10=1, D9=1 */
> >               if (val > 1022)
> > -                     val = (val >> 2) | (3 << 9);
> > -             else if (val > 511)
> > -                     val = (val >> 1) | BIT(9);
> > +                     val = (val >> 2) | AD5933_SETTLE_MUL_4X;
> > +             else if (val > 511) /* Datasheet: If cycles > 511 -> val/2, set bit D9=1 */
> > +                     val = (val >> 1) | AD5933_SETTLE_MUL_2X;
> >
> >               dat = cpu_to_be16(val);
> >               ret = ad5933_i2c_write(st->client,
> > @@ -426,7 +433,7 @@ static ssize_t ad5933_store(struct device *dev,
> >                                      2, (u8 *)&dat);
> >               break;
> >       case AD5933_FREQ_POINTS:
> > -             val = clamp(val, (u16)0, (u16)511);
> > +             val = clamp(val, (u16)0, (u16)AD5933_MAX_FREQ_POINTS);
> >               st->freq_points = val;
> >
> >               dat = cpu_to_be16(val);
>