[PATCH 1/3] drm/gma500: Code cleanup - inline documentation
Patrik Jakobsson
patrik.r.jakobsson at gmail.com
Fri Mar 14 16:04:35 PDT 2014
On Thu, Mar 13, 2014 at 10:49 PM, Arthur Borsboom
<arthurborsboom at gmail.com> wrote:
> Improve readability by adding/changing inline documentation
>
> Signed-off-by: Arthur Borsboom <arthurborsboom at gmail.com>
> ---
> drivers/gpu/drm/gma500/psb_drv.c | 56 +++++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/gma500/psb_drv.h | 24 +++++++++++------
> 2 files changed, 63 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index 1199180..5c6cdd0 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -44,7 +44,20 @@ static int psb_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> MODULE_PARM_DESC(trap_pagefaults, "Error and reset on MMU pagefaults");
> module_param_named(trap_pagefaults, drm_psb_trap_pagefaults, int, 0600);
>
> -
> +/*
> + * The table below contains a mapping of the PCI vendor ID and the PCI Device ID
> + * to the different groups of PowerVR 5-series chip designs
> + *
> + * 0x8086 = Intel Corporation
> + *
> + * PowerVR SGX535 - Poulsbo - Intel GMA 500, Intel Atom Z5xx
> + * PowerVR SGX535 - Moorestown - Intel GMA 600
> + * PowerVR SGX535 - Oaktrail - Intel GMA 600, Intel Atom Z6xx, E6xx
> + * PowerVR SGX540 - Medfield - Intel Atom Z2460
> + * PowerVR SGX544MP2 - Medfield -
> + * PowerVR SGX545 - Cedartrail - Intel GMA 3600, Intel Atom D2500, N2600
> + * PowerVR SGX545 - Cedartrail - Intel GMA 3650, Intel Atom D2550, D2700, N2800
There is no reason to break the 80 col line limit here. Here is Linus'
motivation
if that makes more sense than mine: https://lkml.org/lkml/2012/2/3/394
And sometimes we break it by mistake, but that is a different thing.
> + */
> static DEFINE_PCI_DEVICE_TABLE(pciidlist) = {
> { 0x8086, 0x8108, PCI_ANY_ID, PCI_ANY_ID, 0, 0, (long) &psb_chip_ops },
> { 0x8086, 0x8109, PCI_ANY_ID, PCI_ANY_ID, 0, 0, (long) &psb_chip_ops },
> @@ -207,8 +220,7 @@ static int psb_driver_unload(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
>
> - /* Kill vblank etc here */
> -
> + /* TODO: Kill vblank etc here */
>
> if (dev_priv) {
> if (dev_priv->backlight_device)
> @@ -268,7 +280,22 @@ static int psb_driver_unload(struct drm_device *dev)
> return 0;
> }
>
> -
> +/*
> + * psb_driver_load - setup chip and create an initial config
> + * @dev: DRM device
> + * @flags: startup flags, containing the driver_data field belonging to
> + * the PCI device ID
> + *
> + * The driver load routine has to do several things:
> + * - allocating and initializing driver private data
> + * - performing resource allocation and mapping
> + * - initialize the memory manager
> + * - setup the DRM framebuffer with the allocated memory
> + * - install the IRQ handler
> + * - setup vertical blanking handling
> + * - mode setting
> + * - set inital output configuration
> + */
This is not a constructive comment. It just repeats the DRM documentation. This
might look harmless but it is actually a bad thing. For every comment we add to
the driver, we add one more place that we must remember to update when we change
our code. If you look at the driver, you'll see plenty of places where comments
never got removed, even though the code it talks about is long gone.
And most of the time, the code speaks for itself.
> static int psb_driver_load(struct drm_device *dev, unsigned long chipset)
> {
> struct drm_psb_private *dev_priv;
> @@ -278,6 +305,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)
> struct drm_connector *connector;
> struct gma_encoder *gma_encoder;
>
> + /* allocating and initializing driver private data */
> dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> if (dev_priv == NULL)
> return -ENOMEM;
> @@ -369,6 +397,9 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)
>
> acpi_video_register();
>
> + /*
> + * Setup vertical blanking handling
> + */
Single line comments should be just /* ... comment ... */
See Documentation/CodingStyle Chapter 8.
> ret = drm_vblank_init(dev, dev_priv->num_pipe);
> if (ret)
> goto out_err;
> @@ -416,11 +447,11 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)
> return ret;
> psb_intel_opregion_enable_asle(dev);
> #if 0
> - /*enable runtime pm at last*/
> + /* Enable runtime pm at last */
> pm_runtime_enable(&dev->pdev->dev);
> pm_runtime_set_active(&dev->pdev->dev);
> #endif
> - /*Intel drm driver load is done, continue doing pvr load*/
> + /* Intel drm driver load is done, continue doing pvr load */
> return 0;
> out_err:
> psb_driver_unload(dev);
> @@ -561,7 +592,7 @@ static int psb_mode_operation_ioctl(struct drm_device *dev, void *data,
> arg->data = resp;
> }
>
> - /*do some clean up work*/
> + /* Do some clean up work */
> if (mode)
> drm_mode_destroy(dev, mode);
> mode_op_out:
> @@ -614,8 +645,8 @@ static long psb_unlocked_ioctl(struct file *filp, unsigned int cmd,
> /* FIXME: do we need to wrap the other side of this */
> }
>
> -
> -/* When a client dies:
> +/*
> + * When a client dies:
> * - Check for and clean up flipped page state
> */
> static void psb_driver_preclose(struct drm_device *dev, struct drm_file *priv)
> @@ -655,6 +686,13 @@ static const struct file_operations psb_gem_fops = {
> .read = drm_read,
> };
>
> +/*
> + * DRM driver structure initialization
> + *
> + * The drm_driver structure contains static information that describes
> + * the driver and features it supports, and pointers to methods that DRM
> + * core will call to implement DRM API.
> + */
This also just repeats the DRM documentation. No need for it.
> static struct drm_driver driver = {
> .driver_features = DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | \
> DRIVER_MODESET | DRIVER_GEM ,
> diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
> index 5ad6a03..85c560e 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.h
> +++ b/drivers/gpu/drm/gma500/psb_drv.h
> @@ -34,6 +34,19 @@
> #include "opregion.h"
> #include "oaktrail.h"
>
> +/*
> + * Driver definitions
> + */
This is just stating the obvious. The code speaks for itself.
> +
> +/*
> + * Driver history (deprecated)
Why _add_ a comment that it is deprecated when it never existed?
> + *
> + * The driver version was intended for ABI changes and it's not used anymore.
> + * There are better ways to tell userspace what features we expose.
> + *
> + * TODO: describe better ways
> + */
> +
This can also go.
> /* Append new drm mode definition here, align with libdrm definition */
> #define DRM_MODE_SCALE_NO_SCALE 2
>
> @@ -88,15 +101,11 @@ enum {
> #define _PSB_PGETBL_ENABLED 0x00000001
> #define PSB_SGX_2D_SLAVE_PORT 0x4000
>
> -/* To get rid of */
> +/* TODO: To get rid of */
> #define PSB_TT_PRIV0_LIMIT (256*1024*1024)
> #define PSB_TT_PRIV0_PLIMIT (PSB_TT_PRIV0_LIMIT >> PAGE_SHIFT)
>
> /*
> - * SGX side MMU definitions (these can probably go)
> - */
> -
> -/*
This comment is actually useful. It tells us that the MMU bits defined below
are for the SGX MMU page table entries and not the Intel GTT PTEs.
Though it could be combined with the comment below since they talk about
the same thing.
> * Flags for external memory type field.
> */
> #define PSB_MMU_CACHED_MEMORY 0x0001 /* Bind to MMU only */
> @@ -519,7 +528,7 @@ struct drm_psb_private {
> uint32_t num_pipe;
>
> /*
> - * OSPM info (Power management base) (can go ?)
> + * OSPM info (Power management base) (TODO: can go ?)
> */
> uint32_t ospm_base;
>
> @@ -766,9 +775,8 @@ extern void psb_mmu_remove_pages(struct psb_mmu_pd *pd,
> uint32_t desired_tile_stride,
> uint32_t hw_tile_stride);
> /*
> - *psb_irq.c
> + * psb_irq.c
> */
> -
> extern irqreturn_t psb_irq_handler(int irq, void *arg);
> extern int psb_irq_enable_dpst(struct drm_device *dev);
> extern int psb_irq_disable_dpst(struct drm_device *dev);
> --
> 1.9.0
>
More information about the dri-devel
mailing list