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

[Vaittinen, Matti]

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