drivers/i2c/busses/i2c-k1.c | 206 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 161 insertions(+), 45 deletions(-)
This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
enabling the use of I2C in atomic context.
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changes in v3:
- drop 1-5 patches (have been merged)
- modify commit message
- use readl_poll_timeout_atomic() in wait_pio_xfer()
- use msecs_to_jiffies() when get PIO mode timeout value
- factor out transfer state handling into spacemit_i2c_handle_state().
- do not disable/enable the controller IRQ around PIO transfers.
- consolidate spacemit_i2c_init() interrupt setup
- rename is_pio -> use_pio
- rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
- rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
- rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
- call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
- Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
Changes in v2:
- Patch 1/6:
Patch 3/6:
Patch 4/6:
- nothing
- Patch 2/6:
- remove is_pio judgement in irq_handler()
- Patch 5/6:
- fix wrong comment
- In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
return directly instead of using break.
- Patch 6/6:
- add is_pio judgement in irq_handler()
- use a fixed timeout value when PIO
- use readl_poll_timeout() in spacemit_i2c_wait_bus_idle() when PIO
- Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
---
drivers/i2c/busses/i2c-k1.c | 206 ++++++++++++++++++++++++++++++++++----------
1 file changed, 161 insertions(+), 45 deletions(-)
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 6b918770e612e098b8ad17418f420d87c94df166..21c5dd56e71f5e59434767329fb84d50d5c04178 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -97,6 +97,9 @@
#define SPACEMIT_BUS_RESET_CLK_CNT_MAX 9
+/* Constants */
+#define SPACEMIT_WAIT_TIMEOUT 1000 /* ms */
+
enum spacemit_i2c_state {
SPACEMIT_STATE_IDLE,
SPACEMIT_STATE_START,
@@ -125,6 +128,7 @@ struct spacemit_i2c_dev {
enum spacemit_i2c_state state;
bool read;
+ bool use_pio;
struct completion complete;
u32 status;
};
@@ -206,9 +210,14 @@ static int spacemit_i2c_wait_bus_idle(struct spacemit_i2c_dev *i2c)
if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)))
return 0;
- ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
- val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
- 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
+ if (!i2c->use_pio)
+ ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR,
+ val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
+ 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
+ else
+ ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
+ val, !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
+ 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
if (ret)
spacemit_i2c_reset(i2c);
@@ -226,25 +235,30 @@ static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
{
- u32 val;
-
- /*
- * Unmask interrupt bits for all xfer mode:
- * bus error, arbitration loss detected.
- * For transaction complete signal, we use master stop
- * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
- */
- val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
-
- /*
- * Unmask interrupt bits for interrupt xfer mode:
- * When IDBR receives a byte, an interrupt is triggered.
- *
- * For the tx empty interrupt, it will be enabled in the
- * i2c_start function.
- * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
- */
- val |= SPACEMIT_CR_DRFIE;
+ u32 val = 0;
+
+ if (!i2c->use_pio) {
+ /*
+ * Unmask interrupt bits for all xfer mode:
+ * bus error, arbitration loss detected.
+ * For transaction complete signal, we use master stop
+ * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
+ */
+ val |= SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
+
+ /*
+ * Unmask interrupt bits for interrupt xfer mode:
+ * When IDBR receives a byte, an interrupt is triggered.
+ *
+ * For the tx empty interrupt, it will be enabled in the
+ * i2c_start function.
+ * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
+ */
+ val |= SPACEMIT_CR_DRFIE;
+
+ /* unmask master stop interrupt bit */
+ val |= SPACEMIT_CR_MSDIE;
+ }
if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
val |= SPACEMIT_CR_MODE_FAST;
@@ -256,7 +270,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
val |= SPACEMIT_CR_SCLE;
/* enable master stop detected */
- val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
+ val |= SPACEMIT_CR_MSDE;
writel(val, i2c->base + SPACEMIT_ICR);
@@ -293,10 +307,62 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
/* send start pulse */
val = readl(i2c->base + SPACEMIT_ICR);
val &= ~SPACEMIT_CR_STOP;
- val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
+ val |= SPACEMIT_CR_START | SPACEMIT_CR_TB;
+
+ if (!i2c->use_pio)
+ val |= SPACEMIT_CR_DTEIE;
+
writel(val, i2c->base + SPACEMIT_ICR);
}
+static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
+static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
+{
+ u32 msec = jiffies_to_msecs(i2c->adapt.timeout);
+ ktime_t timeout = ktime_add_ms(ktime_get(), msec);
+ int ret;
+
+ while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
+ udelay(10);
+ i2c->status = readl(i2c->base + SPACEMIT_ISR);
+
+ spacemit_i2c_clear_int_status(i2c, i2c->status);
+
+ if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
+ continue;
+
+ spacemit_i2c_handle_state(i2c);
+
+ /*
+ * This is the last byte to write of the current message.
+ * If we do not wait here, control will proceed directly to start(),
+ * which would overwrite the current data.
+ */
+ if (!i2c->read && !i2c->unprocessed) {
+ ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
+ i2c->status, i2c->status & SPACEMIT_SR_ITE,
+ 30, 1000);
+ if (ret)
+ return 0;
+ /*
+ * In read mode, err_check() in handle_state properly handles what happens
+ * after the MSD bit is set. For writes it is different: in interrupt mode,
+ * an ITE (write-empty) interrupt is triggered after the last byte, and the
+ * MSD-related handling takes place there. In PIO mode, however, we need to
+ * explicitly call err_check() to emulate this step, otherwise the next
+ * transfer will fail.
+ */
+ spacemit_i2c_err_check(i2c);
+ }
+ }
+
+ if (i2c->unprocessed)
+ return 0;
+
+ return 1;
+}
+
static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
{
unsigned long time_left;
@@ -310,10 +376,16 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
reinit_completion(&i2c->complete);
- spacemit_i2c_start(i2c);
+ if (i2c->use_pio) {
+ spacemit_i2c_start(i2c);
+
+ time_left = spacemit_i2c_wait_pio_xfer(i2c);
+ } else {
+ spacemit_i2c_start(i2c);
+ time_left = wait_for_completion_timeout(&i2c->complete,
+ i2c->adapt.timeout);
+ }
- time_left = wait_for_completion_timeout(&i2c->complete,
- i2c->adapt.timeout);
if (!time_left) {
dev_err(i2c->dev, "msg completion timeout\n");
spacemit_i2c_conditionally_reset_bus(i2c);
@@ -341,6 +413,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
{
+ if (!(i2c->status & SPACEMIT_SR_ITE))
+ return;
+
/* if transfer completes, SPACEMIT_ISR will handle it */
if (i2c->status & SPACEMIT_SR_MSD)
return;
@@ -353,14 +428,20 @@ static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
/* SPACEMIT_STATE_IDLE avoids trigger next byte */
i2c->state = SPACEMIT_STATE_IDLE;
- complete(&i2c->complete);
+
+ if (!i2c->use_pio)
+ complete(&i2c->complete);
}
static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
{
+ if (!(i2c->status & SPACEMIT_SR_IRF))
+ return;
+
if (i2c->unprocessed) {
*i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR);
i2c->unprocessed--;
+ return;
}
/* if transfer completes, SPACEMIT_ISR will handle it */
@@ -373,7 +454,9 @@ static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
/* SPACEMIT_STATE_IDLE avoids trigger next byte */
i2c->state = SPACEMIT_STATE_IDLE;
- complete(&i2c->complete);
+
+ if (!i2c->use_pio)
+ complete(&i2c->complete);
}
static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
@@ -408,21 +491,14 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
i2c->state = SPACEMIT_STATE_IDLE;
- complete(&i2c->complete);
+
+ if (!i2c->use_pio)
+ complete(&i2c->complete);
}
-static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c)
{
- struct spacemit_i2c_dev *i2c = devid;
- u32 status, val;
-
- status = readl(i2c->base + SPACEMIT_ISR);
- if (!status)
- return IRQ_HANDLED;
-
- i2c->status = status;
-
- spacemit_i2c_clear_int_status(i2c, status);
+ u32 val;
if (i2c->status & SPACEMIT_SR_ERR)
goto err_out;
@@ -445,7 +521,10 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
}
if (i2c->state != SPACEMIT_STATE_IDLE) {
- val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
+ val |= SPACEMIT_CR_TB;
+ if (i2c->use_pio)
+ val |= SPACEMIT_CR_ALDIE;
+
if (spacemit_i2c_is_last_msg(i2c)) {
/* trigger next byte with stop */
@@ -459,6 +538,23 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
err_out:
spacemit_i2c_err_check(i2c);
+}
+
+static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+{
+ struct spacemit_i2c_dev *i2c = devid;
+ u32 status;
+
+ status = readl(i2c->base + SPACEMIT_ISR);
+ if (!status)
+ return IRQ_HANDLED;
+
+ i2c->status = status;
+
+ spacemit_i2c_clear_int_status(i2c, status);
+
+ spacemit_i2c_handle_state(i2c);
+
return IRQ_HANDLED;
}
@@ -479,20 +575,29 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
}
-static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+static inline int
+spacemit_i2c_xfer_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
{
struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
int ret;
+ i2c->use_pio = use_pio;
+
i2c->msgs = msgs;
i2c->msg_num = num;
- spacemit_i2c_calc_timeout(i2c);
+ if (!i2c->use_pio)
+ spacemit_i2c_calc_timeout(i2c);
+ else
+ i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
spacemit_i2c_init(i2c);
spacemit_i2c_enable(i2c);
+ /* To avoid being affected by the state of the previous transfer */
+ spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
+
ret = spacemit_i2c_wait_bus_idle(i2c);
if (!ret) {
ret = spacemit_i2c_xfer_msg(i2c);
@@ -506,11 +611,21 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
if (ret == -ETIMEDOUT || ret == -EAGAIN)
dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
- ret, i2c->status & SPACEMIT_SR_ERR);
+ ret, i2c->status & SPACEMIT_SR_ERR);
return ret < 0 ? ret : num;
}
+static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+ return spacemit_i2c_xfer_common(adapt, msgs, num, false);
+}
+
+static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
+{
+ return spacemit_i2c_xfer_common(adapt, msgs, num, true);
+}
+
static u32 spacemit_i2c_func(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
@@ -518,6 +633,7 @@ static u32 spacemit_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm spacemit_i2c_algo = {
.xfer = spacemit_i2c_xfer,
+ .xfer_atomic = spacemit_i2c_pio_xfer_atomic,
.functionality = spacemit_i2c_func,
};
---
base-commit: bc574b64121525b24d52e9bab747184181c808dc
change-id: 20250814-k1-i2c-atomic-f1a90cd34364
Best regards,
--
Troy Mitchell <troy.mitchell@linux.spacemit.com>Hi Troy and I2C maintainer,
one thing I'd raise and seek I2C maintainer's suggestion, should this
driver introduce lock to protect multi context - non-atomic vs atomic vs
interrupt context, more than one i2c clients contend single controller?
On 14:07 Mon 29 Sep , Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C in atomic context.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changes in v3:
> - drop 1-5 patches (have been merged)
> - modify commit message
> - use readl_poll_timeout_atomic() in wait_pio_xfer()
> - use msecs_to_jiffies() when get PIO mode timeout value
> - factor out transfer state handling into spacemit_i2c_handle_state().
> - do not disable/enable the controller IRQ around PIO transfers.
> - consolidate spacemit_i2c_init() interrupt setup
> - rename is_pio -> use_pio
> - rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
> - rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
> - rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
> - call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
> - Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
>
> Changes in v2:
> - Patch 1/6:
> Patch 3/6:
> Patch 4/6:
> - nothing
> - Patch 2/6:
> - remove is_pio judgement in irq_handler()
> - Patch 5/6:
> - fix wrong comment
> - In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
> return directly instead of using break.
> - Patch 6/6:
> - add is_pio judgement in irq_handler()
> - use a fixed timeout value when PIO
> - use readl_poll_timeout() in spacemit_i2c_wait_bus_idle() when PIO
>
> - Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
> ---
> drivers/i2c/busses/i2c-k1.c | 206 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 161 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..21c5dd56e71f5e59434767329fb84d50d5c04178 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,9 @@
>
> #define SPACEMIT_BUS_RESET_CLK_CNT_MAX 9
>
[snip]..
> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c);
> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c);
please sort to avoid this kind declaration, if it's inevitable,
you can move to top, before the function, after macro
> +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> +{
> + u32 msec = jiffies_to_msecs(i2c->adapt.timeout);
> + ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> + int ret;
> +
> + while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> + udelay(10);
> + i2c->status = readl(i2c->base + SPACEMIT_ISR);
> +
> + spacemit_i2c_clear_int_status(i2c, i2c->status);
> +
> + if (!(i2c->status & SPACEMIT_SR_IRF) && !(i2c->status & SPACEMIT_SR_ITE))
this is hard to read
mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;
if ((i2c->status & mask) == 0)
..
> + continue;
> +
> + spacemit_i2c_handle_state(i2c);
> +
> + /*
> + * This is the last byte to write of the current message.
> + * If we do not wait here, control will proceed directly to start(),
> + * which would overwrite the current data.
> + */
> + if (!i2c->read && !i2c->unprocessed) {
> + ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> + i2c->status, i2c->status & SPACEMIT_SR_ITE,
> + 30, 1000);
> + if (ret)
> + return 0;
> + /*
> + * In read mode, err_check() in handle_state properly handles what happens
> + * after the MSD bit is set. For writes it is different: in interrupt mode,
> + * an ITE (write-empty) interrupt is triggered after the last byte, and the
> + * MSD-related handling takes place there. In PIO mode, however, we need to
> + * explicitly call err_check() to emulate this step, otherwise the next
> + * transfer will fail.
> + */
> + spacemit_i2c_err_check(i2c);
> + }
> + }
> +
> + if (i2c->unprocessed)
> + return 0;
> +
> + return 1;
> +}
> +
> static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> {
> unsigned long time_left;
> @@ -310,10 +376,16 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>
> reinit_completion(&i2c->complete);
>
> - spacemit_i2c_start(i2c);
..
> + if (i2c->use_pio) {
> + spacemit_i2c_start(i2c);
> +
> + time_left = spacemit_i2c_wait_pio_xfer(i2c);
> + } else {
> + spacemit_i2c_start(i2c);
> + time_left = wait_for_completion_timeout(&i2c->complete,
> + i2c->adapt.timeout);
> + }
suggest to distill a common function spacemit_i2c_wait_xfer_complete()
and converge above if/elselogic inside, which is more readable
>
> - time_left = wait_for_completion_timeout(&i2c->complete,
> - i2c->adapt.timeout);
> if (!time_left) {
> dev_err(i2c->dev, "msg completion timeout\n");
..
> spacemit_i2c_conditionally_reset_bus(i2c);
btw, not introduced in this patch, spacemit_i2c_reset() followed unconditionally after
spacemit_i2c_conditionally_reset_bus() which is very confusing
please double check the logic
> @@ -341,6 +413,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
>
> static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> {
> + if (!(i2c->status & SPACEMIT_SR_ITE))
> + return;
> +
> /* if transfer completes, SPACEMIT_ISR will handle it */
> if (i2c->status & SPACEMIT_SR_MSD)
> return;
[snip]..
>
> -static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +static inline int
> +spacemit_i2c_xfer_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
> {
> struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> int ret;
>
> + i2c->use_pio = use_pio;
> +
> i2c->msgs = msgs;
> i2c->msg_num = num;
>
..
> - spacemit_i2c_calc_timeout(i2c);
> + if (!i2c->use_pio)
> + spacemit_i2c_calc_timeout(i2c);
> + else
> + i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
suggest merge "if else" into spacemit_i2c_calc_timeout(), same as previous suggestion
>
> spacemit_i2c_init(i2c);
>
> spacemit_i2c_enable(i2c);
>
> + /* To avoid being affected by the state of the previous transfer */
> + spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> +
I've not see my previous comment has been addressed!
> ret = spacemit_i2c_wait_bus_idle(i2c);
> if (!ret) {
> ret = spacemit_i2c_xfer_msg(i2c);
> @@ -506,11 +611,21 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
>
> if (ret == -ETIMEDOUT || ret == -EAGAIN)
> dev_err(i2c->dev, "i2c transfer failed, ret %d err 0x%lx\n",
> - ret, i2c->status & SPACEMIT_SR_ERR);
> + ret, i2c->status & SPACEMIT_SR_ERR);
>
have to say, I personally don't like those unrelated changes, which
makes people review the patch even difficult
> return ret < 0 ? ret : num;
> }
>
> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> + return spacemit_i2c_xfer_common(adapt, msgs, num, false);
> +}
> +
> +static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> +{
> + return spacemit_i2c_xfer_common(adapt, msgs, num, true);
> +}
> +
atomic API should not call sleepable function, please check this
spacemit_i2c_pio_xfer_atomic() ->
spacemit_i2c_xfer_common() ->
spacemit_i2c_xfer_msg() ->
spacemit_i2c_conditionally_reset_bus() ->
usleep_range()
spacemit_i2c_check_bus_release() ->
usleep_range()
they are in err handle path, but still need to take care with caution
--
Yixun Lan (dlan)
On Mon, Sep 29, 2025 at 02:07:49PM +0800, Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C in atomic context.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changes in v3:
> - drop 1-5 patches (have been merged)
> - modify commit message
> - use readl_poll_timeout_atomic() in wait_pio_xfer()
> - use msecs_to_jiffies() when get PIO mode timeout value
> - factor out transfer state handling into spacemit_i2c_handle_state().
> - do not disable/enable the controller IRQ around PIO transfers.
> - consolidate spacemit_i2c_init() interrupt setup
> - rename is_pio -> use_pio
> - rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
> - rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
> - rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
> - call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
> - Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
>
> Changes in v2:
> - Patch 1/6:
> Patch 3/6:
> Patch 4/6:
> - nothing
> - Patch 2/6:
> - remove is_pio judgement in irq_handler()
> - Patch 5/6:
> - fix wrong comment
> - In spacemit_i2c_conditionally_reset_bus(), once the condition is met inside the loop, it should
> return directly instead of using break.
> - Patch 6/6:
> - add is_pio judgement in irq_handler()
> - use a fixed timeout value when PIO
> - use readl_poll_timeout() in spacemit_i2c_wait_bus_idle() when PIO
>
> - Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
> ---
> drivers/i2c/busses/i2c-k1.c | 206 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 161 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> {
> unsigned long time_left;
> @@ -310,10 +376,16 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>
> reinit_completion(&i2c->complete);
>
> - spacemit_i2c_start(i2c);
> + if (i2c->use_pio) {
> + spacemit_i2c_start(i2c);
> +
> + time_left = spacemit_i2c_wait_pio_xfer(i2c);
> + } else {
> + spacemit_i2c_start(i2c);
I will put i2c_start() out in the next version.
- Troy
© 2016 - 2025 Red Hat, Inc.