Re: [PATCH] iio: test: fixed-point: new kunit test

[Matti Vaittinen]

On 14/10/2025 00:33, David Lechner wrote:
> Add a kunit test for iio_str_to_fixpoint(). This function has an
> unintuitive API so this is helpful to see how to use it and shows the
> various edge cases.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
> When reviewing [1], I noticed that iio_str_to_fixpoint() has an odd API
> which lead me to find the bug in [2]. To make sure I was understanding
> the API correctly, I wrote a KUnit test for it. So here it is.
>
> [1]: https://lore.kernel.org/linux-iio/20251009173609.992452-3-flavra@xxxxxxxxxxxx/
> [2]: https://lore.kernel.org/linux-iio/20251010-iio-adc-ad7280a-fix-ad7280_store_balance_timer-v1-1-e11746735192@xxxxxxxxxxxx/
> ---
> Discussion unrelated to the patch:
>
> I'm also a little tempted to introduce a new function that is a bit
> easier to use. Many callers of iio_str_to_fixpoint_s64() are doing
> something like int_part * 1000 + fract_part and ignoring the possibility
> of negative values which require special handling.
>
> static int iio_str_to_fixpoint_s64(const char *str, u32 decimal_places, s64 *value)
> {
> 	int int_part, fract_part;
> 	int ret;
>
> 	ret = iio_str_to_fixpoint(str, int_pow(10, decimal_places - 1),
> 				  &int_part, &fract_part);
> 	if (ret)
> 		return ret;
>
> 	*value = (s64)int_part * int_pow(10, decimal_places) +
> 		 (int_part < 0 ? -1 : 1) * fract_part;
>
> 	return 0;
> }

FWIW: I like this.

> ---
>   drivers/iio/test/Kconfig                | 13 ++++++++
>   drivers/iio/test/Makefile               |  1 +
>   drivers/iio/test/iio-test-fixed-point.c | 58 +++++++++++++++++++++++++++++++++
>   3 files changed, 72 insertions(+)
>
> diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
> index 6e65e929791ca247df9ac993fddbb4da557d5dfa..d210067ea59199d342b15bf373e724d0aa2c55a0 100644
> --- a/drivers/iio/test/Kconfig
> +++ b/drivers/iio/test/Kconfig
> @@ -4,6 +4,19 @@

// snip

> +static void iio_test_iio_str_to_fixed_point(struct kunit *test)
> +{
> +	int int_part, fract_part;
> +	int ret;
> +
> +	/* Positive value > 1 */
> +	ret = iio_str_to_fixpoint("1.234", 100, &int_part, &fract_part);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +	KUNIT_EXPECT_EQ(test, int_part * 1000 + fract_part, 1234);

Do you think doing two checks:
	KUNIT_EXPECT_EQ(test, int_part, 1);
	KUNIT_EXPECT_EQ(test, fract_part, 234);

instead of the:
	KUNIT_EXPECT_EQ(test, int_part * 1000 + fract_part, 1234);

would work? For me seeing it all ass
	ret = iio_str_to_fixpoint("1.234", 100, &int_part, &fract_part);
	KUNIT_EXPECT_EQ(test, 0, ret);
	KUNIT_EXPECT_EQ(test, int_part, 1);
	KUNIT_EXPECT_EQ(test, fract_part, 234);

would be super clear - albeit also a line longer :)
(Same applies to the rest of the cases).

Well, it's not critical, so with or without that change:

Reviewed-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

Thanks!

Yours,
	-- Matti