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

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

On Thu, Sep 24, 2026 at 09:54:57AM +0200, Joshua Crofts wrote:
> On Wed, 23 Sep 2026 21:10:23 +0200
> Neil Armstrong <neil.armstrong@linaro.org> wrote:

Joshua, below also something to you to pay attention to on top of the good
parts you covered already.

...

> > +/**
> > + * struct max34417_data - max34417 specific data.
> > + * @regmap:	device register map.
> > + * @dev:	max34417 device.
> > + * @lock:	lock for protecting access to device hardware registers, mostly
> 
> Nit-picking, but... Device, MAX34417, Lock.

Generally speaking it should be consistent with whatever style is being chosen.
If we go with the first capitalized letter, then yes, otherwise below should go
to small first letter. In any case MAX part number should be capitalized (or
someone might think of it as struct max34417).

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

...

> > +static int max34417_read_voltage(struct max34417_data *max34417,
> > +				 const struct iio_chan_spec *chan, int *val)
> > +{
> > +	uint16_t voltage;
> > +	uint8_t buf[3];

uXX types, please. Everywhere.

> > +	int rc;
> > +
> > +	guard(mutex)(&max34417->lock);
> > +
> > +	rc = max34417_accumulator_update(max34417);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = regmap_noinc_read(max34417->regmap, chan->address, &buf, 3);

sizeof()

> > +	if (rc)
> > +		return rc;
> > +
> > +	voltage = buf[2] | ((uint64_t)buf[1] << 8);
> > +	voltage >>= 2;

Not sure what is in the buf[0], but verbatim the above is get_unaligned_le16().

> > +	*val = voltage;
> > +
> > +	return IIO_VAL_INT;
> > +}

...

> > +	power = buf[7];
> > +	power |= ((uint64_t)buf[6] << 8UL);
> > +	power |= ((uint64_t)buf[5] << 16UL);
> > +	power |= ((uint64_t)buf[4] << 24UL);
> > +	power |= ((uint64_t)buf[3] << 32UL);
> > +	power |= ((uint64_t)buf[2] << 40UL);
> > +	power |= ((uint64_t)buf[1] << 48UL);

get_unaligned_be64() / be64_to_cpu().

...

> > +		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
> > +		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
> > +			rsense = MAX34417_DEFAULT_RSENSE;

What if the property is there, but some issue has happened?
We have an idiomatic

	if (_property_present()) {
		rc = _property_read();
		if (rc)
			return ...rc...;
		...
	} else {
		...apply default...
	}

-- 
With Best Regards,
Andy Shevchenko