Re: [PATCH] drm/bridge: ps8640: propagate AUX transfer register errors

[Doug Anderson]

Hi,

On Tue, Jun 23, 2026 at 10:17 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Jun 22, 2026 at 11:06 PM Pengpeng Hou <pengpeng@xxxxxxxxxxx> wrote:
> >
> > ps8640_aux_transfer_msg() programs the AUX address registers, starts the
> > AUX transfer, waits for SWAUX_SEND to clear, and reads the AUX status
> > register. Several of those regmap operations have return values, but the
> > function only checks a stale ret after the status read.
> >
> > Propagate failures from the address write, transfer start, completion
> > poll, and status read. This avoids returning a transfer length when the
> > bridge register transaction or AUX completion wait failed.
> >
> > Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
>
> I think we want this, right?
>
> Fixes: 13afcdd7277e ("drm/bridge: parade-ps8640: Add support for AUX channel")
>
>
> > ---
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index b93514023baa..bf5a36e76c56 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -257,8 +257,12 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
> >         addr_len[PAGE0_SWAUX_LENGTH - base] = (len == 0) ? SWAUX_NO_PAYLOAD :
> >                                               ((len - 1) & SWAUX_LENGTH_MASK);
> >
> > -       regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
> > -                         ARRAY_SIZE(addr_len));
> > +       ret = regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
> > +                               ARRAY_SIZE(addr_len));
> > +       if (ret) {
> > +               DRM_DEV_ERROR(dev, "failed to write AUX address: %d\n", ret);
> > +               return ret;
> > +       }
>
> nit: Include "msg->address" and "len" in the error message?
>
>
> >         if (len && (request == DP_AUX_NATIVE_WRITE ||
> >                     request == DP_AUX_I2C_WRITE)) {
> > @@ -274,13 +278,21 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
> >                 }
> >         }
> >
> > -       regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
> > +       ret = regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
> > +       if (ret) {
> > +               DRM_DEV_ERROR(dev, "failed to start AUX transfer: %d\n", ret);
> > +               return ret;
> > +       }
> >
> >         /* Zero delay loop because i2c transactions are slow already */
> > -       regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
> > -                                !(data & SWAUX_SEND), 0, 50 * 1000);
> > +       ret = regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
> > +                                      !(data & SWAUX_SEND), 0, 50 * 1000);
> > +       if (ret) {
> > +               DRM_DEV_ERROR(dev, "AUX transfer timed out: %d\n", ret);
>
> nit: not all failures here will be a timeout. Can you adjust the error message?
>
>
> > +               return ret;
> > +       }
> >
> > -       regmap_read(map, PAGE0_SWAUX_STATUS, &data);
> > +       ret = regmap_read(map, PAGE0_SWAUX_STATUS, &data);
> >         if (ret) {
> >                 DRM_DEV_ERROR(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n",
> >                               ret);
>
> The one above is super weird since we clearly meant to set "ret" given
> the error message. Not sure how we missed that originally. Oh well.
>
> In any case, the change looks good to me, though it'd be nice if you
> could spin with slightly different error messages.

I was assuming you'd send a v2 with the above nits fixed, but I
haven't seen it. Did I miss it?

-Doug