From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiaoguang Chen Subject: Re: [PATCH 1/2] PM: devfreq: add freq table and available_freqs Date: Thu, 14 Jun 2012 09:50:25 +0800 Message-ID: References: <16928680.915361339563225913.JavaMail.weblogic@epml04> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=e89a8f923de466686a04c264ece8 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: myungjoo.ham@samsung.com Cc: =?UTF-8?B?67CV6rK966+8?= , Xiaoguang Chen , "linux-kernel@vger.kernel.org" , linux-pm@lists.linux-foundation.org List-Id: linux-pm@vger.kernel.org --e89a8f923de466686a04c264ece8 Content-Type: text/plain; charset=ISO-8859-1 Hi, Myungjoo what's your opinion? Thanks Xiaoguang 2012/6/13 Xiaoguang Chen > I think Devfreq should not be combined with OPP, > OPP framework does contain one frequency table, but the frequency is > combined with voltage. some platforms may don't want to use this but > handling voltage seperately in their clock driver. > > and some platforms don't use OPP, and they want a frequency list. > then this is necessary. also devfreq should contain a frequency list even > without any other frameworks, don't you think so ? > > Thanks > Xiaoguang > > > 2012/6/13 MyungJoo Ham > >> > Devfreq framework don't have a frequency table, add it >> > for easy use. >> > >> > Signed-off-by: Xiaoguang Chen >> >> If you need a predefined data structure to support frequency table, >> you can simply use OPP, which has helper functions implemented in >> devfreq subsystem. Is there any reason not to use OPP and to implement >> another data structure to store a frequency table attached to a device? >> >> >> Cheers! >> MyungJoo. >> >> > --- >> > drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++ >> > include/linux/devfreq.h | 12 ++++++++++++ >> > 2 files changed, 38 insertions(+), 0 deletions(-) >> > >> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> > index 70c31d4..2144200 100644 >> > --- a/drivers/devfreq/devfreq.c >> > +++ b/drivers/devfreq/devfreq.c >> > @@ -460,6 +460,17 @@ int devfreq_remove_device(struct devfreq *devfreq) >> > return 0; >> > } >> > >> > +/* >> > + * devfreq_set_freq_table()- Set frequency table for devfreq >> > + * @devfreq The devfreq instance >> > + * @table The frequency table that device supports >> > + */ >> > +void devfreq_set_freq_table(struct devfreq *devfreq, >> > + struct devfreq_frequency_table *table) >> > +{ >> > + devfreq->freq_table = table; >> > +} >> > + >> > static ssize_t show_governor(struct device *dev, >> > struct device_attribute *attr, char *buf) >> > { >> > @@ -472,6 +483,20 @@ static ssize_t show_freq(struct device *dev, >> > return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq); >> > } >> > >> > +static ssize_t show_avail_freq(struct device *dev, >> > + struct device_attribute *attr, char *buf) >> > +{ >> > + int len = 0, i; >> > + struct devfreq *devfreq = to_devfreq(dev); >> > + if (devfreq->freq_table) >> > + for (i = 0; devfreq->freq_table[i].frequency != >> DEVFREQ_TABLE_END; i++) >> > + len += sprintf(buf + len, "%lu\n", >> > + devfreq->freq_table[i].frequency); >> > + if (len == 0) >> > + len += sprintf(buf + len, "No frequency table is >> provided\n"); >> > + return len; >> > +} >> > + >> > static ssize_t show_polling_interval(struct device *dev, >> > struct device_attribute *attr, char >> *buf) >> > { >> > @@ -595,6 +620,7 @@ static struct device_attribute devfreq_attrs[] = { >> > store_polling_interval), >> > __ATTR(min_freq, S_IRUGO | S_IWUSR, show_min_freq, >> store_min_freq), >> > __ATTR(max_freq, S_IRUGO | S_IWUSR, show_max_freq, >> store_max_freq), >> > + __ATTR(available_freqs, S_IRUGO, show_avail_freq, NULL), >> > { }, >> > }; >> > >> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> > index 281c72a..e5e4036 100644 >> > --- a/include/linux/devfreq.h >> > +++ b/include/linux/devfreq.h >> > @@ -52,6 +52,14 @@ struct devfreq_dev_status { >> > */ >> > #define DEVFREQ_FLAG_LEAST_UPPER_BOUND 0x1 >> > >> > +#define DEVFREQ_ENTRY_INVALID (~0) >> > +#define DEVFREQ_TABLE_END (~1) >> > + >> > +struct devfreq_frequency_table { >> > + unsigned int index; >> > + unsigned long frequency; >> > +}; >> > + >> > /** >> > * struct devfreq_dev_profile - Devfreq's user device profile >> > * @initial_freq The operating frequency when devfreq_add_device() >> is >> > @@ -130,6 +138,7 @@ struct devfreq_governor { >> > * "devfreq_monitor" executions to reevaluate >> > * frequency/voltage of the device. Set by >> > * profile's polling_ms interval. >> > + * @freq_table The frequency table that device supports >> > * @data Private data of the governor. The devfreq framework does >> not >> > * touch this. >> > * @being_removed a flag to mark that this object is being removed >> in >> > @@ -157,6 +166,7 @@ struct devfreq { >> > unsigned long polling_jiffies; >> > unsigned long previous_freq; >> > unsigned int next_polling; >> > + struct devfreq_frequency_table *freq_table; >> > >> > void *data; /* private data for governors */ >> > >> > @@ -180,6 +190,8 @@ extern int devfreq_register_opp_notifier(struct >> device *dev, >> > struct devfreq *devfreq); >> > extern int devfreq_unregister_opp_notifier(struct device *dev, >> > struct devfreq *devfreq); >> > +extern void devfreq_set_freq_table(struct devfreq *devfreq, >> > + struct devfreq_frequency_table *table); >> > >> > #ifdef CONFIG_DEVFREQ_GOV_POWERSAVE >> > extern const struct devfreq_governor devfreq_powersave; >> > -- >> > 1.7.0.4 >> > >> > > --e89a8f923de466686a04c264ece8 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Hi, Myungjoo

what's your=A0opinion?

Thanks
Xiaoguang

2012/= 6/13 Xiaoguang Chen <chenxg.marvell@gmail.com>
I think Devfreq should not be combined with = OPP,=A0
OPP framework does contain one frequency table, but the frequen= cy is combined with voltage. some platforms may don't want to use this = but handling voltage seperately in their clock driver.

and some platforms don't use OPP, and they want a f= requency list.
then this is necessary. also devfreq should contain a fr= equency list even without any other frameworks, don't you think so ?

Thanks
Xiaoguang


2012/6/13 MyungJoo Ham <myungjoo.ham@samsung.com>
If you need a predefined data structure to support frequency table, you can simply use OPP, which has helper functions implemented in
devfreq subsystem. Is there any reason not to use OPP and to implement
another data structure to store a frequency table attached to a device?


Cheers!
MyungJoo.

> ---
> =A0drivers/devfreq/devfreq.c | =A0 26 ++++++++++++++++++++++++++
> =A0include/linux/devfreq.h =A0 | =A0 12 ++++++++++++
> =A02 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 70c31d4..2144200 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -460,6 +460,17 @@ int devfreq_remove_device(struct devfreq *devfreq= )
> =A0 =A0 =A0 return 0;
> =A0}
>
> +/*
> + * devfreq_set_freq_table()- Set frequency table for devfreq
> + * @devfreq =A0The devfreq instance
> + * @table =A0 =A0The frequency table that device supports
> + */
> +void devfreq_set_freq_table(struct devfreq *devfreq,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct devfreq_frequ= ency_table *table)
> +{
> + =A0 =A0 devfreq->freq_table =3D table;
> +}
> +
> =A0static ssize_t show_governor(struct device *dev,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct device_a= ttribute *attr, char *buf)
> =A0{
> @@ -472,6 +483,20 @@ static ssize_t show_freq(struct device *dev,
> =A0 =A0 =A0 return sprintf(buf, "%lu\n", to_devfreq(dev)->= ;previous_freq);
> =A0}
>
> +static ssize_t show_avail_freq(struct device *dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct device= _attribute *attr, char *buf)
> +{
> + =A0 =A0 int len =3D 0, i;
> + =A0 =A0 struct devfreq *devfreq =3D to_devfreq(dev);
> + =A0 =A0 if (devfreq->freq_table)
> + =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; devfreq->freq_table[i].freq= uency !=3D DEVFREQ_TABLE_END; i++)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 len +=3D sprintf(buf + len, = "%lu\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0devfreq->freq_table[i].frequency);
> + =A0 =A0 if (len =3D=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 len +=3D sprintf(buf + len, "No frequen= cy table is provided\n");
> + =A0 =A0 return len;
> +}
> +
> =A0static ssize_t show_polling_interval(struct device *dev,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0struct device_attribute *attr, char *buf)
> =A0{
> @@ -595,6 +620,7 @@ static struct device_attribute devfreq_attrs[] =3D= {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0store_polling_interval),
> =A0 =A0 =A0 __ATTR(min_freq, S_IRUGO | S_IWUSR, show_min_freq, store_m= in_freq),
> =A0 =A0 =A0 __ATTR(max_freq, S_IRUGO | S_IWUSR, show_max_freq, store_m= ax_freq),
> + =A0 =A0 __ATTR(available_freqs, S_IRUGO, show_avail_freq, NULL),
> =A0 =A0 =A0 { },
> =A0};
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 281c72a..e5e4036 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -52,6 +52,14 @@ struct devfreq_dev_status {
> =A0 */
> =A0#define DEVFREQ_FLAG_LEAST_UPPER_BOUND =A0 =A0 =A0 =A0 =A0 =A0 =A0 = 0x1
>
> +#define DEVFREQ_ENTRY_INVALID (~0)
> +#define DEVFREQ_TABLE_END =A0 =A0 (~1)
> +
> +struct devfreq_frequency_table {
> + =A0 =A0 unsigned int index;
> + =A0 =A0 unsigned long frequency;
> +};
> +
> =A0/**
> =A0 * struct devfreq_dev_profile - Devfreq's user device profile > =A0 * @initial_freq =A0 =A0 The operating frequency when devfreq_add_d= evice() is
> @@ -130,6 +138,7 @@ struct devfreq_governor {
> =A0 * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "devfreq_monitor" = executions to reevaluate
> =A0 * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 frequency/voltage of the dev= ice. Set by
> =A0 * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 profile's polling_ms int= erval.
> + * @freq_table =A0 =A0 =A0 The frequency table that device supports > =A0 * @data =A0 =A0 Private data of the governor. The devfreq framewor= k does not
> =A0 * =A0 =A0 =A0 =A0 =A0 touch this.
> =A0 * @being_removed =A0 =A0a flag to mark that this object is being r= emoved in
> @@ -157,6 +166,7 @@ struct devfreq {
> =A0 =A0 =A0 unsigned long polling_jiffies;
> =A0 =A0 =A0 unsigned long previous_freq;
> =A0 =A0 =A0 unsigned int next_polling;
> + =A0 =A0 struct devfreq_frequency_table *freq_table;
>
> =A0 =A0 =A0 void *data; /* private data for governors */
>
> @@ -180,6 +190,8 @@ extern int devfreq_register_opp_notifier(struct de= vice *dev,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0struct devfreq *devfreq);
> =A0extern int devfreq_unregister_opp_notifier(struct device *dev,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0struct devfreq *devfreq);
> +extern void devfreq_set_freq_table(struct devfreq *devfreq,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struc= t devfreq_frequency_table *table);
>
> =A0#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
> =A0extern const struct devfreq_governor devfreq_powersave;
> --
> 1.7.0.4
>


--e89a8f923de466686a04c264ece8--