Patchwork [3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm

login
register
mail settings
Submitter Bryan Wu
Date July 1, 2010, 5:02 a.m.
Message ID <1277960546-32336-4-git-send-email-bryan.wu@canonical.com>
Download mbox | patch
Permalink /patch/57470/
State Rejected
Headers show

Comments

Bryan Wu - July 1, 2010, 5:02 a.m.
From: Miguel Vadillo <vadillo@ti.com>

Changes to fix circular dependencies when compiling ipu_pm in
modules mode.

Signed-off-by: Miguel Vadillo <vadillo@ti.com>
---
 arch/arm/plat-omap/clock.c                   |    1 +
 drivers/dsp/syslink/ipu_pm/ipu_pm.c          |   60 ++++++++++++++++++++++++--
 drivers/dsp/syslink/ipu_pm/ipu_pm.h          |   10 +++--
 drivers/dsp/syslink/multicore_ipc/platform.c |   31 ++------------
 4 files changed, 67 insertions(+), 35 deletions(-)
 mode change 100644 => 100755 arch/arm/plat-omap/clock.c
 mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
 mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
 mode change 100644 => 100755 drivers/dsp/syslink/multicore_ipc/platform.c
Bryan Wu - July 1, 2010, 5:14 a.m.
Tim,

This patch fix the module building failure issue which is similar to the patches
from you. And it wrapped those pointers with access helper functions, this solve
the loop dependencies issue.

I think it is OK for me.

Acked-by: Bryan Wu <bryan.wu@canonical.com>

On 07/01/2010 01:02 PM, Bryan Wu wrote:
> From: Miguel Vadillo <vadillo@ti.com>
> 
> Changes to fix circular dependencies when compiling ipu_pm in
> modules mode.
> 
> Signed-off-by: Miguel Vadillo <vadillo@ti.com>
> ---
>  arch/arm/plat-omap/clock.c                   |    1 +
>  drivers/dsp/syslink/ipu_pm/ipu_pm.c          |   60 ++++++++++++++++++++++++--
>  drivers/dsp/syslink/ipu_pm/ipu_pm.h          |   10 +++--
>  drivers/dsp/syslink/multicore_ipc/platform.c |   31 ++------------
>  4 files changed, 67 insertions(+), 35 deletions(-)
>  mode change 100644 => 100755 arch/arm/plat-omap/clock.c
>  mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
>  mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
>  mode change 100644 => 100755 drivers/dsp/syslink/multicore_ipc/platform.c
> 
> diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
> old mode 100644
> new mode 100755
> index 701e4ea..104eaec
> --- a/arch/arm/plat-omap/clock.c
> +++ b/arch/arm/plat-omap/clock.c
> @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(omap_clk_get_by_name);
>  
>  /*
>   * Low level helpers
> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> old mode 100644
> new mode 100755
> index 3806f5a..f27512f
> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = {
>  
>  struct omap_dm_timer *p_gpt;
>  struct clk *p_i2c_clk;
> +struct sms *rcb_table;
> +void *ipu_pm_notifydrv_handle;
> +struct pm_event *pm_event;
>  
>  /** ============================================================================
>   *  Forward declarations of internal functions
> @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId,
>  
>  	/* send the ACK to DUCATI*/
>  	return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>  				SYS_M3,/*DUCATI_PROC*/
>  				PM_RESOURCE,/*PWR_MGMT_EVENT*/
>  				payload,
> @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>  		pm_msg.fields.parm = PM_SUCCESS;
>  		/* send the ACK to DUCATI*/
>  		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>  				SYS_M3,/*DUCATI_PROC*/
>  				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>  				(unsigned int)pm_msg.whole,
> @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>  		pm_msg.fields.parm = PM_SUCCESS;
>  		/* send the ACK to DUCATI*/
>  		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>  				SYS_M3,/*DUCATI_PROC*/
>  				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>  				(unsigned int)pm_msg.whole,
> @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>  		pm_msg.fields.parm = PM_SUCCESS;
>  		/* send the ACK to DUCATI*/
>  		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>  				SYS_M3,/*DUCATI_PROC*/
>  				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>  				(unsigned int)pm_msg.whole,
> @@ -344,6 +347,53 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>  EXPORT_SYMBOL(ipu_pm_notifications);
>  
>  /*
> +  Function for setup ipu_pm module
> + *
> + */
> +int ipu_pm_setup(void *notify_driver_handle)
> +{
> +	u32 i = 0;
> +	ipu_pm_notifydrv_handle = notify_driver_handle;
> +	/* Get the shared RCB */
> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> +		sizeof(struct sms));
> +
> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
> +				GFP_KERNEL);
> +
> +	/* Each event has it own sem */
> +	for (i = 0; i < NUMBER_PM_EVENTS; i++) {
> +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
> +						GFP_KERNEL);
> +		sema_init(pm_event[i].sem_handle, 0);
> +		pm_event[i].event_type = i;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_pm_setup);
> +
> +/*
> +  Function for finish ipu_pm module
> + *
> + */
> +int ipu_pm_finish()
> +{
> +	u32 i = 0;
> +	/* Release the shared RCB */
> +	for (i = 0; i < NUMBER_PM_EVENTS; i++) {
> +		kfree(pm_event[i].sem_handle);
> +		pm_event[i].event_type = 0;
> +	}
> +	kfree(pm_event);
> +	pm_event = NULL;
> +	iounmap(rcb_table);
> +	rcb_table = NULL;
> +	ipu_pm_notifydrv_handle = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_pm_finish);
> +
> +/*
>    Function for get sdma channels from PRCM
>   *
>   */
> @@ -518,3 +568,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
>  	pm_i2c_bus_counter--;
>  }
>  
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> old mode 100644
> new mode 100755
> index e9106fb..4481c6d
> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> @@ -185,10 +185,6 @@ struct rcb_block {
>  
>  };
>  
> -extern struct sms *rcb_table;
> -extern void *platform_notifydrv_handle;
> -extern struct pm_event *pm_event;
> -
>  struct sms {
>  	unsigned rat;
>  	struct rcb_block rcb[RCB_MAX];
> @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId,
>  /* Function for send PM Notifications */
>  int ipu_pm_notifications(enum pm_event_type event_type);
>  
> +/* Function for setup ipu_pm module */
> +int ipu_pm_setup(void *notify_driver_handle);
> +
> +/* Function for finish ipu_pm module */
> +int ipu_pm_finish(void);
> +
>  #endif
>  
> diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c b/drivers/dsp/syslink/multicore_ipc/platform.c
> old mode 100644
> new mode 100755
> index d1c62e7..1cdd686
> --- a/drivers/dsp/syslink/multicore_ipc/platform.c
> +++ b/drivers/dsp/syslink/multicore_ipc/platform.c
> @@ -233,9 +233,6 @@
>   */
>  void *platform_notifydrv_handle;
>  
> -struct pm_event *pm_event;
> -struct sms *rcb_table;
> -
>  /* Handles for SysM3 */
>  void *platform_nsrn_gate_handle_sysm3;
>  void *platform_nsrn_handle_sysm3;
> @@ -982,22 +979,8 @@ void platform_start_callback(void *arg)
>  				goto pm_register_fail;
>  			}
>  
> -			/* Get the shared RCB */
> -			rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> -				sizeof(struct sms));
> -
> -			pm_event =
> -				kzalloc(sizeof(struct pm_event)
> -				* NUMBER_PM_EVENTS, GFP_KERNEL);
> -
> -			/* Each event has it own sem */
> -			for (i = 0; i < NUMBER_PM_EVENTS; i++) {
> -				pm_event[i].sem_handle =
> -					kzalloc(sizeof(struct semaphore),
> -						GFP_KERNEL);
> -				sema_init(pm_event[i].sem_handle, 0);
> -				pm_event[i].event_type = i;
> -			}
> +			ipu_pm_setup(platform_notifydrv_handle);
> +
>  		}
>  		/* END PM */
>  
> @@ -1334,7 +1317,6 @@ void platform_stop_callback(void *arg)
>  	u16 proc_id = (u32) arg;
>  	int index = 0;
>  	u32 nread = 0;
> -	u32 i = 0;
>  
>  	if (proc_id == multiproc_get_id("SysM3"))
>  		index = SMHEAP_SRINDEX_SYSM3;
> @@ -1460,13 +1442,8 @@ void platform_stop_callback(void *arg)
>  				(void *)NULL);
>  			if (status < 0)
>  				printk(KERN_INFO "ERROR UNREGISTERING PM EVENT\n");
> -			for (i = 0; i < NUMBER_PM_EVENTS; i++) {
> -				kfree(pm_event[i].sem_handle);
> -				pm_event[i].event_type = 0;
> -			}
> -			kfree(pm_event);
> -			/* Release the shared RCB */
> -			iounmap(rcb_table);
> +
> +			ipu_pm_finish();
>  		}
>  		/* END PM */
>
Tim Gardner - July 1, 2010, 2:47 p.m.
On 06/30/2010 11:02 PM, Bryan Wu wrote:
> From: Miguel Vadillo<vadillo@ti.com>
>
> Changes to fix circular dependencies when compiling ipu_pm in
> modules mode.
>
> Signed-off-by: Miguel Vadillo<vadillo@ti.com>
> ---
>   arch/arm/plat-omap/clock.c                   |    1 +
>   drivers/dsp/syslink/ipu_pm/ipu_pm.c          |   60 ++++++++++++++++++++++++--
>   drivers/dsp/syslink/ipu_pm/ipu_pm.h          |   10 +++--
>   drivers/dsp/syslink/multicore_ipc/platform.c |   31 ++------------
>   4 files changed, 67 insertions(+), 35 deletions(-)
>   mode change 100644 =>  100755 arch/arm/plat-omap/clock.c
>   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
>   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
>   mode change 100644 =>  100755 drivers/dsp/syslink/multicore_ipc/platform.c
>
> diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
> old mode 100644
> new mode 100755
> index 701e4ea..104eaec
> --- a/arch/arm/plat-omap/clock.c
> +++ b/arch/arm/plat-omap/clock.c
> @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name)
>
>   	return ret;
>   }
> +EXPORT_SYMBOL(omap_clk_get_by_name);
>
>   /*
>    * Low level helpers
> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> old mode 100644
> new mode 100755
> index 3806f5a..f27512f
> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = {
>
>   struct omap_dm_timer *p_gpt;
>   struct clk *p_i2c_clk;
> +struct sms *rcb_table;
> +void *ipu_pm_notifydrv_handle;
> +struct pm_event *pm_event;
>

Is there any reason these aren't static? Their scope appears to be 
purely local to this file.

>   /** ============================================================================
>    *  Forward declarations of internal functions
> @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId,
>
>   	/* send the ACK to DUCATI*/
>   	return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>   				SYS_M3,/*DUCATI_PROC*/
>   				PM_RESOURCE,/*PWR_MGMT_EVENT*/
>   				payload,
> @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>   		pm_msg.fields.parm = PM_SUCCESS;
>   		/* send the ACK to DUCATI*/
>   		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>   				SYS_M3,/*DUCATI_PROC*/
>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>   				(unsigned int)pm_msg.whole,
> @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>   		pm_msg.fields.parm = PM_SUCCESS;
>   		/* send the ACK to DUCATI*/
>   		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>   				SYS_M3,/*DUCATI_PROC*/
>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>   				(unsigned int)pm_msg.whole,
> @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>   		pm_msg.fields.parm = PM_SUCCESS;
>   		/* send the ACK to DUCATI*/
>   		return_val = notify_sendevent(
> -				platform_notifydrv_handle,
> +				ipu_pm_notifydrv_handle,
>   				SYS_M3,/*DUCATI_PROC*/
>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>   				(unsigned int)pm_msg.whole,
> @@ -344,6 +347,53 @@ int ipu_pm_notifications(enum pm_event_type event_type)
>   EXPORT_SYMBOL(ipu_pm_notifications);
>
>   /*
> +  Function for setup ipu_pm module
> + *
> + */
> +int ipu_pm_setup(void *notify_driver_handle)
> +{
> +	u32 i = 0;
> +	ipu_pm_notifydrv_handle = notify_driver_handle;
> +	/* Get the shared RCB */
> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> +		sizeof(struct sms));
> +
> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
> +				GFP_KERNEL);
> +
> +	/* Each event has it own sem */
> +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
> +						GFP_KERNEL);
> +		sema_init(pm_event[i].sem_handle, 0);
> +		pm_event[i].event_type = i;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_pm_setup);
> +

Why bother returning a status code if its just a constant? How about 
checking for failures when calling ioremap() and kzalloc() ?

> +/*
> +  Function for finish ipu_pm module
> + *
> + */
> +int ipu_pm_finish()
> +{
> +	u32 i = 0;
> +	/* Release the shared RCB */
> +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> +		kfree(pm_event[i].sem_handle);
> +		pm_event[i].event_type = 0;
> +	}
> +	kfree(pm_event);
> +	pm_event = NULL;
> +	iounmap(rcb_table);
> +	rcb_table = NULL;
> +	ipu_pm_notifydrv_handle = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_pm_finish);
> +
> +/*
>     Function for get sdma channels from PRCM
>    *
>    */
> @@ -518,3 +568,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
>   	pm_i2c_bus_counter--;
>   }
>
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> old mode 100644
> new mode 100755
> index e9106fb..4481c6d
> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> @@ -185,10 +185,6 @@ struct rcb_block {
>
>   };
>
> -extern struct sms *rcb_table;
> -extern void *platform_notifydrv_handle;
> -extern struct pm_event *pm_event;
> -
>   struct sms {
>   	unsigned rat;
>   	struct rcb_block rcb[RCB_MAX];
> @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId,
>   /* Function for send PM Notifications */
>   int ipu_pm_notifications(enum pm_event_type event_type);
>
> +/* Function for setup ipu_pm module */
> +int ipu_pm_setup(void *notify_driver_handle);
> +
> +/* Function for finish ipu_pm module */
> +int ipu_pm_finish(void);
> +
>   #endif
>
> diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c b/drivers/dsp/syslink/multicore_ipc/platform.c
> old mode 100644
> new mode 100755
> index d1c62e7..1cdd686
> --- a/drivers/dsp/syslink/multicore_ipc/platform.c
> +++ b/drivers/dsp/syslink/multicore_ipc/platform.c
> @@ -233,9 +233,6 @@
>    */
>   void *platform_notifydrv_handle;
>
> -struct pm_event *pm_event;
> -struct sms *rcb_table;
> -
>   /* Handles for SysM3 */
>   void *platform_nsrn_gate_handle_sysm3;
>   void *platform_nsrn_handle_sysm3;
> @@ -982,22 +979,8 @@ void platform_start_callback(void *arg)
>   				goto pm_register_fail;
>   			}
>
> -			/* Get the shared RCB */
> -			rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> -				sizeof(struct sms));
> -
> -			pm_event =
> -				kzalloc(sizeof(struct pm_event)
> -				* NUMBER_PM_EVENTS, GFP_KERNEL);
> -
> -			/* Each event has it own sem */
> -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> -				pm_event[i].sem_handle =
> -					kzalloc(sizeof(struct semaphore),
> -						GFP_KERNEL);
> -				sema_init(pm_event[i].sem_handle, 0);
> -				pm_event[i].event_type = i;
> -			}
> +			ipu_pm_setup(platform_notifydrv_handle);
> +
>   		}
>   		/* END PM */
>
> @@ -1334,7 +1317,6 @@ void platform_stop_callback(void *arg)
>   	u16 proc_id = (u32) arg;
>   	int index = 0;
>   	u32 nread = 0;
> -	u32 i = 0;
>
>   	if (proc_id == multiproc_get_id("SysM3"))
>   		index = SMHEAP_SRINDEX_SYSM3;
> @@ -1460,13 +1442,8 @@ void platform_stop_callback(void *arg)
>   				(void *)NULL);
>   			if (status<  0)
>   				printk(KERN_INFO "ERROR UNREGISTERING PM EVENT\n");
> -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> -				kfree(pm_event[i].sem_handle);
> -				pm_event[i].event_type = 0;
> -			}
> -			kfree(pm_event);
> -			/* Release the shared RCB */
> -			iounmap(rcb_table);
> +
> +			ipu_pm_finish();
>   		}
>   		/* END PM */
>

I realize this is outside the scope of this patch, but why are the 
semaphore events handles (pm_event[i].sem_handle) allocated? In other 
words, why isn't the structure definition:

struct pm_event {
	enum pm_event_type event_type;
	struct semaphore sem;
};

Then the initialization wouldn't have to allocate memory and could just 
call sema_init(&pm_event[i].sem, 0), which makes init and de-init a bit 
simpler.

rtg
Vadillo, Miguel - July 1, 2010, 3:53 p.m.
Answers below.

> -----Original Message-----
> From: Tim Gardner [mailto:tim.gardner@canonical.com]
> Sent: Thursday, July 01, 2010 9:47 AM
> To: Vadillo, Miguel
> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan,
> Sebastien
> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
> ipu_pm
> 
> On 06/30/2010 11:02 PM, Bryan Wu wrote:
> > From: Miguel Vadillo<vadillo@ti.com>
> >
> > Changes to fix circular dependencies when compiling ipu_pm in
> > modules mode.
> >
> > Signed-off-by: Miguel Vadillo<vadillo@ti.com>
> > ---
> >   arch/arm/plat-omap/clock.c                   |    1 +
> >   drivers/dsp/syslink/ipu_pm/ipu_pm.c          |   60
> ++++++++++++++++++++++++--
> >   drivers/dsp/syslink/ipu_pm/ipu_pm.h          |   10 +++--
> >   drivers/dsp/syslink/multicore_ipc/platform.c |   31 ++------------
> >   4 files changed, 67 insertions(+), 35 deletions(-)
> >   mode change 100644 =>  100755 arch/arm/plat-omap/clock.c
> >   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
> >   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
> >   mode change 100644 =>  100755
> drivers/dsp/syslink/multicore_ipc/platform.c
> >
> > diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
> > old mode 100644
> > new mode 100755
> > index 701e4ea..104eaec
> > --- a/arch/arm/plat-omap/clock.c
> > +++ b/arch/arm/plat-omap/clock.c
> > @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name)
> >
> >   	return ret;
> >   }
> > +EXPORT_SYMBOL(omap_clk_get_by_name);
> >
> >   /*
> >    * Low level helpers
> > diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> > old mode 100644
> > new mode 100755
> > index 3806f5a..f27512f
> > --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> > +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
> > @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = {
> >
> >   struct omap_dm_timer *p_gpt;
> >   struct clk *p_i2c_clk;
> > +struct sms *rcb_table;
> > +void *ipu_pm_notifydrv_handle;
> > +struct pm_event *pm_event;
> >
> 
> Is there any reason these aren't static? Their scope appears to be
> purely local to this file.


In the next release we are getting rid of the ipu_pm_notifydrv_handle and we are just providing a handle to access both rcb_table and pm_event.

> 
> >   /**
> ==========================================================================
> ==
> >    *  Forward declarations of internal functions
> > @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId,
> >
> >   	/* send the ACK to DUCATI*/
> >   	return_val = notify_sendevent(
> > -				platform_notifydrv_handle,
> > +				ipu_pm_notifydrv_handle,
> >   				SYS_M3,/*DUCATI_PROC*/
> >   				PM_RESOURCE,/*PWR_MGMT_EVENT*/
> >   				payload,
> > @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type
> event_type)
> >   		pm_msg.fields.parm = PM_SUCCESS;
> >   		/* send the ACK to DUCATI*/
> >   		return_val = notify_sendevent(
> > -				platform_notifydrv_handle,
> > +				ipu_pm_notifydrv_handle,
> >   				SYS_M3,/*DUCATI_PROC*/
> >   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
> >   				(unsigned int)pm_msg.whole,
> > @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type
> event_type)
> >   		pm_msg.fields.parm = PM_SUCCESS;
> >   		/* send the ACK to DUCATI*/
> >   		return_val = notify_sendevent(
> > -				platform_notifydrv_handle,
> > +				ipu_pm_notifydrv_handle,
> >   				SYS_M3,/*DUCATI_PROC*/
> >   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
> >   				(unsigned int)pm_msg.whole,
> > @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type
> event_type)
> >   		pm_msg.fields.parm = PM_SUCCESS;
> >   		/* send the ACK to DUCATI*/
> >   		return_val = notify_sendevent(
> > -				platform_notifydrv_handle,
> > +				ipu_pm_notifydrv_handle,
> >   				SYS_M3,/*DUCATI_PROC*/
> >   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
> >   				(unsigned int)pm_msg.whole,
> > @@ -344,6 +347,53 @@ int ipu_pm_notifications(enum pm_event_type
> event_type)
> >   EXPORT_SYMBOL(ipu_pm_notifications);
> >
> >   /*
> > +  Function for setup ipu_pm module
> > + *
> > + */
> > +int ipu_pm_setup(void *notify_driver_handle)
> > +{
> > +	u32 i = 0;
> > +	ipu_pm_notifydrv_handle = notify_driver_handle;
> > +	/* Get the shared RCB */
> > +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> > +		sizeof(struct sms));
> > +
> > +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
> > +				GFP_KERNEL);
> > +
> > +	/* Each event has it own sem */
> > +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> > +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
> > +						GFP_KERNEL);
> > +		sema_init(pm_event[i].sem_handle, 0);
> > +		pm_event[i].event_type = i;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ipu_pm_setup);
> > +
> 
> Why bother returning a status code if its just a constant? How about
> checking for failures when calling ioremap() and kzalloc() ?

This part is now fixed and will be available for the next release too. We are checking the kzalloc. Also the ioremap is not done anymore since we are requesting a heap from the share memory.
> 
> > +/*
> > +  Function for finish ipu_pm module
> > + *
> > + */
> > +int ipu_pm_finish()
> > +{
> > +	u32 i = 0;
> > +	/* Release the shared RCB */
> > +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> > +		kfree(pm_event[i].sem_handle);
> > +		pm_event[i].event_type = 0;
> > +	}
> > +	kfree(pm_event);
> > +	pm_event = NULL;
> > +	iounmap(rcb_table);
> > +	rcb_table = NULL;
> > +	ipu_pm_notifydrv_handle = NULL;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ipu_pm_finish);
> > +
> > +/*
> >     Function for get sdma channels from PRCM
> >    *
> >    */
> > @@ -518,3 +568,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
> >   	pm_i2c_bus_counter--;
> >   }
> >
> > +MODULE_LICENSE("GPL");
> > +
> > diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> > old mode 100644
> > new mode 100755
> > index e9106fb..4481c6d
> > --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> > +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
> > @@ -185,10 +185,6 @@ struct rcb_block {
> >
> >   };
> >
> > -extern struct sms *rcb_table;
> > -extern void *platform_notifydrv_handle;
> > -extern struct pm_event *pm_event;
> > -
> >   struct sms {
> >   	unsigned rat;
> >   	struct rcb_block rcb[RCB_MAX];
> > @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId,
> >   /* Function for send PM Notifications */
> >   int ipu_pm_notifications(enum pm_event_type event_type);
> >
> > +/* Function for setup ipu_pm module */
> > +int ipu_pm_setup(void *notify_driver_handle);
> > +
> > +/* Function for finish ipu_pm module */
> > +int ipu_pm_finish(void);
> > +
> >   #endif
> >
> > diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c
> b/drivers/dsp/syslink/multicore_ipc/platform.c
> > old mode 100644
> > new mode 100755
> > index d1c62e7..1cdd686
> > --- a/drivers/dsp/syslink/multicore_ipc/platform.c
> > +++ b/drivers/dsp/syslink/multicore_ipc/platform.c
> > @@ -233,9 +233,6 @@
> >    */
> >   void *platform_notifydrv_handle;
> >
> > -struct pm_event *pm_event;
> > -struct sms *rcb_table;
> > -
> >   /* Handles for SysM3 */
> >   void *platform_nsrn_gate_handle_sysm3;
> >   void *platform_nsrn_handle_sysm3;
> > @@ -982,22 +979,8 @@ void platform_start_callback(void *arg)
> >   				goto pm_register_fail;
> >   			}
> >
> > -			/* Get the shared RCB */
> > -			rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> > -				sizeof(struct sms));
> > -
> > -			pm_event =
> > -				kzalloc(sizeof(struct pm_event)
> > -				* NUMBER_PM_EVENTS, GFP_KERNEL);
> > -
> > -			/* Each event has it own sem */
> > -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> > -				pm_event[i].sem_handle =
> > -					kzalloc(sizeof(struct semaphore),
> > -						GFP_KERNEL);
> > -				sema_init(pm_event[i].sem_handle, 0);
> > -				pm_event[i].event_type = i;
> > -			}
> > +			ipu_pm_setup(platform_notifydrv_handle);
> > +
> >   		}
> >   		/* END PM */
> >
> > @@ -1334,7 +1317,6 @@ void platform_stop_callback(void *arg)
> >   	u16 proc_id = (u32) arg;
> >   	int index = 0;
> >   	u32 nread = 0;
> > -	u32 i = 0;
> >
> >   	if (proc_id == multiproc_get_id("SysM3"))
> >   		index = SMHEAP_SRINDEX_SYSM3;
> > @@ -1460,13 +1442,8 @@ void platform_stop_callback(void *arg)
> >   				(void *)NULL);
> >   			if (status<  0)
> >   				printk(KERN_INFO "ERROR UNREGISTERING PM
> EVENT\n");
> > -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
> > -				kfree(pm_event[i].sem_handle);
> > -				pm_event[i].event_type = 0;
> > -			}
> > -			kfree(pm_event);
> > -			/* Release the shared RCB */
> > -			iounmap(rcb_table);
> > +
> > +			ipu_pm_finish();
> >   		}
> >   		/* END PM */
> >
> 
> I realize this is outside the scope of this patch, but why are the
> semaphore events handles (pm_event[i].sem_handle) allocated? In other
> words, why isn't the structure definition:
> 
> struct pm_event {
> 	enum pm_event_type event_type;
> 	struct semaphore sem;
> };
> 
> Then the initialization wouldn't have to allocate memory and could just
> call sema_init(&pm_event[i].sem, 0), which makes init and de-init a bit
> simpler.

This one is a very good point thanks for the tip I will try this and it will probably be included for the next release too.
> 
> rtg
> --
> Tim Gardner tim.gardner@canonical.com

Thanks a lot for the feedback

Regards...
Miguel Vadillo
Cel. 214-587-2910
Bryan Wu - July 2, 2010, 4:23 a.m.
On 07/01/2010 11:53 PM, Vadillo, Miguel wrote:
> Answers below.
> 
>> -----Original Message-----
>> From: Tim Gardner [mailto:tim.gardner@canonical.com]
>> Sent: Thursday, July 01, 2010 9:47 AM
>> To: Vadillo, Miguel
>> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan,
>> Sebastien
>> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
>> ipu_pm
>>
>> On 06/30/2010 11:02 PM, Bryan Wu wrote:
>>> From: Miguel Vadillo<vadillo@ti.com>
>>>
>>> Changes to fix circular dependencies when compiling ipu_pm in
>>> modules mode.
>>>
>>> Signed-off-by: Miguel Vadillo<vadillo@ti.com>
>>> ---
>>>   arch/arm/plat-omap/clock.c                   |    1 +
>>>   drivers/dsp/syslink/ipu_pm/ipu_pm.c          |   60
>> ++++++++++++++++++++++++--
>>>   drivers/dsp/syslink/ipu_pm/ipu_pm.h          |   10 +++--
>>>   drivers/dsp/syslink/multicore_ipc/platform.c |   31 ++------------
>>>   4 files changed, 67 insertions(+), 35 deletions(-)
>>>   mode change 100644 =>  100755 arch/arm/plat-omap/clock.c
>>>   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>>   mode change 100644 =>  100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>>   mode change 100644 =>  100755
>> drivers/dsp/syslink/multicore_ipc/platform.c
>>>
>>> diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
>>> old mode 100644
>>> new mode 100755
>>> index 701e4ea..104eaec
>>> --- a/arch/arm/plat-omap/clock.c
>>> +++ b/arch/arm/plat-omap/clock.c
>>> @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name)
>>>
>>>   	return ret;
>>>   }
>>> +EXPORT_SYMBOL(omap_clk_get_by_name);
>>>
>>>   /*
>>>    * Low level helpers
>>> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>> b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> old mode 100644
>>> new mode 100755
>>> index 3806f5a..f27512f
>>> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = {
>>>
>>>   struct omap_dm_timer *p_gpt;
>>>   struct clk *p_i2c_clk;
>>> +struct sms *rcb_table;
>>> +void *ipu_pm_notifydrv_handle;
>>> +struct pm_event *pm_event;
>>>
>>
>> Is there any reason these aren't static? Their scope appears to be
>> purely local to this file.
> 
> 
> In the next release we are getting rid of the ipu_pm_notifydrv_handle and we are just providing a handle to access both rcb_table and pm_event.
> 

Actually, not only these 3 pointers, but also lots of varibles and functions are
locally in drivers/dsp/syslink/ipu_pm/ipu_pm.c. They can be set as static.

But that is out of this patch.

>>
>>>   /**
>> ==========================================================================
>> ==
>>>    *  Forward declarations of internal functions
>>> @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId,
>>>
>>>   	/* send the ACK to DUCATI*/
>>>   	return_val = notify_sendevent(
>>> -				platform_notifydrv_handle,
>>> +				ipu_pm_notifydrv_handle,
>>>   				SYS_M3,/*DUCATI_PROC*/
>>>   				PM_RESOURCE,/*PWR_MGMT_EVENT*/
>>>   				payload,
>>> @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>>   		pm_msg.fields.parm = PM_SUCCESS;
>>>   		/* send the ACK to DUCATI*/
>>>   		return_val = notify_sendevent(
>>> -				platform_notifydrv_handle,
>>> +				ipu_pm_notifydrv_handle,
>>>   				SYS_M3,/*DUCATI_PROC*/
>>>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>>   				(unsigned int)pm_msg.whole,
>>> @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>>   		pm_msg.fields.parm = PM_SUCCESS;
>>>   		/* send the ACK to DUCATI*/
>>>   		return_val = notify_sendevent(
>>> -				platform_notifydrv_handle,
>>> +				ipu_pm_notifydrv_handle,
>>>   				SYS_M3,/*DUCATI_PROC*/
>>>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>>   				(unsigned int)pm_msg.whole,
>>> @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>>   		pm_msg.fields.parm = PM_SUCCESS;
>>>   		/* send the ACK to DUCATI*/
>>>   		return_val = notify_sendevent(
>>> -				platform_notifydrv_handle,
>>> +				ipu_pm_notifydrv_handle,
>>>   				SYS_M3,/*DUCATI_PROC*/
>>>   				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>>   				(unsigned int)pm_msg.whole,
>>> @@ -344,6 +347,53 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>>   EXPORT_SYMBOL(ipu_pm_notifications);
>>>
>>>   /*
>>> +  Function for setup ipu_pm module
>>> + *
>>> + */
>>> +int ipu_pm_setup(void *notify_driver_handle)
>>> +{
>>> +	u32 i = 0;
>>> +	ipu_pm_notifydrv_handle = notify_driver_handle;
>>> +	/* Get the shared RCB */
>>> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
>>> +		sizeof(struct sms));
>>> +
>>> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
>>> +				GFP_KERNEL);
>>> +
>>> +	/* Each event has it own sem */
>>> +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
>>> +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
>>> +						GFP_KERNEL);
>>> +		sema_init(pm_event[i].sem_handle, 0);
>>> +		pm_event[i].event_type = i;
>>> +	}
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipu_pm_setup);
>>> +
>>
>> Why bother returning a status code if its just a constant? How about
>> checking for failures when calling ioremap() and kzalloc() ?
> 
> This part is now fixed and will be available for the next release too. We are checking the kzalloc. Also the ioremap is not done anymore since we are requesting a heap from the share memory.
>>
>>> +/*
>>> +  Function for finish ipu_pm module
>>> + *
>>> + */
>>> +int ipu_pm_finish()
>>> +{
>>> +	u32 i = 0;
>>> +	/* Release the shared RCB */
>>> +	for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
>>> +		kfree(pm_event[i].sem_handle);
>>> +		pm_event[i].event_type = 0;
>>> +	}
>>> +	kfree(pm_event);
>>> +	pm_event = NULL;
>>> +	iounmap(rcb_table);
>>> +	rcb_table = NULL;
>>> +	ipu_pm_notifydrv_handle = NULL;
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipu_pm_finish);
>>> +
>>> +/*
>>>     Function for get sdma channels from PRCM
>>>    *
>>>    */
>>> @@ -518,3 +568,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
>>>   	pm_i2c_bus_counter--;
>>>   }
>>>
>>> +MODULE_LICENSE("GPL");
>>> +
>>> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>> b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> old mode 100644
>>> new mode 100755
>>> index e9106fb..4481c6d
>>> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> @@ -185,10 +185,6 @@ struct rcb_block {
>>>
>>>   };
>>>
>>> -extern struct sms *rcb_table;
>>> -extern void *platform_notifydrv_handle;
>>> -extern struct pm_event *pm_event;
>>> -
>>>   struct sms {
>>>   	unsigned rat;
>>>   	struct rcb_block rcb[RCB_MAX];
>>> @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId,
>>>   /* Function for send PM Notifications */
>>>   int ipu_pm_notifications(enum pm_event_type event_type);
>>>
>>> +/* Function for setup ipu_pm module */
>>> +int ipu_pm_setup(void *notify_driver_handle);
>>> +
>>> +/* Function for finish ipu_pm module */
>>> +int ipu_pm_finish(void);
>>> +
>>>   #endif
>>>
>>> diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c
>> b/drivers/dsp/syslink/multicore_ipc/platform.c
>>> old mode 100644
>>> new mode 100755
>>> index d1c62e7..1cdd686
>>> --- a/drivers/dsp/syslink/multicore_ipc/platform.c
>>> +++ b/drivers/dsp/syslink/multicore_ipc/platform.c
>>> @@ -233,9 +233,6 @@
>>>    */
>>>   void *platform_notifydrv_handle;
>>>
>>> -struct pm_event *pm_event;
>>> -struct sms *rcb_table;
>>> -
>>>   /* Handles for SysM3 */
>>>   void *platform_nsrn_gate_handle_sysm3;
>>>   void *platform_nsrn_handle_sysm3;
>>> @@ -982,22 +979,8 @@ void platform_start_callback(void *arg)
>>>   				goto pm_register_fail;
>>>   			}
>>>
>>> -			/* Get the shared RCB */
>>> -			rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
>>> -				sizeof(struct sms));
>>> -
>>> -			pm_event =
>>> -				kzalloc(sizeof(struct pm_event)
>>> -				* NUMBER_PM_EVENTS, GFP_KERNEL);
>>> -
>>> -			/* Each event has it own sem */
>>> -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
>>> -				pm_event[i].sem_handle =
>>> -					kzalloc(sizeof(struct semaphore),
>>> -						GFP_KERNEL);
>>> -				sema_init(pm_event[i].sem_handle, 0);
>>> -				pm_event[i].event_type = i;
>>> -			}
>>> +			ipu_pm_setup(platform_notifydrv_handle);
>>> +
>>>   		}
>>>   		/* END PM */
>>>
>>> @@ -1334,7 +1317,6 @@ void platform_stop_callback(void *arg)
>>>   	u16 proc_id = (u32) arg;
>>>   	int index = 0;
>>>   	u32 nread = 0;
>>> -	u32 i = 0;
>>>
>>>   	if (proc_id == multiproc_get_id("SysM3"))
>>>   		index = SMHEAP_SRINDEX_SYSM3;
>>> @@ -1460,13 +1442,8 @@ void platform_stop_callback(void *arg)
>>>   				(void *)NULL);
>>>   			if (status<  0)
>>>   				printk(KERN_INFO "ERROR UNREGISTERING PM
>> EVENT\n");
>>> -			for (i = 0; i<  NUMBER_PM_EVENTS; i++) {
>>> -				kfree(pm_event[i].sem_handle);
>>> -				pm_event[i].event_type = 0;
>>> -			}
>>> -			kfree(pm_event);
>>> -			/* Release the shared RCB */
>>> -			iounmap(rcb_table);
>>> +
>>> +			ipu_pm_finish();
>>>   		}
>>>   		/* END PM */
>>>
>>
>> I realize this is outside the scope of this patch, but why are the
>> semaphore events handles (pm_event[i].sem_handle) allocated? In other
>> words, why isn't the structure definition:
>>
>> struct pm_event {
>> 	enum pm_event_type event_type;
>> 	struct semaphore sem;
>> };
>>
>> Then the initialization wouldn't have to allocate memory and could just
>> call sema_init(&pm_event[i].sem, 0), which makes init and de-init a bit
>> simpler.
> 
> This one is a very good point thanks for the tip I will try this and it will probably be included for the next release too.
>>

And will we got an incremental patch to fix this according to the review. Or we
have to wait for next release.

Thanks,
-Bryan
Vadillo, Miguel - July 2, 2010, 5:27 a.m.
The changes will be on next release if they are easy to do since we are close to the release but we take consideration of all suggestions even if they are out of the scope so thanks in advance for the feedback.

Best Regards.

Miguel Vadillo
Tim Gardner - July 2, 2010, 1:17 p.m.
On 07/01/2010 09:53 AM, Vadillo, Miguel wrote:

>>>    /*
>>> +  Function for setup ipu_pm module
>>> + *
>>> + */
>>> +int ipu_pm_setup(void *notify_driver_handle)
>>> +{
>>> +	u32 i = 0;
>>> +	ipu_pm_notifydrv_handle = notify_driver_handle;
>>> +	/* Get the shared RCB */
>>> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
>>> +		sizeof(struct sms));
>>> +
>>> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
>>> +				GFP_KERNEL);
>>> +
>>> +	/* Each event has it own sem */
>>> +	for (i = 0; i<   NUMBER_PM_EVENTS; i++) {
>>> +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
>>> +						GFP_KERNEL);
>>> +		sema_init(pm_event[i].sem_handle, 0);
>>> +		pm_event[i].event_type = i;
>>> +	}
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipu_pm_setup);
>>> +
>>
>> Why bother returning a status code if its just a constant? How about
>> checking for failures when calling ioremap() and kzalloc() ?
>
> This part is now fixed and will be available for the next release too.
 >We are checking the kzalloc. Also the ioremap is not done anymore 
since we are requesting a heap from the share memory.

So, you're saying this is all different in 2.6.35 (which I assume is the 
'next release') ? That could be awhile.

As far as I can tell from looking at the repositories at 
git://dev.omapzoom.org/pub/scm/integration you wrote these patches just 
for the Ubuntu branch. Since we may have to live with this 2.6.34 based 
kernel for a few weeks more, please take the time to write correct code 
and resend this patch. At least put some BUG_ON()s in it so we catch 
allocation failures.

rtg
Vadillo, Miguel - July 2, 2010, 3:38 p.m.
> -----Original Message-----
> From: Tim Gardner [mailto:tim.gardner@canonical.com]
> Sent: Friday, July 02, 2010 8:17 AM
> To: Vadillo, Miguel
> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan,
> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan
> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
> ipu_pm
> 
> On 07/01/2010 09:53 AM, Vadillo, Miguel wrote:
> 
> >>>    /*
> >>> +  Function for setup ipu_pm module
> >>> + *
> >>> + */
> >>> +int ipu_pm_setup(void *notify_driver_handle)
> >>> +{
> >>> +	u32 i = 0;
> >>> +	ipu_pm_notifydrv_handle = notify_driver_handle;
> >>> +	/* Get the shared RCB */
> >>> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> >>> +		sizeof(struct sms));
> >>> +
> >>> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
> >>> +				GFP_KERNEL);
> >>> +
> >>> +	/* Each event has it own sem */
> >>> +	for (i = 0; i<   NUMBER_PM_EVENTS; i++) {
> >>> +		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
> >>> +						GFP_KERNEL);
> >>> +		sema_init(pm_event[i].sem_handle, 0);
> >>> +		pm_event[i].event_type = i;
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(ipu_pm_setup);
> >>> +
> >>
> >> Why bother returning a status code if its just a constant? How about
> >> checking for failures when calling ioremap() and kzalloc() ?
> >
> > This part is now fixed and will be available for the next release too.
>  >We are checking the kzalloc. Also the ioremap is not done anymore
> since we are requesting a heap from the share memory.
> 
> So, you're saying this is all different in 2.6.35 (which I assume is the
> 'next release') ? That could be awhile.
> 
> As far as I can tell from looking at the repositories at
> git://dev.omapzoom.org/pub/scm/integration you wrote these patches just
> for the Ubuntu branch. Since we may have to live with this 2.6.34 based
> kernel for a few weeks more, please take the time to write correct code
> and resend this patch. At least put some BUG_ON()s in it so we catch
> allocation failures.
> 


Ok in that case I will rework the patches following your suggestions and resend them ASAP, probably next week.

> rtg
> --
> Tim Gardner tim.gardner@canonical.com

Regards...
Miguel Vadillo
Vadillo, Miguel - July 6, 2010, 7:45 p.m.
Tim, Bryan,

	Please find attached the patch that solves the error with the return values in the ipu_pm_setup function.
	Regarding the semaphore and the static declaration both changes will be included in the next release.

	Please let us know what you think. As always your feedback is welcome.

Regards...
Miguel Vadillo
Cel. 214-587-2910


> -----Original Message-----
> From: Vadillo, Miguel
> Sent: Friday, July 02, 2010 10:39 AM
> To: 'tim.gardner@canonical.com'
> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan,
> Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan
> Subject: RE: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
> ipu_pm
> 
> > -----Original Message-----
> > From: Tim Gardner [mailto:tim.gardner@canonical.com]
> > Sent: Friday, July 02, 2010 8:17 AM
> > To: Vadillo, Miguel
> > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@canonical.com; Jan,
> > Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan
> > Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
> > ipu_pm
> >
> > On 07/01/2010 09:53 AM, Vadillo, Miguel wrote:
> >
> > >>>    /*
> > >>> +  Function for setup ipu_pm module
> > >>> + *
> > >>> + */
> > >>> +int ipu_pm_setup(void *notify_driver_handle)
> > >>> +{
> > >>> +	u32 i = 0;
> > >>> +	ipu_pm_notifydrv_handle = notify_driver_handle;
> > >>> +	/* Get the shared RCB */
> > >>> +	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
> > >>> +		sizeof(struct sms));
> > >>> +
> > >>> +	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
> > >>> +				GFP_KERNEL);
> > >>> +
> > >>> +	/* Each event has it own sem */
> > >>> +	for (i = 0; i<   NUMBER_PM_EVENTS; i++) {
> > >>> +		pm_event[i].sem_handle = kzalloc(sizeof(struct
> semaphore),
> > >>> +						GFP_KERNEL);
> > >>> +		sema_init(pm_event[i].sem_handle, 0);
> > >>> +		pm_event[i].event_type = i;
> > >>> +	}
> > >>> +	return 0;
> > >>> +}
> > >>> +EXPORT_SYMBOL(ipu_pm_setup);
> > >>> +
> > >>
> > >> Why bother returning a status code if its just a constant? How about
> > >> checking for failures when calling ioremap() and kzalloc() ?
> > >
> > > This part is now fixed and will be available for the next release too.
> >  >We are checking the kzalloc. Also the ioremap is not done anymore
> > since we are requesting a heap from the share memory.
> >
> > So, you're saying this is all different in 2.6.35 (which I assume is the
> > 'next release') ? That could be awhile.
> >
> > As far as I can tell from looking at the repositories at
> > git://dev.omapzoom.org/pub/scm/integration you wrote these patches just
> > for the Ubuntu branch. Since we may have to live with this 2.6.34 based
> > kernel for a few weeks more, please take the time to write correct code
> > and resend this patch. At least put some BUG_ON()s in it so we catch
> > allocation failures.
> >
> 
> 
> Ok in that case I will rework the patches following your suggestions and
> resend them ASAP, probably next week.
> 
> > rtg
> > --
> > Tim Gardner tim.gardner@canonical.com
> 
> Regards...
> Miguel Vadillo

Patch

diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
old mode 100644
new mode 100755
index 701e4ea..104eaec
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -336,6 +336,7 @@  struct clk *omap_clk_get_by_name(const char *name)
 
 	return ret;
 }
+EXPORT_SYMBOL(omap_clk_get_by_name);
 
 /*
  * Low level helpers
diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
old mode 100644
new mode 100755
index 3806f5a..f27512f
--- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
+++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
@@ -85,6 +85,9 @@  int ipu_timer_list[NUM_IPU_TIMERS] = {
 
 struct omap_dm_timer *p_gpt;
 struct clk *p_i2c_clk;
+struct sms *rcb_table;
+void *ipu_pm_notifydrv_handle;
+struct pm_event *pm_event;
 
 /** ============================================================================
  *  Forward declarations of internal functions
@@ -221,7 +224,7 @@  void ipu_pm_callback(short int procId,
 
 	/* send the ACK to DUCATI*/
 	return_val = notify_sendevent(
-				platform_notifydrv_handle,
+				ipu_pm_notifydrv_handle,
 				SYS_M3,/*DUCATI_PROC*/
 				PM_RESOURCE,/*PWR_MGMT_EVENT*/
 				payload,
@@ -280,7 +283,7 @@  int ipu_pm_notifications(enum pm_event_type event_type)
 		pm_msg.fields.parm = PM_SUCCESS;
 		/* send the ACK to DUCATI*/
 		return_val = notify_sendevent(
-				platform_notifydrv_handle,
+				ipu_pm_notifydrv_handle,
 				SYS_M3,/*DUCATI_PROC*/
 				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
 				(unsigned int)pm_msg.whole,
@@ -301,7 +304,7 @@  int ipu_pm_notifications(enum pm_event_type event_type)
 		pm_msg.fields.parm = PM_SUCCESS;
 		/* send the ACK to DUCATI*/
 		return_val = notify_sendevent(
-				platform_notifydrv_handle,
+				ipu_pm_notifydrv_handle,
 				SYS_M3,/*DUCATI_PROC*/
 				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
 				(unsigned int)pm_msg.whole,
@@ -322,7 +325,7 @@  int ipu_pm_notifications(enum pm_event_type event_type)
 		pm_msg.fields.parm = PM_SUCCESS;
 		/* send the ACK to DUCATI*/
 		return_val = notify_sendevent(
-				platform_notifydrv_handle,
+				ipu_pm_notifydrv_handle,
 				SYS_M3,/*DUCATI_PROC*/
 				PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
 				(unsigned int)pm_msg.whole,
@@ -344,6 +347,53 @@  int ipu_pm_notifications(enum pm_event_type event_type)
 EXPORT_SYMBOL(ipu_pm_notifications);
 
 /*
+  Function for setup ipu_pm module
+ *
+ */
+int ipu_pm_setup(void *notify_driver_handle)
+{
+	u32 i = 0;
+	ipu_pm_notifydrv_handle = notify_driver_handle;
+	/* Get the shared RCB */
+	rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
+		sizeof(struct sms));
+
+	pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
+				GFP_KERNEL);
+
+	/* Each event has it own sem */
+	for (i = 0; i < NUMBER_PM_EVENTS; i++) {
+		pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
+						GFP_KERNEL);
+		sema_init(pm_event[i].sem_handle, 0);
+		pm_event[i].event_type = i;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(ipu_pm_setup);
+
+/*
+  Function for finish ipu_pm module
+ *
+ */
+int ipu_pm_finish()
+{
+	u32 i = 0;
+	/* Release the shared RCB */
+	for (i = 0; i < NUMBER_PM_EVENTS; i++) {
+		kfree(pm_event[i].sem_handle);
+		pm_event[i].event_type = 0;
+	}
+	kfree(pm_event);
+	pm_event = NULL;
+	iounmap(rcb_table);
+	rcb_table = NULL;
+	ipu_pm_notifydrv_handle = NULL;
+	return 0;
+}
+EXPORT_SYMBOL(ipu_pm_finish);
+
+/*
   Function for get sdma channels from PRCM
  *
  */
@@ -518,3 +568,5 @@  inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
 	pm_i2c_bus_counter--;
 }
 
+MODULE_LICENSE("GPL");
+
diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
old mode 100644
new mode 100755
index e9106fb..4481c6d
--- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
+++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
@@ -185,10 +185,6 @@  struct rcb_block {
 
 };
 
-extern struct sms *rcb_table;
-extern void *platform_notifydrv_handle;
-extern struct pm_event *pm_event;
-
 struct sms {
 	unsigned rat;
 	struct rcb_block rcb[RCB_MAX];
@@ -214,5 +210,11 @@  void ipu_pm_notify_callback(short int procId,
 /* Function for send PM Notifications */
 int ipu_pm_notifications(enum pm_event_type event_type);
 
+/* Function for setup ipu_pm module */
+int ipu_pm_setup(void *notify_driver_handle);
+
+/* Function for finish ipu_pm module */
+int ipu_pm_finish(void);
+
 #endif
 
diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c b/drivers/dsp/syslink/multicore_ipc/platform.c
old mode 100644
new mode 100755
index d1c62e7..1cdd686
--- a/drivers/dsp/syslink/multicore_ipc/platform.c
+++ b/drivers/dsp/syslink/multicore_ipc/platform.c
@@ -233,9 +233,6 @@ 
  */
 void *platform_notifydrv_handle;
 
-struct pm_event *pm_event;
-struct sms *rcb_table;
-
 /* Handles for SysM3 */
 void *platform_nsrn_gate_handle_sysm3;
 void *platform_nsrn_handle_sysm3;
@@ -982,22 +979,8 @@  void platform_start_callback(void *arg)
 				goto pm_register_fail;
 			}
 
-			/* Get the shared RCB */
-			rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
-				sizeof(struct sms));
-
-			pm_event =
-				kzalloc(sizeof(struct pm_event)
-				* NUMBER_PM_EVENTS, GFP_KERNEL);
-
-			/* Each event has it own sem */
-			for (i = 0; i < NUMBER_PM_EVENTS; i++) {
-				pm_event[i].sem_handle =
-					kzalloc(sizeof(struct semaphore),
-						GFP_KERNEL);
-				sema_init(pm_event[i].sem_handle, 0);
-				pm_event[i].event_type = i;
-			}
+			ipu_pm_setup(platform_notifydrv_handle);
+
 		}
 		/* END PM */
 
@@ -1334,7 +1317,6 @@  void platform_stop_callback(void *arg)
 	u16 proc_id = (u32) arg;
 	int index = 0;
 	u32 nread = 0;
-	u32 i = 0;
 
 	if (proc_id == multiproc_get_id("SysM3"))
 		index = SMHEAP_SRINDEX_SYSM3;
@@ -1460,13 +1442,8 @@  void platform_stop_callback(void *arg)
 				(void *)NULL);
 			if (status < 0)
 				printk(KERN_INFO "ERROR UNREGISTERING PM EVENT\n");
-			for (i = 0; i < NUMBER_PM_EVENTS; i++) {
-				kfree(pm_event[i].sem_handle);
-				pm_event[i].event_type = 0;
-			}
-			kfree(pm_event);
-			/* Release the shared RCB */
-			iounmap(rcb_table);
+
+			ipu_pm_finish();
 		}
 		/* END PM */