On Thu, Sep 15, 2011 at 10:16:20AM -0500, Timur Tabi wrote:
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.
No, really it is.
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.
It's not that, there's a common idiom in older ASoC drivers where we initialize the registers we're updating in things like hw_params() to zero then run through oring in all the various bitfields they update before a final write at the end. The code is using that, it just happens that we end up with only one bitfield to update. An even better change would be to use snd_soc_update_bits() to make sure only the relevant bitfield is touched which is the current state of the art in idioms for these things.
If I had left that line in the driver, someone would have complained that I'm pre-initializing iface unnecessarily.
That's also the case in the existing code, it's just that the assignment is masking a bug with the default case (and if we didn't consider that a bug then of course there's a change to the default behaviour which also ought to be noted).
In any case, the issue here isn't that the new code is wrong - I'm just pointing out that you're making the patch harder to review because you're making changes beyond a straight substitution in of snd_pcm_format_width() which increases the review difficulty of what should be a trivial review.
Based on the changelog the first thing I'd look for when reviewing is that we've got a series of mechanical 1:1 substitutions of references to SNDRV_PCM_FORMAT and friends with references to snd_pcm_format_width() and friends. If you'd just added a note about the style change it'd have been fine as I'd have been expecting to see changes more like what you've got.
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.
Review cost is an issue, not just in terms of it being less effort but also in terms of increasing the quality of the review - the more that the reviewer has to infer what's going on the greater the chances that they'll infer something that isn't there or miss things they should have seen. Extra changes that sneak in are a classic source of bugs, things being missing from the changelog often means that either they were debug code that went in by accident or they're changes that haven't been thought through properly.
Had the style change come out all in one diff hunk it'd probably have been OK but what actually happened was that the first thing seen was a removal of an assignment to iface which stopped me short. The fact that you generally write fairly verbose and clear changlogs makes this particularly surprising.
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.
There's nothing wrong with the code itself, the issue was about how the process can flow more smoothly.