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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 3/6/23 14:52, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
>> Some light sensors can adjust both the HW-gain and integration time.
>> There are cases where adjusting the integration time has similar impact
>> to the scale of the reported values as gain setting has.
>>
>> IIO users do typically expect to handle scale by a single writable 'scale'
>> entry. Driver should then adjust the gain/time accordingly.
>>
>> It however is difficult for a driver to know whether it should change
>> gain or integration time to meet the requested scale. Usually it is
>> preferred to have longer integration time which usually improves
>> accuracy, but there may be use-cases where long measurement times can be
>> an issue. Thus it can be preferable to allow also changing the
>> integration time - but mitigate the scale impact by also changing the gain
>> underneath. Eg, if integration time change doubles the measured values,
>> the driver can reduce the HW-gain to half.
>>
>> The theory of the computations of gain-time-scale is simple. However,
>> some people (undersigned) got that implemented wrong for more than once.
>>
>> Add some gain-time-scale helpers in order to not dublicate errors in all
>> drivers needing these computations.
> 
> ...
> 
>> +/*
> 
> If it's deliberately not a kernel doc, why to bother to have it looking as one?
> It's really a provocative to some people who will come with a patches to "fix"
> this...

I just liked the kernel-doc format. It's a standard way of explaining 
the parameters and returned value. Function however is intended to be 
internal and thus I don't see a need to make this "official kernel doc".

> 
>> + * iio_gts_get_gain - Convert scale to total gain
>> + *
>> + * Internal helper for converting scale to total gain.
>> + *
>> + * @max:	Maximum linearized scale. As an example, when scale is created
>> + *		in magnitude of NANOs and max scale is 64.1 - The linearized
>> + *		scale is 64 100 000 000.
>> + * @scale:	Linearized scale to compte the gain for.
>> + *
>> + * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
>> + *		is invalid.
>> + */
>> +static int iio_gts_get_gain(const u64 max, const u64 scale)
>> +{
>> +	int tmp = 1;
>> +
>> +	if (scale > max || !scale)
>> +		return -EINVAL;
>> +
>> +	if (U64_MAX - max < scale) {
>> +		/* Risk of overflow */
>> +		if (max - scale < scale)
>> +			return 1;
> 
>> +		while (max - scale > scale * (u64)tmp)
>> +			tmp++;
>> +
>> +		return tmp + 1;
> 
> Can you wait for the comments to appear a bit longer, please?
> I have answered to your query in the previous discussion.
> 

Yep. Sorry for that. I just wanted to get the v3 out before I left the 
computer for a week to allow potential reviewers to check the quite a 
bit reworked series.

>> +	}
>> +
>> +	while (max > scale * (u64) tmp)
> 
> No space for castings?

Thanks,

> 
>> +		tmp++;
>> +
>> +	return tmp;
>> +}
> 
> ...
> 
>> +	/*
>> +	 * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
>> +	 * multiplication followed by division to avoid overflow
> 
> Missing period.
> 
>> +	 */
>> +	if (scaler > NANO || !scaler)
>> +		return -EINVAL;
> 
> Shouldn't be OVERFLOW for the first one?

-EOVERFLOW? I guess it could be. Thanks.

> ...
> 
>> +	*lin_scale = (u64) scale_whole * (u64)scaler +
> 
> No space for casting?

Yes. Thanks

> ...
> 
>> +	ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
>> +				   &gts->max_scale);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gts->hwgain_table = gain_tbl;
>> +	gts->num_hwgain = num_gain;
>> +	gts->itime_table = tim_tbl;
>> +	gts->num_itime = num_times;
>> +	gts->per_time_avail_scale_tables = NULL;
>> +	gts->avail_time_tables = NULL;
>> +	gts->avail_all_scales_table = NULL;
>> +	gts->num_avail_all_scales = 0;
> 
> Just wondering why we can't simply
> 
> 	memset(0)
> 
> beforehand and drop all these 0 assignments?
> 

I guess we can :)

> ...
> 
>> +		/*
>> +		 * Sort the tables for nice output and for easier finding of
>> +		 * unique values
> 
> Missing period. Please, check the style of multi-line comments. I believe it's
> even mentioned in the documentation.
> 
>> +		 */
> 
> ...
> 
>> +		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
>> +		     NULL);
> 
> One line reads better?

I try mostly to keep the good old 80 chars as I often have 3 terminal 
windows fitted on my laptop screen. It works best with the short lines.

> 
> ...
> 
>> +	if (ret && gts->avail_all_scales_table)
> 
> In one case you commented that free(NULL) is okay, in the other, you add
> a duplicative check. Why?

Sorry but what do you mean by dublicative check?

Usually I avoid the kfree(NULL). That's why I commented on it in that 
another case where it was not explicitly disallowed. I'll change that 
for v4 to avoid kfree(NULL) as you suggested.

> 
>> +		kfree(gts->avail_all_scales_table);
> 
> ...
> 
>> +	per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> 
> sizeof(type) is error prone in comparison to sizeof(*var).

Yes and no. In majority of cases where we see sizeof(*var) - the *var is 
no longer a pointer as having pointers to pointers is not _that_ common. 
When we see sizeof(type *) - we instantly know it is a size of a pointer 
and not a size of some other type.

So yes, while having sizeof(*var) makes us tolerant to errors caused by 
variable type changes - it makes us prone to human reader errors. Also, 
if someone changes type of *var from pointer to some other type - then 
he/she is likely to in any case need to revise the array alloactions too.

While I in general agree with you that the sizeof(variable) is better 
than sizeof(type) - I see that in cases like this the sizeof(type *) is 
clearer.

> 
>> +	if (!per_time_gains)
>> +		return ret;
>> +
>> +	per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> 
> Ditto.
> 
>> +	if (!per_time_scales)
>> +		goto free_gains;
> 
> ...
> 
>> +err_free_out:
>> +	while (i) {
>> +		/*
>> +		 * It does not matter if i'th alloc was not succesfull as
>> +		 * kfree(NULL) is safe.
>> +		 */
> 
> Instead, can be just a free of the known allocated i:th member first followed
> by traditional pattern. In that case comment will become redundant.
>

I replied to this in your comments regarding the v2. Sorry for splitting 
the discussion by sending v3 so soon.


>> +		kfree(per_time_scales[i]);
>> +		kfree(per_time_gains[i]);
>> +
>> +		i--;
>> +	}
> 
> ...
> 
>> +	for (i = gts->num_itime - 1; i >= 0; i--) {
> 
> 	while (i--) {
> 
> makes it easier to parse.

This is also something I replied for v2. I think we have a fundamental 
disagreement on this one :/

> 
>> +/**
>> + * iio_gts_all_avail_scales - helper for listing all available scales
>> + * @gts:	Gain time scale descriptor
>> + * @vals:	Returned array of supported scales
>> + * @type:	Type of returned scale values
>> + * @length:	Amount of returned values in array
>> + *
>> + * Returns a value suitable to be returned from read_avail or a negative error
> 
> Missing a return section. Have you run kernel doc to validate this?
No. I think I have never run the kernel doc - probably time to do so :) 
Thanks.

> Missing period.
> 
> Seems these problems occur in many function descriptions.
> 
>> + */
> 
> ...
> 
>> +	/*
>> +	 * Using this function prior building the tables is a driver-error
>> +	 * which should be fixed when the driver is tested for a first time
> 
> Missing period.
> 
>> +	 */
>> +	if (WARN_ON(!gts->num_avail_all_scales))
> 
> Does this justify panic? Note, that any WARN() can become an Oops followed by
> panic and reboot.

No. My initial thinking was that this could only happen if a driver 
which do not build the tables tries to use the helpers. That, in my 
books, would have been 100% reproducible driver error, happening when 
ever the available scales were read.

I did overlook the case where freeing of tables is done in wrong order. 
That type of bug could well escape the initial testing and no - it 
should not bring down the machine. I'll drop the WARN. Thanks.

> 
>> +		return -EINVAL;
> 
> ...
> 
>> +	for (i = 0; i < gts->num_hwgain; i++) {
>> +		/*
>> +		 * It is not expected this function is called for an exactly
>> +		 * matching gain.
>> +		 */
>> +		if (unlikely(gain == gts->hwgain_table[i].gain)) {
>> +			*in_range = true;
>> +			return gain;
>> +		}
> 
>> +		if (!min)
>> +			min = gts->hwgain_table[i].gain;
>> +		else
>> +			min = min(min, gts->hwgain_table[i].gain);
> 
> I was staring at this and have got no clue why it's not a dead code.

Nor can I. It seems obvious to me that the one who wrote this had no 
idea what he was doing XD

Well, I must have had some initial idea of using the minimum value to 
something - but I can't remember what would've been the use. Maybe I was 
initially thinking that I'll return the smallest value in-range if the 
gain given as a parameter was smaller than any of the supported ones.

Thank you for reading this carefully and pointing it out! Well spotted!

> 
>> +		if (gain > gts->hwgain_table[i].gain) {
>> +			if (!diff) {
>> +				diff = gain - gts->hwgain_table[i].gain;
>> +				best = i;
>> +			} else {
>> +				int tmp = gain - gts->hwgain_table[i].gain;
>> +
>> +				if (tmp < diff) {
>> +					diff = tmp;
>> +					best = i;
>> +				}
>> +			}
> 
> 			int tmp = gain - gts->hwgain_table[i].gain;
> 
> 			if (!diff || tmp < diff) {
> 				diff = tmp;
> 				best = i;
> 			}
> 
> ?
> 
> Or did you miss using 'min'? (And I'm wondering how variable name and min()
> macro are not conflicting with each other.
> 
>> +		} else {
>> +			/*
>> +			 * We found valid hwgain which is greater than
>> +			 * reference. So, unless we return a failure below we
>> +			 * will have found an in-range gain
>> +			 */
>> +			*in_range = true;
>> +		}
>> +	}
>> +	/* The requested gain was smaller than anything we support */
>> +	if (!diff) {
>> +		*in_range = false;
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	return gts->hwgain_table[best].gain;
> 
> ...
> 
>> +	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
>> +				       &scale);
> 
> Still can be one line.
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
>> +				      new_gain);
> 
> Ditto.

I still prefer the 80-chars when it does not mean some terribly awkward 
split - or really many rows of arguments.


>> +	if (ret)
>> +		return -EINVAL;
> 
> ...
> 
>> +++ b/drivers/iio/light/iio-gts-helper.h
> 
> Is it _only_ for a Light type of sensors?

This is a very good question. I was asking this from myself but as I 
don't know better I just decided to put it under the light where I'll 
have the use-cases. We can move it to drivers/iio if there will be users 
outside the light sensors.

> 
> ...
> 
>> +#ifndef __IIO_GTS_HELPER__
>> +#define __IIO_GTS_HELPER__
> 
> If yes, perhaps adding LIGHT here?

I'd like to keep this matching the file name, especially when I don't 
know for sure this can't be used elsewhere.

Yours,
	-- Matti

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

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