| Message ID | 1542376031-179816-1-git-send-email-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [hwmon-next,v1,1/1] hwmon: (mlxreg-fan) Fix macros for tacho fault reading | expand |
On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > Fix macros for tacometer fault reading. > This fix is relevant for three Mellanox systems MQMB7, MSN37, MSN34, > which are about to be released to the customers. > At the moment, none of them is at customers sites. > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox FAN driver") > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> Applied. Thanks, Guenter > --- > drivers/hwmon/mlxreg-fan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > index de46577..d8fa4be 100644 > --- a/drivers/hwmon/mlxreg-fan.c > +++ b/drivers/hwmon/mlxreg-fan.c > @@ -51,7 +51,7 @@ > */ > #define MLXREG_FAN_GET_RPM(rval, d, s) (DIV_ROUND_CLOSEST(15000000 * 100, \ > ((rval) + (s)) * (d))) > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) > #define MLXREG_FAN_PWM_DUTY2STATE(duty) (DIV_ROUND_CLOSEST((duty) * \ > MLXREG_FAN_MAX_STATE, \ > MLXREG_FAN_MAX_DUTY))
On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > Fix macros for tacometer fault reading. > This fix is relevant for three Mellanox systems MQMB7, MSN37, MSN34, > which are about to be released to the customers. > At the moment, none of them is at customers sites. > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox FAN driver") > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > --- > drivers/hwmon/mlxreg-fan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > index de46577..d8fa4be 100644 > --- a/drivers/hwmon/mlxreg-fan.c > +++ b/drivers/hwmon/mlxreg-fan.c > @@ -51,7 +51,7 @@ > */ > #define MLXREG_FAN_GET_RPM(rval, d, s) (DIV_ROUND_CLOSEST(15000000 * 100, \ > ((rval) + (s)) * (d))) > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) You might want to check if the "^" is correct here. It looks fishy, since the expression only evaluates to true if a no bit of val outside mask is set. I would have expected "&". Guenter > #define MLXREG_FAN_PWM_DUTY2STATE(duty) (DIV_ROUND_CLOSEST((duty) * \ > MLXREG_FAN_MAX_STATE, \ > MLXREG_FAN_MAX_DUTY))
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Friday, November 16, 2018 6:15 PM > To: Vadim Pasternak <vadimp@mellanox.com> > Cc: linux-hwmon@vger.kernel.org; Michael Shych <michaelsh@mellanox.com> > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix macros for > tacho fault reading > > On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > > Fix macros for tacometer fault reading. > > This fix is relevant for three Mellanox systems MQMB7, MSN37, MSN34, > > which are about to be released to the customers. > > At the moment, none of them is at customers sites. > > > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox FAN > > driver") > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > --- > > drivers/hwmon/mlxreg-fan.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > > index de46577..d8fa4be 100644 > > --- a/drivers/hwmon/mlxreg-fan.c > > +++ b/drivers/hwmon/mlxreg-fan.c > > @@ -51,7 +51,7 @@ > > */ > > #define MLXREG_FAN_GET_RPM(rval, d, s) > (DIV_ROUND_CLOSEST(15000000 * 100, \ > > ((rval) + (s)) * (d))) > > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) > > You might want to check if the "^" is correct here. It looks fishy, since the > expression only evaluates to true if a no bit of val outside mask is set. I would > have expected "&". > > Guenter Hi Guenter, Thanks for you your comment. I'll follow your suggestion and change a macros to" #define MLXREG_FAN_GET_FAULT(val, mask) (!((val & mask) ^ (mask))) > > > #define MLXREG_FAN_PWM_DUTY2STATE(duty) > (DIV_ROUND_CLOSEST((duty) * \ > > MLXREG_FAN_MAX_STATE, > \ > > MLXREG_FAN_MAX_DUTY))
On Mon, Nov 19, 2018 at 06:07:40AM +0000, Vadim Pasternak wrote: > > > > -----Original Message----- > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > Sent: Friday, November 16, 2018 6:15 PM > > To: Vadim Pasternak <vadimp@mellanox.com> > > Cc: linux-hwmon@vger.kernel.org; Michael Shych <michaelsh@mellanox.com> > > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix macros for > > tacho fault reading > > > > On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > > > Fix macros for tacometer fault reading. > > > This fix is relevant for three Mellanox systems MQMB7, MSN37, MSN34, > > > which are about to be released to the customers. > > > At the moment, none of them is at customers sites. > > > > > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox FAN > > > driver") > > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > > --- > > > drivers/hwmon/mlxreg-fan.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > > > index de46577..d8fa4be 100644 > > > --- a/drivers/hwmon/mlxreg-fan.c > > > +++ b/drivers/hwmon/mlxreg-fan.c > > > @@ -51,7 +51,7 @@ > > > */ > > > #define MLXREG_FAN_GET_RPM(rval, d, s) > > (DIV_ROUND_CLOSEST(15000000 * 100, \ > > > ((rval) + (s)) * (d))) > > > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > > > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) > > > > You might want to check if the "^" is correct here. It looks fishy, since the > > expression only evaluates to true if a no bit of val outside mask is set. I would > > have expected "&". > > > > Guenter > > Hi Guenter, > > Thanks for you your comment. > > I'll follow your suggestion and change a macros to" > #define MLXREG_FAN_GET_FAULT(val, mask) (!((val & mask) ^ (mask))) > That seems excessively complicated. Why the xor after the & ? Guenter > > > > > #define MLXREG_FAN_PWM_DUTY2STATE(duty) > > (DIV_ROUND_CLOSEST((duty) * \ > > > MLXREG_FAN_MAX_STATE, > > \ > > > MLXREG_FAN_MAX_DUTY))
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Monday, November 19, 2018 6:15 PM > To: Vadim Pasternak <vadimp@mellanox.com> > Cc: linux-hwmon@vger.kernel.org; Michael Shych <michaelsh@mellanox.com> > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix macros for > tacho fault reading > > On Mon, Nov 19, 2018 at 06:07:40AM +0000, Vadim Pasternak wrote: > > > > > > > -----Original Message----- > > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > > Sent: Friday, November 16, 2018 6:15 PM > > > To: Vadim Pasternak <vadimp@mellanox.com> > > > Cc: linux-hwmon@vger.kernel.org; Michael Shych > > > <michaelsh@mellanox.com> > > > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix > > > macros for tacho fault reading > > > > > > On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > > > > Fix macros for tacometer fault reading. > > > > This fix is relevant for three Mellanox systems MQMB7, MSN37, > > > > MSN34, which are about to be released to the customers. > > > > At the moment, none of them is at customers sites. > > > > > > > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox > > > > FAN > > > > driver") > > > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > > > --- > > > > drivers/hwmon/mlxreg-fan.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/hwmon/mlxreg-fan.c > > > > b/drivers/hwmon/mlxreg-fan.c index de46577..d8fa4be 100644 > > > > --- a/drivers/hwmon/mlxreg-fan.c > > > > +++ b/drivers/hwmon/mlxreg-fan.c > > > > @@ -51,7 +51,7 @@ > > > > */ > > > > #define MLXREG_FAN_GET_RPM(rval, d, s) > > > (DIV_ROUND_CLOSEST(15000000 * 100, \ > > > > ((rval) + (s)) * (d))) > > > > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > > > > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) > > > > > > You might want to check if the "^" is correct here. It looks fishy, > > > since the expression only evaluates to true if a no bit of val > > > outside mask is set. I would have expected "&". > > > > > > Guenter > > > > Hi Guenter, > > > > Thanks for you your comment. > > > > I'll follow your suggestion and change a macros to" > > #define MLXREG_FAN_GET_FAULT(val, mask) (!((val & mask) ^ (mask))) > > > That seems excessively complicated. Why the xor after the & ? I wanted to evaluate MLXREG_FAN_GET_FAULT to true in case HW reports the fault. For 8 bits addressing device in case of fault val is 0xff and mask is set to 0xff. For 16 bits - val will be 0xffff and mask will be set to 0xffff. So, just simply this macros will work for me: #define MLXREG_FAN_GET_FAULT(val, mask) ((val) == (mask)) or in order to be on the safe side: #define MLXREG_FAN_GET_FAULT(val, mask) ((val & mask) == (mask)) > > Guenter > > > > > > > > #define MLXREG_FAN_PWM_DUTY2STATE(duty) > > > (DIV_ROUND_CLOSEST((duty) * \ > > > > MLXREG_FAN_MAX_STATE, > > > \ > > > > MLXREG_FAN_MAX_DUTY))
On Tue, Nov 20, 2018 at 02:46:05PM +0000, Vadim Pasternak wrote: > > > > -----Original Message----- > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > Sent: Monday, November 19, 2018 6:15 PM > > To: Vadim Pasternak <vadimp@mellanox.com> > > Cc: linux-hwmon@vger.kernel.org; Michael Shych <michaelsh@mellanox.com> > > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix macros for > > tacho fault reading > > > > On Mon, Nov 19, 2018 at 06:07:40AM +0000, Vadim Pasternak wrote: > > > > > > > > > > -----Original Message----- > > > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > > > Sent: Friday, November 16, 2018 6:15 PM > > > > To: Vadim Pasternak <vadimp@mellanox.com> > > > > Cc: linux-hwmon@vger.kernel.org; Michael Shych > > > > <michaelsh@mellanox.com> > > > > Subject: Re: [PATCH hwmon-next v1 1/1] hwmon: (mlxreg-fan) Fix > > > > macros for tacho fault reading > > > > > > > > On Fri, Nov 16, 2018 at 01:47:11PM +0000, Vadim Pasternak wrote: > > > > > Fix macros for tacometer fault reading. > > > > > This fix is relevant for three Mellanox systems MQMB7, MSN37, > > > > > MSN34, which are about to be released to the customers. > > > > > At the moment, none of them is at customers sites. > > > > > > > > > > Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox > > > > > FAN > > > > > driver") > > > > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > > > > > --- > > > > > drivers/hwmon/mlxreg-fan.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/hwmon/mlxreg-fan.c > > > > > b/drivers/hwmon/mlxreg-fan.c index de46577..d8fa4be 100644 > > > > > --- a/drivers/hwmon/mlxreg-fan.c > > > > > +++ b/drivers/hwmon/mlxreg-fan.c > > > > > @@ -51,7 +51,7 @@ > > > > > */ > > > > > #define MLXREG_FAN_GET_RPM(rval, d, s) > > > > (DIV_ROUND_CLOSEST(15000000 * 100, \ > > > > > ((rval) + (s)) * (d))) > > > > > -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) > > > > > +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) > > > > > > > > You might want to check if the "^" is correct here. It looks fishy, > > > > since the expression only evaluates to true if a no bit of val > > > > outside mask is set. I would have expected "&". > > > > > > > > Guenter > > > > > > Hi Guenter, > > > > > > Thanks for you your comment. > > > > > > I'll follow your suggestion and change a macros to" > > > #define MLXREG_FAN_GET_FAULT(val, mask) (!((val & mask) ^ (mask))) > > > > > That seems excessively complicated. Why the xor after the & ? > > I wanted to evaluate MLXREG_FAN_GET_FAULT to true in case HW > reports the fault. > For 8 bits addressing device in case of fault val is 0xff and mask is set to 0xff. > For 16 bits - val will be 0xffff and mask will be set to 0xffff. > > So, just simply this macros will work for me: > #define MLXREG_FAN_GET_FAULT(val, mask) ((val) == (mask)) > or in order to be on the safe side: > #define MLXREG_FAN_GET_FAULT(val, mask) ((val & mask) == (mask)) > That would be much easier to understand. Thanks, Guenter > > > > Guenter > > > > > > > > > > > #define MLXREG_FAN_PWM_DUTY2STATE(duty) > > > > (DIV_ROUND_CLOSEST((duty) * \ > > > > > MLXREG_FAN_MAX_STATE, > > > > \ > > > > > MLXREG_FAN_MAX_DUTY))
diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c index de46577..d8fa4be 100644 --- a/drivers/hwmon/mlxreg-fan.c +++ b/drivers/hwmon/mlxreg-fan.c @@ -51,7 +51,7 @@ */ #define MLXREG_FAN_GET_RPM(rval, d, s) (DIV_ROUND_CLOSEST(15000000 * 100, \ ((rval) + (s)) * (d))) -#define MLXREG_FAN_GET_FAULT(val, mask) (!!((val) ^ (mask))) +#define MLXREG_FAN_GET_FAULT(val, mask) (!((val) ^ (mask))) #define MLXREG_FAN_PWM_DUTY2STATE(duty) (DIV_ROUND_CLOSEST((duty) * \ MLXREG_FAN_MAX_STATE, \ MLXREG_FAN_MAX_DUTY))
Fix macros for tacometer fault reading. This fix is relevant for three Mellanox systems MQMB7, MSN37, MSN34, which are about to be released to the customers. At the moment, none of them is at customers sites. Fixes: 65afb4c8e7e4 ("hwmon: (mlxreg-fan) Add support for Mellanox FAN driver") Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> --- drivers/hwmon/mlxreg-fan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)