Hello Thara,
On Fri, 2 Jul 2010 15:48:25 +0530
Thara Gopinath <[email protected]> wrote:
> #include <plat/common.h>
>
> @@ -38,21 +39,45 @@
> */
> struct omap_opp_def {
> char *hwmod_name;
> + char *vdd_name;
>
> unsigned long freq;
> unsigned long u_volt;
>
> + int (*set_rate)(struct device *dev, unsigned long rate);
> + unsigned long (*get_rate) (struct device *dev);
> +
> bool enabled;
> };
It looks strange to me that per-OPP methods are needed to set/get the
rate. These should only be needed at the device level, no ?
And indeed, in your patch 6/7, for every device, the same get/set rate
methods are used for all OPPs of a given device.
> +struct device_opp {
> + struct list_head node;
> + char *vdd_name;
> +
> + struct omap_hwmod *oh;
> + struct device *dev;
> + struct omap_volt_domain *volt_domain;
> +
> + struct list_head opp_list;
> + u32 opp_count;
> + u32 enabled_opp_count;
> +
> + int (*set_rate)(struct device *dev, unsigned long rate);
> + unsigned long (*get_rate) (struct device *dev);
> +};
I know this structure already exists, but do we really need a new
structure for this ? Couldn't these infos (OPP list, set/get rate
methods) be stored in an existing device-specific structure (omap_hwmod
for hardware-related things are omap_device for ~software-related
things) ?
For example, why aren't OPPs attached to the hwmod description of the
particular device ? These OPPs definitions really look like a
characteristic of a particular IP, don't they ?
Whatever choice is made, this structure probably needs a comment on top
of it explaining what it does, since the name isn't very obvious IMO.
Thanks!
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
