| Message ID | 1324377138-32129-8-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | New, archived |
| Headers | show |
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 = <®>; }; 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?
* 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 = <®>; > }; > > 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
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?
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
* 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
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,
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