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

[Srinivas Kandagatla]

On 20/12/2019 15:45, Pierre-Louis Bossart wrote:
>>
>> +
>> +    /**
>> +     * NOTE: there is a strict hw requirement about the ordering of port
>> +     * enables and actual PA enable. PA enable should only happen after
> 
> PA == power amplifiers?
Yes.
> 
>> +     * soundwire ports are enabled if not DC on the line is accumlated
> 
> accumulated
> 
>> +     * resulting in Click/Pop Noise
>> +     */
>> +
>> +    ret = sdw_enable_stream(wsa881x->sruntime);
> 
> I guess this answers to my question above, you are not using the 'usual' 
> mapping between ALSA states and SoundWire stream states. 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.

> 
>> +    if (ret) {
>> +        sdw_deprepare_stream(wsa881x->sruntime);
>> +        return ret;
>> +    }
>> +    wsa881x->stream_prepared = true;
>> +
>> +    return ret;
>> +}
>> +
>> +static int wsa881x_hw_params(struct snd_pcm_substream *substream,
>> +                 struct snd_pcm_hw_params *params,
>> +                 struct snd_soc_dai *dai)
>> +{
>> +    struct wsa881x_priv *wsa881x = dev_get_drvdata(dai->dev);
>> +    int i;
>> +
>> +    wsa881x->active_ports = 0;
>> +    for (i = 0; i < WSA881X_MAX_SWR_PORTS; i++) {
>> +        if (!wsa881x->port_enable[i])
>> +            continue;
>> +
>> +        wsa881x->port_config[wsa881x->active_ports] =
>> +                            wsa881x_pconfig[i];
>> +        wsa881x->active_ports++;
>> +    }
>> +
>> +    return sdw_stream_add_slave(wsa881x->slave, &wsa881x->sconfig,
>> +                    wsa881x->port_config, wsa881x->active_ports,
>> +                    wsa881x->sruntime);
>> +}
>> +
>> +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.

> 
> if (wsa881x->stream_prepared) {
>      sdw_disable_stream(wsa881x->sruntime);
>      sdw_deprepare_stream(wsa881x->sruntime);
>      wsa881x->stream_prepared = false;
> }
> 
>> +    sdw_stream_remove_slave(wsa881x->slave, wsa881x->sruntime);
>> +    wsa881x->stream_prepared = false;
>> +
>> +    return 0;
>> +}
>> +
> 
>> +static struct snd_soc_dai_driver wsa881x_dais[] = {
>> +    [0] = {
> 
> is that [0] needed?
Not really needed!

> 
>> +        .name = "SPKR",
>> +        .id = 0,
>> +        .playback = {
>> +            .stream_name = "SPKR Playback",
>> +            .rate_max = 48000,
>> +            .rate_min = 48000,
>> +            .channels_min = 1,
>> +            .channels_max = 1,
>> +        },
>> +        .ops = &wsa881x_dai_ops,
>> +    },
>> +};