On Tue, Jan 13, 2015 at 2:26 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Tue, Jan 13, 2015 at 02:24:54PM +0000, R, Durgadoss wrote: >> Hi Rodrigo, >> >> >-----Original Message----- >> >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Rodrigo Vivi >> >Sent: Monday, January 12, 2015 11:45 PM >> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >Cc: Vivi, Rodrigo >> >Subject: [PATCH 3/9] drm/i915: PSR HSW/BDW: Fix inverted logic at sink main_link_active bit. >> > >> >We have only two possible states with so many names and combinations that >> >might be confusing. >> > >> >1 - Main link active / enabled / stand by / on >> >2 - Main link disabled / off / full off >> >> In this case, I think we should use 'link_active' instead of 'link_standby' >> since 'active' is what is used by the eDP spec and most of our PSR >> macros. But, I believe we can have this as a separate patch. > > I haven't read the patches in detail, but consistent naming is imo very > important. Also since this seems to be confusing some extension to the psr > kerneldoc overview comment is imo asked for here. I also agree that consistent naming is good. This name caused the confusion of inverted logic somewhere here. But I believe active name can be counfused with active/exit state we have on source side. Also standby is the name used on the source. Actually we have many names for same think * full_link_on at VBT * link_active on DP * transmitter state on VLV/CHV * link_standby on core implementation I prefer the link_standby. > > Rodrigo, can you please update the patches or do a follow up? Either is > fine with me. I prefer a follow-up so we can continue discussion to see if I change my mind on the better name ;) > -Daniel Thank you both, Rodrigo. > >> >> For patches 1-3, >> Reviewed-by: Durgadoss R <durgadoss.r@xxxxxxxxx> >> >> Thanks, >> Durga >> >> > >> >Let's start organizing it by fixing a inverted logic when setting the sink bit. >> > >> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >> >--- >> > drivers/gpu/drm/i915/intel_psr.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> >diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >> >index 3dd8886..0af89db 100644 >> >--- a/drivers/gpu/drm/i915/intel_psr.c >> >+++ b/drivers/gpu/drm/i915/intel_psr.c >> >@@ -163,10 +163,10 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp) >> > /* Enable PSR in sink */ >> > if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby) >> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, >> >- DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); >> >+ DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); >> > else >> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, >> >- DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); >> >+ DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); >> > >> > /* Setup AUX registers */ >> > for (i = 0; i < sizeof(aux_msg); i += 4) >> >-- >> >2.1.0 >> > >> >_______________________________________________ >> >Intel-gfx mailing list >> >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
