"Gopinath, Thara" <thara@xxxxxx> writes: >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Wednesday, April 28, 2010 12:45 AM >>>To: Gopinath, Thara >>>Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; Cousson, Benoit; Sripathy, Vishwanath; Sawant, Anand >>>Subject: Re: [PATCHv3 18/22] OMAP3: PM: Optional reset of voltage during Smartreflex disable. >>> >>>Thara Gopinath <thara@xxxxxx> writes: >>> >>>> Currently whenever smartreflex is disabled the voltage for the >>>> particular VDD is reset to the non-smartreflex compensated level. >>>> This step is unnecessary during dvfs because anyways in the next couple >>>> of steps before re-enabling smartreflex , the voltage level is changed. >>>> >>>> This patch adds the flexibility in the smartreflex framework for the user >>>> to specify whether or not a voltage reset is required after disabling >>>> of smartrefelx. The smartreflex driver just passes on this info >>>> to the smartreflex class driver, which ultimately takes the >>>> decision to reset the voltage or not. >>>> >>>> Signed-off-by: Thara Gopinath <thara@xxxxxx> >>> >>>I don't think this option should be a decision made for each call to >>>omap_smartreflex_[en|dis]able(). Rather it should be an init time >>>option. > > Why do you say this? Anytime we do a disable of smartreflex auto >compensation from user space we need a reset of the voltage is >required. During dvfs during smartreflex disable a reset of the >voltage is not required. And in both these scenarios it is the same >class API that gets called. So the only way for the API to know >whether to reset the voltage or not is through this parameter. Also >IMHO keeping it parameter based allows more flexibility in the >framework for voltage reset. OK, I see now. I was looking at this patch in isolation and didn't notice the usage of it in the next DVFS patch where the ->disable is done without reset. Also, since this new flag is a bool, change it from int to bool. Or better yet add a new function. One thing I'm not a fan of for readability sake is the use of bool flags. It makes me have to go look at the function to see what the flag is used for. I'd rather just see a new function added. Something like omap_smartreflex_disable_noreset() would make the code a bit more readable. Thanks, Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
