On Wed, Jun 21, 2023 at 01:50:34PM -0500, Limonciello, Mario wrote:
> So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
> question would be where should the new "wbrf.c" be stored? The ACPI only
> version most certainly made sense in drivers/acpi/wbrf.c, but a generic
> version that only has an ACPI implementation right now not so much.
>
> On 6/21/2023 1:30 PM, Andrew Lunn wrote:
> > > And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.
> > Why? How is ACPI special that it does not need notifiers?
> ACPI core does has notifiers that are used, but they don't work the same.
> If you look at patch 4, you'll see amdgpu registers and unregisters using
> both
>
> acpi_install_notify_handler()
> and
> acpi_remove_notify_handler()
>
> If we supported both ACPI notifications and non-ACPI notifications
> all consumers would have to have support to register and use both types.
I took a quick look at this:
#define CPM_GPU_NOTIFY_COMMAND 0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+ if (event == CPM_GPU_NOTIFY_COMMAND &&
+ adev->wbrf_event_handler)
+ adev->wbrf_event_handler(adev);
+}
handle is ignored, All you need is the void * data to link back to
your private data.
I find it interesting that CPM_GPU_NOTIFY_COMMAND is defined here. So
nothing else can use it. Should it maybe be called
CPM_AMDGPU_NOTIFY_COMMAND?
Overall, looking at this, i don't see anything which could not be made
abstract:
static void amdgpu_wbrf_event(u32 event, void *data)
{
struct amdgpu_device *adev = (struct amdgpu_device *)data;
if (event == WBRF_GPU_NOTIFY_COMMAND &&
adev->wbrf_event_handler)
adev->wbrf_event_handler(adev);
}
int amdgpu_register_wbrf_notify_handler(struct amdgpu_device *adev,
wbrf_notify_handler handler)
{
struct device *dev = adev->dev);
adev->wbrf_event_handler = handler;
return wbrf_install_notify_handler(dev, amdgpu_wbrf_event, adev);
}
Andrew
