Marius.Cristea@ wrote: > > > 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 Hi Marius, Thanks for your feedback, this is indeed very helpful, as it would be if you shared your code. I addressed some of your comments below in patch v2 thread, replying to Jonathan [1]. If you have more comments about those points please reply on that thread. [1]: https://lore.kernel.org/linux-iio/668bec2a8b23a_6e037017@njaxe.notmuch See also my comments below. > 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. > I think performances depends on the use case, for example if the user is interested in only one type of measurement I don't think reading all registers result in a performance gain. Also consider that there are 8 registers of accumulators that are not currently exposed by the driver but that would be read anyway, so I am not sure it would result in a performance gain either. I would leave it as it is right now to keep the implementation simple and maybe extend it in the future in case performance gains become obvious and necessary for most use cases. Also consider that regmap supports bulk reads. > > 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. > I think that there is no guarantee that the user would always request two successive raw readings, like Voltage and Current, within the same integration period. So for the second reading all registers may be read again and the two values might not be correlated as well. Also consider that the measurements are kept in the device registers until a new integration is complete, so if two registers are read consecutively within the same integration period they would be correlated. > > +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 > I would not mind removing the VSENSE channel, but there are other drivers exposing it (some of them calls it vshunt or shunt_voltage, e.g. ina2xx). Are we sure no one is interested in it? > > + 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. > All right, I would add this as an iio_chan_spec_ext_info. I mentioned this in patch v2 thread [1], please reply there if you have more comments. > > + 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. > Thanks for catching this, I will fix it in next patch version. > > + case PAC1921_CHAN_POWER: { > > > > + /* Power scale factor in mW: > > I think the scale here should be in microWatts. We have (mA)*(mV) > Documentation/ABI/testing/sysfs-bus-iio states it should be in milliwatts: What: /sys/bus/iio/devices/iio:deviceX/in_powerY_raw KernelVersion: 4.5 Contact: linux-iio@xxxxxxxxxxxxxxx Description: Raw (unscaled no bias removal etc.) power measurement from channel Y. The number must always be specified and unique to allow association with event codes. Units after application of scale and offset are milliwatts. > > + * 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. > I will include gains into the scales as suggested. I addressed this in patch v2 thread [1]. Please reply there if you have additional comments. > > > > + > > +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. > The device label from DT is already exposed into its specific sysfs attribute and so available to the users. I think using it as a prefix for the channel would just be a duplication. The pac1934 driver in fact prepends the channel label here not the device label. I think the same does not apply for the pac1921 being a single channel monitor. > > + 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 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. > I would remove filters control entirely based on Jonathan comments in patch v2 thread [1]. Please reply in that thread if you have more comments. > > > > +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. > I would leave this for future extensions. I addressed this in patch v2 thread [1]. Please reply there if you have more comments. > > > > + > > +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. > I would leave this for future extensions since I cannot test it against ACPI. Thanks again for your feedback. Best regards, Matteo
