[PATCH 0/7] ASoC: remove bitwise operations on GPIO level value

[PATCH 0/7] ASoC: remove bitwise operations on GPIO level value

[Vladimir Zapolskiy]

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(-)

-- 
2.1.4

[PATCH 1/7] ASoC: rt5677: add GPIO helper macros

[Vladimir Zapolskiy]

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

Re: [PATCH 1/7] ASoC: rt5677: add GPIO helper macros

[Takashi Iwai]

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

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

[Vladimir Zapolskiy]

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:
-- 
2.1.4

Re: [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Vladimir Zapolskiy]

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

Re: [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [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

Re: [PATCH 2/7] ASoC: rt5677: clean up gpiolib callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Vladimir Zapolskiy]

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, &reg);
 
-	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:
-- 
2.1.4

Re: [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Charles Keepax]

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

Re: [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Linus Walleij]

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

Re: [alsa-devel] [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Charles Keepax]

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

Re: [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Charles Keepax]

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

Re: [PATCH 3/7] ASoC: wm8903: generalize GPIO control register bits

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Vladimir Zapolskiy]

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 = {
-- 
2.1.4

Re: [PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Charles Keepax]

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

Re: [PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Vladimir Zapolskiy]

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

Re: [PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/7] ASoC: wm8903: simplify gpiolib callbacks

[Vladimir Zapolskiy]

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

[PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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)
-- 
2.1.4

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Charles Keepax]

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

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

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.  

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Trent Piepho]

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?

Re: [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 551 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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

Re: [alsa-devel] [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Trent Piepho]

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.

Re: [alsa-devel] [PATCH 5/7] ASoC: wm5100: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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

[PATCH 6/7] ASoC: wm8962: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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);
-- 
2.1.4

Re: [PATCH 6/7] ASoC: wm8962: remove bitwise operations involving GPIO level value

[Charles Keepax]

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

[PATCH 7/7] ASoC: wm8996: remove bitwise operations involving GPIO level value

[Vladimir Zapolskiy]

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

Re: [PATCH 7/7] ASoC: wm8996: remove bitwise operations involving GPIO level value

[Charles Keepax]

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