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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Software state can desynchronize from hardware configuration due to TOCTOU race during concurrent sysfs writes.
- [Low] The i2c_device_id array is missing required named initializers.
--

--- Patch [2]: [PATCH v5 2/2] iio: adc: add MAX40080 current-sense amplifier driver ---
commit 0c56883df0b122ded44daf6d01973fa32a6c55e4
Author: Stefan Popa <[email protected]>

iio: adc: add MAX40080 current-sense amplifier driver
    
This commit adds a direct-mode IIO driver for the MAX40080 bidirectional
current-sense amplifier. It exposes current and voltage channels over an
I2C/SMBus interface.

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

[ ... ]

> +static int max40080_set_range(struct max40080_state *st, unsigned int range)
> +{
> +	int ret;
> +
> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_RANGE_MSK,
> +				   FIELD_PREP(MAX40080_RANGE_MSK, range));
> +	if (ret)
> +		return ret;
> +
> +	st->range = range;
> +
> +	return 0;
> +}

[Severity: Medium]
Since max40080_update_bits() acquires and releases the mutex internally, 
does this leave the assignment of st->range unprotected against concurrent
sysfs writes? 

If two threads write to the sysfs attribute at the same time, could the 
cached software state become permanently desynchronized from the actual 
hardware configuration, causing future readings to be scaled incorrectly?

[ ... ]

> +static int max40080_set_oversampling_ratio(struct max40080_state *st, int val)
> +{
> +	int ret, filter = max40080_oversampling_to_filter(val);
> +
> +	if (filter < 0)
> +		return filter;
> +
> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_FILTER_MSK,
> +				   FIELD_PREP(MAX40080_FILTER_MSK, filter));
> +	if (ret)
> +		return ret;
> +
> +	st->oversampling_ratio = val;
> +
> +	return 0;
> +}

[Severity: Medium]
Similarly to max40080_set_range(), is the update of st->oversampling_ratio
vulnerable to a race condition since it happens after max40080_update_bits()
has released the lock?

[ ... ]

> +static const struct i2c_device_id max40080_i2c_ids[] = {
> +	{ "max40080" },
> +	{ }
> +};

[Severity: Low]
Does this violate the I2C subsystem coding style guidelines which mandate
named initializers for initialized arrays of type struct i2c_device_id? 

Should this be .name = "max40080" to prevent issues if the struct definition 
changes in the future?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2