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