[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4cbc8d6a-c8e0-68a0-e1a4-a552c3a968d7@gmail.com>
Date: Wed, 22 Mar 2023 11:10:16 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Lars-Peter Clausen <lars@...afoo.de>,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v4 4/8] iio: light: Add gain-time-scale helpers
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@...il.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! ~~
Powered by blists - more mailing lists