[alsa-devel] [PATCH 0/7] ASoC: remove bitwise operations on GPIO level value
The series does not contain any functional changes, it touches only implementation of gpiolib .set and .direction_output callbacks.
The main intention of the change is to remove bitwise operations on GPIO high/low level value as a preceding change before updating gpiolib callback signatures to utilize bool type as a representation of GPIO level.
The change covers all input cases of GPIO level (i.e. .set and .direction_output) in sound/*, also the series contains a small clean-ups in rt5677 and wm8903 codec drivers related to gpiolib callbacks.
Vladimir Zapolskiy (7): ASoC: rt5677: add GPIO helper macros ASoC: rt5677: clean up gpiolib callbacks ASoC: wm8903: generalize GPIO control register bits ASoC: wm8903: simplify gpiolib callbacks ASoC: wm5100: remove bitwise operations involving GPIO level value ASoC: wm8962: remove bitwise operations involving GPIO level value ASoC: wm8996: remove bitwise operations involving GPIO level value
include/sound/wm8903.h | 222 ++++++++-------------------------------------- sound/soc/codecs/rt5677.c | 32 +++++-- sound/soc/codecs/rt5677.h | 6 ++ sound/soc/codecs/wm5100.c | 21 ++--- sound/soc/codecs/wm8903.c | 44 ++++----- sound/soc/codecs/wm8962.c | 13 ++- sound/soc/codecs/wm8996.c | 11 ++- 7 files changed, 112 insertions(+), 237 deletions(-)
Add generic macro definitions for GPIO[1-5] direction and value register bits, argument is RT5677_GPIOn.
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.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index 7eca38a..5b84255 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1622,6 +1622,12 @@ #define RT5677_GPIO1_P_NOR (0x0 << 0) #define RT5677_GPIO1_P_INV (0x1 << 0)
+#define RT5677_GPIO_DIR_OUT_MASK(n) (0x3 << (n * 3 + 1)) +#define RT5677_GPIO_DIR_MASK(n) (0x1 << (n * 3 + 2)) +#define RT5677_GPIO_DIR_OUT(n) (0x1 << (n * 3 + 2)) +#define RT5677_GPIO_OUT_MASK(n) (0x1 << (n * 3 + 1)) +#define RT5677_GPIO_OUT_HI(n) (0x1 << (n * 3 + 1)) + /* GPIO Control 3 (0xc2) */ #define RT5677_GPIO6_DIR_MASK (0x1 << 2) #define RT5677_GPIO6_DIR_SFT 2
At Tue, 2 Jun 2015 02:09:12 +0300, Vladimir Zapolskiy wrote:
Add generic macro definitions for GPIO[1-5] direction and value register bits, argument is RT5677_GPIOn.
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.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index 7eca38a..5b84255 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1622,6 +1622,12 @@ #define RT5677_GPIO1_P_NOR (0x0 << 0) #define RT5677_GPIO1_P_INV (0x1 << 0)
+#define RT5677_GPIO_DIR_OUT_MASK(n) (0x3 << (n * 3 + 1)) +#define RT5677_GPIO_DIR_MASK(n) (0x1 << (n * 3 + 2)) +#define RT5677_GPIO_DIR_OUT(n) (0x1 << (n * 3 + 2)) +#define RT5677_GPIO_OUT_MASK(n) (0x1 << (n * 3 + 1)) +#define RT5677_GPIO_OUT_HI(n) (0x1 << (n * 3 + 1))
Put parentheses around n in expansion.
thanks,
Takashi
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(-)
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:
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.
All GPIO1/2/3/4/5 control registers have the same bit map, but in implementation of gpiolib callbacks WM8903_GPn_*, WM8903_GP1_* and WM8903_GP2_* macro are mixed up. Replace particular GPIOn control register bit definitions with generic ones and save ~150 LoCs.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com --- include/sound/wm8903.h | 222 ++++++++-------------------------------------- sound/soc/codecs/wm8903.c | 22 ++--- 2 files changed, 46 insertions(+), 198 deletions(-)
diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h index b310c5a..ac2e252 100644 --- a/include/sound/wm8903.h +++ b/include/sound/wm8903.h @@ -52,198 +52,46 @@
/* * R116 (0x74) - GPIO Control 1 - */ -#define WM8903_GP1_FN_MASK 0x1F00 /* GP1_FN - [12:8] */ -#define WM8903_GP1_FN_SHIFT 8 /* GP1_FN - [12:8] */ -#define WM8903_GP1_FN_WIDTH 5 /* GP1_FN - [12:8] */ -#define WM8903_GP1_DIR 0x0080 /* GP1_DIR */ -#define WM8903_GP1_DIR_MASK 0x0080 /* GP1_DIR */ -#define WM8903_GP1_DIR_SHIFT 7 /* GP1_DIR */ -#define WM8903_GP1_DIR_WIDTH 1 /* GP1_DIR */ -#define WM8903_GP1_OP_CFG 0x0040 /* GP1_OP_CFG */ -#define WM8903_GP1_OP_CFG_MASK 0x0040 /* GP1_OP_CFG */ -#define WM8903_GP1_OP_CFG_SHIFT 6 /* GP1_OP_CFG */ -#define WM8903_GP1_OP_CFG_WIDTH 1 /* GP1_OP_CFG */ -#define WM8903_GP1_IP_CFG 0x0020 /* GP1_IP_CFG */ -#define WM8903_GP1_IP_CFG_MASK 0x0020 /* GP1_IP_CFG */ -#define WM8903_GP1_IP_CFG_SHIFT 5 /* GP1_IP_CFG */ -#define WM8903_GP1_IP_CFG_WIDTH 1 /* GP1_IP_CFG */ -#define WM8903_GP1_LVL 0x0010 /* GP1_LVL */ -#define WM8903_GP1_LVL_MASK 0x0010 /* GP1_LVL */ -#define WM8903_GP1_LVL_SHIFT 4 /* GP1_LVL */ -#define WM8903_GP1_LVL_WIDTH 1 /* GP1_LVL */ -#define WM8903_GP1_PD 0x0008 /* GP1_PD */ -#define WM8903_GP1_PD_MASK 0x0008 /* GP1_PD */ -#define WM8903_GP1_PD_SHIFT 3 /* GP1_PD */ -#define WM8903_GP1_PD_WIDTH 1 /* GP1_PD */ -#define WM8903_GP1_PU 0x0004 /* GP1_PU */ -#define WM8903_GP1_PU_MASK 0x0004 /* GP1_PU */ -#define WM8903_GP1_PU_SHIFT 2 /* GP1_PU */ -#define WM8903_GP1_PU_WIDTH 1 /* GP1_PU */ -#define WM8903_GP1_INTMODE 0x0002 /* GP1_INTMODE */ -#define WM8903_GP1_INTMODE_MASK 0x0002 /* GP1_INTMODE */ -#define WM8903_GP1_INTMODE_SHIFT 1 /* GP1_INTMODE */ -#define WM8903_GP1_INTMODE_WIDTH 1 /* GP1_INTMODE */ -#define WM8903_GP1_DB 0x0001 /* GP1_DB */ -#define WM8903_GP1_DB_MASK 0x0001 /* GP1_DB */ -#define WM8903_GP1_DB_SHIFT 0 /* GP1_DB */ -#define WM8903_GP1_DB_WIDTH 1 /* GP1_DB */ - -/* * R117 (0x75) - GPIO Control 2 - */ -#define WM8903_GP2_FN_MASK 0x1F00 /* GP2_FN - [12:8] */ -#define WM8903_GP2_FN_SHIFT 8 /* GP2_FN - [12:8] */ -#define WM8903_GP2_FN_WIDTH 5 /* GP2_FN - [12:8] */ -#define WM8903_GP2_DIR 0x0080 /* GP2_DIR */ -#define WM8903_GP2_DIR_MASK 0x0080 /* GP2_DIR */ -#define WM8903_GP2_DIR_SHIFT 7 /* GP2_DIR */ -#define WM8903_GP2_DIR_WIDTH 1 /* GP2_DIR */ -#define WM8903_GP2_OP_CFG 0x0040 /* GP2_OP_CFG */ -#define WM8903_GP2_OP_CFG_MASK 0x0040 /* GP2_OP_CFG */ -#define WM8903_GP2_OP_CFG_SHIFT 6 /* GP2_OP_CFG */ -#define WM8903_GP2_OP_CFG_WIDTH 1 /* GP2_OP_CFG */ -#define WM8903_GP2_IP_CFG 0x0020 /* GP2_IP_CFG */ -#define WM8903_GP2_IP_CFG_MASK 0x0020 /* GP2_IP_CFG */ -#define WM8903_GP2_IP_CFG_SHIFT 5 /* GP2_IP_CFG */ -#define WM8903_GP2_IP_CFG_WIDTH 1 /* GP2_IP_CFG */ -#define WM8903_GP2_LVL 0x0010 /* GP2_LVL */ -#define WM8903_GP2_LVL_MASK 0x0010 /* GP2_LVL */ -#define WM8903_GP2_LVL_SHIFT 4 /* GP2_LVL */ -#define WM8903_GP2_LVL_WIDTH 1 /* GP2_LVL */ -#define WM8903_GP2_PD 0x0008 /* GP2_PD */ -#define WM8903_GP2_PD_MASK 0x0008 /* GP2_PD */ -#define WM8903_GP2_PD_SHIFT 3 /* GP2_PD */ -#define WM8903_GP2_PD_WIDTH 1 /* GP2_PD */ -#define WM8903_GP2_PU 0x0004 /* GP2_PU */ -#define WM8903_GP2_PU_MASK 0x0004 /* GP2_PU */ -#define WM8903_GP2_PU_SHIFT 2 /* GP2_PU */ -#define WM8903_GP2_PU_WIDTH 1 /* GP2_PU */ -#define WM8903_GP2_INTMODE 0x0002 /* GP2_INTMODE */ -#define WM8903_GP2_INTMODE_MASK 0x0002 /* GP2_INTMODE */ -#define WM8903_GP2_INTMODE_SHIFT 1 /* GP2_INTMODE */ -#define WM8903_GP2_INTMODE_WIDTH 1 /* GP2_INTMODE */ -#define WM8903_GP2_DB 0x0001 /* GP2_DB */ -#define WM8903_GP2_DB_MASK 0x0001 /* GP2_DB */ -#define WM8903_GP2_DB_SHIFT 0 /* GP2_DB */ -#define WM8903_GP2_DB_WIDTH 1 /* GP2_DB */ - -/* * R118 (0x76) - GPIO Control 3 - */ -#define WM8903_GP3_FN_MASK 0x1F00 /* GP3_FN - [12:8] */ -#define WM8903_GP3_FN_SHIFT 8 /* GP3_FN - [12:8] */ -#define WM8903_GP3_FN_WIDTH 5 /* GP3_FN - [12:8] */ -#define WM8903_GP3_DIR 0x0080 /* GP3_DIR */ -#define WM8903_GP3_DIR_MASK 0x0080 /* GP3_DIR */ -#define WM8903_GP3_DIR_SHIFT 7 /* GP3_DIR */ -#define WM8903_GP3_DIR_WIDTH 1 /* GP3_DIR */ -#define WM8903_GP3_OP_CFG 0x0040 /* GP3_OP_CFG */ -#define WM8903_GP3_OP_CFG_MASK 0x0040 /* GP3_OP_CFG */ -#define WM8903_GP3_OP_CFG_SHIFT 6 /* GP3_OP_CFG */ -#define WM8903_GP3_OP_CFG_WIDTH 1 /* GP3_OP_CFG */ -#define WM8903_GP3_IP_CFG 0x0020 /* GP3_IP_CFG */ -#define WM8903_GP3_IP_CFG_MASK 0x0020 /* GP3_IP_CFG */ -#define WM8903_GP3_IP_CFG_SHIFT 5 /* GP3_IP_CFG */ -#define WM8903_GP3_IP_CFG_WIDTH 1 /* GP3_IP_CFG */ -#define WM8903_GP3_LVL 0x0010 /* GP3_LVL */ -#define WM8903_GP3_LVL_MASK 0x0010 /* GP3_LVL */ -#define WM8903_GP3_LVL_SHIFT 4 /* GP3_LVL */ -#define WM8903_GP3_LVL_WIDTH 1 /* GP3_LVL */ -#define WM8903_GP3_PD 0x0008 /* GP3_PD */ -#define WM8903_GP3_PD_MASK 0x0008 /* GP3_PD */ -#define WM8903_GP3_PD_SHIFT 3 /* GP3_PD */ -#define WM8903_GP3_PD_WIDTH 1 /* GP3_PD */ -#define WM8903_GP3_PU 0x0004 /* GP3_PU */ -#define WM8903_GP3_PU_MASK 0x0004 /* GP3_PU */ -#define WM8903_GP3_PU_SHIFT 2 /* GP3_PU */ -#define WM8903_GP3_PU_WIDTH 1 /* GP3_PU */ -#define WM8903_GP3_INTMODE 0x0002 /* GP3_INTMODE */ -#define WM8903_GP3_INTMODE_MASK 0x0002 /* GP3_INTMODE */ -#define WM8903_GP3_INTMODE_SHIFT 1 /* GP3_INTMODE */ -#define WM8903_GP3_INTMODE_WIDTH 1 /* GP3_INTMODE */ -#define WM8903_GP3_DB 0x0001 /* GP3_DB */ -#define WM8903_GP3_DB_MASK 0x0001 /* GP3_DB */ -#define WM8903_GP3_DB_SHIFT 0 /* GP3_DB */ -#define WM8903_GP3_DB_WIDTH 1 /* GP3_DB */ - -/* * R119 (0x77) - GPIO Control 4 - */ -#define WM8903_GP4_FN_MASK 0x1F00 /* GP4_FN - [12:8] */ -#define WM8903_GP4_FN_SHIFT 8 /* GP4_FN - [12:8] */ -#define WM8903_GP4_FN_WIDTH 5 /* GP4_FN - [12:8] */ -#define WM8903_GP4_DIR 0x0080 /* GP4_DIR */ -#define WM8903_GP4_DIR_MASK 0x0080 /* GP4_DIR */ -#define WM8903_GP4_DIR_SHIFT 7 /* GP4_DIR */ -#define WM8903_GP4_DIR_WIDTH 1 /* GP4_DIR */ -#define WM8903_GP4_OP_CFG 0x0040 /* GP4_OP_CFG */ -#define WM8903_GP4_OP_CFG_MASK 0x0040 /* GP4_OP_CFG */ -#define WM8903_GP4_OP_CFG_SHIFT 6 /* GP4_OP_CFG */ -#define WM8903_GP4_OP_CFG_WIDTH 1 /* GP4_OP_CFG */ -#define WM8903_GP4_IP_CFG 0x0020 /* GP4_IP_CFG */ -#define WM8903_GP4_IP_CFG_MASK 0x0020 /* GP4_IP_CFG */ -#define WM8903_GP4_IP_CFG_SHIFT 5 /* GP4_IP_CFG */ -#define WM8903_GP4_IP_CFG_WIDTH 1 /* GP4_IP_CFG */ -#define WM8903_GP4_LVL 0x0010 /* GP4_LVL */ -#define WM8903_GP4_LVL_MASK 0x0010 /* GP4_LVL */ -#define WM8903_GP4_LVL_SHIFT 4 /* GP4_LVL */ -#define WM8903_GP4_LVL_WIDTH 1 /* GP4_LVL */ -#define WM8903_GP4_PD 0x0008 /* GP4_PD */ -#define WM8903_GP4_PD_MASK 0x0008 /* GP4_PD */ -#define WM8903_GP4_PD_SHIFT 3 /* GP4_PD */ -#define WM8903_GP4_PD_WIDTH 1 /* GP4_PD */ -#define WM8903_GP4_PU 0x0004 /* GP4_PU */ -#define WM8903_GP4_PU_MASK 0x0004 /* GP4_PU */ -#define WM8903_GP4_PU_SHIFT 2 /* GP4_PU */ -#define WM8903_GP4_PU_WIDTH 1 /* GP4_PU */ -#define WM8903_GP4_INTMODE 0x0002 /* GP4_INTMODE */ -#define WM8903_GP4_INTMODE_MASK 0x0002 /* GP4_INTMODE */ -#define WM8903_GP4_INTMODE_SHIFT 1 /* GP4_INTMODE */ -#define WM8903_GP4_INTMODE_WIDTH 1 /* GP4_INTMODE */ -#define WM8903_GP4_DB 0x0001 /* GP4_DB */ -#define WM8903_GP4_DB_MASK 0x0001 /* GP4_DB */ -#define WM8903_GP4_DB_SHIFT 0 /* GP4_DB */ -#define WM8903_GP4_DB_WIDTH 1 /* GP4_DB */ - -/* * R120 (0x78) - GPIO Control 5 */ -#define WM8903_GP5_FN_MASK 0x1F00 /* GP5_FN - [12:8] */ -#define WM8903_GP5_FN_SHIFT 8 /* GP5_FN - [12:8] */ -#define WM8903_GP5_FN_WIDTH 5 /* GP5_FN - [12:8] */ -#define WM8903_GP5_DIR 0x0080 /* GP5_DIR */ -#define WM8903_GP5_DIR_MASK 0x0080 /* GP5_DIR */ -#define WM8903_GP5_DIR_SHIFT 7 /* GP5_DIR */ -#define WM8903_GP5_DIR_WIDTH 1 /* GP5_DIR */ -#define WM8903_GP5_OP_CFG 0x0040 /* GP5_OP_CFG */ -#define WM8903_GP5_OP_CFG_MASK 0x0040 /* GP5_OP_CFG */ -#define WM8903_GP5_OP_CFG_SHIFT 6 /* GP5_OP_CFG */ -#define WM8903_GP5_OP_CFG_WIDTH 1 /* GP5_OP_CFG */ -#define WM8903_GP5_IP_CFG 0x0020 /* GP5_IP_CFG */ -#define WM8903_GP5_IP_CFG_MASK 0x0020 /* GP5_IP_CFG */ -#define WM8903_GP5_IP_CFG_SHIFT 5 /* GP5_IP_CFG */ -#define WM8903_GP5_IP_CFG_WIDTH 1 /* GP5_IP_CFG */ -#define WM8903_GP5_LVL 0x0010 /* GP5_LVL */ -#define WM8903_GP5_LVL_MASK 0x0010 /* GP5_LVL */ -#define WM8903_GP5_LVL_SHIFT 4 /* GP5_LVL */ -#define WM8903_GP5_LVL_WIDTH 1 /* GP5_LVL */ -#define WM8903_GP5_PD 0x0008 /* GP5_PD */ -#define WM8903_GP5_PD_MASK 0x0008 /* GP5_PD */ -#define WM8903_GP5_PD_SHIFT 3 /* GP5_PD */ -#define WM8903_GP5_PD_WIDTH 1 /* GP5_PD */ -#define WM8903_GP5_PU 0x0004 /* GP5_PU */ -#define WM8903_GP5_PU_MASK 0x0004 /* GP5_PU */ -#define WM8903_GP5_PU_SHIFT 2 /* GP5_PU */ -#define WM8903_GP5_PU_WIDTH 1 /* GP5_PU */ -#define WM8903_GP5_INTMODE 0x0002 /* GP5_INTMODE */ -#define WM8903_GP5_INTMODE_MASK 0x0002 /* GP5_INTMODE */ -#define WM8903_GP5_INTMODE_SHIFT 1 /* GP5_INTMODE */ -#define WM8903_GP5_INTMODE_WIDTH 1 /* GP5_INTMODE */ -#define WM8903_GP5_DB 0x0001 /* GP5_DB */ -#define WM8903_GP5_DB_MASK 0x0001 /* GP5_DB */ -#define WM8903_GP5_DB_SHIFT 0 /* GP5_DB */ -#define WM8903_GP5_DB_WIDTH 1 /* GP5_DB */ +#define WM8903_GPn_FN_MASK 0x1F00 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_SHIFT 8 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_WIDTH 5 /* GPn_FN - [12:8] */ +#define WM8903_GPn_DIR 0x0080 /* GPn_DIR */ +#define WM8903_GPn_DIR_MASK 0x0080 /* GPn_DIR */ +#define WM8903_GPn_DIR_SHIFT 7 /* GPn_DIR */ +#define WM8903_GPn_DIR_WIDTH 1 /* GPn_DIR */ +#define WM8903_GPn_OP_CFG 0x0040 /* GPn_OP_CFG */ +#define WM8903_GPn_OP_CFG_MASK 0x0040 /* GPn_OP_CFG */ +#define WM8903_GPn_OP_CFG_SHIFT 6 /* GPn_OP_CFG */ +#define WM8903_GPn_OP_CFG_WIDTH 1 /* GPn_OP_CFG */ +#define WM8903_GPn_IP_CFG 0x0020 /* GPn_IP_CFG */ +#define WM8903_GPn_IP_CFG_MASK 0x0020 /* GPn_IP_CFG */ +#define WM8903_GPn_IP_CFG_SHIFT 5 /* GPn_IP_CFG */ +#define WM8903_GPn_IP_CFG_WIDTH 1 /* GPn_IP_CFG */ +#define WM8903_GPn_LVL 0x0010 /* GPn_LVL */ +#define WM8903_GPn_LVL_MASK 0x0010 /* GPn_LVL */ +#define WM8903_GPn_LVL_SHIFT 4 /* GPn_LVL */ +#define WM8903_GPn_LVL_WIDTH 1 /* GPn_LVL */ +#define WM8903_GPn_PD 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_MASK 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_SHIFT 3 /* GPn_PD */ +#define WM8903_GPn_PD_WIDTH 1 /* GPn_PD */ +#define WM8903_GPn_PU 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_MASK 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_SHIFT 2 /* GPn_PU */ +#define WM8903_GPn_PU_WIDTH 1 /* GPn_PU */ +#define WM8903_GPn_INTMODE 0x0002 /* GPn_INTMODE */ +#define WM8903_GPn_INTMODE_MASK 0x0002 /* GPn_INTMODE */ +#define WM8903_GPn_INTMODE_SHIFT 1 /* GPn_INTMODE */ +#define WM8903_GPn_INTMODE_WIDTH 1 /* GPn_INTMODE */ +#define WM8903_GPn_DB 0x0001 /* GPn_DB */ +#define WM8903_GPn_DB_MASK 0x0001 /* GPn_DB */ +#define WM8903_GPn_DB_SHIFT 0 /* GPn_DB */ +#define WM8903_GPn_DB_WIDTH 1 /* GPn_DB */
#define WM8903_NUM_GPIO 5
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index b5322c1..93f1ce0 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1785,9 +1785,9 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) unsigned int mask, val; int ret;
- mask = WM8903_GP1_FN_MASK | WM8903_GP1_DIR_MASK; - val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GP1_FN_SHIFT) | - WM8903_GP1_DIR; + mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; + val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | + WM8903_GPn_DIR;
ret = regmap_update_bits(wm8903->regmap, WM8903_GPIO_CONTROL_1 + offset, mask, val); @@ -1804,7 +1804,7 @@ static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
regmap_read(wm8903->regmap, WM8903_GPIO_CONTROL_1 + offset, ®);
- return (reg & WM8903_GP1_LVL_MASK) >> WM8903_GP1_LVL_SHIFT; + return (reg & WM8903_GPn_LVL_MASK) >> WM8903_GPn_LVL_SHIFT; }
static int wm8903_gpio_direction_out(struct gpio_chip *chip, @@ -1814,9 +1814,9 @@ static int wm8903_gpio_direction_out(struct gpio_chip *chip, unsigned int mask, val; int ret;
- mask = WM8903_GP1_FN_MASK | WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK; - val = (WM8903_GPn_FN_GPIO_OUTPUT << WM8903_GP1_FN_SHIFT) | - (value << WM8903_GP2_LVL_SHIFT); + mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK | WM8903_GPn_LVL_MASK; + val = (WM8903_GPn_FN_GPIO_OUTPUT << WM8903_GPn_FN_SHIFT) | + (value << WM8903_GPn_LVL_SHIFT);
ret = regmap_update_bits(wm8903->regmap, WM8903_GPIO_CONTROL_1 + offset, mask, val); @@ -1831,8 +1831,8 @@ static void wm8903_gpio_set(struct gpio_chip *chip, unsigned offset, int value) struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
regmap_update_bits(wm8903->regmap, WM8903_GPIO_CONTROL_1 + offset, - WM8903_GP1_LVL_MASK, - !!value << WM8903_GP1_LVL_SHIFT); + WM8903_GPn_LVL_MASK, + !!value << WM8903_GPn_LVL_SHIFT); }
static struct gpio_chip wm8903_template_chip = { @@ -2066,8 +2066,8 @@ static int wm8903_i2c_probe(struct i2c_client *i2c, regmap_write(wm8903->regmap, WM8903_GPIO_CONTROL_1 + i, pdata->gpio_cfg[i] & 0x7fff);
- val = (pdata->gpio_cfg[i] & WM8903_GP1_FN_MASK) - >> WM8903_GP1_FN_SHIFT; + val = (pdata->gpio_cfg[i] & WM8903_GPn_FN_MASK) + >> WM8903_GPn_FN_SHIFT;
switch (val) { case WM8903_GPn_FN_MICBIAS_CURRENT_DETECT:
On Tue, Jun 02, 2015 at 02:09:14AM +0300, Vladimir Zapolskiy wrote:
All GPIO1/2/3/4/5 control registers have the same bit map, but in implementation of gpiolib callbacks WM8903_GPn_*, WM8903_GP1_* and WM8903_GP2_* macro are mixed up. Replace particular GPIOn control register bit definitions with generic ones and save ~150 LoCs.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
If Mark's ok with this I think I am. Generally those register files tend to list all the bits on the chip, but certainly in this case nothing is really added by listing all the GPIO registers seperately.
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
On Tue, Jun 2, 2015 at 1:09 AM, Vladimir Zapolskiy vz@mleia.com wrote:
All GPIO1/2/3/4/5 control registers have the same bit map, but in implementation of gpiolib callbacks WM8903_GPn_*, WM8903_GP1_* and WM8903_GP2_* macro are mixed up. Replace particular GPIOn control register bit definitions with generic ones and save ~150 LoCs.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
This comment is not about the refactoring as such, which is OK...
+#define WM8903_GPn_FN_MASK 0x1F00 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_SHIFT 8 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_WIDTH 5 /* GPn_FN - [12:8] */
Function selection?
+#define WM8903_GPn_PD 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_MASK 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_SHIFT 3 /* GPn_PD */ +#define WM8903_GPn_PD_WIDTH 1 /* GPn_PD */ +#define WM8903_GPn_PU 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_MASK 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_SHIFT 2 /* GPn_PU */ +#define WM8903_GPn_PU_WIDTH 1 /* GPn_PU */
Pull-down/pull-up?
That is pin control, not GPIO.
I know I should probably be a bit relaxed on enforcing strict frameworks on ASoC and DRI/DRM alike, because the drivers are complex and sometimes need to be on top of things rather than split things apart and set up complex cobwebs of subsystem cross-dependencies, but I'd like to know a bit more about the design philisophy here.
I guess this dates back to the time before the pin control subsystem?
Yours, Linus Walleij
On Thu, Jun 04, 2015 at 10:30:10AM +0200, Linus Walleij wrote:
On Tue, Jun 2, 2015 at 1:09 AM, Vladimir Zapolskiy vz@mleia.com wrote:
All GPIO1/2/3/4/5 control registers have the same bit map, but in implementation of gpiolib callbacks WM8903_GPn_*, WM8903_GP1_* and WM8903_GP2_* macro are mixed up. Replace particular GPIOn control register bit definitions with generic ones and save ~150 LoCs.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
This comment is not about the refactoring as such, which is OK...
+#define WM8903_GPn_FN_MASK 0x1F00 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_SHIFT 8 /* GPn_FN - [12:8] */ +#define WM8903_GPn_FN_WIDTH 5 /* GPn_FN - [12:8] */
Function selection?
+#define WM8903_GPn_PD 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_MASK 0x0008 /* GPn_PD */ +#define WM8903_GPn_PD_SHIFT 3 /* GPn_PD */ +#define WM8903_GPn_PD_WIDTH 1 /* GPn_PD */ +#define WM8903_GPn_PU 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_MASK 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_SHIFT 2 /* GPn_PU */ +#define WM8903_GPn_PU_WIDTH 1 /* GPn_PU */
Pull-down/pull-up?
That is pin control, not GPIO.
I know I should probably be a bit relaxed on enforcing strict frameworks on ASoC and DRI/DRM alike, because the drivers are complex and sometimes need to be on top of things rather than split things apart and set up complex cobwebs of subsystem cross-dependencies, but I'd like to know a bit more about the design philisophy here.
I guess this dates back to the time before the pin control subsystem?
Yeah this all dates back to before pin control, I have had some initial looks at converting some of the newer drivers over to use pin control instead but it is still very much a work in progress and to be fair it is debatable when I will find time for older drivers such as this one.
Thanks, Charles
On Thu, Jun 04, 2015 at 10:30:10AM +0200, Linus Walleij wrote:
+#define WM8903_GPn_PU_MASK 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_SHIFT 2 /* GPn_PU */ +#define WM8903_GPn_PU_WIDTH 1 /* GPn_PU */
Pull-down/pull-up?
That is pin control, not GPIO.
Those register definition defines are just an automatically generated dump of the complete CODEC register map, the GPIO pins have some pin control type functionality in there.
I know I should probably be a bit relaxed on enforcing strict frameworks on ASoC and DRI/DRM alike, because the drivers are complex and sometimes need to be on top of things rather than split things apart and set up complex cobwebs of subsystem cross-dependencies, but I'd like to know a bit more about the design philisophy here.
I guess this dates back to the time before the pin control subsystem?
There's that as well, but more generally I'm not sure if there's even any use of those defines for the driver (without looking at the source to check).
On Thu, Jun 04, 2015 at 10:19:24AM +0100, Mark Brown wrote:
On Thu, Jun 04, 2015 at 10:30:10AM +0200, Linus Walleij wrote:
+#define WM8903_GPn_PU_MASK 0x0004 /* GPn_PU */ +#define WM8903_GPn_PU_SHIFT 2 /* GPn_PU */ +#define WM8903_GPn_PU_WIDTH 1 /* GPn_PU */
Pull-down/pull-up?
That is pin control, not GPIO.
Those register definition defines are just an automatically generated dump of the complete CODEC register map, the GPIO pins have some pin control type functionality in there.
I know I should probably be a bit relaxed on enforcing strict frameworks on ASoC and DRI/DRM alike, because the drivers are complex and sometimes need to be on top of things rather than split things apart and set up complex cobwebs of subsystem cross-dependencies, but I'd like to know a bit more about the design philisophy here.
I guess this dates back to the time before the pin control subsystem?
There's that as well, but more generally I'm not sure if there's even any use of those defines for the driver (without looking at the source to check).
Many of the drivers contain a gpio defaults style entry in the pdata, which basically is doing what pin control should be and setting up the pulls/functions etc. But replacing that in all the legacy CODECs would be a massive task.
Thanks, Charles
On Thu, Jun 04, 2015 at 10:24:39AM +0100, Charles Keepax wrote:
On Thu, Jun 04, 2015 at 10:19:24AM +0100, Mark Brown wrote:
There's that as well, but more generally I'm not sure if there's even any use of those defines for the driver (without looking at the source to check).
Many of the drivers contain a gpio defaults style entry in the pdata, which basically is doing what pin control should be and setting up the pulls/functions etc. But replacing that in all the legacy CODECs would be a massive task.
Sure, I just don't know without looking if this particular driver ever bothered doing that. The defines aren't pinctrl support by themselves they're just mechanically generated defines.
To be honest I'm not sure pinctrl is actually making life easier for these devices, especially without ACPI bindings given that people are now deploying ACPI based systems. There's also the whole MFD thing.
The change cleans up gpiolib callbacks, and the main intention is to remove bitwise operations on GPIO level value as a preceding change to switch gpiolib callbacks to utilize bool type to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com --- sound/soc/codecs/wm8903.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 93f1ce0..efac26b 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1783,18 +1783,13 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val; - int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | WM8903_GPn_DIR;
- ret = regmap_update_bits(wm8903->regmap, - WM8903_GPIO_CONTROL_1 + offset, mask, val); - if (ret < 0) - return ret; - - return 0; + return regmap_update_bits(wm8903->regmap, + WM8903_GPIO_CONTROL_1 + offset, mask, val); }
static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset) @@ -1812,27 +1807,26 @@ static int wm8903_gpio_direction_out(struct gpio_chip *chip, { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val; - int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK | WM8903_GPn_LVL_MASK; - val = (WM8903_GPn_FN_GPIO_OUTPUT << WM8903_GPn_FN_SHIFT) | - (value << WM8903_GPn_LVL_SHIFT); + val = WM8903_GPn_FN_GPIO_OUTPUT << WM8903_GPn_FN_SHIFT; + if (value) + val |= 0x1 << WM8903_GPn_LVL_SHIFT;
- ret = regmap_update_bits(wm8903->regmap, - WM8903_GPIO_CONTROL_1 + offset, mask, val); - if (ret < 0) - return ret; - - return 0; + return regmap_update_bits(wm8903->regmap, + WM8903_GPIO_CONTROL_1 + offset, mask, val); }
static void wm8903_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); + unsigned int val = 0; + + if (value) + val = 0x1 << WM8903_GPn_LVL_SHIFT;
regmap_update_bits(wm8903->regmap, WM8903_GPIO_CONTROL_1 + offset, - WM8903_GPn_LVL_MASK, - !!value << WM8903_GPn_LVL_SHIFT); + WM8903_GPn_LVL_MASK, val); }
static struct gpio_chip wm8903_template_chip = {
On Tue, Jun 02, 2015 at 02:09:15AM +0300, Vladimir Zapolskiy wrote:
The change cleans up gpiolib callbacks, and the main intention is to remove bitwise operations on GPIO level value as a preceding change to switch gpiolib callbacks to utilize bool type to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
On Tue, Jun 02, 2015 at 02:09:15AM +0300, Vladimir Zapolskiy wrote:
@@ -1783,18 +1783,13 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val;
int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | WM8903_GPn_DIR;
ret = regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);if (ret < 0)
return ret;return 0;
- return regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);}
static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
This appears to be an unrelated coding style change.
Hello Mark,
On 02.06.2015 22:41, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:15AM +0300, Vladimir Zapolskiy wrote:
@@ -1783,18 +1783,13 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val;
int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | WM8903_GPn_DIR;
ret = regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);if (ret < 0)
return ret;return 0;
- return regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);}
static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
This appears to be an unrelated coding style change.
this particular patch is named "simplify gpiolib callbacks".
Do you prefer to separate the change here in .direction_in implementation from the rest?
-- With best wishes, Vladimir
On Tue, Jun 02, 2015 at 11:18:23PM +0300, Vladimir Zapolskiy wrote:
On 02.06.2015 22:41, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:15AM +0300, Vladimir Zapolskiy wrote:
@@ -1783,18 +1783,13 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val;
int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | WM8903_GPn_DIR;
ret = regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);if (ret < 0)
return ret;return 0;
- return regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);}
static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
This appears to be an unrelated coding style change.
this particular patch is named "simplify gpiolib callbacks".
Do you prefer to separate the change here in .direction_in implementation from the rest?
It doesn't appear to make any changes related to boolean variables (AFAICT it's just removing the ret variable) so it shouldn't be in a change about boolean variables.
On 02.06.2015 23:31, Mark Brown wrote:
On Tue, Jun 02, 2015 at 11:18:23PM +0300, Vladimir Zapolskiy wrote:
On 02.06.2015 22:41, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:15AM +0300, Vladimir Zapolskiy wrote:
@@ -1783,18 +1783,13 @@ static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); unsigned int mask, val;
int ret;
mask = WM8903_GPn_FN_MASK | WM8903_GPn_DIR_MASK; val = (WM8903_GPn_FN_GPIO_INPUT << WM8903_GPn_FN_SHIFT) | WM8903_GPn_DIR;
ret = regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);if (ret < 0)
return ret;return 0;
- return regmap_update_bits(wm8903->regmap,
WM8903_GPIO_CONTROL_1 + offset, mask, val);}
static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset)
This appears to be an unrelated coding style change.
this particular patch is named "simplify gpiolib callbacks".
Do you prefer to separate the change here in .direction_in implementation from the rest?
It doesn't appear to make any changes related to boolean variables (AFAICT it's just removing the ret variable) so it shouldn't be in a change about boolean variables.
Okay, will split the change then.
-- With best wishes, Vladimir
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com --- sound/soc/codecs/wm5100.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c index 4c10cd8..fae9d13 100644 --- a/sound/soc/codecs/wm5100.c +++ b/sound/soc/codecs/wm5100.c @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); + unsigned int val = 0; + + if (value) + val = 0x1 << WM5100_GP1_LVL_SHIFT;
regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, - WM5100_GP1_LVL, !!value << WM5100_GP1_LVL_SHIFT); + WM5100_GP1_LVL, val); }
static int wm5100_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); - int val, ret; + unsigned int val = 0x1 << WM5100_GP1_FN_SHIFT;
- val = (1 << WM5100_GP1_FN_SHIFT) | (!!value << WM5100_GP1_LVL_SHIFT); + if (value) + val |= 0x1 << WM5100_GP1_LVL_SHIFT;
- ret = regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, - WM5100_GP1_FN_MASK | WM5100_GP1_DIR | - WM5100_GP1_LVL, val); - if (ret < 0) - return ret; - else - return 0; + return regmap_update_bits(wm5100->regmap, WM5100_GPIO_CTRL_1 + offset, + WM5100_GP1_FN_MASK | WM5100_GP1_DIR | + WM5100_GP1_LVL, val); }
static int wm5100_gpio_get(struct gpio_chip *chip, unsigned offset)
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
@@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip);
- unsigned int val = 0;
- if (value)
val = 0x1 << WM5100_GP1_LVL_SHIFT;
Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
Hello Mark,
On 02.06.2015 22:45, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
@@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm5100_priv *wm5100 = gpio_to_wm5100(chip);
- unsigned int val = 0;
- if (value)
val = 0x1 << WM5100_GP1_LVL_SHIFT;Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know.
-- With best wishes, Vladimir
On Tue, Jun 02, 2015 at 11:23:41PM +0300, Vladimir Zapolskiy wrote:
On 02.06.2015 22:45, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
- unsigned int val = 0;
- if (value)
val = 0x1 << WM5100_GP1_LVL_SHIFT;
Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know.
Especially after the unrelated style change thing earlier on (which meant I was reading things more carefully than usual) it'd be good to make things as clear as possible - you're right that the val vs value thing isn't helping either.
Like I say I am a bit surprised that the int/bool conversion doesn't do the right thing without any code changes other than the type of the parameter but ICBW, I didn't actually check.
Hello Mark,
On 02.06.2015 23:36, Mark Brown wrote:
On Tue, Jun 02, 2015 at 11:23:41PM +0300, Vladimir Zapolskiy wrote:
On 02.06.2015 22:45, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
- unsigned int val = 0;
- if (value)
val = 0x1 << WM5100_GP1_LVL_SHIFT;Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know.
Especially after the unrelated style change thing earlier on (which meant I was reading things more carefully than usual) it'd be good to make things as clear as possible - you're right that the val vs value thing isn't helping either.
got your concern, will send an update.
Like I say I am a bit surprised that the int/bool conversion doesn't do the right thing without any code changes other than the type of the parameter but ICBW, I didn't actually check.
My tested compilers do the work right, but I can not be sure about the whole variety of compilers, well, probably I should just follow the standard, which in turn is expected to be quite volatile in future revisions regarding usage of "_Bool" or future "bool" type.
If we assume the preferred Linux kernel style to use true/false constants for boolean variables only (see the reference to bool{init,return}.cocci), then even if bitwise and arithmetic operations on bool type are understandable to a compiler, but probably "false << true" (or whatever is hidden under a variable name) is not quite aesthetic for a human eye. This is not a technical argument though, and I'm not sure if any clear code style policy related to the case is agreed.
-- With best wishes, Vladimir
On Tue, Jun 2, 2015 at 1:23 PM, Vladimir Zapolskiy vz@mleia.com wrote:
Hello Mark,
On 02.06.2015 22:45, Mark Brown wrote:
On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote:
@@ -2244,26 +2244,27 @@ static inline struct wm5100_priv
*gpio_to_wm5100(struct gpio_chip *chip)
static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset,
int value)
{ struct wm5100_priv *wm5100 = gpio_to_wm5100(chip);
- unsigned int val = 0;
- if (value)
val = 0x1 << WM5100_GP1_LVL_SHIFT;Write this as an if/else so the reader doesn't have to wonder why you've missed the handling of the false case.
the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know.
const unsigned int val = value ? (0x1 << WM5100_GP1_LVL_SHIFT) : 0;
Two lines shorter....
Have you measured the effect of going from int to bool on code size and speed? Clearly, the C code is getting longer as '!!' is transformed into an if-then-else to set a new scratch variable. But the compiler likely converts both to a conditional branch or cmov type instruction. What I wonder is if converting gpiolib to use bools instead of ints is better just because someone likes it more that way or if there are objective metrics that show improvement.
BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator
And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
On Wed, Jun 03, 2015 at 03:50:04AM -0700, Trent Piepho wrote:
BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator
And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
Yes, this is exactly the point I was trying to make.
Hello Mark, Trent,
thank you for review.
On 03.06.2015 13:50, Trent Piepho wrote:
On Tue, Jun 2, 2015 at 1:23 PM, Vladimir Zapolskiy <vz@mleia.com mailto:vz@mleia.com> wrote:
Hello Mark, On 02.06.2015 22:45, Mark Brown wrote: > On Tue, Jun 02, 2015 at 02:09:16AM +0300, Vladimir Zapolskiy wrote: > >> @@ -2244,26 +2244,27 @@ static inline struct wm5100_priv *gpio_to_wm5100(struct gpio_chip *chip) >> static void wm5100_gpio_set(struct gpio_chip *chip, unsigned offset, int value) >> { >> struct wm5100_priv *wm5100 = gpio_to_wm5100(chip); >> + unsigned int val = 0; >> + >> + if (value) >> + val = 0x1 << WM5100_GP1_LVL_SHIFT; > > Write this as an if/else so the reader doesn't have to wonder why you've > missed the handling of the false case. > the only objection I have is that the resulting code will be two lines longer. If you think this code is not clear enough (is "val" vs. "value" misleading?), I'll change the rest of my patches, which contain the same logic structure, please let me know.const unsigned int val = value ? (0x1 << WM5100_GP1_LVL_SHIFT) : 0;
Two lines shorter....
Have you measured the effect of going from int to bool on code size and speed? Clearly, the C code is getting longer as '!!' is transformed into an if-then-else to set a new scratch variable. But the compiler likely converts both to a conditional branch or cmov type instruction. What I wonder is if converting gpiolib to use bools instead of ints is better just because someone likes it more that way or if there are objective metrics that show improvement.
BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator
And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
I don't think it makes sense to discuss technical aspects of the proposed change, as I mentioned in the discussion yesterday I see no technical issues at this point, and my changes are described as nonfunctional.
The _nontechnical_ problem I see (well, may be I'm just blowing it up) is a Linux kernel code style policy problem, to my knowledge the policy of using bitwise and arithmetic operations on bool type variables and constants is not defined, and since the policy is not yet defined I'm glad to take part in its discussion now.
As an example of one related policy is change of 0/1 constants to false/true, when they are attended by bool type, see c2b49ae678b , 5c216cc3f37 , 7eef08554ca , bool{init,return}.cocci etc.
One may say that the changes above has no value (however it might be related to ABI treatment of _Bool/bool), personally I agree with this accepted code policy.
Another code policy question is to which degree arithmetic and bitwise operations on bool variables and constants are acceptable. In my personal opinion arithmetic and bitwise operations on bool variables are quite undesired, that's why I attempt to replace them in advance in some sound/soc/codecs/* functions before changing the type of input variable representing GPIO level from int to bool. I guess in your personal opinion this proposed change has no technical sense, I respect your opinion and do not insist on my answer to the policy.
My little deal is to let you know that there is another opinion on the subject and send you the changes for review and ack/nak verdict.
-- With best wishes, Vladimir
On Wed, Jun 3, 2015 at 12:13 PM, Vladimir Zapolskiy vz@mleia.com wrote:
BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator
And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
I don't think it makes sense to discuss technical aspects of the proposed change, as I mentioned in the discussion yesterday I see no technical issues at this point, and my changes are described as nonfunctional.
The _nontechnical_ problem I see (well, may be I'm just blowing it up) is a Linux kernel code style policy problem, to my knowledge the policy of using bitwise and arithmetic operations on bool type variables and constants is not defined, and since the policy is not yet defined I'm glad to take part in its discussion now.
I don't believe this is the case. From C99, section 6.5.3.3, paragraph 5:
The result of the logical negation operator ! is 0 if the value of its operand compares unequal to 0, 1 if the value of its operand compares equal to 0. The result has type int.
Thus, it's defined that !!(bool_var) and !!(int_var) will return an int that is either 0 or 1. This has always been part of the C standard and I have a very hard time believing that a future standard will change that. It would break so much code to do so and I don't the case for what it will improve.
So I don't see the point in changing: !!value << SHIFT; into: unsigned int temp_val; if (value) temp_val = 1 << SHIFT; else temp_val = 0;
The former is shorter and simpler and completely defined by the C standard.
Hello Trent,
On 04.06.2015 00:51, Trent Piepho wrote:
On Wed, Jun 3, 2015 at 12:13 PM, Vladimir Zapolskiy vz@mleia.com wrote:
BTW, with a little C algebra: const unsigned int val = value ? 0x1 << WM5100_GP1_LVL_SHIFT : 0; const unsigned int val = (value ? 0x1 : 0) << WM5100_GP1_LVL_SHIFT; const unsigned int val = (!!value) << WM5100_GP1_LVL_SHIFT; // definition of ! operator
And now we're back to where we started, so I don't really see why this is even necessary. The semantics of the ! operator will be changed in a future C version?
I don't think it makes sense to discuss technical aspects of the proposed change, as I mentioned in the discussion yesterday I see no technical issues at this point, and my changes are described as nonfunctional.
The _nontechnical_ problem I see (well, may be I'm just blowing it up) is a Linux kernel code style policy problem, to my knowledge the policy of using bitwise and arithmetic operations on bool type variables and constants is not defined, and since the policy is not yet defined I'm glad to take part in its discussion now.
I don't believe this is the case.
please excuse me for possible misunderstanding, you don't believe that I'm interested in discussing a _nontechnical_ problem stated above?
From C99, section 6.5.3.3, paragraph 5:
C99 is a strict superset over Linux coding style (e.g. remember // comments or "register" specifier), so let's forget about.
-- With best wishes, Vladimir
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com --- sound/soc/codecs/wm8962.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index c5748fd..ad619e6 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -3341,9 +3341,13 @@ static void wm8962_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm8962_priv *wm8962 = gpio_to_wm8962(chip); struct snd_soc_codec *codec = wm8962->codec; + unsigned int val = 0; + + if (value) + val = 0x1 << WM8962_GP2_LVL_SHIFT;
snd_soc_update_bits(codec, WM8962_GPIO_BASE + offset, - WM8962_GP2_LVL, !!value << WM8962_GP2_LVL_SHIFT); + WM8962_GP2_LVL, val); }
static int wm8962_gpio_direction_out(struct gpio_chip *chip, @@ -3351,10 +3355,11 @@ static int wm8962_gpio_direction_out(struct gpio_chip *chip, { struct wm8962_priv *wm8962 = gpio_to_wm8962(chip); struct snd_soc_codec *codec = wm8962->codec; - int ret, val; + unsigned int val = 0x1 << WM8962_GP2_FN_SHIFT; + int ret;
- /* Force function 1 (logic output) */ - val = (1 << WM8962_GP2_FN_SHIFT) | (value << WM8962_GP2_LVL_SHIFT); + if (value) + val |= 0x1 << WM8962_GP2_LVL_SHIFT;
ret = snd_soc_update_bits(codec, WM8962_GPIO_BASE + offset, WM8962_GP2_FN_MASK | WM8962_GP2_LVL, val);
On Tue, Jun 02, 2015 at 02:09:17AM +0300, Vladimir Zapolskiy wrote:
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com --- sound/soc/codecs/wm8996.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wm8996.c b/sound/soc/codecs/wm8996.c index 3dd063f..0dce3a0 100644 --- a/sound/soc/codecs/wm8996.c +++ b/sound/soc/codecs/wm8996.c @@ -2147,18 +2147,23 @@ static inline struct wm8996_priv *gpio_to_wm8996(struct gpio_chip *chip) static void wm8996_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct wm8996_priv *wm8996 = gpio_to_wm8996(chip); + unsigned int val = 0; + + if (value) + val = 0x1 << WM8996_GP1_LVL_SHIFT;
regmap_update_bits(wm8996->regmap, WM8996_GPIO_1 + offset, - WM8996_GP1_LVL, !!value << WM8996_GP1_LVL_SHIFT); + WM8996_GP1_LVL, val); }
static int wm8996_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value) { struct wm8996_priv *wm8996 = gpio_to_wm8996(chip); - int val; + unsigned int val = 0x1 << WM8996_GP1_FN_SHIFT;
- val = (1 << WM8996_GP1_FN_SHIFT) | (!!value << WM8996_GP1_LVL_SHIFT); + if (value) + val |= 0x1 << WM8996_GP1_LVL_SHIFT;
return regmap_update_bits(wm8996->regmap, WM8996_GPIO_1 + offset, WM8996_GP1_FN_MASK | WM8996_GP1_DIR |
On Tue, Jun 02, 2015 at 02:09:18AM +0300, Vladimir Zapolskiy wrote:
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 to represent GPIO level.
No functional change.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com Cc: Charles Keepax ckeepax@opensource.wolfsonmicro.com Cc: Lars-Peter Clausen lars@metafoo.de Cc: Axel Lin axel.lin@ingics.com Cc: patches@opensource.wolfsonmicro.com
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
participants (6)
-
Charles Keepax -
Linus Walleij -
Mark Brown -
Takashi Iwai -
Trent Piepho -
Vladimir Zapolskiy