[Intel-gfx] [PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Paulo Zanoni]

2014-11-03 10:33 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote:
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 8b80d68..f9ddedc 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>>       return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
>>  }
>>
>> +/**
>> + * intel_pipe_update_start() - start update of a set of display registers
>> + * @crtc: the crtc of which the registers are going to be updated
>> + * @start_vbl_count: vblank counter return pointer used for error checking
>> + *
>> + * Mark the start of an update to pipe registers that should be updated
>> + * atomically regarding vblank. If the next vblank will happens within
>> + * the next 100 us, this function waits until the vblank passes.
>> + *
>> + * After a successful call to this function, interrupts will be disabled
>> + * until a subsequent call to intel_pipe_update_end(). That is done to
>> + * avoid random delays. The value written to @start_vbl_count should be
>> + * supplied to intel_pipe_update_end() for error checking.
>> + *
>> + * Return: true if the call was successful
>> + */
>
> It's nice that people now go overboard with kerneldoc, but I think we need
> to strike a good balance. And in general I think documenting static inline
> functions isn't worth it - they really should be self-explanatory as-is.

But patch 3 exports these functions and uses them from another file.

>
> Documentation is imo only really useful for the bigger stuff, which
> usually means it's used in a few places all over. So non-static functions.

The comments he introduced are useful and helped me review patch 3
without having to look at the function implementation and waste 20
minutes wondering what it was supposed to do.

To me, this patch is an improvement to the codebase, so with or
without the extra '*' chars:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

>
> This one here is a bit a corner-case since it's more of a generic piece of
> infrastructure. But otoh I think in the end we'll have exactly one caller
> of these functions for all atomic plane updates we'll need. Everyone else
> (legacy code, modeset code, ...) will just call that higher-level
> function. Not sure what to do here.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni