[PATCH v3 0/7] iio: light: opt3001: driver cleanup

[PATCH v3 0/7] iio: light: opt3001: driver cleanup

[Joshua Crofts via B4 Relay]

This series deals with cleaning up the TI OPT3001 sensor driver,
moving it to more modern kernel practices and improving the code style.

While reviewing, Jonathan Cameron (and eventually Sashiko) found a race
condition where userspace could start interacting with the device
before the hardware IRQ was set up.

Changes include:
- moving the driver to use devm_* functions
- IWYU cleanups
- removal of unnecessary macros and comments
- using dev_err_probe() in probe and probe path functions
- checkpatch.pl warning cleanups
- fixing a race condition found in opt3001_probe() function

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
Changes in v3:
- PATCH 1: fix build error
- PATCH 2: remove struct device member from struct opt3001
- PATCH 6: edit return statements
- Pull additional trailers from previous version's reviews
- Edit commit messages
- Link to v2: https://lore.kernel.org/r/20260512-opt3001-cleanup-v2-0-8018cf3a8a0a@gmail.com

Changes in v2:
- PATCH 1: added patch that fixes race condition
- PATCH 3: remove wrong usage of GENMASK()
- PATCH 4: add patch that moves driver to use local structs
- PATCH 5: add patch that ensures correct parenthesis alignment
- PATCH 6: change int to unsigned int
- PATCH 7: moved opt3001_read_id() function to use dev_err_probe()
- PATCH 9: removed unnecessary dev_err_probe() calls, reordering
- PATCH 10: added patch that adds a comment to mutex declaration
- Link to v1: https://lore.kernel.org/r/20260511-opt3001-cleanup-v1-0-f7879dc3455c@gmail.com

---
Joshua Crofts (7):
      iio: light: opt3001: move device registration to end of probe()
      iio: light: opt3001: use local struct device and i2c_client variables
      iio: light: opt3001: ensure correct parenthesis alignment
      iio: light: opt3001: localize for loop iterator
      iio: light: opt3001: prefer dev_err_probe()
      iio: light: opt3001: move driver to guard(mutex)() use
      iio: light: opt3001: add comment to mutex

 drivers/iio/light/opt3001.c | 378 +++++++++++++++++++++-----------------------
 1 file changed, 179 insertions(+), 199 deletions(-)
---
base-commit: 8678fb54958893818ddeccd05fea560a4e1fc759
change-id: 20260511-opt3001-cleanup-c1411de14392

Best regards,
-- 
Joshua Crofts <joshua.crofts1@gmail.com>

[PATCH v3 1/7] iio: light: opt3001: move device registration to end of probe()

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Move IIO device registration to the end of the probe() function to
prevent a potential race where userspace can interact with the device
before IRQ is configured properly. Additionally, switch
devm_iio_device_register() to its unmanaged counterpart as current
driver implementation mixes managed and unmanaged resources, causing
potential resource leaks.

Also, add iio_device_unregister() to remove() function to correctly
handle teardown.

Fixes: ac663db3678a ("iio: light: opt3001: enable operation w/o IRQ")
Reported-by: Jonathan Cameron <jic23@kernel.org>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260511-opt3001-cleanup-v1-0-f7879dc3455c%40gmail.com?part=7
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index dac3d3818d0a01b4ca53106fa38bb54b8c5373d4..fe53a072b21ddaa26a23c76e82897fdbc1e4d846 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -874,12 +874,6 @@ static int opt3001_probe(struct i2c_client *client)
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &opt3001_info;
 
-	ret = devm_iio_device_register(dev, iio);
-	if (ret) {
-		dev_err(dev, "failed to register IIO device\n");
-		return ret;
-	}
-
 	/* Make use of INT pin only if valid IRQ no. is given */
 	if (irq > 0) {
 		ret = request_threaded_irq(irq, NULL, opt3001_irq,
@@ -894,7 +888,7 @@ static int opt3001_probe(struct i2c_client *client)
 		dev_dbg(opt->dev, "enabling interrupt-less operation\n");
 	}
 
-	return 0;
+	return iio_device_register(iio);
 }
 
 static void opt3001_remove(struct i2c_client *client)
@@ -904,6 +898,8 @@ static void opt3001_remove(struct i2c_client *client)
 	int ret;
 	u16 reg;
 
+	iio_device_unregister(iio);
+
 	if (opt->use_irq)
 		free_irq(client->irq, iio);
 

-- 
2.47.3

[PATCH v3 2/7] iio: light: opt3001: use local struct device and i2c_client variables

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Switch the driver to use local variables for struct device and struct
i2c_client and remove struct device member from struct opt3001, as the
former can be derived from struct client.

While at it, ensure that parentheses alignment is correct in functions
that were changed in this patch.

No functional change.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 149 +++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 71 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index fe53a072b21ddaa26a23c76e82897fdbc1e4d846..5d88a697c245347bcb98254460308fc9a9f375f4 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -101,7 +101,6 @@ struct opt3001_chip_info {
 
 struct opt3001 {
 	struct i2c_client	*client;
-	struct device		*dev;
 
 	struct mutex		lock;
 	bool			ok_to_ignore_lock;
@@ -313,6 +312,8 @@ static const struct iio_chan_spec opt3002_channels[] = {
 
 static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 {
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	u16 mantissa;
 	u16 reg;
@@ -326,12 +327,12 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 		 * doing so will overwrite the low-level limit value however we
 		 * will restore this value later on.
 		 */
-		ret = i2c_smbus_write_word_swapped(opt->client,
-					OPT3001_LOW_LIMIT,
-					OPT3001_LOW_LIMIT_EOC_ENABLE);
+		ret = i2c_smbus_write_word_swapped(client,
+						   OPT3001_LOW_LIMIT,
+						   OPT3001_LOW_LIMIT_EOC_ENABLE);
 		if (ret < 0) {
-			dev_err(opt->dev, "failed to write register %02x\n",
-					OPT3001_LOW_LIMIT);
+			dev_err(dev, "failed to write register %02x\n",
+				OPT3001_LOW_LIMIT);
 			return ret;
 		}
 
@@ -343,21 +344,20 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 	opt->result_ready = false;
 
 	/* Configure for single-conversion mode and start a new conversion */
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		goto err;
 	}
 
 	reg = ret;
 	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SINGLE);
 
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
-			reg);
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
 		goto err;
 	}
 
@@ -375,10 +375,9 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 		msleep(timeout);
 
 		/* Check result ready flag */
-		ret = i2c_smbus_read_word_swapped(opt->client,
-						  OPT3001_CONFIGURATION);
+		ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 		if (ret < 0) {
-			dev_err(opt->dev, "failed to read register %02x\n",
+			dev_err(dev, "failed to read register %02x\n",
 				OPT3001_CONFIGURATION);
 			goto err;
 		}
@@ -389,9 +388,9 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 		}
 
 		/* Obtain value */
-		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
+		ret = i2c_smbus_read_word_swapped(client, OPT3001_RESULT);
 		if (ret < 0) {
-			dev_err(opt->dev, "failed to read register %02x\n",
+			dev_err(dev, "failed to read register %02x\n",
 				OPT3001_RESULT);
 			goto err;
 		}
@@ -416,12 +415,12 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 		 * bit-overlap and therefore can't be done.
 		 */
 		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
-		ret = i2c_smbus_write_word_swapped(opt->client,
+		ret = i2c_smbus_write_word_swapped(client,
 						   OPT3001_LOW_LIMIT,
 						   value);
 		if (ret < 0) {
-			dev_err(opt->dev, "failed to write register %02x\n",
-					OPT3001_LOW_LIMIT);
+			dev_err(dev, "failed to write register %02x\n",
+				OPT3001_LOW_LIMIT);
 			return ret;
 		}
 	}
@@ -444,13 +443,15 @@ static int opt3001_get_int_time(struct opt3001 *opt, int *val, int *val2)
 
 static int opt3001_set_int_time(struct opt3001 *opt, int time)
 {
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	u16 reg;
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		return ret;
 	}
 
@@ -469,8 +470,7 @@ static int opt3001_set_int_time(struct opt3001 *opt, int time)
 		return -EINVAL;
 	}
 
-	return i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
-			reg);
+	return i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
 }
 
 static int opt3001_read_raw(struct iio_dev *iio,
@@ -565,6 +565,8 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 		int val, int val2)
 {
 	struct opt3001 *opt = iio_priv(iio);
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	int whole;
 	int integer;
@@ -583,7 +585,7 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 
 	ret = opt3001_find_scale(opt, val, val2, &exponent);
 	if (ret < 0) {
-		dev_err(opt->dev, "can't find scale for %d.%06u\n", val, val2);
+		dev_err(dev, "can't find scale for %d.%06u\n", val, val2);
 		goto err;
 	}
 
@@ -611,9 +613,9 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 		goto err;
 	}
 
-	ret = i2c_smbus_write_word_swapped(opt->client, reg, value);
+	ret = i2c_smbus_write_word_swapped(client, reg, value);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n", reg);
+		dev_err(dev, "failed to write register %02x\n", reg);
 		goto err;
 	}
 
@@ -637,6 +639,8 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 		enum iio_event_direction dir, bool state)
 {
 	struct opt3001 *opt = iio_priv(iio);
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	u16 mode;
 	u16 reg;
@@ -652,21 +656,20 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
 		: OPT3001_CONFIGURATION_M_SHUTDOWN;
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		goto err;
 	}
 
 	reg = ret;
 	opt3001_set_mode(opt, &reg, mode);
 
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
-			reg);
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
 		goto err;
 	}
 
@@ -688,44 +691,48 @@ static const struct iio_info opt3001_info = {
 
 static int opt3001_read_id(struct opt3001 *opt)
 {
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	char manufacturer[2];
 	u16 device_id;
 	int ret;
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_MANUFACTURER_ID);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_MANUFACTURER_ID);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_MANUFACTURER_ID);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_MANUFACTURER_ID);
 		return ret;
 	}
 
 	manufacturer[0] = ret >> 8;
 	manufacturer[1] = ret & 0xff;
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_DEVICE_ID);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
+		dev_err(dev, "failed to read register %02x\n",
 			OPT3001_DEVICE_ID);
 		return ret;
 	}
 
 	device_id = ret;
 
-	dev_info(opt->dev, "Found %c%c OPT%04x\n", manufacturer[0],
-			manufacturer[1], device_id);
+	dev_info(dev, "Found %c%c OPT%04x\n", manufacturer[0], manufacturer[1],
+		 device_id);
 
 	return 0;
 }
 
 static int opt3001_configure(struct opt3001 *opt)
 {
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	u16 reg;
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		return ret;
 	}
 
@@ -750,28 +757,27 @@ static int opt3001_configure(struct opt3001 *opt)
 	reg &= ~OPT3001_CONFIGURATION_ME;
 	reg &= ~OPT3001_CONFIGURATION_FC_MASK;
 
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
-			reg);
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
 		return ret;
 	}
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_LOW_LIMIT);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_LOW_LIMIT);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_LOW_LIMIT);
 		return ret;
 	}
 
 	opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
 	opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret);
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_HIGH_LIMIT);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_HIGH_LIMIT);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_HIGH_LIMIT);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_HIGH_LIMIT);
 		return ret;
 	}
 
@@ -785,6 +791,8 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
 {
 	struct iio_dev *iio = _iio;
 	struct opt3001 *opt = iio_priv(iio);
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
 	int ret;
 	bool wake_result_ready_queue = false;
 	enum iio_chan_type chan_type = opt->chip_info->chan_type;
@@ -793,10 +801,10 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
 	if (!ok_to_ignore_lock)
 		mutex_lock(&opt->lock);
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		goto out;
 	}
 
@@ -815,10 +823,10 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
 							IIO_EV_DIR_FALLING),
 					iio_get_time_ns(iio));
 	} else if (ret & OPT3001_CONFIGURATION_CRF) {
-		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
+		ret = i2c_smbus_read_word_swapped(client, OPT3001_RESULT);
 		if (ret < 0) {
-			dev_err(opt->dev, "failed to read register %02x\n",
-					OPT3001_RESULT);
+			dev_err(dev, "failed to read register %02x\n",
+				OPT3001_RESULT);
 			goto out;
 		}
 		opt->result = ret;
@@ -851,7 +859,6 @@ static int opt3001_probe(struct i2c_client *client)
 
 	opt = iio_priv(iio);
 	opt->client = client;
-	opt->dev = dev;
 	opt->chip_info = i2c_get_match_data(client);
 
 	mutex_init(&opt->lock);
@@ -885,7 +892,7 @@ static int opt3001_probe(struct i2c_client *client)
 		}
 		opt->use_irq = true;
 	} else {
-		dev_dbg(opt->dev, "enabling interrupt-less operation\n");
+		dev_dbg(dev, "enabling interrupt-less operation\n");
 	}
 
 	return iio_device_register(iio);
@@ -895,6 +902,7 @@ static void opt3001_remove(struct i2c_client *client)
 {
 	struct iio_dev *iio = i2c_get_clientdata(client);
 	struct opt3001 *opt = iio_priv(iio);
+	struct device *dev = &client->dev;
 	int ret;
 	u16 reg;
 
@@ -903,21 +911,20 @@ static void opt3001_remove(struct i2c_client *client)
 	if (opt->use_irq)
 		free_irq(client->irq, iio);
 
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
 		return;
 	}
 
 	reg = ret;
 	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
 
-	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
-			reg);
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
 	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
+		dev_err(dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
 	}
 }
 

-- 
2.47.3

[PATCH v3 3/7] iio: light: opt3001: ensure correct parenthesis alignment

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Ensure correct parenthesis alignment per checkpatch.pl report.

No functional change.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 73 +++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 5d88a697c245347bcb98254460308fc9a9f375f4..09843c31516b4dcfb37e6f5e32c30aa75a03d4cf 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -223,8 +223,8 @@ static const struct opt3001_scale opt3002_scales[] = {
 	},
 };
 
-static int opt3001_find_scale(const struct opt3001 *opt, int val,
-		int val2, u8 *exponent)
+static int opt3001_find_scale(const struct opt3001 *opt, int val, int val2,
+			      u8 *exponent)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {
@@ -242,8 +242,8 @@ static int opt3001_find_scale(const struct opt3001 *opt, int val,
 	return -EINVAL;
 }
 
-static void opt3001_to_iio_ret(struct opt3001 *opt, u8 exponent,
-		u16 mantissa, int *val, int *val2)
+static void opt3001_to_iio_ret(struct opt3001 *opt, u8 exponent, u16 mantissa,
+			       int *val, int *val2)
 {
 	int ret;
 	int whole = opt->chip_info->factor_whole;
@@ -364,8 +364,8 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 	if (opt->use_irq) {
 		/* Wait for the IRQ to indicate the conversion is complete */
 		ret = wait_event_timeout(opt->result_ready_queue,
-				opt->result_ready,
-				msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
+					 opt->result_ready,
+					 msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
 		if (ret == 0)
 			return -ETIMEDOUT;
 	} else {
@@ -474,8 +474,8 @@ static int opt3001_set_int_time(struct opt3001 *opt, int time)
 }
 
 static int opt3001_read_raw(struct iio_dev *iio,
-		struct iio_chan_spec const *chan, int *val, int *val2,
-		long mask)
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
 	int ret;
@@ -506,8 +506,8 @@ static int opt3001_read_raw(struct iio_dev *iio,
 }
 
 static int opt3001_write_raw(struct iio_dev *iio,
-		struct iio_chan_spec const *chan, int val, int val2,
-		long mask)
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
 	int ret;
@@ -532,9 +532,11 @@ static int opt3001_write_raw(struct iio_dev *iio,
 }
 
 static int opt3001_read_event_value(struct iio_dev *iio,
-		const struct iio_chan_spec *chan, enum iio_event_type type,
-		enum iio_event_direction dir, enum iio_event_info info,
-		int *val, int *val2)
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int *val, int *val2)
 {
 	struct opt3001 *opt = iio_priv(iio);
 	int ret = IIO_VAL_INT_PLUS_MICRO;
@@ -544,11 +546,13 @@ static int opt3001_read_event_value(struct iio_dev *iio,
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		opt3001_to_iio_ret(opt, opt->high_thresh_exp,
-				opt->high_thresh_mantissa, val, val2);
+				   opt->high_thresh_mantissa,
+				   val, val2);
 		break;
 	case IIO_EV_DIR_FALLING:
 		opt3001_to_iio_ret(opt, opt->low_thresh_exp,
-				opt->low_thresh_mantissa, val, val2);
+				   opt->low_thresh_mantissa,
+				   val, val2);
 		break;
 	default:
 		ret = -EINVAL;
@@ -560,9 +564,11 @@ static int opt3001_read_event_value(struct iio_dev *iio,
 }
 
 static int opt3001_write_event_value(struct iio_dev *iio,
-		const struct iio_chan_spec *chan, enum iio_event_type type,
-		enum iio_event_direction dir, enum iio_event_info info,
-		int val, int val2)
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info,
+				     int val, int val2)
 {
 	struct opt3001 *opt = iio_priv(iio);
 	struct i2c_client *client = opt->client;
@@ -626,8 +632,9 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 }
 
 static int opt3001_read_event_config(struct iio_dev *iio,
-		const struct iio_chan_spec *chan, enum iio_event_type type,
-		enum iio_event_direction dir)
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
 {
 	struct opt3001 *opt = iio_priv(iio);
 
@@ -635,8 +642,10 @@ static int opt3001_read_event_config(struct iio_dev *iio,
 }
 
 static int opt3001_write_event_config(struct iio_dev *iio,
-		const struct iio_chan_spec *chan, enum iio_event_type type,
-		enum iio_event_direction dir, bool state)
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
 {
 	struct opt3001 *opt = iio_priv(iio);
 	struct i2c_client *client = opt->client;
@@ -812,16 +821,16 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
 			OPT3001_CONFIGURATION_M_CONTINUOUS) {
 		if (ret & OPT3001_CONFIGURATION_FH)
 			iio_push_event(iio,
-					IIO_UNMOD_EVENT_CODE(chan_type, 0,
-							IIO_EV_TYPE_THRESH,
-							IIO_EV_DIR_RISING),
-					iio_get_time_ns(iio));
+				       IIO_UNMOD_EVENT_CODE(chan_type, 0,
+							    IIO_EV_TYPE_THRESH,
+							    IIO_EV_DIR_RISING),
+				       iio_get_time_ns(iio));
 		if (ret & OPT3001_CONFIGURATION_FL)
 			iio_push_event(iio,
-					IIO_UNMOD_EVENT_CODE(chan_type, 0,
-							IIO_EV_TYPE_THRESH,
-							IIO_EV_DIR_FALLING),
-					iio_get_time_ns(iio));
+				       IIO_UNMOD_EVENT_CODE(chan_type, 0,
+							    IIO_EV_TYPE_THRESH,
+							    IIO_EV_DIR_FALLING),
+				       iio_get_time_ns(iio));
 	} else if (ret & OPT3001_CONFIGURATION_CRF) {
 		ret = i2c_smbus_read_word_swapped(client, OPT3001_RESULT);
 		if (ret < 0) {
@@ -884,8 +893,8 @@ static int opt3001_probe(struct i2c_client *client)
 	/* Make use of INT pin only if valid IRQ no. is given */
 	if (irq > 0) {
 		ret = request_threaded_irq(irq, NULL, opt3001_irq,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				"opt3001", iio);
+					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					   "opt3001", iio);
 		if (ret) {
 			dev_err(dev, "failed to request IRQ #%d\n", irq);
 			return ret;

-- 
2.47.3

[PATCH v3 4/7] iio: light: opt3001: localize for loop iterator

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Localize loop iterator to tighten scope and improve code style
per checkpatch.pl report.

No functional change.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 09843c31516b4dcfb37e6f5e32c30aa75a03d4cf..22a73c1afa74988dabf9db10c3b55bd07529fb20 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -226,8 +226,7 @@ static const struct opt3001_scale opt3002_scales[] = {
 static int opt3001_find_scale(const struct opt3001 *opt, int val, int val2,
 			      u8 *exponent)
 {
-	int i;
-	for (i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {
+	for (unsigned int i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {
 		const struct opt3001_scale *scale = &(*opt->chip_info->scales)[i];
 		/*
 		 * Compare the integer and micro parts to determine value scale.

-- 
2.47.3

[PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Switch driver to use dev_err_probe() to unify error messages generated
in *_probe() and probe path functions.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 56 ++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 22a73c1afa74988dabf9db10c3b55bd07529fb20..d5262488dac889066a53d3234762e0557f9e3cbc 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -706,21 +706,17 @@ static int opt3001_read_id(struct opt3001 *opt)
 	int ret;
 
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_MANUFACTURER_ID);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_MANUFACTURER_ID);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to read register %02x\n",
+				     OPT3001_MANUFACTURER_ID);
 
 	manufacturer[0] = ret >> 8;
 	manufacturer[1] = ret & 0xff;
 
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_DEVICE_ID);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_DEVICE_ID);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to read register %02x\n",
+				     OPT3001_DEVICE_ID);
 
 	device_id = ret;
 
@@ -738,11 +734,9 @@ static int opt3001_configure(struct opt3001 *opt)
 	u16 reg;
 
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_CONFIGURATION);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to read register %02x\n",
+				     OPT3001_CONFIGURATION);
 
 	reg = ret;
 
@@ -766,28 +760,22 @@ static int opt3001_configure(struct opt3001 *opt)
 	reg &= ~OPT3001_CONFIGURATION_FC_MASK;
 
 	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
-	if (ret < 0) {
-		dev_err(dev, "failed to write register %02x\n",
-			OPT3001_CONFIGURATION);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to write register %02x\n",
+				     OPT3001_CONFIGURATION);
 
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_LOW_LIMIT);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to read register %02x\n",
+				     OPT3001_LOW_LIMIT);
 
 	opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
 	opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret);
 
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_HIGH_LIMIT);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_HIGH_LIMIT);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to read register %02x\n",
+				     OPT3001_HIGH_LIMIT);
 
 	opt->high_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
 	opt->high_thresh_exp = OPT3001_REG_EXPONENT(ret);
@@ -894,10 +882,10 @@ static int opt3001_probe(struct i2c_client *client)
 		ret = request_threaded_irq(irq, NULL, opt3001_irq,
 					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					   "opt3001", iio);
-		if (ret) {
-			dev_err(dev, "failed to request IRQ #%d\n", irq);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to request IRQ #%d\n",
+					     irq);
 		opt->use_irq = true;
 	} else {
 		dev_dbg(dev, "enabling interrupt-less operation\n");

-- 
2.47.3

[PATCH v3 6/7] iio: light: opt3001: move driver to guard(mutex)() use

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Move driver to use guard(mutex)() macro, to facilitate automatic
locking/unlocking of resources. This modernizes the driver and
improves code style.

While at it, remove unnecessary gotos and return variables.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 140 +++++++++++++++++++-------------------------
 1 file changed, 60 insertions(+), 80 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index d5262488dac889066a53d3234762e0557f9e3cbc..ac1c7ba5679867f6b814894680e317615f2e373b 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -10,6 +10,7 @@
 
 #include <linux/array_size.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
@@ -477,7 +478,6 @@ static int opt3001_read_raw(struct iio_dev *iio,
 			    int *val, int *val2, long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
-	int ret;
 
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
@@ -485,23 +485,17 @@ static int opt3001_read_raw(struct iio_dev *iio,
 	if (chan->type != opt->chip_info->chan_type)
 		return -EINVAL;
 
-	mutex_lock(&opt->lock);
+	guard(mutex)(&opt->lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 	case IIO_CHAN_INFO_PROCESSED:
-		ret = opt3001_get_processed(opt, val, val2);
-		break;
+		return opt3001_get_processed(opt, val, val2);
 	case IIO_CHAN_INFO_INT_TIME:
-		ret = opt3001_get_int_time(opt, val, val2);
-		break;
+		return opt3001_get_int_time(opt, val, val2);
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-	mutex_unlock(&opt->lock);
-
-	return ret;
 }
 
 static int opt3001_write_raw(struct iio_dev *iio,
@@ -509,7 +503,6 @@ static int opt3001_write_raw(struct iio_dev *iio,
 			     int val, int val2, long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
-	int ret;
 
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
@@ -523,11 +516,9 @@ static int opt3001_write_raw(struct iio_dev *iio,
 	if (val != 0)
 		return -EINVAL;
 
-	mutex_lock(&opt->lock);
-	ret = opt3001_set_int_time(opt, val2);
-	mutex_unlock(&opt->lock);
+	guard(mutex)(&opt->lock);
 
-	return ret;
+	return opt3001_set_int_time(opt, val2);
 }
 
 static int opt3001_read_event_value(struct iio_dev *iio,
@@ -538,28 +529,23 @@ static int opt3001_read_event_value(struct iio_dev *iio,
 				    int *val, int *val2)
 {
 	struct opt3001 *opt = iio_priv(iio);
-	int ret = IIO_VAL_INT_PLUS_MICRO;
 
-	mutex_lock(&opt->lock);
+	guard(mutex)(&opt->lock);
 
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		opt3001_to_iio_ret(opt, opt->high_thresh_exp,
 				   opt->high_thresh_mantissa,
 				   val, val2);
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_EV_DIR_FALLING:
 		opt3001_to_iio_ret(opt, opt->low_thresh_exp,
 				   opt->low_thresh_mantissa,
 				   val, val2);
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-	mutex_unlock(&opt->lock);
-
-	return ret;
 }
 
 static int opt3001_write_event_value(struct iio_dev *iio,
@@ -586,12 +572,12 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 	if (val < 0)
 		return -EINVAL;
 
-	mutex_lock(&opt->lock);
+	guard(mutex)(&opt->lock);
 
 	ret = opt3001_find_scale(opt, val, val2, &exponent);
 	if (ret < 0) {
 		dev_err(dev, "can't find scale for %d.%06u\n", val, val2);
-		goto err;
+		return ret;
 	}
 
 	whole = opt->chip_info->factor_whole;
@@ -614,20 +600,16 @@ static int opt3001_write_event_value(struct iio_dev *iio,
 		opt->low_thresh_exp = exponent;
 		break;
 	default:
-		ret = -EINVAL;
-		goto err;
+		return -EINVAL;
 	}
 
 	ret = i2c_smbus_write_word_swapped(client, reg, value);
 	if (ret < 0) {
 		dev_err(dev, "failed to write register %02x\n", reg);
-		goto err;
+		return ret;
 	}
 
-err:
-	mutex_unlock(&opt->lock);
-
-	return ret;
+	return 0;
 }
 
 static int opt3001_read_event_config(struct iio_dev *iio,
@@ -659,7 +641,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 	if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN)
 		return 0;
 
-	mutex_lock(&opt->lock);
+	guard(mutex)(&opt->lock);
 
 	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
 		: OPT3001_CONFIGURATION_M_SHUTDOWN;
@@ -668,7 +650,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 	if (ret < 0) {
 		dev_err(dev, "failed to read register %02x\n",
 			OPT3001_CONFIGURATION);
-		goto err;
+		return ret;
 	}
 
 	reg = ret;
@@ -678,13 +660,10 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 	if (ret < 0) {
 		dev_err(dev, "failed to write register %02x\n",
 			OPT3001_CONFIGURATION);
-		goto err;
+		return ret;
 	}
 
-err:
-	mutex_unlock(&opt->lock);
-
-	return ret;
+	return 0;
 }
 
 static const struct iio_info opt3001_info = {
@@ -726,6 +705,31 @@ static int opt3001_read_id(struct opt3001 *opt)
 	return 0;
 }
 
+static void opt3001_power_off(void *data)
+{
+	struct opt3001 *opt = data;
+	struct i2c_client *client = opt->client;
+	struct device *dev = opt->dev;
+	u16 reg_val;
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(dev, "failed to read register %02x\n",
+			OPT3001_CONFIGURATION);
+		return;
+	}
+
+	reg_val = ret;
+	opt3001_set_mode(opt, &reg_val, OPT3001_CONFIGURATION_M_SHUTDOWN);
+
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION,
+					   reg_val);
+	if (ret < 0)
+		dev_err(dev, "failed to write to register %02x\n",
+			OPT3001_CONFIGURATION);
+}
+
 static int opt3001_configure(struct opt3001 *opt)
 {
 	struct i2c_client *client = opt->client;
@@ -764,6 +768,11 @@ static int opt3001_configure(struct opt3001 *opt)
 		return dev_err_probe(dev, ret, "failed to write register %02x\n",
 				     OPT3001_CONFIGURATION);
 
+	ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register power off function\n");
+
 	ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "failed to read register %02x\n",
@@ -857,7 +866,10 @@ static int opt3001_probe(struct i2c_client *client)
 	opt->client = client;
 	opt->chip_info = i2c_get_match_data(client);
 
-	mutex_init(&opt->lock);
+	ret = devm_mutex_init(dev, &opt->lock);
+	if (ret)
+		return ret;
+
 	init_waitqueue_head(&opt->result_ready_queue);
 	i2c_set_clientdata(client, iio);
 
@@ -879,49 +891,18 @@ static int opt3001_probe(struct i2c_client *client)
 
 	/* Make use of INT pin only if valid IRQ no. is given */
 	if (irq > 0) {
-		ret = request_threaded_irq(irq, NULL, opt3001_irq,
-					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					   "opt3001", iio);
+		ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						"opt3001", iio);
 		if (ret)
-			return dev_err_probe(dev, ret,
-					     "failed to request IRQ #%d\n",
-					     irq);
+			return ret;
+
 		opt->use_irq = true;
 	} else {
 		dev_dbg(dev, "enabling interrupt-less operation\n");
 	}
 
-	return iio_device_register(iio);
-}
-
-static void opt3001_remove(struct i2c_client *client)
-{
-	struct iio_dev *iio = i2c_get_clientdata(client);
-	struct opt3001 *opt = iio_priv(iio);
-	struct device *dev = &client->dev;
-	int ret;
-	u16 reg;
-
-	iio_device_unregister(iio);
-
-	if (opt->use_irq)
-		free_irq(client->irq, iio);
-
-	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
-	if (ret < 0) {
-		dev_err(dev, "failed to read register %02x\n",
-			OPT3001_CONFIGURATION);
-		return;
-	}
-
-	reg = ret;
-	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
-
-	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
-	if (ret < 0) {
-		dev_err(dev, "failed to write register %02x\n",
-			OPT3001_CONFIGURATION);
-	}
+	return devm_iio_device_register(dev, iio);
 }
 
 static const struct opt3001_chip_info opt3001_chip_information = {
@@ -962,7 +943,6 @@ MODULE_DEVICE_TABLE(of, opt3001_of_match);
 
 static struct i2c_driver opt3001_driver = {
 	.probe = opt3001_probe,
-	.remove = opt3001_remove,
 	.id_table = opt3001_id,
 
 	.driver = {

-- 
2.47.3

[PATCH v3 7/7] iio: light: opt3001: add comment to mutex

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Add comment to mutex per checkpatch.pl report.

No functional change.

Reviewed-by: Maxwell Doose <m32285159@gmail.com>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/opt3001.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index ac1c7ba5679867f6b814894680e317615f2e373b..9d24d7a23ef00d160fdae737b6ea1943576a3fcf 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -103,6 +103,7 @@ struct opt3001_chip_info {
 struct opt3001 {
 	struct i2c_client	*client;
 
+	/* Mutex for ensuring one executed command at a time */
 	struct mutex		lock;
 	bool			ok_to_ignore_lock;
 	bool			result_ready;

-- 
2.47.3

Re: [PATCH v3 1/7] iio: light: opt3001: move device registration to end of probe()

[Joshua Crofts]

On Thu, 21 May 2026 at 11:55, Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>         /* Make use of INT pin only if valid IRQ no. is given */
>         if (irq > 0) {
>                 ret = request_threaded_irq(irq, NULL, opt3001_irq,
> @@ -894,7 +888,7 @@ static int opt3001_probe(struct i2c_client *client)
>                 dev_dbg(opt->dev, "enabling interrupt-less operation\n");
>         }
>
> -       return 0;
> +       return iio_device_register(iio);
>  }

As Sashiko correctly pointed out, I'm not freeing the irq on failure,
therefore potentially leaking resources. This will definitely be fixed
in v4.

https://sashiko.dev/#/patchset/20260521-opt3001-cleanup-v3-0-820169dec8c3%40gmail.com

-- 
Kind regards

CJD

Re: [PATCH v3 6/7] iio: light: opt3001: move driver to guard(mutex)() use

[Joshua Crofts]

On Thu, 21 May 2026 at 11:55, Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>
> From: Joshua Crofts <joshua.crofts1@gmail.com>
>
> Move driver to use guard(mutex)() macro, to facilitate automatic
> locking/unlocking of resources. This modernizes the driver and
> improves code style.
>
> While at it, remove unnecessary gotos and return variables.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 140 +++++++++++++++++++-------------------------
>  1 file changed, 60 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index d5262488dac889066a53d3234762e0557f9e3cbc..ac1c7ba5679867f6b814894680e317615f2e373b 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -10,6 +10,7 @@
>
>  #include <linux/array_size.h>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
>  #include <linux/errno.h>
> @@ -477,7 +478,6 @@ static int opt3001_read_raw(struct iio_dev *iio,
>                             int *val, int *val2, long mask)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret;
>
>         if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
>                 return -EBUSY;
> @@ -485,23 +485,17 @@ static int opt3001_read_raw(struct iio_dev *iio,
>         if (chan->type != opt->chip_info->chan_type)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
>         case IIO_CHAN_INFO_PROCESSED:
> -               ret = opt3001_get_processed(opt, val, val2);
> -               break;
> +               return opt3001_get_processed(opt, val, val2);
>         case IIO_CHAN_INFO_INT_TIME:
> -               ret = opt3001_get_int_time(opt, val, val2);
> -               break;
> +               return opt3001_get_int_time(opt, val, val2);
>         default:
> -               ret = -EINVAL;
> +               return -EINVAL;
>         }
> -
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
>  }
>
>  static int opt3001_write_raw(struct iio_dev *iio,
> @@ -509,7 +503,6 @@ static int opt3001_write_raw(struct iio_dev *iio,
>                              int val, int val2, long mask)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret;
>
>         if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
>                 return -EBUSY;
> @@ -523,11 +516,9 @@ static int opt3001_write_raw(struct iio_dev *iio,
>         if (val != 0)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> -       ret = opt3001_set_int_time(opt, val2);
> -       mutex_unlock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
> -       return ret;
> +       return opt3001_set_int_time(opt, val2);
>  }
>
>  static int opt3001_read_event_value(struct iio_dev *iio,
> @@ -538,28 +529,23 @@ static int opt3001_read_event_value(struct iio_dev *iio,
>                                     int *val, int *val2)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret = IIO_VAL_INT_PLUS_MICRO;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         switch (dir) {
>         case IIO_EV_DIR_RISING:
>                 opt3001_to_iio_ret(opt, opt->high_thresh_exp,
>                                    opt->high_thresh_mantissa,
>                                    val, val2);
> -               break;
> +               return IIO_VAL_INT_PLUS_MICRO;
>         case IIO_EV_DIR_FALLING:
>                 opt3001_to_iio_ret(opt, opt->low_thresh_exp,
>                                    opt->low_thresh_mantissa,
>                                    val, val2);
> -               break;
> +               return IIO_VAL_INT_PLUS_MICRO;
>         default:
> -               ret = -EINVAL;
> +               return -EINVAL;
>         }
> -
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
>  }
>
>  static int opt3001_write_event_value(struct iio_dev *iio,
> @@ -586,12 +572,12 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>         if (val < 0)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         ret = opt3001_find_scale(opt, val, val2, &exponent);
>         if (ret < 0) {
>                 dev_err(dev, "can't find scale for %d.%06u\n", val, val2);
> -               goto err;
> +               return ret;
>         }
>
>         whole = opt->chip_info->factor_whole;
> @@ -614,20 +600,16 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>                 opt->low_thresh_exp = exponent;
>                 break;
>         default:
> -               ret = -EINVAL;
> -               goto err;
> +               return -EINVAL;
>         }
>
>         ret = i2c_smbus_write_word_swapped(client, reg, value);
>         if (ret < 0) {
>                 dev_err(dev, "failed to write register %02x\n", reg);
> -               goto err;
> +               return ret;
>         }
>
> -err:
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int opt3001_read_event_config(struct iio_dev *iio,
> @@ -659,7 +641,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN)
>                 return 0;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
>                 : OPT3001_CONFIGURATION_M_SHUTDOWN;
> @@ -668,7 +650,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (ret < 0) {
>                 dev_err(dev, "failed to read register %02x\n",
>                         OPT3001_CONFIGURATION);
> -               goto err;
> +               return ret;
>         }
>
>         reg = ret;
> @@ -678,13 +660,10 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (ret < 0) {
>                 dev_err(dev, "failed to write register %02x\n",
>                         OPT3001_CONFIGURATION);
> -               goto err;
> +               return ret;
>         }
>
> -err:
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static const struct iio_info opt3001_info = {
> @@ -726,6 +705,31 @@ static int opt3001_read_id(struct opt3001 *opt)
>         return 0;
>  }
>
> +static void opt3001_power_off(void *data)
> +{
> +       struct opt3001 *opt = data;
> +       struct i2c_client *client = opt->client;
> +       struct device *dev = opt->dev;
> +       u16 reg_val;
> +       int ret;
> +
> +       ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to read register %02x\n",
> +                       OPT3001_CONFIGURATION);
> +               return;
> +       }
> +
> +       reg_val = ret;
> +       opt3001_set_mode(opt, &reg_val, OPT3001_CONFIGURATION_M_SHUTDOWN);
> +
> +       ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION,
> +                                          reg_val);
> +       if (ret < 0)
> +               dev_err(dev, "failed to write to register %02x\n",
> +                       OPT3001_CONFIGURATION);
> +}
> +
>  static int opt3001_configure(struct opt3001 *opt)
>  {
>         struct i2c_client *client = opt->client;
> @@ -764,6 +768,11 @@ static int opt3001_configure(struct opt3001 *opt)
>                 return dev_err_probe(dev, ret, "failed to write register %02x\n",
>                                      OPT3001_CONFIGURATION);
>
> +       ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "failed to register power off function\n");
> +
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
>         if (ret < 0)
>                 return dev_err_probe(dev, ret, "failed to read register %02x\n",
> @@ -857,7 +866,10 @@ static int opt3001_probe(struct i2c_client *client)
>         opt->client = client;
>         opt->chip_info = i2c_get_match_data(client);
>
> -       mutex_init(&opt->lock);
> +       ret = devm_mutex_init(dev, &opt->lock);
> +       if (ret)
> +               return ret;
> +
>         init_waitqueue_head(&opt->result_ready_queue);
>         i2c_set_clientdata(client, iio);
>
> @@ -879,49 +891,18 @@ static int opt3001_probe(struct i2c_client *client)
>
>         /* Make use of INT pin only if valid IRQ no. is given */
>         if (irq > 0) {
> -               ret = request_threaded_irq(irq, NULL, opt3001_irq,
> -                                          IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -                                          "opt3001", iio);
> +               ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
> +                                               IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                               "opt3001", iio);

Nope nope nope, disregard this patch, obviously I've merged this incorrectly
so that's another reason for a v4 (perhaps some patches could get picked up
though).

-- 
Kind regards

CJD

Re: [PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Maxwell Doose]

Hi Joshua,

On Thu, May 21, 2026 at 5:13 AM Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>
> From: Joshua Crofts <joshua.crofts1@gmail.com>
>
> Switch driver to use dev_err_probe() to unify error messages generated
> in *_probe() and probe path functions.
>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 56 ++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 22a73c1afa74988dabf9db10c3b55bd07529fb20..d5262488dac889066a53d3234762e0557f9e3cbc 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -706,21 +706,17 @@ static int opt3001_read_id(struct opt3001 *opt)
>         int ret;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_MANUFACTURER_ID);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_MANUFACTURER_ID);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_MANUFACTURER_ID);
>
>         manufacturer[0] = ret >> 8;
>         manufacturer[1] = ret & 0xff;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_DEVICE_ID);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_DEVICE_ID);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_DEVICE_ID);
>
>         device_id = ret;
>
> @@ -738,11 +734,9 @@ static int opt3001_configure(struct opt3001 *opt)
>         u16 reg;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_CONFIGURATION);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_CONFIGURATION);
>
>         reg = ret;
>
> @@ -766,28 +760,22 @@ static int opt3001_configure(struct opt3001 *opt)
>         reg &= ~OPT3001_CONFIGURATION_FC_MASK;
>
>         ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to write register %02x\n",
> -                       OPT3001_CONFIGURATION);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to write register %02x\n",
> +                                    OPT3001_CONFIGURATION);
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_LOW_LIMIT);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_LOW_LIMIT);
>
>         opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
>         opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret);
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_HIGH_LIMIT);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_HIGH_LIMIT);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_HIGH_LIMIT);
>
>         opt->high_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
>         opt->high_thresh_exp = OPT3001_REG_EXPONENT(ret);

Ah, but all of the above don't seem like _probe() functions. I'm not
an expert on dev_err_probe() but I'm pretty sure it's only for use (or
at least, designed for use) in _probe() functions. Only function where
I know it's 100% fine in is the one below.

> @@ -894,10 +882,10 @@ static int opt3001_probe(struct i2c_client *client)
>                 ret = request_threaded_irq(irq, NULL, opt3001_irq,
>                                            IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>                                            "opt3001", iio);
> -               if (ret) {
> -                       dev_err(dev, "failed to request IRQ #%d\n", irq);
> -                       return ret;
> -               }
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request IRQ #%d\n",
> +                                            irq);
>                 opt->use_irq = true;
>         } else {
>                 dev_dbg(dev, "enabling interrupt-less operation\n");
>
> --
> 2.47.3
>

Regardless, I'd wait for Jonathan or Andy to give their comments on
this before you make any decisions regarding which ones to switch over
(I also haven't looked at the v1 or v2 so I don't know whether or not
they recommended you put dev_err_probe() in the non-_probe()
functions).

best regards,
max

Re: [PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Joshua Crofts]

On Fri, 22 May 2026 at 03:49, Maxwell Doose <m32285159@gmail.com> wrote:
> Ah, but all of the above don't seem like _probe() functions. I'm not
> an expert on dev_err_probe() but I'm pretty sure it's only for use (or
> at least, designed for use) in _probe() functions. Only function where
> I know it's 100% fine in is the one below.

Hi Max,

Actually if you have any functions that are *only* called in _probe(), then
it is okay to move them to dev_err_probe(), you just have to ensure
you're not calling dev_err_probe() twice - once in the function and
once at the call site.

-- 
Kind regards

CJD

Re: [PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Maxwell Doose]

On Fri, May 22, 2026 at 12:07 AM Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> On Fri, 22 May 2026 at 03:49, Maxwell Doose <m32285159@gmail.com> wrote:
> > Ah, but all of the above don't seem like _probe() functions. I'm not
> > an expert on dev_err_probe() but I'm pretty sure it's only for use (or
> > at least, designed for use) in _probe() functions. Only function where
> > I know it's 100% fine in is the one below.
>
> Hi Max,
>
> Actually if you have any functions that are *only* called in _probe(), then
> it is okay to move them to dev_err_probe(), you just have to ensure
> you're not calling dev_err_probe() twice - once in the function and
> once at the call site.
>

Ahh, interesting, I never knew that. Thanks for letting me know,
that's cool. I guess it's more flexible than I thought.

best regards,
max


> --
> Kind regards
>
> CJD

Re: [PATCH v3 3/7] iio: light: opt3001: ensure correct parenthesis alignment

[Maxwell Doose]

On Thu, May 21, 2026 at 5:10 AM Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>
> From: Joshua Crofts <joshua.crofts1@gmail.com>
>
> Ensure correct parenthesis alignment per checkpatch.pl report.
>
> No functional change.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 73 +++++++++++++++++++++++++--------------------
>  1 file changed, 41 insertions(+), 32 deletions(-)
>
[snip]

LGTM.

Reviewed-by: Maxwell Doose <m32285159@gmail.com>

best regards,
max

Re: [PATCH v3 1/7] iio: light: opt3001: move device registration to end of probe()

[Jonathan Cameron]

On Thu, 21 May 2026 11:55:38 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
> 
> Move IIO device registration to the end of the probe() function to
> prevent a potential race where userspace can interact with the device
> before IRQ is configured properly.

For this one, we really need a specific problem path rather than
generality that userspace interfaces aren't up yet so a spurious interrupt
causes us trouble.  

The intent is that the IIO core should drop anything it gets sent.
Might not be true in some corner case. If so we need to identify what
that is and whether we should add some more defense.

An example is iio_push_event().  That's guarded on iio_dev_opaque->ev_int
which is initialised in iio_device_register_eventset()
There might be problems in there but it will take more analysis.
The Kfifo looks suspicious as it's initialized a few lines too late
(for no obvious reason - ideally we'd look at whether we can
harden that!)

So question becomes is the kfifo used.  That's a driver question
and depends on a few register values. So chase that through to decide
if there is a bug here.

Otherwise it's just a logic improvement, not a fix.



> Additionally, switch
> devm_iio_device_register() to its unmanaged counterpart as current
> driver implementation mixes managed and unmanaged resources, causing
> potential resource leaks.

> 
> Also, add iio_device_unregister() to remove() function to correctly
> handle teardown.
> 
> Fixes: ac663db3678a ("iio: light: opt3001: enable operation w/o IRQ")
> Reported-by: Jonathan Cameron <jic23@kernel.org>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260511-opt3001-cleanup-v1-0-f7879dc3455c%40gmail.com?part=7
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index dac3d3818d0a01b4ca53106fa38bb54b8c5373d4..fe53a072b21ddaa26a23c76e82897fdbc1e4d846 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -874,12 +874,6 @@ static int	(struct i2c_client *client)
>  	iio->modes = INDIO_DIRECT_MODE;
>  	iio->info = &opt3001_info;
>  
> -	ret = devm_iio_device_register(dev, iio);
> -	if (ret) {
> -		dev_err(dev, "failed to register IIO device\n");
> -		return ret;
> -	}
> -
>  	/* Make use of INT pin only if valid IRQ no. is given */
>  	if (irq > 0) {
>  		ret = request_threaded_irq(irq, NULL, opt3001_irq,
> @@ -894,7 +888,7 @@ static int opt3001_probe(struct i2c_client *client)
>  		dev_dbg(opt->dev, "enabling interrupt-less operation\n");
>  	}
>  
> -	return 0;
> +	return iio_device_register(iio);
>  }
>  
>  static void opt3001_remove(struct i2c_client *client)
> @@ -904,6 +898,8 @@ static void opt3001_remove(struct i2c_client *client)
>  	int ret;
>  	u16 reg;
>  
> +	iio_device_unregister(iio);
> +
>  	if (opt->use_irq)
>  		free_irq(client->irq, iio);
>  
> 

Re: [PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Jonathan Cameron]

On Fri, 22 May 2026 00:19:16 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Fri, May 22, 2026 at 12:07 AM Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >
> > On Fri, 22 May 2026 at 03:49, Maxwell Doose <m32285159@gmail.com> wrote:  
> > > Ah, but all of the above don't seem like _probe() functions. I'm not
> > > an expert on dev_err_probe() but I'm pretty sure it's only for use (or
> > > at least, designed for use) in _probe() functions. Only function where
> > > I know it's 100% fine in is the one below.  
> >
> > Hi Max,
> >
> > Actually if you have any functions that are *only* called in _probe(), then
> > it is okay to move them to dev_err_probe(), you just have to ensure
> > you're not calling dev_err_probe() twice - once in the function and
> > once at the call site.
> >  
> 
> Ahh, interesting, I never knew that. Thanks for letting me know,
> that's cool. I guess it's more flexible than I thought.
> 
> best regards,
> max
opt3001_configure() has a name that makes me think it might well be called
elsewhere (it's not though!).  It is probably not worth the churn of
renaming it however.

Jonathan

> 
> 
> > --
> > Kind regards
> >
> > CJD  

Re: [PATCH v3 7/7] iio: light: opt3001: add comment to mutex

[Jonathan Cameron]

On Thu, 21 May 2026 11:55:44 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
> 
> Add comment to mutex per checkpatch.pl report.
> 
> No functional change.
> 
> Reviewed-by: Maxwell Doose <m32285159@gmail.com>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/light/opt3001.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index ac1c7ba5679867f6b814894680e317615f2e373b..9d24d7a23ef00d160fdae737b6ea1943576a3fcf 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -103,6 +103,7 @@ struct opt3001_chip_info {
>  struct opt3001 {
>  	struct i2c_client	*client;
>  
> +	/* Mutex for ensuring one executed command at a time */

The device uses a 'register' type control scheme rather than
command based. So I'm not sure what intended meaning of this is.
I'd say something like

	/*
	 * Ensure data capture sequences and read-modify-write
	 * sequences are not interrupted.
	 */

At least from glancing at the driver I think that's why it's there.

>  	struct mutex		lock;
>  	bool			ok_to_ignore_lock;
This one is odd enough and coupled with the lock. Either comment on both of them
to say how they interact or add two comments.

>  	bool			result_ready;
> 

Re: [PATCH v3 7/7] iio: light: opt3001: add comment to mutex

[Joshua Crofts]

On Fri, 22 May 2026 at 16:13, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 21 May 2026 11:55:44 +0200
> Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>
> > From: Joshua Crofts <joshua.crofts1@gmail.com>
> >
> > Add comment to mutex per checkpatch.pl report.
> >
> > No functional change.
> >
> > Reviewed-by: Maxwell Doose <m32285159@gmail.com>
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> > ---
> >  drivers/iio/light/opt3001.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> > index ac1c7ba5679867f6b814894680e317615f2e373b..9d24d7a23ef00d160fdae737b6ea1943576a3fcf 100644
> > --- a/drivers/iio/light/opt3001.c
> > +++ b/drivers/iio/light/opt3001.c
> > @@ -103,6 +103,7 @@ struct opt3001_chip_info {
> >  struct opt3001 {
> >       struct i2c_client       *client;
> >
> > +     /* Mutex for ensuring one executed command at a time */
>
> The device uses a 'register' type control scheme rather than
> command based. So I'm not sure what intended meaning of this is.
> I'd say something like

Of course, no clue how I got the idea of it being command based.
Must've mixed up the files...

--
Kind regards

CJD