[PATCH 2/2] video: mark "actual_brightness" deprecated

[PATCH 2/2] video: mark "actual_brightness" deprecated

[Zhang Rui]

/sys/class/backlight/.../brightness returns a cached value,
which confuses a lot of users.

Make the "brightness" reflect the actual backlight levels,
and mark /sys/class/backlight/.../brightness as depreacated
in this patch.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/video/backlight/Kconfig     |   17 +++++++++++++++++
 drivers/video/backlight/backlight.c |   11 ++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/video/backlight/Kconfig
===================================================================
--- linux-2.6.orig/drivers/video/backlight/Kconfig
+++ linux-2.6/drivers/video/backlight/Kconfig
@@ -98,6 +98,23 @@ config BACKLIGHT_CLASS_DEVICE
 	  To have support for your specific LCD panel you will have to
 	  select the proper drivers which depend on this option.
 
+config BACKLIGHT_CLASS_DEVICE_LEGACY_INTERFACE
+	bool "Deprecated lowlevel Backlight control legacy sysfs I/F"
+	depends on BACKLIGHT_CLASS_DEVICE
+	default y
+	help
+	  "brightness" file used to return a cached brightness level.
+	  And users need to poke the "actual_brightness" to get the
+	  correct current backlight level.
+	  Now the "brightness" file always reflects the actual
+	  brightness and "actual_brightness" is superfluous.
+
+	  Some applications including X still use this file to get the current
+	  backlight level, thus we can not remove it immediately.
+
+	  Say N here if you're sure no application depends on the
+	  "actual_brightness" file.
+
 config BACKLIGHT_ATMEL_LCDC
 	bool "Atmel LCDC Contrast-as-Backlight control"
 	depends on BACKLIGHT_CLASS_DEVICE && FB_ATMEL
Index: linux-2.6/drivers/video/backlight/backlight.c
===================================================================
--- linux-2.6.orig/drivers/video/backlight/backlight.c
+++ linux-2.6/drivers/video/backlight/backlight.c
@@ -108,9 +108,14 @@ static ssize_t backlight_store_power(str
 static ssize_t backlight_show_brightness(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
+	int rc = -ENXIO;
 	struct backlight_device *bd = to_backlight_device(dev);
 
-	return sprintf(buf, "%d\n", bd->props.brightness);
+	mutex_lock(&bd->ops_lock);
+	if (bd->ops && bd->ops->get_brightness)
+		rc = sprintf(buf, "%d\n", bd->ops->get_brightness(bd));
+	mutex_unlock(&bd->ops_lock);
+	return rc;
 }
 
 static ssize_t backlight_store_brightness(struct device *dev,
@@ -152,6 +157,7 @@ static ssize_t backlight_show_max_bright
 	return sprintf(buf, "%d\n", bd->props.max_brightness);
 }
 
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE_LEGACY_INTERFACE
 static ssize_t backlight_show_actual_brightness(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -165,6 +171,7 @@ static ssize_t backlight_show_actual_bri
 
 	return rc;
 }
+#endif
 
 static struct class *backlight_class;
 
@@ -178,8 +185,10 @@ static struct device_attribute bl_device
 	__ATTR(bl_power, 0644, backlight_show_power, backlight_store_power),
 	__ATTR(brightness, 0644, backlight_show_brightness,
 		     backlight_store_brightness),
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE_LEGACY_INTERFACE
 	__ATTR(actual_brightness, 0444, backlight_show_actual_brightness,
 		     NULL),
+#endif
 	__ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
 	__ATTR_NULL,
 };

Re: [PATCH 2/2] video: mark "actual_brightness" deprecated

[Richard Purdie]

On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> /sys/class/backlight/.../brightness returns a cached value,
> which confuses a lot of users.
> 
> Make the "brightness" reflect the actual backlight levels,
> and mark /sys/class/backlight/.../brightness as depreacated
> in this patch.

This isn't an acceptable approach I'm afraid. We have some hardware
which needs limiting of the backlight brightness in cases of low battery
for example. "brightness" is the user requested value,
"actual_brightness" is the one that is actually active. This also makes
it clear when things like console blanking are active since the actual
value is 0 but brightness preserves the user requested value.

Cheers,

Richard

-- 
Richard Purdie
Intel Open Source Technology Centre

Re: [PATCH 2/2] video: mark "actual_brightness" deprecated

[Zhang Rui]

On Tue, 2008-12-30 at 18:37 +0800, Richard Purdie wrote:
> On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> > /sys/class/backlight/.../brightness returns a cached value,
> > which confuses a lot of users.
> > 
> > Make the "brightness" reflect the actual backlight levels,
> > and mark /sys/class/backlight/.../brightness as depreacated
> > in this patch.
> 
> This isn't an acceptable approach I'm afraid. We have some hardware
> which needs limiting of the backlight brightness in cases of low battery
we use "brightness" as the backlight limiting? then how does it work?
users can always set it to a higher or lower value no matter whether
it's low battery or not. IMO we'd better to use another file instead.
I got a couple of bugs about "the brightness file doesn't work", because
if people can change the backlight by poking "brightness", they prefer
to get the current brightness by reading it. what do you think?

> for example. "brightness" is the user requested value,
> "actual_brightness" is the one that is actually active.
why do we need the "user requested value"?
does some application depend on it?

thanks,
rui

Re: [PATCH 2/2] video: mark "actual_brightness" deprecated

[Richard Purdie]

On Wed, 2008-12-31 at 10:12 +0800, Zhang Rui wrote:
> On Tue, 2008-12-30 at 18:37 +0800, Richard Purdie wrote:
> > On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> > > /sys/class/backlight/.../brightness returns a cached value,
> > > which confuses a lot of users.
> > > 
> > > Make the "brightness" reflect the actual backlight levels,
> > > and mark /sys/class/backlight/.../brightness as depreacated
> > > in this patch.
> > 
> > This isn't an acceptable approach I'm afraid. We have some hardware
> > which needs limiting of the backlight brightness in cases of low battery
> we use "brightness" as the backlight limiting? then how does it work?

Limiting is done by the driver itself so userspace would request "5",
the hardware would only allow "2" due to power contraints so
"birghtness" reads "5" but actual_brightness reads "2".

> users can always set it to a higher or lower value no matter whether
> it's low battery or not. IMO we'd better to use another file instead.
> I got a couple of bugs about "the brightness file doesn't work", because
> if people can change the backlight by poking "brightness", they prefer
> to get the current brightness by reading it. what do you think?

No, this can't work. If you put a value into a sysfs file you should get
the same value back. This is why "actual_brightness" exists and is
readonly, reflecting the end result. Were possible/necessary that is
read from the hardware. People expecting anything else from the
interface aren't using it as designed.

> > for example. "brightness" is the user requested value,
> > "actual_brightness" is the one that is actually active.
> why do we need the "user requested value"?
> does some application depend on it?

Two reasons, one is that some applications might want to see both what
was set and also what the result was. Secondly, I'm uneasy with a sysfs
file that write one value and reads something different. The current
setup doesn't cause any problems as long as people understand what to
expect and why? By merging the two things it gets more confusing and
also removes information which is potentially useful to userspace.

Cheers,

Richard