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?
> 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?
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>
