Re: [Intel-gfx] [PATCH] drm/i915: add register macro definition style guide
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
> Signed-off-by: Jani Nikula
>
> ---
>
> 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
> + *
> + * 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_A0xf000
> + * #define _FOO_B0xf001
> + * #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
Re: [Intel-gfx] [PATCH] drm/i915: add register macro definition style guide
On Mon, 07 Aug 2017, Rodrigo Vivi wrote: > good idea. > > > On Mon, Aug 7, 2017 at 9:10 AM, Daniel Vetter 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 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
Re: [Intel-gfx] [PATCH] drm/i915: add register macro definition style guide
On Mon, 07 Aug 2017, Daniel Vetter 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 >> Signed-off-by: Jani Nikula >> >> --- >> >> 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 acro
Re: [Intel-gfx] [PATCH] drm/i915: add register macro definition style guide
good idea. On Mon, Aug 7, 2017 at 9:10 AM, Daniel Vetter 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 >> Signed-off-by: Jani Nikula >> >> --- >> >> 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 > > 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 o
Re: [Intel-gfx] [PATCH] drm/i915: add register macro definition style guide
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 > Signed-off-by: Jani Nikula > > --- > > 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 > + * > + * EXAMPLE > + * > + * #define _FOO_A0xf000 > + * #define _FOO_B0xf
