Re: [PATCHv12 3/3] ARM:drm ivip Intel FPGA Video and Image Processing Suite
flat view
From: Sam Ravnborg <hidden>
Date: 2019-02-11 06:56:01
Also in:
dri-devel, linux-devicetree, lkml
Hi Hean-Loong, Ong Patch looks good to me, but there is a few trivial things I spotted while browsing the code. See below. Sam
quoted hunk
+++ b/drivers/gpu/drm/ivip/Makefile@@ -0,0 +1,7 @@ +# +# Makefile for the drm device driver. This driver provides support for the +# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. + +obj-$(CONFIG_DRM_IVIP) += ivip.o +ivip-objs := intel_vip_of.o intel_vip_core.o \ + intel_vip_conn.o
You could use: ivip-y := intel_vip_of.o intel_vip_core.o ivip-y += intel_vip_conn.o Using "ivip-y" is the recommend syntax today. And using "+=" you get rid of the ugly "\" to continue a line. (Some people prefer "\" in makefiles, but there are not needed)
quoted hunk
+++ b/drivers/gpu/drm/ivip/intel_vip_core.c@@ -0,0 +1,189 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Intel Corporation. + * + * intel_vip_core.c -- Intel Video and Image Processing(VIP) + * Frame Buffer II driver + * + * This driver supports the Intel VIP Frame Reader component. + * More info on the hardware can be found in the Intel Video + * and Image Processing Suite User Guide at this address + * http://www.altera.com/literature/ug/ug_vip.pdf. + * + * Authors: + * Walter Goossens <waltergoossens@home.nl> + * Thomas Chou <thomas@wytron.com.tw> + * Chris Rauer <crauer@altera.com> + * Ong, Hean-Loong <hean.loong.ong@intel.com> + * + */ + +#include <drm/drmP.h>
Please do not use drmP.h in new drivers, we try to get rid of this file.
quoted hunk
+#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_plane_helper.h> +#include <drm/drm_simple_kms_helper.h> +#include <drm/drm_gem_framebuffer_helper.h>
Sort the list of include files. (Looks like this was properly done before, and only one file is out of palce)
quoted hunk
+static void intelvipfb_enable(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state, struct drm_plane_state * + plane_state)
Fix indent. The parameters on second line and following should be aligned below the opening paranthesis. Use tab(s) + spaces to align properly.
+void intelvipfb_display_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state)
Align parameter
quoted hunk
+} +EXPORT_SYMBOL(intelvipfb_display_pipe_update); + +static struct drm_simple_display_pipe_funcs fbpriv_funcs = { + .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, + .update = intelvipfb_display_pipe_update, + .enable = intelvipfb_enable, + .disable = intelvipfb_disable, +}; + + +int intelvipfb_probe(struct device *dev) +{ + int retval; + struct drm_device *drm; + struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev); + + struct drm_connector *connector; + u32 formats[] = {DRM_FORMAT_XRGB8888}; + + drm = fbpriv->drm; + + drm->dev_private = fbpriv;
It would be simpler to just pass fbpriv as a parameter. There is only one user of intelvipfb_probe() so no need to avoid it. Also it would be more logical to set drm = fbpriv->drm; where memory are allocated.
quoted hunk
+ + intelvipfb_setup_mode_config(drm); + + connector = intelvipfb_conn_setup(drm); + if (!connector) { + dev_err(drm->dev, "Connector setup failed\n"); + goto err_mode_config; + } + + retval = drm_simple_display_pipe_init(drm, + &fbpriv->pipe, + &fbpriv_funcs, + formats, + ARRAY_SIZE(formats), + NULL, connector);
Consider indent, where subsequent parameters are aligned right after the
opening '('.
quoted hunk
+ + if (retval < 0) { + dev_err(drm->dev, "Cannot setup simple display pipe\n"); + goto err_mode_config; + } + + drm_mode_config_reset(drm); + + drm_dev_register(drm, 0); + + drm_fbdev_generic_setup(drm, 32); + + dev_info(drm->dev, "ivip: Successfully created fb\n"); + + return retval; + +err_mode_config: + + drm_mode_config_cleanup(drm); + return -ENODEV; +} +
quoted hunk
diff --git a/drivers/gpu/drm/ivip/intel_vip_of.c b/drivers/gpu/drm/ivip/intel_vip_of.c new file mode 100644 index 0000000..c899e30 --- /dev/null +++ b/drivers/gpu/drm/ivip/intel_vip_of.c@@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Intel Corporation. + * + * intel_vip_of.c -- Intel Video and Image Processing(VIP) + * Frame Buffer II driver
The need for this file confuses me. It does not include only "of" related stuff, but also generic device driver stuff. Maybe merge with intel_vip_core.c? And rename that file to intel_vip_drv.c?
quoted hunk
+ * + * This driver supports the Intel VIP Frame Reader component. + * More info on the hardware can be found in the Intel Video + * and Image Processing Suite User Guide at this address + * http://www.altera.com/literature/ug/ug_vip.pdf. + * + * Authors: + * Ong, Hean-Loong <hean.loong.ong@intel.com> + * + */ +#include <drm/drmP.h>
Drop use of drmP.h
quoted hunk
+static int intelvipfb_of_probe(struct platform_device *pdev) +{ + int retval; + struct resource *reg_res; + struct intelvipfb_priv *fbpriv; + struct device *dev = &pdev->dev; + struct drm_device *drm; + + fbpriv = devm_kzalloc(dev, sizeof(*fbpriv), GFP_KERNEL); + if (!fbpriv) + return -ENOMEM; + + /*setup DRM */
Space before "setup"
quoted hunk
+ +static const struct of_device_id intelvipfb_of_match[] = { + { .compatible = "altr,vip-frame-buffer-2.0" }, + {},
Maybe add "/* sentinel */" comment? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel