Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 01, 2014 at 04:33:31PM +0100, Thierry Reding wrote:
> On Mon, Dec 01, 2014 at 02:48:47PM +0100, Hans Verkuil wrote:
> > Hi Thierry,
> > 
> > Thanks for the review, see my comments below.
> > 
> > On 12/01/2014 02:15 PM, Thierry Reding wrote:
> > > On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
> [...]
> > >> +{
> > >> +	switch (type) {
> > >> +	case HDMI_INFOFRAME_TYPE_VENDOR: return "Vendor";
> > >> +	case HDMI_INFOFRAME_TYPE_AVI: return "Auxiliary Video Information (AVI)";
> > >> +	case HDMI_INFOFRAME_TYPE_SPD: return "Source Product Description (SPD)";
> > >> +	case HDMI_INFOFRAME_TYPE_AUDIO: return "Audio";
> > > 
> > > I'd prefer "case ...:" and "return ...;" on separate lines for
> > > readability.
> > 
> > I actually think that makes it *less* readable. If you really want that, then I'll
> > change it, but I would suggest that you try it yourself first to see if it is
> > really more readable for you. It isn't for me, so I'll keep this for the next
> > version.
> 
> I did, and I still think separate lines are more readable, especially if
> you throw in a blank line after the "return ...;". Anyway, I could keep
> my OCD in check if it weren't for the fact that half of these are the
> cause for checkpatch to complain. And then if you change the ones that
> checkpatch wants you to change, all the others would be inconsistent and
> then I'd complain about the inconsistency...

Throwing in my own unasked-for bikshed opinion ;-)

I agree with Thierry that it's more readable since this way the key and
the value can be lined up easily. That's also possible with the
single-line case/return like you do with some more tabs, but usually means
all the lines overflow the 80 limit badly. So only really works for small
defines/values. So my rule of thumb is
- go with single-line case/return and align them
- but break the lines if they would be too long.

Now back to doing useful stuff for me.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch