On Wed, Oct 29, 2014 at 02:31:41PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 10/29/2014 7:11 AM, Simon Horman wrote:
>
> >>>>From: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
>
> >>>>Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
> >>>>Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
> >>>>---
>
> >>>>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@xxxxxxxxxxx>
>
> >[PATCH] media: soc_camera: rcar_vin: Fix bit field check
>
> >Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
>
> >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?
Nice, I think that is a good idea (although the latter formats aren't
supported by the driver yet, right?).
Its somewhat unobvious how that logic works so perhaps we should add a
comment like this
/* If input and output use the same colorspace, use bypass mode */
if (output_is_yuv)
vnmc |= VNMC_BPS;
/* The above assumes YUV input, toggle BPS for RGB input.
* RGB inputs can be detected by checking that the most-significant
* two bits of INF are set. This corresponds to the bits
* set in VNMC_INF_RGB888. */
if ((vnmc & VNMC_INF_RGB888)) == VNMC_INF_RGB888)
vnmc ^= VNMC_BPS;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
