Re: [PATCH v4 4/8] iio: light: Add gain-time-scale helpers

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 3/20/23 14:01, Matti Vaittinen wrote:
> On 3/19/23 20:08, Jonathan Cameron wrote:
>> On Fri, 17 Mar 2023 16:43:23 +0200
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> I think a bit more care is need with storage of time (unsigned) + decide
>> whether to allow for negative gains.
> 
> My approach was just pretty simple "int is big enough for the times" 
> (2000+ seconds when using usec as time units felt like more than enough 
> for light sensors) and "gains are always positive".
> 
> I have not tested the negative gains at all - but I agree this should've 
> been documented. Currently there is no gts-helper users who need 
> negative gain (or large times for that matter) - so I was not handling 
> them.
> 
> I'll try to check what it would mean code-wise if we converted times to 
> unsigned. Negative times make no sense but allowing negative error 
> values is a simple way to go.
> 
> As for the negative gains - I have no problem of someone adding a 
> support for those if needed, but I don't currently see much point in 
> investing time in that...
> 
>> Whilst they happen I'm not that bothered
>> if that subtlety becomes a device driver problem when calling this.  
>> I'm not
>> sure I've seen a sensor that does both positive and negative gains for 
>> a single
>> channel.
> 
> I agree. If driver needs negative gains, then the driver needs to deal 
> with it. I have no objections if driver authors want to improve these 
> helpers by adding support for negative gains, but if they don't, then 
> they have the exactly same problem they would have without these helpers :)

Back at this. I started reworking things to use unsigned times / gains 
but I am not really happy about how it starts to look like. Using the 
int values but reserving negative values to denote errors keeps things 
cleaner. Also, I don't think we need the extra bit for extending the 
range of supported values - It's hard for me to think we would really 
need gains or times exceeding the maximum signed int. I think negative 
gains are actually more likely so keeping int as type may help one who 
wants to add support for negative gains.

(Although, I assume the integration time multiplying logic with negative 
gains would not work in a same way as with positive gains - so 
supporting negative gains would probably require more than that, or work 
only as a dummy selector <=> gain converter without the time tables).

So, the v5 will likely still use int as type for times and gain but also 
have a check in initialization enforcing this. I will also document this 
restriction in the gain/time struct and init function documentation.

I don't think the v5 is final version, especially because it will be the 
first version looping in the Kunit people. So we can keep iterating this 
for v6 if you still feel using ints is unacceptable :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~