| Message ID | 2929234bd4ecec41c0d012edc52416ef80f3e368.1562734889.git.joe@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | treewide: Fix GENMASK misuses | expand |
On Tue, 9 Jul 2019 22:04:17 -0700 Joe Perches <joe@perches.com> wrote: > Arguments are supposed to be ordered high then low. > > Signed-off-by: Joe Perches <joe@perches.com> Applied to the fixes-togreg branch of iio.git and marked for stable etc. Thanks, Jonathan > --- > drivers/iio/adc/max9611.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c > index 917223d5ff5b..0e3c6529fc4c 100644 > --- a/drivers/iio/adc/max9611.c > +++ b/drivers/iio/adc/max9611.c > @@ -83,7 +83,7 @@ > #define MAX9611_TEMP_MAX_POS 0x7f80 > #define MAX9611_TEMP_MAX_NEG 0xff80 > #define MAX9611_TEMP_MIN_NEG 0xd980 > -#define MAX9611_TEMP_MASK GENMASK(7, 15) > +#define MAX9611_TEMP_MASK GENMASK(15, 7) > #define MAX9611_TEMP_SHIFT 0x07 > #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) > #define MAX9611_TEMP_SCALE_NUM 1000000
On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote: > On Tue, 9 Jul 2019 22:04:17 -0700 > Joe Perches <joe@perches.com> wrote: > > > Arguments are supposed to be ordered high then low. > > > > Signed-off-by: Joe Perches <joe@perches.com> > > Applied to the fixes-togreg branch of iio.git and marked for > stable etc. This mask is used in an init function called from a probe. I don't have this hardware but it looks as if it could never have worked so I doubt the driver and the hardware have ever been tested. Does anyone have this device in actual use? regval = ret & MAX9611_TEMP_MASK; if ((regval > MAX9611_TEMP_MAX_POS && regval < MAX9611_TEMP_MIN_NEG) || regval > MAX9611_TEMP_MAX_NEG) { dev_err(max9611->dev, "Invalid value received from ADC 0x%4x: aborting\n", regval); return -EIO; } > Thanks, > > Jonathan > > > --- > > drivers/iio/adc/max9611.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c > > index 917223d5ff5b..0e3c6529fc4c 100644 > > --- a/drivers/iio/adc/max9611.c > > +++ b/drivers/iio/adc/max9611.c > > @@ -83,7 +83,7 @@ > > #define MAX9611_TEMP_MAX_POS 0x7f80 > > #define MAX9611_TEMP_MAX_NEG 0xff80 > > #define MAX9611_TEMP_MIN_NEG 0xd980 > > -#define MAX9611_TEMP_MASK GENMASK(7, 15) > > +#define MAX9611_TEMP_MASK GENMASK(15, 7) > > #define MAX9611_TEMP_SHIFT 0x07 > > #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) > > #define MAX9611_TEMP_SCALE_NUM 1000000
Hi Joe, Jonathan, On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote: > On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote: > > On Tue, 9 Jul 2019 22:04:17 -0700 > > Joe Perches <joe@perches.com> wrote: > > > > > Arguments are supposed to be ordered high then low. > > > > > > Signed-off-by: Joe Perches <joe@perches.com> > > > > Applied to the fixes-togreg branch of iio.git and marked for > > stable etc. > > This mask is used in an init function called from a probe. > > I don't have this hardware but it looks as if it could > never have worked so I doubt the driver and the hardware > have ever been tested. > Ups, this is embarassing! Thanks for noticing I actually tested the device before sending the driver of course > Does anyone have this device in actual use? > The driver is currently in use in Renesas Gen3 Salvator boards: arch/arm64/boot/dts/renesas/salvator-common.dtsi:466 I might need some time before I can actually test and tell what's happening though. It might work by pure chance. Fortunately the mask is only used for validation of the temperature reading at probe time, and I can tell this passes (we would have noticed otherwise, Salvator is one of the reference Gen3 platforms for upstream development). As said I might need some time before I can test this, but the change is indeed correct. Thanks j > > regval = ret & MAX9611_TEMP_MASK; > > if ((regval > MAX9611_TEMP_MAX_POS && > regval < MAX9611_TEMP_MIN_NEG) || > regval > MAX9611_TEMP_MAX_NEG) { > dev_err(max9611->dev, > "Invalid value received from ADC 0x%4x: aborting\n", > regval); > return -EIO; > } > > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/adc/max9611.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c > > > index 917223d5ff5b..0e3c6529fc4c 100644 > > > --- a/drivers/iio/adc/max9611.c > > > +++ b/drivers/iio/adc/max9611.c > > > @@ -83,7 +83,7 @@ > > > #define MAX9611_TEMP_MAX_POS 0x7f80 > > > #define MAX9611_TEMP_MAX_NEG 0xff80 > > > #define MAX9611_TEMP_MIN_NEG 0xd980 > > > -#define MAX9611_TEMP_MASK GENMASK(7, 15) > > > +#define MAX9611_TEMP_MASK GENMASK(15, 7) > > > #define MAX9611_TEMP_SHIFT 0x07 > > > #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) > > > #define MAX9611_TEMP_SCALE_NUM 1000000 >
Hello, so I finally run some test and... On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote: > On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote: > > On Tue, 9 Jul 2019 22:04:17 -0700 > > Joe Perches <joe@perches.com> wrote: > > > > > Arguments are supposed to be ordered high then low. > > > > > > Signed-off-by: Joe Perches <joe@perches.com> > > > > Applied to the fixes-togreg branch of iio.git and marked for > > stable etc. I don't see it in v5.3-rc2, has it been collected or are we still in time for an additional fix? > > This mask is used in an init function called from a probe. > > I don't have this hardware but it looks as if it could > never have worked so I doubt the driver and the hardware > have ever been tested. > > Does anyone have this device in actual use? Because it turns out this is 2 times embarrassing. The mask definition is indeed wrong, as Joe reported and fixed, but also this line > > regval = ret & MAX9611_TEMP_MASK; is very wrong as regval is read as: ret = max9611_read_single(max9611, CONF_TEMP, ®val); So that should actually be: regval &= MAX9611_TEMP_MASK; not regval = ret & MAX9611_TEMP_MASK; Ups... Yes, it worked by chance, as regval was always 0, which is in the range of acceptable temperatures :/ > > if ((regval > MAX9611_TEMP_MAX_POS && > regval < MAX9611_TEMP_MIN_NEG) || > regval > MAX9611_TEMP_MAX_NEG) { Also reading this condition and how I had defined the temperature calculation formula makes me wonder if this it totally correct, but for the moment: 1) if Joe's patch has been collected, I can send an additional patch to fix how regval is computed. 2) If Joe's patch still have to be collected, the regval computation might be fixed there. Sorry for taking so long to get back to you and thanks for noticing. Thanks j > dev_err(max9611->dev, > "Invalid value received from ADC 0x%4x: aborting\n", > regval); > return -EIO; > } > > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/adc/max9611.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c > > > index 917223d5ff5b..0e3c6529fc4c 100644 > > > --- a/drivers/iio/adc/max9611.c > > > +++ b/drivers/iio/adc/max9611.c > > > @@ -83,7 +83,7 @@ > > > #define MAX9611_TEMP_MAX_POS 0x7f80 > > > #define MAX9611_TEMP_MAX_NEG 0xff80 > > > #define MAX9611_TEMP_MIN_NEG 0xd980 > > > -#define MAX9611_TEMP_MASK GENMASK(7, 15) > > > +#define MAX9611_TEMP_MASK GENMASK(15, 7) > > > #define MAX9611_TEMP_SHIFT 0x07 > > > #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) > > > #define MAX9611_TEMP_SCALE_NUM 1000000 >
On Mon, 29 Jul 2019 23:52:14 +0200 Jacopo Mondi <jacopo@jmondi.org> wrote: > Hello, > so I finally run some test and... > > On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote: > > On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote: > > > On Tue, 9 Jul 2019 22:04:17 -0700 > > > Joe Perches <joe@perches.com> wrote: > > > > > > > Arguments are supposed to be ordered high then low. > > > > > > > > Signed-off-by: Joe Perches <joe@perches.com> > > > > > > Applied to the fixes-togreg branch of iio.git and marked for > > > stable etc. > > I don't see it in v5.3-rc2, has it been collected or are we still in > time for an additional fix? > > > > > This mask is used in an init function called from a probe. > > > > I don't have this hardware but it looks as if it could > > never have worked so I doubt the driver and the hardware > > have ever been tested. > > > > Does anyone have this device in actual use? > > Because it turns out this is 2 times embarrassing. The mask definition > is indeed wrong, as Joe reported and fixed, but also this line > > > > regval = ret & MAX9611_TEMP_MASK; > > is very wrong as regval is read as: > ret = max9611_read_single(max9611, CONF_TEMP, ®val); > > So that should actually be: > regval &= MAX9611_TEMP_MASK; > not > regval = ret & MAX9611_TEMP_MASK; > Ups... > > Yes, it worked by chance, as regval was always 0, which is in the > range of acceptable temperatures :/ > > > > > if ((regval > MAX9611_TEMP_MAX_POS && > > regval < MAX9611_TEMP_MIN_NEG) || > > regval > MAX9611_TEMP_MAX_NEG) { > > Also reading this condition and how I had defined the temperature > calculation formula makes me wonder if this it totally correct, but > for the moment: > > 1) if Joe's patch has been collected, I can send an additional patch to > fix how regval is computed. > 2) If Joe's patch still have to be collected, the regval computation > might be fixed there. I think this will have hit linux-next on the same day as your email. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/iio/adc?id=ae8cc91a7d85e018c0c267f580820b2bb558cd48 So follow up patch please. Thanks! Jonathan > > Sorry for taking so long to get back to you and thanks for noticing. > > Thanks > j > > > dev_err(max9611->dev, > > "Invalid value received from ADC 0x%4x: aborting\n", > > regval); > > return -EIO; > > } > > > > > > > Thanks, > > > > > > Jonathan > > > > > > > --- > > > > drivers/iio/adc/max9611.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c > > > > index 917223d5ff5b..0e3c6529fc4c 100644 > > > > --- a/drivers/iio/adc/max9611.c > > > > +++ b/drivers/iio/adc/max9611.c > > > > @@ -83,7 +83,7 @@ > > > > #define MAX9611_TEMP_MAX_POS 0x7f80 > > > > #define MAX9611_TEMP_MAX_NEG 0xff80 > > > > #define MAX9611_TEMP_MIN_NEG 0xd980 > > > > -#define MAX9611_TEMP_MASK GENMASK(7, 15) > > > > +#define MAX9611_TEMP_MASK GENMASK(15, 7) > > > > #define MAX9611_TEMP_SHIFT 0x07 > > > > #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) > > > > #define MAX9611_TEMP_SCALE_NUM 1000000 > > >
diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c index 917223d5ff5b..0e3c6529fc4c 100644 --- a/drivers/iio/adc/max9611.c +++ b/drivers/iio/adc/max9611.c @@ -83,7 +83,7 @@ #define MAX9611_TEMP_MAX_POS 0x7f80 #define MAX9611_TEMP_MAX_NEG 0xff80 #define MAX9611_TEMP_MIN_NEG 0xd980 -#define MAX9611_TEMP_MASK GENMASK(7, 15) +#define MAX9611_TEMP_MASK GENMASK(15, 7) #define MAX9611_TEMP_SHIFT 0x07 #define MAX9611_TEMP_RAW(_r) ((_r) >> MAX9611_TEMP_SHIFT) #define MAX9611_TEMP_SCALE_NUM 1000000
Arguments are supposed to be ordered high then low. Signed-off-by: Joe Perches <joe@perches.com> --- drivers/iio/adc/max9611.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)