[PATCH v4 0/4] IMX274 fixes and power on and off implementation

[PATCH v4 0/4] IMX274 fixes and power on and off implementation

[Sowjanya Komatineni]

This patch series includes
- Fix for proper Y_OUT_SIZE register configuration.
- Power on/off sequence implementation through runtime PM.
- dt-binding doc update to move input clock and supplies as required
  properties.

Delta between patch versions:
[v4]:	Includes below v4 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 (4):
  media: i2c: imx274: Fix Y_OUT_SIZE register setting
  dt-bindings: media: imx274: Use lower case for supply names
  dt-bindings: media: imx274: Move clock and supplies to required
    properties
  media: i2c: imx274: Add IMX274 power on and off sequence

 .../devicetree/bindings/media/i2c/imx274.txt       |  10 +-
 drivers/media/i2c/imx274.c                         | 153 ++++++++++++++++++++-
 2 files changed, 154 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] 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 v4 2/4] dt-bindings: media: imx274: Use lower case for supply names

[Sowjanya Komatineni]

This patch updates dt-binding to use lower case for supply names.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
index 0727079..d0a5c899 100644
--- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
+++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
@@ -15,9 +15,9 @@ Optional Properties:
 - reset-gpios: Sensor reset GPIO
 - clocks: Reference to the input clock.
 - clock-names: Should be "inck".
-- VANA-supply: Sensor 2.8v analog supply.
-- VDIG-supply: Sensor 1.8v digital core supply.
-- VDDL-supply: Sensor digital IO 1.2v supply.
+- vana-supply: Sensor 2.8v analog supply.
+- vdig-supply: Sensor 1.8v digital core supply.
+- vddl-supply: Sensor digital IO 1.2v supply.
 
 The imx274 device node should contain one 'port' child node with
 an 'endpoint' subnode. For further reading on port node refer to
-- 
2.7.4

[PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sowjanya Komatineni]

Clock and supplies are external to IMX274 sensor and are dependent
on camera module design.

So, this patch moves them to required properties.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
index d0a5c899..b43bed6 100644
--- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
+++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
@@ -10,15 +10,15 @@ at 1440 Mbps.
 Required Properties:
 - compatible: value should be "sony,imx274" for imx274 sensor
 - reg: I2C bus address of the device
-
-Optional Properties:
-- reset-gpios: Sensor reset GPIO
 - clocks: Reference to the input clock.
 - clock-names: Should be "inck".
 - vana-supply: Sensor 2.8v analog supply.
 - vdig-supply: Sensor 1.8v digital core supply.
 - vddl-supply: Sensor digital IO 1.2v supply.
 
+Optional Properties:
+- reset-gpios: Sensor reset GPIO
+
 The imx274 device node should contain one 'port' child node with
 an 'endpoint' subnode. For further reading on port node refer to
 Documentation/devicetree/bindings/media/video-interfaces.txt.
-- 
2.7.4

[PATCH v4 4/4] media: i2c: imx274: Add IMX274 power on and off sequence

[Sowjanya Komatineni]

IMX274 has VANA analog 2.8V supply, VDIG 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 | 151 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 148 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index a4b9dfd..18a1e87 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: imx274 analog and digital supplies
+ * @inck: input clock to imx274
  * @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;
 };
@@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst)
 	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
 }
 
+/*
+ * imx274_power_on - Function called to power on the sensor
+ * @imx274: Pointer to device structure
+ */
+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;
+	}
+
+	usleep_range(1, 2);
+	imx274_reset(imx274, 1);
+
+	return 0;
+
+fail_reg:
+	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
+	clk_disable_unprepare(imx274->inck);
+	return ret;
+}
+
+/*
+ * imx274_power_off - Function called to power off the sensor
+ * @imx274: Pointer to device structure
+ */
+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_get_regulators(struct device *dev, struct stimx274 *imx274)
+{
+	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 +865,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 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
+	pm_runtime_put(&imx274->client->dev);
+
 	return ret;
 }
 
@@ -1327,6 +1416,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)
@@ -1362,6 +1458,7 @@ 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 +1466,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 +1932,14 @@ static int imx274_probe(struct i2c_client *client)
 
 	mutex_init(&imx274->lock);
 
+	imx274->inck = devm_clk_get_optional(&client->dev, "inck");
+	ret = imx274_get_regulators(&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 +1987,23 @@ 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;
+	}
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
 
 	/* 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_disable_rpm;
 	}
 
 	imx274->ctrls.handler.lock = &imx274->lock;
@@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_ctrls;
 	}
 
+	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_disable_rpm:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
 err_me:
 	media_entity_cleanup(&sd->entity);
 err_regmap:
@@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct stimx274 *imx274 = to_imx274(sd);
 
+	pm_runtime_get_sync(&imx274->client->dev);
+
 	/* stop stream */
 	imx274_write_table(imx274, imx274_stop);
 
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
+
+	pm_runtime_put(&client->dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
 	media_entity_cleanup(&sd->entity);
 	mutex_destroy(&imx274->lock);
 	return 0;
 }
 
+static int __maybe_unused imx274_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return imx274_power_off(&client->dev);
+}
+
+static int __maybe_unused imx274_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return imx274_power_on(&client->dev);
+}
+
+static const struct dev_pm_ops imx274_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx274_runtime_suspend, imx274_runtime_resume, 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 v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sakari Ailus]

Hi Sowjanya,

On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
> Clock and supplies are external to IMX274 sensor and are dependent
> on camera module design.
> 
> So, this patch moves them to required properties.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> index d0a5c899..b43bed6 100644
> --- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> @@ -10,15 +10,15 @@ at 1440 Mbps.
>  Required Properties:
>  - compatible: value should be "sony,imx274" for imx274 sensor
>  - reg: I2C bus address of the device
> -
> -Optional Properties:
> -- reset-gpios: Sensor reset GPIO
>  - clocks: Reference to the input clock.
>  - clock-names: Should be "inck".
>  - vana-supply: Sensor 2.8v analog supply.
>  - vdig-supply: Sensor 1.8v digital core supply.
>  - vddl-supply: Sensor digital IO 1.2v supply.

If these have been optional in the past I don't think we can start
requiring them now.

The framework will just give the driver a dummy regulator if one isn't
found.

>  
> +Optional Properties:
> +- reset-gpios: Sensor reset GPIO
> +
>  The imx274 device node should contain one 'port' child node with
>  an 'endpoint' subnode. For further reading on port node refer to
>  Documentation/devicetree/bindings/media/video-interfaces.txt.

-- 
Kind regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Sowjanya,

Thanks for the update.

On Mon, Aug 31, 2020 at 12:52:38PM -0700, Sowjanya Komatineni wrote:
> IMX274 has VANA analog 2.8V supply, VDIG 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 | 151 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 148 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index a4b9dfd..18a1e87 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: imx274 analog and digital supplies

There's interface, too. You could also omit what there is as it's easily
visible elsewhere.

> + * @inck: input clock to imx274
>   * @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;
>  };
> @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst)
>  	usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
>  }
>  
> +/*
> + * imx274_power_on - Function called to power on the sensor
> + * @imx274: Pointer to device structure
> + */
> +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;
> +	}
> +
> +	usleep_range(1, 2);
> +	imx274_reset(imx274, 1);
> +
> +	return 0;
> +
> +fail_reg:
> +	regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies);
> +	clk_disable_unprepare(imx274->inck);
> +	return ret;
> +}
> +
> +/*
> + * imx274_power_off - Function called to power off the sensor
> + * @imx274: Pointer to device structure
> + */
> +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_get_regulators(struct device *dev, struct stimx274 *imx274)
> +{
> +	int i;

unsigned int

> +
> +	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 +865,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 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put(&imx274->client->dev);
> +
>  	return ret;
>  }
>  
> @@ -1327,6 +1416,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)
> @@ -1362,6 +1458,7 @@ 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 +1466,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 +1932,14 @@ static int imx274_probe(struct i2c_client *client)
>  
>  	mutex_init(&imx274->lock);
>  
> +	imx274->inck = devm_clk_get_optional(&client->dev, "inck");
> +	ret = imx274_get_regulators(&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 +1987,23 @@ 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;
> +	}
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
>  
>  	/* 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_disable_rpm;
>  	}
>  
>  	imx274->ctrls.handler.lock = &imx274->lock;
> @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client)
>  		goto err_ctrls;
>  	}
>  
> +	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_disable_rpm:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
>  err_me:
>  	media_entity_cleanup(&sd->entity);
>  err_regmap:
> @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct stimx274 *imx274 = to_imx274(sd);
>  
> +	pm_runtime_get_sync(&imx274->client->dev);
> +
>  	/* stop stream */

This really shouldn't happen and the driver isn't expected to handle it
either.

>  	imx274_write_table(imx274, imx274_stop);
>  
>  	v4l2_async_unregister_subdev(sd);
>  	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
> +
> +	pm_runtime_put(&client->dev);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +
>  	media_entity_cleanup(&sd->entity);
>  	mutex_destroy(&imx274->lock);
>  	return 0;
>  }
>  
> +static int __maybe_unused imx274_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	return imx274_power_off(&client->dev);
> +}
> +
> +static int __maybe_unused imx274_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	return imx274_power_on(&client->dev);
> +}

There's no need for the two wrappers, please remove them.

> +
> +static const struct dev_pm_ops imx274_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx274_runtime_suspend, imx274_runtime_resume, 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,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sakari Ailus]

Hi Sowjanya,

On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
> Clock and supplies are external to IMX274 sensor and are dependent
> on camera module design.
> 
> So, this patch moves them to required properties.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>

One more comment.

Jacopo has been working on converting this to YAML format. Could you rebase
your patch on his? I believe he's still working on some changes. The
subject is "[PATCH v3] dt-bindings: media: imx274: Convert to json-schema".

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sowjanya Komatineni]

On 8/31/20 1:17 PM, Sakari Ailus wrote:
> Hi Sowjanya,
>
> On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
>> Clock and supplies are external to IMX274 sensor and are dependent
>> on camera module design.
>>
>> So, this patch moves them to required properties.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
>> index d0a5c899..b43bed6 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
>> @@ -10,15 +10,15 @@ at 1440 Mbps.
>>   Required Properties:
>>   - compatible: value should be "sony,imx274" for imx274 sensor
>>   - reg: I2C bus address of the device
>> -
>> -Optional Properties:
>> -- reset-gpios: Sensor reset GPIO
>>   - clocks: Reference to the input clock.
>>   - clock-names: Should be "inck".
>>   - vana-supply: Sensor 2.8v analog supply.
>>   - vdig-supply: Sensor 1.8v digital core supply.
>>   - vddl-supply: Sensor digital IO 1.2v supply.
> If these have been optional in the past I don't think we can start
> requiring them now.
>
> The framework will just give the driver a dummy regulator if one isn't
> found.
These were added recently with my patches. So I hope should be ok to 
make them required as they are external to sensor
>
>>   
>> +Optional Properties:
>> +- reset-gpios: Sensor reset GPIO
>> +
>>   The imx274 device node should contain one 'port' child node with
>>   an 'endpoint' subnode. For further reading on port node refer to
>>   Documentation/devicetree/bindings/media/video-interfaces.txt.

Re: [PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sakari Ailus]

On Mon, Aug 31, 2020 at 01:37:21PM -0700, Sowjanya Komatineni wrote:
> 
> On 8/31/20 1:17 PM, Sakari Ailus wrote:
> > Hi Sowjanya,
> > 
> > On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
> > > Clock and supplies are external to IMX274 sensor and are dependent
> > > on camera module design.
> > > 
> > > So, this patch moves them to required properties.
> > > 
> > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > ---
> > >   Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> > > index d0a5c899..b43bed6 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> > > @@ -10,15 +10,15 @@ at 1440 Mbps.
> > >   Required Properties:
> > >   - compatible: value should be "sony,imx274" for imx274 sensor
> > >   - reg: I2C bus address of the device
> > > -
> > > -Optional Properties:
> > > -- reset-gpios: Sensor reset GPIO
> > >   - clocks: Reference to the input clock.
> > >   - clock-names: Should be "inck".
> > >   - vana-supply: Sensor 2.8v analog supply.
> > >   - vdig-supply: Sensor 1.8v digital core supply.
> > >   - vddl-supply: Sensor digital IO 1.2v supply.
> > If these have been optional in the past I don't think we can start
> > requiring them now.
> > 
> > The framework will just give the driver a dummy regulator if one isn't
> > found.
> These were added recently with my patches. So I hope should be ok to make
> them required as they are external to sensor

The bindings were added back in 2017, so they're not really recent.

-- 
Sakari Ailus

Re: [PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sowjanya Komatineni]

On 8/31/20 1:37 PM, Sowjanya Komatineni wrote:
>
> On 8/31/20 1:17 PM, Sakari Ailus wrote:
>> Hi Sowjanya,
>>
>> On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
>>> Clock and supplies are external to IMX274 sensor and are dependent
>>> on camera module design.
>>>
>>> So, this patch moves them to required properties.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>   Documentation/devicetree/bindings/media/i2c/imx274.txt | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt 
>>> b/Documentation/devicetree/bindings/media/i2c/imx274.txt
>>> index d0a5c899..b43bed6 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
>>> +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
>>> @@ -10,15 +10,15 @@ at 1440 Mbps.
>>>   Required Properties:
>>>   - compatible: value should be "sony,imx274" for imx274 sensor
>>>   - reg: I2C bus address of the device
>>> -
>>> -Optional Properties:
>>> -- reset-gpios: Sensor reset GPIO
>>>   - clocks: Reference to the input clock.
>>>   - clock-names: Should be "inck".
>>>   - vana-supply: Sensor 2.8v analog supply.
>>>   - vdig-supply: Sensor 1.8v digital core supply.
>>>   - vddl-supply: Sensor digital IO 1.2v supply.
>> If these have been optional in the past I don't think we can start
>> requiring them now.
>>
>> The framework will just give the driver a dummy regulator if one isn't
>> found.
> These were added recently with my patches. So I hope should be ok to 
> make them required as they are external to sensor

Also made them required as they are external to sensor and also to use 
bulk_enable/disable APIs, devm_regulator_bulk_get()

does not use OPTIONAL_GET.

>>
>>>   +Optional Properties:
>>> +- reset-gpios: Sensor reset GPIO
>>> +
>>>   The imx274 device node should contain one 'port' child node with
>>>   an 'endpoint' subnode. For further reading on port node refer to
>>> Documentation/devicetree/bindings/media/video-interfaces.txt.

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

[Sakari Ailus]

On Mon, Aug 31, 2020 at 01:46:00PM -0700, Sowjanya Komatineni wrote:
> 
> On 8/31/20 1:23 PM, Sakari Ailus wrote:
> > > @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client)
> > >   	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > >   	struct stimx274 *imx274 = to_imx274(sd);
> > > +	pm_runtime_get_sync(&imx274->client->dev);
> > > +
> > >   	/* stop stream */
> > This really shouldn't happen and the driver isn't expected to handle it
> > either.
> 
> Do you mean to remove stop stream during remove()?
> 
> Stop stream is not part of this change and as writes to sensor can't happen
> when power off, added pm_runtime_get_sync

Indeed.

But there certainly isn't a need to power the sensor on to stream off, is
there?

> 
> > >   	imx274_write_table(imx274, imx274_stop);
> > >   	v4l2_async_unregister_subdev(sd);
> > >   	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
> > > +
> > > +	pm_runtime_put(&client->dev);
> > > +	pm_runtime_disable(&client->dev);
> > > +	pm_runtime_set_suspended(&client->dev);
> > > +
> > >   	media_entity_cleanup(&sd->entity);
> > >   	mutex_destroy(&imx274->lock);
> > >   	return 0;
> > >   }

-- 
Sakari Ailus

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

[Sowjanya Komatineni]

On 8/31/20 1:48 PM, Sakari Ailus wrote:
> On Mon, Aug 31, 2020 at 01:46:00PM -0700, Sowjanya Komatineni wrote:
>> On 8/31/20 1:23 PM, Sakari Ailus wrote:
>>>> @@ -1968,19 +2087,45 @@ static int imx274_remove(struct i2c_client *client)
>>>>    	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>    	struct stimx274 *imx274 = to_imx274(sd);
>>>> +	pm_runtime_get_sync(&imx274->client->dev);
>>>> +
>>>>    	/* stop stream */
>>> This really shouldn't happen and the driver isn't expected to handle it
>>> either.
>> Do you mean to remove stop stream during remove()?
>>
>> Stop stream is not part of this change and as writes to sensor can't happen
>> when power off, added pm_runtime_get_sync
> Indeed.
>
> But there certainly isn't a need to power the sensor on to stream off, is
> there?

yeah not required. Will remove get_sync.

Will remove imx274 stop stream during remove() as separate patch as it 
was not introduced in this.

>>>>    	imx274_write_table(imx274, imx274_stop);
>>>>    	v4l2_async_unregister_subdev(sd);
>>>>    	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
>>>> +
>>>> +	pm_runtime_put(&client->dev);
>>>> +	pm_runtime_disable(&client->dev);
>>>> +	pm_runtime_set_suspended(&client->dev);
>>>> +
>>>>    	media_entity_cleanup(&sd->entity);
>>>>    	mutex_destroy(&imx274->lock);
>>>>    	return 0;
>>>>    }

Re: [PATCH v4 3/4] dt-bindings: media: imx274: Move clock and supplies to required properties

[Sowjanya Komatineni]

On 8/31/20 1:26 PM, Sakari Ailus wrote:
> Hi Sowjanya,
>
> On Mon, Aug 31, 2020 at 12:52:37PM -0700, Sowjanya Komatineni wrote:
>> Clock and supplies are external to IMX274 sensor and are dependent
>> on camera module design.
>>
>> So, this patch moves them to required properties.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> One more comment.
>
> Jacopo has been working on converting this to YAML format. Could you rebase
> your patch on his? I believe he's still working on some changes. The
> subject is "[PATCH v3] dt-bindings: media: imx274: Convert to json-schema".
Sure, will keep them as optional and will rebase dt-bindings on 
json-schema patch