lkml.org 
[lkml]   [2013]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
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@nvidia.com>
> ---
> 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.



\
 
 \ /
  Last update: 2013-03-06 04:01    [W:0.078 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site