On Fri, Jun 27, 2025 at 02:36:32PM +0300, Jani Nikula wrote:
> Unify on using read_poll_timeout() throughout instead of mixing with
> readx_poll_timeout(). While the latter can be ever so slightly simpler,
> they are both complicated enough that it's better to unify on one
> approach only.
>
> While at it, better separate the handling of error returns from
> drm_dp_dpcd_readb() and the actual status byte. This is best achieved by
> inlining the read_fec_detected_status() function.
>
> Cc: Imre Deak <imre.d...@intel.com>
> Signed-off-by: Jani Nikula <jani.nik...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 33 +++++++++---------------
> 1 file changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 0405396c7750..fc4587311607 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2339,34 +2339,25 @@ static void intel_dp_sink_set_fec_ready(struct
> intel_dp *intel_dp,
> drm_dbg_kms(display->drm, "Failed to clear FEC detected
> flags\n");
> }
>
> -static int read_fec_detected_status(struct drm_dp_aux *aux)
> -{
> - int ret;
> - u8 status;
> -
> - ret = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status);
> - if (ret < 0)
> - return ret;
> -
> - return status;
> -}
> -
> static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled)
> {
> struct intel_display *display = to_intel_display(aux->drm_dev);
> int mask = enabled ? DP_FEC_DECODE_EN_DETECTED :
> DP_FEC_DECODE_DIS_DETECTED;
> - int status;
> - int err;
> + u8 status = 0;
> + int ret, err;
>
> - err = readx_poll_timeout(read_fec_detected_status, aux, status,
> - status & mask || status < 0,
> - 10000, 200000);
> + ret = read_poll_timeout(drm_dp_dpcd_readb, err,
drm_dp_dpcd_read_byte()? With that it looks ok:
Reviewed-by: Imre Deak <imre.d...@intel.com>
> + err || (status & mask),
> + 10 * 1000, 200 * 1000, false,
Nit: there's also USEC_PER_MSEC.
> + aux, DP_FEC_STATUS, &status);
>
> - if (err || status < 0) {
> + /* Either can be non-zero, but not both */
> + ret = ret ?: err;
> + if (ret) {
> drm_dbg_kms(display->drm,
> - "Failed waiting for FEC %s to get detected: %d
> (status %d)\n",
> - str_enabled_disabled(enabled), err, status);
> - return err ? err : status;
> + "Failed waiting for FEC %s to get detected: %d
> (status 0x%02x)\n",
> + str_enabled_disabled(enabled), ret, status);
> + return ret;
> }
>
> return 0;
> --
> 2.39.5
>
