Re: [PATCH v5 3/4] ASoC: simple-card: add multi-CODECs in DT

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Tue, Nov 25, 2014 at 01:30:14PM +0100, Jean-Francois Moine wrote:

> This patch allows many CODECs per link to be defined in the device tree.

It's also quite big and fiddly and hard to read, the changes that are
being made aren't blindingly obvious and there's quite a few of them.
As I've said before it's really importat that changes are clear and easy
to read, if the code is complex or surprising then the changelog needs
to be that bit more detailed to make thigs clear.  Things like talking
about why the code is being moved out and how it is being transformed
would be really helpful with this one, it's not enough to know the
overall goal of the patch, I also need to know how the patch is intended
to achieve that.

I think this is mostly OK but a couple of things...

> -Example 2 - many DAI links:
> +Example 2 - many DAI links and multi-CODECs:

I'd be much happier with a new example here rather than modifying the
old one.

> @@ -365,8 +359,18 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>  	 */
>  	if (!cpu_args)
>  		dai_link->cpu_dai_name = NULL;
> +	goto out;
>  
>  dai_link_of_err:

This goto out thing here is messy, it's not our normal coding style and
is error prone - better to just duplicate a small amount of cleanup.

> +	for (i = 0, component = dai_link->codecs;
> +	     i < dai_link->num_codecs;
> +	     i++, component++) {
> +		if (!component->of_node)
> +			break;

What's this break doing here, why might we be missing a node and why
should we skip all remaining components rather than just this one as a
result - a continue would be less surprising.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]