| Message ID | 20220630212819.42958-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/5] ACPI: utils: Introduce acpi_match_video_device_handle() helper | expand |
Hi Andy, On Fri, Jul 01, 2022 at 12:28:15AM +0300, Andy Shevchenko wrote: > extern long acpi_is_video_device(acpi_handle handle); > +extern bool acpi_match_video_device_handle(acpi_handle handle); I think we can do slightly better here. The only caller of acpi_is_video_device() is in drivers/acpi/video_detect.c so we can move it there and make it static (is_video_device()). Then we can name this one acpi_is_video_device() instead and in addition make it take struct acpi_device as parameter instead of acpi_handle (I think we should not use acpi_handles in drivers if possible).
On Fri, Jul 1, 2022 at 7:47 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi Andy, > > On Fri, Jul 01, 2022 at 12:28:15AM +0300, Andy Shevchenko wrote: > > extern long acpi_is_video_device(acpi_handle handle); > > +extern bool acpi_match_video_device_handle(acpi_handle handle); > > I think we can do slightly better here. The only caller of > acpi_is_video_device() is in drivers/acpi/video_detect.c so we can move > it there and make it static (is_video_device()). > > Then we can name this one acpi_is_video_device() instead and in addition > make it take struct acpi_device as parameter instead of acpi_handle (I > think we should not use acpi_handles in drivers if possible). Agreed.
On Tue, Jul 05, 2022 at 08:40:30PM +0200, Rafael J. Wysocki wrote: > On Fri, Jul 1, 2022 at 7:47 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Fri, Jul 01, 2022 at 12:28:15AM +0300, Andy Shevchenko wrote: > > > extern long acpi_is_video_device(acpi_handle handle); > > > +extern bool acpi_match_video_device_handle(acpi_handle handle); > > > > I think we can do slightly better here. The only caller of > > acpi_is_video_device() is in drivers/acpi/video_detect.c so we can move > > it there and make it static (is_video_device()). AFAICS, the scan.c is user of it as well. > > Then we can name this one acpi_is_video_device() instead and in addition > > make it take struct acpi_device as parameter instead of acpi_handle (I > > think we should not use acpi_handles in drivers if possible). > > Agreed. Not sure it will help to have acpi device since most of the callers asks for handle to be checked. Taking into account above, what we can do is to rename it to acpi_handle_is_video_device() which should be clearer than initial proposal. Thoughts?
On Sat, Jul 09, 2022 at 12:27:09AM +0300, Andy Shevchenko wrote: > On Tue, Jul 05, 2022 at 08:40:30PM +0200, Rafael J. Wysocki wrote: > > On Fri, Jul 1, 2022 at 7:47 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Fri, Jul 01, 2022 at 12:28:15AM +0300, Andy Shevchenko wrote: ... > > > > extern long acpi_is_video_device(acpi_handle handle); > > > > +extern bool acpi_match_video_device_handle(acpi_handle handle); > > > I think we can do slightly better here. The only caller of > > > acpi_is_video_device() is in drivers/acpi/video_detect.c so we can move > > > it there and make it static (is_video_device()). > > AFAICS, the scan.c is user of it as well. > > > > Then we can name this one acpi_is_video_device() instead and in addition > > > make it take struct acpi_device as parameter instead of acpi_handle (I > > > think we should not use acpi_handles in drivers if possible). > > > > Agreed. > > Not sure it will help to have acpi device since most of the callers asks for > handle to be checked. Actually it's 2:2 now. I'm fine with ACPI device and name like acpi_dev_is_video_device() > Taking into account above, what we can do is to rename it to > > acpi_handle_is_video_device() > > which should be clearer than initial proposal. > > Thoughts?
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 3a9773a09e19..4800aba3b99c 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -929,6 +929,25 @@ static int __init acpi_backlight(char *str) } __setup("acpi_backlight=", acpi_backlight); +static const struct acpi_device_id video_device_ids[] = { + {ACPI_VIDEO_HID, 0}, + {} +}; + +/** + * acpi_match_video_device_handle - match handle against ACPI video device IDs + * @handle: ACPI handle to match + * + * Return: true when matches, otherwise false. + */ +bool acpi_match_video_device_handle(acpi_handle handle) +{ + struct acpi_device *adev = acpi_fetch_acpi_dev(handle); + + return adev && !acpi_match_device_ids(adev, video_device_ids); +} +EXPORT_SYMBOL(acpi_match_video_device_handle); + /** * acpi_match_platform_list - Check if the system matches with a given list * @plat: pointer to acpi_platform_list table terminated by a NULL entry diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 7b96a8bff6d2..c48e8a0df0cc 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -439,6 +439,8 @@ extern char *wmi_get_acpi_device_uid(const char *guid); extern char acpi_video_backlight_string[]; extern long acpi_is_video_device(acpi_handle handle); +extern bool acpi_match_video_device_handle(acpi_handle handle); + extern int acpi_blacklisted(void); extern void acpi_osi_setup(char *str); extern bool acpi_osi_is_win8(void);
There are a couple of users that open code functionality of matching a given handle against ACPI video device IDs. The current approach duplicates ID table along with the matching code. Consolidate it under the acpi_match_video_device_handle() helper's hood. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/utils.c | 19 +++++++++++++++++++ include/linux/acpi.h | 2 ++ 2 files changed, 21 insertions(+)