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 mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
