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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <569A3D06.80701@kernel.org>
Date:	Sat, 16 Jan 2016 12:52:22 +0000
From:	Jonathan Cameron <jic23@...nel.org>
To:	Alexander Koch <mail@...xanderkoch.net>,
	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

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.
> 
> 
	> Best regards
> 
> lynix
>