Hi Sven,
On Sat, Feb 22, 2025 at 01:38:34PM +0000, Sven Peter via B4 Relay wrote:
> The hardware (supposedly) has a 25ms timeout for clock stretching
> and the driver uses 100ms which should be plenty.
Can we add this lines as a comment to the define you are adding?
> The error
> reocvery itself is however lacking.
...
> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
> {
> unsigned int status;
> + int timeout = TRANSFER_TIMEOUT_MS;
>
> status = reg_read(smbus, REG_SMSTA);
> +
> + /* First wait for the bus to go idle */
> + while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
> + msleep(1);
Please, use usleep_range for 1 millisecond timeout.
> + status = reg_read(smbus, REG_SMSTA);
> + }
You could use here readx_poll_timeout() here.
> +
> + if (timeout < 0) {
> + dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n",
> status);
if it's an error, please use an error.
> + return -EIO;
> + }
> +
> + /* If any badness happened or there is data in the FIFOs, reset the
> FIFOs */
> + if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM |
> SMSTA_MTN | SMSTA_MTA)) ||
> + !(status & SMSTA_MTE))
Please, fixe the alignment here.
> + pasemi_reset(smbus);
> +
> + /* Clear the flags */
> reg_write(smbus, REG_SMSTA, status);
> +
> + return 0;
> }
>
> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> - int timeout = 100;
> + int timeout = TRANSFER_TIMEOUT_MS;
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(100));
> + /* XEN should be set when a transaction terminates, whether due
> to error or not */
> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
> + wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(timeout));
what happens if the timeout expires?
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
> } else {
...
> struct pasemi_smbus *smbus = adapter->algo_data;
> int ret, i;
>
> - pasemi_smb_clear(smbus);
> + if (pasemi_smb_clear(smbus))
> + return -EIO;
Can we use
ret = ...
if (ret)
return ret;
This way we return whatever comes from pasemi_smb_clear().
>
> ret = 0;
This way we can remove this line, as well.
Andi
