Re: [PATCH v4 7/9] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers

[Kevin Hilman <khilman@deeprootsystems.com>]

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

>>>-----Original Message-----
>>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>>Sent: Thursday, November 11, 2010 12:43 AM
>>>To: Gopinath, Thara
>>>Cc: linux-omap@vger.kernel.org; paul@pwsan.com; Cousson, Benoit; Sripathy,
>>>Vishwanath; Sawant, Anand
>>>Subject: Re: [PATCH v4 7/9] OMAP3: PM: Adding debug support to Voltage and
>>>Smartreflex drivers
>>>
>>>Thara Gopinath <thara@ti.com> writes:
>>>
>>>> This patch adds debug support to the voltage and smartreflex drivers.
>>>> This means a whole bunch of voltage processor and smartreflex
>>>> parameters are now visible through the pm debugfs.
>>>> The voltage parameters can be viewed at
>>>>         /debug/voltage/vdd_<x>/<parameter>
>>>> and the smartreflex parameters can be viewed at
>>>>         /debug/vdd_<x>/smartreflex/<parameter>
>>>>
>>>> To enable overriding of these parameters from user side, write 1
>>>> into
>>>> 	/debug/voltage/vdd_<x>/override_volt_params
>>>
>>>Please just git rid of any sort of override parameter from sysfs.
>>>
>>>Instead, you can detect in the sysfs code itself if any parameters were
>>>changed and then set the vdd->user_override flag.
>
> But when will I unset this flag??

You can't.

And, AFAICT, it wasn't clear from the current code or docs whether this
could work or was expected to work either, e.g., if you set
override_volt_params back to zero, to the original values all get reused?

If you want to provide this feature, then it should be documented and
made clear that this is an intended goal.

Thinking about this more, the main thing I don't like about this
approach is that the active code paths (enable & disable) have to check
each time if any of these values have been overidden.

Rather than have several places in the active code paths where this
override value is checked, there the sysfs methods should simply update
the values that are used by the core code.  This way, the core would 
not need to know about where the values came from (defalts, volt_data,
user override, etc.)

If you want to provide a way to revert this, then maybe writing -1 will
should switch that value back to the HW default, or volt_data default.

Kevin