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
Hi Sylwester,
Here is another patch in case you decide that it is
better to apply this one.
Thanks
--
Gustavo A. R. Silva
==========
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(-)
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 1b30be72..25c7a7d 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -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:
--
2.5.0
On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
> 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
> 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(-)
>
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
> index 1b30be72..25c7a7d 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -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:
Hi Sylwester,
Quoting Sylwester Nawrocki <[email protected]>:
> On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
>> 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.
>
OK, I will do that.
> --
> Regards,
> Sylwester
>
>> 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]>
>> ---
> ^^^^^
>
I got it.
Thank you
--
Gustavo A. R. Silva