Jonathan Cameron wrote:
> On Wed, 24 Jul 2024 11:08:33 +0200
> Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote:
>
> > Add support for Microchip PAC1921 Power/Current monitor.
> >
> > Implemented features:
> > * capture of bus voltage, sense voltage, current and power measurements
> > in free-run integration mode
> > * support for both raw and triggered buffer reading
> > * support for overflow events
> > * scale attributes to control voltage and current gains
> > * oversampling ratio attribute to control the number of integration
> > samples
> > * sampling rate attribute that reflects the integration period
> > * userspace attribute and DT parameter to control shunt resistor
> > * simple power management support
> >
> > Limitations:
> > * operation mode fixed to free-run integration
> > * READ/INT pin and OUT pin not supported
> > * no controls for measurement resolutions and filters
> >
> > Signed-off-by: Matteo Martelli <matteomartelli3@xxxxxxxxx>
> I had a few more bits of feedback + one change was necessary because of
> this crossing with Nuno's series making iio_dev->masklength private.
> Rather than go around again for such trivial things,
> I've applied it to the testing branch of iio.git with the following diff.
> Note I'll rebase that tree on rc1 once available at which point it'll become
> the togreg branch and get picked up by linux-next etc.
>
> There are a few things inline that I commented on but didn't touch, so
> please also take a look at those and shout if I messed anything up!
> I've been known to make trivial changes that break a driver completely :(
>
> Thanks,
>
> Jonathan
>
Thanks Jonathan,
I reviewed your diff and also tested this version on the HW. All looks good to
me. Please check also my comments below.
>
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index a21dd772467e..d04c6685d780 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -75,12 +75,12 @@ enum pac1921_mxsl {
> * Vbus scale (mV) = max_vbus (mV) / dv_gain / resolution
> */
> static const int pac1921_vbus_scales[][2] = {
> - {31, 280547409}, /* dv_gain x1 */
> - {15, 640273704}, /* dv_gain x2 */
> - {7, 820136852}, /* dv_gain x4 */
> - {3, 910068426}, /* dv_gain x8 */
> - {1, 955034213}, /* dv_gain x16 */
> - {0, 977517106} /* dv_gain x32 */
> + { 31, 280547409 }, /* dv_gain x1 */
> + { 15, 640273704 }, /* dv_gain x2 */
> + { 7, 820136852 }, /* dv_gain x4 */
> + { 3, 910068426 }, /* dv_gain x8 */
> + { 1, 955034213 }, /* dv_gain x16 */
> + { 0, 977517106 }, /* dv_gain x32 */
> };
>
> /*
> @@ -91,14 +91,14 @@ static const int pac1921_vbus_scales[][2] = {
> * Vsense scale (mV) = max_vsense (mV) / di_gain / resolution
> */
> static const int pac1921_vsense_scales[][2] = {
> - {0, 97751710}, /* di_gain x1 */
> - {0, 48875855}, /* di_gain x2 */
> - {0, 24437927}, /* di_gain x4 */
> - {0, 12218963}, /* di_gain x8 */
> - {0, 6109481}, /* di_gain x16 */
> - {0, 3054740}, /* di_gain x32 */
> - {0, 1527370}, /* di_gain x64 */
> - {0, 763685} /* di_gain x128 */
> + { 0, 97751710 }, /* di_gain x1 */
> + { 0, 48875855 }, /* di_gain x2 */
> + { 0, 24437927 }, /* di_gain x4 */
> + { 0, 12218963 }, /* di_gain x8 */
> + { 0, 6109481 }, /* di_gain x16 */
> + { 0, 3054740 }, /* di_gain x32 */
> + { 0, 1527370 }, /* di_gain x64 */
> + { 0, 763685 }, /* di_gain x128 */
> };
>
> /*
> @@ -334,7 +334,7 @@ static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg,
> if (ret)
> return ret;
>
> - *val = (u16)FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val));
> + *val = FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val));
>
> return 0;
> }
> @@ -612,7 +612,7 @@ static int pac1921_update_int_num_samples(struct pac1921_priv *priv,
> if (priv->n_samples == n_samples)
> return 0;
>
> - reg_val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples);
> + reg_val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples);
>
> ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG,
> PAC1921_INT_CFG_SMPL_MASK, reg_val);
> @@ -1017,7 +1017,7 @@ static irqreturn_t pac1921_trigger_handler(int irq, void *p)
> if (ret)
> goto done;
>
> - for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) {
> + iio_for_each_active_channel(idev, bit) {
> u16 val;
>
> ret = pac1921_read_res(priv, idev->channels[ch].address, &val);
> @@ -1054,8 +1054,8 @@ static int pac1921_init(struct pac1921_priv *priv)
> return ret;
>
> /* Configure gains, use 14-bits measurement resolution (HW default) */
> - val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) |
> - (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain);
> + val = FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) |
> + FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain);
> ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val);
> if (ret)
> return ret;
> @@ -1067,7 +1067,7 @@ static int pac1921_init(struct pac1921_priv *priv)
> * - set READ/INT pin override (RIOV) to control operation mode via
> * register instead of pin
> */
> - val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) |
> + val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) |
> PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN |
> PAC1921_INT_CFG_RIOV;
> ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val);
> @@ -1080,8 +1080,8 @@ static int pac1921_init(struct pac1921_priv *priv)
> * - OUT pin full scale range: 3V (HW detault)
> * - no timeout, no sleep, no sleep override, no recalc (HW defaults)
> */
> - val = (u8)FIELD_PREP(PAC1921_CONTROL_MXSL_MASK,
> - PAC1921_MXSL_VPOWER_FREE_RUN);
> + val = FIELD_PREP(PAC1921_CONTROL_MXSL_MASK,
> + PAC1921_MXSL_VPOWER_FREE_RUN);
> ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val);
> if (ret)
> return ret;
> @@ -1153,7 +1153,6 @@ static void pac1921_regulator_disable(void *data)
>
> static int pac1921_probe(struct i2c_client *client)
> {
> - const struct i2c_device_id *id = i2c_client_get_device_id(client);
> struct device *dev = &client->dev;
> struct pac1921_priv *priv;
> struct iio_dev *indio_dev;
> @@ -1202,8 +1201,7 @@ static int pac1921_probe(struct i2c_client *client)
> ret = devm_add_action_or_reset(dev, pac1921_regulator_disable,
> priv->vdd);
> if (ret)
> - return dev_err_probe(
> - dev, ret,
> + return dev_err_probe(dev, ret,
> "Cannot add action for vdd regulator disposal\n");
>
> msleep(PAC1921_POWERUP_TIME_MS);
> @@ -1214,7 +1212,7 @@ static int pac1921_probe(struct i2c_client *client)
>
> priv->iio_info = pac1921_iio;
>
> - indio_dev->name = id->name;
> + indio_dev->name = "pac1921";
> indio_dev->info = &priv->iio_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = pac1921_channels;
>
>
> > +
> > +/*
> > + * Emit on sysfs the list of available scales contained in scales_tbl
> > + *
> > + * TODO:: this function can be replaced with iio_format_avail_list() if the
> > + * latter will ever be exported.
>
> You could just have added a precursor patch doing that.
> If you have time I'd certainly consider a patch that does export that function
> and uses it here.
>
I wasn't sure that one usage was enough to justify the export. I could
definitely do it, I am assuming it would now go to a new patch series since
this has already been merged into testing, right?
> > + *
> > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for
> > + * the current scales table)
> > + */
> > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2],
> > + size_t size, char *buf)
> > +{
> > + ssize_t len = 0;
> > +
> > + for (unsigned int i = 0; i < size; i++) {
> > + if (i != 0) {
> > + len += sysfs_emit_at(buf, len, " ");
> > + if (len >= PAGE_SIZE)
> > + return -EFBIG;
> > + }
> > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0],
> > + scales_tbl[i][1]);
> > + if (len >= PAGE_SIZE)
> > + return -EFBIG;
> > + }
> > +
> > + len += sysfs_emit_at(buf, len, "\n");
> > + return len;
> > +}
> > +
> > +/*
> > + * Read available scales for a specific channel
> > + *
> > + * NOTE: using extended info insted of iio.read_avail() because access to
> > + * current scales must be locked as they depend on shunt resistor which may
> > + * change runtime. Caller of iio.read_avail() would access the table unlocked
> > + * instead.
>
> That's a corner case we should think about closing. Would require an indicator
> to read_avail that the buffer it has been passed is a snapshot that it should
> free on completion of the string building. I don't like passing ownership
> of data around like that, but it is fiddly to do anything else given
> any simple double buffering is subject to race conditions.
>
If I understand your suggestion the driver would allocate a new table and copy
the values into it at each read_avail() call. Then
iio_read_channel_info_avail() would free the buffer if some sort of
free-after-use indicator flag is set. I guess such indicator might be set via an
additional read_avail function argument (would be an extensive API change) or
maybe via a new iio_chan_spec attribute.
> An alternative would use a key of sometype to associate individual read_avail
> calls with new ones to read_avail_release_resource. That might be cleaner.
>
Are you referring to introduce a new read_avail_realease_resource callback that
would be called at the end of iio_read_channel_info_avail() if set? Similarly
to the previous point the driver would allocate a new table and copy the values
into it at each read_avail() call, but the driver would also define a release
callback to free such table. If otherwise you are referring to something less
trivial, is there a similar API in the kernel that can be referred to for
clarity?
> oh well, a cleanup job for another day. I suspect we have drivers today
> that are subject to tearing of their available lists.
>
I've just taken a quick look at the other drivers and the following twos seem
to have the race condition issue since they are updating an available table
during a write_raw() call and also exposing it during a read_avail() call:
* drivers/iio/light/as73211.c: see int_time_avail table
* drivers/iio/adc/ad7192.c: see filter_freq_avail table
There might be others, I've only looked into those that seemed likely to have
this issue after some trivial greps.
Is there already a common way for iio to keep track of open issues (e.g. Issue
tracker/TODO lists/etc)?
> > + */
> > +static ssize_t pac1921_read_scale_avail(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
> > +{
> > + struct pac1921_priv *priv = iio_priv(indio_dev);
> > + const int (*scales_tbl)[2];
> > + size_t size;
> > +
> > + switch (chan->channel) {
> > + case PAC1921_CHAN_VBUS:
> > + scales_tbl = pac1921_vbus_scales;
> > + size = ARRAY_SIZE(pac1921_vbus_scales);
> > + return pac1921_format_scale_avail(scales_tbl, size, buf);
> > +
> > + case PAC1921_CHAN_VSENSE:
> > + scales_tbl = pac1921_vsense_scales;
> > + size = ARRAY_SIZE(pac1921_vsense_scales);
> > + return pac1921_format_scale_avail(scales_tbl, size, buf);
> > +
> > + case PAC1921_CHAN_CURRENT: {
> > + guard(mutex)(&priv->lock);
> > + scales_tbl = priv->current_scales;
> > + size = ARRAY_SIZE(priv->current_scales);
> > + return pac1921_format_scale_avail(scales_tbl, size, buf);
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +#define PAC1921_EXT_INFO_SCALE_AVAIL { \
> > + .name = "scale_available", \
> > + .read = pac1921_read_scale_avail, \
> > + .shared = IIO_SEPARATE, \
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info pac1921_ext_info_voltage[] = {
> > + PAC1921_EXT_INFO_SCALE_AVAIL,
> > + {}
> > +};
>
>
> > +
> > +static irqreturn_t pac1921_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *idev = pf->indio_dev;
> > + struct pac1921_priv *priv = iio_priv(idev);
> > + int ret;
> > + int bit;
> > + int ch = 0;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + if (!pac1921_data_ready(priv))
>
> Interesting corner case that maybe could have done with a comment.
> Seems could be triggered by a spurious interrupt, or sampling too early.
>
> I think only the second one is likely to happen, so shouldn't be a
> problem to acknowledge that trigger.
>
Yes, my intent here was to prevent userspace from receiving invalid data if
sampled too early: for example the user could arm a timer that would trigger an
interrupt before the first integration is complete. This could happen not just
after the first driver initialization phase but also after a configuration
change (gains or number of integration samples reflecting a user change of
scale or oversampling_ratio respectively).
> > + goto done;
> > +
> > + ret = pac1921_check_push_overflow(idev, pf->timestamp);
> > + if (ret)
> > + goto done;
> > +
> > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) {
>
> This needs an update as we crossed with Nuno's series that removes access
> to masklength. I can fix whilst applying by using
> iio_for_each_active_channel()
>
>
> > + u16 val;
> > +
> > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val);
> > + if (ret)
> > + goto done;
> > +
> > + priv->scan.chan[ch++] = val;
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp);
> > +
> > +done:
> > + iio_trigger_notify_done(idev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Initialize device by writing initial configuration and putting it into
> > + * integration state.
> > + *
> > + * Must be called with lock held when called after first initialization
> > + * (e.g. from pm resume)
> > + */
> > +static int pac1921_init(struct pac1921_priv *priv)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + /* Enter READ state before configuration */
> > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
> > + PAC1921_INT_CFG_INTEN, 0);
> > + if (ret)
> > + return ret;
> > +
> > + /* Configure gains, use 14-bits measurement resolution (HW default) */
> > + val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) |
> > + (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain);
>
> Why are these cases needed?
> Each of those values is going to fit in a u8 just fine and it's getting
> written to a much larger variable.
>
FIELD_PREP result type would be a long unsigned int due to the GENMASK type and
-Wconversion would trigger a warning. The explicit casts is just to address
-Wconversion warnings and to "state" that such casts are safe. In this way
with -Wconversion (KBUILD_EXTRA_WARN=3) one could easily spot those other
implicit casts that would end up with unwanted data corruption. I thought it to
be a common practice and I also saw it in some other kernel patches, for
example https://lore.kernel.org/all/1540883612.2354.2.camel@xxxxxxxxxxxx/ , but
maybe it's not that common as I thought.
I also see that maybe in this case casting to unsigned int would have likely
been more clear.
Thanks,
Matteo Martelli
