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.
Thanks!
-Doug
