Re: [PATCH 06/21] media: i2c: imx258: Make V4L2_CID_VBLANK configurable.

From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	linux-media@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 06/21] media: i2c: imx258: Make V4L2_CID_VBLANK configurable.
Date: Wed, 31 May 2023 17:16:17 +0200	[thread overview]
Message-ID: <vnwwrfgwr5ueiheqsnvxz7sh3qjcftocjjxdghvrigfmlss4yu@5xq3tppv4pxg> (raw)
In-Reply-To: <20230530173000.3060865-7-dave.stevenson@raspberrypi.com>

Hi Dave

On Tue, May 30, 2023 at 06:29:45PM +0100, Dave Stevenson wrote:
> The values and ranges of V4L2_CID_VBLANK are all computed,
> so there is no reason for it to be a read only control.
> Remove the register values from the mode lists, add the
> handler, and remove the read only flag.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx258.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 30bae7388c3a..c6fb649abb95 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -30,6 +30,8 @@
>  #define IMX258_VTS_30FPS_VGA		0x034c
>  #define IMX258_VTS_MAX			0xffff
>
> +#define IMX258_REG_VTS			0x0340
> +
>  /* HBLANK control - read only */
>  #define IMX258_PPL_DEFAULT		5352
>
> @@ -202,8 +204,6 @@ static const struct imx258_reg mode_4208x3120_regs[] = {
>  	{ 0x0114, 0x03 },
>  	{ 0x0342, 0x14 },
>  	{ 0x0343, 0xE8 },
> -	{ 0x0340, 0x0C },
> -	{ 0x0341, 0x50 },
>  	{ 0x0344, 0x00 },
>  	{ 0x0345, 0x00 },
>  	{ 0x0346, 0x00 },
> @@ -319,8 +319,6 @@ static const struct imx258_reg mode_2104_1560_regs[] = {
>  	{ 0x0114, 0x03 },
>  	{ 0x0342, 0x14 },
>  	{ 0x0343, 0xE8 },
> -	{ 0x0340, 0x06 },
> -	{ 0x0341, 0x38 },
>  	{ 0x0344, 0x00 },
>  	{ 0x0345, 0x00 },
>  	{ 0x0346, 0x00 },
> @@ -436,8 +434,6 @@ static const struct imx258_reg mode_1048_780_regs[] = {
>  	{ 0x0114, 0x03 },
>  	{ 0x0342, 0x14 },
>  	{ 0x0343, 0xE8 },
> -	{ 0x0340, 0x03 },
> -	{ 0x0341, 0x4C },
>  	{ 0x0344, 0x00 },
>  	{ 0x0345, 0x00 },
>  	{ 0x0346, 0x00 },
> @@ -803,6 +799,11 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  					       BIT(IMX258_HDR_RATIO_MAX));
>  		}
>  		break;
> +	case V4L2_CID_VBLANK:

Should a new vblank value change the exposure limits too ?

> +		ret = imx258_write_reg(imx258, IMX258_REG_VTS,
> +				       IMX258_REG_VALUE_16BIT,
> +				       imx258->cur_mode->height + ctrl->val);
> +		break;
>  	default:
>  		dev_info(&client->dev,
>  			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
> @@ -1214,9 +1215,6 @@ static int imx258_init_controls(struct imx258 *imx258)
>  				IMX258_VTS_MAX - imx258->cur_mode->height, 1,
>  				vblank_def);
>
> -	if (imx258->vblank)
> -		imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -
>  	imx258->hblank = v4l2_ctrl_new_std(
>  				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
>  				IMX258_PPL_DEFAULT - imx258->cur_mode->width,
> --
> 2.25.1
>