Re: [PATCH 2/8 v3] i2c: i801: optimize waiting for HWPEC to finish

[Jean Delvare]

Hi Daniel,

On Wed, 27 Jun 2012 21:54:09 +0800, Daniel Kurtz wrote:
> When a transaction has finished (including the PEC), the SMBus controller
> sets the INTR bit.
> Slightly optimize the polling loop by reading status before the first
> sleep.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-i801.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 51e11eb..8b74e1e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -291,11 +291,11 @@ static void i801_wait_hwpec(struct i801_priv *priv)
>  	int timeout = 0;
>  	int status;
>  
> -	do {
> +	status = inb_p(SMBHSTSTS(priv));
> +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>  		usleep_range(250, 500);
>  		status = inb_p(SMBHSTSTS(priv));
> -	} while ((!(status & SMBHSTSTS_INTR))
> -		 && (timeout++ < MAX_RETRIES));
> +	}
>  
>  	if (timeout > MAX_RETRIES)
>  		dev_dbg(&priv->pci_dev->dev, "PEC Timeout!\n");

Hmm, I would be very cautious here. Deep memories whisper to me
that we have sometimes done it that way in the past due to hardware
bugs. I think it was for the PIIX4 and not the 82801, but I wouldn't be
surprised if early 82801 chips had some hardware bugs as well.

Also note that optimizing the polling code isn't a priority when you're
about to enable interrupts for most chips supported by the driver. If
anything, leaving the polling code unchanged for now could even be a
goal, to make sure that users can disable interrupts if that doesn't
work for them, and things keep working as before.

Plus, PEC isn't used that much (neither you nor me can test it). And
lastly, are you really certain that there is a benefit? Are you sure
that the SMBus controller is always faster to receive and process the
PEC byte than the CPU is to reach the INTR check?

Unless you have actual numbers showing a significant improvement, I'd
rather postpone this change.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/