Re: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator

From: Alex Courbot
Date: Tue Mar 05 2013 - 21:18:50 EST

On 03/06/2013 08:51 AM, Andrew Chew wrote:
The backlight enable regulator is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew <achew@xxxxxxxxxx>
---
Applied all recommendations from Thierry Reding and Alex Courbot, including
making pwm_bl take an optional regulator instead of a GPIO, which solves
the platform data issue (platform data will default the regulator to NULL,
which is the right thing).

.../bindings/video/backlight/pwm-backlight.txt | 1 +
drivers/video/backlight/pwm_bl.c | 53 +++++++++++++++++---
include/linux/pwm_backlight.h | 1 +
3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..e0bccd30 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,7 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+ - en-supply: phandle to the regulator device tree node

You may want to specify what this regulator does - namely, that is enables the BL. May I also suggest to rename it to "enable-supply" since the other properties do not use abbreviations.

[0]: Documentation/devicetree/bindings/pwm/pwm.txt

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c4da5e2 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include <linux/pwm.h>
#include <linux/pwm_backlight.h>
#include <linux/slab.h>
+#include <linux/regulator/consumer.h>

struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator *en_supply;
+ bool en_supply_enabled;

Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled? It would also ensure the driver performs correctly no matter what the initial state of the regulator is.

unsigned int period;
unsigned int lth_brightness;
unsigned int *levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void (*exit)(struct device *);
};

+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+
+ pwm_enable(pb->pwm);
+
+ if (pb->en_supply && !pb->en_supply_enabled) {
+ if (regulator_enable(pb->en_supply) != 0)
+ dev_warn(&bl->dev, "Failed to enable regulator");
+ else
+ pb->en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+
+ if (pb->en_supply && pb->en_supply_enabled) {
+ if (regulator_disable(pb->en_supply) != 0)
+ dev_warn(&bl->dev, "Failed to disable regulator");
+ else
+ pb->en_supply_enabled = false;
+ }
+
+ pwm_disable(pb->pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -52,7 +83,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)

if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;

@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_enable(bl);
}

if (pb->notify_after)
@@ -146,10 +177,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
}

/*
- * TODO: Most users of this driver use a number of GPIOs to control
- * backlight power. Support for specifying these needs to be
- * added.
+ * If "en-supply" is present, use that regulator to enable the
+ * backlight. If a GPIO is used to enable the backlight, make
+ * a fixed regulator with that particular GPIO and use that
+ * regulator for "en-supply".
*/
+ data->en_supply = devm_regulator_get(dev, "en");
+ if (IS_ERR_OR_NULL(data->en_supply)) {

devm_regulator_get() is performed at the wrong place, but I will come back to this later. As a sidenote though: you should use IS_ERR here. regulator_get() will never return NULL - also using IS_ERR_OR_NULL is strongly discouraged and it will probably disappear soon anyway:

https://patchwork.kernel.org/patch/1953211/

+ ret = PTR_ERR(data->en_supply);

... and this is the reason why you should use IS_ERR: because in the (impossible anyway) error case where regulator_get() returns NULL, you will return 0 (success).

+ data->en_supply = NULL;
+ return ret;
+ }

return 0;
}
@@ -207,6 +245,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
} else
max = data->max_brightness;

+ pb->en_supply = data->en_supply;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
@@ -268,7 +307,7 @@ static int pwm_backlight_remove(struct platform_device *pdev)

backlight_device_unregister(bl);
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
if (pb->exit)
pb->exit(&pdev->dev);
return 0;
@@ -283,7 +322,7 @@ static int pwm_backlight_suspend(struct device *dev)
if (pb->notify)
pb->notify(pb->dev, 0);
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..330512b 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,7 @@

struct platform_pwm_backlight_data {
int pwm_id;
+ struct regulator *en_supply;

You should not have this here. Platform data is supposed to provide the necessary information for the driver to resolve the resource - not the resource itself.

Instead machines that rely on platform data will associate the right regulator to the backlight device in their board code, through an instance of the regulator_consumer_supply structure (see include/linux/regulator/machine.h), and submit it to the regulator framework. Thus it is enough for you to just perform a call to devm_regulator_get() in the probe function, and the regulator framework will resolve the right regulator through the device tree or the provided platform data. I.e. you don't have to worry about whether you are using the DT or platform data here.

There is one catch though: in case you don't want to use a regulator, and thus have none defined, regulator_get() will return -EPROBE_DEFER, so you cannot distinguish between "no regulator needed" and "supplier not ready yet" and your driver will always *require* a regulator. So at the end of the day you might still need a "use_enable_regulator" in the platform data to explicitly ask for probe() to look for it. This variable would also be set by parse_dt() if the "enable-supply" property exists.

But this somehow kills the purpose of using a regulator here, since part of the motivation was to avoid this boolean variable. Maybe Thierry has a better idea.

I like the general idea of this patch however - with this and a couple of always-on regulators we should be able to enable the panels of some Tegra boards until the CDF gets merged. It won't be optimal from a power point of view, but at least we will finally see something. :)

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/