cs5535: use BIT() macro for defining bit-flags
suratiamol at gmail.com (Amol Surati)
PATCH 1/1 cs5535: use BIT() macro for defining bit-flags
The BIT() macro is available for defining the required bit-flags.
Since it operates on an unsigned value and expands to an unsigned result, using it, instead of an expression like (1 << x), also fixes the problem of shifting a signed 32-bit value by 31 bits. (e.g. 1 << 31. See CS5536_GPIOM7_PME_FLAG and CS5536_GPIOM7_PME_EN).
--- include/linux/cs5535.h | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
include/linux/cs5535.h +17 -17
@@ -91,21 +91,21 @@ static inline int cs5535_pic_unreqz_select_high(unsigned int group,#define CS5536_PM_GPE0_EN 0x1c/* CS5536_PM1_STS bits */ -#define CS5536_WAK_FLAG (1 << 15) -#define CS5536_RTC_FLAG (1 << 10) -#define CS5536_PWRBTN_FLAG (1 << 8) +#define CS5536_WAK_FLAG BIT(15) +#define CS5536_RTC_FLAG BIT(10) +#define CS5536_PWRBTN_FLAG BIT(8)/* CS5536_PM1_EN bits */ -#define CS5536_PM_PWRBTN (1 << 8) -#define CS5536_PM_RTC (1 << 10) +#define CS5536_PM_PWRBTN BIT(8) +#define CS5536_PM_RTC BIT(10)/* CS5536_PM_GPE0_STS bits */ -#define CS5536_GPIOM7_PME_FLAG (1 << 31) -#define CS5536_GPIOM6_PME_FLAG (1 << 30) +#define CS5536_GPIOM7_PME_FLAG BIT(31) +#define CS5536_GPIOM6_PME_FLAG BIT(30)/* CS5536_PM_GPE0_EN bits */ -#define CS5536_GPIOM7_PME_EN (1 << 31) -#define CS5536_GPIOM6_PME_EN (1 << 30) +#define CS5536_GPIOM7_PME_EN BIT(31) +#define CS5536_GPIOM6_PME_EN BIT(30)/* VSA2 magic values */ #define VSA_VRC_INDEX 0xAC1C
@@ -197,14 +197,14 @@ void cs5535_gpio_setup_event(unsigned offset, int pair, int pme);#define MFGPT_REG_COUNTER 4#define MFGPT_REG_SETUP 6 -#define MFGPT_SETUP_CNTEN (1 << 15) -#define MFGPT_SETUP_CMP2 (1 << 14) -#define MFGPT_SETUP_CMP1 (1 << 13) -#define MFGPT_SETUP_SETUP (1 << 12) -#define MFGPT_SETUP_STOPEN (1 << 11) -#define MFGPT_SETUP_EXTEN (1 << 10) -#define MFGPT_SETUP_REVEN (1 << 5) -#define MFGPT_SETUP_CLKSEL (1 << 4) +#define MFGPT_SETUP_CNTEN BIT(15) +#define MFGPT_SETUP_CMP2 BIT(14) +#define MFGPT_SETUP_CMP1 BIT(13) +#define MFGPT_SETUP_SETUP BIT(12) +#define MFGPT_SETUP_STOPEN BIT(11) +#define MFGPT_SETUP_EXTEN BIT(10) +#define MFGPT_SETUP_REVEN BIT(5) +#define MFGPT_SETUP_CLKSEL BIT(4)struct cs5535_mfgpt_timer ;
Signature · 1 line
2.22.0
PATCH 1/1 cs5535: use BIT() macro for defining bit-flags
Quoted 2 lines · Amol Surati
The
BIT()macro is available for defining the required bit-flags.
Shouldn't bits.h be added?
Quoted 4 lines
Since it operates on an unsigned value and expands to an unsigned result, using it, instead of an expression like (1 << x), also fixes the problem of shifting a signed 32-bit value by 31 bits. (e.g. 1 << 31. See
CS5536_GPIOM7_PME_FLAGandCS5536_GPIOM7_PME_EN).
What problem?
You need to describe that (UB by the standard, though gcc works fine, I never heard it utilizes such).
Signature · 2 lines
With Best Regards, Andy Shevchenko
suratiamol at gmail.com (Amol Surati)
PATCH 1/1 cs5535: use BIT() macro for defining bit-flags
Quoted 6 lines · Andy Shevchenko
Quoted 2 lines · Amol Surati
The
BIT()macro is available for defining the required bit-flags.Shouldn't
bits.hbe added?
Quoted 8 lines
Quoted 4 lines
Since it operates on an unsigned value and expands to an unsigned result, using it, instead of an expression like (1 << x), also fixes the problem of shifting a signed 32-bit value by 31 bits. (e.g. 1 << 31. See
CS5536_GPIOM7_PME_FLAGandCS5536_GPIOM7_PME_EN).What problem?
You need to describe that (UB by the standard, though gcc works fine, I never heard it utilizes such).
Yes. I will send a new version. I also missed including <linux/bitops.h>, although the compilation with the default configuration of my distro still succeeds without it, by chance.
Thank you,
-amol
Quoted 4 lines
Signature · 3 lines
-- With Best Regards, Andy Shevchenko
PATCH 1/1 cs5535: use BIT() macro for defining bit-flags
Quoted 13 lines · Amol Surati
Quoted 11 lines · Andy Shevchenko
Quoted 2 lines · Amol Surati
The
BIT()macro is available for defining the required bit-flags.Shouldn't
bits.hbe added?Quoted 4 lines
Since it operates on an unsigned value and expands to an unsigned result, using it, instead of an expression like (1 << x), also fixes the problem of shifting a signed 32-bit value by 31 bits. (e.g. 1 << 31. See
CS5536_GPIOM7_PME_FLAGandCS5536_GPIOM7_PME_EN).
Quoted 8 lines
Quoted 3 lines
What problem?
You need to describe that (UB by the standard, though gcc works fine, I never heard it utilizes such).Yes. I will send a new version. I also missed including <
linux/bitops.h>, although the compilation with the default configuration of my distro still succeeds without it, by chance.
It's now in a separate bits.h. No need to include complete bitops.h.
Quoted 7 lines
Thank you,
-amolQuoted 4 lines
--
With Best Regards,
Andy Shevchenko
Signature · 2 lines
With Best Regards, Andy Shevchenko
suratiamol at gmail.com (Amol Surati)
PATCH v2 1/1 cs5535: use BIT() macro for defining bit-flags
In reply to suratiamol at gmail.com (Amol Surati)Inside cs5535.h, there are two definitions - CS5536_GPIOM7_PME_FLAG and CS5536_GPIOM7_PME_EN
- which are defined as equal to (1 << 31), where 1 is expected to be of width at least 32-bits. But, since 1 is also treated as signed, its width reduces to 31-bits, a situation which invokes the undefined behaviour (***).
These definitions aren't a problem for gcc; it allows them to be defined and used safely. But other compilers may not, for instance, if such definitions are pulled in, as part of public APIs, into code the compilation of which may be carried out by a variety of compilers with differing compliance levels.
The BIT() macro changes the type of integer 1 to unsigned, thus allowing its shift by width-1.
Use the macro, to make the two definitions standards-compliant, and to maintain consistency.
*** About bitwise shift operators, from the C standard:
"If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined".
---
v2: As suggested by Andy Shevchenko,
- include <linux/bits.h>,
- describe the problem in detail.
include/linux/cs5535.h | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)
include/linux/cs5535.h +18 -17
@@ -8,6 +8,7 @@#ifndef _CS5535_H#define _CS5535_H +#include <linux/bits.h> #include <asm/msr.h> /* MSRs */
@@ -91,21 +92,21 @@ static inline int cs5535_pic_unreqz_select_high(unsigned int group,#define CS5536_PM_GPE0_EN 0x1c/* CS5536_PM1_STS bits */ -#define CS5536_WAK_FLAG (1 << 15) -#define CS5536_RTC_FLAG (1 << 10) -#define CS5536_PWRBTN_FLAG (1 << 8) +#define CS5536_WAK_FLAG BIT(15) +#define CS5536_RTC_FLAG BIT(10) +#define CS5536_PWRBTN_FLAG BIT(8)/* CS5536_PM1_EN bits */ -#define CS5536_PM_PWRBTN (1 << 8) -#define CS5536_PM_RTC (1 << 10) +#define CS5536_PM_PWRBTN BIT(8) +#define CS5536_PM_RTC BIT(10)/* CS5536_PM_GPE0_STS bits */ -#define CS5536_GPIOM7_PME_FLAG (1 << 31) -#define CS5536_GPIOM6_PME_FLAG (1 << 30) +#define CS5536_GPIOM7_PME_FLAG BIT(31) +#define CS5536_GPIOM6_PME_FLAG BIT(30)/* CS5536_PM_GPE0_EN bits */ -#define CS5536_GPIOM7_PME_EN (1 << 31) -#define CS5536_GPIOM6_PME_EN (1 << 30) +#define CS5536_GPIOM7_PME_EN BIT(31) +#define CS5536_GPIOM6_PME_EN BIT(30)/* VSA2 magic values */ #define VSA_VRC_INDEX 0xAC1C
@@ -197,14 +198,14 @@ void cs5535_gpio_setup_event(unsigned offset, int pair, int pme);#define MFGPT_REG_COUNTER 4#define MFGPT_REG_SETUP 6 -#define MFGPT_SETUP_CNTEN (1 << 15) -#define MFGPT_SETUP_CMP2 (1 << 14) -#define MFGPT_SETUP_CMP1 (1 << 13) -#define MFGPT_SETUP_SETUP (1 << 12) -#define MFGPT_SETUP_STOPEN (1 << 11) -#define MFGPT_SETUP_EXTEN (1 << 10) -#define MFGPT_SETUP_REVEN (1 << 5) -#define MFGPT_SETUP_CLKSEL (1 << 4) +#define MFGPT_SETUP_CNTEN BIT(15) +#define MFGPT_SETUP_CMP2 BIT(14) +#define MFGPT_SETUP_CMP1 BIT(13) +#define MFGPT_SETUP_SETUP BIT(12) +#define MFGPT_SETUP_STOPEN BIT(11) +#define MFGPT_SETUP_EXTEN BIT(10) +#define MFGPT_SETUP_REVEN BIT(5) +#define MFGPT_SETUP_CLKSEL BIT(4)struct cs5535_mfgpt_timer ;
Signature · 1 line
2.22.0