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;