diff mbox series

[v12,5/5] media: i2c: imx334: update pixel and link frequency

Message ID 20230301073412.1204574-6-shravan.chippa@microchip.com
State New
Headers
Series media: i2c: imx334: support lower bandwidth mode |

Commit Message

shravan chippa March 1, 2023, 7:34 a.m. UTC 
  From: Shravan Chippa <shravan.chippa@microchip.com>

Update pixel_rate and link frequency for 1920x1080@30
while changing mode.

Add support to handle multiple link frequencies.

Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically.

Update default link frequency from device tree max link
frequency value.

Update init_cfg() function to update the link frequency
menu_skip_mask value.

Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 51 ++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 13 deletions(-)

Comments

Sakari Ailus March 14, 2023, 7:05 p.m. UTC | #1 
Hi Shravan,

On Wed, Mar 01, 2023 at 01:04:12PM +0530, shravan kumar wrote:
> @@ -885,7 +895,13 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>  
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
> +	fmt->format.code = imx334->cur_code;

This does not compile.

> +	imx334_fill_pad_format(imx334, imx334->cur_mode, &fmt);
> +
> +	__v4l2_ctrl_modify_range(imx334->link_freq_ctrl, 0,
> +				 __fls(imx334->menu_skip_mask),
> +				 ~(imx334->menu_skip_mask),
> +				 __ffs(imx334->menu_skip_mask));

You're not holding imx334->mutex here, as you should. Also accessing
imx334->cur_code should only be done while that mutex is acquired.

What's the purpose of calling __v4l2_ctrl_modify_range() here, all these
values are static once probe() function has been called, aren't they?

I'm dropping this patch for now, taking the first four.

>  
>  	return imx334_set_pad_format(sd, sd_state, &fmt);
>  }
shravan chippa March 21, 2023, 3:41 p.m. UTC | #2 
Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: 15 March 2023 12:35 AM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux-imx@nxp.com; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v12 5/5] media: i2c: imx334: update pixel and link frequency
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan,
> 
> On Wed, Mar 01, 2023 at 01:04:12PM +0530, shravan kumar wrote:
> > @@ -885,7 +895,13 @@ static int imx334_init_pad_cfg(struct v4l2_subdev
> *sd,
> >       struct v4l2_subdev_format fmt = { 0 };
> >
> >       fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY :
> V4L2_SUBDEV_FORMAT_ACTIVE;
> > -     imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
> > +     fmt->format.code = imx334->cur_code;
> 
> This does not compile.
> 
> > +     imx334_fill_pad_format(imx334, imx334->cur_mode, &fmt);
> > +
> > +     __v4l2_ctrl_modify_range(imx334->link_freq_ctrl, 0,
> > +                              __fls(imx334->menu_skip_mask),
> > +                              ~(imx334->menu_skip_mask),
> > +                              __ffs(imx334->menu_skip_mask));
> 
> You're not holding imx334->mutex here, as you should. Also accessing
> imx334->cur_code should only be done while that mutex is acquired.
> 
> What's the purpose of calling __v4l2_ctrl_modify_range() here, all these values
> are static once probe() function has been called, aren't they?

v4l2_ctrl_new_int_menu() function limiting only upper and lower frequencies max and min, as of now only two frequencies no problem. If we have more frequencies then we may need to use__v4l2_ctrl_modify_range() with skip mask value.

> 
> I'm dropping this patch for now, taking the first four.

I will try to send it as a separate patch.
> 
> >
> >       return imx334_set_pad_format(sd, sd_state, &fmt);  }
> 
> --
> Kind regards,
> 
> Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 309c706114d2..7958c5b73f3e 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -49,7 +49,8 @@ 
 #define IMX334_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX334_LINK_FREQ	891000000
+#define IMX334_LINK_FREQ_891M	891000000
+#define IMX334_LINK_FREQ_445M	445500000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
@@ -139,12 +140,14 @@  struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
+	unsigned long menu_skip_mask;
 	u32 cur_code;
 	bool streaming;
 };
 
 static const s64 link_freq[] = {
-	IMX334_LINK_FREQ,
+	IMX334_LINK_FREQ_891M,
+	IMX334_LINK_FREQ_445M,
 };
 
 /* Sensor mode registers for 1920x1080@30fps */
@@ -468,7 +471,7 @@  static const struct imx334_mode supported_modes[] = {
 		.vblank_min = 45,
 		.vblank_max = 132840,
 		.pclk = 297000000,
-		.link_freq_idx = 0,
+		.link_freq_idx = 1,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
 			.regs = mode_1920x1080_regs,
@@ -598,6 +601,11 @@  static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
+	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
+				       mode->pclk, 1, mode->pclk);
+	if (ret)
+		return ret;
+
 	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
 				       mode->hblank, 1, mode->hblank);
 	if (ret)
@@ -698,6 +706,8 @@  static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 		pm_runtime_put(imx334->dev);
 
 		break;
+	case V4L2_CID_PIXEL_RATE:
+	case V4L2_CID_LINK_FREQ:
 	case V4L2_CID_HBLANK:
 		ret = 0;
 		break;
@@ -885,7 +895,13 @@  static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
+	fmt->format.code = imx334->cur_code;
+	imx334_fill_pad_format(imx334, imx334->cur_mode, &fmt);
+
+	__v4l2_ctrl_modify_range(imx334->link_freq_ctrl, 0,
+				 __fls(imx334->menu_skip_mask),
+				 ~(imx334->menu_skip_mask),
+				 __ffs(imx334->menu_skip_mask));
 
 	return imx334_set_pad_format(sd, sd_state, &fmt);
 }
@@ -1046,8 +1062,8 @@  static int imx334_parse_hw_config(struct imx334 *imx334)
 	};
 	struct fwnode_handle *ep;
 	unsigned long rate;
+	unsigned int i, j;
 	int ret;
-	int i;
 
 	if (!fwnode)
 		return -ENXIO;
@@ -1097,11 +1113,20 @@  static int imx334_parse_hw_config(struct imx334 *imx334)
 		goto done_endpoint_free;
 	}
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
-		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
+			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
+				set_bit(j, &imx334->menu_skip_mask);
+				break;
+			}
+		}
+
+		if (j == ARRAY_SIZE(link_freq)) {
+			ret = dev_err_probe(imx334->dev, -EINVAL,
+					    "no supported link freq found\n");
 			goto done_endpoint_free;
-
-	ret = -EINVAL;
+		}
+	}
 
 done_endpoint_free:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
@@ -1232,10 +1257,10 @@  static int imx334_init_controls(struct imx334 *imx334)
 	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&imx334_ctrl_ops,
 							V4L2_CID_LINK_FREQ,
-							ARRAY_SIZE(link_freq) -
-							1,
-							mode->link_freq_idx,
+							__fls(imx334->menu_skip_mask),
+							__ffs(imx334->menu_skip_mask),
 							link_freq);
+
 	if (imx334->link_freq_ctrl)
 		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
@@ -1302,7 +1327,7 @@  static int imx334_probe(struct i2c_client *client)
 	}
 
 	/* Set default mode to max resolution */
-	imx334->cur_mode = &supported_modes[0];
+	imx334->cur_mode = &supported_modes[__ffs(imx334->menu_skip_mask)];
 	imx334->cur_code = imx334_mbus_codes[0];
 	imx334->vblank = imx334->cur_mode->vblank;