* [PATCH] drm/i915: add register macro definition style guide
@ 2017-08-04 10:38 Jani Nikula
2017-08-04 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jani Nikula @ 2017-08-04 10:38 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula, Daniel Vetter
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.
+ *
+ * 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;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: add register macro definition style guide
2017-08-04 10:38 [PATCH] drm/i915: add register macro definition style guide Jani Nikula
@ 2017-08-04 10:54 ` Patchwork
2017-08-07 16:10 ` [PATCH] " Daniel Vetter
2017-08-10 3:53 ` Pandiyan, Dhinakaran
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2017-08-04 10:54 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: add register macro definition style guide
URL : https://patchwork.freedesktop.org/series/28370/
State : success
== Summary ==
Series 28370v1 drm/i915: add register macro definition style guide
https://patchwork.freedesktop.org/api/1.0/series/28370/revisions/1/mbox/
Test gem_sync:
Subgroup basic-store-all:
fail -> PASS (fi-ivb-3520m) fdo#100007
Test kms_flip:
Subgroup basic-flip-vs-modeset:
skip -> PASS (fi-skl-x1585l) fdo#101781
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (fi-pnv-d510) fdo#101597
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-byt-j1900) fdo#101705
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:433s
fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:417s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:366s
fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:505s
fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:504s
fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:528s
fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:511s
fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:587s
fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:427s
fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:411s
fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:419s
fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:512s
fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:476s
fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:463s
fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:565s
fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:578s
fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:565s
fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:450s
fi-skl-6700k total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:642s
fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:464s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:424s
fi-skl-x1585l total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:485s
fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:552s
fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:408s
0394eb2d550a7b53adb3e36d0b79904ab8f1ffb2 drm-tip: 2017y-08m-04d-09h-39m-16s UTC integration manifest
f3b77466bab4 drm/i915: add register macro definition style guide
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5326/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: add register macro definition style guide
2017-08-04 10:38 [PATCH] drm/i915: add register macro definition style guide Jani Nikula
2017-08-04 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-08-07 16:10 ` Daniel Vetter
2017-08-07 17:01 ` Rodrigo Vivi
2017-08-08 8:31 ` Jani Nikula
2017-08-10 3:53 ` Pandiyan, Dhinakaran
2 siblings, 2 replies; 7+ messages in thread
From: Daniel Vetter @ 2017-08-07 16:10 UTC (permalink / raw)
To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx
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
>
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: add register macro definition style guide
2017-08-07 16:10 ` [PATCH] " Daniel Vetter
@ 2017-08-07 17:01 ` Rodrigo Vivi
2017-08-08 8:38 ` Jani Nikula
2017-08-08 8:31 ` Jani Nikula
1 sibling, 1 reply; 7+ messages in thread
From: Rodrigo Vivi @ 2017-08-07 17:01 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Jani Nikula, Daniel Vetter, intel-gfx
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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: add register macro definition style guide
2017-08-07 16:10 ` [PATCH] " Daniel Vetter
2017-08-07 17:01 ` Rodrigo Vivi
@ 2017-08-08 8:31 ` Jani Nikula
1 sibling, 0 replies; 7+ messages in thread
From: Jani Nikula @ 2017-08-08 8:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, 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
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: add register macro definition style guide
2017-08-07 17:01 ` Rodrigo Vivi
@ 2017-08-08 8:38 ` Jani Nikula
0 siblings, 0 replies; 7+ messages in thread
From: Jani Nikula @ 2017-08-08 8:38 UTC (permalink / raw)
To: Rodrigo Vivi, Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
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.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: add register macro definition style guide
2017-08-04 10:38 [PATCH] drm/i915: add register macro definition style guide Jani Nikula
2017-08-04 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-08-07 16:10 ` [PATCH] " Daniel Vetter
@ 2017-08-10 3:53 ` Pandiyan, Dhinakaran
2 siblings, 0 replies; 7+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-08-10 3:53 UTC (permalink / raw)
To: Nikula, Jani; +Cc: daniel.vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-08-10 3:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-04 10:38 [PATCH] drm/i915: add register macro definition style guide Jani Nikula
2017-08-04 10:54 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-08-07 16:10 ` [PATCH] " Daniel Vetter
2017-08-07 17:01 ` Rodrigo Vivi
2017-08-08 8:38 ` Jani Nikula
2017-08-08 8:31 ` Jani Nikula
2017-08-10 3:53 ` Pandiyan, Dhinakaran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.