[PATCH] backlight: do not power off backlight when unregistering

[PATCH] backlight: do not power off backlight when unregistering

[Henrique de Moraes Holschuh]

ACPI drivers like ibm-acpi are moving to the backlight sysfs infrastructure.
During ibm-acpi testing, I have noticed that backlight_device_unregister()
sets the display brightness and power to zero.

This causes the display to be dimmed on ibm-acpi module removal.  It will
affect all other ACPI drivers that are being converted to use the backlight
class, as well.

This annoying behaviour in backlight_device_unregister() can either be
reverted, or it can be worked around on acpi drivers by doing a
backlightdevice->props->update_status = NULL before calling
backlight_device_unregister().

Given the, AFAIK, lack of a good reason to disable display backlight as the
_general_ behaviour for the entire sysfs class, the attached patch changes
backlight.c to not do so anymore.

Since the commit that introduced this behaviour (commit
6ca017658b1f902c9bba2cc1017e301581f7728d) apparently did so because the
corgi_bl.c driver powered off the backlight, the attached patch also makes
sure that the corgi_bl.c driver will not have its behaviour changed.

Patch against latest linux-2.6.git.  corgi_bl.c changes untested, as I lack
the hardware to do so.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
--

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 27597c5..de5a6b3 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -259,12 +259,6 @@ void backlight_device_unregister(struct
 					 &bl_class_device_attributes[i]);
 
 	down(&bd->sem);
-	if (likely(bd->props && bd->props->update_status)) {
-		bd->props->brightness = 0;
-		bd->props->power = 0;
-		bd->props->update_status(bd);
-	}
-
 	bd->props = NULL;
 	up(&bd->sem);
 
diff --git a/drivers/video/backlight/corgi_bl.c b/drivers/video/backlight/corgi_bl.c
index d07ecb5..61587ca 100644
--- a/drivers/video/backlight/corgi_bl.c
+++ b/drivers/video/backlight/corgi_bl.c
@@ -135,6 +135,10 @@ static int corgibl_probe(struct platform
 
 static int corgibl_remove(struct platform_device *dev)
 {
+	corgibl_data.power = 0;
+	corgibl_data.brightness = 0;
+	corgibl_send_intensity(corgi_backlight_device);
+
 	backlight_device_unregister(corgi_backlight_device);
 
 	printk("Corgi Backlight Driver Unloaded\n");

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] backlight: do not power off backlight when unregistering

[Richard Purdie]

On Sun, 2006-11-05 at 20:54 -0200, Henrique de Moraes Holschuh wrote:
> ACPI drivers like ibm-acpi are moving to the backlight sysfs infrastructure.
> During ibm-acpi testing, I have noticed that backlight_device_unregister()
> sets the display brightness and power to zero.
> 
> This causes the display to be dimmed on ibm-acpi module removal.  It will
> affect all other ACPI drivers that are being converted to use the backlight
> class, as well.
> 
> This annoying behaviour in backlight_device_unregister() can either be
> reverted, or it can be worked around on acpi drivers by doing a
> backlightdevice->props->update_status = NULL before calling
> backlight_device_unregister().
> 
> Given the, AFAIK, lack of a good reason to disable display backlight as the
> _general_ behaviour for the entire sysfs class, the attached patch changes
> backlight.c to not do so anymore.

The reason the corgi code does this is quite simple. If you don't turn
off the backlight when unloading the backlight driver, nothing can turn
the backlight off. The backlight is the biggest power consumer on corgi
and would cause some power drain even if for example you then suspend
the device. I never want to end up with those bug reports. I reasoned
that other devices would have a similar set of constraints (all the
existing users at the time did). On such hardware, deinitialising
anything you initialised is the norm and makes sense but the PC/ACPI
world is different.

Setting backlightdevice->props->update_status = NULL before
unregistering is a horrible idea and I don't want to see hacks like that
so yes, lets move it back into the drivers. 

> Since the commit that introduced this behaviour (commit
> 6ca017658b1f902c9bba2cc1017e301581f7728d) apparently did so because the
> corgi_bl.c driver powered off the backlight, the attached patch also makes
> sure that the corgi_bl.c driver will not have its behaviour changed.

Those commits were designed to standardise several behaviours amongst
several drivers and this specific case was added to the core rather than
coding it in each of several drivers. We therefore really need to update
those other drivers too (locomo at the very least).

Richard

Re: [PATCH] backlight: do not power off backlight when unregistering

[Henrique de Moraes Holschuh]

On Mon, 06 Nov 2006, Richard Purdie wrote:
> that other devices would have a similar set of constraints (all the
> existing users at the time did). On such hardware, deinitialising
> anything you initialised is the norm and makes sense but the PC/ACPI
> world is different.

Yes, and we don't *init* the hardware with the backlight class either in
ACPI drivers...  AFAIK, anyway.

> Setting backlightdevice->props->update_status = NULL before
> unregistering is a horrible idea and I don't want to see hacks like that
> so yes, lets move it back into the drivers. 

Sure thing.  I am not at all interested into having such a horrible hack in
my local version of ibm-acpi for a second longer than strictly needed, so I
will help if I am able to.

> > Since the commit that introduced this behaviour (commit
> > 6ca017658b1f902c9bba2cc1017e301581f7728d) apparently did so because the
> > corgi_bl.c driver powered off the backlight, the attached patch also makes
> > sure that the corgi_bl.c driver will not have its behaviour changed.
> 
> Those commits were designed to standardise several behaviours amongst
> several drivers and this specific case was added to the core rather than
> coding it in each of several drivers. We therefore really need to update
> those other drivers too (locomo at the very least).

Do you have a list of them?  I can try to produce (untested) patches for
the ones in-tree, if you want me to.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] backlight: do not power off backlight when unregistering

[Henrique de Moraes Holschuh]

On Mon, 06 Nov 2006, Richard Purdie wrote:
> Those commits were designed to standardise several behaviours amongst
> several drivers and this specific case was added to the core rather than
> coding it in each of several drivers. We therefore really need to update
> those other drivers too (locomo at the very least).

The following in-tree drivers need changes:
drivers/video/backlight/locomolcd.c
drivers/video/backlight/hp680_bl.c
drivers/video/backlight/corgi_bl.c

I will repost the patch with locomolcd.c and hp680_bl.c included.

The following in-tree (latest linux-2.6 git tree) drivers are desktop/laptop
devices and likely do not want the "dim and power off backlight on
backlight_device_unregister" behavior:

drivers/video/aty/*
drivers/video/riva/fbdev.c
drivers/video/nvidia/nv_backlight.c
drivers/misc/msi-laptop.c

The ACPI drivers being converted to backlight sysfs are very unlikely to
want the "power off backlight on backlight_device_unregister" behavior, so I
have not hunted after backlight sysfs conversions in the linux-acpi-2.6 git
tree.  Still, linux-acpi is cc'ed.

I have CC'ed the relevant people (please forgive me any ommissions) for the
drivers listed above, so they can chime in if their driver should retain the
"dim and power off backlight on backlight_device_unregister" behaviour.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH] backlight: do not power off backlight when unregistering (try 2)

[Henrique de Moraes Holschuh]

ACPI drivers like ibm-acpi are moving to the backlight sysfs infrastructure.
During ibm-acpi testing, I have noticed that backlight_device_unregister()
sets the display brightness and power to zero.

This causes the display to be dimmed on ibm-acpi module removal.  It will
affect all other ACPI drivers that are being converted to use the backlight
class, as well.  It also affects a number of framebuffer devices that are
used on desktops and laptops which might also not want such behaviour.

Since working around this behaviour requires undesireable hacks, Richard
Purdie decided that we would be better off reverting the changes in the
sysfs class, and adding the code to dim and power off the backlight device
to the drivers that want it.  This patch is my attempt to do so.

Patch against latest linux-2.6.git.  Changes untested, as I lack the
required hardware.  Still, they are trivial enough that, apart from typos,
there is little chance of getting them wrong.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Andriy Skulysh <askulysh@image.kiev.ua>
Cc: Antonino Daplas <adaplas@pol.net>
--

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 27597c5..de5a6b3 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -259,12 +259,6 @@ void backlight_device_unregister(struct
 					 &bl_class_device_attributes[i]);
 
 	down(&bd->sem);
-	if (likely(bd->props && bd->props->update_status)) {
-		bd->props->brightness = 0;
-		bd->props->power = 0;
-		bd->props->update_status(bd);
-	}
-
 	bd->props = NULL;
 	up(&bd->sem);
 
diff --git a/drivers/video/backlight/corgi_bl.c b/drivers/video/backlight/corgi_bl.c
index d07ecb5..61587ca 100644
--- a/drivers/video/backlight/corgi_bl.c
+++ b/drivers/video/backlight/corgi_bl.c
@@ -135,6 +135,10 @@ static int corgibl_probe(struct platform
 
 static int corgibl_remove(struct platform_device *dev)
 {
+	corgibl_data.power = 0;
+	corgibl_data.brightness = 0;
+	corgibl_send_intensity(corgi_backlight_device);
+
 	backlight_device_unregister(corgi_backlight_device);
 
 	printk("Corgi Backlight Driver Unloaded\n");
diff --git a/drivers/video/backlight/hp680_bl.c b/drivers/video/backlight/hp680_bl.c
index e399321..1c569fb 100644
--- a/drivers/video/backlight/hp680_bl.c
+++ b/drivers/video/backlight/hp680_bl.c
@@ -117,6 +117,10 @@ static int __init hp680bl_probe(struct p
 
 static int hp680bl_remove(struct platform_device *dev)
 {
+	hp680bl_data.brightness = 0;
+	hp680bl_data.power = 0;
+	hp680bl_send_intensity(hp680_backlight_device);
+
 	backlight_device_unregister(hp680_backlight_device);
 
 	return 0;
diff --git a/drivers/video/backlight/locomolcd.c b/drivers/video/backlight/locomolcd.c
index 628571c..2d79054 100644
--- a/drivers/video/backlight/locomolcd.c
+++ b/drivers/video/backlight/locomolcd.c
@@ -200,6 +200,10 @@ static int locomolcd_remove(struct locom
 {
 	unsigned long flags;
 
+	locomobl_data.brightness = 0;
+	locomobl_data.power = 0;
+	locomolcd_set_intensity(locomolcd_bl_device);
+
 	backlight_device_unregister(locomolcd_bl_device);
 	local_irq_save(flags);
 	locomolcd_dev = NULL;

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] backlight: do not power off backlight when unregistering (try 2)

[Richard Purdie]

On Thu, 2006-11-09 at 22:32 -0200, Henrique de Moraes Holschuh wrote:
> ACPI drivers like ibm-acpi are moving to the backlight sysfs infrastructure.
> During ibm-acpi testing, I have noticed that backlight_device_unregister()
> sets the display brightness and power to zero.
> 
> This causes the display to be dimmed on ibm-acpi module removal.  It will
> affect all other ACPI drivers that are being converted to use the backlight
> class, as well.  It also affects a number of framebuffer devices that are
> used on desktops and laptops which might also not want such behaviour.
> 
> Since working around this behaviour requires undesireable hacks, Richard
> Purdie decided that we would be better off reverting the changes in the
> sysfs class, and adding the code to dim and power off the backlight device
> to the drivers that want it.  This patch is my attempt to do so.
> 
> Patch against latest linux-2.6.git.  Changes untested, as I lack the
> required hardware.  Still, they are trivial enough that, apart from typos,
> there is little chance of getting them wrong.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Andriy Skulysh <askulysh@image.kiev.ua>
> Cc: Antonino Daplas <adaplas@pol.net>
Acked-by: Richard Purdie <rpurdie@rpsys.net>

Re: [PATCH] backlight: do not power off backlight when unregistering

[James Simmons]

> The following in-tree (latest linux-2.6 git tree) drivers are desktop/laptop
> devices and likely do not want the "dim and power off backlight on
> backlight_device_unregister" behavior:
> 
> drivers/video/aty/*
> drivers/video/riva/fbdev.c
> drivers/video/nvidia/nv_backlight.c
> drivers/misc/msi-laptop.c

...
 
> I have CC'ed the relevant people (please forgive me any ommissions) for the
> drivers listed above, so they can chime in if their driver should retain the
> "dim and power off backlight on backlight_device_unregister" behaviour.

Hm. In the case of some drivers the hardware state on x86 is set back to 
text mode in some cases. So do we in that case dim the backlight?

Re: [PATCH] backlight: do not power off backlight when unregistering

[Henrique de Moraes Holschuh]

On Mon, 20 Nov 2006, James Simmons wrote:
> > The following in-tree (latest linux-2.6 git tree) drivers are desktop/laptop
> > devices and likely do not want the "dim and power off backlight on
> > backlight_device_unregister" behavior:
> > 
> > drivers/video/aty/*
> > drivers/video/riva/fbdev.c
> > drivers/video/nvidia/nv_backlight.c
> > drivers/misc/msi-laptop.c
> 
> ...
>  
> > I have CC'ed the relevant people (please forgive me any ommissions) for the
> > drivers listed above, so they can chime in if their driver should retain the
> > "dim and power off backlight on backlight_device_unregister" behaviour.
> 
> Hm. In the case of some drivers the hardware state on x86 is set back to 
> text mode in some cases. So do we in that case dim the backlight?

I would very much *hate* that happening on any x86 box of mine.  I won't
presume I understand enough of the usage pattern on weird devices, and it
would make some sense to power off the display if you are removing the
*only* way to talk to the video device (but I still think this is naively
assuming the local admin don't want to just leave the display as-is).

But as you said yourself, in regular desktop/laptops (at least x86 ones)
textmode is there, and works just fine, so killing the display on module
removal is wrong IMO.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh