| Message ID | 20220420203320.3035329-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests: alsa: Start validating control names | expand |
On Wed, 20 Apr 2022 22:33:20 +0200, Mark Brown wrote: > > +bool strend(const char *haystack, const char *needle) Missing static? > +{ > + size_t haystack_len = strlen(haystack); > + size_t needle_len = strlen(needle); > + > + if (needle_len > haystack_len) > + return false; > + return strcmp(haystack + haystack_len - needle_len, needle) == 0; > +} > + > +static void test_ctl_name(struct ctl_data *ctl) > +{ > + bool name_ok = true; > + bool check; > + > + /* Only boolean controls should end in Switch */ > + if (strend(ctl->name, "Switch")) { This should be with " Switch" so that it won't check a concatenated word. > + if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) { > + ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n", > + ctl->card->card, ctl->elem, ctl->name); > + name_ok = false; > + } > + } > + > + /* Writeable boolean controls should end in Switch */ > + if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN && > + snd_ctl_elem_info_is_writable(ctl->info)) { > + if (!strend(ctl->name, "Switch")) { > + ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n", > + ctl->card->card, ctl->elem, ctl->name); > + name_ok = false; I'm afraid that this would hit too many when applying to the existing code; although the control name should be indeed with Switch suffix, we tend to allow without suffix for casual non-standard elements. But having the check would help for avoiding such a mistake for the future code, so it's fine to add this strict check, IMO. thanks, Takashi
On Thu, Apr 21, 2022 at 09:50:33AM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > + if (!strend(ctl->name, "Switch")) { > > + ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n", > > + ctl->card->card, ctl->elem, ctl->name); > > + name_ok = false; > I'm afraid that this would hit too many when applying to the existing > code; although the control name should be indeed with Switch suffix, > we tend to allow without suffix for casual non-standard elements. It wasn't looking too bad in my survey of cards I had to hand - the writable check is there because of jacks, which does make sense since you can't actually switch them. Some of that is due to ASoC handling this in the core though. > But having the check would help for avoiding such a mistake for the > future code, so it's fine to add this strict check, IMO. Yeah, that's more the target here.
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index eb2213540fe3..b747dbc023ab 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -27,7 +27,7 @@ #include "../kselftest.h" -#define TESTS_PER_CONTROL 6 +#define TESTS_PER_CONTROL 7 struct card_data { snd_ctl_t *handle; @@ -456,6 +456,44 @@ static void test_ctl_get_value(struct ctl_data *ctl) ctl->card->card, ctl->elem); } +bool strend(const char *haystack, const char *needle) +{ + size_t haystack_len = strlen(haystack); + size_t needle_len = strlen(needle); + + if (needle_len > haystack_len) + return false; + return strcmp(haystack + haystack_len - needle_len, needle) == 0; +} + +static void test_ctl_name(struct ctl_data *ctl) +{ + bool name_ok = true; + bool check; + + /* Only boolean controls should end in Switch */ + if (strend(ctl->name, "Switch")) { + if (snd_ctl_elem_info_get_type(ctl->info) != SND_CTL_ELEM_TYPE_BOOLEAN) { + ksft_print_msg("%d.%d %s ends in Switch but is not boolean\n", + ctl->card->card, ctl->elem, ctl->name); + name_ok = false; + } + } + + /* Writeable boolean controls should end in Switch */ + if (snd_ctl_elem_info_get_type(ctl->info) == SND_CTL_ELEM_TYPE_BOOLEAN && + snd_ctl_elem_info_is_writable(ctl->info)) { + if (!strend(ctl->name, "Switch")) { + ksft_print_msg("%d.%d %s is a writeable boolean but not a Switch\n", + ctl->card->card, ctl->elem, ctl->name); + name_ok = false; + } + } + + ksft_test_result(name_ok, "name.%d.%d\n", + ctl->card->card, ctl->elem); +} + static bool show_mismatch(struct ctl_data *ctl, int index, snd_ctl_elem_value_t *read_val, snd_ctl_elem_value_t *expected_val) @@ -1062,6 +1100,7 @@ int main(void) * test stores the default value for later cleanup. */ test_ctl_get_value(ctl); + test_ctl_name(ctl); test_ctl_write_default(ctl); test_ctl_write_valid(ctl); test_ctl_write_invalid(ctl);
Not much of a test but we keep on getting problems with boolean controls not being called Switches so let's add a few basic checks to help people spot problems. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/alsa/mixer-test.c | 41 ++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)