Patchwork UBUNTU: SAUCE: acpi: video: fix acpi_backlight=video

login
register
mail settings
Submitter Kamal Mostafa
Date May 1, 2010, 7:11 p.m.
Message ID <1272741071-4094-1-git-send-email-kamal@canonical.com>
Download mbox | patch
Permalink /patch/51457/
State Awaiting Upstream
Delegated to: Andy Whitcroft
Headers show

Comments

Kamal Mostafa - May 1, 2010, 7:11 p.m.
Make "acpi_backlight=video" param enable ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO
as intended, instead of incorrectly enabling video output switching.

BugLink: http://bugs.launchpad.net/bugs/573120

Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 drivers/acpi/video_detect.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Chase Douglas - May 1, 2010, 7:43 p.m.
On Sat, May 1, 2010 at 3:11 PM, Kamal Mostafa <kamal@canonical.com> wrote:
> Make "acpi_backlight=video" param enable ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO
> as intended, instead of incorrectly enabling video output switching.
>
> BugLink: http://bugs.launchpad.net/bugs/573120
>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> ---
>  drivers/acpi/video_detect.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index fc2f26b..c5fef01 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -250,7 +250,7 @@ static int __init acpi_backlight(char *str)
>                                ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
>                if (!strcmp("video", str))
>                        acpi_video_support |=
> -                               ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
> +                               ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO;
>        }
>        return 1;
>  }
> --
> 1.7.0.4

Since this is post-release and not a kitten-killer, it should probably
come directly from the stable tree. At the very least, it should be
accepted upstream (hopefully in Linus' tree, but at least in the acpi
tree) before we take it in. This is better overall for our maintenance
in case the patch is faulty, and for the greater Linux community.

If you think this should be included in 10.04 as a pre-stable patch, I
think it should be resent with a few changes:

* Add [Lucid] to the subject so Stefan knows what tree it's for (and
send multiple patches for separate trees where required, unless Stefan
has a better approach)
* Put the SRU justification in the bug report and in the patch commit message
* A justification for pre-stable and pre-upstream inclusion in the
released ubuntu kernel

Otherwise, this looks good to me. You can ack me for the upstream
submission if you want, though I don't carry much weight there yet :).

-- Chase
Kamal Mostafa - May 3, 2010, 9:23 p.m.
On Sat, 2010-05-01 at 15:43 -0400, Chase Douglas wrote:
> On Sat, May 1, 2010 at 3:11 PM, Kamal Mostafa <kamal@canonical.com> wrote:
> > Make "acpi_backlight=video" param enable ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO
> > as intended, instead of incorrectly enabling video output switching.
> >
> > BugLink: http://bugs.launchpad.net/bugs/573120
> >

Thanks for your feedback and ongoing coaching Chase.  :-)  I sincerely
appreciate it!

> Since this is post-release and not a kitten-killer, it should probably
> come directly from the stable tree.  [...]

Agreed.  I'm not inclined to push for this one as an Ubuntu SRU.  And I
already forwarded the patch upstream to LKML and linux-acpi.

Questions:

1. Was it okay that I sent it upstream and to kernel-team@l.u.c
simultaneously, or was I supposed to send to k-t first and wait for some
Ack's before sending upstream?

2. What would have been the proper subject line for this?  I mean, how
do I send a patch to k-t "for the purpose of getting some Ack's" without
asking that it be applied to any Ubuntu tree?  Should I just have left
out "UBUNTU: SAUCE:"?   Or maybe used  "[RFC PATCH]" also?

> 
> Otherwise, this looks good to me. You can ack me for the upstream
> submission if you want, though I don't carry much weight there yet :).

If I hadn't sent it off already, I certainly would have added your Ack.
I get the feeling that I did this "out of order".

 -Kamal
Chase Douglas - May 4, 2010, 12:08 a.m.
On Mon, May 3, 2010 at 5:23 PM, Kamal Mostafa <kamal@canonical.com> wrote:
> Questions:
>
> 1. Was it okay that I sent it upstream and to kernel-team@l.u.c
> simultaneously, or was I supposed to send to k-t first and wait for some
> Ack's before sending upstream?

It's certainly fine to send patches to both at the same time. As for
myself, If I have a patch against upstream I usually submit it there
first, wait for it to be included, and then send it back here as
pre-stable if appropriate. This saves time and limits confusion if you
have to rework your patch for upstream. In cases that are really hairy
(like the load avg patch last month), I'll send it around here as sort
of an RFC, but I think that's unnecessary for simpler patches.

My gut feeling is that Acks from our team in general aren't as useful
as are Acks from people who are involved with the subsystem in
question. It might be helpful to have an Ack or two from us, but only
as a sanity check that multiple people think it's ok. Overall, I've
found that I haven't really needed Acks from others before submission
as long as the patch was good :).

> 2. What would have been the proper subject line for this?  I mean, how
> do I send a patch to k-t "for the purpose of getting some Ack's" without
> asking that it be applied to any Ubuntu tree?  Should I just have left
> out "UBUNTU: SAUCE:"?   Or maybe used  "[RFC PATCH]" also?

I'm not real sure, but [RFC] sounds good to me. As soon as you start
including ubuntu specific tags it begins to look like you sent it for
inclusion in an ubuntu kernel as is.

I hope that helps!

-- Chase

Patch

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index fc2f26b..c5fef01 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -250,7 +250,7 @@  static int __init acpi_backlight(char *str)
 				ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
 		if (!strcmp("video", str))
 			acpi_video_support |=
-				ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
+				ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO;
 	}
 	return 1;
 }