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.
Attachment:
signature.asc
Description: Digital signature
