mbox series

[00/11] imx290: Minor fixes, support for alternate INCK, and more ctrls

Message ID 20230131192016.3476937-1-dave.stevenson@raspberrypi.com
Headers show
Series imx290: Minor fixes, support for alternate INCK, and more ctrls | expand

Message

Dave Stevenson Jan. 31, 2023, 7:20 p.m. UTC 
Hi All

This is a small patch set that fixes a number of issues, adds in support
for the alternate input clock frequency option, and expands the control support
for flips and VBLANK/HBLANK.

My source tree has the 2 patches I've just sent for mono support first, but I
believe the two series should apply in either order.

  Dave

Dave Stevenson (11):
  media: i2c: imx290: Match kernel coding style on whitespace
  media: i2c: imx290: Set the colorspace fields in the format
  media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
  media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
  media: i2c: imx290: Support 60fps in 2 lane operation
  media: i2c: imx290: Use CSI timings as per datasheet
  media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
  media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
  media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
  media: i2c: imx290: Add support for 74.25MHz external clock
  media: i2c: imx290: Add support for H & V Flips

 drivers/media/i2c/imx290.c | 429 ++++++++++++++++++++++++++++---------
 1 file changed, 333 insertions(+), 96 deletions(-)

Comments

Laurent Pinchart Feb. 2, 2023, 12:21 a.m. UTC | #1 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:06PM +0000, Dave Stevenson wrote:
> Fix up a couple of coding style issues regarding missing blank
> lines after declarations, double blank lines, and incorrect
> indentation.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a370f1102334..88c7201510a2 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -106,7 +106,6 @@
>  
>  #define IMX290_VMAX_DEFAULT				1125
>  
> -
>  /*
>   * The IMX290 pixel array is organized as follows:
>   *
> @@ -335,6 +334,7 @@ static const s64 imx290_link_freq_2lanes[] = {
>  	[FREQ_INDEX_1080P] = 445500000,
>  	[FREQ_INDEX_720P] = 297000000,
>  };
> +
>  static const s64 imx290_link_freq_4lanes[] = {
>  	[FREQ_INDEX_1080P] = 222750000,
>  	[FREQ_INDEX_720P] = 148500000,
> @@ -465,7 +465,7 @@ static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *val
>  			      data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "%u-bit read from 0x%04x failed: %d\n",
> -			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
> +			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
>  			 addr & IMX290_REG_ADDR_MASK, ret);
>  		return ret;
>  	}
> @@ -486,7 +486,7 @@ static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
>  			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
> -			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
> +			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
>  			 addr & IMX290_REG_ADDR_MASK, ret);
>  		if (err)
>  			*err = ret;
> @@ -752,8 +752,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  
>  	/* Set init register settings */
>  	ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> -					ARRAY_SIZE(
> -						imx290_global_init_settings));
> +					ARRAY_SIZE(imx290_global_init_settings));
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not set init registers\n");
>  		return ret;
Laurent Pinchart Feb. 2, 2023, 12:29 a.m. UTC | #2 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:07PM +0000, Dave Stevenson wrote:
> The colorspace fields were left untouched in imx290_set_fmt
> which lead to a v4l2-compliance failure.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 88c7201510a2..bf96fd914303 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -897,6 +897,14 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  		fmt->format.code = imx290_formats[0].code[imx290->mono];
>  
>  	fmt->format.field = V4L2_FIELD_NONE;
> +	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->format.ycbcr_enc =
> +		V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace);
> +	fmt->format.quantization =
> +		V4L2_MAP_QUANTIZATION_DEFAULT(true, fmt->format.colorspace,
> +					      fmt->format.ycbcr_enc);
> +	fmt->format.xfer_func =
> +		V4L2_MAP_XFER_FUNC_DEFAULT(fmt->format.colorspace);

Given that all of these are hardcoded, I think it would be more readable
to write

	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
	fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;

>  	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
Laurent Pinchart Feb. 2, 2023, 8:13 a.m. UTC | #3 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:09PM +0000, Dave Stevenson wrote:
> The datasheet lists the link frequency changes between
> 1080p and 720p modes. This is correct that the link frequency
> changes as measured on an oscilloscope.
> 
> Link frequency is not necessarily the same as pixel rate.

That's right. The model where

	htotal * vtotal * frame rate = pixel rate

applies to the pixel array. Things can be retimed when transmitted over
CSI-2 (or over anything, really), with pixels transmitted at a higher or
lower rate, with smaller or larger blanking, as long as it matches the
total horizontal line length in time units. I suppose this could in
theory be done vertically too, but that would require too much memory.

I hope we'll never have a need, on the receiver side, to know the active
line length in time units as sent over CSI-2 :-)

> The datasheet gives standard configurations for 1080p and 720p
> modes at a number of frame rates.
> Looking at the 1080p mode it gives:
> HMAX = 0x898 = 2200
> VMAX = 0x465 = 1125
> 2200 * 1125 * 60fps = 148.5MPix/s
> 
> Looking at the 720p mode it gives:
> HMAX = 0xce4 = 3300
> VMAX = 0x2ee = 750
> 3300 * 750 * 60fps = 148.5Mpix/s
> 
> This driver currently scales the pixel rate proportionally to the
> link frequency, however the above shows that this is not the
> correct thing to do, and currently all frame rate and exposure
> calculations give incorrect results.
> 
> Correctly report the pixel rate as being 148.5MPix/s under any
> mode.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 12946ca9d8d2..bd8729aed43c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -107,6 +107,7 @@
>  
>  #define IMX290_VMAX_DEFAULT				1125
>  
> +#define IMX290_PIXEL_RATE				148500000

A blank line is missing here. With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  /*
>   * The IMX290 pixel array is organized as follows:
>   *
> @@ -190,7 +191,6 @@ struct imx290 {
>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *link_freq;
> -	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
>  };
> @@ -649,15 +649,8 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  {
>  	unsigned int hblank = mode->hmax - mode->width;
>  	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> -	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
> -	u64 pixel_rate;
> -
> -	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -	pixel_rate = link_freq * 2 * imx290->nlanes;
> -	do_div(pixel_rate, imx290_format_info(format->code)->bpp);
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> -	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
>  
>  	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> @@ -707,9 +700,9 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	if (imx290->link_freq)
>  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> -	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> -					       V4L2_CID_PIXEL_RATE,
> -					       1, INT_MAX, 1, 1);
> +	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  IMX290_PIXEL_RATE, IMX290_PIXEL_RATE, 1,
> +			  IMX290_PIXEL_RATE);
>  
>  	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
>  				     V4L2_CID_TEST_PATTERN,
Laurent Pinchart Feb. 2, 2023, 2:58 p.m. UTC | #4 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote:
> The driver exposed V4L2_CID_HBLANK as a read only control to allow
> for exposure calculations and determination of the frame rate.
> 
> Convert to a read/write control so that the frame rate can be
> controlled.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 9ddd6382b127..9006be6e5e7c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -47,6 +47,7 @@
>  #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
>  #define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
>  #define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
> +#define IMX290_HMAX_MAX					0xffff
>  #define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
>  #define IMX290_WINWV_OB					IMX290_REG_8BIT(0x303a)
>  #define IMX290_WINPV					IMX290_REG_16BIT(0x303c)
> @@ -167,7 +168,7 @@ struct imx290_regval {
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> -	u32 hmax;
> +	u32 hmax_min;
>  	u8 link_freq_index;
>  
>  	const struct imx290_regval *data;
> @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,

I didn't find in the documentation a mention that these values are the
minimum the sensor supports. Did you use them as such based on the fact
that anything higher than the nominal hblank value reported by the
datasheet is hopefully guaranteed to work, and lower values are
uncharted territory, or are these the real minimums ? I'm fine using
them as minimum for now even if we could possibly go lower, leaving that
for future patches. I would however mention this in a comment or in the
commit message.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  	{
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  	{
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  		}
>  		break;
>  
> +	case V4L2_CID_HBLANK:
> +		ret = imx290_write(imx290, IMX290_HMAX,
> +				   ctrl->val + imx290->current_mode->width,
> +				   NULL);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  			       const struct v4l2_mbus_framefmt *format,
>  			       const struct imx290_mode *mode)
>  {
> -	unsigned int hblank = mode->hmax - mode->width;
> +	unsigned int hblank_min = mode->hmax_min - mode->width;
> +	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
>  	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
>  
> -	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> +	__v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> +				 hblank_min);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
>  }
>  
> @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
>  				     0, 0, imx290_test_pattern_menu);
>  
> +	/*
> +	 * Actual range will be set from imx290_ctrl_update later in the probe.
> +	 */
>  	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_HBLANK, 1, 1, 1, 1);
> -	if (imx290->hblank)
> -		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
>  	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_VBLANK, 1, 1, 1, 1);
> @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  		return ret;
>  	}
>  
> -	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> -			   NULL);
> -	if (ret)
> -		return ret;
> -
>  	/* Apply customized values from user */
>  	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
Dave Stevenson Feb. 2, 2023, 3:48 p.m. UTC | #5 
Hi Laurent

On Thu, 2 Feb 2023 at 14:58, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
> On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote:
> > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > for exposure calculations and determination of the frame rate.
> >
> > Convert to a read/write control so that the frame rate can be
> > controlled.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 9ddd6382b127..9006be6e5e7c 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -47,6 +47,7 @@
> >  #define IMX290_GAIN                                  IMX290_REG_8BIT(0x3014)
> >  #define IMX290_VMAX                                  IMX290_REG_24BIT(0x3018)
> >  #define IMX290_HMAX                                  IMX290_REG_16BIT(0x301c)
> > +#define IMX290_HMAX_MAX                                      0xffff
> >  #define IMX290_SHS1                                  IMX290_REG_24BIT(0x3020)
> >  #define IMX290_WINWV_OB                                      IMX290_REG_8BIT(0x303a)
> >  #define IMX290_WINPV                                 IMX290_REG_16BIT(0x303c)
> > @@ -167,7 +168,7 @@ struct imx290_regval {
> >  struct imx290_mode {
> >       u32 width;
> >       u32 height;
> > -     u32 hmax;
> > +     u32 hmax_min;
> >       u8 link_freq_index;
> >
> >       const struct imx290_regval *data;
> > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >       {
> >               .width = 1920,
> >               .height = 1080,
> > -             .hmax = 2200,
> > +             .hmax_min = 2200,
>
> I didn't find in the documentation a mention that these values are the
> minimum the sensor supports. Did you use them as such based on the fact
> that anything higher than the nominal hblank value reported by the
> datasheet is hopefully guaranteed to work, and lower values are
> uncharted territory, or are these the real minimums ? I'm fine using
> them as minimum for now even if we could possibly go lower, leaving that
> for future patches. I would however mention this in a comment or in the
> commit message.

The datasheet shows varying HMAX to control frame rate.
Table "List of Setting Register for CSI-2 serial output" for all-pixel
mode gives HMAX values of :
0x14a0 for 25fps
0x1130 for 30fps
0x0a50 for 50fps
0x0898 for 60 fps
0x0528 for 100fps (needs FRSEL = 0 and higher link frequency)
0x044c for 120fps (ditto)

VMAX is fixed at 0x465 (1125 decimal) for all frame rates in all-pixel
(1080p) mode.

The 2200 listed here is the 0x898 for 60fps - the nominal max with the
configured lane / link frequency combinations.

Doing the maths, 4 lanes at 445.5Mbit/s/lane = 1782Mbit/s.
2200 * 1125 * 12bpp * 60fps = 1782Mbit/s. Pixel clock is therefore the
same as link bandwidth at this point. I have no information about
blanking and clock lane behaviour for this sensor, but can believe
that it needs that time for low level CSI2 transitions.

AIUI you'll only be able to decrease this further if you add support
for 891Mbit/s/lane (445.5MHz link freq) on 4 lanes, and drop to 10 bit
readout. Programming HMAX also then becomes more entertaining as it
appears to be half the expected value (0x44c = 1100 decimal, which is
smaller than the width), so it'll need a fair amount of messing to get
all the controls to behave as expected.
That's all outside the scope of this patch set - 60fps was the only
frame rate previously supported, and we've expanded on that with
slower frame rate support.


FWIW I have checked with Sony over the 120fps modes, and experimented a little.
Sony will only support 120fps in 10bit. The flyer for IMX462 that
implies it does 1920x1080@120fps in 12 bit mode is incorrect.
Experimentation had 720p120 working in 12bit, but trying to work
through limiting the various bit depth and resolution options is going
to get really ugly in the driver.

  Dave

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >       {
> >               .width = 1280,
> >               .height = 720,
> > -             .hmax = 3300,
> > +             .hmax_min = 3300,
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >       {
> >               .width = 1920,
> >               .height = 1080,
> > -             .hmax = 2200,
> > +             .hmax_min = 2200,
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >       {
> >               .width = 1280,
> >               .height = 720,
> > -             .hmax = 3300,
> > +             .hmax_min = 3300,
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >               }
> >               break;
> >
> > +     case V4L2_CID_HBLANK:
> > +             ret = imx290_write(imx290, IMX290_HMAX,
> > +                                ctrl->val + imx290->current_mode->width,
> > +                                NULL);
> > +             break;
> > +
> >       default:
> >               ret = -EINVAL;
> >               break;
> > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> >                              const struct v4l2_mbus_framefmt *format,
> >                              const struct imx290_mode *mode)
> >  {
> > -     unsigned int hblank = mode->hmax - mode->width;
> > +     unsigned int hblank_min = mode->hmax_min - mode->width;
> > +     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> >       unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> >
> >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> >
> > -     __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> > +     __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> > +                              hblank_min);
> >       __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> >  }
> >
> > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >                                    ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> >                                    0, 0, imx290_test_pattern_menu);
> >
> > +     /*
> > +      * Actual range will be set from imx290_ctrl_update later in the probe.
> > +      */
> >       imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >                                          V4L2_CID_HBLANK, 1, 1, 1, 1);
> > -     if (imx290->hblank)
> > -             imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >                                          V4L2_CID_VBLANK, 1, 1, 1, 1);
> > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290,
> >               return ret;
> >       }
> >
> > -     ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> > -                        NULL);
> > -     if (ret)
> > -             return ret;
> > -
> >       /* Apply customized values from user */
> >       ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> >       if (ret) {
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Feb. 2, 2023, 4:04 p.m. UTC | #6 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:14PM +0000, Dave Stevenson wrote:
> IMX290_CTRL_07 was written from both imx290_global_init_settings
> and imx290_1080p_settings and imx290_720p_settings.
> 
> Remove it from imx290_global_init_settings as the setting varies
> based on the mode.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 3413d83369ba..5202ef3cc3e6 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -219,7 +219,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
>   */
>  
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
>  	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },
Laurent Pinchart Feb. 2, 2023, 10:03 p.m. UTC | #7 
Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:15PM +0000, Dave Stevenson wrote:
> The sensor supports either a 37.125 or 74.25MHz external, but
> the driver only supported 37.125MHz.
> 
> Add the relevant register configuration for either clock
> frequency option.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
>  1 file changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 5202ef3cc3e6..7f6746f74040 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -102,6 +102,7 @@
>  #define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
>  #define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
>  #define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
> +#define IMX290_INCKSEL7					IMX290_REG_8BIT(0x3480)
>  
>  #define IMX290_PGCTRL_REGEN				BIT(0)
>  #define IMX290_PGCTRL_THRU				BIT(1)
> @@ -159,11 +160,27 @@
>  
>  #define IMX290_NUM_SUPPLIES				3
>  
> +#define CLK_37_125	0
> +#define CLK_74_25	1
> +#define NUM_CLK		2

Please add an IMX290 prefer to avoid future namespace clashes.

> +
>  struct imx290_regval {
>  	u32 reg;
>  	u32 val;
>  };
>  
> +/*
> + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> + */
> +struct imx290_clk_cfg {
> +	u8 incksel1;
> +	u8 incksel2;
> +	u8 incksel3;
> +	u8 incksel4;
> +	u8 incksel5;
> +	u8 incksel6;
> +};
> +
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> @@ -173,6 +190,8 @@ struct imx290_mode {
>  
>  	const struct imx290_regval *data;
>  	u32 data_size;
> +
> +	const struct imx290_clk_cfg *clk_cfg;
>  };
>  
>  struct imx290_csi_cfg {
> @@ -191,6 +210,7 @@ struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
>  	struct regmap *regmap;
> +	u32 xclk_freq;
>  	u8 nlanes;
>  	u8 mono;
>  
> @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
>   */
>  
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },
>  	{ IMX290_WINPV, 0 },
> @@ -269,7 +288,16 @@ static const struct imx290_regval imx290_global_init_settings[] = {
>  	{ IMX290_REG_8BIT(0x33b0), 0x50 },
>  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
>  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },

One less unnamed register, only 42 to go :-D

> +};
> +
> +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> +	{ IMX290_EXTCK_FREQ, 0x2520 },
> +	{ IMX290_INCKSEL7, 0x49 },
> +};
> +
> +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> +	{ IMX290_EXTCK_FREQ, 0x4a40 },
> +	{ IMX290_INCKSEL7, 0x92 },
>  };

Those two arrays are not used, which I assume is not normal :-) A rebase
problem maybe ?

How about moving the INCKSEL7 value to the imx290_clk_cfg structure for
consistency ?

>  
>  static const struct imx290_regval imx290_1080p_settings[] = {
> @@ -279,12 +307,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
>  	{ IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
>  	{ IMX290_Y_OUT_SIZE, 1080 },
> -	{ IMX290_INCKSEL1, 0x18 },
> -	{ IMX290_INCKSEL2, 0x03 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
>  
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -294,12 +316,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
>  	{ IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
>  	{ IMX290_Y_OUT_SIZE, 720 },
> -	{ IMX290_INCKSEL1, 0x20 },
> -	{ IMX290_INCKSEL2, 0x00 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
>  
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct imx290 *imx290)
>  		return ARRAY_SIZE(imx290_link_freq_4lanes);
>  }
>  
> +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> +	[CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x18,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},

As the incksel[0-6] fields are only used in one place, to write all 6 of
them to the device, you could also drop the imx290_clk_cfg structure and
turn this into a imx290_regval array. Entirely up to you.

> +	[CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x0c,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
> +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> +	[CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x20,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x10,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
>  /* Mode configs */
>  static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
> @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
>  
> @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
>  
> @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290 *imx290,
>  	return 0;
>  }
>  
> +static int imx290_set_clock(struct imx290 *imx290)
> +{
> +	int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> +	const struct imx290_mode *mode = imx290->current_mode;
> +	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> +	int ret = 0;

How about turning the clock freq macros into an enum:

enum imx290_clk_freq {
	IMX290_CLK_37_125 = 0,
	IMX290_CLK_74_25 = 1,
};

and replacing in struct imx290

-	u32 xclk_freq;
+	enum imx290_clk_freq xclk_freq;

? Then you could could simply write

	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[imx290->xclk_freq];

(you could also name the field xclk_freq_idx if desired). Up to you, if
you find that messier you can ignore the comment.

> +
> +	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> +
> +	return ret;
> +}
> +
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
>  	int ret = 0;
> @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  		return ret;
>  	}
>  
> +	/* Set clock parameters based on mode and xclk */
> +	ret = imx290_set_clock(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set clocks\n");
> +		return ret;
> +	}
> +
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
>  	int ret;
>  
>  	ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> -				       "clock-frequency", &xclk_freq);
> +				       "clock-frequency", &imx290->xclk_freq);
>  	if (ret) {
>  		dev_err(imx290->dev, "Could not get xclk frequency\n");
>  		return ret;
>  	}
>  
> -	/* external clock must be 37.125 MHz */
> -	if (xclk_freq != 37125000) {
> +	/* external clock must be 37.125 MHz or 74.25MHz */
> +	if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000) {
>  		dev_err(imx290->dev, "External clock frequency %u is not supported\n",
>  			xclk_freq);
>  		return -EINVAL;
Laurent Pinchart Feb. 2, 2023, 10:14 p.m. UTC | #8 
Hi Dave,

On Thu, Feb 02, 2023 at 03:48:19PM +0000, Dave Stevenson wrote:
> On Thu, 2 Feb 2023 at 14:58, Laurent Pinchart wrote:
> > On Tue, Jan 31, 2023 at 07:20:12PM +0000, Dave Stevenson wrote:
> > > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > > for exposure calculations and determination of the frame rate.
> > >
> > > Convert to a read/write control so that the frame rate can be
> > > controlled.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
> > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 9ddd6382b127..9006be6e5e7c 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -47,6 +47,7 @@
> > >  #define IMX290_GAIN                                  IMX290_REG_8BIT(0x3014)
> > >  #define IMX290_VMAX                                  IMX290_REG_24BIT(0x3018)
> > >  #define IMX290_HMAX                                  IMX290_REG_16BIT(0x301c)
> > > +#define IMX290_HMAX_MAX                                      0xffff
> > >  #define IMX290_SHS1                                  IMX290_REG_24BIT(0x3020)
> > >  #define IMX290_WINWV_OB                                      IMX290_REG_8BIT(0x303a)
> > >  #define IMX290_WINPV                                 IMX290_REG_16BIT(0x303c)
> > > @@ -167,7 +168,7 @@ struct imx290_regval {
> > >  struct imx290_mode {
> > >       u32 width;
> > >       u32 height;
> > > -     u32 hmax;
> > > +     u32 hmax_min;
> > >       u8 link_freq_index;
> > >
> > >       const struct imx290_regval *data;
> > > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >       {
> > >               .width = 1920,
> > >               .height = 1080,
> > > -             .hmax = 2200,
> > > +             .hmax_min = 2200,
> >
> > I didn't find in the documentation a mention that these values are the
> > minimum the sensor supports. Did you use them as such based on the fact
> > that anything higher than the nominal hblank value reported by the
> > datasheet is hopefully guaranteed to work, and lower values are
> > uncharted territory, or are these the real minimums ? I'm fine using
> > them as minimum for now even if we could possibly go lower, leaving that
> > for future patches. I would however mention this in a comment or in the
> > commit message.
> 
> The datasheet shows varying HMAX to control frame rate.
> Table "List of Setting Register for CSI-2 serial output" for all-pixel
> mode gives HMAX values of :
> 0x14a0 for 25fps
> 0x1130 for 30fps
> 0x0a50 for 50fps
> 0x0898 for 60 fps
> 0x0528 for 100fps (needs FRSEL = 0 and higher link frequency)
> 0x044c for 120fps (ditto)
> 
> VMAX is fixed at 0x465 (1125 decimal) for all frame rates in all-pixel
> (1080p) mode.
> 
> The 2200 listed here is the 0x898 for 60fps - the nominal max with the
> configured lane / link frequency combinations.
> 
> Doing the maths, 4 lanes at 445.5Mbit/s/lane = 1782Mbit/s.
> 2200 * 1125 * 12bpp * 60fps = 1782Mbit/s. Pixel clock is therefore the
> same as link bandwidth at this point. I have no information about
> blanking and clock lane behaviour for this sensor, but can believe
> that it needs that time for low level CSI2 transitions.
> 
> AIUI you'll only be able to decrease this further if you add support
> for 891Mbit/s/lane (445.5MHz link freq) on 4 lanes, and drop to 10 bit
> readout. Programming HMAX also then becomes more entertaining as it
> appears to be half the expected value (0x44c = 1100 decimal, which is
> smaller than the width), so it'll need a fair amount of messing to get
> all the controls to behave as expected.
> That's all outside the scope of this patch set - 60fps was the only
> frame rate previously supported, and we've expanded on that with
> slower frame rate support.

I'm totally fine with that. Thanks for the explanation.

> FWIW I have checked with Sony over the 120fps modes, and experimented a little.
> Sony will only support 120fps in 10bit. The flyer for IMX462 that
> implies it does 1920x1080@120fps in 12 bit mode is incorrect.
> Experimentation had 720p120 working in 12bit, but trying to work
> through limiting the various bit depth and resolution options is going
> to get really ugly in the driver.

Let's enjoy the beauty of your patch set today and leave the ugliness
for tomorrow :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >       {
> > >               .width = 1280,
> > >               .height = 720,
> > > -             .hmax = 3300,
> > > +             .hmax_min = 3300,
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >       {
> > >               .width = 1920,
> > >               .height = 1080,
> > > -             .hmax = 2200,
> > > +             .hmax_min = 2200,
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >       {
> > >               .width = 1280,
> > >               .height = 720,
> > > -             .hmax = 3300,
> > > +             .hmax_min = 3300,
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > >               }
> > >               break;
> > >
> > > +     case V4L2_CID_HBLANK:
> > > +             ret = imx290_write(imx290, IMX290_HMAX,
> > > +                                ctrl->val + imx290->current_mode->width,
> > > +                                NULL);
> > > +             break;
> > > +
> > >       default:
> > >               ret = -EINVAL;
> > >               break;
> > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> > >                              const struct v4l2_mbus_framefmt *format,
> > >                              const struct imx290_mode *mode)
> > >  {
> > > -     unsigned int hblank = mode->hmax - mode->width;
> > > +     unsigned int hblank_min = mode->hmax_min - mode->width;
> > > +     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > >       unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> > >
> > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > >
> > > -     __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> > > +     __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> > > +                              hblank_min);
> > >       __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> > >  }
> > >
> > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > >                                    ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> > >                                    0, 0, imx290_test_pattern_menu);
> > >
> > > +     /*
> > > +      * Actual range will be set from imx290_ctrl_update later in the probe.
> > > +      */
> > >       imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > >                                          V4L2_CID_HBLANK, 1, 1, 1, 1);
> > > -     if (imx290->hblank)
> > > -             imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > >                                          V4L2_CID_VBLANK, 1, 1, 1, 1);
> > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > >               return ret;
> > >       }
> > >
> > > -     ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> > > -                        NULL);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > >       /* Apply customized values from user */
> > >       ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> > >       if (ret) {
Laurent Pinchart Feb. 2, 2023, 10:16 p.m. UTC | #9 
Hi Dave,

On Tue, Jan 31, 2023 at 07:20:05PM +0000, Dave Stevenson wrote:
> Hi All
> 
> This is a small patch set that fixes a number of issues, adds in support
> for the alternate input clock frequency option, and expands the control support
> for flips and VBLANK/HBLANK.
> 
> My source tree has the 2 patches I've just sent for mono support first, but I
> believe the two series should apply in either order.

Thank you for taking the time to upstream these, much appreciated. I'll
give the series a try with my IMX327, but I'm afraid it will have to
wait until next week after I come back from the FOSDEM.

> Dave Stevenson (11):
>   media: i2c: imx290: Match kernel coding style on whitespace
>   media: i2c: imx290: Set the colorspace fields in the format
>   media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
>   media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
>   media: i2c: imx290: Support 60fps in 2 lane operation
>   media: i2c: imx290: Use CSI timings as per datasheet
>   media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
>   media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
>   media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
>   media: i2c: imx290: Add support for 74.25MHz external clock
>   media: i2c: imx290: Add support for H & V Flips
> 
>  drivers/media/i2c/imx290.c | 429 ++++++++++++++++++++++++++++---------
>  1 file changed, 333 insertions(+), 96 deletions(-)
Alexander Stein Feb. 3, 2023, 7:16 a.m. UTC | #10 
Am Donnerstag, 2. Februar 2023, 18:42:43 CET schrieb Dave Stevenson:
> On Thu, 2 Feb 2023 at 16:04, Dave Stevenson
> 
> <dave.stevenson@raspberrypi.com> wrote:
> > Hi Alexander
> > 
> > On Thu, 2 Feb 2023 at 15:40, Alexander Stein
> > 
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dave,
> > > 
> > > thanks for working on this.
> > > 
> > > Am Dienstag, 31. Januar 2023, 20:20:13 CET schrieb Dave Stevenson:
> > > > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > > > for exposure calculations and determination of the frame rate.
> > > > 
> > > > Convert to a read/write control so that the frame rate can be
> > > > controlled.
> > > > V4L2_CID_VBLANK also sets the limits for the exposure control,
> > > > therefore exposure ranges have to be updated when vblank changes.
> > > > 
> > > > This also updates the default VMAX in 720p mode from 1125 to
> > > > 750 to achieve 60fps and follow the datasheet.
> > > > 
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > > 
> > > >  drivers/media/i2c/imx290.c | 57
> > > >  ++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 45 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 9006be6e5e7c..3413d83369ba 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -46,6 +46,7 @@
> > > > 
> > > >  #define IMX290_BLKLEVEL
> > > 
> > > IMX290_REG_16BIT(0x300a)
> > > 
> > > >  #define IMX290_GAIN
> > > 
> > > IMX290_REG_8BIT(0x3014)
> > > 
> > > >  #define IMX290_VMAX
> > > 
> > > IMX290_REG_24BIT(0x3018)
> > > 
> > > > +#define IMX290_VMAX_MAX                                      0x3ffff
> > > > 
> > > >  #define IMX290_HMAX
> > > 
> > > IMX290_REG_16BIT(0x301c)
> > > 
> > > >  #define IMX290_HMAX_MAX                                      0xffff
> > > >  #define IMX290_SHS1
> > > 
> > > IMX290_REG_24BIT(0x3020)
> > > 
> > > > @@ -106,8 +107,6 @@
> > > > 
> > > >  #define IMX290_PGCTRL_THRU                           BIT(1)
> > > >  #define IMX290_PGCTRL_MODE(n)                                ((n) <<
> > > 
> > > 4)
> > > 
> > > > -#define IMX290_VMAX_DEFAULT                          1125
> > > > -
> > > > 
> > > >  #define IMX290_PIXEL_RATE                            148500000
> > > >  /*
> > > >  
> > > >   * The IMX290 pixel array is organized as follows:
> > > > @@ -169,6 +168,7 @@ struct imx290_mode {
> > > > 
> > > >       u32 width;
> > > >       u32 height;
> > > >       u32 hmax_min;
> > > > 
> > > > +     u32 vmax_min;
> > > > 
> > > >       u8 link_freq_index;
> > > >       
> > > >       const struct imx290_regval *data;
> > > > 
> > > > @@ -206,6 +206,7 @@ struct imx290 {
> > > > 
> > > >       struct v4l2_ctrl *link_freq;
> > > >       struct v4l2_ctrl *hblank;
> > > >       struct v4l2_ctrl *vblank;
> > > > 
> > > > +     struct v4l2_ctrl *exposure;
> > > > 
> > > >  };
> > > >  
> > > >  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > > > 
> > > > @@ -219,7 +220,6 @@ static inline struct imx290 *to_imx290(struct
> > > > v4l2_subdev *_sd)
> > > > 
> > > >  static const struct imx290_regval imx290_global_init_settings[] = {
> > > >  
> > > >       { IMX290_CTRL_07, IMX290_WINMODE_1080P },
> > > > 
> > > > -     { IMX290_VMAX, IMX290_VMAX_DEFAULT },
> > > > 
> > > >       { IMX290_EXTCK_FREQ, 0x2520 },
> > > >       { IMX290_WINWV_OB, 12 },
> > > >       { IMX290_WINPH, 0 },
> > > > 
> > > > @@ -412,6 +412,7 @@ static const struct imx290_mode
> > > > imx290_modes_2lanes[] =
> > > > { .width = 1920,
> > > > 
> > > >               .height = 1080,
> > > >               .hmax_min = 2200,
> > > > 
> > > > +             .vmax_min = 1125,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_1080P,
> > > >               .data = imx290_1080p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > > 
> > > > @@ -420,6 +421,7 @@ static const struct imx290_mode
> > > > imx290_modes_2lanes[] =
> > > > { .width = 1280,
> > > > 
> > > >               .height = 720,
> > > >               .hmax_min = 3300,
> > > > 
> > > > +             .vmax_min = 750,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_720P,
> > > >               .data = imx290_720p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > > 
> > > > @@ -431,6 +433,7 @@ static const struct imx290_mode
> > > > imx290_modes_4lanes[] =
> > > > { .width = 1920,
> > > > 
> > > >               .height = 1080,
> > > >               .hmax_min = 2200,
> > > > 
> > > > +             .vmax_min = 1125,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_1080P,
> > > >               .data = imx290_1080p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > > 
> > > > @@ -439,6 +442,7 @@ static const struct imx290_mode
> > > > imx290_modes_4lanes[] =
> > > > { .width = 1280,
> > > > 
> > > >               .height = 720,
> > > >               .hmax_min = 3300,
> > > > 
> > > > +             .vmax_min = 750,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_720P,
> > > >               .data = imx290_720p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > > 
> > > > @@ -645,7 +649,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > 
> > > >                                            struct imx290, ctrls);
> > > >       
> > > >       const struct v4l2_mbus_framefmt *format;
> > > >       struct v4l2_subdev_state *state;
> > > > 
> > > > -     int ret = 0;
> > > > +     int ret = 0, vmax;
> > > > 
> > > >       /*
> > > >       
> > > >        * Return immediately for controls that don't need to be applied
> > > >        to
> > > 
> > > the
> > > 
> > > > @@ -654,6 +658,18 @@ static int imx290_set_ctrl(struct v4l2_ctrl
> > > > *ctrl)
> > > > 
> > > >       if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> > > >       
> > > >               return 0;
> > > > 
> > > > +     if (ctrl->id == V4L2_CID_VBLANK) {
> > > > +             u32 vmax = ctrl->val + imx290->current_mode->height;
> > > > +
> > > > +             /*
> > > > +              * Changing vblank changes the allowed range for
> > > > exposure.
> > > > +              * We don't supply the current exposure as default here
> > > > as
> > > 
> > > it
> > > 
> > > > +              * may lie outside the new range. We will reset it just
> > > 
> > > below.
> > > 
> > > > +              */
> > > > +             __v4l2_ctrl_modify_range(imx290->exposure,
> > > > +                                      1, vmax - 2, 1, vmax - 2);
> > > > +     }
> > > > +
> > > > 
> > > >       /* V4L2 controls values will be applied only when power is
> > > >       already
> > > 
> > > up */
> > > 
> > > >       if (!pm_runtime_get_if_in_use(imx290->dev))
> > > >       
> > > >               return 0;
> > > > 
> > > > @@ -666,9 +682,23 @@ static int imx290_set_ctrl(struct v4l2_ctrl
> > > > *ctrl)
> > > > 
> > > >               ret = imx290_write(imx290, IMX290_GAIN, ctrl->val,
> > > >               NULL);
> > > >               break;
> > > > 
> > > > +     case V4L2_CID_VBLANK:
> > > > +             ret = imx290_write(imx290, IMX290_VMAX,
> > > > +                                ctrl->val + imx290->current_mode-
> > > >
> > > >height,
> > > >
> > > > +                                NULL);
> > > > +             /*
> > > > +              * Due to the way that exposure is programmed in this
> > > 
> > > sensor in
> > > 
> > > > +              * relation to VMAX, we have to reprogramme it whenever
> > > 
> > > VMAX is
> > > 
> > > > +              * changed.
> > > > +              * Update ctrl so that the V4L2_CID_EXPOSURE case can
> > > 
> > > refer to
> > > 
> > > > +              * it.
> > > > +              */
> > > > +             ctrl = imx290->exposure;
> > > > +             fallthrough;
> > > > 
> > > >       case V4L2_CID_EXPOSURE:
> > > > +             vmax = imx290->vblank->val +
> > > > imx290->current_mode->height;
> > > > 
> > > >               ret = imx290_write(imx290, IMX290_SHS1,
> > > > 
> > > > -                                IMX290_VMAX_DEFAULT - ctrl->val -
> > > 
> > > 1, NULL);
> > > 
> > > > +                                vmax - ctrl->val - 1, NULL);
> > > > 
> > > >               break;
> > > >       
> > > >       case V4L2_CID_TEST_PATTERN:
> > > > @@ -725,13 +755,15 @@ static void imx290_ctrl_update(struct imx290
> > > > *imx290,
> > > > 
> > > >  {
> > > >  
> > > >       unsigned int hblank_min = mode->hmax_min - mode->width;
> > > >       unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > 
> > > > -     unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> > > > +     unsigned int vblank_min = mode->vmax_min - mode->height;
> > > > +     unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > 
> > > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > > >       
> > > >       __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max,
> > > >       1,
> > > >       
> > > >                                hblank_min);
> > > > 
> > > > -     __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1,
> > > > vblank);
> > > > +     __v4l2_ctrl_modify_range(imx290->vblank, vblank_min, vblank_max,
> > > > 1,
> > > > +                              vblank_min);
> > > 
> > > You are missing setting the range for V4L2_CID_EXPOSURE. So it stays at
> > > 1
> > > resulting in a black image.
> > 
> > You're right.
> > I'm always testing with libcamera which will set V4L2_CID_VBLANK to
> > configure the frame rate, and that will update the exposure range.
> 
> Actually I'm going to backtrack somewhat, although I may have
> misunderstood your comment.
> 
> imx290_subdev_init calls imx290_ctrl_update. VBLANK was created with
> min/max/def all set to 1. The update sets a range of 45 to 261063, so
> the old value is now invalid and the new default is adopted. That
> means set_ctrl is called for VBLANK, and that triggers the exposure
> range to be updated. There is no need to update the exposure range in
> imx290_ctrl_update on that basis.

This is how it should be and in imx290_set_ctrl is the bug. There is a 'switch 
(ctrl->id)' for fast return on ctrl which don't need writes to the device. But 
this changed on your series. Removing the fast return for both VBLANK and 
HBLANK I get proper settings for exposure.

> It is missed when changing mode as the current VBLANK value is still
> within range, therefore it doesn't need to change, and doesn't update
> the exposure range. I'll fix that.

But the default VBLANK of 1 is not in range of the minimum of 45, so there is 
definitely an update.

> Are you relying on the initial exposure to be some particular value?
> Is the initial value for any setting actually guaranteed in the spec?
> If your application is expecting max exposure by default, then that
> sounds very fragile and not portable to other sensors, but it can be
> fixed up.

I'm not relying on any specific value, everything is configured/controlled by 
libcamera.

Best regards,
Alexander

> Do note that switching to 1280x720 mode will limit the exposure range
> to 1-763. Switching back to 1920x1080 mode will increase the range
> again, but as the current value is still in range the exposure setting
> will not be changed. What controls get reset when changing mode is
> undefined, but AIUI it should update the ranges and leave everything
> else alone, which is what is done here. Some drivers pepper
> v4l2_ctrl_s_ctrl calls around the place, but what value to adopt seems
> random.
> 
>   Dave
> 
> >   Dave
> >   
> > > Best regards,
> > > Alexander
> > > 
> > > >  }
> > > >  
> > > >  static int imx290_ctrl_init(struct imx290 *imx290)
> > > > 
> > > > @@ -761,9 +793,12 @@ static int imx290_ctrl_init(struct imx290
> > > > *imx290)
> > > > 
> > > >       v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > > >       
> > > >                         V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
> > > > 
> > > > -     v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > > > -                       V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2,
> > > 
> > > 1,
> > > 
> > > > -                       IMX290_VMAX_DEFAULT - 2);
> > > > +     /*
> > > > +      * Correct range will be determined through imx290_ctrl_update
> > > 
> > > setting
> > > 
> > > > +      * V4L2_CID_VBLANK.
> > > > +      */
> > > > +     imx290->exposure = v4l2_ctrl_new_std(&imx290->ctrls,
> > > 
> > > &imx290_ctrl_ops,
> > > 
> > > > +                                          V4L2_CID_EXPOSURE, 1,
> > > 
> > > 1, 1, 1);
> > > 
> > > >       /*
> > > >       
> > > >        * Set the link frequency, pixel rate, horizontal blanking and
> > > 
> > > vertical
> > > 
> > > > @@ -795,8 +830,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > > > 
> > > >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls,
> > > >       &imx290_ctrl_ops,
> > > >       
> > > >                                          V4L2_CID_VBLANK, 1, 1, 1,
> > > 
> > > 1);
> > > 
> > > > -     if (imx290->vblank)
> > > > -             imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > 
> > > >       v4l2_ctrl_new_fwnode_properties(&imx290->ctrls,
> > > >       &imx290_ctrl_ops,
> > > >       
> > > >                                       &props);
Alexander Stein Feb. 3, 2023, 7:19 a.m. UTC | #11 
Hi Dave,

thanks for the patch.

Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson:
> The driver exposed V4L2_CID_HBLANK as a read only control to allow
> for exposure calculations and determination of the frame rate.
> 
> Convert to a read/write control so that the frame rate can be
> controlled.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 9ddd6382b127..9006be6e5e7c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -47,6 +47,7 @@
>  #define IMX290_GAIN					
IMX290_REG_8BIT(0x3014)
>  #define IMX290_VMAX					
IMX290_REG_24BIT(0x3018)
>  #define IMX290_HMAX					
IMX290_REG_16BIT(0x301c)
> +#define IMX290_HMAX_MAX					0xffff
>  #define IMX290_SHS1					
IMX290_REG_24BIT(0x3020)
>  #define IMX290_WINWV_OB					
IMX290_REG_8BIT(0x303a)
>  #define IMX290_WINPV					
IMX290_REG_16BIT(0x303c)
> @@ -167,7 +168,7 @@ struct imx290_regval {
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> -	u32 hmax;
> +	u32 hmax_min;
>  	u8 link_freq_index;
> 
>  	const struct imx290_regval *data;
> @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { {
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { {
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { {
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { {
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)

You will need to remove V4L2_CID_HBLANK on the immediately return check at the 
beginning of the function. Otherwise this setting will never reach the device.

Best regards
Alexander

>  		}
>  		break;
> 
> +	case V4L2_CID_HBLANK:
> +		ret = imx290_write(imx290, IMX290_HMAX,
> +				   ctrl->val + imx290->current_mode-
>width,
> +				   NULL);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  			       const struct v4l2_mbus_framefmt *format,
>  			       const struct imx290_mode *mode)
>  {
> -	unsigned int hblank = mode->hmax - mode->width;
> +	unsigned int hblank_min = mode->hmax_min - mode->width;
> +	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
>  	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> 
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> 
> -	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> +	__v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> +				 hblank_min);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
>  }
> 
> @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  				     
ARRAY_SIZE(imx290_test_pattern_menu) - 1,
>  				     0, 0, imx290_test_pattern_menu);
> 
> +	/*
> +	 * Actual range will be set from imx290_ctrl_update later in the 
probe.
> +	 */
>  	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_HBLANK, 1, 1, 1, 
1);
> -	if (imx290->hblank)
> -		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
>  	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_VBLANK, 1, 1, 1, 
1);
> @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290
> *imx290, return ret;
>  	}
> 
> -	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> -			   NULL);
> -	if (ret)
> -		return ret;
> -
>  	/* Apply customized values from user */
>  	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
Alexander Stein Feb. 3, 2023, 7:44 a.m. UTC | #12 
Hi Dave,

thanks for the patch.

Am Dienstag, 31. Januar 2023, 20:20:15 CET schrieb Dave Stevenson:
> The sensor supports either a 37.125 or 74.25MHz external, but
> the driver only supported 37.125MHz.
> 
> Add the relevant register configuration for either clock
> frequency option.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
>  1 file changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 5202ef3cc3e6..7f6746f74040 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -102,6 +102,7 @@
>  #define IMX290_TCLKPREPARE				
IMX290_REG_16BIT(0x3452)
>  #define IMX290_TLPX					
IMX290_REG_16BIT(0x3454)
>  #define IMX290_X_OUT_SIZE				
IMX290_REG_16BIT(0x3472)
> +#define IMX290_INCKSEL7					
IMX290_REG_8BIT(0x3480)

Please add also defines for the clock setting, e.g.
#define IMX290_INCKSEL7_INCK_37_125	0x49
#define IMX290_INCKSEL7_INCK_74_25	0x92

> 
>  #define IMX290_PGCTRL_REGEN				BIT(0)
>  #define IMX290_PGCTRL_THRU				BIT(1)
> @@ -159,11 +160,27 @@
> 
>  #define IMX290_NUM_SUPPLIES				3
> 
> +#define CLK_37_125	0
> +#define CLK_74_25	1
> +#define NUM_CLK		2
> +

How about using an enum?

enum imx290_clk_freq {
	CLK_37_125 = 0,
	CLK_74_25 = 1,
	NUM_CLK
};

>  struct imx290_regval {
>  	u32 reg;
>  	u32 val;
>  };
> 
> +/*
> + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> + */
> +struct imx290_clk_cfg {
> +	u8 incksel1;
> +	u8 incksel2;
> +	u8 incksel3;
> +	u8 incksel4;
> +	u8 incksel5;
> +	u8 incksel6;
> +};
> +
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> @@ -173,6 +190,8 @@ struct imx290_mode {
> 
>  	const struct imx290_regval *data;
>  	u32 data_size;
> +
> +	const struct imx290_clk_cfg *clk_cfg;
>  };
> 
>  struct imx290_csi_cfg {
> @@ -191,6 +210,7 @@ struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
>  	struct regmap *regmap;
> +	u32 xclk_freq;
>  	u8 nlanes;
>  	u8 mono;
> 
> @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct
> v4l2_subdev *_sd) */
> 
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },
>  	{ IMX290_WINPV, 0 },
> @@ -269,7 +288,16 @@ static const struct imx290_regval
> imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
>  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
>  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },
> +};
> +
> +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> +	{ IMX290_EXTCK_FREQ, 0x2520 },

Please also add defines for these magic numbers.

Best regards,
Alexander

> +	{ IMX290_INCKSEL7, 0x49 },
> +};
> +
> +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> +	{ IMX290_EXTCK_FREQ, 0x4a40 },
> +	{ IMX290_INCKSEL7, 0x92 },
>  };
> 
>  static const struct imx290_regval imx290_1080p_settings[] = {
> @@ -279,12 +307,6 @@ static const struct imx290_regval
> imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
>  	{ IMX290_Y_OUT_SIZE, 1080 },
> -	{ IMX290_INCKSEL1, 0x18 },
> -	{ IMX290_INCKSEL2, 0x03 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
> 
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -294,12 +316,6 @@ static const struct imx290_regval
> imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
>  	{ IMX290_Y_OUT_SIZE, 720 },
> -	{ IMX290_INCKSEL1, 0x20 },
> -	{ IMX290_INCKSEL2, 0x00 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
> 
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct
> imx290 *imx290) return ARRAY_SIZE(imx290_link_freq_4lanes);
>  }
> 
> +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> +	[CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x18,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x0c,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
> +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> +	[CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x20,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x10,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
>  /* Mode configs */
>  static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
> @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { .link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { .link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
> 
> @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { .link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { .link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
> 
> @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290
> *imx290, return 0;
>  }
> 
> +static int imx290_set_clock(struct imx290 *imx290)
> +{
> +	int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> +	const struct imx290_mode *mode = imx290->current_mode;
> +	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> +	int ret = 0;
> +
> +	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> +
> +	return ret;
> +}
> +
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
>  	int ret = 0;
> @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290
> *imx290, return ret;
>  	}
> 
> +	/* Set clock parameters based on mode and xclk */
> +	ret = imx290_set_clock(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set clocks\n");
> +		return ret;
> +	}
> +
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
>  	int ret;
> 
>  	ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> -				       "clock-frequency", &xclk_freq);
> +				       "clock-frequency", &imx290-
>xclk_freq);
>  	if (ret) {
>  		dev_err(imx290->dev, "Could not get xclk frequency\n");
>  		return ret;
>  	}
> 
> -	/* external clock must be 37.125 MHz */
> -	if (xclk_freq != 37125000) {
> +	/* external clock must be 37.125 MHz or 74.25MHz */
> +	if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000) 
{
>  		dev_err(imx290->dev, "External clock frequency %u is not 
supported\n",
>  			xclk_freq);
>  		return -EINVAL;
Alexander Stein Feb. 3, 2023, 7:45 a.m. UTC | #13 
Hi Dave,

thank you for the patch.

Am Dienstag, 31. Januar 2023, 20:20:14 CET schrieb Dave Stevenson:
> IMX290_CTRL_07 was written from both imx290_global_init_settings
> and imx290_1080p_settings and imx290_720p_settings.
> 
> Remove it from imx290_global_init_settings as the setting varies
> based on the mode.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 3413d83369ba..5202ef3cc3e6 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -219,7 +219,6 @@ static inline struct imx290 *to_imx290(struct
> v4l2_subdev *_sd) */
> 
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
>  	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Dave Stevenson Feb. 3, 2023, 8:05 a.m. UTC | #14 
Hi Alexander

On Fri, 3 Feb 2023 at 07:19, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dave,
>
> thanks for the patch.
>
> Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson:
> > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > for exposure calculations and determination of the frame rate.
> >
> > Convert to a read/write control so that the frame rate can be
> > controlled.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 9ddd6382b127..9006be6e5e7c 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -47,6 +47,7 @@
> >  #define IMX290_GAIN
> IMX290_REG_8BIT(0x3014)
> >  #define IMX290_VMAX
> IMX290_REG_24BIT(0x3018)
> >  #define IMX290_HMAX
> IMX290_REG_16BIT(0x301c)
> > +#define IMX290_HMAX_MAX                                      0xffff
> >  #define IMX290_SHS1
> IMX290_REG_24BIT(0x3020)
> >  #define IMX290_WINWV_OB
> IMX290_REG_8BIT(0x303a)
> >  #define IMX290_WINPV
> IMX290_REG_16BIT(0x303c)
> > @@ -167,7 +168,7 @@ struct imx290_regval {
> >  struct imx290_mode {
> >       u32 width;
> >       u32 height;
> > -     u32 hmax;
> > +     u32 hmax_min;
> >       u8 link_freq_index;
> >
> >       const struct imx290_regval *data;
> > @@ -410,7 +411,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > { {
> >               .width = 1920,
> >               .height = 1080,
> > -             .hmax = 2200,
> > +             .hmax_min = 2200,
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > @@ -418,7 +419,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > { {
> >               .width = 1280,
> >               .height = 720,
> > -             .hmax = 3300,
> > +             .hmax_min = 3300,
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > @@ -429,7 +430,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > { {
> >               .width = 1920,
> >               .height = 1080,
> > -             .hmax = 2200,
> > +             .hmax_min = 2200,
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > @@ -437,7 +438,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > { {
> >               .width = 1280,
> >               .height = 720,
> > -             .hmax = 3300,
> > +             .hmax_min = 3300,
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>
> You will need to remove V4L2_CID_HBLANK on the immediately return check at the
> beginning of the function. Otherwise this setting will never reach the device.

What tree are you adding these patches to? I'm basing it on Sakari's
tree at [1] - he's issued a pull for it, so that should be in 6.3.

The only immediate return check at the start of imx290_set_ctrl is
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
   return 0;

The controls are no longer READ_ONLY, therefore they don't return early.
There is no case for V4L2_CID_HBLANK.

Does this also account for the difference you're reporting with V4L2_CID_VBLANK?

 Dave

[1] https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290.c#n567

> Best regards
> Alexander
>
> >               }
> >               break;
> >
> > +     case V4L2_CID_HBLANK:
> > +             ret = imx290_write(imx290, IMX290_HMAX,
> > +                                ctrl->val + imx290->current_mode-
> >width,
> > +                                NULL);
> > +             break;
> > +
> >       default:
> >               ret = -EINVAL;
> >               break;
> > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> >                              const struct v4l2_mbus_framefmt *format,
> >                              const struct imx290_mode *mode)
> >  {
> > -     unsigned int hblank = mode->hmax - mode->width;
> > +     unsigned int hblank_min = mode->hmax_min - mode->width;
> > +     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> >       unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> >
> >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> >
> > -     __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> > +     __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> > +                              hblank_min);
> >       __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> >  }
> >
> > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >
> ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> >                                    0, 0, imx290_test_pattern_menu);
> >
> > +     /*
> > +      * Actual range will be set from imx290_ctrl_update later in the
> probe.
> > +      */
> >       imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >                                          V4L2_CID_HBLANK, 1, 1, 1,
> 1);
> > -     if (imx290->hblank)
> > -             imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >                                          V4L2_CID_VBLANK, 1, 1, 1,
> 1);
> > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290
> > *imx290, return ret;
> >       }
> >
> > -     ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> > -                        NULL);
> > -     if (ret)
> > -             return ret;
> > -
> >       /* Apply customized values from user */
> >       ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> >       if (ret) {
>
>
>
>
Alexander Stein Feb. 3, 2023, 8:39 a.m. UTC | #15 
Hi Dave,

Am Freitag, 3. Februar 2023, 09:05:44 CET schrieb Dave Stevenson:
> Hi Alexander
> 
> On Fri, 3 Feb 2023 at 07:19, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Dave,
> > 
> > thanks for the patch.
> > 
> > Am Dienstag, 31. Januar 2023, 20:20:12 CET schrieb Dave Stevenson:
> > > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > > for exposure calculations and determination of the frame rate.
> > > 
> > > Convert to a read/write control so that the frame rate can be
> > > controlled.
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > 
> > >  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
> > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 9ddd6382b127..9006be6e5e7c 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -47,6 +47,7 @@
> > > 
> > >  #define IMX290_GAIN
> > 
> > IMX290_REG_8BIT(0x3014)
> > 
> > >  #define IMX290_VMAX
> > 
> > IMX290_REG_24BIT(0x3018)
> > 
> > >  #define IMX290_HMAX
> > 
> > IMX290_REG_16BIT(0x301c)
> > 
> > > +#define IMX290_HMAX_MAX                                      0xffff
> > > 
> > >  #define IMX290_SHS1
> > 
> > IMX290_REG_24BIT(0x3020)
> > 
> > >  #define IMX290_WINWV_OB
> > 
> > IMX290_REG_8BIT(0x303a)
> > 
> > >  #define IMX290_WINPV
> > 
> > IMX290_REG_16BIT(0x303c)
> > 
> > > @@ -167,7 +168,7 @@ struct imx290_regval {
> > > 
> > >  struct imx290_mode {
> > >  
> > >       u32 width;
> > >       u32 height;
> > > 
> > > -     u32 hmax;
> > > +     u32 hmax_min;
> > > 
> > >       u8 link_freq_index;
> > >       
> > >       const struct imx290_regval *data;
> > > 
> > > @@ -410,7 +411,7 @@ static const struct imx290_mode
> > > imx290_modes_2lanes[] =
> > > { {
> > > 
> > >               .width = 1920,
> > >               .height = 1080,
> > > 
> > > -             .hmax = 2200,
> > > +             .hmax_min = 2200,
> > > 
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > 
> > > @@ -418,7 +419,7 @@ static const struct imx290_mode
> > > imx290_modes_2lanes[] =
> > > { {
> > > 
> > >               .width = 1280,
> > >               .height = 720,
> > > 
> > > -             .hmax = 3300,
> > > +             .hmax_min = 3300,
> > > 
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > 
> > > @@ -429,7 +430,7 @@ static const struct imx290_mode
> > > imx290_modes_4lanes[] =
> > > { {
> > > 
> > >               .width = 1920,
> > >               .height = 1080,
> > > 
> > > -             .hmax = 2200,
> > > +             .hmax_min = 2200,
> > > 
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > 
> > > @@ -437,7 +438,7 @@ static const struct imx290_mode
> > > imx290_modes_4lanes[] =
> > > { {
> > > 
> > >               .width = 1280,
> > >               .height = 720,
> > > 
> > > -             .hmax = 3300,
> > > +             .hmax_min = 3300,
> > > 
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > 
> > > @@ -686,6 +687,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > 
> > You will need to remove V4L2_CID_HBLANK on the immediately return check at
> > the beginning of the function. Otherwise this setting will never reach
> > the device.
> What tree are you adding these patches to? I'm basing it on Sakari's
> tree at [1] - he's issued a pull for it, so that should be in 6.3.

Thanks for pointing this out. I'm based on linux-next as I need other patches 
for platform support. I've had Laurent's patches included, but only v2 :( my 
bad. With v3 included instead this seems to work.

> The only immediate return check at the start of imx290_set_ctrl is
> if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
>    return 0;
> 
> The controls are no longer READ_ONLY, therefore they don't return early.
> There is no case for V4L2_CID_HBLANK.
> 
> Does this also account for the difference you're reporting with
> V4L2_CID_VBLANK?

Yes, things look good so far. Thanks

Best regards,
Alexander

> [1]
> https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/imx290
> .c#n567
> > Best regards
> > Alexander
> > 
> > >               }
> > >               break;
> > > 
> > > +     case V4L2_CID_HBLANK:
> > > +             ret = imx290_write(imx290, IMX290_HMAX,
> > > +                                ctrl->val + imx290->current_mode-
> > >
> > >width,
> > >
> > > +                                NULL);
> > > +             break;
> > > +
> > > 
> > >       default:
> > >               ret = -EINVAL;
> > >               break;
> > > 
> > > @@ -716,12 +723,14 @@ static void imx290_ctrl_update(struct imx290
> > > *imx290,
> > > 
> > >                              const struct v4l2_mbus_framefmt *format,
> > >                              const struct imx290_mode *mode)
> > >  
> > >  {
> > > 
> > > -     unsigned int hblank = mode->hmax - mode->width;
> > > +     unsigned int hblank_min = mode->hmax_min - mode->width;
> > > +     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > 
> > >       unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> > >       
> > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > > 
> > > -     __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1,
> > > hblank);
> > > +     __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max,
> > > 1,
> > > +                              hblank_min);
> > > 
> > >       __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1,
> > >       vblank);
> > >  
> > >  }
> > > 
> > > @@ -778,10 +787,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > 
> > ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> > 
> > >                                    0, 0, imx290_test_pattern_menu);
> > > 
> > > +     /*
> > > +      * Actual range will be set from imx290_ctrl_update later in the
> > 
> > probe.
> > 
> > > +      */
> > > 
> > >       imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls,
> > >       &imx290_ctrl_ops,
> > >       
> > >                                          V4L2_CID_HBLANK, 1, 1, 1,
> > 
> > 1);
> > 
> > > -     if (imx290->hblank)
> > > -             imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > 
> > >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls,
> > >       &imx290_ctrl_ops,
> > >       
> > >                                          V4L2_CID_VBLANK, 1, 1, 1,
> > 
> > 1);
> > 
> > > @@ -850,11 +860,6 @@ static int imx290_start_streaming(struct imx290
> > > *imx290, return ret;
> > > 
> > >       }
> > > 
> > > -     ret = imx290_write(imx290, IMX290_HMAX,
> > > imx290->current_mode->hmax,
> > > -                        NULL);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > 
> > >       /* Apply customized values from user */
> > >       ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> > >       if (ret) {
Laurent Pinchart Feb. 3, 2023, 8:59 a.m. UTC | #16 
On Fri, Feb 03, 2023 at 08:44:08AM +0100, Alexander Stein wrote:
> Hi Dave,
> 
> thanks for the patch.
> 
> Am Dienstag, 31. Januar 2023, 20:20:15 CET schrieb Dave Stevenson:
> > The sensor supports either a 37.125 or 74.25MHz external, but
> > the driver only supported 37.125MHz.
> > 
> > Add the relevant register configuration for either clock
> > frequency option.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
> >  1 file changed, 103 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 5202ef3cc3e6..7f6746f74040 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -102,6 +102,7 @@
> >  #define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
> >  #define IMX290_TLPX				IMX290_REG_16BIT(0x3454)
> >  #define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
> > +#define IMX290_INCKSEL7				IMX290_REG_8BIT(0x3480)
> 
> Please add also defines for the clock setting, e.g.
> #define IMX290_INCKSEL7_INCK_37_125	0x49
> #define IMX290_INCKSEL7_INCK_74_25	0x92
> 
> >  #define IMX290_PGCTRL_REGEN				BIT(0)
> >  #define IMX290_PGCTRL_THRU				BIT(1)
> > @@ -159,11 +160,27 @@
> > 
> >  #define IMX290_NUM_SUPPLIES				3
> > 
> > +#define CLK_37_125	0
> > +#define CLK_74_25	1
> > +#define NUM_CLK		2
> > +
> 
> How about using an enum?
> 
> enum imx290_clk_freq {
> 	CLK_37_125 = 0,
> 	CLK_74_25 = 1,
> 	NUM_CLK
> };
> 
> >  struct imx290_regval {
> >  	u32 reg;
> >  	u32 val;
> >  };
> > 
> > +/*
> > + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> > + */
> > +struct imx290_clk_cfg {
> > +	u8 incksel1;
> > +	u8 incksel2;
> > +	u8 incksel3;
> > +	u8 incksel4;
> > +	u8 incksel5;
> > +	u8 incksel6;
> > +};
> > +
> >  struct imx290_mode {
> >  	u32 width;
> >  	u32 height;
> > @@ -173,6 +190,8 @@ struct imx290_mode {
> > 
> >  	const struct imx290_regval *data;
> >  	u32 data_size;
> > +
> > +	const struct imx290_clk_cfg *clk_cfg;
> >  };
> > 
> >  struct imx290_csi_cfg {
> > @@ -191,6 +210,7 @@ struct imx290 {
> >  	struct device *dev;
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> > +	u32 xclk_freq;
> >  	u8 nlanes;
> >  	u8 mono;
> > 
> > @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct
> > v4l2_subdev *_sd) */
> > 
> >  static const struct imx290_regval imx290_global_init_settings[] = {
> > -	{ IMX290_EXTCK_FREQ, 0x2520 },
> >  	{ IMX290_WINWV_OB, 12 },
> >  	{ IMX290_WINPH, 0 },
> >  	{ IMX290_WINPV, 0 },
> > @@ -269,7 +288,16 @@ static const struct imx290_regval
> > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
> >  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
> >  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> > -	{ IMX290_REG_8BIT(0x3480), 0x49 },
> > +};
> > +
> > +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> > +	{ IMX290_EXTCK_FREQ, 0x2520 },
> 
> Please also add defines for these magic numbers.

While I really like macros that name register and bits, as they improve
readability, for values that are listed in the documentation without any
explanation I think it doesn't make the code more readable.

> > +	{ IMX290_INCKSEL7, 0x49 },
> > +};
> > +
> > +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> > +	{ IMX290_EXTCK_FREQ, 0x4a40 },
> > +	{ IMX290_INCKSEL7, 0x92 },
> >  };
> > 
> >  static const struct imx290_regval imx290_1080p_settings[] = {
> > @@ -279,12 +307,6 @@ static const struct imx290_regval
> > imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
> >  	{ IMX290_X_OUT_SIZE, 1920 },
> >  	{ IMX290_Y_OUT_SIZE, 1080 },
> > -	{ IMX290_INCKSEL1, 0x18 },
> > -	{ IMX290_INCKSEL2, 0x03 },
> > -	{ IMX290_INCKSEL3, 0x20 },
> > -	{ IMX290_INCKSEL4, 0x01 },
> > -	{ IMX290_INCKSEL5, 0x1a },
> > -	{ IMX290_INCKSEL6, 0x1a },
> >  };
> > 
> >  static const struct imx290_regval imx290_720p_settings[] = {
> > @@ -294,12 +316,6 @@ static const struct imx290_regval
> > imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
> >  	{ IMX290_X_OUT_SIZE, 1280 },
> >  	{ IMX290_Y_OUT_SIZE, 720 },
> > -	{ IMX290_INCKSEL1, 0x20 },
> > -	{ IMX290_INCKSEL2, 0x00 },
> > -	{ IMX290_INCKSEL3, 0x20 },
> > -	{ IMX290_INCKSEL4, 0x01 },
> > -	{ IMX290_INCKSEL5, 0x1a },
> > -	{ IMX290_INCKSEL6, 0x1a },
> >  };
> > 
> >  static const struct imx290_regval imx290_10bit_settings[] = {
> > @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct
> > imx290 *imx290) return ARRAY_SIZE(imx290_link_freq_4lanes);
> >  }
> > 
> > +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> > +	[CLK_37_125] = {
> > +		/* 37.125MHz clock config */
> > +		.incksel1 = 0x18,
> > +		.incksel2 = 0x03,
> > +		.incksel3 = 0x20,
> > +		.incksel4 = 0x01,
> > +		.incksel5 = 0x1a,
> > +		.incksel6 = 0x1a,
> > +	},
> > +	[CLK_74_25] = {
> > +		/* 74.25MHz clock config */
> > +		.incksel1 = 0x0c,
> > +		.incksel2 = 0x03,
> > +		.incksel3 = 0x10,
> > +		.incksel4 = 0x01,
> > +		.incksel5 = 0x1b,
> > +		.incksel6 = 0x1b,
> > +	},
> > +};
> > +
> > +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> > +	[CLK_37_125] = {
> > +		/* 37.125MHz clock config */
> > +		.incksel1 = 0x20,
> > +		.incksel2 = 0x00,
> > +		.incksel3 = 0x20,
> > +		.incksel4 = 0x01,
> > +		.incksel5 = 0x1a,
> > +		.incksel6 = 0x1a,
> > +	},
> > +	[CLK_74_25] = {
> > +		/* 74.25MHz clock config */
> > +		.incksel1 = 0x10,
> > +		.incksel2 = 0x00,
> > +		.incksel3 = 0x10,
> > +		.incksel4 = 0x01,
> > +		.incksel5 = 0x1b,
> > +		.incksel6 = 0x1b,
> > +	},
> > +};
> > +
> >  /* Mode configs */
> >  static const struct imx290_mode imx290_modes_2lanes[] = {
> >  	{
> > @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > { .link_freq_index = FREQ_INDEX_1080P,
> >  		.data = imx290_1080p_settings,
> >  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> > +		.clk_cfg = imx290_1080p_clock_config,
> >  	},
> >  	{
> >  		.width = 1280,
> > @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > { .link_freq_index = FREQ_INDEX_720P,
> >  		.data = imx290_720p_settings,
> >  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> > +		.clk_cfg = imx290_720p_clock_config,
> >  	},
> >  };
> > 
> > @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > { .link_freq_index = FREQ_INDEX_1080P,
> >  		.data = imx290_1080p_settings,
> >  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> > +		.clk_cfg = imx290_1080p_clock_config,
> >  	},
> >  	{
> >  		.width = 1280,
> > @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > { .link_freq_index = FREQ_INDEX_720P,
> >  		.data = imx290_720p_settings,
> >  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> > +		.clk_cfg = imx290_720p_clock_config,
> >  	},
> >  };
> > 
> > @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290
> > *imx290, return 0;
> >  }
> > 
> > +static int imx290_set_clock(struct imx290 *imx290)
> > +{
> > +	int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> > +	const struct imx290_mode *mode = imx290->current_mode;
> > +	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> > +	int ret = 0;
> > +
> > +	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> > +	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> > +	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> > +	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> > +	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> > +	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> > +
> > +	return ret;
> > +}
> > +
> >  static int imx290_set_data_lanes(struct imx290 *imx290)
> >  {
> >  	int ret = 0;
> > @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290
> > *imx290, return ret;
> >  	}
> > 
> > +	/* Set clock parameters based on mode and xclk */
> > +	ret = imx290_set_clock(imx290);
> > +	if (ret < 0) {
> > +		dev_err(imx290->dev, "Could not set clocks\n");
> > +		return ret;
> > +	}
> > +
> >  	/* Set data lane count */
> >  	ret = imx290_set_data_lanes(imx290);
> >  	if (ret < 0) {
> > @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
> >  	int ret;
> > 
> >  	ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> > -				       "clock-frequency", &xclk_freq);
> > +				       "clock-frequency", &imx290->xclk_freq);
> >  	if (ret) {
> >  		dev_err(imx290->dev, "Could not get xclk frequency\n");
> >  		return ret;
> >  	}
> > 
> > -	/* external clock must be 37.125 MHz */
> > -	if (xclk_freq != 37125000) {
> > +	/* external clock must be 37.125 MHz or 74.25MHz */
> > +	if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000) 
> {
> >  		dev_err(imx290->dev, "External clock frequency %u is not supported\n",
> >  			xclk_freq);
> >  		return -EINVAL;
Dave Stevenson Feb. 3, 2023, 9:30 a.m. UTC | #17 
Hi Laurent

On Fri, 3 Feb 2023 at 08:59, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Feb 03, 2023 at 08:44:08AM +0100, Alexander Stein wrote:
> > Hi Dave,
> >
> > thanks for the patch.
> >
> > Am Dienstag, 31. Januar 2023, 20:20:15 CET schrieb Dave Stevenson:
> > > The sensor supports either a 37.125 or 74.25MHz external, but
> > > the driver only supported 37.125MHz.
> > >
> > > Add the relevant register configuration for either clock
> > > frequency option.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 103 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 5202ef3cc3e6..7f6746f74040 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -102,6 +102,7 @@
> > >  #define IMX290_TCLKPREPARE                         IMX290_REG_16BIT(0x3452)
> > >  #define IMX290_TLPX                                IMX290_REG_16BIT(0x3454)
> > >  #define IMX290_X_OUT_SIZE                          IMX290_REG_16BIT(0x3472)
> > > +#define IMX290_INCKSEL7                            IMX290_REG_8BIT(0x3480)
> >
> > Please add also defines for the clock setting, e.g.
> > #define IMX290_INCKSEL7_INCK_37_125   0x49
> > #define IMX290_INCKSEL7_INCK_74_25    0x92
> >
> > >  #define IMX290_PGCTRL_REGEN                                BIT(0)
> > >  #define IMX290_PGCTRL_THRU                         BIT(1)
> > > @@ -159,11 +160,27 @@
> > >
> > >  #define IMX290_NUM_SUPPLIES                                3
> > >
> > > +#define CLK_37_125 0
> > > +#define CLK_74_25  1
> > > +#define NUM_CLK            2
> > > +
> >
> > How about using an enum?
> >
> > enum imx290_clk_freq {
> >       CLK_37_125 = 0,
> >       CLK_74_25 = 1,
> >       NUM_CLK
> > };
> >
> > >  struct imx290_regval {
> > >     u32 reg;
> > >     u32 val;
> > >  };
> > >
> > > +/*
> > > + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> > > + */
> > > +struct imx290_clk_cfg {
> > > +   u8 incksel1;
> > > +   u8 incksel2;
> > > +   u8 incksel3;
> > > +   u8 incksel4;
> > > +   u8 incksel5;
> > > +   u8 incksel6;
> > > +};
> > > +
> > >  struct imx290_mode {
> > >     u32 width;
> > >     u32 height;
> > > @@ -173,6 +190,8 @@ struct imx290_mode {
> > >
> > >     const struct imx290_regval *data;
> > >     u32 data_size;
> > > +
> > > +   const struct imx290_clk_cfg *clk_cfg;
> > >  };
> > >
> > >  struct imx290_csi_cfg {
> > > @@ -191,6 +210,7 @@ struct imx290 {
> > >     struct device *dev;
> > >     struct clk *xclk;
> > >     struct regmap *regmap;
> > > +   u32 xclk_freq;
> > >     u8 nlanes;
> > >     u8 mono;
> > >
> > > @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct
> > > v4l2_subdev *_sd) */
> > >
> > >  static const struct imx290_regval imx290_global_init_settings[] = {
> > > -   { IMX290_EXTCK_FREQ, 0x2520 },
> > >     { IMX290_WINWV_OB, 12 },
> > >     { IMX290_WINPH, 0 },
> > >     { IMX290_WINPV, 0 },
> > > @@ -269,7 +288,16 @@ static const struct imx290_regval
> > > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
> > >     { IMX290_REG_8BIT(0x33b2), 0x1a },
> > >     { IMX290_REG_8BIT(0x33b3), 0x04 },
> > > -   { IMX290_REG_8BIT(0x3480), 0x49 },
> > > +};
> > > +
> > > +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> > > +   { IMX290_EXTCK_FREQ, 0x2520 },
> >
> > Please also add defines for these magic numbers.
>
> While I really like macros that name register and bits, as they improve
> readability, for values that are listed in the documentation without any
> explanation I think it doesn't make the code more readable.

Technically they're not totally magic - it's just the frequency in MHz
as 8q8 fixed point.
(0x2520 = 9504 decimal) * 1/256 = 37.125.
(0x4a40 = 19008 decimal) * 1/256 = 74.25

As there are only 2 acceptable clock rates computing it feels like
overkill, but whatever.
I'll put it in as ((37125 * 256) / 1000). Still some magic, but makes
it a little more obvious what it is and should give the right number
(I need to check for overflows. Rounding seems OK).

  Dave

> > > +   { IMX290_INCKSEL7, 0x49 },
> > > +};
> > > +
> > > +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> > > +   { IMX290_EXTCK_FREQ, 0x4a40 },
> > > +   { IMX290_INCKSEL7, 0x92 },
> > >  };
> > >
> > >  static const struct imx290_regval imx290_1080p_settings[] = {
> > > @@ -279,12 +307,6 @@ static const struct imx290_regval
> > > imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
> > >     { IMX290_X_OUT_SIZE, 1920 },
> > >     { IMX290_Y_OUT_SIZE, 1080 },
> > > -   { IMX290_INCKSEL1, 0x18 },
> > > -   { IMX290_INCKSEL2, 0x03 },
> > > -   { IMX290_INCKSEL3, 0x20 },
> > > -   { IMX290_INCKSEL4, 0x01 },
> > > -   { IMX290_INCKSEL5, 0x1a },
> > > -   { IMX290_INCKSEL6, 0x1a },
> > >  };
> > >
> > >  static const struct imx290_regval imx290_720p_settings[] = {
> > > @@ -294,12 +316,6 @@ static const struct imx290_regval
> > > imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
> > >     { IMX290_X_OUT_SIZE, 1280 },
> > >     { IMX290_Y_OUT_SIZE, 720 },
> > > -   { IMX290_INCKSEL1, 0x20 },
> > > -   { IMX290_INCKSEL2, 0x00 },
> > > -   { IMX290_INCKSEL3, 0x20 },
> > > -   { IMX290_INCKSEL4, 0x01 },
> > > -   { IMX290_INCKSEL5, 0x1a },
> > > -   { IMX290_INCKSEL6, 0x1a },
> > >  };
> > >
> > >  static const struct imx290_regval imx290_10bit_settings[] = {
> > > @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct
> > > imx290 *imx290) return ARRAY_SIZE(imx290_link_freq_4lanes);
> > >  }
> > >
> > > +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> > > +   [CLK_37_125] = {
> > > +           /* 37.125MHz clock config */
> > > +           .incksel1 = 0x18,
> > > +           .incksel2 = 0x03,
> > > +           .incksel3 = 0x20,
> > > +           .incksel4 = 0x01,
> > > +           .incksel5 = 0x1a,
> > > +           .incksel6 = 0x1a,
> > > +   },
> > > +   [CLK_74_25] = {
> > > +           /* 74.25MHz clock config */
> > > +           .incksel1 = 0x0c,
> > > +           .incksel2 = 0x03,
> > > +           .incksel3 = 0x10,
> > > +           .incksel4 = 0x01,
> > > +           .incksel5 = 0x1b,
> > > +           .incksel6 = 0x1b,
> > > +   },
> > > +};
> > > +
> > > +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> > > +   [CLK_37_125] = {
> > > +           /* 37.125MHz clock config */
> > > +           .incksel1 = 0x20,
> > > +           .incksel2 = 0x00,
> > > +           .incksel3 = 0x20,
> > > +           .incksel4 = 0x01,
> > > +           .incksel5 = 0x1a,
> > > +           .incksel6 = 0x1a,
> > > +   },
> > > +   [CLK_74_25] = {
> > > +           /* 74.25MHz clock config */
> > > +           .incksel1 = 0x10,
> > > +           .incksel2 = 0x00,
> > > +           .incksel3 = 0x10,
> > > +           .incksel4 = 0x01,
> > > +           .incksel5 = 0x1b,
> > > +           .incksel6 = 0x1b,
> > > +   },
> > > +};
> > > +
> > >  /* Mode configs */
> > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > >     {
> > > @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > > { .link_freq_index = FREQ_INDEX_1080P,
> > >             .data = imx290_1080p_settings,
> > >             .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > +           .clk_cfg = imx290_1080p_clock_config,
> > >     },
> > >     {
> > >             .width = 1280,
> > > @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> > > { .link_freq_index = FREQ_INDEX_720P,
> > >             .data = imx290_720p_settings,
> > >             .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > +           .clk_cfg = imx290_720p_clock_config,
> > >     },
> > >  };
> > >
> > > @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > > { .link_freq_index = FREQ_INDEX_1080P,
> > >             .data = imx290_1080p_settings,
> > >             .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > +           .clk_cfg = imx290_1080p_clock_config,
> > >     },
> > >     {
> > >             .width = 1280,
> > > @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> > > { .link_freq_index = FREQ_INDEX_720P,
> > >             .data = imx290_720p_settings,
> > >             .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > +           .clk_cfg = imx290_720p_clock_config,
> > >     },
> > >  };
> > >
> > > @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290
> > > *imx290, return 0;
> > >  }
> > >
> > > +static int imx290_set_clock(struct imx290 *imx290)
> > > +{
> > > +   int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> > > +   const struct imx290_mode *mode = imx290->current_mode;
> > > +   const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> > > +   int ret = 0;
> > > +
> > > +   imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> > > +   imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> > > +   imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> > > +   imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> > > +   imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> > > +   imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > >  static int imx290_set_data_lanes(struct imx290 *imx290)
> > >  {
> > >     int ret = 0;
> > > @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290
> > > *imx290, return ret;
> > >     }
> > >
> > > +   /* Set clock parameters based on mode and xclk */
> > > +   ret = imx290_set_clock(imx290);
> > > +   if (ret < 0) {
> > > +           dev_err(imx290->dev, "Could not set clocks\n");
> > > +           return ret;
> > > +   }
> > > +
> > >     /* Set data lane count */
> > >     ret = imx290_set_data_lanes(imx290);
> > >     if (ret < 0) {
> > > @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
> > >     int ret;
> > >
> > >     ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> > > -                                  "clock-frequency", &xclk_freq);
> > > +                                  "clock-frequency", &imx290->xclk_freq);
> > >     if (ret) {
> > >             dev_err(imx290->dev, "Could not get xclk frequency\n");
> > >             return ret;
> > >     }
> > >
> > > -   /* external clock must be 37.125 MHz */
> > > -   if (xclk_freq != 37125000) {
> > > +   /* external clock must be 37.125 MHz or 74.25MHz */
> > > +   if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000)
> > {
> > >             dev_err(imx290->dev, "External clock frequency %u is not supported\n",
> > >                     xclk_freq);
> > >             return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
Dave Stevenson Feb. 3, 2023, 5:04 p.m. UTC | #18 
Hi Laurent

On Thu, 2 Feb 2023 at 22:03, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
> On Tue, Jan 31, 2023 at 07:20:15PM +0000, Dave Stevenson wrote:
> > The sensor supports either a 37.125 or 74.25MHz external, but
> > the driver only supported 37.125MHz.
> >
> > Add the relevant register configuration for either clock
> > frequency option.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
> >  1 file changed, 103 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 5202ef3cc3e6..7f6746f74040 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -102,6 +102,7 @@
> >  #define IMX290_TCLKPREPARE                           IMX290_REG_16BIT(0x3452)
> >  #define IMX290_TLPX                                  IMX290_REG_16BIT(0x3454)
> >  #define IMX290_X_OUT_SIZE                            IMX290_REG_16BIT(0x3472)
> > +#define IMX290_INCKSEL7                                      IMX290_REG_8BIT(0x3480)
> >
> >  #define IMX290_PGCTRL_REGEN                          BIT(0)
> >  #define IMX290_PGCTRL_THRU                           BIT(1)
> > @@ -159,11 +160,27 @@
> >
> >  #define IMX290_NUM_SUPPLIES                          3
> >
> > +#define CLK_37_125   0
> > +#define CLK_74_25    1
> > +#define NUM_CLK              2
>
> Please add an IMX290 prefer to avoid future namespace clashes.
>
> > +
> >  struct imx290_regval {
> >       u32 reg;
> >       u32 val;
> >  };
> >
> > +/*
> > + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> > + */
> > +struct imx290_clk_cfg {
> > +     u8 incksel1;
> > +     u8 incksel2;
> > +     u8 incksel3;
> > +     u8 incksel4;
> > +     u8 incksel5;
> > +     u8 incksel6;
> > +};
> > +
> >  struct imx290_mode {
> >       u32 width;
> >       u32 height;
> > @@ -173,6 +190,8 @@ struct imx290_mode {
> >
> >       const struct imx290_regval *data;
> >       u32 data_size;
> > +
> > +     const struct imx290_clk_cfg *clk_cfg;
> >  };
> >
> >  struct imx290_csi_cfg {
> > @@ -191,6 +210,7 @@ struct imx290 {
> >       struct device *dev;
> >       struct clk *xclk;
> >       struct regmap *regmap;
> > +     u32 xclk_freq;
> >       u8 nlanes;
> >       u8 mono;
> >
> > @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> >   */
> >
> >  static const struct imx290_regval imx290_global_init_settings[] = {
> > -     { IMX290_EXTCK_FREQ, 0x2520 },
> >       { IMX290_WINWV_OB, 12 },
> >       { IMX290_WINPH, 0 },
> >       { IMX290_WINPV, 0 },
> > @@ -269,7 +288,16 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> >       { IMX290_REG_8BIT(0x33b0), 0x50 },
> >       { IMX290_REG_8BIT(0x33b2), 0x1a },
> >       { IMX290_REG_8BIT(0x33b3), 0x04 },
> > -     { IMX290_REG_8BIT(0x3480), 0x49 },
>
> One less unnamed register, only 42 to go :-D
>
> > +};
> > +
> > +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> > +     { IMX290_EXTCK_FREQ, 0x2520 },
> > +     { IMX290_INCKSEL7, 0x49 },
> > +};
> > +
> > +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> > +     { IMX290_EXTCK_FREQ, 0x4a40 },
> > +     { IMX290_INCKSEL7, 0x92 },
> >  };
>
> Those two arrays are not used, which I assume is not normal :-) A rebase
> problem maybe ?

Whoops! Seeing as I'd tested at both xclk_freqs it shows how little they do!

> How about moving the INCKSEL7 value to the imx290_clk_cfg structure for
> consistency ?

These two only vary based on xclk_freq, whilst imx290_clk_cfg varies
with mode and xclk. I kept them separate to make that distinction
obvious, rather than duplicating the values between 1080p and 720p
tables.

> >
> >  static const struct imx290_regval imx290_1080p_settings[] = {
> > @@ -279,12 +307,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
> >       { IMX290_OPB_SIZE_V, 10 },
> >       { IMX290_X_OUT_SIZE, 1920 },
> >       { IMX290_Y_OUT_SIZE, 1080 },
> > -     { IMX290_INCKSEL1, 0x18 },
> > -     { IMX290_INCKSEL2, 0x03 },
> > -     { IMX290_INCKSEL3, 0x20 },
> > -     { IMX290_INCKSEL4, 0x01 },
> > -     { IMX290_INCKSEL5, 0x1a },
> > -     { IMX290_INCKSEL6, 0x1a },
> >  };
> >
> >  static const struct imx290_regval imx290_720p_settings[] = {
> > @@ -294,12 +316,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
> >       { IMX290_OPB_SIZE_V, 4 },
> >       { IMX290_X_OUT_SIZE, 1280 },
> >       { IMX290_Y_OUT_SIZE, 720 },
> > -     { IMX290_INCKSEL1, 0x20 },
> > -     { IMX290_INCKSEL2, 0x00 },
> > -     { IMX290_INCKSEL3, 0x20 },
> > -     { IMX290_INCKSEL4, 0x01 },
> > -     { IMX290_INCKSEL5, 0x1a },
> > -     { IMX290_INCKSEL6, 0x1a },
> >  };
> >
> >  static const struct imx290_regval imx290_10bit_settings[] = {
> > @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct imx290 *imx290)
> >               return ARRAY_SIZE(imx290_link_freq_4lanes);
> >  }
> >
> > +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> > +     [CLK_37_125] = {
> > +             /* 37.125MHz clock config */
> > +             .incksel1 = 0x18,
> > +             .incksel2 = 0x03,
> > +             .incksel3 = 0x20,
> > +             .incksel4 = 0x01,
> > +             .incksel5 = 0x1a,
> > +             .incksel6 = 0x1a,
> > +     },
>
> As the incksel[0-6] fields are only used in one place, to write all 6 of
> them to the device, you could also drop the imx290_clk_cfg structure and
> turn this into a imx290_regval array. Entirely up to you.

If you make them an array of imx290_regval then you also need to
manage the length of those arrays and either maintain that separately
for each one, or try and insert a compile time assert to pick up the
mismatch.
At least a dedicated struct avoids that pitfall.

> > +     [CLK_74_25] = {
> > +             /* 74.25MHz clock config */
> > +             .incksel1 = 0x0c,
> > +             .incksel2 = 0x03,
> > +             .incksel3 = 0x10,
> > +             .incksel4 = 0x01,
> > +             .incksel5 = 0x1b,
> > +             .incksel6 = 0x1b,
> > +     },
> > +};
> > +
> > +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> > +     [CLK_37_125] = {
> > +             /* 37.125MHz clock config */
> > +             .incksel1 = 0x20,
> > +             .incksel2 = 0x00,
> > +             .incksel3 = 0x20,
> > +             .incksel4 = 0x01,
> > +             .incksel5 = 0x1a,
> > +             .incksel6 = 0x1a,
> > +     },
> > +     [CLK_74_25] = {
> > +             /* 74.25MHz clock config */
> > +             .incksel1 = 0x10,
> > +             .incksel2 = 0x00,
> > +             .incksel3 = 0x10,
> > +             .incksel4 = 0x01,
> > +             .incksel5 = 0x1b,
> > +             .incksel6 = 0x1b,
> > +     },
> > +};
> > +
> >  /* Mode configs */
> >  static const struct imx290_mode imx290_modes_2lanes[] = {
> >       {
> > @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > +             .clk_cfg = imx290_1080p_clock_config,
> >       },
> >       {
> >               .width = 1280,
> > @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > +             .clk_cfg = imx290_720p_clock_config,
> >       },
> >  };
> >
> > @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >               .link_freq_index = FREQ_INDEX_1080P,
> >               .data = imx290_1080p_settings,
> >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > +             .clk_cfg = imx290_1080p_clock_config,
> >       },
> >       {
> >               .width = 1280,
> > @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >               .link_freq_index = FREQ_INDEX_720P,
> >               .data = imx290_720p_settings,
> >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > +             .clk_cfg = imx290_720p_clock_config,
> >       },
> >  };
> >
> > @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290 *imx290,
> >       return 0;
> >  }
> >
> > +static int imx290_set_clock(struct imx290 *imx290)
> > +{
> > +     int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> > +     const struct imx290_mode *mode = imx290->current_mode;
> > +     const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> > +     int ret = 0;
>
> How about turning the clock freq macros into an enum:
>
> enum imx290_clk_freq {
>         IMX290_CLK_37_125 = 0,
>         IMX290_CLK_74_25 = 1,
> };
>
> and replacing in struct imx290
>
> -       u32 xclk_freq;
> +       enum imx290_clk_freq xclk_freq;
>
> ? Then you could could simply write
>
>         const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[imx290->xclk_freq];
>
> (you could also name the field xclk_freq_idx if desired). Up to you, if
> you find that messier you can ignore the comment.

Done. Renamed to xclk_idx to make it clear that it's an index.

  Dave

> > +
> > +     imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> > +     imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> > +     imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> > +     imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> > +     imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> > +     imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> > +
> > +     return ret;
> > +}
> > +
> >  static int imx290_set_data_lanes(struct imx290 *imx290)
> >  {
> >       int ret = 0;
> > @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290 *imx290,
> >               return ret;
> >       }
> >
> > +     /* Set clock parameters based on mode and xclk */
> > +     ret = imx290_set_clock(imx290);
> > +     if (ret < 0) {
> > +             dev_err(imx290->dev, "Could not set clocks\n");
> > +             return ret;
> > +     }
> > +
> >       /* Set data lane count */
> >       ret = imx290_set_data_lanes(imx290);
> >       if (ret < 0) {
> > @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
> >       int ret;
> >
> >       ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> > -                                    "clock-frequency", &xclk_freq);
> > +                                    "clock-frequency", &imx290->xclk_freq);
> >       if (ret) {
> >               dev_err(imx290->dev, "Could not get xclk frequency\n");
> >               return ret;
> >       }
> >
> > -     /* external clock must be 37.125 MHz */
> > -     if (xclk_freq != 37125000) {
> > +     /* external clock must be 37.125 MHz or 74.25MHz */
> > +     if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000) {
> >               dev_err(imx290->dev, "External clock frequency %u is not supported\n",
> >                       xclk_freq);
> >               return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Feb. 3, 2023, 6:04 p.m. UTC | #19 
Hi Dave,

On Fri, Feb 03, 2023 at 05:04:13PM +0000, Dave Stevenson wrote:
> On Thu, 2 Feb 2023 at 22:03, Laurent Pinchart wrote:
> > On Tue, Jan 31, 2023 at 07:20:15PM +0000, Dave Stevenson wrote:
> > > The sensor supports either a 37.125 or 74.25MHz external, but
> > > the driver only supported 37.125MHz.
> > >
> > > Add the relevant register configuration for either clock
> > > frequency option.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 120 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 103 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 5202ef3cc3e6..7f6746f74040 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -102,6 +102,7 @@
> > >  #define IMX290_TCLKPREPARE                           IMX290_REG_16BIT(0x3452)
> > >  #define IMX290_TLPX                                  IMX290_REG_16BIT(0x3454)
> > >  #define IMX290_X_OUT_SIZE                            IMX290_REG_16BIT(0x3472)
> > > +#define IMX290_INCKSEL7                                      IMX290_REG_8BIT(0x3480)
> > >
> > >  #define IMX290_PGCTRL_REGEN                          BIT(0)
> > >  #define IMX290_PGCTRL_THRU                           BIT(1)
> > > @@ -159,11 +160,27 @@
> > >
> > >  #define IMX290_NUM_SUPPLIES                          3
> > >
> > > +#define CLK_37_125   0
> > > +#define CLK_74_25    1
> > > +#define NUM_CLK              2
> >
> > Please add an IMX290 prefer to avoid future namespace clashes.
> >
> > > +
> > >  struct imx290_regval {
> > >       u32 reg;
> > >       u32 val;
> > >  };
> > >
> > > +/*
> > > + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> > > + */
> > > +struct imx290_clk_cfg {
> > > +     u8 incksel1;
> > > +     u8 incksel2;
> > > +     u8 incksel3;
> > > +     u8 incksel4;
> > > +     u8 incksel5;
> > > +     u8 incksel6;
> > > +};
> > > +
> > >  struct imx290_mode {
> > >       u32 width;
> > >       u32 height;
> > > @@ -173,6 +190,8 @@ struct imx290_mode {
> > >
> > >       const struct imx290_regval *data;
> > >       u32 data_size;
> > > +
> > > +     const struct imx290_clk_cfg *clk_cfg;
> > >  };
> > >
> > >  struct imx290_csi_cfg {
> > > @@ -191,6 +210,7 @@ struct imx290 {
> > >       struct device *dev;
> > >       struct clk *xclk;
> > >       struct regmap *regmap;
> > > +     u32 xclk_freq;
> > >       u8 nlanes;
> > >       u8 mono;
> > >
> > > @@ -219,7 +239,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > >   */
> > >
> > >  static const struct imx290_regval imx290_global_init_settings[] = {
> > > -     { IMX290_EXTCK_FREQ, 0x2520 },
> > >       { IMX290_WINWV_OB, 12 },
> > >       { IMX290_WINPH, 0 },
> > >       { IMX290_WINPV, 0 },
> > > @@ -269,7 +288,16 @@ static const struct imx290_regval imx290_global_init_settings[] = {
> > >       { IMX290_REG_8BIT(0x33b0), 0x50 },
> > >       { IMX290_REG_8BIT(0x33b2), 0x1a },
> > >       { IMX290_REG_8BIT(0x33b3), 0x04 },
> > > -     { IMX290_REG_8BIT(0x3480), 0x49 },
> >
> > One less unnamed register, only 42 to go :-D
> >
> > > +};
> > > +
> > > +static const struct imx290_regval imx290_37_125mhz_clock[] = {
> > > +     { IMX290_EXTCK_FREQ, 0x2520 },
> > > +     { IMX290_INCKSEL7, 0x49 },
> > > +};
> > > +
> > > +static const struct imx290_regval imx290_74_25mhz_clock[] = {
> > > +     { IMX290_EXTCK_FREQ, 0x4a40 },
> > > +     { IMX290_INCKSEL7, 0x92 },
> > >  };
> >
> > Those two arrays are not used, which I assume is not normal :-) A rebase
> > problem maybe ?
> 
> Whoops! Seeing as I'd tested at both xclk_freqs it shows how little they do!

I was wondering about that :-)

> > How about moving the INCKSEL7 value to the imx290_clk_cfg structure for
> > consistency ?
> 
> These two only vary based on xclk_freq, whilst imx290_clk_cfg varies
> with mode and xclk. I kept them separate to make that distinction
> obvious, rather than duplicating the values between 1080p and 720p
> tables.

incksel3, incksel5 and incksel6 also vary based on the frequency. Up to
you.

> > >  static const struct imx290_regval imx290_1080p_settings[] = {
> > > @@ -279,12 +307,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
> > >       { IMX290_OPB_SIZE_V, 10 },
> > >       { IMX290_X_OUT_SIZE, 1920 },
> > >       { IMX290_Y_OUT_SIZE, 1080 },
> > > -     { IMX290_INCKSEL1, 0x18 },
> > > -     { IMX290_INCKSEL2, 0x03 },
> > > -     { IMX290_INCKSEL3, 0x20 },
> > > -     { IMX290_INCKSEL4, 0x01 },
> > > -     { IMX290_INCKSEL5, 0x1a },
> > > -     { IMX290_INCKSEL6, 0x1a },
> > >  };
> > >
> > >  static const struct imx290_regval imx290_720p_settings[] = {
> > > @@ -294,12 +316,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
> > >       { IMX290_OPB_SIZE_V, 4 },
> > >       { IMX290_X_OUT_SIZE, 1280 },
> > >       { IMX290_Y_OUT_SIZE, 720 },
> > > -     { IMX290_INCKSEL1, 0x20 },
> > > -     { IMX290_INCKSEL2, 0x00 },
> > > -     { IMX290_INCKSEL3, 0x20 },
> > > -     { IMX290_INCKSEL4, 0x01 },
> > > -     { IMX290_INCKSEL5, 0x1a },
> > > -     { IMX290_INCKSEL6, 0x1a },
> > >  };
> > >
> > >  static const struct imx290_regval imx290_10bit_settings[] = {
> > > @@ -405,6 +421,48 @@ static inline int imx290_link_freqs_num(const struct imx290 *imx290)
> > >               return ARRAY_SIZE(imx290_link_freq_4lanes);
> > >  }
> > >
> > > +static const struct imx290_clk_cfg imx290_1080p_clock_config[NUM_CLK] = {
> > > +     [CLK_37_125] = {
> > > +             /* 37.125MHz clock config */
> > > +             .incksel1 = 0x18,
> > > +             .incksel2 = 0x03,
> > > +             .incksel3 = 0x20,
> > > +             .incksel4 = 0x01,
> > > +             .incksel5 = 0x1a,
> > > +             .incksel6 = 0x1a,
> > > +     },
> >
> > As the incksel[0-6] fields are only used in one place, to write all 6 of
> > them to the device, you could also drop the imx290_clk_cfg structure and
> > turn this into a imx290_regval array. Entirely up to you.
> 
> If you make them an array of imx290_regval then you also need to
> manage the length of those arrays and either maintain that separately
> for each one, or try and insert a compile time assert to pick up the
> mismatch.
> At least a dedicated struct avoids that pitfall.

The array is the same for all of them, so we could define it in a macro,
but I agree a structure is a bit more future-proof. Up to you.

> > > +     [CLK_74_25] = {
> > > +             /* 74.25MHz clock config */
> > > +             .incksel1 = 0x0c,
> > > +             .incksel2 = 0x03,
> > > +             .incksel3 = 0x10,
> > > +             .incksel4 = 0x01,
> > > +             .incksel5 = 0x1b,
> > > +             .incksel6 = 0x1b,
> > > +     },
> > > +};
> > > +
> > > +static const struct imx290_clk_cfg imx290_720p_clock_config[NUM_CLK] = {
> > > +     [CLK_37_125] = {
> > > +             /* 37.125MHz clock config */
> > > +             .incksel1 = 0x20,
> > > +             .incksel2 = 0x00,
> > > +             .incksel3 = 0x20,
> > > +             .incksel4 = 0x01,
> > > +             .incksel5 = 0x1a,
> > > +             .incksel6 = 0x1a,
> > > +     },
> > > +     [CLK_74_25] = {
> > > +             /* 74.25MHz clock config */
> > > +             .incksel1 = 0x10,
> > > +             .incksel2 = 0x00,
> > > +             .incksel3 = 0x10,
> > > +             .incksel4 = 0x01,
> > > +             .incksel5 = 0x1b,
> > > +             .incksel6 = 0x1b,
> > > +     },
> > > +};
> > > +
> > >  /* Mode configs */
> > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > >       {
> > > @@ -415,6 +473,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > +             .clk_cfg = imx290_1080p_clock_config,
> > >       },
> > >       {
> > >               .width = 1280,
> > > @@ -424,6 +483,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > +             .clk_cfg = imx290_720p_clock_config,
> > >       },
> > >  };
> > >
> > > @@ -436,6 +496,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >               .link_freq_index = FREQ_INDEX_1080P,
> > >               .data = imx290_1080p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > +             .clk_cfg = imx290_1080p_clock_config,
> > >       },
> > >       {
> > >               .width = 1280,
> > > @@ -445,6 +506,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >               .link_freq_index = FREQ_INDEX_720P,
> > >               .data = imx290_720p_settings,
> > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > +             .clk_cfg = imx290_720p_clock_config,
> > >       },
> > >  };
> > >
> > > @@ -563,6 +625,23 @@ static int imx290_set_register_array(struct imx290 *imx290,
> > >       return 0;
> > >  }
> > >
> > > +static int imx290_set_clock(struct imx290 *imx290)
> > > +{
> > > +     int clk_idx = (imx290->xclk_freq == 37125000) ? 0 : 1;
> > > +     const struct imx290_mode *mode = imx290->current_mode;
> > > +     const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> > > +     int ret = 0;
> >
> > How about turning the clock freq macros into an enum:
> >
> > enum imx290_clk_freq {
> >         IMX290_CLK_37_125 = 0,
> >         IMX290_CLK_74_25 = 1,
> > };
> >
> > and replacing in struct imx290
> >
> > -       u32 xclk_freq;
> > +       enum imx290_clk_freq xclk_freq;
> >
> > ? Then you could could simply write
> >
> >         const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[imx290->xclk_freq];
> >
> > (you could also name the field xclk_freq_idx if desired). Up to you, if
> > you find that messier you can ignore the comment.
> 
> Done. Renamed to xclk_idx to make it clear that it's an index.
> 
> > > +
> > > +     imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> > > +     imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> > > +     imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> > > +     imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> > > +     imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> > > +     imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static int imx290_set_data_lanes(struct imx290 *imx290)
> > >  {
> > >       int ret = 0;
> > > @@ -863,6 +942,13 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > >               return ret;
> > >       }
> > >
> > > +     /* Set clock parameters based on mode and xclk */
> > > +     ret = imx290_set_clock(imx290);
> > > +     if (ret < 0) {
> > > +             dev_err(imx290->dev, "Could not set clocks\n");
> > > +             return ret;
> > > +     }
> > > +
> > >       /* Set data lane count */
> > >       ret = imx290_set_data_lanes(imx290);
> > >       if (ret < 0) {
> > > @@ -1259,14 +1345,14 @@ static int imx290_init_clk(struct imx290 *imx290)
> > >       int ret;
> > >
> > >       ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> > > -                                    "clock-frequency", &xclk_freq);
> > > +                                    "clock-frequency", &imx290->xclk_freq);
> > >       if (ret) {
> > >               dev_err(imx290->dev, "Could not get xclk frequency\n");
> > >               return ret;
> > >       }
> > >
> > > -     /* external clock must be 37.125 MHz */
> > > -     if (xclk_freq != 37125000) {
> > > +     /* external clock must be 37.125 MHz or 74.25MHz */
> > > +     if (imx290->xclk_freq != 37125000 && imx290->xclk_freq != 74250000) {
> > >               dev_err(imx290->dev, "External clock frequency %u is not supported\n",
> > >                       xclk_freq);
> > >               return -EINVAL;