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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

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;
>