| Message ID | 1349821146-19155-1-git-send-email-e-mail@date.by (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote: > This should fix brightness controls on some laptops. > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861 > > Signed-off-by: Igor Murzov <e-mail@date.by> Put more detials into the changelog, please. The BZ entry linked above may or may not be accessible when the changelog is read by someone. > --- > drivers/acpi/video.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index 1e0a9e1..01bb58d 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, > struct acpi_device *dev; > > status = acpi_video_device_enumerate(video); > - if (status) > - return status; > + if (status) { > + if (status == AE_AML_PACKAGE_LIMIT) > + status = 0; /* Ignore this error */ > + else > + return status; > + } First off, please add a comment explaining _why_ we're ignoring the error. Second, what about adding just: if (status == AE_AML_PACKAGE_LIMIT) status = 0; before the "if (status)" check? > > list_for_each_entry(dev, &device->children, node) { Thanks, Rafael
On Wed, 10 Oct 2012 00:58:10 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote: > > This should fix brightness controls on some laptops. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861 > > > > Signed-off-by: Igor Murzov <e-mail@date.by> > > Put more detials into the changelog, please. The BZ entry linked above > may or may not be accessible when the changelog is read by someone. > > > --- > > drivers/acpi/video.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index 1e0a9e1..01bb58d 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, > > struct acpi_device *dev; > > > > status = acpi_video_device_enumerate(video); > > - if (status) > > - return status; > > + if (status) { > > + if (status == AE_AML_PACKAGE_LIMIT) > > + status = 0; /* Ignore this error */ > > + else > > + return status; > > + } > > First off, please add a comment explaining _why_ we're ignoring the error. The problem is that i'm not sure if it's ok to ignore AE_AML_PACKAGE_LIMIT here. Stefan Wilkens in bugzilla claims that video module works fine on his Compaq 6720s in spite of the fact that acpi_video_device_enumerate() is not able to find any video device on his laptop. This fix is just the most obvious one, not necessarily the proper one. > Second, what about adding just: > > if (status == AE_AML_PACKAGE_LIMIT) > status = 0; > > before the "if (status)" check? Ok -- Igor > > > > list_for_each_entry(dev, &device->children, node) { > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 10 of October 2012 04:43:40 Igor Murzov wrote: > On Wed, 10 Oct 2012 00:58:10 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > On Wednesday 10 of October 2012 02:19:06 Igor Murzov wrote: > > > This should fix brightness controls on some laptops. > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861 > > > > > > Signed-off-by: Igor Murzov <e-mail@date.by> > > > > Put more detials into the changelog, please. The BZ entry linked above > > may or may not be accessible when the changelog is read by someone. > > > > > --- > > > drivers/acpi/video.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > > index 1e0a9e1..01bb58d 100644 > > > --- a/drivers/acpi/video.c > > > +++ b/drivers/acpi/video.c > > > @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, > > > struct acpi_device *dev; > > > > > > status = acpi_video_device_enumerate(video); > > > - if (status) > > > - return status; > > > + if (status) { > > > + if (status == AE_AML_PACKAGE_LIMIT) > > > + status = 0; /* Ignore this error */ > > > + else > > > + return status; > > > + } > > > > First off, please add a comment explaining _why_ we're ignoring the error. > > The problem is that i'm not sure if it's ok to ignore > AE_AML_PACKAGE_LIMIT here. This should be written in a comment, then. :-) > Stefan Wilkens in bugzilla > claims that video module works fine on his Compaq 6720s > in spite of the fact that acpi_video_device_enumerate() > is not able to find any video device on his laptop. > This fix is just the most obvious one, not necessarily > the proper one. So say something like "there are systems known to return AE_AML_PACKAGE_LIMIT here, apparently for no valid reason" in a comment. And give a reference to the BZ entry in the changelog. There are too many places where we have put workarounds like this without documenting them properly and now we have problems when we need to change something. Thanks, Rafael
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..01bb58d 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1349,8 +1349,12 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, struct acpi_device *dev; status = acpi_video_device_enumerate(video); - if (status) - return status; + if (status) { + if (status == AE_AML_PACKAGE_LIMIT) + status = 0; /* Ignore this error */ + else + return status; + } list_for_each_entry(dev, &device->children, node) {
This should fix brightness controls on some laptops. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47861 Signed-off-by: Igor Murzov <e-mail@date.by> --- drivers/acpi/video.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)