On Mon, 6 Mar 2023 11:17:15 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Trying not to duplicate what Andy has raised...
At some stage I want to go through the maths very carefully but it's
not happening today and I don't want to delay resolving other remaining comments
so that can wait for a later version. I'm sure it's fine but I like to be
paranoid :)
> +int iio_gts_get_total_gain(struct iio_gts *gts, int gain, int time)
> +{
> + const struct iio_itime_sel_mul *itime;
> +
> + if (!iio_gts_valid_gain(gts, gain))
> + return -EINVAL;
> +
> + if (!gts->num_itime)
> + return gain;
> +
> + itime = iio_gts_find_itime_by_time(gts, time);
> + if (!itime)
> + return -EINVAL;
> +
> + return gain * itime->mul;
> +}
> +EXPORT_SYMBOL(iio_gts_get_total_gain);
All of them want to be in the namespace.
> diff --git a/drivers/iio/light/iio-gts-helper.h b/drivers/iio/light/iio-gts-helper.h
> new file mode 100644
> index 000000000000..4b5a417946f4
> --- /dev/null
> +++ b/drivers/iio/light/iio-gts-helper.h
...
> +int iio_gts_find_new_gain_sel_by_old_gain_time(struct iio_gts *gts,
> + int old_gain, int old_time_sel,
> + int new_time_sel, int *new_gain);
> +int iio_gts_build_avail_tables(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_tables(struct device *dev, struct iio_gts *gts);
> +int iio_gts_build_avail_scale_table(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_scale_table(struct device *dev, struct iio_gts *gts);
> +int iio_gts_build_avail_time_table(struct iio_gts *gts);
> +int devm_iio_gts_build_avail_time_table(struct device *dev, struct iio_gts *gts);
Given most modern IIO drivers use fully devm_ based probing, for now I would not
expose anything else. That will reduce the interface a lot which I think
is probably a good thing at this stage.
Keep the non devm stuff internally though as it is a nice structure to have
an I can see we may want some of these in non devm form in the future.
Similarly - for now don't expose the individual table building functions
as we may never need them in drivers. We (more or less) only support interfaces
that are used and so far they aren't.
For other functions it's worth thinking about whether to not export them
initially. I haven't been through them all to figure out what is not currently used.
> +void iio_gts_purge_avail_scale_table(struct iio_gts *gts);
> +void iio_gts_purge_avail_time_table(struct iio_gts *gts);
> +void iio_gts_purge_avail_tables(struct iio_gts *gts);
> +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,
> + int *length);
> +int iio_gts_avail_scales_for_time(struct iio_gts *gts, int time,
> + const int **vals, int *type, int *length);
> +
> +#endif
