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

[Vladimir Zapolskiy]

On 02.06.2015 23:50, Mark Brown wrote:
> 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?

That's the question of bool type comprehension in my opinion. If bool is
seen from mathematical point of view, then arithmetic and bitwise
operations (except inapplicable shift) may be applied only if the result
is expected to be of bool type as well.

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

IIRC C95 doesn't define bool or _Bool at all, C99 implementations store
one bool value in one "char" (or one byte/8 bit memory cell), so
personally I'm not sure if arithmetic over boolean is so different from
arithmetic over char/unsigned char (with exception of implicit type and
value conversion).

Of course I'm not a C standard writer, but I have a feeling that one day
bool type may become quite distinct from int (or in other words bool
type will not be one of integer types). This may explain why C99 and C11
has "_Bool" type, not a hypothetically finalized in future "bool" type.
Also C99 clearly states that macro "false" and "true" may be undefined
and redefined to any arbitrary values.

I'm going to fix review comments and resend the series tomorrow, if you
find some of the changes wanted, please apply them, the rest may be
actually just covered by a simple function argument type change, which
hopefully happen in 4.2

--
With best wishes,
Vladimir