Re: [PATCH RESEND v7 1/7] drm: atmel-hlcdc: add flag and driver ops to differentiate XLCDC and HLCDC IP

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Manikandan,

On 29.01.2024 11:23, Manikandan Muralidharan wrote:
> Add is_xlcdc flag and LCD IP specific ops in driver data to differentiate
> XLCDC and HLCDC code within the atmel-hlcdc driver files.

I would first prepare the current code base for the addition of XLCDC by
first adding the struct atmel_lcdc_dc_ops, update current code to use it
and after that add XLCDC.

> 
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 37 ++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index 5b5c774e0edf..d5e01ff8c7f9 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -177,6 +177,9 @@ struct atmel_hlcdc_layer_cfg_layout {
>  	int csc;
>  };
>  
> +struct atmel_hlcdc_plane_state;

You can move this forward declaration close the the structure that needs it
(struct atmel_lcdc_dc_ops).

> +struct atmel_hlcdc_dc;

And you can get rid if this one if you move struct atmel_lcdc_dc_ops after
struct atmel_hlcdc_dc definition.

> +
>  /**
>   * Atmel HLCDC DMA descriptor structure
>   *
> @@ -288,6 +291,36 @@ atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *layer)
>  	return container_of(layer, struct atmel_hlcdc_plane, layer);
>  }
>  
> +/**
> + * struct atmel_lcdc_dc_ops - describes atmel_lcdc ops group
> + * to differentiate HLCDC and XLCDC IP code support.
> + * @plane_setup_scaler: update the vertical and horizontal scaling factors
> + * @update_lcdc_buffers: update the each LCDC layers DMA registers.
> + * @lcdc_atomic_disable: disable LCDC interrupts and layers
> + * @lcdc_update_general_settings: update each LCDC layers general
> + * confiugration register.
s/confiugration/configuration

> + * @lcdc_atomic_update: enable the LCDC layers and interrupts.

You may want to keep consistency by adding or not '.' at the end of the
documentation statement (I consider '.' is useless)

> + * @lcdc_csc_init: update the color space conversion co-efficient of
> + * High-end overlay register.
> + * @lcdc_irq_dbg: to raise alert incase of interrupt overrun in any LCDC layer.

s/incase/in case

> + */
> +struct atmel_lcdc_dc_ops {
> +	void (*plane_setup_scaler)(struct atmel_hlcdc_plane *plane,
> +				   struct atmel_hlcdc_plane_state *state);
> +	void (*update_lcdc_buffers)(struct atmel_hlcdc_plane *plane,
> +				    struct atmel_hlcdc_plane_state *state,
> +				    u32 sr, int i);
> +	void (*lcdc_atomic_disable)(struct atmel_hlcdc_plane *plane);
> +	void (*lcdc_update_general_settings)(struct atmel_hlcdc_plane *plane,
> +					     struct atmel_hlcdc_plane_state *state);
> +	void (*lcdc_atomic_update)(struct atmel_hlcdc_plane *plane,
> +				   struct atmel_hlcdc_dc *dc);
> +	void (*lcdc_csc_init)(struct atmel_hlcdc_plane *plane,
> +			      const struct atmel_hlcdc_layer_desc *desc);
> +	void (*lcdc_irq_dbg)(struct atmel_hlcdc_plane *plane,
> +			     const struct atmel_hlcdc_layer_desc *desc);
> +};
> +
>  /**
>   * Atmel HLCDC Display Controller description structure.
>   *
> @@ -304,8 +337,10 @@ atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *layer)
>   * @conflicting_output_formats: true if RGBXXX output formats conflict with
>   *				each other.
>   * @fixed_clksrc: true if clock source is fixed
> + * @is_xlcdc: true if XLCDC IP is supported
>   * @layers: a layer description table describing available layers
>   * @nlayers: layer description table size
> + * @ops: atmel lcdc dc ops
>   */
>  struct atmel_hlcdc_dc_desc {
>  	int min_width;
> @@ -317,8 +352,10 @@ struct atmel_hlcdc_dc_desc {
>  	int max_hpw;
>  	bool conflicting_output_formats;
>  	bool fixed_clksrc;
> +	bool is_xlcdc;
>  	const struct atmel_hlcdc_layer_desc *layers;
>  	int nlayers;
> +	const struct atmel_lcdc_dc_ops *ops;
>  };
>  
>  /**