Re: [PATCH v12 4/9] OMAP2+: dmtimer: convert to platform devices

[Kevin Hilman <khilman@ti.com>]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> Add routines to converts dmtimers to platform devices. The device data
> is obtained from hwmod database of respective platform and is registered
> to device model after successful binding to driver. It also provides
> provision to access timers during early boot when pm_runtime framework
> is not completely up and running.
>
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> Signed-off-by: Thara Gopinath <thara@ti.com>
> Acked-by: Cousson, Benoit <b-cousson@ti.com>

[...]

> +/**
> + * omap_timer_init - build and register timer device with an
> + * associated timer hwmod
> + * @oh:	timer hwmod pointer to be used to build timer device
> + * @user:	parameter that can be passed from calling hwmod API
> + *
> + * Called by omap_hwmod_for_each_by_class to register each of the timer
> + * devices present in the system. The number of timer devices is known
> + * by parsing through the hwmod database for a given class name. At the
> + * end of function call memory is allocated for timer device and it is
> + * registered to the framework ready to be proved by the driver.
> + */
> +static int __init omap_timer_init(struct omap_hwmod *oh, void *unused)
> +{
> +	int id;
> +	int ret = 0;
> +	char *name = "omap_timer";
> +	struct dmtimer_platform_data *pdata;
> +	struct omap_device *od;
> +	struct omap_secure_timer_dev_attr *secure_timer_dev_attr;
> +
> +	/*
> +	 * Extract the IDs from name field in hwmod database
> +	 * and use the same for constructing ids' for the
> +	 * timer devices. In a way, we are avoiding usage of
> +	 * static variable witin the function to do the same.
> +	 * CAUTION: We have to be careful and make sure the
> +	 * name in hwmod database does not change in which case
> +	 * we might either make corresponding change here or
> +	 * switch back static variable mechanism.
> +	 */
> +	sscanf(oh->name, "timer%2d", &id);
> +	if (unlikely(id == system_timer_id))
> +		return ret;

As mentioned already, this check belongs in the patch where the
system_timer is setup.

In addition, I'm not sure this is completely right.

With this check, the system timer will never be converted from an early
device to a real platform_device.   As a result, runtime PM will never
be enabled for this device.

I guess Tony has the final say here, but personally, I don't really like
the special treatment of a system timer, unless it is an init-time only
special treatment.  Since we now have dynamically configurable
clocksources, the concept of a system timer doesn't really exist (except
for in early boot.)  At any time during runtime, we could dynamically
switch the clocksource to a different timer device.  At this point, one
would expect runtime PM for the previous timer to kick in and idle the
timer.  That cannot happen with this approach.

Kevin