Re: [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Vladimir Zapolskiy <vz@xxxxxxxxx>]

Hello Mark,

On 02.06.2015 22:38, Mark Brown wrote:
> On Tue, Jun 02, 2015 at 02:09:13AM +0300, Vladimir Zapolskiy wrote:
> 
>> +		if (value)
>> +			val = RT5677_GPIO_OUT_HI(offset);
> 
> It seems like a greater variation in variable names might be called for
> here.

thank you for review, you mean "val" vs. "value" I guess. May be you
have a better register value naming in mind?

>>  		regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL2,
>> -			0x1 << (offset * 3 + 1), !!value << (offset * 3 + 1));
>> +				   RT5677_GPIO_OUT_MASK(offset), val);
> 
> Besides, isn't the minimal change here just to remove the !! (or do
> nothing)?

this particular change mainly addresses "clean up gpiolib callbacks"
target as it is stated in subject/commit message. Removing of "!!" might
be unsafe, because the input value is not necessary 0 or 1 at the moment.

>  C defines a mapping between boolean and integer values.
> 

As for today it is correct. In the kernel right now there is a movement
of replacing for instance explicit integer constants to false/true, when
they are used with variables of bool type, e.g. see
scripts/coccinelle/misc/bool{init,return}.cocci.

In my opinion changing GPIO level argument from int to bool should be
done, when all confusing cases like "!!false << (offset * 3 + 1)" above
(if just type of "value" is modified) are cleaned up in the code firstly.

--
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