Re: [PATCH 2/3] drm/i915: don't use HPD_PORT_A as an alias for HPD_NONE

[Imre Deak <imre.deak@intel.com>]

On Tue, 2015-07-21 at 13:51 +0530, Sivakumar Thulasimani wrote:
> 
> 
> On 7/21/2015 3:13 AM, Imre Deak wrote:
> 
> > Currently HPD_PORT_A is used as an alias for HPD_NONE to mean that the
> > given port doesn't support long/short HPD pulse detection. SDVO and CRT
> > ports are like this and for these ports we only want to know whether an
> > hot plug event was detected on the corresponding pin. Since at least on
> > BXT we need long/short pulse detection on PORT A as well (added by the
> > next patch) remove this aliasing of HPD_PORT_A/HPD_NONE and let the
> > return value of intel_hpd_pin_to_port() show whether long/short pulse
> > detection is supported on the passed in pin.
> > 
> > No functional change.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  4 ++--
> >  drivers/gpu/drm/i915/i915_irq.c      |  4 ++--
> >  drivers/gpu/drm/i915/intel_hotplug.c | 20 +++++++++++++-------
> >  3 files changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 78d6c7a..ebfd5a5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -206,11 +206,11 @@ enum intel_display_power_domain {
> >  
> >  enum hpd_pin {
> >  	HPD_NONE = 0,
> > -	HPD_PORT_A = HPD_NONE, /* PORT_A is internal */
> >  	HPD_TV = HPD_NONE,     /* TV is known to be unreliable */
> >  	HPD_CRT,
> >  	HPD_SDVO_B,
> >  	HPD_SDVO_C,
> > +	HPD_PORT_A,
> >  	HPD_PORT_B,
> >  	HPD_PORT_C,
> >  	HPD_PORT_D,
> > @@ -2627,7 +2627,7 @@ void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
> >  void intel_hpd_init(struct drm_i915_private *dev_priv);
> >  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> >  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> > -enum port intel_hpd_pin_to_port(enum hpd_pin pin);
> > +bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port);
> >  
> >  /* i915_irq.c */
> >  void i915_queue_hangcheck(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 306de78..4ad7a31 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1276,8 +1276,8 @@ static void intel_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
> >  
> >  		*pin_mask |= BIT(i);
> >  
> > -		port = intel_hpd_pin_to_port(i);
> > -		if (long_pulse_detect(port, dig_hotplug_reg))
> > +		if (intel_hpd_pin_to_port(i, &port) &&
> > +		    long_pulse_detect(port, dig_hotplug_reg))
> >  			*long_mask |= BIT(i);
> >  	}
> feel that this could be more readable when split it to two
> conditions ? 
> if (intel_hpd_pin_to_port(i, &port) == false)
>     continue;
> if (long_pulse_detect(port, dig_hotplug_reg)
>     *long_mask |= BIT(i);

Ok, will change it.

> 
> other than this, this change looks good. 
> 
> 
> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> 
> 
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > index 3c53aac..8cda7b9 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -29,17 +29,23 @@
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >  
> > -enum port intel_hpd_pin_to_port(enum hpd_pin pin)
> > +bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port)
> >  {
> >  	switch (pin) {
> > +	case HPD_PORT_A:
> > +		*port = PORT_A;
> > +		return true;
> >  	case HPD_PORT_B:
> > -		return PORT_B;
> > +		*port = PORT_B;
> > +		return true;
> >  	case HPD_PORT_C:
> > -		return PORT_C;
> > +		*port = PORT_C;
> > +		return true;
> >  	case HPD_PORT_D:
> > -		return PORT_D;
> > +		*port = PORT_D;
> > +		return true;
> >  	default:
> > -		return PORT_A; /* no hpd */
> > +		return false;	/* no hpd */
> >  	}
> >  }
> >  
> > @@ -322,8 +328,8 @@ void intel_hpd_irq_handler(struct drm_device *dev,
> >  		if (!(BIT(i) & pin_mask))
> >  			continue;
> >  
> > -		port = intel_hpd_pin_to_port(i);
> > -		is_dig_port = port && dev_priv->hotplug.irq_port[port];
> > +		is_dig_port = intel_hpd_pin_to_port(i, &port) &&
> > +			      dev_priv->hotplug.irq_port[port];
> >  
> >  		if (is_dig_port) {
> >  			bool long_hpd = long_mask & BIT(i);
> 
> -- 
> regards,
> Sivakumar


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx