| Message ID | 20240704-iio-pac1921-v2-2-0deb95a48409@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: add support for pac1921 | expand |
On Thu, 04 Jul 2024 19:42:02 +0200 Matteo Martelli <matteomartelli3@gmail.com> 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 > * userspace controls for voltage and current gains, measurement > resolution, integration samples and filters enabling/disabling > * simple power management support > > Limitations: > * operation mode fixed to free-run integration > * measurement resolution and filters controls are applied to both VSENSE > and VBUS measurements > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> Hi Matteo, A little too fast for sending a new version. Result is Marius reviewed v1. I may replicate some of Marius' comments but do make sure you cover them all for v3. One big question I have here, is do typical usecases for this device care much about reducing integration time? I'd have thought they were all relatively slow. As such it may make sense to not support some of the modes that need to lower integration time (this is a common thing to decide to skip in the interests of maintainability and user interface complexity reduction). Looks reasonably speedy to me, unless very high numbers of samples are used. So that control makes sense to expose, perhaps not the resolution one. Note that custom ABI is almost always a bad idea. We enable it for the cases where there is no other choice, but reality is those cases then all need custom userspace software, so the controls are much less likely to actually used :( Jonathan > --- > .../ABI/testing/sysfs-bus-iio-adc-pac1921 | 45 + > MAINTAINERS | 7 + > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/pac1921.c | 1038 ++++++++++++++++++++ > 5 files changed, 1101 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > new file mode 100644 > index 000000000000..4a32e2d4207b > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 Quite a bit of custom ABI in here. Rule of thumb is that custom ABI is more or less pointless ABI for 99% of users because standard userspace won't use it. So keep that in mind when defining it. > @@ -0,0 +1,45 @@ > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits > +KernelVersion: 6.10 > +Contact: linux-iio@vger.kernel.org > +Description: > + ADC measurement resolution. Can be either 11 bits or 14 bits > + (default). The driver sets the same resolution for both VBUS and > + VSENSE measurements even if the hardware could be configured to > + measure VBUS and VSENSE with different resolutions. > + This attribute affects the integration time: with 14 bits > + resolution the integration time is increased by a factor of > + 1.9 (the driver considers a factor of 2). See Table 4-5 in > + device datasheet for details. Is the integration time ever high enough that it matters? People tend not to do power measurement 'quickly'. If we are doing it quickly then you'll probably want to be providing buffered support and that does allow you to 'read' the resolution for a part where it changes for some other reason. I haven't yet understood this case. > + > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > +KernelVersion: 6.10 > +Contact: linux-iio@vger.kernel.org > +Description: > + List all possible ADC measurement resolutions: "11 14" > + > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples > +KernelVersion: 6.10 > +Contact: linux-iio@vger.kernel.org > +Description: > + Number of samples taken during a full integration period. Can be > + set to any power of 2 value from 1 (default) to 2048. > + This attribute affects the integration time: higher the number > + of samples, longer the integration time. See Table 4-5 in device > + datasheet for details. Sounds like oversampling_ratio which is standards ABI. So use that or explain why you can't here. > + > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples_available > +KernelVersion: 6.10 > +Contact: linux-iio@vger.kernel.org > +Description: > + List all possible numbers of integration samples: > + "1 2 4 8 16 32 64 128 256 512 1024 2048" > + > +What: /sys/bus/iio/devices/iio:devices/filters_en > +KernelVersion: 6.10 > +Contact: linux-iio@vger.kernel.org > +Description: > + Attribute to enable/disable ADC post filters. Enabled by > + default. > + This attribute affects the integration time: with filters > + enabled the integration time is increased by 50%. See Table 4-5 > + in device datasheet for details. Do we have any idea what this filter is? Datasheet seems very vague indeed and from a control point of view that makes this largely useless. How does userspace know whether to turn it on? We have an existing filter ABI but with so little information no way to fit this in. Gut feeling, leave it on all the time and drop the control interface. > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index f60fe85a30d5..b56e494da970 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -991,6 +991,16 @@ config NPCM_ADC > This driver can also be built as a module. If so, the module > will be called npcm_adc. > > +config PAC1921 > + tristate "Microchip Technology PAC1921 driver" > + depends on I2C Needs to ensure REGMAP_I2C as well I think. Check similar cases. > + help > + Say yes here to build support for Microchip Technology's PAC1921 > + High-Side Power/Current Monitor with Analog Output. > + > + This driver can also be built as a module. If so, the module > + will be called pac1921. > + > config PAC1934 > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c > new file mode 100644 > index 000000000000..879753466093 > --- /dev/null > +++ b/drivers/iio/adc/pac1921.c > @@ -0,0 +1,1038 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * IIO driver for PAC1921 High-Side Power/Current Monitor > + * > + * Copyright (C) 2024 Matteo Martelli <matteomartelli3@gmail.com> > + */ > + > +#include <linux/i2c.h> > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/regmap.h> > + > +/* pac1921 registers */ > +#define PAC1921_REG_GAIN_CFG 0x00 > +#define PAC1921_REG_INT_CFG 0x01 > +#define PAC1921_REG_CONTROL 0x02 > +#define PAC1921_REG_VBUS 0x10 > +#define PAC1921_REG_VSENSE 0x12 > +#define PAC1921_REG_OVERFLOW_STS 0x1C > +#define PAC1921_REG_VPOWER 0x1D > + > +/* pac1921 gain configuration bits */ > +#define PAC1921_GAIN_I_RES BIT(7) > +#define PAC1921_GAIN_V_RES BIT(6) > +#define PAC1921_GAIN_DI_GAIN_SHIFT 3 > +#define PAC1921_GAIN_DI_GAIN_MASK GENMASK(5, PAC1921_GAIN_DI_GAIN_SHIFT) > +#define PAC1921_GAIN_DI_GAIN_MAX 7 > +#define PAC1921_GAIN_DV_GAIN_SHIFT 0 > +#define PAC1921_GAIN_DV_GAIN_MASK GENMASK(2, PAC1921_GAIN_DV_GAIN_SHIFT) Define only the MASKs not SHIFTs and use FIELD_GET(), FIELD_PREP() throughout. Gives more readable code in general as well as halving the number of defines. > +#define PAC1921_GAIN_DV_GAIN_MAX 5 > + > +/* pac1921 integration configuration bits */ > +#define PAC1921_INT_CFG_SMPL_SHIFT 4 > +#define PAC1921_INT_CFG_SMPL_MASK GENMASK(7, PAC1921_INT_CFG_SMPL_SHIFT) > +#define PAC1921_INT_CFG_SMPL_MAX 11 > +#define PAC1921_INT_CFG_VSFEN BIT(3) > +#define PAC1921_INT_CFG_VBFEN BIT(2) > +#define PAC1921_INT_CFG_RIOV BIT(1) > +#define PAC1921_INT_CFG_INTEN BIT(0) > + > +/* pac1921 control bits */ > +#define PAC1921_CONTROL_MXSL_SHIFT 6 > +enum pac1921_mxsl { > + PAC1921_MXSL_VPOWER_PIN = 0, > + PAC1921_MXSL_VSENSE_FREE_RUN = 1, > + PAC1921_MXSL_VBUS_FREE_RUN = 2, > + PAC1921_MXSL_VPOWER_FREE_RUN = 3, > +}; > +#define PAC1921_CONTROL_SLEEP BIT(2) > + > +/* pac1921 overflow status bits */ > +#define PAC1921_OVERFLOW_VSOV BIT(2) > +#define PAC1921_OVERFLOW_VBOV BIT(1) > +#define PAC1921_OVERFLOW_VPOV BIT(0) > + > +/* pac1921 constants */ > +#define PAC1921_MAX_VSENSE_MV 100 > +#define PAC1921_MAX_VBUS_V 32 > +#define PAC1921_RES_RESOLUTION 1023 /* Result registers resolution */ > + > +/* pac1921 defaults */ > +#define PAC1921_DEFAULT_DV_GAIN 0 /* 2^(value): 1x gain */ > +#define PAC1921_DEFAULT_DI_GAIN 0 /* 2^(value): 1x gain */ > +#define PAC1921_DEFAULT_HIGH_RES true /* 14-bit measurement resolution */ > +#define PAC1921_DEFAULT_NUM_SAMPLES 0 /* 2^(value): 1 sample */ > +#define PAC1921_DEFAULT_FILTERS_ENABLED true > + > +/* pac1921 tables to create iio available parameters */ > +static const unsigned int pac1921_di_gains[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > +static const unsigned int pac1921_dv_gains[] = { 1, 2, 4, 8, 16, 32 }; > +enum pac1921_meas_resolution_idx { > + PAC1921_MEAS_RESOLUTION_IDX_LOW = 0, > + PAC1921_MEAS_RESOLUTION_IDX_HIGH, > +}; > +static const char *const pac1921_meas_resolutions[] = { "11", "14" }; > +static const char *const pac1921_integr_num_samples[] = { > + "1", "2", "4", "8", "16", "32", > + "64", "128", "256", "512", "1024", "2048" > +}; > + > +/* pac1921 regmap configuration */ > +static const struct regmap_range pac1921_regmap_wr_ranges[] = { > + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), > +}; Trivial but I'd like a blank line here. > +static const struct regmap_access_table pac1921_regmap_wr_table = { > + .yes_ranges = pac1921_regmap_wr_ranges, > + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges), > +}; here > +static const struct regmap_range pac1921_regmap_rd_ranges[] = { > + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), > + regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1), > +}; here > +static const struct regmap_access_table pac1921_regmap_rd_table = { > + .yes_ranges = pac1921_regmap_rd_ranges, > + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges), > +}; and here as my eyes stuggle a bit with parsing this without the separations. Similar applies above. > +static const struct regmap_config pac1921_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .rd_table = &pac1921_regmap_rd_table, > + .wr_table = &pac1921_regmap_wr_table, > +}; > + > +/* Check if overflow occurred and if so, push the corresponding events. As mentioned below, without controls I don't think userspace has any way to know these are coming. It is useful even for overflow events to provide a) A way to mask them off. b) The threshold values. So that userspace can see that they are overflow events. These will be readonly of course. > + * > + * Must be called with lock held. > + */ > +static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp) > +{ > + struct pac1921_priv *priv = iio_priv(indio_dev); > + unsigned int flags; > + > + int ret = regmap_read(priv->regmap, PAC1921_REG_OVERFLOW_STS, &flags); > + if (ret) > + return ret; > + > + if (flags & PAC1921_OVERFLOW_VBOV && > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VBOV)) { > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE( > + IIO_VOLTAGE, PAC1921_CHAN_VBUS, > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > + timestamp); > + } > + if (flags & PAC1921_OVERFLOW_VSOV && > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VSOV)) { > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE( > + IIO_VOLTAGE, PAC1921_CHAN_VSENSE, > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > + timestamp); > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE( > + IIO_CURRENT, PAC1921_CHAN_CURRENT, > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > + timestamp); > + } > + if (flags & PAC1921_OVERFLOW_VPOV && > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VPOV)) { > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE( > + IIO_POWER, PAC1921_CHAN_POWER, > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > + timestamp); > + } > + > + priv->prev_ovf_flags = (u8)flags; > + > + return 0; > +} > + > +/* Read the value from a result register > + * > + * Result registers contain the most recent averaged values of Vbus, Vsense and > + * Vpower. Each value is 10 bits wide and spread across two consecutive 8 bit > + * registers, with 6 bit LSB zero padding. > + */ > +static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg, > + int *val) > +{ > + u8 val_buf[2]; > + > + int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, &val_buf, > + sizeof(val_buf)); > + if (ret) > + return ret; > + > + *val = (val_buf[0] << 8 | val_buf[1]) >> 6; Looks like it could be done with the self documenting combination of get_unaligned_be16() + FIELD_GET() or use a __b16 for the read and the be16_to_cpu() approach. > + > + return 0; > +} > + > +static int pac1921_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct pac1921_priv *priv = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: { > + guard(mutex)(&priv->lock); Given you grab this in all but error paths, I'd just grab it always and avoid need for the guard() and careful scoping in all these case blocks. > + > + if (!pac1921_data_ready(priv)) > + return -EBUSY; > + > + s64 ts = iio_get_time_ns(indio_dev); > + > + int ret = pac1921_check_push_overflow(indio_dev, ts); > + if (ret) > + return ret; > + > + ret = pac1921_read_res(priv, chan->address, val); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_SCALE: > + switch (chan->channel) { > + case PAC1921_CHAN_VBUS: { > + /* Vbus scale factor in mV: > + * max_vbus (mV) / vgain / resolution > + */ > + guard(mutex)(&priv->lock); > + > + *val = PAC1921_MAX_VBUS_V * 1000; > + *val2 = PAC1921_RES_RESOLUTION << (int)priv->dv_gain; > + > + return IIO_VAL_FRACTIONAL; > + } > + case PAC1921_CHAN_VSENSE: { > + /* Vsense voltage scale factor in mV: > + * max_vsense (mV) / igain / resolution > + */ > + guard(mutex)(&priv->lock); > + > + *val = PAC1921_MAX_VSENSE_MV; > + *val2 = PAC1921_RES_RESOLUTION << (int)priv->di_gain; > + > + return IIO_VAL_FRACTIONAL; > + } > + case PAC1921_CHAN_CURRENT: { > + /* Current scale factor in mA: > + * Vsense LSB (nV) / shunt (uOhm) > + */ > + guard(mutex)(&priv->lock); > + > + *val = pac1921_vsense_lsb(priv->di_gain); > + *val2 = (int)priv->rshunt; > + > + return IIO_VAL_FRACTIONAL; > + } > + case PAC1921_CHAN_POWER: { > + /* Power scale factor in mW: > + * Vsense LSB (nV) * max_vbus (V) / vgain / shunt (uOhm) > + */ > + guard(mutex)(&priv->lock); > + > + *val = pac1921_vsense_lsb(priv->di_gain) * > + (PAC1921_MAX_VBUS_V >> (int)priv->dv_gain); > + *val2 = (int)priv->rshunt; > + > + return IIO_VAL_FRACTIONAL; > + } > + default: > + return -EINVAL; > + } > + break; > + > + case IIO_CHAN_INFO_HARDWAREGAIN: > + switch (chan->channel) { > + case PAC1921_CHAN_VBUS: { > + guard(mutex)(&priv->lock); > + *val = 1 << (int)priv->dv_gain; > + return IIO_VAL_INT; > + } > + case PAC1921_CHAN_VSENSE: > + case PAC1921_CHAN_CURRENT: { > + guard(mutex)(&priv->lock); > + *val = 1 << (int)priv->di_gain; > + return IIO_VAL_INT; > + } > + default: > + return -EINVAL; > + } > + > + case IIO_CHAN_INFO_INT_TIME: { > + /* Integration time is read only: it depends on the number of > + * integration samples, measurement resolution and post filters > + */ > + *val2 = 1000000; /* From usecs to fractional secs */ > + guard(mutex)(&priv->lock); > + *val = (int)priv->integr_period; > + return IIO_VAL_FRACTIONAL; > + } > + default: > + return -EINVAL; > + } > +} > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, > + unsigned int mask, unsigned int val) > +{ > + /* Enter READ state before configuration */ > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_INTEN, 0); > + if (ret) > + return ret; > + > + /* Update configuration value */ > + ret = regmap_update_bits(priv->regmap, reg, mask, val); > + if (ret) > + return ret; > + > + /* Re-enable integration and reset start time */ > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > + if (ret) > + return ret; > + > + priv->integr_start_time = jiffies; Add a comment for why this value. > + priv->first_integr_done = false; Will default to this anyway, so you could skip it unless you feel this is useful from documentation point of view. > + > + return 0; > +} > + > > +static int pac1921_set_int_num_samples(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int val) > +{ > + struct pac1921_priv *priv = iio_priv(indio_dev); > + > + if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX)) > + return -EINVAL; > + > + guard(mutex)(&priv->lock); > + > + if (priv->n_samples == val) > + return 0; > + > + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_SMPL_MASK, > + val << PAC1921_INT_CFG_SMPL_SHIFT); FIELD_PREP() etc. > + if (ret) > + return ret; > + > + priv->n_samples = (u8)val; > + > + return pac1921_update_integr_period(priv); > +} > + > +static ssize_t pac1921_read_filters_enabled(struct iio_dev *indio_dev, > + uintptr_t private, > + const struct iio_chan_spec *chan, > + char *buf) > +{ > + struct pac1921_priv *priv = iio_priv(indio_dev); > + bool enabled; > + > + scoped_guard(mutex, &priv->lock) { > + enabled = priv->filters_en; > + } > + return sysfs_emit(buf, "%d\n", enabled); It's not a fast path hence holding the lock a little longer than necessary doesn't matter, so I'd do the simpler. guard(mutex)(&priv->lock); return sysfs_emit(buf, "%d\n", enabled); > +} > +}; > +static const struct iio_chan_spec_ext_info pac1921_ext_info[] = { > + IIO_ENUM("resolution_bits", IIO_SHARED_BY_ALL, > + &pac1921_resolution_enum), > + IIO_ENUM_AVAILABLE("resolution_bits", IIO_SHARED_BY_ALL, > + &pac1921_resolution_enum), > + IIO_ENUM("integration_samples", IIO_SHARED_BY_ALL, > + &pac1921_int_num_samples_enum), > + IIO_ENUM_AVAILABLE("integration_samples", IIO_SHARED_BY_ALL, > + &pac1921_int_num_samples_enum), > + { > + .name = "filters_en", > + .read = pac1921_read_filters_enabled, > + .write = pac1921_write_filters_enabled, > + .shared = IIO_SHARED_BY_ALL, I address these above with the documentation. > + }, > + {}, No need for a comma after a terminator like this as we will never add anything after it. > +}; > + > +static const struct iio_event_spec pac1921_overflow_event[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, No controls at all? Without some form of enable userspace won't be able to tell these even exist. If the device doesn't support disabling the interrupt, we can fallback to to irq_enable/disable() on the host end of the wire. Ideally disable / enable at the device though. > + }, > +}; > + > +static const struct iio_chan_spec pac1921_channels[] = { > + { > + .type = IIO_VOLTAGE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), Marius pointed out that hardware gain is rarely the way to go. It is typically used when the gain is not directly affecting the thing being read. E.g. light sensitivity of a time of flight sensor. In order to maintain a simple userspace inteface we squash gain related stuff into the scale attributes. There a user can easily see what flexibility is available to them and understand what affect it has on the values they will read back. > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > + .channel = PAC1921_CHAN_VBUS, > + .address = PAC1921_REG_VBUS, > + .scan_index = PAC1921_CHAN_VBUS, > + .scan_type = { > + .sign = 'u', > + .realbits = 10, > + .storagebits = 16, > + .endianness = IIO_CPU > + }, > + .indexed = 1, > + .event_spec = pac1921_overflow_event, > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > + .ext_info = pac1921_ext_info, > + }, > + { > + .type = IIO_VOLTAGE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > + .channel = PAC1921_CHAN_VSENSE, > + .address = PAC1921_REG_VSENSE, > + .scan_index = PAC1921_CHAN_VSENSE, > + .scan_type = { > + .sign = 'u', > + .realbits = 10, > + .storagebits = 16, > + .endianness = IIO_CPU > + }, > + .indexed = 1, > + .event_spec = pac1921_overflow_event, > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > + .ext_info = pac1921_ext_info, > + }, > + { > + .type = IIO_CURRENT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > + .channel = PAC1921_CHAN_CURRENT, > + .address = PAC1921_REG_VSENSE, > + .scan_index = PAC1921_CHAN_CURRENT, > + .scan_type = { > + .sign = 'u', > + .realbits = 10, > + .storagebits = 16, > + .endianness = IIO_CPU > + }, > + .event_spec = pac1921_overflow_event, > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > + .ext_info = pac1921_ext_info, > + }, > + { > + .type = IIO_POWER, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > + .channel = PAC1921_CHAN_POWER, > + .address = PAC1921_REG_VPOWER, > + .scan_index = PAC1921_CHAN_POWER, > + .scan_type = { > + .sign = 'u', > + .realbits = 10, > + .storagebits = 16, > + .endianness = IIO_CPU > + }, > + .event_spec = pac1921_overflow_event, > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > + .ext_info = pac1921_ext_info, > + }, > + IIO_CHAN_SOFT_TIMESTAMP(PAC1921_NUM_MEAS_CHANS), > +}; > + > +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); > + > + guard(mutex)(&priv->lock); > + > + if (!pac1921_data_ready(priv)) > + goto done; > + > + int ret = pac1921_check_push_overflow(idev, pf->timestamp); > + if (ret) > + goto done; > + > + memset(&priv->scan, 0, sizeof(priv->scan)); We normally don't bother. The worse that can happen here is that gaps can contain stale data. We can't leak anything beyond that and such stale data should be harmless. > + > + int bit, ch = 0; Move definitions to the top of the function. Also, don't mix items that assign with ones that don't. Just use separate lines. Obviously fine here as only two of them, but when we get 10+ on a line and only some of them are set, it can be easy to miss bugs. > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { > + int val; > + > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > + if (ret) > + goto done; > + > + priv->scan.chan[ch++] = (u16)val; Why not make read_res take a u16 * and pass in the destination directly? > + } > + > + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); > + > +done: > + iio_trigger_notify_done(idev->trig); > + > + return IRQ_HANDLED; > +} > + > +static int pac1921_init(struct pac1921_priv *priv) > +{ > + /* Time after power-up before ready to begin communications */ > + msleep(20); I'd move that to the caller where we can see the power up happening. If this code gets reorganized in future, the delay may end up in the wrong place. > + > + /* Enter READ state before configuration */ > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_INTEN, 0); > + if (ret) > + return ret; > + > + /* Configure gains and measurements resolution */ > + unsigned int val = priv->di_gain << PAC1921_GAIN_DI_GAIN_SHIFT | > + priv->dv_gain << PAC1921_GAIN_DV_GAIN_SHIFT; FIELD_PREP() throughout. > + if (!priv->high_res) > + val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES; > + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); > + if (ret) > + return ret; > + > + /* Configure integration: Comment style for multiline comments in IIO is /* * Configure... > + * - num of integration samples, filters enabled/disabled > + * - set READ/INT pin override (RIOV) to control operation mode via > + * register instead of pin > + */ > + val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT | > + PAC1921_INT_CFG_RIOV; > + if (priv->filters_en) > + val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN; > + ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val); > + if (ret) > + return ret; > + > + /* Init control register: > + * - VPower free run integration mode > + * - OUT pin full scale range: 3V (HW detault) > + * - no timeout, no sleep, no sleep override, no recalc (HW defaults) > + */ > + val = PAC1921_MXSL_VPOWER_FREE_RUN << PAC1921_CONTROL_MXSL_SHIFT; > + ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val); > + if (ret) > + return ret; > + > + /* Enable integration */ > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > + if (ret) > + return ret; > + > + priv->first_integr_started = true; > + priv->integr_start_time = jiffies; > + > + return pac1921_update_integr_period(priv); > +} > + > + > +static int pac1921_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct pac1921_priv *priv = iio_priv(indio_dev); > + > + guard(mutex)(&priv->lock); > + > + int ret = regulator_enable(priv->vdd); As below. Local variable definitions at the top (old school c style). It obviously doesn't matter here, but it's what reviewers are used to for kernel code. Please fix all cases for v3. > + if (ret) > + return ret; > + > + return pac1921_init(priv); > +} > + > +static DEFINE_SIMPLE_DEV_PM_OPS(pac1921_pm_ops, pac1921_suspend, > + pac1921_resume); > + > +static void pac1921_regulator_disable(void *data) > +{ > + struct pac1921_priv *priv = data; > + > + regulator_disable(priv->vdd); I'd pass in the regulator rather than the private structure. Then this can just be regulator_disable(data); > +} > + > +static int pac1921_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct pac1921_priv *priv; > + > + struct iio_dev *indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); checkpatch probably warns about this. Blank line here. > + if (!indio_dev) > + return -ENOMEM; > + > + priv = iio_priv(indio_dev); > + priv->client = client; > + i2c_set_clientdata(client, indio_dev); > + > + priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config); > + if (IS_ERR(priv->regmap)) > + dev_err_probe(dev, (int)PTR_ERR(priv->regmap), > + "Cannot initialize register map\n"); > + > + mutex_init(&priv->lock); Whilst mutex cleanup only matters in lock debugging cases and isn't really important for this sort of mutex, we now have devm_mutex_init() so good to use that just to avoid anyone having to think if we should cleanup the mutex or not. > + > + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN; > + priv->di_gain = PAC1921_DEFAULT_DI_GAIN; > + priv->high_res = PAC1921_DEFAULT_HIGH_RES; > + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES; > + priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED; > + > + int ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", local variable declarations still belong at the top unless there is a strong readson to do otherwise. > + &priv->rshunt); > + if (ret) > + return dev_err_probe(dev, ret, > + "Cannot read shunt resistor property\n"); > + if (priv->rshunt == 0 || priv->rshunt > INT_MAX) > + return dev_err_probe(dev, -EINVAL, > + "Invalid shunt resistor: %u\n", > + priv->rshunt); > + > + priv->vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(priv->vdd)) > + return dev_err_probe(dev, (int)PTR_ERR(priv->vdd), > + "Cannot get vdd regulator\n"); > + > + ret = regulator_enable(priv->vdd); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot enable vdd regulator\n"); > + > + ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, priv); Check ret. > + > + ret = pac1921_init(priv); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot initialize device\n"); > + > + priv->iio_info = pac1921_iio; > + > + const struct i2c_device_id *id = i2c_client_get_device_id(client); We still mostly (exception is cleanup.h magic) stick to c style of local variable declarations before code. I'd just move this to the top fo this fucntion. > + > + indio_dev->name = id->name; > + indio_dev->info = &priv->iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = pac1921_channels; > + indio_dev->num_channels = ARRAY_SIZE(pac1921_channels); > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + &iio_pollfunc_store_time, > + &pac1921_trigger_handler, NULL); > + if (ret) > + return dev_err_probe(dev, ret, > + "Cannot setup IIO triggered buffer\n"); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot register IIO device\n"); > + > + return 0; > +}
Jonathan Cameron wrote: > On Thu, 04 Jul 2024 19:42:02 +0200 > Matteo Martelli <matteomartelli3@gmail.com> 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 > > * userspace controls for voltage and current gains, measurement > > resolution, integration samples and filters enabling/disabling > > * simple power management support > > > > Limitations: > > * operation mode fixed to free-run integration > > * measurement resolution and filters controls are applied to both VSENSE > > and VBUS measurements > > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > > Hi Matteo, > > A little too fast for sending a new version. Result is Marius reviewed > v1. I may replicate some of Marius' comments but do make sure you > cover them all for v3. > > One big question I have here, is do typical usecases for this device care > much about reducing integration time? I'd have thought they were all > relatively slow. As such it may make sense to not support some > of the modes that need to lower integration time (this is a common thing > to decide to skip in the interests of maintainability and user interface > complexity reduction). > > Looks reasonably speedy to me, unless very high numbers of samples > are used. So that control makes sense to expose, perhaps not > the resolution one. > > Note that custom ABI is almost always a bad idea. We enable it for the > cases where there is no other choice, but reality is those cases then > all need custom userspace software, so the controls are much less likely > to actually used :( > > > Jonathan > Hi Jonathan, Thanks for your feedback, this clarified a lot. I agree to keep the user interface simple, so I will remove the controls that don't look necessary, based on your comments below. Anyway note that some of the comments from Marius (from patch v1) are in contrasts with this, suggesting to actually extend the interface, by exposing the shunt-resistor and controls for the OUT analog pin. I could add the shunt-resistor controls to allow calibration as Marius suggested, but that's also a custom ABI, what are your thoughts on this? Instead I would not introduce controls for the OUT pin, at least not for the moment, since it would require some more considerations on the additional custom controls and an implementation extension to support the pin-controlled integration mode. Right now I defaulted the device operation mode to the free-run power integration mode, which to me it looks like the more versatile since in this way the device exposes all three measurements of voltage, current and power, but the OUT pin setting is fixed to output the power value in a fixed range. I think that the OUT pin might not be used at all in many use cases so I would leave the OUT pin setting as fixed for now and maybe extend it in the future when more use cases arise. I am open to reconsider this though. > > > --- > > .../ABI/testing/sysfs-bus-iio-adc-pac1921 | 45 + > > MAINTAINERS | 7 + > > drivers/iio/adc/Kconfig | 10 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/pac1921.c | 1038 ++++++++++++++++++++ > > 5 files changed, 1101 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > new file mode 100644 > > index 000000000000..4a32e2d4207b > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > Quite a bit of custom ABI in here. > > Rule of thumb is that custom ABI is more or less pointless ABI for 99% of users > because standard userspace won't use it. So keep that in mind when defining it. > > > @@ -0,0 +1,45 @@ > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits > > +KernelVersion: 6.10 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + ADC measurement resolution. Can be either 11 bits or 14 bits > > + (default). The driver sets the same resolution for both VBUS and > > + VSENSE measurements even if the hardware could be configured to > > + measure VBUS and VSENSE with different resolutions. > > + This attribute affects the integration time: with 14 bits > > + resolution the integration time is increased by a factor of > > + 1.9 (the driver considers a factor of 2). See Table 4-5 in > > + device datasheet for details. > > Is the integration time ever high enough that it matters? > People tend not to do power measurement 'quickly'. > > If we are doing it quickly then you'll probably want to be providing buffered > support and that does allow you to 'read' the resolution for a part where > it changes for some other reason. I haven't yet understood this case. I will remove this control and fix the resolution bits to 14 (highest value), same as the HW default. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > +KernelVersion: 6.10 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + List all possible ADC measurement resolutions: "11 14" > > + > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples > > +KernelVersion: 6.10 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Number of samples taken during a full integration period. Can be > > + set to any power of 2 value from 1 (default) to 2048. > > + This attribute affects the integration time: higher the number > > + of samples, longer the integration time. See Table 4-5 in device > > + datasheet for details. > > Sounds like oversampling_ratio which is standards ABI. So use that or explain > why you can't here. I am not sure that this is an oversampling ratio but correct me if I am wrong: generally by increasing the oversampling you would have additional samples in a fixed time period, while in this case by increasing the number of samples you would still have the same number of samples in a fixed time period, but you would have a longer integration period. So maybe the comment is not very clear since this parameter actually means "the number of samples required to complete the integration period". Initially I thought to let the user edit this by writing the integration_time control (which is currently read-only), but since the integration period depends also on the resolution and whether filters are enabled or not, it would have introduced some confusion: what parameter is being changed upon integretion_time write? Maybe after removing resolution and filter controls there would be no such confusion anymore. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples_available > > +KernelVersion: 6.10 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + List all possible numbers of integration samples: > > + "1 2 4 8 16 32 64 128 256 512 1024 2048" > > + > > +What: /sys/bus/iio/devices/iio:devices/filters_en > > +KernelVersion: 6.10 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Attribute to enable/disable ADC post filters. Enabled by > > + default. > > + This attribute affects the integration time: with filters > > + enabled the integration time is increased by 50%. See Table 4-5 > > + in device datasheet for details. > > Do we have any idea what this filter is? Datasheet seems very vague indeed and from > a control point of view that makes this largely useless. How does userspace know > whether to turn it on? > > We have an existing filter ABI but with so little information no way to fit this in. > Gut feeling, leave it on all the time and drop the control interface. I will remove this control and leave it on all the time as the HW default. > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index f60fe85a30d5..b56e494da970 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -991,6 +991,16 @@ config NPCM_ADC > > This driver can also be built as a module. If so, the module > > will be called npcm_adc. > > > > +config PAC1921 > > + tristate "Microchip Technology PAC1921 driver" > > + depends on I2C > > Needs to ensure REGMAP_I2C as well I think. Check similar cases. Yes, I missed it. I think I also need to ensure IIO_BUFFER and IIO_TRIGGERED_BUFFER. > > + help > > + Say yes here to build support for Microchip Technology's PAC1921 > > + High-Side Power/Current Monitor with Analog Output. > > + > > + This driver can also be built as a module. If so, the module > > + will be called pac1921. > > + > > config PAC1934 > > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c > > new file mode 100644 > > index 000000000000..879753466093 > > --- /dev/null > > +++ b/drivers/iio/adc/pac1921.c > > @@ -0,0 +1,1038 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * IIO driver for PAC1921 High-Side Power/Current Monitor > > + * > > + * Copyright (C) 2024 Matteo Martelli <matteomartelli3@gmail.com> > > + */ > > + > > +#include <linux/i2c.h> > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/trigger_consumer.h> > > +#include <linux/iio/triggered_buffer.h> > > +#include <linux/regmap.h> > > + > > +/* pac1921 registers */ > > +#define PAC1921_REG_GAIN_CFG 0x00 > > +#define PAC1921_REG_INT_CFG 0x01 > > +#define PAC1921_REG_CONTROL 0x02 > > +#define PAC1921_REG_VBUS 0x10 > > +#define PAC1921_REG_VSENSE 0x12 > > +#define PAC1921_REG_OVERFLOW_STS 0x1C > > +#define PAC1921_REG_VPOWER 0x1D > > + > > +/* pac1921 gain configuration bits */ > > +#define PAC1921_GAIN_I_RES BIT(7) > > +#define PAC1921_GAIN_V_RES BIT(6) > > +#define PAC1921_GAIN_DI_GAIN_SHIFT 3 > > +#define PAC1921_GAIN_DI_GAIN_MASK GENMASK(5, PAC1921_GAIN_DI_GAIN_SHIFT) > > +#define PAC1921_GAIN_DI_GAIN_MAX 7 > > +#define PAC1921_GAIN_DV_GAIN_SHIFT 0 > > +#define PAC1921_GAIN_DV_GAIN_MASK GENMASK(2, PAC1921_GAIN_DV_GAIN_SHIFT) > > Define only the MASKs not SHIFTs and use FIELD_GET(), FIELD_PREP() throughout. > Gives more readable code in general as well as halving the number of defines. > I was not aware of FIELD_GET() and FIELD_PREP() macros, thanks for pointing them out. I will use them indeed. > > +#define PAC1921_GAIN_DV_GAIN_MAX 5 > > + > > +/* pac1921 integration configuration bits */ > > +#define PAC1921_INT_CFG_SMPL_SHIFT 4 > > +#define PAC1921_INT_CFG_SMPL_MASK GENMASK(7, PAC1921_INT_CFG_SMPL_SHIFT) > > +#define PAC1921_INT_CFG_SMPL_MAX 11 > > +#define PAC1921_INT_CFG_VSFEN BIT(3) > > +#define PAC1921_INT_CFG_VBFEN BIT(2) > > +#define PAC1921_INT_CFG_RIOV BIT(1) > > +#define PAC1921_INT_CFG_INTEN BIT(0) > > + > > +/* pac1921 control bits */ > > +#define PAC1921_CONTROL_MXSL_SHIFT 6 > > +enum pac1921_mxsl { > > + PAC1921_MXSL_VPOWER_PIN = 0, > > + PAC1921_MXSL_VSENSE_FREE_RUN = 1, > > + PAC1921_MXSL_VBUS_FREE_RUN = 2, > > + PAC1921_MXSL_VPOWER_FREE_RUN = 3, > > +}; > > +#define PAC1921_CONTROL_SLEEP BIT(2) > > + > > +/* pac1921 overflow status bits */ > > +#define PAC1921_OVERFLOW_VSOV BIT(2) > > +#define PAC1921_OVERFLOW_VBOV BIT(1) > > +#define PAC1921_OVERFLOW_VPOV BIT(0) > > + > > +/* pac1921 constants */ > > +#define PAC1921_MAX_VSENSE_MV 100 > > +#define PAC1921_MAX_VBUS_V 32 > > +#define PAC1921_RES_RESOLUTION 1023 /* Result registers resolution */ > > + > > +/* pac1921 defaults */ > > +#define PAC1921_DEFAULT_DV_GAIN 0 /* 2^(value): 1x gain */ > > +#define PAC1921_DEFAULT_DI_GAIN 0 /* 2^(value): 1x gain */ > > +#define PAC1921_DEFAULT_HIGH_RES true /* 14-bit measurement resolution */ > > +#define PAC1921_DEFAULT_NUM_SAMPLES 0 /* 2^(value): 1 sample */ > > +#define PAC1921_DEFAULT_FILTERS_ENABLED true > > + > > +/* pac1921 tables to create iio available parameters */ > > +static const unsigned int pac1921_di_gains[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > +static const unsigned int pac1921_dv_gains[] = { 1, 2, 4, 8, 16, 32 }; > > +enum pac1921_meas_resolution_idx { > > + PAC1921_MEAS_RESOLUTION_IDX_LOW = 0, > > + PAC1921_MEAS_RESOLUTION_IDX_HIGH, > > +}; > > +static const char *const pac1921_meas_resolutions[] = { "11", "14" }; > > +static const char *const pac1921_integr_num_samples[] = { > > + "1", "2", "4", "8", "16", "32", > > + "64", "128", "256", "512", "1024", "2048" > > +}; > > + > > +/* pac1921 regmap configuration */ > > +static const struct regmap_range pac1921_regmap_wr_ranges[] = { > > + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), > > +}; > > Trivial but I'd like a blank line here. > Ok. > > +static const struct regmap_access_table pac1921_regmap_wr_table = { > > + .yes_ranges = pac1921_regmap_wr_ranges, > > + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges), > > +}; > > here > Ok. > > +static const struct regmap_range pac1921_regmap_rd_ranges[] = { > > + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), > > + regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1), > > +}; > > here > Ok. > > +static const struct regmap_access_table pac1921_regmap_rd_table = { > > + .yes_ranges = pac1921_regmap_rd_ranges, > > + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges), > > +}; > > and here as my eyes stuggle a bit with parsing this without the > separations. Similar applies above. > Ok. > > +static const struct regmap_config pac1921_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .rd_table = &pac1921_regmap_rd_table, > > + .wr_table = &pac1921_regmap_wr_table, > > +}; > > > + > > +/* Check if overflow occurred and if so, push the corresponding events. > > As mentioned below, without controls I don't think userspace has any way to know > these are coming. It is useful even for overflow events to provide > a) A way to mask them off. > b) The threshold values. So that userspace can see that they are overflow events. > These will be readonly of course. > I see your point, I will add controls. About thresholds, should they be related to raw values or to scaled values? In the first case they would always be fixed to 1023 being the measurement resolution, regardless of the selected gains. In the latter case they would depend on the current gains and I guess that they would be in the same unit of the scale (mV/mA/mW). > > + * > > + * Must be called with lock held. > > + */ > > +static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp) > > +{ > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + unsigned int flags; > > + > > + int ret = regmap_read(priv->regmap, PAC1921_REG_OVERFLOW_STS, &flags); > > + if (ret) > > + return ret; > > + > > + if (flags & PAC1921_OVERFLOW_VBOV && > > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VBOV)) { > > + iio_push_event(indio_dev, > > + IIO_UNMOD_EVENT_CODE( > > + IIO_VOLTAGE, PAC1921_CHAN_VBUS, > > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > > + timestamp); > > + } > > + if (flags & PAC1921_OVERFLOW_VSOV && > > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VSOV)) { > > + iio_push_event(indio_dev, > > + IIO_UNMOD_EVENT_CODE( > > + IIO_VOLTAGE, PAC1921_CHAN_VSENSE, > > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > > + timestamp); > > + iio_push_event(indio_dev, > > + IIO_UNMOD_EVENT_CODE( > > + IIO_CURRENT, PAC1921_CHAN_CURRENT, > > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > > + timestamp); > > + } > > + if (flags & PAC1921_OVERFLOW_VPOV && > > + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VPOV)) { > > + iio_push_event(indio_dev, > > + IIO_UNMOD_EVENT_CODE( > > + IIO_POWER, PAC1921_CHAN_POWER, > > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), > > + timestamp); > > + } > > + > > + priv->prev_ovf_flags = (u8)flags; > > + > > + return 0; > > +} > > + > > +/* Read the value from a result register > > + * > > + * Result registers contain the most recent averaged values of Vbus, Vsense and > > + * Vpower. Each value is 10 bits wide and spread across two consecutive 8 bit > > + * registers, with 6 bit LSB zero padding. > > + */ > > +static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg, > > + int *val) > > +{ > > + u8 val_buf[2]; > > + > > + int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, &val_buf, > > + sizeof(val_buf)); > > + if (ret) > > + return ret; > > + > > + *val = (val_buf[0] << 8 | val_buf[1]) >> 6; > > Looks like it could be done with the self documenting combination of > > get_unaligned_be16() + FIELD_GET() > > or use a __b16 for the read and the be16_to_cpu() approach. Ok. > > + > > + return 0; > > +} > > + > > +static int pac1921_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int *val, > > + int *val2, long mask) > > +{ > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: { > > + guard(mutex)(&priv->lock); > > Given you grab this in all but error paths, I'd just grab it always > and avoid need for the guard() and careful scoping in all these case blocks. > Ok. So just a guard(mutex)(&priv->lock) call at the beginning of the function. > > + > > + if (!pac1921_data_ready(priv)) > > + return -EBUSY; > > + > > + s64 ts = iio_get_time_ns(indio_dev); > > + > > + int ret = pac1921_check_push_overflow(indio_dev, ts); > > + if (ret) > > + return ret; > > + > > + ret = pac1921_read_res(priv, chan->address, val); > > + if (ret) > > + return ret; > > + > > + return IIO_VAL_INT; > > + } > > + case IIO_CHAN_INFO_SCALE: > > + switch (chan->channel) { > > + case PAC1921_CHAN_VBUS: { > > + /* Vbus scale factor in mV: > > + * max_vbus (mV) / vgain / resolution > > + */ > > + guard(mutex)(&priv->lock); > > + > > + *val = PAC1921_MAX_VBUS_V * 1000; > > + *val2 = PAC1921_RES_RESOLUTION << (int)priv->dv_gain; > > + > > + return IIO_VAL_FRACTIONAL; > > + } > > + case PAC1921_CHAN_VSENSE: { > > + /* Vsense voltage scale factor in mV: > > + * max_vsense (mV) / igain / resolution > > + */ > > + guard(mutex)(&priv->lock); > > + > > + *val = PAC1921_MAX_VSENSE_MV; > > + *val2 = PAC1921_RES_RESOLUTION << (int)priv->di_gain; > > + > > + return IIO_VAL_FRACTIONAL; > > + } > > + case PAC1921_CHAN_CURRENT: { > > + /* Current scale factor in mA: > > + * Vsense LSB (nV) / shunt (uOhm) > > + */ > > + guard(mutex)(&priv->lock); > > + > > + *val = pac1921_vsense_lsb(priv->di_gain); > > + *val2 = (int)priv->rshunt; > > + > > + return IIO_VAL_FRACTIONAL; > > + } > > + case PAC1921_CHAN_POWER: { > > + /* Power scale factor in mW: > > + * Vsense LSB (nV) * max_vbus (V) / vgain / shunt (uOhm) > > + */ > > + guard(mutex)(&priv->lock); > > + > > + *val = pac1921_vsense_lsb(priv->di_gain) * > > + (PAC1921_MAX_VBUS_V >> (int)priv->dv_gain); > > + *val2 = (int)priv->rshunt; > > + > > + return IIO_VAL_FRACTIONAL; > > + } > > + default: > > + return -EINVAL; > > + } > > + break; > > + > > + case IIO_CHAN_INFO_HARDWAREGAIN: > > + switch (chan->channel) { > > + case PAC1921_CHAN_VBUS: { > > + guard(mutex)(&priv->lock); > > + *val = 1 << (int)priv->dv_gain; > > + return IIO_VAL_INT; > > + } > > + case PAC1921_CHAN_VSENSE: > > + case PAC1921_CHAN_CURRENT: { > > + guard(mutex)(&priv->lock); > > + *val = 1 << (int)priv->di_gain; > > + return IIO_VAL_INT; > > + } > > + default: > > + return -EINVAL; > > + } > > + > > + case IIO_CHAN_INFO_INT_TIME: { > > + /* Integration time is read only: it depends on the number of > > + * integration samples, measurement resolution and post filters > > + */ > > + *val2 = 1000000; /* From usecs to fractional secs */ > > + guard(mutex)(&priv->lock); > > + *val = (int)priv->integr_period; > > + return IIO_VAL_FRACTIONAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, > > + unsigned int mask, unsigned int val) > > +{ > > + /* Enter READ state before configuration */ > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_INTEN, 0); > > + if (ret) > > + return ret; > > + > > + /* Update configuration value */ > > + ret = regmap_update_bits(priv->regmap, reg, mask, val); > > + if (ret) > > + return ret; > > + > > + /* Re-enable integration and reset start time */ > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > > + if (ret) > > + return ret; > > + > > + priv->integr_start_time = jiffies; > > Add a comment for why this value. > Could you elaborate what's confusing here? The comment above states "reset start time", maybe I should move it above the assignment of priv->integr_start_time? Or it's the use of jiffies that it's confusing? > > + priv->first_integr_done = false; > Will default to this anyway, so you could skip it unless you feel this is > useful from documentation point of view. > That variable is being asserted after the first integration period is complete to avoid spurious data (see pac1921_data_ready()). Here it is being reset after a configuration change to invalidate previous data. So the assignment is necessary. > > + > > + return 0; > > +} > > + > > > > > +static int pac1921_set_int_num_samples(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + unsigned int val) > > +{ > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + > > + if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX)) > > + return -EINVAL; > > + > > + guard(mutex)(&priv->lock); > > + > > + if (priv->n_samples == val) > > + return 0; > > + > > + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_SMPL_MASK, > > + val << PAC1921_INT_CFG_SMPL_SHIFT); > > FIELD_PREP() etc. > Ok. > > + if (ret) > > + return ret; > > + > > + priv->n_samples = (u8)val; > > + > > + return pac1921_update_integr_period(priv); > > +} > > + > > +static ssize_t pac1921_read_filters_enabled(struct iio_dev *indio_dev, > > + uintptr_t private, > > + const struct iio_chan_spec *chan, > > + char *buf) > > +{ > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + bool enabled; > > + > > + scoped_guard(mutex, &priv->lock) { > > + enabled = priv->filters_en; > > + } > > + return sysfs_emit(buf, "%d\n", enabled); > > It's not a fast path hence holding the lock a little longer than necessary doesn't > matter, so I'd do the simpler. > > guard(mutex)(&priv->lock); > > return sysfs_emit(buf, "%d\n", enabled); > Ok, but I think you mean: guard(mutex)(&priv->lock); return sysfs_emit(buf, "%d\n", priv->filters_en); > > +} > > > > +}; > > +static const struct iio_chan_spec_ext_info pac1921_ext_info[] = { > > + IIO_ENUM("resolution_bits", IIO_SHARED_BY_ALL, > > + &pac1921_resolution_enum), > > + IIO_ENUM_AVAILABLE("resolution_bits", IIO_SHARED_BY_ALL, > > + &pac1921_resolution_enum), > > + IIO_ENUM("integration_samples", IIO_SHARED_BY_ALL, > > + &pac1921_int_num_samples_enum), > > + IIO_ENUM_AVAILABLE("integration_samples", IIO_SHARED_BY_ALL, > > + &pac1921_int_num_samples_enum), > > + { > > + .name = "filters_en", > > + .read = pac1921_read_filters_enabled, > > + .write = pac1921_write_filters_enabled, > > + .shared = IIO_SHARED_BY_ALL, > > I address these above with the documentation. > Answered above as well. > > + }, > > + {}, > No need for a comma after a terminator like this as we will never add anything > after it. > Ok. > > > +}; > > + > > +static const struct iio_event_spec pac1921_overflow_event[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_RISING, > > No controls at all? Without some form of enable userspace won't > be able to tell these even exist. If the device doesn't support > disabling the interrupt, we can fallback to to irq_enable/disable() > on the host end of the wire. Ideally disable / enable at the device > though. I will add controls, please see my question about thresholds above. > > + }, > > +}; > > + > > +static const struct iio_chan_spec pac1921_channels[] = { > > + { > > + .type = IIO_VOLTAGE, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), > Marius pointed out that hardware gain is rarely the way to go. > It is typically used when the gain is not directly affecting the > thing being read. E.g. light sensitivity of a time of flight sensor. > > In order to maintain a simple userspace inteface we squash gain related > stuff into the scale attributes. There a user can easily see what > flexibility is available to them and understand what affect it has on > the values they will read back. > I see, thanks for clarify this, I would do it via iio_info.read_avail() callback. > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > > + .channel = PAC1921_CHAN_VBUS, > > + .address = PAC1921_REG_VBUS, > > + .scan_index = PAC1921_CHAN_VBUS, > > + .scan_type = { > > + .sign = 'u', > > + .realbits = 10, > > + .storagebits = 16, > > + .endianness = IIO_CPU > > + }, > > + .indexed = 1, > > + .event_spec = pac1921_overflow_event, > > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > > + .ext_info = pac1921_ext_info, > > + }, > > + { > > + .type = IIO_VOLTAGE, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > > + .channel = PAC1921_CHAN_VSENSE, > > + .address = PAC1921_REG_VSENSE, > > + .scan_index = PAC1921_CHAN_VSENSE, > > + .scan_type = { > > + .sign = 'u', > > + .realbits = 10, > > + .storagebits = 16, > > + .endianness = IIO_CPU > > + }, > > + .indexed = 1, > > + .event_spec = pac1921_overflow_event, > > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > > + .ext_info = pac1921_ext_info, > > + }, > > + { > > + .type = IIO_CURRENT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > > + .channel = PAC1921_CHAN_CURRENT, > > + .address = PAC1921_REG_VSENSE, > > + .scan_index = PAC1921_CHAN_CURRENT, > > + .scan_type = { > > + .sign = 'u', > > + .realbits = 10, > > + .storagebits = 16, > > + .endianness = IIO_CPU > > + }, > > + .event_spec = pac1921_overflow_event, > > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > > + .ext_info = pac1921_ext_info, > > + }, > > + { > > + .type = IIO_POWER, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > > + .channel = PAC1921_CHAN_POWER, > > + .address = PAC1921_REG_VPOWER, > > + .scan_index = PAC1921_CHAN_POWER, > > + .scan_type = { > > + .sign = 'u', > > + .realbits = 10, > > + .storagebits = 16, > > + .endianness = IIO_CPU > > + }, > > + .event_spec = pac1921_overflow_event, > > + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), > > + .ext_info = pac1921_ext_info, > > + }, > > + IIO_CHAN_SOFT_TIMESTAMP(PAC1921_NUM_MEAS_CHANS), > > +}; > > + > > +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); > > + > > + guard(mutex)(&priv->lock); > > + > > + if (!pac1921_data_ready(priv)) > > + goto done; > > + > > + int ret = pac1921_check_push_overflow(idev, pf->timestamp); > > + if (ret) > > + goto done; > > + > > + memset(&priv->scan, 0, sizeof(priv->scan)); > > We normally don't bother. The worse that can happen here is that > gaps can contain stale data. We can't leak anything beyond that and > such stale data should be harmless. > Ok. > > + > > + int bit, ch = 0; > > Move definitions to the top of the function. > Also, don't mix items that assign with ones that don't. Just use separate > lines. Obviously fine here as only two of them, but when we get 10+ on > a line and only some of them are set, it can be easy to miss bugs. > Ok. > > > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { > > + int val; > > + > > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > > + if (ret) > > + goto done; > > + > > + priv->scan.chan[ch++] = (u16)val; > > Why not make read_res take a u16 * and pass in the destination directly? > pac1921_read_res() takes an int *val so that pac1921_read_raw() can pass its int *val argument directly and to avoid an additional u16 var. However I don't mind changing it if that looks more clear. > > + } > > + > > + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); > > + > > +done: > > + iio_trigger_notify_done(idev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int pac1921_init(struct pac1921_priv *priv) > > +{ > > + /* Time after power-up before ready to begin communications */ > > + msleep(20); > > I'd move that to the caller where we can see the power up happening. > If this code gets reorganized in future, the delay may end up in the wrong > place. > Ok. > > + > > + /* Enter READ state before configuration */ > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_INTEN, 0); > > + if (ret) > > + return ret; > > + > > + /* Configure gains and measurements resolution */ > > + unsigned int val = priv->di_gain << PAC1921_GAIN_DI_GAIN_SHIFT | > > + priv->dv_gain << PAC1921_GAIN_DV_GAIN_SHIFT; > FIELD_PREP() throughout. > Ok. > > + if (!priv->high_res) > > + val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES; > > + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); > > + if (ret) > > + return ret; > > + > > + /* Configure integration: > Comment style for multiline comments in IIO is > /* > * Configure... > All right. I will change all multiline comments accordingly. > > + * - num of integration samples, filters enabled/disabled > > + * - set READ/INT pin override (RIOV) to control operation mode via > > + * register instead of pin > > + */ > > + val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT | > > + PAC1921_INT_CFG_RIOV; > > + if (priv->filters_en) > > + val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN; > > + ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val); > > + if (ret) > > + return ret; > > + > > + /* Init control register: > > + * - VPower free run integration mode > > + * - OUT pin full scale range: 3V (HW detault) > > + * - no timeout, no sleep, no sleep override, no recalc (HW defaults) > > + */ > > + val = PAC1921_MXSL_VPOWER_FREE_RUN << PAC1921_CONTROL_MXSL_SHIFT; > > + ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val); > > + if (ret) > > + return ret; > > + > > + /* Enable integration */ > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > > + if (ret) > > + return ret; > > + > > + priv->first_integr_started = true; > > + priv->integr_start_time = jiffies; > > + > > + return pac1921_update_integr_period(priv); > > +} > > + > > > + > > +static int pac1921_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + > > + guard(mutex)(&priv->lock); > > + > > + int ret = regulator_enable(priv->vdd); > > As below. Local variable definitions at the top (old school c style). > It obviously doesn't matter here, but it's what reviewers are used to for > kernel code. > > Please fix all cases for v3. > Ok. > > + if (ret) > > + return ret; > > + > > + return pac1921_init(priv); > > +} > > + > > +static DEFINE_SIMPLE_DEV_PM_OPS(pac1921_pm_ops, pac1921_suspend, > > + pac1921_resume); > > + > > +static void pac1921_regulator_disable(void *data) > > +{ > > + struct pac1921_priv *priv = data; > > + > > + regulator_disable(priv->vdd); > I'd pass in the regulator rather than the private structure. > Then this can just be > > regulator_disable(data); > Ok. > > +} > > + > > +static int pac1921_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct pac1921_priv *priv; > > + > > + struct iio_dev *indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); > > checkpatch probably warns about this. Blank line here. > Ok. > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + priv = iio_priv(indio_dev); > > + priv->client = client; > > + i2c_set_clientdata(client, indio_dev); > > + > > + priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config); > > + if (IS_ERR(priv->regmap)) > > + dev_err_probe(dev, (int)PTR_ERR(priv->regmap), > > + "Cannot initialize register map\n"); > > + > > + mutex_init(&priv->lock); > > Whilst mutex cleanup only matters in lock debugging cases and isn't really important > for this sort of mutex, we now have devm_mutex_init() so good to use > that just to avoid anyone having to think if we should cleanup the mutex or not. > Ok. > > + > > + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN; > > + priv->di_gain = PAC1921_DEFAULT_DI_GAIN; > > + priv->high_res = PAC1921_DEFAULT_HIGH_RES; > > + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES; > > + priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED; > > + > > + int ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", > local variable declarations still belong at the top unless there is a strong > readson to do otherwise. > The reason in general is just to keep the variable scope as tight as possible to avoid unwanted use of such variable earlier than its wanted usage, and since this is allowed with recent C versions, I thought it would be good to start applying it. However, I understand this could make the code harder to read for many people so I will stick to declare variables at the top. > > + &priv->rshunt); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Cannot read shunt resistor property\n"); > > + if (priv->rshunt == 0 || priv->rshunt > INT_MAX) > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid shunt resistor: %u\n", > > + priv->rshunt); > > + > > + priv->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(priv->vdd)) > > + return dev_err_probe(dev, (int)PTR_ERR(priv->vdd), > > + "Cannot get vdd regulator\n"); > > + > > + ret = regulator_enable(priv->vdd); > > + if (ret) > > + return dev_err_probe(dev, ret, "Cannot enable vdd regulator\n"); > > + > > + ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, priv); > > Check ret. > Indeed! Thanks for the catch. > > + > > + ret = pac1921_init(priv); > > + if (ret) > > + return dev_err_probe(dev, ret, "Cannot initialize device\n"); > > + > > + priv->iio_info = pac1921_iio; > > + > > + const struct i2c_device_id *id = i2c_client_get_device_id(client); > We still mostly (exception is cleanup.h magic) stick to c style of > local variable declarations before code. I'd just move this to the top > fo this fucntion. > Ok. > > + > > + indio_dev->name = id->name; > > + indio_dev->info = &priv->iio_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = pac1921_channels; > > + indio_dev->num_channels = ARRAY_SIZE(pac1921_channels); > > + > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > > + &iio_pollfunc_store_time, > > + &pac1921_trigger_handler, NULL); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Cannot setup IIO triggered buffer\n"); > > + > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret) > > + return dev_err_probe(dev, ret, "Cannot register IIO device\n"); > > + > > + return 0; > > +} > Thanks again, Matteo
> > Hi Matteo, > > > > A little too fast for sending a new version. Result is Marius reviewed > > v1. I may replicate some of Marius' comments but do make sure you > > cover them all for v3. > > > > One big question I have here, is do typical usecases for this device care > > much about reducing integration time? I'd have thought they were all > > relatively slow. As such it may make sense to not support some > > of the modes that need to lower integration time (this is a common thing > > to decide to skip in the interests of maintainability and user interface > > complexity reduction). > > > > Looks reasonably speedy to me, unless very high numbers of samples > > are used. So that control makes sense to expose, perhaps not > > the resolution one. > > > > Note that custom ABI is almost always a bad idea. We enable it for the > > cases where there is no other choice, but reality is those cases then > > all need custom userspace software, so the controls are much less likely > > to actually used :( > > > > > > Jonathan > > > > Hi Jonathan, > > Thanks for your feedback, this clarified a lot. > > I agree to keep the user interface simple, so I will remove the > controls that don't look necessary, based on your comments below. Anyway note > that some of the comments from Marius (from patch v1) are in contrasts with > this, suggesting to actually extend the interface, by exposing the > shunt-resistor and controls for the OUT analog pin. > > I could add the shunt-resistor controls to allow calibration as Marius > suggested, but that's also a custom ABI, what are your thoughts on this? This would actually be a generalization of existing device specific ABI that has been through review in the past. See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 for example (similar in other places). So if you want to do this move that ABI up a level to cover multiple devices (removing the entries in specific files as you do so). > > Instead I would not introduce controls for the OUT pin, at least not for the > moment, since it would require some more considerations on the additional > custom controls and an implementation extension to support the pin-controlled > integration mode. Right now I defaulted the device operation mode to the > free-run power integration mode, which to me it looks like the more versatile > since in this way the device exposes all three measurements of voltage, current > and power, but the OUT pin setting is fixed to output the power value in a > fixed range. > I think that the OUT pin might not be used at all in many use cases so I would > leave the OUT pin setting as fixed for now and maybe extend it in the future > when more use cases arise. I am open to reconsider this though. Absolutely fine to not support things in an initial driver. Those that need them can contribute support later, or you can decide to do so if you like. > > > > + > > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > +KernelVersion: 6.10 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + List all possible ADC measurement resolutions: "11 14" > > > + > > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples > > > +KernelVersion: 6.10 > > > +Contact: linux-iio@vger.kernel.org > > > +Description: > > > + Number of samples taken during a full integration period. Can be > > > + set to any power of 2 value from 1 (default) to 2048. > > > + This attribute affects the integration time: higher the number > > > + of samples, longer the integration time. See Table 4-5 in device > > > + datasheet for details. > > > > Sounds like oversampling_ratio which is standards ABI. So use that or explain > > why you can't here. > > I am not sure that this is an oversampling ratio but correct me if I am wrong: > generally by increasing the oversampling you would have additional samples in a > fixed time period, while in this case by increasing the number of samples you > would still have the same number of samples in a fixed time period, but you > would have a longer integration period. So maybe the comment is not very > clear since this parameter actually means "the number of samples required to > complete the integration period". No. Oversampling is independent of the sampling period in general (though here the 'integration time' is very confusing terminology. You may have to have sampling_frequency (if provided) updated to incorporate that the device can't deliver data as quickly. > > Initially I thought to let the user edit this by writing the integration_time > control (which is currently read-only), but since the integration period > depends also on the resolution and whether filters are enabled or not, it would > have introduced some confusion: what parameter is being changed upon > integretion_time write? Maybe after removing resolution and filter controls > there would be no such confusion anymore. Hmm. The documentation seems to have an unusual definition of 'integration' time. That looks like 1/sampling_frequency. In an oversampling device integration time is normally about a single sample, not the aggregate of sampling and read out etc. I guess here the complexity is that integration time isn't about the time taken for a capacitor to charge, but more the time over which power is computed. But then the value is divided by number of samples so I'm even more confused. If we just read 'integration time' as data acquisition time, it makes a lot more sense. > > > + > > > +/* Check if overflow occurred and if so, push the corresponding events. > > > > As mentioned below, without controls I don't think userspace has any way to know > > these are coming. It is useful even for overflow events to provide > > a) A way to mask them off. > > b) The threshold values. So that userspace can see that they are overflow events. > > These will be readonly of course. > > > I see your point, I will add controls. About thresholds, should they be related > to raw values or to scaled values? In the first case they would always be > fixed to 1023 being the measurement resolution, regardless of the selected > gains. In the latter case they would depend on the current gains and I guess > that they would be in the same unit of the scale (mV/mA/mW). Thresholds are in raw units. 1023 is indeed what I'd expect here. ... > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, > > > + unsigned int mask, unsigned int val) > > > +{ > > > + /* Enter READ state before configuration */ > > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > + PAC1921_INT_CFG_INTEN, 0); > > > + if (ret) > > > + return ret; > > > + > > > + /* Update configuration value */ > > > + ret = regmap_update_bits(priv->regmap, reg, mask, val); > > > + if (ret) > > > + return ret; > > > + > > > + /* Re-enable integration and reset start time */ > > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > > > + if (ret) > > > + return ret; > > > + > > > + priv->integr_start_time = jiffies; > > > > Add a comment for why this value. > > > Could you elaborate what's confusing here? The comment above states "reset > start time", maybe I should move it above the assignment of > priv->integr_start_time? Or it's the use of jiffies that it's confusing? Why is it jiffies? Why not jiffies * 42? I'm looking for a datasheet reference for why the particular value is used. > > > > + priv->first_integr_done = false; > > Will default to this anyway, so you could skip it unless you feel this is > > useful from documentation point of view. > > > That variable is being asserted after the first integration period is complete > to avoid spurious data (see pac1921_data_ready()). Here it is being reset after > a configuration change to invalidate previous data. So the assignment is > necessary. Ah. I' missed this happens in a config change path rather than only first time after reset of device. > > > +static ssize_t pac1921_read_filters_enabled(struct iio_dev *indio_dev, > > > + uintptr_t private, > > > + const struct iio_chan_spec *chan, > > > + char *buf) > > > +{ > > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > > + bool enabled; > > > + > > > + scoped_guard(mutex, &priv->lock) { > > > + enabled = priv->filters_en; > > > + } > > > + return sysfs_emit(buf, "%d\n", enabled); > > > > It's not a fast path hence holding the lock a little longer than necessary doesn't > > matter, so I'd do the simpler. > > > > guard(mutex)(&priv->lock); > > > > return sysfs_emit(buf, "%d\n", enabled); > > > Ok, but I think you mean: > > guard(mutex)(&priv->lock); > return sysfs_emit(buf, "%d\n", priv->filters_en); oops. yes. > > > > > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { > > > + int val; > > > + > > > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > > > + if (ret) > > > + goto done; > > > + > > > + priv->scan.chan[ch++] = (u16)val; > > > > Why not make read_res take a u16 * and pass in the destination directly? > > > pac1921_read_res() takes an int *val so that pac1921_read_raw() can pass its > int *val argument directly and to avoid an additional u16 var. However I don't > mind changing it if that looks more clear. I think it would be cleaner to maintain the type up to the caller. > > Thanks again, > Matteo > For future reference, no need to acknowledge stuff you agree with. Much better to crop to the places where there are questions or responses as it saves time for the next step of the discussion! Thanks, Jonathan
Jonathan Cameron wrote: ... > > I could add the shunt-resistor controls to allow calibration as Marius > > suggested, but that's also a custom ABI, what are your thoughts on this? > > This would actually be a generalization of existing device specific ABI > that has been through review in the past. > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > for example (similar in other places). > So if you want to do this move that ABI up a level to cover multiple devices > (removing the entries in specific files as you do so). > I would do this in a separate commit, would you prefer it in this same patch set or in another separate patch? ... > > > > > > + > > > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > > +KernelVersion: 6.10 > > > > +Contact: linux-iio@vger.kernel.org > > > > +Description: > > > > + List all possible ADC measurement resolutions: "11 14" > > > > + > > > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples > > > > +KernelVersion: 6.10 > > > > +Contact: linux-iio@vger.kernel.org > > > > +Description: > > > > + Number of samples taken during a full integration period. Can be > > > > + set to any power of 2 value from 1 (default) to 2048. > > > > + This attribute affects the integration time: higher the number > > > > + of samples, longer the integration time. See Table 4-5 in device > > > > + datasheet for details. > > > > > > Sounds like oversampling_ratio which is standards ABI. So use that or explain > > > why you can't here. > > > > I am not sure that this is an oversampling ratio but correct me if I am wrong: > > generally by increasing the oversampling you would have additional samples in a > > fixed time period, while in this case by increasing the number of samples you > > would still have the same number of samples in a fixed time period, but you > > would have a longer integration period. So maybe the comment is not very > > clear since this parameter actually means "the number of samples required to > > complete the integration period". > > No. Oversampling is independent of the sampling period in general (though > here the 'integration time' is very confusing terminology. You may > have to have sampling_frequency (if provided) updated to incorporate that > the device can't deliver data as quickly. > > > > > Initially I thought to let the user edit this by writing the integration_time > > control (which is currently read-only), but since the integration period > > depends also on the resolution and whether filters are enabled or not, it would > > have introduced some confusion: what parameter is being changed upon > > integretion_time write? Maybe after removing resolution and filter controls > > there would be no such confusion anymore. > > Hmm. The documentation seems to have an unusual definition of 'integration' time. > That looks like 1/sampling_frequency. In an oversampling device integration time > is normally about a single sample, not the aggregate of sampling and read out > etc. > > I guess here the complexity is that integration time isn't about the time > taken for a capacitor to charge, but more the time over which power is computed. > But then the value is divided by number of samples so I'm even more confused. > > If we just read 'integration time' as data acquisition time, it makes a lot > more sense. > I think I now get what you are suggesting, please correct me otherwise: 1. Let's consider the sampling frequency as how often the device provides computed ("integrated") measurements to the host, so this would be 1/"integration period". This is not the internal ADC sampling rate. 2. I will expose sampling_frequency (RO), oversampling_ratio (R/W) and oversampling_ratio_available (RO) to the user, where oversampling_ratio corresponds to what the datasheet refers to as the "number of ADC samples to complete an integration". 3. When the user writes the oversampling_ratio, the sampling_frequency gets updated accordingly. 4. With two real examples: 4.1. The user writes 16 to oversampling_ratio, then reads 43.478 from sampling_frequency: with 16 samples the "integration period" is 23ms (from Table 4-5) so 1/0.023 => 43.478 Hz 4.2. The user writes 2048 to oversampling_ratio, then reads 0.34 from sampling_frequency: with 2048 samples the "integration period" is 2941ms (from Table 4-5) so 1/2.941 => 0.34 Hz 5. Do not expose the integration_time control to avoid confusion: the so called "integration period" can be derived from the sampling frequency as 1/sampling_frequency. ... > > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, > > > > + unsigned int mask, unsigned int val) > > > > +{ > > > > + /* Enter READ state before configuration */ > > > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > > + PAC1921_INT_CFG_INTEN, 0); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* Update configuration value */ > > > > + ret = regmap_update_bits(priv->regmap, reg, mask, val); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* Re-enable integration and reset start time */ > > > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + priv->integr_start_time = jiffies; > > > > > > Add a comment for why this value. > > > > > Could you elaborate what's confusing here? The comment above states "reset > > start time", maybe I should move it above the assignment of > > priv->integr_start_time? Or it's the use of jiffies that it's confusing? > > Why is it jiffies? Why not jiffies * 42? > I'm looking for a datasheet reference for why the particular value is used. > I used jiffies just to track the elapsed time between readings. Something I am not considering here? Of course jiffies granularity might be larger than the minimum sampling frequency. Is there a common better approach? ... > For future reference, no need to acknowledge stuff you agree > with. Much better to crop to the places where there are questions or responses > as it saves time for the next step of the discussion! Ok....oops! > > Thanks, > > Jonathan > Thanks, Matteo
On Tue, 2024-07-09 at 10:21 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Jonathan Cameron wrote: > ... > > > I could add the shunt-resistor controls to allow calibration as > > > Marius > > > suggested, but that's also a custom ABI, what are your thoughts > > > on this? > > > > This would actually be a generalization of existing device specific > > ABI > > that has been through review in the past. > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > for example (similar in other places). > > So if you want to do this move that ABI up a level to cover > > multiple devices > > (removing the entries in specific files as you do so). > > > I would do this in a separate commit, would you prefer it in this > same patch > set or in another separate patch? > > ... > > > > > > > > + > > > > > +What:         > > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > > > +KernelVersion: 6.10 > > > > > +Contact:      linux-iio@vger.kernel.org > > > > > +Description: > > > > > +              List all possible ADC measurement > > > > > resolutions: "11 14" > > > > > + > > > > > +What:         > > > > > /sys/bus/iio/devices/iio:deviceX/integration_samples > > > > > +KernelVersion: 6.10 > > > > > +Contact:      linux-iio@vger.kernel.org > > > > > +Description: > > > > > +              Number of samples taken during a full > > > > > integration period. Can be > > > > > +              set to any power of 2 value from 1 (default) > > > > > to 2048. > > > > > +              This attribute affects the integration time: > > > > > higher the number > > > > > +              of samples, longer the integration time. See > > > > > Table 4-5 in device > > > > > +              datasheet for details. > > > > > > > > Sounds like oversampling_ratio which is standards ABI. So use > > > > that or explain > > > > why you can't here. > > > > > > I am not sure that this is an oversampling ratio but correct me > > > if I am wrong: > > > generally by increasing the oversampling you would have > > > additional samples in a > > > fixed time period, while in this case by increasing the number of > > > samples you > > > would still have the same number of samples in a fixed time > > > period, but you > > > would have a longer integration period. So maybe the comment is > > > not very > > > clear since this parameter actually means "the number of samples > > > required to > > > complete the integration period". > > > > No. Oversampling is independent of the sampling period in general > > (though > > here the 'integration time' is very confusing terminology. You may > > have to have sampling_frequency (if provided) updated to > > incorporate that > > the device can't deliver data as quickly. > > > > > > > > Initially I thought to let the user edit this by writing the > > > integration_time > > > control (which is currently read-only), but since the integration > > > period > > > depends also on the resolution and whether filters are enabled or > > > not, it would > > > have introduced some confusion: what parameter is being changed > > > upon > > > integretion_time write? Maybe after removing resolution and > > > filter controls > > > there would be no such confusion anymore. > > > > Hmm. The documentation seems to have an unusual definition of > > 'integration' time. > > That looks like 1/sampling_frequency. In an oversampling device > > integration time > > is normally about a single sample, not the aggregate of sampling > > and read out > > etc. > > > > I guess here the complexity is that integration time isn't about > > the time > > taken for a capacitor to charge, but more the time over which power > > is computed. > > But then the value is divided by number of samples so I'm even more > > confused. > > > > If we just read 'integration time' as data acquisition time, it > > makes a lot > > more sense. > > > I think I now get what you are suggesting, please correct me > otherwise: > > 1. Let's consider the sampling frequency as how often the device > provides >   computed ("integrated") measurements to the host, so this would be >   1/"integration period". This is not the internal ADC sampling > rate. > > 2. I will expose sampling_frequency (RO), oversampling_ratio (R/W) > and >   oversampling_ratio_available (RO) to the user, where > oversampling_ratio >   corresponds to what the datasheet refers to as the "number of ADC > samples to >   complete an integration". > > 3. When the user writes the oversampling_ratio, the > sampling_frequency gets >   updated accordingly. Let me came with some clarifications. Internal sampling frequency for this device has a fixed value. Based on the number of samples the PAC will internally accumulate the read values and then the average is calculated. I think the "best" approach is to use the oversampling_ratio and the frequency gets updated accordingly and it will be RO. We will also need the oversampling_ratio_available. > > 4. With two real examples: >    4.1. The user writes 16 to oversampling_ratio, then reads 43.478 > from >      sampling_frequency: with 16 samples the "integration period" is > 23ms >      (from Table 4-5) so 1/0.023 => 43.478 Hz >    4.2. The user writes 2048 to oversampling_ratio, then reads 0.34 > from >      sampling_frequency: with 2048 samples the "integration period" > is 2941ms >      (from Table 4-5) so 1/2.941 => 0.34 Hz > > 5. Do not expose the integration_time control to avoid confusion: the > so called >   "integration period" can be derived from the sampling frequency as >   1/sampling_frequency. > > ... > > > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, > > > > > unsigned int reg, > > > > > +                                unsigned int mask, unsigned > > > > > int val) > > > > > +{ > > > > > +      /* Enter READ state before configuration */ > > > > > +      int ret = regmap_update_bits(priv->regmap, > > > > > PAC1921_REG_INT_CFG, > > > > > +                                   PAC1921_INT_CFG_INTEN, > > > > > 0); > > > > > +      if (ret) > > > > > +              return ret; > > > > > + > > > > > +      /* Update configuration value */ > > > > > +      ret = regmap_update_bits(priv->regmap, reg, mask, > > > > > val); > > > > > +      if (ret) > > > > > +              return ret; > > > > > + > > > > > +      /* Re-enable integration and reset start time */ > > > > > +      ret = regmap_update_bits(priv->regmap, > > > > > PAC1921_REG_INT_CFG, > > > > > +                               PAC1921_INT_CFG_INTEN, > > > > > PAC1921_INT_CFG_INTEN); > > > > > +      if (ret) > > > > > +              return ret; > > > > > + > > > > > +      priv->integr_start_time = jiffies; > > > > > > > > Add a comment for why this value. > > > > > > > Could you elaborate what's confusing here? The comment above > > > states "reset > > > start time", maybe I should move it above the assignment of > > > priv->integr_start_time? Or it's the use of jiffies that it's > > > confusing? > > > > Why is it jiffies?  Why not jiffies * 42? > > I'm looking for a datasheet reference for why the particular value > > is used. > > > I used jiffies just to track the elapsed time between readings. > Something I am > not considering here? Of course jiffies granularity might be larger > than the > minimum sampling frequency. Is there a common better approach? > > ... > > For future reference, no need to acknowledge stuff you agree > > with. Much better to crop to the places where there are questions > > or responses > > as it saves time for the next step of the discussion! > Ok....oops! > > > > > Thanks, > > > > Jonathan > > > > Thanks, > Matteo > Thanks, Marius
Hi Matteo, On Mon, 2024-07-08 at 15:39 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > > > > I think that the OUT pin might not be used at all in many use cases > so I would > leave the OUT pin setting as fixed for now and maybe extend it in the > future > when more use cases arise. I am open to reconsider this though. > The OUT functionality could be set from the device tree. > > > > > --- > > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921         |  45 + > > >  MAINTAINERS                                       |   7 + > > >  drivers/iio/adc/Kconfig                           |  10 + > > >  drivers/iio/adc/Makefile                          |   1 + > > >  drivers/iio/adc/pac1921.c                         | 1038 > > > ++++++++++++++++++++ > > >  5 files changed, 1101 insertions(+) > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > new file mode 100644 > > > index 000000000000..4a32e2d4207b > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > Quite a bit of custom ABI in here. > > > > Rule of thumb is that custom ABI is more or less pointless ABI for > > 99% of users > > because standard userspace won't use it. So keep that in mind when > > defining it. > > > > > @@ -0,0 +1,45 @@ > > > +What:             > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > +KernelVersion:    6.10 > > > +Contact:  linux-iio@vger.kernel.org > > > +Description: > > > +          ADC measurement resolution. Can be either 11 bits or > > > 14 bits > > > +          (default). The driver sets the same resolution for > > > both VBUS and > > > +          VSENSE measurements even if the hardware could be > > > configured to > > > +          measure VBUS and VSENSE with different resolutions. > > > +          This attribute affects the integration time: with 14 > > > bits > > > +          resolution the integration time is increased by a > > > factor of > > > +          1.9 (the driver considers a factor of 2). See Table > > > 4-5 in > > > +          device datasheet for details. > > > > Is the integration time ever high enough that it matters? > > People tend not to do power measurement 'quickly'. > > > > If we are doing it quickly then you'll probably want to be > > providing buffered > > support and that does allow you to 'read' the resolution for a part > > where > > it changes for some other reason.  I haven't yet understood this > > case. > > I will remove this control and fix the resolution bits to 14 (highest > value), > same as the HW default. The resolution could be set from the device tree. As default it could be 14 bits like into the hardware. The user could add "microchip,low_resolution_voltage" into the device tree in order to use only 11 bits for voltage samples. > > > > + > > > +What:             > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > +KernelVersion:    6.10 > > > +Contact:  linux-iio@vger.kernel.org > > > +Description: > > > +          List all possible ADC measurement resolutions: "11 > > > 14" > > > + > > > +What:             > > > /sys/bus/iio/devices/iio:deviceX/integration_samples > > > +KernelVersion:    6.10 > > > +Contact:  linux-iio@vger.kernel.org > > > +Description: > > > +          Number of samples taken during a full integration > > > period. Can be > > > +          set to any power of 2 value from 1 (default) to 2048. > > > +          This attribute affects the integration time: higher > > > the number > > > +          of samples, longer the integration time. See Table 4- > > > 5 in device > > > +          datasheet for details. > > > > Sounds like oversampling_ratio which is standards ABI. So use that > > or explain > > why you can't here. > > I am not sure that this is an oversampling ratio but correct me if I > am wrong: > generally by increasing the oversampling you would have additional > samples in a > fixed time period, while in this case by increasing the number of > samples you > would still have the same number of samples in a fixed time period, > but you > would have a longer integration period. So maybe the comment is not > very > clear since this parameter actually means "the number of samples > required to > complete the integration period". > > Initially I thought to let the user edit this by writing the > integration_time > control (which is currently read-only), but since the integration > period > depends also on the resolution and whether filters are enabled or > not, it would > have introduced some confusion: what parameter is being changed upon > integretion_time write? Maybe after removing resolution and filter > controls > there would be no such confusion anymore. > > > > + > > > +What:             > > > /sys/bus/iio/devices/iio:deviceX/integration_samples_available > > > +KernelVersion:    6.10 > > > +Contact:  linux-iio@vger.kernel.org > > > +Description: > > > +          List all possible numbers of integration samples: > > > +          "1 2 4 8 16 32 64 128 256 512 1024 2048" > > > + > > > +What:             /sys/bus/iio/devices/iio:devices/filters_en > > > +KernelVersion:    6.10 > > > +Contact:  linux-iio@vger.kernel.org > > > +Description: > > > +          Attribute to enable/disable ADC post filters. Enabled > > > by > > > +          default. > > > +          This attribute affects the integration time: with > > > filters > > > +          enabled the integration time is increased by 50%. See > > > Table 4-5 > > > +          in device datasheet for details. > > > > Do we have any idea what this filter is? Datasheet seems very vague > > indeed and from > > a control point of view that makes this largely useless. How does > > userspace know > > whether to turn it on? > > > > We have an existing filter ABI but with so little information no > > way to fit this in. > > Gut feeling, leave it on all the time and drop the control > > interface. > > I will remove this control and leave it on all the time as the HW > default. > The filters could be enabled from the device tree. As default it could be disabled. As a small detail here this is a post processing digital filter that could be enabled/disabled inside the PAC module. > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > > index f60fe85a30d5..b56e494da970 100644 > > > --- a/drivers/iio/adc/Kconfig > > > +++ b/drivers/iio/adc/Kconfig > > > @@ -991,6 +991,16 @@ config NPCM_ADC > > >      This driver can also be built as a module. If so, the > > > module > > >      will be called npcm_adc. > > > > > > +config PAC1921 > > > +  tristate "Microchip Technology PAC1921 driver" > > > +  depends on I2C > > > > > > Thanks again, > Matteo Thanks, Marius
Hi Marius, Marius.Cristea@ wrote: > > I think that the OUT pin might not be used at all in many use cases > > so I would > > leave the OUT pin setting as fixed for now and maybe extend it in the > > future > > when more use cases arise. I am open to reconsider this though. > > > > The OUT functionality could be set from the device tree. I think this should to be controlled during runtime since it's a configuration that changes the device operation mode and so also what measurements are exposed to the user. An additional DT property could be useful but I am not sure it would fit in the DT scope. Anyway I will leave this for future extensions. ... > > > > --- > > > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921         |  45 + > > > >  MAINTAINERS                                       |   7 + > > > >  drivers/iio/adc/Kconfig                           |  10 + > > > >  drivers/iio/adc/Makefile                          |   1 + > > > >  drivers/iio/adc/pac1921.c                         | 1038 > > > > ++++++++++++++++++++ > > > >  5 files changed, 1101 insertions(+) > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > new file mode 100644 > > > > index 000000000000..4a32e2d4207b > > > > --- /dev/null > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > Quite a bit of custom ABI in here. > > > > > > Rule of thumb is that custom ABI is more or less pointless ABI for > > > 99% of users > > > because standard userspace won't use it. So keep that in mind when > > > defining it. > > > > > > > @@ -0,0 +1,45 @@ > > > > +What:             > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > > +KernelVersion:    6.10 > > > > +Contact:  linux-iio@vger.kernel.org > > > > +Description: > > > > +          ADC measurement resolution. Can be either 11 bits or > > > > 14 bits > > > > +          (default). The driver sets the same resolution for > > > > both VBUS and > > > > +          VSENSE measurements even if the hardware could be > > > > configured to > > > > +          measure VBUS and VSENSE with different resolutions. > > > > +          This attribute affects the integration time: with 14 > > > > bits > > > > +          resolution the integration time is increased by a > > > > factor of > > > > +          1.9 (the driver considers a factor of 2). See Table > > > > 4-5 in > > > > +          device datasheet for details. > > > > > > Is the integration time ever high enough that it matters? > > > People tend not to do power measurement 'quickly'. > > > > > > If we are doing it quickly then you'll probably want to be > > > providing buffered > > > support and that does allow you to 'read' the resolution for a part > > > where > > > it changes for some other reason.  I haven't yet understood this > > > case. > > > > I will remove this control and fix the resolution bits to 14 (highest > > value), > > same as the HW default. > > The resolution could be set from the device tree. As default it could > be 14 bits like into the hardware. The user could add > "microchip,low_resolution_voltage" into the device tree in order to use > only 11 bits for voltage samples. I think this should be controlled during runtime since it does not depend on the HW design but more on the user preferences about measurements precision. As Jonathan pointed out, since custom ABIs should be avoided when possible, I will leave it out from now until it becomes necessary and fix the resolution to 14 bits, as the HW default. ... > > > > +What:             /sys/bus/iio/devices/iio:devices/filters_en > > > > +KernelVersion:    6.10 > > > > +Contact:  linux-iio@vger.kernel.org > > > > +Description: > > > > +          Attribute to enable/disable ADC post filters. Enabled > > > > by > > > > +          default. > > > > +          This attribute affects the integration time: with > > > > filters > > > > +          enabled the integration time is increased by 50%. See > > > > Table 4-5 > > > > +          in device datasheet for details. > > > > > > Do we have any idea what this filter is? Datasheet seems very vague > > > indeed and from > > > a control point of view that makes this largely useless. How does > > > userspace know > > > whether to turn it on? > > > > > > We have an existing filter ABI but with so little information no > > > way to fit this in. > > > Gut feeling, leave it on all the time and drop the control > > > interface. > > > > I will remove this control and leave it on all the time as the HW > > default. > > > > The filters could be enabled from the device tree. As default it could > be disabled. > As a small detail here this is a post processing digital filter that > could be enabled/disabled inside the PAC module. > Same reasoning of the resolution_bits parameter applies here. I will fix the filters to enabled, as the HW default. If there is any particular reason to prefer the filters fixed as disabled I will change that. ... > Thanks, > Marius Thanks, Matteo
Hi Matteo, On Thu, 2024-07-11 at 09:08 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi Marius, > > Marius.Cristea@ wrote: > > > I think that the OUT pin might not be used at all in many use > > > cases > > > so I would > > > leave the OUT pin setting as fixed for now and maybe extend it in > > > the > > > future > > > when more use cases arise. I am open to reconsider this though. > > > > > > > The OUT functionality could be set from the device tree. > > I think this should to be controlled during runtime since it's a > configuration > that changes the device operation mode and so also what measurements > are > exposed to the user. An additional DT property could be useful but I > am not > sure it would fit in the DT scope. > Anyway I will leave this for future extensions. > I think there are 2 different things here. Setting the configuration at startup by hard-coding things at probe time or taken those from device tree (we can add multiple properties here, as long those properties are documented into the dt-binding file) and the user controlled part at runtime. Because there is no standard interface to change the functionality, it will be easy to startup from the device tree and let the user to do some minor adjustments and not hardcode configuration. > ... > > > > > --- > > > > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921         |  45 + > > > > >  MAINTAINERS                                       |   7 + > > > > >  drivers/iio/adc/Kconfig                           |  10 + > > > > >  drivers/iio/adc/Makefile                          |   1 + > > > > >  drivers/iio/adc/pac1921.c                         | 1038 > > > > > ++++++++++++++++++++ > > > > >  5 files changed, 1101 insertions(+) > > > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc- > > > > > pac1921 > > > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > new file mode 100644 > > > > > index 000000000000..4a32e2d4207b > > > > > --- /dev/null > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > Quite a bit of custom ABI in here. > > > > > > > > Rule of thumb is that custom ABI is more or less pointless ABI > > > > for > > > > 99% of users > > > > because standard userspace won't use it. So keep that in mind > > > > when > > > > defining it. > > > > > > > > > @@ -0,0 +1,45 @@ > > > > > +What: > > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > > > +KernelVersion:    6.10 > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > +Description: > > > > > +          ADC measurement resolution. Can be either 11 bits > > > > > or > > > > > 14 bits > > > > > +          (default). The driver sets the same resolution > > > > > for > > > > > both VBUS and > > > > > +          VSENSE measurements even if the hardware could be > > > > > configured to > > > > > +          measure VBUS and VSENSE with different > > > > > resolutions. > > > > > +          This attribute affects the integration time: with > > > > > 14 > > > > > bits > > > > > +          resolution the integration time is increased by a > > > > > factor of > > > > > +          1.9 (the driver considers a factor of 2). See > > > > > Table > > > > > 4-5 in > > > > > +          device datasheet for details. > > > > > > > > Is the integration time ever high enough that it matters? > > > > People tend not to do power measurement 'quickly'. > > > > > > > > If we are doing it quickly then you'll probably want to be > > > > providing buffered > > > > support and that does allow you to 'read' the resolution for a > > > > part > > > > where > > > > it changes for some other reason.  I haven't yet understood > > > > this > > > > case. > > > > > > I will remove this control and fix the resolution bits to 14 > > > (highest > > > value), > > > same as the HW default. > > > > The resolution could be set from the device tree. As default it > > could > > be 14 bits like into the hardware. The user could add > > "microchip,low_resolution_voltage" into the device tree in order to > > use > > only 11 bits for voltage samples. > > I think this should be controlled during runtime since it does not > depend on > the HW design but more on the user preferences about measurements > precision. > As Jonathan pointed out, since custom ABIs should be avoided when > possible, I > will leave it out from now until it becomes necessary and fix the > resolution to > 14 bits, as the HW default. > Set the configuration from the device tree, will avoid custom ABI. The device tree could be changed also at runtime. > ... > > > > > +What:             > > > > > /sys/bus/iio/devices/iio:devices/filters_en > > > > > +KernelVersion:    6.10 > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > +Description: > > > > > +          Attribute to enable/disable ADC post filters. > > > > > Enabled > > > > > by > > > > > +          default. > > > > > +          This attribute affects the integration time: with > > > > > filters > > > > > +          enabled the integration time is increased by 50%. > > > > > See > > > > > Table 4-5 > > > > > +          in device datasheet for details. > > > > > > > > Do we have any idea what this filter is? Datasheet seems very > > > > vague > > > > indeed and from > > > > a control point of view that makes this largely useless. How > > > > does > > > > userspace know > > > > whether to turn it on? > > > > > > > > We have an existing filter ABI but with so little information > > > > no > > > > way to fit this in. > > > > Gut feeling, leave it on all the time and drop the control > > > > interface. > > > > > > I will remove this control and leave it on all the time as the HW > > > default. > > > > > > > The filters could be enabled from the device tree. As default it > > could > > be disabled. > > As a small detail here this is a post processing digital filter > > that > > could be enabled/disabled inside the PAC module. > > > > Same reasoning of the resolution_bits parameter applies here. I will > fix the > filters to enabled, as the HW default. If there is any particular > reason to > prefer the filters fixed as disabled I will change that. > If the user can change the on/off for the filters it doesn't matter what will be the default behavior. Being a single channel device, the probability for the user to change the filter behavior during runtime is minimal, that was the main reason for letting the user to change the configuration from the device tree and not hardcode it. > ... > > Thanks, > > Marius > > Thanks, > Matteo Thanks, Marius
On Fri, Jul 12, 2024 at 02:41:51PM +0000, Marius.Cristea@microchip.com wrote: > Hi Matteo, > > > On Thu, 2024-07-11 at 09:08 +0200, Matteo Martelli wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > Hi Marius, > > > > Marius.Cristea@ wrote: > > > > I think that the OUT pin might not be used at all in many use > > > > cases > > > > so I would > > > > leave the OUT pin setting as fixed for now and maybe extend it in > > > > the > > > > future > > > > when more use cases arise. I am open to reconsider this though. > > > > > > > > > > The OUT functionality could be set from the device tree. > > > > I think this should to be controlled during runtime since it's a > > configuration > > that changes the device operation mode and so also what measurements > > are > > exposed to the user. An additional DT property could be useful but I > > am not > > sure it would fit in the DT scope. > > Anyway I will leave this for future extensions. > > > > I think there are 2 different things here. Setting the configuration at > startup by hard-coding things at probe time or taken those from device > tree (we can add multiple properties here, as long those properties are > documented into the dt-binding file) and the user controlled part at > runtime. > Because there is no standard interface to change the functionality, it > will be easy to startup from the device tree and let the user to do > some minor adjustments and not hardcode configuration. > > > > ... > > > > > > --- > > > > > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921         |  45 + > > > > > >  MAINTAINERS                                       |   7 + > > > > > >  drivers/iio/adc/Kconfig                           |  10 + > > > > > >  drivers/iio/adc/Makefile                          |   1 + > > > > > >  drivers/iio/adc/pac1921.c                         | 1038 > > > > > > ++++++++++++++++++++ > > > > > >  5 files changed, 1101 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc- > > > > > > pac1921 > > > > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > > new file mode 100644 > > > > > > index 000000000000..4a32e2d4207b > > > > > > --- /dev/null > > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > Quite a bit of custom ABI in here. > > > > > > > > > > Rule of thumb is that custom ABI is more or less pointless ABI > > > > > for > > > > > 99% of users > > > > > because standard userspace won't use it. So keep that in mind > > > > > when > > > > > defining it. > > > > > > > > > > > @@ -0,0 +1,45 @@ > > > > > > +What: > > > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > > > > +KernelVersion:    6.10 > > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > > +Description: > > > > > > +          ADC measurement resolution. Can be either 11 bits > > > > > > or > > > > > > 14 bits > > > > > > +          (default). The driver sets the same resolution > > > > > > for > > > > > > both VBUS and > > > > > > +          VSENSE measurements even if the hardware could be > > > > > > configured to > > > > > > +          measure VBUS and VSENSE with different > > > > > > resolutions. > > > > > > +          This attribute affects the integration time: with > > > > > > 14 > > > > > > bits > > > > > > +          resolution the integration time is increased by a > > > > > > factor of > > > > > > +          1.9 (the driver considers a factor of 2). See > > > > > > Table > > > > > > 4-5 in > > > > > > +          device datasheet for details. > > > > > > > > > > Is the integration time ever high enough that it matters? > > > > > People tend not to do power measurement 'quickly'. > > > > > > > > > > If we are doing it quickly then you'll probably want to be > > > > > providing buffered > > > > > support and that does allow you to 'read' the resolution for a > > > > > part > > > > > where > > > > > it changes for some other reason.  I haven't yet understood > > > > > this > > > > > case. > > > > > > > > I will remove this control and fix the resolution bits to 14 > > > > (highest > > > > value), > > > > same as the HW default. > > > > > > The resolution could be set from the device tree. As default it > > > could > > > be 14 bits like into the hardware. The user could add > > > "microchip,low_resolution_voltage" into the device tree in order to > > > use > > > only 11 bits for voltage samples. > > > > I think this should be controlled during runtime since it does not > > depend on > > the HW design but more on the user preferences about measurements > > precision. > > As Jonathan pointed out, since custom ABIs should be avoided when > > possible, I > > will leave it out from now until it becomes necessary and fix the > > resolution to > > 14 bits, as the HW default. > > > > Set the configuration from the device tree, will avoid custom ABI. The > device tree could be changed also at runtime. Custom ABI in devicetree is not a replacement for custom ABI in userspace. If things are fixed by the hardware and non-discoverable, then sure add devicetree properties - but if it is things like "the user wants 11-bit mode", then that does not sound suitable for a devicetree property at all. And no, you can't just change the devicetree at runtime like that either as far as I understand - that's gonna cause memory leaks etc and I don't think can be done from userspace without out-of-tree patches anyway. Cheers, Conor. > > > ... > > > > > > +What:             > > > > > > /sys/bus/iio/devices/iio:devices/filters_en > > > > > > +KernelVersion:    6.10 > > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > > +Description: > > > > > > +          Attribute to enable/disable ADC post filters. > > > > > > Enabled > > > > > > by > > > > > > +          default. > > > > > > +          This attribute affects the integration time: with > > > > > > filters > > > > > > +          enabled the integration time is increased by 50%. > > > > > > See > > > > > > Table 4-5 > > > > > > +          in device datasheet for details. > > > > > > > > > > Do we have any idea what this filter is? Datasheet seems very > > > > > vague > > > > > indeed and from > > > > > a control point of view that makes this largely useless. How > > > > > does > > > > > userspace know > > > > > whether to turn it on? > > > > > > > > > > We have an existing filter ABI but with so little information > > > > > no > > > > > way to fit this in. > > > > > Gut feeling, leave it on all the time and drop the control > > > > > interface. > > > > > > > > I will remove this control and leave it on all the time as the HW > > > > default. > > > > > > > > > > The filters could be enabled from the device tree. As default it > > > could > > > be disabled. > > > As a small detail here this is a post processing digital filter > > > that > > > could be enabled/disabled inside the PAC module. > > > > > > > Same reasoning of the resolution_bits parameter applies here. I will > > fix the > > filters to enabled, as the HW default. If there is any particular > > reason to > > prefer the filters fixed as disabled I will change that. > > > If the user can change the on/off for the filters it doesn't matter > what will be the default behavior. Being a single channel device, the > probability for the user to change the filter behavior during runtime > is minimal, that was the main reason for letting the user to change the > configuration from the device tree and not hardcode it. > > > ... > > > Thanks, > > > Marius > > > > Thanks, > > Matteo > > Thanks, > Marius
On Tue, 09 Jul 2024 10:21:07 +0200 Matteo Martelli <matteomartelli3@gmail.com> wrote: > Jonathan Cameron wrote: > ... > > > I could add the shunt-resistor controls to allow calibration as Marius > > > suggested, but that's also a custom ABI, what are your thoughts on this? > > > > This would actually be a generalization of existing device specific ABI > > that has been through review in the past. > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > for example (similar in other places). > > So if you want to do this move that ABI up a level to cover multiple devices > > (removing the entries in specific files as you do so). > > > I would do this in a separate commit, would you prefer it in this same patch > set or in another separate patch? Separate commit in this series as otherwise it's not obvious why we are doing it. In theory should be before this patch as then what you use here is already documented, but I don't care that much on the order. > > ... > > > > > > > > + > > > > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available > > > > > +KernelVersion: 6.10 > > > > > +Contact: linux-iio@vger.kernel.org > > > > > +Description: > > > > > + List all possible ADC measurement resolutions: "11 14" > > > > > + > > > > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples > > > > > +KernelVersion: 6.10 > > > > > +Contact: linux-iio@vger.kernel.org > > > > > +Description: > > > > > + Number of samples taken during a full integration period. Can be > > > > > + set to any power of 2 value from 1 (default) to 2048. > > > > > + This attribute affects the integration time: higher the number > > > > > + of samples, longer the integration time. See Table 4-5 in device > > > > > + datasheet for details. > > > > > > > > Sounds like oversampling_ratio which is standards ABI. So use that or explain > > > > why you can't here. > > > > > > I am not sure that this is an oversampling ratio but correct me if I am wrong: > > > generally by increasing the oversampling you would have additional samples in a > > > fixed time period, while in this case by increasing the number of samples you > > > would still have the same number of samples in a fixed time period, but you > > > would have a longer integration period. So maybe the comment is not very > > > clear since this parameter actually means "the number of samples required to > > > complete the integration period". > > > > No. Oversampling is independent of the sampling period in general (though > > here the 'integration time' is very confusing terminology. You may > > have to have sampling_frequency (if provided) updated to incorporate that > > the device can't deliver data as quickly. > > > > > > > > Initially I thought to let the user edit this by writing the integration_time > > > control (which is currently read-only), but since the integration period > > > depends also on the resolution and whether filters are enabled or not, it would > > > have introduced some confusion: what parameter is being changed upon > > > integretion_time write? Maybe after removing resolution and filter controls > > > there would be no such confusion anymore. > > > > Hmm. The documentation seems to have an unusual definition of 'integration' time. > > That looks like 1/sampling_frequency. In an oversampling device integration time > > is normally about a single sample, not the aggregate of sampling and read out > > etc. > > > > I guess here the complexity is that integration time isn't about the time > > taken for a capacitor to charge, but more the time over which power is computed. > > But then the value is divided by number of samples so I'm even more confused. > > > > If we just read 'integration time' as data acquisition time, it makes a lot > > more sense. > > > I think I now get what you are suggesting, please correct me otherwise: > > 1. Let's consider the sampling frequency as how often the device provides > computed ("integrated") measurements to the host, so this would be > 1/"integration period". This is not the internal ADC sampling rate. > > 2. I will expose sampling_frequency (RO), oversampling_ratio (R/W) and > oversampling_ratio_available (RO) to the user, where oversampling_ratio > corresponds to what the datasheet refers to as the "number of ADC samples to > complete an integration". > > 3. When the user writes the oversampling_ratio, the sampling_frequency gets > updated accordingly. > > 4. With two real examples: > 4.1. The user writes 16 to oversampling_ratio, then reads 43.478 from > sampling_frequency: with 16 samples the "integration period" is 23ms > (from Table 4-5) so 1/0.023 => 43.478 Hz > 4.2. The user writes 2048 to oversampling_ratio, then reads 0.34 from > sampling_frequency: with 2048 samples the "integration period" is 2941ms > (from Table 4-5) so 1/2.941 => 0.34 Hz > > 5. Do not expose the integration_time control to avoid confusion: the so called > "integration period" can be derived from the sampling frequency as > 1/sampling_frequency. Yes, that seems like the best approach to me. There is always a slight confusion between trigger sampling_frequency and channel sampling_frequency but only relationship is that the combination of the channel sampling frequencies put a max limit on the trigger sampling (how often we can actually measure things). Sometimes they are the same value because of hardware contraints, sometimes not. > > ... > > > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, > > > > > + unsigned int mask, unsigned int val) > > > > > +{ > > > > > + /* Enter READ state before configuration */ > > > > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > > > + PAC1921_INT_CFG_INTEN, 0); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* Update configuration value */ > > > > > + ret = regmap_update_bits(priv->regmap, reg, mask, val); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* Re-enable integration and reset start time */ > > > > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + priv->integr_start_time = jiffies; > > > > > > > > Add a comment for why this value. > > > > > > > Could you elaborate what's confusing here? The comment above states "reset > > > start time", maybe I should move it above the assignment of > > > priv->integr_start_time? Or it's the use of jiffies that it's confusing? > > > > Why is it jiffies? Why not jiffies * 42? > > I'm looking for a datasheet reference for why the particular value is used. > > > I used jiffies just to track the elapsed time between readings. Something I am > not considering here? Of course jiffies granularity might be larger than the > minimum sampling frequency. Is there a common better approach? Ah, you are using it as the base for another comparison. Integr_start_time sounded like the time at which integration should start not when it did. integr_started_time maybe? Or perhaps I'm projecting my misread onto the naming. Maybe even call out the unit in the name as right now you have start_time in jiffies and integr_period in usecs. A postfix of _jiffies and _usecs would help avoid confusion there.
On Fri, 12 Jul 2024 18:02:54 +0100 Conor Dooley <conor@kernel.org> wrote: > On Fri, Jul 12, 2024 at 02:41:51PM +0000, Marius.Cristea@microchip.com wrote: > > Hi Matteo, > > > > > > On Thu, 2024-07-11 at 09:08 +0200, Matteo Martelli wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > Hi Marius, > > > > > > Marius.Cristea@ wrote: > > > > > I think that the OUT pin might not be used at all in many use > > > > > cases > > > > > so I would > > > > > leave the OUT pin setting as fixed for now and maybe extend it in > > > > > the > > > > > future > > > > > when more use cases arise. I am open to reconsider this though. > > > > > > > > > > > > > The OUT functionality could be set from the device tree. > > > > > > I think this should to be controlled during runtime since it's a > > > configuration > > > that changes the device operation mode and so also what measurements > > > are > > > exposed to the user. An additional DT property could be useful but I > > > am not > > > sure it would fit in the DT scope. > > > Anyway I will leave this for future extensions. > > > > > > > I think there are 2 different things here. Setting the configuration at > > startup by hard-coding things at probe time or taken those from device > > tree (we can add multiple properties here, as long those properties are > > documented into the dt-binding file) and the user controlled part at > > runtime. > > Because there is no standard interface to change the functionality, it > > will be easy to startup from the device tree and let the user to do > > some minor adjustments and not hardcode configuration. There is a quirk here. If out is wired to an ADC on the SoC for some reason then indeed it should be runtime configurable. If it's wired to some types of analog circuitry or a separate thermal monitoring micro controller then it 'might' belong in DT because that 'wiring' is not discoverable. However, this usecase isn't one anyone has 'yet' asked for so for now we have no reason to provide a binding for it. Also if this wiring is the case, we would probably not provide a userspace interface to control the pin (smoke might be the result). If it's wired to an ADC on the linux running SoC then this is definitely a userspace control thing and we've lots of examples on how to do that in tree. > > > > > > > ... > > > > > > > --- > > > > > > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921         |  45 + > > > > > > >  MAINTAINERS                                       |   7 + > > > > > > >  drivers/iio/adc/Kconfig                           |  10 + > > > > > > >  drivers/iio/adc/Makefile                          |   1 + > > > > > > >  drivers/iio/adc/pac1921.c                         | 1038 > > > > > > > ++++++++++++++++++++ > > > > > > >  5 files changed, 1101 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc- > > > > > > > pac1921 > > > > > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > > > new file mode 100644 > > > > > > > index 000000000000..4a32e2d4207b > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 > > > > > > Quite a bit of custom ABI in here. > > > > > > > > > > > > Rule of thumb is that custom ABI is more or less pointless ABI > > > > > > for > > > > > > 99% of users > > > > > > because standard userspace won't use it. So keep that in mind > > > > > > when > > > > > > defining it. > > > > > > > > > > > > > @@ -0,0 +1,45 @@ > > > > > > > +What: > > > > > > > /sys/bus/iio/devices/iio:deviceX/resolution_bits > > > > > > > +KernelVersion:    6.10 > > > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > > > +Description: > > > > > > > +          ADC measurement resolution. Can be either 11 bits > > > > > > > or > > > > > > > 14 bits > > > > > > > +          (default). The driver sets the same resolution > > > > > > > for > > > > > > > both VBUS and > > > > > > > +          VSENSE measurements even if the hardware could be > > > > > > > configured to > > > > > > > +          measure VBUS and VSENSE with different > > > > > > > resolutions. > > > > > > > +          This attribute affects the integration time: with > > > > > > > 14 > > > > > > > bits > > > > > > > +          resolution the integration time is increased by a > > > > > > > factor of > > > > > > > +          1.9 (the driver considers a factor of 2). See > > > > > > > Table > > > > > > > 4-5 in > > > > > > > +          device datasheet for details. > > > > > > > > > > > > Is the integration time ever high enough that it matters? > > > > > > People tend not to do power measurement 'quickly'. > > > > > > > > > > > > If we are doing it quickly then you'll probably want to be > > > > > > providing buffered > > > > > > support and that does allow you to 'read' the resolution for a > > > > > > part > > > > > > where > > > > > > it changes for some other reason.  I haven't yet understood > > > > > > this > > > > > > case. > > > > > > > > > > I will remove this control and fix the resolution bits to 14 > > > > > (highest > > > > > value), > > > > > same as the HW default. > > > > > > > > The resolution could be set from the device tree. As default it > > > > could > > > > be 14 bits like into the hardware. The user could add > > > > "microchip,low_resolution_voltage" into the device tree in order to > > > > use > > > > only 11 bits for voltage samples. > > > > > > I think this should be controlled during runtime since it does not > > > depend on > > > the HW design but more on the user preferences about measurements > > > precision. > > > As Jonathan pointed out, since custom ABIs should be avoided when > > > possible, I > > > will leave it out from now until it becomes necessary and fix the > > > resolution to > > > 14 bits, as the HW default. > > > > > > > Set the configuration from the device tree, will avoid custom ABI. The > > device tree could be changed also at runtime. > > Custom ABI in devicetree is not a replacement for custom ABI in userspace. > If things are fixed by the hardware and non-discoverable, then sure add > devicetree properties - but if it is things like "the user wants 11-bit > mode", then that does not sound suitable for a devicetree property at > all. Indeed, resolution doesn't belong in device tree as it has nothing to do with physical wiring, but is a policy control. I have no problem with providing a userspace ABI, but so far I've not heard a usecase for enabling it at all on this device. Who runs power measurement that needs to be a little bit faster than can be done with 14bit all the time? We have only done resolution control on devices where that resolution is really changing as a result of some other factor (oversampling ratio or similar) so it's been a read only aspect of the ABI. Note we've been round this resolution question many times and so far the number of actual usecases that have materialised is very very small. Jonathan > And no, you can't just change the devicetree at runtime like that either > as far as I understand - that's gonna cause memory leaks etc and I don't > think can be done from userspace without out-of-tree patches anyway. > > Cheers, > Conor. > > > > > > ... > > > > > > > +What:             > > > > > > > /sys/bus/iio/devices/iio:devices/filters_en > > > > > > > +KernelVersion:    6.10 > > > > > > > +Contact:  linux-iio@vger.kernel.org > > > > > > > +Description: > > > > > > > +          Attribute to enable/disable ADC post filters. > > > > > > > Enabled > > > > > > > by > > > > > > > +          default. > > > > > > > +          This attribute affects the integration time: with > > > > > > > filters > > > > > > > +          enabled the integration time is increased by 50%. > > > > > > > See > > > > > > > Table 4-5 > > > > > > > +          in device datasheet for details. > > > > > > > > > > > > Do we have any idea what this filter is? Datasheet seems very > > > > > > vague > > > > > > indeed and from > > > > > > a control point of view that makes this largely useless. How > > > > > > does > > > > > > userspace know > > > > > > whether to turn it on? > > > > > > > > > > > > We have an existing filter ABI but with so little information > > > > > > no > > > > > > way to fit this in. > > > > > > Gut feeling, leave it on all the time and drop the control > > > > > > interface. > > > > > > > > > > I will remove this control and leave it on all the time as the HW > > > > > default. > > > > > > > > > > > > > The filters could be enabled from the device tree. As default it > > > > could > > > > be disabled. > > > > As a small detail here this is a post processing digital filter > > > > that > > > > could be enabled/disabled inside the PAC module. > > > > > > > > > > Same reasoning of the resolution_bits parameter applies here. I will > > > fix the > > > filters to enabled, as the HW default. If there is any particular > > > reason to > > > prefer the filters fixed as disabled I will change that. > > > > > If the user can change the on/off for the filters it doesn't matter > > what will be the default behavior. Being a single channel device, the > > probability for the user to change the filter behavior during runtime > > is minimal, that was the main reason for letting the user to change the > > configuration from the device tree and not hardcode it. > > > > > ... > > > > Thanks, > > > > Marius > > > > > > Thanks, > > > Matteo > > > > Thanks, > > Marius
Jonathan Cameron wrote: > On Tue, 09 Jul 2024 10:21:07 +0200 > Matteo Martelli <matteomartelli3@gmail.com> wrote: > > > Jonathan Cameron wrote: > > ... > > > > I could add the shunt-resistor controls to allow calibration as Marius > > > > suggested, but that's also a custom ABI, what are your thoughts on this? > > > > > > This would actually be a generalization of existing device specific ABI > > > that has been through review in the past. > > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > > for example (similar in other places). > > > So if you want to do this move that ABI up a level to cover multiple devices > > > (removing the entries in specific files as you do so). > > > > > I would do this in a separate commit, would you prefer it in this same patch > > set or in another separate patch? > > Separate commit in this series as otherwise it's not obvious why we are > doing it. In theory should be before this patch as then what you use here > is already documented, but I don't care that much on the order. > Just a few more questions about this point. * I see 3 other drivers exposing the shunt resistor attribute: ina2xx, max9611 and pac1934. While the unit for first two is in Ohms, for the latter it's in micro-Ohms. What should be the unit for the generalized ABI? I would guess Ohms as /sys/bus/iio/devices/iio:deviceX/in_resistance_raw. * If for instance the generalized ABI unit is going to be Ohms, should I still remove the entry from the pac1934 even though it would not be fully compliant with the generalized ABI? * To cover the current exposed attributes, the "What" fields would look like: from max9611: What: /sys/.../iio:deviceX/in_current_shunt_resistor What: /sys/.../iio:deviceX/in_power_shunt_resistor from ina2xx: What: /sys/.../iio:deviceX/in_shunt_resistor from pac1934: What: /sys/.../iio:deviceX/in_shunt_resistorY Does this look correct? I think that for the first two drivers the shunt_resistor can be considered as a channel info property, shared by type for max9611 case and shared by direction for ina2xx case (maybe better to remove "in_" from the What field if the type is not specified?). What seems odd to me is the pac1934 case, since it doesn't fit in the format <type>[Y_]shunt_resistor referred in many other attributes (where I assume <type> is actually [dir_][type_]?). Doesn't it look like pac1934 is exposing additional input channels, that are also writeable? Maybe such case would more clear if the shunt resistor would be an info property of specific channels? For example: in_currentY_shunt_resistor, in_powerY_shunt_resistor and in_engergyY_shunt_resitor. * I would go for a simple and generic description such as: "The value of current sense resistor in Ohms." like it is in Documentation/devicetree/bindings/hwmon/hwmon-common.yaml. Should it include any additional detail? * I am assuming the generalized API would have Date and KernelVersion of today even though the original attributes are older. * Should this ABI be inserted at any particular place of Documentation/ABI/testing/sysfs-bus-iio or just appended at its end? Thanks, Matteo
Hi Matteo, On Tue, 2024-07-16 at 11:20 +0200, Matteo Martelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Jonathan Cameron wrote: > > On Tue, 09 Jul 2024 10:21:07 +0200 > > Matteo Martelli <matteomartelli3@gmail.com> wrote: > > > > > Jonathan Cameron wrote: > > > ... > > > > > I could add the shunt-resistor controls to allow calibration > > > > > as Marius > > > > > suggested, but that's also a custom ABI, what are your > > > > > thoughts on this? > > > > > > > > This would actually be a generalization of existing device > > > > specific ABI > > > > that has been through review in the past. > > > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > > > for example (similar in other places). > > > > So if you want to do this move that ABI up a level to cover > > > > multiple devices > > > > (removing the entries in specific files as you do so). > > > > > > > I would do this in a separate commit, would you prefer it in this > > > same patch > > > set or in another separate patch? > > > > Separate commit in this series as otherwise it's not obvious why we > > are > > doing it. In theory should be before this patch as then what you > > use here > > is already documented, but I don't care that much on the order. > > > Just a few more questions about this point. > > * I see 3 other drivers exposing the shunt resistor attribute: > ina2xx, max9611 > and pac1934. While the unit for first two is in Ohms, for the latter > it's in > micro-Ohms. What should be the unit for the generalized ABI? I would > guess Ohms For measuring current the usual "scale" is part of miliOhms in order to reduce the power dissipation. As a rule of thumb 0.1 miliOhms is a usual value for shunt resistors. I think the "correct" way is to setup the value in sub units of Ohms. Like the current is in miliAmps and the voltage is in miliVolts. > as /sys/bus/iio/devices/iio:deviceX/in_resistance_raw. > > * If for instance the generalized ABI unit is going to be Ohms, > should I still > remove the entry from the pac1934 even though it would not be fully > compliant > with the generalized ABI? > > * To cover the current exposed attributes, the "What" fields would > look like: > from max9611: > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_current_shunt_resistor > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_power_shunt_resistor > from ina2xx: > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistor > from pac1934: > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistorY > Does this look correct? I think that for the first two drivers the > shunt_resistor can be considered as a channel info property, shared > by type for > max9611 case and shared by direction for ina2xx case (maybe better to > remove > "in_" from the What field if the type is not specified?). > What seems odd to me is the pac1934 case, since it doesn't fit in the > format > <type>[Y_]shunt_resistor referred in many other attributes (where I > assume > <type> is actually [dir_][type_]?). > Doesn't it look like pac1934 is exposing additional input channels, > that are > also writeable? Maybe such case would more clear if the shunt > resistor would be > an info property of specific channels? For example: > in_currentY_shunt_resistor, > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > I don't think it will be a good idea to duplicate the same information into multiple attributes like: in_currentY_shunt_resistor, in_powerY_shunt_resistor and in_engergyY_shunt_resitor. The pac1934 device could be viewed like 4 devices that have only one measurement hardware. Changing the shunt for a hardware channel will impact multiple software measurements for that particular channel. For example "sampling_frequency" is only one property per device and not one property per channel. Also I'm not felling comfortable to remove the [dir_] from the name, because this value is dependent of the hardware and we can't have a "available" properties for it. > * I would go for a simple and generic description such as: > "The value of current sense resistor in Ohms." like it is in > Documentation/devicetree/bindings/hwmon/hwmon-common.yaml. Should it > include > any additional detail? > > * I am assuming the generalized API would have Date and KernelVersion > of > today even though the original attributes are older. > > * Should this ABI be inserted at any particular place of > Documentation/ABI/testing/sysfs-bus-iio or just appended at its end? > > Thanks, > Matteo >
On Tue, 16 Jul 2024 11:19:53 +0000 <Marius.Cristea@microchip.com> wrote: > Hi Matteo, > > On Tue, 2024-07-16 at 11:20 +0200, Matteo Martelli wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > Jonathan Cameron wrote: > > > On Tue, 09 Jul 2024 10:21:07 +0200 > > > Matteo Martelli <matteomartelli3@gmail.com> wrote: > > > > > > > Jonathan Cameron wrote: > > > > ... > > > > > > I could add the shunt-resistor controls to allow calibration > > > > > > as Marius > > > > > > suggested, but that's also a custom ABI, what are your > > > > > > thoughts on this? > > > > > > > > > > This would actually be a generalization of existing device > > > > > specific ABI > > > > > that has been through review in the past. > > > > > See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 > > > > > for example (similar in other places). > > > > > So if you want to do this move that ABI up a level to cover > > > > > multiple devices > > > > > (removing the entries in specific files as you do so). > > > > > > > > > I would do this in a separate commit, would you prefer it in this > > > > same patch > > > > set or in another separate patch? > > > > > > Separate commit in this series as otherwise it's not obvious why we > > > are > > > doing it. In theory should be before this patch as then what you > > > use here > > > is already documented, but I don't care that much on the order. > > > > > Just a few more questions about this point. > > > > * I see 3 other drivers exposing the shunt resistor attribute: > > ina2xx, max9611 > > and pac1934. While the unit for first two is in Ohms, for the latter > > it's in > > micro-Ohms. What should be the unit for the generalized ABI? I would > > guess Ohms > > For measuring current the usual "scale" is part of miliOhms in order to > reduce the power dissipation. As a rule of thumb 0.1 miliOhms is a > usual value for shunt resistors. I think the "correct" way is to setup > the value in sub units of Ohms. Like the current is in miliAmps and > the voltage is in miliVolts. The milli thing is historical curiosity (we were trying to maintain interface compatibility with hwmon briefly many years ago - it rapidly proved impractical). For almost all remotely recent units we have gone with the SI base unit. This is an corner of ABI so if things were consistent I'd say copy the others, but failing that ohms for the reason you give. Copy the in_resistance_raw value > > > > as /sys/bus/iio/devices/iio:deviceX/in_resistance_raw. > > > > * If for instance the generalized ABI unit is going to be Ohms, > > should I still > > remove the entry from the pac1934 even though it would not be fully > > compliant > > with the generalized ABI? > > > > * To cover the current exposed attributes, the "What" fields would > > look like: > > from max9611: > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_current_shunt_resistor > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_power_shunt_resistor > > from ina2xx: > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistor > > from pac1934: > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistorY This one is a bit odd in that it describes it if it were a measurable channel in of itself but we probably couldn't figure out a better way to scope it to a specific channel. > > Does this look correct? I think that for the first two drivers the > > shunt_resistor can be considered as a channel info property, shared > > by type for > > max9611 case and shared by direction for ina2xx case (maybe better to > > remove > > "in_" from the What field if the type is not specified?). Keep it consistent. It's valid to provide the in_ and in general over restrict channel attributes, even if not strictly necessary. > > What seems odd to me is the pac1934 case, since it doesn't fit in the > > format > > <type>[Y_]shunt_resistor referred in many other attributes (where I > > assume > > <type> is actually [dir_][type_]?). > > Doesn't it look like pac1934 is exposing additional input channels, > > that are > > also writeable? Maybe such case would more clear if the shunt > > resistor would be > > an info property of specific channels? For example: > > in_currentY_shunt_resistor, > > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > > > I don't think it will be a good idea to duplicate the same information > into multiple attributes like: in_currentY_shunt_resistor, > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > The pac1934 device could be viewed like 4 devices that have only one > measurement hardware. Changing the shunt for a hardware channel will > impact multiple software measurements for that particular channel. Yup. You've > > For example "sampling_frequency" is only one property per device and > not one property per channel. Not necessarily. If it varies per channel it is in_voltageX_sampling_frequency etc That is rare, but we have specific text to cover it in the ABI docs. What: /sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency KernelVersion: 5.20 Contact: linux-iio@vger.kernel.org Description: Some devices have separate controls of sampling frequency for individual channels. If multiple channels are enabled in a scan, then the sampling_frequency of the scan may be computed from the per channel sampling frequencies. > > Also I'm not felling comfortable to remove the [dir_] from the name, > because this value is dependent of the hardware and we can't have a > "available" properties for it. Removing the dir is unnecessary. Just leave that in place. Note we can't change existing ABI of drivers for this sort of thing that wasn't standardized (as we can't argue they break ABI) so they are stuck as they stand. Unfortunately the most consistent path is probably to treat it as a normal attribute, even if that generates a bunch of silly duplication if there is more than one shunt_resistance. I agree it's ugly but it's not the only case of this sort of duplication. It happens for that sampling_frequency case in a few corners were there is on channel that is sampled different from all the others. So I think in_powerY_shut_resistor and in_energyY_shunt_resistor is most consistent with existing 'standard' ABI. This is one where I didn't do a great job in review unfortunately so the one with the index on the end got through. I'm not hugely worried about this mess though as runtime shunt resistor calibration is not that common. If people want good measurements they tend to build their circuit with good components / PCB tracks etc. > > > > > * I would go for a simple and generic description such as: > > "The value of current sense resistor in Ohms." like it is in > > Documentation/devicetree/bindings/hwmon/hwmon-common.yaml. Should it > > include > > any additional detail? That sounds good. It is permissible when generalizing ABI like this to include any particular quirks of the ones you are generalizing. I doubt there are any here though. > > > > * I am assuming the generalized API would have Date and KernelVersion > > of > > today even though the original attributes are older. > > > > * Should this ABI be inserted at any particular place of > > Documentation/ABI/testing/sysfs-bus-iio or just appended at its end? End is fine for this one. We've never attempted to assign an order to that file so grouping only occurs if there are related attributes and I don't see this as closely related to much else. Thanks, Jonathan > > > > Thanks, > > Matteo > > >
Jonathan Cameron wrote: > > > > > > * If for instance the generalized ABI unit is going to be Ohms, > > > should I still > > > remove the entry from the pac1934 even though it would not be fully > > > compliant > > > with the generalized ABI? > > > > > > * To cover the current exposed attributes, the "What" fields would > > > look like: > > > from max9611: > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_current_shunt_resistor > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_power_shunt_resistor > > > from ina2xx: > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistor > > > from pac1934: > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistorY > > This one is a bit odd in that it describes it if it were a measurable > channel in of itself but we probably couldn't figure out a better way > to scope it to a specific channel. > > > > Does this look correct? I think that for the first two drivers the > > > shunt_resistor can be considered as a channel info property, shared > > > by type for > > > max9611 case and shared by direction for ina2xx case (maybe better to > > > remove > > > "in_" from the What field if the type is not specified?). > > Keep it consistent. It's valid to provide the in_ and in general > over restrict channel attributes, even if not strictly necessary. > > > > What seems odd to me is the pac1934 case, since it doesn't fit in the > > > format > > > <type>[Y_]shunt_resistor referred in many other attributes (where I > > > assume > > > <type> is actually [dir_][type_]?). > > > Doesn't it look like pac1934 is exposing additional input channels, > > > that are > > > also writeable? Maybe such case would more clear if the shunt > > > resistor would be > > > an info property of specific channels? For example: > > > in_currentY_shunt_resistor, > > > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > > > > > > > I don't think it will be a good idea to duplicate the same information > > into multiple attributes like: in_currentY_shunt_resistor, > > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > > > The pac1934 device could be viewed like 4 devices that have only one > > measurement hardware. Changing the shunt for a hardware channel will > > impact multiple software measurements for that particular channel. > Yup. You've Sorry Jonathan, is there anything missing in this sentence? Looks like unintentionally truncated: You've ... > > > > For example "sampling_frequency" is only one property per device and > > not one property per channel. > > Not necessarily. If it varies per channel it is > in_voltageX_sampling_frequency etc > That is rare, but we have specific text to cover it in the ABI docs. > > What: /sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency > What: /sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency > What: /sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency > KernelVersion: 5.20 > Contact: linux-iio@vger.kernel.org > Description: > Some devices have separate controls of sampling frequency for > individual channels. If multiple channels are enabled in a scan, > then the sampling_frequency of the scan may be computed from the > per channel sampling frequencies. > > > > > Also I'm not felling comfortable to remove the [dir_] from the name, > > because this value is dependent of the hardware and we can't have a > > "available" properties for it. > Removing the dir is unnecessary. Just leave that in place. > Note we can't change existing ABI of drivers for this sort of thing > that wasn't standardized (as we can't argue they break ABI) so > they are stuck as they stand. > > Unfortunately the most consistent path is probably to treat it as a > normal attribute, even if that generates a bunch of silly duplication > if there is more than one shunt_resistance. > I agree it's ugly but it's not the only case of this sort of duplication. > It happens for that sampling_frequency case in a few corners were there is > on channel that is sampled different from all the others. > > So I think > in_powerY_shut_resistor and in_energyY_shunt_resistor is > most consistent with existing 'standard' ABI. > > This is one where I didn't do a great job in review unfortunately > so the one with the index on the end got through. > > I'm not hugely worried about this mess though as runtime shunt resistor > calibration is not that common. If people want good measurements they > tend to build their circuit with good components / PCB tracks etc. > From your comments I get that in_shunt_resistorY should be added in the generalized ABI (as in the example above) since it is already used and can't be changed. Is this correct? I am still not sure whether in_currentY_shunt_resistor, in_powerY_shunt_resistor and in_energyY_shunt_resistor, should be added or not until a new driver using it comes through. Regarding pac1921, would it be more clear to expose a single in_shunt_resistor (keeping [dir_] for consistency as you suggested) as it is for ina2xx or in_current_shunt_resistor plus in_power_shunt_resistor as it is for max9611? I agree that just exposing it once would be more clear for the user, so I would go for the first case but maybe I am missing something. > > Thanks, > > Jonathan > Thanks, Matteo
On Wed, 17 Jul 2024 16:22:40 +0200 Matteo Martelli <matteomartelli3@gmail.com> wrote: > Jonathan Cameron wrote: Oddly I thought I'd replied to this already but my email client says not... I guess maybe I have a stray draft on another computer. Anyhow, let's try again! > > > > > > > > * If for instance the generalized ABI unit is going to be Ohms, > > > > should I still > > > > remove the entry from the pac1934 even though it would not be fully > > > > compliant > > > > with the generalized ABI? > > > > > > > > * To cover the current exposed attributes, the "What" fields would > > > > look like: > > > > from max9611: > > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_current_shunt_resistor > > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_power_shunt_resistor > > > > from ina2xx: > > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistor > > > > from pac1934: > > > > What:Â Â Â Â Â Â Â Â /sys/.../iio:deviceX/in_shunt_resistorY > > > > This one is a bit odd in that it describes it if it were a measurable > > channel in of itself but we probably couldn't figure out a better way > > to scope it to a specific channel. > > > > > > Does this look correct? I think that for the first two drivers the > > > > shunt_resistor can be considered as a channel info property, shared > > > > by type for > > > > max9611 case and shared by direction for ina2xx case (maybe better to > > > > remove > > > > "in_" from the What field if the type is not specified?). > > > > Keep it consistent. It's valid to provide the in_ and in general > > over restrict channel attributes, even if not strictly necessary. > > > > > > What seems odd to me is the pac1934 case, since it doesn't fit in the > > > > format > > > > <type>[Y_]shunt_resistor referred in many other attributes (where I > > > > assume > > > > <type> is actually [dir_][type_]?). > > > > Doesn't it look like pac1934 is exposing additional input channels, > > > > that are > > > > also writeable? Maybe such case would more clear if the shunt > > > > resistor would be > > > > an info property of specific channels? For example: > > > > in_currentY_shunt_resistor, > > > > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > > > > > > > > > > > I don't think it will be a good idea to duplicate the same information > > > into multiple attributes like: in_currentY_shunt_resistor, > > > in_powerY_shunt_resistor and in_engergyY_shunt_resitor. > > > > > > The pac1934 device could be viewed like 4 devices that have only one > > > measurement hardware. Changing the shunt for a hardware channel will > > > impact multiple software measurements for that particular channel. > > Yup. You've > > Sorry Jonathan, is there anything missing in this sentence? Looks like > unintentionally truncated: You've ... Bad editing of my reply!. Ignore that. > > > > > > > For example "sampling_frequency" is only one property per device and > > > not one property per channel. > > > > Not necessarily. If it varies per channel it is > > in_voltageX_sampling_frequency etc > > That is rare, but we have specific text to cover it in the ABI docs. > > > > What: /sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency > > What: /sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency > > What: /sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency > > KernelVersion: 5.20 > > Contact: linux-iio@vger.kernel.org > > Description: > > Some devices have separate controls of sampling frequency for > > individual channels. If multiple channels are enabled in a scan, > > then the sampling_frequency of the scan may be computed from the > > per channel sampling frequencies. > > > > > > > > Also I'm not felling comfortable to remove the [dir_] from the name, > > > because this value is dependent of the hardware and we can't have a > > > "available" properties for it. > > Removing the dir is unnecessary. Just leave that in place. > > Note we can't change existing ABI of drivers for this sort of thing > > that wasn't standardized (as we can't argue they break ABI) so > > they are stuck as they stand. > > > > Unfortunately the most consistent path is probably to treat it as a > > normal attribute, even if that generates a bunch of silly duplication > > if there is more than one shunt_resistance. > > I agree it's ugly but it's not the only case of this sort of duplication. > > It happens for that sampling_frequency case in a few corners were there is > > on channel that is sampled different from all the others. > > > > So I think > > in_powerY_shut_resistor and in_energyY_shunt_resistor is > > most consistent with existing 'standard' ABI. > > > > This is one where I didn't do a great job in review unfortunately > > so the one with the index on the end got through. > > > > I'm not hugely worried about this mess though as runtime shunt resistor > > calibration is not that common. If people want good measurements they > > tend to build their circuit with good components / PCB tracks etc. > > > > From your comments I get that in_shunt_resistorY should be added in the > generalized ABI (as in the example above) since it is already used and can't be > changed. Is this correct? No. for the one that isn't compliant with our generalization, just leave it where it is in a per device doc. > > I am still not sure whether in_currentY_shunt_resistor, > in_powerY_shunt_resistor and in_energyY_shunt_resistor, should be added or not > until a new driver using it comes through. Ah. I wasn't paying attention to what was needed here. If you don't need them then no need to define them. > > Regarding pac1921, would it be more clear to expose a single in_shunt_resistor > (keeping [dir_] for consistency as you suggested) as it is for ina2xx or > in_current_shunt_resistor plus in_power_shunt_resistor as it is for max9611? I > agree that just exposing it once would be more clear for the user, so I would > go for the first case but maybe I am missing something. It's an interesting question. Is it obvious enough that the shut resistor affects both current and power measurements? I think it is and in general shunt resistor tuning is fairly uncommon thing so just in_current_shunt_resistor sounds fine to me. Jonathan > > > > > Thanks, > > > > Jonathan > > > > Thanks, > Matteo
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 new file mode 100644 index 000000000000..4a32e2d4207b --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 @@ -0,0 +1,45 @@ +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits +KernelVersion: 6.10 +Contact: linux-iio@vger.kernel.org +Description: + ADC measurement resolution. Can be either 11 bits or 14 bits + (default). The driver sets the same resolution for both VBUS and + VSENSE measurements even if the hardware could be configured to + measure VBUS and VSENSE with different resolutions. + This attribute affects the integration time: with 14 bits + resolution the integration time is increased by a factor of + 1.9 (the driver considers a factor of 2). See Table 4-5 in + device datasheet for details. + +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available +KernelVersion: 6.10 +Contact: linux-iio@vger.kernel.org +Description: + List all possible ADC measurement resolutions: "11 14" + +What: /sys/bus/iio/devices/iio:deviceX/integration_samples +KernelVersion: 6.10 +Contact: linux-iio@vger.kernel.org +Description: + Number of samples taken during a full integration period. Can be + set to any power of 2 value from 1 (default) to 2048. + This attribute affects the integration time: higher the number + of samples, longer the integration time. See Table 4-5 in device + datasheet for details. + +What: /sys/bus/iio/devices/iio:deviceX/integration_samples_available +KernelVersion: 6.10 +Contact: linux-iio@vger.kernel.org +Description: + List all possible numbers of integration samples: + "1 2 4 8 16 32 64 128 256 512 1024 2048" + +What: /sys/bus/iio/devices/iio:devices/filters_en +KernelVersion: 6.10 +Contact: linux-iio@vger.kernel.org +Description: + Attribute to enable/disable ADC post filters. Enabled by + default. + This attribute affects the integration time: with filters + enabled the integration time is increased by 50%. See Table 4-5 + in device datasheet for details. diff --git a/MAINTAINERS b/MAINTAINERS index 0cbeb03847f5..a737dc00e29c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14793,6 +14793,13 @@ F: Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml F: drivers/nvmem/microchip-otpc.c F: include/dt-bindings/nvmem/microchip,sama7g5-otpc.h +MICROCHIP PAC1921 POWER/CURRENT MONITOR DRIVER +M: Matteo Martelli <matteomartelli3@gmail.com> +L: linux-iio@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml +F: drivers/iio/adc/pac1921.c + MICROCHIP PAC1934 POWER/ENERGY MONITOR DRIVER M: Marius Cristea <marius.cristea@microchip.com> L: linux-iio@vger.kernel.org diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index f60fe85a30d5..b56e494da970 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -991,6 +991,16 @@ config NPCM_ADC This driver can also be built as a module. If so, the module will be called npcm_adc. +config PAC1921 + tristate "Microchip Technology PAC1921 driver" + depends on I2C + help + Say yes here to build support for Microchip Technology's PAC1921 + High-Side Power/Current Monitor with Analog Output. + + This driver can also be built as a module. If so, the module + will be called pac1921. + config PAC1934 tristate "Microchip Technology PAC1934 driver" depends on I2C diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index d370e066544e..5af30eeff262 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -90,6 +90,7 @@ obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o +obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.o obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c new file mode 100644 index 000000000000..879753466093 --- /dev/null +++ b/drivers/iio/adc/pac1921.c @@ -0,0 +1,1038 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * IIO driver for PAC1921 High-Side Power/Current Monitor + * + * Copyright (C) 2024 Matteo Martelli <matteomartelli3@gmail.com> + */ + +#include <linux/i2c.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/regmap.h> + +/* pac1921 registers */ +#define PAC1921_REG_GAIN_CFG 0x00 +#define PAC1921_REG_INT_CFG 0x01 +#define PAC1921_REG_CONTROL 0x02 +#define PAC1921_REG_VBUS 0x10 +#define PAC1921_REG_VSENSE 0x12 +#define PAC1921_REG_OVERFLOW_STS 0x1C +#define PAC1921_REG_VPOWER 0x1D + +/* pac1921 gain configuration bits */ +#define PAC1921_GAIN_I_RES BIT(7) +#define PAC1921_GAIN_V_RES BIT(6) +#define PAC1921_GAIN_DI_GAIN_SHIFT 3 +#define PAC1921_GAIN_DI_GAIN_MASK GENMASK(5, PAC1921_GAIN_DI_GAIN_SHIFT) +#define PAC1921_GAIN_DI_GAIN_MAX 7 +#define PAC1921_GAIN_DV_GAIN_SHIFT 0 +#define PAC1921_GAIN_DV_GAIN_MASK GENMASK(2, PAC1921_GAIN_DV_GAIN_SHIFT) +#define PAC1921_GAIN_DV_GAIN_MAX 5 + +/* pac1921 integration configuration bits */ +#define PAC1921_INT_CFG_SMPL_SHIFT 4 +#define PAC1921_INT_CFG_SMPL_MASK GENMASK(7, PAC1921_INT_CFG_SMPL_SHIFT) +#define PAC1921_INT_CFG_SMPL_MAX 11 +#define PAC1921_INT_CFG_VSFEN BIT(3) +#define PAC1921_INT_CFG_VBFEN BIT(2) +#define PAC1921_INT_CFG_RIOV BIT(1) +#define PAC1921_INT_CFG_INTEN BIT(0) + +/* pac1921 control bits */ +#define PAC1921_CONTROL_MXSL_SHIFT 6 +enum pac1921_mxsl { + PAC1921_MXSL_VPOWER_PIN = 0, + PAC1921_MXSL_VSENSE_FREE_RUN = 1, + PAC1921_MXSL_VBUS_FREE_RUN = 2, + PAC1921_MXSL_VPOWER_FREE_RUN = 3, +}; +#define PAC1921_CONTROL_SLEEP BIT(2) + +/* pac1921 overflow status bits */ +#define PAC1921_OVERFLOW_VSOV BIT(2) +#define PAC1921_OVERFLOW_VBOV BIT(1) +#define PAC1921_OVERFLOW_VPOV BIT(0) + +/* pac1921 constants */ +#define PAC1921_MAX_VSENSE_MV 100 +#define PAC1921_MAX_VBUS_V 32 +#define PAC1921_RES_RESOLUTION 1023 /* Result registers resolution */ + +/* pac1921 defaults */ +#define PAC1921_DEFAULT_DV_GAIN 0 /* 2^(value): 1x gain */ +#define PAC1921_DEFAULT_DI_GAIN 0 /* 2^(value): 1x gain */ +#define PAC1921_DEFAULT_HIGH_RES true /* 14-bit measurement resolution */ +#define PAC1921_DEFAULT_NUM_SAMPLES 0 /* 2^(value): 1 sample */ +#define PAC1921_DEFAULT_FILTERS_ENABLED true + +/* pac1921 tables to create iio available parameters */ +static const unsigned int pac1921_di_gains[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; +static const unsigned int pac1921_dv_gains[] = { 1, 2, 4, 8, 16, 32 }; +enum pac1921_meas_resolution_idx { + PAC1921_MEAS_RESOLUTION_IDX_LOW = 0, + PAC1921_MEAS_RESOLUTION_IDX_HIGH, +}; +static const char *const pac1921_meas_resolutions[] = { "11", "14" }; +static const char *const pac1921_integr_num_samples[] = { + "1", "2", "4", "8", "16", "32", + "64", "128", "256", "512", "1024", "2048" +}; + +/* pac1921 regmap configuration */ +static const struct regmap_range pac1921_regmap_wr_ranges[] = { + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), +}; +static const struct regmap_access_table pac1921_regmap_wr_table = { + .yes_ranges = pac1921_regmap_wr_ranges, + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges), +}; +static const struct regmap_range pac1921_regmap_rd_ranges[] = { + regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL), + regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1), +}; +static const struct regmap_access_table pac1921_regmap_rd_table = { + .yes_ranges = pac1921_regmap_rd_ranges, + .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges), +}; +static const struct regmap_config pac1921_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .rd_table = &pac1921_regmap_rd_table, + .wr_table = &pac1921_regmap_wr_table, +}; + +enum pac1921_channels { + PAC1921_CHAN_VBUS = 0, + PAC1921_CHAN_VSENSE = 1, + PAC1921_CHAN_CURRENT = 2, + PAC1921_CHAN_POWER = 3, +}; +#define PAC1921_NUM_MEAS_CHANS 4 + +struct pac1921_priv { + struct i2c_client *client; + struct regmap *regmap; + struct regulator *vdd; + struct iio_info iio_info; + + /* Synchronize access to private members, and ensure atomicity of + * consecutive regmap operations. + */ + struct mutex lock; + + u32 rshunt; /* uOhm */ + bool high_res; + u8 dv_gain; + u8 di_gain; + u8 n_samples; + bool filters_en; + u8 prev_ovf_flags; + + bool first_integr_started; + bool first_integr_done; + unsigned long integr_start_time; /* jiffies */ + unsigned int integr_period; /* usecs */ + + struct { + u16 chan[PAC1921_NUM_MEAS_CHANS]; + s64 timestamp __aligned(8); + } scan; +}; + +/* The integration period depends on the configuration of number of integration + * samples, measurement resolution and post filters. The following array + * contains integration periods, in microsecs unit, based on table 4-5 from + * datasheet considering power integration mode, 11-Bit resolution and post + * filters off. Each index corresponds to a specific number of samples from 1 + * to 2048. + */ +static const unsigned int pac1921_integr_periods_us[] = { + 930, 1460, 2410, 4320, 8050, 16100, + 32100, 64200, 128300, 257000, 513000, 1026000 +}; + +/* Calculate and update the integration period + * + * The base integration period is retrieved from pac1921_integr_periods_us + * array, then doubled if 14-Bit resolution is used (even if the actual + * increase factor is about 1.9) and increased by 50% if post filters are + * enabled. + * + * Must be called with lock held. + */ +static int pac1921_update_integr_period(struct pac1921_priv *priv) +{ + if (WARN_ON_ONCE(priv->n_samples >= + ARRAY_SIZE(pac1921_integr_periods_us))) + return -EINVAL; + + priv->integr_period = pac1921_integr_periods_us[priv->n_samples]; + if (priv->high_res) + priv->integr_period *= 2; + if (priv->filters_en) + priv->integr_period += priv->integr_period / 2; + + return 0; +} + +/* Check if first integration after configuration update has completed. + * + * Must be called with lock held. + */ +static bool pac1921_data_ready(struct pac1921_priv *priv) +{ + if (!priv->first_integr_started) + return false; + + if (!priv->first_integr_done) { + unsigned long t_ready = priv->integr_start_time + + usecs_to_jiffies(priv->integr_period); + if (time_before(jiffies, t_ready)) + /* First integration not completed: data not ready */ + return false; + + priv->first_integr_done = true; + } + + return true; +} + +/* Check if overflow occurred and if so, push the corresponding events. + * + * Must be called with lock held. + */ +static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + unsigned int flags; + + int ret = regmap_read(priv->regmap, PAC1921_REG_OVERFLOW_STS, &flags); + if (ret) + return ret; + + if (flags & PAC1921_OVERFLOW_VBOV && + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VBOV)) { + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE( + IIO_VOLTAGE, PAC1921_CHAN_VBUS, + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), + timestamp); + } + if (flags & PAC1921_OVERFLOW_VSOV && + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VSOV)) { + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE( + IIO_VOLTAGE, PAC1921_CHAN_VSENSE, + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), + timestamp); + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE( + IIO_CURRENT, PAC1921_CHAN_CURRENT, + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), + timestamp); + } + if (flags & PAC1921_OVERFLOW_VPOV && + !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VPOV)) { + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE( + IIO_POWER, PAC1921_CHAN_POWER, + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), + timestamp); + } + + priv->prev_ovf_flags = (u8)flags; + + return 0; +} + +/* Read the value from a result register + * + * Result registers contain the most recent averaged values of Vbus, Vsense and + * Vpower. Each value is 10 bits wide and spread across two consecutive 8 bit + * registers, with 6 bit LSB zero padding. + */ +static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg, + int *val) +{ + u8 val_buf[2]; + + int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, &val_buf, + sizeof(val_buf)); + if (ret) + return ret; + + *val = (val_buf[0] << 8 | val_buf[1]) >> 6; + + return 0; +} + +/* Get the Vsense LSB in nV + * + * Used to calculate scale factors for current and power measurements + */ +static inline int pac1921_vsense_lsb(u8 di_gain) +{ + int max = (PAC1921_MAX_VSENSE_MV * 1000000) >> (int)di_gain; + + return DIV_ROUND_CLOSEST(max, PAC1921_RES_RESOLUTION); +} + +static int pac1921_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: { + guard(mutex)(&priv->lock); + + if (!pac1921_data_ready(priv)) + return -EBUSY; + + s64 ts = iio_get_time_ns(indio_dev); + + int ret = pac1921_check_push_overflow(indio_dev, ts); + if (ret) + return ret; + + ret = pac1921_read_res(priv, chan->address, val); + if (ret) + return ret; + + return IIO_VAL_INT; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->channel) { + case PAC1921_CHAN_VBUS: { + /* Vbus scale factor in mV: + * max_vbus (mV) / vgain / resolution + */ + guard(mutex)(&priv->lock); + + *val = PAC1921_MAX_VBUS_V * 1000; + *val2 = PAC1921_RES_RESOLUTION << (int)priv->dv_gain; + + return IIO_VAL_FRACTIONAL; + } + case PAC1921_CHAN_VSENSE: { + /* Vsense voltage scale factor in mV: + * max_vsense (mV) / igain / resolution + */ + guard(mutex)(&priv->lock); + + *val = PAC1921_MAX_VSENSE_MV; + *val2 = PAC1921_RES_RESOLUTION << (int)priv->di_gain; + + return IIO_VAL_FRACTIONAL; + } + case PAC1921_CHAN_CURRENT: { + /* Current scale factor in mA: + * Vsense LSB (nV) / shunt (uOhm) + */ + guard(mutex)(&priv->lock); + + *val = pac1921_vsense_lsb(priv->di_gain); + *val2 = (int)priv->rshunt; + + return IIO_VAL_FRACTIONAL; + } + case PAC1921_CHAN_POWER: { + /* Power scale factor in mW: + * Vsense LSB (nV) * max_vbus (V) / vgain / shunt (uOhm) + */ + guard(mutex)(&priv->lock); + + *val = pac1921_vsense_lsb(priv->di_gain) * + (PAC1921_MAX_VBUS_V >> (int)priv->dv_gain); + *val2 = (int)priv->rshunt; + + return IIO_VAL_FRACTIONAL; + } + default: + return -EINVAL; + } + break; + + case IIO_CHAN_INFO_HARDWAREGAIN: + switch (chan->channel) { + case PAC1921_CHAN_VBUS: { + guard(mutex)(&priv->lock); + *val = 1 << (int)priv->dv_gain; + return IIO_VAL_INT; + } + case PAC1921_CHAN_VSENSE: + case PAC1921_CHAN_CURRENT: { + guard(mutex)(&priv->lock); + *val = 1 << (int)priv->di_gain; + return IIO_VAL_INT; + } + default: + return -EINVAL; + } + + case IIO_CHAN_INFO_INT_TIME: { + /* Integration time is read only: it depends on the number of + * integration samples, measurement resolution and post filters + */ + *val2 = 1000000; /* From usecs to fractional secs */ + guard(mutex)(&priv->lock); + *val = (int)priv->integr_period; + return IIO_VAL_FRACTIONAL; + } + default: + return -EINVAL; + } +} + +/* Perform configuration update sequence: set the device into read state, then + * write the config register and set the device back into integration state. + * Also reset integration start time and mark first integration to be yet + * completed. + * + * Must be called with lock held. + */ +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg, + unsigned int mask, unsigned int val) +{ + /* Enter READ state before configuration */ + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, 0); + if (ret) + return ret; + + /* Update configuration value */ + ret = regmap_update_bits(priv->regmap, reg, mask, val); + if (ret) + return ret; + + /* Re-enable integration and reset start time */ + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); + if (ret) + return ret; + + priv->integr_start_time = jiffies; + priv->first_integr_done = false; + + return 0; +} + +static inline bool pac1921_gain_param_valid(unsigned int gain, unsigned int max) +{ + return (gain > 0 && gain <= (1U << max) && is_power_of_2(gain)); +} + +static int pac1921_update_gain(struct pac1921_priv *priv, + struct iio_chan_spec const *chan, int val) +{ + unsigned int max; + unsigned int mask; + unsigned int shift; + u8 *priv_val; + + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + max = PAC1921_GAIN_DV_GAIN_MAX; + mask = PAC1921_GAIN_DV_GAIN_MASK; + shift = PAC1921_GAIN_DV_GAIN_SHIFT; + priv_val = &priv->dv_gain; + break; + case PAC1921_CHAN_VSENSE: + case PAC1921_CHAN_CURRENT: + max = PAC1921_GAIN_DI_GAIN_MAX; + mask = PAC1921_GAIN_DI_GAIN_MASK; + shift = PAC1921_GAIN_DI_GAIN_SHIFT; + priv_val = &priv->di_gain; + break; + default: + return -EINVAL; + } + + unsigned int gain_param = (unsigned int)val; + + if (!pac1921_gain_param_valid(gain_param, max)) + return -EINVAL; + + u8 gain = (u8)ilog2(gain_param); + + guard(mutex)(&priv->lock); + + if (*priv_val == gain) + return 0; + + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG, mask, + gain << shift); + if (ret) + return ret; + + *priv_val = gain; + + return 0; +} + +static int pac1921_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_HARDWAREGAIN: + return pac1921_update_gain(priv, chan, val); + default: + return -EINVAL; + } +} + +static int pac1921_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_HARDWAREGAIN: + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + *length = ARRAY_SIZE(pac1921_dv_gains); + *vals = pac1921_dv_gains; + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + + case PAC1921_CHAN_VSENSE: + case PAC1921_CHAN_CURRENT: + *length = ARRAY_SIZE(pac1921_di_gains); + *vals = pac1921_di_gains; + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int pac1921_read_label(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, char *label) +{ + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + return sprintf(label, "vbus\n"); + case PAC1921_CHAN_VSENSE: + return sprintf(label, "vsense\n"); + case PAC1921_CHAN_CURRENT: + return sprintf(label, "current\n"); + case PAC1921_CHAN_POWER: + return sprintf(label, "power\n"); + default: + return -EINVAL; + } +} + +static const struct iio_info pac1921_iio = { + .read_raw = pac1921_read_raw, + .write_raw = pac1921_write_raw, + .read_avail = pac1921_read_avail, + .read_label = pac1921_read_label, +}; + +static int pac1921_get_resolution(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + guard(mutex)(&priv->lock); + + if (priv->high_res) + return PAC1921_MEAS_RESOLUTION_IDX_HIGH; + + return PAC1921_MEAS_RESOLUTION_IDX_LOW; +} + +static int pac1921_set_resolution(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int val) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + bool high_res; + + switch (val) { + case PAC1921_MEAS_RESOLUTION_IDX_LOW: + high_res = false; + break; + case PAC1921_MEAS_RESOLUTION_IDX_HIGH: + high_res = true; + break; + default: + WARN_ON_ONCE(1); + return -EINVAL; + } + + guard(mutex)(&priv->lock); + + if (priv->high_res == high_res) + return 0; + + unsigned int mask = PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES; + unsigned int bits = high_res ? + 0 : PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES; + + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG, + mask, bits); + if (ret) + return ret; + + priv->high_res = high_res; + + return pac1921_update_integr_period(priv); +} + +static int pac1921_get_int_num_samples(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + guard(mutex)(&priv->lock); + + return priv->n_samples; +} + +static int pac1921_set_int_num_samples(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int val) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX)) + return -EINVAL; + + guard(mutex)(&priv->lock); + + if (priv->n_samples == val) + return 0; + + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_SMPL_MASK, + val << PAC1921_INT_CFG_SMPL_SHIFT); + if (ret) + return ret; + + priv->n_samples = (u8)val; + + return pac1921_update_integr_period(priv); +} + +static ssize_t pac1921_read_filters_enabled(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + bool enabled; + + scoped_guard(mutex, &priv->lock) { + enabled = priv->filters_en; + } + return sysfs_emit(buf, "%d\n", enabled); +} + +static ssize_t pac1921_write_filters_enabled(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + bool enabled; + + int ret = kstrtobool(buf, &enabled); + if (ret) + return ret; + + guard(mutex)(&priv->lock); + + if (priv->filters_en == enabled) + return len; + + unsigned int mask = PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN; + unsigned int bits = enabled ? + PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN : 0; + + + ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, mask, bits); + if (ret) + return ret; + + priv->filters_en = enabled; + + ret = pac1921_update_integr_period(priv); + if (ret) + return ret; + + return len; +} + +static const struct iio_enum pac1921_resolution_enum = { + .items = pac1921_meas_resolutions, + .num_items = ARRAY_SIZE(pac1921_meas_resolutions), + .get = pac1921_get_resolution, + .set = pac1921_set_resolution, +}; +static const struct iio_enum pac1921_int_num_samples_enum = { + .items = pac1921_integr_num_samples, + .num_items = ARRAY_SIZE(pac1921_integr_num_samples), + .get = pac1921_get_int_num_samples, + .set = pac1921_set_int_num_samples, +}; +static const struct iio_chan_spec_ext_info pac1921_ext_info[] = { + IIO_ENUM("resolution_bits", IIO_SHARED_BY_ALL, + &pac1921_resolution_enum), + IIO_ENUM_AVAILABLE("resolution_bits", IIO_SHARED_BY_ALL, + &pac1921_resolution_enum), + IIO_ENUM("integration_samples", IIO_SHARED_BY_ALL, + &pac1921_int_num_samples_enum), + IIO_ENUM_AVAILABLE("integration_samples", IIO_SHARED_BY_ALL, + &pac1921_int_num_samples_enum), + { + .name = "filters_en", + .read = pac1921_read_filters_enabled, + .write = pac1921_write_filters_enabled, + .shared = IIO_SHARED_BY_ALL, + }, + {}, +}; + +static const struct iio_event_spec pac1921_overflow_event[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + }, +}; + +static const struct iio_chan_spec pac1921_channels[] = { + { + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + .channel = PAC1921_CHAN_VBUS, + .address = PAC1921_REG_VBUS, + .scan_index = PAC1921_CHAN_VBUS, + .scan_type = { + .sign = 'u', + .realbits = 10, + .storagebits = 16, + .endianness = IIO_CPU + }, + .indexed = 1, + .event_spec = pac1921_overflow_event, + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), + .ext_info = pac1921_ext_info, + }, + { + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + .channel = PAC1921_CHAN_VSENSE, + .address = PAC1921_REG_VSENSE, + .scan_index = PAC1921_CHAN_VSENSE, + .scan_type = { + .sign = 'u', + .realbits = 10, + .storagebits = 16, + .endianness = IIO_CPU + }, + .indexed = 1, + .event_spec = pac1921_overflow_event, + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), + .ext_info = pac1921_ext_info, + }, + { + .type = IIO_CURRENT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + .channel = PAC1921_CHAN_CURRENT, + .address = PAC1921_REG_VSENSE, + .scan_index = PAC1921_CHAN_CURRENT, + .scan_type = { + .sign = 'u', + .realbits = 10, + .storagebits = 16, + .endianness = IIO_CPU + }, + .event_spec = pac1921_overflow_event, + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), + .ext_info = pac1921_ext_info, + }, + { + .type = IIO_POWER, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + .channel = PAC1921_CHAN_POWER, + .address = PAC1921_REG_VPOWER, + .scan_index = PAC1921_CHAN_POWER, + .scan_type = { + .sign = 'u', + .realbits = 10, + .storagebits = 16, + .endianness = IIO_CPU + }, + .event_spec = pac1921_overflow_event, + .num_event_specs = ARRAY_SIZE(pac1921_overflow_event), + .ext_info = pac1921_ext_info, + }, + IIO_CHAN_SOFT_TIMESTAMP(PAC1921_NUM_MEAS_CHANS), +}; + +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); + + guard(mutex)(&priv->lock); + + if (!pac1921_data_ready(priv)) + goto done; + + int ret = pac1921_check_push_overflow(idev, pf->timestamp); + if (ret) + goto done; + + memset(&priv->scan, 0, sizeof(priv->scan)); + + int bit, ch = 0; + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { + int val; + + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); + if (ret) + goto done; + + priv->scan.chan[ch++] = (u16)val; + } + + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); + +done: + iio_trigger_notify_done(idev->trig); + + return IRQ_HANDLED; +} + +static int pac1921_init(struct pac1921_priv *priv) +{ + /* Time after power-up before ready to begin communications */ + msleep(20); + + /* Enter READ state before configuration */ + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, 0); + if (ret) + return ret; + + /* Configure gains and measurements resolution */ + unsigned int val = priv->di_gain << PAC1921_GAIN_DI_GAIN_SHIFT | + priv->dv_gain << PAC1921_GAIN_DV_GAIN_SHIFT; + if (!priv->high_res) + val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES; + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); + if (ret) + return ret; + + /* Configure integration: + * - num of integration samples, filters enabled/disabled + * - set READ/INT pin override (RIOV) to control operation mode via + * register instead of pin + */ + val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT | + PAC1921_INT_CFG_RIOV; + if (priv->filters_en) + val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN; + ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val); + if (ret) + return ret; + + /* Init control register: + * - VPower free run integration mode + * - OUT pin full scale range: 3V (HW detault) + * - no timeout, no sleep, no sleep override, no recalc (HW defaults) + */ + val = PAC1921_MXSL_VPOWER_FREE_RUN << PAC1921_CONTROL_MXSL_SHIFT; + ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val); + if (ret) + return ret; + + /* Enable integration */ + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); + if (ret) + return ret; + + priv->first_integr_started = true; + priv->integr_start_time = jiffies; + + return pac1921_update_integr_period(priv); +} + +static int pac1921_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct pac1921_priv *priv = iio_priv(indio_dev); + + guard(mutex)(&priv->lock); + + priv->first_integr_started = false; + priv->first_integr_done = false; + + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, 0); + if (ret) + return ret; + + ret = regmap_update_bits(priv->regmap, PAC1921_REG_CONTROL, + PAC1921_CONTROL_SLEEP, PAC1921_CONTROL_SLEEP); + if (ret) + return ret; + + return regulator_disable(priv->vdd); + +} + +static int pac1921_resume(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct pac1921_priv *priv = iio_priv(indio_dev); + + guard(mutex)(&priv->lock); + + int ret = regulator_enable(priv->vdd); + if (ret) + return ret; + + return pac1921_init(priv); +} + +static DEFINE_SIMPLE_DEV_PM_OPS(pac1921_pm_ops, pac1921_suspend, + pac1921_resume); + +static void pac1921_regulator_disable(void *data) +{ + struct pac1921_priv *priv = data; + + regulator_disable(priv->vdd); +} + +static int pac1921_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct pac1921_priv *priv; + + struct iio_dev *indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); + if (!indio_dev) + return -ENOMEM; + + priv = iio_priv(indio_dev); + priv->client = client; + i2c_set_clientdata(client, indio_dev); + + priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config); + if (IS_ERR(priv->regmap)) + dev_err_probe(dev, (int)PTR_ERR(priv->regmap), + "Cannot initialize register map\n"); + + mutex_init(&priv->lock); + + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN; + priv->di_gain = PAC1921_DEFAULT_DI_GAIN; + priv->high_res = PAC1921_DEFAULT_HIGH_RES; + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES; + priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED; + + int ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", + &priv->rshunt); + if (ret) + return dev_err_probe(dev, ret, + "Cannot read shunt resistor property\n"); + if (priv->rshunt == 0 || priv->rshunt > INT_MAX) + return dev_err_probe(dev, -EINVAL, + "Invalid shunt resistor: %u\n", + priv->rshunt); + + priv->vdd = devm_regulator_get(dev, "vdd"); + if (IS_ERR(priv->vdd)) + return dev_err_probe(dev, (int)PTR_ERR(priv->vdd), + "Cannot get vdd regulator\n"); + + ret = regulator_enable(priv->vdd); + if (ret) + return dev_err_probe(dev, ret, "Cannot enable vdd regulator\n"); + + ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, priv); + + ret = pac1921_init(priv); + if (ret) + return dev_err_probe(dev, ret, "Cannot initialize device\n"); + + priv->iio_info = pac1921_iio; + + const struct i2c_device_id *id = i2c_client_get_device_id(client); + + indio_dev->name = id->name; + indio_dev->info = &priv->iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = pac1921_channels; + indio_dev->num_channels = ARRAY_SIZE(pac1921_channels); + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, + &iio_pollfunc_store_time, + &pac1921_trigger_handler, NULL); + if (ret) + return dev_err_probe(dev, ret, + "Cannot setup IIO triggered buffer\n"); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Cannot register IIO device\n"); + + return 0; +} + +static const struct i2c_device_id pac1921_id[] = { + { .name = "pac1921", 0 }, + {} +}; +MODULE_DEVICE_TABLE(i2c, pac1921_id); + +static const struct of_device_id pac1921_of_match[] = { + { .compatible = "microchip,pac1921" }, + {} +}; +MODULE_DEVICE_TABLE(of, pac1921_of_match); + +static struct i2c_driver pac1921_driver = { + .driver = { + .name = "pac1921", + .pm = pm_sleep_ptr(&pac1921_pm_ops), + .of_match_table = pac1921_of_match, + }, + .probe = pac1921_probe, + .id_table = pac1921_id, +}; + +module_i2c_driver(pac1921_driver); + +MODULE_AUTHOR("Matteo Martelli <matteomartelli3@gmail.com>"); +MODULE_DESCRIPTION("IIO driver for PAC1921 High-Side Power/Current Monitor"); +MODULE_LICENSE("GPL");
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 * userspace controls for voltage and current gains, measurement resolution, integration samples and filters enabling/disabling * simple power management support Limitations: * operation mode fixed to free-run integration * measurement resolution and filters controls are applied to both VSENSE and VBUS measurements Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- .../ABI/testing/sysfs-bus-iio-adc-pac1921 | 45 + MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/pac1921.c | 1038 ++++++++++++++++++++ 5 files changed, 1101 insertions(+)