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

[Meng Yi]

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