On 2 October 2015 at 10:32, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
