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.