Re: [PATCH v2 2/2] iio: light: add AS7343 multi-spectral sensor driver

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Device configuration is not restored on system resume, breaking the sensor if power is lost.
- [Medium] The PM operations structure pointer is incorrectly wrapped with pm_sleep_ptr() instead of pm_ptr().
- [Medium] Missing PM runtime get/put in as7343_read_raw() allows reads while the device is runtime-suspended.
- [Medium] The variable 'val' is evaluated uninitialized when regmap_read() fails.
- [Medium] The device is left powered on during driver unbind and error paths.
- [Low] Incorrect base types used for endianness conversions cause Sparse warnings.
- [Low] Concurrent sysfs reads cause a race condition in as7343_read_raw() when latching and reading sensor data.
--

commit b11536303f5757f091de7d446bfca2f105210812
Author: Chang Yu [off-list ref]

iio: light: add AS7343 multi-spectral sensor driver

This patch adds a driver for the AMS AS7343 14-channel multi-spectral
sensor with I2C interface. It exposes 12 spectral channels via IIO sysfs
and adds basic power management support.

[ ... ]

+static int as7343_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	struct as7343_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int unused;
+	u16 result;

[Severity: Low]
Do these assignments cause static analysis warnings with Sparse? 'result'
is declared as u16 but assigned a __le16 value via regmap_bulk_read() and
passed to le16_to_cpu() below.

+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		/*
+		 * Reading ASTATUS latches all data registers to this read.
+		 * We don't care about the returned saturation/gain status for
+		 * now.
+		 */
+		ret = regmap_read(data->regmap, AS7343_ASTATUS, &unused);

[Severity: Medium]
Can this execute I2C reads against a suspended sensor?

The driver enables runtime PM which allows the device to be suspended
(clearing the measurement enable bit). Reading raw data without waking the
device using pm_runtime_resume_and_get() first could return stale or zero
data.

[Severity: Low]
Can concurrent sysfs reads from different channels cause a race condition
here?

The ASTATUS read latches all channels before the channel data is read.
Without a lock, could interleaved reads result in one thread receiving data
latched by another thread's ASTATUS read?

+		if (ret)
+			return ret;
+
+		ret = regmap_bulk_read(data->regmap, chan->address, &result, 2);
+		if (ret)
+			return ret;
+
+		*val = le16_to_cpu(result);
+		return IIO_VAL_INT;
+	}

[ ... ]

+static int as7343_setup_device(struct device *dev, struct as7343_data *data)
+{
+	unsigned int val;
+	u16 step;

[Severity: Low]
Does this cause a strict typing warning with Sparse? 'step' is declared as
u16 but receives a __le16 value via cpu_to_le16() below.

+	int ret;
+
+	/* Power on */
+	ret = regmap_set_bits(data->regmap, AS7343_ENABLE, AS7343_ENABLE_PON);
+	if (ret)
+		return ret;
+
+	/* Need to set REG_BANK to 1 before we can access ID */
+	ret = regmap_set_bits(data->regmap, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, AS7343_ID, &val);
+	if (val != 0x81)
+		dev_info(dev, "Unknown device ID: %x\n", val);

[Severity: Medium]
If regmap_read() fails, it returns an error code and leaves the stack
variable 'val' uninitialized. Could we be evaluating and printing
uninitialized memory here since the error is ignored?

+
+	ret = regmap_clear_bits(data->regmap, AS7343_CFG0,
+				AS7343_CFG0_REG_BANK);

[ ... ]

+static int as7343_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct as7343_data *data = iio_priv(indio_dev);
+
+	return regmap_clear_bits(data->regmap, AS7343_ENABLE,
+				 AS7343_ENABLE_SP_EN);

[Severity: Medium]
Does this leave the device powered on indefinitely?

as7343_setup_device() sets the PON bit during probe, but this suspend
action only clears SP_EN, meaning PON remains set when the device is
unbound or suspended.

+}
+
+static int as7343_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct as7343_data *data = iio_priv(indio_dev);
+
+	return regmap_set_bits(data->regmap, AS7343_ENABLE,
+			       AS7343_ENABLE_SP_EN);

[Severity: High]
Does this leave the sensor in a broken state after resuming from S3 sleep?

The driver uses REGCACHE_NONE, so all volatile configurations (gain,
integration time, multiplexer) are lost when power to the sensor is lost.
This resume function only sets the SP_EN bit without reinitializing the
sensor's configuration.

+}

[ ... ]

+static int as7343_probe(struct i2c_client *client)
+{

[ ... ]

+	ret = as7343_setup_device(dev, data);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);

[Severity: Medium]
Does this leave the device powered on during error paths?

as7343_setup_device() powers on the device, but if a subsequent I2C
operation in that function fails, this cleanup action is never registered,
leaving the hardware powered on.

+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add suspend action\n");

[ ... ]

+static struct i2c_driver as7343_driver = {
+	.driver = {
+		.name           = "as7343",
+		.of_match_table = as7343_of_match,
+		.pm             = pm_sleep_ptr(&as7343_pm_ops),

[Severity: Medium]
Does this inadvertently drop runtime PM support when CONFIG_PM_SLEEP is
disabled?

The dev_pm_ops structure defines runtime PM callbacks, but using
pm_sleep_ptr() evaluates to NULL when CONFIG_PM_SLEEP=n, even if
CONFIG_PM=y is set. Should this be wrapped with pm_ptr() instead?

+	},
+	.probe      = as7343_probe,
+	.id_table   = as7343_id,
+};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907210042.32552-1-marcus.yu.56@gmail.com?part=2