Hi Shravan,

On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com wrote:

Hi Sakari,

quoted

-----Original Message-----
From: Sakari Ailus <sakari.ailus@iki.fi>
Sent: 24 January 2023 04:33 AM
To: shravan Chippa - I35088 <redacted>
Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
kernel@pengutronix.de; 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 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:

quoted

From: Shravan Chippa <redacted>

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@iki.fi>
Signed-off-by: Shravan Chippa <redacted>
---
 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)

quoted

      struct fwnode_handle *ep;
      unsigned long rate;
      int ret;
-     int i;
+     int i, j;

unsigned int would be nicer.

I will change.

quoted

quoted

      if (!fwnode)
              return -ENXIO;

@@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334

*imx334)

quoted

              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.

I don't see this being checked in imx334_set_pad_format(), for instance.

If a frequency isn't in DT, the driver isn't supposed to be using it
either.

Thanks.
shravan

quoted

quoted

+                             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)

quoted

      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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel