On Tue, Jun 02, 2015 at 11:39:03PM +0300, Vladimir Zapolskiy wrote:
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?
I mean val vs value, yes.
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.
Sure, but if we use that we could also just not make any change to the code except for changing the type of the argument when that's needed.
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.
Hrm, right. I suppose it's more useful than checkpatch cleanups. In any case I'm not sure it's relevant here where we're reading a value?
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.
I have to say I'm not sure I'm finding this is actually adding to the clarity - it was partly a cheap trick for compiler implementation but the int as boolean thing C has does also make this sort of code for mapping values onto bitfields more direct which is handy for low level systems programming like drivers.