linux-kernel - Re: [RFC PATCH 2/6] iio: light: Add gain-time-scale helpers

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: <914a94ac-934b-d268-0ddf-b00681fe42f8@fi.rohmeurope.com>
Date:   Tue, 28 Feb 2023 11:13:20 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Liam Beguin <liambeguin@...il.com>,
        Peter Rosin <peda@...ntia.se>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/6] iio: light: Add gain-time-scale helpers

Hi Jonathan,

I started fixing the comments. I think I have just one thing to discuss ;)

On 2/26/23 18:21, Jonathan Cameron wrote:
> On Wed, 22 Feb 2023 18:14:45 +0200
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> +
>> +static int iio_gts_get_gain(const u64 max, u64 scale)
> 
> trivial but scale equally const in here.

Yes. For this function that's true. I'll update the signature. Still, 
the reason why max is marked const is that the max should remain const 
in general, throughout the lifetime of the "helper object". It is 
fundamentally const value where as the gain, scale and integration time 
may all change over the course.

> Not immediately obvious what this function does from name, so add
> some docs.

I added doc (but not kernel doc) as I hope it helps the readers - but 
would like to point out that this is meant to be internal only function.

>> +int iio_init_iio_gts(int max_scale_int, int max_scale_nano,
>> +		     const struct iio_gain_sel_pair *gain_tbl, int num_gain,
>> +		     const struct iio_itime_sel_mul *tim_tbl, int num_times,
>> +		     struct iio_gts *gts)
>> +{
>> +	int ret;
>> +
>> +	ret = iio_gts_linearize(max_scale_int, max_scale_nano,
>> +				   &gts->max_scale, NANO);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gts->hwgain_table = gain_tbl;
>> +	gts->num_hwgain = num_gain;
>> +	gts->itime_table = tim_tbl;
>> +	gts->num_itime = num_times;
> 
> I think all these need to be provided for this to do anything useful.

I was also pondering this. I actually think I at some point had a TODO 
comment somewhere about deciding if the integration time table(s) should 
be required.

Well, during my 'trial and error' change for bu27034 discussed in the 
other thread (attempting to shift the data to hide the scale impact of 
integration time) I still ended up using these helpers for the gain. I 
liked the ability of providing the gain table without re-inventing the 
table structs and initialization macros in driver. Additionally I still 
used the selector <=> gain conversions. Finally I did add a scale <=> 
"total_gain" helpers - which allowed me to do the get/set scale 
functions in a simple way.

I still ended up having also the integration time tables for the very 
same reason - but just set the multiplication factor to 1 for all times 
(because data shifting was supposed to hide the scale impact until I 
finally understood what you meant with the saturation detection 
problem... <imagine a facepalm emoji here>).

> So check them here and drop the checks in the various other functions.
The rehearsal described above made me to think that (some of) these 
helpers can be useful also when there are only gain tables provided.

Yours,
	-- Matti

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

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