On Fri, 17 Mar 2023 16:43:49 +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.
> Hence some gain-time-scale helpers were introduced.
>
> Add some simple tests to verify the most hairy functions.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
A few comments inline.
As mentioned in previous patch I wonder if you should get rid
of the separation between init and build_tables code. That would require
always building tables in here even when you don't use them, but that may
be a sensible choice as drivers would never expect to not build the tables
anyway (I think).
Jonathan
> diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
> index 0b6e4e278a2f..4d5cfb9fe60b 100644
> --- a/drivers/iio/test/Kconfig
> +++ b/drivers/iio/test/Kconfig
> @@ -4,6 +4,20 @@
> #
>
> # Keep in alphabetical order
> +config IIO_GTS_KUNIT_TEST
> + tristate "Test IIO formatting functions" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + select IIO_GTS_HELPER
> + select TEST_KUNIT_DEVICE_HELPERS
> + default KUNIT_ALL_TESTS
> + help
> + build unit tests for the IIO light sensor gain-time-scale helpers.
> +
> + For more information on KUnit and unit tests in general, please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N. Keep in alphabetical order
> +
> config IIO_RESCALE_KUNIT_TEST
> tristate "Test IIO rescale conversion functions" if !KUNIT_ALL_TESTS
> depends on KUNIT && IIO_RESCALE
> @@ -27,3 +41,5 @@ config IIO_FORMAT_KUNIT_TEST
> to the KUnit documentation in Documentation/dev-tools/kunit/.
>
> If unsure, say N.
> +
> +
Several stray blank lines that shouldn't be in here.
> diff --git a/drivers/iio/test/iio-test-gts.c b/drivers/iio/test/iio-test-gts.c
> new file mode 100644
> index 000000000000..ff9311acd0bb
> --- /dev/null
> +++ b/drivers/iio/test/iio-test-gts.c
> @@ -0,0 +1,461 @@
...
> +static int test_init_iio_gain_scale(struct iio_gts *gts, int max_scale_int,
> + int max_scale_nano)
> +{
> + int ret;
> +
> + ret = iio_init_iio_gts(max_scale_int, max_scale_nano, gts_test_gains,
> + ARRAY_SIZE(gts_test_gains), gts_test_itimes,
> + ARRAY_SIZE(gts_test_itimes), gts);
return iio_init...
Or get rid of this wrapper entirely.
> +
> + return ret;
> +}
> +
> +
> +static void test_iio_gts_avail_test(struct kunit *test)
> +{
> + struct iio_gts gts;
> + int ret;
> + int type, len;
> + const int *vals;
> + struct device *dev;
> +
> + ret = test_init_iio_gain_scale(>s, TEST_SCALE_1X, 0);
I don't follow why the wrapper is useful here. Why not just call it
directly and have the arrays passed in nice and obvious here.
> + KUNIT_EXPECT_EQ(test, 0, ret);
> + if (ret)
> + return;
> +
> + dev = test_kunit_helper_alloc_device(test);
> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
> + if (!dev)
> + return;
> +
> + ret = devm_iio_gts_build_avail_tables(dev, >s);
> + KUNIT_EXPECT_EQ(test, 0, ret);
> + if (ret)
> + goto drop_testdev;
> +
> + /* test table building for times and iio_gts_avail_times() */
> + ret = iio_gts_avail_times(>s, &vals, &type, &len);
> + KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> + if (ret)
> + goto drop_testdev;
> +
> + KUNIT_EXPECT_EQ(test, IIO_VAL_INT, type);
> + KUNIT_EXPECT_EQ(test, 4, len);
> + if (len < 4)
> + goto drop_testdev;
> +
> + test_iio_gts_chk_times(test, vals);
> +
> + /* Test table building for all scales and iio_gts_all_avail_scales() */
> + ret = iio_gts_all_avail_scales(>s, &vals, &type, &len);
> + KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> + if (ret)
> + goto drop_testdev;
> +
> + KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> +
> + test_iio_gts_chk_scales_all(test, >s, vals, len);
> +
> + /*
> + * Test table building for scales/time and
> + * iio_gts_avail_scales_for_time()
> + */
> + ret = iio_gts_avail_scales_for_time(>s, 200000, &vals, &type, &len);
> + KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> + if (ret)
> + goto drop_testdev;
> +
> + KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> + test_iio_gts_chk_scales_t200(test, >s, vals, len);
> +
> +drop_testdev:
> + test_kunit_helper_free_device(test, dev);
> +}
> +
> +static struct kunit_case iio_gts_test_cases[] = {
> + KUNIT_CASE(test_iio_gts_find_gain_for_scale_using_time),
> + KUNIT_CASE(test_iio_gts_find_new_gain_sel_by_old_gain_time),
> + KUNIT_CASE(test_iio_find_closest_gain_low),
> + KUNIT_CASE(test_iio_gts_total_gain_to_scale),
> + KUNIT_CASE(test_iio_gts_avail_test),
> + {}
> +};
> +
> +static struct kunit_suite iio_gts_test_suite = {
> + .name = "iio-gain-time-scale",
> + .test_cases = iio_gts_test_cases,
> +};
> +
> +kunit_test_suite(iio_gts_test_suite);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
> +MODULE_IMPORT_NS(IIO_GTS_HELPER);
> +
Looks like a stray blank line at end of file.
