[PATCH 0/2] dspbridge: reorganize initialization and probe

[PATCH 0/2] dspbridge: reorganize initialization and probe

[Omar Ramirez Luna]

Split the functions to have cleaner error handling paths, this
will aslo cover a case where bridge initialization has failed but
device entry is still available which leads to unknown behavior.

Omar Ramirez Luna (2):
  DSPRBIDGE: split probe from bridge initializations
  DSPBRIDGE: reorganize probe function

 drivers/dsp/bridge/rmgr/drv_interface.c |  202 ++++++++++++++++++-------------
 1 files changed, 118 insertions(+), 84 deletions(-)

[PATCH 1/2] DSPRBIDGE: split probe from bridge initializations

[Omar Ramirez Luna]

A function containing driver low lever initializations has been
created. Now device entry should be available only if the startup
call has completed successfully, in case of error rollback any
of the changes that apply.

Now, device entry is created after bridge initializations
have been completed.

Reported-by: Ameya Palande <ameya.palande@nokia.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/dsp/bridge/rmgr/drv_interface.c |  178 ++++++++++++++++++-------------
 1 files changed, 105 insertions(+), 73 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index c29de2c..f80b15b 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -233,23 +233,125 @@ static struct notifier_block iva_clk_notifier = {
 };
 #endif
 
+/**
+ * omap3_bridge_startup() - perform low lever initializations
+ * @pdev:      pointer to platform device
+ *
+ * Initializes recovery, PM and DVFS required data, before calling
+ * clk and memory init routines.
+ */
+static int omap3_bridge_startup(struct platform_device *pdev)
+{
+	struct dspbridge_platform_data *pdata = pdev->dev.platform_data;
+	struct drv_data *drv_datap = NULL;
+	u32 phys_membase, phys_memsize;
+	int err;
+
+#ifdef CONFIG_BRIDGE_RECOVERY
+	bridge_rec_queue = create_workqueue("bridge_rec_queue");
+	INIT_WORK(&bridge_recovery_work, bridge_recover);
+	INIT_COMPLETION(bridge_comp);
+#endif
+
+#ifdef CONFIG_PM
+	/* Initialize the wait queue */
+	bridge_suspend_data.suspended = 0;
+	init_waitqueue_head(&bridge_suspend_data.suspend_wq);
+
+#ifdef CONFIG_BRIDGE_DVFS
+	for (i = 0; i < 6; i++)
+		pdata->mpu_speed[i] = vdd1_rate_table_bridge[i].rate;
+
+	err = cpufreq_register_notifier(&iva_clk_notifier,
+					CPUFREQ_TRANSITION_NOTIFIER);
+	if (err)
+		pr_err("%s: clk_notifier_register failed for iva2_ck\n",
+								__func__);
+#endif
+#endif
+
+	dsp_clk_init();
+	services_init();
+
+	drv_datap = kzalloc(sizeof(struct drv_data), GFP_KERNEL);
+	if (!drv_datap) {
+		err = -ENOMEM;
+		goto err1;
+	}
+
+	drv_datap->shm_size = shm_size;
+	drv_datap->tc_wordswapon = tc_wordswapon;
+
+	if (base_img) {
+		drv_datap->base_img = kmalloc(strlen(base_img) + 1, GFP_KERNEL);
+		if (!drv_datap->base_img) {
+			err = -ENOMEM;
+			goto err2;
+		}
+		strncpy(drv_datap->base_img, base_img, strlen(base_img) + 1);
+	}
+
+	dev_set_drvdata(bridge, drv_datap);
+
+	if (shm_size < 0x10000) {	/* 64 KB */
+		err = -EINVAL;
+		pr_err("%s: shm size must be at least 64 KB\n", __func__);
+		goto err3;
+	}
+	dev_dbg(bridge, "%s: requested shm_size = 0x%x\n", __func__, shm_size);
+
+	phys_membase = pdata->phys_mempool_base;
+	phys_memsize = pdata->phys_mempool_size;
+	if (phys_membase > 0 && phys_memsize > 0)
+		mem_ext_phys_pool_init(phys_membase, phys_memsize);
+
+	if (tc_wordswapon)
+		dev_dbg(bridge, "%s: TC Word Swap is enabled\n", __func__);
+
+	driver_context = dsp_init(&err);
+	if (err) {
+		pr_err("DSP Bridge driver initialization failed\n");
+		goto err4;
+	}
+
+	return 0;
+
+err4:
+	mem_ext_phys_pool_release();
+err3:
+	kfree(drv_datap->base_img);
+err2:
+	kfree(drv_datap);
+err1:
+#ifdef CONFIG_BRIDGE_DVFS
+	cpufreq_unregister_notifier(&iva_clk_notifier,
+					CPUFREQ_TRANSITION_NOTIFIER);
+#endif
+	dsp_clk_exit();
+	services_exit();
+
+	return err;
+}
+
 static int __devinit omap34_xx_bridge_probe(struct platform_device *pdev)
 {
 	int status;
-	u32 init_status = 0;
 	dev_t dev = 0;
 	int result;
 #ifdef CONFIG_BRIDGE_DVFS
 	int i = 0;
 #endif
-	struct dspbridge_platform_data *pdata = pdev->dev.platform_data;
-	struct drv_data *drv_datap = NULL;
 
 	omap_dspbridge_dev = pdev;
 
 	/* Global bridge device */
 	bridge = &omap_dspbridge_dev->dev;
 
+	/* Bridge low level initializations */
+	status = omap3_bridge_startup(pdev);
+	if (status)
+		goto err1;
+
 	/* use 2.6 device model */
 	result = alloc_chrdev_region(&dev, 0, 1, driver_name);
 	if (result < 0) {
@@ -277,76 +379,6 @@ static int __devinit omap34_xx_bridge_probe(struct platform_device *pdev)
 	device_create(bridge_class, NULL, MKDEV(driver_major, 0),
 		      NULL, "DspBridge");
 
-#ifdef CONFIG_PM
-	/* Initialize the wait queue */
-	if (!status) {
-		bridge_suspend_data.suspended = 0;
-		init_waitqueue_head(&bridge_suspend_data.suspend_wq);
-	}
-#endif
-
-	dsp_clk_init();
-	services_init();
-
-	/*  Autostart flag.  This should be set to true if the DSP image should
-	 *  be loaded and run during bridge module initialization */
-	drv_datap = kzalloc(sizeof(struct drv_data), GFP_KERNEL);
-	if (drv_datap) {
-		drv_datap->shm_size = shm_size;
-		drv_datap->tc_wordswapon = tc_wordswapon;
-		if (base_img) {
-			drv_datap->base_img = kmalloc(strlen(base_img) + 1,
-								GFP_KERNEL);
-			if (drv_datap->base_img)
-				strncpy(drv_datap->base_img, base_img,
-							strlen(base_img) + 1);
-			else
-				status = -ENOMEM;
-		}
-	} else {
-		init_status = -ENOMEM;
-	}
-	if (shm_size < 0x10000) {	/* 64 KB */
-		init_status = -EINVAL;
-		status = -1;
-		pr_err("%s: shm size must be at least 64 KB\n", __func__);
-	}
-	dev_dbg(bridge, "%s: requested shm_size = 0x%x\n", __func__, shm_size);
-
-	if ((pdata->phys_mempool_base > 0) && (pdata->phys_mempool_size > 0))
-		mem_ext_phys_pool_init(pdata->phys_mempool_base,
-						pdata->phys_mempool_size);
-	if (tc_wordswapon)
-		dev_dbg(bridge, "%s: TC Word Swap is enabled\n", __func__);
-
-	if (DSP_SUCCEEDED(init_status)) {
-#ifdef CONFIG_BRIDGE_DVFS
-		for (i = 0; i < 6; i++)
-			pdata->mpu_speed[i] = vdd1_rate_table_bridge[i].rate;
-
-		if (!cpufreq_register_notifier(&iva_clk_notifier,
-						CPUFREQ_TRANSITION_NOTIFIER))
-			pr_err("%s: clk_notifier_register failed for iva2_ck\n",
-			       __func__);
-#endif
-		dev_set_drvdata(bridge, drv_datap);
-		driver_context = dsp_init(&init_status);
-		if (DSP_FAILED(init_status)) {
-			status = -1;
-			pr_err("DSP Bridge driver initialization failed\n");
-		} else {
-			pr_info("DSP Bridge driver loaded\n");
-		}
-	}
-
-#ifdef CONFIG_BRIDGE_RECOVERY
-	bridge_rec_queue = create_workqueue("bridge_rec_queue");
-	INIT_WORK(&bridge_recovery_work, bridge_recover);
-	INIT_COMPLETION(bridge_comp);
-#endif
-	DBC_ASSERT(status == 0);
-	DBC_ASSERT(DSP_SUCCEEDED(init_status));
-
 	return 0;
 
 err2:
-- 
1.7.1

[PATCH 2/2] DSPBRIDGE: reorganize probe function

[Omar Ramirez Luna]

Renaming and deleting unused variables, few enhancement on
error path.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/dsp/bridge/rmgr/drv_interface.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index f80b15b..16a74c4 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -335,9 +335,8 @@ err1:
 
 static int __devinit omap34_xx_bridge_probe(struct platform_device *pdev)
 {
-	int status;
+	int err;
 	dev_t dev = 0;
-	int result;
 #ifdef CONFIG_BRIDGE_DVFS
 	int i = 0;
 #endif
@@ -348,43 +347,46 @@ static int __devinit omap34_xx_bridge_probe(struct platform_device *pdev)
 	bridge = &omap_dspbridge_dev->dev;
 
 	/* Bridge low level initializations */
-	status = omap3_bridge_startup(pdev);
-	if (status)
+	err = omap3_bridge_startup(pdev);
+	if (err)
 		goto err1;
 
 	/* use 2.6 device model */
-	result = alloc_chrdev_region(&dev, 0, 1, driver_name);
-	if (result < 0) {
+	err = alloc_chrdev_region(&dev, 0, 1, driver_name);
+	if (err) {
 		pr_err("%s: Can't get major %d\n", __func__, driver_major);
 		goto err1;
 	}
 
-	driver_major = MAJOR(dev);
-
 	cdev_init(&bridge_cdev, &bridge_fops);
 	bridge_cdev.owner = THIS_MODULE;
 
-	status = cdev_add(&bridge_cdev, dev, 1);
-	if (status) {
+	err = cdev_add(&bridge_cdev, dev, 1);
+	if (err) {
 		pr_err("%s: Failed to add bridge device\n", __func__);
 		goto err2;
 	}
 
 	/* udev support */
 	bridge_class = class_create(THIS_MODULE, "ti_bridge");
-
-	if (IS_ERR(bridge_class))
+	if (IS_ERR(bridge_class)) {
 		pr_err("%s: Error creating bridge class\n", __func__);
+		goto err3;
+	}
 
+	driver_major = MAJOR(dev);
 	device_create(bridge_class, NULL, MKDEV(driver_major, 0),
 		      NULL, "DspBridge");
+	pr_info("DSP Bridge driver loaded\n");
 
 	return 0;
 
+err3:
+	cdev_del(&bridge_cdev);
 err2:
 	unregister_chrdev_region(dev, 1);
 err1:
-	return result;
+	return err;
 }
 
 static int __devexit omap34_xx_bridge_remove(struct platform_device *pdev)
-- 
1.7.1

Re: [PATCH 0/2] dspbridge: reorganize initialization and probe

[Felipe Contreras]

On Wed, Jun 9, 2010 at 3:48 AM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> Split the functions to have cleaner error handling paths, this
> will aslo cover a case where bridge initialization has failed but
> device entry is still available which leads to unknown behavior.

Great, I was wondering why this was not the case.

Now that you are on this, do you know if there's something missing in
order to automatically load the module when somebody tries to open
/dev/DspBridge?

-- 
Felipe Contreras

Re: [PATCH 0/2] dspbridge: reorganize initialization and probe

[Ameya Palande]

On Wed, 2010-06-09 at 09:25 +0200, ext Felipe Contreras wrote:
> On Wed, Jun 9, 2010 at 3:48 AM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> > Split the functions to have cleaner error handling paths, this
> > will aslo cover a case where bridge initialization has failed but
> > device entry is still available which leads to unknown behavior.
> 
> Great, I was wondering why this was not the case.
> 
> Now that you are on this, do you know if there's something missing in
> order to automatically load the module when somebody tries to open
> /dev/DspBridge?

I thought /dev/DspBridge will only appear if you load the module ;)

Cheers,
Ameya.

Re: [PATCH 0/2] dspbridge: reorganize initialization and probe

[Felipe Contreras]

On Wed, Jun 9, 2010 at 11:49 AM, Ameya Palande <ameya.palande@nokia.com> wrote:
> On Wed, 2010-06-09 at 09:25 +0200, ext Felipe Contreras wrote:
>> On Wed, Jun 9, 2010 at 3:48 AM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
>> > Split the functions to have cleaner error handling paths, this
>> > will aslo cover a case where bridge initialization has failed but
>> > device entry is still available which leads to unknown behavior.
>>
>> Great, I was wondering why this was not the case.
>>
>> Now that you are on this, do you know if there's something missing in
>> order to automatically load the module when somebody tries to open
>> /dev/DspBridge?
>
> I thought /dev/DspBridge will only appear if you load the module ;)

Kind of...
$ sudo touch /dev/DspBridge
See? I have dspbridge on my laptop ;)

I think it's possible to have udev rules that create the file, and
automatically load the module when accessed, but I don't know much
about udev.

-- 
Felipe Contreras

RE: [PATCH 0/2] dspbridge: reorganize initialization and probe

[Ramirez Luna, Omar]

>From: Ramirez Luna, Omar
>
>Split the functions to have cleaner error handling paths, this
>will aslo cover a case where bridge initialization has failed but
>device entry is still available which leads to unknown behavior.
>
>Omar Ramirez Luna (2):
>  DSPRBIDGE: split probe from bridge initializations
>  DSPBRIDGE: reorganize probe function
>
> drivers/dsp/bridge/rmgr/drv_interface.c |  202 ++++++++++++++++++-------------
> 1 files changed, 118 insertions(+), 84 deletions(-)

Pushed to dspbridge.

- omar