linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: linux-media@vger.kernel.org
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>
Subject: [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev
Date: Mon, 16 Jan 2023 16:44:51 +0200	[thread overview]
Message-ID: <20230116144454.1012-15-laurent.pinchart@ideasonboard.com> (raw)
In-Reply-To: <20230116144454.1012-1-laurent.pinchart@ideasonboard.com>

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: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:

    - Use the correct error label when subdev registration fails
---
 drivers/media/i2c/imx290.c | 59 ++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 324d30ed5617..4185835f065d 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. Those controls are 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.
 	 */
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return 0;
@@ -1049,22 +1047,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;
 	}
@@ -1079,20 +1075,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)
 };
 
 /* ----------------------------------------------------------------------------
@@ -1271,20 +1280,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;
 	}
 
 	/*
@@ -1298,6 +1302,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
@@ -1306,7 +1315,7 @@ static int imx290_probe(struct i2c_client *client)
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
 		dev_err(dev, "Could not register v4l2 device\n");
-		goto err_pm;
+		goto err_subdev;
 	}
 
 	/*
@@ -1318,12 +1327,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;
 }
 
@@ -1341,7 +1350,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


  parent reply	other threads:[~2023-01-16 14:56 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-16 14:44 [PATCH v3 00/17] media: i2c: imx290: Miscellaneous improvements Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 01/17] media: i2c: imx290: Group functions in sections Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 02/17] media: i2c: imx290: Factor out subdev init and cleanup to functions Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 03/17] media: i2c: imx290: Factor out control update code to a function Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 04/17] media: i2c: imx290: Access link_freq_index directly Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 05/17] media: i2c: imx290: Pass format and mode to imx290_calc_pixel_rate() Laurent Pinchart
2023-01-16 15:00   ` Alexander Stein
2023-01-16 14:44 ` [PATCH v3 06/17] media: i2c: imx290: Compute pixel rate and blanking in one place Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 07/17] media: i2c: imx290: Factor out black level setting to a function Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 08/17] media: i2c: imx290: Factor out DT parsing to separate function Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 09/17] media: i2c: imx290: Use dev_err_probe() Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 10/17] media: i2c: imx290: Factor out clock initialization to separate function Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state Laurent Pinchart
2023-01-16 15:05   ` Alexander Stein
2023-01-16 14:44 ` [PATCH v3 12/17] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 13/17] media: i2c: imx290: Use runtime PM autosuspend Laurent Pinchart
2023-01-16 14:44 ` Laurent Pinchart [this message]
2023-02-27 17:52   ` [PATCH v3 14/17] media: i2c: imx290: Initialize runtime PM before subdev Guenter Roeck
2023-03-12 13:10     ` Linux regression tracking (Thorsten Leemhuis)
2023-03-12 13:34       ` Laurent Pinchart
2023-03-12 13:59         ` Linux regression tracking (Thorsten Leemhuis)
2023-01-16 14:44 ` [PATCH v3 15/17] media: i2c: imx290: Configure data lanes at start time Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 16/17] media: i2c: imx290: Simplify imx290_set_data_lanes() Laurent Pinchart
2023-01-16 14:44 ` [PATCH v3 17/17] media: i2c: imx290: Handle error from imx290_set_data_lanes() Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230116144454.1012-15-laurent.pinchart@ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=alexander.stein@ew.tq-group.com \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=linux-media@vger.kernel.org \
    --cc=mani@kernel.org \
    --cc=sakari.ailus@iki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).