>>-----Original Message----- >>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>Sent: Thursday, September 16, 2010 8:58 PM >>To: Gopinath, Thara; Paul Walmsley >>Cc: linux-omap@xxxxxxxxxxxxxxx; Sripathy, Vishwanath; Sawant, Anand; Cousson, Benoit >>Subject: Re: [PATCH 05/13] OMAP: Introduce device specific set rate and get rate in device opp >>structures. >> >>"Gopinath, Thara" <thara@xxxxxx> writes: >> >>>>>-----Original Message----- >>>>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>>>Sent: Friday, September 03, 2010 5:12 AM >>>>>To: Gopinath, Thara >>>>>Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; 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@xxxxxx> 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@xxxxxx> >>>>> >>>>>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. Hello Kevin, I basically agree with all your other suggestions except this. I do not think device files are the correct place for this. For starters definitely mpu, l3 and iva devices are not built from any device file. I do not like the idea of device set rate APIs spread across different files. What I am proposing is like we have opp_populate_rate_fns(which I have introduced) we should have something like omap_device_populate_rate_fns. This can be then called from one file to register the set_rate and get_rate APIs for all the relevant devices for a Soc. This will be done through an init function. And the device specific set_rate and get_rate can be implemented in this file. I am not sure which is this file at this point of time. Maybe for OMAP3 opp3xxx_data.c or pm34xx.c. What do you think about this? Regards Thara >> >>> 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 -- 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
