* [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
@ 2025-04-16 14:22 Gabriel Shahrouzi
2025-04-18 15:37 ` Jonathan Cameron
2025-04-19 15:42 ` Marcelo Schmitt
0 siblings, 2 replies; 7+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-16 14:22 UTC (permalink / raw)
To: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich
Cc: gshahrouzi, skhan, kernelmentees, stable
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@vger.kernel.org
Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.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 */
#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))
+
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-16 14:22 [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet Gabriel Shahrouzi
@ 2025-04-18 15:37 ` Jonathan Cameron
2025-04-18 18:11 ` Gabriel Shahrouzi
2025-04-19 15:42 ` Marcelo Schmitt
1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2025-04-18 15:37 UTC (permalink / raw)
To: Gabriel Shahrouzi
Cc: gregkh, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
On Wed, 16 Apr 2025 10:22:19 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.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@vger.kernel.org
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.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.
> #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.
> +
> 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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-18 15:37 ` Jonathan Cameron
@ 2025-04-18 18:11 ` Gabriel Shahrouzi
0 siblings, 0 replies; 7+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-18 18:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: gregkh, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
On Fri, Apr 18, 2025 at 11:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 16 Apr 2025 10:22:19 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.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@vger.kernel.org
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.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);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-16 14:22 [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet Gabriel Shahrouzi
2025-04-18 15:37 ` Jonathan Cameron
@ 2025-04-19 15:42 ` Marcelo Schmitt
2025-04-20 0:25 ` Gabriel Shahrouzi
1 sibling, 1 reply; 7+ messages in thread
From: Marcelo Schmitt @ 2025-04-19 15:42 UTC (permalink / raw)
To: Gabriel Shahrouzi
Cc: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
Hi Gabriel,
Probably a thing for a separate patch but, would it make code more readable if
use masks and bitfield to set register data?
See comments bellow.
Regards,
Marcelo
On 04/16, Gabriel Shahrouzi 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@vger.kernel.org
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.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 */
> #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))
In addition to making the above a mask as suggested by Jonathan, we could also
have a mask for the number of settling time cycles. E.g.
#define AD5933_SETTLING_TIME_CYCLES_MSK GENMASK(8, 0)
Would also need to update defines to something like
#define AD5933_SETTLE_MUL_2X 0x1
#define AD5933_SETTLE_MUL_4X 0x3
masks and define names up to you.
> +
> 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;
then this would become something like
reg_data &= ~AD5933_SETTLE_MUL_MSK;
reg_data |= FIELD_PREP(AD5933_SETTLE_MUL_MSK, AD5933_SETTLE_MUL_4X);
reg_data &= ~AD5933_SETTLING_TIME_CYCLES_MSK;
reg_data |= FIELD_PREP(AD5933_SETTLING_TIME_CYCLES_MSK, val >> 2);
...
Though, I guess it would then be preferable to use masks and bitfield macros for
all other places where we handle register data in ad5933 driver. Probably
something for a different patch (if worth it).
> + else if (val > 511) /* Datasheet: If cycles > 511 -> val/2, set bit D9=1 */
> + val = (val >> 1) | AD5933_SETTLE_MUL_2X;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-19 15:42 ` Marcelo Schmitt
@ 2025-04-20 0:25 ` Gabriel Shahrouzi
2025-04-20 0:48 ` Marcelo Schmitt
0 siblings, 1 reply; 7+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-20 0:25 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
On Sat, Apr 19, 2025 at 11:41 AM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> Hi Gabriel,
>
> Probably a thing for a separate patch but, would it make code more readable if
> use masks and bitfield to set register data?
> See comments bellow.
>
> Regards,
> Marcelo
>
> On 04/16, Gabriel Shahrouzi 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@vger.kernel.org
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.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 */
> > #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))
> In addition to making the above a mask as suggested by Jonathan, we could also
> have a mask for the number of settling time cycles. E.g.
> #define AD5933_SETTLING_TIME_CYCLES_MSK GENMASK(8, 0)
>
> Would also need to update defines to something like
> #define AD5933_SETTLE_MUL_2X 0x1
> #define AD5933_SETTLE_MUL_4X 0x3
>
> masks and define names up to you.
>
> > +
> > 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;
> then this would become something like
>
> reg_data &= ~AD5933_SETTLE_MUL_MSK;
> reg_data |= FIELD_PREP(AD5933_SETTLE_MUL_MSK, AD5933_SETTLE_MUL_4X);
> reg_data &= ~AD5933_SETTLING_TIME_CYCLES_MSK;
> reg_data |= FIELD_PREP(AD5933_SETTLING_TIME_CYCLES_MSK, val >> 2);
I currently have:
val >>= 2;
val |= FIELD_PREP(AD5933_SETTLING_MULTIPLIER_MASK,
AD5933_SETTLING_MULTIPLIER_VAL_X4);
which assumes val only has bits within a certain range which I believe
is the case here but not completely sure. Would it be better to clear
the bits regardless and then apply said operations?
> ...
>
> Though, I guess it would then be preferable to use masks and bitfield macros for
> all other places where we handle register data in ad5933 driver. Probably
> something for a different patch (if worth it).
I separated the original fix from the refactoring so the patches stay
modular. I can apply the use of masks and bitfield macros for other
register data. Should the refactoring be all in one patch or spread
across multiple?
>
> > + else if (val > 511) /* Datasheet: If cycles > 511 -> val/2, set bit D9=1 */
> > + val = (val >> 1) | AD5933_SETTLE_MUL_2X;
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-20 0:25 ` Gabriel Shahrouzi
@ 2025-04-20 0:48 ` Marcelo Schmitt
2025-04-20 1:12 ` Gabriel Shahrouzi
0 siblings, 1 reply; 7+ messages in thread
From: Marcelo Schmitt @ 2025-04-20 0:48 UTC (permalink / raw)
To: Gabriel Shahrouzi
Cc: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
...
> > > @@ -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;
> > then this would become something like
> >
> > reg_data &= ~AD5933_SETTLE_MUL_MSK;
> > reg_data |= FIELD_PREP(AD5933_SETTLE_MUL_MSK, AD5933_SETTLE_MUL_4X);
> > reg_data &= ~AD5933_SETTLING_TIME_CYCLES_MSK;
> > reg_data |= FIELD_PREP(AD5933_SETTLING_TIME_CYCLES_MSK, val >> 2);
> I currently have:
> val >>= 2;
> val |= FIELD_PREP(AD5933_SETTLING_MULTIPLIER_MASK,
> AD5933_SETTLING_MULTIPLIER_VAL_X4);
> which assumes val only has bits within a certain range which I believe
> is the case here but not completely sure. Would it be better to clear
> the bits regardless and then apply said operations?
Nah, since val is being clamped to max settling time value, I think this is good.
> > ...
> >
> > Though, I guess it would then be preferable to use masks and bitfield macros for
> > all other places where we handle register data in ad5933 driver. Probably
> > something for a different patch (if worth it).
> I separated the original fix from the refactoring so the patches stay
> modular. I can apply the use of masks and bitfield macros for other
> register data. Should the refactoring be all in one patch or spread
> across multiple?
I believe all mask/bitfield refactoring can be done in one patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet
2025-04-20 0:48 ` Marcelo Schmitt
@ 2025-04-20 1:12 ` Gabriel Shahrouzi
0 siblings, 0 replies; 7+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-20 1:12 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees, stable
On Sat, Apr 19, 2025 at 8:47 PM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> ...
> > > > @@ -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;
> > > then this would become something like
> > >
> > > reg_data &= ~AD5933_SETTLE_MUL_MSK;
> > > reg_data |= FIELD_PREP(AD5933_SETTLE_MUL_MSK, AD5933_SETTLE_MUL_4X);
> > > reg_data &= ~AD5933_SETTLING_TIME_CYCLES_MSK;
> > > reg_data |= FIELD_PREP(AD5933_SETTLING_TIME_CYCLES_MSK, val >> 2);
> > I currently have:
> > val >>= 2;
> > val |= FIELD_PREP(AD5933_SETTLING_MULTIPLIER_MASK,
> > AD5933_SETTLING_MULTIPLIER_VAL_X4);
> > which assumes val only has bits within a certain range which I believe
> > is the case here but not completely sure. Would it be better to clear
> > the bits regardless and then apply said operations?
> Nah, since val is being clamped to max settling time value, I think this is good.
Got it.
>
> > > ...
> > >
> > > Though, I guess it would then be preferable to use masks and bitfield macros for
> > > all other places where we handle register data in ad5933 driver. Probably
> > > something for a different patch (if worth it).
> > I separated the original fix from the refactoring so the patches stay
> > modular. I can apply the use of masks and bitfield macros for other
> > register data. Should the refactoring be all in one patch or spread
> > across multiple?
>
> I believe all mask/bitfield refactoring can be done in one patch.
Sounds good.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-20 1:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-16 14:22 [PATCH] iio: ad5933: Correct settling cycles encoding per datasheet Gabriel Shahrouzi
2025-04-18 15:37 ` Jonathan Cameron
2025-04-18 18:11 ` Gabriel Shahrouzi
2025-04-19 15:42 ` Marcelo Schmitt
2025-04-20 0:25 ` Gabriel Shahrouzi
2025-04-20 0:48 ` Marcelo Schmitt
2025-04-20 1:12 ` Gabriel Shahrouzi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox