Re: [PATCH] drm/i915: add register macro definition style guide

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 07 Aug 2017, Daniel Vetter <daniel@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@ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula@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?

I'm confused about what you think we should and should not include in
the sphinx docs. I've seen you remove a bunch of documentation for i915
internal functions that I thought were useful, and you said weren't
needed because they were internal. And here I thought nobody needs to
read this until they're about edit the file. So I thought about this and
opted against.

>
>> + * 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.
>> + *
>> + * 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."
>
> Feel free to reword/place more suitably.

I already have this there: "Indent the bit and bit field macros using
two extra spaces between #define and the macro name."

>> + *
>> + * 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.

Yeah, maybe. Part of the reason I started this. But I didn't think it
was common enough worth mentioning, and I wasn't really sure what the
rule was... Registers prefixed, contents postfixed? Ugh.

BR,
Jani.

> 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@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
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx