Re: [PATCH v5 5/5] iio: adc: add support for PAC1711

[Andy Shevchenko]

On Tue, Nov 03, 2026 at 04:37:15PM +0200, 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.

...

> +config PAC1711
> +	tristate "Microchip Technology PAC1711 driver"

Align this text with the Summary at the top comment and MODULE_DESCRIPTION()
that all three read the same (or quite similar, like the same with or without
word "driver").

> +	depends on I2C
> +	help
> +	  Say yes here to build support for Microchip Technology's PAC1711,
> +	  PAC1721, PAC1811 and PAC1821 Single-Channel Power Monitors with
> +	  Accumulator.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called pac1711.

...

> + * IIO driver for PAC1711 Single-Channel DC Power/Energy Monitor

^^^ See above.

...

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

I failed to see how it's used/ dev_printk.h, yes, but the rest of device.h?

> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/jiffies.h>
> +#include <linux/kstrtox.h>
> +#include <linux/module.h>
> +#include <linux/math64.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/property.h>

+ time.h // for MSEC_PER_SEC

> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/workqueue.h>

+ blank line.

> +#include <asm/byteorder.h>
> +
> +#include <linux/iio/iio.h>

...

> +#define PAC1711_VOLTAGE_MILLIVOLTS_MAX	(42 * MILLI)
> +#define PAC1721_VOLTAGE_MILLIVOLTS_MAX	(9 * MILLI)

#define PAC1711_VOLTAGE_mV_MAX	(42 * MILLI)
#define PAC1721_VOLTAGE_mV_MAX	(9 * MILLI)

...

> +/* Full FSR power is multiplied by 10^9 to ensure precise scale calculation. */
> +#define PAC1711_PRODUCT_VOLTAGE_PV_FSR	(4200ULL * NANO)
> +#define PAC1721_PRODUCT_VOLTAGE_PV_FSR	(900ULL * NANO)

_pV (I assume picovolts is in mind?) Note, this also the same as above with
just a multiplier. Why do we need them separately (or independent)?

...

> +/* PAC1711/PAC1811 VBUS FSR: 42V and 21V */
> +#define PAC1711_VBUS_FSR_UV		(42 * MICRO)
> +#define PAC1711_VBUS_FSR_HALF_UV	(21 * MICRO)

> +/* PAC1721/PAC1821 VBUS FSR: 9V and 4.5V */
> +#define PAC1721_VBUS_FSR_UV		(9 * MICRO)
> +#define PAC1721_VBUS_FSR_HALF_UV	(4500 * MILLI)
> +
> +/* VSENSE FSR: 100mV and 50mV */
> +#define PAC1711_VSENSE_FSR_UV		(100 * MILLI)
> +#define PAC1711_VSENSE_FSR_HALF_UV	(50 * MILLI)

_UV ==> _uV

...

> +#define PAC1711_ACC_ENERGY_STR		"energy"
> +#define PAC1711_ACC_CHARGE_STR		"charge"

One-time use, kill the macro, it's visible from the below.

> +static const char * const pac1711_acc_sources[] = {
> +	[PAC1711_ACCMODE_VPOWER] = PAC1711_ACC_ENERGY_STR,
> +	[PAC1711_ACCMODE_VSENSE] = PAC1711_ACC_CHARGE_STR,
> +};

...

> +static const int pac1711_vbus_range_tbl[2][3][2] = {
> +	[PAC1711_VOLTAGE_RANGE_IDX] = {
> +		[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, PAC1711_VBUS_FSR_UV },
> +		[PAC1711_FULL_RANGE_BIPOLAR]  = { -(int)PAC1711_VBUS_FSR_UV, PAC1711_VBUS_FSR_UV },
> +		[PAC1711_HALF_RANGE_BIPOLAR]  = { -(int)PAC1711_VBUS_FSR_HALF_UV,
> +						  PAC1711_VBUS_FSR_HALF_UV },
> +	},
> +	[PAC1721_VOLTAGE_RANGE_IDX] = {
> +		[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, PAC1721_VBUS_FSR_UV },
> +		[PAC1711_FULL_RANGE_BIPOLAR]  = { -(int)PAC1721_VBUS_FSR_UV, PAC1721_VBUS_FSR_UV },
> +		[PAC1711_HALF_RANGE_BIPOLAR]  = { -(int)PAC1721_VBUS_FSR_HALF_UV,
> +						  PAC1721_VBUS_FSR_HALF_UV },

There is an indentation issue, also no need to format '=' on the same column.

		[PAC1711_FULL_RANGE_UNIPOLAR] = { 0, PAC1721_VBUS_FSR_uV },
		// alternatively
		[PAC1711_FULL_RANGE_UNIPOLAR] = {
			0,
			PAC1721_VBUS_FSR_uV,
		},
		[PAC1711_FULL_RANGE_BIPOLAR]  = {
			-(int)PAC1721_VBUS_FSR_uV,
			PAC1721_VBUS_FSR_uV,
		},
		[PAC1711_HALF_RANGE_BIPOLAR]  = {
			-(int)PAC1721_VBUS_FSR_HALF_uV,
			PAC1721_VBUS_FSR_HALF_uV,
		},

> +	},
> +};

...and so on...

...

> +enum {
> +	PAC1711_SAMP_8192SPS = 0,
> +	PAC1711_SAMP_4096SPS = 1,
> +	PAC1711_SAMP_1024SPS = 2,
> +	PAC1711_SAMP_256SPS = 3,
> +	PAC1711_SAMP_64SPS = 4,
> +	PAC1711_SAMP_8SPS = 5,
> +};

This enum is not directly used in HW, but okay, it might make code robust with
the explicit assignments.

...

> +struct pac1711_features {
> +	u64 voltage_pv_fsr;
> +	int voltage_millivolts_max;
> +	const char *name;
> +	u8 voltage_range_idx;
> +	bool has_16bit_resolution;

Run `pahole` and act accordingly. Same for other data types where the order
is not strict.

> +};

...

> +static int pac1711_send_refresh(struct pac1711_chip_info *info, u32 wait_time)
> +{
> +	struct i2c_client *client = info->client;
> +	int ret;
> +
> +	/* Writing a REFRESH or a REFRESH_V command */
> +	ret = i2c_smbus_write_byte(client, PAC1711_REFRESH_REG_ADDR);
> +	if (ret) {
> +		dev_err(&client->dev, "%s - cannot send Refresh cmd (0x%02X)\n",
> +			__func__, PAC1711_REFRESH_REG_ADDR);

__func__ means that the message itself is poorly written (not enough uniqueness
to understand from what flow point it comes. Ditto for all __func__ uses, revisit
all the messages and make them more precise if required and drop __func__.

> +		return ret;
> +	}
> +
> +	/* Register data retrieval timestamp */
> +	info->chip_reg_data.jiffies_tstamp = jiffies;
> +
> +	/* Wait till the data is available */
> +	fsleep(wait_time);
> +
> +	return 0;
> +}

...

> +	/* Read the ctrl/status registers for this snapshot */
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_ACT_REG_ADDR,
> +					    sizeof(val_be16), (u8 *)&val_be16);
> +	if (ret != sizeof(val_be16)) {
> +		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
> +			__func__, PAC1711_CTRL_ACT_REG_ADDR);
> +		return ret < 0 ? ret : -EIO;
> +	}

No nested branches, it's harder to follow. (Here and elsewhere.)

...

> +static int pac1711_reg_snapshot(struct pac1711_chip_info *info, bool *refreshed)
> +{
> +	u32 wait_time;

Units?

> +	int ret;
> +
> +	guard(mutex)(&info->lock);
> +
> +	/* No hardware refresh is needed */
> +	if (refreshed)
> +		*refreshed = false;
> +
> +	/* Return early if the minimum polling time hasn't elapsed */
> +	if (!time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
> +			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS)))
> +		return 0;
> +
> +	wait_time = (1024 * 1000) / pac1711_samp_rate_map_tbl[info->sample_rate_idx];

Depending on the above, 1000 can be replaced with named multiplier.
Same for elsewhere magic 1000.

> +	/* A hardware refresh will happen */
> +	ret = pac1711_reg_snapshot_locked(info, wait_time);
> +	if (!ret && refreshed)

No. use the regular pattern

	if (ret)
		return ret;

	if (refreshed)
		...

	return 0;

> +		*refreshed = true;
> +
> +	return ret;
> +}

...

> +	if (!ret)
> +		info->enable_acc = val;

Why two times to check ret?

> +	if (ret)
> +		dev_err(dev, "failed to reset accumulator: %d\n", ret);
> +
> +	return ret;

...

> +		guard(mutex)(&info->lock);

+ blank line.

> +		shunt = info->shunt;
> +
> +		switch (chan->address) {
> +		case PAC1711_VBUS_REG_ADDR:
> +			*val = info->features->voltage_millivolts_max;
> +			*val2 = (info->vbus_mode == PAC1711_FULL_RANGE_BIPOLAR) ? 15 : 16;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case PAC1711_VSENSE_REG_ADDR:
> +			/*
> +			 * Currents - scale for mA - depends on the channel's shunt value
> +			 * (100mV * 1000000) / (2^16 * shunt(uohm))
> +			 */
> +			*val = 1526;
> +			*val2 = shunt;
> +
> +			if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
> +				*val = *val << 1;

				*val *= 2;

> +
> +			return IIO_VAL_FRACTIONAL;
> +		case PAC1711_VPOWER_REG_ADDR:
> +		case PAC1711_VACC_REG_ADDR:
> +			if (chan->type == IIO_CHARGE) {
> +				/*
> +				 * Coulomb counter scale:
> +				 * 16-bit resolution: (100mV * NANO) / 2^16
> +				 * 12-bit resolution: (100mV * NANO) / 2^12
> +				 */
> +				u64 ref = info->features->has_16bit_resolution ?
> +						  1525878906250ULL : 24414062500000ULL;

Split definition and assignment.

> +				if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
> +					ref = ref << 1;

					ref *= 2;

> +				ref = div_u64(ref, shunt);
> +				*val2 = do_div(ref, 1 * NANO);
> +				*val = ref;
> +
> +				return IIO_VAL_INT_PLUS_NANO;
> +			}
> +
> +			/*
> +			 * Power - uW - it will use the combined scale
> +			 * for current and voltage
> +			 * current(mA) * voltage(mV) = power (uW)
> +			 */
> +			tmp = info->features->voltage_pv_fsr;
> +			do_div(tmp, shunt);
> +
> +			if (chan->type == IIO_ENERGY)
> +				*val2 = info->features->has_16bit_resolution ? 32 : 24;
> +			else
> +				*val2 = 32;
> +
> +			if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
> +				*val2 -= 1;
> +
> +			if (info->vbus_mode == PAC1711_FULL_RANGE_BIPOLAR)
> +				*val2 -= 1;
> +
> +			shift = fls64(tmp) - 31;
> +			if (shift > 0) {
> +				tmp >>= shift;
> +				*val2 -= shift;
> +			}
> +
> +			*val = (int)tmp;

Why casting?

> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}

...

Basically the last 2/3rd I sparsely checked. It's unbelievably that v5 of
the series has tons of small style issues. Also the driver length is too big,
can you split this to a few, smaller ones?

-- 
With Best Regards,
Andy Shevchenko