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

[Neil Armstrong]

On 9/24/26 16:20, Andy Shevchenko wrote:
> 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@xxxxxxxxxx> 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().

Will switch to get_unaligned_xx()

> ...
>
> > > +		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...
> 	}

Will switch to that

Thanks,
Neil