<div dir="ltr">Hi Patrik,<div><br></div><div>Thanks for reviewing.</div><div><br></div><div>* The 80 character comment slipped with all the 80+ pci definitions. Will fix.</div><div>* Some single line comments were already before in multiple line comment style. Nevertheless, good moment to get rid of them. Will fix.</div>
<div>* 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.</div><div>* 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.</div>
<div>* SGU MMX comment will be merged.</div><div>* The 72 column git commit makes sense. Maybe this explains why my editor was predefined with this number. ;) Will fix.</div><div><br></div><div>Be prepared for three new patches.<br>
<div class="gmail_extra"><br><div class="gmail_quote">On 15 March 2014 00:04, Patrik Jakobsson <span dir="ltr"><<a href="mailto:patrik.r.jakobsson@gmail.com" target="_blank">patrik.r.jakobsson@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Mar 13, 2014 at 10:49 PM, Arthur Borsboom<br>
<<a href="mailto:arthurborsboom@gmail.com">arthurborsboom@gmail.com</a>> wrote:<br>
> Improve readability by adding/changing inline documentation<br>
><br>
> Signed-off-by: Arthur Borsboom <<a href="mailto:arthurborsboom@gmail.com">arthurborsboom@gmail.com</a>><br>
> ---<br>
> drivers/gpu/drm/gma500/psb_drv.c | 56 +++++++++++++++++++++++++++++++++-------<br>
> drivers/gpu/drm/gma500/psb_drv.h | 24 +++++++++++------<br>
> 2 files changed, 63 insertions(+), 17 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c<br>
> index 1199180..5c6cdd0 100644<br>
> --- a/drivers/gpu/drm/gma500/psb_drv.c<br>
> +++ b/drivers/gpu/drm/gma500/psb_drv.c<br>
> @@ -44,7 +44,20 @@ static int psb_probe(struct pci_dev *pdev, const struct pci_device_id *ent);<br>
> MODULE_PARM_DESC(trap_pagefaults, "Error and reset on MMU pagefaults");<br>
> module_param_named(trap_pagefaults, drm_psb_trap_pagefaults, int, 0600);<br>
><br>
> -<br>
> +/*<br>
> + * The table below contains a mapping of the PCI vendor ID and the PCI Device ID<br>
> + * to the different groups of PowerVR 5-series chip designs<br>
> + *<br>
> + * 0x8086 = Intel Corporation<br>
> + *<br>
> + * PowerVR SGX535 - Poulsbo - Intel GMA 500, Intel Atom Z5xx<br>
> + * PowerVR SGX535 - Moorestown - Intel GMA 600<br>
> + * PowerVR SGX535 - Oaktrail - Intel GMA 600, Intel Atom Z6xx, E6xx<br>
> + * PowerVR SGX540 - Medfield - Intel Atom Z2460<br>
> + * PowerVR SGX544MP2 - Medfield -<br>
> + * PowerVR SGX545 - Cedartrail - Intel GMA 3600, Intel Atom D2500, N2600<br>
> + * PowerVR SGX545 - Cedartrail - Intel GMA 3650, Intel Atom D2550, D2700, N2800<br>
<br>
</div></div>There is no reason to break the 80 col line limit here. Here is Linus'<br>
motivation<br>
if that makes more sense than mine: <a href="https://lkml.org/lkml/2012/2/3/394" target="_blank">https://lkml.org/lkml/2012/2/3/394</a><br>
<br>
And sometimes we break it by mistake, but that is a different thing.<br>
<div><div class="h5"><br>
> + */<br>
> static DEFINE_PCI_DEVICE_TABLE(pciidlist) = {<br>
> { 0x8086, 0x8108, PCI_ANY_ID, PCI_ANY_ID, 0, 0, (long) &psb_chip_ops },<br>
> { 0x8086, 0x8109, PCI_ANY_ID, PCI_ANY_ID, 0, 0, (long) &psb_chip_ops },<br>
> @@ -207,8 +220,7 @@ static int psb_driver_unload(struct drm_device *dev)<br>
> {<br>
> struct drm_psb_private *dev_priv = dev->dev_private;<br>
><br>
> - /* Kill vblank etc here */<br>
> -<br>
> + /* TODO: Kill vblank etc here */<br>
><br>
> if (dev_priv) {<br>
> if (dev_priv->backlight_device)<br>
> @@ -268,7 +280,22 @@ static int psb_driver_unload(struct drm_device *dev)<br>
> return 0;<br>
> }<br>
><br>
> -<br>
> +/*<br>
> + * psb_driver_load - setup chip and create an initial config<br>
> + * @dev: DRM device<br>
> + * @flags: startup flags, containing the driver_data field belonging to<br>
> + * the PCI device ID<br>
> + *<br>
> + * The driver load routine has to do several things:<br>
> + * - allocating and initializing driver private data<br>
> + * - performing resource allocation and mapping<br>
> + * - initialize the memory manager<br>
> + * - setup the DRM framebuffer with the allocated memory<br>
> + * - install the IRQ handler<br>
> + * - setup vertical blanking handling<br>
> + * - mode setting<br>
> + * - set inital output configuration<br>
> + */<br>
<br>
</div></div>This is not a constructive comment. It just repeats the DRM documentation. This<br>
might look harmless but it is actually a bad thing. For every comment we add to<br>
the driver, we add one more place that we must remember to update when we change<br>
our code. If you look at the driver, you'll see plenty of places where comments<br>
never got removed, even though the code it talks about is long gone.<br>
<br>
And most of the time, the code speaks for itself.<br>
<div class=""><br>
> static int psb_driver_load(struct drm_device *dev, unsigned long chipset)<br>
> {<br>
> struct drm_psb_private *dev_priv;<br>
> @@ -278,6 +305,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)<br>
> struct drm_connector *connector;<br>
> struct gma_encoder *gma_encoder;<br>
><br>
> + /* allocating and initializing driver private data */<br>
> dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);<br>
> if (dev_priv == NULL)<br>
> return -ENOMEM;<br>
> @@ -369,6 +397,9 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)<br>
><br>
> acpi_video_register();<br>
><br>
> + /*<br>
> + * Setup vertical blanking handling<br>
> + */<br>
<br>
</div>Single line comments should be just /* ... comment ... */<br>
See Documentation/CodingStyle Chapter 8.<br>
<div><div class="h5"><br>
> ret = drm_vblank_init(dev, dev_priv->num_pipe);<br>
> if (ret)<br>
> goto out_err;<br>
> @@ -416,11 +447,11 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)<br>
> return ret;<br>
> psb_intel_opregion_enable_asle(dev);<br>
> #if 0<br>
> - /*enable runtime pm at last*/<br>
> + /* Enable runtime pm at last */<br>
> pm_runtime_enable(&dev->pdev->dev);<br>
> pm_runtime_set_active(&dev->pdev->dev);<br>
> #endif<br>
> - /*Intel drm driver load is done, continue doing pvr load*/<br>
> + /* Intel drm driver load is done, continue doing pvr load */<br>
> return 0;<br>
> out_err:<br>
> psb_driver_unload(dev);<br>
> @@ -561,7 +592,7 @@ static int psb_mode_operation_ioctl(struct drm_device *dev, void *data,<br>
> arg->data = resp;<br>
> }<br>
><br>
> - /*do some clean up work*/<br>
> + /* Do some clean up work */<br>
> if (mode)<br>
> drm_mode_destroy(dev, mode);<br>
> mode_op_out:<br>
> @@ -614,8 +645,8 @@ static long psb_unlocked_ioctl(struct file *filp, unsigned int cmd,<br>
> /* FIXME: do we need to wrap the other side of this */<br>
> }<br>
><br>
> -<br>
> -/* When a client dies:<br>
> +/*<br>
> + * When a client dies:<br>
> * - Check for and clean up flipped page state<br>
> */<br>
> static void psb_driver_preclose(struct drm_device *dev, struct drm_file *priv)<br>
> @@ -655,6 +686,13 @@ static const struct file_operations psb_gem_fops = {<br>
> .read = drm_read,<br>
> };<br>
><br>
> +/*<br>
> + * DRM driver structure initialization<br>
> + *<br>
> + * The drm_driver structure contains static information that describes<br>
> + * the driver and features it supports, and pointers to methods that DRM<br>
> + * core will call to implement DRM API.<br>
> + */<br>
<br>
</div></div>This also just repeats the DRM documentation. No need for it.<br>
<div class=""><br>
> static struct drm_driver driver = {<br>
> .driver_features = DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | \<br>
> DRIVER_MODESET | DRIVER_GEM ,<br>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h<br>
> index 5ad6a03..85c560e 100644<br>
> --- a/drivers/gpu/drm/gma500/psb_drv.h<br>
> +++ b/drivers/gpu/drm/gma500/psb_drv.h<br>
> @@ -34,6 +34,19 @@<br>
> #include "opregion.h"<br>
> #include "oaktrail.h"<br>
><br>
> +/*<br>
> + * Driver definitions<br>
> + */<br>
<br>
</div>This is just stating the obvious. The code speaks for itself.<br>
<div class=""><br>
> +<br>
> +/*<br>
> + * Driver history (deprecated)<br>
<br>
</div>Why _add_ a comment that it is deprecated when it never existed?<br>
<div class=""><br>
> + *<br>
> + * The driver version was intended for ABI changes and it's not used anymore.<br>
> + * There are better ways to tell userspace what features we expose.<br>
> + *<br>
> + * TODO: describe better ways<br>
> + */<br>
> +<br>
<br>
</div>This can also go.<br>
<div class=""><br>
> /* Append new drm mode definition here, align with libdrm definition */<br>
> #define DRM_MODE_SCALE_NO_SCALE 2<br>
><br>
> @@ -88,15 +101,11 @@ enum {<br>
> #define _PSB_PGETBL_ENABLED 0x00000001<br>
> #define PSB_SGX_2D_SLAVE_PORT 0x4000<br>
><br>
> -/* To get rid of */<br>
> +/* TODO: To get rid of */<br>
> #define PSB_TT_PRIV0_LIMIT (256*1024*1024)<br>
> #define PSB_TT_PRIV0_PLIMIT (PSB_TT_PRIV0_LIMIT >> PAGE_SHIFT)<br>
><br>
> /*<br>
> - * SGX side MMU definitions (these can probably go)<br>
> - */<br>
> -<br>
> -/*<br>
<br>
</div>This comment is actually useful. It tells us that the MMU bits defined below<br>
are for the SGX MMU page table entries and not the Intel GTT PTEs.<br>
Though it could be combined with the comment below since they talk about<br>
the same thing.<br>
<div class="HOEnZb"><div class="h5"><br>
> * Flags for external memory type field.<br>
> */<br>
> #define PSB_MMU_CACHED_MEMORY 0x0001 /* Bind to MMU only */<br>
> @@ -519,7 +528,7 @@ struct drm_psb_private {<br>
> uint32_t num_pipe;<br>
><br>
> /*<br>
> - * OSPM info (Power management base) (can go ?)<br>
> + * OSPM info (Power management base) (TODO: can go ?)<br>
> */<br>
> uint32_t ospm_base;<br>
><br>
> @@ -766,9 +775,8 @@ extern void psb_mmu_remove_pages(struct psb_mmu_pd *pd,<br>
> uint32_t desired_tile_stride,<br>
> uint32_t hw_tile_stride);<br>
> /*<br>
> - *psb_irq.c<br>
> + * psb_irq.c<br>
> */<br>
> -<br>
> extern irqreturn_t psb_irq_handler(int irq, void *arg);<br>
> extern int psb_irq_enable_dpst(struct drm_device *dev);<br>
> extern int psb_irq_disable_dpst(struct drm_device *dev);<br>
> --<br>
> 1.9.0<br>
><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div style="color:rgb(136,136,136)"><font face="tahoma, sans-serif">Arthur Borsboom</font></div><div style="color:rgb(136,136,136)">
<span style="text-align:right;color:rgb(153,153,153);font-family:tahoma,sans-serif;font-size:small">11 Rue du Manerick</span></div><div style="color:rgb(136,136,136)"><span style="text-align:right;color:rgb(153,153,153);font-family:tahoma,sans-serif;font-size:small">44740 Batz Sur Mer, France</span></div>
<div><span style="color:rgb(136,136,136);text-align:right">Mob: </span><font color="#888888">+33785927118</font><br></div><div><div style="color:rgb(136,136,136);font-size:small;font-family:Tahoma">Email: <a href="mailto:arthurborsboom@gmail.com" style="color:rgb(17,85,204)" target="_blank">arthurborsboom@gmail.com</a></div>
</div><div style="color:rgb(136,136,136);font-size:small;font-family:Tahoma">Skype: Arthur Borsboom, The Hague, The Netherlands</div><div style="color:rgb(136,136,136);font-size:small;font-family:Tahoma"><br></div><div style="color:rgb(136,136,136);font-size:small;font-family:Tahoma">
<a href="http://uk.linkedin.com/in/arthurborsboom" target="_blank"><img src="https://static.licdn.com/scds/common/u/img/webpromo/btn_viewmy_160x25.png" alt=" View Arthur's LinkedIn profile"></a><br></div><span style="color:rgb(136,136,136);font-family:Tahoma"></span></div>
</div></div></div>