On 15 February 2016 at 22:32, Mark Brown <broonie@xxxxxxxxxx> 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
>> + 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.
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);
>> + 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.
will modify the comment formatting.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
