Re: [PATCH v9 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Jonathan Cameron]

> The MAX40080 is a bidirectional current-sense amplifier with an
> integrated 12-bit ADC and an I2C/SMBus interface. It measures the

[Severity: Medium]
The commit message claims to expose a hardware-gain attribute, but is this
actually implemented in the code?

Looking at max40080_channels, the promised hardware-gain attribute appears to
be structurally omitted in favor of standard scale representations:

-

This one seems to be valid... If nothing else comes up I'll tidy
that up whilst applying.


> voltage across an external shunt resistor and the input bus voltage,
> storing the results in an internal FIFO.
> 
> Add a direct-mode IIO driver exposing the current and voltage channels
> with raw, scale and hardware-gain attributes, a configurable
> oversampling (digital averaging) ratio, and PEC-protected register
> access. The current scale is derived from the shunt resistor value
> described in the device tree.
> 
> The driver operates in single-measurement mode: each raw read triggers
> an on-demand conversion via SMBus Quick Command and returns a matched
> current/voltage pair. This avoids the latency and complexity of the
> continuous FIFO mode while ensuring each read reflects the current
> state. The two selectable current-sense ranges are exposed through
> scale/scale_available.
> 
> Continuous FIFO buffering, threshold events and the alert interrupt are
> intentionally left out of this initial submission and may be added
> later.
> 
> Co-developed-by: Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>
> Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Reviewed-by: Siratul Islam <siratul.islam@xxxxxxxxx>

One other trivial thing inline. I'll tidy up whilst applying

> diff --git a/drivers/iio/adc/max40080.c b/drivers/iio/adc/max40080.c
> new file mode 100644
> index 000000000000..6864ce1cb063
> --- /dev/null
> +++ b/drivers/iio/adc/max40080.c

> +static int max40080_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int range;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->type == IIO_CURRENT) {
> +			ret = max40080_get_current(st, val);
> +			if (ret)
> +				return ret;
> +		} else if (chan->type == IIO_VOLTAGE) {
> +			ret = max40080_get_voltage(st, val);
> +			if (ret)
> +				return ret;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_CURRENT) {
> +			/*
> +			 * The selectable current-sense range is exposed through
> +			 * scale: each RANGE setting has its own precomputed
> +			 * mA-per-code value. Userspace picks the range by writing
> +			 * the matching scale.
> +			 *
> +			 * Use READ_ONCE to ensure the compiler reads st->range
> +			 * exactly once, so val and val2 come from the same setting
> +			 * even if a concurrent write changes st->range.
> +			 */

Wrapping is a bit odd. It is to just over 80 chars (83 or so).  No
readability gain, so 80 makes more sense.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>