Re: [PATCH v3 2/2] iio: adc: add support for PAC1711

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

On Wed, Sep 09, 2026 at 03:23:35PM +0300, Ariana Lazar wrote:
> This is the iio driver for Microchip PAC1711, PAC1721, PAC1811 and
> PAC1821 single-channel power monitors with accumulator. The PAC1711 and
> PAC1721 devices use 12-bit resolution for voltage and current measurements
> and 24 bits for power calculations, while PAC1811 and PAC1821 have 16-bit
> resolution and use 32 bits for power calculations. The 56-bit accumulator
> register accumulates power (energy) or current (Coulomb counter).
> 
> PAC1711 and PAC1811 measure up to 42V Full-Scale Range, respectively 9V for
> PAC1721 and PAC1821.

...

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/byteorder/generic.h>

No, it should be asm/byteorder.h...

> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kstrtox.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/math64.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/property.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/workqueue.h>

...somewhere here.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

...

> +#define PAC1711_MAX_RFSH_LIMIT_MS	60000

60 * MSEC_PER_SEC


> +/* 50msec is the timeout for validity of the cached registers */
> +#define PAC1711_MIN_POLLING_TIME_MS	50

> +/*
> + * 1000usec is the minimum wait time for normal conversions when sample

1 ms

> + * rate doesn't change

Missing period at the end.

> + */
> +#define PAC1711_MIN_UPDATE_WAIT_TIME_US	1000

1 * USEC_PER_MSEC

...

> +/* 42000mV */

42 * MILLI

> +#define PAC1711_VOLTAGE_MILLIVOLTS_MAX	42000
> +#define PAC1721_VOLTAGE_MILLIVOLTS_MAX	9000

9 * MILLI

...

> +/* Maximum power-product value - 42 V * 0.1 V */
> +#define PAC1711_PRODUCT_VOLTAGE_PV_FSR	(4200ULL * NANO)
> +#define PAC1721_PRODUCT_VOLTAGE_PV_FSR	(900ULL * NANO)

The comment and the values are not in sync. I'm confused, for example,
by how NANO appears at all there.

...

> +#define PAC1711_DEV_ATTR(name)		(&iio_dev_attr_##name.dev_attr.attr)

Unneeded, please use the values directly, do not hide the rest.

...

> +static const int pac1711_vbus_range_tbl[2][3][2] = {
> +	[PAC1711_VOLTAGE_RANGE_IDX] = {
> +		[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, 42000000 },
> +		[PAC1711_FULL_RANGE_BIPOLAR]  = { -42000000, 42000000 },
> +		[PAC1711_HALF_RANGE_BIPOLAR]  = { -21000000, 21000000 },
> +	},
> +	[PAC1721_VOLTAGE_RANGE_IDX] = {
> +		[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, 9000000 },
> +		[PAC1711_FULL_RANGE_BIPOLAR]  = { -9000000, 9000000 },
> +		[PAC1711_HALF_RANGE_BIPOLAR]  = { -4500000, 4500000 },
> +	},
> +};
> +
> +static const int pac1711_vsense_range_tbl[3][2] = {
> +	[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, 100000 },
> +	[PAC1711_FULL_RANGE_BIPOLAR] = { -100000, 100000 },
> +	[PAC1711_HALF_RANGE_BIPOLAR] = { -50000, 50000 },
> +};

Perhaps above needs more definitions as I see the similarities with
the existing ones.

...

> +struct reg_data {
> +	s64		vacc;
> +	s64		acc_val;
> +	s64		vpower;
> +	s32		vsense;
> +	s32		vbus;
> +	u32		acc_count;

> +	unsigned long	jiffies_tstamp;

I would put variadic size variables at the bottom as they doesn't sound
like HW related.

> +	u16		ctrl_act_reg;
> +	u16		ctrl_lat_reg;
> +	u8		meas_regs[PAC1711_MEAS_REG_SNAPSHOT_LEN];
> +};

...

> +static inline u64 pac1711_get_unaligned_be56(u8 *p)

const u8 *p

> +{
> +	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> +		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
> +}

Currently it seems this will be the first user (maybe some more are hiding somewhere)
of this. Nevertheless I would dare to put it directly into include/linux/unaligned.h.
This will eliminate possible duplication in the future.

...

For now I stopped here. Can you split this patch at least to two (introduce a
basic support for some part number(s) followed by the other part numbers to be
added and maybe some optional features?

...

> +static struct i2c_driver pac1711_driver = {
> +	.driver	 = {
> +		.name = "pac1711",
> +		.of_match_table = pac1711_of_match,
> +	},
> +	.probe = pac1711_probe,
> +	.id_table = pac1711_id,
> +};

> +

Unneeded blank line.

> +module_i2c_driver(pac1711_driver);

-- 
With Best Regards,
Andy Shevchenko