question about arguments position

The issue here is that the position of arguments in the call to camif_hw_set_effect() function do not match the order of the parameters:

camif->colorfx_cb is passed to cr
camif->colorfx_cr is passed to cb

This is the function prototype:

void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
unsigned int cr, unsigned int cb)

My question here is if this is intentional?

In case it is not, I will send a patch to fix it. But first it would be great to hear any comment about it.

Hi Gustavo,

Quoted 13 lines · Gustavo A. R. Silva

The issue here is that the position of arguments in the call to camif_hw_set_effect() function do not match the order of the parameters:

camif->colorfx_cb is passed to cr
camif->colorfx_cr is passed to cb

This is the function prototype:

void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
unsigned int cr, unsigned int cb)

My question here is if this is intentional?

Quoted 2 lines

In case it is not, I will send a patch to fix it. But first it would be great to hear any comment about it.

You are right, it seems you have found a real bug. Feel free to send a patch. The best thing to do now might be to change the function prototype to:

void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
unsigned int cb, unsigned int cr)

Fix function prototype so the position of arguments camif->colorfx_cb and camif->colorfx_cr match the order of the parameters when calling camif_hw_set_effect() function.

Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141

Cc Sylwester Nawrocki <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/media/platform/s3c-camif/camif-regs.c | 2 +-
 drivers/media/platform/s3c-camif/camif-regs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/s3c-camif/camif-regs.c +0 -0

index 812fb3a..d70ffef 100644

drivers/media/platform/s3c-camif/camif-regs.h +1 -1

index 5ad36c1..dfb49a5 100644
@@ -255,7 +255,7 @@ void camif_hw_set_output_dma(struct camif_vp *vp);
 void camif_hw_set_target_format(struct camif_vp *vp);
 void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
 void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
-			unsigned int cr, unsigned int cb);
+			unsigned int cb, unsigned int cr);
 void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
 			      int index);
 void camif_hw_dump_regs(struct camif_dev *camif, const char *label);

drivers/media/platform/s3c-camif/camif-regs.c void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern) · line 64

Quoted 11 lines

drivers/media/platform/s3c-camif/camif-regs.c +1 -1

@@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
  }
 
  void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
-			unsigned int cr, unsigned int cb)
+			unsigned int cb, unsigned int cr)
  {
  	static const struct v4l2_control colorfx[] = {
  		{ V4L2_COLORFX_NONE,		CIIMGEFF_FIN_BYPASS },

This will also affect this line:

cfg |= cr | (cb << 13);

cr and cb are now swapped so this will result in a different color.

Sylwester, who is wrong here: the prototype or how this function is called?

I suspect that Gustavo is right and that the prototype is wrong. But in that case this patch should also change the cfg assignment.

Hi Sylwester,

Here is another patch in case you decide that it is better to apply this one.

Fix the position of the arguments in function call.

Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

  drivers/media/platform/s3c-camif/camif-capture.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/s3c-camif/camif-capture.c +2 -2

index 1b30be72..25c7a7d 100644
@@ -80,7 +80,7 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
  	camif_hw_set_test_pattern(camif, camif->test_pattern);
  	if (variant->has_img_effect)
  		camif_hw_set_effect(camif, camif->colorfx,
-				camif->colorfx_cb, camif->colorfx_cr);
+				camif->colorfx_cr, camif->colorfx_cb);
  	if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
  		camif_hw_set_input_path(vp);
  	camif_cfg_video_path(vp);

@@ -364,7 +364,7 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
  		camif_hw_set_test_pattern(camif, camif->test_pattern);
  		if (camif->variant->has_img_effect)
  			camif_hw_set_effect(camif, camif->colorfx,
-				    camif->colorfx_cb, camif->colorfx_cr);
+				    camif->colorfx_cr, camif->colorfx_cb);
  		vp->state &= ~ST_VP_CONFIG;
  	}
  unlock:

Quoted 4 lines

Hi Sylwester,

Here is another patch in case you decide that it is better to apply this one.

Thanks, I applied this patch. In future please put any comments only after the scissors ("---") line, the comments can be then discarded automatically and there will be no need for manually editing the patch before applying.

--
Regards,
Sylwester

Quoted 6 lines

Fix the position of the arguments in function call.

Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141

Signed-off-by Gustavo A. R. Silva <[email protected]>

---

^^^^^