RE: [PATCH 02/13] OMAP: Introduce device specific set rate and get rate in omap_device structure

[Vishwanath Sripathy <vishwanath.bs@ti.com>]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@ti.com]
> Sent: Friday, February 04, 2011 5:17 AM
> To: Vishwanath BS
> Cc: linux-omap@vger.kernel.org; patches@linaro.org; Thara Gopinath
> Subject: Re: [PATCH 02/13] OMAP: Introduce device specific set rate and
> get rate in omap_device structure
>
> Vishwanath BS <vishwanath.bs@ti.com> writes:
>
> > From: Thara Gopinath <thara@ti.com>
> >
> > This patch extends the omap_device 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 omap device layer
> > namely omap_device_set_rate that can be called to set a new
> operating
> > rate for a device, omap_device_get_rate that can be called to retrieve
> > the operating frequency for a device and
> omap_device_register_dvfs_callbacks
> > to register the device specific set_rate and get_rate functions.
> > The omap_device_set_rate and omap_device_get_rate does some
> routine error
> > checks and finally calls into the device specific set_rate
> > and get_rate APIs populated through omap_device_populate_rate_fns.
> >
> > Signed-off-by: Thara Gopinath <thara@ti.com>
> > Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> > ---
> >  arch/arm/plat-omap/include/plat/omap_device.h |    9 +++++
> >  arch/arm/plat-omap/omap_device.c              |   49
> +++++++++++++++++++++++++
> >  2 files changed, 58 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/include/plat/omap_device.h
> b/arch/arm/plat-omap/include/plat/omap_device.h
> > index e4c349f..204fb0a 100644
> > --- a/arch/arm/plat-omap/include/plat/omap_device.h
> > +++ b/arch/arm/plat-omap/include/plat/omap_device.h
> > @@ -50,6 +50,8 @@ extern struct device omap_device_parent;
> >   * @hwmods: (one .. many per omap_device)
> >   * @hwmods_cnt: ARRAY_SIZE() of @hwmods
> >   * @pm_lats: ptr to an omap_device_pm_latency table
> > + * @set_rate: fn ptr to change the operating rate.
> > + * @get_rate: fn ptr to retrieve the current operating rate.
> >   * @pm_lats_cnt: ARRAY_SIZE() of what is passed to @pm_lats
> >   * @pm_lat_level: array index of the last odpl entry executed - -1 if
> never
> >   * @dev_wakeup_lat: dev wakeup latency in nanoseconds
> > @@ -73,6 +75,8 @@ struct omap_device {
> >  	s8				pm_lat_level;
> >  	u8				hwmods_cnt;
> >  	u8				_state;
> > +	int (*set_rate)(struct device *dev, unsigned long rate);
> > +	unsigned long (*get_rate) (struct device *dev);
> >  };
> >
> >  /* Device driver interface (call via platform_data fn ptrs) */
> > @@ -107,6 +111,11 @@ void __iomem
> *omap_device_get_rt_va(struct omap_device *od);
> >  int omap_device_align_pm_lat(struct platform_device *pdev,
> >  			     u32 new_wakeup_lat_limit);
> >  struct powerdomain *omap_device_get_pwrdm(struct omap_device
> *od);
> > +int omap_device_set_rate(struct device *dev, unsigned long freq);
> > +unsigned long omap_device_get_rate(struct device *dev);
> > +void omap_device_register_dvfs_callbacks(struct device *dev,
> > +		int (*set_rate)(struct device *dev, unsigned long rate),
> > +		unsigned long (*get_rate) (struct device *dev));
> >  u32 omap_device_get_context_loss_count(struct platform_device
> *pdev);
> >
> >  /* Other */
> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-
> omap/omap_device.c
> > index a84e849..4cee430 100644
> > --- a/arch/arm/plat-omap/omap_device.c
> > +++ b/arch/arm/plat-omap/omap_device.c
> > @@ -810,6 +810,55 @@ int omap_device_enable_clocks(struct
> omap_device *od)
> >  	return 0;
> >  }
> >
> > +int omap_device_set_rate(struct device *dev, unsigned long freq)
> > +{
> > +	struct platform_device *pdev;
> > +	struct omap_device *od;
> > +
> > +	pdev = container_of(dev, struct platform_device, dev);
>
> There is a macro for this in device.h:
>
>       pdev = to_platform_device(dev)
>
> This needs to be fixed all over this series.
OK. Will fix it in next vesion.
>
> > +	od = _find_by_pdev(pdev);
> > +
> > +	if (!od->set_rate) {
> > +		dev_err(dev, "%s: No set_rate API for scaling device\n",
> > +			__func__);
> > +		return -ENODATA;
> > +	}
> > +
> > +	return od->set_rate(dev, freq);
> > +}
> > +
> > +unsigned long omap_device_get_rate(struct device *dev)
> > +{
> > +	struct platform_device *pdev;
> > +	struct omap_device *od;
> > +
> > +	pdev = container_of(dev, struct platform_device, dev);
> > +	od = _find_by_pdev(pdev);
>
> Should also check here for a valid omap_device, but making sure its
> parent is omap_device_parent.
OK
>
> > +
> > +
> > +	if (!od->get_rate) {
> > +		dev_err(dev, "%s: No get rate API for the device\n",
> > +			__func__);
> > +		return 0;
> > +	}
> > +
> > +	return od->get_rate(dev);
> > +}
> > +
> > +void omap_device_register_dvfs_callbacks(struct device *dev,
> > +		int (*set_rate)(struct device *dev, unsigned long rate),
> > +		unsigned long (*get_rate) (struct device *dev))
> > +{
> > +	struct platform_device *pdev;
> > +	struct omap_device *od;
> > +
> > +	pdev = container_of(dev, struct platform_device, dev);
> > +	od = _find_by_pdev(pdev);
> > +
>
> and here
OK

Vishwa
>
> > +	od->set_rate = set_rate;
> > +	od->get_rate = get_rate;
> > +}
> > +
> >  struct device omap_device_parent = {
> >  	.init_name	= "omap",
> >  	.parent         = &platform_bus,
>
> Kevin