Messages in this thread | | | Date | Mon, 13 Mar 2023 14:52:45 +0200 | Subject | Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers | From | Matti Vaittinen <> |
| |
On 3/12/23 19:06, Jonathan Cameron wrote: > On Mon, 6 Mar 2023 11:17:15 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > > 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 :) >
This is more than welcome! I tried to add some test cases for verifying some parts - but extra pair of eyes is always more than appreciated! I've written and read way too many bugs to not appreciate a healthy amount of paranoia :)
>> +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.
Seems like I accidentally did not use the EXPORT_SYMBOL_GPL for this one. It must thus have evaded my conversion to name space one. Thanks!
> > >> 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 >
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |