linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] i2c: imx: adapting the mainline
@ 2025-04-21  5:36 Troy Mitchell
  2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
  2025-04-21  5:36 ` [PATCH 2/2] i2c: imx: drop master prefix Troy Mitchell
  0 siblings, 2 replies; 10+ messages in thread
From: Troy Mitchell @ 2025-04-21  5:36 UTC (permalink / raw)
  To: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
	Sascha Hauer, Fabio Estevam
  Cc: linux-i2c, imx, linux-arm-kernel, linux-kernel, Troy Mitchell,
	Yongchao Jia

Since this patch[1], we have new callback function names.
Since this patch[2], we can use `guard` to call `spin_lock_irqsave`
and release this lock when it goes out of scope.

Link:
https://lore.kernel.org/all/20240706112116.24543-2-wsa+renesas@sang-engineering.com/ [1]
https://lore.kernel.org/all/20250227221924.265259-10-lyude@redhat.com/ [2]

Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
Troy Mitchell (2):
      i2c: imx: use guard to take spinlock
      i2c: imx: drop master prefix

 drivers/i2c/busses/i2c-imx.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
---
base-commit: 9d7a0577c9db35c4cc52db90bc415ea248446472
change-id: 20250421-i2c-imx-update-d11d66dd87e8

Best regards,
-- 
Troy Mitchell <troymitchell988@gmail.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-21  5:36 [PATCH 0/2] i2c: imx: adapting the mainline Troy Mitchell
@ 2025-04-21  5:36 ` Troy Mitchell
  2025-04-21  7:28   ` kernel test robot
                     ` (2 more replies)
  2025-04-21  5:36 ` [PATCH 2/2] i2c: imx: drop master prefix Troy Mitchell
  1 sibling, 3 replies; 10+ messages in thread
From: Troy Mitchell @ 2025-04-21  5:36 UTC (permalink / raw)
  To: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
	Sascha Hauer, Fabio Estevam
  Cc: linux-i2c, imx, linux-arm-kernel, linux-kernel, Troy Mitchell,
	Yongchao Jia

Use guard to automatically release the lock after going out of scope
instead of calling it manually.

Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
 drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9e5d454d8318..cb96a57df4a0 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,30 +1126,27 @@ 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);
+	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) {
 		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;
+				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;
 }

-- 
2.34.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] i2c: imx: drop master prefix
  2025-04-21  5:36 [PATCH 0/2] i2c: imx: adapting the mainline Troy Mitchell
  2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
@ 2025-04-21  5:36 ` Troy Mitchell
  2025-04-22 16:10   ` Ahmad Fatoum
  1 sibling, 1 reply; 10+ messages in thread
From: Troy Mitchell @ 2025-04-21  5:36 UTC (permalink / raw)
  To: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
	Sascha Hauer, Fabio Estevam
  Cc: linux-i2c, imx, linux-arm-kernel, linux-kernel, Troy Mitchell,
	Yongchao Jia

In light of the recent updates to the i2c subsystem,
drop master prefix.

Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
 drivers/i2c/busses/i2c-imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index cb96a57df4a0..dd07fde79632 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1690,8 +1690,8 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
 }
 
 static const struct i2c_algorithm i2c_imx_algo = {
-	.master_xfer = i2c_imx_xfer,
-	.master_xfer_atomic = i2c_imx_xfer_atomic,
+	.xfer = i2c_imx_xfer,
+	.xfer_atomic = i2c_imx_xfer_atomic,
 	.functionality = i2c_imx_func,
 	.reg_slave	= i2c_imx_reg_slave,
 	.unreg_slave	= i2c_imx_unreg_slave,

-- 
2.34.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
@ 2025-04-21  7:28   ` kernel test robot
  2025-04-21 18:05   ` Frank Li
  2025-04-22 15:12   ` Jonathan Cameron
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-04-21  7:28 UTC (permalink / raw)
  To: Troy Mitchell, Oleksij Rempel, Pengutronix Kernel Team,
	Andi Shyti, Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: llvm, oe-kbuild-all, linux-i2c, imx, linux-arm-kernel,
	linux-kernel, Troy Mitchell, Yongchao Jia

Hi Troy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9d7a0577c9db35c4cc52db90bc415ea248446472]

url:    https://github.com/intel-lab-lkp/linux/commits/Troy-Mitchell/i2c-imx-use-guard-to-take-spinlock/20250421-133753
base:   9d7a0577c9db35c4cc52db90bc415ea248446472
patch link:    https://lore.kernel.org/r/20250421-i2c-imx-update-v1-1-1137f1f353d5%40gmail.com
patch subject: [PATCH 1/2] i2c: imx: use guard to take spinlock
config: hexagon-randconfig-001-20250421 (https://download.01.org/0day-ci/archive/20250421/202504211501.MNwGdl5F-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250421/202504211501.MNwGdl5F-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504211501.MNwGdl5F-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-imx.c:1140:17: warning: unused variable 'ret' [-Wunused-variable]
                                   irqreturn_t ret;
                                               ^
   1 warning generated.


vim +/ret +1140 drivers/i2c/busses/i2c-imx.c

aa11e38ce6fe88 Darius Augulis     2009-01-30  1124  
f7414cd6923fd7 Biwen Li           2020-11-11  1125  static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
f7414cd6923fd7 Biwen Li           2020-11-11  1126  {
f7414cd6923fd7 Biwen Li           2020-11-11  1127  	struct imx_i2c_struct *i2c_imx = dev_id;
f7414cd6923fd7 Biwen Li           2020-11-11  1128  	unsigned int ctl, status;
f7414cd6923fd7 Biwen Li           2020-11-11  1129  
d3973a577b8e10 Troy Mitchell      2025-04-21  1130  	guard(spinlock_irqsave)(&i2c_imx->slave_lock);
d3973a577b8e10 Troy Mitchell      2025-04-21  1131  
f7414cd6923fd7 Biwen Li           2020-11-11  1132  	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
f7414cd6923fd7 Biwen Li           2020-11-11  1133  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22  1134  
f7414cd6923fd7 Biwen Li           2020-11-11  1135  	if (status & I2SR_IIF) {
f7414cd6923fd7 Biwen Li           2020-11-11  1136  		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
d3973a577b8e10 Troy Mitchell      2025-04-21  1137  
05ae60bc24f765 Kevin Paul Herbert 2020-12-22  1138  		if (i2c_imx->slave) {
05ae60bc24f765 Kevin Paul Herbert 2020-12-22  1139  			if (!(ctl & I2CR_MSTA)) {
f89bf95632b416 Corey Minyard      2021-11-12 @1140  				irqreturn_t ret;
f89bf95632b416 Corey Minyard      2021-11-12  1141  
d3973a577b8e10 Troy Mitchell      2025-04-21  1142  				return i2c_imx_slave_handle(i2c_imx,
f89bf95632b416 Corey Minyard      2021-11-12  1143  							    status, ctl);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22  1144  			}
f89bf95632b416 Corey Minyard      2021-11-12  1145  			i2c_imx_slave_finish_op(i2c_imx);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22  1146  		}
d3973a577b8e10 Troy Mitchell      2025-04-21  1147  
f7414cd6923fd7 Biwen Li           2020-11-11  1148  		return i2c_imx_master_isr(i2c_imx, status);
f7414cd6923fd7 Biwen Li           2020-11-11  1149  	}
f7414cd6923fd7 Biwen Li           2020-11-11  1150  
aa11e38ce6fe88 Darius Augulis     2009-01-30  1151  	return IRQ_NONE;
aa11e38ce6fe88 Darius Augulis     2009-01-30  1152  }
aa11e38ce6fe88 Darius Augulis     2009-01-30  1153  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
  2025-04-21  7:28   ` kernel test robot
@ 2025-04-21 18:05   ` Frank Li
  2025-04-22 15:12   ` Jonathan Cameron
  2 siblings, 0 replies; 10+ messages in thread
From: Frank Li @ 2025-04-21 18:05 UTC (permalink / raw)
  To: Troy Mitchell
  Cc: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
	Sascha Hauer, Fabio Estevam, linux-i2c, imx, linux-arm-kernel,
	linux-kernel, Yongchao Jia

On Mon, Apr 21, 2025 at 01:36:38PM +0800, Troy Mitchell wrote:
> Use guard to automatically release the lock after going out of scope
> instead of calling it manually.
>
> Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
> Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9e5d454d8318..cb96a57df4a0 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,30 +1126,27 @@ 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);
> +	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) {
>  		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;
> +				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;
>  }
>
> --
> 2.34.1
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
  2025-04-21  7:28   ` kernel test robot
  2025-04-21 18:05   ` Frank Li
@ 2025-04-22 15:12   ` Jonathan Cameron
  2025-04-22 15:26     ` Troy Mitchell
  2 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2025-04-22 15:12 UTC (permalink / raw)
  To: Troy Mitchell
  Cc: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
	Sascha Hauer, Fabio Estevam, linux-i2c, imx, linux-arm-kernel,
	linux-kernel, Yongchao Jia

On Mon, 21 Apr 2025 13:36:38 +0800
Troy Mitchell <troymitchell988@gmail.com> wrote:

> Use guard to automatically release the lock after going out of scope
> instead of calling it manually.

Drive by review, but this changes behavior in a subtle way so we
should have more commentary here...

> 
> Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
> Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9e5d454d8318..cb96a57df4a0 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>

>  
> @@ -1125,30 +1126,27 @@ 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);
> +	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) {
>  		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;
> +				return i2c_imx_slave_handle(i2c_imx,
> +							    status, ctl);
>  			}
>  			i2c_imx_slave_finish_op(i2c_imx);
>  		}
> -		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);

In this path the patch changes the lock release to occur after
i2c_imx_master_isr(i2c_imx, status);

That may well be safe; I have no idea!  You should talk about that
in the patch description if it is.

> +
>  		return i2c_imx_master_isr(i2c_imx, status);
>  	}
> -	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>  
>  	return IRQ_NONE;
>  }
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-22 15:12   ` Jonathan Cameron
@ 2025-04-22 15:26     ` Troy Mitchell
  2025-04-22 15:53       ` Ahmad Fatoum
  0 siblings, 1 reply; 10+ messages in thread
From: Troy Mitchell @ 2025-04-22 15:26 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: troymitchell988, Oleksij Rempel, Pengutronix Kernel Team,
	Andi Shyti, Shawn Guo, Sascha Hauer, Fabio Estevam, linux-i2c,
	imx, linux-arm-kernel, linux-kernel, Yongchao Jia

On 2025/4/22 23:12, Jonathan Cameron wrote:
> On Mon, 21 Apr 2025 13:36:38 +0800
> Troy Mitchell <troymitchell988@gmail.com> wrote:
> 
>> Use guard to automatically release the lock after going out of scope
>> instead of calling it manually.
> Drive by review, but this changes behavior in a subtle way so we
> should have more commentary here...

Thanks for your review!

> 
>> Co-developed-by: Yongchao Jia <jyc0019@gmail.com>
>> Signed-off-by: Yongchao Jia <jyc0019@gmail.com>
>> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index 9e5d454d8318..cb96a57df4a0 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>
>>  
>> @@ -1125,30 +1126,27 @@ 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);
>> +	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) {
>>  		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;
>> +				return i2c_imx_slave_handle(i2c_imx,
>> +							    status, ctl);
>>  			}
>>  			i2c_imx_slave_finish_op(i2c_imx);
>>  		}
>> -		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> In this path the patch changes the lock release to occur after
> i2c_imx_master_isr(i2c_imx, status);
> 
> That may well be safe; I have no idea!  You should talk about that
> in the patch description if it is.
You're correct that this change slightly alters the lock release timing.

However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be
entered with the lock held — there's no requirement for the lock to be released
before calling them.

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

I'll update the commit message to clarify this.

> 
>> +
>>  		return i2c_imx_master_isr(i2c_imx, status);
>>  	}
>> -	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>>  
>>  	return IRQ_NONE;
>>  }

-- 
Troy Mitchell



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-22 15:26     ` Troy Mitchell
@ 2025-04-22 15:53       ` Ahmad Fatoum
  2025-04-22 16:05         ` Ahmad Fatoum
  0 siblings, 1 reply; 10+ messages in thread
From: Ahmad Fatoum @ 2025-04-22 15:53 UTC (permalink / raw)
  To: Troy Mitchell, Jonathan Cameron
  Cc: imx, Andi Shyti, Fabio Estevam, Sascha Hauer, linux-kernel,
	Oleksij Rempel, linux-i2c, Pengutronix Kernel Team, Yongchao Jia,
	Shawn Guo, linux-arm-kernel

Hello Troy,

On 4/22/25 17:26, Troy Mitchell wrote:
> On 2025/4/22 23:12, Jonathan Cameron wrote:
>> On Mon, 21 Apr 2025 13:36:38 +0800

[snip]

>> In this path the patch changes the lock release to occur after
>> i2c_imx_master_isr(i2c_imx, status);
>>
>> That may well be safe; I have no idea!  You should talk about that
>> in the patch description if it is.
> You're correct that this change slightly alters the lock release timing.
> 
> However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be
> entered with the lock held — there's no requirement for the lock to be released
> before calling them.

Why would we want to hold a spinlock out of all things longer than
absolutely necessary?

> Using guard(spinlock_irqsave) simplifies the control flow by ensuring consistent
> and automatic unlock, which improves readability without affecting correctness.
> 
> I'll update the commit message to clarify this.

Sorry, I don't think that this simplification is worth potentially
increasing the time we spend with preemption disabled,
i2c_imx_master_isr() even has a loop inside.

Guards that don't increase critical section size however are fine by me.

Thanks,
Ahmad

> 
>>
>>> +
>>>  		return i2c_imx_master_isr(i2c_imx, status);
>>>  	}
>>> -	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>>>  
>>>  	return IRQ_NONE;
>>>  }
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] i2c: imx: use guard to take spinlock
  2025-04-22 15:53       ` Ahmad Fatoum
@ 2025-04-22 16:05         ` Ahmad Fatoum
  0 siblings, 0 replies; 10+ messages in thread
From: Ahmad Fatoum @ 2025-04-22 16:05 UTC (permalink / raw)
  To: Troy Mitchell, Jonathan Cameron
  Cc: linux-arm-kernel, imx, Andi Shyti, Shawn Guo, Sascha Hauer,
	linux-kernel, Oleksij Rempel, linux-i2c, Pengutronix Kernel Team,
	Fabio Estevam, Yongchao Jia

On 4/22/25 17:53, Ahmad Fatoum wrote:
> Hello Troy,
> 
> On 4/22/25 17:26, Troy Mitchell wrote:
>> On 2025/4/22 23:12, Jonathan Cameron wrote:
>>> On Mon, 21 Apr 2025 13:36:38 +0800
> 
> [snip]
> 
>>> In this path the patch changes the lock release to occur after
>>> i2c_imx_master_isr(i2c_imx, status);
>>>
>>> That may well be safe; I have no idea!  You should talk about that
>>> in the patch description if it is.
>> You're correct that this change slightly alters the lock release timing.
>>
>> However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be
>> entered with the lock held — there's no requirement for the lock to be released
>> before calling them.
> 
> Why would we want to hold a spinlock out of all things longer than
> absolutely necessary?
> 
>> Using guard(spinlock_irqsave) simplifies the control flow by ensuring consistent
>> and automatic unlock, which improves readability without affecting correctness.
>>
>> I'll update the commit message to clarify this.
> 
> Sorry, I don't think that this simplification is worth potentially
> increasing the time we spend with preemption disabled,
> i2c_imx_master_isr() even has a loop inside.

Ah the loop is in lpi2c_imx_master_isr, sorry about the confusion.
I still think that the simplification isn't worth increasing critical
section size.

> Guards that don't increase critical section size however are fine by me.

Thanks,
Ahmad

> 
> Thanks,
> Ahmad
> 
>>
>>>
>>>> +
>>>>  		return i2c_imx_master_isr(i2c_imx, status);
>>>>  	}
>>>> -	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>>>>  
>>>>  	return IRQ_NONE;
>>>>  }
>>
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] i2c: imx: drop master prefix
  2025-04-21  5:36 ` [PATCH 2/2] i2c: imx: drop master prefix Troy Mitchell
@ 2025-04-22 16:10   ` Ahmad Fatoum
  0 siblings, 0 replies; 10+ messages in thread
From: Ahmad Fatoum @ 2025-04-22 16:10 UTC (permalink / raw)
  To: Troy Mitchell, Oleksij Rempel, Pengutronix Kernel Team,
	Andi Shyti, Shawn Guo, Sascha Hauer, Fabio Estevam
  Cc: linux-arm-kernel, imx, linux-kernel, linux-i2c, Yongchao Jia

On 4/21/25 07:36, Troy Mitchell wrote:
> In light of the recent updates to the i2c subsystem,
> drop master prefix.
> 
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Please use a different cover letter title for your v2, adapting the
mainline doesn't tell the reader anything about what the series is about.

Perhaps something like i2c: imx: lock guards and cleanup

Thanks,
Ahmad

> ---
>  drivers/i2c/busses/i2c-imx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cb96a57df4a0..dd07fde79632 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1690,8 +1690,8 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
>  }
>  
>  static const struct i2c_algorithm i2c_imx_algo = {
> -	.master_xfer = i2c_imx_xfer,
> -	.master_xfer_atomic = i2c_imx_xfer_atomic,
> +	.xfer = i2c_imx_xfer,
> +	.xfer_atomic = i2c_imx_xfer_atomic,
>  	.functionality = i2c_imx_func,
>  	.reg_slave	= i2c_imx_reg_slave,
>  	.unreg_slave	= i2c_imx_unreg_slave,
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-04-22 19:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-21  5:36 [PATCH 0/2] i2c: imx: adapting the mainline Troy Mitchell
2025-04-21  5:36 ` [PATCH 1/2] i2c: imx: use guard to take spinlock Troy Mitchell
2025-04-21  7:28   ` kernel test robot
2025-04-21 18:05   ` Frank Li
2025-04-22 15:12   ` Jonathan Cameron
2025-04-22 15:26     ` Troy Mitchell
2025-04-22 15:53       ` Ahmad Fatoum
2025-04-22 16:05         ` Ahmad Fatoum
2025-04-21  5:36 ` [PATCH 2/2] i2c: imx: drop master prefix Troy Mitchell
2025-04-22 16:10   ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).