[PATCH 0/5] staging: tidspbridge: clean up drv_interface.c

[PATCH 0/5] staging: tidspbridge: clean up drv_interface.c

[Víctor Manuel Jáquez Leal]

I'm trying to learn how to contribute to the kernel and dsp/bridge is
a module that I have used for a while.

These patches are the result of this first effort. It is a clean up of
the file drv_interface.c which is the entry point of the kernel
module.

I would like to have some review in order to know if I could keep
moving in this direction.

Thanks

vmjl

Víctor Manuel Jáquez Leal (5):
  staging: tidspbridge: more readable code
  staging: tidspbridge: remove unused header
  staging: tidspbridge: Lindent to drv_interface.c
  staging: tidspbridge: silence the compiler
  staging: tidspbridge: remove header inclusions

 drivers/staging/tidspbridge/rmgr/drv_interface.c |  352 +++++++++++-----------
 drivers/staging/tidspbridge/rmgr/drv_interface.h |   28 --
 2 files changed, 171 insertions(+), 209 deletions(-)
 delete mode 100644 drivers/staging/tidspbridge/rmgr/drv_interface.h

-- 
1.7.8.3

[PATCH 1/5] staging: tidspbridge: more readable code

[Víctor Manuel Jáquez Leal]

Uppercase function names are not pretty. Also the code flow readability is
enhanced.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 76cfc6e..85f6e8e 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -428,7 +428,7 @@ func_cont:
 }
 
 #ifdef CONFIG_PM
-static int BRIDGE_SUSPEND(struct platform_device *pdev, pm_message_t state)
+static int bridge_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	u32 status;
 	u32 command = PWR_EMERGENCYDEEPSLEEP;
@@ -441,7 +441,7 @@ static int BRIDGE_SUSPEND(struct platform_device *pdev, pm_message_t state)
 	return 0;
 }
 
-static int BRIDGE_RESUME(struct platform_device *pdev)
+static int bridge_resume(struct platform_device *pdev)
 {
 	u32 status;
 
@@ -453,9 +453,6 @@ static int BRIDGE_RESUME(struct platform_device *pdev)
 	wake_up(&bridge_suspend_data.suspend_wq);
 	return 0;
 }
-#else
-#define BRIDGE_SUSPEND NULL
-#define BRIDGE_RESUME NULL
 #endif
 
 static struct platform_driver bridge_driver = {
@@ -464,8 +461,10 @@ static struct platform_driver bridge_driver = {
 		   },
 	.probe = omap34_xx_bridge_probe,
 	.remove = __devexit_p(omap34_xx_bridge_remove),
-	.suspend = BRIDGE_SUSPEND,
-	.resume = BRIDGE_RESUME,
+#ifdef CONFIG_PM
+	.suspend = bridge_suspend,
+	.resume = bridge_resume,
+#endif
 };
 
 static int __init bridge_init(void)
-- 
1.7.8.3

Re: [PATCH 1/5] staging: tidspbridge: more readable code

[Felipe Contreras]

2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
> Uppercase function names are not pretty. Also the code flow readability is
> enhanced.

Looks good to me.

-- 
Felipe Contreras

Re: [PATCH 1/5] staging: tidspbridge: more readable code

[Ramirez Luna, Omar]

2012/1/24 Felipe Contreras <felipe.contreras@gmail.com>:
> 2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
>> Uppercase function names are not pretty. Also the code flow readability is
>> enhanced.
>
> Looks good to me.

FWIW, I agree.

Regards,

Omar

[PATCH 2/5] staging: tidspbridge: remove unused header

[Víctor Manuel Jáquez Leal]

No functional changes.

The header file drv_interface.h was only used locally, hence there's no need
to have it.

Also the only prototyped functions were the file_operations callbacks, then
this commit moves them up to avoid prototyping too.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |  313 +++++++++++-----------
 drivers/staging/tidspbridge/rmgr/drv_interface.h |   28 --
 2 files changed, 155 insertions(+), 186 deletions(-)
 delete mode 100644 drivers/staging/tidspbridge/rmgr/drv_interface.h

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 85f6e8e..5ea753c 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -48,9 +48,6 @@
 /*  ----------------------------------- Resource Manager */
 #include <dspbridge/pwr.h>
 
-/*  ----------------------------------- This */
-#include <drv_interface.h>
-
 #include <dspbridge/resourcecleanup.h>
 #include <dspbridge/chnl.h>
 #include <dspbridge/proc.h>
@@ -133,6 +130,161 @@ MODULE_VERSION(DSPBRIDGE_VERSION);
 
 static char *driver_name = DRIVER_NAME;
 
+/*
+ * This function is called when an application opens handle to the
+ * bridge driver.
+ */
+static int bridge_open(struct inode *ip, struct file *filp)
+{
+	int status = 0;
+	struct process_context *pr_ctxt = NULL;
+
+	/*
+	 * Allocate a new process context and insert it into global
+	 * process context list.
+	 */
+
+#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
+	if (recover) {
+		if (filp->f_flags & O_NONBLOCK ||
+			wait_for_completion_interruptible(&bridge_open_comp))
+			return -EBUSY;
+	}
+#endif
+	pr_ctxt = kzalloc(sizeof(struct process_context), GFP_KERNEL);
+	if (pr_ctxt) {
+		pr_ctxt->res_state = PROC_RES_ALLOCATED;
+		spin_lock_init(&pr_ctxt->dmm_map_lock);
+		INIT_LIST_HEAD(&pr_ctxt->dmm_map_list);
+		spin_lock_init(&pr_ctxt->dmm_rsv_lock);
+		INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list);
+
+		pr_ctxt->node_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
+		if (pr_ctxt->node_id) {
+			idr_init(pr_ctxt->node_id);
+		} else {
+			status = -ENOMEM;
+			goto err;
+		}
+
+		pr_ctxt->stream_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
+		if (pr_ctxt->stream_id)
+			idr_init(pr_ctxt->stream_id);
+		else
+			status = -ENOMEM;
+	} else {
+		status = -ENOMEM;
+	}
+err:
+	filp->private_data = pr_ctxt;
+#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
+	if (!status)
+		atomic_inc(&bridge_cref);
+#endif
+	return status;
+}
+
+/*
+ * This function is called when an application closes handle to the bridge
+ * driver.
+ */
+static int bridge_release(struct inode *ip, struct file *filp)
+{
+	int status = 0;
+	struct process_context *pr_ctxt;
+
+	if (!filp->private_data) {
+		status = -EIO;
+		goto err;
+	}
+
+	pr_ctxt = filp->private_data;
+	flush_signals(current);
+	drv_remove_all_resources(pr_ctxt);
+	proc_detach(pr_ctxt);
+	kfree(pr_ctxt);
+
+	filp->private_data = NULL;
+
+err:
+#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
+	if (!atomic_dec_return(&bridge_cref))
+		complete(&bridge_comp);
+#endif
+	return status;
+}
+
+/* This function provides IO interface to the bridge driver. */
+static long bridge_ioctl(struct file *filp, unsigned int code,
+			 unsigned long args)
+{
+	int status;
+	u32 retval = 0;
+	union trapped_args buf_in;
+
+	DBC_REQUIRE(filp != NULL);
+#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
+	if (recover) {
+		status = -EIO;
+		goto err;
+	}
+#endif
+#ifdef CONFIG_PM
+	status = omap34_xxbridge_suspend_lockout(&bridge_suspend_data, filp);
+	if (status != 0)
+		return status;
+#endif
+
+	if (!filp->private_data) {
+		status = -EIO;
+		goto err;
+	}
+
+	status = copy_from_user(&buf_in, (union trapped_args *)args,
+				sizeof(union trapped_args));
+
+	if (!status) {
+		status = api_call_dev_ioctl(code, &buf_in, &retval,
+					     filp->private_data);
+
+		if (!status) {
+			status = retval;
+		} else {
+			dev_dbg(bridge, "%s: IOCTL Failed, code: 0x%x "
+				"status 0x%x\n", __func__, code, status);
+			status = -1;
+		}
+
+	}
+
+err:
+	return status;
+}
+
+/* This function maps kernel space memory to user space memory. */
+static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	u32 offset = vma->vm_pgoff << PAGE_SHIFT;
+	u32 status;
+
+	DBC_ASSERT(vma->vm_start < vma->vm_end);
+
+	vma->vm_flags |= VM_RESERVED | VM_IO;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	dev_dbg(bridge, "%s: vm filp %p offset %x start %lx end %lx page_prot "
+		"%lx flags %lx\n", __func__, filp, offset,
+		vma->vm_start, vma->vm_end, vma->vm_page_prot, vma->vm_flags);
+
+	status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+				 vma->vm_end - vma->vm_start,
+				 vma->vm_page_prot);
+	if (status != 0)
+		status = -EAGAIN;
+
+	return status;
+}
+
 static const struct file_operations bridge_fops = {
 	.open = bridge_open,
 	.release = bridge_release,
@@ -477,161 +629,6 @@ static void __exit bridge_exit(void)
 	platform_driver_unregister(&bridge_driver);
 }
 
-/*
- * This function is called when an application opens handle to the
- * bridge driver.
- */
-static int bridge_open(struct inode *ip, struct file *filp)
-{
-	int status = 0;
-	struct process_context *pr_ctxt = NULL;
-
-	/*
-	 * Allocate a new process context and insert it into global
-	 * process context list.
-	 */
-
-#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
-	if (recover) {
-		if (filp->f_flags & O_NONBLOCK ||
-			wait_for_completion_interruptible(&bridge_open_comp))
-			return -EBUSY;
-	}
-#endif
-	pr_ctxt = kzalloc(sizeof(struct process_context), GFP_KERNEL);
-	if (pr_ctxt) {
-		pr_ctxt->res_state = PROC_RES_ALLOCATED;
-		spin_lock_init(&pr_ctxt->dmm_map_lock);
-		INIT_LIST_HEAD(&pr_ctxt->dmm_map_list);
-		spin_lock_init(&pr_ctxt->dmm_rsv_lock);
-		INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list);
-
-		pr_ctxt->node_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
-		if (pr_ctxt->node_id) {
-			idr_init(pr_ctxt->node_id);
-		} else {
-			status = -ENOMEM;
-			goto err;
-		}
-
-		pr_ctxt->stream_id = kzalloc(sizeof(struct idr), GFP_KERNEL);
-		if (pr_ctxt->stream_id)
-			idr_init(pr_ctxt->stream_id);
-		else
-			status = -ENOMEM;
-	} else {
-		status = -ENOMEM;
-	}
-err:
-	filp->private_data = pr_ctxt;
-#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
-	if (!status)
-		atomic_inc(&bridge_cref);
-#endif
-	return status;
-}
-
-/*
- * This function is called when an application closes handle to the bridge
- * driver.
- */
-static int bridge_release(struct inode *ip, struct file *filp)
-{
-	int status = 0;
-	struct process_context *pr_ctxt;
-
-	if (!filp->private_data) {
-		status = -EIO;
-		goto err;
-	}
-
-	pr_ctxt = filp->private_data;
-	flush_signals(current);
-	drv_remove_all_resources(pr_ctxt);
-	proc_detach(pr_ctxt);
-	kfree(pr_ctxt);
-
-	filp->private_data = NULL;
-
-err:
-#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
-	if (!atomic_dec_return(&bridge_cref))
-		complete(&bridge_comp);
-#endif
-	return status;
-}
-
-/* This function provides IO interface to the bridge driver. */
-static long bridge_ioctl(struct file *filp, unsigned int code,
-			 unsigned long args)
-{
-	int status;
-	u32 retval = 0;
-	union trapped_args buf_in;
-
-	DBC_REQUIRE(filp != NULL);
-#ifdef CONFIG_TIDSPBRIDGE_RECOVERY
-	if (recover) {
-		status = -EIO;
-		goto err;
-	}
-#endif
-#ifdef CONFIG_PM
-	status = omap34_xxbridge_suspend_lockout(&bridge_suspend_data, filp);
-	if (status != 0)
-		return status;
-#endif
-
-	if (!filp->private_data) {
-		status = -EIO;
-		goto err;
-	}
-
-	status = copy_from_user(&buf_in, (union trapped_args *)args,
-				sizeof(union trapped_args));
-
-	if (!status) {
-		status = api_call_dev_ioctl(code, &buf_in, &retval,
-					     filp->private_data);
-
-		if (!status) {
-			status = retval;
-		} else {
-			dev_dbg(bridge, "%s: IOCTL Failed, code: 0x%x "
-				"status 0x%x\n", __func__, code, status);
-			status = -1;
-		}
-
-	}
-
-err:
-	return status;
-}
-
-/* This function maps kernel space memory to user space memory. */
-static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	u32 offset = vma->vm_pgoff << PAGE_SHIFT;
-	u32 status;
-
-	DBC_ASSERT(vma->vm_start < vma->vm_end);
-
-	vma->vm_flags |= VM_RESERVED | VM_IO;
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-
-	dev_dbg(bridge, "%s: vm filp %p offset %x start %lx end %lx page_prot "
-		"%lx flags %lx\n", __func__, filp, offset,
-		vma->vm_start, vma->vm_end, vma->vm_page_prot, vma->vm_flags);
-
-	status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-				 vma->vm_end - vma->vm_start,
-				 vma->vm_page_prot);
-	if (status != 0)
-		status = -EAGAIN;
-
-	return status;
-}
-
 /* To remove all process resources before removing the process from the
  * process context list */
 int drv_remove_all_resources(void *process_ctxt)
diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.h b/drivers/staging/tidspbridge/rmgr/drv_interface.h
deleted file mode 100644
index ab07060..0000000
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.h
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * drv_interface.h
- *
- * DSP-BIOS Bridge driver support functions for TI OMAP processors.
- *
- * Copyright (C) 2005-2006 Texas Instruments, Inc.
- *
- * This package is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
- */
-
-#ifndef	_DRV_INTERFACE_H_
-#define _DRV_INTERFACE_H_
-
-/* Prototypes for all functions in this bridge */
-static int __init bridge_init(void);	/* Initialize bridge */
-static void __exit bridge_exit(void);	/* Opposite of initialize */
-static int bridge_open(struct inode *ip, struct file *filp);	/* Open */
-static int bridge_release(struct inode *ip, struct file *filp);	/* Release */
-static long bridge_ioctl(struct file *filp, unsigned int code,
-				unsigned long args);
-static int bridge_mmap(struct file *filp, struct vm_area_struct *vma);
-#endif /* ifndef _DRV_INTERFACE_H_ */
-- 
1.7.8.3

Re: [PATCH 2/5] staging: tidspbridge: remove unused header

[Felipe Contreras]

2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
> No functional changes.
>
> The header file drv_interface.h was only used locally, hence there's no need
> to have it.
>
> Also the only prototyped functions were the file_operations callbacks, then
> this commit moves them up to avoid prototyping too.

'Also' is a keyword that usually denotes that we are talking about two
patches here: one that reshuffles the code, and another one that
removes the header.

But looks good to me either way.

-- 
Felipe Contreras

Re: [PATCH 2/5] staging: tidspbridge: remove unused header

[Ramirez Luna, Omar]

2012/1/24 Felipe Contreras <felipe.contreras@gmail.com>:
> 2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
>> No functional changes.
>>
>> The header file drv_interface.h was only used locally, hence there's no need
>> to have it.
>>
>> Also the only prototyped functions were the file_operations callbacks, then
>> this commit moves them up to avoid prototyping too.
...
>
> But looks good to me either way.

FWIW, I agree.

Regards,

Omar

[PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Víctor Manuel Jáquez Leal]

No functional changes.

According to Lindent, the file drv_internface.c had some lines with bad
indentation.

This commit is the output of Lindent.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 5ea753c..c7015f5 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -147,7 +147,7 @@ static int bridge_open(struct inode *ip, struct file *filp)
 #ifdef CONFIG_TIDSPBRIDGE_RECOVERY
 	if (recover) {
 		if (filp->f_flags & O_NONBLOCK ||
-			wait_for_completion_interruptible(&bridge_open_comp))
+		    wait_for_completion_interruptible(&bridge_open_comp))
 			return -EBUSY;
 	}
 #endif
@@ -245,7 +245,7 @@ static long bridge_ioctl(struct file *filp, unsigned int code,
 
 	if (!status) {
 		status = api_call_dev_ioctl(code, &buf_in, &retval,
-					     filp->private_data);
+					    filp->private_data);
 
 		if (!status) {
 			status = retval;
@@ -342,9 +342,10 @@ static void bridge_recover(struct work_struct *work)
 	if (atomic_read(&bridge_cref)) {
 		INIT_COMPLETION(bridge_comp);
 		while (!wait_for_completion_timeout(&bridge_comp,
-						msecs_to_jiffies(REC_TIMEOUT)))
-			pr_info("%s:%d handle(s) still opened\n",
-					__func__, atomic_read(&bridge_cref));
+						    msecs_to_jiffies
+						    (REC_TIMEOUT)))
+			pr_info("%s:%d handle(s) still opened\n", __func__,
+				atomic_read(&bridge_cref));
 	}
 	dev = dev_get_first();
 	dev_get_dev_node(dev, &dev_node);
@@ -363,10 +364,10 @@ void bridge_recover_schedule(void)
 #endif
 #ifdef CONFIG_TIDSPBRIDGE_DVFS
 static int dspbridge_scale_notification(struct notifier_block *op,
-		unsigned long val, void *ptr)
+					unsigned long val, void *ptr)
 {
 	struct omap_dsp_platform_data *pdata =
-		omap_dspbridge_dev->dev.platform_data;
+	    omap_dspbridge_dev->dev.platform_data;
 
 	if (CPUFREQ_POSTCHANGE == val && pdata->dsp_get_opp)
 		pwr_pm_post_scale(PRCM_VDD1, pdata->dsp_get_opp());
@@ -413,7 +414,7 @@ static int omap3_bridge_startup(struct platform_device *pdev)
 					CPUFREQ_TRANSITION_NOTIFIER);
 	if (err)
 		pr_err("%s: clk_notifier_register failed for iva2_ck\n",
-								__func__);
+		       __func__);
 #endif
 #endif
 
@@ -471,7 +472,7 @@ err2:
 err1:
 #ifdef CONFIG_TIDSPBRIDGE_DVFS
 	cpufreq_unregister_notifier(&iva_clk_notifier,
-					CPUFREQ_TRANSITION_NOTIFIER);
+				    CPUFREQ_TRANSITION_NOTIFIER);
 #endif
 	dsp_clk_exit();
 
@@ -547,10 +548,9 @@ static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev)
 		pr_err("%s: Failed to retrieve the object handle\n", __func__);
 		goto func_cont;
 	}
-
 #ifdef CONFIG_TIDSPBRIDGE_DVFS
 	if (cpufreq_unregister_notifier(&iva_clk_notifier,
-						CPUFREQ_TRANSITION_NOTIFIER))
+					CPUFREQ_TRANSITION_NOTIFIER))
 		pr_err("%s: cpufreq_unregister_notifier failed for iva2_ck\n",
 		       __func__);
 #endif /* #ifdef CONFIG_TIDSPBRIDGE_DVFS */
-- 
1.7.8.3

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Ramirez Luna, Omar]

2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
> No functional changes.
>
> According to Lindent, the file drv_internface.c had some lines with bad
> indentation.
>
> This commit is the output of Lindent.

Usually lindent tends to do whatever it wants, unless carefully configured...

...
> @@ -342,9 +342,10 @@ static void bridge_recover(struct work_struct *work)
>        if (atomic_read(&bridge_cref)) {
>                INIT_COMPLETION(bridge_comp);
>                while (!wait_for_completion_timeout(&bridge_comp,
> -                                               msecs_to_jiffies(REC_TIMEOUT)))
> -                       pr_info("%s:%d handle(s) still opened\n",
> -                                       __func__, atomic_read(&bridge_cref));
> +                                                   msecs_to_jiffies
> +                                                   (REC_TIMEOUT)))

Like here, it just split msecs_to_jiffies(REC_TIMEOUT) into 2 lines
making it a little harder to read.

> +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> +                               atomic_read(&bridge_cref));

I remember the rule was to break lines as far to the right as
possible, no? Chapter 2 CodingStyle, same for the other similar
changes.

...
> @@ -547,10 +548,9 @@ static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev)
>                pr_err("%s: Failed to retrieve the object handle\n", __func__);
>                goto func_cont;
>        }
> -

Blank line removed?

>  #ifdef CONFIG_TIDSPBRIDGE_DVFS
>        if (cpufreq_unregister_notifier(&iva_clk_notifier,
> -                                               CPUFREQ_TRANSITION_NOTIFIER))
> +                                       CPUFREQ_TRANSITION_NOTIFIER))
>                pr_err("%s: cpufreq_unregister_notifier failed for iva2_ck\n",
>                       __func__);
>  #endif /* #ifdef CONFIG_TIDSPBRIDGE_DVFS */
> --
> 1.7.8.3
>

Regards,

Omar

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Víctor M. Jáquez L.]

On Mon, Jan 30, 2012 at 11:25:34AM -0600, Ramirez Luna, Omar wrote:
> 2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
> > No functional changes.
> >
> > According to Lindent, the file drv_internface.c had some lines with bad
> > indentation.
> >
> > This commit is the output of Lindent.
> 
> Usually lindent tends to do whatever it wants, unless carefully
> configured...

Thanks Omar!

I'll post a v2 of this patch set, with a couple things more and with more
attention on what lindent does in this one, if that's ok.

vmjl

> 
> ...
> > @@ -342,9 +342,10 @@ static void bridge_recover(struct work_struct *work)
> >        if (atomic_read(&bridge_cref)) {
> >                INIT_COMPLETION(bridge_comp);
> >                while (!wait_for_completion_timeout(&bridge_comp,
> > -                                               msecs_to_jiffies(REC_TIMEOUT)))
> > -                       pr_info("%s:%d handle(s) still opened\n",
> > -                                       __func__, atomic_read(&bridge_cref));
> > +                                                   msecs_to_jiffies
> > +                                                   (REC_TIMEOUT)))
> 
> Like here, it just split msecs_to_jiffies(REC_TIMEOUT) into 2 lines
> making it a little harder to read.
> 
> > +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> > +                               atomic_read(&bridge_cref));
> 
> I remember the rule was to break lines as far to the right as
> possible, no? Chapter 2 CodingStyle, same for the other similar
> changes.
> 
> ...
> > @@ -547,10 +548,9 @@ static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev)
> >                pr_err("%s: Failed to retrieve the object handle\n", __func__);
> >                goto func_cont;
> >        }
> > -
> 
> Blank line removed?
> 
> >  #ifdef CONFIG_TIDSPBRIDGE_DVFS
> >        if (cpufreq_unregister_notifier(&iva_clk_notifier,
> > -                                               CPUFREQ_TRANSITION_NOTIFIER))
> > +                                       CPUFREQ_TRANSITION_NOTIFIER))
> >                pr_err("%s: cpufreq_unregister_notifier failed for iva2_ck\n",
> >                       __func__);
> >  #endif /* #ifdef CONFIG_TIDSPBRIDGE_DVFS */
> > --
> > 1.7.8.3
> >
> 
> Regards,
> 
> Omar
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On Mon, Jan 30, 2012 at 11:25:34AM -0600, Ramirez Luna, Omar wrote:
> > +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> > +                               atomic_read(&bridge_cref));
> 
> I remember the rule was to break lines as far to the right as
> possible, no? Chapter 2 CodingStyle, same for the other similar
> changes.
> 

It doesn't mean you have to right justify things, it just means
indented.  The original code is fine here and the new code is fine
here.  It's up to whoever writes the code to decide.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Joe Perches]

On Mon, 2012-01-30 at 22:29 +0300, Dan Carpenter wrote:
> On Mon, Jan 30, 2012 at 11:25:34AM -0600, Ramirez Luna, Omar wrote:
> > > +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> > > +                               atomic_read(&bridge_cref));
> > I remember the rule was to break lines as far to the right as
> > possible, no? Chapter 2 CodingStyle, same for the other similar
> > changes.
> It doesn't mean you have to right justify things, it just means
> indented.  The original code is fine here and the new code is fine
> here.  It's up to whoever writes the code to decide.

I concur.

My personal preference is to use a new line after the format
string if necessary.

ie:
	pr_<level>("fmt\n"[, args to 80 columns if all fit])
or
	pr_<level>("fmt\n",
		   args when single line exceeds 80 columns);

So for this case:
			pr_info("%s:%d handle(s) still opened\n",
				__func__, atomic_read(&bridge_cref));

I've done a patch here to tidspbridge that standardizes
printk output.

Basically, the patch adds
#define pr_fmt(fmt) KBUILD_MODNAME "%s: ", __func__
to prefix "tidspbridge:%s:", removes the leading
"%s:...",  __func__ from the uses, coalesces
formats and does argument alignment.

It cleans up the DBC_ASSERT, DBC_REQUIRE and DBC_ENSURE
macros too.

I'm waiting for the Makefile change and whatever
patches Víctor produces to be applied.  I'll then
redo my patch and submit it.

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Víctor M. Jáquez L.]

On Mon, Jan 30, 2012 at 11:53:00AM -0800, Joe Perches wrote:
> On Mon, 2012-01-30 at 22:29 +0300, Dan Carpenter wrote:
> > On Mon, Jan 30, 2012 at 11:25:34AM -0600, Ramirez Luna, Omar wrote:
> > > > +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> > > > +                               atomic_read(&bridge_cref));
> > > I remember the rule was to break lines as far to the right as
> > > possible, no? Chapter 2 CodingStyle, same for the other similar
> > > changes.
> > It doesn't mean you have to right justify things, it just means
> > indented.  The original code is fine here and the new code is fine
> > here.  It's up to whoever writes the code to decide.
> 
> I concur.
> 
> My personal preference is to use a new line after the format
> string if necessary.
> 
> ie:
> 	pr_<level>("fmt\n"[, args to 80 columns if all fit])
> or
> 	pr_<level>("fmt\n",
> 		   args when single line exceeds 80 columns);
> 
> So for this case:
> 			pr_info("%s:%d handle(s) still opened\n",
> 				__func__, atomic_read(&bridge_cref));
> 
> I've done a patch here to tidspbridge that standardizes
> printk output.
> 
> Basically, the patch adds
> #define pr_fmt(fmt) KBUILD_MODNAME "%s: ", __func__
> to prefix "tidspbridge:%s:", removes the leading
> "%s:...",  __func__ from the uses, coalesces
> formats and does argument alignment.
> 
> It cleans up the DBC_ASSERT, DBC_REQUIRE and DBC_ENSURE
> macros too.

hehehe... I also have one for this... But I prefer yours: I'm a newbie :)

vmjl

> 
> I'm waiting for the Makefile change and whatever
> patches Víctor produces to be applied.  I'll then
> redo my patch and submit it.
> 
> 
> 

Re: [PATCH 3/5] staging: tidspbridge: Lindent to drv_interface.c

[Joe Perches]

On Mon, 2012-01-30 at 21:33 +0100, Víctor M. Jáquez L. wrote:
> On Mon, Jan 30, 2012 at 11:53:00AM -0800, Joe Perches wrote:
> > I've done a patch here to tidspbridge that standardizes
> > printk output.
> > 
> > Basically, the patch adds
> > #define pr_fmt(fmt) KBUILD_MODNAME "%s: ", __func__
> > to prefix "tidspbridge:%s:", removes the leading
> > "%s:...",  __func__ from the uses, coalesces
> > formats and does argument alignment.
> > 
> > It cleans up the DBC_ASSERT, DBC_REQUIRE and DBC_ENSURE
> > macros too.
> 
> hehehe... I also have one for this... But I prefer yours: I'm a newbie :)

Well, given that you haven't done many of these, I prefer yours.

[PATCH 4/5] staging: tidspbridge: silence the compiler

[Víctor Manuel Jáquez Leal]

Silence the warning when compiling drv_interface.c

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index c7015f5..3bbe443 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -273,8 +273,9 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	dev_dbg(bridge, "%s: vm filp %p offset %x start %lx end %lx page_prot "
-		"%lx flags %lx\n", __func__, filp, offset,
-		vma->vm_start, vma->vm_end, vma->vm_page_prot, vma->vm_flags);
+		"%ulx flags %lx\n", __func__, filp, offset,
+		vma->vm_start, vma->vm_end, vma->vm_page_prot,
+		vma->vm_flags);
 
 	status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
 				 vma->vm_end - vma->vm_start,
-- 
1.7.8.3

Re: [PATCH 4/5] staging: tidspbridge: silence the compiler

[Ramirez Luna, Omar]

2012/1/23 Víctor Manuel Jáquez Leal <vjaquez@igalia.com>:
> Silence the warning when compiling drv_interface.c
>
> Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
> ---
>  drivers/staging/tidspbridge/rmgr/drv_interface.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
> index c7015f5..3bbe443 100644
> --- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
> +++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
> @@ -273,8 +273,9 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
>        vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>
>        dev_dbg(bridge, "%s: vm filp %p offset %x start %lx end %lx page_prot "
> -               "%lx flags %lx\n", __func__, filp, offset,
> -               vma->vm_start, vma->vm_end, vma->vm_page_prot, vma->vm_flags);
> +               "%ulx flags %lx\n", __func__, filp, offset,
> +               vma->vm_start, vma->vm_end, vma->vm_page_prot,
> +               vma->vm_flags);
>
>        status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>                                 vma->vm_end - vma->vm_start,
> --
> 1.7.8.3
>

Looks good.

Regards,

Omar

[PATCH 5/5] staging: tidspbridge: remove header inclusions

[Víctor Manuel Jáquez Leal]

drv_interface.c include several header files that are not really used.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 3bbe443..96cc2e9 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -16,11 +16,8 @@
  * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
  */
 
-/*  ----------------------------------- Host OS */
-
 #include <plat/dsp.h>
 
-#include <dspbridge/host_os.h>
 #include <linux/types.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -38,10 +35,8 @@
 
 /*  ----------------------------------- OS Adaptation Layer */
 #include <dspbridge/clk.h>
-#include <dspbridge/sync.h>
 
 /*  ----------------------------------- Platform Manager */
-#include <dspbridge/dspapi-ioctl.h>
 #include <dspbridge/dspapi.h>
 #include <dspbridge/dspdrv.h>
 
@@ -49,10 +44,8 @@
 #include <dspbridge/pwr.h>
 
 #include <dspbridge/resourcecleanup.h>
-#include <dspbridge/chnl.h>
 #include <dspbridge/proc.h>
 #include <dspbridge/dev.h>
-#include <dspbridge/drv.h>
 
 #ifdef CONFIG_TIDSPBRIDGE_DVFS
 #include <mach-omap2/omap3-opp.h>
-- 
1.7.8.3