* re: iio: mlx90614: Implement filter configuration @ 2015-10-01 22:16 Dan Carpenter 2015-10-01 23:09 ` Crt Mori 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2015-10-01 22:16 UTC (permalink / raw) To: cmo; +Cc: linux-iio Hello Crt Mori, The patch 764589b688a1: "iio: mlx90614: Implement filter configuration" from Aug 17, 2015, leads to the following static checker warning: drivers/iio/temperature/mlx90614.c:167 mlx90614_iir_search() warn: this cast is a no-op drivers/iio/temperature/mlx90614.c 158 ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG); 159 if (ret > 0) 160 return ret; 161 162 /* Write changed values */ 163 ret = mlx90614_write_word(client, MLX90614_CONFIG, 164 (i << MLX90614_CONFIG_IIR_SHIFT) | 165 (((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | 166 ((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) & 167 (~(u16) MLX90614_CONFIG_IIR_MASK))); Quite a few of these casts make no sense. It's not clear what was intended. (~(u16) MLX90614_CONFIG_IIR_MASK) So we take int 0x7 and cast it to u16, then because of type promotion we convert it to int and do a bitwise negate. The static checker warning is because often that means (u16)~MLX90614_CONFIG_IIR_MASK is intended instead. In this case it looks like we could just remove the cast with no harm done. But why are we ANDing it with "ret" which is a negative error code??? I think there is some other typo here beyond the extra casts. 168 return ret; regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-01 22:16 iio: mlx90614: Implement filter configuration Dan Carpenter @ 2015-10-01 23:09 ` Crt Mori 2015-10-02 7:00 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Crt Mori @ 2015-10-01 23:09 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio Hello Dan Carpenter, Thanks for checking. You found an error although not where checker pointed. See explanation inline On 2 October 2015 at 00:16, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Hello Crt Mori, > > The patch 764589b688a1: "iio: mlx90614: Implement filter > configuration" from Aug 17, 2015, leads to the following static > checker warning: > > drivers/iio/temperature/mlx90614.c:167 mlx90614_iir_search() > warn: this cast is a no-op > > drivers/iio/temperature/mlx90614.c > 158 ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG); > 159 if (ret > 0) Here is the error: it should be if (ret < 0) . Do you want to send a bugfix patch? > 160 return ret; > 161 > 162 /* Write changed values */ > 163 ret = mlx90614_write_word(client, MLX90614_CONFIG, > 164 (i << MLX90614_CONFIG_IIR_SHIFT) | > 165 (((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | > 166 ((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) & > 167 (~(u16) MLX90614_CONFIG_IIR_MASK))); > > Quite a few of these casts make no sense. It's not clear what was > intended. > > (~(u16) MLX90614_CONFIG_IIR_MASK) (~((u16) MLX90614_CONFIG_IIR_MASK)) does not seem better, or it does? This was intended (so to cast value to 16 bit, then negate it) > > So we take int 0x7 and cast it to u16, then because of type promotion > we convert it to int and do a bitwise negate. The static checker > warning is because often that means (u16)~MLX90614_CONFIG_IIR_MASK is > intended instead. In this case it looks like we could just remove the > cast with no harm done. True, we could remove the cast, but I wanted to make sure that u16 data type is used since ret and i are not that datatypes. Also macros in theory are not u16 and I like doing bitwise operations on known (equal) datatypes. > > But why are we ANDing it with "ret" which is a negative error code??? This is a bug which you found. There should not be negative ret in this case because of above if statement (line 160). There should be a result of currently read CONFIG register in there and because we need to set two bits with two masks as well as maintain all other values unchanged there is that bitwise operation. In first part it shifts IIR value in position, then it shifts fixed 0x7 value in FIR position (since we have put a fixed FIR value - see table in commit message), then it applies negative FIR and IIR mask to current register values so that other bits remain unchanged. Because sensor offers also changes to FIR, I would like to keep it this way in case someone decides he/she needs to change FIR as well. > I think there is some other typo here beyond the extra casts. > > 168 return ret; Why is this a typo? Function does return negative value if write fails so we should pass it back up. > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-01 23:09 ` Crt Mori @ 2015-10-02 7:00 ` Dan Carpenter 2015-10-02 8:12 ` Crt Mori 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2015-10-02 7:00 UTC (permalink / raw) To: Crt Mori; +Cc: linux-iio On Thu, Oct 01, 2015 at 04:09:02PM -0700, Crt Mori wrote: > Hello Dan Carpenter, > Thanks for checking. You found an error although not where checker pointed. > See explanation inline > On 2 October 2015 at 00:16, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Hello Crt Mori, > > > > The patch 764589b688a1: "iio: mlx90614: Implement filter > > configuration" from Aug 17, 2015, leads to the following static > > checker warning: > > > > drivers/iio/temperature/mlx90614.c:167 mlx90614_iir_search() > > warn: this cast is a no-op > > > > drivers/iio/temperature/mlx90614.c > > 158 ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG); > > 159 if (ret > 0) > Here is the error: it should be if (ret < 0) . Do you want to send a > bugfix patch? Nah. Just give me a reported-by tag, please. > > 160 return ret; > > 161 > > 162 /* Write changed values */ > > 163 ret = mlx90614_write_word(client, MLX90614_CONFIG, > > 164 (i << MLX90614_CONFIG_IIR_SHIFT) | > > 165 (((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | > > 166 ((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) & > > 167 (~(u16) MLX90614_CONFIG_IIR_MASK))); > > > > Quite a few of these casts make no sense. It's not clear what was > > intended. > > > > (~(u16) MLX90614_CONFIG_IIR_MASK) > (~((u16) MLX90614_CONFIG_IIR_MASK)) does not seem better, or it does? This was > intended (so to cast value to 16 bit, then negate it) Because of type promotion what actually happens is we cast it to u16 then we cast it to int then we negate it. > > > > So we take int 0x7 and cast it to u16, then because of type promotion > > we convert it to int and do a bitwise negate. The static checker > > warning is because often that means (u16)~MLX90614_CONFIG_IIR_MASK is > > intended instead. In this case it looks like we could just remove the > > cast with no harm done. > True, we could remove the cast, but I wanted to make sure that u16 > data type is used > since ret and i are not that datatypes. Because of type promotion int type is used not u16. The u16 casts are misleading no-ops because people think that u16 data type is used. Hence the static checker warning. It looks a little more readable without any casts and it works exactly the same since the casts were just for decoration. ret = mlx90614_write_word(client, MLX90614_CONFIG, (i << MLX90614_CONFIG_IIR_SHIFT) | ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | (ret & ~MLX90614_CONFIG_FIR_MASK) & (~MLX90614_CONFIG_IIR_MASK))); The 0x7 is a magic number. I would think it would be a named _MASK macro. Ah, we do have a macro for that. ret = mlx90614_write_word(client, MLX90614_CONFIG, (i << MLX90614_CONFIG_IIR_SHIFT) | (MLX90614_CONFIG_FIR_MASK | (ret & ~MLX90614_CONFIG_FIR_MASK) & (~MLX90614_CONFIG_IIR_MASK))); The MLX90614_CONFIG_FIR_MASK and ~MLX90614_CONFIG_FIR_MASK can be simplified. ret = mlx90614_write_word(client, MLX90614_CONFIG, (i << MLX90614_CONFIG_IIR_SHIFT) | (ret | MLX90614_CONFIG_FIR_MASK & ~MLX90614_CONFIG_IIR_MASK)); Once we have simplified the code, then it looks buggy to me. I suspect that it is supposed to look like: ret = mlx90614_write_word(client, MLX90614_CONFIG, (i << MLX90614_CONFIG_IIR_SHIFT) | (ret << MLX90614_CONFIG_FIR_SHIFT); Or possibly like: ret = mlx90614_write_word(client, MLX90614_CONFIG, (i << MLX90614_CONFIG_IIR_SHIFT) | (ret & MLX90614_CONFIG_FIR_MASK); Keep in mind that: ret & MLX90614_CONFIG_FIR_MASK And ret & MLX90614_CONFIG_FIR_MASK & ~MLX90614_CONFIG_IIR_MASK are equivalent. But the first one is simpler. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-02 7:00 ` Dan Carpenter @ 2015-10-02 8:12 ` Crt Mori 2015-10-02 8:32 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Crt Mori @ 2015-10-02 8:12 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio On 2 October 2015 at 09:00, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Oct 01, 2015 at 04:09:02PM -0700, Crt Mori wrote: >> Hello Dan Carpenter, >> Thanks for checking. You found an error although not where checker pointed. >> See explanation inline >> On 2 October 2015 at 00:16, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> > Hello Crt Mori, >> > >> > The patch 764589b688a1: "iio: mlx90614: Implement filter >> > configuration" from Aug 17, 2015, leads to the following static >> > checker warning: >> > >> > drivers/iio/temperature/mlx90614.c:167 mlx90614_iir_search() >> > warn: this cast is a no-op >> > >> > drivers/iio/temperature/mlx90614.c >> > 158 ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG); >> > 159 if (ret > 0) >> Here is the error: it should be if (ret < 0) . Do you want to send a >> bugfix patch? > > Nah. Just give me a reported-by tag, please. > OK, will prepare bugfix in few hours. >> > 160 return ret; >> > 161 >> > 162 /* Write changed values */ >> > 163 ret = mlx90614_write_word(client, MLX90614_CONFIG, >> > 164 (i << MLX90614_CONFIG_IIR_SHIFT) | >> > 165 (((u16) ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | >> > 166 ((u16) ret & (~((u16) MLX90614_CONFIG_FIR_MASK))))) & >> > 167 (~(u16) MLX90614_CONFIG_IIR_MASK))); >> > >> > Quite a few of these casts make no sense. It's not clear what was >> > intended. >> > >> > (~(u16) MLX90614_CONFIG_IIR_MASK) >> (~((u16) MLX90614_CONFIG_IIR_MASK)) does not seem better, or it does? This was >> intended (so to cast value to 16 bit, then negate it) > > Because of type promotion what actually happens is we cast it to u16 > then we cast it to int then we negate it. > >> > >> > So we take int 0x7 and cast it to u16, then because of type promotion >> > we convert it to int and do a bitwise negate. The static checker >> > warning is because often that means (u16)~MLX90614_CONFIG_IIR_MASK is >> > intended instead. In this case it looks like we could just remove the >> > cast with no harm done. >> True, we could remove the cast, but I wanted to make sure that u16 >> data type is used >> since ret and i are not that datatypes. > > Because of type promotion int type is used not u16. The u16 casts are > misleading no-ops because people think that u16 data type is used. > Hence the static checker warning. > OK, seems like I am one of the people thinking that. Will follow your advice and cast afterwards. > It looks a little more readable without any casts and it works exactly > the same since the casts were just for decoration. > Like I said they are on most processors, but might not be on others. I rather have it defined than let compiler decide > ret = mlx90614_write_word(client, MLX90614_CONFIG, > (i << MLX90614_CONFIG_IIR_SHIFT) | > ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | > (ret & ~MLX90614_CONFIG_FIR_MASK) & > (~MLX90614_CONFIG_IIR_MASK))); > > The 0x7 is a magic number. I would think it would be a named _MASK > macro. Ah, we do have a macro for that. That macro is for mask and it is just coincidence that they are the same. I would like to keep the way it is in case someone wants to change FIR values as well. > > ret = mlx90614_write_word(client, MLX90614_CONFIG, > (i << MLX90614_CONFIG_IIR_SHIFT) | > (MLX90614_CONFIG_FIR_MASK | > (ret & ~MLX90614_CONFIG_FIR_MASK) & > (~MLX90614_CONFIG_IIR_MASK))); > > The MLX90614_CONFIG_FIR_MASK and ~MLX90614_CONFIG_FIR_MASK can be > simplified. > > ret = mlx90614_write_word(client, MLX90614_CONFIG, > (i << MLX90614_CONFIG_IIR_SHIFT) | > (ret | MLX90614_CONFIG_FIR_MASK & > ~MLX90614_CONFIG_IIR_MASK)); > > Once we have simplified the code, then it looks buggy to me. I suspect > that it is supposed to look like: > > ret = mlx90614_write_word(client, MLX90614_CONFIG, > (i << MLX90614_CONFIG_IIR_SHIFT) | > (ret << MLX90614_CONFIG_FIR_SHIFT); > > Or possibly like: > > ret = mlx90614_write_word(client, MLX90614_CONFIG, > (i << MLX90614_CONFIG_IIR_SHIFT) | > (ret & MLX90614_CONFIG_FIR_MASK); > > Keep in mind that: > > ret & MLX90614_CONFIG_FIR_MASK > > And > ret & MLX90614_CONFIG_FIR_MASK & ~MLX90614_CONFIG_IIR_MASK > > are equivalent. But the first one is simpler. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-02 8:12 ` Crt Mori @ 2015-10-02 8:32 ` Dan Carpenter 2015-10-02 8:48 ` Crt Mori 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2015-10-02 8:32 UTC (permalink / raw) To: Crt Mori; +Cc: linux-iio On Fri, Oct 02, 2015 at 10:12:01AM +0200, Crt Mori wrote: > > It looks a little more readable without any casts and it works exactly > > the same since the casts were just for decoration. > > > > Like I said they are on most processors, but might not be on others. And monkeys *might* fly out of my butt. > I rather have it defined than let compiler decide Focus on writing simple code and not making it portable for time travellers to 1980. > > > ret = mlx90614_write_word(client, MLX90614_CONFIG, > > (i << MLX90614_CONFIG_IIR_SHIFT) | > > ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | > > (ret & ~MLX90614_CONFIG_FIR_MASK) & > > (~MLX90614_CONFIG_IIR_MASK))); > > > > The 0x7 is a magic number. I would think it would be a named _MASK > > macro. Ah, we do have a macro for that. > > That macro is for mask and it is just coincidence that they are the > same. I would like to keep the > way it is in case someone wants to change FIR values as well. Did you read the rest of the email? This code make no sense. We know that it has never been tested since the earlier ret test was inverted so it was unreachable. Why are you defending something that is so clearly wrong? Anyway get rid of the magic number at least. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-02 8:32 ` Dan Carpenter @ 2015-10-02 8:48 ` Crt Mori 2015-10-02 8:56 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Crt Mori @ 2015-10-02 8:48 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio On 2 October 2015 at 10:32, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Fri, Oct 02, 2015 at 10:12:01AM +0200, Crt Mori wrote: >> > It looks a little more readable without any casts and it works exactly >> > the same since the casts were just for decoration. >> > >> >> Like I said they are on most processors, but might not be on others. > > And monkeys *might* fly out of my butt. > >> I rather have it defined than let compiler decide > > Focus on writing simple code and not making it portable for time > travellers to 1980. > >> >> > ret = mlx90614_write_word(client, MLX90614_CONFIG, >> > (i << MLX90614_CONFIG_IIR_SHIFT) | >> > ((0x7 << MLX90614_CONFIG_FIR_SHIFT) | >> > (ret & ~MLX90614_CONFIG_FIR_MASK) & >> > (~MLX90614_CONFIG_IIR_MASK))); >> > >> > The 0x7 is a magic number. I would think it would be a named _MASK >> > macro. Ah, we do have a macro for that. >> >> That macro is for mask and it is just coincidence that they are the >> same. I would like to keep the >> way it is in case someone wants to change FIR values as well. > > Did you read the rest of the email? This code make no sense. We know > that it has never been tested since the earlier ret test was inverted so > it was unreachable. Why are you defending something that is so clearly > wrong? Anyway get rid of the magic number at least. > Actually that part of code was extensively tested on BeagleBone Black with sensor attached - the ret value was artifact of reviewing process. I also wrote unit tests for that function to be certain it works as intended. Before this I had: if( ret >=0) { /* Write changed values */ } but it was decided that it goes too deep so I changed it to if (ret > 0) return; /* Write changed values */ which was wrong. I actually still have the first version in my local branch, so all other tests in last month passed on that old if statement. > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-02 8:48 ` Crt Mori @ 2015-10-02 8:56 ` Dan Carpenter 2015-10-02 9:04 ` Crt Mori 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2015-10-02 8:56 UTC (permalink / raw) To: Crt Mori; +Cc: linux-iio Ok. Hm... Then what are the possible positive values of: ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG) regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: iio: mlx90614: Implement filter configuration 2015-10-02 8:56 ` Dan Carpenter @ 2015-10-02 9:04 ` Crt Mori 0 siblings, 0 replies; 8+ messages in thread From: Crt Mori @ 2015-10-02 9:04 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio On 2 October 2015 at 10:56, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Ok. Hm... Then what are the possible positive values of: > > ret = i2c_smbus_read_word_data(client, MLX90614_CONFIG) > 16 bit Config register value, which has some bits fixed to maintain calibration (14...11;7;3). > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-02 9:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-01 22:16 iio: mlx90614: Implement filter configuration Dan Carpenter 2015-10-01 23:09 ` Crt Mori 2015-10-02 7:00 ` Dan Carpenter 2015-10-02 8:12 ` Crt Mori 2015-10-02 8:32 ` Dan Carpenter 2015-10-02 8:48 ` Crt Mori 2015-10-02 8:56 ` Dan Carpenter 2015-10-02 9:04 ` Crt Mori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).