The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew <[email protected]>
---
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
[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;
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)) {
+ ret = PTR_ERR(data->en_supply);
+ 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;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
--
1.7.9.5
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> > ---
> > 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.
On 03/05/2013 07:18 PM, Alex Courbot wrote:
> On 03/06/2013 08:51 AM, Andrew Chew wrote:
>> The backlight enable regulator is specified in the device tree node for
>> backlight.
>> diff --git a/include/linux/pwm_backlight.h
>> 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.
...
> 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.
A driver that requires a regulator always requires that regulator. If a
particular board doesn't have SW control over the power source, you're
supposed to provide a dummy (fixed) regulator so that the driver doesn't
care about the difference.
On 03/06/2013 11:41 AM, Andrew Chew wrote:
>>> 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.
You're right, consumers can share regulators and the calls to
enable/disable need to be balanced. Also there is no way to check the
intensity of the backlight prior to the change to detect a transition,
so I guess your approach is indeed the most appropriate here.
Alex.
On 03/06/2013 01:20 PM, Stephen Warren wrote:
> On 03/05/2013 07:18 PM, Alex Courbot wrote:
>> On 03/06/2013 08:51 AM, Andrew Chew wrote:
>>> The backlight enable regulator is specified in the device tree node for
>>> backlight.
>
>>> diff --git a/include/linux/pwm_backlight.h
>
>>> 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.
> ...
>> 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.
>
> A driver that requires a regulator always requires that regulator. If a
> particular board doesn't have SW control over the power source, you're
> supposed to provide a dummy (fixed) regulator so that the driver doesn't
> care about the difference.
That's good to know, thanks. So does this mean that Andrew should make
the enable regulator mandatory and update current users to provide a
dummy one?
Alex.
On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
> On 03/06/2013 11:41 AM, Andrew Chew wrote:
> >>> 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.
>
> You're right, consumers can share regulators and the calls to
> enable/disable need to be balanced. Also there is no way to check
> the intensity of the backlight prior to the change to detect a
> transition, so I guess your approach is indeed the most appropriate
> here.
I think the right thing to do here is just enable the regulator when
the pwm-backlight driver needs it. If it is shared with other devices
they'll have to do the same and the reference counting should only
disable the regulator when there are no users.
Tracking this via platform data won't work because platform data is
statically defined at compile time. So if indeed there was another user
of the regulator it enable/disable the regulator at any time and your
en_supply_enabled would be wrong.
Thierry
On Wed, Mar 06, 2013 at 01:56:40PM +0900, Alex Courbot wrote:
> On 03/06/2013 01:20 PM, Stephen Warren wrote:
> >On 03/05/2013 07:18 PM, Alex Courbot wrote:
> >>On 03/06/2013 08:51 AM, Andrew Chew wrote:
> >>>The backlight enable regulator is specified in the device tree node for
> >>>backlight.
> >
> >>>diff --git a/include/linux/pwm_backlight.h
> >
> >>> 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.
> >...
> >>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.
> >
> >A driver that requires a regulator always requires that regulator. If a
> >particular board doesn't have SW control over the power source, you're
> >supposed to provide a dummy (fixed) regulator so that the driver doesn't
> >care about the difference.
>
> That's good to know, thanks. So does this mean that Andrew should
> make the enable regulator mandatory and update current users to
> provide a dummy one?
That would be the right thing to do. I was planning to move all users of
pwm-backlight to use PWM lookup tables as well at some point (in order
to get rid of the legacy pwm_request() calls), so maybe we can do both
in one go.
Thierry
On 03/06/2013 04:00 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
>> On 03/06/2013 11:41 AM, Andrew Chew wrote:
>>>>> 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.
>>
>> You're right, consumers can share regulators and the calls to
>> enable/disable need to be balanced. Also there is no way to check
>> the intensity of the backlight prior to the change to detect a
>> transition, so I guess your approach is indeed the most appropriate
>> here.
>
> I think the right thing to do here is just enable the regulator when
> the pwm-backlight driver needs it. If it is shared with other devices
> they'll have to do the same and the reference counting should only
> disable the regulator when there are no users.
>
> Tracking this via platform data won't work because platform data is
> statically defined at compile time. So if indeed there was another user
> of the regulator it enable/disable the regulator at any time and your
> en_supply_enabled would be wrong.
Oh wait. I thought regulator_enable/disable calls needed to be balanced,
is that not the case? So every consumer receives a different regulator
handle in case of a shared regulator, which becomes disabled if all
handles are disabled? In that case yes, we won't have to bother about a
status variable here and balancing calls. Sorry for the confusion.
Alex.
On Wed, Mar 06, 2013 at 05:37:42PM +0900, Alex Courbot wrote:
> On 03/06/2013 04:00 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
> >>On 03/06/2013 11:41 AM, Andrew Chew wrote:
> >>>>> 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.
> >>
> >>You're right, consumers can share regulators and the calls to
> >>enable/disable need to be balanced. Also there is no way to check
> >>the intensity of the backlight prior to the change to detect a
> >>transition, so I guess your approach is indeed the most appropriate
> >>here.
> >
> >I think the right thing to do here is just enable the regulator when
> >the pwm-backlight driver needs it. If it is shared with other devices
> >they'll have to do the same and the reference counting should only
> >disable the regulator when there are no users.
> >
> >Tracking this via platform data won't work because platform data is
> >statically defined at compile time. So if indeed there was another user
> >of the regulator it enable/disable the regulator at any time and your
> >en_supply_enabled would be wrong.
>
> Oh wait. I thought regulator_enable/disable calls needed to be
> balanced, is that not the case? So every consumer receives a
> different regulator handle in case of a shared regulator, which
> becomes disabled if all handles are disabled? In that case yes, we
> won't have to bother about a status variable here and balancing
> calls. Sorry for the confusion.
I think they'll receive the exact same handle to the regulator. Calls
will remain balanced as long as they are balanced in each user.
Thierry