Re: [Intel-gfx] [PATCH 6/6] drm/i915/display: Dump the new cdclk config values

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Sep 19, 2022 at 12:46:45PM -0700, Navare, Manasi wrote:
> On Fri, Sep 16, 2022 at 05:44:04PM -0700, Anusha Srivatsa wrote:
> > Add a helper function to get stringify values of the
> > desired cdclk action and dump it with rest of the
> > cdclk config values
> > 
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> Please add Suggested-by: field to give proper credits as per our
> discussion. This applies to other patches as well to add proper credits
> to other folks who suggested design changes/ fixes.
> This needs to be followed as per the upstream patch review methodology.

Having the suggested by is a good way to give the proper credits when the whole
big idea and design was co authored, or started by the other developer.

We really need to mind about it. However it is not a very good thing for all
of the patches we have. Many ideas come out of conversation with many folks
and we cannot simply start adding the suggested by list with all the names
involved.

There's always a threshold there that we should mind. And the official rule
as reference is this one:

"A Suggested-by: tag indicates that the patch idea is suggested by the person
named and ensures credit to the person for the idea. Please note that this tag
should not be added without the reporter’s permission, especially if the idea
 was not posted in a public forum." [1]

As a reference we don't keep adding official suggested-by tags for any and all
of the suggestions we receive during reviews. The simple name version in the
commit message is what we traditionally uses for small things.

Looking to the history of this series here I see that this patch looks like
an evolution of the previous series with small increment and suggestions from
multiple folks. Not sure if it is really worthwhile to have to add the official
tag and start to ping everyone to get the ack if it is okay or not to add it.

[1] - https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index bc627daade3e..12f5e4d23245 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1688,6 +1688,19 @@ static u32 cdclk_squash_waveform(struct drm_i915_private *dev_priv,
> >  
> >  	return 0xffff;
> >  }
> 
> Missing newline causing checkpatch error
> 
> Manasi
> 
> > +static const char *cdclk_sequence_to_string(enum cdclk_sequence cdclk_sequence)
> > +{
> > +	switch (cdclk_sequence) {
> > +	case CDCLK_SQUASH_ONLY:
> > +		return "Squash only";
> > +	case CDCLK_CRAWL_ONLY:
> > +		return "Crawl only";
> > +	case CDCLK_LEGACY:
> > +		return "Legacy method";
> > +	default:
> > +		return "Not a valid cdclk sequence";
> > +	}
> > +}
> >  
> >  static void dg2_prog_squash_ctl(struct drm_i915_private *i915, u16 waveform)
> >  {
> > @@ -2083,10 +2096,11 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >  			     const struct intel_cdclk_config *cdclk_config,
> >  			     const char *context)
> >  {
> > -	drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n",
> > +	drm_dbg_kms(&i915->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d, %s action\n",
> >  		    context, cdclk_config->cdclk, cdclk_config->vco,
> >  		    cdclk_config->ref, cdclk_config->bypass,
> > -		    cdclk_config->voltage_level);
> > +		    cdclk_config->voltage_level,
> > +		    cdclk_sequence_to_string(cdclk_config->steps->action));
> >  }
> >  
> >  /**
> > -- 
> > 2.25.1
> >