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

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Fri, 2017-08-04 at 13:38 +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_
>  
> +/*
> + * 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.
> + *
> + * 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.

Thanks for writing this!

Do you mind including an example for defining bit-fields using
function-like macros?

With or without that, since this guide agrees with most of the existing
definitions in i915_reg.h
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> + *
> + * 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.
> + *
> + * 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.
> + *
> + * 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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx