Hi Matteo,
Thank you very much for helping us adding PAC1921 support. I'm also
developing a similar driver and I could share the code with you to make
the driver better.
Also I have a few review comments, please, see bellow:
Best regards,
Marius
On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:
> +
> +/* 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),
> +};
I know that the regmap is the way forward, but the PAC devices are more
efficient if bulk read is done. Also when you are reading all values at
the same time, the Voltage, Current and the Power numbers will came
from the same sampling time and they will be correlated to each other.
> +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,
Not sure if this channel is useful. None of our clients ask us to
provide this information. This measurement could be calculated based on
the shunt and the current
> + 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 iio_info iio_info;
> +
> + /* Synchronize access to private members, and ensure
> atomicity of
> + * consecutive regmap operations.
> + */
> + struct mutex lock;
> +
> + u32 rshunt; /* uOhm */
As in the case of PAC1934 I would like to change the rshunt value
during the runtime. This is useful during a calibration phase.
> + bool high_res;
> + u8 dv_gain;
> + u8 di_gain;
> + u8 n_samples;
> + bool filters_en;
> + u8 prev_ovf_flags;
> +
> + bool first_integr_started;
> + bool first_integr_done;
> + unsigned long integr_start_time; /* jiffies */
> + unsigned int integr_period; /* usecs */
> +
> + struct {
> + u16 chan[PAC1921_NUM_MEAS_CHANS];
> + s64 timestamp __aligned(8);
> + } scan;
> +};
> +
> +/* The integration period depends on the configuration of number of
> integration
I think it should be a multi line comment here. The same comment for
multiple places in the driver.
> + * samples, measurement resolution and post filters. The following
> array
> + * contains integration periods, in microsecs unit, based on table
> 4-5 from
> + * datasheet considering power integration mode, 11-Bit resolution
> and post
> + * filters off. Each index corresponds to a specific number of
> samples from 1
> + * to 2048.
> + */
> +static const unsigned int pac1921_integr_periods_us[] = {
> + 930, 1460, 2410, 4320, 8050, 16100,
> + 32100, 64200, 128300, 257000, 513000, 1026000
> +};
>
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->channel) {
> + case PAC1921_CHAN_VBUS: {
> + /* Vbus scale factor in mV:
> + * max_vbus (mV) / vgain / resolution
> + */
> + guard(mutex)(&priv->lock);
> +
> + *val = PAC1921_MAX_VBUS_V * 1000;
> + *val2 = PAC1921_RES_RESOLUTION << (int)priv-
> >dv_gain;
> +
> + return IIO_VAL_FRACTIONAL;
> + }
> + case PAC1921_CHAN_VSENSE: {
> + /* Vsense voltage scale factor in mV:
> + * max_vsense (mV) / igain / resolution
> + */
> + guard(mutex)(&priv->lock);
> +
> + *val = PAC1921_MAX_VSENSE_MV;
> + *val2 = PAC1921_RES_RESOLUTION << (int)priv-
> >di_gain;
> +
> + return IIO_VAL_FRACTIONAL;
> + }
> + case PAC1921_CHAN_CURRENT: {
> + /* Current scale factor in mA:
> + * Vsense LSB (nV) / shunt (uOhm)
> + */
> + guard(mutex)(&priv->lock);
> +
> + *val = pac1921_vsense_lsb(priv->di_gain);
> + *val2 = (int)priv->rshunt;
> +
> + return IIO_VAL_FRACTIONAL;
> + }
> + case PAC1921_CHAN_POWER: {
>
> + /* Power scale factor in mW:
I think the scale here should be in microWatts. We have (mA)*(mV)
> + * Vsense LSB (nV) * max_vbus (V) / vgain /
> shunt (uOhm)
> + */
> + guard(mutex)(&priv->lock);
> +
> + *val = pac1921_vsense_lsb(priv->di_gain) *
> + (PAC1921_MAX_VBUS_V >> (int)priv-
> >dv_gain);
> + *val2 = (int)priv->rshunt;
> +
> + return IIO_VAL_FRACTIONAL;
> + }
> + default:
> + return -EINVAL;
> + }
> + break;
> +
> + case IIO_CHAN_INFO_HARDWAREGAIN:
HARDWAREGAIN should not be used here. The gain (for voltage and for the
current) should be included into the appropriate scale. A property
named scale_available should be added and the used could change the
gain by setting the scale. Please see PAC1934 and few other drivers.
> +static int pac1921_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int
> val,
> + int val2, long mask)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + return pac1921_update_gain(priv, chan, val);
This should not be needed...
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int pac1921_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int
> *length,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
this should be changed to use "scale_available"
> + switch (chan->channel) {
> + case PAC1921_CHAN_VBUS:
> + *length = ARRAY_SIZE(pac1921_dv_gains);
> + *vals = pac1921_dv_gains;
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_LIST;
> +
> + case PAC1921_CHAN_VSENSE:
> + case PAC1921_CHAN_CURRENT:
> + *length = ARRAY_SIZE(pac1921_di_gains);
> + *vals = pac1921_di_gains;
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int pac1921_read_label(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, char
> *label)
> +{
there is a "label" into the device tree that should be used here in
order to identify the device and what the device measures, like
GPU_vbus or IO_vbus.
> + switch (chan->channel) {
> + case PAC1921_CHAN_VBUS:
> + return sprintf(label, "vbus\n");
> + case PAC1921_CHAN_VSENSE:
> + return sprintf(label, "vsense\n");
> + case PAC1921_CHAN_CURRENT:
> + return sprintf(label, "current\n");
> + case PAC1921_CHAN_POWER:
> + return sprintf(label, "power\n");
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info pac1921_iio = {
> + .read_raw = pac1921_read_raw,
> + .write_raw = pac1921_write_raw,
> + .read_avail = pac1921_read_avail,
> + .read_label = pac1921_read_label,
> +};
> +
> +static int pac1921_get_resolution(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> + guard(mutex)(&priv->lock);
> +
> + if (priv->high_res)
> + return PAC1921_MEAS_RESOLUTION_IDX_HIGH;
> +
> + return PAC1921_MEAS_RESOLUTION_IDX_LOW;
> +}
> +
> +static int pac1921_set_resolution(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int val)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> + bool high_res;
> +
> + switch (val) {
> + case PAC1921_MEAS_RESOLUTION_IDX_LOW:
> + high_res = false;
> + break;
> + case PAC1921_MEAS_RESOLUTION_IDX_HIGH:
> + high_res = true;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> + guard(mutex)(&priv->lock);
> +
> + if (priv->high_res == high_res)
> + return 0;
> +
> + unsigned int mask = PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
> + unsigned int bits = high_res ?
> + 0 : PAC1921_GAIN_I_RES |
> PAC1921_GAIN_V_RES;
> +
> + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG,
> + mask, bits);
> + if (ret)
> + return ret;
> +
> + priv->high_res = high_res;
> +
> + return pac1921_update_integr_period(priv);
> +}
> +
> +static int pac1921_get_int_num_samples(struct iio_dev *indio_dev,
> + const struct iio_chan_spec
> *chan)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> + guard(mutex)(&priv->lock);
> +
> + return priv->n_samples;
> +}
> +
> +static int pac1921_set_int_num_samples(struct iio_dev *indio_dev,
> + const struct iio_chan_spec
> *chan,
> + unsigned int val)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> + if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX))
> + return -EINVAL;
> +
> + guard(mutex)(&priv->lock);
> +
> + if (priv->n_samples == val)
> + return 0;
> +
> + int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG,
> + PAC1921_INT_CFG_SMPL_MASK,
> + val <<
> PAC1921_INT_CFG_SMPL_SHIFT);
> + if (ret)
> + return ret;
> +
> + priv->n_samples = (u8)val;
> +
> + return pac1921_update_integr_period(priv);
> +}
> +
> +static ssize_t pac1921_read_filters_enabled(struct iio_dev
> *indio_dev,
> + uintptr_t private,
> + const struct
> iio_chan_spec *chan,
> + char *buf)
> +{
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> + bool enabled;
> +
> + scoped_guard(mutex, &priv->lock) {
> + enabled = priv->filters_en;
> + }
> + return sysfs_emit(buf, "%d\n", enabled);
> +}
> +
> +static ssize_t pac1921_write_filters_enabled(struct iio_dev
> *indio_dev,
> + uintptr_t private,
> + const struct
> iio_chan_spec *chan,
> + const char *buf, size_t
> len)
I would like to set each filter separately and let the user to decide
how to use this functionality. Not always both filters are needed.
> +static int pac1921_init(struct pac1921_priv *priv)
> +{
> + /* Enter READ state before configuration */
> + int ret = regmap_update_bits(priv->regmap,
> PAC1921_REG_INT_CFG,
> + PAC1921_INT_CFG_INTEN, 0);
> + if (ret)
> + return ret;
> +
> + /* Configure gains and measurements resolution */
> + unsigned int val = priv->di_gain <<
> PAC1921_GAIN_DI_GAIN_SHIFT |
> + priv->dv_gain <<
> PAC1921_GAIN_DV_GAIN_SHIFT;
> + if (!priv->high_res)
> + val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
> + ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val);
> + if (ret)
> + return ret;
> +
> + /* Configure integration:
> + * - num of integration samples, filters enabled/disabled
> + * - set READ/INT pin override (RIOV) to control operation
> mode via
> + * register instead of pin
> + */
> + val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT |
> + PAC1921_INT_CFG_RIOV;
> + if (priv->filters_en)
> + val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN;
> + ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val);
> + if (ret)
> + return ret;
> +
> + /* Init control register:
> + * - VPower free run integration mode
User should control what to output on the "OUT DAC" pin.
> + * - OUT pin full scale range: 3V (HW detault)
Full range for OUT should be configurable.
> + * - no timeout, no sleep, no sleep override, no recalc (HW
> defaults)
> + */
> + val = PAC1921_MXSL_VPOWER_FREE_RUN <<
> PAC1921_CONTROL_MXSL_SHIFT;
> + ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val);
> + if (ret)
> + return ret;
> +
> + /* Enable integration */
> + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
> + PAC1921_INT_CFG_INTEN,
> PAC1921_INT_CFG_INTEN);
> + if (ret)
> + return ret;
> +
> + priv->first_integr_started = true;
> + priv->integr_start_time = jiffies;
> +
> + return pac1921_update_integr_period(priv);
> +}
> +static int pac1921_parse_properties(struct pac1921_priv *priv)
> +{
> + struct device *dev = &priv->client->dev;
> + u32 prop;
> +
> + int ret = device_property_read_u32(dev, "shunt-resistor-
> micro-ohms",
> + &prop);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Cannot read shunt resistor
> property\n");
> + if (prop == 0 || prop > INT_MAX)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid shunt resistor: %u\n",
> prop);
> + priv->rshunt = prop;
> +
> + if (!device_property_read_u32(dev, "microchip,dv-gain",
> &prop)) {
> + if (!pac1921_gain_param_valid(prop,
> PAC1921_GAIN_DV_GAIN_MAX))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid dv_gain: %u\n",
> prop);
> +
> + priv->dv_gain = (u8)ilog2(prop);
> + }
> + if (!device_property_read_u32(dev, "microchip,di-gain",
> &prop)) {
> + if (!pac1921_gain_param_valid(prop,
> PAC1921_GAIN_DI_GAIN_MAX))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid di_gain: %u\n",
> prop);
> +
> + priv->di_gain = (u8)ilog2(prop);
> + }
> + return 0;
> +}
> +
> +static int pac1921_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct pac1921_priv *priv;
> +
> + struct iio_dev *indio_dev = devm_iio_device_alloc(dev,
> sizeof(*priv));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + priv = iio_priv(indio_dev);
> + priv->client = client;
> + i2c_set_clientdata(client, indio_dev);
> +
> + priv->regmap = devm_regmap_init_i2c(client,
> &pac1921_regmap_config);
> + if (IS_ERR(priv->regmap))
> + dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> + "Cannot initialize register map\n");
> +
> + mutex_init(&priv->lock);
> +
> + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
> + priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
> + priv->high_res = PAC1921_DEFAULT_HIGH_RES;
> + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
> + priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED;
> +
> + int ret = pac1921_parse_properties(priv);
I would like to handle also the ACPI for x86 boards and not only the
device tree.
> + if (ret)
> + return ret;
> +
> + ret = pac1921_init(priv);
> + if (ret)
> + return ret;
> +
> + priv->iio_info = pac1921_iio;
> +
> + const struct i2c_device_id *id =
> i2c_client_get_device_id(client);
> +
> + indio_dev->name = id->name;
> + indio_dev->info = &priv->iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = pac1921_channels;
> + indio_dev->num_channels = ARRAY_SIZE(pac1921_channels);
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +
> &iio_pollfunc_store_time,
> +
> &pac1921_trigger_handler, NULL);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Cannot setup IIO triggered
> buffer\n");
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot register IIO
> device\n");
> +
> + return 0;
> +}
> +
