Re: [PATCH v2 146/146] ASoC: soc-core: remove legacy style dai_link

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

Please don't top-post.

On 6/6/19 1:25 PM, Rojewski, Cezary wrote:
> Hmm, guess reviewing 001 proved redundant after all. Unless I got it wrong, you are removing code implemented in that very patch (the 001).

Not quite. There was already code to convert codecs and platforms to the 
new representation but the cpu part was missing. The first patch only 
deals with cpu dais. The last patch removes all the conversions for 
codec/platform/cpu and uses the new representation across the board, so 
there's more code removed in the last patch than added in the first.

> Any chance for eliminating ping-pong effect and doing the "right" changes from the get-go? Especially the renames are confusing here (s/cleanup_platform/cleanup_legacy/) if you intend to remove them soon after.

Using a ping-pong analogy for a 146-patch series is pushing it. It's 
first make then break to avoid bisect issues. And the names match what 
is used in the existing code. maybe the naming isn't to your liking but 
it's what has been used for a while.

Note that the last patch is going to break all the non-upstream machine 
drivers so you will have quite a bit of work to do on your own when you 
rebase.

> If there is no other way around it and solution is accepted, a note, perhaps in 001 would be helpful for future readers.
>
> Czarek
>
> > From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> >
> > All drivers switched to modern style dai_link
> > (= struct snd_soc_dai_link_component).
> > Let's remove legacy style dai_link.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> > ---
> > include/sound/soc.h  |  65 ++------------------
> > sound/soc/soc-core.c | 165 +++------------------------------------------------
> > 2 files changed, 12 insertions(+), 218 deletions(-)
> >
> > diff --git a/include/sound/soc.h b/include/sound/soc.h
> > index 0fa79b8..055e6d0 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -901,77 +901,33 @@ struct snd_soc_dai_link {
> > 	const char *stream_name;		/* Stream name */
> >
> > 	/*
> > -	 *	cpu_name
> > -	 *	cpu_of_node
> > -	 *	cpu_dai_name
> > -	 *
> > -	 * These are legacy style, and will be replaced to
> > -	 * modern style (= snd_soc_dai_link_component) in the future,
> > -	 * but, not yet supported so far.
> > -	 * If modern style was supported for CPU, all driver will switch
> > -	 * to use it, and, legacy style code will be removed from ALSA SoC.
> > -	 */
> > -	/*
> > 	 * You MAY specify the link's CPU-side device, either by device name,
> > 	 * or by DT/OF node, but not both. If this information is omitted,
> > 	 * the CPU-side DAI is matched using .cpu_dai_name only, which
> > hence
> > 	 * must be globally unique. These fields are currently typically used
> > 	 * only for codec to codec links, or systems using device tree.
> > 	 */
> > -	const char *cpu_name;
> > -	struct device_node *cpu_of_node;
> > 	/*
> > 	 * You MAY specify the DAI name of the CPU DAI. If this information is
> > 	 * omitted, the CPU-side DAI is matched using
> > .cpu_name/.cpu_of_node
> > 	 * only, which only works well when that device exposes a single DAI.
> > 	 */
> > -	const char *cpu_dai_name;
> > -
> > 	struct snd_soc_dai_link_component *cpus;
> > 	unsigned int num_cpus;
> >
> > 	/*
> > -	 *	codec_name
> > -	 *	codec_of_node
> > -	 *	codec_dai_name
> > -	 *
> > -	 * These are legacy style, it will be converted to modern style
> > -	 * (= snd_soc_dai_link_component) automatically in soc-core
> > -	 * if driver is using legacy style.
> > -	 * Driver shouldn't use both legacy and modern style in the same time.
> > -	 * If modern style was supported for CPU, all driver will switch
> > -	 * to use it, and, legacy style code will be removed from ALSA SoC.
> > -	 */
> > -	/*
> > 	 * You MUST specify the link's codec, either by device name, or by
> > 	 * DT/OF node, but not both.
> > 	 */
> > -	const char *codec_name;
> > -	struct device_node *codec_of_node;
> > 	/* You MUST specify the DAI name within the codec */
> > -	const char *codec_dai_name;
> > -
> > 	struct snd_soc_dai_link_component *codecs;
> > 	unsigned int num_codecs;
> >
> > 	/*
> > -	 *	platform_name
> > -	 *	platform_of_node
> > -	 *
> > -	 * These are legacy style, it will be converted to modern style
> > -	 * (= snd_soc_dai_link_component) automatically in soc-core
> > -	 * if driver is using legacy style.
> > -	 * Driver shouldn't use both legacy and modern style in the same time.
> > -	 * If modern style was supported for CPU, all driver will switch
> > -	 * to use it, and, legacy style code will be removed from ALSA SoC.
> > -	 */
> > -	/*
> > 	 * You MAY specify the link's platform/PCM/DMA driver, either by
> > 	 * device name, or by DT/OF node, but not both. Some forms of link
> > 	 * do not need a platform.
> > 	 */
> > -	const char *platform_name;
> > -	struct device_node *platform_of_node;
> > 	struct snd_soc_dai_link_component *platforms;
> > 	unsigned int num_platforms;
> >
> > @@ -1033,13 +989,6 @@ struct snd_soc_dai_link {
> > 	/* Do not create a PCM for this DAI link (Backend link) */
> > 	unsigned int ignore:1;
> >
> > -	/*
> > -	 * This driver uses legacy platform naming. Set by the core, machine
> > -	 * drivers should not modify this value.
> > -	 */
> > -	unsigned int legacy_platform:1;
> > -	unsigned int legacy_cpu:1;
> > -
> > 	struct list_head list; /* DAI link list of the soc card */
> > 	struct snd_soc_dobj dobj; /* For topology */
> > };
> > @@ -1699,15 +1648,11 @@ int
> > snd_soc_fixup_dai_links_platform_name(struct snd_soc_card *card,
> > 		if (!name)
> > 			return -ENOMEM;
> >
> > -		if (dai_link->platforms)
> > -			/* only single platform is supported for now */
> > -			dai_link->platforms->name = name;
> > -		else
> > -			/*
> > -			 * legacy mode, this case will be removed when all
> > -			 * derivers are switched to modern style dai_link.
> > -			 */
> > -			dai_link->platform_name = name;
> > +		if (!dai_link->platforms)
> > +			return -EINVAL;
> > +
> > +		/* only single platform is supported for now */
> > +		dai_link->platforms->name = name;
> > 	}
> >
> > 	return 0;
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index e069dfb..b28dda9 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -1052,167 +1052,18 @@ static void soc_remove_dai_links(struct
> > snd_soc_card *card)
> > 	}
> > }
> >
> > -static int snd_soc_init_cpu(struct snd_soc_card *card,
> > -			    struct snd_soc_dai_link *dai_link)
> > -{
> > -	struct snd_soc_dai_link_component *cpu = dai_link->cpus;
> > -
> > -	/*
> > -	 * REMOVE ME
> > -	 *
> > -	 * This is glue code for Legacy vs Modern dai_link.
> > -	 * This function will be removed if all derivers are switched to
> > -	 * modern style dai_link.
> > -	 * Driver shouldn't use both legacy and modern style in the same time.
> > -	 * see
> > -	 *	soc.h :: struct snd_soc_dai_link
> > -	 */
> > -	/* convert Legacy platform link */
> > -	if (!cpu) {
> > -		cpu = devm_kzalloc(card->dev,
> > -				   sizeof(struct snd_soc_dai_link_component),
> > -				   GFP_KERNEL);
> > -		if (!cpu)
> > -			return -ENOMEM;
> > -
> > -		dai_link->cpus		= cpu;
> > -		dai_link->num_cpus	= 1;
> > -		dai_link->legacy_cpu	= 1;
> > -
> > -		cpu->name	= dai_link->cpu_name;
> > -		cpu->of_node	= dai_link->cpu_of_node;
> > -		cpu->dai_name	= dai_link->cpu_dai_name;
> > -	}
> > -
> > -	if (!dai_link->cpus) {
> > -		dev_err(card->dev, "ASoC: DAI link has no CPUs\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -static int snd_soc_init_platform(struct snd_soc_card *card,
> > -				 struct snd_soc_dai_link *dai_link)
> > -{
> > -	struct snd_soc_dai_link_component *platform = dai_link->platforms;
> > -
> > -	/*
> > -	 * REMOVE ME
> > -	 *
> > -	 * This is glue code for Legacy vs Modern dai_link.
> > -	 * This function will be removed if all derivers are switched to
> > -	 * modern style dai_link.
> > -	 * Driver shouldn't use both legacy and modern style in the same time.
> > -	 * see
> > -	 *	soc.h :: struct snd_soc_dai_link
> > -	 */
> > -	/* convert Legacy platform link */
> > -	if (!platform) {
> > -		platform = devm_kzalloc(card->dev,
> > -				sizeof(struct snd_soc_dai_link_component),
> > -				GFP_KERNEL);
> > -		if (!platform)
> > -			return -ENOMEM;
> > -
> > -		dai_link->platforms	  = platform;
> > -		dai_link->num_platforms	  = 1;
> > -		dai_link->legacy_platform = 1;
> > -		platform->name		  = dai_link->platform_name;
> > -		platform->of_node	  = dai_link->platform_of_node;
> > -		platform->dai_name	  = NULL;
> > -	}
> > -
> > -	/* if there's no platform we match on the empty platform */
> > -	if (!platform->name &&
> > -	    !platform->of_node)
> > -		platform->name = "snd-soc-dummy";
> > -
> > -	return 0;
> > -}
> > -
> > -static void soc_cleanup_legacy(struct snd_soc_card *card)
> > -{
> > -	struct snd_soc_dai_link *link;
> > -	int i;
> > -	/*
> > -	 * FIXME
> > -	 *
> > -	 * this function should be removed with snd_soc_init_platform
> > -	 */
> > -
> > -	for_each_card_prelinks(card, i, link) {
> > -		if (link->legacy_platform) {
> > -			link->legacy_platform = 0;
> > -			link->platforms       = NULL;
> > -		}
> > -		if (link->legacy_cpu) {
> > -			link->legacy_cpu = 0;
> > -			link->cpus = NULL;
> > -		}
> > -	}
> > -}
> > -
> > -static int snd_soc_init_multicodec(struct snd_soc_card *card,
> > -				   struct snd_soc_dai_link *dai_link)
> > -{
> > -	/*
> > -	 * REMOVE ME
> > -	 *
> > -	 * This is glue code for Legacy vs Modern dai_link.
> > -	 * This function will be removed if all derivers are switched to
> > -	 * modern style dai_link.
> > -	 * Driver shouldn't use both legacy and modern style in the same time.
> > -	 * see
> > -	 *	soc.h :: struct snd_soc_dai_link
> > -	 */
> > -
> > -	/* Legacy codec/codec_dai link is a single entry in multicodec */
> > -	if (dai_link->codec_name || dai_link->codec_of_node ||
> > -	    dai_link->codec_dai_name) {
> > -		dai_link->num_codecs = 1;
> > -
> > -		dai_link->codecs = devm_kzalloc(card->dev,
> > -				sizeof(struct snd_soc_dai_link_component),
> > -				GFP_KERNEL);
> > -		if (!dai_link->codecs)
> > -			return -ENOMEM;
> > -
> > -		dai_link->codecs[0].name = dai_link->codec_name;
> > -		dai_link->codecs[0].of_node = dai_link->codec_of_node;
> > -		dai_link->codecs[0].dai_name = dai_link->codec_dai_name;
> > -	}
> > -
> > -	if (!dai_link->codecs) {
> > -		dev_err(card->dev, "ASoC: DAI link has no CODECs\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	return 0;
> > -}
> > +static struct snd_soc_dai_link_component dummy_link = COMP_DUMMY();
> >
> > static int soc_init_dai_link(struct snd_soc_card *card,
> > 			     struct snd_soc_dai_link *link)
> > {
> > -	int i, ret;
> > +	int i;
> > 	struct snd_soc_dai_link_component *codec;
> >
> > -	ret = snd_soc_init_cpu(card, link);
> > -	if (ret) {
> > -		dev_err(card->dev, "ASoC: failed to init cpu\n");
> > -		return ret;
> > -	}
> > -
> > -	ret = snd_soc_init_platform(card, link);
> > -	if (ret) {
> > -		dev_err(card->dev, "ASoC: failed to init multiplatform\n");
> > -		return ret;
> > -	}
> > -
> > -	ret = snd_soc_init_multicodec(card, link);
> > -	if (ret) {
> > -		dev_err(card->dev, "ASoC: failed to init multicodec\n");
> > -		return ret;
> > +	/* default Platform */
> > +	if (!link->platforms || !link->num_platforms) {
> > +		link->platforms = &dummy_link;
> > +		link->num_platforms = 1;
> > 	}
> >
> > 	for_each_link_codecs(link, i, codec) {
> > @@ -2059,7 +1910,7 @@ static void soc_check_tplg_fes(struct snd_soc_card
> > *card)
> > 				 card->dai_link[i].name);
> >
> > 			/* override platform component */
> > -			if (snd_soc_init_platform(card, dai_link) < 0) {
> > +			if (!dai_link->platforms) {
> > 				dev_err(card->dev, "init platform error");
> > 				continue;
> > 			}
> > @@ -2110,7 +1961,6 @@ static int soc_cleanup_card_resources(struct
> > snd_soc_card *card)
> > 	/* remove and free each DAI */
> > 	soc_remove_dai_links(card);
> > 	soc_remove_pcm_runtimes(card);
> > -	soc_cleanup_legacy(card);
> >
> > 	/* remove auxiliary devices */
> > 	soc_remove_aux_devices(card);
> > @@ -2867,7 +2717,6 @@ int snd_soc_register_card(struct snd_soc_card
> > *card)
> >
> > 		ret = soc_init_dai_link(card, link);
> > 		if (ret) {
> > -			soc_cleanup_legacy(card);
> > 			dev_err(card->dev, "ASoC: failed to init link %s\n",
> > 				link->name);
> > 			mutex_unlock(&client_mutex);
> > --
> > 2.7.4
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel