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] [day] [month] [year] [list]

Message-ID: <00f42b6c-857b-4eef-a0da-83053998135a@riscstar.com>
Date: Thu, 15 Jan 2026 11:02:17 -0600
From: Alex Elder <elder@...cstar.com>
To: Andi Shyti <andi.shyti@...nel.org>,
 Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Cc: Yixun Lan <dlan@...too.org>, 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 1/14/26 9:47 AM, 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.

I agree with both of these comments/suggestions.

And if fsleep() were used, spacemit_i2c_delay() might be able
to just go away.  (The range used in fsleep() isn't quite the
same as what you're using, but this is heuristic anyway.)

However the delay used in spacemit_i2c_check_bus_release() is
90-150 microseconds, which would lead to sleeping in fsleep().
Is there any chance 10 microseconds (or less) could be used
for all delays?  Even if not, fsleep() might help here.

>> +}
> 
> ...

. . .

>> -	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.

Yes!  It's a next-level skill:  beyond just changing the
code, breaking the change into good, independent pieces
that build on each other, to facilitate review.

Thanks!
					-Alex

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