Hi,
On 29/08/2022 20:15, Doug Anderson wrote:
Hi,
On Wed, Aug 24, 2022 at 6:01 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
From: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxx>
I'll drop this patch.
Tomi
