Re: [PATCH v2] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888 input support

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 10/29/2014 7:11 AM, Simon Horman wrote:

>>>> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

>>>> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>>>> ---

>>>> This patch is against master branch of linuxtv.org/media_tree.git.

>>>> v2 [Yoshihiro Kaneko]
>>>> * remove unused/useless definition as suggested by Sergei Shtylyov

>>>     I didn't say it's useless, I just suspected that you missed the necessary
>>> test somewhere...

>> Sorry for my inaccurate description.

>>>>    drivers/media/platform/soc_camera/rcar_vin.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)

>>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
>>>> b/drivers/media/platform/soc_camera/rcar_vin.c
>>>> index 20defcb..cb5e682 100644
>>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
>>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
>>>> @@ -74,6 +74,7 @@
>>>>    #define VNMC_INF_YUV10_BT656  (2 << 16)
>>>>    #define VNMC_INF_YUV10_BT601  (3 << 16)
>>>>    #define VNMC_INF_YUV16                (5 << 16)
>>>> +#define VNMC_INF_RGB888                (6 << 16)
>>>>    #define VNMC_VUP              (1 << 10)
>>>>    #define VNMC_IM_ODD           (0 << 3)
>>>>    #define VNMC_IM_ODD_EVEN      (1 << 3)

>>> [...]

>>>> @@ -331,6 +336,9 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>>>>          if (output_is_yuv)
>>>>                  vnmc |= VNMC_BPS;
>>>>
>>>> +       if (vnmc & VNMC_INF_RGB888)
>>>> +               vnmc ^= VNMC_BPS;
>>>> +

>>>     Hm, this also changes the behavior for VNMC_INF_YUV16 and
>>> VNMC_INF_YUV10_BT{601|656}. Is this actually intended?

>> Probably this code is incorrect.
>> Thank you for your review.

> Thanks, I have confirmed with Matsuoka-san that there is a problem here.

> He has provided the following fix. Could you see about squashing it into
> the above patch and reposting?

> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

> [PATCH] media: soc_camera: rcar_vin: Fix bit field check

> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 013d75c..da62d94 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -94,7 +94,7 @@
>   #define VNMC_INF_YUV8_BT601	(1 << 16)
>   #define VNMC_INF_YUV16		(5 << 16)
>   #define VNMC_INF_RGB888		(6 << 16)
> -#define VNMC_INF_RGB_MASK	(6 << 16)
> +#define VNMC_INF_MASK		(7 << 16)

    #define it above VNMC_INF_YUV8_BT656 please.

>   #define VNMC_VUP		(1 << 10)
>   #define VNMC_IM_ODD		(0 << 3)
>   #define VNMC_IM_ODD_EVEN	(1 << 3)
> @@ -675,7 +675,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>   	if (output_is_yuv)
>   		vnmc |= VNMC_BPS;
>
> -	if (vnmc & VNMC_INF_RGB_MASK)
> +	if ((vnmc & VNMC_INF_MASK) = VNMC_INF_RGB888)

    Is he sure it shouldn't be (vnmc & VNMC_INF_RGB888) = VNMC_INF_RGB888 to 
also cover 16-bit RGB666 and 12-bit RGB88?

WBR, Sergei