[alsa-devel] [RFC 3/4] ASoC: Enable dynamic DAIlink insertion & removal

[Mark Brown]

On Mon, Feb 15, 2016 at 05:49:31PM +0530, Vaibhav Agarwal wrote:

> With the evolvement of modularized platforms, codecs can be
> dynamically added to/removed from a platform.
> Thus, there is a need to add FE/BE DAIs to an existing sound card
> in response to codec inserted/removed.

Like I say please format your changelogs normally.  It makes things much
easier to read.  I'm still not seeing a user or how this will work on
the client side here.

> Another kconfig option SND_SOC_DYNAMIC_DAILINK (default set to y)
> is added to avoid compilation issues for client (machine, codec)
> drivers with other kernel versions.

No, don't do this.  We don't care about random other kernel versions, if
we did the whole kernel would be full of ifdefs.  We have config options
for things like adding substantial size to the kernel.

> +int snd_soc_dapm_link_dai_widgets_component(struct snd_soc_card *card,
> +					    struct snd_soc_dapm_context *dapm);
> +void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card,
> +					   struct snd_soc_pcm_runtime *rtd);

Why void?

> +void dpcm_fe_disconnect(struct snd_soc_pcm_runtime *be, int stream);
>  void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream);

This seems like it should be a separate patch, it's not strongly tied to
the rest of the code.

> index 3dda0c4..44d8568 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -796,6 +796,8 @@ struct snd_soc_component {
>  
>  	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
>  	unsigned int registered_as_component:1;
> +	/* registered dynamically in response to dynamic DAI links */
> +	unsigned int dynamic_registered:1;

dynamically.

> +int snd_soc_add_dailink(struct snd_soc_card *card,
> +			struct snd_soc_dai_link *dai_link);
> +void snd_soc_remove_dailink(struct snd_soc_card *card, const char *link_name);

Everywhere else we write dai_link.

> +config SND_SOC_DYNAMIC_DAILINK
> +	bool
> +        default y
> +

Like I say I don't see a need for this but note also that the
indentation seems broken - might be worth checking this isn't creeping
in elsewhere.

> +static void soc_remove_component_controls(struct snd_soc_component *component)

This is only used once which means we're going to end up with two
different ways of removing controls, one of which is essentially never
used and hence likely to break.  It would be better to ensure that the
removal path is the same as much of the time as possible so that things
are more maintainable.  This is an issue through the patch, it feels
like it'd be clearer and easier to review if it first rearranged things
to split up things for reuse by dynamic addition and then separately
added new interfaces that do the dyanmic stuff.

> +
> +		long_name = control->name;
> +		prefix = component->name_prefix;
> +		name_len = sizeof(id.name);

name_len is completely pointless here...

> +		if (prefix)
> +			snprintf(id.name, name_len, "%s %s", prefix,
> +				 long_name);
> +		else
> +			strlcpy(id.name, long_name, sizeof(id.name));

...it's not even used here.  Just don't bother with the intermediate
variables, they make things harder to follow.

> +	/*
> +	 * should be done, only in case
> +	 * component probed after card instantiation
> +	 * assumptions:
> +	 * relevant DAI links are already removed
> +	 * mutex acquired for soc-card
> +	 * semaphore acquired for sound card
> +	 */

Please fix the
formatting of
this comment
so it looks more
like normal kernel
code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20160215/1d51bc56/attachment.sig>