drm/i915: add register macro definition style guide

Submitted by Jani Nikula on Aug. 4, 2017, 10:38 a.m.

Details

Message ID 20170804103836.17629-1-jani.nikula@intel.com
State New
Headers show
Series "drm/i915: add register macro definition style guide" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Aug. 4, 2017, 10:38 a.m.
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(+)

Patch hide | download patch | download mbox

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

Comments

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?

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

> + *
> + * 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@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
>
good idea.


On Mon, Aug 7, 2017 at 9:10 AM, 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?
>
>> + * 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@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
>>
On Mon, 07 Aug 2017, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> good idea.
>
>
> On Mon, Aug 7, 2017 at 9:10 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Aug 04, 2017 at 01:38:36PM +0300, Jani Nikula wrote:
>>
>>> + *
>>> + * 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...

With mask and shift defined you don't need to always look at the spec.

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

Added.

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

Thanks.

BR,
Jani.
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;