Message-ID: <569A42C4.7010508@alexanderkoch.net>
Date:	Sat, 16 Jan 2016 14:16:52 +0100
From:	Alexander Koch <mail@...xanderkoch.net>
To:	Jonathan Cameron <jic23@...nel.org>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc:	knaack.h@....de, lars@...afoo.de, mhornung.linux@...il.com,
	dannenberg@...com, balbi@...com, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: light: opt3001: enable operation w/o IRQ

Am 16.01.2016 um 13:53 schrieb Jonathan Cameron:
> 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.

Okay then, so will send a v2 of the patch that includes this bool
change, shortly.

While I'm at it, maybe I should include a second refactoring commit that
changes the other bitfield members of the opt3001-struct that are used
as bool as well - namely 'ok_to_ignore_lock' and 'result_ready'. I hope
this is okay.


Best regards

Alex

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

Powered by blists - more mailing lists