On 03.12.2021 11:41, Jean Delvare wrote:
> Hi Heiner,
>
> On Thu, 02 Dec 2021 10:57:43 +0100, Heiner Kallweit wrote:
>> In interrupt mode we clear the status flags twice, in the interrupt
>> handler and in i801_check_post(). Remove clearing the status flags
>> from i801_check_post() and let i801_wait_intr() clear them in
>> polling mode. Another benefit is that now only checks for error
>> conditions are left in i801_check_post(), thus better matching the
>> function name.
>>
>> Note: There's a comment in i801_check_post() that i801_wait_intr()
>> clears the error status bits. Actually this hasn't been true until
>> this change.
>
> While reviewing this, I have noticed other comments which seem outdated:
>
> * In i801_isr(), "->status must be cleared before the next transaction
> is started." We no longer do this since
> 1de93d5d521717cbb77cc9796a4df141d800d608, and things are working
> well, so clearly this is no longer necessary and the comment could be
> removed.
> * The comment before i801_check_post() is obsoleted by your
> proposed change.
>
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 7446bed78..82553e0cb 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -438,9 +438,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
>> dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
>> }
>>
>> - /* Clear status flags except BYTE_DONE, to be cleared by caller */
>> - outb_p(status, SMBHSTSTS(priv));
>> -
>> return result;
>> }
>>
>> @@ -455,8 +452,10 @@ static int i801_wait_intr(struct i801_priv *priv)
>> status = inb_p(SMBHSTSTS(priv));
>> busy = status & SMBHSTSTS_HOST_BUSY;
>> status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>> - if (!busy && status)
>> + if (!busy && status) {
>> + outb_p(status, SMBHSTSTS(priv));
>> return status;
>> + }
>> } while (time_is_after_eq_jiffies(timeout));
>>
>> return -ETIMEDOUT;
>
> I like the idea to make things consistent between the interrupt and
> poll modes, however I'm afraid there's one code path not covered by
> your patch. If i801_wait_byte_done() returns an error flag in
> i801_block_transaction_byte_by_byte(), we jump directly to
> i801_check_post() without calling i801_wait_intr(). In that case, the
> error flags are not cleared, right? Doesn't look good.
>
Indeed, good point. I have an idea, I'll propose a patch that we can
use as discussion basis.
> Makes me wonder if we shouldn't actually let i801_check_post() clear
> the flags and remove that piece of code everywhere else? Or is there a
> reason why it has to be done earlier in i801_isr(), that I do not
> remember?
>
I'd like to achieve that check_pre/post() only check and handle error
conditions. Both functions shouldn't be involved in the normal flow.
> I'm also wondering if there's actually any reason to clear the flags at
> all at the end of the transaction, considering that we'll be doing it
> again at the beginning of the next transaction anyway to be on the safe
> side.
>
Clearing flags in check_pre() should just be a last resort (IMO).
The regular flow should leave the flags in a clean state.
