mbox series

[v1,0/2] kselftest: alsa: Small enhancements to mixer-test

Message ID 20211217130213.3893415-1-broonie@kernel.org
Headers show
Series kselftest: alsa: Small enhancements to mixer-test | expand

Message

Mark Brown Dec. 17, 2021, 1:02 p.m. UTC 
These two patches improve the mixer test, checking that the default
values for enums are valid and extending them to cover all the values
for multi-value controls, not just the first one.  It also factors out
the validation that values are OK for controls for future reuse.

Mark Brown (2):
  kselftest: alsa: Factor out check that values meet constraints
  kselftest: alsa: Validate values read from enumerations

 tools/testing/selftests/alsa/mixer-test.c | 158 ++++++++++++++--------
 1 file changed, 99 insertions(+), 59 deletions(-)


base-commit: b73dad806533cad55df41a9c0349969b56d4ff7f

Comments

Cezary Rojewski Dec. 17, 2021, 1:32 p.m. UTC | #1 
On 2021-12-17 2:02 PM, Mark Brown wrote:
> To simplify the code a bit and allow future reuse factor the checks that
> values we read are valid out of test_ctl_get_value() into a separate
> function which can be reused later. As part of this extend the test to
> check all the values for the control, not just the first one.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

...

> +/*
> + * Check that the provided value meets the constraints for the
> + * provided control.
> + */
> +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val)
> +{
> +	int i;
> +	bool valid = true;
> +
> +	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
> +		if (!ctl_value_index_valid(ctl, val, i))
> +			valid = false;

Correct me I'm wrong, but it seems a 'return false' would suffice. Is 
the continuation of looping still needed once a single check found above 
evaluates to true?


Regards,
Czarek

> +
> +	return valid;
> +}
Cezary Rojewski Dec. 17, 2021, 2:54 p.m. UTC | #2 
On 2021-12-17 3:38 PM, Mark Brown wrote:
> On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote:
>> On 2021-12-17 2:02 PM, Mark Brown wrote:
> 
>>> +	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
>>> +		if (!ctl_value_index_valid(ctl, val, i))
>>> +			valid = false;
> 
>> Correct me I'm wrong, but it seems a 'return false' would suffice. Is the
>> continuation of looping still needed once a single check found above
>> evaluates to true?

Just read my initial message and clearly an 'if' : (. Sorry for the 
confusion.

> It doesn't affect the result of the test but it will cause us to print a
> diagnostic message for each invalid value rather than just the first one
> we see (eg, if both channels in a stereo control have an invalid value)
> which seems like it's more helpful to people working with the output
> than just the first error.

So there is a good reason to do so. And I agree with such approach. 
Thanks for explaining, Mark!


Regards,
Czarek
Takashi Iwai Dec. 25, 2021, 8:13 a.m. UTC | #3 
On Fri, 17 Dec 2021 14:02:11 +0100,
Mark Brown wrote:
> 
> These two patches improve the mixer test, checking that the default
> values for enums are valid and extending them to cover all the values
> for multi-value controls, not just the first one.  It also factors out
> the validation that values are OK for controls for future reuse.
> 
> Mark Brown (2):
>   kselftest: alsa: Factor out check that values meet constraints
>   kselftest: alsa: Validate values read from enumerations

Applied both patches now.  Thanks.


Takashi