Re: [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
> This allows to drop the custom drm_encoder_cleanup() actions.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> New in v3, example conversion of drm_simple_encoder_init() users.
> 
> This and the following patches depend on the drm/imx conversion to use managed
> resources [1].
> 
> [1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
> ---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 19 ++++---------------
>  drivers/gpu/drm/imx/imx-ldb.c          | 20 ++++----------------
>  drivers/gpu/drm/imx/imx-tve.c          | 22 ++++------------------
>  drivers/gpu/drm/imx/parallel-display.c | 22 ++++------------------
>  4 files changed, 16 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index 16be8bd92653..87428fb23d9f 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -188,13 +188,6 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
>  
> -static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> -	struct drm_encoder *encoder = data;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  			    void *data)
>  {
> @@ -203,9 +196,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder;
>  	int ret;
>  
> -	hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL);

Where does this apply to? Neither upstream nor linux-next seems to have
the drmm_ conversion for imx already applied, and that's kinda the juicy
part I'd like to look at a bit. The patches here are just mechanical
conversion. Can you pls include the drmm_ conversion too (maybe even
squash these patches here in, I think that would be more readable)?

Or am I looking at the wrong tree?

Anyway I think it looks all neat.
-Daniel

> -	if (!hdmi_encoder)
> -		return -ENOMEM;
> +	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct imx_hdmi_encoder,
> +						 encoder, DRM_MODE_ENCODER_TMDS);
> +	if (IS_ERR(hdmi_encoder))
> +		return PTR_ERR(hdmi_encoder);
>  
>  	hdmi_encoder->hdmi = dev_get_drvdata(dev);
>  	encoder = &hdmi_encoder->encoder;
> @@ -215,11 +209,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  		return ret;
>  
>  	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> -
> -	ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
>  
>  	return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
>  }
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index d4beb58f509d..dbfe39e2f7f6 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -414,13 +414,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
>  	return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
>  }
>  
> -static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> -	struct drm_encoder *encoder = data;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int imx_ldb_register(struct drm_device *drm,
>  	struct imx_ldb_channel *imx_ldb_ch)
>  {
> @@ -430,20 +423,15 @@ static int imx_ldb_register(struct drm_device *drm,
>  	struct drm_encoder *encoder;
>  	int ret;
>  
> -	ldb_encoder = drmm_kzalloc(drm, sizeof(*ldb_encoder), GFP_KERNEL);
> -	if (!ldb_encoder)
> -		return -ENOMEM;
> +	ldb_encoder = drmm_simple_encoder_alloc(drm, struct imx_ldb_encoder,
> +						encoder, DRM_MODE_ENCODER_LVDS);
> +	if (IS_ERR(ldb_encoder))
> +		return PTR_ERR(ldb_encoder);
>  
>  	ldb_encoder->channel = imx_ldb_ch;
>  	connector = &ldb_encoder->connector;
>  	encoder = &ldb_encoder->encoder;
>  
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
> -
> -	ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index bac025eafa1f..0746f0b425df 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -433,13 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
>  	return 0;
>  }
>  
> -static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> -	struct drm_encoder *encoder = ptr;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static void imx_tve_disable_regulator(void *data)
>  {
>  	struct imx_tve *tve = data;
> @@ -498,22 +491,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  	encoder_type = tve->mode == TVE_MODE_VGA ?
>  		       DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
>  
> -	tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL);
> -	if (!tvee)
> -		return -ENOMEM;
> +	tvee = drmm_simple_encoder_alloc(drm, struct imx_tve_encoder, encoder,
> +					 encoder_type);
> +	if (IS_ERR(tvee))
> +		return PTR_ERR(tvee);
>  
>  	tvee->tve = tve;
>  	encoder = &tvee->encoder;
>  	connector = &tvee->connector;
>  
> -	ret = drm_simple_encoder_init(drm, encoder, encoder_type);
> -	if (ret)
> -		return ret;
> -
> -	ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 50b5b89c2db2..9b1ec7e73c30 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,13 +258,6 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
>  	.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
>  };
>  
> -static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> -	struct drm_encoder *encoder = ptr;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct drm_device *drm = data;
> @@ -275,23 +268,16 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  	struct drm_bridge *bridge;
>  	int ret;
>  
> -	imxpd_encoder = drmm_kzalloc(drm, sizeof(*imxpd_encoder), GFP_KERNEL);
> -	if (!imxpd_encoder)
> -		return -ENOMEM;
> +	imxpd_encoder = drmm_simple_encoder_alloc(drm, struct imx_parallel_display_encoder,
> +						  encoder, DRM_MODE_ENCODER_NONE);
> +	if (IS_ERR(imxpd_encoder))
> +		return PTR_ERR(imxpd_encoder);
>  
>  	imxpd_encoder->pd = imxpd;
>  	connector = &imxpd_encoder->connector;
>  	encoder = &imxpd_encoder->encoder;
>  	bridge = &imxpd_encoder->bridge;
>  
> -	ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
> -	if (ret)
> -		return ret;
> -
> -	ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
>  	if (ret)
>  		return ret;
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel