Hi Shravan,
On Sat, Jan 21, 2023 at 09:07:13AM +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.
>
> Add support to handle multiple link frequencies.
>
> Suggested-by: Sakari Ailus <sakari.ailus@xxxxxx>
> Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> ---
> drivers/media/i2c/imx334.c | 41 ++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 309c706114d2..62b104eaa437 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;
> @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> struct fwnode_handle *ep;
> unsigned long rate;
> int ret;
> - int i;
> + int i, j;
unsigned int would be nicer.
>
> if (!fwnode)
> return -ENXIO;
> @@ -1097,11 +1107,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);
Is there a guarantee that you'll only be using the modes with the listed
frequencies? I don't see one but I might have missed it.
> + 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 +1251,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;
>
--
Kind regards,
Sakari Ailus
