| Message ID | 20240724-iio-pac1921-v4-3-723698e903a3@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: add support for pac1921 | expand |
On Wed, 24 Jul 2024 11:08:33 +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 > * scale attributes to control voltage and current gains > * oversampling ratio attribute to control the number of integration > samples > * sampling rate attribute that reflects the integration period > * userspace attribute and DT parameter to control shunt resistor > * simple power management support > > Limitations: > * operation mode fixed to free-run integration > * READ/INT pin and OUT pin not supported > * no controls for measurement resolutions and filters > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> I had a few more bits of feedback + one change was necessary because of this crossing with Nuno's series making iio_dev->masklength private. Rather than go around again for such trivial things, I've applied it to the testing branch of iio.git with the following diff. Note I'll rebase that tree on rc1 once available at which point it'll become the togreg branch and get picked up by linux-next etc. There are a few things inline that I commented on but didn't touch, so please also take a look at those and shout if I messed anything up! I've been known to make trivial changes that break a driver completely :( Thanks, Jonathan diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c index a21dd772467e..d04c6685d780 100644 --- a/drivers/iio/adc/pac1921.c +++ b/drivers/iio/adc/pac1921.c @@ -75,12 +75,12 @@ enum pac1921_mxsl { * Vbus scale (mV) = max_vbus (mV) / dv_gain / resolution */ static const int pac1921_vbus_scales[][2] = { - {31, 280547409}, /* dv_gain x1 */ - {15, 640273704}, /* dv_gain x2 */ - {7, 820136852}, /* dv_gain x4 */ - {3, 910068426}, /* dv_gain x8 */ - {1, 955034213}, /* dv_gain x16 */ - {0, 977517106} /* dv_gain x32 */ + { 31, 280547409 }, /* dv_gain x1 */ + { 15, 640273704 }, /* dv_gain x2 */ + { 7, 820136852 }, /* dv_gain x4 */ + { 3, 910068426 }, /* dv_gain x8 */ + { 1, 955034213 }, /* dv_gain x16 */ + { 0, 977517106 }, /* dv_gain x32 */ }; /* @@ -91,14 +91,14 @@ static const int pac1921_vbus_scales[][2] = { * Vsense scale (mV) = max_vsense (mV) / di_gain / resolution */ static const int pac1921_vsense_scales[][2] = { - {0, 97751710}, /* di_gain x1 */ - {0, 48875855}, /* di_gain x2 */ - {0, 24437927}, /* di_gain x4 */ - {0, 12218963}, /* di_gain x8 */ - {0, 6109481}, /* di_gain x16 */ - {0, 3054740}, /* di_gain x32 */ - {0, 1527370}, /* di_gain x64 */ - {0, 763685} /* di_gain x128 */ + { 0, 97751710 }, /* di_gain x1 */ + { 0, 48875855 }, /* di_gain x2 */ + { 0, 24437927 }, /* di_gain x4 */ + { 0, 12218963 }, /* di_gain x8 */ + { 0, 6109481 }, /* di_gain x16 */ + { 0, 3054740 }, /* di_gain x32 */ + { 0, 1527370 }, /* di_gain x64 */ + { 0, 763685 }, /* di_gain x128 */ }; /* @@ -334,7 +334,7 @@ static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg, if (ret) return ret; - *val = (u16)FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val)); + *val = FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val)); return 0; } @@ -612,7 +612,7 @@ static int pac1921_update_int_num_samples(struct pac1921_priv *priv, if (priv->n_samples == n_samples) return 0; - reg_val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples); + reg_val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples); ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, PAC1921_INT_CFG_SMPL_MASK, reg_val); @@ -1017,7 +1017,7 @@ static irqreturn_t pac1921_trigger_handler(int irq, void *p) if (ret) goto done; - for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { + iio_for_each_active_channel(idev, bit) { u16 val; ret = pac1921_read_res(priv, idev->channels[ch].address, &val); @@ -1054,8 +1054,8 @@ static int pac1921_init(struct pac1921_priv *priv) return ret; /* Configure gains, use 14-bits measurement resolution (HW default) */ - val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | - (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); + val = FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | + FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); if (ret) return ret; @@ -1067,7 +1067,7 @@ static int pac1921_init(struct pac1921_priv *priv) * - set READ/INT pin override (RIOV) to control operation mode via * register instead of pin */ - val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | + val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN | PAC1921_INT_CFG_RIOV; ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val); @@ -1080,8 +1080,8 @@ static int pac1921_init(struct pac1921_priv *priv) * - OUT pin full scale range: 3V (HW detault) * - no timeout, no sleep, no sleep override, no recalc (HW defaults) */ - val = (u8)FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, - PAC1921_MXSL_VPOWER_FREE_RUN); + val = FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, + PAC1921_MXSL_VPOWER_FREE_RUN); ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val); if (ret) return ret; @@ -1153,7 +1153,6 @@ static void pac1921_regulator_disable(void *data) static int pac1921_probe(struct i2c_client *client) { - const struct i2c_device_id *id = i2c_client_get_device_id(client); struct device *dev = &client->dev; struct pac1921_priv *priv; struct iio_dev *indio_dev; @@ -1202,8 +1201,7 @@ static int pac1921_probe(struct i2c_client *client) ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, priv->vdd); if (ret) - return dev_err_probe( - dev, ret, + return dev_err_probe(dev, ret, "Cannot add action for vdd regulator disposal\n"); msleep(PAC1921_POWERUP_TIME_MS); @@ -1214,7 +1212,7 @@ static int pac1921_probe(struct i2c_client *client) priv->iio_info = pac1921_iio; - indio_dev->name = id->name; + indio_dev->name = "pac1921"; indio_dev->info = &priv->iio_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = pac1921_channels; > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c > new file mode 100644 > index 000000000000..a21dd772467e > --- /dev/null > +++ b/drivers/iio/adc/pac1921.c > @@ -0,0 +1,1263 @@ > +/* > + * Pre-computed scales for SENSE voltage > + * format: IIO_VAL_INT_PLUS_NANO > + * unit: mV > + * > + * Vsense scale (mV) = max_vsense (mV) / di_gain / resolution > + */ > +static const int pac1921_vsense_scales[][2] = { > + {0, 97751710}, /* di_gain x1 */ > + {0, 48875855}, /* di_gain x2 */ > + {0, 24437927}, /* di_gain x4 */ > + {0, 12218963}, /* di_gain x8 */ > + {0, 6109481}, /* di_gain x16 */ > + {0, 3054740}, /* di_gain x32 */ > + {0, 1527370}, /* di_gain x64 */ > + {0, 763685} /* di_gain x128 */ > +}; trivial but generally prefer { 0, 763685 }, etc > + > +/* > + * Emit on sysfs the list of available scales contained in scales_tbl > + * > + * TODO:: this function can be replaced with iio_format_avail_list() if the > + * latter will ever be exported. You could just have added a precursor patch doing that. If you have time I'd certainly consider a patch that does export that function and uses it here. > + * > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for > + * the current scales table) > + */ > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], > + size_t size, char *buf) > +{ > + ssize_t len = 0; > + > + for (unsigned int i = 0; i < size; i++) { > + if (i != 0) { > + len += sysfs_emit_at(buf, len, " "); > + if (len >= PAGE_SIZE) > + return -EFBIG; > + } > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], > + scales_tbl[i][1]); > + if (len >= PAGE_SIZE) > + return -EFBIG; > + } > + > + len += sysfs_emit_at(buf, len, "\n"); > + return len; > +} > + > +/* > + * Read available scales for a specific channel > + * > + * NOTE: using extended info insted of iio.read_avail() because access to > + * current scales must be locked as they depend on shunt resistor which may > + * change runtime. Caller of iio.read_avail() would access the table unlocked > + * instead. That's a corner case we should think about closing. Would require an indicator to read_avail that the buffer it has been passed is a snapshot that it should free on completion of the string building. I don't like passing ownership of data around like that, but it is fiddly to do anything else given any simple double buffering is subject to race conditions. An alternative would use a key of sometype to associate individual read_avail calls with new ones to read_avail_release_resource. That might be cleaner. oh well, a cleanup job for another day. I suspect we have drivers today that are subject to tearing of their available lists. > + */ > +static ssize_t pac1921_read_scale_avail(struct iio_dev *indio_dev, > + uintptr_t private, > + const struct iio_chan_spec *chan, > + char *buf) > +{ > + struct pac1921_priv *priv = iio_priv(indio_dev); > + const int (*scales_tbl)[2]; > + size_t size; > + > + switch (chan->channel) { > + case PAC1921_CHAN_VBUS: > + scales_tbl = pac1921_vbus_scales; > + size = ARRAY_SIZE(pac1921_vbus_scales); > + return pac1921_format_scale_avail(scales_tbl, size, buf); > + > + case PAC1921_CHAN_VSENSE: > + scales_tbl = pac1921_vsense_scales; > + size = ARRAY_SIZE(pac1921_vsense_scales); > + return pac1921_format_scale_avail(scales_tbl, size, buf); > + > + case PAC1921_CHAN_CURRENT: { > + guard(mutex)(&priv->lock); > + scales_tbl = priv->current_scales; > + size = ARRAY_SIZE(priv->current_scales); > + return pac1921_format_scale_avail(scales_tbl, size, buf); > + } > + default: > + return -EINVAL; > + } > +} > + > +#define PAC1921_EXT_INFO_SCALE_AVAIL { \ > + .name = "scale_available", \ > + .read = pac1921_read_scale_avail, \ > + .shared = IIO_SEPARATE, \ > +} > + > +static const struct iio_chan_spec_ext_info pac1921_ext_info_voltage[] = { > + PAC1921_EXT_INFO_SCALE_AVAIL, > + {} > +}; > + > +static irqreturn_t pac1921_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *idev = pf->indio_dev; > + struct pac1921_priv *priv = iio_priv(idev); > + int ret; > + int bit; > + int ch = 0; > + > + guard(mutex)(&priv->lock); > + > + if (!pac1921_data_ready(priv)) Interesting corner case that maybe could have done with a comment. Seems could be triggered by a spurious interrupt, or sampling too early. I think only the second one is likely to happen, so shouldn't be a problem to acknowledge that trigger. > + goto done; > + > + ret = pac1921_check_push_overflow(idev, pf->timestamp); > + if (ret) > + goto done; > + > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { This needs an update as we crossed with Nuno's series that removes access to masklength. I can fix whilst applying by using iio_for_each_active_channel() > + u16 val; > + > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > + if (ret) > + goto done; > + > + priv->scan.chan[ch++] = val; > + } > + > + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); > + > +done: > + iio_trigger_notify_done(idev->trig); > + > + return IRQ_HANDLED; > +} > + > +/* > + * Initialize device by writing initial configuration and putting it into > + * integration state. > + * > + * Must be called with lock held when called after first initialization > + * (e.g. from pm resume) > + */ > +static int pac1921_init(struct pac1921_priv *priv) > +{ > + unsigned int val; > + int ret; > + > + /* Enter READ state before configuration */ > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > + PAC1921_INT_CFG_INTEN, 0); > + if (ret) > + return ret; > + > + /* Configure gains, use 14-bits measurement resolution (HW default) */ > + val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | > + (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); Why are these cases needed? Each of those values is going to fit in a u8 just fine and it's getting written to a much larger variable. > + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); > + if (ret) > + return ret; > + > + /* > + * Configure integration: > + * - num of integration samples > + * - filters enabled (HW default) > + * - set READ/INT pin override (RIOV) to control operation mode via > + * register instead of pin > + */ > + val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | > + PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN | > + PAC1921_INT_CFG_RIOV; > + 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 = (u8)FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, > + PAC1921_MXSL_VPOWER_FREE_RUN); > + 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_started_time_jiffies = jiffies; > + priv->integr_period_usecs = pac1921_int_periods_usecs[priv->n_samples]; > + > + return 0; > +} ... > +static int pac1921_probe(struct i2c_client *client) > +{ > + const struct i2c_device_id *id = i2c_client_get_device_id(client); With the change suggested below this wouldn't be needed. > + struct device *dev = &client->dev; > + struct pac1921_priv *priv; > + struct iio_dev *indio_dev; > + int ret; > + > + 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"); > + > + devm_mutex_init(dev, &priv->lock); > + > + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN; > + priv->di_gain = PAC1921_DEFAULT_DI_GAIN; > + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES; > + > + ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", > + &priv->rshunt_uohm); > + if (ret) > + return dev_err_probe(dev, ret, > + "Cannot read shunt resistor property\n"); > + if (priv->rshunt_uohm == 0 || priv->rshunt_uohm > INT_MAX) > + return dev_err_probe(dev, -EINVAL, > + "Invalid shunt resistor: %u\n", > + priv->rshunt_uohm); > + > + pac1921_calc_current_scales(priv); > + > + 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->vdd); > + if (ret) > + return dev_err_probe( > + dev, ret, Trivial: I'd move the dev, ret to previous line. > + "Cannot add action for vdd regulator disposal\n"); > + > + msleep(PAC1921_POWERUP_TIME_MS); > + > + ret = pac1921_init(priv); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot initialize device\n"); > + > + priv->iio_info = pac1921_iio; > + > + indio_dev->name = id->name; This tends to end up flakey at best when different types of firmware are taken into account - IIRC particularly when fallback compatibles are in use. So better to switch this to "pac1921" here. > + 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);
Jonathan Cameron wrote: > On Wed, 24 Jul 2024 11:08:33 +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 > > * scale attributes to control voltage and current gains > > * oversampling ratio attribute to control the number of integration > > samples > > * sampling rate attribute that reflects the integration period > > * userspace attribute and DT parameter to control shunt resistor > > * simple power management support > > > > Limitations: > > * operation mode fixed to free-run integration > > * READ/INT pin and OUT pin not supported > > * no controls for measurement resolutions and filters > > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > I had a few more bits of feedback + one change was necessary because of > this crossing with Nuno's series making iio_dev->masklength private. > Rather than go around again for such trivial things, > I've applied it to the testing branch of iio.git with the following diff. > Note I'll rebase that tree on rc1 once available at which point it'll become > the togreg branch and get picked up by linux-next etc. > > There are a few things inline that I commented on but didn't touch, so > please also take a look at those and shout if I messed anything up! > I've been known to make trivial changes that break a driver completely :( > > Thanks, > > Jonathan > Thanks Jonathan, I reviewed your diff and also tested this version on the HW. All looks good to me. Please check also my comments below. > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c > index a21dd772467e..d04c6685d780 100644 > --- a/drivers/iio/adc/pac1921.c > +++ b/drivers/iio/adc/pac1921.c > @@ -75,12 +75,12 @@ enum pac1921_mxsl { > * Vbus scale (mV) = max_vbus (mV) / dv_gain / resolution > */ > static const int pac1921_vbus_scales[][2] = { > - {31, 280547409}, /* dv_gain x1 */ > - {15, 640273704}, /* dv_gain x2 */ > - {7, 820136852}, /* dv_gain x4 */ > - {3, 910068426}, /* dv_gain x8 */ > - {1, 955034213}, /* dv_gain x16 */ > - {0, 977517106} /* dv_gain x32 */ > + { 31, 280547409 }, /* dv_gain x1 */ > + { 15, 640273704 }, /* dv_gain x2 */ > + { 7, 820136852 }, /* dv_gain x4 */ > + { 3, 910068426 }, /* dv_gain x8 */ > + { 1, 955034213 }, /* dv_gain x16 */ > + { 0, 977517106 }, /* dv_gain x32 */ > }; > > /* > @@ -91,14 +91,14 @@ static const int pac1921_vbus_scales[][2] = { > * Vsense scale (mV) = max_vsense (mV) / di_gain / resolution > */ > static const int pac1921_vsense_scales[][2] = { > - {0, 97751710}, /* di_gain x1 */ > - {0, 48875855}, /* di_gain x2 */ > - {0, 24437927}, /* di_gain x4 */ > - {0, 12218963}, /* di_gain x8 */ > - {0, 6109481}, /* di_gain x16 */ > - {0, 3054740}, /* di_gain x32 */ > - {0, 1527370}, /* di_gain x64 */ > - {0, 763685} /* di_gain x128 */ > + { 0, 97751710 }, /* di_gain x1 */ > + { 0, 48875855 }, /* di_gain x2 */ > + { 0, 24437927 }, /* di_gain x4 */ > + { 0, 12218963 }, /* di_gain x8 */ > + { 0, 6109481 }, /* di_gain x16 */ > + { 0, 3054740 }, /* di_gain x32 */ > + { 0, 1527370 }, /* di_gain x64 */ > + { 0, 763685 }, /* di_gain x128 */ > }; > > /* > @@ -334,7 +334,7 @@ static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg, > if (ret) > return ret; > > - *val = (u16)FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val)); > + *val = FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val)); > > return 0; > } > @@ -612,7 +612,7 @@ static int pac1921_update_int_num_samples(struct pac1921_priv *priv, > if (priv->n_samples == n_samples) > return 0; > > - reg_val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples); > + reg_val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples); > > ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, > PAC1921_INT_CFG_SMPL_MASK, reg_val); > @@ -1017,7 +1017,7 @@ static irqreturn_t pac1921_trigger_handler(int irq, void *p) > if (ret) > goto done; > > - for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { > + iio_for_each_active_channel(idev, bit) { > u16 val; > > ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > @@ -1054,8 +1054,8 @@ static int pac1921_init(struct pac1921_priv *priv) > return ret; > > /* Configure gains, use 14-bits measurement resolution (HW default) */ > - val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | > - (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); > + val = FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | > + FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); > ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); > if (ret) > return ret; > @@ -1067,7 +1067,7 @@ static int pac1921_init(struct pac1921_priv *priv) > * - set READ/INT pin override (RIOV) to control operation mode via > * register instead of pin > */ > - val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | > + val = FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | > PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN | > PAC1921_INT_CFG_RIOV; > ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val); > @@ -1080,8 +1080,8 @@ static int pac1921_init(struct pac1921_priv *priv) > * - OUT pin full scale range: 3V (HW detault) > * - no timeout, no sleep, no sleep override, no recalc (HW defaults) > */ > - val = (u8)FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, > - PAC1921_MXSL_VPOWER_FREE_RUN); > + val = FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, > + PAC1921_MXSL_VPOWER_FREE_RUN); > ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val); > if (ret) > return ret; > @@ -1153,7 +1153,6 @@ static void pac1921_regulator_disable(void *data) > > static int pac1921_probe(struct i2c_client *client) > { > - const struct i2c_device_id *id = i2c_client_get_device_id(client); > struct device *dev = &client->dev; > struct pac1921_priv *priv; > struct iio_dev *indio_dev; > @@ -1202,8 +1201,7 @@ static int pac1921_probe(struct i2c_client *client) > ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, > priv->vdd); > if (ret) > - return dev_err_probe( > - dev, ret, > + return dev_err_probe(dev, ret, > "Cannot add action for vdd regulator disposal\n"); > > msleep(PAC1921_POWERUP_TIME_MS); > @@ -1214,7 +1212,7 @@ static int pac1921_probe(struct i2c_client *client) > > priv->iio_info = pac1921_iio; > > - indio_dev->name = id->name; > + indio_dev->name = "pac1921"; > indio_dev->info = &priv->iio_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = pac1921_channels; > > > > + > > +/* > > + * Emit on sysfs the list of available scales contained in scales_tbl > > + * > > + * TODO:: this function can be replaced with iio_format_avail_list() if the > > + * latter will ever be exported. > > You could just have added a precursor patch doing that. > If you have time I'd certainly consider a patch that does export that function > and uses it here. > I wasn't sure that one usage was enough to justify the export. I could definitely do it, I am assuming it would now go to a new patch series since this has already been merged into testing, right? > > + * > > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for > > + * the current scales table) > > + */ > > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], > > + size_t size, char *buf) > > +{ > > + ssize_t len = 0; > > + > > + for (unsigned int i = 0; i < size; i++) { > > + if (i != 0) { > > + len += sysfs_emit_at(buf, len, " "); > > + if (len >= PAGE_SIZE) > > + return -EFBIG; > > + } > > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], > > + scales_tbl[i][1]); > > + if (len >= PAGE_SIZE) > > + return -EFBIG; > > + } > > + > > + len += sysfs_emit_at(buf, len, "\n"); > > + return len; > > +} > > + > > +/* > > + * Read available scales for a specific channel > > + * > > + * NOTE: using extended info insted of iio.read_avail() because access to > > + * current scales must be locked as they depend on shunt resistor which may > > + * change runtime. Caller of iio.read_avail() would access the table unlocked > > + * instead. > > That's a corner case we should think about closing. Would require an indicator > to read_avail that the buffer it has been passed is a snapshot that it should > free on completion of the string building. I don't like passing ownership > of data around like that, but it is fiddly to do anything else given > any simple double buffering is subject to race conditions. > If I understand your suggestion the driver would allocate a new table and copy the values into it at each read_avail() call. Then iio_read_channel_info_avail() would free the buffer if some sort of free-after-use indicator flag is set. I guess such indicator might be set via an additional read_avail function argument (would be an extensive API change) or maybe via a new iio_chan_spec attribute. > An alternative would use a key of sometype to associate individual read_avail > calls with new ones to read_avail_release_resource. That might be cleaner. > Are you referring to introduce a new read_avail_realease_resource callback that would be called at the end of iio_read_channel_info_avail() if set? Similarly to the previous point the driver would allocate a new table and copy the values into it at each read_avail() call, but the driver would also define a release callback to free such table. If otherwise you are referring to something less trivial, is there a similar API in the kernel that can be referred to for clarity? > oh well, a cleanup job for another day. I suspect we have drivers today > that are subject to tearing of their available lists. > I've just taken a quick look at the other drivers and the following twos seem to have the race condition issue since they are updating an available table during a write_raw() call and also exposing it during a read_avail() call: * drivers/iio/light/as73211.c: see int_time_avail table * drivers/iio/adc/ad7192.c: see filter_freq_avail table There might be others, I've only looked into those that seemed likely to have this issue after some trivial greps. Is there already a common way for iio to keep track of open issues (e.g. Issue tracker/TODO lists/etc)? > > + */ > > +static ssize_t pac1921_read_scale_avail(struct iio_dev *indio_dev, > > + uintptr_t private, > > + const struct iio_chan_spec *chan, > > + char *buf) > > +{ > > + struct pac1921_priv *priv = iio_priv(indio_dev); > > + const int (*scales_tbl)[2]; > > + size_t size; > > + > > + switch (chan->channel) { > > + case PAC1921_CHAN_VBUS: > > + scales_tbl = pac1921_vbus_scales; > > + size = ARRAY_SIZE(pac1921_vbus_scales); > > + return pac1921_format_scale_avail(scales_tbl, size, buf); > > + > > + case PAC1921_CHAN_VSENSE: > > + scales_tbl = pac1921_vsense_scales; > > + size = ARRAY_SIZE(pac1921_vsense_scales); > > + return pac1921_format_scale_avail(scales_tbl, size, buf); > > + > > + case PAC1921_CHAN_CURRENT: { > > + guard(mutex)(&priv->lock); > > + scales_tbl = priv->current_scales; > > + size = ARRAY_SIZE(priv->current_scales); > > + return pac1921_format_scale_avail(scales_tbl, size, buf); > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +#define PAC1921_EXT_INFO_SCALE_AVAIL { \ > > + .name = "scale_available", \ > > + .read = pac1921_read_scale_avail, \ > > + .shared = IIO_SEPARATE, \ > > +} > > + > > +static const struct iio_chan_spec_ext_info pac1921_ext_info_voltage[] = { > > + PAC1921_EXT_INFO_SCALE_AVAIL, > > + {} > > +}; > > > > + > > +static irqreturn_t pac1921_trigger_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *idev = pf->indio_dev; > > + struct pac1921_priv *priv = iio_priv(idev); > > + int ret; > > + int bit; > > + int ch = 0; > > + > > + guard(mutex)(&priv->lock); > > + > > + if (!pac1921_data_ready(priv)) > > Interesting corner case that maybe could have done with a comment. > Seems could be triggered by a spurious interrupt, or sampling too early. > > I think only the second one is likely to happen, so shouldn't be a > problem to acknowledge that trigger. > Yes, my intent here was to prevent userspace from receiving invalid data if sampled too early: for example the user could arm a timer that would trigger an interrupt before the first integration is complete. This could happen not just after the first driver initialization phase but also after a configuration change (gains or number of integration samples reflecting a user change of scale or oversampling_ratio respectively). > > + goto done; > > + > > + ret = pac1921_check_push_overflow(idev, pf->timestamp); > > + if (ret) > > + goto done; > > + > > + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { > > This needs an update as we crossed with Nuno's series that removes access > to masklength. I can fix whilst applying by using > iio_for_each_active_channel() > > > > + u16 val; > > + > > + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); > > + if (ret) > > + goto done; > > + > > + priv->scan.chan[ch++] = val; > > + } > > + > > + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); > > + > > +done: > > + iio_trigger_notify_done(idev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > +/* > > + * Initialize device by writing initial configuration and putting it into > > + * integration state. > > + * > > + * Must be called with lock held when called after first initialization > > + * (e.g. from pm resume) > > + */ > > +static int pac1921_init(struct pac1921_priv *priv) > > +{ > > + unsigned int val; > > + int ret; > > + > > + /* Enter READ state before configuration */ > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > + PAC1921_INT_CFG_INTEN, 0); > > + if (ret) > > + return ret; > > + > > + /* Configure gains, use 14-bits measurement resolution (HW default) */ > > + val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | > > + (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); > > Why are these cases needed? > Each of those values is going to fit in a u8 just fine and it's getting > written to a much larger variable. > FIELD_PREP result type would be a long unsigned int due to the GENMASK type and -Wconversion would trigger a warning. The explicit casts is just to address -Wconversion warnings and to "state" that such casts are safe. In this way with -Wconversion (KBUILD_EXTRA_WARN=3) one could easily spot those other implicit casts that would end up with unwanted data corruption. I thought it to be a common practice and I also saw it in some other kernel patches, for example https://lore.kernel.org/all/1540883612.2354.2.camel@smigroup.net/ , but maybe it's not that common as I thought. I also see that maybe in this case casting to unsigned int would have likely been more clear. Thanks, Matteo Martelli
> > > + > > > +/* > > > + * Emit on sysfs the list of available scales contained in scales_tbl > > > + * > > > + * TODO:: this function can be replaced with iio_format_avail_list() if the > > > + * latter will ever be exported. > > > > You could just have added a precursor patch doing that. > > If you have time I'd certainly consider a patch that does export that function > > and uses it here. > > > I wasn't sure that one usage was enough to justify the export. I could > definitely do it, I am assuming it would now go to a new patch series since > this has already been merged into testing, right? The requirements for justifying exporting an existing function is less than it would be to add a new one. As such I think it makes sense. As you note, needs a separate patch on top of the tree. > > > > + * > > > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for > > > + * the current scales table) > > > + */ > > > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], > > > + size_t size, char *buf) > > > +{ > > > + ssize_t len = 0; > > > + > > > + for (unsigned int i = 0; i < size; i++) { > > > + if (i != 0) { > > > + len += sysfs_emit_at(buf, len, " "); > > > + if (len >= PAGE_SIZE) > > > + return -EFBIG; > > > + } > > > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], > > > + scales_tbl[i][1]); > > > + if (len >= PAGE_SIZE) > > > + return -EFBIG; > > > + } > > > + > > > + len += sysfs_emit_at(buf, len, "\n"); > > > + return len; > > > +} > > > + > > > +/* > > > + * Read available scales for a specific channel > > > + * > > > + * NOTE: using extended info insted of iio.read_avail() because access to > > > + * current scales must be locked as they depend on shunt resistor which may > > > + * change runtime. Caller of iio.read_avail() would access the table unlocked > > > + * instead. > > > > That's a corner case we should think about closing. Would require an indicator > > to read_avail that the buffer it has been passed is a snapshot that it should > > free on completion of the string building. I don't like passing ownership > > of data around like that, but it is fiddly to do anything else given > > any simple double buffering is subject to race conditions. > > > If I understand your suggestion the driver would allocate a new table and copy > the values into it at each read_avail() call. Then > iio_read_channel_info_avail() would free the buffer if some sort of > free-after-use indicator flag is set. I guess such indicator might be set via an > additional read_avail function argument (would be an extensive API change) or > maybe via a new iio_chan_spec attribute. Probably needs to be in read_avail() as otherwise we end up with yet more masks. However, doesn't need to be global. read_avail_ext() could be added that is used in preference to read_avail() if it is supplied. That new one can be used only be drivers that need to handle the allocation and free. However I prefer the explicit resource free option as we can in theory at least do much cleverer things than simply freeing the buffer. > > > An alternative would use a key of sometype to associate individual read_avail > > calls with new ones to read_avail_release_resource. That might be cleaner. > > > Are you referring to introduce a new read_avail_realease_resource callback that > would be called at the end of iio_read_channel_info_avail() if set? Similarly > to the previous point the driver would allocate a new table and copy the values > into it at each read_avail() call, but the driver would also define a release > callback to free such table. If otherwise you are referring to something less > trivial, is there a similar API in the kernel that can be referred to for > clarity? Indeed what you suggest. Key is it puts the burden on the driver to do it's own management. That avoids handing ownership of the buffer to the core which is a pattern I'm not that keen on if we can avoid it. The new callback would take the buffer pointer that came back from read_avail() and pass that back to the driver. In simple case the driver could just free the buffer. However, it could also do some cleverer stuff to keep it around if a write hasn't raced with this code. That might make sense if it's a big table and calculating the values is expensive. > > > oh well, a cleanup job for another day. I suspect we have drivers today > > that are subject to tearing of their available lists. > > > I've just taken a quick look at the other drivers and the following twos seem > to have the race condition issue since they are updating an available table > during a write_raw() call and also exposing it during a read_avail() call: > * drivers/iio/light/as73211.c: see int_time_avail table > * drivers/iio/adc/ad7192.c: see filter_freq_avail table > > There might be others, I've only looked into those that seemed likely to have > this issue after some trivial greps. > > Is there already a common way for iio to keep track of open issues (e.g. Issue > tracker/TODO lists/etc)? Not really. Email to the list tends to be the most we do for tracking. I have had various todo lists public over the years, but they tend to rot. Fix stuff before we forget about it! :( > > > > +static int pac1921_init(struct pac1921_priv *priv) > > > +{ > > > + unsigned int val; > > > + int ret; > > > + > > > + /* Enter READ state before configuration */ > > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, > > > + PAC1921_INT_CFG_INTEN, 0); > > > + if (ret) > > > + return ret; > > > + > > > + /* Configure gains, use 14-bits measurement resolution (HW default) */ > > > + val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | > > > + (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); > > > > Why are these cases needed? > > Each of those values is going to fit in a u8 just fine and it's getting > > written to a much larger variable. > > > FIELD_PREP result type would be a long unsigned int due to the GENMASK type and > -Wconversion would trigger a warning. The explicit casts is just to address > -Wconversion warnings and to "state" that such casts are safe. In these cases the compiler can see the value range so it 'shouldn't' be necessary. The compiler should be able to trivially establish there isn't a problem. I can't see enough of the example to tell if the compiler has the visibility of that function call that would be necessary to establish if the value is big enough. Of course, sometimes we get a dumb compiler so maybe it will still warn. GCC docs state: " The option should not warn for explicit conversions or for cases where the value cannot in fact change despite the implicit conversion." https://gcc.gnu.org/wiki/NewWconversion and we should be in that category here. A lot of compiler work goes into ensuring that the false positive rates for this sort of warning are low. This particular case of a mask and shift by compile time constants should be easy to figure out! Jonathan > In this way > with -Wconversion (KBUILD_EXTRA_WARN=3) one could easily spot those other > implicit casts that would end up with unwanted data corruption. I thought it to > be a common practice and I also saw it in some other kernel patches, for > example https://lore.kernel.org/all/1540883612.2354.2.camel@smigroup.net/ , but > maybe it's not that common as I thought. > I also see that maybe in this case casting to unsigned int would have likely > been more clear. > > Thanks, > Matteo Martelli
Jonathan Cameron wrote: > > > > + > > > > +/* > > > > + * Emit on sysfs the list of available scales contained in scales_tbl > > > > + * > > > > + * TODO:: this function can be replaced with iio_format_avail_list() if the > > > > + * latter will ever be exported. > > > > > > You could just have added a precursor patch doing that. > > > If you have time I'd certainly consider a patch that does export that function > > > and uses it here. > > > > > I wasn't sure that one usage was enough to justify the export. I could > > definitely do it, I am assuming it would now go to a new patch series since > > this has already been merged into testing, right? > The requirements for justifying exporting an existing function is less > than it would be to add a new one. As such I think it makes sense. > > As you note, needs a separate patch on top of the tree. > I will try to address this more generally by adding a new read_avail_release_resource() iio_info function, see below. If that goes through, exporting the iio_format_avail_list() would not be necessary since the driver could directly use the read_avail iio_info function. > > > > > > + * > > > > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for > > > > + * the current scales table) > > > > + */ > > > > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], > > > > + size_t size, char *buf) > > > > +{ > > > > + ssize_t len = 0; > > > > + > > > > + for (unsigned int i = 0; i < size; i++) { > > > > + if (i != 0) { > > > > + len += sysfs_emit_at(buf, len, " "); > > > > + if (len >= PAGE_SIZE) > > > > + return -EFBIG; > > > > + } > > > > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], > > > > + scales_tbl[i][1]); > > > > + if (len >= PAGE_SIZE) > > > > + return -EFBIG; > > > > + } > > > > + > > > > + len += sysfs_emit_at(buf, len, "\n"); > > > > + return len; > > > > +} > > > > + > > > > +/* > > > > + * Read available scales for a specific channel > > > > + * > > > > + * NOTE: using extended info insted of iio.read_avail() because access to > > > > + * current scales must be locked as they depend on shunt resistor which may > > > > + * change runtime. Caller of iio.read_avail() would access the table unlocked > > > > + * instead. > > > > > > That's a corner case we should think about closing. Would require an indicator > > > to read_avail that the buffer it has been passed is a snapshot that it should > > > free on completion of the string building. I don't like passing ownership > > > of data around like that, but it is fiddly to do anything else given > > > any simple double buffering is subject to race conditions. > > > > > If I understand your suggestion the driver would allocate a new table and copy > > the values into it at each read_avail() call. Then > > iio_read_channel_info_avail() would free the buffer if some sort of > > free-after-use indicator flag is set. I guess such indicator might be set via an > > additional read_avail function argument (would be an extensive API change) or > > maybe via a new iio_chan_spec attribute. > > Probably needs to be in read_avail() as otherwise we end up with yet more masks. > However, doesn't need to be global. read_avail_ext() could be added that > is used in preference to read_avail() if it is supplied. That new one can > be used only be drivers that need to handle the allocation and free. > However I prefer the explicit resource free option as we can in theory > at least do much cleverer things than simply freeing the buffer. > > > > > > An alternative would use a key of sometype to associate individual read_avail > > > calls with new ones to read_avail_release_resource. That might be cleaner. > > > > > Are you referring to introduce a new read_avail_realease_resource callback that > > would be called at the end of iio_read_channel_info_avail() if set? Similarly > > to the previous point the driver would allocate a new table and copy the values > > into it at each read_avail() call, but the driver would also define a release > > callback to free such table. If otherwise you are referring to something less > > trivial, is there a similar API in the kernel that can be referred to for > > clarity? > > Indeed what you suggest. Key is it puts the burden on the driver to do it's > own management. That avoids handing ownership of the buffer to the core > which is a pattern I'm not that keen on if we can avoid it. > > The new callback would take the buffer pointer that came back from read_avail() > and pass that back to the driver. In simple case the driver could just > free the buffer. However, it could also do some cleverer stuff to keep > it around if a write hasn't raced with this code. That might make sense if > it's a big table and calculating the values is expensive. > I am trying to achieve this and it looks pretty straightforward for the case we considered, iio would be extended like the following: diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index e6fad8a6a1fc..fe6ad8e9722f 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -860,12 +860,20 @@ static ssize_t iio_read_channel_info_avail(struct device *dev, return ret; switch (ret) { case IIO_AVAIL_LIST: - return iio_format_avail_list(buf, vals, type, length); + ret = iio_format_avail_list(buf, vals, type, length); + break; case IIO_AVAIL_RANGE: - return iio_format_avail_range(buf, vals, type); + ret = iio_format_avail_range(buf, vals, type); + break; default: - return -EINVAL; + ret = -EINVAL; } + + if (indio_dev->info->read_avail_release_resource) + indio_dev->info->read_avail_release_resource( + indio_dev, this_attr->c, vals, this_attr->address); + + return ret; } /** diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index f6c0499853bb..0ab08b94bad0 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -491,6 +491,10 @@ struct iio_info { int *length, long mask); + void (*read_avail_release_resource)(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int *vals, long mask); + int (*write_raw)(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, And with the following usage example for the pac1921 driver: 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_SCALE: switch (chan->channel) { //... case PAC1921_CHAN_CURRENT: { struct pac1921_priv *priv = iio_priv(indio_dev); size_t len; int *buf; len = ARRAY_SIZE(priv->current_scales) * 2; buf = kmalloc_array(len, sizeof(int), GFP_KERNEL); if (!buf) return -ENOMEM; for (unsigned int i = 0; i < len; i++) buf[i] = ((int *)priv->current_scales)[i]; *vals = buf; *length = (int)len; *type = IIO_VAL_INT_PLUS_NANO; return IIO_AVAIL_LIST; } default: return -EINVAL; } default: return -EINVAL; } } static void pac1921_read_avail_release_res(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, const int *vals, long mask) { if (mask == IIO_CHAN_INFO_SCALE && chan->channel == PAC1921_CHAN_CURRENT) kfree(vals); } static const struct iio_info pac1921_iio = { //... .read_avail = pac1921_read_avail, .read_avail_release_resource = pac1921_read_avail_release_res, }; However I noticed that some consumer drivers also expose the producer's available lists through the following functions: - iio_read_avail_channel_attribute() - iio_read_avail_channel_raw() - iio_channel_read_max() - iio_channel_read_min() While addressing the read_max()/read_min() is trivial since the release_resource() can be called at the end of those function, I think the first twos should be tracked as well for later release by the consumer drivers. So for example the consumer driver would also expose a iio_read_avail_channel_attribute_release_resource() (any suggestion for shorter function names?) mapped to the read_avail_release_resource() iio_info function. However the fact that iio_read_avail_channel_attribute() locks on info_exist_lock, makes me think that the driver could be unregistered between a read_avail() and a read_avail_release_resource() and in that case an allocated list would be leaked, right? Any suggestion on how best handle this case? My guess is to let iio destroy the list at some point during device release, that would be done if the list allocation was done through devm_kmalloc (or similar) but I think it would result in double frees during usual case, so maybe there should be a way to let it free the list only if not already freed? Or maybe a complete different approach? > > > > > oh well, a cleanup job for another day. I suspect we have drivers today > > > that are subject to tearing of their available lists. > > > > > I've just taken a quick look at the other drivers and the following twos seem > > to have the race condition issue since they are updating an available table > > during a write_raw() call and also exposing it during a read_avail() call: > > * drivers/iio/light/as73211.c: see int_time_avail table > > * drivers/iio/adc/ad7192.c: see filter_freq_avail table > > > > There might be others, I've only looked into those that seemed likely to have > > this issue after some trivial greps. > > > > Is there already a common way for iio to keep track of open issues (e.g. Issue > > tracker/TODO lists/etc)? > > Not really. Email to the list tends to be the most we do for tracking. > I have had various todo lists public over the years, but they tend to rot. > > Fix stuff before we forget about it! :( > I could try to provide fix patches for those two drivers as well, but I could not test them on the real HW. I am wondering whether to add them to the same release_resource() patch series or into a separate series since those fixes could be sit for a while waiting for additional tests. Thanks, Matteo Martelli
On Tue, 06 Aug 2024 11:53:12 +0200 Matteo Martelli <matteomartelli3@gmail.com> wrote: > Jonathan Cameron wrote: > > > > > + > > > > > +/* > > > > > + * Emit on sysfs the list of available scales contained in scales_tbl > > > > > + * > > > > > + * TODO:: this function can be replaced with iio_format_avail_list() if the > > > > > + * latter will ever be exported. > > > > > > > > You could just have added a precursor patch doing that. > > > > If you have time I'd certainly consider a patch that does export that function > > > > and uses it here. > > > > > > > I wasn't sure that one usage was enough to justify the export. I could > > > definitely do it, I am assuming it would now go to a new patch series since > > > this has already been merged into testing, right? > > The requirements for justifying exporting an existing function is less > > than it would be to add a new one. As such I think it makes sense. > > > > As you note, needs a separate patch on top of the tree. > > > I will try to address this more generally by adding a new > read_avail_release_resource() iio_info function, see below. If that goes > through, exporting the iio_format_avail_list() would not be necessary since the > driver could directly use the read_avail iio_info function. > > > > > > > > > + * > > > > > + * Must be called with lock held if the scales_tbl can change runtime (e.g. for > > > > > + * the current scales table) > > > > > + */ > > > > > +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], > > > > > + size_t size, char *buf) > > > > > +{ > > > > > + ssize_t len = 0; > > > > > + > > > > > + for (unsigned int i = 0; i < size; i++) { > > > > > + if (i != 0) { > > > > > + len += sysfs_emit_at(buf, len, " "); > > > > > + if (len >= PAGE_SIZE) > > > > > + return -EFBIG; > > > > > + } > > > > > + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], > > > > > + scales_tbl[i][1]); > > > > > + if (len >= PAGE_SIZE) > > > > > + return -EFBIG; > > > > > + } > > > > > + > > > > > + len += sysfs_emit_at(buf, len, "\n"); > > > > > + return len; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Read available scales for a specific channel > > > > > + * > > > > > + * NOTE: using extended info insted of iio.read_avail() because access to > > > > > + * current scales must be locked as they depend on shunt resistor which may > > > > > + * change runtime. Caller of iio.read_avail() would access the table unlocked > > > > > + * instead. > > > > > > > > That's a corner case we should think about closing. Would require an indicator > > > > to read_avail that the buffer it has been passed is a snapshot that it should > > > > free on completion of the string building. I don't like passing ownership > > > > of data around like that, but it is fiddly to do anything else given > > > > any simple double buffering is subject to race conditions. > > > > > > > If I understand your suggestion the driver would allocate a new table and copy > > > the values into it at each read_avail() call. Then > > > iio_read_channel_info_avail() would free the buffer if some sort of > > > free-after-use indicator flag is set. I guess such indicator might be set via an > > > additional read_avail function argument (would be an extensive API change) or > > > maybe via a new iio_chan_spec attribute. > > > > Probably needs to be in read_avail() as otherwise we end up with yet more masks. > > However, doesn't need to be global. read_avail_ext() could be added that > > is used in preference to read_avail() if it is supplied. That new one can > > be used only be drivers that need to handle the allocation and free. > > However I prefer the explicit resource free option as we can in theory > > at least do much cleverer things than simply freeing the buffer. > > > > > > > > > An alternative would use a key of sometype to associate individual read_avail > > > > calls with new ones to read_avail_release_resource. That might be cleaner. > > > > > > > Are you referring to introduce a new read_avail_realease_resource callback that > > > would be called at the end of iio_read_channel_info_avail() if set? Similarly > > > to the previous point the driver would allocate a new table and copy the values > > > into it at each read_avail() call, but the driver would also define a release > > > callback to free such table. If otherwise you are referring to something less > > > trivial, is there a similar API in the kernel that can be referred to for > > > clarity? > > > > Indeed what you suggest. Key is it puts the burden on the driver to do it's > > own management. That avoids handing ownership of the buffer to the core > > which is a pattern I'm not that keen on if we can avoid it. > > > > The new callback would take the buffer pointer that came back from read_avail() > > and pass that back to the driver. In simple case the driver could just > > free the buffer. However, it could also do some cleverer stuff to keep > > it around if a write hasn't raced with this code. That might make sense if > > it's a big table and calculating the values is expensive. > > > I am trying to achieve this and it looks pretty straightforward for the case we > considered, iio would be extended like the following: > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index e6fad8a6a1fc..fe6ad8e9722f 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -860,12 +860,20 @@ static ssize_t iio_read_channel_info_avail(struct device *dev, > return ret; > switch (ret) { > case IIO_AVAIL_LIST: > - return iio_format_avail_list(buf, vals, type, length); > + ret = iio_format_avail_list(buf, vals, type, length); > + break; > case IIO_AVAIL_RANGE: > - return iio_format_avail_range(buf, vals, type); > + ret = iio_format_avail_range(buf, vals, type); > + break; > default: > - return -EINVAL; > + ret = -EINVAL; > } > + > + if (indio_dev->info->read_avail_release_resource) > + indio_dev->info->read_avail_release_resource( > + indio_dev, this_attr->c, vals, this_attr->address); > + > + return ret; > } > > /** > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index f6c0499853bb..0ab08b94bad0 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -491,6 +491,10 @@ struct iio_info { > int *length, > long mask); > > + void (*read_avail_release_resource)(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int *vals, long mask); > + > int (*write_raw)(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int val, > > And with the following usage example for the pac1921 driver: > > 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_SCALE: > switch (chan->channel) { > //... > case PAC1921_CHAN_CURRENT: { > struct pac1921_priv *priv = iio_priv(indio_dev); > size_t len; > int *buf; > > len = ARRAY_SIZE(priv->current_scales) * 2; > buf = kmalloc_array(len, sizeof(int), GFP_KERNEL); > if (!buf) > return -ENOMEM; > > for (unsigned int i = 0; i < len; i++) > buf[i] = ((int *)priv->current_scales)[i]; > > *vals = buf; > *length = (int)len; > *type = IIO_VAL_INT_PLUS_NANO; > return IIO_AVAIL_LIST; > } > default: > return -EINVAL; > } > default: > return -EINVAL; > } > } > > static void pac1921_read_avail_release_res(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > const int *vals, long mask) > { > if (mask == IIO_CHAN_INFO_SCALE && > chan->channel == PAC1921_CHAN_CURRENT) > kfree(vals); > } > > static const struct iio_info pac1921_iio = { > //... > .read_avail = pac1921_read_avail, > .read_avail_release_resource = pac1921_read_avail_release_res, > }; > > However I noticed that some consumer drivers also expose the producer's > available lists through the following functions: > - iio_read_avail_channel_attribute() > - iio_read_avail_channel_raw() > - iio_channel_read_max() > - iio_channel_read_min() > > While addressing the read_max()/read_min() is trivial since the > release_resource() can be called at the end of those function, I think the > first twos should be tracked as well for later release by the consumer drivers. We can mostly avoid this by taking a copy in the consumers that use these interfaces then immediately calling the release. > So for example the consumer driver would also expose a > iio_read_avail_channel_attribute_release_resource() (any suggestion for shorter > function names?) mapped to the read_avail_release_resource() iio_info function. > However the fact that iio_read_avail_channel_attribute() locks on > info_exist_lock, makes me think that the driver could be unregistered between a > read_avail() and a read_avail_release_resource() and in that case an allocated > list would be leaked, right? Any suggestion on how best handle this case? My > guess is to let iio destroy the list at some point during device release, that > would be done if the list allocation was done through devm_kmalloc (or similar) > but I think it would result in double frees during usual case, so maybe there > should be a way to let it free the list only if not already freed? Or maybe a > complete different approach? Locking is a bit of a pain. I don't want to reference count for something as trivial as this. Perhaps the original idea of a release callback isn't best solution for these in kernel interfaces and we should just 'always' make a copy of the data to avoid the lifetime issue. I don't want to do that for the IIO core case because it's a big waste of memory and we don't have the lifetime issues, but for the in kernel consumer interfaces copying sounds fine. > > > > > > > > oh well, a cleanup job for another day. I suspect we have drivers today > > > > that are subject to tearing of their available lists. > > > > > > > I've just taken a quick look at the other drivers and the following twos seem > > > to have the race condition issue since they are updating an available table > > > during a write_raw() call and also exposing it during a read_avail() call: > > > * drivers/iio/light/as73211.c: see int_time_avail table > > > * drivers/iio/adc/ad7192.c: see filter_freq_avail table > > > > > > There might be others, I've only looked into those that seemed likely to have > > > this issue after some trivial greps. > > > > > > Is there already a common way for iio to keep track of open issues (e.g. Issue > > > tracker/TODO lists/etc)? > > > > Not really. Email to the list tends to be the most we do for tracking. > > I have had various todo lists public over the years, but they tend to rot. > > > > Fix stuff before we forget about it! :( > > > I could try to provide fix patches for those two drivers as well, but I could > not test them on the real HW. I am wondering whether to add them to the same > release_resource() patch series or into a separate series since those fixes > could be sit for a while waiting for additional tests. Either is fine. I don't necessarily have to pick the whole series up in one go. Just put those other drivers towards the end. Jonathan > > Thanks, > Matteo Martelli >
diff --git a/MAINTAINERS b/MAINTAINERS index 06ecfa64a39a..eb32f8915202 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..0993253b435c 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -991,6 +991,19 @@ 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 + select REGMAP_I2C + select IIO_BUFFER + select 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 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..a21dd772467e --- /dev/null +++ b/drivers/iio/adc/pac1921.c @@ -0,0 +1,1263 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * IIO driver for PAC1921 High-Side Power/Current Monitor + * + * Copyright (C) 2024 Matteo Martelli <matteomartelli3@gmail.com> + */ + +#include <asm/unaligned.h> +#include <linux/bitfield.h> +#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> +#include <linux/units.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_DI_GAIN_MASK GENMASK(5, 3) +#define PAC1921_GAIN_DV_GAIN_MASK GENMASK(2, 0) + +/* pac1921 integration configuration bits */ +#define PAC1921_INT_CFG_SMPL_MASK GENMASK(7, 4) +#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_MASK GENMASK(7, 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 result registers mask and resolution */ +#define PAC1921_RES_MASK GENMASK(15, 6) +#define PAC1921_RES_RESOLUTION 1023 + +/* 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 +/* Time to first communication after power up (tINT_T) */ +#define PAC1921_POWERUP_TIME_MS 20 +/* Time from Sleep State to Start of Integration Period (tSLEEP_TO_INT) */ +#define PAC1921_SLEEP_TO_INT_TIME_US 86 + +/* pac1921 defaults */ +#define PAC1921_DEFAULT_DV_GAIN 0 /* 2^(value): 1x gain (HW default) */ +#define PAC1921_DEFAULT_DI_GAIN 0 /* 2^(value): 1x gain (HW default) */ +#define PAC1921_DEFAULT_NUM_SAMPLES 0 /* 2^(value): 1 sample (HW default) */ + +/* + * Pre-computed scale factors for BUS voltage + * format: IIO_VAL_INT_PLUS_NANO + * unit: mV + * + * Vbus scale (mV) = max_vbus (mV) / dv_gain / resolution + */ +static const int pac1921_vbus_scales[][2] = { + {31, 280547409}, /* dv_gain x1 */ + {15, 640273704}, /* dv_gain x2 */ + {7, 820136852}, /* dv_gain x4 */ + {3, 910068426}, /* dv_gain x8 */ + {1, 955034213}, /* dv_gain x16 */ + {0, 977517106} /* dv_gain x32 */ +}; + +/* + * Pre-computed scales for SENSE voltage + * format: IIO_VAL_INT_PLUS_NANO + * unit: mV + * + * Vsense scale (mV) = max_vsense (mV) / di_gain / resolution + */ +static const int pac1921_vsense_scales[][2] = { + {0, 97751710}, /* di_gain x1 */ + {0, 48875855}, /* di_gain x2 */ + {0, 24437927}, /* di_gain x4 */ + {0, 12218963}, /* di_gain x8 */ + {0, 6109481}, /* di_gain x16 */ + {0, 3054740}, /* di_gain x32 */ + {0, 1527370}, /* di_gain x64 */ + {0, 763685} /* di_gain x128 */ +}; + +/* + * Numbers of samples used to integrate measurements at the end of an + * integration period. + * + * Changing the number of samples affects the integration period: higher the + * number of samples, longer the integration period. + * + * These correspond to the oversampling ratios available exposed to userspace. + */ +static const int pac1921_int_num_samples[] = { + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048 +}; + +/* + * 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, 14-Bit resolution and post + * filters on. Each index corresponds to a specific number of samples from 1 + * to 2048. + */ +static const unsigned int pac1921_int_periods_usecs[] = { + 2720, /* 1 sample */ + 4050, /* 2 samples */ + 6790, /* 4 samples */ + 12200, /* 8 samples */ + 23000, /* 16 samples */ + 46000, /* 32 samples */ + 92000, /* 64 samples */ + 184000, /* 128 samples */ + 368000, /* 256 samples */ + 736000, /* 512 samples */ + 1471000, /* 1024 samples */ + 2941000 /* 2048 samples */ +}; + +/* 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; /* uOhm */ + u8 dv_gain; + u8 di_gain; + u8 n_samples; + u8 prev_ovf_flags; + u8 ovf_enabled_events; + + bool first_integr_started; + bool first_integr_done; + unsigned long integr_started_time_jiffies; + unsigned int integr_period_usecs; + + int current_scales[ARRAY_SIZE(pac1921_vsense_scales)][2]; + + struct { + u16 chan[PAC1921_NUM_MEAS_CHANS]; + s64 timestamp __aligned(8); + } scan; +}; + +/* + * 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; + + /* + * Data valid after the device entered into integration state, + * considering worst case where the device was in sleep state, + * and completed the first integration period. + */ + t_ready = priv->integr_started_time_jiffies + + usecs_to_jiffies(PAC1921_SLEEP_TO_INT_TIME_US) + + usecs_to_jiffies(priv->integr_period_usecs); + + if (time_before(jiffies, t_ready)) + return false; + + priv->first_integr_done = true; + } + + return true; +} + +static inline void pac1921_calc_scale(int dividend, int divisor, int *val, + int *val2) +{ + s64 tmp; + + tmp = div_s64(dividend * (s64)NANO, divisor); + *val = (int)div_s64_rem(tmp, NANO, val2); +} + +/* + * Fill the table of scale factors for current + * format: IIO_VAL_INT_PLUS_NANO + * unit: mA + * + * Vsense LSB (nV) = max_vsense (nV) * di_gain / resolution + * Current scale (mA) = Vsense LSB (nV) / shunt (uOhm) + * + * Must be called with held lock when updating after first initialization. + */ +static void pac1921_calc_current_scales(struct pac1921_priv *priv) +{ + for (unsigned int i = 0; i < ARRAY_SIZE(priv->current_scales); i++) { + int max = (PAC1921_MAX_VSENSE_MV * MICRO) >> i; + int vsense_lsb = DIV_ROUND_CLOSEST(max, PAC1921_RES_RESOLUTION); + + pac1921_calc_scale(vsense_lsb, (int)priv->rshunt_uohm, + &priv->current_scales[i][0], + &priv->current_scales[i][1]); + } +} + +/* + * 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; + + 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) && + priv->ovf_enabled_events & 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) && + priv->ovf_enabled_events & 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) && + priv->ovf_enabled_events & 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, + u16 *val) +{ + int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, val, + sizeof(*val)); + if (ret) + return ret; + + *val = (u16)FIELD_GET(PAC1921_RES_MASK, get_unaligned_be16(val)); + + 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); + + guard(mutex)(&priv->lock); + + switch (mask) { + case IIO_CHAN_INFO_RAW: { + s64 ts; + u16 res_val; + int ret; + + if (!pac1921_data_ready(priv)) + return -EBUSY; + + ts = iio_get_time_ns(indio_dev); + + ret = pac1921_check_push_overflow(indio_dev, ts); + if (ret) + return ret; + + ret = pac1921_read_res(priv, chan->address, &res_val); + if (ret) + return ret; + + *val = (int)res_val; + + return IIO_VAL_INT; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + *val = pac1921_vbus_scales[priv->dv_gain][0]; + *val2 = pac1921_vbus_scales[priv->dv_gain][1]; + return IIO_VAL_INT_PLUS_NANO; + + case PAC1921_CHAN_VSENSE: + *val = pac1921_vsense_scales[priv->di_gain][0]; + *val2 = pac1921_vsense_scales[priv->di_gain][1]; + return IIO_VAL_INT_PLUS_NANO; + + case PAC1921_CHAN_CURRENT: + *val = priv->current_scales[priv->di_gain][0]; + *val2 = priv->current_scales[priv->di_gain][1]; + return IIO_VAL_INT_PLUS_NANO; + + case PAC1921_CHAN_POWER: { + /* + * Power scale factor in mW: + * Current scale (mA) * max_vbus (V) / dv_gain + */ + + /* Get current scale based on di_gain */ + int *curr_scale = priv->current_scales[priv->di_gain]; + + /* Convert current_scale from INT_PLUS_NANO to INT */ + s64 tmp = curr_scale[0] * (s64)NANO + curr_scale[1]; + + /* Multiply by max_vbus (V) / dv_gain */ + tmp *= PAC1921_MAX_VBUS_V >> (int)priv->dv_gain; + + /* Convert back to INT_PLUS_NANO */ + *val = (int)div_s64_rem(tmp, NANO, val2); + + return IIO_VAL_INT_PLUS_NANO; + } + default: + return -EINVAL; + } + + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + *val = pac1921_int_num_samples[priv->n_samples]; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SAMP_FREQ: + /* + * The sampling frequency (Hz) is read-only and corresponds to + * how often the device provides integrated measurements into + * the result registers, thus it's 1/integration_period. + * The integration period depends on the number of integration + * samples, measurement resolution and post filters. + * + * 1/(integr_period_usecs/MICRO) = MICRO/integr_period_usecs + */ + *val = MICRO; + *val2 = (int)priv->integr_period_usecs; + return IIO_VAL_FRACTIONAL; + + 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_OVERSAMPLING_RATIO: + *type = IIO_VAL_INT; + *vals = pac1921_int_num_samples; + *length = ARRAY_SIZE(pac1921_int_num_samples); + return IIO_AVAIL_LIST; + 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 */ + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN); + if (ret) + return ret; + + /* + * Reset integration started time and mark this integration period as + * the first one so that new measurements will be considered as valid + * only at the end of this integration period. + */ + priv->integr_started_time_jiffies = jiffies; + priv->first_integr_done = false; + + return 0; +} + +/* + * Retrieve the index of the given scale (represented by scale_val and + * scale_val2) from scales_tbl. The returned index (if found) is the log2 of + * the gain corresponding to the given scale. + * + * Must be called with lock held if the scales_tbl can change runtime (e.g. for + * the current scales table) + */ +static int pac1921_lookup_scale(const int (*const scales_tbl)[2], size_t size, + int scale_val, int scale_val2) +{ + for (unsigned int i = 0; i < size; i++) + if (scales_tbl[i][0] == scale_val && + scales_tbl[i][1] == scale_val2) + return (int)i; + + return -EINVAL; +} + +/* + * Configure device with the given gain (only if changed) + * + * Must be called with lock held. + */ +static int pac1921_update_gain(struct pac1921_priv *priv, u8 *priv_val, u8 gain, + unsigned int mask) +{ + unsigned int reg_val; + int ret; + + if (*priv_val == gain) + return 0; + + reg_val = (gain << __ffs(mask)) & mask; + ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG, mask, reg_val); + if (ret) + return ret; + + *priv_val = gain; + + return 0; +} + +/* + * Given a scale factor represented by scale_val and scale_val2 with format + * IIO_VAL_INT_PLUS_NANO, find the corresponding gain value and write it to the + * device. + * + * Must be called with lock held. + */ +static int pac1921_update_gain_from_scale(struct pac1921_priv *priv, + struct iio_chan_spec const *chan, + int scale_val, int scale_val2) +{ + int ret; + + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + ret = pac1921_lookup_scale(pac1921_vbus_scales, + ARRAY_SIZE(pac1921_vbus_scales), + scale_val, scale_val2); + if (ret < 0) + return ret; + + return pac1921_update_gain(priv, &priv->dv_gain, (u8)ret, + PAC1921_GAIN_DV_GAIN_MASK); + case PAC1921_CHAN_VSENSE: + ret = pac1921_lookup_scale(pac1921_vsense_scales, + ARRAY_SIZE(pac1921_vsense_scales), + scale_val, scale_val2); + if (ret < 0) + return ret; + + return pac1921_update_gain(priv, &priv->di_gain, (u8)ret, + PAC1921_GAIN_DI_GAIN_MASK); + case PAC1921_CHAN_CURRENT: + ret = pac1921_lookup_scale(priv->current_scales, + ARRAY_SIZE(priv->current_scales), + scale_val, scale_val2); + if (ret < 0) + return ret; + + return pac1921_update_gain(priv, &priv->di_gain, (u8)ret, + PAC1921_GAIN_DI_GAIN_MASK); + default: + return -EINVAL; + } +} + +/* + * Retrieve the index of the given number of samples from the constant table. + * The returned index (if found) is the log2 of the given num_samples. + */ +static int pac1921_lookup_int_num_samples(int num_samples) +{ + for (unsigned int i = 0; i < ARRAY_SIZE(pac1921_int_num_samples); i++) + if (pac1921_int_num_samples[i] == num_samples) + return (int)i; + + return -EINVAL; +} + +/* + * Update the device with the given number of integration samples. + * + * Must be called with lock held. + */ +static int pac1921_update_int_num_samples(struct pac1921_priv *priv, + int num_samples) +{ + unsigned int reg_val; + u8 n_samples; + int ret; + + ret = pac1921_lookup_int_num_samples(num_samples); + if (ret < 0) + return ret; + + n_samples = (u8)ret; + + if (priv->n_samples == n_samples) + return 0; + + reg_val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, n_samples); + + ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_SMPL_MASK, reg_val); + if (ret) + return ret; + + priv->n_samples = n_samples; + + priv->integr_period_usecs = pac1921_int_periods_usecs[priv->n_samples]; + + return 0; +} + +static int pac1921_write_raw_get_fmt(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long info) +{ + switch (info) { + case IIO_CHAN_INFO_SCALE: + return IIO_VAL_INT_PLUS_NANO; + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +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); + + guard(mutex)(&priv->lock); + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + return pac1921_update_gain_from_scale(priv, chan, val, val2); + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: + return pac1921_update_int_num_samples(priv, val); + 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 int pac1921_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + + guard(mutex)(&priv->lock); + + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + return !!(priv->ovf_enabled_events & PAC1921_OVERFLOW_VBOV); + case PAC1921_CHAN_VSENSE: + case PAC1921_CHAN_CURRENT: + return !!(priv->ovf_enabled_events & PAC1921_OVERFLOW_VSOV); + case PAC1921_CHAN_POWER: + return !!(priv->ovf_enabled_events & PAC1921_OVERFLOW_VPOV); + default: + return -EINVAL; + } +} + +static int pac1921_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int state) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + u8 ovf_bit; + + guard(mutex)(&priv->lock); + + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + ovf_bit = PAC1921_OVERFLOW_VBOV; + break; + case PAC1921_CHAN_VSENSE: + case PAC1921_CHAN_CURRENT: + ovf_bit = PAC1921_OVERFLOW_VSOV; + break; + case PAC1921_CHAN_POWER: + ovf_bit = PAC1921_OVERFLOW_VPOV; + break; + default: + return -EINVAL; + } + + if (state) + priv->ovf_enabled_events |= ovf_bit; + else + priv->ovf_enabled_events &= ~ovf_bit; + + return 0; +} + +static int pac1921_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int *val, + int *val2) +{ + switch (info) { + case IIO_EV_INFO_VALUE: + *val = PAC1921_RES_RESOLUTION; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static const struct iio_info pac1921_iio = { + .read_raw = pac1921_read_raw, + .read_avail = pac1921_read_avail, + .write_raw = pac1921_write_raw, + .write_raw_get_fmt = pac1921_write_raw_get_fmt, + .read_label = pac1921_read_label, + .read_event_config = pac1921_read_event_config, + .write_event_config = pac1921_write_event_config, + .read_event_value = pac1921_read_event_value, +}; + +static ssize_t pac1921_read_shunt_resistor(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + int vals[2]; + + if (chan->channel != PAC1921_CHAN_CURRENT) + return -EINVAL; + + guard(mutex)(&priv->lock); + + vals[0] = (int)priv->rshunt_uohm; + vals[1] = MICRO; + + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals); +} + +static ssize_t pac1921_write_shunt_resistor(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); + u64 rshunt_uohm; + int val, val_fract; + int ret; + + if (chan->channel != PAC1921_CHAN_CURRENT) + return -EINVAL; + + ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract); + if (ret) + return ret; + + rshunt_uohm = (u32)val * MICRO + (u32)val_fract; + if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX) + return -EINVAL; + + guard(mutex)(&priv->lock); + + priv->rshunt_uohm = (u32)rshunt_uohm; + + pac1921_calc_current_scales(priv); + + return len; +} + +/* + * Emit on sysfs the list of available scales contained in scales_tbl + * + * TODO:: this function can be replaced with iio_format_avail_list() if the + * latter will ever be exported. + * + * Must be called with lock held if the scales_tbl can change runtime (e.g. for + * the current scales table) + */ +static ssize_t pac1921_format_scale_avail(const int (*const scales_tbl)[2], + size_t size, char *buf) +{ + ssize_t len = 0; + + for (unsigned int i = 0; i < size; i++) { + if (i != 0) { + len += sysfs_emit_at(buf, len, " "); + if (len >= PAGE_SIZE) + return -EFBIG; + } + len += sysfs_emit_at(buf, len, "%d.%09d", scales_tbl[i][0], + scales_tbl[i][1]); + if (len >= PAGE_SIZE) + return -EFBIG; + } + + len += sysfs_emit_at(buf, len, "\n"); + return len; +} + +/* + * Read available scales for a specific channel + * + * NOTE: using extended info insted of iio.read_avail() because access to + * current scales must be locked as they depend on shunt resistor which may + * change runtime. Caller of iio.read_avail() would access the table unlocked + * instead. + */ +static ssize_t pac1921_read_scale_avail(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct pac1921_priv *priv = iio_priv(indio_dev); + const int (*scales_tbl)[2]; + size_t size; + + switch (chan->channel) { + case PAC1921_CHAN_VBUS: + scales_tbl = pac1921_vbus_scales; + size = ARRAY_SIZE(pac1921_vbus_scales); + return pac1921_format_scale_avail(scales_tbl, size, buf); + + case PAC1921_CHAN_VSENSE: + scales_tbl = pac1921_vsense_scales; + size = ARRAY_SIZE(pac1921_vsense_scales); + return pac1921_format_scale_avail(scales_tbl, size, buf); + + case PAC1921_CHAN_CURRENT: { + guard(mutex)(&priv->lock); + scales_tbl = priv->current_scales; + size = ARRAY_SIZE(priv->current_scales); + return pac1921_format_scale_avail(scales_tbl, size, buf); + } + default: + return -EINVAL; + } +} + +#define PAC1921_EXT_INFO_SCALE_AVAIL { \ + .name = "scale_available", \ + .read = pac1921_read_scale_avail, \ + .shared = IIO_SEPARATE, \ +} + +static const struct iio_chan_spec_ext_info pac1921_ext_info_voltage[] = { + PAC1921_EXT_INFO_SCALE_AVAIL, + {} +}; + +static const struct iio_chan_spec_ext_info pac1921_ext_info_current[] = { + PAC1921_EXT_INFO_SCALE_AVAIL, + { + .name = "shunt_resistor", + .read = pac1921_read_shunt_resistor, + .write = pac1921_write_shunt_resistor, + .shared = IIO_SEPARATE, + }, + {} +}; + +static const struct iio_event_spec pac1921_overflow_event[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), + .mask_separate = BIT(IIO_EV_INFO_ENABLE), + }, +}; + +static const struct iio_chan_spec pac1921_channels[] = { + { + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | + BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .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_voltage, + }, + { + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | + BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .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_voltage, + }, + { + .type = IIO_CURRENT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | + BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .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_current, + }, + { + .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_OVERSAMPLING_RATIO) | + BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .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), + }, + 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); + int ret; + int bit; + int ch = 0; + + guard(mutex)(&priv->lock); + + if (!pac1921_data_ready(priv)) + goto done; + + ret = pac1921_check_push_overflow(idev, pf->timestamp); + if (ret) + goto done; + + for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) { + u16 val; + + ret = pac1921_read_res(priv, idev->channels[ch].address, &val); + if (ret) + goto done; + + priv->scan.chan[ch++] = val; + } + + iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp); + +done: + iio_trigger_notify_done(idev->trig); + + return IRQ_HANDLED; +} + +/* + * Initialize device by writing initial configuration and putting it into + * integration state. + * + * Must be called with lock held when called after first initialization + * (e.g. from pm resume) + */ +static int pac1921_init(struct pac1921_priv *priv) +{ + unsigned int val; + int ret; + + /* Enter READ state before configuration */ + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG, + PAC1921_INT_CFG_INTEN, 0); + if (ret) + return ret; + + /* Configure gains, use 14-bits measurement resolution (HW default) */ + val = (u8)FIELD_PREP(PAC1921_GAIN_DI_GAIN_MASK, priv->di_gain) | + (u8)FIELD_PREP(PAC1921_GAIN_DV_GAIN_MASK, priv->dv_gain); + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val); + if (ret) + return ret; + + /* + * Configure integration: + * - num of integration samples + * - filters enabled (HW default) + * - set READ/INT pin override (RIOV) to control operation mode via + * register instead of pin + */ + val = (u8)FIELD_PREP(PAC1921_INT_CFG_SMPL_MASK, priv->n_samples) | + PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN | + PAC1921_INT_CFG_RIOV; + 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 = (u8)FIELD_PREP(PAC1921_CONTROL_MXSL_MASK, + PAC1921_MXSL_VPOWER_FREE_RUN); + 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_started_time_jiffies = jiffies; + priv->integr_period_usecs = pac1921_int_periods_usecs[priv->n_samples]; + + return 0; +} + +static int pac1921_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct pac1921_priv *priv = iio_priv(indio_dev); + int ret; + + guard(mutex)(&priv->lock); + + priv->first_integr_started = false; + priv->first_integr_done = false; + + 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); + int ret; + + guard(mutex)(&priv->lock); + + ret = regulator_enable(priv->vdd); + if (ret) + return ret; + + msleep(PAC1921_POWERUP_TIME_MS); + + 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 regulator *regulator = data; + + regulator_disable(regulator); +} + +static int pac1921_probe(struct i2c_client *client) +{ + const struct i2c_device_id *id = i2c_client_get_device_id(client); + struct device *dev = &client->dev; + struct pac1921_priv *priv; + struct iio_dev *indio_dev; + int ret; + + 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"); + + devm_mutex_init(dev, &priv->lock); + + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN; + priv->di_gain = PAC1921_DEFAULT_DI_GAIN; + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES; + + ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", + &priv->rshunt_uohm); + if (ret) + return dev_err_probe(dev, ret, + "Cannot read shunt resistor property\n"); + if (priv->rshunt_uohm == 0 || priv->rshunt_uohm > INT_MAX) + return dev_err_probe(dev, -EINVAL, + "Invalid shunt resistor: %u\n", + priv->rshunt_uohm); + + pac1921_calc_current_scales(priv); + + 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->vdd); + if (ret) + return dev_err_probe( + dev, ret, + "Cannot add action for vdd regulator disposal\n"); + + msleep(PAC1921_POWERUP_TIME_MS); + + ret = pac1921_init(priv); + if (ret) + return dev_err_probe(dev, ret, "Cannot initialize device\n"); + + priv->iio_info = pac1921_iio; + + 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 * scale attributes to control voltage and current gains * oversampling ratio attribute to control the number of integration samples * sampling rate attribute that reflects the integration period * userspace attribute and DT parameter to control shunt resistor * simple power management support Limitations: * operation mode fixed to free-run integration * READ/INT pin and OUT pin not supported * no controls for measurement resolutions and filters Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/pac1921.c | 1263 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1284 insertions(+)