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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

...





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux