[PATCH v5 1/4] drm/bridge: ti-sn65dsi86: check AUX errors better

[Tomi Valkeinen]

Hi,

On 29/08/2022 20:15, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
> <tomi.valkeinen at ideasonboard.com> wrote:
>>
>> From: Tomi Valkeinen <tomi.valkeinen+renesas at ideasonboard.com>
>>
>> The driver does not check AUX_IRQ_STATUS_NAT_I2C_FAIL bit at all when
>> sending AUX transfers,
> 
> It doesn't? What about a few lines down from where your patch modifies
> that reads:
> 
>    else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
> 
> That seems like it's checking that bit?

You're right, the patch is obviously broken.

>> which leads to the driver not returning an error.
> 
> Right that it doesn't return an error. I guess the question is: should
> it? Right now it sets the proper reply (DP_AUX_I2C_REPLY_NACK or
> DP_AUX_NATIVE_REPLY_NACK) and returns 0. Is it supposed to be
> returning an error code? What problem are you fixing?

I encountered a problem where the monitor was not replying properly and 
the driver was just getting AUX_IRQ_STATUS_NAT_I2C_FAIL errors, but the 
drm_dp_dpcd_read functions were not returning an error and the driver 
didn't understand that none of the transactions are actually going through.

Of course, now that I try I'm unable to reproduce the situation, I can 
only see AUX_IRQ_STATUS_AUX_RPLY_TOUT when something is not right, never 
AUX_IRQ_STATUS_NAT_I2C_FAIL.

Looking at the code, AUX_IRQ_STATUS_NAT_I2C_FAIL sets the len to 0, so 
that should cause a failure when the driver compares the return value of 
drm_dp_dpcd_read to the expected number of bytes. So I have trouble 
understanding the behavior I saw.

I did have WIP IRQ code in my branch at that time, though, and I'm now 
kind of suspecting that code, as it also somehow triggers the DSI RX 
issues I mention in the other mail.

> In commit 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux
> failures"), at least, we thought that returning "0" and setting the
> "reply" was the correct thing to do (though we didn't have any good
> setup to test all the error paths).
> 
> ...and looking through the code at drm_dp_i2c_do_msg(), I see that it
> only ever looks at DP_AUX_I2C_REPLY_NACK if "ret" was 0.
> 
> So I guess I would say:
> 
> 1. Your patch doesn't seem right to me.
> 
> 2. If your patch is actually fixing a problem, you should at least
> modify it so that it doesn't create dead code (the old handling of
> AUX_IRQ_STATUS_NAT_I2C_FAIL is no longer reachable after your patch.
> 
> Naked-by: Douglas Anderson <dianders at chromium.org>

I'll drop this patch.

  Tomi