diff mbox

drm/i915: preserve other DP_TEST_SINK bits.

Message ID 009801cfdc36$2f1454c0$8d3cfe40$@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Todd Previte Sept. 29, 2014, 10:39 p.m. UTC
Hi Rodrigo,

Looks good. Only thing that needs to be removed is that extra blank line
between the last part of the function and the return statement. Otherwise...

Reviewed-by: Todd Previte <tprevite@gmail.com>

-----Original Message-----
From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] 
Sent: Monday, September 29, 2014 3:30 PM
To: intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi; Todd Previte
Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.

Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
reserved reading all 0s. But when reviewing my latest changes on sink crc
Todd warned me that on new specs we have other valid bits on this reg that
we might want to preserve.

Cc: Todd Previte <tprevite@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

 	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@
-3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
*crc)
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		return -EIO;
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			buf & ~DP_TEST_SINK_START);
+
 	return 0;
 }
 
--
1.9.3

Comments

Daniel Vetter Sept. 30, 2014, 7:42 a.m. UTC | #1
On Mon, Sep 29, 2014 at 03:39:06PM -0700, Todd Previte wrote:
> Hi Rodrigo,
> 
> Looks good. Only thing that needs to be removed is that extra blank line
> between the last part of the function and the return statement. Otherwise...

Looking around in our driver the return statement at the end of the
function is often on a separate line.
> 
> Reviewed-by: Todd Previte <tprevite@gmail.com>

Queued for -next, thanks for the patch.
> 
> -----Original Message-----
> From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] 
> Sent: Monday, September 29, 2014 3:30 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Rodrigo Vivi; Todd Previte
> Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
> 
> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
> reserved reading all 0s. But when reviewing my latest changes on sink crc
> Todd warned me that on new specs we have other valid bits on this reg that
> we might want to preserve.

Why was this not mentioned in the review to that patch? I've
double-checked Todd's reply and I've only found a "lgtm, r-b".

If you spot something odd, or something that should be addressed in a
follow-up patch, that should be part of your review reply - review is much
more than rubber-stamping with r-b tags.

And if that review happened offline or in Portland, pls always try to
summarize the main points discussed in the patch or reply for the benefit
of everyone else. We're a globally distributed team, so active and
inclusive communication is absolutely key.

Thanks, Daniel

> 
> Cc: Todd Previte <tprevite@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
> *crc)
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
>  		return -ENOTTY;
>  
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       DP_TEST_SINK_START) < 0)
> +				buf | DP_TEST_SINK_START) < 0)
>  		return -EIO;
>  
>  	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@
> -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
> *crc)
>  	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>  		return -EIO;
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +			buf & ~DP_TEST_SINK_START);
> +
>  	return 0;
>  }
>  
> --
> 1.9.3
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3821,8 +3821,9 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
*crc)
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       DP_TEST_SINK_START) < 0)
+				buf | DP_TEST_SINK_START) < 0)
 		return -EIO;