Re: [RESEND PATCH v11 12/18] drm: exynos: dsi: Consolidate component and bridge

[Marek Vasut <marex@denx.de>]

On 1/23/23 16:12, Jagan Teki wrote:
> DSI host registration, attach and detach operations are quite
> different for the component and bridge-based DRM drivers.
> 
> Supporting generic bridge driver to use both component and bridge
> based DRM drivers can be tricky and would require additional host
> related operation hooks.
> 
> Add host operation hooks for registering and unregistering Exynos
> and generic drivers, where Exynos hooks are used in existing Exynos
> component based DRM drivers and generic hooks are used in i.MX8M
> bridge based DRM drivers.
> 
> Add host attach and detach operation hooks for Exynos component
> DRM drivers and those get invoked while DSI core host attach and
> detach gets called.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v11:
> - none
> Changes for v10:
> - split from previous series patch
> "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge"
> 
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 179 ++++++++++++++++++------
>   1 file changed, 140 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 7afbbe30d1d3..fc7f00ab01b4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -250,6 +250,8 @@ struct exynos_dsi_transfer {
>   	u16 rx_done;
>   };
>   
> +struct exynos_dsi;

Is this forward declaration really necessary ? Can't the structures 
below be reordered to get rid of this ?

>   #define DSIM_STATE_ENABLED		BIT(0)
>   #define DSIM_STATE_INITIALIZED		BIT(1)
>   #define DSIM_STATE_CMD_LPM		BIT(2)
> @@ -281,12 +283,19 @@ struct exynos_dsi_driver_data {
>   	const unsigned int *reg_values;
>   };
>   
> +struct exynos_dsim_host_ops {
> +	int (*register_host)(struct exynos_dsi *dsim);
> +	void (*unregister_host)(struct exynos_dsi *dsim);
> +	int (*attach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
> +	int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
> +};
> +
>   struct exynos_dsi_plat_data {
>   	enum exynos_dsi_type hw_type;
> +	const struct exynos_dsim_host_ops *host_ops;
>   };
>   
>   struct exynos_dsi {
> -	struct drm_encoder encoder;
>   	struct mipi_dsi_host dsi_host;
>   	struct drm_bridge bridge;
>   	struct drm_bridge *out_bridge;
> @@ -316,6 +325,12 @@ struct exynos_dsi {
>   
>   	const struct exynos_dsi_driver_data *driver_data;
>   	const struct exynos_dsi_plat_data *plat_data;
> +
> +	void *priv;
> +};
> +
> +struct exynos_dsi_enc {
> +	struct drm_encoder encoder;
>   };
>   
>   #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
> @@ -1319,10 +1334,11 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>   
>   static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>   {
> -	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
> +	struct exynos_dsi *dsim = (struct exynos_dsi *)dev_id;

Is the rename really needed  ?

> +	struct exynos_dsi_enc *dsi = dsim->priv;

Call this variable something else , like dsi_enc , and you shouldn't 
need the rename above ...

>   	struct drm_encoder *encoder = &dsi->encoder;
>   
> -	if (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE)
> +	if (dsim->state & DSIM_STATE_VIDOUT_AVAILABLE)

... and the rename here .

>   		exynos_drm_crtc_te_handler(encoder->crtc);
>   
>   	return IRQ_HANDLED;


[...]

>   static void exynos_dsi_unbind(struct device *dev, struct device *master,
>   				void *data)
>   {
> -	struct exynos_dsi *dsi = dev_get_drvdata(dev);
> +	struct exynos_dsi *dsim = dev_get_drvdata(dev);

Please avoid the variable renames globally, that should simplify this 
patch and remove unrelated changes.

> -	exynos_dsi_atomic_disable(&dsi->bridge, NULL);
> +	dsim->bridge.funcs->atomic_disable(&dsim->bridge, NULL);
>   
> -	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	mipi_dsi_host_unregister(&dsim->dsi_host);
>   }

[...]

With that fixed:

Reviewed-by: Marek Vasut <marex@denx.de>