Re: [alsa-devel] [PATCH v10 2/2] ASoC: codecs: add wsa881x amplifier support - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, broonie@kernel.org
Cc: robh@kernel.org, alsa-devel@alsa-project.org,
	bgoswami@codeaurora.org, spapothi@codeaurora.org,
	lgirdwood@gmail.com, vkoul@kernel.org
Subject: Re: [alsa-devel] [PATCH v10 2/2] ASoC: codecs: add wsa881x amplifier support
Date: Fri, 20 Dec 2019 11:38:16 -0600	[thread overview]
Message-ID: <5205e403-f4ec-0850-b813-d5250d121377@linux.intel.com> (raw)
In-Reply-To: <30c40a3b-2d70-b1a2-117b-63f0ece65cef@linaro.org>

>> Enabling the stream will cause a bank switch and (zero?) data to be 
>> transmitted, is this intentional?
>>
> I guess Yes!
> Myself and Vinod spent few weeks understanding the audio glitches if we 
> enable PA before soundwire ports, and finally hw guys came in with this 
> information, that PA has to be disabled before soundwire ports are enabled.
> 
>> If this is due to the order with the PA, then where is the PA handled?
>>
> 
> PA enable/mute are handled as part of DAPM and digital mute.

adding a comment would be nice then.

We can expect additional deviations from the initial ALSA->SoundWire 
mapping, it seems that the notion of 'prepare' was interpreted in 
different ways and some devices need to combine prepare/enable and 
disable/deprepare in the .trigger callback, i.e. prepare is used to 
'prepare streaming'. Others take a lot of time and and the 'prepare' is 
really 'prepare analog/power resources', which and require the two parts 
to be split. The standard is ambiguous on this, so both interpretations 
are legit, so we'll probably have to adjust in the SoundWire core at 
some point.

>>> +static int wsa881x_hw_free(struct snd_pcm_substream *substream,
>>> +               struct snd_soc_dai *dai)
>>> +{
>>> +    struct wsa881x_priv *wsa881x = dev_get_drvdata(dai->dev);
>>> +
>>> +    sdw_disable_stream(wsa881x->sruntime);
>>> +    sdw_deprepare_stream(wsa881x->sruntime);
>>
>> This works if you do a hw_params->prepare->hw_free transition, but 
>> isn't it possible to have hw_params->hw_free as well? In that case the 
>> stream would not enabled/prepared, so shouldn't you have the same test 
>> as in prepare?
> 
> Am not 100% sure if we would just have hw_params->hw_free, If that is 
> true, then yes we need the same check here too. However soundwire core 
> should throw invalid state error in such cases too.

It's probably better if you proactively check, we've seen cases where 
when the FE hw_params failed, the BE hw_free was called without the BE 
hw_params being invoked, so you should really test that everything was 
properly allocated and not rely on another state machine/framework.

With that feel free to add my tag below for the next revision

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel