Re: [Intel-gfx] [PATCH] drm/i915: Move hotplug inversion logic into separate helper

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 19 Sep 2022, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Make the code more readable, which will be more apparent as new
> platforms with different hotplug inversion needs are added.
>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index de06f293e173..c53d21ae197f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3263,6 +3263,21 @@ static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> +static void setup_hotplug_inversion(struct drm_i915_private *dev_priv)
> +{
> +	u32 invert_bits;
> +
> +	if (HAS_PCH_DG1(dev_priv))
> +		invert_bits = INVERT_DDIA_HPD |
> +			      INVERT_DDIB_HPD |
> +			      INVERT_DDIC_HPD |
> +			      INVERT_DDID_HPD;

Nitpick, the indentation will be off compared to automated indentation.

> +	else
> +		return;
> +
> +	intel_uncore_rmw(&dev_priv->uncore, SOUTH_CHICKEN1, 0, invert_bits);
> +}
> +
>  static u32 ibx_hotplug_enables(struct drm_i915_private *i915,
>  			       enum hpd_pin pin)
>  {
> @@ -3413,15 +3428,7 @@ static u32 gen11_hotplug_enables(struct drm_i915_private *i915,
>  
>  static void dg1_hpd_irq_setup(struct drm_i915_private *dev_priv)
>  {
> -	u32 val;
> -
> -	val = intel_uncore_read(&dev_priv->uncore, SOUTH_CHICKEN1);
> -	val |= (INVERT_DDIA_HPD |
> -		INVERT_DDIB_HPD |
> -		INVERT_DDIC_HPD |
> -		INVERT_DDID_HPD);
> -	intel_uncore_write(&dev_priv->uncore, SOUTH_CHICKEN1, val);
> -
> +	setup_hotplug_inversion(dev_priv);

Since you're already in a platform specific function here, seems a bit
odd to call a new generic function that needs to have another if ladder
platform check. What are we gaining here? The end result is
de-duplicating just one line of intel_uncore_rmw(). I'm not convinced.

BR,
Jani.


>  	icp_hpd_irq_setup(dev_priv);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center