diff mbox series

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

Message ID 20230823104444.1954663-9-bryan.odonoghue@linaro.org (mailing list archive)
State Superseded
Headers show
Series media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand

Commit Message

Bryan O'Donoghue Aug. 23, 2023, 10:44 a.m. UTC
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(-)

Comments

Konrad Dybcio Aug. 26, 2023, 10:03 a.m. UTC | #1
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
Laurent Pinchart Aug. 28, 2023, 6:51 p.m. UTC | #2
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;
>  	}
Bryan O'Donoghue Aug. 28, 2023, 7:43 p.m. UTC | #3
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
Bryan O'Donoghue Sept. 4, 2023, 12:24 p.m. UTC | #4
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 mbox series

Patch

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;
 	}