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

[PATCH 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.

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

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
Joshua Crofts (7):
      iio: light: opt3001: make headers conform to iwyu
      iio: light: opt3001: use macros from bits.h header
      iio: light: opt3001: prefer dev_err_probe()
      iio: light: opt3001: move driver to guard(mutex)() use
      iio: light: opt3001: localize for loop iterator
      iio: light: opt3001: remove unnecessary comments
      iio: light: opt3001: switch driver to managed resources

 drivers/iio/light/opt3001.c | 208 +++++++++++++++++++-------------------------
 1 file changed, 88 insertions(+), 120 deletions(-)
---
base-commit: 74d173f29572951629d1e0b7456b424006e51b87
change-id: 20260511-opt3001-cleanup-c1411de14392

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

[PATCH 1/7] iio: light: opt3001: make headers conform to iwyu

[Joshua Crofts via B4 Relay]

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

Remove kernel.h proxy header, device.h, irq.h, slab.h as they are
unnecessary and add missing headers (array_size.h, dev_printk.h,
errno.h, jiffies.h, wait.h) to enforce IWYU principle and reduce
transitive dependencies. Also, replace bitops.h with bits.h as only
the BIT() macro is used.

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

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 53bc455b7bad142695d9fecc6cabb934fb3ace0c..e6c3665f46be47129097c3997c7551ba9a8f6440 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -8,18 +8,19 @@
  * Based on previous work from: Felipe Balbi <balbi@ti.com>
  */
 
-#include <linux/bitops.h>
+#include <linux/array_size.h>
+#include <linux/bits.h>
 #include <linux/delay.h>
-#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/irq.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/jiffies.h>
 #include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
-#include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/wait.h>
 
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>

-- 
2.47.3

[PATCH 2/7] iio: light: opt3001: use macros from bits.h header

[Joshua Crofts via B4 Relay]

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

Use GENMASK() and BIT() macros from bits.h header where it makes
sense. While at it, remove unused macro.

No functional change.

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

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index e6c3665f46be47129097c3997c7551ba9a8f6440..47f0e01a95e772954de0f1337ac0d881b8b32de6 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -33,17 +33,16 @@
 #define OPT3001_MANUFACTURER_ID	0x7e
 #define OPT3001_DEVICE_ID	0x7f
 
-#define OPT3001_CONFIGURATION_RN_MASK	(0xf << 12)
+#define OPT3001_CONFIGURATION_RN_MASK	GENMASK(15, 12)
 #define OPT3001_CONFIGURATION_RN_AUTO	(0xc << 12)
 
 #define OPT3001_CONFIGURATION_CT	BIT(11)
 
-#define OPT3001_CONFIGURATION_M_MASK	(3 << 9)
+#define OPT3001_CONFIGURATION_M_MASK	GENMASK(10, 9)
 #define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
-#define OPT3001_CONFIGURATION_M_SINGLE	(1 << 9)
+#define OPT3001_CONFIGURATION_M_SINGLE	BIT(9)
 #define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */
 
-#define OPT3001_CONFIGURATION_OVF	BIT(8)
 #define OPT3001_CONFIGURATION_CRF	BIT(7)
 #define OPT3001_CONFIGURATION_FH	BIT(6)
 #define OPT3001_CONFIGURATION_FL	BIT(5)
@@ -51,7 +50,7 @@
 #define OPT3001_CONFIGURATION_POL	BIT(3)
 #define OPT3001_CONFIGURATION_ME	BIT(2)
 
-#define OPT3001_CONFIGURATION_FC_MASK	(3 << 0)
+#define OPT3001_CONFIGURATION_FC_MASK	GENMASK(1, 0)
 
 /* The end-of-conversion enable is located in the low-limit register */
 #define OPT3001_LOW_LIMIT_EOC_ENABLE	0xc000

-- 
2.47.3

[PATCH 3/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 | 51 ++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 47f0e01a95e772954de0f1337ac0d881b8b32de6..4d9a17be04555aca368f7bc70a9d9a47fb5b279e 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -723,11 +723,10 @@ static int opt3001_configure(struct opt3001 *opt)
 	u16 reg;
 
 	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_CONFIGURATION);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(opt->dev, ret,
+				     "failed to read register %02x\n",
+				     OPT3001_CONFIGURATION);
 
 	reg = ret;
 
@@ -752,28 +751,25 @@ static int opt3001_configure(struct opt3001 *opt)
 
 	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
 			reg);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(opt->dev, ret,
+				     "failed to write register %02x\n",
+				     OPT3001_CONFIGURATION);
 
 	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_LOW_LIMIT);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_LOW_LIMIT);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(opt->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(opt->client, OPT3001_HIGH_LIMIT);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to read register %02x\n",
-				OPT3001_HIGH_LIMIT);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(opt->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);
@@ -875,20 +871,19 @@ static int opt3001_probe(struct i2c_client *client)
 	iio->info = &opt3001_info;
 
 	ret = devm_iio_device_register(dev, iio);
-	if (ret) {
-		dev_err(dev, "failed to register IIO device\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register IIO device\n");
 
 	/* 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);
-		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(opt->dev, "enabling interrupt-less operation\n");

-- 
2.47.3

[PATCH 4/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.

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

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 4d9a17be04555aca368f7bc70a9d9a47fb5b279e..bdff785bcfa9744cd25d32c5d00d62a11dd9d866 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>
@@ -478,7 +479,6 @@ static int opt3001_read_raw(struct iio_dev *iio,
 		long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
-	int ret;
 
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
@@ -486,23 +486,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,
@@ -510,7 +504,6 @@ static int opt3001_write_raw(struct iio_dev *iio,
 		long mask)
 {
 	struct opt3001 *opt = iio_priv(iio);
-	int ret;
 
 	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
 		return -EBUSY;
@@ -524,11 +517,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,
@@ -537,26 +528,21 @@ 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,
@@ -579,12 +565,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(opt->dev, "can't find scale for %d.%06u\n", val, val2);
-		goto err;
+		return ret;
 	}
 
 	whole = opt->chip_info->factor_whole;
@@ -607,18 +593,12 @@ 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(opt->client, reg, value);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(opt->dev, "failed to write register %02x\n", reg);
-		goto err;
-	}
-
-err:
-	mutex_unlock(&opt->lock);
 
 	return ret;
 }
@@ -647,7 +627,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;
@@ -656,7 +636,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 	if (ret < 0) {
 		dev_err(opt->dev, "failed to read register %02x\n",
 				OPT3001_CONFIGURATION);
-		goto err;
+		return ret;
 	}
 
 	reg = ret;
@@ -664,14 +644,9 @@ static int opt3001_write_event_config(struct iio_dev *iio,
 
 	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
 			reg);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(opt->dev, "failed to write register %02x\n",
 				OPT3001_CONFIGURATION);
-		goto err;
-	}
-
-err:
-	mutex_unlock(&opt->lock);
 
 	return ret;
 }

-- 
2.47.3

[PATCH 5/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 fix checkpatch.pl error.

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 bdff785bcfa9744cd25d32c5d00d62a11dd9d866..3719757f6ed7784cbf2e4aa659e5c69a39b0cce2 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -228,8 +228,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 (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 6/7] iio: light: opt3001: remove unnecessary comments

[Joshua Crofts via B4 Relay]

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

Remove unnecessary comments as code is self explanatory.

No functional change.

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

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 3719757f6ed7784cbf2e4aa659e5c69a39b0cce2..155794bb28f68a945e20b083e382561ca6ad462e 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -369,7 +369,6 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 		if (ret == 0)
 			return -ETIMEDOUT;
 	} else {
-		/* Sleep for result ready time */
 		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
 			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
 		msleep(timeout);
@@ -388,7 +387,6 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 			goto err;
 		}
 
-		/* Obtain value */
 		ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT);
 		if (ret < 0) {
 			dev_err(opt->dev, "failed to read register %02x\n",
@@ -919,7 +917,7 @@ static const struct opt3001_chip_info opt3002_chip_information = {
 static const struct i2c_device_id opt3001_id[] = {
 	{ "opt3001", (kernel_ulong_t)&opt3001_chip_information },
 	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
-	{ } /* Terminating Entry */
+	{ }
 };
 MODULE_DEVICE_TABLE(i2c, opt3001_id);
 

-- 
2.47.3

[PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Joshua Crofts via B4 Relay]

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

Move the driver to use devm_* functions to automate resource
management and simplify error handling. This also allows removal
of the opt3001_remove() function.

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

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 155794bb28f68a945e20b083e382561ca6ad462e..3ea18d8993da627ae226ac414e8035d8c968861b 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -804,6 +804,29 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
 	return IRQ_HANDLED;
 }
 
+static void opt3001_power_off(void *data)
+{
+	struct opt3001 *opt = data;
+	int ret;
+	u16 reg;
+
+	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->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);
+	if (ret < 0)
+		dev_err(opt->dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
+}
+
 static int opt3001_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -822,7 +845,10 @@ static int opt3001_probe(struct i2c_client *client)
 	opt->dev = dev;
 	opt->chip_info = i2c_get_match_data(client);
 
-	mutex_init(&opt->lock);
+	ret = devm_mutex_init(dev, &opt->lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
+
 	init_waitqueue_head(&opt->result_ready_queue);
 	i2c_set_clientdata(client, iio);
 
@@ -836,6 +862,10 @@ static int opt3001_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
+	if (ret)
+		return ret;
+
 	iio->name = client->name;
 	iio->channels = *opt->chip_info->channels;
 	iio->num_channels = opt->chip_info->num_channels;
@@ -849,9 +879,9 @@ 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",
@@ -864,34 +894,6 @@ static int opt3001_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void opt3001_remove(struct i2c_client *client)
-{
-	struct iio_dev *iio = i2c_get_clientdata(client);
-	struct opt3001 *opt = iio_priv(iio);
-	int ret;
-	u16 reg;
-
-	if (opt->use_irq)
-		free_irq(client->irq, iio);
-
-	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
-	if (ret < 0) {
-		dev_err(opt->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);
-	if (ret < 0) {
-		dev_err(opt->dev, "failed to write register %02x\n",
-				OPT3001_CONFIGURATION);
-	}
-}
-
 static const struct opt3001_chip_info opt3001_chip_information = {
 	.channels = &opt3001_channels,
 	.chan_type = IIO_LIGHT,
@@ -930,7 +932,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

Re: [PATCH 2/7] iio: light: opt3001: use macros from bits.h header

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:07PM +0200, Joshua Crofts via B4 Relay wrote:

> Use GENMASK() and BIT() macros from bits.h header where it makes
> sense. While at it, remove unused macro.

...

>  #define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
> -#define OPT3001_CONFIGURATION_M_SINGLE	(1 << 9)
> +#define OPT3001_CONFIGURATION_M_SINGLE	BIT(9)
>  #define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */

No to this and similar changes. It's clear that this is a shifted plain value,
it's not a bitfield. If in doubt, always consult with datasheet, if there is
no such publicly available, ask driver author/maintainers of the subsystem.

(The rule of thumb, whenever you see `0 << (x)` in the group of definitions,
 the above applies.)

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:08PM +0200, Joshua Crofts via B4 Relay wrote:

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

...

>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to read register %02x\n",
> -				OPT3001_CONFIGURATION);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(opt->dev, ret,

We have client available, and here is dev in use. With this being said,
I think the proper order of cleanups is to get rid of dev member and
use local 'dev' variable here

	struct device *dev = &opt->client->dev;

> +				     "failed to read register %02x\n",
> +				     OPT3001_CONFIGURATION);

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:09PM +0200, Joshua Crofts via B4 Relay wrote:

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

This one LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

...

Side note for the additional patch, in case you want to address that.

>  		dev_err(opt->dev, "failed to read register %02x\n",
>  				OPT3001_CONFIGURATION);

Broken indentation.

...

>  	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
>  			reg);

Ditto.

...

>  		dev_err(opt->dev, "failed to write register %02x\n",
>  				OPT3001_CONFIGURATION);

Ditto.

...

There may be more, I haven't checked, but maybe we want to clean this up as
well at some point.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] iio: light: opt3001: use macros from bits.h header

[Joshua Crofts]

On Mon, 11 May 2026 at 13:01, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 12:04:07PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Use GENMASK() and BIT() macros from bits.h header where it makes
> > sense. While at it, remove unused macro.
>
> ...
>
> >  #define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
> > -#define OPT3001_CONFIGURATION_M_SINGLE       (1 << 9)
> > +#define OPT3001_CONFIGURATION_M_SINGLE       BIT(9)
> >  #define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */
>
> No to this and similar changes. It's clear that this is a shifted plain value,
> it's not a bitfield. If in doubt, always consult with datasheet, if there is
> no such publicly available, ask driver author/maintainers of the subsystem.

Yeah, I knew this was a bit of a gamble, will fix in the next iteration.

-- 
Kind regards

CJD

Re: [PATCH 5/7] iio: light: opt3001: localize for loop iterator

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:10PM +0200, Joshua Crofts via B4 Relay wrote:

> Localize loop iterator to tighten scope and fix checkpatch.pl error.

Strictly speaking you do not fixing anything here related to checkpatch.pl,
you fix the code to *address* the pointed out issue.

Yeah, I know this language style issue is present in tons of the commits...

> No functional change.

...

> -	int i;
> -	for (i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {
> +	for (int i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) {

Maybe make it unsigned as well?


-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Mon, 11 May 2026 at 13:07, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 12:04:09PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Move driver to use guard(mutex)() macro, to facilitate automatic
> > locking/unlocking of resources. This modernizes the driver and
> > improves code style.
>
> This one LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>
> ...
>
> Side note for the additional patch, in case you want to address that.
>
> >               dev_err(opt->dev, "failed to read register %02x\n",
> >                               OPT3001_CONFIGURATION);
>
> Broken indentation.
>
> ...
>
> >       ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
> >                       reg);
>
> Ditto.
>
> ...
>
> >               dev_err(opt->dev, "failed to write register %02x\n",
> >                               OPT3001_CONFIGURATION);
>
> Ditto.
>
> ...
>
> There may be more, I haven't checked, but maybe we want to clean this up as
> well at some point.

There are about 30 errors when running checkpatch with the --strict flag haha,
I wanted to address it in another series, but I guess I can do it now
(only annoying
thing is that there will be lines longer than 80 characters).

-- 
Kind regards

CJD

Re: [PATCH 6/7] iio: light: opt3001: remove unnecessary comments

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:11PM +0200, Joshua Crofts via B4 Relay wrote:

> Remove unnecessary comments as code is self explanatory.

I won't touch the first two. They might still help to easy navigate in the code
for anybody who reads it in the first time.

...

> -	{ } /* Terminating Entry */
> +	{ }

Only this one makes sense to me, but since it's just comment, I would dare to
kill 'em all in each of the subfolders of IIO in a single patch (id est one
patch for all in iio/adc, one for iio/dac, and so on).

-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Mon, 11 May 2026 at 13:04, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 12:04:08PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Switch driver to use dev_err_probe() to unify
> > error messages generated in *_probe() and probe path functions.
>
> ...
>
> >       ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > -     if (ret < 0) {
> > -             dev_err(opt->dev, "failed to read register %02x\n",
> > -                             OPT3001_CONFIGURATION);
> > -             return ret;
> > -     }
> > +     if (ret < 0)
> > +             return dev_err_probe(opt->dev, ret,
>
> We have client available, and here is dev in use. With this being said,
> I think the proper order of cleanups is to get rid of dev member and
> use local 'dev' variable here
>
>         struct device *dev = &opt->client->dev;
>
> > +                                  "failed to read register %02x\n",
> > +                                  OPT3001_CONFIGURATION);

Fair enough, I'll slip in a preparatory patch.

-- 
Kind regards

CJD

Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Andy Shevchenko]

On Mon, May 11, 2026 at 12:04:12PM +0200, Joshua Crofts via B4 Relay wrote:

> Move the driver to use devm_* functions to automate resource
> management and simplify error handling. This also allows removal
> of the opt3001_remove() function.

...

> -		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",

You need to drop now duplicate error message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] iio: light: opt3001: remove unnecessary comments

[Joshua Crofts]

On Mon, 11 May 2026 at 13:11, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 12:04:11PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Remove unnecessary comments as code is self explanatory.
>
> I won't touch the first two. They might still help to easy navigate in the code
> for anybody who reads it in the first time.
>
> ...
>
> > -     { } /* Terminating Entry */
> > +     { }
>
> Only this one makes sense to me, but since it's just comment, I would dare to
> kill 'em all in each of the subfolders of IIO in a single patch (id est one
> patch for all in iio/adc, one for iio/dac, and so on).
>

Fair, I'll drop this patch and do a wider refactoring.

-- 
Kind regards

CJD

Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Joshua Crofts]

On Mon, 11 May 2026 at 13:13, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 12:04:12PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Move the driver to use devm_* functions to automate resource
> > management and simplify error handling. This also allows removal
> > of the opt3001_remove() function.
>
> ...
>
> > -             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",
>
> You need to drop now duplicate error message.

Oh yeah, I remember you doing that fix in the AK8975 series :)
Good point.

-- 
Kind regards

CJD

Re: [PATCH 2/7] iio: light: opt3001: use macros from bits.h header

[Jonathan Cameron]

On Mon, 11 May 2026 13:07:29 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Mon, 11 May 2026 at 13:01, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Mon, May 11, 2026 at 12:04:07PM +0200, Joshua Crofts via B4 Relay wrote:
> >  
> > > Use GENMASK() and BIT() macros from bits.h header where it makes
> > > sense. While at it, remove unused macro.  
> >
> > ...
> >  
> > >  #define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
> > > -#define OPT3001_CONFIGURATION_M_SINGLE       (1 << 9)
> > > +#define OPT3001_CONFIGURATION_M_SINGLE       BIT(9)
> > >  #define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */  
> >
> > No to this and similar changes. It's clear that this is a shifted plain value,
> > it's not a bitfield. If in doubt, always consult with datasheet, if there is
> > no such publicly available, ask driver author/maintainers of the subsystem.  
> 
> Yeah, I knew this was a bit of a gamble, will fix in the next iteration.

Bigger change but you could move to defining all these field values
without the shift and using FIELD_PREP() / FIELD_GET() with the mask (that is
one line above these).

That is how we'd do it in a modern driver.

J
> 

Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Jonathan Cameron]

On Mon, 11 May 2026 12:04:12 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
> 
> Move the driver to use devm_* functions to automate resource
> management and simplify error handling. This also allows removal
> of the opt3001_remove() function.
> 
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
Hi Joshua

A few things inline.

Jonathan

> ---
>  drivers/iio/light/opt3001.c | 67 +++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 155794bb28f68a945e20b083e382561ca6ad462e..3ea18d8993da627ae226ac414e8035d8c968861b 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -804,6 +804,29 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
>  	return IRQ_HANDLED;
>  }
>  
> +static void opt3001_power_off(void *data)
> +{
> +	struct opt3001 *opt = data;
> +	int ret;
> +	u16 reg;
> +
> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->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);
> +	if (ret < 0)
> +		dev_err(opt->dev, "failed to write register %02x\n",
> +			OPT3001_CONFIGURATION);
> +}
> +
>  static int opt3001_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -822,7 +845,10 @@ static int opt3001_probe(struct i2c_client *client)
>  	opt->dev = dev;
>  	opt->chip_info = i2c_get_match_data(client);
>  
> -	mutex_init(&opt->lock);
> +	ret = devm_mutex_init(dev, &opt->lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize mutex\n");

Don't print on this one.  I'm fairly sure it can only return -ENOMEM as
part of devm registration failing and for those we don't print errors.
dev_err_probe() won't print anything anyway so it's just noise having
it here.


> +
>  	init_waitqueue_head(&opt->result_ready_queue);
>  	i2c_set_clientdata(client, iio);
>  
> @@ -836,6 +862,10 @@ static int opt3001_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> +	if (ret)
> +		return ret;

This is undoing only part of what happens in the previous call (I think)
and if we get an error in there (opt3001_configure()) we don't turn the
power off again.  I'd move this registration into that function - probably after
the configuration write but before the thresholds etc are set.

That way those error paths are covered and it's more obvious what this
is undoing.

> +
>  	iio->name = client->name;
>  	iio->channels = *opt->chip_info->channels;
>  	iio->num_channels = opt->chip_info->num_channels;
> @@ -849,9 +879,9 @@ 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,

Hmm. Without hardware too risky I think to touch it, but why is this only registering
the interrupt after the userspace interfaces? That seems to be a nasty race.

> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						"opt3001", iio);
>  		if (ret)
>  			return dev_err_probe(dev, ret,
>  					     "failed to request IRQ #%d\n",
> @@ -864,34 +894,6 @@ static int opt3001_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static void opt3001_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *iio = i2c_get_clientdata(client);
> -	struct opt3001 *opt = iio_priv(iio);
> -	int ret;
> -	u16 reg;
> -
> -	if (opt->use_irq)
> -		free_irq(client->irq, iio);
> -
> -	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> -	if (ret < 0) {
> -		dev_err(opt->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);
> -	if (ret < 0) {
> -		dev_err(opt->dev, "failed to write register %02x\n",
> -				OPT3001_CONFIGURATION);
> -	}
> -}
> -
>  static const struct opt3001_chip_info opt3001_chip_information = {
>  	.channels = &opt3001_channels,
>  	.chan_type = IIO_LIGHT,
> @@ -930,7 +932,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 = {
> 

Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Joshua Crofts]

On Mon, 11 May 2026 at 15:32, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 11 May 2026 12:04:12 +0200
> Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:
>
> > From: Joshua Crofts <joshua.crofts1@gmail.com>
> >
> > Move the driver to use devm_* functions to automate resource
> > management and simplify error handling. This also allows removal
> > of the opt3001_remove() function.
> >
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Hi Joshua
>
> A few things inline.
>
> Jonathan
>
> > ---
> >  drivers/iio/light/opt3001.c | 67 +++++++++++++++++++++++----------------------
> >  1 file changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> > index 155794bb28f68a945e20b083e382561ca6ad462e..3ea18d8993da627ae226ac414e8035d8c968861b 100644
> > --- a/drivers/iio/light/opt3001.c
> > +++ b/drivers/iio/light/opt3001.c
> > @@ -804,6 +804,29 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static void opt3001_power_off(void *data)
> > +{
> > +     struct opt3001 *opt = data;
> > +     int ret;
> > +     u16 reg;
> > +
> > +     ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > +     if (ret < 0) {
> > +             dev_err(opt->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);
> > +     if (ret < 0)
> > +             dev_err(opt->dev, "failed to write register %02x\n",
> > +                     OPT3001_CONFIGURATION);
> > +}
> > +
> >  static int opt3001_probe(struct i2c_client *client)
> >  {
> >       struct device *dev = &client->dev;
> > @@ -822,7 +845,10 @@ static int opt3001_probe(struct i2c_client *client)
> >       opt->dev = dev;
> >       opt->chip_info = i2c_get_match_data(client);
> >
> > -     mutex_init(&opt->lock);
> > +     ret = devm_mutex_init(dev, &opt->lock);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
>
> Don't print on this one.  I'm fairly sure it can only return -ENOMEM as
> part of devm registration failing and for those we don't print errors.
> dev_err_probe() won't print anything anyway so it's just noise having
> it here.

Agreed.

>
> > +
> >       init_waitqueue_head(&opt->result_ready_queue);
> >       i2c_set_clientdata(client, iio);
> >
> > @@ -836,6 +862,10 @@ static int opt3001_probe(struct i2c_client *client)
> >       if (ret)
> >               return ret;
> >
> > +     ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> > +     if (ret)
> > +             return ret;
>
> This is undoing only part of what happens in the previous call (I think)
> and if we get an error in there (opt3001_configure()) we don't turn the
> power off again.  I'd move this registration into that function - probably after
> the configuration write but before the thresholds etc are set.
>
> That way those error paths are covered and it's more obvious what this
> is undoing.

Okay, seems logical.

> > +
> >       iio->name = client->name;
> >       iio->channels = *opt->chip_info->channels;
> >       iio->num_channels = opt->chip_info->num_channels;
> > @@ -849,9 +879,9 @@ 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,
>
> Hmm. Without hardware too risky I think to touch it, but why is this only registering
> the interrupt after the userspace interfaces? That seems to be a nasty race.

Good point - this is the way it was ordered originally, so this potential
race has been sitting in the driver for 11 years.

-- 
Kind regards

CJD