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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 29 Dec 2014 16:30:04 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Jean-Francois Moine <moinejf@...e.fr>
Cc:	Liam Girdwood <lgirdwood@...il.com>, alsa-devel@...a-project.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] ASoC: simple-card: add multi-CODECs in DT

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.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)