[1/1] media: i2c: max96717: add test pattern ctrl

Message ID 20240617145958.1819069-2-tomm.merciai@gmail.com (mailing list archive)
State New
Headers
Series media: i2c: max96717: add test pattern ctrl |

Commit Message

Tommaso Merciai June 17, 2024, 2:59 p.m. UTC
  Add v4l2 test pattern control.

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++---
 1 file changed, 194 insertions(+), 16 deletions(-)
  

Comments

Julien Massot June 25, 2024, 7:54 a.m. UTC | #1
Hi Tommaso,

Thanks for your patch.

I tested it on my setup and can capture frames. Here's a link to an 
example: https://pasteboard.co/j8yHuE4YdYDV.jpg.

I had some trouble with link validation because my sensor doesn't 
support the RGB format. Once we have internal pad support, we won't need 
to worry about the serializer creating an incompatible video stream for 
the sensor.

In the future, it would be great if we could support the serializer 
without needing a connected sensor. This way, we can use it as a pattern 
generator with this patch. However, not all GMSL2 serializers work this 
way. For example, the MAX9295A can't generate an internal PCLK and 
relies on the sensor to provide the video stream.

Overall, this patch is very useful as it lets us receive a pattern from 
the serializer, which helps validate the GMSL2 connection.

It also handles issues related to generator timing, bits per pixel 
(bpp), and tunnel mode that users might encounter.

On 6/17/24 4:59 PM, Tommaso Merciai wrote:
> Add v4l2 test pattern control.
> 
> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> ---
>   drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++---
>   1 file changed, 194 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 949306485873..c263bbca7318 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -16,6 +16,7 @@
>   #include <linux/regmap.h>
>   
>   #include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
>   #include <media/v4l2-fwnode.h>
>   #include <media/v4l2-subdev.h>
>   
> @@ -36,11 +37,37 @@
>   #define MAX96717_DEV_ID  CCI_REG8(0xd)
>   #define MAX96717_DEV_REV CCI_REG8(0xe)
>   #define MAX96717_DEV_REV_MASK GENMASK(3, 0)
> +#define MAX96717_IO_CHK0 CCI_REG8(0x24f)
In MAX96717 datasheet this register is named VTX1.
Can you also move these definitions to the VTX section?
> +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1)
>   
>   /* VID_TX Z */
> +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110)
> +#define MAX96717_VIDEO_AUTO_BPP BIT(3)
>   #define MAX96717_VIDEO_TX2 CCI_REG8(0x112)
>   #define MAX96717_VIDEO_PCLKDET BIT(7)
>   
> +/* VRX_PATGEN_0 */
For serializer these registers are named VTX_Z instead of VRX.

> +#define MAX96717_PATGEN_0              CCI_REG8(0x24e)
#define MAX96717_VTX_0              CCI_REG8(0x24e)
> +#define MAX96717_PATGEN_1              CCI_REG8(0x26b)
Can you keep this define ordered by address?
> +#define MAX96717_PATGEN_MODE           GENMASK(1, 0)
> +#define MAX96717_PATGEN_VS_DLY         CCI_REG24(0x250)
#define MAX96717_VTX_VS_DLY
> +#define MAX96717_PATGEN_VS_HIGH        CCI_REG24(0x253)
> +#define MAX96717_PATGEN_VS_LOW         CCI_REG24(0x256)
> +#define MAX96717_PATGEN_V2H            CCI_REG24(0x259)
> +#define MAX96717_PATGEN_HS_HIGH        CCI_REG16(0x25c)
> +#define MAX96717_PATGEN_HS_LOW         CCI_REG16(0x25e)
> +#define MAX96717_PATGEN_HS_CNT         CCI_REG16(0x260)
> +#define MAX96717_PATGEN_V2D            CCI_REG24(0x262)
> +#define MAX96717_PATGEN_DE_HIGH        CCI_REG16(0x265)
> +#define MAX96717_PATGEN_DE_LOW         CCI_REG16(0x267)
> +#define MAX96717_PATGEN_DE_CNT         CCI_REG16(0x269)
> +#define MAX96717_PATGEN_GRAD_INC       CCI_REG8(0x26c)
> +#define MAX96717_PATGEN_CHKB_COLOR_A   CCI_REG24(0x26d)
> +#define MAX96717_PATGEN_CHKB_COLOR_B   CCI_REG24(0x270)
> +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273)
> +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274)
> +#define MAX96717_PATGEN_CHKB_ALT       CCI_REG8(0x275)
> +
>   /* GPIO */
>   #define MAX96717_NUM_GPIO         11
>   #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> @@ -82,6 +109,12 @@
>   /* MISC */
>   #define PIO_SLEW_1 CCI_REG8(0x570)
>   
> +enum max96717_vpg_mode {
> +	MAX96717_VPG_DISABLED = 0,
> +	MAX96717_VPG_CHECKERBOARD = 1,
> +	MAX96717_VPG_GRADIENT = 2,
> +};
> +
>   struct max96717_priv {
>   	struct i2c_client		  *client;
>   	struct regmap			  *regmap;
> @@ -89,6 +122,7 @@ struct max96717_priv {
>   	struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
>   	struct v4l2_subdev                sd;
>   	struct media_pad                  pads[MAX96717_PORTS];
> +	struct v4l2_ctrl_handler          ctrl_handler;
>   	struct v4l2_async_notifier        notifier;
>   	struct v4l2_subdev                *source_sd;
>   	u16                               source_sd_pad;
> @@ -96,6 +130,7 @@ struct max96717_priv {
>   	u8                                pll_predef_index;
>   	struct clk_hw                     clk_hw;
>   	struct gpio_chip                  gpio_chip;
> +	enum max96717_vpg_mode            pattern;
>   };
>   
>   static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start)
>   			       start ? MAX96717_START_PORT_B : 0, NULL);
>   }
>   
> +static int max96717_apply_patgen_timing(struct max96717_priv *priv,
> +					struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_mbus_framefmt *fmt =
> +		v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE);
> +	const u32 h_active = fmt->width;
> +	const u32 h_fp = 88;
> +	const u32 h_sw = 44;
> +	const u32 h_bp = 148;
> +	u32 h_tot;
> +	const u32 v_active = fmt->height;
> +	const u32 v_fp = 4;
> +	const u32 v_sw = 5;
> +	const u32 v_bp = 36;
> +	u32 v_tot;
> +	int ret = 0;
> +
> +	h_tot = h_active + h_fp + h_sw + h_bp;
> +	v_tot = v_active + v_fp + v_sw + v_bp;
> +
> +	/* 75 Mhz pixel clock */
> +	cci_update_bits(priv->regmap, MAX96717_IO_CHK0,
> +			MAX96717_PATTERN_CLK_FREQ, 0xa, &ret);
> +
> +	dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height,
> +		 fmt->width);
> +
> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW,
> +		  (v_active + v_fp + v_bp) * h_tot, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp,
> +		  &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_V2D,
> +		  h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp,
> +		  &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret);
> +	/* B G R */
> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret);
> +	/* B G R */
> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret);
> +	cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret);
> +
> +	return ret;
> +}
> +
> +static int max96717_apply_patgen(struct max96717_priv *priv,
> +				 struct v4l2_subdev_state *state)
> +{
> +	unsigned int val;
> +	int ret = 0;
> +
> +	if (priv->pattern)
> +		ret = max96717_apply_patgen_timing(priv, state);
> +
> +	cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0,
> +		  &ret);
> +
> +	val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern);
> +	cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE,
> +			val, &ret);
> +	return ret;
> +}
> +
> +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct max96717_priv *priv =
> +		container_of(ctrl->handler, struct max96717_priv, ctrl_handler);
> +	int ret;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_TEST_PATTERN:
> +		if (priv->enabled_source_streams)
> +			return -EBUSY;
> +		priv->pattern = ctrl->val;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Use bpp from bpp register */
> +	ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0,
> +			      MAX96717_VIDEO_AUTO_BPP,
> +			      priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP,
> +			      NULL);
> +
> +	/* Pattern generator doesn't work with tunnel mode */
Can you add a comment saying that the deserializer should manage the link in
pixel mode as well.
> +	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
> +			       MAX96717_TUN_MODE,
> +			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);
> +}
> +
> +static const char * const max96717_test_pattern[] = {
> +	"Disabled",
> +	"Checkerboard",
> +	"Gradient"
> +};
> +
> +static const struct v4l2_ctrl_ops max96717_ctrl_ops = {
> +	.s_ctrl = max96717_s_ctrl,
> +};
> +
>   static int max96717_gpiochip_get(struct gpio_chip *gpiochip,
>   				 unsigned int offset)
>   {
> @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
>   	u64 sink_streams;
>   	int ret;
>   
> -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> -						       MAX96717_PAD_SOURCE,
> -						       MAX96717_PAD_SINK,
> -						       &streams_mask);
> -
>   	if (!priv->enabled_source_streams)
>   		max96717_start_csi(priv, true);
>   
> -	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
> -					 sink_streams);
> -	if (ret) {
> -		dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> -			sink_streams);
> +	ret = max96717_apply_patgen(priv, state);
> +	if (ret)
>   		goto stop_csi;
> +
> +	if (!priv->pattern) {
> +		sink_streams =
> +			v4l2_subdev_state_xlate_streams(state,
> +							MAX96717_PAD_SOURCE,
> +							MAX96717_PAD_SINK,
> +							&streams_mask);
> +
> +		ret = v4l2_subdev_enable_streams(priv->source_sd,
> +						 priv->source_sd_pad,
> +						 sink_streams);
> +		if (ret) {
> +			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> +				sink_streams);
> +			goto stop_csi;
> +		}
>   	}
>   
>   	priv->enabled_source_streams |= streams_mask;
> @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
>   	if (!priv->enabled_source_streams)
>   		max96717_start_csi(priv, false);
>   
> -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> -						       MAX96717_PAD_SOURCE,
> -						       MAX96717_PAD_SINK,
> -						       &streams_mask);
> +	if (!priv->pattern) {
> +		int ret;
> +
> +		sink_streams =
> +			v4l2_subdev_state_xlate_streams(state,
> +							MAX96717_PAD_SOURCE,
> +							MAX96717_PAD_SINK,
> +							&streams_mask);
>   
> -	return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
> -					   sink_streams);
> +		ret = v4l2_subdev_disable_streams(priv->source_sd,
> +						  priv->source_sd_pad,
> +						  sink_streams);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>   }
>   
>   static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
> @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv)
>   	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops);
>   	priv->sd.internal_ops = &max96717_internal_ops;
>   
> +	v4l2_ctrl_handler_init(&priv->ctrl_handler, 1);
> +	priv->sd.ctrl_handler = &priv->ctrl_handler;
> +
> +	v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler,
> +				     &max96717_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(max96717_test_pattern) - 1,
> +				     0, 0, max96717_test_pattern);
> +	if (priv->ctrl_handler.error) {
> +		ret = priv->ctrl_handler.error;
> +		goto err_free_ctrl;
> +	}
> +
>   	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>   	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>   	priv->sd.entity.ops = &max96717_entity_ops;
> @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv)
>   	v4l2_subdev_cleanup(&priv->sd);
>   err_entity_cleanup:
>   	media_entity_cleanup(&priv->sd.entity);
> +err_free_ctrl:
> +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
>   
>   	return ret;
>   }
> @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv)
>   	v4l2_async_nf_cleanup(&priv->notifier);
>   	v4l2_subdev_cleanup(&priv->sd);
>   	media_entity_cleanup(&priv->sd.entity);
> +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
>   }
>   
>   struct max96717_pll_predef_freq {

Regards,
Julien
  
Tommaso Merciai June 25, 2024, 5:31 p.m. UTC | #2
On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote:

Hi Julien,

> Hi Tommaso,
> 
> Thanks for your patch.
> 
> I tested it on my setup and can capture frames. Here's a link to an example:
> https://pasteboard.co/j8yHuE4YdYDV.jpg.

Nice! Thanks for sharing and testing.

> 
> I had some trouble with link validation because my sensor doesn't support
> the RGB format. Once we have internal pad support, we won't need to worry
> about the serializer creating an incompatible video stream for the sensor.
> 
> In the future, it would be great if we could support the serializer without
> needing a connected sensor. This way, we can use it as a pattern generator
> with this patch. However, not all GMSL2 serializers work this way. For
> example, the MAX9295A can't generate an internal PCLK and relies on the
> sensor to provide the video stream.

Fully agree.

> 
> Overall, this patch is very useful as it lets us receive a pattern from the
> serializer, which helps validate the GMSL2 connection.
> 
> It also handles issues related to generator timing, bits per pixel (bpp),
> and tunnel mode that users might encounter.

Fully agree, in my case enabling this test pattern help me a lot on
validating the gmsl2 pipe. 

> 
> On 6/17/24 4:59 PM, Tommaso Merciai wrote:
> > Add v4l2 test pattern control.
> > 
> > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > ---
> >   drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++---
> >   1 file changed, 194 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 949306485873..c263bbca7318 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/regmap.h>
> >   #include <media/v4l2-cci.h>
> > +#include <media/v4l2-ctrls.h>
> >   #include <media/v4l2-fwnode.h>
> >   #include <media/v4l2-subdev.h>
> > @@ -36,11 +37,37 @@
> >   #define MAX96717_DEV_ID  CCI_REG8(0xd)
> >   #define MAX96717_DEV_REV CCI_REG8(0xe)
> >   #define MAX96717_DEV_REV_MASK GENMASK(3, 0)
> > +#define MAX96717_IO_CHK0 CCI_REG8(0x24f)
> In MAX96717 datasheet this register is named VTX1.
> Can you also move these definitions to the VTX section?
> > +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1)
> >   /* VID_TX Z */
> > +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110)
> > +#define MAX96717_VIDEO_AUTO_BPP BIT(3)
> >   #define MAX96717_VIDEO_TX2 CCI_REG8(0x112)
> >   #define MAX96717_VIDEO_PCLKDET BIT(7)
> > +/* VRX_PATGEN_0 */
> For serializer these registers are named VTX_Z instead of VRX.
> 
> > +#define MAX96717_PATGEN_0              CCI_REG8(0x24e)
> #define MAX96717_VTX_0              CCI_REG8(0x24e)
> > +#define MAX96717_PATGEN_1              CCI_REG8(0x26b)
> Can you keep this define ordered by address?
> > +#define MAX96717_PATGEN_MODE           GENMASK(1, 0)
> > +#define MAX96717_PATGEN_VS_DLY         CCI_REG24(0x250)
> #define MAX96717_VTX_VS_DLY
> > +#define MAX96717_PATGEN_VS_HIGH        CCI_REG24(0x253)
> > +#define MAX96717_PATGEN_VS_LOW         CCI_REG24(0x256)
> > +#define MAX96717_PATGEN_V2H            CCI_REG24(0x259)
> > +#define MAX96717_PATGEN_HS_HIGH        CCI_REG16(0x25c)
> > +#define MAX96717_PATGEN_HS_LOW         CCI_REG16(0x25e)
> > +#define MAX96717_PATGEN_HS_CNT         CCI_REG16(0x260)
> > +#define MAX96717_PATGEN_V2D            CCI_REG24(0x262)
> > +#define MAX96717_PATGEN_DE_HIGH        CCI_REG16(0x265)
> > +#define MAX96717_PATGEN_DE_LOW         CCI_REG16(0x267)
> > +#define MAX96717_PATGEN_DE_CNT         CCI_REG16(0x269)
> > +#define MAX96717_PATGEN_GRAD_INC       CCI_REG8(0x26c)
> > +#define MAX96717_PATGEN_CHKB_COLOR_A   CCI_REG24(0x26d)
> > +#define MAX96717_PATGEN_CHKB_COLOR_B   CCI_REG24(0x270)
> > +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273)
> > +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274)
> > +#define MAX96717_PATGEN_CHKB_ALT       CCI_REG8(0x275)
> > +

So your plan is to move all this stuff here:


/* VTX_Z */
#define MAX96717_VTX0                  CCI_REG8(0x24e)
#define MAX96717_VTX1                  CCI_REG8(0x24f)
#define MAX96717_PATTERN_CLK_FREQ      GENMASK(3, 1)
#define MAX96717_VTX_VS_DLY            CCI_REG24(0x250)
#define MAX96717_VTX_VS_HIGH           CCI_REG24(0x253)
#define MAX96717_VTX_VS_LOW            CCI_REG24(0x256)
#define MAX96717_VTX_V2H               CCI_REG24(0x259)
#define MAX96717_VTX_HS_HIGH           CCI_REG16(0x25c)
#define MAX96717_VTX_HS_LOW            CCI_REG16(0x25e)
#define MAX96717_VTX_HS_CNT            CCI_REG16(0x260)
#define MAX96717_VTX_V2D               CCI_REG24(0x262)
#define MAX96717_VTX_DE_HIGH           CCI_REG16(0x265)
#define MAX96717_VTX_DE_LOW            CCI_REG16(0x267)
#define MAX96717_VTX_DE_CNT            CCI_REG16(0x269)
#define MAX96717_VTX29                 CCI_REG8(0x26b)
#define MAX96717_VTX_MODE              GENMASK(1, 0)
#define MAX96717_VTX_GRAD_INC          CCI_REG8(0x26c)
#define MAX96717_VTX_CHKB_COLOR_A      CCI_REG24(0x26d)
#define MAX96717_VTX_CHKB_COLOR_B      CCI_REG24(0x270)
#define MAX96717_VTX_CHKB_RPT_CNT_A    CCI_REG8(0x273)
#define MAX96717_VTX_CHKB_RPT_CNT_B    CCI_REG8(0x274)
#define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)

In a fixed order right? :-)



> >   /* GPIO */
> >   #define MAX96717_NUM_GPIO         11
> >   #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> > @@ -82,6 +109,12 @@
> >   /* MISC */
> >   #define PIO_SLEW_1 CCI_REG8(0x570)
> > +enum max96717_vpg_mode {
> > +	MAX96717_VPG_DISABLED = 0,
> > +	MAX96717_VPG_CHECKERBOARD = 1,
> > +	MAX96717_VPG_GRADIENT = 2,
> > +};
> > +
> >   struct max96717_priv {
> >   	struct i2c_client		  *client;
> >   	struct regmap			  *regmap;
> > @@ -89,6 +122,7 @@ struct max96717_priv {
> >   	struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> >   	struct v4l2_subdev                sd;
> >   	struct media_pad                  pads[MAX96717_PORTS];
> > +	struct v4l2_ctrl_handler          ctrl_handler;
> >   	struct v4l2_async_notifier        notifier;
> >   	struct v4l2_subdev                *source_sd;
> >   	u16                               source_sd_pad;
> > @@ -96,6 +130,7 @@ struct max96717_priv {
> >   	u8                                pll_predef_index;
> >   	struct clk_hw                     clk_hw;
> >   	struct gpio_chip                  gpio_chip;
> > +	enum max96717_vpg_mode            pattern;
> >   };
> >   static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> > @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start)
> >   			       start ? MAX96717_START_PORT_B : 0, NULL);
> >   }
> > +static int max96717_apply_patgen_timing(struct max96717_priv *priv,
> > +					struct v4l2_subdev_state *state)
> > +{
> > +	struct v4l2_mbus_framefmt *fmt =
> > +		v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE);
> > +	const u32 h_active = fmt->width;
> > +	const u32 h_fp = 88;
> > +	const u32 h_sw = 44;
> > +	const u32 h_bp = 148;
> > +	u32 h_tot;
> > +	const u32 v_active = fmt->height;
> > +	const u32 v_fp = 4;
> > +	const u32 v_sw = 5;
> > +	const u32 v_bp = 36;
> > +	u32 v_tot;
> > +	int ret = 0;
> > +
> > +	h_tot = h_active + h_fp + h_sw + h_bp;
> > +	v_tot = v_active + v_fp + v_sw + v_bp;
> > +
> > +	/* 75 Mhz pixel clock */
> > +	cci_update_bits(priv->regmap, MAX96717_IO_CHK0,
> > +			MAX96717_PATTERN_CLK_FREQ, 0xa, &ret);
> > +
> > +	dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height,
> > +		 fmt->width);
> > +
> > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW,
> > +		  (v_active + v_fp + v_bp) * h_tot, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp,
> > +		  &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_V2D,
> > +		  h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp,
> > +		  &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret);
> > +	/* B G R */
> > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret);
> > +	/* B G R */
> > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret);
> > +	cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int max96717_apply_patgen(struct max96717_priv *priv,
> > +				 struct v4l2_subdev_state *state)
> > +{
> > +	unsigned int val;
> > +	int ret = 0;
> > +
> > +	if (priv->pattern)
> > +		ret = max96717_apply_patgen_timing(priv, state);
> > +
> > +	cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0,
> > +		  &ret);
> > +
> > +	val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern);
> > +	cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE,
> > +			val, &ret);
> > +	return ret;
> > +}
> > +
> > +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct max96717_priv *priv =
> > +		container_of(ctrl->handler, struct max96717_priv, ctrl_handler);
> > +	int ret;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_TEST_PATTERN:
> > +		if (priv->enabled_source_streams)
> > +			return -EBUSY;
> > +		priv->pattern = ctrl->val;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Use bpp from bpp register */
> > +	ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0,
> > +			      MAX96717_VIDEO_AUTO_BPP,
> > +			      priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP,
> > +			      NULL);
> > +
> > +	/* Pattern generator doesn't work with tunnel mode */
> Can you add a comment saying that the deserializer should manage the link in
> pixel mode as well.

Something like:
	/* Pattern generator doesn't work with tunnel mode.
	   Is mandatory to put also the deserializer into pixel mode.   
        */

What do you think?

Thanks & Regards,
Tommaso

> > +	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
> > +			       MAX96717_TUN_MODE,
> > +			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);
> > +}
> > +
> > +static const char * const max96717_test_pattern[] = {
> > +	"Disabled",
> > +	"Checkerboard",
> > +	"Gradient"
> > +};
> > +
> > +static const struct v4l2_ctrl_ops max96717_ctrl_ops = {
> > +	.s_ctrl = max96717_s_ctrl,
> > +};
> > +
> >   static int max96717_gpiochip_get(struct gpio_chip *gpiochip,
> >   				 unsigned int offset)
> >   {
> > @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
> >   	u64 sink_streams;
> >   	int ret;
> > -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> > -						       MAX96717_PAD_SOURCE,
> > -						       MAX96717_PAD_SINK,
> > -						       &streams_mask);
> > -
> >   	if (!priv->enabled_source_streams)
> >   		max96717_start_csi(priv, true);
> > -	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
> > -					 sink_streams);
> > -	if (ret) {
> > -		dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> > -			sink_streams);
> > +	ret = max96717_apply_patgen(priv, state);
> > +	if (ret)
> >   		goto stop_csi;
> > +
> > +	if (!priv->pattern) {
> > +		sink_streams =
> > +			v4l2_subdev_state_xlate_streams(state,
> > +							MAX96717_PAD_SOURCE,
> > +							MAX96717_PAD_SINK,
> > +							&streams_mask);
> > +
> > +		ret = v4l2_subdev_enable_streams(priv->source_sd,
> > +						 priv->source_sd_pad,
> > +						 sink_streams);
> > +		if (ret) {
> > +			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> > +				sink_streams);
> > +			goto stop_csi;
> > +		}
> >   	}
> >   	priv->enabled_source_streams |= streams_mask;
> > @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
> >   	if (!priv->enabled_source_streams)
> >   		max96717_start_csi(priv, false);
> > -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> > -						       MAX96717_PAD_SOURCE,
> > -						       MAX96717_PAD_SINK,
> > -						       &streams_mask);
> > +	if (!priv->pattern) {
> > +		int ret;
> > +
> > +		sink_streams =
> > +			v4l2_subdev_state_xlate_streams(state,
> > +							MAX96717_PAD_SOURCE,
> > +							MAX96717_PAD_SINK,
> > +							&streams_mask);
> > -	return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
> > -					   sink_streams);
> > +		ret = v4l2_subdev_disable_streams(priv->source_sd,
> > +						  priv->source_sd_pad,
> > +						  sink_streams);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> >   }
> >   static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
> > @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv)
> >   	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops);
> >   	priv->sd.internal_ops = &max96717_internal_ops;
> > +	v4l2_ctrl_handler_init(&priv->ctrl_handler, 1);
> > +	priv->sd.ctrl_handler = &priv->ctrl_handler;
> > +
> > +	v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler,
> > +				     &max96717_ctrl_ops,
> > +				     V4L2_CID_TEST_PATTERN,
> > +				     ARRAY_SIZE(max96717_test_pattern) - 1,
> > +				     0, 0, max96717_test_pattern);
> > +	if (priv->ctrl_handler.error) {
> > +		ret = priv->ctrl_handler.error;
> > +		goto err_free_ctrl;
> > +	}
> > +
> >   	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
> >   	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> >   	priv->sd.entity.ops = &max96717_entity_ops;
> > @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv)
> >   	v4l2_subdev_cleanup(&priv->sd);
> >   err_entity_cleanup:
> >   	media_entity_cleanup(&priv->sd.entity);
> > +err_free_ctrl:
> > +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
> >   	return ret;
> >   }
> > @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv)
> >   	v4l2_async_nf_cleanup(&priv->notifier);
> >   	v4l2_subdev_cleanup(&priv->sd);
> >   	media_entity_cleanup(&priv->sd.entity);
> > +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
> >   }
> >   struct max96717_pll_predef_freq {
> 
> Regards,
> Julien
  
Tommaso Merciai June 26, 2024, 8:37 a.m. UTC | #3
On Tue, Jun 25, 2024 at 07:31:05PM +0200, Tommaso Merciai wrote:
> On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote:
> 
> Hi Julien,
> 
> > Hi Tommaso,
> > 
> > Thanks for your patch.
> > 
> > I tested it on my setup and can capture frames. Here's a link to an example:
> > https://pasteboard.co/j8yHuE4YdYDV.jpg.
> 
> Nice! Thanks for sharing and testing.
> 
> > 
> > I had some trouble with link validation because my sensor doesn't support
> > the RGB format. Once we have internal pad support, we won't need to worry
> > about the serializer creating an incompatible video stream for the sensor.
> > 
> > In the future, it would be great if we could support the serializer without
> > needing a connected sensor. This way, we can use it as a pattern generator
> > with this patch. However, not all GMSL2 serializers work this way. For
> > example, the MAX9295A can't generate an internal PCLK and relies on the
> > sensor to provide the video stream.
> 
> Fully agree.
> 
> > 
> > Overall, this patch is very useful as it lets us receive a pattern from the
> > serializer, which helps validate the GMSL2 connection.
> > 
> > It also handles issues related to generator timing, bits per pixel (bpp),
> > and tunnel mode that users might encounter.
> 
> Fully agree, in my case enabling this test pattern help me a lot on
> validating the gmsl2 pipe. 
> 
> > 
> > On 6/17/24 4:59 PM, Tommaso Merciai wrote:
> > > Add v4l2 test pattern control.
> > > 
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > ---
> > >   drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++---
> > >   1 file changed, 194 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > > index 949306485873..c263bbca7318 100644
> > > --- a/drivers/media/i2c/max96717.c
> > > +++ b/drivers/media/i2c/max96717.c
> > > @@ -16,6 +16,7 @@
> > >   #include <linux/regmap.h>
> > >   #include <media/v4l2-cci.h>
> > > +#include <media/v4l2-ctrls.h>
> > >   #include <media/v4l2-fwnode.h>
> > >   #include <media/v4l2-subdev.h>
> > > @@ -36,11 +37,37 @@
> > >   #define MAX96717_DEV_ID  CCI_REG8(0xd)
> > >   #define MAX96717_DEV_REV CCI_REG8(0xe)
> > >   #define MAX96717_DEV_REV_MASK GENMASK(3, 0)
> > > +#define MAX96717_IO_CHK0 CCI_REG8(0x24f)
> > In MAX96717 datasheet this register is named VTX1.
> > Can you also move these definitions to the VTX section?
> > > +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1)
> > >   /* VID_TX Z */
> > > +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110)
> > > +#define MAX96717_VIDEO_AUTO_BPP BIT(3)
> > >   #define MAX96717_VIDEO_TX2 CCI_REG8(0x112)
> > >   #define MAX96717_VIDEO_PCLKDET BIT(7)
> > > +/* VRX_PATGEN_0 */
> > For serializer these registers are named VTX_Z instead of VRX.
> > 
> > > +#define MAX96717_PATGEN_0              CCI_REG8(0x24e)
> > #define MAX96717_VTX_0              CCI_REG8(0x24e)
> > > +#define MAX96717_PATGEN_1              CCI_REG8(0x26b)
> > Can you keep this define ordered by address?
> > > +#define MAX96717_PATGEN_MODE           GENMASK(1, 0)
> > > +#define MAX96717_PATGEN_VS_DLY         CCI_REG24(0x250)
> > #define MAX96717_VTX_VS_DLY
> > > +#define MAX96717_PATGEN_VS_HIGH        CCI_REG24(0x253)
> > > +#define MAX96717_PATGEN_VS_LOW         CCI_REG24(0x256)
> > > +#define MAX96717_PATGEN_V2H            CCI_REG24(0x259)
> > > +#define MAX96717_PATGEN_HS_HIGH        CCI_REG16(0x25c)
> > > +#define MAX96717_PATGEN_HS_LOW         CCI_REG16(0x25e)
> > > +#define MAX96717_PATGEN_HS_CNT         CCI_REG16(0x260)
> > > +#define MAX96717_PATGEN_V2D            CCI_REG24(0x262)
> > > +#define MAX96717_PATGEN_DE_HIGH        CCI_REG16(0x265)
> > > +#define MAX96717_PATGEN_DE_LOW         CCI_REG16(0x267)
> > > +#define MAX96717_PATGEN_DE_CNT         CCI_REG16(0x269)
> > > +#define MAX96717_PATGEN_GRAD_INC       CCI_REG8(0x26c)
> > > +#define MAX96717_PATGEN_CHKB_COLOR_A   CCI_REG24(0x26d)
> > > +#define MAX96717_PATGEN_CHKB_COLOR_B   CCI_REG24(0x270)
> > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273)
> > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274)
> > > +#define MAX96717_PATGEN_CHKB_ALT       CCI_REG8(0x275)
> > > +
> 
> So your plan is to move all this stuff here:
> 
> 
> /* VTX_Z */
> #define MAX96717_VTX0                  CCI_REG8(0x24e)
> #define MAX96717_VTX1                  CCI_REG8(0x24f)
> #define MAX96717_PATTERN_CLK_FREQ      GENMASK(3, 1)
> #define MAX96717_VTX_VS_DLY            CCI_REG24(0x250)
> #define MAX96717_VTX_VS_HIGH           CCI_REG24(0x253)
> #define MAX96717_VTX_VS_LOW            CCI_REG24(0x256)
> #define MAX96717_VTX_V2H               CCI_REG24(0x259)
> #define MAX96717_VTX_HS_HIGH           CCI_REG16(0x25c)
> #define MAX96717_VTX_HS_LOW            CCI_REG16(0x25e)
> #define MAX96717_VTX_HS_CNT            CCI_REG16(0x260)
> #define MAX96717_VTX_V2D               CCI_REG24(0x262)
> #define MAX96717_VTX_DE_HIGH           CCI_REG16(0x265)
> #define MAX96717_VTX_DE_LOW            CCI_REG16(0x267)
> #define MAX96717_VTX_DE_CNT            CCI_REG16(0x269)
> #define MAX96717_VTX29                 CCI_REG8(0x26b)
> #define MAX96717_VTX_MODE              GENMASK(1, 0)
> #define MAX96717_VTX_GRAD_INC          CCI_REG8(0x26c)
> #define MAX96717_VTX_CHKB_COLOR_A      CCI_REG24(0x26d)
> #define MAX96717_VTX_CHKB_COLOR_B      CCI_REG24(0x270)
> #define MAX96717_VTX_CHKB_RPT_CNT_A    CCI_REG8(0x273)
> #define MAX96717_VTX_CHKB_RPT_CNT_B    CCI_REG8(0x274)
> #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
> 
> In a fixed order right? :-)
> 
> 
> 
> > >   /* GPIO */
> > >   #define MAX96717_NUM_GPIO         11
> > >   #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> > > @@ -82,6 +109,12 @@
> > >   /* MISC */
> > >   #define PIO_SLEW_1 CCI_REG8(0x570)
> > > +enum max96717_vpg_mode {
> > > +	MAX96717_VPG_DISABLED = 0,
> > > +	MAX96717_VPG_CHECKERBOARD = 1,
> > > +	MAX96717_VPG_GRADIENT = 2,
> > > +};
> > > +
> > >   struct max96717_priv {
> > >   	struct i2c_client		  *client;
> > >   	struct regmap			  *regmap;
> > > @@ -89,6 +122,7 @@ struct max96717_priv {
> > >   	struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> > >   	struct v4l2_subdev                sd;
> > >   	struct media_pad                  pads[MAX96717_PORTS];
> > > +	struct v4l2_ctrl_handler          ctrl_handler;
> > >   	struct v4l2_async_notifier        notifier;
> > >   	struct v4l2_subdev                *source_sd;
> > >   	u16                               source_sd_pad;
> > > @@ -96,6 +130,7 @@ struct max96717_priv {
> > >   	u8                                pll_predef_index;
> > >   	struct clk_hw                     clk_hw;
> > >   	struct gpio_chip                  gpio_chip;
> > > +	enum max96717_vpg_mode            pattern;
> > >   };
> > >   static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> > > @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start)
> > >   			       start ? MAX96717_START_PORT_B : 0, NULL);
> > >   }
> > > +static int max96717_apply_patgen_timing(struct max96717_priv *priv,
> > > +					struct v4l2_subdev_state *state)
> > > +{
> > > +	struct v4l2_mbus_framefmt *fmt =
> > > +		v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE);
> > > +	const u32 h_active = fmt->width;
> > > +	const u32 h_fp = 88;
> > > +	const u32 h_sw = 44;
> > > +	const u32 h_bp = 148;
> > > +	u32 h_tot;
> > > +	const u32 v_active = fmt->height;
> > > +	const u32 v_fp = 4;
> > > +	const u32 v_sw = 5;
> > > +	const u32 v_bp = 36;
> > > +	u32 v_tot;
> > > +	int ret = 0;
> > > +
> > > +	h_tot = h_active + h_fp + h_sw + h_bp;
> > > +	v_tot = v_active + v_fp + v_sw + v_bp;
> > > +
> > > +	/* 75 Mhz pixel clock */
> > > +	cci_update_bits(priv->regmap, MAX96717_IO_CHK0,
> > > +			MAX96717_PATTERN_CLK_FREQ, 0xa, &ret);
> > > +
> > > +	dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height,
> > > +		 fmt->width);
> > > +
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW,
> > > +		  (v_active + v_fp + v_bp) * h_tot, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp,
> > > +		  &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_V2D,
> > > +		  h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp,
> > > +		  &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret);
> > > +	/* B G R */
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret);
> > > +	/* B G R */
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret);
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int max96717_apply_patgen(struct max96717_priv *priv,
> > > +				 struct v4l2_subdev_state *state)
> > > +{
> > > +	unsigned int val;
> > > +	int ret = 0;
> > > +
> > > +	if (priv->pattern)
> > > +		ret = max96717_apply_patgen_timing(priv, state);
> > > +
> > > +	cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0,
> > > +		  &ret);
> > > +
> > > +	val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern);
> > > +	cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE,
> > > +			val, &ret);
> > > +	return ret;
> > > +}
> > > +
> > > +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct max96717_priv *priv =
> > > +		container_of(ctrl->handler, struct max96717_priv, ctrl_handler);
> > > +	int ret;
> > > +
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_TEST_PATTERN:
> > > +		if (priv->enabled_source_streams)
> > > +			return -EBUSY;
> > > +		priv->pattern = ctrl->val;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Use bpp from bpp register */
> > > +	ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0,
> > > +			      MAX96717_VIDEO_AUTO_BPP,
> > > +			      priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP,
> > > +			      NULL);
> > > +
> > > +	/* Pattern generator doesn't work with tunnel mode */
> > Can you add a comment saying that the deserializer should manage the link in
> > pixel mode as well.
> 
> Something like:
> 	/* Pattern generator doesn't work with tunnel mode.
> 	   Is mandatory to put also the deserializer into pixel mode.   
>         */

Actually I plan to add to add the following comment:

	/*
	 * Pattern generator doesn't work with tunnel mode.
	 * Needs RGB color format and deserializer tunnel mode must be disabled.
	 */
	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
			       MAX96717_TUN_MODE,
			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);

Let me know if this can be ok for you.

Thanks & Regards,
Tommaso

> 
> What do you think?
> 
> Thanks & Regards,
> Tommaso
> 
> > > +	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
> > > +			       MAX96717_TUN_MODE,
> > > +			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);
> > > +}
> > > +
> > > +static const char * const max96717_test_pattern[] = {
> > > +	"Disabled",
> > > +	"Checkerboard",
> > > +	"Gradient"
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_ops max96717_ctrl_ops = {
> > > +	.s_ctrl = max96717_s_ctrl,
> > > +};
> > > +
> > >   static int max96717_gpiochip_get(struct gpio_chip *gpiochip,
> > >   				 unsigned int offset)
> > >   {
> > > @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
> > >   	u64 sink_streams;
> > >   	int ret;
> > > -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> > > -						       MAX96717_PAD_SOURCE,
> > > -						       MAX96717_PAD_SINK,
> > > -						       &streams_mask);
> > > -
> > >   	if (!priv->enabled_source_streams)
> > >   		max96717_start_csi(priv, true);
> > > -	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
> > > -					 sink_streams);
> > > -	if (ret) {
> > > -		dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> > > -			sink_streams);
> > > +	ret = max96717_apply_patgen(priv, state);
> > > +	if (ret)
> > >   		goto stop_csi;
> > > +
> > > +	if (!priv->pattern) {
> > > +		sink_streams =
> > > +			v4l2_subdev_state_xlate_streams(state,
> > > +							MAX96717_PAD_SOURCE,
> > > +							MAX96717_PAD_SINK,
> > > +							&streams_mask);
> > > +
> > > +		ret = v4l2_subdev_enable_streams(priv->source_sd,
> > > +						 priv->source_sd_pad,
> > > +						 sink_streams);
> > > +		if (ret) {
> > > +			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> > > +				sink_streams);
> > > +			goto stop_csi;
> > > +		}
> > >   	}
> > >   	priv->enabled_source_streams |= streams_mask;
> > > @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
> > >   	if (!priv->enabled_source_streams)
> > >   		max96717_start_csi(priv, false);
> > > -	sink_streams = v4l2_subdev_state_xlate_streams(state,
> > > -						       MAX96717_PAD_SOURCE,
> > > -						       MAX96717_PAD_SINK,
> > > -						       &streams_mask);
> > > +	if (!priv->pattern) {
> > > +		int ret;
> > > +
> > > +		sink_streams =
> > > +			v4l2_subdev_state_xlate_streams(state,
> > > +							MAX96717_PAD_SOURCE,
> > > +							MAX96717_PAD_SINK,
> > > +							&streams_mask);
> > > -	return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
> > > -					   sink_streams);
> > > +		ret = v4l2_subdev_disable_streams(priv->source_sd,
> > > +						  priv->source_sd_pad,
> > > +						  sink_streams);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > >   }
> > >   static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
> > > @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv)
> > >   	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops);
> > >   	priv->sd.internal_ops = &max96717_internal_ops;
> > > +	v4l2_ctrl_handler_init(&priv->ctrl_handler, 1);
> > > +	priv->sd.ctrl_handler = &priv->ctrl_handler;
> > > +
> > > +	v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler,
> > > +				     &max96717_ctrl_ops,
> > > +				     V4L2_CID_TEST_PATTERN,
> > > +				     ARRAY_SIZE(max96717_test_pattern) - 1,
> > > +				     0, 0, max96717_test_pattern);
> > > +	if (priv->ctrl_handler.error) {
> > > +		ret = priv->ctrl_handler.error;
> > > +		goto err_free_ctrl;
> > > +	}
> > > +
> > >   	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
> > >   	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> > >   	priv->sd.entity.ops = &max96717_entity_ops;
> > > @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv)
> > >   	v4l2_subdev_cleanup(&priv->sd);
> > >   err_entity_cleanup:
> > >   	media_entity_cleanup(&priv->sd.entity);
> > > +err_free_ctrl:
> > > +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
> > >   	return ret;
> > >   }
> > > @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv)
> > >   	v4l2_async_nf_cleanup(&priv->notifier);
> > >   	v4l2_subdev_cleanup(&priv->sd);
> > >   	media_entity_cleanup(&priv->sd.entity);
> > > +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
> > >   }
> > >   struct max96717_pll_predef_freq {
> > 
> > Regards,
> > Julien
  
Julien Massot June 27, 2024, 12:37 p.m. UTC | #4
Hi Tommaso,

On 6/26/24 10:37 AM, Tommaso Merciai wrote:
> On Tue, Jun 25, 2024 at 07:31:05PM +0200, Tommaso Merciai wrote:
>> On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote:
>>
>> Hi Julien,
>>
>>> Hi Tommaso,
>>>
>>> Thanks for your patch.
>>>
>>> I tested it on my setup and can capture frames. Here's a link to an example:
>>> https://pasteboard.co/j8yHuE4YdYDV.jpg.
>>
>> Nice! Thanks for sharing and testing.
>>
>>>
>>> I had some trouble with link validation because my sensor doesn't support
>>> the RGB format. Once we have internal pad support, we won't need to worry
>>> about the serializer creating an incompatible video stream for the sensor.
>>>
>>> In the future, it would be great if we could support the serializer without
>>> needing a connected sensor. This way, we can use it as a pattern generator
>>> with this patch. However, not all GMSL2 serializers work this way. For
>>> example, the MAX9295A can't generate an internal PCLK and relies on the
>>> sensor to provide the video stream.
>>
>> Fully agree.
>>
>>>
>>> Overall, this patch is very useful as it lets us receive a pattern from the
>>> serializer, which helps validate the GMSL2 connection.
>>>
>>> It also handles issues related to generator timing, bits per pixel (bpp),
>>> and tunnel mode that users might encounter.
>>
>> Fully agree, in my case enabling this test pattern help me a lot on
>> validating the gmsl2 pipe.
>>
>>>
>>> On 6/17/24 4:59 PM, Tommaso Merciai wrote:
>>>> Add v4l2 test pattern control.
>>>>
>>>> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
>>>> ---
>>>>    drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++---
>>>>    1 file changed, 194 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
>>>> index 949306485873..c263bbca7318 100644
>>>> --- a/drivers/media/i2c/max96717.c
>>>> +++ b/drivers/media/i2c/max96717.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include <linux/regmap.h>
>>>>    #include <media/v4l2-cci.h>
>>>> +#include <media/v4l2-ctrls.h>
>>>>    #include <media/v4l2-fwnode.h>
>>>>    #include <media/v4l2-subdev.h>
>>>> @@ -36,11 +37,37 @@
>>>>    #define MAX96717_DEV_ID  CCI_REG8(0xd)
>>>>    #define MAX96717_DEV_REV CCI_REG8(0xe)
>>>>    #define MAX96717_DEV_REV_MASK GENMASK(3, 0)
>>>> +#define MAX96717_IO_CHK0 CCI_REG8(0x24f)
>>> In MAX96717 datasheet this register is named VTX1.
>>> Can you also move these definitions to the VTX section?
>>>> +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1)
>>>>    /* VID_TX Z */
>>>> +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110)
>>>> +#define MAX96717_VIDEO_AUTO_BPP BIT(3)
>>>>    #define MAX96717_VIDEO_TX2 CCI_REG8(0x112)
>>>>    #define MAX96717_VIDEO_PCLKDET BIT(7)
>>>> +/* VRX_PATGEN_0 */
>>> For serializer these registers are named VTX_Z instead of VRX.
>>>
>>>> +#define MAX96717_PATGEN_0              CCI_REG8(0x24e)
>>> #define MAX96717_VTX_0              CCI_REG8(0x24e)
>>>> +#define MAX96717_PATGEN_1              CCI_REG8(0x26b)
>>> Can you keep this define ordered by address?
>>>> +#define MAX96717_PATGEN_MODE           GENMASK(1, 0)
>>>> +#define MAX96717_PATGEN_VS_DLY         CCI_REG24(0x250)
>>> #define MAX96717_VTX_VS_DLY
>>>> +#define MAX96717_PATGEN_VS_HIGH        CCI_REG24(0x253)
>>>> +#define MAX96717_PATGEN_VS_LOW         CCI_REG24(0x256)
>>>> +#define MAX96717_PATGEN_V2H            CCI_REG24(0x259)
>>>> +#define MAX96717_PATGEN_HS_HIGH        CCI_REG16(0x25c)
>>>> +#define MAX96717_PATGEN_HS_LOW         CCI_REG16(0x25e)
>>>> +#define MAX96717_PATGEN_HS_CNT         CCI_REG16(0x260)
>>>> +#define MAX96717_PATGEN_V2D            CCI_REG24(0x262)
>>>> +#define MAX96717_PATGEN_DE_HIGH        CCI_REG16(0x265)
>>>> +#define MAX96717_PATGEN_DE_LOW         CCI_REG16(0x267)
>>>> +#define MAX96717_PATGEN_DE_CNT         CCI_REG16(0x269)
>>>> +#define MAX96717_PATGEN_GRAD_INC       CCI_REG8(0x26c)
>>>> +#define MAX96717_PATGEN_CHKB_COLOR_A   CCI_REG24(0x26d)
>>>> +#define MAX96717_PATGEN_CHKB_COLOR_B   CCI_REG24(0x270)
>>>> +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273)
>>>> +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274)
>>>> +#define MAX96717_PATGEN_CHKB_ALT       CCI_REG8(0x275)
>>>> +
>>
>> So your plan is to move all this stuff here:
>>
>>
>> /* VTX_Z */
>> #define MAX96717_VTX0                  CCI_REG8(0x24e)
>> #define MAX96717_VTX1                  CCI_REG8(0x24f)
>> #define MAX96717_PATTERN_CLK_FREQ      GENMASK(3, 1)
>> #define MAX96717_VTX_VS_DLY            CCI_REG24(0x250)
>> #define MAX96717_VTX_VS_HIGH           CCI_REG24(0x253)
>> #define MAX96717_VTX_VS_LOW            CCI_REG24(0x256)
>> #define MAX96717_VTX_V2H               CCI_REG24(0x259)
>> #define MAX96717_VTX_HS_HIGH           CCI_REG16(0x25c)
>> #define MAX96717_VTX_HS_LOW            CCI_REG16(0x25e)
>> #define MAX96717_VTX_HS_CNT            CCI_REG16(0x260)
>> #define MAX96717_VTX_V2D               CCI_REG24(0x262)
>> #define MAX96717_VTX_DE_HIGH           CCI_REG16(0x265)
>> #define MAX96717_VTX_DE_LOW            CCI_REG16(0x267)
>> #define MAX96717_VTX_DE_CNT            CCI_REG16(0x269)
>> #define MAX96717_VTX29                 CCI_REG8(0x26b)
>> #define MAX96717_VTX_MODE              GENMASK(1, 0)
>> #define MAX96717_VTX_GRAD_INC          CCI_REG8(0x26c)
>> #define MAX96717_VTX_CHKB_COLOR_A      CCI_REG24(0x26d)
>> #define MAX96717_VTX_CHKB_COLOR_B      CCI_REG24(0x270)
>> #define MAX96717_VTX_CHKB_RPT_CNT_A    CCI_REG8(0x273)
>> #define MAX96717_VTX_CHKB_RPT_CNT_B    CCI_REG8(0x274)
>> #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
>>
>> In a fixed order right? :-)
Yes sounds good to me
>>
>>
>>
>>>>    /* GPIO */
>>>>    #define MAX96717_NUM_GPIO         11
>>>>    #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
>>>> @@ -82,6 +109,12 @@
>>>>    /* MISC */
>>>>    #define PIO_SLEW_1 CCI_REG8(0x570)
>>>> +enum max96717_vpg_mode {
>>>> +	MAX96717_VPG_DISABLED = 0,
>>>> +	MAX96717_VPG_CHECKERBOARD = 1,
>>>> +	MAX96717_VPG_GRADIENT = 2,
>>>> +};
>>>> +
>>>>    struct max96717_priv {
>>>>    	struct i2c_client		  *client;
>>>>    	struct regmap			  *regmap;
>>>> @@ -89,6 +122,7 @@ struct max96717_priv {
>>>>    	struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
>>>>    	struct v4l2_subdev                sd;
>>>>    	struct media_pad                  pads[MAX96717_PORTS];
>>>> +	struct v4l2_ctrl_handler          ctrl_handler;
>>>>    	struct v4l2_async_notifier        notifier;
>>>>    	struct v4l2_subdev                *source_sd;
>>>>    	u16                               source_sd_pad;
>>>> @@ -96,6 +130,7 @@ struct max96717_priv {
>>>>    	u8                                pll_predef_index;
>>>>    	struct clk_hw                     clk_hw;
>>>>    	struct gpio_chip                  gpio_chip;
>>>> +	enum max96717_vpg_mode            pattern;
>>>>    };
>>>>    static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
>>>> @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start)
>>>>    			       start ? MAX96717_START_PORT_B : 0, NULL);
>>>>    }
>>>> +static int max96717_apply_patgen_timing(struct max96717_priv *priv,
>>>> +					struct v4l2_subdev_state *state)
>>>> +{
>>>> +	struct v4l2_mbus_framefmt *fmt =
>>>> +		v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE);
>>>> +	const u32 h_active = fmt->width;
>>>> +	const u32 h_fp = 88;
>>>> +	const u32 h_sw = 44;
>>>> +	const u32 h_bp = 148;
>>>> +	u32 h_tot;
>>>> +	const u32 v_active = fmt->height;
>>>> +	const u32 v_fp = 4;
>>>> +	const u32 v_sw = 5;
>>>> +	const u32 v_bp = 36;
>>>> +	u32 v_tot;
>>>> +	int ret = 0;
>>>> +
>>>> +	h_tot = h_active + h_fp + h_sw + h_bp;
>>>> +	v_tot = v_active + v_fp + v_sw + v_bp;
>>>> +
>>>> +	/* 75 Mhz pixel clock */
>>>> +	cci_update_bits(priv->regmap, MAX96717_IO_CHK0,
>>>> +			MAX96717_PATTERN_CLK_FREQ, 0xa, &ret);
>>>> +
>>>> +	dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height,
>>>> +		 fmt->width);
>>>> +
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW,
>>>> +		  (v_active + v_fp + v_bp) * h_tot, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp,
>>>> +		  &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_V2D,
>>>> +		  h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp,
>>>> +		  &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret);
>>>> +	/* B G R */
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret);
>>>> +	/* B G R */
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret);
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int max96717_apply_patgen(struct max96717_priv *priv,
>>>> +				 struct v4l2_subdev_state *state)
>>>> +{
>>>> +	unsigned int val;
>>>> +	int ret = 0;
>>>> +
>>>> +	if (priv->pattern)
>>>> +		ret = max96717_apply_patgen_timing(priv, state);
>>>> +
>>>> +	cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0,
>>>> +		  &ret);
>>>> +
>>>> +	val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern);
>>>> +	cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE,
>>>> +			val, &ret);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl)
>>>> +{
>>>> +	struct max96717_priv *priv =
>>>> +		container_of(ctrl->handler, struct max96717_priv, ctrl_handler);
>>>> +	int ret;
>>>> +
>>>> +	switch (ctrl->id) {
>>>> +	case V4L2_CID_TEST_PATTERN:
>>>> +		if (priv->enabled_source_streams)
>>>> +			return -EBUSY;
>>>> +		priv->pattern = ctrl->val;
>>>> +		break;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* Use bpp from bpp register */
>>>> +	ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0,
>>>> +			      MAX96717_VIDEO_AUTO_BPP,
>>>> +			      priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP,
>>>> +			      NULL);
>>>> +
>>>> +	/* Pattern generator doesn't work with tunnel mode */
>>> Can you add a comment saying that the deserializer should manage the link in
>>> pixel mode as well.
>>
>> Something like:
>> 	/* Pattern generator doesn't work with tunnel mode.
>> 	   Is mandatory to put also the deserializer into pixel mode.
>>          */
> 
> Actually I plan to add to add the following comment:
> 
> 	/*
> 	 * Pattern generator doesn't work with tunnel mode.
> 	 * Needs RGB color format and deserializer tunnel mode must be disabled.
> 	 */
> 	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
> 			       MAX96717_TUN_MODE,
> 			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);
> 
> Let me know if this can be ok for you.
Yes perfect

Regards,
  

Patch

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 949306485873..c263bbca7318 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -16,6 +16,7 @@ 
 #include <linux/regmap.h>
 
 #include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -36,11 +37,37 @@ 
 #define MAX96717_DEV_ID  CCI_REG8(0xd)
 #define MAX96717_DEV_REV CCI_REG8(0xe)
 #define MAX96717_DEV_REV_MASK GENMASK(3, 0)
+#define MAX96717_IO_CHK0 CCI_REG8(0x24f)
+#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1)
 
 /* VID_TX Z */
+#define MAX96717_VIDEO_TX0 CCI_REG8(0x110)
+#define MAX96717_VIDEO_AUTO_BPP BIT(3)
 #define MAX96717_VIDEO_TX2 CCI_REG8(0x112)
 #define MAX96717_VIDEO_PCLKDET BIT(7)
 
+/* VRX_PATGEN_0 */
+#define MAX96717_PATGEN_0              CCI_REG8(0x24e)
+#define MAX96717_PATGEN_1              CCI_REG8(0x26b)
+#define MAX96717_PATGEN_MODE           GENMASK(1, 0)
+#define MAX96717_PATGEN_VS_DLY         CCI_REG24(0x250)
+#define MAX96717_PATGEN_VS_HIGH        CCI_REG24(0x253)
+#define MAX96717_PATGEN_VS_LOW         CCI_REG24(0x256)
+#define MAX96717_PATGEN_V2H            CCI_REG24(0x259)
+#define MAX96717_PATGEN_HS_HIGH        CCI_REG16(0x25c)
+#define MAX96717_PATGEN_HS_LOW         CCI_REG16(0x25e)
+#define MAX96717_PATGEN_HS_CNT         CCI_REG16(0x260)
+#define MAX96717_PATGEN_V2D            CCI_REG24(0x262)
+#define MAX96717_PATGEN_DE_HIGH        CCI_REG16(0x265)
+#define MAX96717_PATGEN_DE_LOW         CCI_REG16(0x267)
+#define MAX96717_PATGEN_DE_CNT         CCI_REG16(0x269)
+#define MAX96717_PATGEN_GRAD_INC       CCI_REG8(0x26c)
+#define MAX96717_PATGEN_CHKB_COLOR_A   CCI_REG24(0x26d)
+#define MAX96717_PATGEN_CHKB_COLOR_B   CCI_REG24(0x270)
+#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273)
+#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274)
+#define MAX96717_PATGEN_CHKB_ALT       CCI_REG8(0x275)
+
 /* GPIO */
 #define MAX96717_NUM_GPIO         11
 #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
@@ -82,6 +109,12 @@ 
 /* MISC */
 #define PIO_SLEW_1 CCI_REG8(0x570)
 
+enum max96717_vpg_mode {
+	MAX96717_VPG_DISABLED = 0,
+	MAX96717_VPG_CHECKERBOARD = 1,
+	MAX96717_VPG_GRADIENT = 2,
+};
+
 struct max96717_priv {
 	struct i2c_client		  *client;
 	struct regmap			  *regmap;
@@ -89,6 +122,7 @@  struct max96717_priv {
 	struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
 	struct v4l2_subdev                sd;
 	struct media_pad                  pads[MAX96717_PORTS];
+	struct v4l2_ctrl_handler          ctrl_handler;
 	struct v4l2_async_notifier        notifier;
 	struct v4l2_subdev                *source_sd;
 	u16                               source_sd_pad;
@@ -96,6 +130,7 @@  struct max96717_priv {
 	u8                                pll_predef_index;
 	struct clk_hw                     clk_hw;
 	struct gpio_chip                  gpio_chip;
+	enum max96717_vpg_mode            pattern;
 };
 
 static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
@@ -131,6 +166,115 @@  static inline int max96717_start_csi(struct max96717_priv *priv, bool start)
 			       start ? MAX96717_START_PORT_B : 0, NULL);
 }
 
+static int max96717_apply_patgen_timing(struct max96717_priv *priv,
+					struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *fmt =
+		v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE);
+	const u32 h_active = fmt->width;
+	const u32 h_fp = 88;
+	const u32 h_sw = 44;
+	const u32 h_bp = 148;
+	u32 h_tot;
+	const u32 v_active = fmt->height;
+	const u32 v_fp = 4;
+	const u32 v_sw = 5;
+	const u32 v_bp = 36;
+	u32 v_tot;
+	int ret = 0;
+
+	h_tot = h_active + h_fp + h_sw + h_bp;
+	v_tot = v_active + v_fp + v_sw + v_bp;
+
+	/* 75 Mhz pixel clock */
+	cci_update_bits(priv->regmap, MAX96717_IO_CHK0,
+			MAX96717_PATTERN_CLK_FREQ, 0xa, &ret);
+
+	dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height,
+		 fmt->width);
+
+	cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW,
+		  (v_active + v_fp + v_bp) * h_tot, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp,
+		  &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_V2D,
+		  h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp,
+		  &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret);
+	/* B G R */
+	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret);
+	/* B G R */
+	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret);
+	cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret);
+
+	return ret;
+}
+
+static int max96717_apply_patgen(struct max96717_priv *priv,
+				 struct v4l2_subdev_state *state)
+{
+	unsigned int val;
+	int ret = 0;
+
+	if (priv->pattern)
+		ret = max96717_apply_patgen_timing(priv, state);
+
+	cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0,
+		  &ret);
+
+	val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern);
+	cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE,
+			val, &ret);
+	return ret;
+}
+
+static int max96717_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct max96717_priv *priv =
+		container_of(ctrl->handler, struct max96717_priv, ctrl_handler);
+	int ret;
+
+	switch (ctrl->id) {
+	case V4L2_CID_TEST_PATTERN:
+		if (priv->enabled_source_streams)
+			return -EBUSY;
+		priv->pattern = ctrl->val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Use bpp from bpp register */
+	ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0,
+			      MAX96717_VIDEO_AUTO_BPP,
+			      priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP,
+			      NULL);
+
+	/* Pattern generator doesn't work with tunnel mode */
+	return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11,
+			       MAX96717_TUN_MODE,
+			       priv->pattern ? 0 : MAX96717_TUN_MODE, &ret);
+}
+
+static const char * const max96717_test_pattern[] = {
+	"Disabled",
+	"Checkerboard",
+	"Gradient"
+};
+
+static const struct v4l2_ctrl_ops max96717_ctrl_ops = {
+	.s_ctrl = max96717_s_ctrl,
+};
+
 static int max96717_gpiochip_get(struct gpio_chip *gpiochip,
 				 unsigned int offset)
 {
@@ -352,20 +496,28 @@  static int max96717_enable_streams(struct v4l2_subdev *sd,
 	u64 sink_streams;
 	int ret;
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state,
-						       MAX96717_PAD_SOURCE,
-						       MAX96717_PAD_SINK,
-						       &streams_mask);
-
 	if (!priv->enabled_source_streams)
 		max96717_start_csi(priv, true);
 
-	ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad,
-					 sink_streams);
-	if (ret) {
-		dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
-			sink_streams);
+	ret = max96717_apply_patgen(priv, state);
+	if (ret)
 		goto stop_csi;
+
+	if (!priv->pattern) {
+		sink_streams =
+			v4l2_subdev_state_xlate_streams(state,
+							MAX96717_PAD_SOURCE,
+							MAX96717_PAD_SINK,
+							&streams_mask);
+
+		ret = v4l2_subdev_enable_streams(priv->source_sd,
+						 priv->source_sd_pad,
+						 sink_streams);
+		if (ret) {
+			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
+				sink_streams);
+			goto stop_csi;
+		}
 	}
 
 	priv->enabled_source_streams |= streams_mask;
@@ -394,13 +546,23 @@  static int max96717_disable_streams(struct v4l2_subdev *sd,
 	if (!priv->enabled_source_streams)
 		max96717_start_csi(priv, false);
 
-	sink_streams = v4l2_subdev_state_xlate_streams(state,
-						       MAX96717_PAD_SOURCE,
-						       MAX96717_PAD_SINK,
-						       &streams_mask);
+	if (!priv->pattern) {
+		int ret;
+
+		sink_streams =
+			v4l2_subdev_state_xlate_streams(state,
+							MAX96717_PAD_SOURCE,
+							MAX96717_PAD_SINK,
+							&streams_mask);
 
-	return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad,
-					   sink_streams);
+		ret = v4l2_subdev_disable_streams(priv->source_sd,
+						  priv->source_sd_pad,
+						  sink_streams);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
@@ -513,6 +675,19 @@  static int max96717_subdev_init(struct max96717_priv *priv)
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops);
 	priv->sd.internal_ops = &max96717_internal_ops;
 
+	v4l2_ctrl_handler_init(&priv->ctrl_handler, 1);
+	priv->sd.ctrl_handler = &priv->ctrl_handler;
+
+	v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler,
+				     &max96717_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(max96717_test_pattern) - 1,
+				     0, 0, max96717_test_pattern);
+	if (priv->ctrl_handler.error) {
+		ret = priv->ctrl_handler.error;
+		goto err_free_ctrl;
+	}
+
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
 	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	priv->sd.entity.ops = &max96717_entity_ops;
@@ -552,6 +727,8 @@  static int max96717_subdev_init(struct max96717_priv *priv)
 	v4l2_subdev_cleanup(&priv->sd);
 err_entity_cleanup:
 	media_entity_cleanup(&priv->sd.entity);
+err_free_ctrl:
+	v4l2_ctrl_handler_free(&priv->ctrl_handler);
 
 	return ret;
 }
@@ -563,6 +740,7 @@  static void max96717_subdev_uninit(struct max96717_priv *priv)
 	v4l2_async_nf_cleanup(&priv->notifier);
 	v4l2_subdev_cleanup(&priv->sd);
 	media_entity_cleanup(&priv->sd.entity);
+	v4l2_ctrl_handler_free(&priv->ctrl_handler);
 }
 
 struct max96717_pll_predef_freq {