Skip to contentSkip to conversation

staging: tidspbridge: clean up drv_interface.c

PATCH Superseded Víctor Manuel Jáquez Leal 5 participants 17 messages Jan 23, 2012 – Jan 30, 2012 Staging

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover staging: tidspbridge: clean up drv_interface.c

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
Signature · 1 line
1.7.8.3
#1 lore raw

PATCH 1/5 staging: tidspbridge: more readable code

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

---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
drivers/staging/tidspbridge/rmgr/drv_interface.c +6 -7
index 76cfc6e..85f6e8e 100644
@@ -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)
Signature · 1 line
1.7.8.3
#2 lore raw

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

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 2 lines

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

Looks good to me.

Signature · 1 line
Felipe Contreras
#3 lore raw

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

2012/1/24 Felipe Contreras <[email protected]>:

Quoted 5 lines

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 2 lines

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

Looks good to me.

FWIW, I agree.

Regards,

Omar

#4 lore raw

PATCH 2/5 staging: tidspbridge: remove unused header

In reply to 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.

---
 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
drivers/staging/tidspbridge/rmgr/drv_interface.c +155 -158
index 85f6e8e..5ea753c 100644
@@ -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)
drivers/staging/tidspbridge/rmgr/drv_interface.h deleted +0 -28
deleted file mode 100644
index ab07060..0000000
@@ -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_ */
Signature · 1 line
1.7.8.3
#5 lore raw

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

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 7 lines

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.

Signature · 1 line
Felipe Contreras
#6 lore raw

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

2012/1/24 Felipe Contreras <[email protected]>:

Quoted 8 lines

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 7 lines

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.

...

Quoted 2 lines

But looks good to me either way.

FWIW, I agree.

Regards,

Omar

#7 lore raw

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

In reply to 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.

---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
drivers/staging/tidspbridge/rmgr/drv_interface.c +11 -11
index 5ea753c..c7015f5 100644
@@ -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 */
Signature · 1 line
1.7.8.3
#8 lore raw

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

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 6 lines

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...

...

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 346

Quoted 9 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -3
@@ -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.

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 348

Quoted 2 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -0
+                       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.

...

drivers/staging/tidspbridge/rmgr/drv_interface.c static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev) · line 550

Quoted 5 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +0 -1
@@ -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?

Quoted 10 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +1 -2
  #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

#9 lore raw

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

Quoted 10 lines · Omar

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 6 lines

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

Quoted 50 lines

...

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 346

Quoted 9 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -3
@@ -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.

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 348

Quoted 2 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -0
+                       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.

...

drivers/staging/tidspbridge/rmgr/drv_interface.c static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev) · line 550

Quoted 5 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +0 -1
@@ -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?

Quoted 10 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +1 -2
  #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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

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

In reply to Ramirez Luna, Omar
Quoted 7 lines · Omar
Quoted 2 lines

+                       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

#11 lore raw

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

Quoted 9 lines · Dan Carpenter
Quoted 5 lines · Omar

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 348

Quoted 2 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -0
+                       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.

#12 lore raw

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

Quoted 29 lines · Joe Perches
Quoted 9 lines · Dan Carpenter
Quoted 5 lines · Omar

drivers/staging/tidspbridge/rmgr/drv_interface.c static void bridge_recover(struct work_struct *work) · line 348

Quoted 2 lines
drivers/staging/tidspbridge/rmgr/drv_interface.c +2 -0
+                       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.

Quoted 8 lines

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

Quoted 7 lines

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.

#13 lore raw

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

Quoted 10 lines · Víctor M. Jáquez L.
Quoted 9 lines · Joe Perches

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.

Quoted 4 lines
Quoted 2 lines

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.

#14 lore raw

PATCH 4/5 staging: tidspbridge: silence the compiler

In reply to Víctor Manuel Jáquez Leal

Silence the warning when compiling drv_interface.c

---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/staging/tidspbridge/rmgr/drv_interface.c +3 -2
index c7015f5..3bbe443 100644
@@ -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,
Signature · 1 line
1.7.8.3
#15 lore raw

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

2012/1/23 Víctor Manuel Jáquez Leal <[email protected]>:

Quoted 21 lines

Silence the warning when compiling drv_interface.c

---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/staging/tidspbridge/rmgr/drv_interface.c +0 -0
index c7015f5..3bbe443 100644
@@ -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);

Quoted 5 lines

       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

#16 lore raw

PATCH 5/5 staging: tidspbridge: remove header inclusions

In reply to Víctor Manuel Jáquez Leal

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

---
 drivers/staging/tidspbridge/rmgr/drv_interface.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
drivers/staging/tidspbridge/rmgr/drv_interface.c +0 -7
index 3bbe443..96cc2e9 100644
@@ -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>
Signature · 1 line
1.7.8.3