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>
