Thanks, Alex! Makes sense to me. There's one comment I'm not sure about,
though, described inline.
> 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 <ac...@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.
Are you sure this works? I'm concerned about the (bizarre and unlikely) case
where this supply is shared with another driver, so I use en_supply_enabled
to track the state of the supply such that I can ignore that case.
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
