[PATCH v4 0/2] use guard()() to handle synchronisation

[PATCH v4 0/2] use guard()() to handle synchronisation

[Felipe Ribeiro de Souza]

Refactor ingenic_adc_read_chan_info_raw() and replace mutex_lock()
and mutex_unlock() calls with guard()() in drivers/iio/adc/ingenic-adc.c

Felipe Ribeiro de Souza (2):
  iio: adc: ingenic-adc: refactor ingenic_adc_read_chan_info_raw()
  iio: adc: ingenic-adc: use guard()() to handle synchronisation

 drivers/iio/adc/ingenic-adc.c | 56 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] iio: adc: ingenic-adc: refactor ingenic_adc_read_chan_info_raw()

[Felipe Ribeiro de Souza]

Extract the sample logic from ingenic_adc_read_chan_info_raw() into
a new helper function ingenic_adc_read_chan_locked() to improve code
readability and modularity.

The helper handles the mutex-protected section for sampling channels,
while the main function manages clock enabling/disabling.

Signed-off-by: Felipe Ribeiro de Souza <felipers@ime.usp.br>
Co-developed-by: Lucas Ivars Cadima Ciziks <lucas@ciziks.com>
Signed-off-by: Lucas Ivars Cadima Ciziks <lucas@ciziks.com>

---
v2:
  - Adjust order of #include.
  - Replace guard() in ingenic_adc_read_chan_info_raw() by scoped_guard()
    to preserve original behavior.
v3:
  - Split out ingenic_adc_read_chan_info_raw() logic to helper function.
v4:
  - Adjust code style for better readability.
---
 drivers/iio/adc/ingenic-adc.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 1e802c8779a4c..ad1b7bbc059a4 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -628,22 +628,15 @@ static int ingenic_adc_read_avail(struct iio_dev *iio_dev,
 	}
 }
 
-static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
-					  struct iio_chan_spec const *chan,
-					  int *val)
+static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,
+					struct iio_chan_spec const *chan,
+					int *val)
 {
 	int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
-	struct ingenic_adc *adc = iio_priv(iio_dev);
-
-	ret = clk_enable(adc->clk);
-	if (ret) {
-		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n",
-			ret);
-		return ret;
-	}
 
 	/* We cannot sample the aux channels in parallel. */
 	mutex_lock(&adc->aux_lock);
+
 	if (adc->soc_data->has_aux_md && engine == 0) {
 		switch (chan->channel) {
 		case INGENIC_ADC_AUX0:
@@ -678,6 +671,25 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
 	ret = IIO_VAL_INT;
 out:
 	mutex_unlock(&adc->aux_lock);
+
+	return ret;
+}
+
+static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
+					  struct iio_chan_spec const *chan,
+					  int *val)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+	int ret;
+
+	ret = clk_enable(adc->clk);
+	if (ret) {
+		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = ingenic_adc_read_chan_locked(adc, chan, val);
+
 	clk_disable(adc->clk);
 
 	return ret;
-- 
2.43.0

[PATCH v4 2/2] iio: adc: ingenic-adc: use guard()() to handle synchronisation

[Felipe Ribeiro de Souza]

Replace mutex_lock() and mutex_unlock() calls with guard()()
in functions ingenic_adc_set_adcmd(), ingenic_adc_set_config(),
ingenic_adc_enable(), ingenic_adc_capture() and
ingenic_adc_read_chan_locked().

This removes the need to call the unlock function, as the lock is
automatically released when the function return or the scope exits
for any other case.

Signed-off-by: Felipe Ribeiro de Souza <felipers@ime.usp.br>
Co-developed-by: Lucas Ivars Cadima Ciziks <lucas@ciziks.com>
Signed-off-by: Lucas Ivars Cadima Ciziks <lucas@ciziks.com>

---
v2:
  - Adjust order of #include.
  - Replace guard() in ingenic_adc_read_chan_info_raw() by scoped_guard()
    to preserve original behavior.
v3:
  - Split out ingenic_adc_read_chan_info_raw() logic to helper function.
v4:
  - Adjust code style for better readability.
---
 drivers/iio/adc/ingenic-adc.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index ad1b7bbc059a4..5622b62840e6e 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -7,6 +7,8 @@
  */
 
 #include <dt-bindings/iio/adc/ingenic,adc.h>
+
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
@@ -115,7 +117,7 @@ static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, unsigned long mask)
 {
 	struct ingenic_adc *adc = iio_priv(iio_dev);
 
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 
 	/* Init ADCMD */
 	readl(adc->base + JZ_ADC_REG_ADCMD);
@@ -162,8 +164,6 @@ static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, unsigned long mask)
 
 	/* We're done */
 	writel(0, adc->base + JZ_ADC_REG_ADCMD);
-
-	mutex_unlock(&adc->lock);
 }
 
 static void ingenic_adc_set_config(struct ingenic_adc *adc,
@@ -172,13 +172,11 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
 {
 	uint32_t cfg;
 
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 
 	cfg = readl(adc->base + JZ_ADC_REG_CFG) & ~mask;
 	cfg |= val;
 	writel(cfg, adc->base + JZ_ADC_REG_CFG);
-
-	mutex_unlock(&adc->lock);
 }
 
 static void ingenic_adc_enable_unlocked(struct ingenic_adc *adc,
@@ -201,9 +199,8 @@ static void ingenic_adc_enable(struct ingenic_adc *adc,
 			       int engine,
 			       bool enabled)
 {
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 	ingenic_adc_enable_unlocked(adc, engine, enabled);
-	mutex_unlock(&adc->lock);
 }
 
 static int ingenic_adc_capture(struct ingenic_adc *adc,
@@ -218,7 +215,7 @@ static int ingenic_adc_capture(struct ingenic_adc *adc,
 	 * probably due to the switch of VREF. We must keep the lock here to
 	 * avoid races with the buffer enable/disable functions.
 	 */
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 	cfg = readl(adc->base + JZ_ADC_REG_CFG);
 	writel(cfg & ~JZ_ADC_REG_CFG_CMD_SEL, adc->base + JZ_ADC_REG_CFG);
 
@@ -229,7 +226,6 @@ static int ingenic_adc_capture(struct ingenic_adc *adc,
 		ingenic_adc_enable_unlocked(adc, engine, false);
 
 	writel(cfg, adc->base + JZ_ADC_REG_CFG);
-	mutex_unlock(&adc->lock);
 
 	return ret;
 }
@@ -635,7 +631,7 @@ static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,
 	int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
 
 	/* We cannot sample the aux channels in parallel. */
-	mutex_lock(&adc->aux_lock);
+	guard(mutex)(&adc->aux_lock);
 
 	if (adc->soc_data->has_aux_md && engine == 0) {
 		switch (chan->channel) {
@@ -655,7 +651,7 @@ static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,
 
 	ret = ingenic_adc_capture(adc, engine);
 	if (ret)
-		goto out;
+		return ret;
 
 	switch (chan->channel) {
 	case INGENIC_ADC_AUX0:
@@ -668,11 +664,7 @@ static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,
 		break;
 	}
 
-	ret = IIO_VAL_INT;
-out:
-	mutex_unlock(&adc->aux_lock);
-
-	return ret;
+	return IIO_VAL_INT;
 }
 
 static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
-- 
2.43.0

Re: [PATCH v4 2/2] iio: adc: ingenic-adc: use guard()() to handle synchronisation

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:18:31PM -0300, Felipe Ribeiro de Souza wrote:
> Replace mutex_lock() and mutex_unlock() calls with guard()()
> in functions ingenic_adc_set_adcmd(), ingenic_adc_set_config(),
> ingenic_adc_enable(), ingenic_adc_capture() and
> ingenic_adc_read_chan_locked().
> 
> This removes the need to call the unlock function, as the lock is
> automatically released when the function return or the scope exits
> for any other case.

...

> static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,

>  	switch (chan->channel) {
>  	case INGENIC_ADC_AUX0:

>  		break;
>  	}
>  
> -	ret = IIO_VAL_INT;
> -out:
> -	mutex_unlock(&adc->aux_lock);
> -
> -	return ret;
> +	return IIO_VAL_INT;
>  }

Just a side note (it was already like this in the original code).
The switch-case has no default case and in unlikely event of code
modification or run-time flow we will return this instead of some
error code. That said, I think it's better to return directly from
each of the cases and add default case with an error code returned
(probably in a preparatory patch?). In any case it's up to you and
maintainers, since your patch doesn't change the state of affairs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/2] use guard()() to handle synchronisation

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 10:18:29PM -0300, Felipe Ribeiro de Souza wrote:
> Refactor ingenic_adc_read_chan_info_raw() and replace mutex_lock()
> and mutex_unlock() calls with guard()() in drivers/iio/adc/ingenic-adc.c

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

One side note in case you want to address it keep my tag for the existing
changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/2] use guard()() to handle synchronisation

[Jonathan Cameron]

On Fri, 24 Apr 2026 11:15:00 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Apr 22, 2026 at 10:18:29PM -0300, Felipe Ribeiro de Souza wrote:
> > Refactor ingenic_adc_read_chan_info_raw() and replace mutex_lock()
> > and mutex_unlock() calls with guard()() in drivers/iio/adc/ingenic-adc.c  
Note for future, the cover letter title needs to be more specific.


[PATCH v4 0/2] iio: adc: ingenic: use guard()() to handle synchronization

That is what it turns up as in tools like patchwork and sashiko rather than
the names of individual patches so I'd like it to be clear in what it affects.

Speaking of which Sashiko raises a valid concern with the naming.
_locked() normally means a lock is already held rather than it
won't be taken.  However, locked naming is in line with existing _unlocked()
naming in this driver. 

Now I'm not sure what best solution to this is. We don't have particularly
firm naming conventions in IIO but we probably should have.

We could move to the reasonably common variant of __xxx() means the
variant of xxx() that should be called with a lock held.  With the addition of
__must_hold() markings that should be clear.

I've been meaning to mess around with the new context analysis stuff so I had
a play.  Note that enabling it for all ADCs gives a mess so I'll need
to dig into that.  My understanding is that this stuff is prone to false
positives, but maybe some are correct or reflect missing markings.

The makefile change is not something I'd upstream yet
but if you have clang 22 or later and enable CONFIG_WARN_CONTEXT_ANALYSIS
this will turn it on for this one driver.

If you want to confirm it is working flip the __must_hold to __must_not_hold()
and you'll get something like:

drivers/iio/adc/ingenic-adc.c:202:2: warning: cannot call function '__ingenic_adc_enable' while mutex 'adc->lock' is held [-Wthread-safety-analysis]
  202 |         __ingenic_adc_enable(adc, engine, enabled);
      |         ^

which is cool as the form combines effectively documenting the locking rules
and checking them at compile time.

If other reviewers are fine with the naming convention, then a precursor
patch to change the existing _unlocked() naming + add the __must_hold()
marking then the new naming folded into patch 1 of this series.


diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 097357d146ba..a06807609bde 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -3,6 +3,7 @@
 # Makefile for IIO ADC drivers
 #
 
+CONTEXT_ANALYSIS_ingenic-adc.o=y
 obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
 
 # When adding new entries keep the list in alphabetical order
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 5622b62840e6..a4d5d139af5f 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -179,9 +179,8 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
 	writel(cfg, adc->base + JZ_ADC_REG_CFG);
 }
 
-static void ingenic_adc_enable_unlocked(struct ingenic_adc *adc,
-					int engine,
-					bool enabled)
+static void __ingenic_adc_enable(struct ingenic_adc *adc, int engine,
+				 bool enabled) __must_hold(&adc->lock)
 {
 	u8 val;
 
@@ -200,7 +199,7 @@ static void ingenic_adc_enable(struct ingenic_adc *adc,
 			       bool enabled)
 {
 	guard(mutex)(&adc->lock);
-	ingenic_adc_enable_unlocked(adc, engine, enabled);
+	__ingenic_adc_enable(adc, engine, enabled);
 }
 
 static int ingenic_adc_capture(struct ingenic_adc *adc,
@@ -219,11 +218,11 @@ static int ingenic_adc_capture(struct ingenic_adc *adc,
 	cfg = readl(adc->base + JZ_ADC_REG_CFG);
 	writel(cfg & ~JZ_ADC_REG_CFG_CMD_SEL, adc->base + JZ_ADC_REG_CFG);
 
-	ingenic_adc_enable_unlocked(adc, engine, true);
+	__ingenic_adc_enable(adc, engine, true);
 	ret = readb_poll_timeout(adc->base + JZ_ADC_REG_ENABLE, val,
 				 !(val & BIT(engine)), 250, 1000);
 	if (ret)
-		ingenic_adc_enable_unlocked(adc, engine, false);
+		__ingenic_adc_enable(adc, engine, false);
 
 	writel(cfg, adc->base + JZ_ADC_REG_CFG);
 
@@ -624,9 +623,8 @@ static int ingenic_adc_read_avail(struct iio_dev *iio_dev,
 	}
 }
 
-static int ingenic_adc_read_chan_locked(struct ingenic_adc *adc,
-					struct iio_chan_spec const *chan,
-					int *val)
+static int ingenic_adc_read_chan(struct ingenic_adc *adc,
+				 struct iio_chan_spec const *chan, int *val)
 {
 	int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
 
@@ -680,7 +678,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
 		return ret;
 	}
 
-	ret = ingenic_adc_read_chan_locked(adc, chan, val);
+	ret = ingenic_adc_read_chan(adc, chan, val);
 
 	clk_disable(adc->clk);
 



> 
> Now LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> One side note in case you want to address it keep my tag for the existing
> changes.
Separate change anyway so I'm fine with that as a follow up.


>