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

Commit Message

Meng Yi May 15, 2016, 8:34 a.m. UTC

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 <[e-mail]>
Signed-off-by: Xiubo Li <[e-mail]>
Signed-off-by: Jianwei Wang <[e-mail]>
Signed-off-by: Meng Yi <[e-mail]>
---
 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

Comments

Boris BREZILLON May 16, 2016, 12:45 p.m. UTC | #1

Hi Meng,

On Sun, 15 May 2016 16:34:44 +0800
Meng Yi <[e-mail]> 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/

Not sure I understand why it depends on the sii902x driver I posted.
The code I see here seems capable of interfacing with any kind of
drm_bridge, not only the sii9022 one.

> 
> Signed-off-by: Alison Wang <[e-mail]>
> Signed-off-by: Xiubo Li <[e-mail]>
> Signed-off-by: Jianwei Wang <[e-mail]>
> Signed-off-by: Meng Yi <[e-mail]>
> ---
>  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
> + *
> + * 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>
> +#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);

Just sharing my understanding of the DRM bridge infrastructure, and
I'll let Daniel (and other experimented DRM/KMS developers) correct me
if I'm wrong.

The Sil9022 is a bridge taking its pixel stream from an RGB encoder
(also known as ENCODER_NONE), and converting it into a TDMS (HDMI or
DVI) stream. So, your encoder on the fsl-dcu side is not an HDMI
encoder but an RGB (ENCODER_NONE) encoder.
Switching to this approach will let you support any kind of bridge
(RGB->DVI, RGB->TDMS, RGB->LVDS, ...) without having to add boilerplate
code for each of them.

Regards,

Boris

Meng Yi May 16, 2016, 1:56 p.m. UTC | #2

Hi Boris,

On Monday, May 16, 2016 8:45 PM 
Boris Brezillon <[e-mail]> wrote:
> 
> Hi Meng,
> 
> On Sun, 15 May 2016 16:34:44 +0800
> Meng Yi <[e-mail]> 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/
> 
> Not sure I understand why it depends on the sii902x driver I posted.
> The code I see here seems capable of interfacing with any kind of drm_bridge,
> not only the sii9022 one.
> 
Yes,  The code here is capable of interfacing with any kind of drm_bridge, now we are using 
Ls1021a-twr(using sii9022a as transmitter) to test the code, maybe I should change the comments.

> >
> > Signed-off-by: Alison Wang <[e-mail]>
> > Signed-off-by: Xiubo Li <[e-mail]>
> > Signed-off-by: Jianwei Wang <[e-mail]>
> > Signed-off-by: Meng Yi <[e-mail]>
> > ---
> >  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
> > + *
> > + * 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>
> > +#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);
> 
> Just sharing my understanding of the DRM bridge infrastructure, and I'll let
> Daniel (and other experimented DRM/KMS developers) correct me if I'm
> wrong.
> 
> The Sil9022 is a bridge taking its pixel stream from an RGB encoder (also known
> as ENCODER_NONE), and converting it into a TDMS (HDMI or
> DVI) stream. So, your encoder on the fsl-dcu side is not an HDMI encoder but
> an RGB (ENCODER_NONE) encoder.
> Switching to this approach will let you support any kind of bridge (RGB->DVI,
> RGB->TDMS, RGB->LVDS, ...) without having to add boilerplate code for each of
> them.
Thanks for your sharing. I had read your article < The DRM/KMS subsystem from a newbie's point of view>, it helped me a lot when I was starting to develop DRM/KMS.

Best Regards,
Meng Yi

Stefan Agner June 10, 2016, 2:35 a.m. UTC | #3

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 <[e-mail]>
> Signed-off-by: Xiubo Li <[e-mail]>
> Signed-off-by: Jianwei Wang <[e-mail]>
> Signed-off-by: Meng Yi <[e-mail]>
> ---
>  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__ */

Meng Yi June 12, 2016, 1:55 a.m. UTC | #4

Hi Stefan,

Thanks for your comments, and some feedback below:

> > +
> > +#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.
> 

Will change that in next version.

> > +#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
> 

Since there are lots of return value to check, if any error occurs, it will jump out of while loop and handled in the end.
I think it will be verbose using "goto" statements.

> > +
> > +	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.

OK, it will be removed in next version.

> 
> > 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?
> 

It means global.

> > +
> > +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?
> 

Yeah, we can do that. Anyway it just a choice. sounds like auto detect is better ^_^

> > +
> > +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.
> 

It already handled within the calling function. So just return here.

Thanks,
Meng

Stefan Agner June 12, 2016, 10 p.m. UTC | #5

On 2016-06-11 18:55, Meng Yi wrote:
> Hi Stefan,
> 
> Thanks for your comments, and some feedback below:
> 
>> > +
>> > +#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.
>>
> 
> Will change that in next version.
> 
>> > +#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
>>
> 
> Since there are lots of return value to check, if any error occurs, it
> will jump out of while loop and handled in the end.
> I think it will be verbose using "goto" statements.

I don't agree. each break will be a goto statement, and you don't need
to check pointers before freeing them in the error handling part (you
need to add labels though). Also, you don't need a while statement. So
code wise you should get a net zero or even some lines less. You also
"safe" a indention level with goto style.

And most importantly, it is the error handling style used across the
kernel, and hence documented in the CodingStyle.txt document... Sorry,
if you want that patch applied, you don't get to choose what to use...

--
Stefan


> 
>> > +
>> > +	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.
> 
> OK, it will be removed in next version.
> 
>>
>> > 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?
>>
> 
> It means global.
> 
>> > +
>> > +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?
>>
> 
> Yeah, we can do that. Anyway it just a choice. sounds like auto detect
> is better ^_^
> 
>> > +
>> > +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.
>>
> 
> It already handled within the calling function. So just return here.
> 
> Thanks,
> Meng

Patch

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
+ *
+ * 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>
+#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);
+
+	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");
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;
+
+static int __init enable_hdmi_setup(char *str)
+{
+	g_enable_hdmi = true;
+
+	return 1;
+}
+
+__setup("hdmi", enable_hdmi_setup);
+
+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;
+	}
+
 	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__ */