Re: [PATCH v3 2/2] iio: adc: add support for PAC1711
From: Andy Shevchenko
Date: Wed Sep 09 2026 - 10:43:11 EST
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