Message ID | 20250427-i2c-imx-update-v2-1-d312e394b573@gmail.com |
---|---|
State | New |
Headers | show |
Series | i2c: imx: adapting the mainline | expand |
> 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
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 --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,