Received-SPF: pass (google.com: domain of linux-kernel+bounces-25330-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sat, 13 Jan 2024 16:12:29 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>, Lars-Peter Clausen
 <lars@metafoo.de>, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH 2/3] iio: test: test gain-time-scale helpers
Message-ID: <20240113161229.48d63b54@jic23-huawei>
In-Reply-To: <6b839dd533fd93b75c2e6f6a8f2286233d4901fb.1704881096.git.mazziesaccount@gmail.com>
References: <cover.1704881096.git.mazziesaccount@gmail.com>
	<6b839dd533fd93b75c2e6f6a8f2286233d4901fb.1704881096.git.mazziesaccount@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 10 Jan 2024 12:12:55 +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.
> Hence some gain-time-scale helpers were introduced.
> 
> Add some simple tests to verify the most hairy functions.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
Hi Matti,

All seems reasonable to me. Some trivial formatting things inline
+ I'm not planning to check the maths as you are the expert in all of
this so I'll just trust you!

Also if this fails you get to pick up the pieces :)

Jonathan

>  CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/iio/test/iio-test-gts.c b/drivers/iio/test/iio-test-gts.c
> new file mode 100644
> index 000000000000..4d5271b0c7bc
> --- /dev/null
> +++ b/drivers/iio/test/iio-test-gts.c
> @@ -0,0 +1,517 @@
..

> +
> +static void test_iio_find_closest_gain_low(struct kunit *test)
> +{
> +	struct device *dev;
> +	bool in_range;
> +	int ret;
> +
> +	const struct iio_gain_sel_pair gts_test_gains_gain_low[] = {
> +		GAIN_SCALE_GAIN(4, TEST_GSEL_4),
> +		GAIN_SCALE_GAIN(16, TEST_GSEL_16),
> +		GAIN_SCALE_GAIN(32, TEST_GSEL_32),
> +	};
> +
> +	dev = test_init_iio_gain_scale(test, &gts);
> +	if (!dev)
> +		return;
> +
> +	ret = iio_find_closest_gain_low(&gts, 2, &in_range);
> +	KUNIT_EXPECT_EQ(test, 1, ret);
> +	KUNIT_EXPECT_EQ(test, true, in_range);
> +
> +	ret = iio_find_closest_gain_low(&gts, 1, &in_range);
> +	KUNIT_EXPECT_EQ(test, 1, ret);
> +	KUNIT_EXPECT_EQ(test, true, in_range);
> +
> +	ret = iio_find_closest_gain_low(&gts, 4095, &in_range);
> +	KUNIT_EXPECT_EQ(test, 2048, ret);
> +	KUNIT_EXPECT_EQ(test, true, in_range);
> +
> +	ret = iio_find_closest_gain_low(&gts, 4097, &in_range);
> +	KUNIT_EXPECT_EQ(test, 4096, ret);
> +	KUNIT_EXPECT_EQ(test, false, in_range);
> +
> +	kunit_device_unregister(test, dev);
> +
> +	dev = __test_init_iio_gain_scale(test, &gts, gts_test_gains_gain_low,
> +				ARRAY_SIZE(gts_test_gains_gain_low),
> +				gts_test_itimes, ARRAY_SIZE(gts_test_itimes));
> +	if (!dev)
> +		return;
> +
> +	ret = iio_find_closest_gain_low(&gts, 3, &in_range);
> +	KUNIT_EXPECT_EQ(test, -EINVAL, ret);
> +	KUNIT_EXPECT_EQ(test, false, in_range);
> +
As below.

> +}
> +
> +static void test_iio_gts_total_gain_to_scale(struct kunit *test)
> +{
> +	struct device *dev;
> +	int ret, scale_int, scale_nano;
> +
> +	dev = test_init_iio_gain_scale(test, &gts);
> +	if (!dev)
> +		return;
> +
> +	ret = iio_gts_total_gain_to_scale(&gts, 1, &scale_int, &scale_nano);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
> +	KUNIT_EXPECT_EQ(test, 0, scale_nano);
> +
> +	ret = iio_gts_total_gain_to_scale(&gts, 1, &scale_int, &scale_nano);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	KUNIT_EXPECT_EQ(test, TEST_SCALE_1X, scale_int);
> +	KUNIT_EXPECT_EQ(test, 0, scale_nano);
> +
> +	ret = iio_gts_total_gain_to_scale(&gts, 4096 * 8, &scale_int,
> +					  &scale_nano);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	KUNIT_EXPECT_EQ(test, 0, scale_int);
> +	KUNIT_EXPECT_EQ(test, TEST_SCALE_NANO_4096X8, scale_nano);
> +
No need for a blank line here.

> +}
> +

> +static void test_iio_gts_chk_scales_all(struct kunit *test, struct iio_gts *gts,
> +					const int *vals, int len)
> +{
> +	static const int gains[] = {1, 2, 4, 8, 16, 32, 64, 128, 256, 512,
> +				    1024, 2048, 4096, 4096 * 2, 4096 * 4,
> +				    4096 * 8};
> +
> +	int expected[ARRAY_SIZE(gains) * 2];
> +	int i, ret;
> +	int exp_len = ARRAY_SIZE(gains) * 2;

Use this for expected[*] just above?

> +
> +	KUNIT_EXPECT_EQ(test, exp_len, len);
> +	if (len != exp_len)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(gains); i++) {
> +		ret = iio_gts_total_gain_to_scale(gts, gains[i],
> +						  &expected[2 * i],
> +						  &expected[2 * i + 1]);
> +		KUNIT_EXPECT_EQ(test, 0, ret);
> +		if (ret)
> +			return;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(expected); i++)
> +		KUNIT_EXPECT_EQ(test, expected[i], vals[i]);
> +}

> +
> +static void test_iio_gts_avail_test(struct kunit *test)
> +{
> +	struct device *dev;
> +	int ret;
> +	int type, len;
> +	const int *vals;
> +
> +	dev = test_init_iio_gain_scale(test, &gts);
> +	if (!dev)
> +		return;
> +
> +	/* test table building for times and iio_gts_avail_times() */
> +	ret = iio_gts_avail_times(&gts, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		return;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_MICRO, type);
> +	KUNIT_EXPECT_EQ(test, 8, len);
> +	if (len < 8)
> +		return;
> +
> +	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(&gts, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		return;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> +
> +	test_iio_gts_chk_scales_all(test, &gts, vals, len);
> +
> +	/*
> +	 * Test table building for scales/time and
> +	 * iio_gts_avail_scales_for_time()
> +	 */
> +	ret = iio_gts_avail_scales_for_time(&gts, 200000, &vals, &type, &len);
> +	KUNIT_EXPECT_EQ(test, IIO_AVAIL_LIST, ret);
> +	if (ret)
> +		return;
> +
> +	KUNIT_EXPECT_EQ(test, IIO_VAL_INT_PLUS_NANO, type);
> +	test_iio_gts_chk_scales_t200(test, &gts, vals, len);
> +}
> +
> +static struct kunit_case iio_gts_test_cases[] = {
> +	KUNIT_CASE(test_init_iio_gts_invalid),
> +	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@gmail.com>");
> +MODULE_DESCRIPTION("Test IIO light sensor gain-time-scale helpers");
> +MODULE_IMPORT_NS(IIO_GTS_HELPER);
> +
Looks like a bonus blank line at the end here.

Jonathan