Current driver sets Y_OUT_SIZE to crop height + 8 and seeing long frame
errors on the receiver size as expected height on reciever is crop height.
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/media/i2c/imx274.c | 170 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 55869ff..8a34c07 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
*/
@@ -501,6 +504,10 @@ struct imx274_ctrls {
* @frame_rate: V4L2 frame rate structure
* @regmap: Pointer to regmap structure
* @reset_gpio: Pointer to reset gpio
+ * @vana_supply: VANA analog supply regulator
+ * @vdig_supply: VDIG digital core supply regulator
+ * @vddl_supply: VDDL digital io supply regulator
+ * @xclk: system clock to imx274
* @lock: Mutex structure
* @mode: Parameters for the selected readout mode
*/
@@ -514,6 +521,10 @@ struct stimx274 {
struct v4l2_fract frame_interval;
struct regmap *regmap;
struct gpio_desc *reset_gpio;
+ struct regulator *vana_supply;
+ struct regulator *vdig_supply;
+ struct regulator *vddl_supply;
+ struct clk *xclk;
struct mutex lock; /* mutex lock for operations */
const struct imx274_mode *mode;
};
@@ -767,6 +778,138 @@ 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;
+
+ ret = clk_prepare_enable(imx274->xclk);
+ if (ret) {
+ dev_err(&imx274->client->dev, "Failed to enable clock\n");
+ return ret;
+ }
+
+ if (imx274->vana_supply) {
+ ret = regulator_enable(imx274->vana_supply);
+ if (ret < 0) {
+ dev_err(&imx274->client->dev,
+ "Failed to enable VANA supply: %d\n", ret);
+ goto disable_clk;
+ }
+ }
+
+ if (imx274->vdig_supply) {
+ ret = regulator_enable(imx274->vdig_supply);
+ if (ret < 0) {
+ dev_err(&imx274->client->dev,
+ "Failed to enable VDIG supply: %d\n", ret);
+ goto disable_vana_reg;
+ }
+ }
+
+ if (imx274->vddl_supply) {
+ ret = regulator_enable(imx274->vddl_supply);
+ if (ret < 0) {
+ dev_err(&imx274->client->dev,
+ "Failed to enable VDDL supply: %d\n", ret);
+ goto disable_vdig_reg;
+ }
+ }
+
+ usleep_range(1, 2);
+ imx274_reset(imx274, 1);
+
+ return 0;
+
+disable_vdig_reg:
+ if (imx274->vdig_supply)
+ regulator_disable(imx274->vdig_supply);
+disable_vana_reg:
+ if (imx274->vana_supply)
+ regulator_disable(imx274->vana_supply);
+disable_clk:
+ clk_disable_unprepare(imx274->xclk);
+ 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);
+
+ if (imx274->vddl_supply)
+ regulator_disable(imx274->vddl_supply);
+
+ if (imx274->vdig_supply)
+ regulator_disable(imx274->vdig_supply);
+
+ if (imx274->vana_supply)
+ regulator_disable(imx274->vana_supply);
+
+ clk_disable_unprepare(imx274->xclk);
+
+ return 0;
+}
+
+static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
+{
+ int i;
+
+ imx274->vana_supply = devm_regulator_get_optional(dev, "VANA");
+ if (IS_ERR(imx274->vana_supply)) {
+ if (PTR_ERR(imx274->vana_supply) != -ENODEV) {
+ if (PTR_ERR(imx274->vana_supply) != -EPROBE_DEFER)
+ dev_err(&imx274->client->dev,
+ "Failed to get VANA supply: %ld\n",
+ PTR_ERR(imx274->vana_supply));
+ return PTR_ERR(imx274->vana_supply);
+ }
+
+ imx274->vana_supply = NULL;
+ }
+
+ imx274->vdig_supply = devm_regulator_get_optional(dev, "VDIG");
+ if (IS_ERR(imx274->vdig_supply)) {
+ if (PTR_ERR(imx274->vdig_supply) != -ENODEV) {
+ if (PTR_ERR(imx274->vdig_supply) != -EPROBE_DEFER)
+ dev_err(&imx274->client->dev,
+ "Failed to get VDIG supply: %ld\n",
+ PTR_ERR(imx274->vdig_supply));
+ return PTR_ERR(imx274->vdig_supply);
+ }
+
+ imx274->vdig_supply = NULL;
+ }
+
+ imx274->vddl_supply = devm_regulator_get_optional(dev, "VDDL");
+ if (IS_ERR(imx274->vddl_supply)) {
+ if (PTR_ERR(imx274->vddl_supply) != -ENODEV) {
+ if (PTR_ERR(imx274->vddl_supply) != -EPROBE_DEFER)
+ dev_err(&imx274->client->dev,
+ "Failed to get VDIG supply: %ld\n",
+ PTR_ERR(imx274->vddl_supply));
+ return PTR_ERR(imx274->vddl_supply);
+ }
+
+ imx274->vddl_supply = NULL;
+ }
+
+ return 0;
+}
+
/**
* imx274_s_ctrl - This is used to set the imx274 V4L2 controls
* @ctrl: V4L2 control to be set
@@ -1836,6 +1979,19 @@ static int imx274_probe(struct i2c_client *client)
mutex_init(&imx274->lock);
+ imx274->xclk = devm_clk_get_optional(&client->dev, "xclk");
+ ret = clk_set_rate(imx274->xclk, IMX274_DEFAULT_CLK_FREQ);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to set xclk rate\n");
+ return ret;
+ }
+
+ 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 +2039,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;
@@ -1958,6 +2119,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 +2138,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
Hi Sowjanya,
your patch looks good, but I have a few comments about the coding style,
see below.
On 15/07/20 06:24, 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 <[email protected]>
> ---
> drivers/media/i2c/imx274.c | 170 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 167 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 55869ff..8a34c07 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
> */
> @@ -501,6 +504,10 @@ struct imx274_ctrls {
> * @frame_rate: V4L2 frame rate structure
> * @regmap: Pointer to regmap structure
> * @reset_gpio: Pointer to reset gpio
> + * @vana_supply: VANA analog supply regulator
> + * @vdig_supply: VDIG digital core supply regulator
> + * @vddl_supply: VDDL digital io supply regulator
> + * @xclk: system clock to imx274
> * @lock: Mutex structure
> * @mode: Parameters for the selected readout mode
> */
> @@ -514,6 +521,10 @@ struct stimx274 {
> struct v4l2_fract frame_interval;
> struct regmap *regmap;
> struct gpio_desc *reset_gpio;
> + struct regulator *vana_supply;
> + struct regulator *vdig_supply;
> + struct regulator *vddl_supply;
> + struct clk *xclk;
> struct mutex lock; /* mutex lock for operations */
> const struct imx274_mode *mode;
> };
> @@ -767,6 +778,138 @@ 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;
> +
> + ret = clk_prepare_enable(imx274->xclk);
> + if (ret) {
> + dev_err(&imx274->client->dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + if (imx274->vana_supply) {
> + ret = regulator_enable(imx274->vana_supply);
> + if (ret < 0) {
> + dev_err(&imx274->client->dev,
> + "Failed to enable VANA supply: %d\n", ret);
> + goto disable_clk;
> + }
> + }
> +
> + if (imx274->vdig_supply) {
> + ret = regulator_enable(imx274->vdig_supply);
> + if (ret < 0) {
> + dev_err(&imx274->client->dev,
> + "Failed to enable VDIG supply: %d\n", ret);
> + goto disable_vana_reg;
> + }
> + }
> +
> + if (imx274->vddl_supply) {
> + ret = regulator_enable(imx274->vddl_supply);
> + if (ret < 0) {
> + dev_err(&imx274->client->dev,
> + "Failed to enable VDDL supply: %d\n", ret);
> + goto disable_vdig_reg;
> + }
> + }
> +
> + usleep_range(1, 2);
> + imx274_reset(imx274, 1);
> +
> + return 0;
> +
> +disable_vdig_reg:
> + if (imx274->vdig_supply)
> + regulator_disable(imx274->vdig_supply);
> +disable_vana_reg:
> + if (imx274->vana_supply)
> + regulator_disable(imx274->vana_supply);
> +disable_clk:
> + clk_disable_unprepare(imx274->xclk);
> + 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);
> +
> + if (imx274->vddl_supply)
> + regulator_disable(imx274->vddl_supply);
> +
> + if (imx274->vdig_supply)
> + regulator_disable(imx274->vdig_supply);
> +
> + if (imx274->vana_supply)
> + regulator_disable(imx274->vana_supply);
> +
> + clk_disable_unprepare(imx274->xclk);
> +
> + return 0;
> +}
> +
> +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
> +{
To make code much more readable:
int err;
> + int i;
> +
> + imx274->vana_supply = devm_regulator_get_optional(dev, "VANA");
> + if (IS_ERR(imx274->vana_supply)) {
err = PTR_ERR(imx274->vana_supply)
and then use 'err' everywhere in place of PTR_ERR(imx274->vana_supply).
> + if (PTR_ERR(imx274->vana_supply) != -ENODEV) {
> + if (PTR_ERR(imx274->vana_supply) != -EPROBE_DEFER)
> + dev_err(&imx274->client->dev,
> + "Failed to get VANA supply: %ld\n",
> + PTR_ERR(imx274->vana_supply));
> + return PTR_ERR(imx274->vana_supply);
> + }
> +
> + imx274->vana_supply = NULL;
> + }
> +
> + imx274->vdig_supply = devm_regulator_get_optional(dev, "VDIG");
> + if (IS_ERR(imx274->vdig_supply)) {
> + if (PTR_ERR(imx274->vdig_supply) != -ENODEV) {
> + if (PTR_ERR(imx274->vdig_supply) != -EPROBE_DEFER)
> + dev_err(&imx274->client->dev,
> + "Failed to get VDIG supply: %ld\n",
> + PTR_ERR(imx274->vdig_supply));
> + return PTR_ERR(imx274->vdig_supply);
> + }
> +
> + imx274->vdig_supply = NULL;
> + }
> +
> + imx274->vddl_supply = devm_regulator_get_optional(dev, "VDDL");
> + if (IS_ERR(imx274->vddl_supply)) {
> + if (PTR_ERR(imx274->vddl_supply) != -ENODEV) {
> + if (PTR_ERR(imx274->vddl_supply) != -EPROBE_DEFER)
> + dev_err(&imx274->client->dev,
> + "Failed to get VDIG supply: %ld\n",
> + PTR_ERR(imx274->vddl_supply));
> + return PTR_ERR(imx274->vddl_supply);
> + }
> +
> + imx274->vddl_supply = NULL;
> + }
Here, as well as in imx274_power_on(), you have 3 identical sections for
the 3 regulators. Why not having an array of 3 elements in struct
stimx274 and then access them in a loop? You'll need also the names:
static const char* regulator_name[3] = { "VANA", "VDIG", "VDDL" };
> /**
> * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
> * @ctrl: V4L2 control to be set
> @@ -1836,6 +1979,19 @@ static int imx274_probe(struct i2c_client *client)
>
> mutex_init(&imx274->lock);
>
> + imx274->xclk = devm_clk_get_optional(&client->dev, "xclk");
> + ret = clk_set_rate(imx274->xclk, IMX274_DEFAULT_CLK_FREQ);
Nitpicking, but I'd move this line closer to the end of the function in
order to set the clock rate only if all the get operations succeeded.
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to set xclk rate\n");
> + return ret;
> + }
> +
> + 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 +2039,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;
> @@ -1958,6 +2119,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 +2138,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;
>
--
Luca
On 7/15/20 10:46 AM, Luca Ceresoli wrote:
> Hi Sowjanya,
>
> your patch looks good, but I have a few comments about the coding style,
> see below.
Thanks Luca. Will consider them for v2.
>
> On 15/07/20 06:24, 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 <[email protected]>
>> ---
>> drivers/media/i2c/imx274.c | 170 ++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 167 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 55869ff..8a34c07 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
>> */
>> @@ -501,6 +504,10 @@ struct imx274_ctrls {
>> * @frame_rate: V4L2 frame rate structure
>> * @regmap: Pointer to regmap structure
>> * @reset_gpio: Pointer to reset gpio
>> + * @vana_supply: VANA analog supply regulator
>> + * @vdig_supply: VDIG digital core supply regulator
>> + * @vddl_supply: VDDL digital io supply regulator
>> + * @xclk: system clock to imx274
>> * @lock: Mutex structure
>> * @mode: Parameters for the selected readout mode
>> */
>> @@ -514,6 +521,10 @@ struct stimx274 {
>> struct v4l2_fract frame_interval;
>> struct regmap *regmap;
>> struct gpio_desc *reset_gpio;
>> + struct regulator *vana_supply;
>> + struct regulator *vdig_supply;
>> + struct regulator *vddl_supply;
>> + struct clk *xclk;
>> struct mutex lock; /* mutex lock for operations */
>> const struct imx274_mode *mode;
>> };
>> @@ -767,6 +778,138 @@ 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;
>> +
>> + ret = clk_prepare_enable(imx274->xclk);
>> + if (ret) {
>> + dev_err(&imx274->client->dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + if (imx274->vana_supply) {
>> + ret = regulator_enable(imx274->vana_supply);
>> + if (ret < 0) {
>> + dev_err(&imx274->client->dev,
>> + "Failed to enable VANA supply: %d\n", ret);
>> + goto disable_clk;
>> + }
>> + }
>> +
>> + if (imx274->vdig_supply) {
>> + ret = regulator_enable(imx274->vdig_supply);
>> + if (ret < 0) {
>> + dev_err(&imx274->client->dev,
>> + "Failed to enable VDIG supply: %d\n", ret);
>> + goto disable_vana_reg;
>> + }
>> + }
>> +
>> + if (imx274->vddl_supply) {
>> + ret = regulator_enable(imx274->vddl_supply);
>> + if (ret < 0) {
>> + dev_err(&imx274->client->dev,
>> + "Failed to enable VDDL supply: %d\n", ret);
>> + goto disable_vdig_reg;
>> + }
>> + }
>> +
>> + usleep_range(1, 2);
>> + imx274_reset(imx274, 1);
>> +
>> + return 0;
>> +
>> +disable_vdig_reg:
>> + if (imx274->vdig_supply)
>> + regulator_disable(imx274->vdig_supply);
>> +disable_vana_reg:
>> + if (imx274->vana_supply)
>> + regulator_disable(imx274->vana_supply);
>> +disable_clk:
>> + clk_disable_unprepare(imx274->xclk);
>> + 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);
>> +
>> + if (imx274->vddl_supply)
>> + regulator_disable(imx274->vddl_supply);
>> +
>> + if (imx274->vdig_supply)
>> + regulator_disable(imx274->vdig_supply);
>> +
>> + if (imx274->vana_supply)
>> + regulator_disable(imx274->vana_supply);
>> +
>> + clk_disable_unprepare(imx274->xclk);
>> +
>> + return 0;
>> +}
>> +
>> +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274)
>> +{
> To make code much more readable:
>
> int err;
>
>> + int i;
>> +
>> + imx274->vana_supply = devm_regulator_get_optional(dev, "VANA");
>> + if (IS_ERR(imx274->vana_supply)) {
> err = PTR_ERR(imx274->vana_supply)
>
> and then use 'err' everywhere in place of PTR_ERR(imx274->vana_supply).
>
>> + if (PTR_ERR(imx274->vana_supply) != -ENODEV) {
>> + if (PTR_ERR(imx274->vana_supply) != -EPROBE_DEFER)
>> + dev_err(&imx274->client->dev,
>> + "Failed to get VANA supply: %ld\n",
>> + PTR_ERR(imx274->vana_supply));
>> + return PTR_ERR(imx274->vana_supply);
>> + }
>> +
>> + imx274->vana_supply = NULL;
>> + }
>> +
>> + imx274->vdig_supply = devm_regulator_get_optional(dev, "VDIG");
>> + if (IS_ERR(imx274->vdig_supply)) {
>> + if (PTR_ERR(imx274->vdig_supply) != -ENODEV) {
>> + if (PTR_ERR(imx274->vdig_supply) != -EPROBE_DEFER)
>> + dev_err(&imx274->client->dev,
>> + "Failed to get VDIG supply: %ld\n",
>> + PTR_ERR(imx274->vdig_supply));
>> + return PTR_ERR(imx274->vdig_supply);
>> + }
>> +
>> + imx274->vdig_supply = NULL;
>> + }
>> +
>> + imx274->vddl_supply = devm_regulator_get_optional(dev, "VDDL");
>> + if (IS_ERR(imx274->vddl_supply)) {
>> + if (PTR_ERR(imx274->vddl_supply) != -ENODEV) {
>> + if (PTR_ERR(imx274->vddl_supply) != -EPROBE_DEFER)
>> + dev_err(&imx274->client->dev,
>> + "Failed to get VDIG supply: %ld\n",
>> + PTR_ERR(imx274->vddl_supply));
>> + return PTR_ERR(imx274->vddl_supply);
>> + }
>> +
>> + imx274->vddl_supply = NULL;
>> + }
> Here, as well as in imx274_power_on(), you have 3 identical sections for
> the 3 regulators. Why not having an array of 3 elements in struct
> stimx274 and then access them in a loop? You'll need also the names:
>
> static const char* regulator_name[3] = { "VANA", "VDIG", "VDDL" };
>
>> /**
>> * imx274_s_ctrl - This is used to set the imx274 V4L2 controls
>> * @ctrl: V4L2 control to be set
>> @@ -1836,6 +1979,19 @@ static int imx274_probe(struct i2c_client *client)
>>
>> mutex_init(&imx274->lock);
>>
>> + imx274->xclk = devm_clk_get_optional(&client->dev, "xclk");
>> + ret = clk_set_rate(imx274->xclk, IMX274_DEFAULT_CLK_FREQ);
> Nitpicking, but I'd move this line closer to the end of the function in
> order to set the clock rate only if all the get operations succeeded.
>
>> + if (ret < 0) {
>> + dev_err(&client->dev, "Failed to set xclk rate\n");
>> + return ret;
>> + }
>> +
>> + 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 +2039,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;
>> @@ -1958,6 +2119,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 +2138,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;
>>
>