Re: [’PATCH’ 2/3] ASoC: codecs: add new SoundWire-based SN624x

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

> +static const struct snd_soc_dapm_route sn6242_sdca_map[] = {
> +	{ "Headphone", NULL, "sn6242 HP" },
> +	{ "sn6242 MIC2", NULL, "Headset Mic" },
> +};
> +
> +static struct snd_soc_jack_pin senary_sdca_jack_pins[] = {
> +	{
> +		.pin    = "Headphone",
> +		.mask   = SND_JACK_HEADPHONE,
> +	},
> +	{
> +		.pin    = "Headset Mic",
> +		.mask   = SND_JACK_MICROPHONE,
> +	},
> +};
> +
> +static const char * const need_sdca_suffix[] = {
> +	"sn6242", "sn6244", "sn6247"
> +};
> +
> +int asoc_sdw_senary_sdca_jack_rtd_init(struct snd_soc_pcm_runtime *rtd, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_card *card = rtd->card;
> +	struct snd_soc_dapm_context *dapm = snd_soc_card_to_dapm(card);
> +	struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_component *component;
> +	struct snd_soc_jack *jack;
> +	int ret;
> +	int i;
> +
> +	component = dai->component;
> +	card->components = devm_kasprintf(card->dev, GFP_KERNEL,
> +					  "%s hs:%s",
> +					  card->components, component->name_prefix);
> +	if (!card->components)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ARRAY_SIZE(need_sdca_suffix); i++) {
> +		if (strstr(component->name_prefix, need_sdca_suffix[i])) {
> +			/* Add -sdca suffix for existing UCMs */
> +			card->components = devm_kasprintf(card->dev, GFP_KERNEL,
> +							  "%s-sdca", card->components);
> +			if (!card->components)
> +				return -ENOMEM;
> +			break;
> +		}
> +	}
> +
> +	if (strstr(component->name_prefix, "sn6242")) {
> +		ret = snd_soc_dapm_add_routes(dapm, sn6242_sdca_map,
> +					      ARRAY_SIZE(sn6242_sdca_map));
> +	} else if (strstr(component->name_prefix, "sn6244")) {
> +		ret = snd_soc_dapm_add_routes(dapm, sn6242_sdca_map,
> +					      ARRAY_SIZE(sn6242_sdca_map));
> +	} else if (strstr(component->name_prefix, "sn6247")) {
> +		ret = snd_soc_dapm_add_routes(dapm, sn6242_sdca_map,
> +					      ARRAY_SIZE(sn6242_sdca_map));

looks like all branches do the same thing, consider refactoring all this...

> +	} else {
> +		dev_err(card->dev, "%s is not supported\n", component->name_prefix);
> +		return -EINVAL;
> +	}
> +
> +	if (ret) {
> +		dev_err(card->dev, "senary sdca jack map addition failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_soc_card_jack_new_pins(rtd->card, "Headset Jack",
> +					 SND_JACK_HEADSET | SND_JACK_BTN_0 |
> +					 SND_JACK_BTN_1 | SND_JACK_BTN_2 |
> +					 SND_JACK_BTN_3,
> +					 &ctx->sdw_headset,
> +					 senary_sdca_jack_pins,
> +					 ARRAY_SIZE(senary_sdca_jack_pins));
> +	if (ret) {
> +		dev_err(rtd->card->dev, "Headset Jack creation failed: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	jack = &ctx->sdw_headset;
> +
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);
> +
> +	ret = snd_soc_component_set_jack(component, jack, NULL);
> +
> +	if (ret)
> +		dev_err(rtd->card->dev, "Headset Jack call-back failed: %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS(asoc_sdw_senary_sdca_jack_rtd_init, "SND_SOC_SDW_UTILS");
> +
> +int asoc_sdw_senary_sdca_jack_exit(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link)
> +{
> +	struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
> +
> +	if (!ctx->headset_codec_dev)
> +		return 0;
> +
> +	if (!SOC_SDW_JACK_JDSRC(ctx->mc_quirk))
> +		return 0;
> +
> +	device_remove_software_node(ctx->headset_codec_dev);
> +	put_device(ctx->headset_codec_dev);
> +	ctx->headset_codec_dev = NULL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(asoc_sdw_senary_sdca_jack_exit, "SND_SOC_SDW_UTILS");
> +
> +int asoc_sdw_senary_sdca_jack_init(struct snd_soc_card *card,
> +			       struct snd_soc_dai_link *dai_links,
> +			       struct asoc_sdw_codec_info *info,
> +			       bool playback)
> +{
> +	struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
> +	struct device *sdw_dev;
> +	int ret;
> +
> +	/*
> +	 * Jack detection should be only initialized once for headsets since
> +	 * the playback/capture is sharing the same jack
> +	 */
> +	if (ctx->headset_codec_dev)
> +		return 0;
> +
> +	sdw_dev = bus_find_device_by_name(&sdw_bus_type, NULL, dai_links->codecs[0].name);
> +	if (!sdw_dev)
> +		return -EPROBE_DEFER;
> +
> +	ret = senary_sdca_jack_add_codec_device_props(sdw_dev, ctx->mc_quirk);
> +	if (ret < 0) {
> +		put_device(sdw_dev);
> +		return ret;
> +	}
> +	ctx->headset_codec_dev = sdw_dev;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(asoc_sdw_senary_sdca_jack_init, "SND_SOC_SDW_UTILS");

quite a few lines from this file are just copy-pasted, is it time to try
and share these helpers?

> diff --git a/sound/soc/sdw_utils/soc_sdw_utils.c b/sound/soc/sdw_utils/soc_sdw_utils.c
> index d8db8fc5313e..603e57a4f09c 100644
> --- a/sound/soc/sdw_utils/soc_sdw_utils.c
> +++ b/sound/soc/sdw_utils/soc_sdw_utils.c
> @@ -1228,6 +1228,148 @@ struct asoc_sdw_codec_info codec_info_list[] = {
>  		},
>  		.dai_num = 1,
>  	},
> +	{
> +		.part_id = 0x6244,
> +		.name_prefix = "sn6242",
> +		.ignore_internal_dmic = true,
> +		.dais = {
> +			{
> +				.direction = {true, true},
> +				.dai_name = "sn6242-sdca-aif",
> +				.dai_type = SOC_SDW_DAI_TYPE_JACK,
> +				.dailink = {SOC_SDW_JACK_OUT_DAI_ID, SOC_SDW_JACK_IN_DAI_ID},
> +				.init = asoc_sdw_senary_sdca_jack_init,
> +				.exit = asoc_sdw_senary_sdca_jack_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_jack_rtd_init,
> +				.controls = generic_jack_controls,
> +				.num_controls = ARRAY_SIZE(generic_jack_controls),
> +				.widgets = generic_jack_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_jack_widgets),
> +			},
> +			{
> +				.direction = {true, false},
> +				.dai_name = "sn6242-sdca-aif2",
> +				.component_name = "sn6242",

not sure why there is a component_name only for capture?

> +				.dai_type = SOC_SDW_DAI_TYPE_AMP,
> +				.dailink = {SOC_SDW_AMP_OUT_DAI_ID, SOC_SDW_UNUSED_DAI_ID},
> +				.init = asoc_sdw_senary_amp_init,
> +				.exit = asoc_sdw_senary_amp_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_spk_rtd_init,
> +				.controls = generic_spk_controls,
> +				.num_controls = ARRAY_SIZE(generic_spk_controls),
> +				.widgets = generic_spk_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_spk_widgets),
> +				.quirk = SOC_SDW_CODEC_SPKR,
> +				.quirk_exclude = true,
> +			},
> +			{
> +				.direction = {false, true},
> +				.dai_name = "sn6242-sdca-aif3",
> +				.dai_type = SOC_SDW_DAI_TYPE_MIC,
> +				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_DMIC_DAI_ID},
> +				.rtd_init = asoc_sdw_senary_dmic_rtd_init,
> +				.quirk = SOC_SDW_CODEC_MIC,
> +				.quirk_exclude = true,
> +			},
> +		},
> +		.dai_num = 3,
> +	},
> +	{
> +		.part_id = 0x6242,
> +		.name_prefix = "sn6242",
> +		.ignore_internal_dmic = true,
> +		.dais = {
> +			{
> +				.direction = {true, true},
> +				.dai_name = "sn6242-sdca-aif",
> +				.dai_type = SOC_SDW_DAI_TYPE_JACK,
> +				.dailink = {SOC_SDW_JACK_OUT_DAI_ID, SOC_SDW_JACK_IN_DAI_ID},
> +				.init = asoc_sdw_senary_sdca_jack_init,
> +				.exit = asoc_sdw_senary_sdca_jack_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_jack_rtd_init,
> +				.controls = generic_jack_controls,
> +				.num_controls = ARRAY_SIZE(generic_jack_controls),
> +				.widgets = generic_jack_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_jack_widgets),
> +			},
> +			{
> +				.direction = {true, false},
> +				.dai_name = "sn6242-sdca-aif2",
> +				.component_name = "sn6242",
> +				.dai_type = SOC_SDW_DAI_TYPE_AMP,
> +				.dailink = {SOC_SDW_AMP_OUT_DAI_ID, SOC_SDW_UNUSED_DAI_ID},
> +				.init = asoc_sdw_senary_amp_init,
> +				.exit = asoc_sdw_senary_amp_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_spk_rtd_init,
> +				.controls = generic_spk_controls,
> +				.num_controls = ARRAY_SIZE(generic_spk_controls),
> +				.widgets = generic_spk_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_spk_widgets),
> +				.quirk = SOC_SDW_CODEC_SPKR,
> +				.quirk_exclude = true,
> +			},
> +			{
> +				.direction = {false, true},
> +				.dai_name = "sn6242-sdca-aif3",
> +				.dai_type = SOC_SDW_DAI_TYPE_MIC,
> +				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_DMIC_DAI_ID},
> +				.rtd_init = asoc_sdw_senary_dmic_rtd_init,
> +				.quirk = SOC_SDW_CODEC_MIC,
> +				.quirk_exclude = true,
> +			},
> +		},
> +		.dai_num = 3,
> +	},
> +	{
> +		.part_id = 0x6247,
> +		.name_prefix = "sn6242",
> +		.ignore_internal_dmic = true,
> +		.dais = {
> +			{
> +				.direction = {true, true},
> +				.dai_name = "sn6242-sdca-aif",
> +				.dai_type = SOC_SDW_DAI_TYPE_JACK,
> +				.dailink = {SOC_SDW_JACK_OUT_DAI_ID, SOC_SDW_JACK_IN_DAI_ID},
> +				.init = asoc_sdw_senary_sdca_jack_init,
> +				.exit = asoc_sdw_senary_sdca_jack_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_jack_rtd_init,
> +				.controls = generic_jack_controls,
> +				.num_controls = ARRAY_SIZE(generic_jack_controls),
> +				.widgets = generic_jack_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_jack_widgets),
> +			},
> +			{
> +				.direction = {true, false},
> +				.dai_name = "sn6242-sdca-aif2",
> +				.component_name = "sn6242",
> +				.dai_type = SOC_SDW_DAI_TYPE_AMP,
> +				.dailink = {SOC_SDW_AMP_OUT_DAI_ID, SOC_SDW_UNUSED_DAI_ID},
> +				.init = asoc_sdw_senary_amp_init,
> +				.exit = asoc_sdw_senary_amp_exit,
> +				.rtd_init = asoc_sdw_senary_sdca_spk_rtd_init,
> +				.controls = generic_spk_controls,
> +				.num_controls = ARRAY_SIZE(generic_spk_controls),
> +				.widgets = generic_spk_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_spk_widgets),
> +				.quirk = SOC_SDW_CODEC_SPKR,
> +				.quirk_exclude = true,
> +			},
> +			{
> +				.direction = {false, true},
> +				.dai_name = "sn6242-sdca-aif3",
> +				.dai_type = SOC_SDW_DAI_TYPE_MIC,
> +				.dailink = {SOC_SDW_UNUSED_DAI_ID, SOC_SDW_DMIC_DAI_ID},
> +				.rtd_init = asoc_sdw_senary_dmic_rtd_init,
> +				.widgets = generic_dmic_widgets,
> +				.num_widgets = ARRAY_SIZE(generic_dmic_widgets),
> +				.controls = generic_dmic_controls,
> +				.num_controls = ARRAY_SIZE(generic_dmic_controls),
> +				.quirk = SOC_SDW_CODEC_MIC,
> +				.quirk_exclude = true,
> +			},
> +		},
> +		.dai_num = 3,

all those 3 dais look identical, the only difference is the part_id.
Isn't there a better way to define and represent these dais?

> +	},
>  };
>  EXPORT_SYMBOL_NS(codec_info_list, "SND_SOC_SDW_UTILS");
>