Re: [RFC 3/7] OMAP: Introduce voltage domain pointer and device specific set rate and get rate in device opp structures.

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Hello Thara,

On Fri,  2 Jul 2010 15:48:25 +0530
Thara Gopinath <thara@ti.com> 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