* [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).