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 09:45:01 -0600	[thread overview]
Message-ID: <052d527b-77a5-172e-2b18-ca635242b440@linux.intel.com> (raw)
In-Reply-To: <20191220115629.8293-3-srinivas.kandagatla@linaro.org>

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,
> +	},
> +};
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel