Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:

...

> > > +#define AD4062_MON_VAL_MAX_GAIN		1999970
> > 
> > This is decimal...
> > 
> > > +#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
> > 
> > ...and this is hexadecimal. Can you make these consistent?
> > Also, is there any explanation of the number above? To me
> > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > number multiplied by 1000000 or so? In any case some elaboration
> > would be good to have.
> > 
> Since this is not a magic number, I will use directly below.
> It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000

Okay, at least it will explain the value.

...

> > > +	if (val < 1 || val > BIT(st->chip->max_avg + 1))
> > 
> > in_range() ?
> > 
> > 	in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> > 
> > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > to have all 32 bits set, however it's most unlikely to happen here anyway.
> > 
> Sure, but requires locals to not trigger suspicious usage of sizeof.
>   	// ...
>   	const u32 _max = GENMASK(st->chip->max_avg, 0);
>   	const u32 _min = 1;
>   	int ret;
>   
>   	if (in_range(val, _min, _max))
> > > +		return -EINVAL;

It's fine.

...

> > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > +{
> > > +	/* See datasheet page 31 */
> > > +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > +
> > > +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > 
> > Why u64?
> > 
> > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > more than 4 billions?
> > 
> This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> than 4 seconds, even though I do timeout after 1 seconds in the raw
> acquisition.

Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
and that fits u32. Can you refactor to avoid 64-bit arithmetics?

> > > +}

...

> > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > +{
> > > +	u8 val = AD4062_SOFT_RESET;
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Wait AD4062 treset time */
> > > +	fsleep(5000);
> > 
> > 5 * USEC_PER_MSEC
> > 
> > This gives a hint on the units without even a need to comment or look somewhere
> > else.
> >
> // TODO
> Since the device functional blocks are powered when voltage is supplied,
> here we can stick with the treset datasheet value 60ns (ndelay(60)).

Add a comment and it will work for me, thanks!

> > > +	return 0;
> > > +}

...

> > > +static const int ad4062_oversampling_avail[] = {
> > > +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> > 
> > It's not easy to count them at glance, please add a comment with indices.
> > 
> Ack, will use
>   static const int ad4062_oversampling_avail[] = {
>           BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
>   	BIT(9), BIT(10), BIT(11), BIT(12),
>   };

Of course you can use bit notations, but what I meant is to have

	1, 2, 4, 8, 16, 32, 64, 128,		/*  0 -  7 */
	256, 512, 1024, 2048, 4096,		/*  8 - 12 */

(or something alike).

> > > +};

...

> > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > +{
> > > +	u16 gain;
> > > +	int ret;
> > > +
> > > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > +			       &st->buf.be16, sizeof(st->buf.be16));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	gain = get_unaligned_be16(st->buf.bytes);
> > > +
> > > +	/* From datasheet: code out = code in × mon_val/0x8000 */
> > > +	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
> > 
> > > +	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> > > +				AD4062_MON_VAL_MIDDLE_POINT);
> > 
> > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> > 
> > > +	return IIO_VAL_INT_PLUS_NANO;
> > > +}
> > 
> Can be improved to
> 
>   static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
>   {
>   	int ret;
>   
>   	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
>   			       &st->buf.be16, sizeof(st->buf.be16));
>   	if (ret)
>   		return ret;
>   
>   	/* From datasheet: code out = code in × mon_val/0x8000 */
>   	*val = get_unaligned_be16(st->buf.bytes) * 2;
>   	*val2 = 16;
>   
>   	return IIO_VAL_FRACTIONAL_LOG2;
>   }

Much better, thanks!

...

> > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> > 
> > Forgot to wrap this line.
> > 
> ack
> > > +{
> > > +	u64 gain;
> > > +	int ret;
> > > +
> > > +	if (gain_int < 0 || gain_frac < 0)
> > > +		return -EINVAL;
> > > +
> > > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > 
> > > +
> > 
> > Redundant blank line.
> > 
> Ack.
> > > +	if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > +		return -EINVAL;
> > > +
> > > +	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > +						 MICRO),
> > > +			   st->buf.bytes);
> > 
> > Also in doubt here about 64-bit division.
> > 
> This can be slightly improved to
> 
>   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
>   				      int gain_frac)
>   {
>   	u32 gain;
>   	int ret;
>   
>   	if (gain_int < 0 || gain_frac < 0)
>   		return -EINVAL;
>   
>   	gain = gain_int * MICRO + gain_frac;
>   	if (gain > 1999970)

But this magic should be changed to what you explained to me
(as in 0xffff/0x8000 with the proper precision, and this
 can be done in 32-bit space).

Or even better

	if (gain_int < 0 || gain_int > 1)
		return -EINVAL;

	if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
		return -EINVAL;

>   		return -EINVAL;
>   
>   	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
>   						 MICRO),

...with temporary variable at minimum.

But again, I still don't see the need for 64-bit space.

>   			   st->buf.bytes);
>   
>   	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
>   				&st->buf.be16, sizeof(st->buf.be16));
>   	if (ret)
>   		return ret;
>   
>   	/* Enable scale if gain is not equal to one */
>   	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
>   				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>   				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>   					     !(gain_int == 1 && gain_frac == 0)));
>   }
> 
> To provide the enough resolution to compute every step (e.g., 0xFFFF and
> 0xFFFE) with the arbitrary user input.

...

> > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > 
> > Can be named without leading double underscore? Preference is to use
> > the suffix, like _no_pm (but you can find better one).
> > 
> Since there is one usage of this method, can be merged into ad4062_read_chan_raw.

Good choice!

...

> > > +	struct i3c_priv_xfer t[2] = {
> > > +		{
> > > +			.data.out = &st->reg_addr_conv,
> > > +			.len = sizeof(st->reg_addr_conv),
> > > +			.rnw = false,
> > > +		},
> > > +		{
> > > +			.data.in = &st->buf.be32,
> > > +			.len = sizeof(st->buf.be32),
> > > +			.rnw = true,
> > > +		}
> > > +	};

> > > +	/* Change address pointer to trigger conversion */
> > > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> > 
> > Why array? Just split them on per transfer and use separately. This gives a bit
> > odd feeling that the two goes together, but no. They are semi-related as we
> > have a special condition after the first one.
> > 
> For this commit sure, but in the next a fallback method is introduced
> for when the gp1 gpio line is not connected.
> There are two register to trigger and read samples:
> 
> * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> * write CONV_TRIGGER - [conversion] -> read value -> write ...
> 
> The first allows almost twice the sampling frequency, but does not work
> with the fallback because In-Band-Interrupt for CONV_READ are not
> yielded.

Do you mean that the same array is reused differently? If so, then okay.

> > > +	if (ret)
> > > +		return ret;

...

> > > +	fsleep(4000);
> > 
> > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> > 
> Will add
> 	/* Wait device functional blocks to power up */
> Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> that the device is not ready to switch to acquisition mode for
> conversions.

Good!

-- 
With Best Regards,
Andy Shevchenko