Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy <vz@xxxxxxxxx>]

Hello Mark,

On 02.06.2015 23:36, Mark Brown wrote:
> On Tue, Jun 02, 2015 at 11:23:41PM +0300, Vladimir Zapolskiy wrote:
>> On 02.06.2015 22:45, Mark Brown wrote:
>>> On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
> 
>>>> +	unsigned int val = 0;
>>>> +
>>>> +	if (value)
>>>> +		val = 0x1 << WM5100_GP1_LVL_SHIFT;
> 
>>> Write this as an if/else so the reader doesn't have to wonder why you've
>>> missed the handling of the false case.  
> 
>> the only objection I have is that the resulting code will be two lines
>> longer. If you think this code is not clear enough (is "val" vs. "value"
>> misleading?), I'll change the rest of my patches, which contain the same
>> logic structure, please let me know.
> 
> Especially after the unrelated style change thing earlier on (which
> meant I was reading things more carefully than usual) it'd be good to
> make things as clear as possible - you're right that the val vs value
> thing isn't helping either.  

got your concern, will send an update.

> Like I say I am a bit surprised that the int/bool conversion doesn't do
> the right thing without any code changes other than the type of the
> parameter but ICBW, I didn't actually check.
> 

My tested compilers do the work right, but I can not be sure about the
whole variety of compilers, well, probably I should just follow the
standard, which in turn is expected to be quite volatile in future
revisions regarding usage of "_Bool" or future "bool" type.

If we assume the preferred Linux kernel style to use true/false
constants for boolean variables only (see the reference to
bool{init,return}.cocci), then even if bitwise and arithmetic operations
on bool type are understandable to a compiler, but probably "false <<
true" (or whatever is hidden under a variable name) is not quite
aesthetic for a human eye. This is not a technical argument though, and
I'm not sure if any clear code style policy related to the case is agreed.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html