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 + * + * 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__ */
Add port node to DCU node using "OF graph" discription.
Signed-off-by: Alison Wang b18965@freescale.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 --- Documentation/devicetree/bindings/display/fsl,dcu.txt | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt b/Documentation/devicetree/bindings/display/fsl,dcu.txt index ae55cde..c973d94 100644 --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt @@ -17,6 +17,10 @@ Required properties: Optional properties: - fsl,tcon: The phandle to the timing controller node.
+Optional nodes: + +- Video port 0 for the HDMI output + Examples: dcu: dcu@2ce0000 { compatible = "fsl,ls1021a-dcu"; @@ -26,4 +30,9 @@ dcu: dcu@2ce0000 { big-endian; fsl,panel = <&panel>; fsl,tcon = <&tcon>; + port@0 { + dcu_out_hdmi: endpoint { + remote-endpoint = <&sii9022a_out>; + }; + }; };
Hi Meng,
On Sun, 15 May 2016 16:34:44 +0800 Meng Yi meng.yi@nxp.com 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 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 \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
Hi Boris,
On Monday, May 16, 2016 8:45 PM Boris Brezillon boris.brezillon@free-electrons.com wrote:
Hi Meng,
On Sun, 15 May 2016 16:34:44 +0800 Meng Yi meng.yi@nxp.com 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 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 \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
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 \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__ */
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
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
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Meng Yi -
Stefan Agner