| Message ID | 1379972467-11243-11-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/23/2013 03:41 PM, Thierry Reding wrote: > The default for backlight devices is to be enabled immediately when > registering with the backlight core. This can be useful for setups that > use a simple framebuffer device and where the backlight cannot otherwise > be hooked up to the panel. > > However, when dealing with more complex setups, such as those of recent > ARM SoCs, this can be problematic. Since the backlight is usually setup > separately from the display controller, the probe order is not usually > deterministic. That can lead to situations where the backlight will be > powered up and the panel will show an uninitialized framebuffer. > > Furthermore, subsystems such as DRM have advanced functionality to set > the power mode of a panel. In order to allow such setups to power up the > panel at exactly the right moment, a way is needed to prevent the > backlight core from powering the backlight up automatically when it is > registered. > > This commit introduces a new boot_off field in the platform data (and > also implements getting the same information from device tree). When set > the initial backlight power mode will be set to "off". > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > + - backlight-boot-off: keep the backlight disabled on boot A few thoughts: 1) Does this property indicate that: a) The current state of the backlight at boot. In which case, this will need bootloader involvement to modify the value in the DT at boot time based on whether the bootloader turned on the backlight:-( b) That the driver should not turn on the backlight immediately? That seems to describe driver behaviour more than HW. Is it appropriate to put that into DT? Your suggestion to make the backlight not turn on by default might be a better fix? 2) Should the driver instead attempt to read the current state of the GPIO output to determine whether the backlight is on? This may not be possible on all HW. 3) Doesn't the following code in probe() (added in a previous patch) need to be updated? > + if (gpio_is_valid(pb->enable_gpio)) { > + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) > + gpio_set_value(pb->enable_gpio, 0); > + else > + gpio_set_value(pb->enable_gpio, 1); > + } ... That assumes that the backlight is on at boot, and hence presumably after this patch still always turns on the backlight, only to turn it off very quickly once the following code in this patch executes: (and perhaps we also need to avoid turning the backlight off then on if it was already on at boot) > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > @@ -318,6 +320,12 @@ static int pwm_backlight_probe(struct platform_device *pdev) > } > > bl->props.brightness = data->dft_brightness; > + > + if (data->boot_off) > + bl->props.power = FB_BLANK_POWERDOWN; > + else > + bl->props.power = FB_BLANK_UNBLANK;
On Tue, Oct 01, 2013 at 12:50:51PM -0600, Stephen Warren wrote: > On 09/23/2013 03:41 PM, Thierry Reding wrote: > > The default for backlight devices is to be enabled immediately when > > registering with the backlight core. This can be useful for setups that > > use a simple framebuffer device and where the backlight cannot otherwise > > be hooked up to the panel. > > > > However, when dealing with more complex setups, such as those of recent > > ARM SoCs, this can be problematic. Since the backlight is usually setup > > separately from the display controller, the probe order is not usually > > deterministic. That can lead to situations where the backlight will be > > powered up and the panel will show an uninitialized framebuffer. > > > > Furthermore, subsystems such as DRM have advanced functionality to set > > the power mode of a panel. In order to allow such setups to power up the > > panel at exactly the right moment, a way is needed to prevent the > > backlight core from powering the backlight up automatically when it is > > registered. > > > > This commit introduces a new boot_off field in the platform data (and > > also implements getting the same information from device tree). When set > > the initial backlight power mode will be set to "off". > > > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > > > + - backlight-boot-off: keep the backlight disabled on boot > > A few thoughts: > > 1) Does this property indicate that: > > a) The current state of the backlight at boot. In which case, this will > need bootloader involvement to modify the value in the DT at boot time > based on whether the bootloader turned on the backlight:-( I was pretty much following the regulator bindings here. But the meaning is different indeed, see below. > b) That the driver should not turn on the backlight immediately? That > seems to describe driver behaviour more than HW. Is it appropriate to > put that into DT? That's what it was supposed to mean. The idea is, and I mentioned that in the commit message, that when wired up with a higher-level API such as DRM, then usually that framework knows exactly when to enable the backlight. Having the backlight enable itself on probe may cause the panel to light up with no content or garbage. > Your suggestion to make the backlight not turn on by default might be a > better fix? I think so too, but the problem in this case is that users of this driver heretofore assumed that it would be turned on by default. I think that's been common practice for all backlight drivers. Adding a property to DT was supposed to be a way to keep backwards compatibility with any existing users while at the same time allowing the backlight to be used in a more "modern" system. I don't really have any other ideas how to achieve this. It is quite possible that the only reason why turning on the backlight at probe time is the current default is because backlight_properties' .power field is by default initialized to 0, which turns out to be the value of FB_BLANK_UNBLANK. That's been the source of quite a bit of confusion (I could tell you stories of how people tried to convince me that there must be a bug in the Linux kernel because writing a 0 to the backlight's bl_power field in sysfs turns the backlight on instead of off). The same is true for DPMS modes in DRM and X, where "on" has the value 0. Now there have been plans to overhaul the backlight subsystem for quite some time. One of the goals was to decouple it from the FB subsystem, which might help solve this to some degree. At the same time sysfs is an ABI, so we can't just go and change it randomly. The same is true for the default behaviour of enabling the backlight at boot. That can equally be considered an ABI and changing it will cause the majority of devices to regress, and that's not something that I look forward to taking the blame for... > 2) Should the driver instead attempt to read the current state of the > GPIO output to determine whether the backlight is on? This may not be > possible on all HW. > > 3) Doesn't the following code in probe() (added in a previous patch) > need to be updated? > > > + if (gpio_is_valid(pb->enable_gpio)) { > > + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) > > + gpio_set_value(pb->enable_gpio, 0); > > + else > > + gpio_set_value(pb->enable_gpio, 1); > > + } > > ... That assumes that the backlight is on at boot, and hence presumably > after this patch still always turns on the backlight, only to turn it > off very quickly once the following code in this patch executes: > > (and perhaps we also need to avoid turning the backlight off then on if > it was already on at boot) Yes, that's a good point. Depending on the outcome of the above discussion I'll update this to match the expectations. Thierry
On Tue, Oct 01, 2013 at 12:50:51PM -0600, Stephen Warren wrote: > On 09/23/2013 03:41 PM, Thierry Reding wrote: [...] > > + if (gpio_is_valid(pb->enable_gpio)) { > > + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) > > + gpio_set_value(pb->enable_gpio, 0); > > + else > > + gpio_set_value(pb->enable_gpio, 1); > > + } > > ... That assumes that the backlight is on at boot, and hence presumably > after this patch still always turns on the backlight, only to turn it > off very quickly once the following code in this patch executes: I was just going to fix this, but then saw that the code in .probe() is actually this: if (gpio_is_valid(pb->enable_gpio)) { unsigned long flags; if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) flags = GPIOF_OUT_INIT_HIGH; else flags = GPIOF_OUT_INIT_LOW; ret = gpio_request_one(pb->enable_gpio, flags, "enable"); ... } Which sets the backlight up to be disabled by default and is consistent with the PWM and regulator setup. Only later, depending on the value of boot_off it gets enabled (or stays disabled). > (and perhaps we also need to avoid turning the backlight off then on if > it was already on at boot) That's true. Although I'm not sure if that's even possible. I think the pinctrl driver doesn't touch the registers, but what about the GPIO and PWM drivers. It might be impossible to always query the boot status and set the internal state based on that. The easiest would probably be if both the bootloader and the kernel use the same DT, in which case the bootloader could set the backlight-boot-on property, in which case we wouldn't need to do anything at .probe() time really. Thierry
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt index 3898f26..1271886 100644 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt @@ -16,6 +16,7 @@ Optional properties: "pwms" property (see PWM binding[0]) - enable-gpios: a list of GPIOs to enable and disable the backlight - power-supply: regulator for supply voltage + - backlight-boot-off: keep the backlight disabled on boot [0]: Documentation/devicetree/bindings/pwm/pwm.txt diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index a2b3876..3b2d9cf 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -184,6 +184,8 @@ static int pwm_backlight_parse_dt(struct device *dev, if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW)) data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW; + data->boot_off = of_property_read_bool(node, "backlight-boot-off"); + return 0; } @@ -318,6 +320,12 @@ static int pwm_backlight_probe(struct platform_device *pdev) } bl->props.brightness = data->dft_brightness; + + if (data->boot_off) + bl->props.power = FB_BLANK_POWERDOWN; + else + bl->props.power = FB_BLANK_UNBLANK; + backlight_update_status(bl); platform_set_drvdata(pdev, bl); diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h index 2de2e27..ea4a239 100644 --- a/include/linux/pwm_backlight.h +++ b/include/linux/pwm_backlight.h @@ -18,6 +18,8 @@ struct platform_pwm_backlight_data { unsigned int *levels; int enable_gpio; unsigned long enable_gpio_flags; + bool boot_off; + int (*init)(struct device *dev); int (*notify)(struct device *dev, int brightness); void (*notify_after)(struct device *dev, int brightness);
The default for backlight devices is to be enabled immediately when registering with the backlight core. This can be useful for setups that use a simple framebuffer device and where the backlight cannot otherwise be hooked up to the panel. However, when dealing with more complex setups, such as those of recent ARM SoCs, this can be problematic. Since the backlight is usually setup separately from the display controller, the probe order is not usually deterministic. That can lead to situations where the backlight will be powered up and the panel will show an uninitialized framebuffer. Furthermore, subsystems such as DRM have advanced functionality to set the power mode of a panel. In order to allow such setups to power up the panel at exactly the right moment, a way is needed to prevent the backlight core from powering the backlight up automatically when it is registered. This commit introduces a new boot_off field in the platform data (and also implements getting the same information from device tree). When set the initial backlight power mode will be set to "off". Signed-off-by: Thierry Reding <treding@nvidia.com> --- Note: Perhaps it would be more useful to make this the default behaviour in the backlight core? Many other subsystems and frameworks assume that a resource is off unless explicitly enabled. --- .../devicetree/bindings/video/backlight/pwm-backlight.txt | 1 + drivers/video/backlight/pwm_bl.c | 8 ++++++++ include/linux/pwm_backlight.h | 2 ++ 3 files changed, 11 insertions(+)