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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux