diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..28954b3 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -247,6 +247,10 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
 {
 	int err;
 	struct gr2d *gr2d = NULL;
+	/* Cause drm_device is created in host1x driver. So
+	   if host1x drivers loads after tegradrm, then in this
+	   gr2d_probe function, this "drm_device" will be NULL.
+	   How can we handle this? Defer driver probe? */
 	struct platform_device *drm_device =
 		host1x_drm_device(to_platform_device(dev->dev.parent));
 	struct tegradrm *tegradrm = platform_get_drvdata(drm_device);
@@ -273,6 +277,11 @@ static int __devinit gr2d_probe(struct
platform_device *dev)

 	gr2d->syncpt = host1x_syncpt_alloc(dev, 0);
 	if (!gr2d->syncpt) {
+		/* Do we really need this?
+		   After "host1x_channel_alloc", the refcount of this
+		   channel is 0. So calling host1x_channel_put here will
+		   make the refcount going to negative.
+		   I suppose we should call "host1x_free_channel" here. */
 		host1x_channel_put(gr2d->channel);
 		return -ENOMEM;
 	}
@@ -284,6 +293,7 @@ static int __devinit gr2d_probe(struct
platform_device *dev)

 	err = tegra_drm_register_client(tegradrm, &gr2d->client);
 	if (err)
+		/* Add "host1x_free_channel" */
 		return err;

 	platform_set_drvdata(dev, gr2d);
diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
index 3705cae..ed83b9e 100644
--- a/drivers/gpu/host1x/channel.c
+++ b/drivers/gpu/host1x/channel.c
@@ -95,6 +95,9 @@ struct host1x_channel *host1x_channel_alloc(struct
platform_device *pdev)
 	int max_channels = host1x->info.nb_channels;
 	int err;

+	/* Is it necessary to add mutex protection here?
+	   I'm just wondering in a smp system, multiple host1x clients
+	   may try to alloc their channels simultaneously... */
 	if (chindex > max_channels)
 		return NULL;

diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
index 86d5c70..f2bd5aa 100644
--- a/drivers/gpu/host1x/debug.c
+++ b/drivers/gpu/host1x/debug.c
@@ -164,6 +164,10 @@ static const struct file_operations
host1x_debug_fops = {

 void host1x_debug_init(struct host1x *host1x)
 {
+	/* I think it's better to set this directory name the same with
+	   the driver's name -- defined in dev.c:
+	   #define DRIVER_NAME	"tegra-host1x"
+	   */
 	struct dentry *de = debugfs_create_dir("tegra_host", NULL);

 	if (!de)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 07e8813..01ed10d 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -38,6 +38,7 @@

 struct host1x *host1x;

+/* Called by drm unlocked ioctl function. So do we need a lock here? */
 void host1x_syncpt_incr_byid(u32 id)
 {
 	struct host1x_syncpt *sp = host1x->syncpt + id;
@@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id)
 }
 EXPORT_SYMBOL(host1x_syncpt_read_byid);

+/* Called by drm unlocked ioctl function. So do we need a lock here? */
 int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)
 {
 	struct host1x_syncpt *sp = host1x->syncpt + id;
@@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)

 	err = host1x_alloc_resources(host);
 	if (err) {
+		/* We don't have chip_ops right now, so here the
+		   error message is somewhat improper */
 		dev_err(&dev->dev, "failed to init chip support\n");
 		goto fail;
 	}
@@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev)
 	if (!host->syncpt)
 		goto fail;

+	/* I suggest create a dedicate function for initializing nop sp.
+	   First this "_host1x_syncpt_alloc" looks like an internal/static
+	   function.
+	   Then actually "host1x_syncpt_alloc" & "_host1x_syncpt_alloc" all
+	   serve host1x client(e.g: gr2d) so it's not suitable to use them
+	   for nop sp.
+	   Just create a wrapper function to call _host1x_syncpt_alloc is OK.
+	   This will make the code more readable. */
 	host->nop_sp = _host1x_syncpt_alloc(host, NULL, 0);
 	if (!host->nop_sp)
 		goto fail;
@@ -191,6 +203,7 @@ static int host1x_probe(struct platform_device *dev)
 	}

 	err = clk_prepare_enable(host->clk);
+	/* Add a dev_err here */
 	if (err < 0)
 		goto fail;

@@ -209,9 +222,12 @@ static int host1x_probe(struct platform_device *dev)
 	return 0;

 fail:
+	/* host1x->syncpt isn't released here... */
+	/* host1x->intr isn't release here... */
+	/* Remove debugfs stuffs? */
 	host1x_syncpt_free(host->nop_sp);
 	host1x_unregister_drm_device(host);
-	kfree(host);
+	kfree(host); /* not necessary*/
 	return err;
 }

@@ -222,6 +238,7 @@ static int __exit host1x_remove(struct
platform_device *dev)
 	host1x_syncpt_deinit(host);
 	host1x_unregister_drm_device(host);
 	clk_disable_unprepare(host->clk);
+	/* Remove debugfs stuffs? */
 	return 0;
 }

diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index 05f8544..58f4c71 100644
--- a/drivers/gpu/host1x/dev.h
+++ b/drivers/gpu/host1x/dev.h
@@ -36,6 +36,7 @@ struct platform_device;
 struct output;

 struct host1x_channel_ops {
+	/* Seems no one uses this member. Remove it. */
 	const char *soc_name;
 	int (*init)(struct host1x_channel *,
 		    struct host1x *,
@@ -125,12 +126,18 @@ struct host1x {
 	struct host1x_syncpt *syncpt;
 	struct host1x_intr intr;
 	struct platform_device *dev;
+
+	/* Seems no one uses this. Remove it. */
 	atomic_t clientid;
 	struct host1x_device_info info;
 	struct clk *clk;

+	/* Put some comments for this member.
+	   For guys who're not familiar with nop sp, I think they'll
+	   definitely get confused about this. */
 	struct host1x_syncpt *nop_sp;

+	/* Seems no one uses this member. Remove it. */
 	const char *soc_name;
 	struct host1x_channel_ops channel_op;
 	struct host1x_cdma_ops cdma_op;
@@ -140,6 +147,7 @@ struct host1x {
 	struct host1x_intr_ops intr_op;

 	struct host1x_channel chlist;
+
 	int allocated_channels;

 	struct dentry *debugfs;
diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
index efcb9be..e112001 100644
--- a/drivers/gpu/host1x/intr.c
+++ b/drivers/gpu/host1x/intr.c
@@ -329,8 +329,13 @@ void host1x_intr_deinit(struct host1x_intr *intr)
 void host1x_intr_start(struct host1x_intr *intr, u32 hz)
 {
 	struct host1x *host1x = intr_to_host1x(intr);
+
+	/* Why we need to lock here? Seems like this function is
+	   called by host1x's probe only. */
 	mutex_lock(&intr->mutex);

+	/* "init_host_sync" has already been called in function
+	   host1x_intr_init. Remove this line. */
 	host1x->intr_op.init_host_sync(intr);
 	host1x->intr_op.set_host_clocks_per_usec(intr,
 			DIV_ROUND_UP(hz, 1000000));
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 07cbca5..9a234ad 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -309,6 +309,8 @@ struct host1x_syncpt *host1x_syncpt_init(struct
host1x *host)
 	struct host1x_syncpt *syncpt, *sp;
 	int i;

+	/* Consider devm_kzalloc here. Then you can forget the release
+	   stuffs about this "syncpt". */
 	syncpt = sp = kzalloc(sizeof(struct host1x_syncpt) * host->info.nb_pts,
 		GFP_KERNEL);