Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

Bryan O'Donoghue
2023-09-04T12:24:21Z

On 28/08/2023 19:51, Laurent Pinchart wrote:
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -592,15 +592,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>> csid->camss = camss;
>> csid->id = id;
>>
>> - if (camss->res->version == CAMSS_8x16) {
>> + switch (camss->res->version) {
>> + case CAMSS_8x16:
>> csid->ops = &csid_ops_4_1;
>> - } else if (camss->res->version == CAMSS_8x96 ||
>> - camss->res->version == CAMSS_660) {
>> + break;
>> + case CAMSS_8x96:
>> + case CAMSS_660:
>> csid->ops = &csid_ops_4_7;
>> - } else if (camss->res->version == CAMSS_845 ||
>> - camss->res->version == CAMSS_8250) {
>> + break;
>> + case CAMSS_845:
>> + case CAMSS_8250:
>> csid->ops = &csid_ops_gen2;
>> - } else {
>> + break;
>> + default:
>> return -EINVAL;
> This should never happen, as adding support for a new SoC should come
> with an update for all the applicable switch/case statements. It's
> useful to let the compiler complain if someone forgets to do so, but
> with a default case, you will only see the issue at runtime. Could it be
> caught at compile time ?
>

This can be done in fact.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch_002denum-303

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

typedef enum {
MO = 0,
LARRY,
CURLY,
BINGO,
}my_type;

int main (int argc, char *argv[])
{
my_type x;
time_t t;

srand((unsigned) time(&t));

x = rand() % BINGO;

switch(x) {
case MO:
printf("mo\n");
break;
case LARRY:
printf("larry\n");
break;
default:
printf("blargh\n");
break;

}

return 0;
}

gcc -o test test.c -Wswitch-enum
test.c: In function ‘main’:
test.c:38:9: warning: enumeration value ‘CURLY’ not handled in switch
[-Wswitch-enum]
38 | switch(x) {
| ^~~~~~

It looks like we only enable that switch for tools though

grep -r "Wswitch-enum" *
tools/scripts/Makefile.include:EXTRA_WARNINGS += -Wswitch-enum
tools/bpf/bpftool/Makefile:CFLAGS += $(filter-out -Wswitch-enum
-Wnested-externs,$(EXTRA_WARNINGS))

I'll still implement the code though, since if we do introduce the
switch for the kernel it would be caught.

---
bod