Re: [PATCH] ASoC: Intel: boards: add stereo playback by woofer speaker

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

Hi Mac,

> +#define SOF_RT1011_SPEAKER_WL		BIT(0)
> +#define SOF_RT1011_SPEAKER_WR		BIT(1)
> +#define SOF_RT1011_SPEAKER_TL		BIT(2)
> +#define SOF_RT1011_SPEAKER_TR		BIT(3)
> +#define SPK_CH 4

use a prefix maybe for consistency?
It's also unclear why this is needed when you can have 2 or more 
channels, and looking below

> +
> +/* Default: Woofer+Tweeter speakers  */

It's more like ALL devices have Woofers.
Some devices don't have tweeters.

the WL and WR quirks are always on apparently.

> +static unsigned long sof_rt1011_quirk = SOF_RT1011_SPEAKER_WL | SOF_RT1011_SPEAKER_WR |
> +					SOF_RT1011_SPEAKER_TL | SOF_RT1011_SPEAKER_TR;
> +
> +static int sof_rt1011_quirk_cb(const struct dmi_system_id *id)
> +{
> +	sof_rt1011_quirk = (unsigned long)id->driver_data;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id sof_rt1011_quirk_table[] = {
> +	{
> +		.callback = sof_rt1011_quirk_cb,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Google"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Palkia"),
> +	},
> +		.driver_data = (void *)(SOF_RT1011_SPEAKER_WL |
> +					SOF_RT1011_SPEAKER_WR),
> +	},
> +	{
> +	}
> +};

> +static const struct snd_soc_dapm_widget cml_rt1011_tt_widgets[] = {
> +	SND_SOC_DAPM_SPK("TL Ext Spk", NULL),
> +	SND_SOC_DAPM_SPK("TR Ext Spk", NULL),
> +};
> +
>   static const struct snd_soc_dapm_route cml_rt1011_rt5682_map[] = {
>   	/*speaker*/
>   	{"TL Ext Spk", NULL, "TL SPO"},

Something's not right, if I look at the code after applying this patch I 
get:

static const struct snd_soc_dapm_route cml_rt1011_rt5682_map[] = {
	/*speaker*/
	{"TL Ext Spk", NULL, "TL SPO"},
	{"TR Ext Spk", NULL, "TR SPO"},

That's duplicaged from [1]

> @@ -82,6 +118,12 @@ static const struct snd_soc_dapm_route cml_rt1011_rt5682_map[] = {
>   	{"DMic", NULL, "SoC DMIC"},
>   };
>   
> +static const struct snd_soc_dapm_route cml_rt1011_tt_map[] = {
> +	/*TL/TR speaker*/
> +	{"TL Ext Spk", NULL, "TL SPO" },
> +	{"TR Ext Spk", NULL, "TR SPO" },
> +};

[1] we should remove the tweeeter maps in cml_rt1011_rt5682_map, no?

>   static int cml_rt5682_hw_params(struct snd_pcm_substream *substream,
>   				struct snd_pcm_hw_params *params)
>   {
> @@ -192,31 +263,52 @@ static int cml_rt1011_hw_params(struct snd_pcm_substream *substream,
>   		 * The feedback is captured for each codec individually.
>   		 * Hence all 4 codecs use 1 Tx slot each for feedback.
>   		 */
> -		if (!strcmp(codec_dai->component->name, "i2c-10EC1011:00")) {
> -			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> -						       0x4, 0x1, 4, 24);
> -			if (ret < 0)
> -				break;
> -		}
> -		if (!strcmp(codec_dai->component->name, "i2c-10EC1011:02")) {
> -			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> -						       0x1, 0x1, 4, 24);
> -			if (ret < 0)
> -				break;
> -		}
> -		/* TDM Rx slot 2 is used for Right Woofer & Tweeters pair */
> -		if (!strcmp(codec_dai->component->name, "i2c-10EC1011:01")) {
> -			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> -						       0x8, 0x2, 4, 24);
> -			if (ret < 0)
> -				break;
> -		}
> -		if (!strcmp(codec_dai->component->name, "i2c-10EC1011:03")) {
> -			ret = snd_soc_dai_set_tdm_slot(codec_dai,
> -						       0x2, 0x2, 4, 24);
> -			if (ret < 0)
> -				break;
> +		if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_TL |
> +					SOF_RT1011_SPEAKER_TR)) {
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:00")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x4, 0x1, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
> +
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:02")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x1, 0x1, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
> +
> +			/* TDM Rx slot 2 is used for Right Woofer & Tweeters pair */
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:01")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x8, 0x2, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
> +
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:03")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x2, 0x2, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
> +		} else {
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:00")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x4, 0x1, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
> +
> +			if (!strcmp(codec_dai->component->name, "i2c-10EC1011:01")) {
> +				ret = snd_soc_dai_set_tdm_slot(codec_dai,
> +							       0x8, 0x2, 4, 24);
> +				if (ret < 0)
> +					break;
> +			}
>   		}

the if/else case can be simplified. The baseline is two woofers, so they 
can be added unconditionally, and then you can add what's missing for 
the tweeters. That way you have a consistent way of handling the TL/TR 
quirk.
>   static int snd_cml_rt1011_probe(struct platform_device *pdev)
>   {
> +	struct snd_soc_dai_link_component *rt1011_dais_components;
> +	struct snd_soc_codec_conf *rt1011_dais_confs;
>   	struct card_private *ctx;
>   	struct snd_soc_acpi_mach *mach;
>   	const char *platform_name;
> -	int ret;
> +	int ret, i;
>   
>   	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);

D'oh! Did we again let this slip in?

cml_rt1011_rt5682.c:    ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), 
GFP_ATOMIC);
sof_da7219_max98373.c:  ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), 
GFP_ATOMIC);

This should be fixed in a separate patch, we don't need th ATOMIC 
attribute in any machine drivers - copy-paste!

>   	if (!ctx)
> @@ -456,6 +541,59 @@ static int snd_cml_rt1011_probe(struct platform_device *pdev)
>   	snd_soc_card_cml.dev = &pdev->dev;
>   	platform_name = mach->mach_params.platform;
>   
> +	dmi_check_system(sof_rt1011_quirk_table);
> +
> +	dev_info(&pdev->dev, "sof_rt1011_quirk = %lx\n", sof_rt1011_quirk);
> +
> +	if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_TL |
> +				SOF_RT1011_SPEAKER_TR)) {
> +		rt1011_dais_confs = devm_kzalloc(&pdev->dev,
> +					sizeof(struct snd_soc_codec_conf) *
> +					SPK_CH, GFP_KERNEL);
> +
> +		rt1011_dais_components = devm_kzalloc(&pdev->dev,
> +					sizeof(struct snd_soc_dai_link_component) *
> +					SPK_CH, GFP_KERNEL);
> +
> +		for (i = 0; i < SPK_CH; i++) {
> +			rt1011_dais_confs[i].dlc.name = devm_kasprintf(&pdev->dev,
> +								GFP_KERNEL,
> +								"i2c-10EC1011:0%d",
> +								i);

Check for NULL and return -ENOMEM for all 3 devm_ calls above?