[PATCH 1/2] media: ov5645: Remove unneeded regulator_set_voltage()

[PATCH 1/2] media: ov5645: Remove unneeded regulator_set_voltage()

[Fabio Estevam]

There is no need to call regulator_set_voltage() for each regulator
that powers the camera.

The voltage value for each regulator should be retrieved from the
device tree, so remove the unneeded regulator_set_voltage().

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/media/i2c/ov5645.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 124c8df04633..4e302dc15177 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -34,10 +34,6 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#define OV5645_VOLTAGE_ANALOG               2800000
-#define OV5645_VOLTAGE_DIGITAL_CORE         1500000
-#define OV5645_VOLTAGE_DIGITAL_IO           1800000
-
 #define OV5645_SYSTEM_CTRL0		0x3008
 #define		OV5645_SYSTEM_CTRL0_START	0x02
 #define		OV5645_SYSTEM_CTRL0_STOP	0x42
@@ -1156,42 +1152,18 @@ static int ov5645_probe(struct i2c_client *client,
 		return PTR_ERR(ov5645->io_regulator);
 	}
 
-	ret = regulator_set_voltage(ov5645->io_regulator,
-				    OV5645_VOLTAGE_DIGITAL_IO,
-				    OV5645_VOLTAGE_DIGITAL_IO);
-	if (ret < 0) {
-		dev_err(dev, "cannot set io voltage\n");
-		return ret;
-	}
-
 	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
 	if (IS_ERR(ov5645->core_regulator)) {
 		dev_err(dev, "cannot get core regulator\n");
 		return PTR_ERR(ov5645->core_regulator);
 	}
 
-	ret = regulator_set_voltage(ov5645->core_regulator,
-				    OV5645_VOLTAGE_DIGITAL_CORE,
-				    OV5645_VOLTAGE_DIGITAL_CORE);
-	if (ret < 0) {
-		dev_err(dev, "cannot set core voltage\n");
-		return ret;
-	}
-
 	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
 	if (IS_ERR(ov5645->analog_regulator)) {
 		dev_err(dev, "cannot get analog regulator\n");
 		return PTR_ERR(ov5645->analog_regulator);
 	}
 
-	ret = regulator_set_voltage(ov5645->analog_regulator,
-				    OV5645_VOLTAGE_ANALOG,
-				    OV5645_VOLTAGE_ANALOG);
-	if (ret < 0) {
-		dev_err(dev, "cannot set analog voltage\n");
-		return ret;
-	}
-
 	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
 	if (IS_ERR(ov5645->enable_gpio)) {
 		dev_err(dev, "cannot get enable gpio\n");
-- 
2.17.1

[PATCH 2/2] media: ov5645: Use regulator_bulk() functions

[Fabio Estevam]

The code can be simplified by using the regulator_bulk() functions,
so switch to it.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/media/i2c/ov5645.c | 94 +++++++++-----------------------------
 1 file changed, 21 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 4e302dc15177..a37ae5d5ff12 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -58,6 +58,15 @@
 #define OV5645_SDE_SAT_U		0x5583
 #define OV5645_SDE_SAT_V		0x5584
 
+/* regulator supplies */
+static const char * const ov5645_supply_name[] = {
+	"vdddo", /* Digital I/O (1.8V) supply */
+	"vddd",  /* Digital Core (1.5V) supply */
+	"vdda",  /* Analog (2.8V) supply */
+};
+
+#define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -82,9 +91,7 @@ struct ov5645 {
 	struct v4l2_rect crop;
 	struct clk *xclk;
 
-	struct regulator *io_regulator;
-	struct regulator *core_regulator;
-	struct regulator *analog_regulator;
+	struct regulator_bulk_data supplies[OV5645_NUM_SUPPLIES];
 
 	const struct ov5645_mode_info *current_mode;
 
@@ -529,55 +536,6 @@ static const struct ov5645_mode_info ov5645_mode_info_data[] = {
 	},
 };
 
-static int ov5645_regulators_enable(struct ov5645 *ov5645)
-{
-	int ret;
-
-	ret = regulator_enable(ov5645->io_regulator);
-	if (ret < 0) {
-		dev_err(ov5645->dev, "set io voltage failed\n");
-		return ret;
-	}
-
-	ret = regulator_enable(ov5645->analog_regulator);
-	if (ret) {
-		dev_err(ov5645->dev, "set analog voltage failed\n");
-		goto err_disable_io;
-	}
-
-	ret = regulator_enable(ov5645->core_regulator);
-	if (ret) {
-		dev_err(ov5645->dev, "set core voltage failed\n");
-		goto err_disable_analog;
-	}
-
-	return 0;
-
-err_disable_analog:
-	regulator_disable(ov5645->analog_regulator);
-err_disable_io:
-	regulator_disable(ov5645->io_regulator);
-
-	return ret;
-}
-
-static void ov5645_regulators_disable(struct ov5645 *ov5645)
-{
-	int ret;
-
-	ret = regulator_disable(ov5645->core_regulator);
-	if (ret < 0)
-		dev_err(ov5645->dev, "core regulator disable failed\n");
-
-	ret = regulator_disable(ov5645->analog_regulator);
-	if (ret < 0)
-		dev_err(ov5645->dev, "analog regulator disable failed\n");
-
-	ret = regulator_disable(ov5645->io_regulator);
-	if (ret < 0)
-		dev_err(ov5645->dev, "io regulator disable failed\n");
-}
-
 static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
 {
 	u8 regbuf[3];
@@ -676,15 +634,14 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
 {
 	int ret;
 
-	ret = ov5645_regulators_enable(ov5645);
-	if (ret < 0) {
+	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = clk_prepare_enable(ov5645->xclk);
 	if (ret < 0) {
 		dev_err(ov5645->dev, "clk prepare enable failed\n");
-		ov5645_regulators_disable(ov5645);
+		regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
 		return ret;
 	}
 
@@ -704,7 +661,7 @@ static void ov5645_set_power_off(struct ov5645 *ov5645)
 	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
 	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
 	clk_disable_unprepare(ov5645->xclk);
-	ov5645_regulators_disable(ov5645);
+	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
 }
 
 static int ov5645_s_power(struct v4l2_subdev *sd, int on)
@@ -1089,6 +1046,7 @@ static int ov5645_probe(struct i2c_client *client,
 	struct device_node *endpoint;
 	struct ov5645 *ov5645;
 	u8 chip_id_high, chip_id_low;
+	unsigned int i;
 	u32 xclk_freq;
 	int ret;
 
@@ -1146,23 +1104,13 @@ static int ov5645_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ov5645->io_regulator = devm_regulator_get(dev, "vdddo");
-	if (IS_ERR(ov5645->io_regulator)) {
-		dev_err(dev, "cannot get io regulator\n");
-		return PTR_ERR(ov5645->io_regulator);
-	}
-
-	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
-	if (IS_ERR(ov5645->core_regulator)) {
-		dev_err(dev, "cannot get core regulator\n");
-		return PTR_ERR(ov5645->core_regulator);
-	}
+	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
+		ov5645->supplies[i].supply = ov5645_supply_name[i];
 
-	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
-	if (IS_ERR(ov5645->analog_regulator)) {
-		dev_err(dev, "cannot get analog regulator\n");
-		return PTR_ERR(ov5645->analog_regulator);
-	}
+	ret = devm_regulator_bulk_get(dev, OV5645_NUM_SUPPLIES,
+				      ov5645->supplies);
+	if (ret < 0)
+		return ret;
 
 	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
 	if (IS_ERR(ov5645->enable_gpio)) {
-- 
2.17.1

Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions

[Sakari Ailus]

Hi Fabio,

On Wed, Jun 26, 2019 at 08:56:14PM -0300, Fabio Estevam wrote:
> The code can be simplified by using the regulator_bulk() functions,
> so switch to it.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  drivers/media/i2c/ov5645.c | 94 +++++++++-----------------------------
>  1 file changed, 21 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 4e302dc15177..a37ae5d5ff12 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -58,6 +58,15 @@
>  #define OV5645_SDE_SAT_U		0x5583
>  #define OV5645_SDE_SAT_V		0x5584
>  
> +/* regulator supplies */
> +static const char * const ov5645_supply_name[] = {
> +	"vdddo", /* Digital I/O (1.8V) supply */
> +	"vddd",  /* Digital Core (1.5V) supply */
> +	"vdda",  /* Analog (2.8V) supply */
> +};
> +
> +#define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
> +
>  struct reg_value {
>  	u16 reg;
>  	u8 val;
> @@ -82,9 +91,7 @@ struct ov5645 {
>  	struct v4l2_rect crop;
>  	struct clk *xclk;
>  
> -	struct regulator *io_regulator;
> -	struct regulator *core_regulator;
> -	struct regulator *analog_regulator;
> +	struct regulator_bulk_data supplies[OV5645_NUM_SUPPLIES];
>  
>  	const struct ov5645_mode_info *current_mode;
>  
> @@ -529,55 +536,6 @@ static const struct ov5645_mode_info ov5645_mode_info_data[] = {
>  	},
>  };
>  
> -static int ov5645_regulators_enable(struct ov5645 *ov5645)
> -{
> -	int ret;
> -
> -	ret = regulator_enable(ov5645->io_regulator);
> -	if (ret < 0) {
> -		dev_err(ov5645->dev, "set io voltage failed\n");
> -		return ret;
> -	}
> -
> -	ret = regulator_enable(ov5645->analog_regulator);
> -	if (ret) {
> -		dev_err(ov5645->dev, "set analog voltage failed\n");
> -		goto err_disable_io;
> -	}
> -
> -	ret = regulator_enable(ov5645->core_regulator);
> -	if (ret) {
> -		dev_err(ov5645->dev, "set core voltage failed\n");
> -		goto err_disable_analog;
> -	}

This appears to change the order in which the regulators are enabled. Is
that intentional? Does the sensor support this order as well?

> -
> -	return 0;
> -
> -err_disable_analog:
> -	regulator_disable(ov5645->analog_regulator);
> -err_disable_io:
> -	regulator_disable(ov5645->io_regulator);
> -
> -	return ret;
> -}
> -
> -static void ov5645_regulators_disable(struct ov5645 *ov5645)
> -{
> -	int ret;
> -
> -	ret = regulator_disable(ov5645->core_regulator);
> -	if (ret < 0)
> -		dev_err(ov5645->dev, "core regulator disable failed\n");
> -
> -	ret = regulator_disable(ov5645->analog_regulator);
> -	if (ret < 0)
> -		dev_err(ov5645->dev, "analog regulator disable failed\n");
> -
> -	ret = regulator_disable(ov5645->io_regulator);
> -	if (ret < 0)
> -		dev_err(ov5645->dev, "io regulator disable failed\n");
> -}
> -
>  static int ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val)
>  {
>  	u8 regbuf[3];
> @@ -676,15 +634,14 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
>  {
>  	int ret;
>  
> -	ret = ov5645_regulators_enable(ov5645);
> -	if (ret < 0) {
> +	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = clk_prepare_enable(ov5645->xclk);
>  	if (ret < 0) {
>  		dev_err(ov5645->dev, "clk prepare enable failed\n");
> -		ov5645_regulators_disable(ov5645);
> +		regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
>  		return ret;
>  	}
>  
> @@ -704,7 +661,7 @@ static void ov5645_set_power_off(struct ov5645 *ov5645)
>  	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
>  	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
>  	clk_disable_unprepare(ov5645->xclk);
> -	ov5645_regulators_disable(ov5645);
> +	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
>  }
>  
>  static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> @@ -1089,6 +1046,7 @@ static int ov5645_probe(struct i2c_client *client,
>  	struct device_node *endpoint;
>  	struct ov5645 *ov5645;
>  	u8 chip_id_high, chip_id_low;
> +	unsigned int i;
>  	u32 xclk_freq;
>  	int ret;
>  
> @@ -1146,23 +1104,13 @@ static int ov5645_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ov5645->io_regulator = devm_regulator_get(dev, "vdddo");
> -	if (IS_ERR(ov5645->io_regulator)) {
> -		dev_err(dev, "cannot get io regulator\n");
> -		return PTR_ERR(ov5645->io_regulator);
> -	}
> -
> -	ov5645->core_regulator = devm_regulator_get(dev, "vddd");
> -	if (IS_ERR(ov5645->core_regulator)) {
> -		dev_err(dev, "cannot get core regulator\n");
> -		return PTR_ERR(ov5645->core_regulator);
> -	}
> +	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
> +		ov5645->supplies[i].supply = ov5645_supply_name[i];
>  
> -	ov5645->analog_regulator = devm_regulator_get(dev, "vdda");
> -	if (IS_ERR(ov5645->analog_regulator)) {
> -		dev_err(dev, "cannot get analog regulator\n");
> -		return PTR_ERR(ov5645->analog_regulator);
> -	}
> +	ret = devm_regulator_bulk_get(dev, OV5645_NUM_SUPPLIES,
> +				      ov5645->supplies);
> +	if (ret < 0)
> +		return ret;
>  
>  	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
>  	if (IS_ERR(ov5645->enable_gpio)) {

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/2] media: ov5645: Remove unneeded regulator_set_voltage()

[Sakari Ailus]

Hi Fabio,

On Wed, Jun 26, 2019 at 08:56:13PM -0300, Fabio Estevam wrote:
> There is no need to call regulator_set_voltage() for each regulator
> that powers the camera.
> 
> The voltage value for each regulator should be retrieved from the
> device tree, so remove the unneeded regulator_set_voltage().
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

Thanks for the patchset.

I wonder if there are chances of this breaking something as the driver did
not depend on the voltage being set correctly in DT. But we don't seem to
have any users in DT source shipped with the kernel. So I guess I'll merge
these at least if no-one complains (see the comment on the 2nd patch, too).

No other sensor (I²C) driver appears to be touching the regulator voltage
either.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions

[Fabio Estevam]

Hi Sakari,

On Thu, Jun 27, 2019 at 1:27 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:

> This appears to change the order in which the regulators are enabled. Is
> that intentional? Does the sensor support this order as well?

Good catch! I have sent a v2 that preserves the regulator enable order.

Thanks

Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions

[Todor Tomov]

Hello Fabio,

On Thu, Jun 27, 2019 at 9:24 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Sakari,
>
> On Thu, Jun 27, 2019 at 1:27 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> > This appears to change the order in which the regulators are enabled. Is
> > that intentional? Does the sensor support this order as well?
>
> Good catch! I have sent a v2 that preserves the regulator enable order.

Thank you for the patch.
The question about using the regulator_bulk API seems to come
regularly from time to time.
This has been discussed on [1] and I believe the conclusion has been
that the regulator_bulk API doesn't guarantee the order of enabling of
the regulators. So in theory this is possible to cause problems in
some corner cases and we have agreed to leave the order explicit.

Best regards,
Todor

[1] https://patchwork.linuxtv.org/patch/36918/

P.S. Sorry for previous emails, hopefully my client is set up correctly now.

Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions

[Fabio Estevam]

Hi Todor,

On Mon, Jul 1, 2019 at 6:29 PM Todor Tomov <todor.too@gmail.com> wrote:

> Thank you for the patch.
> The question about using the regulator_bulk API seems to come
> regularly from time to time.
> This has been discussed on [1] and I believe the conclusion has been
> that the regulator_bulk API doesn't guarantee the order of enabling of
> the regulators. So in theory this is possible to cause problems in
> some corner cases and we have agreed to leave the order explicit.

Thanks for the explanation.

I was not aware that the regulator bulk API did not guarantee the
order of enabling regulators, so this patch can be discarded.

I think we probably need to get rid of regulator bulk API in the
ov5640 driver as well.

Will prepare a patch for ov5640 soon.

Thanks