| Message ID | 1433200158-6890-2-git-send-email-vz@mleia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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)? C defines a mapping between boolean and integer values.
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
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.
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
On Wed, Jun 03, 2015 at 12:54:22AM +0300, Vladimir Zapolskiy wrote: > 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. You're confusing several different things here - it's just the same as the handling of NULL and 0. An implementation can make NULL be anything that amuses it (so long as nothing standards conforming can tell) but if you write 0 in a pointer context it has to be a NULL pointer, and if you use a NULL pointer in an integer context it must evaluate to 0. The in memory representation is potentially a separate thing to what the source code does, though practically speaking people are unlikely to implment anything too extravagent. I suspect you'll find that the use of _Bool in current standards is simply to avoid breaking existing standards conforming code that uses bool by suddenly making that a keyword.
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index 31d969a..28908f5a 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -4507,16 +4507,23 @@ static inline struct rt5677_priv *gpio_to_rt5677(struct gpio_chip *chip) static void rt5677_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct rt5677_priv *rt5677 = gpio_to_rt5677(chip); + unsigned int val = 0; switch (offset) { case RT5677_GPIO1 ... RT5677_GPIO5: + if (value) + val = RT5677_GPIO_OUT_HI(offset); + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL2, - 0x1 << (offset * 3 + 1), !!value << (offset * 3 + 1)); + RT5677_GPIO_OUT_MASK(offset), val); break; case RT5677_GPIO6: + if (value) + val = RT5677_GPIO6_OUT_HI; + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL3, - RT5677_GPIO6_OUT_MASK, !!value << RT5677_GPIO6_OUT_SFT); + RT5677_GPIO6_OUT_MASK, val); break; default: @@ -4528,18 +4535,27 @@ static int rt5677_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value) { struct rt5677_priv *rt5677 = gpio_to_rt5677(chip); + unsigned int val; switch (offset) { case RT5677_GPIO1 ... RT5677_GPIO5: + val = RT5677_GPIO_DIR_OUT(offset); + + if (value) + val |= RT5677_GPIO_OUT_HI(offset); + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL2, - 0x3 << (offset * 3 + 1), - (0x2 | !!value) << (offset * 3 + 1)); + RT5677_GPIO_DIR_OUT_MASK(offset), val); break; case RT5677_GPIO6: + val = RT5677_GPIO6_DIR_OUT; + + if (value) + val |= RT5677_GPIO6_OUT_HI; + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL3, - RT5677_GPIO6_DIR_MASK | RT5677_GPIO6_OUT_MASK, - RT5677_GPIO6_DIR_OUT | !!value << RT5677_GPIO6_OUT_SFT); + RT5677_GPIO6_DIR_MASK | RT5677_GPIO6_OUT_MASK, val); break; default: @@ -4568,12 +4584,12 @@ static int rt5677_gpio_direction_in(struct gpio_chip *chip, unsigned offset) switch (offset) { case RT5677_GPIO1 ... RT5677_GPIO5: regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL2, - 0x1 << (offset * 3 + 2), 0x0); + RT5677_GPIO_DIR_MASK(offset), 0x0); break; case RT5677_GPIO6: regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL3, - RT5677_GPIO6_DIR_MASK, RT5677_GPIO6_DIR_IN); + RT5677_GPIO6_DIR_MASK, RT5677_GPIO6_DIR_IN); break; default:
The main intention of the change is to remove bitwise operations on GPIO level value as a preceding change before updating gpiolib callbacks to utilize bool type representing a GPIO level. Usage of generic over GPIO[1-5] macros allows to remove calculations with magic numbers. No functional change. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> Cc: Bard Liao <bardliao@realtek.com> Cc: Oder Chiou <oder_chiou@realtek.com> --- sound/soc/codecs/rt5677.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)