Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

Hi Pierre,

A couple of comments below

On Wed, Aug 21, 2019 at 03:17:19PM -0500, Pierre-Louis Bossart wrote:
> These callbacks are invoked when a matching hw_params/hw_free() DAI
> operation takes place, and will result in IPC operations with the SOF
> firmware.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/sof/intel/hda.c | 66 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index e754058e3679..1e84ea9e6fce 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -53,6 +53,70 @@ static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
>  					0);
>  }
>  
> +static int sdw_config_stream(void *arg, void *s, void *dai,
> +			     void *params, int link_id, int alh_stream_id)

I realise, that these function prototypes aren't being introduced by these 
patches, but just wondering whether such overly generic prototype is really 
a good idea here, whether some of those "void *" pointers could be given 
real types. The first one could be "struct device *" etc.

> +{
> +	struct snd_sof_dev *sdev = arg;
> +	struct snd_soc_dai *d = dai;
> +	struct sof_ipc_dai_config config;
> +	struct sof_ipc_reply reply;
> +	int ret;
> +	u32 size = sizeof(config);
> +
> +	memset(&config, 0, size);
> +	config.hdr.size = size;
> +	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
> +	config.type = SOF_DAI_INTEL_ALH;
> +	config.dai_index = (link_id << 8) | (d->id);
> +	config.alh.stream_id = alh_stream_id;

Entirely up to you, in such cases I usually do something like

+	struct sof_ipc_dai_config config = {
+		.type = SOF_DAI_INTEL_ALH,
+		.hre = {
+			.size = sizeof(config),
+			.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG,
+			...

which then also avoids a memset(). But that's mostly a matter of personal 
preference, since this is on stack, the compiler would probably internally 
anyway translate the above initialisation to a memset() with all the 
following assignments.

> +
> +	/* send message to DSP */
> +	ret = sof_ipc_tx_message(sdev->ipc,
> +				 config.hdr.cmd, &config, size, &reply,
> +				 sizeof(reply));
> +	if (ret < 0) {
> +		dev_err(sdev->dev,
> +			"error: failed to set DAI hw_params for link %d dai->id %d ALH %d\n",

Are readers really expected to understand what "dai->id" means? Wouldn't 
"DAI ID" be friendlier, although I understand you - who might not know 
what "x->y" stands for?.. ;-)

> +			link_id, d->id, alh_stream_id);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sdw_free_stream(void *arg, void *s, void *dai, int link_id)
> +{
> +	struct snd_sof_dev *sdev = arg;
> +	struct snd_soc_dai *d = dai;
> +	struct sof_ipc_dai_config config;
> +	struct sof_ipc_reply reply;
> +	int ret;
> +	u32 size = sizeof(config);
> +
> +	memset(&config, 0, size);
> +	config.hdr.size = size;
> +	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
> +	config.type = SOF_DAI_INTEL_ALH;
> +	config.dai_index = (link_id << 8) | d->id;
> +	config.alh.stream_id = 0xFFFF; /* invalid value on purpose */

ditto

> +
> +	/* send message to DSP */
> +	ret = sof_ipc_tx_message(sdev->ipc,
> +				 config.hdr.cmd, &config, size, &reply,
> +				 sizeof(reply));
> +	if (ret < 0) {
> +		dev_err(sdev->dev,
> +			"error: failed to free stream for link %d dai->id %d\n",
> +			link_id, d->id);

ditto

> +	}
> +
> +	return ret;
> +}
> +
> +static const struct sdw_intel_ops sdw_callback = {
> +	.config_stream = sdw_config_stream,
> +	.free_stream = sdw_free_stream,
> +};
> +
>  static int hda_sdw_init(struct snd_sof_dev *sdev)
>  {
>  	acpi_handle handle;
> @@ -67,6 +131,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>  	res.mmio_base = sdev->bar[HDA_DSP_BAR];
>  	res.irq = sdev->ipc_irq;
>  	res.parent = sdev->dev;
> +	res.ops = &sdw_callback;
> +	res.arg = sdev;
>  
>  	sdw = sdw_intel_init(handle, &res);
>  	if (!sdw) {

Hm, looks like this function is using spaces for indentation... Let me check 
if this is coming from an earlier patch

Thanks
Guennadi

> -- 
> 2.20.1
>