Re: [PATCHv3 18/22] OMAP3: PM: Optional reset of voltage during Smartreflex disable.

[Kevin Hilman <khilman@deeprootsystems.com>]

"Gopinath, Thara" <thara@ti.com> writes:

>>>-----Original Message-----
>>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>>Sent: Wednesday, April 28, 2010 12:45 AM
>>>To: Gopinath, Thara
>>>Cc: linux-omap@vger.kernel.org; paul@pwsan.com; Cousson, Benoit; Sripathy, Vishwanath; Sawant, Anand
>>>Subject: Re: [PATCHv3 18/22] OMAP3: PM: Optional reset of voltage during Smartreflex disable.
>>>
>>>Thara Gopinath <thara@ti.com> 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@ti.com>
>>>
>>>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