Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting
@ 2020-07-20 17:01 Sowjanya Komatineni
  2020-07-20 17:01 ` [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni
  2020-07-20 17:01 ` [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni
  0 siblings, 2 replies; 9+ messages in thread
From: Sowjanya Komatineni @ 2020-07-20 17:01 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil, luca,
	leonl, robh+dt, lgirdwood, broonie
  Cc: linux-media, devicetree, linux-kernel

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 e6aa9f3..55869ff 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies
  2020-07-20 17:01 [PATCH v3 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni
@ 2020-07-20 17:01 ` Sowjanya Komatineni
  2020-07-31 16:19   ` Sakari Ailus
  2020-07-20 17:01 ` [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni
  1 sibling, 1 reply; 9+ messages in thread
From: Sowjanya Komatineni @ 2020-07-20 17:01 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil, luca,
	leonl, robh+dt, lgirdwood, broonie
  Cc: linux-media, devicetree, linux-kernel

This patch adds IMX274 optional external clock input and voltage
supplies to device tree bindings.

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 Documentation/devicetree/bindings/media/i2c/imx274.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
index 80f2e89..0727079 100644
--- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
+++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
@@ -13,6 +13,11 @@ Required Properties:
 
 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.
 
 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
  2020-07-20 17:01 [PATCH v3 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni
  2020-07-20 17:01 ` [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni
@ 2020-07-20 17:01 ` Sowjanya Komatineni
  2020-07-21 14:03   ` Luca Ceresoli
  2020-07-31 16:26   ` Sakari Ailus
  1 sibling, 2 replies; 9+ messages in thread
From: Sowjanya Komatineni @ 2020-07-20 17:01 UTC (permalink / raw)
  To: skomatineni, thierry.reding, jonathanh, frankc, hverkuil, luca,
	leonl, robh+dt, lgirdwood, broonie
  Cc: linux-media, devicetree, linux-kernel

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.

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 55869ff..7157b1d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of_gpio.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 +132,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 +511,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 +526,8 @@ struct stimx274 {
 	struct v4l2_fract frame_interval;
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
+	struct regulator *supplies[IMX274_NUM_SUPPLIES];
+	struct clk *inck;
 	struct mutex lock; /* mutex lock for operations */
 	const struct imx274_mode *mode;
 };
@@ -767,6 +781,99 @@ 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 i, ret;
+
+	ret = clk_prepare_enable(imx274->inck);
+	if (ret) {
+		dev_err(&imx274->client->dev,
+			"Failed to enable input clock: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
+		if (imx274->supplies[i]) {
+			ret = regulator_enable(imx274->supplies[i]);
+			if (ret < 0) {
+				dev_err(&imx274->client->dev,
+					"Failed to enable %s supply: %d\n",
+					imx274_supply_names[i], ret);
+				goto fail_reg;
+			}
+		}
+	}
+
+	usleep_range(1, 2);
+	imx274_reset(imx274, 1);
+
+	return 0;
+
+fail_reg:
+	for (--i; i >= 0; i--) {
+		if (imx274->supplies[i])
+			regulator_disable(imx274->supplies[i]);
+	}
+
+	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);
+	int i;
+
+	imx274_reset(imx274, 0);
+
+	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
+		if (imx274->supplies[i])
+			regulator_disable(imx274->supplies[i]);
+	}
+
+	clk_disable_unprepare(imx274->inck);
+
+	return 0;
+}
+
+static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
+{
+	int i, err;
+	const char *supply;
+
+	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
+		supply = imx274_supply_names[i];
+		imx274->supplies[i] = devm_regulator_get_optional(dev, supply);
+		if (!IS_ERR(imx274->supplies[i]))
+			continue;
+		err = PTR_ERR(imx274->supplies[i]);
+		if (err != -ENODEV) {
+			if (err != -EPROBE_DEFER)
+				dev_err(&imx274->client->dev,
+					"Failed to get %s supply: %d\n",
+					supply, err);
+			return err;
+		}
+
+		imx274->supplies[i] = NULL;
+	}
+
+	return 0;
+}
+
 /**
  * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
  * @ctrl: V4L2 control to be set
@@ -1836,6 +1943,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;
@@ -1883,15 +1998,23 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_me;
 	}
 
-	/* pull sensor out of reset */
-	imx274_reset(imx274, 1);
+	/* keep sensor in reset before power on */
+	imx274_reset(imx274, 0);
+
+	/* 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;
@@ -1958,6 +2081,8 @@ static int imx274_probe(struct i2c_client *client)
 
 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:
@@ -1975,6 +2100,7 @@ static int imx274_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
+	imx274_power_off(&client->dev);
 	media_entity_cleanup(&sd->entity);
 	mutex_destroy(&imx274->lock);
 	return 0;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
  2020-07-20 17:01 ` [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni
@ 2020-07-21 14:03   ` Luca Ceresoli
  2020-07-21 14:28     ` Sowjanya Komatineni
  2020-07-31 16:26   ` Sakari Ailus
  1 sibling, 1 reply; 9+ messages in thread
From: Luca Ceresoli @ 2020-07-21 14:03 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
	leonl, robh+dt, lgirdwood, broonie
  Cc: linux-media, devicetree, linux-kernel

Hi Sowjanya,

On 20/07/20 19:01, 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.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---

In the future please add a log of changes since previous versions here.

As far as the patch is concerned:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

Thanks.
-- 
Luca

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
  2020-07-21 14:03   ` Luca Ceresoli
@ 2020-07-21 14:28     ` Sowjanya Komatineni
  0 siblings, 0 replies; 9+ messages in thread
From: Sowjanya Komatineni @ 2020-07-21 14:28 UTC (permalink / raw)
  To: Luca Ceresoli, thierry.reding, jonathanh, frankc, hverkuil,
	leonl, robh+dt, lgirdwood, broonie
  Cc: linux-media, devicetree, linux-kernel


On 7/21/20 7:03 AM, Luca Ceresoli wrote:
> Hi Sowjanya,
>
> On 20/07/20 19:01, 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.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
> In the future please add a log of changes since previous versions here.
>
> As far as the patch is concerned:
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> Thanks.

Sure.

Thanks

Sowjanya


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies
  2020-07-20 17:01 ` [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni
@ 2020-07-31 16:19   ` Sakari Ailus
  2020-07-31 16:28     ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2020-07-31 16:19 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, frankc, hverkuil, luca, leonl,
	robh+dt, lgirdwood, broonie, linux-media, devicetree,
	linux-kernel

Hi Sowjanya,

On Mon, Jul 20, 2020 at 10:01:33AM -0700, Sowjanya Komatineni wrote:
> This patch adds IMX274 optional external clock input and voltage
> supplies to device tree bindings.
> 
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/imx274.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> index 80f2e89..0727079 100644
> --- a/Documentation/devicetree/bindings/media/i2c/imx274.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt
> @@ -13,6 +13,11 @@ Required Properties:
>  
>  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.

I believe lower case is preferred.

>  
>  The imx274 device node should contain one 'port' child node with
>  an 'endpoint' subnode. For further reading on port node refer to

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
  2020-07-20 17:01 ` [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni
  2020-07-21 14:03   ` Luca Ceresoli
@ 2020-07-31 16:26   ` Sakari Ailus
  2020-07-31 16:34     ` Sowjanya Komatineni
  1 sibling, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2020-07-31 16:26 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, frankc, hverkuil, luca, leonl,
	robh+dt, lgirdwood, broonie, linux-media, devicetree,
	linux-kernel

Hi Sowjanya,

Thanks for the patch.

On Mon, Jul 20, 2020 at 10:01:34AM -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.

The sensor appears to be able to use other frequencies, too. Could you
check in the driver the frequency is correct? This should be found in DT
bindings, too.

> 
> This patch adds support for IMX274 power on and off to enable and
> disable these supplies and external clock.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/media/i2c/imx274.c | 132 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 129 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 55869ff..7157b1d 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/of_gpio.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 +132,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)

Please use ARRAY_SIZE() directly.

> +
>  /*
>   * imx274 I2C operation related structure
>   */
> @@ -501,6 +511,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 +526,8 @@ struct stimx274 {
>  	struct v4l2_fract frame_interval;
>  	struct regmap *regmap;
>  	struct gpio_desc *reset_gpio;
> +	struct regulator *supplies[IMX274_NUM_SUPPLIES];
> +	struct clk *inck;
>  	struct mutex lock; /* mutex lock for operations */
>  	const struct imx274_mode *mode;
>  };
> @@ -767,6 +781,99 @@ 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 i, ret;
> +
> +	ret = clk_prepare_enable(imx274->inck);
> +	if (ret) {
> +		dev_err(&imx274->client->dev,
> +			"Failed to enable input clock: %d\n", ret);
> +		return ret;
> +	}
> +

Could you use regulator_bulk_enable() instead? Same for disable.

> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
> +		if (imx274->supplies[i]) {
> +			ret = regulator_enable(imx274->supplies[i]);
> +			if (ret < 0) {
> +				dev_err(&imx274->client->dev,
> +					"Failed to enable %s supply: %d\n",
> +					imx274_supply_names[i], ret);
> +				goto fail_reg;
> +			}
> +		}
> +	}
> +
> +	usleep_range(1, 2);

This is a very low value. Does that 1 µs come from the sensor datasheet?

> +	imx274_reset(imx274, 1);
> +
> +	return 0;
> +
> +fail_reg:
> +	for (--i; i >= 0; i--) {
> +		if (imx274->supplies[i])
> +			regulator_disable(imx274->supplies[i]);
> +	}
> +
> +	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);
> +	int i;
> +
> +	imx274_reset(imx274, 0);
> +
> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
> +		if (imx274->supplies[i])
> +			regulator_disable(imx274->supplies[i]);
> +	}
> +
> +	clk_disable_unprepare(imx274->inck);
> +
> +	return 0;
> +}
> +
> +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
> +{
> +	int i, err;
> +	const char *supply;
> +
> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
> +		supply = imx274_supply_names[i];
> +		imx274->supplies[i] = devm_regulator_get_optional(dev, supply);
> +		if (!IS_ERR(imx274->supplies[i]))
> +			continue;
> +		err = PTR_ERR(imx274->supplies[i]);
> +		if (err != -ENODEV) {
> +			if (err != -EPROBE_DEFER)
> +				dev_err(&imx274->client->dev,
> +					"Failed to get %s supply: %d\n",
> +					supply, err);
> +			return err;
> +		}
> +
> +		imx274->supplies[i] = NULL;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
>   * @ctrl: V4L2 control to be set
> @@ -1836,6 +1943,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;
> @@ -1883,15 +1998,23 @@ static int imx274_probe(struct i2c_client *client)
>  		goto err_me;
>  	}
>  
> -	/* pull sensor out of reset */
> -	imx274_reset(imx274, 1);
> +	/* keep sensor in reset before power on */
> +	imx274_reset(imx274, 0);
> +
> +	/* power on the sensor */
> +	ret = imx274_power_on(&client->dev);

This driver would really benefit from runtime PM support. I guess you
usually don't want to leave the device powered on when it's not in use?

Please see e.g. the ov8856 driver.

> +	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;
> @@ -1958,6 +2081,8 @@ static int imx274_probe(struct i2c_client *client)
>  
>  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:
> @@ -1975,6 +2100,7 @@ static int imx274_remove(struct i2c_client *client)
>  
>  	v4l2_async_unregister_subdev(sd);
>  	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
> +	imx274_power_off(&client->dev);
>  	media_entity_cleanup(&sd->entity);
>  	mutex_destroy(&imx274->lock);
>  	return 0;

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies
  2020-07-31 16:19   ` Sakari Ailus
@ 2020-07-31 16:28     ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2020-07-31 16:28 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Sowjanya Komatineni, thierry.reding, jonathanh, frankc, hverkuil,
	luca, leonl, robh+dt, lgirdwood, linux-media, devicetree,
	linux-kernel


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

On Fri, Jul 31, 2020 at 07:19:08PM +0300, Sakari Ailus wrote:

> > +- VANA-supply: Sensor 2.8v analog supply.
> > +- VDIG-supply: Sensor 1.8v digital core supply.
> > +- VDDL-supply: Sensor digital IO 1.2v supply.

> I believe lower case is preferred.

Either is fine from my POV.  The code always used to use upper case but
it's not clear if that's the best choice when it makes its way into
bindings.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence
  2020-07-31 16:26   ` Sakari Ailus
@ 2020-07-31 16:34     ` Sowjanya Komatineni
  0 siblings, 0 replies; 9+ messages in thread
From: Sowjanya Komatineni @ 2020-07-31 16:34 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: thierry.reding, jonathanh, frankc, hverkuil, luca, leonl,
	robh+dt, lgirdwood, broonie, linux-media, devicetree,
	linux-kernel


On 7/31/20 9:26 AM, Sakari Ailus wrote:
> Hi Sowjanya,
>
> Thanks for the patch.
>
> On Mon, Jul 20, 2020 at 10:01:34AM -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.
> The sensor appears to be able to use other frequencies, too. Could you
> check in the driver the frequency is correct? This should be found in DT
> bindings, too.

External input clock is not in DT. So added it as part of this series.

We are mostly using 24Mhz I/P with IMX274 on designs we have and also on 
leopard module which has onboard XTAL for 24Mhz

>> This patch adds support for IMX274 power on and off to enable and
>> disable these supplies and external clock.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   drivers/media/i2c/imx274.c | 132 +++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 129 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 55869ff..7157b1d 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of_gpio.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 +132,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)
> Please use ARRAY_SIZE() directly.
>
>> +
>>   /*
>>    * imx274 I2C operation related structure
>>    */
>> @@ -501,6 +511,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 +526,8 @@ struct stimx274 {
>>   	struct v4l2_fract frame_interval;
>>   	struct regmap *regmap;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator *supplies[IMX274_NUM_SUPPLIES];
>> +	struct clk *inck;
>>   	struct mutex lock; /* mutex lock for operations */
>>   	const struct imx274_mode *mode;
>>   };
>> @@ -767,6 +781,99 @@ 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 i, ret;
>> +
>> +	ret = clk_prepare_enable(imx274->inck);
>> +	if (ret) {
>> +		dev_err(&imx274->client->dev,
>> +			"Failed to enable input clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
> Could you use regulator_bulk_enable() instead? Same for disable.

Using regulator_bulk_enable() makes these regulators mandatory.

But not all camera module designs expect power supply externally.

Leopard module don't need to enable these separately as their design has 
onboard supplies that get enabled without SW control for individually 
turning them on.

Some custom designs depend on sourcing externally and SW should enable 
them like one of the other design module we have for Jetson TX1. 
Depending on camera module design, these supplies can be optional.

>
>> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
>> +		if (imx274->supplies[i]) {
>> +			ret = regulator_enable(imx274->supplies[i]);
>> +			if (ret < 0) {
>> +				dev_err(&imx274->client->dev,
>> +					"Failed to enable %s supply: %d\n",
>> +					imx274_supply_names[i], ret);
>> +				goto fail_reg;
>> +			}
>> +		}
>> +	}
>> +
>> +	usleep_range(1, 2);
> This is a very low value. Does that 1 µs come from the sensor datasheet?
>
>> +	imx274_reset(imx274, 1);
>> +
>> +	return 0;
>> +
>> +fail_reg:
>> +	for (--i; i >= 0; i--) {
>> +		if (imx274->supplies[i])
>> +			regulator_disable(imx274->supplies[i]);
>> +	}
>> +
>> +	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);
>> +	int i;
>> +
>> +	imx274_reset(imx274, 0);
>> +
>> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
>> +		if (imx274->supplies[i])
>> +			regulator_disable(imx274->supplies[i]);
>> +	}
>> +
>> +	clk_disable_unprepare(imx274->inck);
>> +
>> +	return 0;
>> +}
>> +
>> +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
>> +{
>> +	int i, err;
>> +	const char *supply;
>> +
>> +	for (i = 0; i < IMX274_NUM_SUPPLIES; i++) {
>> +		supply = imx274_supply_names[i];
>> +		imx274->supplies[i] = devm_regulator_get_optional(dev, supply);
>> +		if (!IS_ERR(imx274->supplies[i]))
>> +			continue;
>> +		err = PTR_ERR(imx274->supplies[i]);
>> +		if (err != -ENODEV) {
>> +			if (err != -EPROBE_DEFER)
>> +				dev_err(&imx274->client->dev,
>> +					"Failed to get %s supply: %d\n",
>> +					supply, err);
>> +			return err;
>> +		}
>> +
>> +		imx274->supplies[i] = NULL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
>>    * @ctrl: V4L2 control to be set
>> @@ -1836,6 +1943,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;
>> @@ -1883,15 +1998,23 @@ static int imx274_probe(struct i2c_client *client)
>>   		goto err_me;
>>   	}
>>   
>> -	/* pull sensor out of reset */
>> -	imx274_reset(imx274, 1);
>> +	/* keep sensor in reset before power on */
>> +	imx274_reset(imx274, 0);
>> +
>> +	/* power on the sensor */
>> +	ret = imx274_power_on(&client->dev);
> This driver would really benefit from runtime PM support. I guess you
> usually don't want to leave the device powered on when it's not in use?
>
> Please see e.g. the ov8856 driver.
>
>> +	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;
>> @@ -1958,6 +2081,8 @@ static int imx274_probe(struct i2c_client *client)
>>   
>>   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:
>> @@ -1975,6 +2100,7 @@ static int imx274_remove(struct i2c_client *client)
>>   
>>   	v4l2_async_unregister_subdev(sd);
>>   	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
>> +	imx274_power_off(&client->dev);
>>   	media_entity_cleanup(&sd->entity);
>>   	mutex_destroy(&imx274->lock);
>>   	return 0;

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 17:01 [PATCH v3 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni
2020-07-20 17:01 ` [PATCH v3 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni
2020-07-31 16:19   ` Sakari Ailus
2020-07-31 16:28     ` Mark Brown
2020-07-20 17:01 ` [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni
2020-07-21 14:03   ` Luca Ceresoli
2020-07-21 14:28     ` Sowjanya Komatineni
2020-07-31 16:26   ` Sakari Ailus
2020-07-31 16:34     ` Sowjanya Komatineni

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git