linux-kernel - Re: [PATCH v6 2/2] i2c: spacemit: introduce pio for k1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aWeyT8gb8Z31S_V9@zenone.zhora.eu>
Date: Wed, 14 Jan 2026 16:47:22 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Cc: Yixun Lan <dlan@...too.org>, Alex Elder <elder@...cstar.com>, 
	Aurelien Jarno <aurelien@...el32.net>, Michael Opdenacker <michael.opdenacker@...tcommit.com>, 
	Troy Mitchell <troymitchell988@...il.com>, linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH v6 2/2] i2c: spacemit: introduce pio for k1

Hi Troy,

...

> @@ -171,6 +176,16 @@ static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
>  	return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
>  }
>  
> +static inline void spacemit_i2c_delay(struct spacemit_i2c_dev *i2c,
> +				      unsigned int min_us,
> +				      unsigned int max_us)
> +{
> +	if (i2c->use_pio)
> +		udelay(max_us);

We need some control on how much we want to sleep in atomic. This
can have effects on the whole system.

> +	else
> +		usleep_range(min_us, max_us);

If we assume that max_us = min_us * 2 we don't need to pass it as
a parameter. Even better you can use fsleep here which does it
for you.

> +}

...

> +	if (!i2c->use_pio) {
> +		/*
> +		 * Enable interrupt bits for all xfer mode:
> +		 * bus error, arbitration loss detected.
> +		 */
> +		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().
> +		 * We don't want a TX empty interrupt until we start
> +		 * a transfer in i2c_start().
> +		 */
> +		val |= SPACEMIT_CR_DRFIE;
> +
> +		/* Enable master stop interrupt bit.

Please run checkpatch.pl before sending the patch. Here the
comment format is wrong.

> +		 * For transaction complete signal, we use master stop
> +		 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> +		 */
> +		val |= SPACEMIT_CR_MSDIE;
> +	}

...

> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	if (i2c->status & SPACEMIT_SR_ERR)
> +		goto err_out;
> +
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> +
> +	switch (i2c->state) {
> +	case SPACEMIT_STATE_START:
> +		spacemit_i2c_handle_start(i2c);
> +		break;
> +	case SPACEMIT_STATE_READ:
> +		spacemit_i2c_handle_read(i2c);
> +		break;
> +	case SPACEMIT_STATE_WRITE:
> +		spacemit_i2c_handle_write(i2c);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (i2c->state != SPACEMIT_STATE_IDLE) {
> +		val |= SPACEMIT_CR_TB;
> +		if (i2c->use_pio)
> +			val |= SPACEMIT_CR_ALDIE;

Why are we enabling ALDIE here?

> +
> +

please drop one blank line here

> +		if (spacemit_i2c_is_last_msg(i2c)) {
> +			/* trigger next byte with stop */
> +			val |= SPACEMIT_CR_STOP;
> +
> +			if (i2c->read)
> +				val |= SPACEMIT_CR_ACKNAK;
> +		}
> +		writel(val, i2c->base + SPACEMIT_ICR);
> +	}

...

> @@ -431,41 +605,8 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  
>  	spacemit_i2c_clear_int_status(i2c, status);
>  
> -	if (i2c->status & SPACEMIT_SR_ERR)
> -		goto err_out;
> -
> -	val = readl(i2c->base + SPACEMIT_ICR);
> -	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> -
> -	switch (i2c->state) {
> -	case SPACEMIT_STATE_START:
> -		spacemit_i2c_handle_start(i2c);
> -		break;
> -	case SPACEMIT_STATE_READ:
> -		spacemit_i2c_handle_read(i2c);
> -		break;
> -	case SPACEMIT_STATE_WRITE:
> -		spacemit_i2c_handle_write(i2c);
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	if (i2c->state != SPACEMIT_STATE_IDLE) {
> -		val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
> -
> -		if (spacemit_i2c_is_last_msg(i2c)) {
> -			/* trigger next byte with stop */
> -			val |= SPACEMIT_CR_STOP;
> -
> -			if (i2c->read)
> -				val |= SPACEMIT_CR_ACKNAK;
> -		}
> -		writel(val, i2c->base + SPACEMIT_ICR);
> -	}
> +	spacemit_i2c_handle_state(i2c);

Next time this can be on a separate patch as a preparatory patch
to make the review of this one a bit easier.

>  
> -err_out:
> -	spacemit_i2c_err_check(i2c);
>  	return IRQ_HANDLED;
>  }

Thanks,
Andi