[PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[Maxwell Doose]

scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
calls. Replace them with the newer guard(mutex)() for cleaner RAII
patterns and to improve maintainability.

Add new helper function scd30_trigger_handler_helper_locked() containing
the critical section for scd30_trigger_handler(). After moving
scd30_trigger_handler()'s critical section into the new helper, tune up
control logic to return ret early and not memcpy() if it's an error
condition.

In addition, small refactor to replace "?:" operator with regular
if/else returns.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
 - Fix callback issue as noted by Jonathan v1.
 - Refactor critical section of scd30_trigger_handler() into helper
   called scd30_trigger_handler_helper_locked().
 - Revert removal of goto in scd30_trigger_handler().

 v3:
 - Tune up helper to return early on failure condition per Jonathan's
   suggestion.

 v4:
 - Forgot to commit changes listed in v3, now fixed.

 drivers/iio/chemical/scd30_core.c | 68 ++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index a665fcb78806..8dbba9a7c426 100644
--- a/drivers/iio/chemical/scd30_core.c
+++ b/drivers/iio/chemical/scd30_core.c
@@ -368,11 +368,13 @@ static ssize_t calibration_auto_enable_show(struct device *dev, struct device_at
 	int ret;
 	u16 val;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_read(state, CMD_ASC, &val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: sysfs_emit(buf, "%d\n", val);
+	ret = scd30_command_read(state, CMD_ASC, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t calibration_auto_enable_store(struct device *dev, struct device_attribute *attr,
@@ -387,11 +389,13 @@ static ssize_t calibration_auto_enable_store(struct device *dev, struct device_a
 	if (ret)
 		return ret;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_write(state, CMD_ASC, val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: len;
+	ret = scd30_command_write(state, CMD_ASC, val);
+	if (ret)
+		return ret;
+
+	return len;
 }
 
 static ssize_t calibration_forced_value_show(struct device *dev, struct device_attribute *attr,
@@ -402,11 +406,13 @@ static ssize_t calibration_forced_value_show(struct device *dev, struct device_a
 	int ret;
 	u16 val;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_read(state, CMD_FRC, &val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: sysfs_emit(buf, "%d\n", val);
+	ret = scd30_command_read(state, CMD_FRC, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
@@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
 	if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
 		return -EINVAL;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_write(state, CMD_FRC, val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: len;
+	ret = scd30_command_write(state, CMD_FRC, val);
+	if (ret)
+		return ret;
+
+	return len;
 }
 
 static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
@@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+/* Meant ONLY for scd30_trigger_handler() */
+static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
+					       int *scan_data, int arr_size)
+{
+	struct scd30_state *state = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&state->lock);
+
+	if (!iio_trigger_using_own(indio_dev))
+		ret = scd30_read_poll(state);
+	else
+		ret = scd30_read_meas(state);
+	if (ret)
+		return ret;
+	memcpy(scan_data, state->meas, arr_size);
+	return 0;
+}
+
 static irqreturn_t scd30_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
-	struct scd30_state *state = iio_priv(indio_dev);
 	struct {
 		int data[SCD30_MEAS_COUNT];
 		aligned_s64 ts;
 	} scan = { };
 	int ret;
 
-	mutex_lock(&state->lock);
-	if (!iio_trigger_using_own(indio_dev))
-		ret = scd30_read_poll(state);
-	else
-		ret = scd30_read_meas(state);
-	memcpy(scan.data, state->meas, sizeof(state->meas));
-	mutex_unlock(&state->lock);
+	ret = scd30_trigger_handler_helper_locked(indio_dev, scan.data, sizeof(scan.data));
 	if (ret)
 		goto out;
 
-- 
2.54.0

Re: [PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[Jonathan Cameron]

On Sat, 23 May 2026 13:25:32 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> calls. Replace them with the newer guard(mutex)() for cleaner RAII
> patterns and to improve maintainability.
> 
> Add new helper function scd30_trigger_handler_helper_locked() containing
> the critical section for scd30_trigger_handler(). After moving
> scd30_trigger_handler()'s critical section into the new helper, tune up
> control logic to return ret early and not memcpy() if it's an error
> condition.
> 
> In addition, small refactor to replace "?:" operator with regular
> if/else returns.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
Just some naming things inline.

I thought about just changing them and applying but decided
I'd rather you took another look to make sure you agree
with the suggested changes (and if you do send me a v5)

Thanks,

Jonathan

>  static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
> @@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
>  	if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
>  		return -EINVAL;
>  
> -	mutex_lock(&state->lock);
> -	ret = scd30_command_write(state, CMD_FRC, val);
> -	mutex_unlock(&state->lock);
> +	guard(mutex)(&state->lock);
>  
> -	return ret ?: len;
> +	ret = scd30_command_write(state, CMD_FRC, val);
> +	if (ret)
> +		return ret;
> +
> +	return len;
>  }
>  
>  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> @@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
>  	return IRQ_HANDLED;
>  }
>  
> +/* Meant ONLY for scd30_trigger_handler() */

No need to say this in a comment. The code naming is clear enough. However...

> +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> +					       int *scan_data, int arr_size)

Avoid using _locked() in naming.  It isn't clear to readers if that
means it is locked already, or will lock..  Also somewhat unnecessary
here. We don't need the function name to say why there is a helper.

arr_size would normally mean array size - i.e. how many ints
there are in scan_data.  Here it is the size of scan_data - probably
also make it a size_t

So this whole thing becomes

static int scd30_trigger_handler_helper(struct iio_dev *indio_dev,
					int *scan_data, size_t scan_data_size)





> +{
> +	struct scd30_state *state = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&state->lock);
> +
> +	if (!iio_trigger_using_own(indio_dev))
> +		ret = scd30_read_poll(state);
> +	else
> +		ret = scd30_read_meas(state);
> +	if (ret)
> +		return ret;
> +	memcpy(scan_data, state->meas, arr_size);
> +	return 0;
> +}
> +
>  static irqreturn_t scd30_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> -	struct scd30_state *state = iio_priv(indio_dev);
>  	struct {
>  		int data[SCD30_MEAS_COUNT];
>  		aligned_s64 ts;
>  	} scan = { };
>  	int ret;
>  
> -	mutex_lock(&state->lock);
> -	if (!iio_trigger_using_own(indio_dev))
> -		ret = scd30_read_poll(state);
> -	else
> -		ret = scd30_read_meas(state);
> -	memcpy(scan.data, state->meas, sizeof(state->meas));
> -	mutex_unlock(&state->lock);
> +	ret = scd30_trigger_handler_helper_locked(indio_dev, scan.data, sizeof(scan.data));
>  	if (ret)
>  		goto out;
>  

Re: [PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[Maxwell Doose]

On Tue, May 26, 2026 at 9:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 23 May 2026 13:25:32 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
>
> > scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> > calls. Replace them with the newer guard(mutex)() for cleaner RAII
> > patterns and to improve maintainability.
> >
> > Add new helper function scd30_trigger_handler_helper_locked() containing
> > the critical section for scd30_trigger_handler(). After moving
> > scd30_trigger_handler()'s critical section into the new helper, tune up
> > control logic to return ret early and not memcpy() if it's an error
> > condition.
> >
> > In addition, small refactor to replace "?:" operator with regular
> > if/else returns.
> >
> > Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> Just some naming things inline.
>
> I thought about just changing them and applying but decided
> I'd rather you took another look to make sure you agree
> with the suggested changes (and if you do send me a v5)
>
> Thanks,
>
> Jonathan
>
> >  static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
> > @@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
> >       if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
> >               return -EINVAL;
> >
> > -     mutex_lock(&state->lock);
> > -     ret = scd30_command_write(state, CMD_FRC, val);
> > -     mutex_unlock(&state->lock);
> > +     guard(mutex)(&state->lock);
> >
> > -     return ret ?: len;
> > +     ret = scd30_command_write(state, CMD_FRC, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return len;
> >  }
> >
> >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > @@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> >       return IRQ_HANDLED;
> >  }
> >
> > +/* Meant ONLY for scd30_trigger_handler() */
>
> No need to say this in a comment. The code naming is clear enough. However...
>

Agree here with the removal of said comment.

> > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > +                                            int *scan_data, int arr_size)
>
> Avoid using _locked() in naming.  It isn't clear to readers if that
> means it is locked already, or will lock..  Also somewhat unnecessary
> here. We don't need the function name to say why there is a helper.
>

Ah...I'm honestly not sure. I know we've decided that _locked was
appropriate in certain cases, I wonder if that may also be the case
here. I guess though since I'd have to send a v5 anyways I'll change
that.

>
> arr_size would normally mean array size - i.e. how many ints
> there are in scan_data.  Here it is the size of scan_data - probably
> also make it a size_t
>

Hm, perhaps? I definitely agree with size_t, I'm not so sure on
changing the name. However like I said since I'd likely have to send a
v5 anyways I'll switch up the naming.

>
> So this whole thing becomes
>
> static int scd30_trigger_handler_helper(struct iio_dev *indio_dev,
>                                         int *scan_data, size_t scan_data_size)
>
>

Sounds good.

best regards,
max

Re: [PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[Jonathan Cameron]

On Tue, 26 May 2026 15:31:10 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, May 26, 2026 at 9:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 23 May 2026 13:25:32 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> >  
> > > scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> > > calls. Replace them with the newer guard(mutex)() for cleaner RAII
> > > patterns and to improve maintainability.
> > >
> > > Add new helper function scd30_trigger_handler_helper_locked() containing
> > > the critical section for scd30_trigger_handler(). After moving
> > > scd30_trigger_handler()'s critical section into the new helper, tune up
> > > control logic to return ret early and not memcpy() if it's an error
> > > condition.
> > >
> > > In addition, small refactor to replace "?:" operator with regular
> > > if/else returns.
> > >
> > > Signed-off-by: Maxwell Doose <m32285159@gmail.com>  
> > Just some naming things inline.
> >
> > I thought about just changing them and applying but decided
> > I'd rather you took another look to make sure you agree
> > with the suggested changes (and if you do send me a v5)
> >
> > Thanks,
> >
> > Jonathan
> >  
> > >  static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
> > > @@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
> > >       if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
> > >               return -EINVAL;
> > >
> > > -     mutex_lock(&state->lock);
> > > -     ret = scd30_command_write(state, CMD_FRC, val);
> > > -     mutex_unlock(&state->lock);
> > > +     guard(mutex)(&state->lock);
> > >
> > > -     return ret ?: len;
> > > +     ret = scd30_command_write(state, CMD_FRC, val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return len;
> > >  }
> > >
> > >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > @@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > >       return IRQ_HANDLED;
> > >  }
> > >
> > > +/* Meant ONLY for scd30_trigger_handler() */  
> >
> > No need to say this in a comment. The code naming is clear enough. However...
> >  
> 
> Agree here with the removal of said comment.
> 
> > > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > > +                                            int *scan_data, int arr_size)  
> >
> > Avoid using _locked() in naming.  It isn't clear to readers if that
> > means it is locked already, or will lock..  Also somewhat unnecessary
> > here. We don't need the function name to say why there is a helper.
> >  
> 
> Ah...I'm honestly not sure. I know we've decided that _locked was
> appropriate in certain cases, I wonder if that may also be the case
> here. I guess though since I'd have to send a v5 anyways I'll change
> that.
> 
> >
> > arr_size would normally mean array size - i.e. how many ints
> > there are in scan_data.  Here it is the size of scan_data - probably
> > also make it a size_t
> >  
> 
> Hm, perhaps? I definitely agree with size_t, I'm not so sure on
> changing the name. However like I said since I'd likely have to send a
> v5 anyways I'll switch up the naming.

Given people will associate arr_size with ARRAY_SIZE() which on this array
would return the number of ints (so /4), that name definitely needs to change.
Exactly what it changes to is a different question!

> 
> >
> > So this whole thing becomes
> >
> > static int scd30_trigger_handler_helper(struct iio_dev *indio_dev,
> >                                         int *scan_data, size_t scan_data_size)
> >
> >  
> 
> Sounds good.
> 
> best regards,
> max

Re: [PATCH v4] iio: chemical: scd30: Replace manual locking with RAII locking

[Maxwell Doose]

On Wed, May 27, 2026 at 5:29 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 26 May 2026 15:31:10 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
>
> > On Tue, May 26, 2026 at 9:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Sat, 23 May 2026 13:25:32 -0500
> > > Maxwell Doose <m32285159@gmail.com> wrote:
> > >
> > > > scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> > > > calls. Replace them with the newer guard(mutex)() for cleaner RAII
> > > > patterns and to improve maintainability.
> > > >
> > > > Add new helper function scd30_trigger_handler_helper_locked() containing
> > > > the critical section for scd30_trigger_handler(). After moving
> > > > scd30_trigger_handler()'s critical section into the new helper, tune up
> > > > control logic to return ret early and not memcpy() if it's an error
> > > > condition.
> > > >
> > > > In addition, small refactor to replace "?:" operator with regular
> > > > if/else returns.
> > > >
> > > > Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> > > Just some naming things inline.
> > >
> > > I thought about just changing them and applying but decided
> > > I'd rather you took another look to make sure you agree
> > > with the suggested changes (and if you do send me a v5)
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > >  static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
> > > > @@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
> > > >       if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
> > > >               return -EINVAL;
> > > >
> > > > -     mutex_lock(&state->lock);
> > > > -     ret = scd30_command_write(state, CMD_FRC, val);
> > > > -     mutex_unlock(&state->lock);
> > > > +     guard(mutex)(&state->lock);
> > > >
> > > > -     return ret ?: len;
> > > > +     ret = scd30_command_write(state, CMD_FRC, val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return len;
> > > >  }
> > > >
> > > >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > > @@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > > >       return IRQ_HANDLED;
> > > >  }
> > > >
> > > > +/* Meant ONLY for scd30_trigger_handler() */
> > >
> > > No need to say this in a comment. The code naming is clear enough. However...
> > >
> >
> > Agree here with the removal of said comment.
> >
> > > > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > > > +                                            int *scan_data, int arr_size)
> > >
> > > Avoid using _locked() in naming.  It isn't clear to readers if that
> > > means it is locked already, or will lock..  Also somewhat unnecessary
> > > here. We don't need the function name to say why there is a helper.
> > >
> >
> > Ah...I'm honestly not sure. I know we've decided that _locked was
> > appropriate in certain cases, I wonder if that may also be the case
> > here. I guess though since I'd have to send a v5 anyways I'll change
> > that.
> >
> > >
> > > arr_size would normally mean array size - i.e. how many ints
> > > there are in scan_data.  Here it is the size of scan_data - probably
> > > also make it a size_t
> > >
> >
> > Hm, perhaps? I definitely agree with size_t, I'm not so sure on
> > changing the name. However like I said since I'd likely have to send a
> > v5 anyways I'll switch up the naming.
>
> Given people will associate arr_size with ARRAY_SIZE() which on this array
> would return the number of ints (so /4), that name definitely needs to change.
> Exactly what it changes to is a different question!
>

I wonder, perhaps either size (not as good) or arr_size_bytes? Maybe
also scan_data_byte_size? Probably still something better out there,
those are just some names I can come up with off the top of my head.

best regards,
max



> >
> > >
> > > So this whole thing becomes
> > >
> > > static int scd30_trigger_handler_helper(struct iio_dev *indio_dev,
> > >                                         int *scan_data, size_t scan_data_size)
> > >
> > >
> >
> > Sounds good.
> >
> > best regards,
> > max
>