[Intel-gfx] i915 vs checkpatch

[Rodrigo Vivi]

On Thu, Mar 01, 2018 at 06:13:31PM +0200, Jani Nikula wrote:
> 
> I went through the recent checkpatch reports, and here's my take.
> 
> On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler at intel.com> wrote:
> >  2. Which of the checkpatch checks we want to disabled for i915?
> 
> I'd like to have these silenced:
> 
> CHECK: No space is necessary after a cast
> WARNING: line over 80 characters
> WARNING: quoted string split across lines
> 
> I'd prefer we conform to the last two too, but there's just too much
> noise and too many cases where we explicitly should ignore them.
> 
> For the time being, I think we may have to silence these ones too, but
> I'd like us to discuss enforcing them:
> 
> CHECK: Prefer kernel type 'u16' over 'uint16_t'
> CHECK: Prefer kernel type 'u32' over 'uint32_t'
> CHECK: Prefer kernel type 'u64' over 'uint64_t'
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
> CHECK: Prefer using the BIT macro
> 
> The BIT macros is one that I'd consider accepting a one-time conversion
> of i915_reg.h and after that use it exclusively. But up to debate.

For this one I just wonder if we would need to do a massive
change before. Because it would get ugly to have mixed cases.

ack on all the rest of the list here on this email.

> 
> >  3. How strongly do we want to enforce the rest?
> 
> It depends on the case. Some of the warnings are notes to check, will be
> emitted even for correct code, and can't be automatically enforced.
> 
> Of the recently reported ones, I'd like to enforce:
> 
> CHECK: Alignment should match open parenthesis
> CHECK: Blank lines aren't necessary after an open brace '{'
> CHECK: Lines should not end with a '('
> CHECK: Please don't use multiple blank lines
> CHECK: Please use a blank line after function/struct/union/enum declarations
> CHECK: Unbalanced braces around else statement
> CHECK: Unnecessary parentheses around 'level >= 1'
> CHECK: Unnecessary parentheses around 'pipe == PIPE_A'
> CHECK: braces {} should be used on all arms of this statement
> CHECK: multiple assignments should be avoided
> CHECK: spaces preferred around that '*' (ctx:VxV)
> CHECK: spaces preferred around that '<<' (ctx:VxV)
> CHECK: spinlock_t definition without comment
> CHECK: struct mutex definition without comment
> ERROR: Missing Signed-off-by: line(s)
> ERROR: Unrecognized email address: Foo Bar <foo.bar at example.com'
> ERROR: code indent should use tabs where possible
> ERROR: spaces required around that '=' (ctx:VxW)
> ERROR: trailing whitespace
> WARNING: 'consistancy' may be misspelled - perhaps 'consistency'?
> WARNING: 'writting' may be misspelled - perhaps 'writing'?
> WARNING: Missing a blank line after declarations
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> WARNING: Statements should start on a tabstop
> WARNING: please, no space before tabs
> WARNING: please, no spaces at the start of a line
> WARNING: suspect code indent for conditional statements (8, 17)
> 
> These ones should be double checked by author/reviewer/committer. These
> can't be automatically enforced:
> 
> CHECK: Macro argument reuse 'info' - possible side-effects?
> ERROR: Macros with complex values should be enclosed in parentheses
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
> WARNING: Macros with flow control statements should be avoided
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> 
> If we have the filtering in place in dim, we could require the committer
> to pass a parameter to dim to apply patches with the above warnings.
> 
> >  4. Do we want to change what's already in the tree, for compliance?
> 
> With the exception of the BIT() macro, I still think not. But patch
> series touching existing code should move towards compliance (for want
> of a better word).
> 
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dim-tools mailing list
> dim-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools