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

From: Troy Mitchell <troymitchell988@gmail.com>
To: "Andi Shyti" <andi.shyti@kernel.org>,
	"Troy Mitchell" <troy.mitchell@linux.spacemit.com>
Cc: "Yixun Lan" <dlan@gentoo.org>, "Alex Elder" <elder@riscstar.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Michael Opdenacker" <michael.opdenacker@rootcommit.com>,
	"Troy Mitchell" <troymitchell988@gmail.com>,
	<linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-riscv@lists.infradead.org>, <spacemit@lists.linux.dev>
Subject: Re: [PATCH v6 2/2] i2c: spacemit: introduce pio for k1
Date: Sat, 07 Feb 2026 21:49:47 +0800	[thread overview]
Message-ID: <DG8S0EEA55Z6.3RVC23NWTEW1S@linux.spacemit.com> (raw)
In-Reply-To: <aWeyT8gb8Z31S_V9@zenone.zhora.eu>

On Wed Jan 14, 2026 at 11:47 PM CST, Andi Shyti wrote:
> 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.
Thanks. I'll use fsleep here and remove max_us.
>
...
>> +}
>
> ...
>
>> +
>> +	if (i2c->state != SPACEMIT_STATE_IDLE) {
>> +		val |= SPACEMIT_CR_TB;
>> +		if (i2c->use_pio)
>> +			val |= SPACEMIT_CR_ALDIE;
>
> Why are we enabling ALDIE here?
Should be:
if (!i2c->use_pio)
>
>> +
>> +
>
> 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.
Thanks. I'll do that.

                              - Troy
>
>>  
>> -err_out:
>> -	spacemit_i2c_err_check(i2c);
>>  	return IRQ_HANDLED;
>>  }
>
> Thanks,
> Andi