Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [CI,1/2] drm/i915/icl: Add allowed DP rates for Icelake
Date: Tue, 12 Jun 2018 14:52:45 -0700	[thread overview]
Message-ID: <20180612215244.GT2228@intel.com> (raw)
In-Reply-To: <87k1r42y0f.fsf@intel.com>

On Tue, Jun 12, 2018 at 11:46:08AM +0300, Jani Nikula wrote:
> On Mon, 11 Jun 2018, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > Em Seg, 2018-06-11 às 22:35 +0000, Patchwork escreveu:
> >> == Series Details ==
> >> 
> >> Series: series starting with [CI,1/2] drm/i915/icl: Add allowed DP
> >> rates for Icelake
> >> URL   : https://patchwork.freedesktop.org/series/44595/
> >> State : warning
> >> 
> >> == Summary ==
> >> 
> >> $ dim checkpatch origin/drm-tip
> >> e6e6b2f7af58 drm/i915/icl: Add allowed DP rates for Icelake
> >> 3fe43cb729fe drm/i915/dp: Add support for HBR3 and TPS4 during link
> >> training
> >> -:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> >> #26: FILE: drivers/gpu/drm/i915/i915_reg.h:8694:
> >> +#define  DP_TP_CTL_LINK_TRAIN_PAT4		(5<<8)
> >
> > Dear maintainers,
> >
> > I get this type of error way too often. What's the most desirable thing
> > here?
> >
> > 1 - Make it "(5 << 8)" so checkpatch doesn't complain, which will leave
> > the coding style inconsistent with the surrounding lines.
> 
> I don't like the inconsistency.

me neither...

> 
> > 2 - Drive-by fix all the bits around it so everybody in the same
> > definition has nice spaces, 2.a: in the same patch, 2.b: in a separate
> > patch.
> 
> Fine by me. Both a and b. I was kind of hoping this would have happened
> more.
> 
> > 3 - Just ignore the checkpatch message, push code as-is.
> 
> Also fine by me.

what I'm currently doing...

> 
> > 4 - Blacklist this check from checkpatch.
> 
> Unfortunately the SPACING class in checkpatch would silence much, much
> more than just this specific thing, so it would be a net negative.

Let's keep the style we want there even if this cause warnings while we
haven't finished the standardization.

> 
> > 5 - Submit a separate patch fixing all the spacing errors on i915_reg.h
> > once and for all. Live happily ever after.
> 
> It would be annoying for a while with conflicts, but I'd be fine. Not
> sure if it would be better to do it in some arbitrary chunks rather than
> mass change.

I believe I prefer one mass commit. So we convert once for all
and cause rebase conflict on internal branch only once. So we solve all
at one and be happy...

> 
> > 6 - Submit a separate patch converting everything to BIT() on
> > i915_reg.h.
> 
> Same as above.

Do we really want BIT everywhere?!

Thanks,
Rodrigo.

> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx