[PATCH 1/2] drm/fsl-dcu: Add HDMI driver for freescale DCU

Stefan Agner stefan at agner.ch
Fri Jun 10 02:35:44 UTC 2016


Hi Meng,

Some comments below.

On 2016-05-15 01:34, Meng Yi wrote:
> This driver add the basic functions for Encoder, and link the Encoder to
> appropriate DRM bridge.
> This driver is depend on sii9022 driver(drm_bridge approach),which is
> sent by Boris Brezillon to community but not merged.
> https://patchwork.kernel.org/patch/8600921/
> 
> Signed-off-by: Alison Wang <alison.wang at nxp.com>
> Signed-off-by: Xiubo Li <lixiubo at cmss.chinamobile.com>
> Signed-off-by: Jianwei Wang <jianwei.wang.chn at gmail.com>
> Signed-off-by: Meng Yi <meng.yi at nxp.com>
> ---
>  drivers/gpu/drm/fsl-dcu/Makefile             |   1 +
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c   | 194 +++++++++++++++++++++++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c    |  29 ++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h |   4 +
>  4 files changed, 228 insertions(+)
>  create mode 100644 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/Makefile b/drivers/gpu/drm/fsl-dcu/Makefile
> index b35a292..12e2245 100644
> --- a/drivers/gpu/drm/fsl-dcu/Makefile
> +++ b/drivers/gpu/drm/fsl-dcu/Makefile
> @@ -1,6 +1,7 @@
>  fsl-dcu-drm-y := fsl_dcu_drm_drv.o \
>  		 fsl_dcu_drm_kms.o \
>  		 fsl_dcu_drm_rgb.o \
> +		 fsl_dcu_drm_hdmi.o \
>  		 fsl_dcu_drm_plane.o \
>  		 fsl_dcu_drm_crtc.o \
>  		 fsl_dcu_drm_fbdev.o \
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> new file mode 100644
> index 0000000..76441c7
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> @@ -0,0 +1,194 @@
> +/*
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + *
> + * Freescale DCU drm device driver

A bit more precise?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/fsl_devices.h>
> +#include <linux/i2c.h>

I think you don't use i2c here anymore, so this include (and probably a
lot others) are obsolete.

> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/backlight.h>
> +#include <linux/of_graph.h>
> +#include <video/videomode.h>
> +#include <video/of_display_timing.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_encoder_slave.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +
> +#include "fsl_dcu_drm_drv.h"
> +#include "fsl_dcu_drm_output.h"
> +
> +static void
> +fsl_dcu_drm_hdmienc_mode_set(struct drm_encoder *encoder,
> +					 struct drm_display_mode *mode,
> +					 struct drm_display_mode *adjusted_mode)
> +{
> +}
> +
> +static int
> +fsl_dcu_drm_hdmienc_atomic_check(struct drm_encoder *encoder,
> +				 struct drm_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state)
> +{
> +	return 0;
> +}
> +
> +static void
> +fsl_dcu_drm_hdmienc_disable(struct drm_encoder *encoder)
> +{
> +}
> +
> +static void
> +fsl_dcu_drm_hdmienc_enable(struct drm_encoder *encoder)
> +{
> +}
> +
> +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> +	.atomic_check = fsl_dcu_drm_hdmienc_atomic_check,
> +	.disable = fsl_dcu_drm_hdmienc_disable,
> +	.enable = fsl_dcu_drm_hdmienc_enable,
> +	.mode_set = fsl_dcu_drm_hdmienc_mode_set,
> +};
> +
> +static void fsl_dcu_drm_hdmienc_destroy(struct drm_encoder *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +}
> +
> +void fsl_dcu_drm_hdmienc_reset(struct drm_encoder *encoder)
> +{
> +}
> +
> +static const struct drm_encoder_funcs encoder_funcs = {
> +	.reset = fsl_dcu_drm_hdmienc_reset,
> +	.destroy = fsl_dcu_drm_hdmienc_destroy,
> +};
> +
> +int fsl_dcu_drm_hdmienc_create(struct fsl_dcu_drm_device *fsl_dev,
> +			       struct drm_crtc *crtc)
> +{
> +	struct drm_encoder *encoder;
> +	int ret;
> +
> +	do {
> +		encoder = devm_kzalloc(fsl_dev->dev,
> +					sizeof(struct drm_encoder), GFP_KERNEL);
> +		encoder->possible_crtcs = 1;
> +		ret = drm_encoder_init(fsl_dev->drm, encoder, &encoder_funcs,
> +				       DRM_MODE_ENCODER_TMDS, NULL);
> +		if (ret < 0) {
> +			devm_kfree(fsl_dev->dev, encoder);
> +			break;
> +		}
> +
> +		drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +		encoder->crtc = crtc;
> +
> +		return 0;
> +	} while (false);

Not sure where you have this error handling style from, but it is
definitely not common in Linux. Much more common is to have a error
handling at the end of the function with labels and use goto statements
in the error cases above, see Chapter 7:
https://www.kernel.org/doc/Documentation/CodingStyle

> +
> +	DRM_ERROR("failed to initialize hdmi encoder\n");
> +	if (encoder)
> +		devm_kfree(fsl_dev->dev, encoder);
> +
> +	crtc->funcs->destroy(crtc);
> +	return ret;
> +}
> +
> +static struct drm_encoder *fsl_dcu_drm_hdmi_find_encoder(struct
> drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> +		if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS)
> +			return encoder;
> +	}
> +
> +	return NULL;
> +}
> +
> +int fsl_dcu_drm_hdmienc_attach_bridge(struct fsl_dcu_drm_device *fsl_dev)
> +{
> +	struct drm_device *drm_dev = fsl_dev->drm;
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *bridge;
> +	int ret;
> +	struct device_node *entity;
> +	struct of_endpoint *ep;
> +	struct device_node *ep_node;
> +	struct device_node *parent = fsl_dev->dev->of_node;
> +
> +	do {
> +		ep = devm_kzalloc(fsl_dev->dev,
> +					sizeof(struct of_endpoint), GFP_KERNEL);
> +		if (!ep)
> +			break;
> +
> +		ep_node = devm_kzalloc(fsl_dev->dev,
> +					sizeof(struct device_node), GFP_KERNEL);
> +		if (!ep_node)
> +			break;
> +
> +		ep_node = of_graph_get_next_endpoint(parent, NULL);
> +		if (!ep_node)
> +			break;
> +
> +		ret = of_graph_parse_endpoint(ep_node, ep);
> +		if (ret < 0) {
> +			of_node_put(ep_node);
> +			break;
> +		}
> +
> +		entity = of_graph_get_remote_port_parent(ep->local_node);
> +		if (!entity)
> +			break;
> +
> +		bridge = of_drm_find_bridge(entity);
> +		if (!bridge)
> +			break;
> +
> +		encoder = fsl_dcu_drm_hdmi_find_encoder(drm_dev);
> +		if (!encoder)
> +			break;
> +
> +		encoder->bridge = bridge;
> +		bridge->encoder = encoder;
> +
> +		ret = drm_bridge_attach(drm_dev, bridge);
> +		if (ret)
> +			break;
> +
> +		return 0;
> +	} while (false);
> +
> +	DRM_ERROR("failed to attach the bridge\n");
> +
> +	if (ep)
> +		devm_kfree(fsl_dev->dev, ep);
> +
> +	if (ep_node)
> +		devm_kfree(fsl_dev->dev, ep_node);
> +
> +	encoder->funcs->destroy(encoder);
> +	return ret;
> +}
> +
> +MODULE_AUTHOR("NXP Semiconductor, Inc.");
> +MODULE_DESCRIPTION("NXP LS1021A DCU driver");
> +MODULE_LICENSE("GPL");

I don't think this is a kernel module on its own, hence this is not
needed.

> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> index c564ec6..a7fe1d2 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> @@ -17,10 +17,29 @@
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
>  
> +bool g_enable_hdmi;

What is the meaning of the g_ prefix?

> +
> +static int __init enable_hdmi_setup(char *str)
> +{
> +	g_enable_hdmi = true;
> +
> +	return 1;
> +}
> +
> +__setup("hdmi", enable_hdmi_setup);

I am not sure yet whether I like that module parameter... Can't we just
rely on device tree whether there is a HDMI encoder to attach or not?

> +
> +static void fsl_dcu_drm_output_poll_changed(struct drm_device *dev)
> +{
> +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> +
> +	drm_fbdev_cma_hotplug_event(fsl_dev->fbdev);
> +}
> +
>  static const struct drm_mode_config_funcs fsl_dcu_drm_mode_config_funcs = {
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  	.fb_create = drm_fb_cma_create,
> +	.output_poll_changed = fsl_dcu_drm_output_poll_changed,
>  };
>  
>  int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev)
> @@ -47,6 +66,16 @@ int fsl_dcu_drm_modeset_init(struct
> fsl_dcu_drm_device *fsl_dev)
>  	if (ret)
>  		goto fail_connector;
>  
> +	if (g_enable_hdmi) {
> +		ret = fsl_dcu_drm_hdmienc_create(fsl_dev, &fsl_dev->crtc);
> +		if (ret)
> +			return ret;
> +
> +		ret = fsl_dcu_drm_hdmienc_attach_bridge(fsl_dev);
> +		if (ret)
> +			return ret;
> +	}
> +

Above we go to fail_connector on error, and in your calls you just
return. That does not look like a proper error handling.

--
Stefan


>  	drm_mode_config_reset(fsl_dev->drm);
>  	drm_kms_helper_poll_init(fsl_dev->drm);
>  
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> index 7093109..8915b07 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> @@ -29,5 +29,9 @@ int fsl_dcu_drm_connector_create(struct
> fsl_dcu_drm_device *fsl_dev,
>  				 struct drm_encoder *encoder);
>  int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
>  			       struct drm_crtc *crtc);
> +int fsl_dcu_drm_hdmienc_attach_bridge(struct fsl_dcu_drm_device *fsl_dev);
> +int fsl_dcu_drm_hdmienc_create(struct fsl_dcu_drm_device *fsl_dev,
> +			       struct drm_crtc *crtc);
> +
>  
>  #endif /* __FSL_DCU_DRM_CONNECTOR_H__ */


More information about the dri-devel mailing list