[Intel-gfx] [PATCH 02/11] drm: Consolidate and document sysfs support

Tue Apr 4 14:46:21 UTC 2017

On 04/04/2017 11:52 AM, Daniel Vetter wrote:
> - remove docs for internal func, doesn't add value
> - add short overview snippet instead explaining that drivers don't
>   have to bother themselves with reg/unreg concerns
> - drop the ttm comment about drmP.h, drmP.h is disappearing ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  Documentation/gpu/drm-uapi.rst | 10 ++++++
>  drivers/gpu/drm/drm_sysfs.c    | 70 ++++++++++++++++--------------------------
>  include/drm/drmP.h             |  5 +--
>  include/drm/drm_sysfs.h        |  8 ++---
>  4 files changed, 42 insertions(+), 51 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 76356c86e358..8b0f0e403f0c 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -219,6 +219,16 @@ Debugfs Support
>  .. kernel-doc:: drivers/gpu/drm/drm_debugfs.c
>     :export:
>  
> +Sysfs Support
> +=============
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_sysfs.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_sysfs.c
> +   :export:
> +
> +
>  VBlank event handling
>  =====================
>  
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 513288b5c2f6..1c5b5ce1fd7f 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -25,6 +25,20 @@
>  #define to_drm_minor(d) dev_get_drvdata(d)
>  #define to_drm_connector(d) dev_get_drvdata(d)
>  
> +/**
> + * DOC: overview
> + *
> + * DRM provides very little additional support to drivers for sysfs
> + * interactions, beyond just all the standard stuff. Drivers who want to expose
> + * additional sysfs properties and property groups can attach them at either
> + * &drm_device.dev or &drm_connector.kdev.
> + *
> + * Registration is automatically handled when calling drm_dev_register(), or
> + * drm_connector_register() in case of hot-plugged connectors. Unregistration is
> + * also automatically handled by drm_dev_unregister() and
> + * drm_connector_unregister().
> + */
> +
>  static struct device_type drm_sysfs_device_minor = {
>  	.name = "drm_minor"
>  };
> @@ -250,15 +264,6 @@ static const struct attribute_group *connector_dev_groups[] = {
>  	NULL
>  };
>  
> -/**
> - * drm_sysfs_connector_add - add a connector to sysfs
> - * @connector: connector to add
> - *
> - * Create a connector device in sysfs, along with its associated connector
> - * properties (so far, connection status, dpms, mode list and edid) and
> - * generate a hotplug event so userspace knows there's a new connector
> - * available.
> - */
>  int drm_sysfs_connector_add(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
> @@ -285,19 +290,6 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  	return 0;
>  }
>  
> -/**
> - * drm_sysfs_connector_remove - remove an connector device from sysfs
> - * @connector: connector to remove
> - *
> - * Remove @connector and its associated attributes from sysfs.  Note that
> - * the device model core will take care of sending the "remove" uevent
> - * at this time, so we don't need to do it.
> - *
> - * Note:
> - * This routine should only be called if the connector was previously
> - * successfully registered.  If @connector hasn't been registered yet,
> - * you'll likely see a panic somewhere deep in sysfs code when called.
> - */
>  void drm_sysfs_connector_remove(struct drm_connector *connector)
>  {
>  	if (!connector->kdev)
> @@ -333,20 +325,6 @@ static void drm_sysfs_release(struct device *dev)
>  	kfree(dev);
>  }
>  
> -/**
> - * drm_sysfs_minor_alloc() - Allocate sysfs device for given minor
> - * @minor: minor to allocate sysfs device for
> - *
> - * This allocates a new sysfs device for @minor and returns it. The device is
> - * not registered nor linked. The caller has to use device_add() and
> - * device_del() to register and unregister it.
> - *
> - * Note that dev_get_drvdata() on the new device will return the minor.
> - * However, the device does not hold a ref-count to the minor nor to the
> - * underlying drm_device. This is unproblematic as long as you access the
> - * private data only in sysfs callbacks. device_del() disables those
> - * synchronously, so they cannot be called after you cleanup a minor.
> - */
>  struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>  {
>  	const char *minor_str;
> @@ -384,15 +362,13 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>  }
>  
>  /**
> - * drm_class_device_register - Register a struct device in the drm class.
> + * drm_class_device_register - register new device with the DRM sysfs class
> + * @dev: device to register
>   *
> - * @dev: pointer to struct device to register.
> - *
> - * @dev should have all relevant members pre-filled with the exception
> - * of the class member. In particular, the device_type member must
> - * be set.
> + * Registers a new &struct device within the DRM sysfs class. Essentially only
> + * used by ttm to have a place for its global settings. Drivers should never use
> + * this.
>   */
> -
>  int drm_class_device_register(struct device *dev)
>  {
>  	if (!drm_class || IS_ERR(drm_class))
> @@ -403,6 +379,14 @@ int drm_class_device_register(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(drm_class_device_register);
>  
> +/**
> + * drm_class_device_unregister - unregister device with the DRM sysfs class
> + * @dev: device to unregister
> + *
> + * Unregisters a &struct device from the DRM sysfs class. Essentially only used
> + * by ttm to have a place for its global settings. Drivers should never use
> + * this.
> + */
>  void drm_class_device_unregister(struct device *dev)
>  {
>  	return device_unregister(dev);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3bfafcdb8710..e1daa4f343cd 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -80,6 +80,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_debugfs.h>
>  #include <drm/drm_ioctl.h>
> +#include <drm/drm_sysfs.h>
>  
>  struct module;
>  
> @@ -512,10 +513,6 @@ static inline int drm_device_is_unplugged(struct drm_device *dev)
>   * DMA quiscent + idle. DMA quiescent usually requires the hardware lock.
>   */
>  
> -			       /* sysfs support (drm_sysfs.c) */
> -extern void drm_sysfs_hotplug_event(struct drm_device *dev);
> -
> -
>  /*@}*/
>  
>  /* returns true if currently okay to sleep */
> diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> index 23418c1f10d1..70c9a1074aca 100644
> --- a/include/drm/drm_sysfs.h
> +++ b/include/drm/drm_sysfs.h
> @@ -1,12 +1,12 @@
>  #ifndef _DRM_SYSFS_H_
>  #define _DRM_SYSFS_H_
>  
> -/**
> - * This minimalistic include file is intended for users (read TTM) that
> - * don't want to include the full drmP.h file.
> - */
> +struct drm_device;
> +struct device;
>  
>  int drm_class_device_register(struct device *dev);
>  void drm_class_device_unregister(struct device *dev);
>  
> +void drm_sysfs_hotplug_event(struct drm_device *dev);
> +
>  #endif
> 

Reviewed-by: Neil Armstrong <narmstrong at baylibre.com>