Received-SPF: pass (google.com: domain of linux-kernel+bounces-38903-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 25 Jan 2024 16:19:17 +0000
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Umang Jain <umang.jain@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Matthias Fend <matthias.fend@emfend.at>
Subject: Re: [PATCH 4/4] media: i2c: imx335: Add support for test pattern
 generator
Message-ID: <ZbKKBcf2PX8u3JGV@kekkonen.localdomain>
References: <20240125154908.465191-1-umang.jain@ideasonboard.com>
 <20240125154908.465191-5-umang.jain@ideasonboard.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240125154908.465191-5-umang.jain@ideasonboard.com>

Hi Umang,

On Thu, Jan 25, 2024 at 09:19:08PM +0530, Umang Jain wrote:
> From: Matthias Fend <matthias.fend@emfend.at>
> 
> Add support for the sensor's test pattern generator.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 99 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index e64ee99cbae4..f9a2337a80c0 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -45,6 +45,21 @@
>  /* Group hold register */
>  #define IMX335_REG_HOLD		0x3001
>  
> +/* Test pattern generator */
> +#define IMX335_REG_TPG		0x329e
> +#define IMX335_TPG_ALL_000	0
> +#define IMX335_TPG_ALL_FFF	1
> +#define IMX335_TPG_ALL_555	2
> +#define IMX335_TPG_ALL_AAA	3
> +#define IMX335_TPG_TOG_555_AAA	4
> +#define IMX335_TPG_TOG_AAA_555	5
> +#define IMX335_TPG_TOG_000_555	6
> +#define IMX335_TPG_TOG_555_000	7
> +#define IMX335_TPG_TOG_000_FFF	8
> +#define IMX335_TPG_TOG_FFF_000	9
> +#define IMX335_TPG_H_COLOR_BARS 10
> +#define IMX335_TPG_V_COLOR_BARS 11
> +
>  /* Input clock rate */
>  #define IMX335_INCLK_RATE	24000000
>  
> @@ -162,6 +177,38 @@ struct imx335 {
>  };
>  
>  
> +static const char * const imx335_tpg_menu[] = {
> +	"Disabled",
> +	"All 000h",
> +	"All FFFh",
> +	"All 555h",
> +	"All AAAh",
> +	"Toggle 555/AAAh",
> +	"Toggle AAA/555h",
> +	"Toggle 000/555h",
> +	"Toggle 555/000h",
> +	"Toggle 000/FFFh",
> +	"Toggle FFF/000h",
> +	"Horizontal color bars",
> +	"Vertical color bars",
> +};
> +
> +static const int imx335_tpg_val[] = {
> +	IMX335_TPG_ALL_000,
> +	IMX335_TPG_ALL_000,
> +	IMX335_TPG_ALL_FFF,
> +	IMX335_TPG_ALL_555,
> +	IMX335_TPG_ALL_AAA,
> +	IMX335_TPG_TOG_555_AAA,
> +	IMX335_TPG_TOG_AAA_555,
> +	IMX335_TPG_TOG_000_555,
> +	IMX335_TPG_TOG_555_000,
> +	IMX335_TPG_TOG_000_FFF,
> +	IMX335_TPG_TOG_FFF_000,
> +	IMX335_TPG_H_COLOR_BARS,
> +	IMX335_TPG_V_COLOR_BARS,
> +};
> +
>  /* Sensor mode registers */
>  static const struct imx335_reg mode_2592x1940_regs[] = {
>  	{0x3000, 0x01},
> @@ -505,6 +552,46 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
>  	return ret;
>  }
>  
> +static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
> +{
> +	int ret;
> +
> +	if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
> +		return -EINVAL;

This is unnecessary, the control framework ensures this already.

> +
> +	if (pattern_index) {
> +		const struct imx335_reg tpg_enable_regs[] = {
> +			{ 0x3148, 0x10 },
> +			{ 0x3280, 0x00 },
> +			{ 0x329c, 0x01 },
> +			{ 0x32a0, 0x11 },
> +			{ 0x3302, 0x00 },
> +			{ 0x3303, 0x00 },
> +			{ 0x336c, 0x00 },
> +		};
> +
> +		ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1, imx335_tpg_val[pattern_index]);

Can you do:

	$ ./scripts/checkpatch.pl --strict --max-line-length=80

on these?

> +		if (ret)
> +			return ret;
> +
> +		ret = imx335_write_regs(imx335, tpg_enable_regs, ARRAY_SIZE(tpg_enable_regs));

Return already here.

> +	} else {
> +		const struct imx335_reg tpg_disable_regs[] = {
> +			{ 0x3148, 0x00 },
> +			{ 0x3280, 0x01 },
> +			{ 0x329c, 0x00 },
> +			{ 0x32a0, 0x10 },
> +			{ 0x3302, 0x32 },
> +			{ 0x3303, 0x00 },
> +			{ 0x336c, 0x01 },
> +		};
> +
> +		ret = imx335_write_regs(imx335, tpg_disable_regs, ARRAY_SIZE(tpg_disable_regs));

And here.

> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * imx335_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -558,6 +645,10 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
>  
>  		ret = imx335_update_exp_gain(imx335, exposure, analog_gain);
>  
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = imx335_update_test_pattern(imx335, ctrl->val);
> +
>  		break;
>  	default:
>  		dev_err(imx335->dev, "Invalid control %d\n", ctrl->id);
> @@ -1122,7 +1213,7 @@ static int imx335_init_controls(struct imx335 *imx335)
>  	u32 lpfr;
>  	int ret;
>  
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 6);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
>  	if (ret)
>  		return ret;
>  
> @@ -1156,6 +1247,12 @@ static int imx335_init_controls(struct imx335 *imx335)
>  						mode->vblank_max,
>  						1, mode->vblank);
>  
> +	v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
> +				     &imx335_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(imx335_tpg_menu) - 1,
> +				     0, 0, imx335_tpg_menu);
> +
>  	/* Read only controls */
>  	imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
>  					      &imx335_ctrl_ops,

-- 
Regards,

Sakari Ailus