RE: [PATCH 01/04] OMAP3 SRF: Generic shared resource f/w

["Rajendra Nayak" <rnayak@ti.com>]

> -----Original Message-----
> From: Dasgupta, Romit [mailto:romit@ti.com] 
> Sent: Friday, October 17, 2008 1:06 PM
> To: Nayak, Rajendra; linux-omap@vger.kernel.org
> Subject: RE: [PATCH 01/04] OMAP3 SRF: Generic shared resource f/w
> 
> >> 1) curr_level of the shared_resource is not updated in
> >> update_resource_level
> >
> >Yes, its not. Its done as part of the platform specific 
> change_level call.
> 
> [Romit] Kindly see below. I was thinking that it is better 
> that we do not access any fields of struct shared_resource 
> from outside this file. Would the line below solve the problem?

The actual *target_level* of the resource will depend on the resource type.
Hence for say a "per_pwrdm_latency" resource if some user request's for a 
latency of no more than 130ms, it inturn translates to a PWRDM_RET level
depending on the specific latency to go to RET/OFF.

So what is currently done is the current_level of the resource in the above case 
is set to PWRDM_RET and if done the way you are suggesting will be set to 130ms.
 
> 
> >> >+static int update_resource_level(struct shared_resource *resp)
> >> >+{
> >> >+      struct users_list *user;
> >> >+      unsigned long target_level;
> >> >+      int ret;
> >> >+
> >> >+      /* Regenerate the target_value for the resource */
> >> >+      target_level = RES_DEFAULTLEVEL;
> >> >+      list_for_each_entry(user, &resp->users_list, node)
> >> >+              if (user->level > target_level)
> >> >+                      target_level = user->level;
> >> >+
> >> >+      pr_debug("SRF: Changing Level for resource %s to %ld\n",
> >> >+                              resp->name, target_level);
> >> >+      ret = resp->ops->change_level(resp, target_level);
> >> >+      if (ret) {
> >> >+              printk(KERN_ERR "Unable to Change"
> >> >+                                      "level for resource
> >> %s to %ld\n",
> >> >+              resp->name, target_level);
> >> >+      } else 
> 	          resp->curr_level = target_level; /* [Romit]  
> Should be done here.*/
> >> >+      return ret;
> >> >+}
> >
> >> 2) resource_request invokes spin_lock_irqsave and then if it
> >> is a request for a new device it invokes get_user().
> >> get_user() calls kmalloc with GFP_KERNEL. So it can sleep.
> >> Hence you will sleep with spinlocks held!!
> >
> >Right, so I'll probably have to add a GFP_ATOMIC flag to that.
> >I am now thinking If I really need spinlocks, think I can do 
> with mutex's instead.
> >The spinlocks were put in place to take care of the omap-pm 
> hooks from
> >clock f/w which no longer seem to be needed.
> [Romit] Trying to review the rest of the patches in the 
> patchset to get a better picture.
> 
>