[PATCH v6 0/3] IMX274 fixes and power on and off implementation

[PATCH v6 0/3] IMX274 fixes and power on and off implementation

[Sowjanya Komatineni]

This patch series includes
- Fix for proper Y_OUT_SIZE register configuration.
- Remove sensor i2c register writes to stop stream during remove.
- Power on/off sequence implementation through runtime PM.

Delta between patch versions:
[v6]:	Includes below v5 feedback
	- Added separate small patch to remove i2c writes to stop sensor
	  during remove.
	- Removed dt-binding patch from this series as they are posted by
	  Jacob.
	  https://www.spinics.net/lists/linux-renesas-soc/msg52831.html
	- Uses udelay for 2uS delay after regulators are on before
	  releasing reset.
	- Moved v4l2_ctrl_handler setup to do during start streaming to
	  configure user controls when sensor power is on during streaming.
	- Other minor changes based on v5 feedback.

[v5]:	Includes below v4 feedback
	- dt-bindings patch to add optional clock and supplies and
	  rebased on below json-schema patch.
	  https://patchwork.kernel.org/patch/11732875/
	- Other minor v4 feedbacks.

[v4]:	Includes below v3 feedback
	- Implemented power on/off through Runtime PM.
	- Use regulator bulk APIs.
	- Use lower case for supply names.

[v3]:	Includes below v2 feedback
	- Removed explicit clk_set_rate from driver as default external
	  input clock rate can be configured through DT.

[v2]:	Includes below changes based on v1 feedback
	- External input clock name changed from xclk to inck.
	- implementation change for get regulators to store all in array.
	- To keep in reset low prior to regulators power on.


Sowjanya Komatineni (3):
  media: i2c: imx274: Fix Y_OUT_SIZE register setting
  media: i2c: imx274: Remove stop stream i2c writes during remove
  media: i2c: imx274: Add IMX274 power on and off sequence

 drivers/media/i2c/imx274.c | 187 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 134 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH v6 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting

[Sowjanya Komatineni]

As per Sony IMX274 Y_OUT_SIZE should be the height of effective
image output from the sensor which are the actual total lines
sent over MIPI CSI to receiver.

So, Y_OUT_SIZE should be same as crop height and this patch fixes it.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 6011cec..a4b9dfd 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1163,7 +1163,7 @@ static int imx274_apply_trimming(struct stimx274 *imx274)
 		(-imx274->crop.top / 2) : (imx274->crop.top / 2);
 	v_cut = (IMX274_MAX_HEIGHT - imx274->crop.height) / 2;
 	write_v_size = imx274->crop.height + 22;
-	y_out_size   = imx274->crop.height + 14;
+	y_out_size   = imx274->crop.height;
 
 	err = imx274_write_mbreg(imx274, IMX274_HMAX_REG_LSB, hmax, 2);
 	if (!err)
-- 
2.7.4

[PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Sowjanya Komatineni]

Sensor should already be in standby during remove and there is no
need to configure sensor registers for stream stop.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/media/i2c/imx274.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index a4b9dfd..5e515f0 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1968,9 +1968,6 @@ static int imx274_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct stimx274 *imx274 = to_imx274(sd);
 
-	/* stop stream */
-	imx274_write_table(imx274, imx274_stop);
-
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
 	media_entity_cleanup(&sd->entity);
-- 
2.7.4

[PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Sowjanya Komatineni]

IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
io 1.2V supply which are optional based on camera module design.

IMX274 also need external 24Mhz clock and is optional based on
camera module design.

This patch adds support for IMX274 power on and off to enable and
disable these supplies and external clock.

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
 1 file changed, 134 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 5e515f0..b3057a5 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -18,7 +18,9 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/videodev2.h>
@@ -131,6 +133,15 @@
 #define IMX274_TABLE_WAIT_MS			0
 #define IMX274_TABLE_END			1
 
+/* regulator supplies */
+static const char * const imx274_supply_names[] = {
+	"vddl",  /* IF (1.2V) supply */
+	"vdig",  /* Digital Core (1.8V) supply */
+	"vana",  /* Analog (2.8V) supply */
+};
+
+#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
+
 /*
  * imx274 I2C operation related structure
  */
@@ -501,6 +512,8 @@ struct imx274_ctrls {
  * @frame_rate: V4L2 frame rate structure
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
+ * @supplies: List of analog and digital supply regulators
+ * @inck: Pointer to sensor input clock
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
  */
@@ -514,6 +527,8 @@ struct stimx274 {
 	struct v4l2_fract frame_interval;
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
+	struct clk *inck;
 	struct mutex lock; /* mutex lock for operations */
 	const struct imx274_mode *mode;
 };
@@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
 {
 	int err = 0;
 
+	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
+	if (err) {
+		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
+		return err;
+	}
+
 	/*
 	 * Refer to "Standby Cancel Sequence when using CSI-2" in
 	 * imx274 datasheet, it should wait 10ms or more here.
@@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
 	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
 }
 
+static int imx274_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct stimx274 *imx274 = to_imx274(sd);
+	int ret;
+
+	/* keep sensor in reset before power on */
+	imx274_reset(imx274, 0);
+
+	ret = clk_prepare_enable(imx274->inck);
+	if (ret) {
+		dev_err(&imx274->client->dev,
+			"Failed to enable input clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
+	if (ret) {
+		dev_err(&imx274->client->dev,
+			"Failed to enable regulators: %d\n", ret);
+		goto fail_reg;
+	}
+
+	udelay(2);
+	imx274_reset(imx274, 1);
+
+	return 0;
+
+fail_reg:
+	clk_disable_unprepare(imx274->inck);
+	return ret;
+}
+
+static int imx274_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct stimx274 *imx274 = to_imx274(sd);
+
+	imx274_reset(imx274, 0);
+
+	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
+
+	clk_disable_unprepare(imx274->inck);
+
+	return 0;
+}
+
+static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
+{
+	unsigned int i;
+
+	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
+		imx274->supplies[i].supply = imx274_supply_names[i];
+
+	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
+					imx274->supplies);
+}
+
 /**
  * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
  * @ctrl: V4L2 control to be set
@@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct stimx274 *imx274 = to_imx274(sd);
 	int ret = -EINVAL;
 
+	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
+		return 0;
+
 	dev_dbg(&imx274->client->dev,
 		"%s : s_ctrl: %s, value: %d\n", __func__,
 		ctrl->name, ctrl->val);
@@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
+	pm_runtime_put(&imx274->client->dev);
+
 	return ret;
 }
 
@@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_load_default(struct stimx274 *priv)
+static void imx274_load_default(struct stimx274 *priv)
 {
-	int ret;
-
 	/* load default control values */
 	priv->frame_interval.numerator = 1;
 	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
@@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv)
 	priv->ctrls.gain->val = IMX274_DEF_GAIN;
 	priv->ctrls.vflip->val = 0;
 	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
-
-	/* update frame rate */
-	ret = imx274_set_frame_interval(priv,
-					priv->frame_interval);
-	if (ret)
-		return ret;
-
-	/* update exposure time */
-	ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val);
-	if (ret)
-		return ret;
-
-	/* update gain */
-	ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val);
-	if (ret)
-		return ret;
-
-	/* update vflip */
-	ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val);
-	if (ret)
-		return ret;
-
-	return 0;
 }
 
 /**
@@ -1327,6 +1388,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	mutex_lock(&imx274->lock);
 
 	if (on) {
+		ret = pm_runtime_get_sync(&imx274->client->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(&imx274->client->dev);
+			mutex_unlock(&imx274->lock);
+			return ret;
+		}
+
 		/* load mode registers */
 		ret = imx274_mode_regs(imx274);
 		if (ret)
@@ -1347,12 +1415,6 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 		if (ret)
 			goto fail;
 
-		/* update exposure time */
-		ret = __v4l2_ctrl_s_ctrl(imx274->ctrls.exposure,
-					 imx274->ctrls.exposure->val);
-		if (ret)
-			goto fail;
-
 		/* start stream */
 		ret = imx274_start_stream(imx274);
 		if (ret)
@@ -1362,6 +1424,8 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 		ret = imx274_write_table(imx274, imx274_stop);
 		if (ret)
 			goto fail;
+
+		pm_runtime_put(&imx274->client->dev);
 	}
 
 	mutex_unlock(&imx274->lock);
@@ -1369,6 +1433,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	return 0;
 
 fail:
+	pm_runtime_put(&imx274->client->dev);
 	mutex_unlock(&imx274->lock);
 	dev_err(&imx274->client->dev, "s_stream failed\n");
 	return ret;
@@ -1834,6 +1899,17 @@ static int imx274_probe(struct i2c_client *client)
 
 	mutex_init(&imx274->lock);
 
+	imx274->inck = devm_clk_get_optional(&client->dev, "inck");
+	if (IS_ERR(imx274->inck))
+		return PTR_ERR(imx274->inck);
+
+	ret = imx274_regulators_get(&client->dev, imx274);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to get power regulators, err: %d\n", ret);
+		return ret;
+	}
+
 	/* initialize format */
 	imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING];
 	imx274->crop.width = IMX274_MAX_WIDTH;
@@ -1881,15 +1957,20 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_me;
 	}
 
-	/* pull sensor out of reset */
-	imx274_reset(imx274, 1);
+	/* power on the sensor */
+	ret = imx274_power_on(&client->dev);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"%s : imx274 power on failed\n", __func__);
+		goto err_me;
+	}
 
 	/* initialize controls */
 	ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4);
 	if (ret < 0) {
 		dev_err(&client->dev,
 			"%s : ctrl handler init Failed\n", __func__);
-		goto err_me;
+		goto err_power_off;
 	}
 
 	imx274->ctrls.handler.lock = &imx274->lock;
@@ -1925,22 +2006,8 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_ctrls;
 	}
 
-	/* setup default controls */
-	ret = v4l2_ctrl_handler_setup(&imx274->ctrls.handler);
-	if (ret) {
-		dev_err(&client->dev,
-			"Error %d setup default controls\n", ret);
-		goto err_ctrls;
-	}
-
 	/* load default control values */
-	ret = imx274_load_default(imx274);
-	if (ret) {
-		dev_err(&client->dev,
-			"%s : imx274_load_default failed %d\n",
-			__func__, ret);
-		goto err_ctrls;
-	}
+	imx274_load_default(imx274);
 
 	/* register subdevice */
 	ret = v4l2_async_register_subdev(sd);
@@ -1951,11 +2018,17 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_ctrls;
 	}
 
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
+
 	dev_info(&client->dev, "imx274 : imx274 probe success !\n");
 	return 0;
 
 err_ctrls:
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
+err_power_off:
+	imx274_power_off(&client->dev);
 err_me:
 	media_entity_cleanup(&sd->entity);
 err_regmap:
@@ -1968,16 +2041,27 @@ static int imx274_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct stimx274 *imx274 = to_imx274(sd);
 
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		imx274_power_off(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
+
 	media_entity_cleanup(&sd->entity);
 	mutex_destroy(&imx274->lock);
 	return 0;
 }
 
+static const struct dev_pm_ops imx274_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, NULL)
+};
+
 static struct i2c_driver imx274_i2c_driver = {
 	.driver = {
 		.name	= DRIVER_NAME,
+		.pm = &imx274_pm_ops,
 		.of_match_table	= imx274_of_id_table,
 	},
 	.probe_new	= imx274_probe,
-- 
2.7.4

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7464 bytes --]

On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
> io 1.2V supply which are optional based on camera module design.
> 
> IMX274 also need external 24Mhz clock and is optional based on
> camera module design.
> 
> This patch adds support for IMX274 power on and off to enable and
> disable these supplies and external clock.
> 
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 134 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 5e515f0..b3057a5 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -18,7 +18,9 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_gpio.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/videodev2.h>
> @@ -131,6 +133,15 @@
>  #define IMX274_TABLE_WAIT_MS			0
>  #define IMX274_TABLE_END			1
>  
> +/* regulator supplies */
> +static const char * const imx274_supply_names[] = {
> +	"vddl",  /* IF (1.2V) supply */
> +	"vdig",  /* Digital Core (1.8V) supply */
> +	"vana",  /* Analog (2.8V) supply */

According to the device tree bindings these should be uppercase. Did I
miss a patch that updates the bindings?

I think the preference is for supply names to be lowercase and given
that there are no users of this binding yet we could update it without
breaking any existing device trees.

> +};
> +
> +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> +
>  /*
>   * imx274 I2C operation related structure
>   */
> @@ -501,6 +512,8 @@ struct imx274_ctrls {
>   * @frame_rate: V4L2 frame rate structure
>   * @regmap: Pointer to regmap structure
>   * @reset_gpio: Pointer to reset gpio
> + * @supplies: List of analog and digital supply regulators
> + * @inck: Pointer to sensor input clock
>   * @lock: Mutex structure
>   * @mode: Parameters for the selected readout mode
>   */
> @@ -514,6 +527,8 @@ struct stimx274 {
>  	struct v4l2_fract frame_interval;
>  	struct regmap *regmap;
>  	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
> +	struct clk *inck;
>  	struct mutex lock; /* mutex lock for operations */
>  	const struct imx274_mode *mode;
>  };
> @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
>  {
>  	int err = 0;
>  
> +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
> +	if (err) {
> +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
> +		return err;
> +	}
> +
>  	/*
>  	 * Refer to "Standby Cancel Sequence when using CSI-2" in
>  	 * imx274 datasheet, it should wait 10ms or more here.
> @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
>  	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
>  }
>  
> +static int imx274_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct stimx274 *imx274 = to_imx274(sd);
> +	int ret;
> +
> +	/* keep sensor in reset before power on */
> +	imx274_reset(imx274, 0);
> +
> +	ret = clk_prepare_enable(imx274->inck);
> +	if (ret) {
> +		dev_err(&imx274->client->dev,
> +			"Failed to enable input clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
> +	if (ret) {
> +		dev_err(&imx274->client->dev,
> +			"Failed to enable regulators: %d\n", ret);
> +		goto fail_reg;
> +	}
> +
> +	udelay(2);

This looks like some sort of extra delay to make sure all the supply
voltages have settled. Should this perhaps be encoded as part of the
regulator ramp-up times? Or is this really an IC-specific delay that
is needed for some internal timing?

> +	imx274_reset(imx274, 1);
> +
> +	return 0;
> +
> +fail_reg:
> +	clk_disable_unprepare(imx274->inck);
> +	return ret;
> +}
> +
> +static int imx274_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct stimx274 *imx274 = to_imx274(sd);
> +
> +	imx274_reset(imx274, 0);
> +
> +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> +
> +	clk_disable_unprepare(imx274->inck);
> +
> +	return 0;
> +}
> +
> +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
> +		imx274->supplies[i].supply = imx274_supply_names[i];
> +
> +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
> +					imx274->supplies);
> +}
> +
>  /**
>   * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
>   * @ctrl: V4L2 control to be set
> @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>  	struct stimx274 *imx274 = to_imx274(sd);
>  	int ret = -EINVAL;
>  
> +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
> +		return 0;

I'm not sure I understand this, and sorry if this has been discussed
earlier. Aren't there any other mechanisms in place to ensure that a
control can only be configured when in use? If so, then is this even
necessary?

If not, silently ignoring at this point seems like it could cause subtle
failures by ignoring some control settings and applying others if the
timing is right.

> +
>  	dev_dbg(&imx274->client->dev,
>  		"%s : s_ctrl: %s, value: %d\n", __func__,
>  		ctrl->name, ctrl->val);
> @@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put(&imx274->client->dev);
> +
>  	return ret;
>  }
>  
> @@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>   *
>   * Return: 0 on success, errors otherwise
>   */
> -static int imx274_load_default(struct stimx274 *priv)
> +static void imx274_load_default(struct stimx274 *priv)
>  {
> -	int ret;
> -
>  	/* load default control values */
>  	priv->frame_interval.numerator = 1;
>  	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
> @@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv)
>  	priv->ctrls.gain->val = IMX274_DEF_GAIN;
>  	priv->ctrls.vflip->val = 0;
>  	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
> -
> -	/* update frame rate */
> -	ret = imx274_set_frame_interval(priv,
> -					priv->frame_interval);
> -	if (ret)
> -		return ret;
> -
> -	/* update exposure time */
> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val);
> -	if (ret)
> -		return ret;
> -
> -	/* update gain */
> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val);
> -	if (ret)
> -		return ret;
> -
> -	/* update vflip */
> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val);
> -	if (ret)
> -		return ret;

This is not moved to somewhere else, so I assume the equivalent will
happen somewhere higher up in the stack? Might be worth mentioning in
the commit message why this can be dropped.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Mon, Sep 21, 2020 at 02:39:37PM -0700, Sowjanya Komatineni wrote:
> As per Sony IMX274 Y_OUT_SIZE should be the height of effective
> image output from the sensor which are the actual total lines
> sent over MIPI CSI to receiver.
> 
> So, Y_OUT_SIZE should be same as crop height and this patch fixes it.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/media/i2c/imx274.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

On Mon, Sep 21, 2020 at 02:39:38PM -0700, Sowjanya Komatineni wrote:
> Sensor should already be in standby during remove and there is no
> need to configure sensor registers for stream stop.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/media/i2c/imx274.c | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Luca Ceresoli]

Hi,

On 21/09/20 23:39, Sowjanya Komatineni wrote:
> Sensor should already be in standby during remove and there is no
> need to configure sensor registers for stream stop.

I beg your pardon for the newbie question: does the V4L2 framework
guarantee that the stream is stopped (.s_stream(..., 0)) before removing
the driver?

Thanks,
-- 
Luca

Re: [PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Sakari Ailus]

Hi Luca,

On Tue, Sep 22, 2020 at 10:09:33AM +0200, Luca Ceresoli wrote:
> Hi,
> 
> On 21/09/20 23:39, Sowjanya Komatineni wrote:
> > Sensor should already be in standby during remove and there is no
> > need to configure sensor registers for stream stop.
> 
> I beg your pardon for the newbie question: does the V4L2 framework
> guarantee that the stream is stopped (.s_stream(..., 0)) before removing
> the driver?

It doesn't. That's however one of the lesser concerns, and I don't think
it'd help if drivers tried to prepare for that.

-- 
Sakari Ailus

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 7951 bytes --]

Hi Thierry

On Tue, Sep 22, 2020 at 09:55:01AM +0200, Thierry Reding wrote:
> On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> > IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
> > io 1.2V supply which are optional based on camera module design.
> >
> > IMX274 also need external 24Mhz clock and is optional based on
> > camera module design.
> >
> > This patch adds support for IMX274 power on and off to enable and
> > disable these supplies and external clock.
> >
> > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > ---
> >  drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
> >  1 file changed, 134 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > index 5e515f0..b3057a5 100644
> > --- a/drivers/media/i2c/imx274.c
> > +++ b/drivers/media/i2c/imx274.c
> > @@ -18,7 +18,9 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of_gpio.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/v4l2-mediabus.h>
> >  #include <linux/videodev2.h>
> > @@ -131,6 +133,15 @@
> >  #define IMX274_TABLE_WAIT_MS			0
> >  #define IMX274_TABLE_END			1
> >
> > +/* regulator supplies */
> > +static const char * const imx274_supply_names[] = {
> > +	"vddl",  /* IF (1.2V) supply */
> > +	"vdig",  /* Digital Core (1.8V) supply */
> > +	"vana",  /* Analog (2.8V) supply */
>
> According to the device tree bindings these should be uppercase. Did I
> miss a patch that updates the bindings?

Yes! Sorry, there's been some chrun around these bindings
https://patchwork.linuxtv.org/project/linux-media/patch/20200917144416.GN834@valkosipuli.retiisi.org.uk/

It should get in for 5.9 as a late fix

>
> I think the preference is for supply names to be lowercase and given
> that there are no users of this binding yet we could update it without
> breaking any existing device trees.
>
> > +};
> > +
> > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> > +
> >  /*
> >   * imx274 I2C operation related structure
> >   */
> > @@ -501,6 +512,8 @@ struct imx274_ctrls {
> >   * @frame_rate: V4L2 frame rate structure
> >   * @regmap: Pointer to regmap structure
> >   * @reset_gpio: Pointer to reset gpio
> > + * @supplies: List of analog and digital supply regulators
> > + * @inck: Pointer to sensor input clock
> >   * @lock: Mutex structure
> >   * @mode: Parameters for the selected readout mode
> >   */
> > @@ -514,6 +527,8 @@ struct stimx274 {
> >  	struct v4l2_fract frame_interval;
> >  	struct regmap *regmap;
> >  	struct gpio_desc *reset_gpio;
> > +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
> > +	struct clk *inck;
> >  	struct mutex lock; /* mutex lock for operations */
> >  	const struct imx274_mode *mode;
> >  };
> > @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
> >  {
> >  	int err = 0;
> >
> > +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
> > +	if (err) {
> > +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
> > +		return err;
> > +	}
> > +
> >  	/*
> >  	 * Refer to "Standby Cancel Sequence when using CSI-2" in
> >  	 * imx274 datasheet, it should wait 10ms or more here.
> > @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
> >  	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
> >  }
> >
> > +static int imx274_power_on(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct stimx274 *imx274 = to_imx274(sd);
> > +	int ret;
> > +
> > +	/* keep sensor in reset before power on */
> > +	imx274_reset(imx274, 0);
> > +
> > +	ret = clk_prepare_enable(imx274->inck);
> > +	if (ret) {
> > +		dev_err(&imx274->client->dev,
> > +			"Failed to enable input clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > +	if (ret) {
> > +		dev_err(&imx274->client->dev,
> > +			"Failed to enable regulators: %d\n", ret);
> > +		goto fail_reg;
> > +	}
> > +
> > +	udelay(2);
>
> This looks like some sort of extra delay to make sure all the supply
> voltages have settled. Should this perhaps be encoded as part of the
> regulator ramp-up times? Or is this really an IC-specific delay that
> is needed for some internal timing?
>
> > +	imx274_reset(imx274, 1);
> > +
> > +	return 0;
> > +
> > +fail_reg:
> > +	clk_disable_unprepare(imx274->inck);
> > +	return ret;
> > +}
> > +
> > +static int imx274_power_off(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct stimx274 *imx274 = to_imx274(sd);
> > +
> > +	imx274_reset(imx274, 0);
> > +
> > +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > +
> > +	clk_disable_unprepare(imx274->inck);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
> > +		imx274->supplies[i].supply = imx274_supply_names[i];
> > +
> > +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
> > +					imx274->supplies);
> > +}
> > +
> >  /**
> >   * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
> >   * @ctrl: V4L2 control to be set
> > @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> >  	struct stimx274 *imx274 = to_imx274(sd);
> >  	int ret = -EINVAL;
> >
> > +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
> > +		return 0;
>
> I'm not sure I understand this, and sorry if this has been discussed
> earlier. Aren't there any other mechanisms in place to ensure that a
> control can only be configured when in use? If so, then is this even
> necessary?
>
> If not, silently ignoring at this point seems like it could cause subtle
> failures by ignoring some control settings and applying others if the
> timing is right.
>
> > +
> >  	dev_dbg(&imx274->client->dev,
> >  		"%s : s_ctrl: %s, value: %d\n", __func__,
> >  		ctrl->name, ctrl->val);
> > @@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >
> > +	pm_runtime_put(&imx274->client->dev);
> > +
> >  	return ret;
> >  }
> >
> > @@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
> >   *
> >   * Return: 0 on success, errors otherwise
> >   */
> > -static int imx274_load_default(struct stimx274 *priv)
> > +static void imx274_load_default(struct stimx274 *priv)
> >  {
> > -	int ret;
> > -
> >  	/* load default control values */
> >  	priv->frame_interval.numerator = 1;
> >  	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
> > @@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv)
> >  	priv->ctrls.gain->val = IMX274_DEF_GAIN;
> >  	priv->ctrls.vflip->val = 0;
> >  	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
> > -
> > -	/* update frame rate */
> > -	ret = imx274_set_frame_interval(priv,
> > -					priv->frame_interval);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/* update exposure time */
> > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/* update gain */
> > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/* update vflip */
> > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val);
> > -	if (ret)
> > -		return ret;
>
> This is not moved to somewhere else, so I assume the equivalent will
> happen somewhere higher up in the stack? Might be worth mentioning in
> the commit message why this can be dropped.
>
> Thierry



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Sowjanya Komatineni]

On 9/22/20 12:55 AM, Thierry Reding wrote:
> On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
>> IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
>> io 1.2V supply which are optional based on camera module design.
>>
>> IMX274 also need external 24Mhz clock and is optional based on
>> camera module design.
>>
>> This patch adds support for IMX274 power on and off to enable and
>> disable these supplies and external clock.
>>
>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
>>   1 file changed, 134 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 5e515f0..b3057a5 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -18,7 +18,9 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of_gpio.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/slab.h>
>>   #include <linux/v4l2-mediabus.h>
>>   #include <linux/videodev2.h>
>> @@ -131,6 +133,15 @@
>>   #define IMX274_TABLE_WAIT_MS			0
>>   #define IMX274_TABLE_END			1
>>   
>> +/* regulator supplies */
>> +static const char * const imx274_supply_names[] = {
>> +	"vddl",  /* IF (1.2V) supply */
>> +	"vdig",  /* Digital Core (1.8V) supply */
>> +	"vana",  /* Analog (2.8V) supply */
> According to the device tree bindings these should be uppercase. Did I
> miss a patch that updates the bindings?
>
> I think the preference is for supply names to be lowercase and given
> that there are no users of this binding yet we could update it without
> breaking any existing device trees.
>
>> +};
>> +
>> +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
>> +
>>   /*
>>    * imx274 I2C operation related structure
>>    */
>> @@ -501,6 +512,8 @@ struct imx274_ctrls {
>>    * @frame_rate: V4L2 frame rate structure
>>    * @regmap: Pointer to regmap structure
>>    * @reset_gpio: Pointer to reset gpio
>> + * @supplies: List of analog and digital supply regulators
>> + * @inck: Pointer to sensor input clock
>>    * @lock: Mutex structure
>>    * @mode: Parameters for the selected readout mode
>>    */
>> @@ -514,6 +527,8 @@ struct stimx274 {
>>   	struct v4l2_fract frame_interval;
>>   	struct regmap *regmap;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
>> +	struct clk *inck;
>>   	struct mutex lock; /* mutex lock for operations */
>>   	const struct imx274_mode *mode;
>>   };
>> @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
>>   {
>>   	int err = 0;
>>   
>> +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
>> +	if (err) {
>> +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
>> +		return err;
>> +	}
>> +
>>   	/*
>>   	 * Refer to "Standby Cancel Sequence when using CSI-2" in
>>   	 * imx274 datasheet, it should wait 10ms or more here.
>> @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
>>   	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
>>   }
>>   
>> +static int imx274_power_on(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct stimx274 *imx274 = to_imx274(sd);
>> +	int ret;
>> +
>> +	/* keep sensor in reset before power on */
>> +	imx274_reset(imx274, 0);
>> +
>> +	ret = clk_prepare_enable(imx274->inck);
>> +	if (ret) {
>> +		dev_err(&imx274->client->dev,
>> +			"Failed to enable input clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
>> +	if (ret) {
>> +		dev_err(&imx274->client->dev,
>> +			"Failed to enable regulators: %d\n", ret);
>> +		goto fail_reg;
>> +	}
>> +
>> +	udelay(2);
> This looks like some sort of extra delay to make sure all the supply
> voltages have settled. Should this perhaps be encoded as part of the
> regulator ramp-up times? Or is this really an IC-specific delay that
> is needed for some internal timing?
This is IC-specific delay after power on regulators before releasing reset.
>
>> +	imx274_reset(imx274, 1);
>> +
>> +	return 0;
>> +
>> +fail_reg:
>> +	clk_disable_unprepare(imx274->inck);
>> +	return ret;
>> +}
>> +
>> +static int imx274_power_off(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct stimx274 *imx274 = to_imx274(sd);
>> +
>> +	imx274_reset(imx274, 0);
>> +
>> +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
>> +
>> +	clk_disable_unprepare(imx274->inck);
>> +
>> +	return 0;
>> +}
>> +
>> +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
>> +		imx274->supplies[i].supply = imx274_supply_names[i];
>> +
>> +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
>> +					imx274->supplies);
>> +}
>> +
>>   /**
>>    * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
>>    * @ctrl: V4L2 control to be set
>> @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>>   	struct stimx274 *imx274 = to_imx274(sd);
>>   	int ret = -EINVAL;
>>   
>> +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
>> +		return 0;
> I'm not sure I understand this, and sorry if this has been discussed
> earlier. Aren't there any other mechanisms in place to ensure that a
> control can only be configured when in use? If so, then is this even
> necessary?
>
> If not, silently ignoring at this point seems like it could cause subtle
> failures by ignoring some control settings and applying others if the
> timing is right.

With this patch, v4l2_ctrl setup is moved to start stream so all the 
control values selected gets programmed during stream start. So s_ctrl 
callback execution happens during that time after sensor rpm resume and 
I don't think we need here either but I see all sensor drivers with RPM 
enabled checking for this. So added just to make sure sensor programming 
don't happen when power is off.

Sakari/Jacob,

Can you please clarify if we can remove check pm_runtime_get_if_in_use() 
in s_ctrl callback as v4l2_ctrl handler setup happens during stream 
start where power is already on by then?

>> +
>>   	dev_dbg(&imx274->client->dev,
>>   		"%s : s_ctrl: %s, value: %d\n", __func__,
>>   		ctrl->name, ctrl->val);
>> @@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>>   		break;
>>   	}
>>   
>> +	pm_runtime_put(&imx274->client->dev);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>    *
>>    * Return: 0 on success, errors otherwise
>>    */
>> -static int imx274_load_default(struct stimx274 *priv)
>> +static void imx274_load_default(struct stimx274 *priv)
>>   {
>> -	int ret;
>> -
>>   	/* load default control values */
>>   	priv->frame_interval.numerator = 1;
>>   	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
>> @@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv)
>>   	priv->ctrls.gain->val = IMX274_DEF_GAIN;
>>   	priv->ctrls.vflip->val = 0;
>>   	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
>> -
>> -	/* update frame rate */
>> -	ret = imx274_set_frame_interval(priv,
>> -					priv->frame_interval);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* update exposure time */
>> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* update gain */
>> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* update vflip */
>> -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val);
>> -	if (ret)
>> -		return ret;
> This is not moved to somewhere else, so I assume the equivalent will
> happen somewhere higher up in the stack? Might be worth mentioning in
> the commit message why this can be dropped.
OK. Will add in commit message.
>
> Thierry

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Sakari Ailus]

Hi Sowjanya,

On Tue, Sep 22, 2020 at 09:13:57AM -0700, Sowjanya Komatineni wrote:
> 
> On 9/22/20 12:55 AM, Thierry Reding wrote:
> > On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> > > IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
> > > io 1.2V supply which are optional based on camera module design.
> > > 
> > > IMX274 also need external 24Mhz clock and is optional based on
> > > camera module design.
> > > 
> > > This patch adds support for IMX274 power on and off to enable and
> > > disable these supplies and external clock.
> > > 
> > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > ---
> > >   drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
> > >   1 file changed, 134 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > index 5e515f0..b3057a5 100644
> > > --- a/drivers/media/i2c/imx274.c
> > > +++ b/drivers/media/i2c/imx274.c
> > > @@ -18,7 +18,9 @@
> > >   #include <linux/kernel.h>
> > >   #include <linux/module.h>
> > >   #include <linux/of_gpio.h>
> > > +#include <linux/pm_runtime.h>
> > >   #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/v4l2-mediabus.h>
> > >   #include <linux/videodev2.h>
> > > @@ -131,6 +133,15 @@
> > >   #define IMX274_TABLE_WAIT_MS			0
> > >   #define IMX274_TABLE_END			1
> > > +/* regulator supplies */
> > > +static const char * const imx274_supply_names[] = {
> > > +	"vddl",  /* IF (1.2V) supply */
> > > +	"vdig",  /* Digital Core (1.8V) supply */
> > > +	"vana",  /* Analog (2.8V) supply */
> > According to the device tree bindings these should be uppercase. Did I
> > miss a patch that updates the bindings?
> > 
> > I think the preference is for supply names to be lowercase and given
> > that there are no users of this binding yet we could update it without
> > breaking any existing device trees.
> > 
> > > +};
> > > +
> > > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> > > +
> > >   /*
> > >    * imx274 I2C operation related structure
> > >    */
> > > @@ -501,6 +512,8 @@ struct imx274_ctrls {
> > >    * @frame_rate: V4L2 frame rate structure
> > >    * @regmap: Pointer to regmap structure
> > >    * @reset_gpio: Pointer to reset gpio
> > > + * @supplies: List of analog and digital supply regulators
> > > + * @inck: Pointer to sensor input clock
> > >    * @lock: Mutex structure
> > >    * @mode: Parameters for the selected readout mode
> > >    */
> > > @@ -514,6 +527,8 @@ struct stimx274 {
> > >   	struct v4l2_fract frame_interval;
> > >   	struct regmap *regmap;
> > >   	struct gpio_desc *reset_gpio;
> > > +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
> > > +	struct clk *inck;
> > >   	struct mutex lock; /* mutex lock for operations */
> > >   	const struct imx274_mode *mode;
> > >   };
> > > @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
> > >   {
> > >   	int err = 0;
> > > +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
> > > +	if (err) {
> > > +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
> > > +		return err;
> > > +	}
> > > +
> > >   	/*
> > >   	 * Refer to "Standby Cancel Sequence when using CSI-2" in
> > >   	 * imx274 datasheet, it should wait 10ms or more here.
> > > @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
> > >   	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
> > >   }
> > > +static int imx274_power_on(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > +	int ret;
> > > +
> > > +	/* keep sensor in reset before power on */
> > > +	imx274_reset(imx274, 0);
> > > +
> > > +	ret = clk_prepare_enable(imx274->inck);
> > > +	if (ret) {
> > > +		dev_err(&imx274->client->dev,
> > > +			"Failed to enable input clock: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > +	if (ret) {
> > > +		dev_err(&imx274->client->dev,
> > > +			"Failed to enable regulators: %d\n", ret);
> > > +		goto fail_reg;
> > > +	}
> > > +
> > > +	udelay(2);
> > This looks like some sort of extra delay to make sure all the supply
> > voltages have settled. Should this perhaps be encoded as part of the
> > regulator ramp-up times? Or is this really an IC-specific delay that
> > is needed for some internal timing?
> This is IC-specific delay after power on regulators before releasing reset.
> > 
> > > +	imx274_reset(imx274, 1);
> > > +
> > > +	return 0;
> > > +
> > > +fail_reg:
> > > +	clk_disable_unprepare(imx274->inck);
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx274_power_off(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > +
> > > +	imx274_reset(imx274, 0);
> > > +
> > > +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > +
> > > +	clk_disable_unprepare(imx274->inck);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
> > > +		imx274->supplies[i].supply = imx274_supply_names[i];
> > > +
> > > +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
> > > +					imx274->supplies);
> > > +}
> > > +
> > >   /**
> > >    * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
> > >    * @ctrl: V4L2 control to be set
> > > @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> > >   	struct stimx274 *imx274 = to_imx274(sd);
> > >   	int ret = -EINVAL;
> > > +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
> > > +		return 0;
> > I'm not sure I understand this, and sorry if this has been discussed
> > earlier. Aren't there any other mechanisms in place to ensure that a
> > control can only be configured when in use? If so, then is this even
> > necessary?
> > 
> > If not, silently ignoring at this point seems like it could cause subtle
> > failures by ignoring some control settings and applying others if the
> > timing is right.
> 
> With this patch, v4l2_ctrl setup is moved to start stream so all the control
> values selected gets programmed during stream start. So s_ctrl callback
> execution happens during that time after sensor rpm resume and I don't think
> we need here either but I see all sensor drivers with RPM enabled checking
> for this. So added just to make sure sensor programming don't happen when
> power is off.
> 
> Sakari/Jacob,
> 
> Can you please clarify if we can remove check pm_runtime_get_if_in_use() in
> s_ctrl callback as v4l2_ctrl handler setup happens during stream start where
> power is already on by then?

The controls are accessible also when streaming is disabled. So you may end
up here without the device being powered on. Therefore the check is needed.

> 
> > > +
> > >   	dev_dbg(&imx274->client->dev,
> > >   		"%s : s_ctrl: %s, value: %d\n", __func__,
> > >   		ctrl->name, ctrl->val);
> > > @@ -811,6 +895,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> > >   		break;
> > >   	}
> > > +	pm_runtime_put(&imx274->client->dev);
> > > +
> > >   	return ret;
> > >   }
> > > @@ -1269,10 +1355,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
> > >    *
> > >    * Return: 0 on success, errors otherwise
> > >    */
> > > -static int imx274_load_default(struct stimx274 *priv)
> > > +static void imx274_load_default(struct stimx274 *priv)
> > >   {
> > > -	int ret;
> > > -
> > >   	/* load default control values */
> > >   	priv->frame_interval.numerator = 1;
> > >   	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
> > > @@ -1280,29 +1364,6 @@ static int imx274_load_default(struct stimx274 *priv)
> > >   	priv->ctrls.gain->val = IMX274_DEF_GAIN;
> > >   	priv->ctrls.vflip->val = 0;
> > >   	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
> > > -
> > > -	/* update frame rate */
> > > -	ret = imx274_set_frame_interval(priv,
> > > -					priv->frame_interval);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	/* update exposure time */
> > > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.exposure, priv->ctrls.exposure->val);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	/* update gain */
> > > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.gain, priv->ctrls.gain->val);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	/* update vflip */
> > > -	ret = v4l2_ctrl_s_ctrl(priv->ctrls.vflip, priv->ctrls.vflip->val);
> > > -	if (ret)
> > > -		return ret;
> > This is not moved to somewhere else, so I assume the equivalent will
> > happen somewhere higher up in the stack? Might be worth mentioning in
> > the commit message why this can be dropped.
> OK. Will add in commit message.
> > 
> > Thierry

-- 
Sakari Ailus

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8307 bytes --]

On Tue, Sep 22, 2020 at 07:20:25PM +0300, Sakari Ailus wrote:
> Hi Sowjanya,
> 
> On Tue, Sep 22, 2020 at 09:13:57AM -0700, Sowjanya Komatineni wrote:
> > 
> > On 9/22/20 12:55 AM, Thierry Reding wrote:
> > > On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> > > > IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
> > > > io 1.2V supply which are optional based on camera module design.
> > > > 
> > > > IMX274 also need external 24Mhz clock and is optional based on
> > > > camera module design.
> > > > 
> > > > This patch adds support for IMX274 power on and off to enable and
> > > > disable these supplies and external clock.
> > > > 
> > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > > ---
> > > >   drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
> > > >   1 file changed, 134 insertions(+), 50 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > > index 5e515f0..b3057a5 100644
> > > > --- a/drivers/media/i2c/imx274.c
> > > > +++ b/drivers/media/i2c/imx274.c
> > > > @@ -18,7 +18,9 @@
> > > >   #include <linux/kernel.h>
> > > >   #include <linux/module.h>
> > > >   #include <linux/of_gpio.h>
> > > > +#include <linux/pm_runtime.h>
> > > >   #include <linux/regmap.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >   #include <linux/slab.h>
> > > >   #include <linux/v4l2-mediabus.h>
> > > >   #include <linux/videodev2.h>
> > > > @@ -131,6 +133,15 @@
> > > >   #define IMX274_TABLE_WAIT_MS			0
> > > >   #define IMX274_TABLE_END			1
> > > > +/* regulator supplies */
> > > > +static const char * const imx274_supply_names[] = {
> > > > +	"vddl",  /* IF (1.2V) supply */
> > > > +	"vdig",  /* Digital Core (1.8V) supply */
> > > > +	"vana",  /* Analog (2.8V) supply */
> > > According to the device tree bindings these should be uppercase. Did I
> > > miss a patch that updates the bindings?
> > > 
> > > I think the preference is for supply names to be lowercase and given
> > > that there are no users of this binding yet we could update it without
> > > breaking any existing device trees.
> > > 
> > > > +};
> > > > +
> > > > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> > > > +
> > > >   /*
> > > >    * imx274 I2C operation related structure
> > > >    */
> > > > @@ -501,6 +512,8 @@ struct imx274_ctrls {
> > > >    * @frame_rate: V4L2 frame rate structure
> > > >    * @regmap: Pointer to regmap structure
> > > >    * @reset_gpio: Pointer to reset gpio
> > > > + * @supplies: List of analog and digital supply regulators
> > > > + * @inck: Pointer to sensor input clock
> > > >    * @lock: Mutex structure
> > > >    * @mode: Parameters for the selected readout mode
> > > >    */
> > > > @@ -514,6 +527,8 @@ struct stimx274 {
> > > >   	struct v4l2_fract frame_interval;
> > > >   	struct regmap *regmap;
> > > >   	struct gpio_desc *reset_gpio;
> > > > +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
> > > > +	struct clk *inck;
> > > >   	struct mutex lock; /* mutex lock for operations */
> > > >   	const struct imx274_mode *mode;
> > > >   };
> > > > @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
> > > >   {
> > > >   	int err = 0;
> > > > +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
> > > > +	if (err) {
> > > > +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > >   	/*
> > > >   	 * Refer to "Standby Cancel Sequence when using CSI-2" in
> > > >   	 * imx274 datasheet, it should wait 10ms or more here.
> > > > @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
> > > >   	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
> > > >   }
> > > > +static int imx274_power_on(struct device *dev)
> > > > +{
> > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > > +	int ret;
> > > > +
> > > > +	/* keep sensor in reset before power on */
> > > > +	imx274_reset(imx274, 0);
> > > > +
> > > > +	ret = clk_prepare_enable(imx274->inck);
> > > > +	if (ret) {
> > > > +		dev_err(&imx274->client->dev,
> > > > +			"Failed to enable input clock: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > > +	if (ret) {
> > > > +		dev_err(&imx274->client->dev,
> > > > +			"Failed to enable regulators: %d\n", ret);
> > > > +		goto fail_reg;
> > > > +	}
> > > > +
> > > > +	udelay(2);
> > > This looks like some sort of extra delay to make sure all the supply
> > > voltages have settled. Should this perhaps be encoded as part of the
> > > regulator ramp-up times? Or is this really an IC-specific delay that
> > > is needed for some internal timing?
> > This is IC-specific delay after power on regulators before releasing reset.
> > > 
> > > > +	imx274_reset(imx274, 1);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +fail_reg:
> > > > +	clk_disable_unprepare(imx274->inck);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int imx274_power_off(struct device *dev)
> > > > +{
> > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > > +
> > > > +	imx274_reset(imx274, 0);
> > > > +
> > > > +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > > +
> > > > +	clk_disable_unprepare(imx274->inck);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
> > > > +		imx274->supplies[i].supply = imx274_supply_names[i];
> > > > +
> > > > +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
> > > > +					imx274->supplies);
> > > > +}
> > > > +
> > > >   /**
> > > >    * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
> > > >    * @ctrl: V4L2 control to be set
> > > > @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >   	struct stimx274 *imx274 = to_imx274(sd);
> > > >   	int ret = -EINVAL;
> > > > +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
> > > > +		return 0;
> > > I'm not sure I understand this, and sorry if this has been discussed
> > > earlier. Aren't there any other mechanisms in place to ensure that a
> > > control can only be configured when in use? If so, then is this even
> > > necessary?
> > > 
> > > If not, silently ignoring at this point seems like it could cause subtle
> > > failures by ignoring some control settings and applying others if the
> > > timing is right.
> > 
> > With this patch, v4l2_ctrl setup is moved to start stream so all the control
> > values selected gets programmed during stream start. So s_ctrl callback
> > execution happens during that time after sensor rpm resume and I don't think
> > we need here either but I see all sensor drivers with RPM enabled checking
> > for this. So added just to make sure sensor programming don't happen when
> > power is off.
> > 
> > Sakari/Jacob,
> > 
> > Can you please clarify if we can remove check pm_runtime_get_if_in_use() in
> > s_ctrl callback as v4l2_ctrl handler setup happens during stream start where
> > power is already on by then?
> 
> The controls are accessible also when streaming is disabled. So you may end
> up here without the device being powered on. Therefore the check is needed.

In that case shouldn't this return an error rather than silently
ignoring the request? From my reading of the code this current
implementation would allow someone to configure a control while
streaming is disabled, and that configuration will then succeed
without doing anything.

Or am I missing something and all controls will be reapplied when
streaming resumes and this is actually safe to ignore?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Sakari Ailus]

On Wed, Sep 23, 2020 at 10:07:05AM +0200, Thierry Reding wrote:
> On Tue, Sep 22, 2020 at 07:20:25PM +0300, Sakari Ailus wrote:
> > Hi Sowjanya,
> > 
> > On Tue, Sep 22, 2020 at 09:13:57AM -0700, Sowjanya Komatineni wrote:
> > > 
> > > On 9/22/20 12:55 AM, Thierry Reding wrote:
> > > > On Mon, Sep 21, 2020 at 02:39:39PM -0700, Sowjanya Komatineni wrote:
> > > > > IMX274 has analog 2.8V supply, digital core 1.8V supply, and vddl digital
> > > > > io 1.2V supply which are optional based on camera module design.
> > > > > 
> > > > > IMX274 also need external 24Mhz clock and is optional based on
> > > > > camera module design.
> > > > > 
> > > > > This patch adds support for IMX274 power on and off to enable and
> > > > > disable these supplies and external clock.
> > > > > 
> > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > > > ---
> > > > >   drivers/media/i2c/imx274.c | 184 +++++++++++++++++++++++++++++++++------------
> > > > >   1 file changed, 134 insertions(+), 50 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > > > index 5e515f0..b3057a5 100644
> > > > > --- a/drivers/media/i2c/imx274.c
> > > > > +++ b/drivers/media/i2c/imx274.c
> > > > > @@ -18,7 +18,9 @@
> > > > >   #include <linux/kernel.h>
> > > > >   #include <linux/module.h>
> > > > >   #include <linux/of_gpio.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > >   #include <linux/regmap.h>
> > > > > +#include <linux/regulator/consumer.h>
> > > > >   #include <linux/slab.h>
> > > > >   #include <linux/v4l2-mediabus.h>
> > > > >   #include <linux/videodev2.h>
> > > > > @@ -131,6 +133,15 @@
> > > > >   #define IMX274_TABLE_WAIT_MS			0
> > > > >   #define IMX274_TABLE_END			1
> > > > > +/* regulator supplies */
> > > > > +static const char * const imx274_supply_names[] = {
> > > > > +	"vddl",  /* IF (1.2V) supply */
> > > > > +	"vdig",  /* Digital Core (1.8V) supply */
> > > > > +	"vana",  /* Analog (2.8V) supply */
> > > > According to the device tree bindings these should be uppercase. Did I
> > > > miss a patch that updates the bindings?
> > > > 
> > > > I think the preference is for supply names to be lowercase and given
> > > > that there are no users of this binding yet we could update it without
> > > > breaking any existing device trees.
> > > > 
> > > > > +};
> > > > > +
> > > > > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> > > > > +
> > > > >   /*
> > > > >    * imx274 I2C operation related structure
> > > > >    */
> > > > > @@ -501,6 +512,8 @@ struct imx274_ctrls {
> > > > >    * @frame_rate: V4L2 frame rate structure
> > > > >    * @regmap: Pointer to regmap structure
> > > > >    * @reset_gpio: Pointer to reset gpio
> > > > > + * @supplies: List of analog and digital supply regulators
> > > > > + * @inck: Pointer to sensor input clock
> > > > >    * @lock: Mutex structure
> > > > >    * @mode: Parameters for the selected readout mode
> > > > >    */
> > > > > @@ -514,6 +527,8 @@ struct stimx274 {
> > > > >   	struct v4l2_fract frame_interval;
> > > > >   	struct regmap *regmap;
> > > > >   	struct gpio_desc *reset_gpio;
> > > > > +	struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES];
> > > > > +	struct clk *inck;
> > > > >   	struct mutex lock; /* mutex lock for operations */
> > > > >   	const struct imx274_mode *mode;
> > > > >   };
> > > > > @@ -726,6 +741,12 @@ static int imx274_start_stream(struct stimx274 *priv)
> > > > >   {
> > > > >   	int err = 0;
> > > > > +	err = __v4l2_ctrl_handler_setup(&priv->ctrls.handler);
> > > > > +	if (err) {
> > > > > +		dev_err(&priv->client->dev, "Error %d setup controls\n", err);
> > > > > +		return err;
> > > > > +	}
> > > > > +
> > > > >   	/*
> > > > >   	 * Refer to "Standby Cancel Sequence when using CSI-2" in
> > > > >   	 * imx274 datasheet, it should wait 10ms or more here.
> > > > > @@ -767,6 +788,66 @@ static void imx274_reset(struct stimx274 *priv, int rst)
> > > > >   	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
> > > > >   }
> > > > > +static int imx274_power_on(struct device *dev)
> > > > > +{
> > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > > > +	int ret;
> > > > > +
> > > > > +	/* keep sensor in reset before power on */
> > > > > +	imx274_reset(imx274, 0);
> > > > > +
> > > > > +	ret = clk_prepare_enable(imx274->inck);
> > > > > +	if (ret) {
> > > > > +		dev_err(&imx274->client->dev,
> > > > > +			"Failed to enable input clock: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > > > +	if (ret) {
> > > > > +		dev_err(&imx274->client->dev,
> > > > > +			"Failed to enable regulators: %d\n", ret);
> > > > > +		goto fail_reg;
> > > > > +	}
> > > > > +
> > > > > +	udelay(2);
> > > > This looks like some sort of extra delay to make sure all the supply
> > > > voltages have settled. Should this perhaps be encoded as part of the
> > > > regulator ramp-up times? Or is this really an IC-specific delay that
> > > > is needed for some internal timing?
> > > This is IC-specific delay after power on regulators before releasing reset.
> > > > 
> > > > > +	imx274_reset(imx274, 1);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +fail_reg:
> > > > > +	clk_disable_unprepare(imx274->inck);
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int imx274_power_off(struct device *dev)
> > > > > +{
> > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > > +	struct stimx274 *imx274 = to_imx274(sd);
> > > > > +
> > > > > +	imx274_reset(imx274, 0);
> > > > > +
> > > > > +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> > > > > +
> > > > > +	clk_disable_unprepare(imx274->inck);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx274_regulators_get(struct device *dev, struct stimx274 *imx274)
> > > > > +{
> > > > > +	unsigned int i;
> > > > > +
> > > > > +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++)
> > > > > +		imx274->supplies[i].supply = imx274_supply_names[i];
> > > > > +
> > > > > +	return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES,
> > > > > +					imx274->supplies);
> > > > > +}
> > > > > +
> > > > >   /**
> > > > >    * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
> > > > >    * @ctrl: V4L2 control to be set
> > > > > @@ -781,6 +862,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > >   	struct stimx274 *imx274 = to_imx274(sd);
> > > > >   	int ret = -EINVAL;
> > > > > +	if (!pm_runtime_get_if_in_use(&imx274->client->dev))
> > > > > +		return 0;
> > > > I'm not sure I understand this, and sorry if this has been discussed
> > > > earlier. Aren't there any other mechanisms in place to ensure that a
> > > > control can only be configured when in use? If so, then is this even
> > > > necessary?
> > > > 
> > > > If not, silently ignoring at this point seems like it could cause subtle
> > > > failures by ignoring some control settings and applying others if the
> > > > timing is right.
> > > 
> > > With this patch, v4l2_ctrl setup is moved to start stream so all the control
> > > values selected gets programmed during stream start. So s_ctrl callback
> > > execution happens during that time after sensor rpm resume and I don't think
> > > we need here either but I see all sensor drivers with RPM enabled checking
> > > for this. So added just to make sure sensor programming don't happen when
> > > power is off.
> > > 
> > > Sakari/Jacob,
> > > 
> > > Can you please clarify if we can remove check pm_runtime_get_if_in_use() in
> > > s_ctrl callback as v4l2_ctrl handler setup happens during stream start where
> > > power is already on by then?
> > 
> > The controls are accessible also when streaming is disabled. So you may end
> > up here without the device being powered on. Therefore the check is needed.
> 
> In that case shouldn't this return an error rather than silently
> ignoring the request? From my reading of the code this current
> implementation would allow someone to configure a control while
> streaming is disabled, and that configuration will then succeed
> without doing anything.
> 
> Or am I missing something and all controls will be reapplied when
> streaming resumes and this is actually safe to ignore?

The value of V4L2 controls is not tied to the power state of the device.
The driver is responsible for ensuring the value set by the user is applied
to the hardware when needed. That does not happen immediately if the device
is powered off but it's not an issue.

-- 
Sakari Ailus

Re: [PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Luca Ceresoli]

On 22/09/20 10:47, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Sep 22, 2020 at 10:09:33AM +0200, Luca Ceresoli wrote:
>> Hi,
>>
>> On 21/09/20 23:39, Sowjanya Komatineni wrote:
>>> Sensor should already be in standby during remove and there is no
>>> need to configure sensor registers for stream stop.
>>
>> I beg your pardon for the newbie question: does the V4L2 framework
>> guarantee that the stream is stopped (.s_stream(..., 0)) before removing
>> the driver?
> 
> It doesn't. That's however one of the lesser concerns, and I don't think
> it'd help if drivers tried to prepare for that.

Thanks for the clarification.

I've been working with hardware where the sensor is always powered. In
this case, and with this patch applied, the sensor would keep producing
frames after driver removal. This looks wrong, unless I'm overlooking
something.

BTW at first sight it looks like the framework should take care of
stopping the stream before removal, not the individual drivers, but
maybe there are good reasons this is not done?

-- 
Luca

Re: [PATCH v6 2/3] media: i2c: imx274: Remove stop stream i2c writes during remove

[Sakari Ailus]

On Thu, Sep 24, 2020 at 12:09:22PM +0200, Luca Ceresoli wrote:
> On 22/09/20 10:47, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > On Tue, Sep 22, 2020 at 10:09:33AM +0200, Luca Ceresoli wrote:
> >> Hi,
> >>
> >> On 21/09/20 23:39, Sowjanya Komatineni wrote:
> >>> Sensor should already be in standby during remove and there is no
> >>> need to configure sensor registers for stream stop.
> >>
> >> I beg your pardon for the newbie question: does the V4L2 framework
> >> guarantee that the stream is stopped (.s_stream(..., 0)) before removing
> >> the driver?
> > 
> > It doesn't. That's however one of the lesser concerns, and I don't think
> > it'd help if drivers tried to prepare for that.
> 
> Thanks for the clarification.
> 
> I've been working with hardware where the sensor is always powered. In
> this case, and with this patch applied, the sensor would keep producing
> frames after driver removal. This looks wrong, unless I'm overlooking
> something.
> 
> BTW at first sight it looks like the framework should take care of
> stopping the stream before removal, not the individual drivers, but
> maybe there are good reasons this is not done?

Yes, it should.

This is a complex problem area to address. The framework (V4L2) wasn't
originally built with object refcounting when support was added for complex
cameras that comprise multiple devices because it wasn't thought it would
be really needed. And it's hard to add that later on.

Either way, there's little individual drivers can do to address that; the
framework support needs to be there first.

The same goes with Media controller; it's not just V4L2.

-- 
Sakari Ailus