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 <[email protected]>
> ---
> 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.
> +}