[alsa-devel] [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Timur Tabi]

Mark Brown wrote:
> On Tue, Sep 13, 2011 at 12:59:35PM -0500, Timur Tabi wrote:
> 
>> -	iface = 0;
>> -
> 
> Random stylistic changes like this make review harder, especially when
> they're not mentioned in the changelog. 

This is not a random stylistic change.  My patch changes the way iface is
calculated.  Therefore, removing an unnecessary pre-initialization of iface is
both obvious and expected.

The "iface = 0" line is used in the original code because the driver did not
have a "default" case for the "switch" statement.  Since I added a default case,
there was no need to pre-initialize iface anymore.

If I had left that line in the driver, someone would have complained that I'm
pre-initializing iface unnecessarily.

> In this case it's not clear
> from the diff alone that there are no other places where iface gets
> written to

Ok, so that means that you need to look at the original code in order to
understand the change completely.  What's wrong with that?  I just don't see how
this is a valid criticism for any patch.

> An important aspect of review is checking that the change being made
> actually do what they're supposed to do, unexpected additional stuff is
> a red flag - you have to work out if it's something the sender meant to
> include, what it's supposed to do and if it's a good idea.

Again, I just don't understand this complaint of my patch.  I've carefully read
your email, and I re-examined my patch and the original code, and I still don't
think that there's anything wrong with the patch.

-- 
Timur Tabi
Linux kernel developer at Freescale