Re: [Intel-gfx] [PATCH 2/6] drm/i915/display: add cdclk action struct to cdclk_config

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> The struct has the action to be performed - squash, crawl
> or modeset and the corresponding cdclk which is the desired
> cdclk. This is the structure that gets populated during
> atomic check once it is determined what the cdclk change looks
> like
>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index c674879a84a5..3869f93e8ad2 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -11,13 +11,27 @@
>  #include "intel_display.h"
>  #include "intel_global_state.h"
>  
> +#define	MAX_CDCLK_ACTIONS	1

Okay, this review is just nitpicks, but they'll need to get fixed
eventually so here goes.

No tab after #define.

> +
>  struct drm_i915_private;
>  struct intel_atomic_state;
>  struct intel_crtc_state;
>  
> +enum cdclk_sequence {

Needs to be named intel_ something.

> +	CDCLK_INVALID_ACTION = -1,
> +
> +	CDCLK_SQUASH_ONLY = 0,
> +	CDCLK_CRAWL_ONLY,
> +	CDCLK_LEGACY,
> +};
> +
>  struct intel_cdclk_config {
>  	unsigned int cdclk, vco, ref, bypass;
>  	u8 voltage_level;
> +	struct cdclk_step {

Needs to be named intel_ something.

Since this is used independently, I'd prefer it to be defined outside of
struct intel_cdclk_config.

> +		enum cdclk_sequence action;
> +		u32 cdclk;
> +	} steps[MAX_CDCLK_ACTIONS];
>  };
>  
>  struct intel_cdclk_state {

-- 
Jani Nikula, Intel Open Source Graphics Center