[PATCH] drm/amdgpu: update documentation for amdgpu_irq.c

[Michel Dänzer]

Hi Slava,


On 2018-06-07 11:46 PM, Slava Abramov wrote:
> Add/update function level documentation and add reference to amdgpu_irq.c
> in amdgpu.rst
> 
> Signed-off-by: Slava Abramov <slava.abramov at amd.com>

Thanks for working on this. I have some pretty detailed feedback below,
but other than that, it looks good.


> diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> index a4852f9a6141..63b0bc3c30d1 100644
> --- a/Documentation/gpu/amdgpu.rst
> +++ b/Documentation/gpu/amdgpu.rst
> @@ -44,3 +44,12 @@ MMU Notifier
>  
>  .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>     :internal:
> +
> +Interrupt Handling
> +------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +   :doc: Interrupt Handling
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +   :internal:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 3a5ca462abf0..07a2642de634 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -25,6 +25,11 @@
>   *          Alex Deucher
>   *          Jerome Glisse
>   */
> +
> +/**
> + * DOC: Interrupt Handling
> + */

An empty DOC comment is pointless. :) Can you add some introductory /
overview text here? If not, remove this comment and the corresponding
two lines with :doc: in amdgpu.rst.


> @@ -44,7 +49,7 @@
>  #define AMDGPU_WAIT_IDLE_TIMEOUT 200
>  
>  /*
> - * Handle hotplug events outside the interrupt handler proper.
> + * XXX: handle hotplug events outside the interrupt handler proper.

This doesn't need XXX. It's saying that hotplug event handling is
deferred from the interrupt handler to a work handler, which is required
because mutexes cannot be locked in an interrupt handler (because
mutex_lock may sleep).

Maybe this could be integrated into the amdgpu_hotplug_work_func
function comment.


> @@ -142,14 +154,12 @@ irqreturn_t amdgpu_irq_handler(int irq, void *arg)
>  }
>  
>  /**
> - * amdgpu_msi_ok - asic specific msi checks
> + * amdgpu_msi_ok - check whether MSI functionality is enabled
>   *
> - * @adev: amdgpu device pointer
> + * @adev: amdgpu device pointer (**not used**)

The convention seems to be just "not used" or "unused", without
asterisks (same for other function comments).

Actually, here in amdgpu_msi_ok the unused adev parameter should
probably simply be removed (in a separate change).


> - * Handles asic specific MSI checks to determine if
> - * MSIs should be enabled on a particular chip (all asics).

Consider keeping the "(all ASICs)" here as well, for consistency with
the other function comments.

> - * Returns true if MSIs should be enabled, false if MSIs
> - * should not be enabled.
> + * Checks whether MSI functionality has been disabled via module parameter.
> + * Returns true if MSIs are allowed to be enabled or false otherwise

The format for describing a return value is:

* Returns:
* true if MSIs are allowed to be enabled or false otherwise

(same for other function comments)


> @@ -176,7 +187,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  
>  	spin_lock_init(&adev->irq.lock);
>  
> -	/* enable msi */
> +	/* enable msi if not disabled by module parameter */

Acronyms like MSI should be spelled as all capitals in prose. (There are
more acronyms in other comments)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer