Re: [PATCH] watchdog: make imx2_wdt report boot status correctly

[Oskar Schirmer]

Hi Wolfram,

On Thu, Feb 16, 2012 at 14:35:10 +0100, Wolfram Sang wrote:
> besides this minor thing (which may be just personal taste)...
> 
> > +	case WDIOC_GETBOOTSTATUS:
> > +		val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> > +		new_value = 0;
> > +		if (val & IMX2_WDT_WRSR_TOUT)
> > +			new_value = WDIOF_CARDRESET;
> 
> I'd go for this to save some lines:
> 		new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;

A good alternative for sure.

Seen from the perspective of code compactness ("save some lines"),
one could try to be consequent here, and save the "val" variable
altogether, ending up with some "new_value = __raw_readw ... & ... ? ... : 0;"
But eventually there might be code readability issues, so it's
a good idea to keep the balance.

An argument for the original, longer version might be it is easier
to extend, when more flags need to be handled, like
"else if (val & ...) new_value = ...", while nested conditional
expressions would most likely become quite complex soon.

Actually, my personal taste doesn't show a preference for
one version or the other, so I'm ok with Your proposal, too.

  Oskar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/