Le 19/07/2024 à 19:38,
marius.cristea-UWL1GkI3JZL3oGB3hsPCZA@xxxxxxxxxxxxxxxx a écrit :
From: Marius Cristea <marius.cristea-UWL1GkI3JZL3oGB3hsPCZA@xxxxxxxxxxxxxxxx>
This is the iio driver for Microchip PAC194X and PAC195X
series of Power Monitors with Accumulator.
Signed-off-by: Marius Cristea <marius.cristea-UWL1GkI3JZL3oGB3hsPCZA@xxxxxxxxxxxxxxxx>
---
.../ABI/testing/sysfs-bus-iio-adc-pac1944 | 9 +
MAINTAINERS | 7 +
drivers/iio/adc/Kconfig | 13 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/pac1944.c | 3528 +++++++++++++++++
5 files changed, 3558 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
create mode 100644 drivers/iio/adc/pac1944.c
Hi,
below a few comments and nitpicks.
- You could slightly improve style by removing extra spaces in
several places where you have <SPACE><SPACE>= or =<SPACE><SPACE>.
- pac1944_oc_limit_nsamples_store(),
pac1944_uc_limit_nsamples_store(),
pac1944_op_limit_nsamples_store(),
pac1944_ov_limit_nsamples_store(),
pac1944_uv_limit_nsamples_store()
look really similar and could be defined with a macro with 3
parameters to save some LoC
- same for pac1944_slow_alert_store() and
pac1944_gpio_alert_store(), if it makes sense for just 2 functions
- the same could be done for some show functions
- (unlikely(check_add_overflow()): unlikely is not needed, I think
that it is already handled that way (see __must_check_overflow())
...
+static int pac1944_send_refresh(struct pac1944_chip_info *info,
+ u8 refresh_cmd, u32 wait_time)
+{
+ struct i2c_client *client = info->client;
+ int ret;
+
+ /* Writing a REFRESH or a REFRESH_V command */
+ ret = i2c_smbus_write_byte(client, refresh_cmd);
+ if (ret) {
+ dev_err(&client->dev, "%s - cannot send Refresh cmd (0x%02X) to PAC1944\n",
+ __func__, refresh_cmd);
+ return ret;
+ }
+
+ /* Register data retrieval timestamp */
+ info->chip_reg_data.jiffies_tstamp = jiffies;
+ /* Wait till the data is available */
+ usleep_range(wait_time, wait_time + 100);
+
+ return ret;
Nitpick : return 0;
+}
...
+static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time)
+{
+ int ret;
Missing newline
+ /*
+ * Check if the minimal elapsed time has passed and if so,
+ * re-read the chip, otherwise the cached info is just fine
+ */
+ if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
+ msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) {
+ /*
+ * We need to re-read the chip values
+ * call the pac1944_reg_snapshot
+ */
+ ret = pac1944_reg_snapshot(info, true,
+ PAC1944_REFRESH_REG_ADDR,
+ wait_time);
+ /*
+ * Re-schedule the work for the read registers timeout
+ * (to prevent chip regs saturation)
+ */
+ cancel_delayed_work_sync(&info->work_chip_rfsh);
+ schedule_delayed_work(&info->work_chip_rfsh,
+ msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS));
+ }
+
+ return ret;
+}
...
+static ssize_t pac1944_in_voltage_acc_raw_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct pac1944_chip_info *info = iio_priv(indio_dev);
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+ int ret;
+ s64 acc_voltage;
+ u32 samples_count;
+ u64 tmp_u64;
+
+ ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US);
+ if (ret < 0)
+ return 0;
+
+ acc_voltage = info->chip_reg_data.acc_val[this_attr->address];
+ samples_count = info->chip_reg_data.total_samples_nr[this_attr->address];
+
+ tmp_u64 = div_u64(abs(acc_voltage), samples_count);
+
+ if (unlikely(is_negative(acc_voltage)))
unlikely() does not seem really needed here. It does not look like a hot
path.
+ return sysfs_emit(buf, "-%lld\n", tmp_u64);
+ else
+ return sysfs_emit(buf, "%lld\n", tmp_u64);
+}
...
+static int pac1944_read_thresh(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)
+{
+ struct pac1944_chip_info *chip_info = iio_priv(indio_dev);
+ int idx;
+
+ /* into the datasheet channels are noted from 1 to 4 */
+ idx = chan->channel - 1;
+
+ switch (chan->type) {
Should it be protected by chip_info->lock, as done in
pac1944_read_event_config()?
+ case IIO_VOLTAGE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ *val = chip_info->overvoltage[idx];
+ return IIO_VAL_INT;
+ case IIO_EV_DIR_FALLING:
+ *val = chip_info->undervoltage[idx];
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CURRENT:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ *val = chip_info->overcurrent[idx];
+ return IIO_VAL_INT;
+ case IIO_EV_DIR_FALLING:
+ *val = chip_info->undercurrent[idx];
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_POWER:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ *val = chip_info->overpower[idx];
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int pac1944_write_thresh(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)
+{
+ struct pac1944_chip_info *chip_info = iio_priv(indio_dev);
+ int idx, ret;
+
+ /* into the datasheet channels are noted from 1 to 4 */
+ idx = chan->channel - 1;
+
Why are scoped_guard() in each cases?
Could'nt it be done as ni pac1944_read_event_config()?
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ scoped_guard(mutex, &chip_info->lock) {
+ ret = pac1944_update_alert_16b(&indio_dev->dev,
+ PAC1944_OV_LIMIT_REG_ADDR + idx,
+ (int)(PAC1944_CH01UV_MASK >> idx),
+ val);
+ if (!ret)
+ chip_info->overvoltage[idx] = val;
+ }
+ return ret;
...
+static int pac1944_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 pac1944_chip_info *info = iio_priv(indio_dev);
+ struct i2c_client *client = info->client;
+ int idx, val, mask, ret;
+ bool update = false;
+ u8 tmp[PAC1944_ALERT_ENABLE_REG_LEN];
+
+ /* into the datasheet channels are noted from 1 to 4 */
+ idx = chan->channel - 1;
+
+ guard(mutex)(&info->lock);
This could maybe be moved after the switch(), if this makes sense?
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
...
+static int pac1944_prep_iio_channels(struct pac1944_chip_info *info,
+ struct iio_dev *indio_dev)
+{
+ struct device *dev = &info->client->dev;
+ struct iio_chan_spec *ch_sp;
+ int channel_size, attribute_count;
+ int cnt;
+ void *dyn_ch_struct, *tmp_data;
+
+ /* Finding out dynamically how many IIO channels we need */
+ attribute_count = 0;
+ channel_size = 0;
+ for (cnt = 0; cnt < info->phys_channels; cnt++) {
+ if (!info->active_channels[cnt])
+ continue;
+
+ /* add the size of the properties of one chip physical channel */
+ channel_size += sizeof(pac1944_single_channel);
+ /* count how many enabled channels we have */
+ attribute_count += ARRAY_SIZE(pac1944_single_channel);
+ dev_info(dev, ":%s: Channel %d active\n", __func__, cnt + 1);
+ }
+
+ dyn_ch_struct = devm_kzalloc(dev, channel_size, GFP_KERNEL);
+ if (!dyn_ch_struct)
+ return -EINVAL;
-ENOMEM?
+
+ tmp_data = dyn_ch_struct;
+ /* Populate the dynamic channels and make all the adjustments */
+ for (cnt = 0; cnt < info->phys_channels; cnt++) {
+ if (!info->active_channels[cnt])
+ continue;
...
+static int pac1944_probe(struct i2c_client *client)
+{
+ struct pac1944_chip_info *info;
+ struct iio_dev *indio_dev;
+ const struct pac1944_features *chip;
+ bool match = false;
+ int cnt, ret;
+ struct device *dev = &client->dev;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ info = iio_priv(indio_dev);
+ info->client = client;
+
+ ret = pac1944_chip_identify(info);
+ if (ret < 0) {
+ /*
+ * If failed to identify the hardware based on internal
+ * registers,try using fallback compatible in device tree to
Nitpick: space missing after the comma.
+ * deal with some newer part number.
+ */
+ chip = i2c_get_match_data(client);
+ if (!chip)
+ return -EINVAL;
+
+ info->chip_variant = chip->prod_id;
+ info->phys_channels = chip->phys_channels;
+ indio_dev->name = chip->name;
+ } else {
...
+ ret = pac1944_reg_snapshot(info, true, false,
+ PAC1944_MIN_UPDATE_WAIT_TIME_US);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_iio_device_register(&client->dev, indio_dev);
+
Nitpick: needeless new line
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Can't register IIO device\n");
+
+ return 0;
+}
CJ
...
