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

[Aurelien Jarno <aurelien@aurel32.net>]

Hi,

On 2025-12-29 10:07, Troy Mitchell wrote:
> > > @@ -474,6 +608,11 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> > >   	unsigned long timeout;
> > >   	int idx = 0, cnt = 0;
> > > +	if (i2c->use_pio) {
> > > +		i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
> > 
> > Again, why is a rough 1000 millisecond timeout OK for PIO, while a
> > fairly precise timeout value based on the number of bytes to be
> > transferred and the transfer bit rate computed for interrupt mode?
> Sorry I didn't see this.
> 
> In interrupt-driven mode we wait for a single completion event, so the
> timeout needs to reflect the worst-case transfer duration to avoid
> spurious timeouts.
> In PIO mode the loop is driven by FIFO/status progress, and the timeout
> is only a safeguard against a stalled bus rather than an exact transfer
> time.
> Therefore a simple conservative value is sufficient there.

I do think the PIO code is correct wrt timeout. It is not possible to 
predict the time of a transaction as an I2C peripheral can slow down a 
transaction by pulling the SCL line down. Therefore a fixed timeout 
should be used in all cases not only for PIO, and this matches what is 
done in all other I2C drivers.

That said it's probably out of scope for this patch series.

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net