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

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@nxp.com Signed-off-by: Xiubo Li lixiubo@cmss.chinamobile.com Signed-off-by: Jianwei Wang jianwei.wang.chn@gmail.com Signed-off-by: Meng Yi meng.yi@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__ */