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