Hi, On Tue, Jun 23, 2026 at 10:17 AM Doug Anderson <[email protected]> wrote: > > Hi, > > On Mon, Jun 22, 2026 at 11:06 PM Pengpeng Hou <[email protected]> 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 <[email protected]> > > 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
