[Intel-gfx] [PATCH] drm/i915: add register macro definition style guide

Mon Aug 7 17:01:15 UTC 2017

good idea.

On Mon, Aug 7, 2017 at 9:10 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Fri, Aug 04, 2017 at 01:38:36PM +0300, Jani Nikula wrote:
>> This is not to try to force a new style; this is my interpretation of
>> what the most common existing style is.
>>
>> With hopes I don't need to answer so many questions about style going
>> forward.
>>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>
>> ---
>>
>> N.b. only the *interpretation* of the existing style is up for debate
>> here. Proposals to *change* the style going forward can be done in other
>> patches changing this description. However, I doubt the usefulness of
>> such changes, with the possible exception of promoting the use of BIT().
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 77 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b2546ade2c45..324cf04d6bfe 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -25,6 +25,83 @@
>>  #ifndef _I915_REG_H_
>>  #define _I915_REG_H_
>>
>> +/*
>
> DOC: section, plus pull it into our kerneldoc?
>
>> + * The i915 register macro definition style guide.
>> + *
>> + * Follow the style described here for new macros, and while changing existing
>> + * macros. Do not mass change existing definitions just to update the style.
>> + *
>> + * LAYOUT
>> + *
>> + * Keep helper macros near the top. For example, _PIPE() and friends.
>> + *
>> + * Prefix macros that generally should not be used outside of this file with
>> + * underscore '_'. For example, _PIPE() and friends, single instances of
>> + * registers that are defined solely for the use by function-like macros.
>> + *
>> + * Avoid using the underscore prefixed macros outside of this file. There are
>> + * exceptions, but keep them to a minimum.
>> + *
>> + * There are two basic types of register definitions: Single registers and
>> + * register groups. Register groups are registers which have two or more
>> + * instances, for example one per pipe, port, transcoder, etc. Register groups
>> + * should be defined using function-like macros.
>> + *
>> + * For single registers, define the register offset first, followed by register
>> + * contents.
>> + *
>> + * For register groups, define the register instance offsets first, prefixed
>> + * with underscore, followed by a function-like macro choosing the right
>> + * instance based on the parameter, followed by register contents.
>> + *
>> + * Define the register contents from most significant to least significant
>> + * bit. Indent the bit and bit field macros using two extra spaces between
>> + * #define and the macro name.
>
> Maybe note that since hw engineers love to use bit 31 for enabling a block
> this gives some natural ordering.
>
>> + *
>> + * For bit fields, define a _MASK and a _SHIFT macro. Define bit field contents
>> + * so that they are already shifted in place, and can be directly OR'd. For
>> + * convenience, function-like macros may be used to define bit fields, but do
>> + * note that the macros may be needed to read as well as write the register
>> + * contents.

I'd mention define as needed if needed...
to avoid people adding many definition for shifts and masks that will
never get used...

Also I prefer the function-like macros to set the bits if we only need
to set but never read back...

>> + *
>> + * Define bits using (1 << N) instead of BIT(N). We may change this in the
>> + * future, but this is the prevailing style.
>> + *
>> + * Group the register and its contents together without blank lines, separate
>> + * from other registers and their contents with one blank line.
>> + *
>> + * Indent macro values from macro names using TABs. Use braces in macro values
>> + * as needed to avoid unintended precedence after macro substitution. Use spaces
>> + * in macro values according to kernel coding style. Use lower case in
>> + * hexadecimal values.
>
> I think we should add:
>
> "Indent register contents macros by an additional space, to set them off
> from the register they are for."

agree.

also maybe to add that between name and content the real tabs are required
as much tab as needed to make sure that all bits defines on that bit
is vertically aligned.

but with or without any change:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

>
> Feel free to reword/place more suitably.
>
>> + *
>> + * NAMING
>> + *
>> + * Try to name registers according to the specs. If the register name changes in
>> + * the specs from platform to another, stick to the original name.
>> + *
>> + * Try to re-use existing register macro definitions. Only add new macros for
>> + * new register offsets, or when the register contents have changed enough to
>> + * warrant a full redefinition.
>> + *
>> + * When a register or a bit (field) changes for a new platform, prefix the new
>> + * macro using the platform acronym or generation. For example, SKL_ or
>> + * GEN8_. The prefix signifies the start platform/generation of the register.
>
> s/of/using/
>
> Note that we also have piles of register definitions using platform
> postfix. That tends to be used when we have an extension of an existing
> register (i.e. for new bit values), instead of a completely new register
> set.
>
> Since you want to just describe the current style I think this should be
> added.
>
> I'll leave the nits to your judgement, but imo the kerneldoc DOC: section
> should be done. With that:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
>> + *
>> + * EXAMPLE
>> + *
>> + * #define _FOO_A                    0xf000
>> + * #define _FOO_B                    0xf001
>> + * #define FOO(pipe)                 _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>> + * #define   FOO_ENABLE                      (1 << 31)
>> + * #define   FOO_MODE_MASK           (0xf << 16)
>> + * #define   FOO_MODE_SHIFT          16
>> + * #define   FOO_MODE_BAR            (0 << 16)
>> + * #define   FOO_MODE_BAZ            (1 << 16)
>> + * #define   GEN6_FOO_MODE_QUX               (2 << 16)
>> + *
>> + */
>> +
>>  typedef struct {
>>       uint32_t reg;
>>  } i915_reg_t;
>> --
>> 2.11.0
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br