Re: [PATCH 3/3] iio: gts: simplify scale table build

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 30/11/2024 20:01, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 18:51:00 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The GTS helpers offer two different set of "available scales" -tables.
>> Drivers can choose to advertice the scales which are available on a
>> currently selected integration time (by just changing the hwgain).
>> Another option is to list all scales which can be supported using any of
>> the integration times. This is useful for drivers which allow scale
>> setting to also change the integration time to meet the scale user
>> prefers.
>>
>> The helper function which build these tables for the GTS did firstbuild
> The helper function which builds these tables for the GTS first builds the "time specific" ..
> 
>> the "time specific" scale arrays for all the times. This is done by
>> calculating the scales based on the integration time specific "total
>> gain" arrays (gain contributed by both the integration time and hw-gain).
>>
>> After this the helper code calculates an array for all available scales.
>> This is done combining all the time specific total-gains into one sorted
>> array, removing dublicate gains and finally converting the gains to
>> scales as above.
>>
>> This can be somewhat simplified by changing the logic for calculating
>> the 'all available scales' -array to directly use the time specific
>> scale arrays instead of time specific total-gain arrays. Code can
>> directly just add all the already computed time specific scales to one
>> big 'all scales'-array, keep it sorted and remove duplicates.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
> Minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>
>> This has been tested by IIO-gts kunit tests only. All testing is
>> appreciated.
>>
>> Comparing the scales is not as pretty as comparing the gains was, as
>> scales are in two ints where the gains were in one. This makes the code
>> slightly more hairy. I however believe that the logic is now more
>> obvious. This might be more important for one reading this later...
>> ---
>>   drivers/iio/industrialio-gts-helper.c | 109 ++++++++++----------------
>>   1 file changed, 42 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>> index 7f900f578f1d..31101848b194 100644
>> --- a/drivers/iio/industrialio-gts-helper.c
>> +++ b/drivers/iio/industrialio-gts-helper.c
>> @@ -191,86 +191,61 @@ static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **sca
>>   	return 0;
>>   }
>>   
>> -static int combine_gain_tables(struct iio_gts *gts, int **gains,
>> -			       int *all_gains, size_t gain_bytes)
>> +static int scale_eq(int *sc1, int *sc2)
>>   {
>> -	int i, new_idx, time_idx;
>> +	return *sc1 == *sc2 && *(sc1 + 1) == *(sc2 + 1);
> 	return sc1[0] == sc2[0] && sc1[1] == sc2[1];
> 
> Would be easier to read in my opinion.

I agree. (As with the other (ptr + 1) cases you commented)

>> +}
>>   
>> -	/*
>> -	 * We assume all the gains for same integration time were unique.
>> -	 * It is likely the first time table had greatest time multiplier as
>> -	 * the times are in the order of preference and greater times are
>> -	 * usually preferred. Hence we start from the last table which is likely
>> -	 * to have the smallest total gains.
>> -	 */
>> -	time_idx = gts->num_itime - 1;
>> -	memcpy(all_gains, gains[time_idx], gain_bytes);
>> -	new_idx = gts->num_hwgain;
>> +static int scale_smaller(int *sc1, int *sc2)
>> +{
>> +	if (*sc1 != *sc2)
>> +		return *sc1 < *sc2;
>> +
>> +	/* If integer parts are equal, fixp parts */
>> +	return *(sc1 + 1) < *(sc2 + 1);
>> +}
>> +
>> +static int do_combined_scaletable(struct iio_gts *gts, int **scales, size_t scale_bytes)
>> +{
>> +	int t_idx, i, new_idx;
>> +	int *all_scales = kcalloc(gts->num_itime, scale_bytes, GFP_KERNEL);
>>   
>> -	while (time_idx-- > 0) {
>> -		for (i = 0; i < gts->num_hwgain; i++) {
>> -			int candidate = gains[time_idx][i];
>> +	if (!all_scales)
>> +		return -ENOMEM;
>> +
>> +	t_idx = gts->num_itime - 1;
>> +	memcpy(all_scales, scales[t_idx], scale_bytes);
>> +	new_idx = gts->num_hwgain * 2;
>> +
>> +	while (t_idx-- > 0) {
> maybe a reverse for loop is clearer
> 
> 	for (tidx = t_idx; tidx; tidx--)
> For me a for loop indicates bounds are known and we change the index
> one per loop.

I could've said that :)

> While loop indicates either unknown bounds, or that we are
> modifying the index other than than in the loop controls.

I'm not entirely sure why I've used while here, could be a result of 
some review discussion.

I'll fix these for the next version.

Yours,
	-- Matti