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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] iio: adc: pac1711: wrong power and energy scale units in pac1711_read_raw()
- [High] iio: adc: pac1711: missing vdd-supply regulator enable in pac1711_probe()
- [Medium] iio: adc: pac1711: in_shunt_resistor uses micro-ohms instead of Ohms
- [Medium] iio: adc: pac1711: signed int overflow of shunt in pac1711_read_raw()
- [Medium] iio: adc: pac1711: premature jiffies_tstamp update in pac1711_send_refresh()
- [Low] iio: adc: pac1711: premature sample_rate_idx update in pac1711_write_raw()
--

commit c4f8dfb4bdee1e8ce1711a47ff2fad6880c4a9a3
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: adc: add support for PAC1711

This commit adds an IIO ADC driver for the Microchip PAC1711, PAC1721,
PAC1811, and PAC1821 single-channel power and current monitors with hardware
accumulators.

> diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
> new file mode 100644
> index 0000000000000..6adc37a7ba0be
> --- /dev/null
> +++ b/drivers/iio/adc/pac1711.c
> @@ -0,0 +1,1233 @@

[ ... ]

> +/* 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)

[ ... ]

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

[ ... ]

> +static int pac1711_reg_snapshot(struct pac1711_chip_info *info, bool *refreshed)
> +{
> +	u32 wait_time;
> +	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];
> +
> +	/* A hardware refresh will happen */
> +	ret = pac1711_reg_snapshot_locked(info, wait_time);
> +	if (!ret && refreshed)
> +		*refreshed = true;
> +
> +	return ret;
> +}

[ ... ]

> +static ssize_t pac1711_read_shunt_resistor(struct iio_dev *indio_dev, uintptr_t private,
> +					   const struct iio_chan_spec *ch, char *buf)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +
> +	guard(mutex)(&info->lock);
> +
> +	return sysfs_emit(buf, "%u\n", info->shunt);
> +}
> +
> +static int pac1711_set_acc_enable(struct pac1711_chip_info *info, int val)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	u32 wait_time, sample_rate;
> +	int ret = 0;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	guard(mutex)(&info->lock);
> +
> +	if (val == info->enable_acc)
> +		return 0;
> +
> +	/*
> +	 * Force a hardware refresh to zero out the internal accumulators
> +	 * and start a new measurement window, as the chip accumulates
> +	 * continuously in the background.
> +	 */
> +	if (val) {
> +		sample_rate = pac1711_samp_rate_map_tbl[info->sample_rate_idx];
> +		wait_time = (1024 * 1000) / sample_rate;
> +		ret = pac1711_send_refresh(info, wait_time);
> +	} else {
> +		info->chip_reg_data.acc_val = 0;
> +		info->chip_reg_data.vacc = 0;
> +		info->chip_reg_data.acc_count = 0;
> +	}

[Severity: Medium]
Should info->chip_reg_data.jiffies_tstamp be updated only after
pac1711_reg_snapshot_locked() has finished reading the measurement registers,
rather than inside pac1711_send_refresh()?

Currently, pac1711_send_refresh() sets info->chip_reg_data.jiffies_tstamp =
jiffies before sleeping for wait_time and before any registers are read into
info->chip_reg_data.

When pac1711_set_acc_enable() re-enables accumulation, it calls
pac1711_send_refresh() without reading the measurement registers into
info->chip_reg_data. If userspace reads in_voltage0_raw, in_current0_raw, or
in_power0_raw within 50 ms (PAC1711_MIN_POLLING_TIME_MS),
pac1711_reg_snapshot() sees the updated timestamp and returns early with stale
cached values. The same issue occurs if an I2C block read fails in
pac1711_reg_snapshot_locked() after pac1711_send_refresh() succeeds.

Conversely, at 8 SPS where wait_time is 128 ms, recording jiffies_tstamp
before fsleep(wait_time) means the 50 ms cache window has already expired by
the time pac1711_send_refresh() returns.

> +
> +	if (!ret)
> +		info->enable_acc = val;
> +
> +	if (ret)
> +		dev_err(dev, "failed to reset accumulator: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +/*
> + * The value of the shunt resistor may be known only at runtime and set by a client
> + * application. This attribute allows to set its value in micro-ohms. Y is the channel
> + * number. The value is used to calculate current, power and accumulated energy or
> + * Coulomb counter.
> + */
> +static ssize_t pac1711_write_shunt_resistor(struct iio_dev *indio_dev, uintptr_t private,
> +					    const struct iio_chan_spec *ch, const char *buf,
> +					    size_t len)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	struct device *dev = &info->client->dev;
> +	unsigned int sh_val;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &sh_val);
> +	if (ret) {
> +		dev_err(dev, "Shunt value is not valid\n");
> +		return ret;
> +	}
> +
> +	if (sh_val == 0)
> +		return -EINVAL;
> +
> +	guard(mutex)(&info->lock);
> +
> +	info->shunt = sh_val;
> +
> +	return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info pac1711_ext_info[] = {
> +	{
> +		.name = "in_shunt_resistor",
> +		.read = pac1711_read_shunt_resistor,
> +		.write = pac1711_write_shunt_resistor,
> +		.shared = IIO_SHARED_BY_ALL,
> +	},
> +	{ }
> +};

[Severity: Medium]
Does exposing in_shunt_resistor in micro-ohms violate the IIO sysfs ABI?

Documentation/ABI/testing/sysfs-bus-iio specifies that in_shunt_resistor is
expressed in Ohms. Here, pac1711_read_shunt_resistor() emits info->shunt
directly in micro-ohms via sysfs_emit(), and pac1711_write_shunt_resistor()
parses the input as an integer in micro-ohms via kstrtouint().

Should this format the value in Ohms using iio_format_value() with
IIO_VAL_FRACTIONAL (or IIO_VAL_INT_PLUS_MICRO) and parse writes with
iio_str_to_fixpoint(), matching other power monitor drivers such as pac1921?

[ ... ]

> +	case IIO_CHAN_INFO_SCALE: {
> +		u32 shunt;
> +
> +		guard(mutex)(&info->lock);
> +		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;
> +
> +			return IIO_VAL_FRACTIONAL;

[Severity: Medium]
Can *val2 overflow to a negative value in pac1711_read_raw() when info->shunt
exceeds INT_MAX?

Both pac1711_parse_fw() and pac1711_write_shunt_resistor() accept 32-bit
unsigned values up to U32_MAX into info->shunt, only rejecting zero. When
shunt > INT_MAX, assigning it to the signed int *val2 for IIO_VAL_FRACTIONAL
wraps *val2 negative, causing iio_format_value() to output a negative current
scale. Should shunt values greater than INT_MAX be rejected?

> +		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;
> +
> +				if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
> +					ref = ref << 1;
> +
> +				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;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;

[Severity: High]
Are the scale units for IIO_POWER and IIO_ENERGY in pac1711_read_raw() off by
factors of 1,000 and 1,000,000 respectively?

According to Documentation/ABI/testing/sysfs-bus-iio, power channels
(in_powerY_raw * in_powerY_scale) must be reported in milliwatts (mW), and
energy channels (in_energy_raw * in_energy_scale) must be reported in Joules
(J).

However, PAC1711_PRODUCT_VOLTAGE_PV_FSR (4200ULL * NANO = 4.2 * 10^12) and
PAC1721_PRODUCT_VOLTAGE_PV_FSR (900ULL * NANO = 9 * 10^11) compute the
numerator in microwatts * micro-ohms, as noted in the comment above. Dividing
by shunt (in micro-ohms) and returning IIO_VAL_FRACTIONAL_LOG2 with *val2 = 32
(or 24) yields a scale in microwatts for IIO_POWER (1,000x too large for mW)
and microjoules for IIO_ENERGY (1,000,000x too large for Joules).

[ ... ]

> +static int pac1711_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &info->client->dev;
> +	int new_idx, ret, refresh_time;
> +	s32 old_samp_rate;
> +	__be16 val_be16;
> +	u32 wait_time;
> +	u16 val_u16;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		scoped_guard(mutex, &info->lock) {
> +			old_samp_rate = pac1711_samp_rate_map_tbl[info->sample_rate_idx];
> +			new_idx = pac1711_get_samp_rate_idx(val);
> +			if (new_idx < 0)
> +				return new_idx;
> +
> +			ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_ACT_REG_ADDR,
> +							    sizeof(val_u16), (u8 *)&val_be16);
> +			if (ret != sizeof(val_u16)) {
> +				dev_err(&client->dev, "cannot read regs from 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret < 0 ? ret : -EIO;
> +			}
> +
> +			val_u16 = be16_to_cpu(val_be16);
> +			FIELD_MODIFY(PAC1711_CTRL_SAMPLE_MODE_MASK, &val_u16, new_idx);
> +			val_be16 = cpu_to_be16(val_u16);
> +			ret = i2c_smbus_write_i2c_block_data(client, PAC1711_CTRL_REG_ADDR,
> +							     sizeof(val_be16), (u8 *)&val_be16);
> +			if (ret) {
> +				dev_err(dev, "Failed to configure sampling mode in 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret;
> +			}
> +
> +			info->sample_rate_idx = new_idx;
> +			info->chip_reg_data.ctrl_act_reg = val_u16;
> +
> +			/* Force register snapshot and timestamp update with a refresh. */
> +			refresh_time = msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS) + 1;
> +			info->chip_reg_data.jiffies_tstamp = jiffies - refresh_time;
> +			wait_time = (1024 * 1000) / old_samp_rate;
> +
> +			/*
> +			 * We must wait for cycle completion before updating the
> +			 * sample rate value.
> +			 */
> +			ret = pac1711_reg_snapshot_locked(info, wait_time);
> +			if (ret) {
> +				dev_err(dev, "%s - cannot snapshot ctrl and measurement regs\n",
> +					__func__);
> +				return ret;
> +			}
> +		}

[Severity: Low]
In pac1711_write_raw(), info->sample_rate_idx is updated to new_idx before
pac1711_reg_snapshot_locked() sends the refresh command that activates the
new control register settings. If pac1711_reg_snapshot_locked() fails to send
the refresh command, won't the chip remain at old_samp_rate while
info->sample_rate_idx reports new_idx?

Also, in the error path of the i2c_smbus_write_i2c_block_data() call above,
dev_err() logs PAC1711_CTRL_ACT_REG_ADDR instead of PAC1711_CTRL_REG_ADDR.

[ ... ]

> +static int pac1711_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct pac1711_chip_info *info;
> +	struct iio_dev *indio_dev;
> +	u32 wait_time;
> +	int ret, err;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +	info->client = client;
> +
> +	ret = pac1711_chip_identify(indio_dev, info);

[Severity: High]
Does pac1711_probe() need to enable the vdd-supply regulator before accessing
the chip over I2C?

Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml requires
vdd-supply, but pac1711_probe() never calls devm_regulator_get_enable(dev,
"vdd") before calling pac1711_chip_identify() and pac1711_chip_configure(). If
the regulator is not already enabled by the bootloader, I2C communication will
fail during probe, or the regulator core may disable the unclaimed supply at
late_initcall while the device is in use.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-pac1711-v5-0-71c20101a52e@microchip.com?part=5