Re: [PATCH 1/2] iio: gts-helper: add helpers to ease searches of gain_sel and new_gain

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 21 Dec 2024 20:19:21 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 20/12/2024 21:28, Javier Carrasco wrote:
> > This helper functions reduce the burden in the drivers that want to
> > fetch a gain selector in all available times or a new optimal gain.
> > 
> > The former is currently achieved by calling
> > iio_gts_find_gain_sel_for_scale_using_time() for the current time
> > selector, and then iterating over the rest of time selectors if the
> > gain selector was not found.
> > 
> > The latter requires a combination of multiple iio-gts helpers to find
> > the new gain, look for an optimal gain if there was no exact match, and
> > set a minimum gain if the optimal gain is not in the range of available
> > gains.
> > 
> > Provide simpler workflows by means of functions that address common
> > patterns in the users of the iio-gts helpers.
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> 
> Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
A couple of comments inline.

Thanks,

Jonathan

> > ---
> >   drivers/iio/industrialio-gts-helper.c | 74 +++++++++++++++++++++++++++++++++++
> >   include/linux/iio/iio-gts-helper.h    |  5 +++
> >   2 files changed, 79 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> > index 3b5a99815062..f88b0b7192dd 100644
> > --- a/drivers/iio/industrialio-gts-helper.c
> > +++ b/drivers/iio/industrialio-gts-helper.c
> > @@ -915,6 +915,38 @@ int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel
> >   }
> >   EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_for_scale_using_time, "IIO_GTS_HELPER");
> >   
> > +/**
> > + * iio_gts_find_gain_sel_in_times - Fetch gain selector in the available times.
> > + * @gts:	Gain time scale descriptor
> > + * @scale_int:	Integral part of the scale (typically val1)
> > + * @scale_nano:	Fractional part of the scale (nano or ppb)
> > + * @gain_sel:	Pointer to value where gain selector is stored.
> > + * @time_sel:	Pointer to value where time selector is stored.
> > + *
> > + * Wrapper around iio_gts_find_gain_for_scale_using_time() to fetch the
> > + * gain selector for all supported integration times.
> > + *
> > + * Return: 0 on success and -EINVAL on error.
> > + */
> > +int iio_gts_find_gain_sel_in_times(struct iio_gts *gts, int scale_int,
> > +				   int scale_nano, int *gain_sel, int *time_sel)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < gts->num_itime; i++) {
> > +		*time_sel = gts->itime_table[i].sel;
> > +		ret = iio_gts_find_gain_sel_for_scale_using_time(gts, *time_sel,
> > +								 scale_int,
> > +								 scale_nano,
> > +								 gain_sel);
> > +		if (!ret)
> > +			return 0;
Trivial but slight preference for keeping the error path out of line rather than
the good path.

Easily done with:

		if (ret)
			continue;

		return 0;

> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_gts_find_gain_sel_in_times, "IIO_GTS_HELPER");
> > +
> >   static int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> >   {
> >   	const struct iio_itime_sel_mul *itime;
> > @@ -1086,6 +1118,48 @@ int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
> >   }
> >   EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_old_gain_time, "IIO_GTS_HELPER");
> >   
> > +/**
> > + * iio_gts_find_new_gain_by_gain_time_min - compensate for time change
> > + * @gts:	Gain time scale descriptor
> > + * @old_gain:	Previously set gain
> > + * @old_time:	Selector corresponding previously set time
> > + * @new_time:	Selector corresponding new time to be set
> > + * @new_gain:	Pointer to value where new gain is to be written
> > + * @in_range:	Indicate if the @new_gain was in the range of
> > + *		supported gains.
> > + *
> > + * Wrapper around iio_gts_find_new_gain_by_old_gain_time() that tries to
> > + * set an optimal value if no exact match was found, defaulting to the
> > + * minimum gain to avoid saturations if the optimal value is not in the
> > + * range of supported gains.
> > + *
> > + * Return: 0 on success and a negative value if no gain was found.
> > + */
> > +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> > +					   int old_time, int new_time,
> > +					   int *new_gain, bool *in_range)
> > +{
> > +	int ret;
> > +
> > +	*in_range = true;
> > +	ret = iio_gts_find_new_gain_by_old_gain_time(gts, old_gain, old_time,
> > +						     new_time, new_gain);
> > +	if (*new_gain < 0)
> > +		return -EINVAL;
> > +
> > +	if (ret) {
> > +		*new_gain = iio_find_closest_gain_low(gts, *new_gain, in_range);
> > +		if (*new_gain < 0) {
> > +			*new_gain = iio_gts_get_min_gain(gts);
> > +			if (*new_gain < 0)
> > +				return ret;

The return value here is possibly misleading as it comes from the original
iio_gts_find_new_gain_by_old_gain_time() call.
Sure it may have the same value, but I'd rather it was set explicitly here
so it was clear that we have a case where all fallbacks failed.


> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_gts_find_new_gain_by_gain_time_min, "IIO_GTS_HELPER");
> > +
> >   MODULE_LICENSE("GPL");
> >   MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
> >   MODULE_DESCRIPTION("IIO light sensor gain-time-scale helpers");
> > diff --git a/include/linux/iio/iio-gts-helper.h b/include/linux/iio/iio-gts-helper.h
> > index 9cb6c80dea71..ae91ad008cc8 100644
> > --- a/include/linux/iio/iio-gts-helper.h
> > +++ b/include/linux/iio/iio-gts-helper.h
> > @@ -188,6 +188,8 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
> >   int iio_gts_find_gain_sel_for_scale_using_time(struct iio_gts *gts, int time_sel,
> >   					       int scale_int, int scale_nano,
> >   					       int *gain_sel);
> > +int iio_gts_find_gain_sel_in_times(struct iio_gts *gts, int scale_int,
> > +				   int scale_nano, int *gain_sel, int *time_sel);
> >   int iio_gts_get_scale(struct iio_gts *gts, int gain, int time, int *scale_int,
> >   		      int *scale_nano);
> >   int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> > @@ -196,6 +198,9 @@ int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> >   int iio_gts_find_new_gain_by_old_gain_time(struct iio_gts *gts, int old_gain,
> >   					   int old_time, int new_time,
> >   					   int *new_gain);
> > +int iio_gts_find_new_gain_by_gain_time_min(struct iio_gts *gts, int old_gain,
> > +					   int old_time, int new_time,
> > +					   int *new_gain, bool *in_range);
> >   int iio_gts_avail_times(struct iio_gts *gts,  const int **vals, int *type,
> >   			int *length);
> >   int iio_gts_all_avail_scales(struct iio_gts *gts, const int **vals, int *type,
> >   
>