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

[PATCH v2 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 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

[PATCH v2 2/3] dt-bindings: media: imx274: Add optional input clock and supplies

[Sowjanya Komatineni]

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

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

[PATCH v2 3/3] 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.

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 55869ff..c3f7bcd 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>
@@ -27,6 +28,8 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
+#define IMX274_DEFAULT_CLK_FREQ			24000000
+
 /*
  * See "SHR, SVR Setting" in datasheet
  */
@@ -131,6 +134,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 +513,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 +528,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 +783,98 @@ 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 clock\n");
+		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 +1944,13 @@ 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,26 @@ 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;
+	}
+
+	ret = clk_set_rate(imx274->inck, IMX274_DEFAULT_CLK_FREQ);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to set INCK clock rate\n");
+		return ret;
+	}
 
 	/* 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 +2084,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 +2103,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

Re: [PATCH v2 2/3] dt-bindings: media: imx274: Add optional input clock and supplies

[Luca Ceresoli]

On 17/07/20 21:15, Sowjanya Komatineni wrote:
> This patch adds IMX274 optional external clock input and voltage
> supplies to device tree bindings.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>


-- 
Luca

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

[Luca Ceresoli]

Hi,

On 17/07/20 21:15, 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>
> ---
>  drivers/media/i2c/imx274.c | 135 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 132 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 55869ff..c3f7bcd 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>
> @@ -27,6 +28,8 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
>  
> +#define IMX274_DEFAULT_CLK_FREQ			24000000
> +
>  /*
>   * See "SHR, SVR Setting" in datasheet
>   */
> @@ -131,6 +134,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 +513,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 +528,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 +783,98 @@ 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 clock\n");
> +		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;
> +			}
> +		}
> +	}

To me it looks a lot better now, thanks!

> +
> +	usleep_range(1, 2);
> +	imx274_reset(imx274, 1);

With your patch, the first time you enable the imx274 the reset pin is
in an unknown state. It would be probably nicer to keep the chip under
reset while enabling the clocks and deassert reset after they are all
up. Some datasheets require or suggest that, and even when not required
it seems a wise idea.

> +
> +	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 +1944,13 @@ 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,26 @@ 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;
> +	}
> +
> +	ret = clk_set_rate(imx274->inck, IMX274_DEFAULT_CLK_FREQ);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to set INCK clock rate\n");
> +		return ret;
> +	}

I don't think it is _wrong_ (some drivers do than), but I think
clk_set_rate() is better called before enabling the clock, in order to
avoid glitches.

-- 
Luca