| Message ID | 20230823104444.1954663-9-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand |
On 23.08.2023 12:44, Bryan O'Donoghue wrote: > I refuse to add another SoC to this convoluted if/else structure. By the > time we added in a third SoC we ought to have transitioned these control > flow strutures to switches. > > Adding in another Soc or two will make some of these if statements into > five clause behemoths. > > Introduce switches in the obvious places to despaghetiify. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
Hi Bryan, Thank you for the patch. On Wed, Aug 23, 2023 at 11:44:37AM +0100, Bryan O'Donoghue wrote: > I refuse to add another SoC to this convoluted if/else structure. By the > time we added in a third SoC we ought to have transitioned these control > flow strutures to switches. > > Adding in another Soc or two will make some of these if statements into > five clause behemoths. > > Introduce switches in the obvious places to despaghetiify. I like the switch/case better as well, but the current code isn't as bad as you make it sound. I don't think you need to make such a personal statement in the commit message and threaten of starting a hunger strike if the driver doesn't use switch/case :-) Just say that the switch/case reads more naturally for this specific usage. > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../media/platform/qcom/camss/camss-csid.c | 16 ++++--- > .../media/platform/qcom/camss/camss-csiphy.c | 22 ++++++--- > drivers/media/platform/qcom/camss/camss-vfe.c | 45 +++++++++++++------ > .../media/platform/qcom/camss/camss-video.c | 16 ++++--- > 4 files changed, 68 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index fd04ed112b564..5dbbcda5232ac 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ 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 ? > } > csid->ops->subdev_init(csid); > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 593aec5c97bc2..0e8c2a59ea241 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -557,21 +557,31 @@ int msm_csiphy_subdev_init(struct camss *camss, > csiphy->id = id; > csiphy->cfg.combo_mode = 0; > > - if (camss->res->version == CAMSS_8x16) { > + switch (camss->res->version) { > + case CAMSS_8x16: > + { > csiphy->ops = &csiphy_ops_2ph_1_0; > csiphy->formats = csiphy_formats_8x16; > csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16); > - } else if (camss->res->version == CAMSS_8x96 || > - camss->res->version == CAMSS_660) { > + break; > + } > + case CAMSS_8x96: > + case CAMSS_660: > + { > csiphy->ops = &csiphy_ops_3ph_1_0; > csiphy->formats = csiphy_formats_8x96; > csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96); > - } else if (camss->res->version == CAMSS_845 || > - camss->res->version == CAMSS_8250) { > + break; > + } > + case CAMSS_845: > + case CAMSS_8250: > + { > csiphy->ops = &csiphy_ops_3ph_1_0; > csiphy->formats = csiphy_formats_sdm845; > csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845); > - } else { > + break; > + } > + default: > return -EINVAL; > } > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c > index b789b3b2e4cfd..8f48401e31cd3 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c > @@ -170,7 +170,9 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, > { > struct vfe_device *vfe = to_vfe(line); > > - if (vfe->camss->res->version == CAMSS_8x16) > + switch (vfe->camss->res->version) { > + case CAMSS_8x16: > + { > switch (sink_code) { > case MEDIA_BUS_FMT_YUYV8_1X16: > { > @@ -218,10 +220,13 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, > > return sink_code; > } > - else if (vfe->camss->res->version == CAMSS_8x96 || > - vfe->camss->res->version == CAMSS_660 || > - vfe->camss->res->version == CAMSS_845 || > - vfe->camss->res->version == CAMSS_8250) > + break; > + } > + case CAMSS_8x96: > + case CAMSS_660: > + case CAMSS_845: > + case CAMSS_8250: > + { > switch (sink_code) { > case MEDIA_BUS_FMT_YUYV8_1X16: > { > @@ -281,8 +286,12 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, > > return sink_code; > } > - else > - return 0; > + break; > + } > + default: > + break; > + } > + return 0; > } > > int vfe_reset(struct vfe_device *vfe) > @@ -1397,7 +1406,9 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, > init_completion(&l->output.sof); > init_completion(&l->output.reg_update); > > - if (camss->res->version == CAMSS_8x16) { > + switch (camss->res->version) { > + case CAMSS_8x16: > + { > if (i == VFE_LINE_PIX) { > l->formats = formats_pix_8x16; > l->nformats = ARRAY_SIZE(formats_pix_8x16); > @@ -1405,8 +1416,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, > l->formats = formats_rdi_8x16; > l->nformats = ARRAY_SIZE(formats_rdi_8x16); > } > - } else if (camss->res->version == CAMSS_8x96 || > - camss->res->version == CAMSS_660) { > + break; > + } > + case CAMSS_8x96: > + case CAMSS_660: > + { > if (i == VFE_LINE_PIX) { > l->formats = formats_pix_8x96; > l->nformats = ARRAY_SIZE(formats_pix_8x96); > @@ -1414,11 +1428,16 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, > l->formats = formats_rdi_8x96; > l->nformats = ARRAY_SIZE(formats_rdi_8x96); > } > - } else if (camss->res->version == CAMSS_845 || > - camss->res->version == CAMSS_8250) { > + break; > + } > + case CAMSS_845: > + case CAMSS_8250: > + { > l->formats = formats_rdi_845; > l->nformats = ARRAY_SIZE(formats_rdi_845); > - } else { > + break; > + } > + default: > return -EINVAL; > } > } > diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c > index 46a89b5f6c171..e695724437ce1 100644 > --- a/drivers/media/platform/qcom/camss/camss-video.c > +++ b/drivers/media/platform/qcom/camss/camss-video.c > @@ -1006,7 +1006,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, > > mutex_init(&video->lock); > > - if (video->camss->res->version == CAMSS_8x16) { > + switch (video->camss->res->version) { > + case CAMSS_8x16: > if (is_pix) { > video->formats = formats_pix_8x16; > video->nformats = ARRAY_SIZE(formats_pix_8x16); > @@ -1014,8 +1015,9 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, > video->formats = formats_rdi_8x16; > video->nformats = ARRAY_SIZE(formats_rdi_8x16); > } > - } else if (video->camss->res->version == CAMSS_8x96 || > - video->camss->res->version == CAMSS_660) { > + break; > + case CAMSS_8x96: > + case CAMSS_660: > if (is_pix) { > video->formats = formats_pix_8x96; > video->nformats = ARRAY_SIZE(formats_pix_8x96); > @@ -1023,11 +1025,13 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, > video->formats = formats_rdi_8x96; > video->nformats = ARRAY_SIZE(formats_rdi_8x96); > } > - } else if (video->camss->res->version == CAMSS_845 || > - video->camss->res->version == CAMSS_8250) { > + break; > + case CAMSS_845: > + case CAMSS_8250: > video->formats = formats_rdi_845; > video->nformats = ARRAY_SIZE(formats_rdi_845); > - } else { > + break; > + default: > ret = -EINVAL; > goto error_video_register; > }
On 28/08/2023 19:51, Laurent Pinchart wrote: >> + 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 ? > Off the top of my head, I don't think it could be easily caught. An assert() would catch it early in runtime.. --- bod
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
diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c index fd04ed112b564..5dbbcda5232ac 100644 --- a/drivers/media/platform/qcom/camss/camss-csid.c +++ 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; } csid->ops->subdev_init(csid); diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c index 593aec5c97bc2..0e8c2a59ea241 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c @@ -557,21 +557,31 @@ int msm_csiphy_subdev_init(struct camss *camss, csiphy->id = id; csiphy->cfg.combo_mode = 0; - if (camss->res->version == CAMSS_8x16) { + switch (camss->res->version) { + case CAMSS_8x16: + { csiphy->ops = &csiphy_ops_2ph_1_0; csiphy->formats = csiphy_formats_8x16; csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16); - } else if (camss->res->version == CAMSS_8x96 || - camss->res->version == CAMSS_660) { + break; + } + case CAMSS_8x96: + case CAMSS_660: + { csiphy->ops = &csiphy_ops_3ph_1_0; csiphy->formats = csiphy_formats_8x96; csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96); - } else if (camss->res->version == CAMSS_845 || - camss->res->version == CAMSS_8250) { + break; + } + case CAMSS_845: + case CAMSS_8250: + { csiphy->ops = &csiphy_ops_3ph_1_0; csiphy->formats = csiphy_formats_sdm845; csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845); - } else { + break; + } + default: return -EINVAL; } diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c index b789b3b2e4cfd..8f48401e31cd3 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe.c +++ b/drivers/media/platform/qcom/camss/camss-vfe.c @@ -170,7 +170,9 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, { struct vfe_device *vfe = to_vfe(line); - if (vfe->camss->res->version == CAMSS_8x16) + switch (vfe->camss->res->version) { + case CAMSS_8x16: + { switch (sink_code) { case MEDIA_BUS_FMT_YUYV8_1X16: { @@ -218,10 +220,13 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, return sink_code; } - else if (vfe->camss->res->version == CAMSS_8x96 || - vfe->camss->res->version == CAMSS_660 || - vfe->camss->res->version == CAMSS_845 || - vfe->camss->res->version == CAMSS_8250) + break; + } + case CAMSS_8x96: + case CAMSS_660: + case CAMSS_845: + case CAMSS_8250: + { switch (sink_code) { case MEDIA_BUS_FMT_YUYV8_1X16: { @@ -281,8 +286,12 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code, return sink_code; } - else - return 0; + break; + } + default: + break; + } + return 0; } int vfe_reset(struct vfe_device *vfe) @@ -1397,7 +1406,9 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, init_completion(&l->output.sof); init_completion(&l->output.reg_update); - if (camss->res->version == CAMSS_8x16) { + switch (camss->res->version) { + case CAMSS_8x16: + { if (i == VFE_LINE_PIX) { l->formats = formats_pix_8x16; l->nformats = ARRAY_SIZE(formats_pix_8x16); @@ -1405,8 +1416,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, l->formats = formats_rdi_8x16; l->nformats = ARRAY_SIZE(formats_rdi_8x16); } - } else if (camss->res->version == CAMSS_8x96 || - camss->res->version == CAMSS_660) { + break; + } + case CAMSS_8x96: + case CAMSS_660: + { if (i == VFE_LINE_PIX) { l->formats = formats_pix_8x96; l->nformats = ARRAY_SIZE(formats_pix_8x96); @@ -1414,11 +1428,16 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, l->formats = formats_rdi_8x96; l->nformats = ARRAY_SIZE(formats_rdi_8x96); } - } else if (camss->res->version == CAMSS_845 || - camss->res->version == CAMSS_8250) { + break; + } + case CAMSS_845: + case CAMSS_8250: + { l->formats = formats_rdi_845; l->nformats = ARRAY_SIZE(formats_rdi_845); - } else { + break; + } + default: return -EINVAL; } } diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c index 46a89b5f6c171..e695724437ce1 100644 --- a/drivers/media/platform/qcom/camss/camss-video.c +++ b/drivers/media/platform/qcom/camss/camss-video.c @@ -1006,7 +1006,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, mutex_init(&video->lock); - if (video->camss->res->version == CAMSS_8x16) { + switch (video->camss->res->version) { + case CAMSS_8x16: if (is_pix) { video->formats = formats_pix_8x16; video->nformats = ARRAY_SIZE(formats_pix_8x16); @@ -1014,8 +1015,9 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, video->formats = formats_rdi_8x16; video->nformats = ARRAY_SIZE(formats_rdi_8x16); } - } else if (video->camss->res->version == CAMSS_8x96 || - video->camss->res->version == CAMSS_660) { + break; + case CAMSS_8x96: + case CAMSS_660: if (is_pix) { video->formats = formats_pix_8x96; video->nformats = ARRAY_SIZE(formats_pix_8x96); @@ -1023,11 +1025,13 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, video->formats = formats_rdi_8x96; video->nformats = ARRAY_SIZE(formats_rdi_8x96); } - } else if (video->camss->res->version == CAMSS_845 || - video->camss->res->version == CAMSS_8250) { + break; + case CAMSS_845: + case CAMSS_8250: video->formats = formats_rdi_845; video->nformats = ARRAY_SIZE(formats_rdi_845); - } else { + break; + default: ret = -EINVAL; goto error_video_register; }
I refuse to add another SoC to this convoluted if/else structure. By the time we added in a third SoC we ought to have transitioned these control flow strutures to switches. Adding in another Soc or two will make some of these if statements into five clause behemoths. Introduce switches in the obvious places to despaghetiify. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../media/platform/qcom/camss/camss-csid.c | 16 ++++--- .../media/platform/qcom/camss/camss-csiphy.c | 22 ++++++--- drivers/media/platform/qcom/camss/camss-vfe.c | 45 +++++++++++++------ .../media/platform/qcom/camss/camss-video.c | 16 ++++--- 4 files changed, 68 insertions(+), 31 deletions(-)