[PATCH v2 00/17] media: i2c: imx290: Miscellaneous improvements

[PATCH v2 00/17] media: i2c: imx290: Miscellaneous improvements

[Laurent Pinchart]

Hello,

This patch series contains miscellaneous cleanups and improvements for
the imx290 driver. The most notable changes are usage of the V4L2 subdev
active state (11/17) and runtime PM autosuspend (13/17). Please see
invididual patches for details.

Laurent Pinchart (17):
  media: i2c: imx290: Group functions in sections
  media: i2c: imx290: Factor out subdev init and cleanup to functions
  media: i2c: imx290: Factor out control update code to a function
  media: i2c: imx290: Access link_freq_index directly
  media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()
  media: i2c: imx290: Compute pixel rate and blanking in one place
  media: i2c: imx290: Factor out black level setting to a function
  media: i2c: imx290: Factor out DT parsing to separate function
  media: i2c: imx290: Use dev_err_probe()
  media: i2c: imx290: Factor out clock initialization to separate
    function
  media: i2c: imx290: Use V4L2 subdev active state
  media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt
  media: i2c: imx290: Use runtime PM autosuspend
  media: i2c: imx290: Initialize runtime PM before subdev
  media: i2c: imx290: Configure data lanes at start time
  media: i2c: imx290: Simplify imx290_set_data_lanes()
  media: i2c: imx290: Handle error from imx290_set_data_lanes()

 drivers/media/i2c/imx290.c | 1128 +++++++++++++++++++-----------------
 1 file changed, 605 insertions(+), 523 deletions(-)


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
Regards,

Laurent Pinchart

[PATCH v2 01/17] media: i2c: imx290: Group functions in sections

[Laurent Pinchart]

Move functions around to group them in logical sections, in order to
improve readability. As a result, the IMX290_NUM_SUPPLIES macro has to
be changed. No other code change is included, only moves.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 626 +++++++++++++++++++------------------
 1 file changed, 325 insertions(+), 301 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 218ded13fd80..ca2fa57c28fe 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -152,13 +152,7 @@
 #define IMX290_PIXEL_ARRAY_RECORDING_WIDTH		1920
 #define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT		1080
 
-static const char * const imx290_supply_name[] = {
-	"vdda",
-	"vddd",
-	"vdddo",
-};
-
-#define IMX290_NUM_SUPPLIES ARRAY_SIZE(imx290_supply_name)
+#define IMX290_NUM_SUPPLIES				3
 
 struct imx290_regval {
 	u32 reg;
@@ -199,31 +193,14 @@ struct imx290 {
 	struct mutex lock;
 };
 
-struct imx290_pixfmt {
-	u32 code;
-	u8 bpp;
-};
+static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
+{
+	return container_of(_sd, struct imx290, sd);
+}
 
-static const struct imx290_pixfmt imx290_formats[] = {
-	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
-	{ MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
-};
-
-static const struct regmap_config imx290_regmap_config = {
-	.reg_bits = 16,
-	.val_bits = 8,
-};
-
-static const char * const imx290_test_pattern_menu[] = {
-	"Disabled",
-	"Sequence Pattern 1",
-	"Horizontal Color-bar Chart",
-	"Vertical Color-bar Chart",
-	"Sequence Pattern 2",
-	"Gradation Pattern 1",
-	"Gradation Pattern 2",
-	"000/555h Toggle Pattern",
-};
+/* -----------------------------------------------------------------------------
+ * Modes and formats
+ */
 
 static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
@@ -438,10 +415,19 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
 		return ARRAY_SIZE(imx290_modes_4lanes);
 }
 
-static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
-{
-	return container_of(_sd, struct imx290, sd);
-}
+struct imx290_pixfmt {
+	u32 code;
+	u8 bpp;
+};
+
+static const struct imx290_pixfmt imx290_formats[] = {
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
+	{ MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
+};
+
+/* -----------------------------------------------------------------------------
+ * Register access
+ */
 
 static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
 {
@@ -501,18 +487,94 @@ static int imx290_set_register_array(struct imx290 *imx290,
 	return 0;
 }
 
-/* Stop streaming */
-static int imx290_stop_streaming(struct imx290 *imx290)
+static int imx290_set_data_lanes(struct imx290 *imx290)
 {
-	int ret = 0;
+	int ret = 0, laneval, frsel;
 
-	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
+	switch (imx290->nlanes) {
+	case 2:
+		laneval = 0x01;
+		frsel = 0x02;
+		break;
+	case 4:
+		laneval = 0x03;
+		frsel = 0x01;
+		break;
+	default:
+		/*
+		 * We should never hit this since the data lane count is
+		 * validated in probe itself
+		 */
+		dev_err(imx290->dev, "Lane configuration not supported\n");
+		return -EINVAL;
+	}
 
-	msleep(30);
+	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
+	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
 
-	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
+	return ret;
 }
 
+static int imx290_write_current_format(struct imx290 *imx290)
+{
+	int ret;
+
+	switch (imx290->current_format.code) {
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
+						ARRAY_SIZE(
+							imx290_10bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+		break;
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+						ARRAY_SIZE(
+							imx290_12bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+		break;
+	default:
+		dev_err(imx290->dev, "Unknown pixel format\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+	return imx290->current_mode->link_freq_index;
+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+	u8 index = imx290_get_link_freq_index(imx290);
+
+	return *(imx290_link_freqs_ptr(imx290) + index);
+}
+
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
+{
+	s64 link_freq = imx290_get_link_freq(imx290);
+	u8 nlanes = imx290->nlanes;
+	u64 pixel_rate;
+
+	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	pixel_rate = link_freq * 2 * nlanes;
+	do_div(pixel_rate, imx290->bpp);
+	return pixel_rate;
+}
+
+/* ----------------------------------------------------------------------------
+ * Controls
+ */
+
 static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct imx290 *imx290 = container_of(ctrl->handler,
@@ -566,6 +628,187 @@ static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
 	.s_ctrl = imx290_set_ctrl,
 };
 
+static const char * const imx290_test_pattern_menu[] = {
+	"Disabled",
+	"Sequence Pattern 1",
+	"Horizontal Color-bar Chart",
+	"Vertical Color-bar Chart",
+	"Sequence Pattern 2",
+	"Gradation Pattern 1",
+	"Gradation Pattern 2",
+	"000/555h Toggle Pattern",
+};
+
+static int imx290_ctrl_init(struct imx290 *imx290)
+{
+	struct v4l2_fwnode_device_properties props;
+	unsigned int blank;
+	int ret;
+
+	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
+	if (ret < 0)
+		return ret;
+
+	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
+	imx290->ctrls.lock = &imx290->lock;
+
+	/*
+	 * The sensor has an analog gain and a digital gain, both controlled
+	 * through a single gain value, expressed in 0.3dB increments. Values
+	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+	 * up to 72.0dB (240) add further digital gain. Limit the range to
+	 * analog gain only, support for digital gain can be added separately
+	 * if needed.
+	 *
+	 * The IMX327 and IMX462 are largely compatible with the IMX290, but
+	 * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
+	 * gain. When support for those sensors gets added to the driver, the
+	 * gain control should be adjusted accordingly.
+	 */
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+			  V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
+
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
+			  IMX290_VMAX_DEFAULT - 2);
+
+	imx290->link_freq =
+		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       imx290_link_freqs_num(imx290) - 1, 0,
+				       imx290_link_freqs_ptr(imx290));
+	if (imx290->link_freq)
+		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       1, INT_MAX, 1,
+					       imx290_calc_pixel_rate(imx290));
+
+	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
+				     0, 0, imx290_test_pattern_menu);
+
+	blank = imx290->current_mode->hmax - imx290->current_mode->width;
+	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					   V4L2_CID_HBLANK, blank, blank, 1,
+					   blank);
+	if (imx290->hblank)
+		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
+	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					   V4L2_CID_VBLANK, blank, blank, 1,
+					   blank);
+	if (imx290->vblank)
+		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
+					&props);
+
+	imx290->sd.ctrl_handler = &imx290->ctrls;
+
+	if (imx290->ctrls.error) {
+		ret = imx290->ctrls.error;
+		v4l2_ctrl_handler_free(&imx290->ctrls);
+		return ret;
+	}
+
+	return 0;
+}
+
+/* ----------------------------------------------------------------------------
+ * Subdev operations
+ */
+
+/* Start streaming */
+static int imx290_start_streaming(struct imx290 *imx290)
+{
+	int ret;
+
+	/* Set init register settings */
+	ret = imx290_set_register_array(imx290, imx290_global_init_settings,
+					ARRAY_SIZE(
+						imx290_global_init_settings));
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set init registers\n");
+		return ret;
+	}
+
+	/* 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;
+	}
+
+	/* Apply default values of current mode */
+	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
+					imx290->current_mode->data_size);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set current mode\n");
+		return ret;
+	}
+
+	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
+			   NULL);
+	if (ret)
+		return ret;
+
+	/* Apply customized values from user */
+	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
+	if (ret) {
+		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
+		return ret;
+	}
+
+	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
+
+	msleep(30);
+
+	/* Start streaming */
+	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
+}
+
+/* Stop streaming */
+static int imx290_stop_streaming(struct imx290 *imx290)
+{
+	int ret = 0;
+
+	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
+
+	msleep(30);
+
+	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
+}
+
+static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx290 *imx290 = to_imx290(sd);
+	int ret = 0;
+
+	if (enable) {
+		ret = pm_runtime_resume_and_get(imx290->dev);
+		if (ret < 0)
+			goto unlock_and_return;
+
+		ret = imx290_start_streaming(imx290);
+		if (ret) {
+			dev_err(imx290->dev, "Start stream failed\n");
+			pm_runtime_put(imx290->dev);
+			goto unlock_and_return;
+		}
+	} else {
+		imx290_stop_streaming(imx290);
+		pm_runtime_put(imx290->dev);
+	}
+
+unlock_and_return:
+
+	return ret;
+}
+
 static struct v4l2_mbus_framefmt *
 imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
 		      u32 which)
@@ -627,30 +870,6 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
-{
-	return imx290->current_mode->link_freq_index;
-}
-
-static s64 imx290_get_link_freq(struct imx290 *imx290)
-{
-	u8 index = imx290_get_link_freq_index(imx290);
-
-	return *(imx290_link_freqs_ptr(imx290) + index);
-}
-
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
-{
-	s64 link_freq = imx290_get_link_freq(imx290);
-	u8 nlanes = imx290->nlanes;
-	u64 pixel_rate;
-
-	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = link_freq * 2 * nlanes;
-	do_div(pixel_rate, imx290->bpp);
-	return pixel_rate;
-}
-
 static int imx290_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *fmt)
@@ -774,151 +993,31 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int imx290_write_current_format(struct imx290 *imx290)
-{
-	int ret;
-
-	switch (imx290->current_format.code) {
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
-						ARRAY_SIZE(
-							imx290_10bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
-		break;
-	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
-						ARRAY_SIZE(
-							imx290_12bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
-		break;
-	default:
-		dev_err(imx290->dev, "Unknown pixel format\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/* Start streaming */
-static int imx290_start_streaming(struct imx290 *imx290)
-{
-	int ret;
-
-	/* Set init register settings */
-	ret = imx290_set_register_array(imx290, imx290_global_init_settings,
-					ARRAY_SIZE(
-						imx290_global_init_settings));
-	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set init registers\n");
-		return ret;
-	}
-
-	/* 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;
-	}
-
-	/* Apply default values of current mode */
-	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
-					imx290->current_mode->data_size);
-	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set current mode\n");
-		return ret;
-	}
-
-	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
-			   NULL);
-	if (ret)
-		return ret;
-
-	/* Apply customized values from user */
-	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
-	if (ret) {
-		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
-		return ret;
-	}
-
-	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
-
-	msleep(30);
-
-	/* Start streaming */
-	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
-}
-
-static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
-{
-	struct imx290 *imx290 = to_imx290(sd);
-	int ret = 0;
-
-	if (enable) {
-		ret = pm_runtime_resume_and_get(imx290->dev);
-		if (ret < 0)
-			goto unlock_and_return;
-
-		ret = imx290_start_streaming(imx290);
-		if (ret) {
-			dev_err(imx290->dev, "Start stream failed\n");
-			pm_runtime_put(imx290->dev);
-			goto unlock_and_return;
-		}
-	} else {
-		imx290_stop_streaming(imx290);
-		pm_runtime_put(imx290->dev);
-	}
-
-unlock_and_return:
-
-	return ret;
-}
-
-static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
-{
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(imx290->supplies); i++)
-		imx290->supplies[i].supply = imx290_supply_name[i];
-
-	return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx290->supplies),
-				       imx290->supplies);
-}
-
-static int imx290_set_data_lanes(struct imx290 *imx290)
-{
-	int ret = 0, laneval, frsel;
-
-	switch (imx290->nlanes) {
-	case 2:
-		laneval = 0x01;
-		frsel = 0x02;
-		break;
-	case 4:
-		laneval = 0x03;
-		frsel = 0x01;
-		break;
-	default:
-		/*
-		 * We should never hit this since the data lane count is
-		 * validated in probe itself
-		 */
-		dev_err(imx290->dev, "Lane configuration not supported\n");
-		return -EINVAL;
-	}
-
-	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
-	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
-	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
-
-	return ret;
-}
+static const struct v4l2_subdev_video_ops imx290_video_ops = {
+	.s_stream = imx290_set_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
+	.init_cfg = imx290_entity_init_cfg,
+	.enum_mbus_code = imx290_enum_mbus_code,
+	.enum_frame_size = imx290_enum_frame_size,
+	.get_fmt = imx290_get_fmt,
+	.set_fmt = imx290_set_fmt,
+	.get_selection = imx290_get_selection,
+};
+
+static const struct v4l2_subdev_ops imx290_subdev_ops = {
+	.video = &imx290_video_ops,
+	.pad = &imx290_pad_ops,
+};
+
+static const struct media_entity_operations imx290_subdev_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+/* ----------------------------------------------------------------------------
+ * Power management
+ */
 
 static int imx290_power_on(struct device *dev)
 {
@@ -966,105 +1065,30 @@ static const struct dev_pm_ops imx290_pm_ops = {
 	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
 };
 
-static const struct v4l2_subdev_video_ops imx290_video_ops = {
-	.s_stream = imx290_set_stream,
-};
-
-static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
-	.init_cfg = imx290_entity_init_cfg,
-	.enum_mbus_code = imx290_enum_mbus_code,
-	.enum_frame_size = imx290_enum_frame_size,
-	.get_fmt = imx290_get_fmt,
-	.set_fmt = imx290_set_fmt,
-	.get_selection = imx290_get_selection,
-};
+/* ----------------------------------------------------------------------------
+ * Probe & remove
+ */
 
-static const struct v4l2_subdev_ops imx290_subdev_ops = {
-	.video = &imx290_video_ops,
-	.pad = &imx290_pad_ops,
+static const struct regmap_config imx290_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
 };
 
-static const struct media_entity_operations imx290_subdev_entity_ops = {
-	.link_validate = v4l2_subdev_link_validate,
+static const char * const imx290_supply_name[IMX290_NUM_SUPPLIES] = {
+	"vdda",
+	"vddd",
+	"vdddo",
 };
 
-static int imx290_ctrl_init(struct imx290 *imx290)
+static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
 {
-	struct v4l2_fwnode_device_properties props;
-	unsigned int blank;
-	int ret;
+	unsigned int i;
 
-	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
-	if (ret < 0)
-		return ret;
+	for (i = 0; i < ARRAY_SIZE(imx290->supplies); i++)
+		imx290->supplies[i].supply = imx290_supply_name[i];
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
-	imx290->ctrls.lock = &imx290->lock;
-
-	/*
-	 * The sensor has an analog gain and a digital gain, both controlled
-	 * through a single gain value, expressed in 0.3dB increments. Values
-	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
-	 * up to 72.0dB (240) add further digital gain. Limit the range to
-	 * analog gain only, support for digital gain can be added separately
-	 * if needed.
-	 *
-	 * The IMX327 and IMX462 are largely compatible with the IMX290, but
-	 * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
-	 * gain. When support for those sensors gets added to the driver, the
-	 * gain control should be adjusted accordingly.
-	 */
-	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
-
-	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
-			  IMX290_VMAX_DEFAULT - 2);
-
-	imx290->link_freq =
-		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
-				       V4L2_CID_LINK_FREQ,
-				       imx290_link_freqs_num(imx290) - 1, 0,
-				       imx290_link_freqs_ptr(imx290));
-	if (imx290->link_freq)
-		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					       V4L2_CID_PIXEL_RATE,
-					       1, INT_MAX, 1,
-					       imx290_calc_pixel_rate(imx290));
-
-	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
-				     V4L2_CID_TEST_PATTERN,
-				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
-				     0, 0, imx290_test_pattern_menu);
-
-	blank = imx290->current_mode->hmax - imx290->current_mode->width;
-	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					   V4L2_CID_HBLANK, blank, blank, 1,
-					   blank);
-	if (imx290->hblank)
-		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
-	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					   V4L2_CID_VBLANK, blank, blank, 1,
-					   blank);
-	if (imx290->vblank)
-		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
-					&props);
-
-	imx290->sd.ctrl_handler = &imx290->ctrls;
-
-	if (imx290->ctrls.error) {
-		ret = imx290->ctrls.error;
-		v4l2_ctrl_handler_free(&imx290->ctrls);
-		return ret;
-	}
-
-	return 0;
+	return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx290->supplies),
+				       imx290->supplies);
 }
 
 /*
-- 
Regards,

Laurent Pinchart

[PATCH v2 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions

[Laurent Pinchart]

The probe() function is large. Make it more readable by factoring the
subdev initialization code out. While at it, rename the error labels as
the "free_" prefix isn't accurate.

No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Free control handler in imx290_subdev_init() error path
---
 drivers/media/i2c/imx290.c | 108 +++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index ca2fa57c28fe..5529bd39238f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1015,6 +1015,47 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
+static int imx290_subdev_init(struct imx290 *imx290)
+{
+	struct i2c_client *client = to_i2c_client(imx290->dev);
+	int ret;
+
+	/*
+	 * Initialize the frame format. In particular, imx290->current_mode
+	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
+	 * below relies on these fields.
+	 */
+	imx290_entity_init_cfg(&imx290->sd, NULL);
+
+	ret = imx290_ctrl_init(imx290);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Control initialization error %d\n", ret);
+		return ret;
+	}
+
+	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
+	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx290->sd.dev = imx290->dev;
+	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
+	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not register media entity\n");
+		v4l2_ctrl_handler_free(&imx290->ctrls);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void imx290_subdev_cleanup(struct imx290 *imx290)
+{
+	media_entity_cleanup(&imx290->sd.entity);
+	v4l2_ctrl_handler_free(&imx290->ctrls);
+}
+
 /* ----------------------------------------------------------------------------
  * Power management
  */
@@ -1147,10 +1188,10 @@ static int imx290_probe(struct i2c_client *client)
 	fwnode_handle_put(endpoint);
 	if (ret == -ENXIO) {
 		dev_err(dev, "Unsupported bus type, should be CSI2\n");
-		goto free_err;
+		goto err_endpoint;
 	} else if (ret) {
 		dev_err(dev, "Parsing endpoint node failed\n");
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	/* Get number of data lanes */
@@ -1158,7 +1199,7 @@ static int imx290_probe(struct i2c_client *client)
 	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
 		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
 		ret = -EINVAL;
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
@@ -1166,7 +1207,7 @@ static int imx290_probe(struct i2c_client *client)
 	if (!ep.nr_of_link_frequencies) {
 		dev_err(dev, "link-frequency property not found in DT\n");
 		ret = -EINVAL;
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	/* Check that link frequences for all the modes are in device tree */
@@ -1174,7 +1215,7 @@ static int imx290_probe(struct i2c_client *client)
 	if (fq) {
 		dev_err(dev, "Link frequency of %lld is not supported\n", fq);
 		ret = -EINVAL;
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	/* get system clock (xclk) */
@@ -1182,14 +1223,14 @@ static int imx290_probe(struct i2c_client *client)
 	if (IS_ERR(imx290->xclk)) {
 		dev_err(dev, "Could not get xclk");
 		ret = PTR_ERR(imx290->xclk);
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 				       &xclk_freq);
 	if (ret) {
 		dev_err(dev, "Could not get xclk frequency\n");
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	/* external clock must be 37.125 MHz */
@@ -1197,19 +1238,19 @@ static int imx290_probe(struct i2c_client *client)
 		dev_err(dev, "External clock frequency %u is not supported\n",
 			xclk_freq);
 		ret = -EINVAL;
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	ret = clk_set_rate(imx290->xclk, xclk_freq);
 	if (ret) {
 		dev_err(dev, "Could not set xclk frequency\n");
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	ret = imx290_get_regulators(dev, imx290);
 	if (ret < 0) {
 		dev_err(dev, "Cannot get regulators\n");
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
@@ -1217,48 +1258,26 @@ static int imx290_probe(struct i2c_client *client)
 	if (IS_ERR(imx290->rst_gpio)) {
 		dev_err(dev, "Cannot get reset gpio\n");
 		ret = PTR_ERR(imx290->rst_gpio);
-		goto free_err;
+		goto err_endpoint;
 	}
 
 	mutex_init(&imx290->lock);
 
-	/*
-	 * Initialize the frame format. In particular, imx290->current_mode
-	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
-	 * below relies on these fields.
-	 */
-	imx290_entity_init_cfg(&imx290->sd, NULL);
-
-	ret = imx290_ctrl_init(imx290);
-	if (ret < 0) {
-		dev_err(dev, "Control initialization error %d\n", ret);
-		goto free_mutex;
-	}
-
-	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
-	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	imx290->sd.dev = &client->dev;
-	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
-	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-
-	imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
-	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
-	if (ret < 0) {
-		dev_err(dev, "Could not register media entity\n");
-		goto free_ctrl;
-	}
+	ret = imx290_subdev_init(imx290);
+	if (ret)
+		goto err_mutex;
 
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
 		dev_err(dev, "Could not register v4l2 device\n");
-		goto free_entity;
+		goto err_subdev;
 	}
 
 	/* Power on the device to match runtime PM state below */
 	ret = imx290_power_on(dev);
 	if (ret < 0) {
 		dev_err(dev, "Could not power on the device\n");
-		goto free_entity;
+		goto err_subdev;
 	}
 
 	pm_runtime_set_active(dev);
@@ -1269,13 +1288,11 @@ static int imx290_probe(struct i2c_client *client)
 
 	return 0;
 
-free_entity:
-	media_entity_cleanup(&imx290->sd.entity);
-free_ctrl:
-	v4l2_ctrl_handler_free(&imx290->ctrls);
-free_mutex:
+err_subdev:
+	imx290_subdev_cleanup(imx290);
+err_mutex:
 	mutex_destroy(&imx290->lock);
-free_err:
+err_endpoint:
 	v4l2_fwnode_endpoint_free(&ep);
 
 	return ret;
@@ -1287,8 +1304,7 @@ static void imx290_remove(struct i2c_client *client)
 	struct imx290 *imx290 = to_imx290(sd);
 
 	v4l2_async_unregister_subdev(sd);
-	media_entity_cleanup(&sd->entity);
-	v4l2_ctrl_handler_free(sd->ctrl_handler);
+	imx290_subdev_cleanup(imx290);
 
 	mutex_destroy(&imx290->lock);
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions

[Alexander Stein]

Hello Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:47 CET schrieb Laurent Pinchart:
> The probe() function is large. Make it more readable by factoring the
> subdev initialization code out. While at it, rename the error labels as
> the "free_" prefix isn't accurate.
> 
> No functional change intended.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Free control handler in imx290_subdev_init() error path

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 108 +++++++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index ca2fa57c28fe..5529bd39238f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1015,6 +1015,47 @@ static const struct media_entity_operations
> imx290_subdev_entity_ops = { .link_validate = v4l2_subdev_link_validate,
>  };
> 
> +static int imx290_subdev_init(struct imx290 *imx290)
> +{
> +	struct i2c_client *client = to_i2c_client(imx290->dev);
> +	int ret;
> +
> +	/*
> +	 * Initialize the frame format. In particular, imx290->current_mode
> +	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() 
call
> +	 * below relies on these fields.
> +	 */
> +	imx290_entity_init_cfg(&imx290->sd, NULL);
> +
> +	ret = imx290_ctrl_init(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Control initialization error %d\n", 
ret);
> +		return ret;
> +	}
> +
> +	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> +	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx290->sd.dev = imx290->dev;
> +	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
> +	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not register media entity\n");
> +		v4l2_ctrl_handler_free(&imx290->ctrls);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void imx290_subdev_cleanup(struct imx290 *imx290)
> +{
> +	media_entity_cleanup(&imx290->sd.entity);
> +	v4l2_ctrl_handler_free(&imx290->ctrls);
> +}
> +
>  /*
> ---------------------------------------------------------------------------
> - * Power management
>   */
> @@ -1147,10 +1188,10 @@ static int imx290_probe(struct i2c_client *client)
>  	fwnode_handle_put(endpoint);
>  	if (ret == -ENXIO) {
>  		dev_err(dev, "Unsupported bus type, should be CSI2\n");
> -		goto free_err;
> +		goto err_endpoint;
>  	} else if (ret) {
>  		dev_err(dev, "Parsing endpoint node failed\n");
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	/* Get number of data lanes */
> @@ -1158,7 +1199,7 @@ static int imx290_probe(struct i2c_client *client)
>  	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
>  		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
>  		ret = -EINVAL;
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> @@ -1166,7 +1207,7 @@ static int imx290_probe(struct i2c_client *client)
>  	if (!ep.nr_of_link_frequencies) {
>  		dev_err(dev, "link-frequency property not found in DT\n");
>  		ret = -EINVAL;
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	/* Check that link frequences for all the modes are in device tree 
*/
> @@ -1174,7 +1215,7 @@ static int imx290_probe(struct i2c_client *client)
>  	if (fq) {
>  		dev_err(dev, "Link frequency of %lld is not supported\n", 
fq);
>  		ret = -EINVAL;
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	/* get system clock (xclk) */
> @@ -1182,14 +1223,14 @@ static int imx290_probe(struct i2c_client *client)
>  	if (IS_ERR(imx290->xclk)) {
>  		dev_err(dev, "Could not get xclk");
>  		ret = PTR_ERR(imx290->xclk);
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>  				       &xclk_freq);
>  	if (ret) {
>  		dev_err(dev, "Could not get xclk frequency\n");
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	/* external clock must be 37.125 MHz */
> @@ -1197,19 +1238,19 @@ static int imx290_probe(struct i2c_client *client)
>  		dev_err(dev, "External clock frequency %u is not 
supported\n",
>  			xclk_freq);
>  		ret = -EINVAL;
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	ret = clk_set_rate(imx290->xclk, xclk_freq);
>  	if (ret) {
>  		dev_err(dev, "Could not set xclk frequency\n");
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	ret = imx290_get_regulators(dev, imx290);
>  	if (ret < 0) {
>  		dev_err(dev, "Cannot get regulators\n");
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
> @@ -1217,48 +1258,26 @@ static int imx290_probe(struct i2c_client *client)
>  	if (IS_ERR(imx290->rst_gpio)) {
>  		dev_err(dev, "Cannot get reset gpio\n");
>  		ret = PTR_ERR(imx290->rst_gpio);
> -		goto free_err;
> +		goto err_endpoint;
>  	}
> 
>  	mutex_init(&imx290->lock);
> 
> -	/*
> -	 * Initialize the frame format. In particular, imx290->current_mode
> -	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() 
call
> -	 * below relies on these fields.
> -	 */
> -	imx290_entity_init_cfg(&imx290->sd, NULL);
> -
> -	ret = imx290_ctrl_init(imx290);
> -	if (ret < 0) {
> -		dev_err(dev, "Control initialization error %d\n", ret);
> -		goto free_mutex;
> -	}
> -
> -	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> -	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> -	imx290->sd.dev = &client->dev;
> -	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
> -	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -
> -	imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> -	if (ret < 0) {
> -		dev_err(dev, "Could not register media entity\n");
> -		goto free_ctrl;
> -	}
> +	ret = imx290_subdev_init(imx290);
> +	if (ret)
> +		goto err_mutex;
> 
>  	ret = v4l2_async_register_subdev(&imx290->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Could not register v4l2 device\n");
> -		goto free_entity;
> +		goto err_subdev;
>  	}
> 
>  	/* Power on the device to match runtime PM state below */
>  	ret = imx290_power_on(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "Could not power on the device\n");
> -		goto free_entity;
> +		goto err_subdev;
>  	}
> 
>  	pm_runtime_set_active(dev);
> @@ -1269,13 +1288,11 @@ static int imx290_probe(struct i2c_client *client)
> 
>  	return 0;
> 
> -free_entity:
> -	media_entity_cleanup(&imx290->sd.entity);
> -free_ctrl:
> -	v4l2_ctrl_handler_free(&imx290->ctrls);
> -free_mutex:
> +err_subdev:
> +	imx290_subdev_cleanup(imx290);
> +err_mutex:
>  	mutex_destroy(&imx290->lock);
> -free_err:
> +err_endpoint:
>  	v4l2_fwnode_endpoint_free(&ep);
> 
>  	return ret;
> @@ -1287,8 +1304,7 @@ static void imx290_remove(struct i2c_client *client)
>  	struct imx290 *imx290 = to_imx290(sd);
> 
>  	v4l2_async_unregister_subdev(sd);
> -	media_entity_cleanup(&sd->entity);
> -	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +	imx290_subdev_cleanup(imx290);
> 
>  	mutex_destroy(&imx290->lock);

[PATCH v2 03/17] media: i2c: imx290: Factor out control update code to a function

[Laurent Pinchart]

Move the control update code to a separate function to group it with all
the control-related code and make imx290_set_fmt() more readable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Correctly handle the case where imx290_ctrl_update() gets called
  before controls are initialized
---
 drivers/media/i2c/imx290.c | 43 ++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 5529bd39238f..991e7285c40c 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -639,6 +639,28 @@ static const char * const imx290_test_pattern_menu[] = {
 	"000/555h Toggle Pattern",
 };
 
+static void imx290_ctrl_update(struct imx290 *imx290,
+			       const struct imx290_mode *mode)
+{
+	unsigned int hblank = mode->hmax - mode->width;
+	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+
+	/*
+	 * This function may be called from imx290_set_fmt() before controls
+	 * get created by imx290_ctrl_init(). Return immediately in that case.
+	 */
+	if (!imx290->ctrls.lock)
+		return;
+
+	__v4l2_ctrl_s_ctrl(imx290->link_freq,
+			   imx290_get_link_freq_index(imx290));
+	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+				 imx290_calc_pixel_rate(imx290));
+
+	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
+	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
+}
+
 static int imx290_ctrl_init(struct imx290 *imx290)
 {
 	struct v4l2_fwnode_device_properties props;
@@ -904,26 +926,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		imx290->current_mode = mode;
 		imx290->bpp = imx290_formats[i].bpp;
 
-		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));
-
-		if (imx290->hblank) {
-			unsigned int hblank = mode->hmax - mode->width;
-
-			__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank,
-						 1, hblank);
-		}
-
-		if (imx290->vblank) {
-			unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
-
-			__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank,
-						 1, vblank);
-		}
+		imx290_ctrl_update(imx290, mode);
 	}
 
 	*format = fmt->format;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/17] media: i2c: imx290: Factor out control update code to a function

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:48 CET schrieb Laurent Pinchart:
> Move the control update code to a separate function to group it with all
> the control-related code and make imx290_set_fmt() more readable.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Correctly handle the case where imx290_ctrl_update() gets called
>   before controls are initialized

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 43 ++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 5529bd39238f..991e7285c40c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -639,6 +639,28 @@ static const char * const imx290_test_pattern_menu[] =
> { "000/555h Toggle Pattern",
>  };
> 
> +static void imx290_ctrl_update(struct imx290 *imx290,
> +			       const struct imx290_mode *mode)
> +{
> +	unsigned int hblank = mode->hmax - mode->width;
> +	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> +
> +	/*
> +	 * This function may be called from imx290_set_fmt() before controls
> +	 * get created by imx290_ctrl_init(). Return immediately in that 
case.
> +	 */
> +	if (!imx290->ctrls.lock)
> +		return;
> +
> +	__v4l2_ctrl_s_ctrl(imx290->link_freq,
> +			   imx290_get_link_freq_index(imx290));
> +	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> +				 imx290_calc_pixel_rate(imx290));
> +
> +	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> +	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> +}
> +
>  static int imx290_ctrl_init(struct imx290 *imx290)
>  {
>  	struct v4l2_fwnode_device_properties props;
> @@ -904,26 +926,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  		imx290->current_mode = mode;
>  		imx290->bpp = imx290_formats[i].bpp;
> 
> -		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));
> -
> -		if (imx290->hblank) {
> -			unsigned int hblank = mode->hmax - mode->width;
> -
> -			__v4l2_ctrl_modify_range(imx290->hblank, hblank, 
hblank,
> -						 1, hblank);
> -		}
> -
> -		if (imx290->vblank) {
> -			unsigned int vblank = IMX290_VMAX_DEFAULT - 
mode->height;
> -
> -			__v4l2_ctrl_modify_range(imx290->vblank, vblank, 
vblank,
> -						 1, vblank);
> -		}
> +		imx290_ctrl_update(imx290, mode);
>  	}
> 
>  	*format = fmt->format;

[PATCH v2 04/17] media: i2c: imx290: Access link_freq_index directly

[Laurent Pinchart]

The imx290_get_link_freq_index() function hides the fact that it relies
on the imx290 current_mode field, which obfuscates the code instead of
making it more readable. Inline it in the callers, and use the mode
pointer we already have in imx290_ctrl_update() instead of using the
current_mode field.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 991e7285c40c..4ad6eab4f2e2 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,14 +547,9 @@ static int imx290_write_current_format(struct imx290 *imx290)
 	return 0;
 }
 
-static inline u8 imx290_get_link_freq_index(struct imx290 *imx290)
-{
-	return imx290->current_mode->link_freq_index;
-}
-
 static s64 imx290_get_link_freq(struct imx290 *imx290)
 {
-	u8 index = imx290_get_link_freq_index(imx290);
+	u8 index = imx290->current_mode->link_freq_index;
 
 	return *(imx290_link_freqs_ptr(imx290) + index);
 }
@@ -652,8 +647,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 	if (!imx290->ctrls.lock)
 		return;
 
-	__v4l2_ctrl_s_ctrl(imx290->link_freq,
-			   imx290_get_link_freq_index(imx290));
+	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
 				 imx290_calc_pixel_rate(imx290));
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()

[Laurent Pinchart]

Avoid accessing the imx290 current_format and current_mode fields in
imx290_calc_pixel_rate() to prepare for the removal of those fields.
Among the two callers of the function, imx290_ctrl_update() has an
explicit mode pointer already, and we can also give it a format pointer.
Use those explicitly.

While at it, inline the imx290_get_link_freq() function in
imx290_calc_pixel_rate() as it is only called there.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Drop format argument from imx290_calc_pixel_rate()
---
 drivers/media/i2c/imx290.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 4ad6eab4f2e2..8f141df74e2f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,21 +547,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
 	return 0;
 }
 
-static s64 imx290_get_link_freq(struct imx290 *imx290)
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
+				  const struct v4l2_mbus_framefmt *format,
+				  const struct imx290_mode *mode)
 {
-	u8 index = imx290->current_mode->link_freq_index;
-
-	return *(imx290_link_freqs_ptr(imx290) + index);
-}
-
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
-{
-	s64 link_freq = imx290_get_link_freq(imx290);
-	u8 nlanes = imx290->nlanes;
+	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
 	u64 pixel_rate;
 
 	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = link_freq * 2 * nlanes;
+	pixel_rate = link_freq * 2 * imx290->nlanes;
 	do_div(pixel_rate, imx290->bpp);
 	return pixel_rate;
 }
@@ -635,6 +629,7 @@ static const char * const imx290_test_pattern_menu[] = {
 };
 
 static void imx290_ctrl_update(struct imx290 *imx290,
+			       const struct v4l2_mbus_framefmt *format,
 			       const struct imx290_mode *mode)
 {
 	unsigned int hblank = mode->hmax - mode->width;
@@ -649,7 +644,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
-				 imx290_calc_pixel_rate(imx290));
+				 imx290_calc_pixel_rate(imx290, format, mode));
 
 	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
 	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
@@ -659,6 +654,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 {
 	struct v4l2_fwnode_device_properties props;
 	unsigned int blank;
+	u64 pixel_rate;
 	int ret;
 
 	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
@@ -696,10 +692,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	if (imx290->link_freq)
 		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	pixel_rate = imx290_calc_pixel_rate(imx290, &imx290->current_format,
+					    imx290->current_mode);
 	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE,
-					       1, INT_MAX, 1,
-					       imx290_calc_pixel_rate(imx290));
+					       1, INT_MAX, 1, pixel_rate);
 
 	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
@@ -920,7 +917,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		imx290->current_mode = mode;
 		imx290->bpp = imx290_formats[i].bpp;
 
-		imx290_ctrl_update(imx290, mode);
+		imx290_ctrl_update(imx290, &fmt->format, mode);
 	}
 
 	*format = fmt->format;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:50 CET schrieb Laurent Pinchart:
> Avoid accessing the imx290 current_format and current_mode fields in
> imx290_calc_pixel_rate() to prepare for the removal of those fields.
> Among the two callers of the function, imx290_ctrl_update() has an
> explicit mode pointer already, and we can also give it a format pointer.
> Use those explicitly.
> 
> While at it, inline the imx290_get_link_freq() function in
> imx290_calc_pixel_rate() as it is only called there.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Drop format argument from imx290_calc_pixel_rate()
> ---
>  drivers/media/i2c/imx290.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4ad6eab4f2e2..8f141df74e2f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -547,21 +547,15 @@ static int imx290_write_current_format(struct imx290
> *imx290) return 0;
>  }
> 
> -static s64 imx290_get_link_freq(struct imx290 *imx290)
> +static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
> +				  const struct v4l2_mbus_framefmt 
*format,

Maybe some mishap, but the format parameter is still here.
Otherwise:
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> +				  const struct imx290_mode *mode)
>  {
> -	u8 index = imx290->current_mode->link_freq_index;
> -
> -	return *(imx290_link_freqs_ptr(imx290) + index);
> -}
> -
> -static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
> -{
> -	s64 link_freq = imx290_get_link_freq(imx290);
> -	u8 nlanes = imx290->nlanes;
> +	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode-
>link_freq_index];
>  	u64 pixel_rate;
> 
>  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -	pixel_rate = link_freq * 2 * nlanes;
> +	pixel_rate = link_freq * 2 * imx290->nlanes;
>  	do_div(pixel_rate, imx290->bpp);
>  	return pixel_rate;
>  }
> @@ -635,6 +629,7 @@ static const char * const imx290_test_pattern_menu[] = {
> };
> 
>  static void imx290_ctrl_update(struct imx290 *imx290,
> +			       const struct v4l2_mbus_framefmt *format,
>  			       const struct imx290_mode *mode)
>  {
>  	unsigned int hblank = mode->hmax - mode->width;
> @@ -649,7 +644,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> 
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
>  	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> -				 imx290_calc_pixel_rate(imx290));
> +				 imx290_calc_pixel_rate(imx290, 
format, mode));
> 
>  	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> @@ -659,6 +654,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  {
>  	struct v4l2_fwnode_device_properties props;
>  	unsigned int blank;
> +	u64 pixel_rate;
>  	int ret;
> 
>  	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
> @@ -696,10 +692,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	if (imx290->link_freq)
>  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
> +	pixel_rate = imx290_calc_pixel_rate(imx290, &imx290->current_format,
> +					    imx290->current_mode);
>  	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, 
&imx290_ctrl_ops,
>  					       V4L2_CID_PIXEL_RATE,
> -					       1, INT_MAX, 1,
> -					       
imx290_calc_pixel_rate(imx290));
> +					       1, INT_MAX, 1, 
pixel_rate);
> 
>  	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
>  				     V4L2_CID_TEST_PATTERN,
> @@ -920,7 +917,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  		imx290->current_mode = mode;
>  		imx290->bpp = imx290_formats[i].bpp;
> 
> -		imx290_ctrl_update(imx290, mode);
> +		imx290_ctrl_update(imx290, &fmt->format, mode);
>  	}
> 
>  	*format = fmt->format;

Re: [PATCH v2 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate()

[Laurent Pinchart]

Hi Alexander,

On Mon, Jan 16, 2023 at 11:22:49AM +0100, Alexander Stein wrote:
> Am Samstag, 14. Januar 2023, 18:17:50 CET schrieb Laurent Pinchart:
> > Avoid accessing the imx290 current_format and current_mode fields in
> > imx290_calc_pixel_rate() to prepare for the removal of those fields.
> > Among the two callers of the function, imx290_ctrl_update() has an
> > explicit mode pointer already, and we can also give it a format pointer.
> > Use those explicitly.
> > 
> > While at it, inline the imx290_get_link_freq() function in
> > imx290_calc_pixel_rate() as it is only called there.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Drop format argument from imx290_calc_pixel_rate()
> > ---
> >  drivers/media/i2c/imx290.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 4ad6eab4f2e2..8f141df74e2f 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -547,21 +547,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
> >  	return 0;
> >  }
> > 
> > -static s64 imx290_get_link_freq(struct imx290 *imx290)
> > +static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
> > +				  const struct v4l2_mbus_framefmt *format,
> 
> Maybe some mishap, but the format parameter is still here.

Hmmmm... Maybe I got it wrong when rebasing. I'll fix it in v3.

> Otherwise:
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> > +				  const struct imx290_mode *mode)
> >  {
> > -	u8 index = imx290->current_mode->link_freq_index;
> > -
> > -	return *(imx290_link_freqs_ptr(imx290) + index);
> > -}
> > -
> > -static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
> > -{
> > -	s64 link_freq = imx290_get_link_freq(imx290);
> > -	u8 nlanes = imx290->nlanes;
> > +	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
> >  	u64 pixel_rate;
> > 
> >  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > -	pixel_rate = link_freq * 2 * nlanes;
> > +	pixel_rate = link_freq * 2 * imx290->nlanes;
> >  	do_div(pixel_rate, imx290->bpp);
> >  	return pixel_rate;
> >  }
> > @@ -635,6 +629,7 @@ static const char * const imx290_test_pattern_menu[] = {
> > };
> > 
> >  static void imx290_ctrl_update(struct imx290 *imx290,
> > +			       const struct v4l2_mbus_framefmt *format,
> >  			       const struct imx290_mode *mode)
> >  {
> >  	unsigned int hblank = mode->hmax - mode->width;
> > @@ -649,7 +644,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> > 
> >  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> >  	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> > -				 imx290_calc_pixel_rate(imx290));
> > +				 imx290_calc_pixel_rate(imx290, format, mode));
> > 
> >  	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> >  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> > @@ -659,6 +654,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >  {
> >  	struct v4l2_fwnode_device_properties props;
> >  	unsigned int blank;
> > +	u64 pixel_rate;
> >  	int ret;
> > 
> >  	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
> > @@ -696,10 +692,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >  	if (imx290->link_freq)
> >  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > 
> > +	pixel_rate = imx290_calc_pixel_rate(imx290, &imx290->current_format,
> > +					    imx290->current_mode);
> >  	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >  					       V4L2_CID_PIXEL_RATE,
> > -					       1, INT_MAX, 1,
> > -					       imx290_calc_pixel_rate(imx290));
> > +					       1, INT_MAX, 1, pixel_rate);
> > 
> >  	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
> >  				     V4L2_CID_TEST_PATTERN,
> > @@ -920,7 +917,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  		imx290->current_mode = mode;
> >  		imx290->bpp = imx290_formats[i].bpp;
> > 
> > -		imx290_ctrl_update(imx290, mode);
> > +		imx290_ctrl_update(imx290, &fmt->format, mode);
> >  	}
> > 
> >  	*format = fmt->format;

-- 
Regards,

Laurent Pinchart

[PATCH v2 06/17] media: i2c: imx290: Compute pixel rate and blanking in one place

[Laurent Pinchart]

The hblank, vblank, pixel rate and link frequency values and limits are
currently computed when creating controls, in imx290_ctrl_init(), and
updated in imx290_ctrl_update(). This duplicates the logic in different
places. Simplify the code by setting the control values and limits to
hardcoded values when creating the controls, and call
imx290_ctrl_update() to then update them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:

- Fix typo in comment
---
 drivers/media/i2c/imx290.c | 46 +++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 8f141df74e2f..66cb16f20852 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -547,19 +547,6 @@ static int imx290_write_current_format(struct imx290 *imx290)
 	return 0;
 }
 
-static u64 imx290_calc_pixel_rate(struct imx290 *imx290,
-				  const struct v4l2_mbus_framefmt *format,
-				  const struct imx290_mode *mode)
-{
-	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
-	u64 pixel_rate;
-
-	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = link_freq * 2 * imx290->nlanes;
-	do_div(pixel_rate, imx290->bpp);
-	return pixel_rate;
-}
-
 /* ----------------------------------------------------------------------------
  * Controls
  */
@@ -634,6 +621,8 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 {
 	unsigned int hblank = mode->hmax - mode->width;
 	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
+	u64 pixel_rate;
 
 	/*
 	 * This function may be called from imx290_set_fmt() before controls
@@ -642,9 +631,12 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 	if (!imx290->ctrls.lock)
 		return;
 
+	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	pixel_rate = link_freq * 2 * imx290->nlanes;
+	do_div(pixel_rate, imx290->bpp);
+
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
-	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
-				 imx290_calc_pixel_rate(imx290, format, mode));
+	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
 
 	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
 	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
@@ -653,8 +645,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 static int imx290_ctrl_init(struct imx290 *imx290)
 {
 	struct v4l2_fwnode_device_properties props;
-	unsigned int blank;
-	u64 pixel_rate;
 	int ret;
 
 	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
@@ -684,6 +674,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
 			  IMX290_VMAX_DEFAULT - 2);
 
+	/*
+	 * Set the link frequency, pixel rate, horizontal blanking and vertical
+	 * blanking to hardcoded values, they will be updated by
+	 * imx290_ctrl_update().
+	 */
 	imx290->link_freq =
 		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
 				       V4L2_CID_LINK_FREQ,
@@ -692,28 +687,22 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	if (imx290->link_freq)
 		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	pixel_rate = imx290_calc_pixel_rate(imx290, &imx290->current_format,
-					    imx290->current_mode);
 	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE,
-					       1, INT_MAX, 1, pixel_rate);
+					       1, INT_MAX, 1, 1);
 
 	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
 				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
 				     0, 0, imx290_test_pattern_menu);
 
-	blank = imx290->current_mode->hmax - imx290->current_mode->width;
 	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					   V4L2_CID_HBLANK, blank, blank, 1,
-					   blank);
+					   V4L2_CID_HBLANK, 1, 1, 1, 1);
 	if (imx290->hblank)
 		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
 	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					   V4L2_CID_VBLANK, blank, blank, 1,
-					   blank);
+					   V4L2_CID_VBLANK, 1, 1, 1, 1);
 	if (imx290->vblank)
 		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
@@ -728,6 +717,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 		return ret;
 	}
 
+	mutex_lock(imx290->ctrls.lock);
+	imx290_ctrl_update(imx290, &imx290->current_format,
+			   imx290->current_mode);
+	mutex_unlock(imx290->ctrls.lock);
+
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 07/17] media: i2c: imx290: Factor out black level setting to a function

[Laurent Pinchart]

The black level programmed in the BLKLEVEL register depends on the
output format. The black level value computation is currently performed
in imx290_set_ctrl(), in addition to having different black level values
in the output-specific register value tables. Move it to a separate
function to simplify the imx290_set_ctrl() code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v1:

- Drop bpp variable in imx290_write_current_format()
---
 drivers/media/i2c/imx290.c | 50 ++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 66cb16f20852..0dc536893ebf 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -152,6 +152,9 @@
 #define IMX290_PIXEL_ARRAY_RECORDING_WIDTH		1920
 #define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT		1080
 
+/* Equivalent value for 16bpp */
+#define IMX290_BLACK_LEVEL_DEFAULT			3840
+
 #define IMX290_NUM_SUPPLIES				3
 
 struct imx290_regval {
@@ -315,7 +318,6 @@ static const struct imx290_regval imx290_10bit_settings[] = {
 	{ IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
 	{ IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
 	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
-	{ IMX290_BLKLEVEL, 60 },
 };
 
 static const struct imx290_regval imx290_12bit_settings[] = {
@@ -325,7 +327,6 @@ static const struct imx290_regval imx290_12bit_settings[] = {
 	{ IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
 	{ IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
 	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
-	{ IMX290_BLKLEVEL, 240 },
 };
 
 /* supported link frequencies */
@@ -516,35 +517,40 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
 	return ret;
 }
 
+static int imx290_set_black_level(struct imx290 *imx290,
+				  unsigned int black_level, int *err)
+{
+	return imx290_write(imx290, IMX290_BLKLEVEL,
+			    black_level >> (16 - imx290->bpp), err);
+}
+
 static int imx290_write_current_format(struct imx290 *imx290)
 {
+	const struct imx290_regval *regs;
+	unsigned int num_regs;
 	int ret;
 
 	switch (imx290->current_format.code) {
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
-						ARRAY_SIZE(
-							imx290_10bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
+		regs = imx290_10bit_settings;
+		num_regs = ARRAY_SIZE(imx290_10bit_settings);
 		break;
 	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
-						ARRAY_SIZE(
-							imx290_12bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
+		regs = imx290_12bit_settings;
+		num_regs = ARRAY_SIZE(imx290_12bit_settings);
 		break;
 	default:
 		dev_err(imx290->dev, "Unknown pixel format\n");
 		return -EINVAL;
 	}
 
-	return 0;
+	ret = imx290_set_register_array(imx290, regs, num_regs);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set format registers\n");
+		return ret;
+	}
+
+	return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
 }
 
 /* ----------------------------------------------------------------------------
@@ -573,7 +579,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
-			imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
+			imx290_set_black_level(imx290, 0, &ret);
 			usleep_range(10000, 11000);
 			imx290_write(imx290, IMX290_PGCTRL,
 				     (u8)(IMX290_PGCTRL_REGEN |
@@ -582,12 +588,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		} else {
 			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
 			usleep_range(10000, 11000);
-			if (imx290->bpp == 10)
-				imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
-					     &ret);
-			else /* 12 bits per pixel */
-				imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
-					     &ret);
+			imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
+					       &ret);
 		}
 		break;
 	default:
-- 
Regards,

Laurent Pinchart

[PATCH v2 08/17] media: i2c: imx290: Factor out DT parsing to separate function

[Laurent Pinchart]

Make the probe() function more readable by factoring out the DT parsing
code to a separate function. No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 43 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 0dc536893ebf..18c1e5c429a2 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
 	return 0;
 }
 
-static int imx290_probe(struct i2c_client *client)
+static int imx290_parse_dt(struct imx290 *imx290)
 {
-	struct device *dev = &client->dev;
-	struct fwnode_handle *endpoint;
 	/* Only CSI2 is supported for now: */
 	struct v4l2_fwnode_endpoint ep = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	struct imx290 *imx290;
-	u32 xclk_freq;
+	struct fwnode_handle *endpoint;
+	int ret;
 	s64 fq;
-	int ret;
 
-	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
-	if (!imx290)
-		return -ENOMEM;
-
-	imx290->dev = dev;
-	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
-	if (IS_ERR(imx290->regmap)) {
-		dev_err(dev, "Unable to initialize I2C\n");
-		return -ENODEV;
-	}
-
-	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
 	if (!endpoint) {
-		dev_err(dev, "Endpoint node not found\n");
+		dev_err(imx290->dev, "Endpoint node not found\n");
 		return -EINVAL;
 	}
 
 	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
 	fwnode_handle_put(endpoint);
 	if (ret == -ENXIO) {
-		dev_err(dev, "Unsupported bus type, should be CSI2\n");
-		goto err_endpoint;
+		dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n");
+		goto done;
 	} else if (ret) {
-		dev_err(dev, "Parsing endpoint node failed\n");
-		goto err_endpoint;
+		dev_err(imx290->dev, "Parsing endpoint node failed\n");
+		goto done;
 	}
 
 	/* Get number of data lanes */
 	imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
 	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
-		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
+		dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290->nlanes);
 		ret = -EINVAL;
-		goto err_endpoint;
+		goto done;
 	}
 
-	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
+	dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes);
 
 	if (!ep.nr_of_link_frequencies) {
-		dev_err(dev, "link-frequency property not found in DT\n");
+		dev_err(imx290->dev, "link-frequency property not found in DT\n");
 		ret = -EINVAL;
-		goto err_endpoint;
+		goto done;
 	}
 
 	/* Check that link frequences for all the modes are in device tree */
 	fq = imx290_check_link_freqs(imx290, &ep);
 	if (fq) {
-		dev_err(dev, "Link frequency of %lld is not supported\n", fq);
+		dev_err(imx290->dev, "Link frequency of %lld is not supported\n",
+			fq);
 		ret = -EINVAL;
-		goto err_endpoint;
+		goto done;
 	}
 
+	ret = 0;
+
+done:
+	v4l2_fwnode_endpoint_free(&ep);
+	return ret;
+}
+
+static int imx290_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct imx290 *imx290;
+	u32 xclk_freq;
+	int ret;
+
+	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
+	if (!imx290)
+		return -ENOMEM;
+
+	imx290->dev = dev;
+	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
+	if (IS_ERR(imx290->regmap)) {
+		dev_err(dev, "Unable to initialize I2C\n");
+		return -ENODEV;
+	}
+
+	ret = imx290_parse_dt(imx290);
+	if (ret)
+		return ret;
+
 	/* get system clock (xclk) */
 	imx290->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(imx290->xclk)) {
 		dev_err(dev, "Could not get xclk");
-		ret = PTR_ERR(imx290->xclk);
-		goto err_endpoint;
+		return PTR_ERR(imx290->xclk);
 	}
 
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 				       &xclk_freq);
 	if (ret) {
 		dev_err(dev, "Could not get xclk frequency\n");
-		goto err_endpoint;
+		return ret;
 	}
 
 	/* external clock must be 37.125 MHz */
 	if (xclk_freq != 37125000) {
 		dev_err(dev, "External clock frequency %u is not supported\n",
 			xclk_freq);
-		ret = -EINVAL;
-		goto err_endpoint;
+		return -EINVAL;
 	}
 
 	ret = clk_set_rate(imx290->xclk, xclk_freq);
 	if (ret) {
 		dev_err(dev, "Could not set xclk frequency\n");
-		goto err_endpoint;
+		return ret;
 	}
 
 	ret = imx290_get_regulators(dev, imx290);
 	if (ret < 0) {
 		dev_err(dev, "Cannot get regulators\n");
-		goto err_endpoint;
+		return ret;
 	}
 
 	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
 						   GPIOD_OUT_HIGH);
 	if (IS_ERR(imx290->rst_gpio)) {
 		dev_err(dev, "Cannot get reset gpio\n");
-		ret = PTR_ERR(imx290->rst_gpio);
-		goto err_endpoint;
+		return PTR_ERR(imx290->rst_gpio);
 	}
 
 	mutex_init(&imx290->lock);
@@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client)
 	pm_runtime_enable(dev);
 	pm_runtime_idle(dev);
 
-	v4l2_fwnode_endpoint_free(&ep);
-
 	return 0;
 
 err_subdev:
 	imx290_subdev_cleanup(imx290);
 err_mutex:
 	mutex_destroy(&imx290->lock);
-err_endpoint:
-	v4l2_fwnode_endpoint_free(&ep);
 
 	return ret;
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/17] media: i2c: imx290: Factor out DT parsing to separate function

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart:
> Make the probe() function more readable by factoring out the DT parsing
> code to a separate function. No functional change intended.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 0dc536893ebf..18c1e5c429a2 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct
> imx290 *imx290, return 0;
>  }
> 
> -static int imx290_probe(struct i2c_client *client)
> +static int imx290_parse_dt(struct imx290 *imx290)
>  {
> -	struct device *dev = &client->dev;
> -	struct fwnode_handle *endpoint;
>  	/* Only CSI2 is supported for now: */
>  	struct v4l2_fwnode_endpoint ep = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	struct imx290 *imx290;
> -	u32 xclk_freq;
> +	struct fwnode_handle *endpoint;
> +	int ret;
>  	s64 fq;
> -	int ret;
> 
> -	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> -	if (!imx290)
> -		return -ENOMEM;
> -
> -	imx290->dev = dev;
> -	imx290->regmap = devm_regmap_init_i2c(client, 
&imx290_regmap_config);
> -	if (IS_ERR(imx290->regmap)) {
> -		dev_err(dev, "Unable to initialize I2C\n");
> -		return -ENODEV;
> -	}
> -
> -	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), 
NULL);
>  	if (!endpoint) {
> -		dev_err(dev, "Endpoint node not found\n");
> +		dev_err(imx290->dev, "Endpoint node not found\n");
>  		return -EINVAL;
>  	}
> 
>  	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
>  	fwnode_handle_put(endpoint);
>  	if (ret == -ENXIO) {
> -		dev_err(dev, "Unsupported bus type, should be CSI2\n");
> -		goto err_endpoint;
> +		dev_err(imx290->dev, "Unsupported bus type, should be 
CSI2\n");
> +		goto done;
>  	} else if (ret) {
> -		dev_err(dev, "Parsing endpoint node failed\n");
> -		goto err_endpoint;
> +		dev_err(imx290->dev, "Parsing endpoint node failed\n");
> +		goto done;
>  	}
> 
>  	/* Get number of data lanes */
>  	imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
>  	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
> -		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
> +		dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290-
>nlanes);
>  		ret = -EINVAL;
> -		goto err_endpoint;
> +		goto done;
>  	}
> 
> -	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> +	dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes);
> 
>  	if (!ep.nr_of_link_frequencies) {
> -		dev_err(dev, "link-frequency property not found in DT\n");
> +		dev_err(imx290->dev, "link-frequency property not found in 
DT\n");
>  		ret = -EINVAL;
> -		goto err_endpoint;
> +		goto done;
>  	}
> 
>  	/* Check that link frequences for all the modes are in device tree 
*/
>  	fq = imx290_check_link_freqs(imx290, &ep);
>  	if (fq) {
> -		dev_err(dev, "Link frequency of %lld is not supported\n", 
fq);
> +		dev_err(imx290->dev, "Link frequency of %lld is not 
supported\n",
> +			fq);
>  		ret = -EINVAL;
> -		goto err_endpoint;
> +		goto done;
>  	}
> 
> +	ret = 0;
> +
> +done:
> +	v4l2_fwnode_endpoint_free(&ep);
> +	return ret;
> +}
> +
> +static int imx290_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct imx290 *imx290;
> +	u32 xclk_freq;
> +	int ret;
> +
> +	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> +	if (!imx290)
> +		return -ENOMEM;
> +
> +	imx290->dev = dev;
> +	imx290->regmap = devm_regmap_init_i2c(client, 
&imx290_regmap_config);
> +	if (IS_ERR(imx290->regmap)) {
> +		dev_err(dev, "Unable to initialize I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = imx290_parse_dt(imx290);
> +	if (ret)
> +		return ret;
> +
>  	/* get system clock (xclk) */
>  	imx290->xclk = devm_clk_get(dev, "xclk");
>  	if (IS_ERR(imx290->xclk)) {
>  		dev_err(dev, "Could not get xclk");
> -		ret = PTR_ERR(imx290->xclk);
> -		goto err_endpoint;
> +		return PTR_ERR(imx290->xclk);

Please use dev_err_probe() here.

>  	}
> 
>  	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>  				       &xclk_freq);
>  	if (ret) {
>  		dev_err(dev, "Could not get xclk frequency\n");
> -		goto err_endpoint;
> +		return ret;
>  	}
> 
>  	/* external clock must be 37.125 MHz */
>  	if (xclk_freq != 37125000) {
>  		dev_err(dev, "External clock frequency %u is not 
supported\n",
>  			xclk_freq);
> -		ret = -EINVAL;
> -		goto err_endpoint;
> +		return -EINVAL;
>  	}
> 
>  	ret = clk_set_rate(imx290->xclk, xclk_freq);
>  	if (ret) {
>  		dev_err(dev, "Could not set xclk frequency\n");
> -		goto err_endpoint;
> +		return ret;
>  	}
> 
>  	ret = imx290_get_regulators(dev, imx290);
>  	if (ret < 0) {
>  		dev_err(dev, "Cannot get regulators\n");
> -		goto err_endpoint;
> +		return ret;

Please use dev_err_probe() here.

Regards,
Alexander

>  	}
> 
>  	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
>  						   
GPIOD_OUT_HIGH);
>  	if (IS_ERR(imx290->rst_gpio)) {
>  		dev_err(dev, "Cannot get reset gpio\n");
> -		ret = PTR_ERR(imx290->rst_gpio);
> -		goto err_endpoint;
> +		return PTR_ERR(imx290->rst_gpio);
>  	}
> 
>  	mutex_init(&imx290->lock);
> @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client)
>  	pm_runtime_enable(dev);
>  	pm_runtime_idle(dev);
> 
> -	v4l2_fwnode_endpoint_free(&ep);
> -
>  	return 0;
> 
>  err_subdev:
>  	imx290_subdev_cleanup(imx290);
>  err_mutex:
>  	mutex_destroy(&imx290->lock);
> -err_endpoint:
> -	v4l2_fwnode_endpoint_free(&ep);
> 
>  	return ret;
>  }

Re: [PATCH v2 08/17] media: i2c: imx290: Factor out DT parsing to separate function

[Laurent Pinchart]

Hi Alexander,

On Mon, Jan 16, 2023 at 11:33:47AM +0100, Alexander Stein wrote:
> Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart:
> > Make the probe() function more readable by factoring out the DT parsing
> > code to a separate function. No functional change intended.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++-----------------
> >  1 file changed, 52 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 0dc536893ebf..18c1e5c429a2 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct
> > imx290 *imx290, return 0;
> >  }
> > 
> > -static int imx290_probe(struct i2c_client *client)
> > +static int imx290_parse_dt(struct imx290 *imx290)
> >  {
> > -	struct device *dev = &client->dev;
> > -	struct fwnode_handle *endpoint;
> >  	/* Only CSI2 is supported for now: */
> >  	struct v4l2_fwnode_endpoint ep = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > -	struct imx290 *imx290;
> > -	u32 xclk_freq;
> > +	struct fwnode_handle *endpoint;
> > +	int ret;
> >  	s64 fq;
> > -	int ret;
> > 
> > -	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > -	if (!imx290)
> > -		return -ENOMEM;
> > -
> > -	imx290->dev = dev;
> > -	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
> > -	if (IS_ERR(imx290->regmap)) {
> > -		dev_err(dev, "Unable to initialize I2C\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
> >  	if (!endpoint) {
> > -		dev_err(dev, "Endpoint node not found\n");
> > +		dev_err(imx290->dev, "Endpoint node not found\n");
> >  		return -EINVAL;
> >  	}
> > 
> >  	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
> >  	fwnode_handle_put(endpoint);
> >  	if (ret == -ENXIO) {
> > -		dev_err(dev, "Unsupported bus type, should be CSI2\n");
> > -		goto err_endpoint;
> > +		dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n");
> > +		goto done;
> >  	} else if (ret) {
> > -		dev_err(dev, "Parsing endpoint node failed\n");
> > -		goto err_endpoint;
> > +		dev_err(imx290->dev, "Parsing endpoint node failed\n");
> > +		goto done;
> >  	}
> > 
> >  	/* Get number of data lanes */
> >  	imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
> >  	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
> > -		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
> > +		dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290-
> >nlanes);
> >  		ret = -EINVAL;
> > -		goto err_endpoint;
> > +		goto done;
> >  	}
> > 
> > -	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> > +	dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes);
> > 
> >  	if (!ep.nr_of_link_frequencies) {
> > -		dev_err(dev, "link-frequency property not found in DT\n");
> > +		dev_err(imx290->dev, "link-frequency property not found in DT\n");
> >  		ret = -EINVAL;
> > -		goto err_endpoint;
> > +		goto done;
> >  	}
> > 
> >  	/* Check that link frequences for all the modes are in device tree */
> >  	fq = imx290_check_link_freqs(imx290, &ep);
> >  	if (fq) {
> > -		dev_err(dev, "Link frequency of %lld is not supported\n", fq);
> > +		dev_err(imx290->dev, "Link frequency of %lld is not supported\n",
> > +			fq);
> >  		ret = -EINVAL;
> > -		goto err_endpoint;
> > +		goto done;
> >  	}
> > 
> > +	ret = 0;
> > +
> > +done:
> > +	v4l2_fwnode_endpoint_free(&ep);
> > +	return ret;
> > +}
> > +
> > +static int imx290_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct imx290 *imx290;
> > +	u32 xclk_freq;
> > +	int ret;
> > +
> > +	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > +	if (!imx290)
> > +		return -ENOMEM;
> > +
> > +	imx290->dev = dev;
> > +	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
> > +	if (IS_ERR(imx290->regmap)) {
> > +		dev_err(dev, "Unable to initialize I2C\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = imx290_parse_dt(imx290);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* get system clock (xclk) */
> >  	imx290->xclk = devm_clk_get(dev, "xclk");
> >  	if (IS_ERR(imx290->xclk)) {
> >  		dev_err(dev, "Could not get xclk");
> > -		ret = PTR_ERR(imx290->xclk);
> > -		goto err_endpoint;
> > +		return PTR_ERR(imx290->xclk);
> 
> Please use dev_err_probe() here.

It's done in the next patch in this series, which converts the driver to
dev_err_probe(). This patch only factors out code.

> >  	}
> > 
> >  	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> >  				       &xclk_freq);
> >  	if (ret) {
> >  		dev_err(dev, "Could not get xclk frequency\n");
> > -		goto err_endpoint;
> > +		return ret;
> >  	}
> > 
> >  	/* external clock must be 37.125 MHz */
> >  	if (xclk_freq != 37125000) {
> >  		dev_err(dev, "External clock frequency %u is not supported\n",
> >  			xclk_freq);
> > -		ret = -EINVAL;
> > -		goto err_endpoint;
> > +		return -EINVAL;
> >  	}
> > 
> >  	ret = clk_set_rate(imx290->xclk, xclk_freq);
> >  	if (ret) {
> >  		dev_err(dev, "Could not set xclk frequency\n");
> > -		goto err_endpoint;
> > +		return ret;
> >  	}
> > 
> >  	ret = imx290_get_regulators(dev, imx290);
> >  	if (ret < 0) {
> >  		dev_err(dev, "Cannot get regulators\n");
> > -		goto err_endpoint;
> > +		return ret;
> 
> Please use dev_err_probe() here.
> 
> >  	}
> > 
> >  	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
> >  						   GPIOD_OUT_HIGH);
> >  	if (IS_ERR(imx290->rst_gpio)) {
> >  		dev_err(dev, "Cannot get reset gpio\n");
> > -		ret = PTR_ERR(imx290->rst_gpio);
> > -		goto err_endpoint;
> > +		return PTR_ERR(imx290->rst_gpio);
> >  	}
> > 
> >  	mutex_init(&imx290->lock);
> > @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client)
> >  	pm_runtime_enable(dev);
> >  	pm_runtime_idle(dev);
> > 
> > -	v4l2_fwnode_endpoint_free(&ep);
> > -
> >  	return 0;
> > 
> >  err_subdev:
> >  	imx290_subdev_cleanup(imx290);
> >  err_mutex:
> >  	mutex_destroy(&imx290->lock);
> > -err_endpoint:
> > -	v4l2_fwnode_endpoint_free(&ep);
> > 
> >  	return ret;
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/17] media: i2c: imx290: Factor out DT parsing to separate function

[Alexander Stein]

Hi Laurent,

Am Montag, 16. Januar 2023, 11:48:28 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Mon, Jan 16, 2023 at 11:33:47AM +0100, Alexander Stein wrote:
> > Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart:
> > > Make the probe() function more readable by factoring out the DT parsing
> > > code to a separate function. No functional change intended.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++-----------------
> > >  1 file changed, 52 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 0dc536893ebf..18c1e5c429a2 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const
> > > struct
> > > imx290 *imx290, return 0;
> > > 
> > >  }
> > > 
> > > -static int imx290_probe(struct i2c_client *client)
> > > +static int imx290_parse_dt(struct imx290 *imx290)
> > > 
> > >  {
> > > 
> > > -	struct device *dev = &client->dev;
> > > -	struct fwnode_handle *endpoint;
> > > 
> > >  	/* Only CSI2 is supported for now: */
> > >  	struct v4l2_fwnode_endpoint ep = {
> > >  	
> > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > >  	
> > >  	};
> > > 
> > > -	struct imx290 *imx290;
> > > -	u32 xclk_freq;
> > > +	struct fwnode_handle *endpoint;
> > > +	int ret;
> > > 
> > >  	s64 fq;
> > > 
> > > -	int ret;
> > > 
> > > -	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > > -	if (!imx290)
> > > -		return -ENOMEM;
> > > -
> > > -	imx290->dev = dev;
> > > -	imx290->regmap = devm_regmap_init_i2c(client, 
&imx290_regmap_config);
> > > -	if (IS_ERR(imx290->regmap)) {
> > > -		dev_err(dev, "Unable to initialize I2C\n");
> > > -		return -ENODEV;
> > > -	}
> > > -
> > > -	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > > +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev),
> > > NULL);
> > > 
> > >  	if (!endpoint) {
> > > 
> > > -		dev_err(dev, "Endpoint node not found\n");
> > > +		dev_err(imx290->dev, "Endpoint node not found\n");
> > > 
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > >  	
> > >  	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
> > >  	fwnode_handle_put(endpoint);
> > >  	if (ret == -ENXIO) {
> > > 
> > > -		dev_err(dev, "Unsupported bus type, should be CSI2\n");
> > > -		goto err_endpoint;
> > > +		dev_err(imx290->dev, "Unsupported bus type, should be 
CSI2\n");
> > > +		goto done;
> > > 
> > >  	} else if (ret) {
> > > 
> > > -		dev_err(dev, "Parsing endpoint node failed\n");
> > > -		goto err_endpoint;
> > > +		dev_err(imx290->dev, "Parsing endpoint node failed\n");
> > > +		goto done;
> > > 
> > >  	}
> > >  	
> > >  	/* Get number of data lanes */
> > >  	imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
> > >  	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
> > > 
> > > -		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
> > > +		dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290-
> > >
> > >nlanes);
> > >
> > >  		ret = -EINVAL;
> > > 
> > > -		goto err_endpoint;
> > > +		goto done;
> > > 
> > >  	}
> > > 
> > > -	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> > > +	dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes);
> > > 
> > >  	if (!ep.nr_of_link_frequencies) {
> > > 
> > > -		dev_err(dev, "link-frequency property not found in DT\n");
> > > +		dev_err(imx290->dev, "link-frequency property not found in 
DT\n");
> > > 
> > >  		ret = -EINVAL;
> > > 
> > > -		goto err_endpoint;
> > > +		goto done;
> > > 
> > >  	}
> > >  	
> > >  	/* Check that link frequences for all the modes are in device tree 
*/
> > >  	fq = imx290_check_link_freqs(imx290, &ep);
> > >  	if (fq) {
> > > 
> > > -		dev_err(dev, "Link frequency of %lld is not supported\n", 
fq);
> > > +		dev_err(imx290->dev, "Link frequency of %lld is not 
supported\n",
> > > +			fq);
> > > 
> > >  		ret = -EINVAL;
> > > 
> > > -		goto err_endpoint;
> > > +		goto done;
> > > 
> > >  	}
> > > 
> > > +	ret = 0;
> > > +
> > > +done:
> > > +	v4l2_fwnode_endpoint_free(&ep);
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx290_probe(struct i2c_client *client)
> > > +{
> > > +	struct device *dev = &client->dev;
> > > +	struct imx290 *imx290;
> > > +	u32 xclk_freq;
> > > +	int ret;
> > > +
> > > +	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > > +	if (!imx290)
> > > +		return -ENOMEM;
> > > +
> > > +	imx290->dev = dev;
> > > +	imx290->regmap = devm_regmap_init_i2c(client, 
&imx290_regmap_config);
> > > +	if (IS_ERR(imx290->regmap)) {
> > > +		dev_err(dev, "Unable to initialize I2C\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = imx290_parse_dt(imx290);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > 
> > >  	/* get system clock (xclk) */
> > >  	imx290->xclk = devm_clk_get(dev, "xclk");
> > >  	if (IS_ERR(imx290->xclk)) {
> > >  	
> > >  		dev_err(dev, "Could not get xclk");
> > > 
> > > -		ret = PTR_ERR(imx290->xclk);
> > > -		goto err_endpoint;
> > > +		return PTR_ERR(imx290->xclk);
> > 
> > Please use dev_err_probe() here.
> 
> It's done in the next patch in this series, which converts the driver to
> dev_err_probe(). This patch only factors out code.

I just noticed :-/
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> > >  	}
> > >  	
> > >  	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> > >  	
> > >  				       &xclk_freq);
> > >  	
> > >  	if (ret) {
> > >  	
> > >  		dev_err(dev, "Could not get xclk frequency\n");
> > > 
> > > -		goto err_endpoint;
> > > +		return ret;
> > > 
> > >  	}
> > >  	
> > >  	/* external clock must be 37.125 MHz */
> > >  	if (xclk_freq != 37125000) {
> > >  	
> > >  		dev_err(dev, "External clock frequency %u is not 
supported\n",
> > >  		
> > >  			xclk_freq);
> > > 
> > > -		ret = -EINVAL;
> > > -		goto err_endpoint;
> > > +		return -EINVAL;
> > > 
> > >  	}
> > >  	
> > >  	ret = clk_set_rate(imx290->xclk, xclk_freq);
> > >  	if (ret) {
> > >  	
> > >  		dev_err(dev, "Could not set xclk frequency\n");
> > > 
> > > -		goto err_endpoint;
> > > +		return ret;
> > > 
> > >  	}
> > >  	
> > >  	ret = imx290_get_regulators(dev, imx290);
> > >  	if (ret < 0) {
> > >  	
> > >  		dev_err(dev, "Cannot get regulators\n");
> > > 
> > > -		goto err_endpoint;
> > > +		return ret;
> > 
> > Please use dev_err_probe() here.
> > 
> > >  	}
> > >  	
> > >  	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
> > >  	
> > >  						   
GPIOD_OUT_HIGH);
> > >  	
> > >  	if (IS_ERR(imx290->rst_gpio)) {
> > >  	
> > >  		dev_err(dev, "Cannot get reset gpio\n");
> > > 
> > > -		ret = PTR_ERR(imx290->rst_gpio);
> > > -		goto err_endpoint;
> > > +		return PTR_ERR(imx290->rst_gpio);
> > > 
> > >  	}
> > >  	
> > >  	mutex_init(&imx290->lock);
> > > 
> > > @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client
> > > *client)
> > > 
> > >  	pm_runtime_enable(dev);
> > >  	pm_runtime_idle(dev);
> > > 
> > > -	v4l2_fwnode_endpoint_free(&ep);
> > > -
> > > 
> > >  	return 0;
> > >  
> > >  err_subdev:
> > >  	imx290_subdev_cleanup(imx290);
> > >  
> > >  err_mutex:
> > >  	mutex_destroy(&imx290->lock);
> > > 
> > > -err_endpoint:
> > > -	v4l2_fwnode_endpoint_free(&ep);
> > > 
> > >  	return ret;
> > >  
> > >  }

[PATCH v2 09/17] media: i2c: imx290: Use dev_err_probe()

[Laurent Pinchart]

Improve error handling in the probe() function with dev_err_probe().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 18c1e5c429a2..adccd0662afe 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1226,10 +1226,9 @@ static int imx290_probe(struct i2c_client *client)
 
 	/* get system clock (xclk) */
 	imx290->xclk = devm_clk_get(dev, "xclk");
-	if (IS_ERR(imx290->xclk)) {
-		dev_err(dev, "Could not get xclk");
-		return PTR_ERR(imx290->xclk);
-	}
+	if (IS_ERR(imx290->xclk))
+		return dev_err_probe(dev, PTR_ERR(imx290->xclk),
+				     "Could not get xclk");
 
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 				       &xclk_freq);
@@ -1252,17 +1251,14 @@ static int imx290_probe(struct i2c_client *client)
 	}
 
 	ret = imx290_get_regulators(dev, imx290);
-	if (ret < 0) {
-		dev_err(dev, "Cannot get regulators\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot get regulators\n");
 
 	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
 						   GPIOD_OUT_HIGH);
-	if (IS_ERR(imx290->rst_gpio)) {
-		dev_err(dev, "Cannot get reset gpio\n");
-		return PTR_ERR(imx290->rst_gpio);
-	}
+	if (IS_ERR(imx290->rst_gpio))
+		return dev_err_probe(dev, PTR_ERR(imx290->rst_gpio),
+				     "Cannot get reset gpio\n");
 
 	mutex_init(&imx290->lock);
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 10/17] media: i2c: imx290: Factor out clock initialization to separate function

[Laurent Pinchart]

Move the external clock initialization code from probe() to a separate
function to improve readability. No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 57 +++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index adccd0662afe..b01a627c8c03 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1122,6 +1122,34 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
 				       imx290->supplies);
 }
 
+static int imx290_init_clk(struct imx290 *imx290)
+{
+	u32 xclk_freq;
+	int ret;
+
+	ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
+				       "clock-frequency", &xclk_freq);
+	if (ret) {
+		dev_err(imx290->dev, "Could not get xclk frequency\n");
+		return ret;
+	}
+
+	/* external clock must be 37.125 MHz */
+	if (xclk_freq != 37125000) {
+		dev_err(imx290->dev, "External clock frequency %u is not supported\n",
+			xclk_freq);
+		return -EINVAL;
+	}
+
+	ret = clk_set_rate(imx290->xclk, xclk_freq);
+	if (ret) {
+		dev_err(imx290->dev, "Could not set xclk frequency\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Returns 0 if all link frequencies used by the driver for the given number
  * of MIPI data lanes are mentioned in the device tree, or the value of the
@@ -1206,7 +1234,6 @@ static int imx290_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct imx290 *imx290;
-	u32 xclk_freq;
 	int ret;
 
 	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
@@ -1224,32 +1251,12 @@ static int imx290_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	/* get system clock (xclk) */
+	/* Acquire resources. */
 	imx290->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(imx290->xclk))
 		return dev_err_probe(dev, PTR_ERR(imx290->xclk),
 				     "Could not get xclk");
 
-	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
-				       &xclk_freq);
-	if (ret) {
-		dev_err(dev, "Could not get xclk frequency\n");
-		return ret;
-	}
-
-	/* external clock must be 37.125 MHz */
-	if (xclk_freq != 37125000) {
-		dev_err(dev, "External clock frequency %u is not supported\n",
-			xclk_freq);
-		return -EINVAL;
-	}
-
-	ret = clk_set_rate(imx290->xclk, xclk_freq);
-	if (ret) {
-		dev_err(dev, "Could not set xclk frequency\n");
-		return ret;
-	}
-
 	ret = imx290_get_regulators(dev, imx290);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Cannot get regulators\n");
@@ -1260,8 +1267,14 @@ static int imx290_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(imx290->rst_gpio),
 				     "Cannot get reset gpio\n");
 
+	/* Initialize external clock frequency. */
+	ret = imx290_init_clk(imx290);
+	if (ret)
+		return ret;
+
 	mutex_init(&imx290->lock);
 
+	/* Initialize and register subdev. */
 	ret = imx290_subdev_init(imx290);
 	if (ret)
 		goto err_mutex;
-- 
Regards,

Laurent Pinchart

[PATCH v2 11/17] media: i2c: imx290: Use V4L2 subdev active state

[Laurent Pinchart]

Use the V4L2 subdev active state API to store the active format. This
simplifies the driver not only by dropping the imx290 current_format
field, but it also allows dropping the imx290 lock, replaced with the
state lock.

The lock check in imx290_ctrl_update() can be dropped as
imx290_set_fmt() can't be called anywmore with which set to ACTIVE
before controls are initialized.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Remove lock check from imx290_ctrl_update()
- Drop unrelated change
- Fix error handling in imx290_subdev_init()
- Handle errors from v4l2_subdev_init_finalize()
- Return immediately from imx290_set_ctrl() for read-only controls
- Call v4l2_subdev_cleanup()
---
 drivers/media/i2c/imx290.c | 170 +++++++++++++++++--------------------
 1 file changed, 80 insertions(+), 90 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index b01a627c8c03..94d23eea36be 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -177,12 +177,12 @@ struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	u8 nlanes;
-	u8 bpp;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
-	struct v4l2_mbus_framefmt current_format;
+
 	const struct imx290_mode *current_mode;
+	u8 bpp;
 
 	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
 	struct gpio_desc *rst_gpio;
@@ -192,8 +192,6 @@ struct imx290 {
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
-
-	struct mutex lock;
 };
 
 static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
@@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290 *imx290,
 			    black_level >> (16 - imx290->bpp), err);
 }
 
-static int imx290_write_current_format(struct imx290 *imx290)
+static int imx290_setup_format(struct imx290 *imx290,
+			       const struct v4l2_mbus_framefmt *format)
 {
 	const struct imx290_regval *regs;
 	unsigned int num_regs;
 	int ret;
 
-	switch (imx290->current_format.code) {
+	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
 		regs = imx290_10bit_settings;
 		num_regs = ARRAY_SIZE(imx290_10bit_settings);
@@ -561,12 +560,31 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct imx290 *imx290 = container_of(ctrl->handler,
 					     struct imx290, ctrls);
+	const struct v4l2_mbus_framefmt *format;
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
+	/*
+	 * Return immediately for controls that don't need to be applied to the
+	 * device. This includes all controls modified in imx290_ctrl_update(),
+	 * which is called at probe time before runtime PM is initialized, so
+	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
+	 */
+	switch (ctrl->id) {
+	case V4L2_CID_LINK_FREQ:
+	case V4L2_CID_PIXEL_RATE:
+	case V4L2_CID_VBLANK:
+	case V4L2_CID_HBLANK:
+		return 0;
+	}
+
 	/* V4L2 controls values will be applied only when power is already up */
 	if (!pm_runtime_get_if_in_use(imx290->dev))
 		return 0;
 
+	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
+	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+
 	switch (ctrl->id) {
 	case V4L2_CID_ANALOGUE_GAIN:
 		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
@@ -592,6 +610,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 					       &ret);
 		}
 		break;
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -626,13 +645,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
 	u64 pixel_rate;
 
-	/*
-	 * This function may be called from imx290_set_fmt() before controls
-	 * get created by imx290_ctrl_init(). Return immediately in that case.
-	 */
-	if (!imx290->ctrls.lock)
-		return;
-
 	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
 	pixel_rate = link_freq * 2 * imx290->nlanes;
 	do_div(pixel_rate, imx290->bpp);
@@ -654,7 +666,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 		return ret;
 
 	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
-	imx290->ctrls.lock = &imx290->lock;
 
 	/*
 	 * The sensor has an analog gain and a digital gain, both controlled
@@ -719,11 +730,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 		return ret;
 	}
 
-	mutex_lock(imx290->ctrls.lock);
-	imx290_ctrl_update(imx290, &imx290->current_format,
-			   imx290->current_mode);
-	mutex_unlock(imx290->ctrls.lock);
-
 	return 0;
 }
 
@@ -732,8 +738,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
  */
 
 /* Start streaming */
-static int imx290_start_streaming(struct imx290 *imx290)
+static int imx290_start_streaming(struct imx290 *imx290,
+				  struct v4l2_subdev_state *state)
 {
+	const struct v4l2_mbus_framefmt *format;
 	int ret;
 
 	/* Set init register settings */
@@ -746,7 +754,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
 	}
 
 	/* Apply the register values related to current frame format */
-	ret = imx290_write_current_format(imx290);
+	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+	ret = imx290_setup_format(imx290, format);
 	if (ret < 0) {
 		dev_err(imx290->dev, "Could not set frame format\n");
 		return ret;
@@ -766,7 +775,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 
 	/* Apply customized values from user */
-	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
+	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
 	if (ret) {
 		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
 		return ret;
@@ -795,39 +804,32 @@ static int imx290_stop_streaming(struct imx290 *imx290)
 static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct imx290 *imx290 = to_imx290(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
 	if (enable) {
 		ret = pm_runtime_resume_and_get(imx290->dev);
 		if (ret < 0)
-			goto unlock_and_return;
+			goto unlock;
 
-		ret = imx290_start_streaming(imx290);
+		ret = imx290_start_streaming(imx290, state);
 		if (ret) {
 			dev_err(imx290->dev, "Start stream failed\n");
 			pm_runtime_put(imx290->dev);
-			goto unlock_and_return;
+			goto unlock;
 		}
 	} else {
 		imx290_stop_streaming(imx290);
 		pm_runtime_put(imx290->dev);
 	}
 
-unlock_and_return:
-
+unlock:
+	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
-static struct v4l2_mbus_framefmt *
-imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
-		      u32 which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		return &imx290->current_format;
-	else
-		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
-}
-
 static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -862,23 +864,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int imx290_get_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *fmt)
-{
-	struct imx290 *imx290 = to_imx290(sd);
-	struct v4l2_mbus_framefmt *framefmt;
-
-	mutex_lock(&imx290->lock);
-
-	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
-	fmt->format = *framefmt;
-
-	mutex_unlock(&imx290->lock);
-
-	return 0;
-}
-
 static int imx290_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *fmt)
@@ -888,8 +873,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *format;
 	unsigned int i;
 
-	mutex_lock(&imx290->lock);
-
 	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
 				      imx290_modes_num(imx290), width, height,
 				      fmt->format.width, fmt->format.height);
@@ -907,7 +890,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.code = imx290_formats[i].code;
 	fmt->format.field = V4L2_FIELD_NONE;
 
-	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
+	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		imx290->current_mode = mode;
@@ -918,8 +901,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 
 	*format = fmt->format;
 
-	mutex_unlock(&imx290->lock);
-
 	return 0;
 }
 
@@ -927,14 +908,11 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
 {
-	struct imx290 *imx290 = to_imx290(sd);
 	struct v4l2_mbus_framefmt *format;
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP: {
-		format = imx290_get_pad_format(imx290, sd_state, sel->which);
-
-		mutex_lock(&imx290->lock);
+		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
 		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
 			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
@@ -943,7 +921,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
 		sel->r.width = format->width;
 		sel->r.height = format->height;
 
-		mutex_unlock(&imx290->lock);
 		return 0;
 	}
 
@@ -972,11 +949,13 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
 static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_subdev_format fmt = { 0 };
-
-	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.format.width = 1920;
-	fmt.format.height = 1080;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.format = {
+			.width = 1920,
+			.height = 1080,
+		},
+	};
 
 	imx290_set_fmt(subdev, sd_state, &fmt);
 
@@ -991,7 +970,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
 	.init_cfg = imx290_entity_init_cfg,
 	.enum_mbus_code = imx290_enum_mbus_code,
 	.enum_frame_size = imx290_enum_frame_size,
-	.get_fmt = imx290_get_fmt,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx290_set_fmt,
 	.get_selection = imx290_get_selection,
 };
@@ -1008,20 +987,12 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
 static int imx290_subdev_init(struct imx290 *imx290)
 {
 	struct i2c_client *client = to_i2c_client(imx290->dev);
+	const struct v4l2_mbus_framefmt *format;
+	struct v4l2_subdev_state *state;
 	int ret;
 
-	/*
-	 * Initialize the frame format. In particular, imx290->current_mode
-	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
-	 * below relies on these fields.
-	 */
-	imx290_entity_init_cfg(&imx290->sd, NULL);
-
-	ret = imx290_ctrl_init(imx290);
-	if (ret < 0) {
-		dev_err(imx290->dev, "Control initialization error %d\n", ret);
-		return ret;
-	}
+	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
+	imx290->bpp = imx290_formats[0].bpp;
 
 	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
 	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1033,15 +1004,40 @@ static int imx290_subdev_init(struct imx290 *imx290)
 	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
 	if (ret < 0) {
 		dev_err(imx290->dev, "Could not register media entity\n");
-		v4l2_ctrl_handler_free(&imx290->ctrls);
 		return ret;
 	}
 
+	ret = imx290_ctrl_init(imx290);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Control initialization error %d\n", ret);
+		goto err_media;
+	}
+
+	imx290->sd.state_lock = imx290->ctrls.lock;
+
+	ret = v4l2_subdev_init_finalize(&imx290->sd);
+	if (ret < 0) {
+		dev_err(imx290->dev, "subdev initialization error %d\n", ret);
+		goto err_ctrls;
+	}
+
+	state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
+	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
+	imx290_ctrl_update(imx290, format, imx290->current_mode);
+	v4l2_subdev_unlock_state(state);
+
 	return 0;
+
+err_ctrls:
+	v4l2_ctrl_handler_free(&imx290->ctrls);
+err_media:
+	media_entity_cleanup(&imx290->sd.entity);
+	return ret;
 }
 
 static void imx290_subdev_cleanup(struct imx290 *imx290)
 {
+	v4l2_subdev_cleanup(&imx290->sd);
 	media_entity_cleanup(&imx290->sd.entity);
 	v4l2_ctrl_handler_free(&imx290->ctrls);
 }
@@ -1272,12 +1268,10 @@ static int imx290_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	mutex_init(&imx290->lock);
-
 	/* Initialize and register subdev. */
 	ret = imx290_subdev_init(imx290);
 	if (ret)
-		goto err_mutex;
+		return ret;
 
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
@@ -1300,8 +1294,6 @@ static int imx290_probe(struct i2c_client *client)
 
 err_subdev:
 	imx290_subdev_cleanup(imx290);
-err_mutex:
-	mutex_destroy(&imx290->lock);
 
 	return ret;
 }
@@ -1314,8 +1306,6 @@ static void imx290_remove(struct i2c_client *client)
 	v4l2_async_unregister_subdev(sd);
 	imx290_subdev_cleanup(imx290);
 
-	mutex_destroy(&imx290->lock);
-
 	pm_runtime_disable(imx290->dev);
 	if (!pm_runtime_status_suspended(imx290->dev))
 		imx290_power_off(imx290->dev);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/17] media: i2c: imx290: Use V4L2 subdev active state

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:56 CET schrieb Laurent Pinchart:
> Use the V4L2 subdev active state API to store the active format. This
> simplifies the driver not only by dropping the imx290 current_format
> field, but it also allows dropping the imx290 lock, replaced with the
> state lock.
> 
> The lock check in imx290_ctrl_update() can be dropped as
> imx290_set_fmt() can't be called anywmore with which set to ACTIVE
> before controls are initialized.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Remove lock check from imx290_ctrl_update()
> - Drop unrelated change
> - Fix error handling in imx290_subdev_init()
> - Handle errors from v4l2_subdev_init_finalize()
> - Return immediately from imx290_set_ctrl() for read-only controls
> - Call v4l2_subdev_cleanup()
> ---
>  drivers/media/i2c/imx290.c | 170 +++++++++++++++++--------------------
>  1 file changed, 80 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index b01a627c8c03..94d23eea36be 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -177,12 +177,12 @@ struct imx290 {
>  	struct clk *xclk;
>  	struct regmap *regmap;
>  	u8 nlanes;
> -	u8 bpp;
> 
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
> -	struct v4l2_mbus_framefmt current_format;
> +
>  	const struct imx290_mode *current_mode;
> +	u8 bpp;
> 
>  	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
>  	struct gpio_desc *rst_gpio;
> @@ -192,8 +192,6 @@ struct imx290 {
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
> -
> -	struct mutex lock;
>  };
> 
>  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> @@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290
> *imx290, black_level >> (16 - imx290->bpp), err);
>  }
> 
> -static int imx290_write_current_format(struct imx290 *imx290)
> +static int imx290_setup_format(struct imx290 *imx290,
> +			       const struct v4l2_mbus_framefmt *format)
>  {
>  	const struct imx290_regval *regs;
>  	unsigned int num_regs;
>  	int ret;
> 
> -	switch (imx290->current_format.code) {
> +	switch (format->code) {
>  	case MEDIA_BUS_FMT_SRGGB10_1X10:
>  		regs = imx290_10bit_settings;
>  		num_regs = ARRAY_SIZE(imx290_10bit_settings);
> @@ -561,12 +560,31 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct imx290 *imx290 = container_of(ctrl->handler,
>  					     struct imx290, ctrls);
> +	const struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
> 
> +	/*
> +	 * Return immediately for controls that don't need to be applied to 
the
> +	 * device. This includes all controls modified in 
imx290_ctrl_update(),
> +	 * which is called at probe time before runtime PM is initialized, 
so
> +	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> +	 */
> +	switch (ctrl->id) {
> +	case V4L2_CID_LINK_FREQ:
> +	case V4L2_CID_PIXEL_RATE:
> +	case V4L2_CID_VBLANK:
> +	case V4L2_CID_HBLANK:
> +		return 0;
> +	}

I don't like keeping the IDs synchronized in two functions. Isn't checking for 
'ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY' enough?
On that side V4L2_CID_PIXEL_RATE is missing the read-only flag, no?

>  	/* V4L2 controls values will be applied only when power is already 
up */
>  	if (!pm_runtime_get_if_in_use(imx290->dev))
>  		return 0;
> 
> +	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_ANALOGUE_GAIN:
>  		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
> @@ -592,6 +610,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  					       &ret);
>  		}
>  		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -626,13 +645,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode-
>link_freq_index];
>  	u64 pixel_rate;
> 
> -	/*
> -	 * This function may be called from imx290_set_fmt() before controls
> -	 * get created by imx290_ctrl_init(). Return immediately in that 
case.
> -	 */
> -	if (!imx290->ctrls.lock)
> -		return;
> -
>  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>  	pixel_rate = link_freq * 2 * imx290->nlanes;
>  	do_div(pixel_rate, imx290->bpp);
> @@ -654,7 +666,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  		return ret;
> 
>  	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> -	imx290->ctrls.lock = &imx290->lock;
> 
>  	/*
>  	 * The sensor has an analog gain and a digital gain, both controlled
> @@ -719,11 +730,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  		return ret;
>  	}
> 
> -	mutex_lock(imx290->ctrls.lock);
> -	imx290_ctrl_update(imx290, &imx290->current_format,
> -			   imx290->current_mode);
> -	mutex_unlock(imx290->ctrls.lock);
> -
>  	return 0;
>  }
> 
> @@ -732,8 +738,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>   */
> 
>  /* Start streaming */
> -static int imx290_start_streaming(struct imx290 *imx290)
> +static int imx290_start_streaming(struct imx290 *imx290,
> +				  struct v4l2_subdev_state *state)
>  {
> +	const struct v4l2_mbus_framefmt *format;
>  	int ret;
> 
>  	/* Set init register settings */
> @@ -746,7 +754,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> }
> 
>  	/* Apply the register values related to current frame format */
> -	ret = imx290_write_current_format(imx290);
> +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +	ret = imx290_setup_format(imx290, format);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not set frame format\n");
>  		return ret;
> @@ -766,7 +775,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
> return ret;
> 
>  	/* Apply customized values from user */
> -	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> +	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
>  		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
>  		return ret;
> @@ -795,39 +804,32 @@ static int imx290_stop_streaming(struct imx290
> *imx290) static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct imx290 *imx290 = to_imx290(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
> 
> +	state = v4l2_subdev_lock_and_get_active_state(sd);

Is there ever the case that state==NULL? If so you will deference a NULL 
pointer in v4l2_subdev_unlock_state() later on.

Best regards,
Alexander

>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(imx290->dev);
>  		if (ret < 0)
> -			goto unlock_and_return;
> +			goto unlock;
> 
> -		ret = imx290_start_streaming(imx290);
> +		ret = imx290_start_streaming(imx290, state);
>  		if (ret) {
>  			dev_err(imx290->dev, "Start stream failed\n");
>  			pm_runtime_put(imx290->dev);
> -			goto unlock_and_return;
> +			goto unlock;
>  		}
>  	} else {
>  		imx290_stop_streaming(imx290);
>  		pm_runtime_put(imx290->dev);
>  	}
> 
> -unlock_and_return:
> -
> +unlock:
> +	v4l2_subdev_unlock_state(state);
>  	return ret;
>  }
> 
> -static struct v4l2_mbus_framefmt *
> -imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state
> *state, -		      u32 which)
> -{
> -	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return &imx290->current_format;
> -	else
> -		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> -}
> -
>  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum 
*code)
> @@ -862,23 +864,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> *sd, return 0;
>  }
> 
> -static int imx290_get_fmt(struct v4l2_subdev *sd,
> -			  struct v4l2_subdev_state *sd_state,
> -			  struct v4l2_subdev_format *fmt)
> -{
> -	struct imx290 *imx290 = to_imx290(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> -
> -	mutex_lock(&imx290->lock);
> -
> -	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> -	fmt->format = *framefmt;
> -
> -	mutex_unlock(&imx290->lock);
> -
> -	return 0;
> -}
> -
>  static int imx290_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *sd_state,
>  			  struct v4l2_subdev_format *fmt)
> @@ -888,8 +873,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *format;
>  	unsigned int i;
> 
> -	mutex_lock(&imx290->lock);
> -
>  	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
>  				      imx290_modes_num(imx290), width, 
height,
>  				      fmt->format.width, fmt-
>format.height);
> @@ -907,7 +890,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	fmt->format.code = imx290_formats[i].code;
>  	fmt->format.field = V4L2_FIELD_NONE;
> 
> -	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> 
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx290->current_mode = mode;
> @@ -918,8 +901,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> 
>  	*format = fmt->format;
> 
> -	mutex_unlock(&imx290->lock);
> -
>  	return 0;
>  }
> 
> @@ -927,14 +908,11 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_selection *sel)
>  {
> -	struct imx290 *imx290 = to_imx290(sd);
>  	struct v4l2_mbus_framefmt *format;
> 
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP: {
> -		format = imx290_get_pad_format(imx290, sd_state, sel-
>which);
> -
> -		mutex_lock(&imx290->lock);
> +		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> 
>  		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
>  			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - 
format->height) / 2;
> @@ -943,7 +921,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
>  		sel->r.width = format->width;
>  		sel->r.height = format->height;
> 
> -		mutex_unlock(&imx290->lock);
>  		return 0;
>  	}
> 
> @@ -972,11 +949,13 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
>  				  struct v4l2_subdev_state *sd_state)
>  {
> -	struct v4l2_subdev_format fmt = { 0 };
> -
> -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : 
V4L2_SUBDEV_FORMAT_ACTIVE;
> -	fmt.format.width = 1920;
> -	fmt.format.height = 1080;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.format = {
> +			.width = 1920,
> +			.height = 1080,
> +		},
> +	};
> 
>  	imx290_set_fmt(subdev, sd_state, &fmt);
> 
> @@ -991,7 +970,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops =
> { .init_cfg = imx290_entity_init_cfg,
>  	.enum_mbus_code = imx290_enum_mbus_code,
>  	.enum_frame_size = imx290_enum_frame_size,
> -	.get_fmt = imx290_get_fmt,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = imx290_set_fmt,
>  	.get_selection = imx290_get_selection,
>  };
> @@ -1008,20 +987,12 @@ static const struct media_entity_operations
> imx290_subdev_entity_ops = { static int imx290_subdev_init(struct imx290
> *imx290)
>  {
>  	struct i2c_client *client = to_i2c_client(imx290->dev);
> +	const struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
>  	int ret;
> 
> -	/*
> -	 * Initialize the frame format. In particular, imx290->current_mode
> -	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() 
call
> -	 * below relies on these fields.
> -	 */
> -	imx290_entity_init_cfg(&imx290->sd, NULL);
> -
> -	ret = imx290_ctrl_init(imx290);
> -	if (ret < 0) {
> -		dev_err(imx290->dev, "Control initialization error %d\n", 
ret);
> -		return ret;
> -	}
> +	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> +	imx290->bpp = imx290_formats[0].bpp;
> 
>  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
>  	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1033,15 +1004,40 @@ static int imx290_subdev_init(struct imx290 *imx290)
> ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad); if (ret
> < 0) {
>  		dev_err(imx290->dev, "Could not register media entity\n");
> -		v4l2_ctrl_handler_free(&imx290->ctrls);
>  		return ret;
>  	}
> 
> +	ret = imx290_ctrl_init(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Control initialization error %d\n", 
ret);
> +		goto err_media;
> +	}
> +
> +	imx290->sd.state_lock = imx290->ctrls.lock;
> +
> +	ret = v4l2_subdev_init_finalize(&imx290->sd);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "subdev initialization error %d\n", 
ret);
> +		goto err_ctrls;
> +	}
> +
> +	state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
> +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +	imx290_ctrl_update(imx290, format, imx290->current_mode);
> +	v4l2_subdev_unlock_state(state);
> +
>  	return 0;
> +
> +err_ctrls:
> +	v4l2_ctrl_handler_free(&imx290->ctrls);
> +err_media:
> +	media_entity_cleanup(&imx290->sd.entity);
> +	return ret;
>  }
> 
>  static void imx290_subdev_cleanup(struct imx290 *imx290)
>  {
> +	v4l2_subdev_cleanup(&imx290->sd);
>  	media_entity_cleanup(&imx290->sd.entity);
>  	v4l2_ctrl_handler_free(&imx290->ctrls);
>  }
> @@ -1272,12 +1268,10 @@ static int imx290_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
> 
> -	mutex_init(&imx290->lock);
> -
>  	/* Initialize and register subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
> -		goto err_mutex;
> +		return ret;
> 
>  	ret = v4l2_async_register_subdev(&imx290->sd);
>  	if (ret < 0) {
> @@ -1300,8 +1294,6 @@ static int imx290_probe(struct i2c_client *client)
> 
>  err_subdev:
>  	imx290_subdev_cleanup(imx290);
> -err_mutex:
> -	mutex_destroy(&imx290->lock);
> 
>  	return ret;
>  }
> @@ -1314,8 +1306,6 @@ static void imx290_remove(struct i2c_client *client)
>  	v4l2_async_unregister_subdev(sd);
>  	imx290_subdev_cleanup(imx290);
> 
> -	mutex_destroy(&imx290->lock);
> -
>  	pm_runtime_disable(imx290->dev);
>  	if (!pm_runtime_status_suspended(imx290->dev))
>  		imx290_power_off(imx290->dev);

Re: [PATCH v2 11/17] media: i2c: imx290: Use V4L2 subdev active state

[Laurent Pinchart]

Hi Alexander,

On Mon, Jan 16, 2023 at 12:04:34PM +0100, Alexander Stein wrote:
> Am Samstag, 14. Januar 2023, 18:17:56 CET schrieb Laurent Pinchart:
> > Use the V4L2 subdev active state API to store the active format. This
> > simplifies the driver not only by dropping the imx290 current_format
> > field, but it also allows dropping the imx290 lock, replaced with the
> > state lock.
> > 
> > The lock check in imx290_ctrl_update() can be dropped as
> > imx290_set_fmt() can't be called anywmore with which set to ACTIVE
> > before controls are initialized.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Remove lock check from imx290_ctrl_update()
> > - Drop unrelated change
> > - Fix error handling in imx290_subdev_init()
> > - Handle errors from v4l2_subdev_init_finalize()
> > - Return immediately from imx290_set_ctrl() for read-only controls
> > - Call v4l2_subdev_cleanup()
> > ---
> >  drivers/media/i2c/imx290.c | 170 +++++++++++++++++--------------------
> >  1 file changed, 80 insertions(+), 90 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index b01a627c8c03..94d23eea36be 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -177,12 +177,12 @@ struct imx290 {
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> >  	u8 nlanes;
> > -	u8 bpp;
> > 
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pad;
> > -	struct v4l2_mbus_framefmt current_format;
> > +
> >  	const struct imx290_mode *current_mode;
> > +	u8 bpp;
> > 
> >  	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
> >  	struct gpio_desc *rst_gpio;
> > @@ -192,8 +192,6 @@ struct imx290 {
> >  	struct v4l2_ctrl *pixel_rate;
> >  	struct v4l2_ctrl *hblank;
> >  	struct v4l2_ctrl *vblank;
> > -
> > -	struct mutex lock;
> >  };
> > 
> >  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > @@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290
> > *imx290, black_level >> (16 - imx290->bpp), err);
> >  }
> > 
> > -static int imx290_write_current_format(struct imx290 *imx290)
> > +static int imx290_setup_format(struct imx290 *imx290,
> > +			       const struct v4l2_mbus_framefmt *format)
> >  {
> >  	const struct imx290_regval *regs;
> >  	unsigned int num_regs;
> >  	int ret;
> > 
> > -	switch (imx290->current_format.code) {
> > +	switch (format->code) {
> >  	case MEDIA_BUS_FMT_SRGGB10_1X10:
> >  		regs = imx290_10bit_settings;
> >  		num_regs = ARRAY_SIZE(imx290_10bit_settings);
> > @@ -561,12 +560,31 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >  	struct imx290 *imx290 = container_of(ctrl->handler,
> >  					     struct imx290, ctrls);
> > +	const struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_subdev_state *state;
> >  	int ret = 0;
> > 
> > +	/*
> > +	 * Return immediately for controls that don't need to be applied to the
> > +	 * device. This includes all controls modified in imx290_ctrl_update(),
> > +	 * which is called at probe time before runtime PM is initialized, so
> > +	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> > +	 */
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_LINK_FREQ:
> > +	case V4L2_CID_PIXEL_RATE:
> > +	case V4L2_CID_VBLANK:
> > +	case V4L2_CID_HBLANK:
> > +		return 0;
> > +	}
> 
> I don't like keeping the IDs synchronized in two functions. Isn't checking for 
> 'ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY' enough?

It should be. I'll do so.

> On that side V4L2_CID_PIXEL_RATE is missing the read-only flag, no?

That's right. I'll add a patch in v3 to make it read-only.

> >  	/* V4L2 controls values will be applied only when power is already up */
> >  	if (!pm_runtime_get_if_in_use(imx290->dev))
> >  		return 0;
> > 
> > +	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > +
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_ANALOGUE_GAIN:
> >  		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
> > @@ -592,6 +610,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >  					       &ret);
> >  		}
> >  		break;
> > +
> >  	default:
> >  		ret = -EINVAL;
> >  		break;
> > @@ -626,13 +645,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> >  	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
> >  	u64 pixel_rate;
> > 
> > -	/*
> > -	 * This function may be called from imx290_set_fmt() before controls
> > -	 * get created by imx290_ctrl_init(). Return immediately in that case.
> > -	 */
> > -	if (!imx290->ctrls.lock)
> > -		return;
> > -
> >  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> >  	pixel_rate = link_freq * 2 * imx290->nlanes;
> >  	do_div(pixel_rate, imx290->bpp);
> > @@ -654,7 +666,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >  		return ret;
> > 
> >  	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> > -	imx290->ctrls.lock = &imx290->lock;
> > 
> >  	/*
> >  	 * The sensor has an analog gain and a digital gain, both controlled
> > @@ -719,11 +730,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >  		return ret;
> >  	}
> > 
> > -	mutex_lock(imx290->ctrls.lock);
> > -	imx290_ctrl_update(imx290, &imx290->current_format,
> > -			   imx290->current_mode);
> > -	mutex_unlock(imx290->ctrls.lock);
> > -
> >  	return 0;
> >  }
> > 
> > @@ -732,8 +738,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >   */
> > 
> >  /* Start streaming */
> > -static int imx290_start_streaming(struct imx290 *imx290)
> > +static int imx290_start_streaming(struct imx290 *imx290,
> > +				  struct v4l2_subdev_state *state)
> >  {
> > +	const struct v4l2_mbus_framefmt *format;
> >  	int ret;
> > 
> >  	/* Set init register settings */
> > @@ -746,7 +754,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> > }
> > 
> >  	/* Apply the register values related to current frame format */
> > -	ret = imx290_write_current_format(imx290);
> > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > +	ret = imx290_setup_format(imx290, format);
> >  	if (ret < 0) {
> >  		dev_err(imx290->dev, "Could not set frame format\n");
> >  		return ret;
> > @@ -766,7 +775,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
> >  		return ret;
> > 
> >  	/* Apply customized values from user */
> > -	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> > +	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> >  	if (ret) {
> >  		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> >  		return ret;
> > @@ -795,39 +804,32 @@ static int imx290_stop_streaming(struct imx290 *imx290)
> >  static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct imx290 *imx290 = to_imx290(sd);
> > +	struct v4l2_subdev_state *state;
> >  	int ret = 0;
> > 
> > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> 
> Is there ever the case that state==NULL? If so you will deference a NULL 
> pointer in v4l2_subdev_unlock_state() later on.

No, that can't happen, the driver calls v4l2_subdev_init_finalize() at
probe time, so sd->active_state can't be NULL.

> >  	if (enable) {
> >  		ret = pm_runtime_resume_and_get(imx290->dev);
> >  		if (ret < 0)
> > -			goto unlock_and_return;
> > +			goto unlock;
> > 
> > -		ret = imx290_start_streaming(imx290);
> > +		ret = imx290_start_streaming(imx290, state);
> >  		if (ret) {
> >  			dev_err(imx290->dev, "Start stream failed\n");
> >  			pm_runtime_put(imx290->dev);
> > -			goto unlock_and_return;
> > +			goto unlock;
> >  		}
> >  	} else {
> >  		imx290_stop_streaming(imx290);
> >  		pm_runtime_put(imx290->dev);
> >  	}
> > 
> > -unlock_and_return:
> > -
> > +unlock:
> > +	v4l2_subdev_unlock_state(state);
> >  	return ret;
> >  }
> > 
> > -static struct v4l2_mbus_framefmt *
> > -imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
> > -		      u32 which)
> > -{
> > -	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		return &imx290->current_format;
> > -	else
> > -		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> > -}
> > -
> >  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -862,23 +864,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd
> >  	return 0;
> >  }
> > 
> > -static int imx290_get_fmt(struct v4l2_subdev *sd,
> > -			  struct v4l2_subdev_state *sd_state,
> > -			  struct v4l2_subdev_format *fmt)
> > -{
> > -	struct imx290 *imx290 = to_imx290(sd);
> > -	struct v4l2_mbus_framefmt *framefmt;
> > -
> > -	mutex_lock(&imx290->lock);
> > -
> > -	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> > -	fmt->format = *framefmt;
> > -
> > -	mutex_unlock(&imx290->lock);
> > -
> > -	return 0;
> > -}
> > -
> >  static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  			  struct v4l2_subdev_state *sd_state,
> >  			  struct v4l2_subdev_format *fmt)
> > @@ -888,8 +873,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  	struct v4l2_mbus_framefmt *format;
> >  	unsigned int i;
> > 
> > -	mutex_lock(&imx290->lock);
> > -
> >  	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
> >  				      imx290_modes_num(imx290), width, height,
> >  				      fmt->format.width, fmt->format.height);
> > @@ -907,7 +890,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  	fmt->format.code = imx290_formats[i].code;
> >  	fmt->format.field = V4L2_FIELD_NONE;
> > 
> > -	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > 
> >  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >  		imx290->current_mode = mode;
> > @@ -918,8 +901,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > 
> >  	*format = fmt->format;
> > 
> > -	mutex_unlock(&imx290->lock);
> > -
> >  	return 0;
> >  }
> > 
> > @@ -927,14 +908,11 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> >  				struct v4l2_subdev_state *sd_state,
> >  				struct v4l2_subdev_selection *sel)
> >  {
> > -	struct imx290 *imx290 = to_imx290(sd);
> >  	struct v4l2_mbus_framefmt *format;
> > 
> >  	switch (sel->target) {
> >  	case V4L2_SEL_TGT_CROP: {
> > -		format = imx290_get_pad_format(imx290, sd_state, sel->which);
> > -
> > -		mutex_lock(&imx290->lock);
> > +		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > 
> >  		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> >  			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
> > @@ -943,7 +921,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> >  		sel->r.width = format->width;
> >  		sel->r.height = format->height;
> > 
> > -		mutex_unlock(&imx290->lock);
> >  		return 0;
> >  	}
> > 
> > @@ -972,11 +949,13 @@ static int imx290_get_selection(struct v4l2_subdev *sd
> >  static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
> >  				  struct v4l2_subdev_state *sd_state)
> >  {
> > -	struct v4l2_subdev_format fmt = { 0 };
> > -
> > -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > -	fmt.format.width = 1920;
> > -	fmt.format.height = 1080;
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_TRY,
> > +		.format = {
> > +			.width = 1920,
> > +			.height = 1080,
> > +		},
> > +	};
> > 
> >  	imx290_set_fmt(subdev, sd_state, &fmt);
> > 
> > @@ -991,7 +970,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> >  	.init_cfg = imx290_entity_init_cfg,
> >  	.enum_mbus_code = imx290_enum_mbus_code,
> >  	.enum_frame_size = imx290_enum_frame_size,
> > -	.get_fmt = imx290_get_fmt,
> > +	.get_fmt = v4l2_subdev_get_fmt,
> >  	.set_fmt = imx290_set_fmt,
> >  	.get_selection = imx290_get_selection,
> >  };
> > @@ -1008,20 +987,12 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
> >  static int imx290_subdev_init(struct imx290 *imx290)
> >  {
> >  	struct i2c_client *client = to_i2c_client(imx290->dev);
> > +	const struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_subdev_state *state;
> >  	int ret;
> > 
> > -	/*
> > -	 * Initialize the frame format. In particular, imx290->current_mode
> > -	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
> > -	 * below relies on these fields.
> > -	 */
> > -	imx290_entity_init_cfg(&imx290->sd, NULL);
> > -
> > -	ret = imx290_ctrl_init(imx290);
> > -	if (ret < 0) {
> > -		dev_err(imx290->dev, "Control initialization error %d\n", ret);
> > -		return ret;
> > -	}
> > +	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > +	imx290->bpp = imx290_formats[0].bpp;
> > 
> >  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> >  	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > @@ -1033,15 +1004,40 @@ static int imx290_subdev_init(struct imx290 *imx290)
> >  	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> >  	if (ret < 0) {
> >  		dev_err(imx290->dev, "Could not register media entity\n");
> > -		v4l2_ctrl_handler_free(&imx290->ctrls);
> >  		return ret;
> >  	}
> > 
> > +	ret = imx290_ctrl_init(imx290);
> > +	if (ret < 0) {
> > +		dev_err(imx290->dev, "Control initialization error %d\n", ret);
> > +		goto err_media;
> > +	}
> > +
> > +	imx290->sd.state_lock = imx290->ctrls.lock;
> > +
> > +	ret = v4l2_subdev_init_finalize(&imx290->sd);
> > +	if (ret < 0) {
> > +		dev_err(imx290->dev, "subdev initialization error %d\n", ret);
> > +		goto err_ctrls;
> > +	}
> > +
> > +	state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
> > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > +	imx290_ctrl_update(imx290, format, imx290->current_mode);
> > +	v4l2_subdev_unlock_state(state);
> > +
> >  	return 0;
> > +
> > +err_ctrls:
> > +	v4l2_ctrl_handler_free(&imx290->ctrls);
> > +err_media:
> > +	media_entity_cleanup(&imx290->sd.entity);
> > +	return ret;
> >  }
> > 
> >  static void imx290_subdev_cleanup(struct imx290 *imx290)
> >  {
> > +	v4l2_subdev_cleanup(&imx290->sd);
> >  	media_entity_cleanup(&imx290->sd.entity);
> >  	v4l2_ctrl_handler_free(&imx290->ctrls);
> >  }
> > @@ -1272,12 +1268,10 @@ static int imx290_probe(struct i2c_client *client)
> >  	if (ret)
> >  		return ret;
> > 
> > -	mutex_init(&imx290->lock);
> > -
> >  	/* Initialize and register subdev. */
> >  	ret = imx290_subdev_init(imx290);
> >  	if (ret)
> > -		goto err_mutex;
> > +		return ret;
> > 
> >  	ret = v4l2_async_register_subdev(&imx290->sd);
> >  	if (ret < 0) {
> > @@ -1300,8 +1294,6 @@ static int imx290_probe(struct i2c_client *client)
> > 
> >  err_subdev:
> >  	imx290_subdev_cleanup(imx290);
> > -err_mutex:
> > -	mutex_destroy(&imx290->lock);
> > 
> >  	return ret;
> >  }
> > @@ -1314,8 +1306,6 @@ static void imx290_remove(struct i2c_client *client)
> >  	v4l2_async_unregister_subdev(sd);
> >  	imx290_subdev_cleanup(imx290);
> > 
> > -	mutex_destroy(&imx290->lock);
> > -
> >  	pm_runtime_disable(imx290->dev);
> >  	if (!pm_runtime_status_suspended(imx290->dev))
> >  		imx290_power_off(imx290->dev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/17] media: i2c: imx290: Use V4L2 subdev active state

[Laurent Pinchart]

Hi Alexander,

On Mon, Jan 16, 2023 at 01:58:50PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 16, 2023 at 12:04:34PM +0100, Alexander Stein wrote:
> > Am Samstag, 14. Januar 2023, 18:17:56 CET schrieb Laurent Pinchart:
> > > Use the V4L2 subdev active state API to store the active format. This
> > > simplifies the driver not only by dropping the imx290 current_format
> > > field, but it also allows dropping the imx290 lock, replaced with the
> > > state lock.
> > > 
> > > The lock check in imx290_ctrl_update() can be dropped as
> > > imx290_set_fmt() can't be called anywmore with which set to ACTIVE
> > > before controls are initialized.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Remove lock check from imx290_ctrl_update()
> > > - Drop unrelated change
> > > - Fix error handling in imx290_subdev_init()
> > > - Handle errors from v4l2_subdev_init_finalize()
> > > - Return immediately from imx290_set_ctrl() for read-only controls
> > > - Call v4l2_subdev_cleanup()
> > > ---
> > >  drivers/media/i2c/imx290.c | 170 +++++++++++++++++--------------------
> > >  1 file changed, 80 insertions(+), 90 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index b01a627c8c03..94d23eea36be 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -177,12 +177,12 @@ struct imx290 {
> > >  	struct clk *xclk;
> > >  	struct regmap *regmap;
> > >  	u8 nlanes;
> > > -	u8 bpp;
> > > 
> > >  	struct v4l2_subdev sd;
> > >  	struct media_pad pad;
> > > -	struct v4l2_mbus_framefmt current_format;
> > > +
> > >  	const struct imx290_mode *current_mode;
> > > +	u8 bpp;
> > > 
> > >  	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
> > >  	struct gpio_desc *rst_gpio;
> > > @@ -192,8 +192,6 @@ struct imx290 {
> > >  	struct v4l2_ctrl *pixel_rate;
> > >  	struct v4l2_ctrl *hblank;
> > >  	struct v4l2_ctrl *vblank;
> > > -
> > > -	struct mutex lock;
> > >  };
> > > 
> > >  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > > @@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290
> > > *imx290, black_level >> (16 - imx290->bpp), err);
> > >  }
> > > 
> > > -static int imx290_write_current_format(struct imx290 *imx290)
> > > +static int imx290_setup_format(struct imx290 *imx290,
> > > +			       const struct v4l2_mbus_framefmt *format)
> > >  {
> > >  	const struct imx290_regval *regs;
> > >  	unsigned int num_regs;
> > >  	int ret;
> > > 
> > > -	switch (imx290->current_format.code) {
> > > +	switch (format->code) {
> > >  	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > >  		regs = imx290_10bit_settings;
> > >  		num_regs = ARRAY_SIZE(imx290_10bit_settings);
> > > @@ -561,12 +560,31 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >  	struct imx290 *imx290 = container_of(ctrl->handler,
> > >  					     struct imx290, ctrls);
> > > +	const struct v4l2_mbus_framefmt *format;
> > > +	struct v4l2_subdev_state *state;
> > >  	int ret = 0;
> > > 
> > > +	/*
> > > +	 * Return immediately for controls that don't need to be applied to the
> > > +	 * device. This includes all controls modified in imx290_ctrl_update(),
> > > +	 * which is called at probe time before runtime PM is initialized, so
> > > +	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> > > +	 */
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_LINK_FREQ:
> > > +	case V4L2_CID_PIXEL_RATE:
> > > +	case V4L2_CID_VBLANK:
> > > +	case V4L2_CID_HBLANK:
> > > +		return 0;
> > > +	}
> > 
> > I don't like keeping the IDs synchronized in two functions. Isn't checking for 
> > 'ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY' enough?
> 
> It should be. I'll do so.
> 
> > On that side V4L2_CID_PIXEL_RATE is missing the read-only flag, no?
> 
> That's right. I'll add a patch in v3 to make it read-only.

Actually that's not needed, V4L2_CID_PIXEL_RATE is made read-only by the
control framework. See the v4l2_ctrl_fill() function (called by
v4l2_ctrl_new_std()) in drivers/media/v4l2-core/v4l2-ctrls-defs.c.

> > >  	/* V4L2 controls values will be applied only when power is already up */
> > >  	if (!pm_runtime_get_if_in_use(imx290->dev))
> > >  		return 0;
> > > 
> > > +	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> > > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > > +
> > >  	switch (ctrl->id) {
> > >  	case V4L2_CID_ANALOGUE_GAIN:
> > >  		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
> > > @@ -592,6 +610,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  					       &ret);
> > >  		}
> > >  		break;
> > > +
> > >  	default:
> > >  		ret = -EINVAL;
> > >  		break;
> > > @@ -626,13 +645,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> > >  	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
> > >  	u64 pixel_rate;
> > > 
> > > -	/*
> > > -	 * This function may be called from imx290_set_fmt() before controls
> > > -	 * get created by imx290_ctrl_init(). Return immediately in that case.
> > > -	 */
> > > -	if (!imx290->ctrls.lock)
> > > -		return;
> > > -
> > >  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > >  	pixel_rate = link_freq * 2 * imx290->nlanes;
> > >  	do_div(pixel_rate, imx290->bpp);
> > > @@ -654,7 +666,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > >  		return ret;
> > > 
> > >  	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> > > -	imx290->ctrls.lock = &imx290->lock;
> > > 
> > >  	/*
> > >  	 * The sensor has an analog gain and a digital gain, both controlled
> > > @@ -719,11 +730,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > >  		return ret;
> > >  	}
> > > 
> > > -	mutex_lock(imx290->ctrls.lock);
> > > -	imx290_ctrl_update(imx290, &imx290->current_format,
> > > -			   imx290->current_mode);
> > > -	mutex_unlock(imx290->ctrls.lock);
> > > -
> > >  	return 0;
> > >  }
> > > 
> > > @@ -732,8 +738,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > >   */
> > > 
> > >  /* Start streaming */
> > > -static int imx290_start_streaming(struct imx290 *imx290)
> > > +static int imx290_start_streaming(struct imx290 *imx290,
> > > +				  struct v4l2_subdev_state *state)
> > >  {
> > > +	const struct v4l2_mbus_framefmt *format;
> > >  	int ret;
> > > 
> > >  	/* Set init register settings */
> > > @@ -746,7 +754,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> > > }
> > > 
> > >  	/* Apply the register values related to current frame format */
> > > -	ret = imx290_write_current_format(imx290);
> > > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > > +	ret = imx290_setup_format(imx290, format);
> > >  	if (ret < 0) {
> > >  		dev_err(imx290->dev, "Could not set frame format\n");
> > >  		return ret;
> > > @@ -766,7 +775,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
> > >  		return ret;
> > > 
> > >  	/* Apply customized values from user */
> > > -	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> > > +	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> > >  	if (ret) {
> > >  		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> > >  		return ret;
> > > @@ -795,39 +804,32 @@ static int imx290_stop_streaming(struct imx290 *imx290)
> > >  static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> > >  {
> > >  	struct imx290 *imx290 = to_imx290(sd);
> > > +	struct v4l2_subdev_state *state;
> > >  	int ret = 0;
> > > 
> > > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> > 
> > Is there ever the case that state==NULL? If so you will deference a NULL 
> > pointer in v4l2_subdev_unlock_state() later on.
> 
> No, that can't happen, the driver calls v4l2_subdev_init_finalize() at
> probe time, so sd->active_state can't be NULL.
> 
> > >  	if (enable) {
> > >  		ret = pm_runtime_resume_and_get(imx290->dev);
> > >  		if (ret < 0)
> > > -			goto unlock_and_return;
> > > +			goto unlock;
> > > 
> > > -		ret = imx290_start_streaming(imx290);
> > > +		ret = imx290_start_streaming(imx290, state);
> > >  		if (ret) {
> > >  			dev_err(imx290->dev, "Start stream failed\n");
> > >  			pm_runtime_put(imx290->dev);
> > > -			goto unlock_and_return;
> > > +			goto unlock;
> > >  		}
> > >  	} else {
> > >  		imx290_stop_streaming(imx290);
> > >  		pm_runtime_put(imx290->dev);
> > >  	}
> > > 
> > > -unlock_and_return:
> > > -
> > > +unlock:
> > > +	v4l2_subdev_unlock_state(state);
> > >  	return ret;
> > >  }
> > > 
> > > -static struct v4l2_mbus_framefmt *
> > > -imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
> > > -		      u32 which)
> > > -{
> > > -	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > -		return &imx290->current_format;
> > > -	else
> > > -		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> > > -}
> > > -
> > >  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
> > >  				 struct v4l2_subdev_state *sd_state,
> > >  				 struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -862,23 +864,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd
> > >  	return 0;
> > >  }
> > > 
> > > -static int imx290_get_fmt(struct v4l2_subdev *sd,
> > > -			  struct v4l2_subdev_state *sd_state,
> > > -			  struct v4l2_subdev_format *fmt)
> > > -{
> > > -	struct imx290 *imx290 = to_imx290(sd);
> > > -	struct v4l2_mbus_framefmt *framefmt;
> > > -
> > > -	mutex_lock(&imx290->lock);
> > > -
> > > -	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> > > -	fmt->format = *framefmt;
> > > -
> > > -	mutex_unlock(&imx290->lock);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >  			  struct v4l2_subdev_state *sd_state,
> > >  			  struct v4l2_subdev_format *fmt)
> > > @@ -888,8 +873,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >  	struct v4l2_mbus_framefmt *format;
> > >  	unsigned int i;
> > > 
> > > -	mutex_lock(&imx290->lock);
> > > -
> > >  	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
> > >  				      imx290_modes_num(imx290), width, height,
> > >  				      fmt->format.width, fmt->format.height);
> > > @@ -907,7 +890,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >  	fmt->format.code = imx290_formats[i].code;
> > >  	fmt->format.field = V4L2_FIELD_NONE;
> > > 
> > > -	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> > > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > 
> > >  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > >  		imx290->current_mode = mode;
> > > @@ -918,8 +901,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > > 
> > >  	*format = fmt->format;
> > > 
> > > -	mutex_unlock(&imx290->lock);
> > > -
> > >  	return 0;
> > >  }
> > > 
> > > @@ -927,14 +908,11 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> > >  				struct v4l2_subdev_state *sd_state,
> > >  				struct v4l2_subdev_selection *sel)
> > >  {
> > > -	struct imx290 *imx290 = to_imx290(sd);
> > >  	struct v4l2_mbus_framefmt *format;
> > > 
> > >  	switch (sel->target) {
> > >  	case V4L2_SEL_TGT_CROP: {
> > > -		format = imx290_get_pad_format(imx290, sd_state, sel->which);
> > > -
> > > -		mutex_lock(&imx290->lock);
> > > +		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > 
> > >  		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> > >  			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
> > > @@ -943,7 +921,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> > >  		sel->r.width = format->width;
> > >  		sel->r.height = format->height;
> > > 
> > > -		mutex_unlock(&imx290->lock);
> > >  		return 0;
> > >  	}
> > > 
> > > @@ -972,11 +949,13 @@ static int imx290_get_selection(struct v4l2_subdev *sd
> > >  static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
> > >  				  struct v4l2_subdev_state *sd_state)
> > >  {
> > > -	struct v4l2_subdev_format fmt = { 0 };
> > > -
> > > -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > > -	fmt.format.width = 1920;
> > > -	fmt.format.height = 1080;
> > > +	struct v4l2_subdev_format fmt = {
> > > +		.which = V4L2_SUBDEV_FORMAT_TRY,
> > > +		.format = {
> > > +			.width = 1920,
> > > +			.height = 1080,
> > > +		},
> > > +	};
> > > 
> > >  	imx290_set_fmt(subdev, sd_state, &fmt);
> > > 
> > > @@ -991,7 +970,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> > >  	.init_cfg = imx290_entity_init_cfg,
> > >  	.enum_mbus_code = imx290_enum_mbus_code,
> > >  	.enum_frame_size = imx290_enum_frame_size,
> > > -	.get_fmt = imx290_get_fmt,
> > > +	.get_fmt = v4l2_subdev_get_fmt,
> > >  	.set_fmt = imx290_set_fmt,
> > >  	.get_selection = imx290_get_selection,
> > >  };
> > > @@ -1008,20 +987,12 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
> > >  static int imx290_subdev_init(struct imx290 *imx290)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(imx290->dev);
> > > +	const struct v4l2_mbus_framefmt *format;
> > > +	struct v4l2_subdev_state *state;
> > >  	int ret;
> > > 
> > > -	/*
> > > -	 * Initialize the frame format. In particular, imx290->current_mode
> > > -	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
> > > -	 * below relies on these fields.
> > > -	 */
> > > -	imx290_entity_init_cfg(&imx290->sd, NULL);
> > > -
> > > -	ret = imx290_ctrl_init(imx290);
> > > -	if (ret < 0) {
> > > -		dev_err(imx290->dev, "Control initialization error %d\n", ret);
> > > -		return ret;
> > > -	}
> > > +	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > > +	imx290->bpp = imx290_formats[0].bpp;
> > > 
> > >  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> > >  	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > @@ -1033,15 +1004,40 @@ static int imx290_subdev_init(struct imx290 *imx290)
> > >  	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> > >  	if (ret < 0) {
> > >  		dev_err(imx290->dev, "Could not register media entity\n");
> > > -		v4l2_ctrl_handler_free(&imx290->ctrls);
> > >  		return ret;
> > >  	}
> > > 
> > > +	ret = imx290_ctrl_init(imx290);
> > > +	if (ret < 0) {
> > > +		dev_err(imx290->dev, "Control initialization error %d\n", ret);
> > > +		goto err_media;
> > > +	}
> > > +
> > > +	imx290->sd.state_lock = imx290->ctrls.lock;
> > > +
> > > +	ret = v4l2_subdev_init_finalize(&imx290->sd);
> > > +	if (ret < 0) {
> > > +		dev_err(imx290->dev, "subdev initialization error %d\n", ret);
> > > +		goto err_ctrls;
> > > +	}
> > > +
> > > +	state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
> > > +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> > > +	imx290_ctrl_update(imx290, format, imx290->current_mode);
> > > +	v4l2_subdev_unlock_state(state);
> > > +
> > >  	return 0;
> > > +
> > > +err_ctrls:
> > > +	v4l2_ctrl_handler_free(&imx290->ctrls);
> > > +err_media:
> > > +	media_entity_cleanup(&imx290->sd.entity);
> > > +	return ret;
> > >  }
> > > 
> > >  static void imx290_subdev_cleanup(struct imx290 *imx290)
> > >  {
> > > +	v4l2_subdev_cleanup(&imx290->sd);
> > >  	media_entity_cleanup(&imx290->sd.entity);
> > >  	v4l2_ctrl_handler_free(&imx290->ctrls);
> > >  }
> > > @@ -1272,12 +1268,10 @@ static int imx290_probe(struct i2c_client *client)
> > >  	if (ret)
> > >  		return ret;
> > > 
> > > -	mutex_init(&imx290->lock);
> > > -
> > >  	/* Initialize and register subdev. */
> > >  	ret = imx290_subdev_init(imx290);
> > >  	if (ret)
> > > -		goto err_mutex;
> > > +		return ret;
> > > 
> > >  	ret = v4l2_async_register_subdev(&imx290->sd);
> > >  	if (ret < 0) {
> > > @@ -1300,8 +1294,6 @@ static int imx290_probe(struct i2c_client *client)
> > > 
> > >  err_subdev:
> > >  	imx290_subdev_cleanup(imx290);
> > > -err_mutex:
> > > -	mutex_destroy(&imx290->lock);
> > > 
> > >  	return ret;
> > >  }
> > > @@ -1314,8 +1306,6 @@ static void imx290_remove(struct i2c_client *client)
> > >  	v4l2_async_unregister_subdev(sd);
> > >  	imx290_subdev_cleanup(imx290);
> > > 
> > > -	mutex_destroy(&imx290->lock);
> > > -
> > >  	pm_runtime_disable(imx290->dev);
> > >  	if (!pm_runtime_status_suspended(imx290->dev))
> > >  		imx290_power_off(imx290->dev);

-- 
Regards,

Laurent Pinchart

[PATCH v2 12/17] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt

[Laurent Pinchart]

The imx290_pixfmt structure contains information about formats,
currently limited to the bpp value. Extend it with the register settings
for each format, and rename it to imx290_format_info to make its purpose
clearer. Add a function named imx290_format_info() to look up format
info for a media bus code, and use it through the code. This allows
dropping the imx290 bpp field as the value is now looked up dynamically.

The error handling in imx290_setup_format() can also be dropped, as the
format is guaranteed by imx290_set_fmt() to be valid.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 84 ++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 94d23eea36be..b16fd76e0737 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -182,7 +182,6 @@ struct imx290 {
 	struct media_pad pad;
 
 	const struct imx290_mode *current_mode;
-	u8 bpp;
 
 	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
 	struct gpio_desc *rst_gpio;
@@ -414,16 +413,41 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
 		return ARRAY_SIZE(imx290_modes_4lanes);
 }
 
-struct imx290_pixfmt {
+struct imx290_format_info {
 	u32 code;
 	u8 bpp;
+	const struct imx290_regval *regs;
+	unsigned int num_regs;
 };
 
-static const struct imx290_pixfmt imx290_formats[] = {
-	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
-	{ MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
+static const struct imx290_format_info imx290_formats[] = {
+	{
+		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+		.bpp = 10,
+		.regs = imx290_10bit_settings,
+		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
+		.bpp = 12,
+		.regs = imx290_12bit_settings,
+		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
+	}
 };
 
+static const struct imx290_format_info *imx290_format_info(u32 code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
+		const struct imx290_format_info *info = &imx290_formats[i];
+
+		if (info->code == code)
+			return info;
+	}
+
+	return NULL;
+}
+
 /* -----------------------------------------------------------------------------
  * Register access
  */
@@ -516,40 +540,31 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
 }
 
 static int imx290_set_black_level(struct imx290 *imx290,
+				  const struct v4l2_mbus_framefmt *format,
 				  unsigned int black_level, int *err)
 {
+	unsigned int bpp = imx290_format_info(format->code)->bpp;
+
 	return imx290_write(imx290, IMX290_BLKLEVEL,
-			    black_level >> (16 - imx290->bpp), err);
+			    black_level >> (16 - bpp), err);
 }
 
 static int imx290_setup_format(struct imx290 *imx290,
 			       const struct v4l2_mbus_framefmt *format)
 {
-	const struct imx290_regval *regs;
-	unsigned int num_regs;
+	const struct imx290_format_info *info;
 	int ret;
 
-	switch (format->code) {
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		regs = imx290_10bit_settings;
-		num_regs = ARRAY_SIZE(imx290_10bit_settings);
-		break;
-	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		regs = imx290_12bit_settings;
-		num_regs = ARRAY_SIZE(imx290_12bit_settings);
-		break;
-	default:
-		dev_err(imx290->dev, "Unknown pixel format\n");
-		return -EINVAL;
-	}
+	info = imx290_format_info(format->code);
 
-	ret = imx290_set_register_array(imx290, regs, num_regs);
+	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
 	if (ret < 0) {
 		dev_err(imx290->dev, "Could not set format registers\n");
 		return ret;
 	}
 
-	return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
+	return imx290_set_black_level(imx290, format,
+				      IMX290_BLACK_LEVEL_DEFAULT, &ret);
 }
 
 /* ----------------------------------------------------------------------------
@@ -597,7 +612,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
-			imx290_set_black_level(imx290, 0, &ret);
+			imx290_set_black_level(imx290, format, 0, &ret);
 			usleep_range(10000, 11000);
 			imx290_write(imx290, IMX290_PGCTRL,
 				     (u8)(IMX290_PGCTRL_REGEN |
@@ -606,8 +621,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		} else {
 			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
 			usleep_range(10000, 11000);
-			imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
-					       &ret);
+			imx290_set_black_level(imx290, format,
+					       IMX290_BLACK_LEVEL_DEFAULT, &ret);
 		}
 		break;
 
@@ -647,7 +662,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 
 	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
 	pixel_rate = link_freq * 2 * imx290->nlanes;
-	do_div(pixel_rate, imx290->bpp);
+	do_div(pixel_rate, imx290_format_info(format->code)->bpp);
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
@@ -849,8 +864,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
 	const struct imx290 *imx290 = to_imx290(sd);
 	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
 
-	if ((fse->code != imx290_formats[0].code) &&
-	    (fse->code != imx290_formats[1].code))
+	if (!imx290_format_info(fse->code))
 		return -EINVAL;
 
 	if (fse->index >= imx290_modes_num(imx290))
@@ -871,7 +885,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	struct imx290 *imx290 = to_imx290(sd);
 	const struct imx290_mode *mode;
 	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
 
 	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
 				      imx290_modes_num(imx290), width, height,
@@ -880,21 +893,15 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
 
-	for (i = 0; i < ARRAY_SIZE(imx290_formats); i++)
-		if (imx290_formats[i].code == fmt->format.code)
-			break;
+	if (!imx290_format_info(fmt->format.code))
+		fmt->format.code = imx290_formats[0].code;
 
-	if (i >= ARRAY_SIZE(imx290_formats))
-		i = 0;
-
-	fmt->format.code = imx290_formats[i].code;
 	fmt->format.field = V4L2_FIELD_NONE;
 
 	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		imx290->current_mode = mode;
-		imx290->bpp = imx290_formats[i].bpp;
 
 		imx290_ctrl_update(imx290, &fmt->format, mode);
 	}
@@ -992,7 +999,6 @@ static int imx290_subdev_init(struct imx290 *imx290)
 	int ret;
 
 	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
-	imx290->bpp = imx290_formats[0].bpp;
 
 	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
 	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-- 
Regards,

Laurent Pinchart

[PATCH v2 13/17] media: i2c: imx290: Use runtime PM autosuspend

[Laurent Pinchart]

Use runtime PM autosuspend to avoid powering off the sensor during fast
stop-reconfigure-restart cycles. This also fixes runtime PM handling in
the probe function that didn't suspend the device, effectively leaving
it resumed forever.

While at it, improve documentation of power management in probe() and
remove().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Enable autosuspend before registering the subdev
- Decrease the PM usage count after registering the subdev
- Use pm_runtime_put_autosuspend() in imx290_set_ctrl()
---
 drivers/media/i2c/imx290.c | 58 +++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index b16fd76e0737..6a3e93c10fb1 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -631,7 +631,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
-	pm_runtime_put(imx290->dev);
+	pm_runtime_mark_last_busy(imx290->dev);
+	pm_runtime_put_autosuspend(imx290->dev);
 
 	return ret;
 }
@@ -832,12 +833,13 @@ static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
 		ret = imx290_start_streaming(imx290, state);
 		if (ret) {
 			dev_err(imx290->dev, "Start stream failed\n");
-			pm_runtime_put(imx290->dev);
+			pm_runtime_put_sync(imx290->dev);
 			goto unlock;
 		}
 	} else {
 		imx290_stop_streaming(imx290);
-		pm_runtime_put(imx290->dev);
+		pm_runtime_mark_last_busy(imx290->dev);
+		pm_runtime_put_autosuspend(imx290->dev);
 	}
 
 unlock:
@@ -1274,33 +1276,59 @@ static int imx290_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	/* Initialize and register subdev. */
+	/* Initialize the V4L2 subdev. */
 	ret = imx290_subdev_init(imx290);
 	if (ret)
 		return ret;
 
-	ret = v4l2_async_register_subdev(&imx290->sd);
-	if (ret < 0) {
-		dev_err(dev, "Could not register v4l2 device\n");
-		goto err_subdev;
-	}
-
-	/* Power on the device to match runtime PM state below */
+	/*
+	 * Enable power management. The driver supports runtime PM, but needs to
+	 * work when runtime PM is disabled in the kernel. To that end, power
+	 * the sensor on manually here.
+	 */
 	ret = imx290_power_on(dev);
 	if (ret < 0) {
 		dev_err(dev, "Could not power on the device\n");
 		goto err_subdev;
 	}
 
+	/*
+	 * Enable runtime PM with autosuspend. As the device has been powered
+	 * manually, mark it as active, and increase the usage count without
+	 * resuming the device.
+	 */
 	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
 	pm_runtime_enable(dev);
-	pm_runtime_idle(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	/*
+	 * Finally, register the V4L2 subdev. This must be done after
+	 * initializing everything as the subdev can be used immediately after
+	 * being registered.
+	 */
+	ret = v4l2_async_register_subdev(&imx290->sd);
+	if (ret < 0) {
+		dev_err(dev, "Could not register v4l2 device\n");
+		goto err_subdev;
+	}
+
+	/*
+	 * Decrease the PM usage count. The device will get suspended after the
+	 * autosuspend delay, turning the power off.
+	 */
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return 0;
 
+err_pm:
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	imx290_power_off(dev);
 err_subdev:
 	imx290_subdev_cleanup(imx290);
-
 	return ret;
 }
 
@@ -1312,6 +1340,10 @@ static void imx290_remove(struct i2c_client *client)
 	v4l2_async_unregister_subdev(sd);
 	imx290_subdev_cleanup(imx290);
 
+	/*
+	 * Disable runtime PM. In case runtime PM is disabled in the kernel,
+	 * make sure to turn power off manually.
+	 */
 	pm_runtime_disable(imx290->dev);
 	if (!pm_runtime_status_suspended(imx290->dev))
 		imx290_power_off(imx290->dev);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 13/17] media: i2c: imx290: Use runtime PM autosuspend

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:58 CET schrieb Laurent Pinchart:
> Use runtime PM autosuspend to avoid powering off the sensor during fast
> stop-reconfigure-restart cycles. This also fixes runtime PM handling in
> the probe function that didn't suspend the device, effectively leaving
> it resumed forever.
> 
> While at it, improve documentation of power management in probe() and
> remove().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Enable autosuspend before registering the subdev
> - Decrease the PM usage count after registering the subdev
> - Use pm_runtime_put_autosuspend() in imx290_set_ctrl()
> ---
>  drivers/media/i2c/imx290.c | 58 +++++++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index b16fd76e0737..6a3e93c10fb1 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -631,7 +631,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
> 
> -	pm_runtime_put(imx290->dev);
> +	pm_runtime_mark_last_busy(imx290->dev);
> +	pm_runtime_put_autosuspend(imx290->dev);
> 
>  	return ret;
>  }
> @@ -832,12 +833,13 @@ static int imx290_set_stream(struct v4l2_subdev *sd,
> int enable) ret = imx290_start_streaming(imx290, state);
>  		if (ret) {
>  			dev_err(imx290->dev, "Start stream failed\n");
> -			pm_runtime_put(imx290->dev);
> +			pm_runtime_put_sync(imx290->dev);
>  			goto unlock;
>  		}
>  	} else {
>  		imx290_stop_streaming(imx290);
> -		pm_runtime_put(imx290->dev);
> +		pm_runtime_mark_last_busy(imx290->dev);
> +		pm_runtime_put_autosuspend(imx290->dev);
>  	}
> 
>  unlock:
> @@ -1274,33 +1276,59 @@ static int imx290_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
> 
> -	/* Initialize and register subdev. */
> +	/* Initialize the V4L2 subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
>  		return ret;
> 
> -	ret = v4l2_async_register_subdev(&imx290->sd);
> -	if (ret < 0) {
> -		dev_err(dev, "Could not register v4l2 device\n");
> -		goto err_subdev;
> -	}
> -
> -	/* Power on the device to match runtime PM state below */
> +	/*
> +	 * Enable power management. The driver supports runtime PM, but 
needs to
> +	 * work when runtime PM is disabled in the kernel. To that end, 
power
> +	 * the sensor on manually here.
> +	 */
>  	ret = imx290_power_on(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "Could not power on the device\n");
>  		goto err_subdev;
>  	}
> 
> +	/*
> +	 * Enable runtime PM with autosuspend. As the device has been 
powered
> +	 * manually, mark it as active, and increase the usage count without
> +	 * resuming the device.
> +	 */
>  	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
>  	pm_runtime_enable(dev);
> -	pm_runtime_idle(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	/*
> +	 * Finally, register the V4L2 subdev. This must be done after
> +	 * initializing everything as the subdev can be used immediately 
after
> +	 * being registered.
> +	 */
> +	ret = v4l2_async_register_subdev(&imx290->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "Could not register v4l2 device\n");
> +		goto err_subdev;

I assume this should be err_pm. Otherwise
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Best regards,
Alexander

> +	}
> +
> +	/*
> +	 * Decrease the PM usage count. The device will get suspended after 
the
> +	 * autosuspend delay, turning the power off.
> +	 */
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> 
>  	return 0;
> 
> +err_pm:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	imx290_power_off(dev);
>  err_subdev:
>  	imx290_subdev_cleanup(imx290);
> -
>  	return ret;
>  }
> 
> @@ -1312,6 +1340,10 @@ static void imx290_remove(struct i2c_client *client)
>  	v4l2_async_unregister_subdev(sd);
>  	imx290_subdev_cleanup(imx290);
> 
> +	/*
> +	 * Disable runtime PM. In case runtime PM is disabled in the kernel,
> +	 * make sure to turn power off manually.
> +	 */
>  	pm_runtime_disable(imx290->dev);
>  	if (!pm_runtime_status_suspended(imx290->dev))
>  		imx290_power_off(imx290->dev);

[PATCH v2 14/17] media: i2c: imx290: Initialize runtime PM before subdev

[Laurent Pinchart]

Initializing the subdev before runtime PM means that no subdev
initialization can interact with the runtime PM framework. This can be
problematic when modifying controls, as the .s_ctrl() handler commonly
calls pm_runtime_get_if_in_use(). These code paths are not trivial,
making the driver fragile and possibly causing subtle bugs.

To make the subdev initialization more robust, initialize runtime PM
first.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 57 ++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6a3e93c10fb1..c3ffb23104bf 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -581,9 +581,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	/*
 	 * Return immediately for controls that don't need to be applied to the
-	 * device. This includes all controls modified in imx290_ctrl_update(),
-	 * which is called at probe time before runtime PM is initialized, so
-	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
+	 * device.
 	 */
 	switch (ctrl->id) {
 	case V4L2_CID_LINK_FREQ:
@@ -1054,22 +1052,20 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
  * Power management
  */
 
-static int imx290_power_on(struct device *dev)
+static int imx290_power_on(struct imx290 *imx290)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct imx290 *imx290 = to_imx290(sd);
 	int ret;
 
 	ret = clk_prepare_enable(imx290->xclk);
 	if (ret) {
-		dev_err(dev, "Failed to enable clock\n");
+		dev_err(imx290->dev, "Failed to enable clock\n");
 		return ret;
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
 				    imx290->supplies);
 	if (ret) {
-		dev_err(dev, "Failed to enable regulators\n");
+		dev_err(imx290->dev, "Failed to enable regulators\n");
 		clk_disable_unprepare(imx290->xclk);
 		return ret;
 	}
@@ -1084,20 +1080,33 @@ static int imx290_power_on(struct device *dev)
 	return 0;
 }
 
-static int imx290_power_off(struct device *dev)
+static void imx290_power_off(struct imx290 *imx290)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct imx290 *imx290 = to_imx290(sd);
-
 	clk_disable_unprepare(imx290->xclk);
 	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290->supplies);
+}
+
+static int imx290_runtime_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx290 *imx290 = to_imx290(sd);
+
+	return imx290_power_on(imx290);
+}
+
+static int imx290_runtime_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx290 *imx290 = to_imx290(sd);
+
+	imx290_power_off(imx290);
 
 	return 0;
 }
 
 static const struct dev_pm_ops imx290_pm_ops = {
-	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
+	SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
 };
 
 /* ----------------------------------------------------------------------------
@@ -1276,20 +1285,15 @@ static int imx290_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	/* Initialize the V4L2 subdev. */
-	ret = imx290_subdev_init(imx290);
-	if (ret)
-		return ret;
-
 	/*
 	 * Enable power management. The driver supports runtime PM, but needs to
 	 * work when runtime PM is disabled in the kernel. To that end, power
 	 * the sensor on manually here.
 	 */
-	ret = imx290_power_on(dev);
+	ret = imx290_power_on(imx290);
 	if (ret < 0) {
 		dev_err(dev, "Could not power on the device\n");
-		goto err_subdev;
+		return ret;
 	}
 
 	/*
@@ -1303,6 +1307,11 @@ static int imx290_probe(struct i2c_client *client)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
+	/* Initialize the V4L2 subdev. */
+	ret = imx290_subdev_init(imx290);
+	if (ret)
+		goto err_pm;
+
 	/*
 	 * Finally, register the V4L2 subdev. This must be done after
 	 * initializing everything as the subdev can be used immediately after
@@ -1323,12 +1332,12 @@ static int imx290_probe(struct i2c_client *client)
 
 	return 0;
 
+err_subdev:
+	imx290_subdev_cleanup(imx290);
 err_pm:
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
-	imx290_power_off(dev);
-err_subdev:
-	imx290_subdev_cleanup(imx290);
+	imx290_power_off(imx290);
 	return ret;
 }
 
@@ -1346,7 +1355,7 @@ static void imx290_remove(struct i2c_client *client)
 	 */
 	pm_runtime_disable(imx290->dev);
 	if (!pm_runtime_status_suspended(imx290->dev))
-		imx290_power_off(imx290->dev);
+		imx290_power_off(imx290);
 	pm_runtime_set_suspended(imx290->dev);
 }
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 14/17] media: i2c: imx290: Initialize runtime PM before subdev

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:17:59 CET schrieb Laurent Pinchart:
> Initializing the subdev before runtime PM means that no subdev
> initialization can interact with the runtime PM framework. This can be
> problematic when modifying controls, as the .s_ctrl() handler commonly
> calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> making the driver fragile and possibly causing subtle bugs.
> 
> To make the subdev initialization more robust, initialize runtime PM
> first.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 57 ++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 6a3e93c10fb1..c3ffb23104bf 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -581,9 +581,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> 
>  	/*
>  	 * Return immediately for controls that don't need to be applied to 
the
> -	 * device. This includes all controls modified in 
imx290_ctrl_update(),
> -	 * which is called at probe time before runtime PM is initialized, 
so
> -	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> +	 * device.
>  	 */
>  	switch (ctrl->id) {
>  	case V4L2_CID_LINK_FREQ:
> @@ -1054,22 +1052,20 @@ static void imx290_subdev_cleanup(struct imx290
> *imx290) * Power management
>   */
> 
> -static int imx290_power_on(struct device *dev)
> +static int imx290_power_on(struct imx290 *imx290)
>  {
> -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> -	struct imx290 *imx290 = to_imx290(sd);
>  	int ret;
> 
>  	ret = clk_prepare_enable(imx290->xclk);
>  	if (ret) {
> -		dev_err(dev, "Failed to enable clock\n");
> +		dev_err(imx290->dev, "Failed to enable clock\n");
>  		return ret;
>  	}
> 
>  	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
>  				    imx290->supplies);
>  	if (ret) {
> -		dev_err(dev, "Failed to enable regulators\n");
> +		dev_err(imx290->dev, "Failed to enable regulators\n");
>  		clk_disable_unprepare(imx290->xclk);
>  		return ret;
>  	}
> @@ -1084,20 +1080,33 @@ static int imx290_power_on(struct device *dev)
>  	return 0;
>  }
> 
> -static int imx290_power_off(struct device *dev)
> +static void imx290_power_off(struct imx290 *imx290)
>  {
> -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> -	struct imx290 *imx290 = to_imx290(sd);
> -
>  	clk_disable_unprepare(imx290->xclk);
>  	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
>  	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290-
>supplies);
> +}
> +
> +static int imx290_runtime_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx290 *imx290 = to_imx290(sd);
> +
> +	return imx290_power_on(imx290);
> +}
> +
> +static int imx290_runtime_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx290 *imx290 = to_imx290(sd);
> +
> +	imx290_power_off(imx290);
> 
>  	return 0;
>  }
> 
>  static const struct dev_pm_ops imx290_pm_ops = {
> -	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
> +	SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, 
NULL)
>  };
> 
>  /*
> ---------------------------------------------------------------------------
> - @@ -1276,20 +1285,15 @@ static int imx290_probe(struct i2c_client *client)
> if (ret)
>  		return ret;
> 
> -	/* Initialize the V4L2 subdev. */
> -	ret = imx290_subdev_init(imx290);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * Enable power management. The driver supports runtime PM, but 
needs to
>  	 * work when runtime PM is disabled in the kernel. To that end, 
power
>  	 * the sensor on manually here.
>  	 */
> -	ret = imx290_power_on(dev);
> +	ret = imx290_power_on(imx290);
>  	if (ret < 0) {
>  		dev_err(dev, "Could not power on the device\n");
> -		goto err_subdev;
> +		return ret;
>  	}
> 
>  	/*
> @@ -1303,6 +1307,11 @@ static int imx290_probe(struct i2c_client *client)
>  	pm_runtime_set_autosuspend_delay(dev, 1000);
>  	pm_runtime_use_autosuspend(dev);
> 
> +	/* Initialize the V4L2 subdev. */
> +	ret = imx290_subdev_init(imx290);
> +	if (ret)
> +		goto err_pm;
> +
>  	/*
>  	 * Finally, register the V4L2 subdev. This must be done after
>  	 * initializing everything as the subdev can be used immediately 
after
> @@ -1323,12 +1332,12 @@ static int imx290_probe(struct i2c_client *client)
> 
>  	return 0;
> 
> +err_subdev:
> +	imx290_subdev_cleanup(imx290);
>  err_pm:
>  	pm_runtime_disable(dev);
>  	pm_runtime_put_noidle(dev);
> -	imx290_power_off(dev);
> -err_subdev:
> -	imx290_subdev_cleanup(imx290);
> +	imx290_power_off(imx290);
>  	return ret;
>  }
> 
> @@ -1346,7 +1355,7 @@ static void imx290_remove(struct i2c_client *client)
>  	 */
>  	pm_runtime_disable(imx290->dev);
>  	if (!pm_runtime_status_suspended(imx290->dev))
> -		imx290_power_off(imx290->dev);
> +		imx290_power_off(imx290);
>  	pm_runtime_set_suspended(imx290->dev);
>  }

Re: [PATCH v2 14/17] media: i2c: imx290: Initialize runtime PM before subdev

[Laurent Pinchart]

Hi Alexander,

On Mon, Jan 16, 2023 at 12:14:46PM +0100, Alexander Stein wrote:
> Am Samstag, 14. Januar 2023, 18:17:59 CET schrieb Laurent Pinchart:
> > Initializing the subdev before runtime PM means that no subdev
> > initialization can interact with the runtime PM framework. This can be
> > problematic when modifying controls, as the .s_ctrl() handler commonly
> > calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> > making the driver fragile and possibly causing subtle bugs.
> > 
> > To make the subdev initialization more robust, initialize runtime PM
> > first.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you for giving me my own ack, but yours would be more useful :-)

> > ---
> >  drivers/media/i2c/imx290.c | 57 ++++++++++++++++++++++----------------
> >  1 file changed, 33 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 6a3e93c10fb1..c3ffb23104bf 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -581,9 +581,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > 
> >  	/*
> >  	 * Return immediately for controls that don't need to be applied to the
> > -	 * device. This includes all controls modified in imx290_ctrl_update(),
> > -	 * which is called at probe time before runtime PM is initialized, so
> > -	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> > +	 * device.
> >  	 */
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_LINK_FREQ:
> > @@ -1054,22 +1052,20 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> >   * Power management
> >   */
> > 
> > -static int imx290_power_on(struct device *dev)
> > +static int imx290_power_on(struct imx290 *imx290)
> >  {
> > -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > -	struct imx290 *imx290 = to_imx290(sd);
> >  	int ret;
> > 
> >  	ret = clk_prepare_enable(imx290->xclk);
> >  	if (ret) {
> > -		dev_err(dev, "Failed to enable clock\n");
> > +		dev_err(imx290->dev, "Failed to enable clock\n");
> >  		return ret;
> >  	}
> > 
> >  	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
> >  				    imx290->supplies);
> >  	if (ret) {
> > -		dev_err(dev, "Failed to enable regulators\n");
> > +		dev_err(imx290->dev, "Failed to enable regulators\n");
> >  		clk_disable_unprepare(imx290->xclk);
> >  		return ret;
> >  	}
> > @@ -1084,20 +1080,33 @@ static int imx290_power_on(struct device *dev)
> >  	return 0;
> >  }
> > 
> > -static int imx290_power_off(struct device *dev)
> > +static void imx290_power_off(struct imx290 *imx290)
> >  {
> > -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > -	struct imx290 *imx290 = to_imx290(sd);
> > -
> >  	clk_disable_unprepare(imx290->xclk);
> >  	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
> >  	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290->supplies);
> > +}
> > +
> > +static int imx290_runtime_resume(struct device *dev)
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct imx290 *imx290 = to_imx290(sd);
> > +
> > +	return imx290_power_on(imx290);
> > +}
> > +
> > +static int imx290_runtime_suspend(struct device *dev)
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct imx290 *imx290 = to_imx290(sd);
> > +
> > +	imx290_power_off(imx290);
> > 
> >  	return 0;
> >  }
> > 
> >  static const struct dev_pm_ops imx290_pm_ops = {
> > -	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
> > +	SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
> >  };
> > 
> >  /* ----------------------------------------------------------------------------
> > @@ -1276,20 +1285,15 @@ static int imx290_probe(struct i2c_client *client)
> > if (ret)
> >  		return ret;
> > 
> > -	/* Initialize the V4L2 subdev. */
> > -	ret = imx290_subdev_init(imx290);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/*
> >  	 * Enable power management. The driver supports runtime PM, but needs to
> >  	 * work when runtime PM is disabled in the kernel. To that end, power
> >  	 * the sensor on manually here.
> >  	 */
> > -	ret = imx290_power_on(dev);
> > +	ret = imx290_power_on(imx290);
> >  	if (ret < 0) {
> >  		dev_err(dev, "Could not power on the device\n");
> > -		goto err_subdev;
> > +		return ret;
> >  	}
> > 
> >  	/*
> > @@ -1303,6 +1307,11 @@ static int imx290_probe(struct i2c_client *client)
> >  	pm_runtime_set_autosuspend_delay(dev, 1000);
> >  	pm_runtime_use_autosuspend(dev);
> > 
> > +	/* Initialize the V4L2 subdev. */
> > +	ret = imx290_subdev_init(imx290);
> > +	if (ret)
> > +		goto err_pm;
> > +
> >  	/*
> >  	 * Finally, register the V4L2 subdev. This must be done after
> >  	 * initializing everything as the subdev can be used immediately after
> > @@ -1323,12 +1332,12 @@ static int imx290_probe(struct i2c_client *client)
> > 
> >  	return 0;
> > 
> > +err_subdev:
> > +	imx290_subdev_cleanup(imx290);
> >  err_pm:
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_put_noidle(dev);
> > -	imx290_power_off(dev);
> > -err_subdev:
> > -	imx290_subdev_cleanup(imx290);
> > +	imx290_power_off(imx290);
> >  	return ret;
> >  }
> > 
> > @@ -1346,7 +1355,7 @@ static void imx290_remove(struct i2c_client *client)
> >  	 */
> >  	pm_runtime_disable(imx290->dev);
> >  	if (!pm_runtime_status_suspended(imx290->dev))
> > -		imx290_power_off(imx290->dev);
> > +		imx290_power_off(imx290);
> >  	pm_runtime_set_suspended(imx290->dev);
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 14/17] media: i2c: imx290: Initialize runtime PM before subdev

[Alexander Stein]

Hi Laurent,

Am Montag, 16. Januar 2023, 12:49:22 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Mon, Jan 16, 2023 at 12:14:46PM +0100, Alexander Stein wrote:
> > Am Samstag, 14. Januar 2023, 18:17:59 CET schrieb Laurent Pinchart:
> > > Initializing the subdev before runtime PM means that no subdev
> > > initialization can interact with the runtime PM framework. This can be
> > > problematic when modifying controls, as the .s_ctrl() handler commonly
> > > calls pm_runtime_get_if_in_use(). These code paths are not trivial,
> > > making the driver fragile and possibly causing subtle bugs.
> > > 
> > > To make the subdev initialization more robust, initialize runtime PM
> > > first.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you for giving me my own ack, but yours would be more useful :-)

oh... Sorry. I messed up my copy and paste :(

Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> > > ---
> > > 
> > >  drivers/media/i2c/imx290.c | 57 ++++++++++++++++++++++----------------
> > >  1 file changed, 33 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 6a3e93c10fb1..c3ffb23104bf 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -581,9 +581,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > > 
> > >  	/*
> > >  	
> > >  	 * Return immediately for controls that don't need to be applied to
> > >  	 the
> > > 
> > > -	 * device. This includes all controls modified in
> > > imx290_ctrl_update(),
> > > -	 * which is called at probe time before runtime PM is initialized, 
so
> > > -	 * we can't proceed to the pm_runtime_get_if_in_use() call below.
> > > +	 * device.
> > > 
> > >  	 */
> > >  	
> > >  	switch (ctrl->id) {
> > > 
> > >  	case V4L2_CID_LINK_FREQ:
> > > @@ -1054,22 +1052,20 @@ static void imx290_subdev_cleanup(struct imx290
> > > *imx290)> > 
> > >   * Power management
> > >   */
> > > 
> > > -static int imx290_power_on(struct device *dev)
> > > +static int imx290_power_on(struct imx290 *imx290)
> > > 
> > >  {
> > > 
> > > -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > -	struct imx290 *imx290 = to_imx290(sd);
> > > 
> > >  	int ret;
> > >  	
> > >  	ret = clk_prepare_enable(imx290->xclk);
> > >  	if (ret) {
> > > 
> > > -		dev_err(dev, "Failed to enable clock\n");
> > > +		dev_err(imx290->dev, "Failed to enable clock\n");
> > > 
> > >  		return ret;
> > >  	
> > >  	}
> > >  	
> > >  	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
> > >  	
> > >  				    imx290->supplies);
> > >  	
> > >  	if (ret) {
> > > 
> > > -		dev_err(dev, "Failed to enable regulators\n");
> > > +		dev_err(imx290->dev, "Failed to enable regulators\n");
> > > 
> > >  		clk_disable_unprepare(imx290->xclk);
> > >  		return ret;
> > >  	
> > >  	}
> > > 
> > > @@ -1084,20 +1080,33 @@ static int imx290_power_on(struct device *dev)
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > -static int imx290_power_off(struct device *dev)
> > > +static void imx290_power_off(struct imx290 *imx290)
> > > 
> > >  {
> > > 
> > > -	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > -	struct imx290 *imx290 = to_imx290(sd);
> > > -
> > > 
> > >  	clk_disable_unprepare(imx290->xclk);
> > >  	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
> > >  	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies),
> > >  	imx290->supplies);
> > > 
> > > +}
> > > +
> > > +static int imx290_runtime_resume(struct device *dev)
> > > +{
> > > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > +	struct imx290 *imx290 = to_imx290(sd);
> > > +
> > > +	return imx290_power_on(imx290);
> > > +}
> > > +
> > > +static int imx290_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > +	struct imx290 *imx290 = to_imx290(sd);
> > > +
> > > +	imx290_power_off(imx290);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > >  
> > >  static const struct dev_pm_ops imx290_pm_ops = {
> > > 
> > > -	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
> > > +	SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume,
> > > NULL)
> > > 
> > >  };
> > >  
> > >  /*
> > >  ----------------------------------------------------------------------
> > >  ------> > 
> > > @@ -1276,20 +1285,15 @@ static int imx290_probe(struct i2c_client
> > > *client)
> > > if (ret)
> > > 
> > >  		return ret;
> > > 
> > > -	/* Initialize the V4L2 subdev. */
> > > -	ret = imx290_subdev_init(imx290);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > 
> > >  	/*
> > >  	
> > >  	 * Enable power management. The driver supports runtime PM, but 
needs
> > >  	 to
> > >  	 * work when runtime PM is disabled in the kernel. To that end, 
power
> > >  	 * the sensor on manually here.
> > >  	 */
> > > 
> > > -	ret = imx290_power_on(dev);
> > > +	ret = imx290_power_on(imx290);
> > > 
> > >  	if (ret < 0) {
> > >  	
> > >  		dev_err(dev, "Could not power on the device\n");
> > > 
> > > -		goto err_subdev;
> > > +		return ret;
> > > 
> > >  	}
> > >  	
> > >  	/*
> > > 
> > > @@ -1303,6 +1307,11 @@ static int imx290_probe(struct i2c_client
> > > *client)
> > > 
> > >  	pm_runtime_set_autosuspend_delay(dev, 1000);
> > >  	pm_runtime_use_autosuspend(dev);
> > > 
> > > +	/* Initialize the V4L2 subdev. */
> > > +	ret = imx290_subdev_init(imx290);
> > > +	if (ret)
> > > +		goto err_pm;
> > > +
> > > 
> > >  	/*
> > >  	
> > >  	 * Finally, register the V4L2 subdev. This must be done after
> > >  	 * initializing everything as the subdev can be used immediately 
after
> > > 
> > > @@ -1323,12 +1332,12 @@ static int imx290_probe(struct i2c_client
> > > *client)
> > > 
> > >  	return 0;
> > > 
> > > +err_subdev:
> > > +	imx290_subdev_cleanup(imx290);
> > > 
> > >  err_pm:
> > >  	pm_runtime_disable(dev);
> > >  	pm_runtime_put_noidle(dev);
> > > 
> > > -	imx290_power_off(dev);
> > > -err_subdev:
> > > -	imx290_subdev_cleanup(imx290);
> > > +	imx290_power_off(imx290);
> > > 
> > >  	return ret;
> > >  
> > >  }
> > > 
> > > @@ -1346,7 +1355,7 @@ static void imx290_remove(struct i2c_client
> > > *client)
> > > 
> > >  	 */
> > >  	
> > >  	pm_runtime_disable(imx290->dev);
> > >  	if (!pm_runtime_status_suspended(imx290->dev))
> > > 
> > > -		imx290_power_off(imx290->dev);
> > > +		imx290_power_off(imx290);
> > > 
> > >  	pm_runtime_set_suspended(imx290->dev);
> > >  
> > >  }

[PATCH v2 15/17] media: i2c: imx290: Configure data lanes at start time

[Laurent Pinchart]

There's no need to configure the data lanes in the runtime PM resume
handler. Do so in imx290_start_streaming() instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index c3ffb23104bf..67a2edd38e91 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -767,6 +767,9 @@ static int imx290_start_streaming(struct imx290 *imx290,
 		return ret;
 	}
 
+	/* Set data lane count */
+	imx290_set_data_lanes(imx290);
+
 	/* Apply the register values related to current frame format */
 	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
 	ret = imx290_setup_format(imx290, format);
@@ -1074,9 +1077,6 @@ static int imx290_power_on(struct imx290 *imx290)
 	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
 	usleep_range(30000, 31000);
 
-	/* Set data lane count */
-	imx290_set_data_lanes(imx290);
-
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes()

[Laurent Pinchart]

There's no need to check for an incorrect number of data lanes in
imx290_set_data_lanes() as the value is validated at probe() time. Drop
the check.

The PHY_LANE_NUM and CSI_LANE_MODE registers are programmed with a value
equal to the number of lanes minus one. Compute it instead of handling
it in the switch/case.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 67a2edd38e91..a8167119534b 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -512,28 +512,21 @@ static int imx290_set_register_array(struct imx290 *imx290,
 
 static int imx290_set_data_lanes(struct imx290 *imx290)
 {
-	int ret = 0, laneval, frsel;
+	int ret = 0;
+	u32 frsel;
 
 	switch (imx290->nlanes) {
 	case 2:
-		laneval = 0x01;
+	default:
 		frsel = 0x02;
 		break;
 	case 4:
-		laneval = 0x03;
 		frsel = 0x01;
 		break;
-	default:
-		/*
-		 * We should never hit this since the data lane count is
-		 * validated in probe itself
-		 */
-		dev_err(imx290->dev, "Lane configuration not supported\n");
-		return -EINVAL;
 	}
 
-	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
-	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+	imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
+	imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, &ret);
 	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
 
 	return ret;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes()

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:18:01 CET schrieb Laurent Pinchart:
> There's no need to check for an incorrect number of data lanes in
> imx290_set_data_lanes() as the value is validated at probe() time. Drop
> the check.
> 
> The PHY_LANE_NUM and CSI_LANE_MODE registers are programmed with a value
> equal to the number of lanes minus one. Compute it instead of handling
> it in the switch/case.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 67a2edd38e91..a8167119534b 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -512,28 +512,21 @@ static int imx290_set_register_array(struct imx290
> *imx290,
> 
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
> -	int ret = 0, laneval, frsel;
> +	int ret = 0;
> +	u32 frsel;
> 
>  	switch (imx290->nlanes) {
>  	case 2:
> -		laneval = 0x01;
> +	default:
>  		frsel = 0x02;
>  		break;
>  	case 4:
> -		laneval = 0x03;
>  		frsel = 0x01;
>  		break;
> -	default:
> -		/*
> -		 * We should never hit this since the data lane count is
> -		 * validated in probe itself
> -		 */
> -		dev_err(imx290->dev, "Lane configuration not 
supported\n");
> -		return -EINVAL;
>  	}
> 
> -	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
> -	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
> +	imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
> +	imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, 
&ret);
>  	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
> 
>  	return ret;

[PATCH v2 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes()

[Laurent Pinchart]

Check the error status returned by imx290_set_data_lanes() in its
caller and propagate it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- New patch
---
 drivers/media/i2c/imx290.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a8167119534b..62d8d75aec3e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -761,7 +761,11 @@ static int imx290_start_streaming(struct imx290 *imx290,
 	}
 
 	/* Set data lane count */
-	imx290_set_data_lanes(imx290);
+	ret = imx290_set_data_lanes(imx290);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set data lanes\n");
+		return ret;
+	}
 
 	/* Apply the register values related to current frame format */
 	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes()

[Alexander Stein]

Hi Laurent,

thanks for the update.

Am Samstag, 14. Januar 2023, 18:18:02 CET schrieb Laurent Pinchart:
> Check the error status returned by imx290_set_data_lanes() in its
> caller and propagate it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
> Changes since v1:
> 
> - New patch
> ---
>  drivers/media/i2c/imx290.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a8167119534b..62d8d75aec3e 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -761,7 +761,11 @@ static int imx290_start_streaming(struct imx290
> *imx290, }
> 
>  	/* Set data lane count */
> -	imx290_set_data_lanes(imx290);
> +	ret = imx290_set_data_lanes(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set data lanes\n");
> +		return ret;
> +	}
> 
>  	/* Apply the register values related to current frame format */
>  	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);