Re: [PATCH 2/2] iio: light: opt3001: enable operation w/o IRQ

[Jonathan Cameron <jic23@kernel.org>]

On 16/01/16 12:52, Jonathan Cameron wrote:
> On 12/01/16 20:17, Alexander Koch wrote:
>> Am 12.01.2016 um 20:27 schrieb Peter Meerwald-Stadler:
>>> On Tue, 12 Jan 2016, Alexander Koch wrote:
>>>
>>>> Enable operation of the TI OPT3001 light sensor without having an
>>>> interrupt line available to connect the INT pin to.
>>>>
>>>> In this operation mode, we issue a conversion request and simply wait
>>>> for the conversion time available as timeout value, determined from
>>>> integration time configuration and the worst-case time given in the data
>>>> sheet (sect. 6.5, table on p. 5):
>>>>
>>>>   short integration time (100ms): 110ms + 3ms = 113ms
>>>>    long integration time (800ms): 880ms + 3ms = 883ms
>>>>
>>>> This change is transparent as behaviour defaults to using the interrupt
>>>> method if an interrupt no. is configured via device tree. Interrupt-less
>>>> operation mode is performed when no valid interrupt no. is given.
>>>
>>> looks good, I'd rather use a bool for use_irq and the msecs_to_jiffies() 
>>> call moved from the #define to the code (which is not strictly necessary 
>>> for the patch) -- matter of taste
>>
>> Thanks - actually this is my first patch, so positive feedback much
>> appreciated!
>>
>> Concerning the bool for 'use_irq': I first had it that way but then
>> opted for the bit field of length 1 as I wasn't sure whether bool would
>> get optimized to the same level by the compiler.
> Bit fields are often less efficient as the compiler has to separate them out
> using shifts and masks.  Also from a space point of view the data structure
> will be considerably padded anyway for a couple of reasons:
> 1) It contains u32 fields so will at least be padded to a multiple of u32.
> 2) Memory allocations may well be a good bit larger depending on exact
> sizes vs the blob levels available in the memory allocator.
> 
> Basic rule of thumb - keep things simple and let the compiler do the work.
> So a bool is suitable here.
> 
>>
>> I'm a bit irritated by your comment concerning the msecs_to_jiffies()
>> call, as my patch indeed moves this call from the #define to the code.
>> Did you mean it the other way round, then?
> Presumably ;) 
>> My reason to move it was that I need to work with microseconds for the
>> IRQ-less operation mode, and jiffies are only required in one place for
>> the IRQ mode.
> Now perhaps the 'right' way to do this would be have been a precursor patch
> removing the define rather than lumping what is an an connected change (in
> many ways) in here.  Overall I agree the change is worthwhile and trivial.
> As Peter said, it's a matter of taste!  We both happen to disagree with him
> on this point.

ps. Should have said that other than the bit field vs bool change, the patch
looks good to me.

Jonathan
>>
>>
> 	> Best regards
>>
>> lynix
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>