Re: [PATCH v2 01/37] drm: Add drm_module_{pci,platform}_driver() helper macros

[Thomas Zimmermann]

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@xxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> ---
>
> (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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature