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

[Matti Vaittinen]

On 30/11/2024 20:01, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 18:51:00 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
> 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