Re: [Intel-gfx] [PATCH v3 5/6] drm/i915/audio: clean up LPE audio init/cleanup calls

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 04, 2021 at 06:18:57PM +0200, Jani Nikula wrote:
> Unify audio init/cleanup paths wrt LPE audio, and base the logic on the
> return values from LPE audio calls. Move the platform device check on
> cleanup to intel_lpe_audio.c, thereby limiting all audio.lpe substruct
> access to that file.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c     | 14 ++++++++------
>  drivers/gpu/drm/i915/display/intel_lpe_audio.c |  6 ++++--
>  drivers/gpu/drm/i915/display/intel_lpe_audio.h |  4 ++--
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index 24e76657d561..aa7037021376 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -1403,8 +1403,10 @@ static void i915_audio_component_cleanup(struct drm_i915_private *dev_priv)
>   */
>  void intel_audio_init(struct drm_i915_private *dev_priv)
>  {
> -	if (intel_lpe_audio_init(dev_priv) < 0)
> -		i915_audio_component_init(dev_priv);
> +	if (!intel_lpe_audio_init(dev_priv))
> +		return;
> +
> +	i915_audio_component_init(dev_priv);

The logic here is already a bit funky. Technically we should not
init the component stuff except when LPE audio is not present.
Ie. we should only do it when intel_lpe_audio_init() returns
-ENODEV.

>  }
>  
>  /**
> @@ -1414,8 +1416,8 @@ void intel_audio_init(struct drm_i915_private *dev_priv)
>   */
>  void intel_audio_deinit(struct drm_i915_private *dev_priv)
>  {
> -	if ((dev_priv)->audio.lpe.platdev != NULL)
> -		intel_lpe_audio_teardown(dev_priv);
> -	else
> -		i915_audio_component_cleanup(dev_priv);
> +	if (!intel_lpe_audio_teardown(dev_priv))
> +		return;
> +
> +	i915_audio_component_cleanup(dev_priv);

Here it would probably make more sense to just call both
unconditionally so we don't have to care what happened during
init.

>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_lpe_audio.c b/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> index 4970bf146c4a..a2984718d136 100644
> --- a/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_lpe_audio.c
> @@ -296,10 +296,10 @@ int intel_lpe_audio_init(struct drm_i915_private *dev_priv)
>   *
>   * release all the resources for LPE audio <-> i915 bridge.
>   */
> -void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
> +int intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_LPE_AUDIO(dev_priv))
> -		return;
> +		return -ENODEV;
>  
>  	lpe_audio_platdev_destroy(dev_priv);
>  
> @@ -307,6 +307,8 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>  
>  	dev_priv->audio.lpe.irq = -1;
>  	dev_priv->audio.lpe.platdev = NULL;
> +
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_lpe_audio.h b/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> index f848c5038714..030874623872 100644
> --- a/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_lpe_audio.h
> @@ -12,8 +12,8 @@ enum pipe;
>  enum port;
>  struct drm_i915_private;
>  
> -int  intel_lpe_audio_init(struct drm_i915_private *dev_priv);
> -void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
> +int intel_lpe_audio_init(struct drm_i915_private *dev_priv);
> +int intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
>  			    enum pipe pipe, enum port port,
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel