| Message ID | 20240628151346.1152838-8-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (amc6821) Various improvements | expand |
Hi Guenter, On 6/28/24 5:13 PM, Guenter Roeck wrote: > Use BIT() and GENMASK() for bit and mask definitions > to help distinguish bit and mask definitions from other > defines and to make the code easier to read. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++-------------------- > 1 file changed, 36 insertions(+), 35 deletions(-) > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > index 03ce2e3ffd86..042e2044de7b 100644 > --- a/drivers/hwmon/amc6821.c > +++ b/drivers/hwmon/amc6821.c > @@ -8,6 +8,7 @@ > * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de> > */ > > +#include <linux/bits.h> > #include <linux/err.h> > #include <linux/hwmon.h> > #include <linux/hwmon-sysfs.h> > @@ -68,46 +69,46 @@ enum chips { amc6821 }; > #define AMC6821_REG_TACH_SETTINGL 0x1e > #define AMC6821_REG_TACH_SETTINGH 0x1f > > -#define AMC6821_CONF1_START 0x01 > -#define AMC6821_CONF1_FAN_INT_EN 0x02 > -#define AMC6821_CONF1_FANIE 0x04 > -#define AMC6821_CONF1_PWMINV 0x08 > -#define AMC6821_CONF1_FAN_FAULT_EN 0x10 > -#define AMC6821_CONF1_FDRC0 0x20 > -#define AMC6821_CONF1_FDRC1 0x40 > -#define AMC6821_CONF1_THERMOVIE 0x80 > +#define AMC6821_CONF1_START BIT(0) > +#define AMC6821_CONF1_FAN_INT_EN BIT(1) > +#define AMC6821_CONF1_FANIE BIT(2) > +#define AMC6821_CONF1_PWMINV BIT(3) > +#define AMC6821_CONF1_FAN_FAULT_EN BIT(4) > +#define AMC6821_CONF1_FDRC0 BIT(5) > +#define AMC6821_CONF1_FDRC1 BIT(7) Should be BIT(6) here if I'm not mistaken. The rest looks fine to me. We could also start to use FIELD_GET, FIELD_PREP, ... instead of manually shifting and masking in-code. Cheers, Quentin
On 7/1/24 04:31, Quentin Schulz wrote: > Hi Guenter, > > On 6/28/24 5:13 PM, Guenter Roeck wrote: >> Use BIT() and GENMASK() for bit and mask definitions >> to help distinguish bit and mask definitions from other >> defines and to make the code easier to read. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++-------------------- >> 1 file changed, 36 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >> index 03ce2e3ffd86..042e2044de7b 100644 >> --- a/drivers/hwmon/amc6821.c >> +++ b/drivers/hwmon/amc6821.c >> @@ -8,6 +8,7 @@ >> * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de> >> */ >> +#include <linux/bits.h> >> #include <linux/err.h> >> #include <linux/hwmon.h> >> #include <linux/hwmon-sysfs.h> >> @@ -68,46 +69,46 @@ enum chips { amc6821 }; >> #define AMC6821_REG_TACH_SETTINGL 0x1e >> #define AMC6821_REG_TACH_SETTINGH 0x1f >> -#define AMC6821_CONF1_START 0x01 >> -#define AMC6821_CONF1_FAN_INT_EN 0x02 >> -#define AMC6821_CONF1_FANIE 0x04 >> -#define AMC6821_CONF1_PWMINV 0x08 >> -#define AMC6821_CONF1_FAN_FAULT_EN 0x10 >> -#define AMC6821_CONF1_FDRC0 0x20 >> -#define AMC6821_CONF1_FDRC1 0x40 >> -#define AMC6821_CONF1_THERMOVIE 0x80 >> +#define AMC6821_CONF1_START BIT(0) >> +#define AMC6821_CONF1_FAN_INT_EN BIT(1) >> +#define AMC6821_CONF1_FANIE BIT(2) >> +#define AMC6821_CONF1_PWMINV BIT(3) >> +#define AMC6821_CONF1_FAN_FAULT_EN BIT(4) >> +#define AMC6821_CONF1_FDRC0 BIT(5) >> +#define AMC6821_CONF1_FDRC1 BIT(7) > > Should be BIT(6) here if I'm not mistaken. > Yes, I had fixed that later but accidentally applied the fix to later patch. Guenter
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index 03ce2e3ffd86..042e2044de7b 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -8,6 +8,7 @@ * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de> */ +#include <linux/bits.h> #include <linux/err.h> #include <linux/hwmon.h> #include <linux/hwmon-sysfs.h> @@ -68,46 +69,46 @@ enum chips { amc6821 }; #define AMC6821_REG_TACH_SETTINGL 0x1e #define AMC6821_REG_TACH_SETTINGH 0x1f -#define AMC6821_CONF1_START 0x01 -#define AMC6821_CONF1_FAN_INT_EN 0x02 -#define AMC6821_CONF1_FANIE 0x04 -#define AMC6821_CONF1_PWMINV 0x08 -#define AMC6821_CONF1_FAN_FAULT_EN 0x10 -#define AMC6821_CONF1_FDRC0 0x20 -#define AMC6821_CONF1_FDRC1 0x40 -#define AMC6821_CONF1_THERMOVIE 0x80 +#define AMC6821_CONF1_START BIT(0) +#define AMC6821_CONF1_FAN_INT_EN BIT(1) +#define AMC6821_CONF1_FANIE BIT(2) +#define AMC6821_CONF1_PWMINV BIT(3) +#define AMC6821_CONF1_FAN_FAULT_EN BIT(4) +#define AMC6821_CONF1_FDRC0 BIT(5) +#define AMC6821_CONF1_FDRC1 BIT(7) +#define AMC6821_CONF1_THERMOVIE BIT(7) -#define AMC6821_CONF2_PWM_EN 0x01 -#define AMC6821_CONF2_TACH_MODE 0x02 -#define AMC6821_CONF2_TACH_EN 0x04 -#define AMC6821_CONF2_RTFIE 0x08 -#define AMC6821_CONF2_LTOIE 0x10 -#define AMC6821_CONF2_RTOIE 0x20 -#define AMC6821_CONF2_PSVIE 0x40 -#define AMC6821_CONF2_RST 0x80 +#define AMC6821_CONF2_PWM_EN BIT(0) +#define AMC6821_CONF2_TACH_MODE BIT(1) +#define AMC6821_CONF2_TACH_EN BIT(2) +#define AMC6821_CONF2_RTFIE BIT(3) +#define AMC6821_CONF2_LTOIE BIT(4) +#define AMC6821_CONF2_RTOIE BIT(5) +#define AMC6821_CONF2_PSVIE BIT(6) +#define AMC6821_CONF2_RST BIT(7) -#define AMC6821_CONF3_THERM_FAN_EN 0x80 -#define AMC6821_CONF3_REV_MASK 0x0F +#define AMC6821_CONF3_THERM_FAN_EN BIT(7) +#define AMC6821_CONF3_REV_MASK GENMASK(3, 0) -#define AMC6821_CONF4_OVREN 0x10 -#define AMC6821_CONF4_TACH_FAST 0x20 -#define AMC6821_CONF4_PSPR 0x40 -#define AMC6821_CONF4_MODE 0x80 +#define AMC6821_CONF4_OVREN BIT(4) +#define AMC6821_CONF4_TACH_FAST BIT(5) +#define AMC6821_CONF4_PSPR BIT(6) +#define AMC6821_CONF4_MODE BIT(7) -#define AMC6821_STAT1_RPM_ALARM 0x01 -#define AMC6821_STAT1_FANS 0x02 -#define AMC6821_STAT1_RTH 0x04 -#define AMC6821_STAT1_RTL 0x08 -#define AMC6821_STAT1_R_THERM 0x10 -#define AMC6821_STAT1_RTF 0x20 -#define AMC6821_STAT1_LTH 0x40 -#define AMC6821_STAT1_LTL 0x80 +#define AMC6821_STAT1_RPM_ALARM BIT(0) +#define AMC6821_STAT1_FANS BIT(1) +#define AMC6821_STAT1_RTH BIT(2) +#define AMC6821_STAT1_RTL BIT(3) +#define AMC6821_STAT1_R_THERM BIT(4) +#define AMC6821_STAT1_RTF BIT(5) +#define AMC6821_STAT1_LTH BIT(6) +#define AMC6821_STAT1_LTL BIT(7) -#define AMC6821_STAT2_RTC 0x08 -#define AMC6821_STAT2_LTC 0x10 -#define AMC6821_STAT2_LPSV 0x20 -#define AMC6821_STAT2_L_THERM 0x40 -#define AMC6821_STAT2_THERM_IN 0x80 +#define AMC6821_STAT2_RTC BIT(3) +#define AMC6821_STAT2_LTC BIT(4) +#define AMC6821_STAT2_LPSV BIT(5) +#define AMC6821_STAT2_L_THERM BIT(6) +#define AMC6821_STAT2_THERM_IN BIT(7) enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX, IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
Use BIT() and GENMASK() for bit and mask definitions to help distinguish bit and mask definitions from other defines and to make the code easier to read. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 35 deletions(-)