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,
>> + >s->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! ~~
