Message-ID: <krbiav67bscvqs6bumx5ay5tk4axeuc4z7gbn26nxgaoqrdfiz@dqzqpgcpclnz>
Date: Mon, 8 Dec 2025 22:16:35 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	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 v3 3/9] iio: adc: Add support for ad4062

On Sat, Dec 06, 2025 at 05:34:59PM +0000, Jonathan Cameron wrote:
> On Fri, 5 Dec 2025 16:12:04 +0100
> Jorge Marques <jorge.marques@...log.com> wrote:
> 
> > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > register (SAR) analog-to-digital converter (ADC) with low-power and
> > threshold monitoring modes.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
Hi Jonathan,
> Hi Jorge,
> 
> I replied late to some of the earlier review discussion as I've been
> away.  Make sure you check those as well as this review as I may have
> forgotten to repeat something here.
> 
> Thanks,
> 
> Jonathan
> 
> > +#include <linux/iio/sysfs.h>
> 
> What is this here for?  It is not needed in a typical modern IIO driver.
> (One day I hope to finish getting rid of the remaining users and drop this
> header!)
> 
IIO_DEVICE_ATTR_RW for events/sampling_frequency in the events commit.
I will add only add at that commit.
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/math.h>
> > +#include <linux/minmax.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +#include <linux/unaligned.h>
> > +#include <linux/util_macros.h>
> 
> > +static const struct iio_scan_type ad4062_scan_type_12_s[] = {
> > +	[AD4062_SCAN_TYPE_SAMPLE] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> 
> Not 12?
Yes, and for burst avg mode, 14 bits.
> 
> > +		.storagebits = 32,
> Given we are doing data mangling anyway why not store in a 16 bit value.
> 
> BTW it would have been easier to spot issues with this if you'd introduced
> the scan type stuff with the use of scans in the patch that adds buffered
> support.  So please move this stuff there.
>
This can be done, just note that for ad4062 in burst avg mode the
realbits is 24 bits, so the storagebits is 32 bits only on that case
and will requires a few conditionals to handle just this case.

To not overly complicated the logic, for ad4062 I will always read
32-bits still. st->reg_addr_conv then takes:
	// IBI Fallback
	st->reg_addr_conv = st->chip->prod_id == 0x7C ? AD4062_REG_CONV_TRIGGER_32BITS :
							AD4062_REG_CONV_TRIGGER_16BITS;
	// GPO IRQ
	st->reg_addr_conv = st->chip->prod_id == 0x7C ? AD4062_REG_CONV_READ_32BITS :
							AD4062_REG_CONV_READ_16BITS;

Then, for sample size:
	const bool is_32b = st->chip->prod_id == 0x7C;
	const size_t _sizeof = is_32b ? sizeof(st->buf.be32) : sizeof(st->buf.be16);
instead of
	const bool is_32b = st->mode == AD4062_BURST_AVERAGING_MODE && st->chip->prod_id == 0x7C;
	const size_t _sizeof = is_32b ? sizeof(st->buf.be32) : sizeof(st->buf.be16);
	+ extra st->reg_addr_conv_avg that may or may not be equal to
	st->reg_addr_conv.

Note that the header section of the I3C transfer (8-bits) occurs
at 1MHz, while the reading in 12.5MHz. I wouldn't go as far as say it is
negligible, but for the part, protocol and software overhead, it
wouldn't provide ground-breaking higher effective maximum
sampling frequency.
> > +		.endianness = IIO_BE,
> > +	},
> > +	[AD4062_SCAN_TYPE_BURST_AVG] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 32,
> > +		.endianness = IIO_BE,
> > +	},
> > +};
> 
 
> > +
> > +static const struct ad4062_chip_info ad4060_chip_info = {
> > +	.name = "ad4060",
> > +	.channels = { AD4062_CHAN(12) },
> > +	.prod_id = 0x7A,
> > +	.max_avg = AD4060_MAX_AVG,
> 
> This is a little confusing. I guess it's the maximum register value, not the
> number of samples averaged.  Perhaps rename.
This isn't doing much, I will just replace with the value and bump the
type from u8 to u16.
ad4060 max_avg:  256
ad4062 max_avg: 4096
> 
> > +};
> > +
> > +static const struct ad4062_chip_info ad4062_chip_info = {
> > +	.name = "ad4062",
> > +	.channels = { AD4062_CHAN(16) },
> > +	.prod_id = 0x7C,
> > +	.max_avg = AD4062_MAX_AVG,
> > +};
> > +
> > +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> > +{
> > +	const u32 _max = GENMASK(st->chip->max_avg, 0)  + 1;
> 
> One too many spaces before +
> 
Here just
	const u32 _max = st->chip->avg_max;
...
> > +		.data.in = &st->buf.be32,
> > +		.len = sizeof(st->buf.be32),
> > +		.rnw = true,
> > +	};
> > +
> > +	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> 
> Rafael did propose a cleaner way of doing this. I'm not sure if it went
> in during the merge window or not.  Take a look to see if it anything
> has been added in the PM pull request (IIRC Rafael converted all existing
> users to the new scheme in that patch set so should be easy to find.)
> 
Yep got merged.
https://github.com/torvalds/linux/commit/ef8057b07c72a817537856b98d6e7493b9404eaf
Will do
	PM_RUNTIME_ACQUIRE(&st->i3cdev->dev, pm);
	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
	if (ret)
		return ret;

> > +	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_set_operation_mode(st, st->mode);
> > +	if (ret)
> > +		return ret;
> > +
Best regards,
Jorge

Powered by blists - more mailing lists