[PATCH] iio: light: apds9999: register standby action after enabling device

[PATCH] iio: light: apds9999: register standby action after enabling device

[kr494167]

From: Surendra Singh Chouhan <kr494167@gmail.com>

apds9999_init() called devm_add_action_or_reset() at the start
of the function, before register configuration and before enabling the
device via APDS9999_MAIN_CTRL_LS_EN.

If register initialization failed during apds9999_init() (e.g. SMBus
write failures on LS_MEAS_RATE or LS_GAIN), devm_add_action_or_reset()
immediately triggered apds9999_standby(), writing to the control
register on a device that failed initialization and was never enabled.

Fix this by registering devm_add_action_or_reset() only after
APDS9999_MAIN_CTRL_LS_EN is successfully written to the control
register.

Fixes: 5f9363e52300 ("iio: light: add support for APDS9999 sensor")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
 drivers/iio/light/apds9999.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
index 43fa9992c9c2..62620c4a2fbc 100644
--- a/drivers/iio/light/apds9999.c
+++ b/drivers/iio/light/apds9999.c
@@ -100,10 +100,6 @@ static int apds9999_init(struct apds9999_data *data)
 	u8 regval;
 	int ret;
 
-	ret = devm_add_action_or_reset(dev, apds9999_standby, client);
-	if (ret)
-		return ret;
-
 	guard(mutex)(&data->lock);
 
 	regval = FIELD_PREP(APDS9999_LS_RES_MASK, APDS9999_RES_18BIT) |
@@ -121,8 +117,12 @@ static int apds9999_init(struct apds9999_data *data)
 		return ret;
 	data->als_gain_idx = APDS9999_GAIN_3X;
 
-	return i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
-					 APDS9999_MAIN_CTRL_LS_EN);
+	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
+					APDS9999_MAIN_CTRL_LS_EN);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, apds9999_standby, client);
 }
 
 static int apds9999_read_channel(struct apds9999_data *data, u8 reg,
-- 
2.55.0

Re: [PATCH] iio: light: apds9999: register standby action after enabling device

[Jonathan Cameron]

On Fri, 24 Jul 2026 08:28:02 +0530
kr494167@gmail.com wrote:

> From: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> apds9999_init() called devm_add_action_or_reset() at the start
> of the function, before register configuration and before enabling the
> device via APDS9999_MAIN_CTRL_LS_EN.
> 
> If register initialization failed during apds9999_init() (e.g. SMBus
> write failures on LS_MEAS_RATE or LS_GAIN), devm_add_action_or_reset()
> immediately triggered apds9999_standby(), writing to the control
> register on a device that failed initialization and was never enabled.
> 
> Fix this by registering devm_add_action_or_reset() only after
> APDS9999_MAIN_CTRL_LS_EN is successfully written to the control
> register.
> 
> Fixes: 5f9363e52300 ("iio: light: add support for APDS9999 sensor")

Given the side effect of this is write that is harmless, I don't
think the fixes tag is justified. 

Having said that this is a logical improvement.

I would like Jose to have taken a look though before I consider picking
it up so let us leave it on list for now.

Thanks,

Jonathan

> Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
> ---
>  drivers/iio/light/apds9999.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
> index 43fa9992c9c2..62620c4a2fbc 100644
> --- a/drivers/iio/light/apds9999.c
> +++ b/drivers/iio/light/apds9999.c
> @@ -100,10 +100,6 @@ static int apds9999_init(struct apds9999_data *data)
>  	u8 regval;
>  	int ret;
>  
> -	ret = devm_add_action_or_reset(dev, apds9999_standby, client);
> -	if (ret)
> -		return ret;
> -
>  	guard(mutex)(&data->lock);
>  
>  	regval = FIELD_PREP(APDS9999_LS_RES_MASK, APDS9999_RES_18BIT) |
> @@ -121,8 +117,12 @@ static int apds9999_init(struct apds9999_data *data)
>  		return ret;
>  	data->als_gain_idx = APDS9999_GAIN_3X;
>  
> -	return i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> -					 APDS9999_MAIN_CTRL_LS_EN);
> +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> +					APDS9999_MAIN_CTRL_LS_EN);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, apds9999_standby, client);
>  }
>  
>  static int apds9999_read_channel(struct apds9999_data *data, u8 reg,

Re: [PATCH] iio: light: apds9999: register standby action after enabling device

[Jonathan Cameron]

On Sun, 26 Jul 2026 20:50:30 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 24 Jul 2026 08:28:02 +0530
> kr494167@gmail.com wrote:
> 
> > From: Surendra Singh Chouhan <kr494167@gmail.com>
> > 
> > apds9999_init() called devm_add_action_or_reset() at the start
> > of the function, before register configuration and before enabling the
> > device via APDS9999_MAIN_CTRL_LS_EN.
> > 
> > If register initialization failed during apds9999_init() (e.g. SMBus
> > write failures on LS_MEAS_RATE or LS_GAIN), devm_add_action_or_reset()
> > immediately triggered apds9999_standby(), writing to the control
> > register on a device that failed initialization and was never enabled.
> > 
> > Fix this by registering devm_add_action_or_reset() only after
> > APDS9999_MAIN_CTRL_LS_EN is successfully written to the control
> > register.
> > 
> > Fixes: 5f9363e52300 ("iio: light: add support for APDS9999 sensor")  
> 
> Given the side effect of this is write that is harmless, I don't
> think the fixes tag is justified. 
> 
> Having said that this is a logical improvement.
> 
> I would like Jose to have taken a look though before I consider picking
> it up so let us leave it on list for now.
> 
Seems they are busy or missed this.  So let me gamble...

Applied to the testing branch of iio.git which I will be rebasing on rc1
once available. Note that in the meantime I am happy to rebase so late
comments or tags are fine.

thanks

Jonathan

> Thanks,
> 
> Jonathan
> 
> > Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
> > ---
> >  drivers/iio/light/apds9999.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/light/apds9999.c b/drivers/iio/light/apds9999.c
> > index 43fa9992c9c2..62620c4a2fbc 100644
> > --- a/drivers/iio/light/apds9999.c
> > +++ b/drivers/iio/light/apds9999.c
> > @@ -100,10 +100,6 @@ static int apds9999_init(struct apds9999_data *data)
> >  	u8 regval;
> >  	int ret;
> >  
> > -	ret = devm_add_action_or_reset(dev, apds9999_standby, client);
> > -	if (ret)
> > -		return ret;
> > -
> >  	guard(mutex)(&data->lock);
> >  
> >  	regval = FIELD_PREP(APDS9999_LS_RES_MASK, APDS9999_RES_18BIT) |
> > @@ -121,8 +117,12 @@ static int apds9999_init(struct apds9999_data *data)
> >  		return ret;
> >  	data->als_gain_idx = APDS9999_GAIN_3X;
> >  
> > -	return i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> > -					 APDS9999_MAIN_CTRL_LS_EN);
> > +	ret = i2c_smbus_write_byte_data(client, APDS9999_REG_MAIN_CTRL,
> > +					APDS9999_MAIN_CTRL_LS_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(dev, apds9999_standby, client);
> >  }
> >  
> >  static int apds9999_read_channel(struct apds9999_data *data, u8 reg,  
> 
>