soc_camera: Add the ability to bind regulators to soc_camedra devices

PATCH Superseded Alberto Panizzo 2 participants 4 messages Nov 30, 2010 – Dec 1, 2010 Media (V4L/DVB)

Share & tools

Alberto Panizzo

author

2010-11-30 17:13 UTC #0 lore raw

PATCH v2 1/1 soc_camera: Add the ability to bind regulators to soc_camedra devices

In certain machines, camera devices are supplied directly by a number of regulators. This patch add the ability to drive these regulators directly by the soc_camera driver.

Signed-off-by Alberto Panizzo <[email protected]>

---

v2 changes:
It is used the more standard regulator_bulk API, thanks to Mark Brown for pointing this.

 drivers/media/video/soc_camera.c |   73 +++++++++++++++++++++++++++-----------
 include/media/soc_camera.h       |    5 +++
 2 files changed, 57 insertions(+), 21 deletions(-)

drivers/media/video/soc_camera.c +52 ~~-21~~

index 43848a7..f1c2094 100644
@@ -43,6 +43,41 @@ static LIST_HEAD(hosts);
 static LIST_HEAD(devices);
 static DEFINE_MUTEX(list_lock);		/* Protects the list of hosts */
 
+static int soc_camera_power_set(struct soc_camera_device *icd,
+				struct soc_camera_link *icl,
+				int power_on)
+{
+	int ret = 0;
+
+	if (power_on) {
+		ret = regulator_bulk_enable(icl->num_regulators,
+					    icl->regulators);
+	} else {
+		ret = regulator_bulk_disable(icl->num_regulators,
+					     icl->regulators);
+	}
+	if (ret < 0) {
+		dev_err(icd->pdev, "Cannot %s regulators",
+			power_on ? "ENABLE" : "DISABLE");
+		goto err;
+	}
+
+	if (icl->power) {
+		ret = icl->power(icd->pdev, power_on);
+		if (ret < 0) {
+			dev_err(icd->pdev,
+				"Platform failed to power %s the camera.\n",
+				power_on ? "ON" : "OFF");
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	return ret;
+}
+
 const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc)
 {

@@ -375,11 +410,9 @@ static int soc_camera_open(struct file *file)
 			},
 		};
 
-		if (icl->power) {
-			ret = icl->power(icd->pdev, 1);
-			if (ret < 0)
-				goto epower;
-		}
+		ret = soc_camera_power_set(icd, icl, 1);
+		if (ret < 0)
+			goto epower;
 
 		/* The camera could have been already on, try to reset */
 		if (icl->reset)

@@ -425,8 +458,7 @@ esfmt:
 eresume:
 	ici->ops->remove(icd);
 eiciadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	icd->use_count--;
 	mutex_unlock(&icd->video_lock);

@@ -450,8 +482,7 @@ static int soc_camera_close(struct file *file)
 
 		ici->ops->remove(icd);
 
-		if (icl->power)
-			icl->power(icd->pdev, 0);
+		soc_camera_power_set(icd, icl, 0);
 	}
 
 	if (icd->streamer == file)

@@ -941,14 +972,14 @@ static int soc_camera_probe(struct device *dev)
 
 	dev_info(dev, "Probing %s\n", dev_name(dev));
 
-	if (icl->power) {
-		ret = icl->power(icd->pdev, 1);
-		if (ret < 0) {
-			dev_err(dev,
-				"Platform failed to power-on the camera.\n");
-			goto epower;
-		}
-	}
+	ret = regulator_bulk_get(icd->pdev, icl->num_regulators,
+				 icl->regulators);
+	if (ret)
+		goto epower;
+
+	ret = soc_camera_power_set(icd, icl, 1);
+	if (ret < 0)
+		goto epower;
 
 	/* The camera could have been already on, try to reset */
 	if (icl->reset)

@@ -1021,8 +1052,7 @@ static int soc_camera_probe(struct device *dev)
 
 	ici->ops->remove(icd);
 
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 
 	mutex_unlock(&icd->video_lock);

@@ -1044,8 +1074,7 @@ eadddev:
 evdc:
 	ici->ops->remove(icd);
 eadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	return ret;
 }

@@ -1081,6 +1110,8 @@ static int soc_camera_remove(struct device *dev)
 	}
 	soc_camera_free_user_formats(icd);
 
+	regulator_bulk_free(icl->num_regulators, icl->regulators);
+
 	return 0;
 }

include/media/soc_camera.h +5 -0

index 86e3631..3e6b903 100644
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
+#include <linux/regulator/consumer.h>
 #include <linux/videodev2.h>
 #include <media/videobuf-core.h>
 #include <media/v4l2-device.h>

@@ -108,6 +109,10 @@ struct soc_camera_link {
 	const char *module_name;
 	void *priv;
 
+	/* Optional regulators that have to be managed on power on/off events */
+	struct regulator_bulk_data *regulators;
+	int num_regulators;
+
 	/*
 	 * For non-I2C devices platform platform has to provide methods to
 	 * add a device to the system and to remove

Signature · 1 line

1.6.3.3

Guennadi Liakhovetski

2010-12-01 17:26 UTC #1 lore raw

drivers/media/video/soc_camera.c

Quoted 19 lines · Alberto Panizzo

In certain machines, camera devices are supplied directly by a number of regulators. This patch add the ability to drive these regulators directly by the soc_camera driver.
Signed-off-by Alberto Panizzo <[email protected]>
---
v2 changes:
It is used the more standard regulator_bulk API, thanks to Mark Brown for pointing this.
 drivers/media/video/soc_camera.c |   73 +++++++++++++++++++++++++++-----------
 include/media/soc_camera.h       |    5 +++
 2 files changed, 57 insertions(+), 21 deletions(-)
drivers/media/video/soc_camera.c +0 -0
index 43848a7..f1c2094 100644

Have to

#include <linux/regulator/consumer.h>

here

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 50

Quoted 9 lines

drivers/media/video/soc_camera.c +5 -0

@@ -43,6 +43,41 @@ static LIST_HEAD(hosts);
 static LIST_HEAD(devices);
 static DEFINE_MUTEX(list_lock);		/* Protects the list of hosts */
 
+static int soc_camera_power_set(struct soc_camera_device *icd,
+				struct soc_camera_link *icl,
+				int power_on)
+{
+	int ret = 0;

"= 0" unneeded.

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 58

Quoted 8 lines

drivers/media/video/soc_camera.c +8 -0

+
+	if (power_on) {
+		ret = regulator_bulk_enable(icl->num_regulators,
+					    icl->regulators);
+	} else {
+		ret = regulator_bulk_disable(icl->num_regulators,
+					     icl->regulators);
+	}

superfluous braces

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 61

Quoted 3 lines

drivers/media/video/soc_camera.c +3 -0

+	if (ret < 0) {
+		dev_err(icd->pdev, "Cannot %s regulators",
+			power_on ? "ENABLE" : "DISABLE");

why capitals?

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 62

Quoted 1 line

drivers/media/video/soc_camera.c +1 -0
+		goto err;

just return ret

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 70

Quoted 8 lines

drivers/media/video/soc_camera.c +8 -0

+	}
+
+	if (icl->power) {
+		ret = icl->power(icd->pdev, power_on);
+		if (ret < 0) {
+			dev_err(icd->pdev,
+				"Platform failed to power %s the camera.\n",
+				power_on ? "ON" : "OFF");

why capitals?

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 71

Quoted 1 line

drivers/media/video/soc_camera.c +1 -0
+			goto err;

just return ret, although, if switching on failed, and the platform us also using regulators, don't you want to turn them off? Still I would do this here inline without a goto, but that's already a matter of taste, so, if you prefer, in this case a goto would be justified.

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 78

Quoted 7 lines

drivers/media/video/soc_camera.c +7 -0
+		}
+	}
+
+	return 0;
+
+err:
+	return ret;

with the above, this might go.

Quoted 45 lines

drivers/media/video/soc_camera.c +7 ~~-10~~

+}
+
 const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc)
 {

@@ -375,11 +410,9 @@ static int soc_camera_open(struct file *file)
 			},
 		};
 
-		if (icl->power) {
-			ret = icl->power(icd->pdev, 1);
-			if (ret < 0)
-				goto epower;
-		}
+		ret = soc_camera_power_set(icd, icl, 1);
+		if (ret < 0)
+			goto epower;
 
 		/* The camera could have been already on, try to reset */
 		if (icl->reset)

@@ -425,8 +458,7 @@ esfmt:
 eresume:
 	ici->ops->remove(icd);
 eiciadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	icd->use_count--;
 	mutex_unlock(&icd->video_lock);

@@ -450,8 +482,7 @@ static int soc_camera_close(struct file *file)
 
 		ici->ops->remove(icd);
 
-		if (icl->power)
-			icl->power(icd->pdev, 0);
+		soc_camera_power_set(icd, icl, 0);
 	}
 
 	if (icd->streamer == file)

@@ -941,14 +972,14 @@ static int soc_camera_probe(struct device *dev)
 
 	dev_info(dev, "Probing %s\n", dev_name(dev));
 
-	if (icl->power) {

drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 977

Quoted 10 lines

drivers/media/video/soc_camera.c +3 -7

-		ret = icl->power(icd->pdev, 1);
-		if (ret < 0) {
-			dev_err(dev,
-				"Platform failed to power-on the camera.\n");
-			goto epower;
-		}
-	}
+	ret = regulator_bulk_get(icd->pdev, icl->num_regulators,
+				 icl->regulators);
+	if (ret)

"if (ret < 0)" for consistency, please

drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 982

Quoted 5 lines

drivers/media/video/soc_camera.c +5 -0

+		goto epower;
+
+	ret = soc_camera_power_set(icd, icl, 1);
+	if (ret < 0)
+		goto epower;

You need a new label for this error - you also have to free regulators, if this fails.

Quoted 37 lines

drivers/media/video/soc_camera.c +4 -4

 	/* The camera could have been already on, try to reset */
 	if (icl->reset)

@@ -1021,8 +1052,7 @@ static int soc_camera_probe(struct device *dev)
 
 	ici->ops->remove(icd);
 
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 
 	mutex_unlock(&icd->video_lock);

@@ -1044,8 +1074,7 @@ eadddev:
 evdc:
 	ici->ops->remove(icd);
 eadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	return ret;
 }

@@ -1081,6 +1110,8 @@ static int soc_camera_remove(struct device *dev)
 	}
 	soc_camera_free_user_formats(icd);
 
+	regulator_bulk_free(icl->num_regulators, icl->regulators);
+
 	return 0;
 }

include/media/soc_camera.h +0 -0

index 86e3631..3e6b903 100644

include/media/soc_camera.h line 18

Quoted 4 lines

include/media/soc_camera.h +1 -0

@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
+#include <linux/regulator/consumer.h>

No need to include the header here, just declare

struct regulator_bulk_data;

Quoted 16 lines

include/media/soc_camera.h +4 -0

 #include <linux/videodev2.h>
 #include <media/videobuf-core.h>
 #include <media/v4l2-device.h>

@@ -108,6 +109,10 @@ struct soc_camera_link {
 	const char *module_name;
 	void *priv;
 
+	/* Optional regulators that have to be managed on power on/off events */
+	struct regulator_bulk_data *regulators;
+	int num_regulators;
+
 	/*
 	 * For non-I2C devices platform platform has to provide methods to
 	 * add a device to the system and to remove

Signature · 1 line

1.6.3.3

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Alberto Panizzo

author

2010-12-01 19:23 UTC #2 lore raw

Quoted 51 lines · Guennadi Liakhovetski

drivers/media/video/soc_camera.c
Quoted 19 lines · Alberto Panizzo
In certain machines, camera devices are supplied directly by a number of regulators. This patch add the ability to drive these regulators directly by the soc_camera driver.
Signed-off-by Alberto Panizzo <[email protected]>
---
v2 changes:
It is used the more standard regulator_bulk API, thanks to Mark Brown for pointing this.
 drivers/media/video/soc_camera.c |   73 +++++++++++++++++++++++++++-----------
 include/media/soc_camera.h       |    5 +++
 2 files changed, 57 insertions(+), 21 deletions(-)
drivers/media/video/soc_camera.c +0 -0
index 43848a7..f1c2094 100644
Have to
#include <linux/regulator/consumer.h>
here
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 50
Quoted 9 lines
drivers/media/video/soc_camera.c +5 -0
@@ -43,6 +43,41 @@ static LIST_HEAD(hosts);
 static LIST_HEAD(devices);
 static DEFINE_MUTEX(list_lock);		/* Protects the list of hosts */
 
+static int soc_camera_power_set(struct soc_camera_device *icd,
+				struct soc_camera_link *icl,
+				int power_on)
+{
+	int ret = 0;
"= 0" unneeded.
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 58
Quoted 8 lines
drivers/media/video/soc_camera.c +8 -0
+
+	if (power_on) {
+		ret = regulator_bulk_enable(icl->num_regulators,
+					    icl->regulators);
+	} else {
+		ret = regulator_bulk_disable(icl->num_regulators,
+					     icl->regulators);
+	}
superfluous braces

Quoted 5 lines

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 61
Quoted 3 lines
drivers/media/video/soc_camera.c +3 -0
+	if (ret < 0) {
+		dev_err(icd->pdev, "Cannot %s regulators",
+			power_on ? "ENABLE" : "DISABLE");
why capitals?

To distinguish between hardcoded words and produced ones. Otherwise I prefer to separate the two messages and put them in the two branches of the if.

What do you think about?

Quoted 16 lines

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 62
Quoted 2 lines
drivers/media/video/soc_camera.c +1 -0
+		goto err;
just return ret
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 70
Quoted 8 lines
drivers/media/video/soc_camera.c +8 -0
+	}
+
+	if (icl->power) {
+		ret = icl->power(icd->pdev, power_on);
+		if (ret < 0) {
+			dev_err(icd->pdev,
+				"Platform failed to power %s the camera.\n",
+				power_on ? "ON" : "OFF");
why capitals?

Quoted 6 lines

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 71
Quoted 1 line
drivers/media/video/soc_camera.c +1 -0
+			goto err;
just return ret, although, if switching on failed, and the platform us also using regulators, don't you want to turn them off? Still I would do this here inline without a goto, but that's already a matter of taste, so, if you prefer, in this case a goto would be justified.

regulator_bulk is an all or none API, so if an error happen enabling some regulators, it automatically disable the previous enabled ones.

Quoted 55 lines

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 78

Quoted 8 lines

drivers/media/video/soc_camera.c +7 -0
+		}
+	}
+
+	return 0;
+
+err:
+	return ret;

with the above, this might go.

Quoted 44 lines

drivers/media/video/soc_camera.c +7 -9

+}
+
 const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc)
 {

@@ -375,11 +410,9 @@ static int soc_camera_open(struct file *file)
 			},
 		};
 
-		if (icl->power) {
-			ret = icl->power(icd->pdev, 1);
-			if (ret < 0)
-				goto epower;
-		}
+		ret = soc_camera_power_set(icd, icl, 1);
+		if (ret < 0)
+			goto epower;
 
 		/* The camera could have been already on, try to reset */
 		if (icl->reset)

@@ -425,8 +458,7 @@ esfmt:
 eresume:
 	ici->ops->remove(icd);
 eiciadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	icd->use_count--;
 	mutex_unlock(&icd->video_lock);

@@ -450,8 +482,7 @@ static int soc_camera_close(struct file *file)
 
 		ici->ops->remove(icd);
 
-		if (icl->power)
-			icl->power(icd->pdev, 0);
+		soc_camera_power_set(icd, icl, 0);
 	}
 
 	if (icd->streamer == file)

@@ -941,14 +972,14 @@ static int soc_camera_probe(struct device *dev)
 
 	dev_info(dev, "Probing %s\n", dev_name(dev));

Quoted 13 lines

drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 977
Quoted 11 lines
drivers/media/video/soc_camera.c +3 -8
-	if (icl->power) {
-		ret = icl->power(icd->pdev, 1);
-		if (ret < 0) {
-			dev_err(dev,
-				"Platform failed to power-on the camera.\n");
-			goto epower;
-		}
-	}
+	ret = regulator_bulk_get(icd->pdev, icl->num_regulators,
+				 icl->regulators);
+	if (ret)
"if (ret < 0)" for consistency, please

regulator_bulk_get return 0 on success..

Quoted 9 lines

drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 982
Quoted 6 lines
drivers/media/video/soc_camera.c +5 -0
+		goto epower;
+
+	ret = soc_camera_power_set(icd, icl, 1);
+	if (ret < 0)
+		goto epower;
You need a new label for this error - you also have to free regulators, if this fails.

The same as before regulator_bulk_get is an all or none call that free regulators itself on error.

Maybe return ret instead of goto epower?

Quoted 45 lines

Quoted 38 lines

drivers/media/video/soc_camera.c +4 -4

 	/* The camera could have been already on, try to reset */
 	if (icl->reset)

@@ -1021,8 +1052,7 @@ static int soc_camera_probe(struct device *dev)
 
 	ici->ops->remove(icd);
 
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 
 	mutex_unlock(&icd->video_lock);

@@ -1044,8 +1074,7 @@ eadddev:
 evdc:
 	ici->ops->remove(icd);
 eadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	return ret;
 }

@@ -1081,6 +1110,8 @@ static int soc_camera_remove(struct device *dev)
 	}
 	soc_camera_free_user_formats(icd);
 
+	regulator_bulk_free(icl->num_regulators, icl->regulators);
+
 	return 0;
 }

include/media/soc_camera.h +0 -0

index 86e3631..3e6b903 100644

include/media/soc_camera.h line 18

Quoted 4 lines

include/media/soc_camera.h +1 -0

@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
+#include <linux/regulator/consumer.h>

No need to include the header here, just declare

Quoted 1 line

struct regulator_bulk_data;

Ok, this is a coding style tip that I didn't know.

Thanks to you Guennadi!

Signature · 4 lines

Alberto!

        Be Persistent!
                - Greg Kroah-Hartman (FOSDEM 2010)

Guennadi Liakhovetski

2010-12-01 20:11 UTC #3 lore raw

Quoted 58 lines · Alberto Panizzo

Quoted 51 lines · Guennadi Liakhovetski
drivers/media/video/soc_camera.c
Quoted 19 lines · Alberto Panizzo
In certain machines, camera devices are supplied directly by a number of regulators. This patch add the ability to drive these regulators directly by the soc_camera driver.
Signed-off-by Alberto Panizzo <[email protected]>
---
v2 changes:
It is used the more standard regulator_bulk API, thanks to Mark Brown for pointing this.
 drivers/media/video/soc_camera.c |   73 +++++++++++++++++++++++++++-----------
 include/media/soc_camera.h       |    5 +++
 2 files changed, 57 insertions(+), 21 deletions(-)
drivers/media/video/soc_camera.c +0 -0
index 43848a7..f1c2094 100644
Have to
#include <linux/regulator/consumer.h>
here
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 50
Quoted 9 lines
drivers/media/video/soc_camera.c +5 -0
@@ -43,6 +43,41 @@ static LIST_HEAD(hosts);
 static LIST_HEAD(devices);
 static DEFINE_MUTEX(list_lock);		/* Protects the list of hosts */
 
+static int soc_camera_power_set(struct soc_camera_device *icd,
+				struct soc_camera_link *icl,
+				int power_on)
+{
+	int ret = 0;
"= 0" unneeded.
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 58
Quoted 8 lines
drivers/media/video/soc_camera.c +8 -0
+
+	if (power_on) {
+		ret = regulator_bulk_enable(icl->num_regulators,
+					    icl->regulators);
+	} else {
+		ret = regulator_bulk_disable(icl->num_regulators,
+					     icl->regulators);
+	}
superfluous braces
Quoted 5 lines
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 61
Quoted 3 lines
drivers/media/video/soc_camera.c +3 -0
+	if (ret < 0) {
+		dev_err(icd->pdev, "Cannot %s regulators",
+			power_on ? "ENABLE" : "DISABLE");
why capitals?

Quoted 5 lines

To distinguish between hardcoded words and produced ones. Otherwise I prefer to separate the two messages and put them in the two branches of the if.
What do you think about?

Well, up to you, really, but just think, when you see in dmesg

Cannot ENABLE regulators

Cannot enable regulators

which of the two will look more naturally and would you really care, whether enable / disable were hardcoded or produced?

Quoted 22 lines

Quoted 15 lines
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 62
Quoted 1 line
drivers/media/video/soc_camera.c +1 -0
+		goto err;
just return ret
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 70
Quoted 8 lines
drivers/media/video/soc_camera.c +8 -0
+	}
+
+	if (icl->power) {
+		ret = icl->power(icd->pdev, power_on);
+		if (ret < 0) {
+			dev_err(icd->pdev,
+				"Platform failed to power %s the camera.\n",
+				power_on ? "ON" : "OFF");
why capitals?
Quoted 6 lines
drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 71
Quoted 1 line
drivers/media/video/soc_camera.c +1 -0
+			goto err;
just return ret, although, if switching on failed, and the platform us also using regulators, don't you want to turn them off? Still I would do this here inline without a goto, but that's already a matter of taste, so, if you prefer, in this case a goto would be justified.

Quoted 2 lines

regulator_bulk is an all or none API, so if an error happen enabling some regulators, it automatically disable the previous enabled ones.

Sorry, that's not what I mean. I mean, if both are used - regulators and the .power() callback. Regulators succeeded, but .power(1) failed. In this case you exit without disabling regulators again.

Quoted 68 lines

Quoted 54 lines

drivers/media/video/soc_camera.c static LIST_HEAD(hosts); · line 78

Quoted 7 lines

drivers/media/video/soc_camera.c +7 -0
+		}
+	}
+
+	return 0;
+
+err:
+	return ret;

with the above, this might go.

Quoted 44 lines

drivers/media/video/soc_camera.c +7 -9

+}
+
 const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
 	struct soc_camera_device *icd, unsigned int fourcc)
 {

@@ -375,11 +410,9 @@ static int soc_camera_open(struct file *file)
 			},
 		};
 
-		if (icl->power) {
-			ret = icl->power(icd->pdev, 1);
-			if (ret < 0)
-				goto epower;
-		}
+		ret = soc_camera_power_set(icd, icl, 1);
+		if (ret < 0)
+			goto epower;
 
 		/* The camera could have been already on, try to reset */
 		if (icl->reset)

@@ -425,8 +458,7 @@ esfmt:
 eresume:
 	ici->ops->remove(icd);
 eiciadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	icd->use_count--;
 	mutex_unlock(&icd->video_lock);

@@ -450,8 +482,7 @@ static int soc_camera_close(struct file *file)
 
 		ici->ops->remove(icd);
 
-		if (icl->power)
-			icl->power(icd->pdev, 0);
+		soc_camera_power_set(icd, icl, 0);
 	}
 
 	if (icd->streamer == file)

@@ -941,14 +972,14 @@ static int soc_camera_probe(struct device *dev)
 
 	dev_info(dev, "Probing %s\n", dev_name(dev));

Quoted 13 lines

drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 977
Quoted 11 lines
drivers/media/video/soc_camera.c +3 -8
-	if (icl->power) {
-		ret = icl->power(icd->pdev, 1);
-		if (ret < 0) {
-			dev_err(dev,
-				"Platform failed to power-on the camera.\n");
-			goto epower;
-		}
-	}
+	ret = regulator_bulk_get(icd->pdev, icl->num_regulators,
+				 icl->regulators);
+	if (ret)
"if (ret < 0)" for consistency, please

Quoted 1 line

regulator_bulk_get return 0 on success..

I know, just as well as most other calls do. So, in this case both tests "if (ret)" or "if (ret < 0)" are equivalent, but since the rest of the file uses the "< 0" form, I'd prefer to be consistent.

Quoted 11 lines

Quoted 8 lines
drivers/media/video/soc_camera.c static int soc_camera_probe(struct device *dev) · line 982
Quoted 5 lines
drivers/media/video/soc_camera.c +5 -0
+		goto epower;
+
+	ret = soc_camera_power_set(icd, icl, 1);
+	if (ret < 0)
+		goto epower;
You need a new label for this error - you also have to free regulators, if this fails.
The same as before regulator_bulk_get is an all or none call that free regulators itself on error.

No. This is not for failing regulator_bulk_get(), this is for failing soc_camera_power_set(icd, icl, 1), in which case probing is aborted with an error, so, you have to free all resources, including regulators.

Quoted 1 line

Maybe return ret instead of goto epower?

You could do for the very first failure, yes. But there's no need to change this.

Quoted 43 lines

Quoted 36 lines

drivers/media/video/soc_camera.c +4 -4

 	/* The camera could have been already on, try to reset */
 	if (icl->reset)

@@ -1021,8 +1052,7 @@ static int soc_camera_probe(struct device *dev)
 
 	ici->ops->remove(icd);
 
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 
 	mutex_unlock(&icd->video_lock);

@@ -1044,8 +1074,7 @@ eadddev:
 evdc:
 	ici->ops->remove(icd);
 eadd:
-	if (icl->power)
-		icl->power(icd->pdev, 0);
+	soc_camera_power_set(icd, icl, 0);
 epower:
 	return ret;
 }

@@ -1081,6 +1110,8 @@ static int soc_camera_remove(struct device *dev)
 	}
 	soc_camera_free_user_formats(icd);
 
+	regulator_bulk_free(icl->num_regulators, icl->regulators);
+
 	return 0;
 }

include/media/soc_camera.h +0 -0

index 86e3631..3e6b903 100644

include/media/soc_camera.h line 18

Quoted 4 lines

include/media/soc_camera.h +1 -0

@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
+#include <linux/regulator/consumer.h>

No need to include the header here, just declare

Quoted 5 lines

Quoted 1 line
struct regulator_bulk_data;
Ok, this is a coding style tip that I didn't know.
Thanks to you Guennadi!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/