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

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

On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > 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:

Please, remove the context you are agree with or which has no need
to be answered, it helps to parse and reply.

...

> > > > > +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?
> 
> Ok, any frequency lower than 1 Hz does not make sense.

Depends on the cases, we have sub-Hz sensors or some other stuff.
So, "...does not make sense in _this_ case." That's what I implied.

>   static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)

Shouldn't fosc be unsigned?

>   {
>   	/* See datasheet page 31 */

It's fine, but better to add a formula here or more information about
the calculations done in the function.

>   	u32 period = NSEC_PER_SEC / fosc;

period_ns ?

(We usually add units to this kind of variables for better understanding
 of the calculations)

>   	u32 n_avg = BIT(oversamp_ratio) - 1;
>   
>   	/* Result is less than 1 Hz */
>   	if (n_avg >= fosc)
>   		return 1;

+ blank line.

>   	return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
>   }

LGTM, thanks!

> > > > > +}

...

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

> gain_frac would be 999999 max, or 999970 for the limit that fits in the
> register after the math. I think > 1.999.970 is self explanatory.

On the place of unprepared reader this is a complete magic number without
scale, without understanding where it came from, etc.

So, can you define it as a derivative from the other constants and with
a comment perhaps?

> > >   		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.
> 
> Well, by dividing mon_val and micro values by a common divisor the
> operation fit in 32-bits:
> 
>   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
>                                         int gain_frac)
>   {

	/* Divide numerator and denumerator by known great common divider */

>           const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
>           const u32 micro = MICRO / 64;

Yep, I suggested the same in another patch under review (not yours) for
the similar cases where we definitely may easily avoid overflow.

Alternatively you can use gcd().

>           const u32 gain = gain_int * MICRO + gain_frac;
>           int ret;
> 
>           if (gain_int < 0 || gain_frac < 0)
>                   return -EINVAL;
> 
>           if (gain > 1999970)
>                   return -EINVAL;
> 
>           put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), 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)));

Btw, I think you can move this check up and save in a temporary variable which
might affect the binary size of the compiled object as accesses to the gain_int
and gain_frac will be grouped in the same place with potential of the reusing
the CPU register(s)..

>   }

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

-- 
With Best Regards,
Andy Shevchenko