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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <DG8S0EEA55Z6.3RVC23NWTEW1S@linux.spacemit.com>
Date: Sat, 07 Feb 2026 21:49:47 +0800
From: Troy Mitchell <troymitchell988@...il.com>
To: "Andi Shyti" <andi.shyti@...nel.org>, "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

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