Hi Shravan On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote: > From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx> > > Update pixel_rate and link frequency for 1920x1080@30 > while changing mode. > > Add dummy ctrl cases for pixel_rate and link frequency > to avoid error while changing the modes dynamically > > Suggested-by: Sakari Ailus <sakari.ailus@xxxxxx> > Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx> > --- > drivers/media/i2c/imx334.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c > index 0315e1c9541d..8c3ba660abae 100644 > --- a/drivers/media/i2c/imx334.c > +++ b/drivers/media/i2c/imx334.c > @@ -50,6 +50,7 @@ > > /* CSI2 HW configuration */ > #define IMX334_LINK_FREQ 891000000 I guess you want to rename this one to IMX334_LINK_FREQ_891M Give our previous discussion this seems correct for the following mode { .width = 3840, .height = 2160, .hblank = 560, .vblank = 2340, .vblank_min = 90, .vblank_max = 132840, .pclk = 594000000, .link_freq_idx = 0, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_3840x2160_regs), .regs = mode_3840x2160_regs, }, }, { duration: (3840+560) * (2160+2340) / 594000000 = 33sec = 30FPS link_freq (3840+560) * (2160+2340) * 30 * 12 / 8 = 891000000 Which works well if we use min_vblank = 90 for 60FPS duration: (3840+560) * (2160+90) / 594000000 = 0.16 = 60 FPS link_freq (3840+560) * (2160+90) * 60 * 12 / 8 = 891000000 > +#define IMX334_LINK_FREQ_445M 445500000 But this doesn't work well for me { .width = 1920, .height = 1080, .hblank = 280, .vblank = 1170, .vblank_min = 90, .vblank_max = 132840, .pclk = 74250000, .link_freq_idx = 1, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs), .regs = mode_1920x1080_regs, }, }, duration: (1920+280) * (1080+1170) / 74250000 = 66msec = 16FPS link_freq = (1920+280) * (1080+1170) * 60 * 10 / 8 = 371250000 Do you agree with the above or have I missed something ? I understand you get 30 FPS with the 1920*1080 mode so could you please check in the newly introduce mode register table what are the actual values for the blankings and compute the pixel_rate and link_freq accordingly ? > #define IMX334_NUM_DATA_LANES 4 > > #define IMX334_REG_MIN 0x00 > @@ -145,6 +146,7 @@ struct imx334 { > > static const s64 link_freq[] = { > IMX334_LINK_FREQ, > + IMX334_LINK_FREQ_445M, > }; > > /* Sensor mode registers */ > @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = { > .vblank_min = 90, > .vblank_max = 132840, > .pclk = 74250000, > - .link_freq_idx = 0, > + .link_freq_idx = 1, > .reg_list = { > .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs), > .regs = mode_1920x1080_regs, > @@ -598,6 +600,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 +705,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: Same question as for patch 1/4: Do we need these safety checks for read-only controls ? Thanks j > ret = 0; > break; > -- > 2.34.1 >