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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fue274bpfgsytm5z25rdsawbfa2s4vu4ypoqrrmrx5dfe2jgdq@ocjqribzo5mn>
Date: Thu, 4 Dec 2025 22:37:38 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, 
	Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

On Fri, Nov 28, 2025 at 09:25:50PM +0200, Andy Shevchenko wrote:
Hi Andy,

> 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:
> 
> >   static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
> 
> Shouldn't fosc be unsigned?
> 
Yep
> >   {
> >   	/* 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)
> 
Ack.
> > > >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > >   				      int 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?
> 
(my proposal is after all your comments below)
> > > >   		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().
> 
> >           put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
> 
> 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)..
> 
> >   }
> 
I believe this is clear and fits all points:

 	/* Divide numerator and denumerator by known great common divider */
	const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
	const u32 micro = MICRO / 64;
	const u32 gain_fp = gain_int * MICRO + gain_frac;
	const u32 reg_val = DIV_ROUND_CLOSEST(gain_fp * mon_val, micro);
	int ret;

	/* Checks if the gain is in range and the value fits the field */
	if (gain_int < 0 || gain_int > 1 || reg_val > BIT(16) - 1)
		return -EINVAL;

	put_unaligned_be16(reg_val, st->buf.bytes);

Explains 64 value. Checks if is in range [0, 2), then if fits the
register field for corner case of range (1.999970, 2) (0x10000). Full
formula is in the previous method ad4062_get_chan_calibscale.


> > > >   			   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
> 
> 
Best Regards,
Jorge