[v4,3/6] i2c: npcm: Modify timeout evaluation mechanism

From: Tyrone Ting <kfting@nuvoton.com>

From: Tyrone Ting <kfting@nuvoton.com>

The users want to connect a lot of masters on the same bus.
This timeout is used to determine the time it takes to take bus ownership.
The transactions are very long, so waiting 35ms is not enough.

Increase the timeout and treat it as the total timeout, including retries.
The total timeout is 2 seconds now.

The i2c core layer will have chances to retry to call the i2c driver
transfer function if the i2c driver reports that the bus is busy and
returns EAGAIN.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Hi Tyrone,

...

> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2d034503d8bc..68f3d47323ab 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -2132,19 +2132,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		}
>  	}
>  
> -	/*
> -	 * Adaptive TimeOut: estimated time in usec + 100% margin:
> -	 * 2: double the timeout for clock stretching case
> -	 * 9: bits per transaction (including the ack/nack)
> -	 */
> -	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
> -	timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
>  	if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
>  		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
>  		return -EINVAL;
>  	}
>  
> -	time_left = jiffies + timeout + 1;
> +	time_left = jiffies + bus->adap.timeout / bus->adap.retries + 1;
>  	do {
>  		/*
>  		 * we must clear slave address immediately when the bus is not
> @@ -2183,6 +2176,14 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
>  				       write_data, read_data, read_PEC,
>  				       read_block)) {
> +		/*
> +		 * Adaptive TimeOut: estimated time in usec + 100% margin:
> +		 * 2: double the timeout for clock stretching case
> +		 * 9: bits per transaction (including the ack/nack)
> +		 */
> +		timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
> +		timeout = max_t(unsigned long, bus->adap.timeout / bus->adap.retries,
> +				usecs_to_jiffies(timeout_usec));
>  		time_left = wait_for_completion_timeout(&bus->cmd_complete,
>  							timeout);
>  
> @@ -2308,7 +2309,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
>  	adap = &bus->adap;
>  	adap->owner = THIS_MODULE;
>  	adap->retries = 3;
> -	adap->timeout = msecs_to_jiffies(35);
> +	adap->timeout = 2 * HZ;

Same here, I need some good description of why timeout is set to
2 seconds. If the datasheet says 35ms, I do not exclude that
someone in the future will send a patch saying "we don't need to
wait 2 seconds, wait 35ms as per datasheet".

Thanks,
Andi

>  	adap->algo = &npcm_i2c_algo;
>  	adap->quirks = &npcm_i2c_quirks;
>  	adap->algo_data = bus;
> -- 
> 2.34.1
>

Hi Andi:

Thank you for your feedback.

Your comments will be addressed.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月26日 週四 上午4:24寫道：
>
> Hi Tyrone,
>
> ...
>
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 2d034503d8bc..68f3d47323ab 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -2132,19 +2132,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >               }
> >       }
> >
> > -     /*
> > -      * Adaptive TimeOut: estimated time in usec + 100% margin:
> > -      * 2: double the timeout for clock stretching case
> > -      * 9: bits per transaction (including the ack/nack)
> > -      */
> > -     timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
> > -     timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
> >       if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
> >               dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
> >               return -EINVAL;
> >       }
> >
> > -     time_left = jiffies + timeout + 1;
> > +     time_left = jiffies + bus->adap.timeout / bus->adap.retries + 1;
> >       do {
> >               /*
> >                * we must clear slave address immediately when the bus is not
> > @@ -2183,6 +2176,14 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >       if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
> >                                      write_data, read_data, read_PEC,
> >                                      read_block)) {
> > +             /*
> > +              * Adaptive TimeOut: estimated time in usec + 100% margin:
> > +              * 2: double the timeout for clock stretching case
> > +              * 9: bits per transaction (including the ack/nack)
> > +              */
> > +             timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
> > +             timeout = max_t(unsigned long, bus->adap.timeout / bus->adap.retries,
> > +                             usecs_to_jiffies(timeout_usec));
> >               time_left = wait_for_completion_timeout(&bus->cmd_complete,
> >                                                       timeout);
> >
> > @@ -2308,7 +2309,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
> >       adap = &bus->adap;
> >       adap->owner = THIS_MODULE;
> >       adap->retries = 3;
> > -     adap->timeout = msecs_to_jiffies(35);
> > +     adap->timeout = 2 * HZ;
>
> Same here, I need some good description of why timeout is set to
> 2 seconds. If the datasheet says 35ms, I do not exclude that
> someone in the future will send a patch saying "we don't need to
> wait 2 seconds, wait 35ms as per datasheet".
>
> Thanks,
> Andi
>
> >       adap->algo = &npcm_i2c_algo;
> >       adap->quirks = &npcm_i2c_quirks;
> >       adap->algo_data = bus;
> > --
> > 2.34.1
> >

Have a nice day.

Regards,
Tyrone