Re: [PATCH v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

Hi Andrey,

On Tue, May 26, 2020 at 12:27:17PM +0300, Andrey Konovalov wrote:

Hi Sakari,

Thank you for the review!

You're welcome!

On 26.05.2020 12:12, Sakari Ailus wrote:

quoted

Hi Andrey,

On Sun, May 24, 2020 at 10:25:00PM +0300, Andrey Konovalov wrote:

quoted

From: Manivannan Sadhasivam <redacted>

IMX290 operates with multiple link frequency and pixel rate combinations.
The initial driver used a single setting for both but since we now have
the lane count support in place, let's add configurable link frequency
and pixel rate.

Signed-off-by: Manivannan Sadhasivam <redacted>
Signed-off-by: Andrey Konovalov <redacted>
---
  drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
  1 file changed, 66 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a361c9ac8bd5..e800557cf423 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c

@@ -38,8 +38,6 @@
  #define IMX290_HMAX_2_720 0x19C8
  #define IMX290_HMAX_4_720 0x0CE4
-#define IMX290_DEFAULT_LINK_FREQ 445500000
-
  static const char * const imx290_supply_name[] = {
  	"vdda",
  	"vddd",

@@ -56,8 +54,6 @@ struct imx290_regval {
  struct imx290_mode {
  	u32 width;
  	u32 height;
-	u32 pixel_rate;
-	u32 link_freq_index;
  	const struct imx290_regval *data;
  	u32 data_size;

@@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
  };
  /* supported link frequencies */
-static const s64 imx290_link_freq[] = {
-	IMX290_DEFAULT_LINK_FREQ,
+static const s64 imx290_link_freq_2lanes[] = {
+	891000000, /* 1920x1080 -  2 lane */
+	594000000, /* 1280x720  -  2 lane */
+};
+static const s64 imx290_link_freq_4lanes[] = {
+	445500000, /* 1920x1080 -  4 lane */
+	297000000, /* 1280x720  -  4 lane */
  };
  /* Mode configs */

@@ -259,16 +260,12 @@ static const struct imx290_mode imx290_modes[] = {
  		.height = 1080,
  		.data = imx290_1080p_settings,
  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
-		.pixel_rate = 178200000,
-		.link_freq_index = 0,
  	},
  	{
  		.width = 1280,
  		.height = 720,
  		.data = imx290_720p_settings,
  		.data_size = ARRAY_SIZE(imx290_720p_settings),
-		.pixel_rate = 178200000,
-		.link_freq_index = 0,
  	},
  };

@@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
  	return 0;
  }
+static u8 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+	const struct imx290_mode *cur_mode = imx290->current_mode;
+
+	return (cur_mode->width == 1920) ? 0 : 1;

Could you use (imx290->current_mode - imx290_modes) / sizeof(*imx290_modes)
or something like that? It'd have fewer chances of breaking if new modes
are added.

quoted

+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+	u8 index = imx290_get_link_freq_index(imx290);
+
+	if (imx290->nlanes == 4)
+		return imx290_link_freq_4lanes[index];
+	else
+		return imx290_link_freq_2lanes[index];

Or even better: store the link frequencies to the modes themselves. They
are a property of the modes after all.

Then we will get two sets (for 2 lanes and for 4 lanes) of two modes (1080p and 720p), right?

Correct.

quoted

quoted

+}
+
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
+{
+	s64 link_freq = imx290_get_link_freq(imx290);
+	u8 nlanes = imx290->nlanes;
+
+	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	return (link_freq * 2 * nlanes / 10);
+}
+
  static int imx290_set_fmt(struct v4l2_subdev *sd,
  			  struct v4l2_subdev_pad_config *cfg,
  		      struct v4l2_subdev_format *fmt)

@@ -475,10 +498,14 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
  		format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
  	} else {
  		format = &imx290->current_format;
-		__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
-		__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, mode->pixel_rate);
-
  		imx290->current_mode = mode;
+
+		if (imx290->link_freq)
+			__v4l2_ctrl_s_ctrl(imx290->link_freq,
+					   imx290_get_link_freq_index(imx290));
+		if (imx290->pixel_rate)
+			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+						 imx290_calc_pixel_rate(imx290));
  	}
  	*format = fmt->format;

@@ -502,12 +529,11 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
  	return 0;
  }
-static int imx290_write_current_format(struct imx290 *imx290,
-				       struct v4l2_mbus_framefmt *format)
+static int imx290_write_current_format(struct imx290 *imx290)
  {
  	int ret;
-	switch (format->code) {
+	switch (imx290->current_format.code) {
  	case MEDIA_BUS_FMT_SRGGB10_1X10:
  		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
  						ARRAY_SIZE(

@@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
  		return ret;
  	}
-	/* Set current frame format */
-	ret = imx290_write_current_format(imx290, &imx290->current_format);
+	/* Apply the register values related to current frame format */
+	ret = imx290_write_current_format(imx290);
  	if (ret < 0) {
  		dev_err(imx290->dev, "Could not set frame format\n");
  		return ret;

@@ -821,12 +847,6 @@ static int imx290_probe(struct i2c_client *client)
  		goto free_err;
  	}
-	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {

This check needs to be modified to correspond to the driver's new
capabilities, not removed.

Agreed.
Do I understand correct that as the driver uses two link frequencies
for a given number of lanes now, it must check that *the both* frequencies
(for the given number of lanes) are listed in the device tree node?

Yes. The smiapp driver does this, for example.

-- 
Sakari Ailus

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