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@xxxxxxx> > Signed-off-by: Xiubo Li <lixiubo@xxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Jianwei Wang <jianwei.wang.chn@xxxxxxxxx> > Signed-off-by: Meng Yi <meng.yi@xxxxxxx> > --- > 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__ */ _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
