diff mbox series

[v1,1/5] ACPI: utils: Introduce acpi_match_video_device_handle() helper

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

Commit Message

Andy Shevchenko June 30, 2022, 9:28 p.m. UTC
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(+)

Comments

Mika Westerberg July 1, 2022, 5:47 a.m. UTC | #1
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).
Rafael J. Wysocki July 5, 2022, 6:40 p.m. UTC | #2
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.
Andy Shevchenko July 8, 2022, 9:27 p.m. UTC | #3
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?
Andy Shevchenko July 8, 2022, 9:31 p.m. UTC | #4
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 mbox series

Patch

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);