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

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

On Fri, 05 Nov 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

Right.

>
>>  }
>>  
>>  /**
>> @@ -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.

Yeah, why not.

Thanks for the review, pushed everything except this patch, will
follow-up.


BR,
Jani.


>
>>  }
>> 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

-- 
Jani Nikula, Intel Open Source Graphics Center