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

[Jonathan Cameron <jic23@kernel.org>]

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.


>