media: ov5645: Remove unneeded regulator_set_voltage()
Subsystems (2)
PATCH 1/2 media: ov5645: Remove unneeded regulator_set_voltage()
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().
--- drivers/media/i2c/ov5645.c | 28 ---------------------------- 1 file changed, 28 deletions(-)
drivers/media/i2c/ov5645.c +0 -28
@@ -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" );
Signature · 1 line
2.17.1
PATCH 2/2 media: ov5645: Use regulator_bulk() functions
The code can be simplified by using the regulator_bulk() functions, so switch to it.
--- drivers/media/i2c/ov5645.c | 94 +++++++++----------------------------- 1 file changed, 21 insertions(+), 73 deletions(-)
drivers/media/i2c/ov5645.c +21 -73
@@ -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)) {
Signature · 1 line
2.17.1
Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions
Hi Fabio,
Quoted 54 lines · Fabio Estevam
The code can be simplified by using the
regulator_bulk()functions, so switch to it.Signed-off-by Fabio Estevam <[email protected]>--- drivers/media/i2c/ov5645.c | 94 +++++++++----------------------------- 1 file changed, 21 insertions(+), 73 deletions(-)drivers/media/i2c/ov5645.c +10
-14@@ -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);
drivers/media/i2c/ov5645.c static const struct ov5645_mode_info ov5645_mode_info_data[] = { · line 552
Quoted 10 lines
drivers/media/i2c/ov5645.c +0
-10-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?
Quoted 97 lines
drivers/media/i2c/ov5645.c +11
-49- -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)) {
Signature · 3 lines
Regards, Sakari Ailus
Re: [PATCH 2/2] media: ov5645: Use regulator_bulk() functions
Hi Sakari,
Quoted 2 lines · Sakari Ailus
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
Hello Fabio,
Quoted 9 lines · Fabio Estevam
Hi Sakari,
Quoted 2 lines · Sakari Ailus
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
Hi Todor,
Quoted 7 lines · Todor Tomov
Thank you for the patch.
The question about using theregulator_bulkAPI seems to come regularly from time to time.
This has been discussed on [1] and I believe the conclusion has been that theregulator_bulkAPI 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
Hi Fabio,
Quoted 7 lines · 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 <[email protected]>
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.
Signature · 3 lines
Regards, Sakari Ailus