Re: [RFC PATCH 6/6] hwmon: OMAP4: On die temperature sensor driver

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]

Hi,

On Fri, Aug 12, 2011 at 08:56:29AM +0530, Rajendra Nayak wrote:
> On 8/12/2011 3:07 AM, Roger Quadros wrote:
> >On 08/11/2011 01:55 PM, Felipe Balbi wrote:
> >>Hi,
> >>
> >>On Thu, Aug 11, 2011 at 09:54:09PM +0300, Felipe Balbi wrote:
> >>>>>you need some other way to handle this. Why do you need to manually set
> >>>>>the rate rather than having hwmod handle this for you ?
> >>>>>
> >>>>>your argument that "it's a one time setting" is not enough to have this
> >>>>>in the driver. Drivers should not care about clocks anymore, this should
> >>>>>have been done on another layer.
> >>>>
> >>>>Hwmod will have no idea on the rate required.
> >>>
> >>>does the rate need to change ? Also, I have not mentioned hwmod anytime
> >>
> >>i did mention hwmod, nevermind that part. Still I'm not sure where is
> >>the right place to handle this.
> >>
> >
> >Aren't the omap_device_pm_latency callbacks the right place to do it?
> >
> >e.g. in the following snippet from mach-omap2/temp_sensor_device.c
> >
> >+static struct omap_device_pm_latency omap_temp_sensor_latency[] = {
> >+	{
> >+	 .deactivate_func = omap_device_idle_hwmods,
> >+	 .activate_func = omap_device_enable_hwmods,
> >+	 .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> >+	}
> >+};
> >
> >instead of directly pointing activate_func to omap_device_enable_hwmods,
> >it could point to a function that sets the required clock rate and then
> >enables the hwmod.
> 
> FWIK, its a one time requirement to set the clock rate to the
> right rate the device can operate in based on what a platform
> supports. What you are suggesting would add the overhead of doing
> this every time the device is runtime enabled/idled.

if it's a one time setting, why don't you just change the clock fwk to
handle this nicely ? Maybe provide a different ->enable() function which
would already set the correct rate ?

Russell, what would be the best way here ? driver needs clock to be at a
particular rate for the device to work, but it's a one time setting and
I don't think driver should be doing clk_get() - clk_enable() -
clk_set_rate(), where should that functionality be put ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]