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
