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

[Yixun Lan <dlan@gentoo.org>]

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)

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv