diff mbox

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

Message ID 1433200158-6890-2-git-send-email-vz@mleia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vladimir Zapolskiy June 1, 2015, 11:09 p.m. UTC
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(-)

Comments

Mark Brown June 2, 2015, 7:38 p.m. UTC | #1
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.
Vladimir Zapolskiy June 2, 2015, 8:39 p.m. UTC | #2
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
Mark Brown June 2, 2015, 8:50 p.m. UTC | #3
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.
Vladimir Zapolskiy June 2, 2015, 9:54 p.m. UTC | #4
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
Mark Brown June 3, 2015, 11:38 a.m. UTC | #5
On Wed, Jun 03, 2015 at 12:54:22AM +0300, Vladimir Zapolskiy wrote:

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

You're confusing several different things here - it's just the same as
the handling of NULL and 0.  An implementation can make NULL be anything
that amuses it (so long as nothing standards conforming can tell) but if
you write 0 in a pointer context it has to be a NULL pointer, and if you
use a NULL pointer in an integer context it must evaluate to 0.  The in
memory representation is potentially a separate thing to what the source
code does, though practically speaking people are unlikely to implment
anything too extravagent.

I suspect you'll find that the use of _Bool in current standards is
simply to avoid breaking existing standards conforming code that uses
bool by suddenly making that a keyword.
diff mbox

Patch

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: