On Tue, Jan 13, 2015 at 2:26 PM, Daniel Vetter <dan...@ffwll.ch> wrote: > On Tue, Jan 13, 2015 at 02:24:54PM +0000, R, Durgadoss wrote: >> Hi Rodrigo, >> >> >-----Original Message----- >> >From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf >> >Of Rodrigo Vivi >> >Sent: Monday, January 12, 2015 11:45 PM >> >To: intel-gfx@lists.freedesktop.org >> >Cc: Vivi, Rodrigo >> >Subject: [Intel-gfx] [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 <durgados...@intel.com> >> >> Thanks, >> Durga >> >> > >> >Let's start organizing it by fixing a inverted logic when setting the sink >> >bit. >> > >> >Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com> >> >--- >> > 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@lists.freedesktop.org >> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
