[alsa-devel] [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150602/5aca2d5f/attachment.sig>