Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

Bjorn Helgaas
2008-11-17T19:52:21Z

On Sunday 16 November 2008 03:07:31 pm Thomas Renninger wrote:
> On Friday 14 November 2008 04:27:42 pm Bjorn Helgaas wrote:
> > On Friday 01 August 2008 09:37:55 am Thomas Renninger wrote:
> > > @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device
> > > *device, will get autoloaded and the device might still match
> > > against another driver.
> > > */
> > > - if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> > > + if (acpi_is_video_device(device))
> > > cid_add = ACPI_VIDEO_HID;
> > > else if (ACPI_SUCCESS(acpi_bay_match(device)))
> > > cid_add = ACPI_BAY_HID;
> >
> > It doesn't seem right to me to make this core behavior depend
> > on a config option. With this approach, the ACPI device tree may
> > or may not contain an ACPI_VIDEO_HID device, depending on whether
> > CONFIG_ACPI_VIDEO is set, and that seems capricious.
> >
> > What is the benefit of moving this code out of scan.c? It's not
> > a very big function, and I think the consistency is worth the extra
> > code.
> This was done because the function is re-used later to detect which kind of
> video functionalities the video device provides.
> I somehow agree, but I don't see much of a disadvantage having this separated.
> You do not have the video device marked as such if CONFIG_ACPI_VIDEO is not
> compiled in, but it shouldn't hurt?

My personal opinion is that the device tree should be the same,
regardless of which drivers were selected at build-time. It should
be possible to build a kernel with CONFIG_ACPI_VIDEO=n, then decide
later that you want to load a video driver. Plus, it took me about
half an hour to figure out why the Makefile looks weird:

obj-$(CONFIG_ACPI_VIDEO) += video.o
ifdef CONFIG_ACPI_VIDEO
obj-y += video_detect.o
endif

(I first thought video_detect.o could just be moved up to the
first line along with video.o.)

> The two corresponding functions:
> acpi_backlight_cap_match
> acpi_is_video_device
>
> might also go back to video.c again and get exported there. I can do that if
> you think it's worth it.

As long as we use the "self-defined HID" strategy for binding drivers,
I think those HIDs should be considered part of the core, and they
should be set whenever CONFIG_ACPI=y. I don't think they should
depend on which drivers were selected at build-time. In some ways,
it might be nicer if a driver could supply its own "match" function,
and then all the video-related special cases could be in the video
driver.

Bjorn