Re: [PATCH 3/8] drm/edid: Cleanup kerneldoc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 29, 2014 at 11:44:34AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> This fixes up a couple of inconsistencies with kerneldoc comments for
> the EDID related functions. Functions that return a value now use the
> "Return" section consistently. For some exported symbols the kernel-doc
> comments were not in the appropriate format and therefore not included
> in the generated documentation. Others had no kernel-doc at all.
> 
> While at it, fix a couple of whitespace issues and capitalize common
> abbreviations (i2c -> I2C, edid -> EDID, ...).
> 
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>

Two comments below for further polish, but this looks good imo.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> ---
>  drivers/gpu/drm/drm_edid.c | 128 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 79 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 696186401955..0a52ded1b380 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -984,9 +984,13 @@ static const u8 edid_header[] = {
>  	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>  };
>  
> - /*
> - * Sanity check the header of the base EDID block.  Return 8 if the header
> - * is perfect, down to 0 if it's totally wrong.
> +/**
> + * drm_edid_header_is_valid - sanity check the header of the base EDID block
> + * @raw_edid: pointer to raw base EDID block
> + *
> + * Sanity check the header of the base EDID block.
> + *
> + * Return: 8 if the header is perfect, down to 0 if it's totally wrong.
>   */
>  int drm_edid_header_is_valid(const u8 *raw_edid)
>  {
> @@ -1005,9 +1009,16 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
>  MODULE_PARM_DESC(edid_fixup,
>  		 "Minimum number of valid EDID header bytes (0-8, default 6)");
>  
> -/*
> - * Sanity check the EDID block (base or extension).  Return 0 if the block
> - * doesn't check out, or 1 if it's valid.
> +/**
> + * drm_edid_block_valid - Sanity check the EDID block (base or extension)
> + * @raw_edid: pointer to raw EDID block
> + * @block: type of block to validate (0 for base, extension otherwise)
> + * @print_bad_edid: if true, dump bad EDID blocks to the console
> + *
> + * Validate a base or extension EDID block and optionally dump bad blocks to
> + * the console.
> + *
> + * Return: True if the block is valid, false otherwise.
>   */
>  bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>  {
> @@ -1077,6 +1088,8 @@ EXPORT_SYMBOL(drm_edid_block_valid);
>   * @edid: EDID data
>   *
>   * Sanity-check an entire EDID record (including extensions)
> + *
> + * Return: True if the EDID data is valid, false otherwise.
>   */
>  bool drm_edid_is_valid(struct edid *edid)
>  {
> @@ -1096,18 +1109,15 @@ EXPORT_SYMBOL(drm_edid_is_valid);
>  
>  #define DDC_SEGMENT_ADDR 0x30
>  /**
> - * Get EDID information via I2C.
> - *
> - * @adapter : i2c device adaptor
> + * drm_do_probe_ddc_edid() - get EDID information via I2C
> + * @adapter: I2C device adaptor
>   * @buf: EDID data buffer to be filled
>   * @block: 128 byte EDID block to start fetching from
>   * @len: EDID data buffer length to fetch
>   *
> - * Returns:
> - *
> - * 0 on success or -1 on failure.
> + * Try to fetch EDID information by calling I2C driver functions.
>   *
> - * Try to fetch EDID information by calling i2c driver function.
> + * Return: 0 on success or -1 on failure.

I wonder whether we shouldn't just switch to the common negative error
codes as the return value on failure, with -ENXIO as-is and -EIO when
running out of retry attempts. Just an idea for a follow-up patch.

>   */
>  static int
>  drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> @@ -1118,7 +1128,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>  	unsigned char xfers = segment ? 3 : 2;
>  	int ret, retries = 5;
>  
> -	/* The core i2c driver will automatically retry the transfer if the
> +	/*
> +	 * The core I2C driver will automatically retry the transfer if the
>  	 * adapter reports EAGAIN. However, we find that bit-banging transfers
>  	 * are susceptible to errors under a heavily loaded machine and
>  	 * generate spurious NAKs and timeouts. Retrying the transfer
> @@ -1144,10 +1155,10 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>  			}
>  		};
>  
> -	/*
> -	 * Avoid sending the segment addr to not upset non-compliant ddc
> -	 * monitors.
> -	 */
> +		/*
> +		 * Avoid sending the segment addr to not upset non-compliant
> +		 * DDC monitors.
> +		 */
>  		ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
>  
>  		if (ret == -ENXIO) {
> @@ -1246,12 +1257,10 @@ out:
>  }
>  
>  /**
> - * Probe DDC presence.
> - * @adapter: i2c adapter to probe
> + * drm_probe_ddc() - probe DDC presence
> + * @adapter: I2C adapter to probe
>   *
> - * Returns:
> - *
> - * 1 on success
> + * Return: True on success, false on failure.
>   */
>  bool
>  drm_probe_ddc(struct i2c_adapter *adapter)
> @@ -1265,12 +1274,12 @@ EXPORT_SYMBOL(drm_probe_ddc);
>  /**
>   * drm_get_edid - get EDID data, if available
>   * @connector: connector we're probing
> - * @adapter: i2c adapter to use for DDC
> + * @adapter: I2C adapter to use for DDC
>   *
> - * Poke the given i2c channel to grab EDID data if possible.  If found,
> + * Poke the given I2C channel to grab EDID data if possible.  If found,
>   * attach it to the connector.
>   *
> - * Return edid data or NULL if we couldn't find any.
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
>   */
>  struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
> @@ -1288,7 +1297,7 @@ EXPORT_SYMBOL(drm_get_edid);
>   * drm_edid_duplicate - duplicate an EDID and the extensions
>   * @edid: EDID to duplicate
>   *
> - * Return duplicate edid or NULL on allocation failure.
> + * Return: Pointer to duplicated EDID or NULL on allocation failure.
>   */
>  struct edid *drm_edid_duplicate(const struct edid *edid)
>  {
> @@ -1411,7 +1420,8 @@ mode_is_rb(const struct drm_display_mode *mode)
>   * @rb: Mode reduced-blanking-ness
>   *
>   * Walk the DMT mode list looking for a match for the given parameters.
> - * Return a newly allocated copy of the mode, or NULL if not found.
> + *
> + * Return: A newly allocated copy of the mode, or NULL if not found.
>   */
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
> @@ -2139,7 +2149,7 @@ do_established_modes(struct detailed_timing *timing, void *c)
>  
>  /**
>   * add_established_modes - get est. modes from EDID and add them
> - * @connector: connector of for the EDID block
> + * @connector: connector to add mode(s) to
>   * @edid: EDID block to scan
>   *
>   * Each EDID block contains a bitmap of the supported "established modes" list
> @@ -2201,7 +2211,7 @@ do_standard_modes(struct detailed_timing *timing, void *c)
>  
>  /**
>   * add_standard_modes - get std. modes from EDID and add them
> - * @connector: connector of for the EDID block
> + * @connector: connector to add mode(s) to
>   * @edid: EDID block to scan
>   *
>   * Standard modes can be calculated using the appropriate standard (DMT,
> @@ -2422,7 +2432,7 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode)
>   * drm_match_cea_mode - look for a CEA mode matching given mode
>   * @to_match: display mode
>   *
> - * Returns the CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
> + * Return: The CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
>   * mode.
>   */
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> @@ -3020,11 +3030,9 @@ monitor_name(struct detailed_timing *t, void *data)
>   * @connector: connector corresponding to the HDMI/DP sink
>   * @edid: EDID to parse
>   *
> - * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver.
> - * Some ELD fields are left to the graphics driver caller:
> - * - Conn_Type
> - * - HDCP
> - * - Port_ID
> + * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver. The
> + * Conn_Type, HDCP and Port_ID ELD fields are left for the graphics driver to
> + * fill in.
>   */
>  void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3108,9 +3116,10 @@ EXPORT_SYMBOL(drm_edid_to_eld);
>   * @sads: pointer that will be set to the extracted SADs
>   *
>   * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
> - * Note: returned pointer needs to be kfreed
>   *
> - * Return number of found SADs or negative number on error.
> + * Note: The returned pointer needs to be freed using kfree().
> + *
> + * Return: The number of found SADs or negative number on error.
>   */
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
>  {
> @@ -3167,9 +3176,11 @@ EXPORT_SYMBOL(drm_edid_to_sad);
>   * @sadb: pointer to the speaker block
>   *
>   * Looks for CEA EDID block and extracts the Speaker Allocation Data Block from it.
> - * Note: returned pointer needs to be kfreed
>   *
> - * Return number of found Speaker Allocation Blocks or negative number on error.
> + * Note: The returned pointer needs to be freed using kfree().
> + *
> + * Return: The number of found Speaker Allocation Blocks or negative number on
> + * error.
>   */
>  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
>  {
> @@ -3216,9 +3227,12 @@ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
>  EXPORT_SYMBOL(drm_edid_to_speaker_allocation);
>  
>  /**
> - * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
> + * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay
>   * @connector: connector associated with the HDMI/DP sink
>   * @mode: the display mode
> + *
> + * Return: The HDMI/DP sink's audio-video sync delay in milliseconds or 0 if
> + * the sink doesn't support audio or video.
>   */
>  int drm_av_sync_delay(struct drm_connector *connector,
>  		      struct drm_display_mode *mode)
> @@ -3260,6 +3274,9 @@ EXPORT_SYMBOL(drm_av_sync_delay);
>   *
>   * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
>   * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
> + *
> + * Return: The connector associated with the first HDMI/DP sink that has ELD
> + * attached to it.
>   */
>  struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  				     struct drm_display_mode *mode)
> @@ -3276,11 +3293,12 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  EXPORT_SYMBOL(drm_select_eld);
>  
>  /**
> - * drm_detect_hdmi_monitor - detect whether monitor is hdmi.
> + * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>   * @edid: monitor EDID information
>   *
>   * Parse the CEA extension according to CEA-861-B.
> - * Return true if HDMI, false if not or unknown.
> + *
> + * Return: True if the monitor is HDMI, false if not or unknown.
>   */
>  bool drm_detect_hdmi_monitor(struct edid *edid)
>  {
> @@ -3318,6 +3336,7 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>   * audio format, assume at least 'basic audio' support, even if 'basic
>   * audio' is not defined in EDID.
>   *
> + * Return: True if the monitor supports audio, false otherwise.
>   */
>  bool drm_detect_monitor_audio(struct edid *edid)
>  {
> @@ -3361,6 +3380,8 @@ EXPORT_SYMBOL(drm_detect_monitor_audio);
>   * Check whether the monitor reports the RGB quantization range selection
>   * as supported. The AVI infoframe can then be used to inform the monitor
>   * which quantization range (full or limited) is used.
> + *
> + * Return: True if the RGB quantization range is selectable, false otherwise.
>   */
>  bool drm_rgb_quant_range_selectable(struct edid *edid)
>  {
> @@ -3465,11 +3486,11 @@ static void drm_add_display_info(struct edid *edid,
>  /**
>   * drm_add_edid_modes - add modes from EDID data, if available
>   * @connector: connector we're probing
> - * @edid: edid data
> + * @edid: EDID data
>   *
>   * Add the specified modes to the connector's mode list.
>   *
> - * Return number of modes added or 0 if we couldn't find any.
> + * Return: The number of modes added or 0 if we couldn't find any.
>   */
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3531,7 +3552,7 @@ EXPORT_SYMBOL(drm_add_edid_modes);
>   * Add the specified modes to the connector's mode list. Only when the
>   * hdisplay/vdisplay is not beyond the given limit, it will be added.
>   *
> - * Return number of modes added or 0 if we couldn't find any.
> + * Return: The number of modes added or 0 if we couldn't find any.
>   */
>  int drm_add_modes_noedid(struct drm_connector *connector,
>  			int hdisplay, int vdisplay)

I wonder whether this wouldn't make more sense in drm_modes.c now, but
again follow-up patch material.

> @@ -3570,13 +3591,22 @@ int drm_add_modes_noedid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_add_modes_noedid);
>  
> +/**
> + * drm_set_preferred_mode - Sets the preferred mode of a connector
> + * @connector: connector whose mode list should be processed
> + * @hpref: horizontal resolution of preferred mode
> + * @vpref: vertical resolution of preferred mode
> + *
> + * Marks a mode as preferred if it matches the resolution specified by @hpref
> + * and @vpref.
> + */
>  void drm_set_preferred_mode(struct drm_connector *connector,
>  			   int hpref, int vpref)
>  {
>  	struct drm_display_mode *mode;
>  
>  	list_for_each_entry(mode, &connector->probed_modes, head) {
> -		if (mode->hdisplay  == hpref &&
> +		if (mode->hdisplay == hpref &&
>  		    mode->vdisplay == vpref)
>  			mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	}
> @@ -3589,7 +3619,7 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
>   * @frame: HDMI AVI infoframe
>   * @mode: DRM display mode
>   *
> - * Returns 0 on success or a negative error code on failure.
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> @@ -3654,7 +3684,7 @@ s3d_structure_from_display_mode(const struct drm_display_mode *mode)
>   * 4k or stereoscopic 3D mode. So when giving any other mode as input this
>   * function will return -EINVAL, error that can be safely ignored.
>   *
> - * Returns 0 on success or a negative error code on failure.
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
> -- 
> 1.9.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux