Re: [PATCH v2 6/6] i2c: spacemit: introduce pio for k1

Yixun Lan
2025-09-27T10:56:23Z

On 10:02 Thu 25 Sep , Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C with interrupts disabled.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> drivers/i2c/busses/i2c-k1.c | 164 +++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 140 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -97,6 +97,9 @@
>
..
> static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> {
> unsigned long time_left;
> @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>
> reinit_completion(&i2c->complete);
>
> - spacemit_i2c_start(i2c);
> + if (i2c->is_pio) {
> + /* We disable the interrupt to avoid unintended spurious triggers. */
the comment is suspicious, and seems wrong..
> + disable_irq(i2c->irq);
> +
I guess this doesn't disable interrupt in the hardware layer, it will still
fire interrupt once enabled, so instead of calling disable_irq(), why not
dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
enableing) of ICR REGISTER, disabling them should prevent the interrupt
triggered?

> + /*
> + * The K1 I2C controller has a quirk:
> + * when doing PIO transfers, the status register must be cleared
> + * of all other bits before issuing a START.
> + * Failing to do so will prevent the transfer from working properly.
> + */
> + spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
this also doesn't seem correct to me, the irq status bit should be cleared once
interrupt occur, not dealing it here before sending msg, this seems a
wrong procedure

> +
> + spacemit_i2c_start(i2c);
> + time_left = spacemit_i2c_wait_pio_xfer(i2c);
> +
> + enable_irq(i2c->irq);
> + } 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 +417,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)

--
Yixun Lan (dlan)