Re: [PATCH] media: soc_camera: rcar_vin: Fix alignment of clipping size

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

Hello.

On 10/31/2014 12:10 PM, Yoshihiro Kaneko wrote:

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

> Since the Start Line Pre-Clip register, the Start Pixel Pre-Clip
> register and the End Line Post-Clip register do not have restriction

    Hm, my R-Car H1 manual says to specify an even number for the Start Pixel 
Pre-Clip Register.

> of H/W to a setting value, the processing of alignment is unnecessary.
> This patch corrects so that processing of alignment is not performed.

> However, the End Pixel Post-Clip register has restriction
> of H/W which must be an even number value at the time of the
> output of YCbCr-422 format. By this patch, the processing of
> alignment to an even number value is added on the above-mentioned
> conditions.

    Well, the R-Car H1/M1A manuals don't specify such restriction.

> The variable set to a register is as follows.

>   - Start Line Pre-Clip register
>     cam->vin_top
>   - Start Pixel Pre-Clip register
>     cam->vin_left
>   - End Line Post-Clip register
>     pix->height
>   - End Pixel Post-Clip register
>     pix->width

> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
>   drivers/media/platform/soc_camera/rcar_vin.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)

> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index d3d2f7d..1934e15 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
[...]
> @@ -1761,8 +1761,18 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>   	}
>
>   	/* FIXME: calculate using depth and bus width */
> -	v4l_bound_align_image(&pix->width, 2, VIN_MAX_WIDTH, 1,
> -			      &pix->height, 4, VIN_MAX_HEIGHT, 2, 0);
> +	/*
> +	 * When performing a YCbCr-422 format output, even if it performs
> +	 * odd number clipping by pixel post clip processing, it is outputted

    s/outputted/output/ -- it's an irregular verb.

> +	 * to a memory per even pixels.
> +	 */
> +	if ((pixfmt = V4L2_PIX_FMT_NV16) || (pixfmt = V4L2_PIX_FMT_YUYV) ||
> +		(pixfmt = V4L2_PIX_FMT_UYVY))

    This is certainly asking to be a *switch* statement instead...

> +		v4l_bound_align_image(&pix->width, 5, VIN_MAX_WIDTH, 1,
> +				      &pix->height, 2, VIN_MAX_HEIGHT, 0, 0);
> +	else
> +		v4l_bound_align_image(&pix->width, 5, VIN_MAX_WIDTH, 0,
> +				      &pix->height, 2, VIN_MAX_HEIGHT, 0, 0);

WBR, Sergei