Re: [PATCH 05/13] OMAP: Introduce device specific set rate and get rate in device opp structures.

[Kevin Hilman <khilman@deeprootsystems.com>]

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

>>>-----Original Message-----
>>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>>Sent: Friday, September 03, 2010 5:12 AM
>>>To: Gopinath, Thara
>>>Cc: linux-omap@vger.kernel.org; paul@pwsan.com; Sripathy, Vishwanath; Sawant, Anand; Cousson, Benoit
>>>Subject: Re: [PATCH 05/13] OMAP: Introduce device specific set rate and get rate in device opp
>>>structures.
>>>
>>>Thara Gopinath <thara@ti.com> writes:
>>>
>>>> This patch extends the device opp structure to contain
>>>> pointers to scale the operating rate of the
>>>> device and to retrieve the operating rate of the device.
>>>> This patch also adds the three new APIs in the opp layer
>>>> namely opp_set_rate that can be called to set a new operating
>>>> rate for a device, opp_get_rate that can be called to retrieve
>>>> the operating frequency for a device and opp_populate_rate_fns
>>>> to populte the device specific set_rate and get_rate API's.
>>>> The opp_set_rate and opp_get_rate does some routine error
>>>> checks and finally calls into the device specific set_rate
>>>> and get_rate APIs populated through opp_populate_rate_fns.
>>>>
>>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>>
>>>As I think about this more, I'm not sure the OPP layer is the right
>>>layer for the get/set rate functions.  The OPP layer is currently just
>>>the data store for OPP data. To me, these set/get functions are better
>>>associated directly with an omap_device instead of an OPP.
>>>
>>>For instance, the new OPP APIs are a bit confusing in terms of OPPs.
>>>e.g: opp_get_rate().  What is the "rate" of an OPP, and what's the
>>>difference with opp_get_freq()?
>>>
>>>What's really happening is the rate is being changed for a device, and
>>>the need for specific hooks are at the device level, not the OPP level.
>>>For example, this current approach would not scale if you needed
>>>multiple devices in the same domain that each needed custom
>>>set_rate/get_rate hooks.
>>>
>>>So instead, what about adding custom hooks at the omap_device level?
>>>They could be registered at omap_device_build() time in the device
>>>specific code.
>
> This is exactly what I had in my mind when I started implementing this.
> But then Paul said hwmod or omap_device is not the place to keep it. 

IIRC, Paul's concern was that *hwmod* was not the right place for this
(and I agree.)  However, I think omap_device is the right place for
this.

Paul?

> Also I do not want the set rate get rate APIs for devices that require
> changes dividers in the PRCM clock module to be spread out in the
> system. Makes it very very difficult to debug.  If we agree to add the
> set_rate and get_rate in the omap_device structure, I would like to
> have one more API in the omap_device layer to register these APIs
> device wise. Thus we can keep these set rate and get rate APIs in one
> single file.

Agreed.  And those functions should be in the respective
device-specific file, where the omap_device_build() etc are done for
that device.

> Also if we move these hooks to omap_device struct we will need to
> rename the current omap_device_set_rate (the main API) to
> omap_device_scale and introduce a new omap_device_set_rate which just
> finds out the omap device from the passed dev pointer and does a
> od->set_rate. Similarly for get rate also.

Yes, that's what I was thinking.

Kevin