[PATCH 1/3] drm/gma500: Code cleanup - inline documentation
Arthur Borsboom
arthurborsboom at gmail.com
Sat Mar 15 00:31:55 PDT 2014
Hi Patrik,
Thanks for reviewing.
* The 80 character comment slipped with all the 80+ pci definitions. Will
fix.
* Some single line comments were already before in multiple line comment
style. Nevertheless, good moment to get rid of them. Will fix.
* Although for a beginner like me, I believe inline DRM documentation makes
reading the code easier. However, you are right that maintaining the same
documentation twice is a waste of time. Will fix.
* Driver definitions and driver history were copied from i915 to get more
in line. Nevertheless, you might be right; it might be an overkill. Will
fix.
* SGU MMX comment will be merged.
* The 72 column git commit makes sense. Maybe this explains why my editor
was predefined with this number. ;) Will fix.
Be prepared for three new patches.
On 15 March 2014 00:04, Patrik Jakobsson <patrik.r.jakobsson at gmail.com>wrote:
> 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
> >
>
--
Arthur Borsboom
11 Rue du Manerick
44740 Batz Sur Mer, France
Mob: +33785927118
Email: arthurborsboom at gmail.com
Skype: Arthur Borsboom, The Hague, The Netherlands
[image: View Arthur's LinkedIn
profile]<http://uk.linkedin.com/in/arthurborsboom>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140315/53f08bef/attachment-0001.html>
More information about the dri-devel
mailing list