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

[Jonathan Cameron]

On Tue, 28 Jul 2026 15:03:49 +0300
Ariana Lazar <ariana.lazar@xxxxxxxxxxxxx> 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.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@xxxxxxxxxxxxx>

A few trivial things from me that I don't think overlapped with existing review
comments.

>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
> diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ee6adee31524e73371450d04bd501c545bd682b6
> --- /dev/null
> +++ b/drivers/iio/adc/pac1711.c
> @@ -0,0 +1,1274 @@



> +
> +static int pac1711_retrieve_data(struct pac1711_chip_info *info, u32 wait_time)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Check if the minimal elapsed time has passed and if so,
> +	 * read again the chip, otherwise use the cached info.
> +	 */
> +	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
> +			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
> +		ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
> +					   wait_time);
> +
> +		/*
> +		 * Re-schedule the work for the read registers timeout
> +		 * (to prevent chip regs saturation)
> +		 */
> +		cancel_delayed_work_sync(&info->work_chip_rfsh);
> +		schedule_delayed_work(&info->work_chip_rfsh,
> +				      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
		return ret in here, possibly bring ret into narrow scope.
> +	}
> +
> +	return ret;

return 0 out here

> +}

> +
> +static ssize_t pac1711_in_enable_acc_store(struct device *dev, struct device_attribute *attr,
> +					   const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &info->lock) {

Where it doesn't make any difference I'd generally use guard() rather
than scoped_guard()


> +		info->enable_acc = val;
> +		if (!val) {
> +			info->chip_reg_data.acc_val = 0;
> +			info->chip_reg_data.total_samples_nr = 0;
> +		}
> +	}
> +
> +	return count;
> +}

...

> +static struct attribute *pac1711_power_acc_attr[] = {
> +	PAC1711_DEV_ATTR(in_energy_raw),
> +	PAC1711_DEV_ATTR(in_energy_scale),
> +	PAC1711_DEV_ATTR(in_energy_en),
> +	NULL,
> +};
> +
> +static struct attribute *pac1711_coulomb_counter_attr[] = {
> +	PAC1711_DEV_ATTR(in_coulomb_counter_raw),
> +	PAC1711_DEV_ATTR(in_coulomb_counter_scale),
> +	PAC1711_DEV_ATTR(in_coulomb_counter_en),
> +	NULL,

No comma on final entries if they are intended to ensure nothing comes
after that point.

> +};



> +
> +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;
> +	s32 old_samp_rate;
> +	int new_idx, ret;
> +	__be16 tmp_be16;
> +	u16 tmp_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(tmp_u16), (u8 *)&tmp_be16);
> +			if (ret < 0) {
> +				dev_err(&client->dev, "cannot read regs from 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret;
> +			}
> +
> +			tmp_u16 = be16_to_cpu(tmp_be16);
> +			tmp_u16 &= ~PAC1711_CTRL_SAMPLE_MODE_MASK;
> +			tmp_u16 |= FIELD_PREP(PAC1711_CTRL_SAMPLE_MODE_MASK, new_idx);

Use FIELD_MODIFY().

> +			tmp_be16 = cpu_to_be16(tmp_u16);
> +
> +			ret = i2c_smbus_write_word_data(client, PAC1711_CTRL_REG_ADDR, tmp_be16);
> +			if (ret < 0) {
> +				dev_err(&client->dev, "Failed to configure sampling mode\n");
> +				return ret;
> +			}
> +
> +			info->sample_rate_idx = new_idx;
> +			info->chip_reg_data.ctrl_act_reg = tmp_u16;
> +		}
> +
> +		/* Force register snapshot and timestamp update with a refresh. */
> +		info->chip_reg_data.jiffies_tstamp -= msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS);
> +		ret = pac1711_retrieve_data(info, (1024 / old_samp_rate) * 1000);
> +		if (ret) {
> +			dev_err(dev, "%s - cannot snapshot ctrl and measurement regs\n", __func__);
> +			return ret;
> +		}
> +
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static const struct i2c_device_id pac1711_id[] = {
> +	{ .name = "pac1711", .driver_data = (kernel_ulong_t)&pac1711_chip_features },
> +	{ .name = "pac1721", .driver_data = (kernel_ulong_t)&pac1721_chip_features },
> +	{ .name = "pac1811", .driver_data = (kernel_ulong_t)&pac1811_chip_features },
> +	{ .name = "pac1821", .driver_data = (kernel_ulong_t)&pac1821_chip_features },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pac1711_id);
> +
> +static const struct of_device_id pac1711_of_match[] = {
> +	{
> +		.compatible = "microchip,pac1711",
> +		.data = &pac1711_chip_features

trailing comma should be there as we might add other things in future.
Also, why for the i2c_device_id table did you decided to keep them on one line
but in these entrees which are shorter, you have broken them out across multiple lines?
	{ .compatible = "microchip,pac1711", .compatible = "microchip,pac1711" },

I'm fine with both styles, but not inconsistency.

> +	},
> +	{
> +		.compatible = "microchip,pac1721",
> +		.data = &pac1721_chip_features
> +	},
> +	{
> +		.compatible = "microchip,pac1811",
> +		.data = &pac1811_chip_features
> +	},
> +	{
> +		.compatible = "microchip,pac1821",
> +		.data = &pac1821_chip_features
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pac1711_of_match);

>