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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Oct 28, 2026 at 01:12:40PM -0500, Victor Chernyakin wrote:

...

> > +/**
> > + * 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

Not really. The only requirement is to keep a consistent style (either all with
periods as you pointed out, or all without).

...

> > +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:

This makes sense as the function is dedicated for overflow check.

> 	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);

...

> > +	*val = lower_32_bits(power);
> > +	*val2 = upper_32_bits(power);
> 
> iio_val_s64_decompose(power, val, val2)?

Good catch!

...

> > +		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()?

If they are separated, it should be carefully checked with datasheet.

...

> > +static unsigned int max34417_calc_input_correction(u32 rsense_microOhm)
> 
> Spell the return type as u32, to match the parameter

If it's going to be used as an input for regmap APIs unsigned it makes perfect
sense.

-- 
With Best Regards,
Andy Shevchenko