lkml.org 
[lkml]   [2021]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 01/37] drm: Add drm_module_{pci,platform}_driver() helper macros
From
Hi Javier,

looks good already. Some comments are below.

Am 17.12.21 um 01:37 schrieb Javier Martinez Canillas:
> According to disable Documentation/admin-guide/kernel-parameters.txt, the
> nomodeset parameter can be used to disable kernel modesetting.
>
> DRM drivers will not perform display-mode changes or accelerated rendering
> and only the system framebuffer will be available if it was set-up.
>
> But only a few DRM drivers currently check for nomodeset, so let's add two
> helper macros that can be used by DRM drivers for PCI and platform devices
> to have module init functions that checks if the drivers could be loaded.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> (no changes since v1)
>
> include/drm/drm_drv.h | 50 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h

I worked on a similar patch today and found that drm_drv.h is actually
not a good place. Half of DRM includes this file and now it all depends
on linux/pci.h and linux/platform.h (and probably other later).

I propose to put the module helpers into <drm/drm_module.h> and include
it where necessary.

> index f6159acb8856..4001d73428c5 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -29,6 +29,8 @@
>
> #include <linux/list.h>
> #include <linux/irqreturn.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
>
> #include <drm/drm_device.h>
>
> @@ -604,4 +606,52 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name);
>
> extern bool drm_firmware_drivers_only(void);
>
> +/**
> + * drm_pci_register_driver() - register a DRM driver for PCI devices
> + * @drv: PCI driver structure
> + *
> + * Returns zero on success or a negative errno code on failure.
> + */
> +static inline int drm_pci_register_driver(struct pci_driver *drv)

This should be declared as __init, so it goes into a separate section of
the module. IIRC the page in the init section are released after the
module has been loaded.

I'd either not document the register functions, or explicitly say that
the module macros are the preferred way of using them.

> +{
> + if (drm_firmware_drivers_only())
> + return -ENODEV;
> +
> + return pci_register_driver(drv);
> +}
> +
> +/**
> + * drm_module_pci_driver() - helper macro for registering a DRM PCI driver

Docs for the __pci_driver argument

> + *
> + * Helper macro for DRM PCI drivers which do not do anything special in their
> + * module init/exit and just need the DRM specific module init.
> + */
> +#define drm_module_pci_driver(__pci_driver) \
> + module_driver(__pci_driver, drm_pci_register_driver, \
> + pci_unregister_driver)
> +
> +/**
> + * drm_platform_driver_register - register a DRM driver for platform devices
> + * @drv: platform driver structure
> + *
> + * Returns zero on success or a negative errno code on failure.
> + */
> +static inline int drm_platform_driver_register(struct platform_driver *drv)


> +{
> + if (drm_firmware_drivers_only())
> + return -ENODEV;
> +
> + return platform_driver_register(drv);
> +}
> +
> +/**
> + * drm_module_platform_driver() - helper macro for registering a DRM platform driver

Docs for the __platform_driver argument

Best regards
Thomas

> + *
> + * Helper macro for DRM platform drivers which do not do anything special in their
> + * module init/exit and just need the DRM specific module init.
> + */
> +#define drm_module_platform_driver(__platform_driver) \
> + module_driver(__platform_driver, drm_platform_driver_register, \
> + platform_driver_unregister)
> +
> #endif
>

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-12-17 15:33    [W:0.136 / U:1.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site