Re: [PATCH 2/3] iio: test: test gain-time-scale helpers

[Jonathan Cameron]

On Mon, 15 Jan 2024 15:01:32 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> On 1/13/24 18:12, Jonathan Cameron wrote:
> > On Wed, 10 Jan 2024 12:12:55 +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>
> >>  
> 
> ...
> 
> >> +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?  
> 
> Doing:
> const int exp_len = ARRAY_SIZE(gains) * 2;
> int expected[exp_len];
> 
> gives me:
> warning: ISO C90 forbids variable length array ‘expected’ [-Wvla]

Huh.  That's a compiler being impressively stupid :(

Just leave it as it is - maybe add a comment to so no one tries to tidy this
up in future.

> 
> I could drop the whole exp_len variable, but I prefer test code which is 
> as obvious as it gets if any of the checks fails. For me the check:
> 
> >> +	KUNIT_EXPECT_EQ(test, exp_len, len);
> >> +	if (len != exp_len)
> >> +		return;  
> 
> is (very slightly) more obvious than:
> 	KUNIT_EXPECT_EQ(test, ARRAY_SIZE(gains) * 2, len);
> 	if (len != ARRAY_SIZE(gains) * 2)
> 		return;
> 
> I guess I'll leave this one as it is. Just kick me in v2 if I 
> misunderstood you :)

yeah, leave it be. Annoying but such is life.

> 
> Yours,
> 	-- Matti
>