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);
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);
/* 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 */
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);dat = cpu_to_be16(val);
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);
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);
/* 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 */
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;
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;
...
@@ -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.
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.
linux-stable-mirror@lists.linaro.org
-
Gabriel Shahrouzi -
Jonathan Cameron -
Marcelo Schmitt