On Fri, 05 Nov 2021, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
