Re: [PATCH 1/2] ASoC: qcom: sdm845: handle soundwire stream

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	broonie@kernel.org
Cc: alsa-devel@alsa-project.org, lgirdwood@gmail.com, perex@perex.cz,
	linux-kernel@vger.kernel.org, vkoul@kernel.org
Subject: Re: [PATCH 1/2] ASoC: qcom: sdm845: handle soundwire stream
Date: Wed, 18 Mar 2020 11:59:21 +0000	[thread overview]
Message-ID: <69c72f5a-e72e-b7b3-90cb-a7354dcb175d@linaro.org> (raw)
In-Reply-To: <8daeeb26-851b-8311-30f5-5d285ccbc255@linux.intel.com>

On 17/03/2020 13:07, Pierre-Louis Bossart wrote:
> 
>> @@ -45,11 +48,20 @@ static int sdm845_slim_snd_hw_params(struct 
>> snd_pcm_substream *substream,
>>       struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>       struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>       struct snd_soc_dai *codec_dai;
>> +    struct sdm845_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card);
>>       u32 rx_ch[SLIM_MAX_RX_PORTS], tx_ch[SLIM_MAX_TX_PORTS];
>> +    struct sdw_stream_runtime *sruntime;
>>       u32 rx_ch_cnt = 0, tx_ch_cnt = 0;
>>       int ret = 0, i;
>>       for_each_rtd_codec_dais(rtd, i, codec_dai) {
>> +        sruntime = snd_soc_dai_get_sdw_stream(codec_dai,
>> +                              substream->stream);
>> +        if (sruntime != ERR_PTR(-ENOTSUPP))
>> +            pdata->sruntime[cpu_dai->id] = sruntime;
>> +        else
>> +            pdata->sruntime[cpu_dai->id] = NULL;
>> +
> 
> Can you explain this part?
> The get_sdw_stream() is supposed to return what was set by 
> set_sdw_stream(), so if it's not supported isn't this an error?

In this case its not an error because we have
totally 3 codecs in this path.
One wcd934x Slimbus codec and two wsa881x Soundwire Codec.

wcd934x codec side will return ENOTSUPP which is not an error.

> 
>>           ret = snd_soc_dai_get_channel_map(codec_dai,
>>                   &tx_ch_cnt, tx_ch, &rx_ch_cnt, rx_ch);
>> @@ -425,8 +437,65 @@ static void  sdm845_snd_shutdown(struct 
>> snd_pcm_substream *substream)
>>       }
>>   }
>> +static int sdm845_snd_prepare(struct snd_pcm_substream *substream)
>> +{
>> +    struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +    struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> +    struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +    struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
>> +    int ret;
>> +
>> +    if (!sruntime)
>> +        return 0;
> 
> same here, isn't this an error?

These callbacks are used for other dais aswell in this case
HDMI, Slimbus and Soundwire, so sruntime being null is not treated as error.

> 
>> +    if (data->stream_prepared[cpu_dai->id]) {
>> +        sdw_disable_stream(sruntime);
>> +        sdw_deprepare_stream(sruntime);
>> +        data->stream_prepared[cpu_dai->id] = false;
>> +    }
>> +
>> +    ret = sdw_prepare_stream(sruntime);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /**
>> +     * NOTE: there is a strict hw requirement about the ordering of port
>> +     * enables and actual WSA881x PA enable. PA enable should only 
>> happen
>> +     * after soundwire ports are enabled if not DC on the line is
>> +     * accumulated resulting in Click/Pop Noise
>> +     * PA enable/mute are handled as part of codec DAPM and digital 
>> mute.
>> +     */
>> +
>> +    ret = sdw_enable_stream(sruntime);
>> +    if (ret) {
>> +        sdw_deprepare_stream(sruntime);
>> +        return ret;
>> +    }
>> +    data->stream_prepared[cpu_dai->id] = true;
>> +
>> +    return ret;
>> +}
>> +
>> +static int sdm845_snd_hw_free(struct snd_pcm_substream *substream)
>> +{
>> +    struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +    struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
>> +    struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +    struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
>> +
>> +    if (sruntime && data->stream_prepared[cpu_dai->id]) {
> 
> and here?
> 
> Really wondering where the stream is actually allocated and set.

Controller is allocating the runtime in this case.

> 
>> +        sdw_disable_stream(sruntime);
>> +        sdw_deprepare_stream(sruntime);
>> +        data->stream_prepared[cpu_dai->id] = false;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct snd_soc_ops sdm845_be_ops = {
>>       .hw_params = sdm845_snd_hw_params,
>> +    .hw_free = sdm845_snd_hw_free,
>> +    .prepare = sdm845_snd_prepare,
>>       .startup = sdm845_snd_startup,
>>       .shutdown = sdm845_snd_shutdown,
>>   };
>>