Re: [PATCH v4 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Victor Chernyakin]

> +/**
> + * struct max34417_data - MAX34417 specific data.
> + * @indio_dev:  IIO Device.
> + * @regmap:	Device register map.
> + * @lock:	Lock for protecting access to device hardware registers, mostly
> + *		for reading common accumulator count and control register.
> + * @input_correction:	Correction based on the Rsense value from channel nodes.
> + * @input_label:	Channel label from channel nodes.
> + * @cam_mode:		State of the Continuous Accumulate Mode feature
> + * @acc_power:		Saved power accumulator for each channel when in CAM mode
> + * @acc_count:		Saved counter for each channel when in CAM mode
> + */

All the sentences should have periods at the end

> +static int max34417_check_ovf(struct max34417_data *max34417)
> +{
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (control & MAX34417_CONTROL_OVF) {
> +		rc = regmap_clear_bits(max34417->regmap, MAX34417_CONTROL_REG,
> +				       MAX34417_CONTROL_OVF);
> +		if (rc)
> +			return rc;
> +
> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}

Up to you, but could reduce indentation here:

	if (!(control & MAX34417_CONTROL_OVF))
		return 0;

	rc = regmap_clear_bits(max34417->regmap, MAX34417_CONTROL_REG,
			       MAX34417_CONTROL_OVF);
	if (rc)
		return rc;

	return regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);


> +static int max34417_read_power(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	u64 power;
> +	int rc;
> +
> +	/* In SMM mode, trigger a measurement cycle manually */
> +	if (!max34417->cam_mode) {
> +		rc = max34417_trigger_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/*
> +	 * Move the new UPDATE data into the SMBus read registers.
> +	 * In CAM mode this will also reset the accumulators, returning
> +	 * the power accumulated since the last UPDATE.
> +	 */
> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only get the accumulator counter in CAM mode, in SMM mode
> +	 * we only accumulate only once, so the counter is 1
> +	 */

"only" is duplicated

> +	if (max34417->cam_mode) {
> +		u64 acc_count;
> +		u64 acc_power;
> +
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +
> +		/* Get current channel power, counter and reset them */
> +		acc_power = max34417->acc_power[chan->channel];
> +		acc_count = max34417->acc_count[chan->channel];
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +
> +		if (acc_count == 0) {
> +			dev_warn_ratelimited(&max34417->indio_dev->dev,
> +					     "unexpected zero accumulator count\n");
> +
> +			*val = 0;
> +			*val2 = 0;
> +
> +			return IIO_VAL_INT_64;
> +		}
> +
> +		power = div64_u64(acc_power, acc_count);

Shorter and makes the branches more symmetrical:

	if (acc_count == 0) {
		dev_warn_ratelimited(&max34417->indio_dev->dev,
				     "unexpected zero accumulator count\n");

		power = 0;
	} else {
		power = div64_u64(acc_power, acc_count);
	}


> +	} else {
> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
> +		if (rc)
> +			return rc;
> +
> +		rc = max34417_reset_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	*val = lower_32_bits(power);
> +	*val2 = upper_32_bits(power);

iio_val_s64_decompose(power, val, val2)?

> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	u64 acc_power;
> +	int rc;
> +
> +	/* Move the data to the SMBus registers */
> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Retrieve all channels to avoid missing data for other channels */
> +	rc = max34417_save_accumulators(max34417);
> +	if (rc)
> +		return rc;
> +
> +	/* Get current channel power and reset them */
> +	acc_power = max34417->acc_power[chan->channel];
> +	max34417->acc_power[chan->channel] = 0;
> +	max34417->acc_count[chan->channel] = 0;
> +
> +	*val = lower_32_bits(acc_power);
> +	*val2 = upper_32_bits(acc_power);

ditto, iio_val_s64_decompose(acc_power, val, val2)?

> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
> +{
> +	int rc;
> +
> +	if (max34417->cam_mode && !val) {
> +		rc = regmap_clear_bits(max34417->regmap, MAX34417_CONTROL_REG,
> +				       MAX34417_CONTROL_CAM | MAX34417_CONTROL_SMM);
> +		if (rc)
> +			return rc;
> +
> +		/*
> +		 * If CAM mode must be set, the Control byte must be set to
> +		 * desired values with CAM = 0, then followed by an UPDATE
> +		 */
> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +	} else if (!max34417->cam_mode && val) {
> +		/*
> +		 * After this, the CAM mode bit can be set and another
> +		 * UPDATE must be sent.
> +		 */
> +		rc = regmap_clear_bits(max34417->regmap, MAX34417_CONTROL_REG,
> +				       MAX34417_CONTROL_SMM);
> +		if (rc)
> +			return rc;
> +		rc = regmap_set_bits(max34417->regmap, MAX34417_CONTROL_REG,
> +				     MAX34417_CONTROL_CAM);
> +		if (rc)
> +			return rc;
> +

Can the regmap_clear_bits() and regmap_set_bits() calls be
merged into one regmap_update_bits()?

> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +
> +		/* Reset counters and accumulators */
> +		for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {
> +			max34417->acc_power[i] = 0;
> +			max34417->acc_count[i] = 0;
> +		}
> +	}
> +
> +	max34417->cam_mode = val;
> +
> +	return 0;
> +}
> +
> +static int max34417_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max34417_data *max34417 = iio_priv(indio_dev);
> +
> +	guard(mutex)(&max34417->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return max34417_read_voltage(max34417, chan, val);
> +		case IIO_POWER:
> +			/* In default mode, we return a single measurement */
> +			if (max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_power(max34417, chan, val, val2);
> +		case IIO_ENERGY:
> +			if (!max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_energy(max34417, chan, val, val2);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		if (chan->type != IIO_POWER)
> +			return -EINVAL;
> +
> +		/* In CAM mode, we return an average measurement */
> +		if (!max34417->cam_mode)
> +			return -EINVAL;
> +
> +		return max34417_read_power(max34417, chan, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLTAGE) {
> +			/* Scale to mV */
> +			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
> +			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_POWER) {
> +			/*
> +			 * Scale to mW
> +			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 */
> +			*val = max34417->input_correction[chan->channel] * MILLIWATT_PER_WATT;
> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_ENERGY) {
> +			/*
> +			 * Scale to mJ
> +			 * P=(1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 * t=(Counter / Samples per Second)
> +			 * E=(P * t)
> +			 * Can be simplified to:
> +			 * (1000 * Power Accumulation * Correction) / (2^30 * SPS)
> +			 * The denominator doesn't fit in 32bit so scale down both terms.
> +			 */
> +			*val = max34417->input_correction[chan->channel];
> +			*val2 = div_u64(BIT_ULL(MAX34417_PWR_AVG_FULL_SCALE_BITS) *
> +					MAX34417_FAST_SPS_CAM, MILLI);
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +		return -EINVAL;

Maybe make this a switch instead of an if-chain?

> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = max34417->cam_mode;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int max34417_read_label(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       char *label)
> +{
> +	struct max34417_data *max34417 = iio_priv(indio_dev);
> +	const char *input_label = max34417->input_label[chan->channel];
> +
> +	if (input_label) {
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return sysfs_emit(label, "%s-voltage\n", input_label);
> +		case IIO_POWER:
> +			return sysfs_emit(label, "%s-power\n", input_label);
> +		case IIO_ENERGY:
> +			return sysfs_emit(label, "%s-energy\n", input_label);
> +		default:
> +			return 0;
> +		}
> +	}
> +
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
> +	case IIO_POWER:
> +		return sysfs_emit(label, "channel%d-power\n", chan->channel);
> +	case IIO_ENERGY:
> +		return sysfs_emit(label, "channel%d-energy\n", chan->channel);
> +	default:
> +		return 0;
> +	}
> +}

Up to you if you prefer it, but an alternative to duplicating this switch
is to add the following near the start of max34417_probe():

max34417->input_label[0] = "channel0";
max34417->input_label[1] = "channel1";
max34417->input_label[2] = "channel2";
max34417->input_label[3] = "channel3";

> +
> +static const struct iio_info max34417_info = {
> +	.read_raw	= max34417_read_raw,
> +	.write_raw	= max34417_write_raw,
> +	.read_label	= max34417_read_label,
> +};
> +
> +static unsigned int max34417_calc_input_correction(u32 rsense_microOhm)

Spell the return type as u32, to match the parameter