Patchwork [RFC,7/7] pwm-backlight: Add rudimentary device-tree support

login
register
mail settings
Submitter Thierry Reding
Date Dec. 20, 2011, 10:32 a.m.
Message ID <1324377138-32129-8-git-send-email-thierry.reding@avionic-design.de>
Download mbox | patch
Permalink /patch/132386/
State New, archived
Headers show

Comments

Thierry Reding - Dec. 20, 2011, 10:32 a.m.
This commit adds very basic support for device-tree probing. Currently,
only a PWM and maximum and default brightness values can be specified.
Enabling or disabling backlight power via GPIOs is not yet supported.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../bindings/video/backlight/pwm-backlight         |   16 +++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |   70 ++++++++++++++++++-
 3 files changed, 83 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight
Stephen Warren - Dec. 20, 2011, 11:33 p.m.
Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This commit adds very basic support for device-tree probing. Currently,
> only a PWM and maximum and default brightness values can be specified.
> Enabling or disabling backlight power via GPIOs is not yet supported.

> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight

> +Required properties:
> +  - compatible: "pwm-backlight"
> +  - default-brightness: the default brightness setting
> +  - max-brightness: the maximum brightness setting

What are the units of those two properties? Percentage seems like a
reasonable choice, although that's not what the patch implements.

> +  - pwm: OF device-tree PWM specification
> +
> +Example:
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		default-brightness = <224>;
> +		max-brightness = <255>;
> +		pwm = <&pwm 0 5000000>;
> +	};

This may be fine, but I'm not sure that representing the backlight as a
standalone object is correct; I wonder if you want to represent a complete
LCD display complex, including backlight, various GPIOs, and other display
properties, all in the one node? That said, I suppose you could easily
layer this as follows:

reg: regulator {
    // GPIO regulator
};

bl: backlight {
    compatible = "pwm-backlight";
    default-brightness = <224>;
    max-brightness = <255>;
    pwm = <&pwm 0 5000000>;
    power-supply = <&reg>;
};

lcd@x {
    backlight = <&bl>;
    ...
};

so this probably is OK.

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
...
> +static int pwm_backlight_parse_dt(struct device *dev,
> +				  struct platform_pwm_backlight_data *data)
...
> +	/*
> +	 * TODO: Most users of this driver use a number of GPIOs to control
> +	 *       backlight power. Support for specifying these needs to be
> +	 *       added.
> +	 */

At least for the power GPIO, this should probably modeled as a GPIO-based
fixed voltage regulator. Are there other GPIOs that are directly related
to a backlight rather than an LCD complex?
Thierry Reding - Dec. 21, 2011, 9:32 a.m.
* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
[...]
> > +  - default-brightness: the default brightness setting
> > +  - max-brightness: the maximum brightness setting
> 
> What are the units of those two properties? Percentage seems like a
> reasonable choice, although that's not what the patch implements.

That's just the way the pwm-backlight driver works. Typically the maximum
brightness is set to 255. I think you can set these values to pretty much
anything, and the driver will convert the brightness set via sysfs to the
range from 0 to max-brightness.

> > +Example:
> > +
> > +	backlight {
> > +		compatible = "pwm-backlight";
> > +		default-brightness = <224>;
> > +		max-brightness = <255>;
> > +		pwm = <&pwm 0 5000000>;
> > +	};
> 
> This may be fine, but I'm not sure that representing the backlight as a
> standalone object is correct;

I don't think there really is any other way because we need the device node in
order to have the corresponding platform device instantiated.

> I wonder if you want to represent a complete
> LCD display complex, including backlight, various GPIOs, and other display
> properties, all in the one node? That said, I suppose you could easily
> layer this as follows:
> 
> reg: regulator {
>     // GPIO regulator
> };
> 
> bl: backlight {
>     compatible = "pwm-backlight";
>     default-brightness = <224>;
>     max-brightness = <255>;
>     pwm = <&pwm 0 5000000>;
>     power-supply = <&reg>;
> };
> 
> lcd@x {
>     backlight = <&bl>;
>     ...
> };
> 
> so this probably is OK.

This looks pretty reasonable. I actually like it. I don't think there's any
code to resolve the &bl reference from the lcd driver yet, but that should be
rather easy to do.

> > +	/*
> > +	 * TODO: Most users of this driver use a number of GPIOs to control
> > +	 *       backlight power. Support for specifying these needs to be
> > +	 *       added.
> > +	 */
> 
> At least for the power GPIO, this should probably modeled as a GPIO-based
> fixed voltage regulator. Are there other GPIOs that are directly related
> to a backlight rather than an LCD complex?

Currently some platforms seem to use more than a single GPIO for the power.
PXA/Magician has two, depending on the brightness. Viper for example takes a
shortcut and controls both the backlight power and LCD enable from the
pwm-backlight callbacks. I guess if/when those machines are converted, they
can use a complete LCD complex as you described.

Thierry
Stephen Warren - Dec. 21, 2011, 6:20 p.m.
Thierry Reding wrote at Wednesday, December 21, 2011 2:33 AM:
> * Stephen Warren wrote:
> > Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> [...]
> > > +  - default-brightness: the default brightness setting
> > > +  - max-brightness: the maximum brightness setting
> >
> > What are the units of those two properties? Percentage seems like a
> > reasonable choice, although that's not what the patch implements.
> 
> That's just the way the pwm-backlight driver works. Typically the maximum
> brightness is set to 255. I think you can set these values to pretty much
> anything, and the driver will convert the brightness set via sysfs to the
> range from 0 to max-brightness.

It sounds like this numbering scheme is some SW abstraction then and not
directly related to HW. It's questionable that this should be represented
in DT, although if it's not I'm not sure where else it could be.

I think from a pure HW perspective, you need to represent:
* PWM period. nS is fine here.
* Minimum PWM duty cycle. nS or percentage is probably fine here. I don't
  know which is more likely to be specified in data sheets.
  (This is missing in the current patch, but supported in pwm_bl.c, so
  I assume there's a need for this, and hence it should be added to DT)
* Maximum PWM duty cycle. nS or percentage is probably fine here. I don't
  know which is more likely to be specified in data sheets.
  (I think this is assumed to be 100% duty cycle by pwm_bl.c, so perhaps
  there isn't a need to represent this in DT?)

Now, the Linux PWM driver then needs to know a range to map those min/max
HW duty cycles to. These are the values where it's unclear to me if/how
DT should represent them. It looks like the SW min value is always 0, and
the SW max value is what max-brightness is in the patch. Is there any
particular reason that max-brightness has to be a particular value, or
even defined in the DT at all? Could we hard-code it to 255 in the DT
parsing code, and not store it in the DT at all; would anything break
if we did that?

If we do need to represent it in DT, given it's a Linux-specific value,
perhaps the property name should have a "linux," or "linux-pwm-bl,"
prefix?

Thinking some more, perhaps the concept of "number of distinct useful
brightness steps" is a reasonable pure HW concept. If we rename max-
brightness to something representing that concept, we can still use the
value as max_brightness in the driver (well, subtract 1) and everyone's
happy? Perhaps "num-brightness-levels"?

If we store a "default brightness" in the DT, perhaps we should represent
it in the same way as the min/max PWM duty cycle values, and convert it
to the 0..max_brighness range when parsing the DT. I'm not sure what we'd
do if the selected value didn't align with a value obtainable using one
of the "num-brightness-steps" though. I guess that implies that the default
should be an integer step ID (as it is in your patch), not a duty cycle?
Mitch Bradley - Dec. 21, 2011, 7:04 p.m.
On 12/21/2011 8:20 AM, Stephen Warren wrote:
> Thierry Reding wrote at Wednesday, December 21, 2011 2:33 AM:
>> * Stephen Warren wrote:
>>> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
>> [...]
>>>> +  - default-brightness: the default brightness setting
>>>> +  - max-brightness: the maximum brightness setting
>>>
>>> What are the units of those two properties? Percentage seems like a
>>> reasonable choice, although that's not what the patch implements.
>>
>> That's just the way the pwm-backlight driver works. Typically the maximum
>> brightness is set to 255. I think you can set these values to pretty much
>> anything, and the driver will convert the brightness set via sysfs to the
>> range from 0 to max-brightness.
>
> It sounds like this numbering scheme is some SW abstraction then and not
> directly related to HW. It's questionable that this should be represented
> in DT, although if it's not I'm not sure where else it could be.
>
> I think from a pure HW perspective, you need to represent:
> * PWM period. nS is fine here.
> * Minimum PWM duty cycle. nS or percentage is probably fine here. I don't
>    know which is more likely to be specified in data sheets.
>    (This is missing in the current patch, but supported in pwm_bl.c, so
>    I assume there's a need for this, and hence it should be added to DT)
> * Maximum PWM duty cycle. nS or percentage is probably fine here. I don't
>    know which is more likely to be specified in data sheets.
>    (I think this is assumed to be 100% duty cycle by pwm_bl.c, so perhaps
>    there isn't a need to represent this in DT?)
>
> Now, the Linux PWM driver then needs to know a range to map those min/max
> HW duty cycles to. These are the values where it's unclear to me if/how
> DT should represent them. It looks like the SW min value is always 0, and
> the SW max value is what max-brightness is in the patch. Is there any
> particular reason that max-brightness has to be a particular value, or
> even defined in the DT at all? Could we hard-code it to 255 in the DT
> parsing code, and not store it in the DT at all; would anything break
> if we did that?
>
> If we do need to represent it in DT, given it's a Linux-specific value,
> perhaps the property name should have a "linux," or "linux-pwm-bl,"
> prefix?
>
> Thinking some more, perhaps the concept of "number of distinct useful
> brightness steps" is a reasonable pure HW concept.

On the PWM-controlled backlights that I have used, there is no such 
quantization at the hardware level.  The light (LED or fluorescent tube) 
sees an analog signal, typically a low-pass-filtered PWM waveform, and 
responds accordingly.  Human factors may establish a limit on the number 
of useful distinct levels, but there is no way to determine that from a 
data sheet, and thus no definitive hardware-defined value to assign to a 
property.

The objective hardware parameters are often

a) clock frequency choices for the PWM input
b) counter choices for base period
c) counter choices for on phase

There is usually wide latitude for choosing (a) and (b).  Depending on 
the analog filtering and the response characteristics of the light 
source, some choices may result in flickering.  So it would be 
reasonable to report frequency and base period settings that, on the 
hardware in question, result in a reasonable number of flicker-free 
brightness steps.

To further complicate matters, the relationship between PWM duty cycle 
and perceived brightness is usually nonlinear, so equally-spaced duty 
cycle percentages might not result in the best perceived brightness ramp.

One useful way to describe a given hardware system would be to have 
properties reporting values that work well for that hardware.  You would 
need to report a good base clock frequency, a good base period, and an 
array of N values that give a good perceived brightness ramp.

The backlight control on my PC laptop has 16 levels, 0 to 15.  That 
seems adequate to me.

> If we rename max-
> brightness to something representing that concept, we can still use the
> value as max_brightness in the driver (well, subtract 1) and everyone's
> happy? Perhaps "num-brightness-levels"?
>
> If we store a "default brightness" in the DT, perhaps we should represent
> it in the same way as the min/max PWM duty cycle values, and convert it
> to the 0..max_brighness range when parsing the DT. I'm not sure what we'd
> do if the selected value didn't align with a value obtainable using one
> of the "num-brightness-steps" though. I guess that implies that the default
> should be an integer step ID (as it is in your patch), not a duty cycle?
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - Dec. 22, 2011, 7:45 a.m.
* Mitch Bradley wrote:
> To further complicate matters, the relationship between PWM duty
> cycle and perceived brightness is usually nonlinear, so
> equally-spaced duty cycle percentages might not result in the best
> perceived brightness ramp.

Yes, I've seen that on numerous devices as well.

> One useful way to describe a given hardware system would be to have
> properties reporting values that work well for that hardware.  You
> would need to report a good base clock frequency, a good base
> period, and an array of N values that give a good perceived
> brightness ramp.

I'm not sure I quite understand what you mean by base period. Would the
base period not be simply the inverse of the frequency? Or should the base
period be the "step size" to multiply each element of the brightness levels
with?

> The backlight control on my PC laptop has 16 levels, 0 to 15.  That
> seems adequate to me.

I have a laptop that uses 8 level, 0 to 7. Combining that with the above it
should be easy to represent in the DT:

	bl: backlight {
		pwms = <&pwm 0 5000000>;
		base-period = <...>;
		brightness-levels = <...>;
	};

However, that would entail some major modifications to the pwm-backlight
driver to make it compute the actual duty cycle based on the array of
brightness levels.

This also raises a more general question: in a lot of drivers the DT binding
is used to provide an alternative source for platform data. With that
assumption, is it still reasonable to describe data in DT is such a different
way from the platform data? I mean in this particular case, there will be no
way to convert the DT data to the corresponding platform data because there
simply is no correspondence.

Thierry

Patch

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
new file mode 100644
index 0000000..ce65280
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
@@ -0,0 +1,16 @@ 
+pwm-backlight bindings
+
+Required properties:
+  - compatible: "pwm-backlight"
+  - default-brightness: the default brightness setting
+  - max-brightness: the maximum brightness setting
+  - pwm: OF device-tree PWM specification
+
+Example:
+
+	backlight {
+		compatible = "pwm-backlight";
+		default-brightness = <224>;
+		max-brightness = <255>;
+		pwm = <&pwm 0 5000000>;
+	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 278aeaa..51c3642 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -233,7 +233,7 @@  config BACKLIGHT_CARILLO_RANCH
 
 config BACKLIGHT_PWM
 	tristate "Generic PWM based Backlight Driver"
-	depends on HAVE_PWM
+	depends on HAVE_PWM || PWM
 	help
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8b5b2a4..742934c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -17,6 +17,7 @@ 
 #include <linux/fb.h>
 #include <linux/backlight.h>
 #include <linux/err.h>
+#include <linux/of_pwm.h>
 #include <linux/pwm.h>
 #include <linux/pwm_backlight.h>
 #include <linux/slab.h>
@@ -83,17 +84,77 @@  static const struct backlight_ops pwm_backlight_ops = {
 	.check_fb	= pwm_backlight_check_fb,
 };
 
+#ifdef CONFIG_OF
+static int pwm_backlight_parse_dt(struct device *dev,
+				  struct platform_pwm_backlight_data *data)
+{
+	struct device_node *node = dev->of_node;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	memset(data, 0, sizeof(*data));
+
+	ret = of_get_named_pwm(node, "pwm", 0, &data->pwm_period_ns);
+	if (ret < 0)
+		return ret;
+
+	data->pwm_id = ret;
+
+	ret = of_property_read_u32(node, "default-brightness", &value);
+	if (ret < 0)
+		return ret;
+
+	data->dft_brightness = value;
+
+	ret = of_property_read_u32(node, "max-brightness", &value);
+	if (ret < 0)
+		return ret;
+
+	data->max_brightness = value;
+
+	/*
+	 * TODO: Most users of this driver use a number of GPIOs to control
+	 *       backlight power. Support for specifying these needs to be
+	 *       added.
+	 */
+
+	return 0;
+}
+
+static struct of_device_id pwm_backlight_of_match[] = {
+	{ .compatible = "pwm-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
+#else
+static int pwm_backlight_parse_dt(struct device *dev,
+				  struct platform_pwm_backlight_data *data)
+{
+	return -ENODEV;
+}
+#endif
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct backlight_properties props;
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct platform_pwm_backlight_data defdata;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	int ret;
 
 	if (!data) {
-		dev_err(&pdev->dev, "failed to find platform data\n");
-		return -EINVAL;
+		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to find platform data\n");
+			return ret;
+		}
+
+		data = &defdata;
 	}
 
 	if (data->init) {
@@ -198,8 +259,9 @@  static int pwm_backlight_resume(struct platform_device *pdev)
 
 static struct platform_driver pwm_backlight_driver = {
 	.driver		= {
-		.name	= "pwm-backlight",
-		.owner	= THIS_MODULE,
+		.name		= "pwm-backlight",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pwm_backlight_of_match),
 	},
 	.probe		= pwm_backlight_probe,
 	.remove		= pwm_backlight_remove,