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

[Pierre-Louis Bossart]

Looks mostly good, couple of comments below.

> +static int wsa881x_prepare(struct snd_pcm_substream *substream,
> +			   struct snd_soc_dai *dai)
> +{
> +	struct wsa881x_priv *wsa881x = dev_get_drvdata(dai->dev);
> +	int ret;
> +
> +	if (wsa881x->stream_prepared) {
> +		sdw_disable_stream(wsa881x->sruntime);
> +		sdw_deprepare_stream(wsa881x->sruntime);
> +		wsa881x->stream_prepared = false;
> +	}

in what scenario would you have a transition from a stream active to 
prepared?

> +
> +
> +	ret = sdw_prepare_stream(wsa881x->sruntime);
> +	if (ret)
> +		return ret;
> +
> +	/**
> +	 * 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?

> +	 * 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?

If this is due to the order with the PA, then where is the PA handled?


> +	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?

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?

> +		.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,
> +	},
> +};