Re: [PATCH] ASoC: Intel: sof_cs42l42: add support for adl_mx98360a_cs4242 and BT offload audio

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-05-04 2:24 PM, Terry Chen wrote:
> This patch adds driver data for adl_mx98360a_cs4242 which supports
> two max98360a speaker amplifiers on SSP1 and cs42l42 headphone codec
> on SSP0 running on ADL platform. Also add the capability to machine
> driver of creating DAI Link for BT offload. Although BT offload
> always uses SSP2 port but we reserve the flexibility to assign
> the port number in macro.


Title's length seems off. Please shorten it a bit. Commit message 
formatting (line width) is not cohevise across the lines either.

Another suggestion: skip redundant bits at the front e.g.: s/This patch 
adds/Add/. When adding new code (new functionality), it's good to 
plainly state: "To be able to do X we add capability Y".
The last sentence in your message is a side note. It's good that you're 
being transparent here, my only advice is that you add a new line before 
such notes as these are not the main dish here, really.

Title suggest you're doing two things here - adding support for some ADL 
configuration (1) and BT audio offload (2). Recommendation is to 
splitting non-atomic changes so each patch in the series answers single 
problem.

> Signed-off-by: Terry Chen <terry_chen@wistron.corp-partner.google.com>
> ---
>   sound/soc/intel/boards/sof_cs42l42.c          | 88 ++++++++++++++++++-
>   .../intel/common/soc-acpi-intel-adl-match.c   |  8 ++
>   2 files changed, 92 insertions(+), 4 deletions(-)
> 

...

> @@ -278,6 +284,13 @@ static struct snd_soc_dai_link_component dmic_component[] = {
>   	}
>   };
>   
> +static struct snd_soc_dai_link_component dummy_component[] = {
> +	{
> +		.name = "snd-soc-dummy",
> +		.dai_name = "snd-soc-dummy-dai",
> +	}
> +};

Isn't there some SND_SOC_DAILINK_DEF() macro for that already?

>   static int create_spk_amp_dai_links(struct device *dev,
>   				    struct snd_soc_dai_link *links,
>   				    struct snd_soc_dai_link_component *cpus,
> @@ -467,9 +480,52 @@ static int create_hdmi_dai_links(struct device *dev,
>   	return -ENOMEM;
>   }
>   
> +static int create_bt_offload_dai_links(struct device *dev,
> +				       struct snd_soc_dai_link *links,
> +				       struct snd_soc_dai_link_component *cpus,
> +				       int *id, int ssp_bt)
> +{
> +	int ret = 0;

Looks like there is no need for 'ret' variable here.

> +
> +	/* bt offload */
> +	if (!(sof_cs42l42_quirk & SOF_BT_OFFLOAD_PRESENT))
> +		return 0;
> +
> +	links[*id].name = devm_kasprintf(dev, GFP_KERNEL, "SSP%d-BT",
> +					 ssp_bt);

Current limit is 100.

> +	if (!links[*id].name)
> +		goto devm_err;

This 'goto' statement seems redundant. I might be missing something but 
returning -ENOMEM should suffice. Label 'devm_err' does not do anything 
special anyway.

> +
> +	links[*id].id = *id;
> +	links[*id].codecs = dummy_component;
> +	links[*id].num_codecs = ARRAY_SIZE(dummy_component);
> +	links[*id].platforms = platform_component;
> +	links[*id].num_platforms = ARRAY_SIZE(platform_component);
> +
> +	links[*id].dpcm_playback = 1;
> +	links[*id].dpcm_capture = 1;
> +	links[*id].no_pcm = 1;
> +	links[*id].cpus = &cpus[*id];
> +	links[*id].num_cpus = 1;
> +
> +	links[*id].cpus->dai_name = devm_kasprintf(dev, GFP_KERNEL,
> +						   "SSP%d Pin",
> +						   ssp_bt);

Ditto - limit 100. Applies to every single occurence below too.

> +	if (!links[*id].cpus->dai_name)
> +		goto devm_err;
> +
> +	(*id)++;

Are the parenthesis needed? Should be save to drop them.

> +
> +	return 0;
> +
> +devm_err:
> +	return ret;

Once 'ret/devm_err' situation is clarified, this block can be dropped.

> +}