[PATCH v4 0/2] iio: light: cm3323: fix and cleanup

[PATCH v4 0/2] iio: light: cm3323: fix and cleanup

[Aldo Conte]

Series fixes a bug and does a cleanup of the CM3323 
color light sensor driver.

Patch 1 fixes data->reg_conf being incorrectly initialized with the 
return value of the i2c_smbus_write_word_data() instead of the actual 	
register value. Tested with i2c-stub on a Raspberri Pi 3B.

Patch 2 converts dev_err() to dev_err_probe() in the probe path.

Changes in v4:
- Patch 2: Add Reviewed-by tag from Andy Shevchenko

Changes in v3:
- Patch 2: remove extra blank line after cm3323_init() error path
  in cm3323_probe()
  
Changes in v2:
- Patch 1: fix typos in commit message
- Patch 2: add struct device *dev to shorten dev_err_probe() calls
  to one-liners (suggested by Andy Shevchenko)

Aldo Conte (2):
  iio: light: cm3323: fix reg_conf not being initialized correctly
  iio: light: cm3323: use dev_err_probe() in probe path

 drivers/iio/light/cm3323.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

-- 
2.53.0

[PATCH v4 1/2] iio: light: cm3323: fix reg_conf not being initialized correctly

[Aldo Conte]

The code stores the return value of i2c_smbus_write_word_data()
in data->reg_conf; however, this value represents the result
of the write operation and not the value actually written to
the configuration register. This meant that the contents of
data->reg_conf did not truly reflect the contents
of the hardware register.

Instead, save the value of the register before the write
and use this value in the I2C write.

The bug was found by code inspection: i2c_smbus_write_word_data()
returns 0 on success, not the value written to the register.

Tested using i2c-stub on a Raspberry Pi 3B running a custom 6.19.10
kernel. Before loading the driver, the configuration register 0x00
CM3323_CMD_CONF was populated with 0x0030 using
`i2cset -y 11 0x10 0x00 0x0030 w`, encoding an integration time of 320ms
in bits[6:4].

Due to incorrect initialization of data->reg_conf in
cm3323_init(), the print of integration_time returns 0.040000
instead of the expected 0.320000. This happens because the read of the
integration_time depends on cm3323_get_it_bits() that is based on the
value of data->reg_conf, which is erroneously set to 0.

With this fix applied, data->reg_conf correctly saves 0x0030 after init
and the successive integration_time reports 0.320000 as expected.

Fixes: 8b0544263761 ("iio: light: Add support for Capella CM3323 color sensor")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/cm3323.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 79ad6e2209ca..0fe61b8a7029 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -89,15 +89,14 @@ static int cm3323_init(struct iio_dev *indio_dev)
 
 	/* enable sensor and set auto force mode */
 	ret &= ~(CM3323_CONF_SD_BIT | CM3323_CONF_AF_BIT);
+	data->reg_conf = ret;
 
-	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF, ret);
+	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF, data->reg_conf);
 	if (ret < 0) {
 		dev_err(&data->client->dev, "Error writing reg_conf\n");
 		return ret;
 	}
 
-	data->reg_conf = ret;
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH v4 1/2] iio: light: cm3323: fix reg_conf not being initialized correctly

[Jonathan Cameron]

On Tue,  7 Apr 2026 17:17:01 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> The code stores the return value of i2c_smbus_write_word_data()
> in data->reg_conf; however, this value represents the result
> of the write operation and not the value actually written to
> the configuration register. This meant that the contents of
> data->reg_conf did not truly reflect the contents
> of the hardware register.
> 
> Instead, save the value of the register before the write
> and use this value in the I2C write.
> 
> The bug was found by code inspection: i2c_smbus_write_word_data()
> returns 0 on success, not the value written to the register.
> 
> Tested using i2c-stub on a Raspberry Pi 3B running a custom 6.19.10
> kernel. Before loading the driver, the configuration register 0x00
> CM3323_CMD_CONF was populated with 0x0030 using
> `i2cset -y 11 0x10 0x00 0x0030 w`, encoding an integration time of 320ms
> in bits[6:4].
> 
> Due to incorrect initialization of data->reg_conf in
> cm3323_init(), the print of integration_time returns 0.040000
> instead of the expected 0.320000. This happens because the read of the
> integration_time depends on cm3323_get_it_bits() that is based on the
> value of data->reg_conf, which is erroneously set to 0.
> 
> With this fix applied, data->reg_conf correctly saves 0x0030 after init
> and the successive integration_time reports 0.320000 as expected.
> 
> Fixes: 8b0544263761 ("iio: light: Add support for Capella CM3323 color sensor")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
Applied this patch, but patch 2 will need to wait for this to make its way
into upstream - probably middle of the next cycle before that is in place.

thanks,

Jonathan

> ---
>  drivers/iio/light/cm3323.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 79ad6e2209ca..0fe61b8a7029 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -89,15 +89,14 @@ static int cm3323_init(struct iio_dev *indio_dev)
>  
>  	/* enable sensor and set auto force mode */
>  	ret &= ~(CM3323_CONF_SD_BIT | CM3323_CONF_AF_BIT);
> +	data->reg_conf = ret;
>  
> -	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF, ret);
> +	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF, data->reg_conf);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev, "Error writing reg_conf\n");
>  		return ret;
>  	}
>  
> -	data->reg_conf = ret;
> -
>  	return 0;
>  }
>  

Re: [PATCH v4 1/2] iio: light: cm3323: fix reg_conf not being initialized correctly

[Aldo Conte]

On 20/04/26 19:53, Jonathan Cameron wrote:
> On Tue,  7 Apr 2026 17:17:01 +0200
> Aldo Conte <aldocontelk@gmail.com> wrote:
> 
>> The code stores the return value of i2c_smbus_write_word_data()
>> in data->reg_conf; however, this value represents the result
>> of the write operation and not the value actually written to
>> the configuration register. This meant that the contents of
>> data->reg_conf did not truly reflect the contents
>> of the hardware register.
>>
>> Instead, save the value of the register before the write
>> and use this value in the I2C write.
>>
>> The bug was found by code inspection: i2c_smbus_write_word_data()
>> returns 0 on success, not the value written to the register.
>>
>> Tested using i2c-stub on a Raspberry Pi 3B running a custom 6.19.10
>> kernel. Before loading the driver, the configuration register 0x00
>> CM3323_CMD_CONF was populated with 0x0030 using
>> `i2cset -y 11 0x10 0x00 0x0030 w`, encoding an integration time of 320ms
>> in bits[6:4].
>>
>> Due to incorrect initialization of data->reg_conf in
>> cm3323_init(), the print of integration_time returns 0.040000
>> instead of the expected 0.320000. This happens because the read of the
>> integration_time depends on cm3323_get_it_bits() that is based on the
>> value of data->reg_conf, which is erroneously set to 0.
>>
>> With this fix applied, data->reg_conf correctly saves 0x0030 after init
>> and the successive integration_time reports 0.320000 as expected.
>>
>> Fixes: 8b0544263761 ("iio: light: Add support for Capella CM3323 color sensor")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> Applied this patch, but patch 2 will need to wait for this to make its way
> into upstream - probably middle of the next cycle before that is in place.
> 
> thanks,
> 
> Jonathan
> 

Hi Jonathan,

The fix this depended on (commit 1f4f0bcc5255 "iio: light: cm3323: fix
reg_conf not being initialized correctly") has since landed in mainline.

Is there anything still needed on my side for patch 2 (iio: light: cm3323: use 
dev_err_probe() in probe path) to proceed?

Thanks,
Aldo

[PATCH v4 2/2] iio: light: cm3323: use dev_err_probe() in probe path

[Aldo Conte]

Replace dev_err() calls with dev_err_probe() in cm3323_init() and
cm3323_probe(). cm3323_init() is called by cm3323_probe(),  so using
dev_err_probe() ensures that deferred probing is handled
correctly and simplifies error paths.

Remove the redundant error message in cm3323_probe().

Tested on a Raspberry Pi 3B using i2c-stub. The driver probes
successfully and successfully read integration_time after this change.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
Changes in v4:
- Add Reviewed-by tag from Andy Shevchenko

Changes in v3:
- Remove extra blank line after cm3323_init() error path
  in cm3323_probe()
  
Changes in v2:
- Add struct device *dev local variable to shorten dev_err_probe()
  calls to one-liners (suggested by Andy Shevchenko)
  
 drivers/iio/light/cm3323.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 0fe61b8a7029..c8112685e415 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -80,22 +80,19 @@ static int cm3323_init(struct iio_dev *indio_dev)
 {
 	int ret;
 	struct cm3323_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
 
 	ret = i2c_smbus_read_word_data(data->client, CM3323_CMD_CONF);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_conf\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Error reading reg_conf\n");
 
 	/* enable sensor and set auto force mode */
 	ret &= ~(CM3323_CONF_SD_BIT | CM3323_CONF_AF_BIT);
 	data->reg_conf = ret;
 
 	ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF, data->reg_conf);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_conf\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Error writing reg_conf\n");
 
 	return 0;
 }
@@ -236,10 +233,8 @@ static int cm3323_probe(struct i2c_client *client)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = cm3323_init(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "cm3323 chip init failed\n");
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = devm_add_action_or_reset(&client->dev, cm3323_disable, indio_dev);
 	if (ret < 0)
-- 
2.53.0