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@xxxxxxxxx> 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! ~~
