* Re: [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation
2026-04-16 1:18 [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation Felipe Ribeiro de Souza
@ 2026-04-16 10:08 ` Paul Cercueil
2026-04-17 5:18 ` Rong Zhang
2026-04-17 8:01 ` Andy Shevchenko
2 siblings, 0 replies; 5+ messages in thread
From: Paul Cercueil @ 2026-04-16 10:08 UTC (permalink / raw)
To: Felipe Ribeiro de Souza, jic23, dlechner, nuno.sa, andy
Cc: Lucas Ivars Cadima Ciziks, linux-mips, linux-iio
Hi Felipe,
Le mercredi 15 avril 2026 à 22:18 -0300, Felipe Ribeiro de Souza a
écrit :
> Replace mutex_lock(&lock) and mutex_unlock(&lock) calls with
> guard(mutex)(&lock) in functions ingenic_adc_set_adcmd,
> ingenic_adc_set_config, ingenic_adc_enable, ingenic_adc_capture
> and ingenic_adc_read_chan_info_raw.
>
> 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>
I see a slight behaviour change in ingenic_adc_read_chan_info_raw()
where the mutex is now unlocked after the clk_disable(). I think that's
fine though. So:
Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Cheers,
-Paul
> ---
> drivers/iio/adc/ingenic-adc.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-
> adc.c
> index 1e802c877..bb7abdcd8 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -20,6 +20,7 @@
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> +#include <linux/cleanup.h>
>
> #define JZ_ADC_REG_ENABLE 0x00
> #define JZ_ADC_REG_CFG 0x04
> @@ -115,7 +116,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 +163,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 +171,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 +198,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 +214,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 +225,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;
> }
> @@ -643,7 +638,8 @@ static int ingenic_adc_read_chan_info_raw(struct
> iio_dev *iio_dev,
> }
>
> /* We cannot sample the aux channels in parallel. */
> - mutex_lock(&adc->aux_lock);
> + guard(mutex)(&adc->lock);
> +
> if (adc->soc_data->has_aux_md && engine == 0) {
> switch (chan->channel) {
> case INGENIC_ADC_AUX0:
> @@ -677,7 +673,6 @@ static int ingenic_adc_read_chan_info_raw(struct
> iio_dev *iio_dev,
>
> ret = IIO_VAL_INT;
> out:
> - mutex_unlock(&adc->aux_lock);
> clk_disable(adc->clk);
>
> return ret;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation
2026-04-16 1:18 [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation Felipe Ribeiro de Souza
2026-04-16 10:08 ` Paul Cercueil
@ 2026-04-17 5:18 ` Rong Zhang
2026-04-17 8:01 ` Andy Shevchenko
2 siblings, 0 replies; 5+ messages in thread
From: Rong Zhang @ 2026-04-17 5:18 UTC (permalink / raw)
To: Felipe Ribeiro de Souza, paul, jic23, dlechner, nuno.sa, andy
Cc: Rong Zhang, Lucas Ivars Cadima Ciziks, linux-mips, linux-iio
Hi Felipe,
On Wed, 2026-04-15 at 22:18 -0300, Felipe Ribeiro de Souza wrote:
> Replace mutex_lock(&lock) and mutex_unlock(&lock) calls with
> guard(mutex)(&lock) in functions ingenic_adc_set_adcmd,
> ingenic_adc_set_config, ingenic_adc_enable, ingenic_adc_capture
> and ingenic_adc_read_chan_info_raw.
>
> 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>
> ---
> drivers/iio/adc/ingenic-adc.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index 1e802c877..bb7abdcd8 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -20,6 +20,7 @@
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> +#include <linux/cleanup.h>
Can you keep #include sorted?
Thanks,
Rong
>
> #define JZ_ADC_REG_ENABLE 0x00
> #define JZ_ADC_REG_CFG 0x04
> @@ -115,7 +116,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 +163,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 +171,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 +198,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 +214,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 +225,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;
> }
> @@ -643,7 +638,8 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
> }
>
> /* We cannot sample the aux channels in parallel. */
> - mutex_lock(&adc->aux_lock);
> + guard(mutex)(&adc->lock);
> +
> if (adc->soc_data->has_aux_md && engine == 0) {
> switch (chan->channel) {
> case INGENIC_ADC_AUX0:
> @@ -677,7 +673,6 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,
>
> ret = IIO_VAL_INT;
> out:
> - mutex_unlock(&adc->aux_lock);
> clk_disable(adc->clk);
>
> return ret;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation
2026-04-16 1:18 [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation Felipe Ribeiro de Souza
2026-04-16 10:08 ` Paul Cercueil
2026-04-17 5:18 ` Rong Zhang
@ 2026-04-17 8:01 ` Andy Shevchenko
2026-04-19 17:21 ` Jonathan Cameron
2 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2026-04-17 8:01 UTC (permalink / raw)
To: Felipe Ribeiro de Souza
Cc: paul, jic23, dlechner, nuno.sa, andy, Lucas Ivars Cadima Ciziks,
linux-mips, linux-iio
On Wed, Apr 15, 2026 at 10:18:11PM -0300, Felipe Ribeiro de Souza wrote:
> Replace mutex_lock(&lock) and mutex_unlock(&lock) calls with
> guard(mutex)(&lock) in functions ingenic_adc_set_adcmd,
> ingenic_adc_set_config, ingenic_adc_enable, ingenic_adc_capture
> and ingenic_adc_read_chan_info_raw.
We refer to the functions as func().
> 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.
...
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> +#include <linux/cleanup.h>
Preserve ordering.
...
> /* We cannot sample the aux channels in parallel. */
> - mutex_lock(&adc->aux_lock);
> + guard(mutex)(&adc->lock);
...
> ret = IIO_VAL_INT;
> out:
> - mutex_unlock(&adc->aux_lock);
> clk_disable(adc->clk);
Not sure about this. At bare minimum you should elaborate on this change in the
commit message.
> return ret;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: adc: ingenic-adc: use guard(mutex)(&lock) to handle synchronisation
2026-04-17 8:01 ` Andy Shevchenko
@ 2026-04-19 17:21 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2026-04-19 17:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Felipe Ribeiro de Souza, paul, dlechner, nuno.sa, andy,
Lucas Ivars Cadima Ciziks, linux-mips, linux-iio
On Fri, 17 Apr 2026 11:01:48 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Wed, Apr 15, 2026 at 10:18:11PM -0300, Felipe Ribeiro de Souza wrote:
> > Replace mutex_lock(&lock) and mutex_unlock(&lock) calls with
> > guard(mutex)(&lock) in functions ingenic_adc_set_adcmd,
> > ingenic_adc_set_config, ingenic_adc_enable, ingenic_adc_capture
> > and ingenic_adc_read_chan_info_raw.
>
> We refer to the functions as func().
>
> > 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.
>
> ...
>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > #include <linux/property.h>
> > +#include <linux/cleanup.h>
>
> Preserve ordering.
>
> ...
>
> > /* We cannot sample the aux channels in parallel. */
> > - mutex_lock(&adc->aux_lock);
> > + guard(mutex)(&adc->lock);
>
> ...
>
> > ret = IIO_VAL_INT;
> > out:
Holding the lock a bit longer might be fine, but please go read the
comments in cleanup.h. The short version is don't combine use
of guard() with any gotos in the same function.
It's too high risk given different compiler behaviour and the chance of
code getting reorganized in the future
I wonder if anyone would be interested in applying ACQUIRE() and ACQUIRE_ERR()
to clock management. Might be worth seeing if that is of interest and general
enough as then you could handle the clk_disable() as well.
> > - mutex_unlock(&adc->aux_lock);
> > clk_disable(adc->clk);
>
> Not sure about this. At bare minimum you should elaborate on this change in the
> commit message.
>
> > return ret;
>
^ permalink raw reply [flat|nested] 5+ messages in thread