On 15 February 2016 at 22:32, Mark Brown broonie@kernel.org wrote:
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.
Will take care of formatting. I'll also add some more details in commit message to show possible usage
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.
Agreed.
+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?
No functional change in snd_soc_dapm_connect_dai_link_widgets(). I have refactored it for individual pcm_runtime instead of complete soc_card.
+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.
Sure, will share separate patch for this change.
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.
will make the change in next patchset
+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.
Another API snd_soc_add_dai_link() already exists (added by Mengdong). How about renaming it to snd_soc_create_dai_link() ?
+config SND_SOC_DYNAMIC_DAILINK
booldefault yLike 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.
Got the point. I'll remove this completely.
+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.
Will split this patch further between preparation & actual new interfaces.
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);elsestrlcpy(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.
will modify the comment formatting.