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

[Bryan O'Donoghue]

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