[PATCH 0/3] drivers: media: imx283 improvements

[PATCH 0/3] drivers: media: imx283 improvements

[Kieran Bingham]

These changes have been tested on a Raspberry Pi 5 and an i.MX8MP
platform and fix issues detected with handling exposure on blanking
updates and correction of the formats supported and cropping.


Kieran Bingham (1):
  media: i2c: imx283: Recalculate SHR on blanking changes

Stefan Klug (2):
  media: i2c: imx283: Report correct V4L2_SEL_TGT_CROP
  media: i2c: imx283: Fix handling of unsupported mbus codes

 drivers/media/i2c/imx283.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

-- 
2.50.1

[PATCH 1/3] media: i2c: imx283: Report correct V4L2_SEL_TGT_CROP

[Kieran Bingham]

From: Stefan Klug <stefan.klug@ideasonboard.com>

The target crop rectangle is initialized with the crop of the default
sensor mode. This is incorrect when a different sensor mode gets
selected. Fix that by updating the crop rectangle when changing the
sensor mode.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx283.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 67e8bb432d10..06c4b01868c0 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -956,6 +956,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_format *fmt)
 {
+	struct v4l2_rect *crop;
 	struct v4l2_mbus_framefmt *format;
 	const struct imx283_mode *mode;
 	struct imx283 *imx283 = to_imx283(sd);
@@ -982,6 +983,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 
 	*format = fmt->format;
 
+	crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
+	*crop = mode->crop;
+
 	return 0;
 }
 
-- 
2.50.1

[PATCH 2/3] media: i2c: imx283: Recalculate SHR on blanking changes

[Kieran Bingham]

The exposure control on the imx283 is handled through the SHR register.
This value is calculated based upon the hmax and vmax registers as a
property of the total line and frame length.

Ensure that the SHR is updated whenever the blankings update and adjust
the frame intervals to ensure the correct exposure is configured on the
sensor.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx283.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 06c4b01868c0..582975ac849f 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -807,6 +807,11 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
 		dev_dbg(imx283->dev, "V4L2_CID_HBLANK : %d  HMAX : %u\n",
 			ctrl->val, imx283->hmax);
 		ret = cci_write(imx283->cci, IMX283_REG_HMAX, imx283->hmax, NULL);
+
+		/* Recompute the SHR based on the new timings */
+		shr = imx283_shr(imx283, mode, imx283->exposure->val);
+		cci_write(imx283->cci, IMX283_REG_SHR, shr, &ret);
+
 		break;
 
 	case V4L2_CID_VBLANK:
@@ -814,6 +819,11 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
 		dev_dbg(imx283->dev, "V4L2_CID_VBLANK : %d  VMAX : %u\n",
 			ctrl->val, imx283->vmax);
 		ret = cci_write(imx283->cci, IMX283_REG_VMAX, imx283->vmax, NULL);
+
+		/* Recompute the SHR based on the new timings */
+		shr = imx283_shr(imx283, mode, imx283->exposure->val);
+		cci_write(imx283->cci, IMX283_REG_SHR, shr, &ret);
+
 		break;
 
 	case V4L2_CID_ANALOGUE_GAIN:
-- 
2.50.1

[PATCH 3/3] media: i2c: imx283: Fix handling of unsupported mbus codes

[Kieran Bingham]

From: Stefan Klug <stefan.klug@ideasonboard.com>

When the code requested by imx283_set_pad_format() is not supported, a
kernel exception occurs due to dereferencing the mode variable which is
null. Fix that by correcting the code to a valid value before getting
the mode table.

While at it, remove the cases for the other unsupported codes in
get_mode_table.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx283.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 582975ac849f..48a92bc8e6f1 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -576,23 +576,31 @@ static inline struct imx283 *to_imx283(struct v4l2_subdev *sd)
 	return container_of_const(sd, struct imx283, sd);
 }
 
+static inline int get_format_code(unsigned int code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx283_mbus_codes); i++)
+		if (imx283_mbus_codes[i] == code)
+			break;
+
+	if (i >= ARRAY_SIZE(imx283_mbus_codes))
+		i = 0;
+
+	return imx283_mbus_codes[i];
+}
+
 static inline void get_mode_table(unsigned int code,
 				  const struct imx283_mode **mode_list,
 				  unsigned int *num_modes)
 {
 	switch (code) {
 	case MEDIA_BUS_FMT_SRGGB12_1X12:
-	case MEDIA_BUS_FMT_SGRBG12_1X12:
-	case MEDIA_BUS_FMT_SGBRG12_1X12:
-	case MEDIA_BUS_FMT_SBGGR12_1X12:
 		*mode_list = supported_modes_12bit;
 		*num_modes = ARRAY_SIZE(supported_modes_12bit);
 		break;
 
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
-	case MEDIA_BUS_FMT_SGRBG10_1X10:
-	case MEDIA_BUS_FMT_SGBRG10_1X10:
-	case MEDIA_BUS_FMT_SBGGR10_1X10:
 		*mode_list = supported_modes_10bit;
 		*num_modes = ARRAY_SIZE(supported_modes_10bit);
 		break;
@@ -973,6 +981,8 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx283_mode *mode_list;
 	unsigned int num_modes;
 
+	fmt->format.code = get_format_code(fmt->format.code);
+
 	get_mode_table(fmt->format.code, &mode_list, &num_modes);
 
 	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
@@ -1371,8 +1381,6 @@ static int imx283_init_controls(struct imx283 *imx283)
 
 	imx283->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx283_ctrl_ops, V4L2_CID_VFLIP,
 					  0, 1, 1, 0);
-	if (imx283->vflip)
-		imx283->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx283_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
-- 
2.50.1

Re: [PATCH 1/3] media: i2c: imx283: Report correct V4L2_SEL_TGT_CROP

[Kieran Bingham]

Quoting Kieran Bingham (2025-09-18 14:27:51)
> From: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> The target crop rectangle is initialized with the crop of the default
> sensor mode. This is incorrect when a different sensor mode gets
> selected. Fix that by updating the crop rectangle when changing the
> sensor mode.

This seems good to me and tests well.

It's arbitrary but perhaps we should add this:

Fixes: ccb4eb4496fa ("media: i2c: Add imx283 camera sensor driver")
Cc: stable@vger.kernel.org # v6.10-rc1-70-gccb4eb4496fa


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/imx283.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 67e8bb432d10..06c4b01868c0 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -956,6 +956,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_state *sd_state,
>                                  struct v4l2_subdev_format *fmt)
>  {
> +       struct v4l2_rect *crop;
>         struct v4l2_mbus_framefmt *format;
>         const struct imx283_mode *mode;
>         struct imx283 *imx283 = to_imx283(sd);
> @@ -982,6 +983,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>  
>         *format = fmt->format;
>  
> +       crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
> +       *crop = mode->crop;
> +
>         return 0;
>  }
>  
> -- 
> 2.50.1
>

Re: [PATCH 3/3] media: i2c: imx283: Fix handling of unsupported mbus codes

[Kieran Bingham]

Quoting Kieran Bingham (2025-09-18 14:27:53)
> From: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> When the code requested by imx283_set_pad_format() is not supported, a
> kernel exception occurs due to dereferencing the mode variable which is
> null. Fix that by correcting the code to a valid value before getting
> the mode table.
> 
> While at it, remove the cases for the other unsupported codes in
> get_mode_table.
> 

Here too, this fixes the original submission:

Cc: stable@vger.kernel.org # v6.10-rc1-70-gccb4eb4496fa
Fixes: ccb4eb4496fa ("media: i2c: Add imx283 camera sensor driver")
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/imx283.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 582975ac849f..48a92bc8e6f1 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -576,23 +576,31 @@ static inline struct imx283 *to_imx283(struct v4l2_subdev *sd)
>         return container_of_const(sd, struct imx283, sd);
>  }
>  
> +static inline int get_format_code(unsigned int code)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(imx283_mbus_codes); i++)
> +               if (imx283_mbus_codes[i] == code)
> +                       break;
> +
> +       if (i >= ARRAY_SIZE(imx283_mbus_codes))
> +               i = 0;
> +
> +       return imx283_mbus_codes[i];
> +}
> +
>  static inline void get_mode_table(unsigned int code,
>                                   const struct imx283_mode **mode_list,
>                                   unsigned int *num_modes)
>  {
>         switch (code) {
>         case MEDIA_BUS_FMT_SRGGB12_1X12:
> -       case MEDIA_BUS_FMT_SGRBG12_1X12:
> -       case MEDIA_BUS_FMT_SGBRG12_1X12:
> -       case MEDIA_BUS_FMT_SBGGR12_1X12:
>                 *mode_list = supported_modes_12bit;
>                 *num_modes = ARRAY_SIZE(supported_modes_12bit);
>                 break;
>  
>         case MEDIA_BUS_FMT_SRGGB10_1X10:
> -       case MEDIA_BUS_FMT_SGRBG10_1X10:
> -       case MEDIA_BUS_FMT_SGBRG10_1X10:
> -       case MEDIA_BUS_FMT_SBGGR10_1X10:
>                 *mode_list = supported_modes_10bit;
>                 *num_modes = ARRAY_SIZE(supported_modes_10bit);
>                 break;
> @@ -973,6 +981,8 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>         const struct imx283_mode *mode_list;
>         unsigned int num_modes;
>  
> +       fmt->format.code = get_format_code(fmt->format.code);
> +
>         get_mode_table(fmt->format.code, &mode_list, &num_modes);
>  
>         mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> @@ -1371,8 +1381,6 @@ static int imx283_init_controls(struct imx283 *imx283)
>  
>         imx283->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx283_ctrl_ops, V4L2_CID_VFLIP,
>                                           0, 1, 1, 0);
> -       if (imx283->vflip)
> -               imx283->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>  
>         v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx283_ctrl_ops,
>                                      V4L2_CID_TEST_PATTERN,
> -- 
> 2.50.1
>