[PATCH 1/4] OMAP: DSS2: Remove suspicous and unused TAAL regulator API usage

The TAAL driver contains some regulator support which is currently unused
(the code is there but the one panel supported by the driver doesn't have
any regulators provided). This code mostly looks like an open coded
version of the regulator core bulk API.

The only additional feature is that a voltage range can be set once when
the device is opened, though this is never varied at runtime. The general
expectation is that if the device is not actively managing the voltage of
the device (eg, doing DVFS) then any configuration will be done using the
constraints rather than by drivers, saving them code and ensuring that
they work well with systems where the voltage is not configurable.

If systems are added needing regulator support this can be added back in,
though it should be based on core features rather than open coding things.

Signed-off-by: Mark Brown <redacted>
---
 drivers/video/omap2/displays/panel-taal.c |   80 -----------------------------
 1 file changed, 80 deletions(-)

diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
index 0f21fa5..72d6307 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c

@@ -30,7 +30,6 @@
 #include <linux/gpio.h>
 #include <linux/workqueue.h>
 #include <linux/slab.h>
-#include <linux/regulator/consumer.h>
 #include <linux/mutex.h>
 
 #include <video/omapdss.h>

@@ -55,73 +54,6 @@ static int _taal_enable_te(struct omap_dss_device *dssdev, bool enable);
 
 static int taal_panel_reset(struct omap_dss_device *dssdev);
 
-struct panel_regulator {
-	struct regulator *regulator;
-	const char *name;
-	int min_uV;
-	int max_uV;
-};
-
-static void free_regulators(struct panel_regulator *regulators, int n)
-{
-	int i;
-
-	for (i = 0; i < n; i++) {
-		/* disable/put in reverse order */
-		regulator_disable(regulators[n - i - 1].regulator);
-		regulator_put(regulators[n - i - 1].regulator);
-	}
-}
-
-static int init_regulators(struct omap_dss_device *dssdev,
-			struct panel_regulator *regulators, int n)
-{
-	int r, i, v;
-
-	for (i = 0; i < n; i++) {
-		struct regulator *reg;
-
-		reg = regulator_get(&dssdev->dev, regulators[i].name);
-		if (IS_ERR(reg)) {
-			dev_err(&dssdev->dev, "failed to get regulator %s\n",
-				regulators[i].name);
-			r = PTR_ERR(reg);
-			goto err;
-		}
-
-		/* FIXME: better handling of fixed vs. variable regulators */
-		v = regulator_get_voltage(reg);
-		if (v < regulators[i].min_uV || v > regulators[i].max_uV) {
-			r = regulator_set_voltage(reg, regulators[i].min_uV,
-						regulators[i].max_uV);
-			if (r) {
-				dev_err(&dssdev->dev,
-					"failed to set regulator %s voltage\n",
-					regulators[i].name);
-				regulator_put(reg);
-				goto err;
-			}
-		}
-
-		r = regulator_enable(reg);
-		if (r) {
-			dev_err(&dssdev->dev, "failed to enable regulator %s\n",
-				regulators[i].name);
-			regulator_put(reg);
-			goto err;
-		}
-
-		regulators[i].regulator = reg;
-	}
-
-	return 0;
-
-err:
-	free_regulators(regulators, i);
-
-	return r;
-}
-
 /**
  * struct panel_config - panel configuration
  * @name: panel name

@@ -150,8 +82,6 @@ struct panel_config {
 		unsigned int low;
 	} reset_sequence;
 
-	struct panel_regulator *regulators;
-	int num_regulators;
 };
 
 enum {

@@ -977,11 +907,6 @@ static int taal_probe(struct omap_dss_device *dssdev)
 
 	atomic_set(&td->do_update, 0);
 
-	r = init_regulators(dssdev, panel_config->regulators,
-			panel_config->num_regulators);
-	if (r)
-		goto err_reg;
-
 	td->workqueue = create_singlethread_workqueue("taal_esd");
 	if (td->workqueue = NULL) {
 		dev_err(&dssdev->dev, "can't create ESD workqueue\n");

@@ -1075,8 +1000,6 @@ err_gpio:
 err_bl:
 	destroy_workqueue(td->workqueue);
 err_wq:
-	free_regulators(panel_config->regulators, panel_config->num_regulators);
-err_reg:
 	kfree(td);
 err:
 	return r;

@@ -1113,9 +1036,6 @@ static void __exit taal_remove(struct omap_dss_device *dssdev)
 	/* reset, to be sure that the panel is in a valid state */
 	taal_hw_reset(dssdev);
 
-	free_regulators(td->panel_config->regulators,
-			td->panel_config->num_regulators);
-
 	kfree(td);
 }
 

-- 
1.7.10

It is possible for regulator_enable() to fail and if it does fail that's
generally a bad sign for anything we try to do with the hardware afterwards
so check for and immediately return an error if regulator_enable() fails.

Signed-off-by: Mark Brown <redacted>
---
 drivers/video/omap2/dss/venc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 9c3daf7..abfbd4a 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c

@@ -443,7 +443,9 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 	dispc_set_digit_size(dssdev->panel.timings.x_res,
 			dssdev->panel.timings.y_res/2);
 
-	regulator_enable(venc.vdda_dac_reg);
+	r = regulator_enable(venc.vdda_dac_reg);
+	if (r)
+		goto err;
 
 	if (dssdev->platform_enable)
 		dssdev->platform_enable(dssdev);

-- 
1.7.10

It is possible for regulator_enable() to fail and if it does fail that's
generally a bad sign for anything we try to do with the hardware afterwards
so check for and immediately return an error if regulator_enable() fails.

Signed-off-by: Mark Brown <redacted>
Acked-by: Grazvydas Ignotas <redacted>
---
 drivers/video/omap2/displays/panel-tpo-td043mtea1.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
index 32f3fcd..74c6b87 100644
--- a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
+++ b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c

@@ -272,11 +272,14 @@ static const struct omap_video_timings tpo_td043_timings = {
 static int tpo_td043_power_on(struct tpo_td043_device *tpo_td043)
 {
 	int nreset_gpio = tpo_td043->nreset_gpio;
+	int r;
 
 	if (tpo_td043->powered_on)
 		return 0;
 
-	regulator_enable(tpo_td043->vcc_reg);
+	r = regulator_enable(tpo_td043->vcc_reg);
+	if (r != 0)
+		return r;
 
 	/* wait for regulator to stabilize */
 	msleep(160);

-- 
1.7.10

Since any power on stabilisation delay for the supply itself should be
taken care of transparently by the regulator API when the regulator is
enabled the additional delay that the TPO-TD03MTEA1 driver adds after
that returned should be due to the requirements of the device itself
rather than the supply (the delay is also suspicously long for one for
a regulator to ramp). Correct the comment to avoid misleading people
taking this code as a reference.

Signed-off-by: Mark Brown <redacted>
Acked-by: Grazvydas Ignotas <redacted>
---
 drivers/video/omap2/displays/panel-tpo-td043mtea1.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
index 74c6b87..987cb84 100644
--- a/drivers/video/omap2/displays/panel-tpo-td043mtea1.c
+++ b/drivers/video/omap2/displays/panel-tpo-td043mtea1.c

@@ -281,7 +281,7 @@ static int tpo_td043_power_on(struct tpo_td043_device *tpo_td043)
 	if (r != 0)
 		return r;
 
-	/* wait for regulator to stabilize */
+	/* wait for panel to stabilize */
 	msleep(160);
 
 	if (gpio_is_valid(nreset_gpio))

-- 
1.7.10

Hi,

On Thu, 2012-05-03 at 11:57 +0100, Mark Brown wrote:

The TAAL driver contains some regulator support which is currently unused
(the code is there but the one panel supported by the driver doesn't have
any regulators provided). This code mostly looks like an open coded
version of the regulator core bulk API.

The only additional feature is that a voltage range can be set once when
the device is opened, though this is never varied at runtime. The general
expectation is that if the device is not actively managing the voltage of
the device (eg, doing DVFS) then any configuration will be done using the
constraints rather than by drivers, saving them code and ensuring that
they work well with systems where the voltage is not configurable.

If systems are added needing regulator support this can be added back in,
though it should be based on core features rather than open coding things.

I've already applied this and the three other patches that you sent in
March to my omapdss tree. Have there been any changes?

 Tomi

On Thu, May 03, 2012 at 04:11:00PM +0300, Tomi Valkeinen wrote:

I've already applied this and the three other patches that you sent in
March to my omapdss tree. Have there been any changes?

No, it's a resend - if you've applied these changes they're not showing
up in -next.

On Thu, 2012-05-03 at 14:21 +0100, Mark Brown wrote:

On Thu, May 03, 2012 at 04:11:00PM +0300, Tomi Valkeinen wrote:

quoted

I've already applied this and the three other patches that you sent in
March to my omapdss tree. Have there been any changes?

No, it's a resend - if you've applied these changes they're not showing
up in -next.

Yes, I seem to have forgotten to update my for-next branch. I'll do it
right away.

 Tomi

On Thu, May 03, 2012 at 04:23:27PM +0300, Tomi Valkeinen wrote:

On Thu, 2012-05-03 at 14:21 +0100, Mark Brown wrote:

quoted

On Thu, May 03, 2012 at 04:11:00PM +0300, Tomi Valkeinen wrote:

quoted

quoted

I've already applied this and the three other patches that you sent in
March to my omapdss tree. Have there been any changes?

quoted

No, it's a resend - if you've applied these changes they're not showing
up in -next.

Yes, I seem to have forgotten to update my for-next branch. I'll do it
right away.

Great, thanks!