Re: [PATCH v1 2/2] iio: adc: adding support for PAC194X

From: Christophe JAILLET
Date: Fri Jul 19 2024 - 15:06:00 EST

Next message: David Sterba: "[GIT PULL] Btrfs build fix for 6.11"
Previous message: Jens Axboe: "Re: [PATCH V6] io_uring: releasing CPU resources when polling"
In reply to: marius.cristea: "[PATCH v1 2/2] iio: adc: adding support for PAC194X"
Next in thread: kernel test robot: "Re: [PATCH v1 2/2] iio: adc: adding support for PAC194X"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

...

Next message: David Sterba: "[GIT PULL] Btrfs build fix for 6.11"
Previous message: Jens Axboe: "Re: [PATCH V6] io_uring: releasing CPU resources when polling"
In reply to: marius.cristea: "[PATCH v1 2/2] iio: adc: adding support for PAC194X"
Next in thread: kernel test robot: "Re: [PATCH v1 2/2] iio: adc: adding support for PAC194X"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]