| Message ID | 20170504214200.GA22855@embeddedgus (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/2017 11:42 PM, Gustavo A. R. Silva wrote: > 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 <sylvester.nawrocki@gmail.com> > Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com> > --- > 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(-) > > diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c > index 812fb3a..d70ffef 100644 > --- a/drivers/media/platform/s3c-camif/camif-regs.c > +++ b/drivers/media/platform/s3c-camif/camif-regs.c > @@ -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. Regards, Hans > diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h > index 5ad36c1..dfb49a5 100644 > --- a/drivers/media/platform/s3c-camif/camif-regs.h > +++ b/drivers/media/platform/s3c-camif/camif-regs.h > @@ -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); >
On 05/22/2017 11:02 AM, Hans Verkuil wrote: >> --- a/drivers/media/platform/s3c-camif/camif-regs.c >> +++ b/drivers/media/platform/s3c-camif/camif-regs.c >> @@ -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. The function is currently called in a wrong way, it's clear from looking at the prototype. CR should end up in bits 0:7 and CR in bits 20:13 of the register. So yes, colour will change after applying the patch - to the expected one, matching the user API documentation. Unfortunately I can't test it because I have only the s3c2440 SoC based evaluation board where the image effect is not supported. Probably a more straightforward fix would be to amend the callers (apologies Gustavo for misleading suggestions). But I'm inclined to apply the $subject patch as is to just close this bug report case. -- Regards, Sylwester
diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c index 812fb3a..d70ffef 100644 --- a/drivers/media/platform/s3c-camif/camif-regs.c +++ b/drivers/media/platform/s3c-camif/camif-regs.c @@ -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 }, diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h index 5ad36c1..dfb49a5 100644 --- a/drivers/media/platform/s3c-camif/camif-regs.h +++ b/drivers/media/platform/s3c-camif/camif-regs.h @@ -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);
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 <sylvester.nawrocki@gmail.com> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com> --- 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(-)