diff mbox series

[v2,1/2] i2c: imx: use guard to take spinlock

Message ID 20250427-i2c-imx-update-v2-1-d312e394b573@gmail.com
State Superseded
Headers show
Series i2c: imx: adapting the mainline | expand

Commit Message

Troy Mitchell April 27, 2025, 6:16 a.m. UTC 
Use guard to automatically release the lock after going out of scope
instead of calling it manually.

i2c_imx_slave_handle() can safely be entered with the lock held.

Refactored the i2c_imx_isr function so that i2c_imx_master_isr
does not participate in the guard scope

So Using guard(spinlock_irqsave) simplifies the control flow
by ensuring consistent and automatic unlock,
which improves readability without affecting correctness.

Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
 drivers/i2c/busses/i2c-imx.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Comments

Peng Fan April 28, 2025, 12:55 a.m. UTC | #1 
> Subject: [PATCH v2 1/2] i2c: imx: use guard to take spinlock
> 
> Use guard to automatically release the lock after going out of scope
> instead of calling it manually.
> 
> i2c_imx_slave_handle() can safely be entered with the lock held.
> 
> Refactored the i2c_imx_isr function so that i2c_imx_master_isr does
> not participate in the guard scope
> 
> So Using guard(spinlock_irqsave) simplifies the control flow by
> ensuring consistent and automatic unlock, which improves readability
> without affecting correctness.
> 
> Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
> Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9e5d454d8318..a98bf01c5dde 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -23,6 +23,7 @@
> 
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
> +#include <linux/cleanup.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> @@ -891,13 +892,13 @@ static enum hrtimer_restart
> i2c_imx_slave_timeout(struct hrtimer *t)
>  	struct imx_i2c_struct *i2c_imx = container_of(t, struct
> imx_i2c_struct,
>  						      slave_timer);
>  	unsigned int ctl, status;
> -	unsigned long flags;
> 
> -	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> +	guard(spinlock_irqsave)(&i2c_imx->slave_lock);
> +
>  	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  	i2c_imx_slave_handle(i2c_imx, status, ctl);
> -	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> +
>  	return HRTIMER_NORESTART;
>  }
> 
> @@ -1125,32 +1126,28 @@ static irqreturn_t i2c_imx_isr(int irq, void
> *dev_id)  {
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int ctl, status;
> -	unsigned long flags;
> 
> -	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> -	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> -	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	{
> +		guard(spinlock_irqsave)(&i2c_imx->slave_lock);

Use 
scoped_guard(spinlock_irqsave, & i2c_imx->slave_lock) {
 ...
}

Regards,
Peng
Troy Mitchell April 28, 2025, 4:57 a.m. UTC | #2 
On Mon, Apr 28, 2025 at 12:55:48AM +0000, Peng Fan wrote:
> > Subject: [PATCH v2 1/2] i2c: imx: use guard to take spinlock
> > 
> > Use guard to automatically release the lock after going out of scope
> > instead of calling it manually.
> > 
> > i2c_imx_slave_handle() can safely be entered with the lock held.
> > 
> > Refactored the i2c_imx_isr function so that i2c_imx_master_isr does
> > not participate in the guard scope
> > 
> > So Using guard(spinlock_irqsave) simplifies the control flow by
> > ensuring consistent and automatic unlock, which improves readability
> > without affecting correctness.
> > 
> > Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
> > Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 39 ++++++++++++++++++---------------------
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 9e5d454d8318..a98bf01c5dde 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
...
> > @@ -1125,32 +1126,28 @@ static irqreturn_t i2c_imx_isr(int irq, void
> > *dev_id)  {
> >  	struct imx_i2c_struct *i2c_imx = dev_id;
> >  	unsigned int ctl, status;
> > -	unsigned long flags;
> > 
> > -	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> > -	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > -	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > +	{
> > +		guard(spinlock_irqsave)(&i2c_imx->slave_lock);
> 
> Use 
> scoped_guard(spinlock_irqsave, & i2c_imx->slave_lock) {
>  ...
> }
>
Ok, I'll use it in the next version,
and if no one has other suggestions, I'll send v3 ASAP.
> 
> Regards,
> Peng
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9e5d454d8318..a98bf01c5dde 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -23,6 +23,7 @@ 
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
@@ -891,13 +892,13 @@  static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t)
 	struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct,
 						      slave_timer);
 	unsigned int ctl, status;
-	unsigned long flags;
 
-	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
+	guard(spinlock_irqsave)(&i2c_imx->slave_lock);
+
 	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	i2c_imx_slave_handle(i2c_imx, status, ctl);
-	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
+
 	return HRTIMER_NORESTART;
 }
 
@@ -1125,32 +1126,28 @@  static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 {
 	struct imx_i2c_struct *i2c_imx = dev_id;
 	unsigned int ctl, status;
-	unsigned long flags;
 
-	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
-	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
-	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	{
+		guard(spinlock_irqsave)(&i2c_imx->slave_lock);
+
+		status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+		ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+
+		if (!(status & I2SR_IIF))
+			return IRQ_NONE;
 
-	if (status & I2SR_IIF) {
 		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
+
 		if (i2c_imx->slave) {
-			if (!(ctl & I2CR_MSTA)) {
-				irqreturn_t ret;
-
-				ret = i2c_imx_slave_handle(i2c_imx,
-							   status, ctl);
-				spin_unlock_irqrestore(&i2c_imx->slave_lock,
-						       flags);
-				return ret;
-			}
+			if (!(ctl & I2CR_MSTA))
+				return i2c_imx_slave_handle(i2c_imx,
+							    status, ctl);
+
 			i2c_imx_slave_finish_op(i2c_imx);
 		}
-		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
-		return i2c_imx_master_isr(i2c_imx, status);
 	}
-	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
 
-	return IRQ_NONE;
+	return i2c_imx_master_isr(i2c_imx, status);
 }
 
 static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,