Hi Sakari,
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxx>
> Sent: 24 January 2023 04:33 AM
> To: shravan Chippa - I35088 <Shravan.Chippa@xxxxxxxxxxxxx>
> Cc: paul.j.murphy@xxxxxxxxx; daniele.alessandrelli@xxxxxxxxx;
> mchehab@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; linux-imx@xxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH RESEND v10 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 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.
I will change.
>
> >
> > 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.
If I understand it correctly, the question here is, the listed freqeunceis and modes are one to one mapped? Then yes.
Thanks.
shravan
>
> > + 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
