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

[Vaittinen, Matti]

Thanks for the review Andy,

On 3/2/23 17:05, Andy Shevchenko wrote:
> On Thu, Mar 02, 2023 at 12:57:54PM +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.
> 
> ...
> 
>> +/*
> 
> Is it intentionally _not_ a kernel doc?

yes.

>> + * iio_gts_get_gain - Convert scale to total gain
> 
>> + * Internal helper for converting scale to total gain.
> 
> Otherwise this line should go after the fields, I remember kernel doc warnings
> on the similar cases.
> 
>> + * @max:	Maximum linearized scale. As an example, when scale is creted in
> 
> creted?
> 
> IIRC I already pointed out to the very same mistake in your code in the past
> (sorry, if my memory doesn't serve me well).

I Don't think you have previously reviewed this patch.

>> +{
>> +	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)
> 
> Space is not required after casting.
> 
>> +			tmp++;
>> +
>> +		return tmp + 1;
> 
> Wondering why you can't simplify this to
> 
> 		max -= scale;
> 		tmp++;
> 

Sorry, but I don't follow. Can you please show me the complete 
suggestion? Exactly what should be replaced by the:
 > 		max -= scale;
 > 		tmp++;


>> +	}
>> +
>> +	while (max > scale * (u64) tmp)
>> +		tmp++;
>> +
>> +	return tmp;
>> +}

>> +/*
>> + * gain_get_scale_fraction - get the gain or time based on scale and known one
>> + *
>> + * Internal helper for computing unknown fraction of total gain.
>> + * Compute either gain or time based on scale and either the gain or time
>> + * depending on which one is known.
>> + *
>> + * @max:	Maximum linearized scale. As an example, when scale is creted in
> 
> creted?
> 
> Is it mistakenly stored in your spellcheck database? Or is it simply
> copy'n'paste typo?

Typo.

>> + *		magnitude of NANOs and max scale is 64.1 - The linearized
>> + *		scale is 64 100 000 000.
>> + * @scale:	Linearized scale to compute the gain/time for.
>> + * @known:	Either integration time or gain depending on which one is known
>> + * @unknown:	Pointer to variable where the computed gain/time is stored
>> + *
>> + * Return:	0 on success
>> + */
> 
>> +EXPORT_SYMBOL_GPL(iio_init_iio_gts);
> 
> I haven't noticed if you put these all exports into a proper namespace.
> If no, please do.

I haven't. And in many cases I actually see very little to no value in 
doing so as collisions are unlikely when symbols are appropriately prefixed.

Anyways, it seems the IIO uses namespaces so let's play along. Any good 
suggestions for a name?

> ...
> 
>> +	all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int),
> 
> Something from overflow.h is very suitable here.
> 
>> +			    GFP_KERNEL);
>> +	if (!all_gains)
>> +		return -ENOMEM;
> 
> ...
> 
>> +	memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int));
> 
> Maybe it's better to have a temporary which will be calculated as array_size()
> for allocation and reused here? 

Good suggestion, thanks.

> ...
> 
>> +	for (i = gts->num_itime - 2; i >= 0; i--)
> 
> Yeah, if you put this into temporary, like
> 
> 	i = gts->num_itime - 1;
> 
> this becomes
> 
> 	while (i--) {
> 

I prefer for(). It looks clearer to me...

 > Note, you may re-use that i (maybe renamed to something better in the 
memcpy()
 > above as well).

...but this makes sense so Ok.

> Note, you missed {} for better reading.

Didn't miss it but left it out intentionally. {} does not help me as 
indentiation makes it clear. Can add one for you though.

> 
>> +		for (j = 0; j < gts->num_hwgain; j++) {
>> +			int candidate = gains[i][j];
>> +			int chk;
>> +
>> +			if (candidate > all_gains[new_idx - 1]) {
>> +				all_gains[new_idx] = candidate;
>> +				new_idx++;
>> +
>> +				continue;
>> +			}
>> +			for (chk = 0; chk < new_idx; chk++)
>> +				if (candidate <= all_gains[chk])
>> +					break;
>> +
>> +			if (candidate == all_gains[chk])
>> +				continue;
>> +
>> +			memmove(&all_gains[chk + 1], &all_gains[chk],
>> +				(new_idx - chk) * sizeof(int));
>> +			all_gains[chk] = candidate;
>> +			new_idx++;
>> +		}
> 
> ...
> 
>> +	gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
>> +					      2 * sizeof(int), GFP_KERNEL);
>> +	if (!gts->avail_all_scales_table)
>> +		ret = -ENOMEM;
>> +	else
>> +		for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
> 
> Much easier to read if you move this...
> 
>> +			ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
>> +					&gts->avail_all_scales_table[i * 2],
>> +					&gts->avail_all_scales_table[i * 2 + 1]);
> 
> ...here as
> 
> 		if (ret)
> 			break;

I think the !ret in loop condition is obvious. Adding break and brackets 
would not improve this.

> 
>> +	kfree(all_gains);
>> +	if (ret && gts->avail_all_scales_table)
>> +		kfree(gts->avail_all_scales_table);
>> +
>> +	return ret;
> 
> But Wouldn't be better to use goto labels?

I don't see benefit in this case. Handling return value and goto would 
require brackets. The current one is much simpler for me. I am just 
considering dropping that 'else' which is not needed.

> ...
> 
>> +	while (i) {
> 
> Instead of doing standard
> 
> 	while (i--) {
> 
>> +		/*
>> +		 * It does not matter if i'th alloc was not succesfull as
>> +		 * kfree(NULL) is safe.
>> +		 */
> 
> You add this comment, ...
> 
>> +		kfree(per_time_gains[i]);
>> +		kfree(per_time_scales[i]);
> 
> ...an additional loop, ...

The comment is there to explain what I think you missed. We have two 
arrays there. We don't know whether the allocation of first one was 
successful so we try freeing both.

> 
>> +
>> +		i--;
> 
> ...and a line of code.
> 
>> +	}
> 
> Why?

Because, as the comment says, it does not matter if allocation was 
succesfull. kfree(NULL) is safe.

> 
>> +	for (i = gts->num_itime - 1; i >= 0; i--) {
> 
> 	i = gts->num_itime;
> 
> 	while (i--) {

I prefer for() when initializing a variable is needed. Furthermore, 
having var-- or --var in a condition is less clear.

> 
>> +		int new = gts->itime_table[i].time_us;
>> +
>> +		if (times[idx] < new) {
>> +			times[idx++] = new;
>> +			continue;
>> +		}
>> +
>> +		for (j = 0; j <= idx; j++) {
>> +			if (times[j] > new) {
>> +				memmove(&times[j + 1], &times[j], (idx - j) * sizeof(int));
>> +				times[j] = new;
>> +				idx++;
>> +			}
>> +		}
>> +	}
> 
> ...
> 
>> +void iio_gts_purge_avail_time_table(struct iio_gts *gts)
>> +{
>> +	if (gts->num_avail_time_tables) {
> 
> 	if (!...)
> 		return;

Does not improve this as we only have just one small conditional block 
of code which we either execute or not. It is clear at a glance. Would 
make sense if we had longer function.

>> +		kfree(gts->avail_time_tables);
>> +		gts->avail_time_tables = NULL;
>> +		gts->num_avail_time_tables = 0;
>> +	}
>> +}
> 
> ...
> 
>> +			if (!diff) {
> 
> Why not positive conditional?

Because !diff is a special condition and we check explicitly for it.

> 
> 			if (diff) {
> 				...
> 			} else {
> 				...
> 			}
> 
>> +				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;
>> +				}
>> +			}
> 
> ...
> 
>> +	ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);
> 
>> +
> 
> Redundant blank line.

Thanks.

> 
>> +	if (ret || !iio_gts_valid_gain(gts, *gain))
> 
> Why error code is shadowed?

Not sure what I thought of. Could have been because -EINVAL is in any 
case the only error I can think of. But yes, it is better to not 
'shadow' the return value from gain_get_scale_fraction(); So, thanks.

> 
>> +		return -EINVAL;
>> +
> 
> ...
> 
>> +	ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
>> +				       &scale);
> 
> Single line if fine.

I like to (mostly) keep the 80-column limit. Helps me to fit three 
editor windows next to each-others on my laptop screen.

>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
>> +				      new_gain);
> 
> Ditto.
> 
>> +	if (ret)
>> +		return -EINVAL;
> 
> ...
> 
>> +#ifndef __GAIN_TIME_SCALE_HELPER__
>> +#define __GAIN_TIME_SCALE_HELPER__
> 
> __IIO_... ?

Well spotted. Thanks. I renamed the file and forgot the old guardian.

By the way, I will be online only occasionally during the next week. So 
please don't think I ignore reviews if you don't hear of me.

Best Regards,
	-- Matti

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

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