Hi,
On Wed, Jul 04, 2018 at 11:51:30AM +0900, Chanwoo Choi wrote:
> Hi,
>
> On 2018년 07월 04일 08:46, Matthias Kaehlcke wrote:
> > Move variables related with devfreq policy changes from struct devfreq
> > to the new struct devfreq_policy and add a policy field to struct devfreq.
> >
> > The following variables are moved:
> >
> > df->min/max_freq => p->user.min/max_freq
> > df->scaling_min/max_freq => p->devinfo.min/max_freq
> > df->governor => p->governor
> > df->governor_name => p->governor_name
> >
> > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> > ---
> > Changes in v5:
> > - none
> >
> > Changes in v4:
> > - added 'Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>' tag
> >
> > Changes in v3:
> > - none
> >
> > Changes in v2:
> > - performance, powersave and simpleondemand governors don't need changes
> > with "PM / devfreq: Don't adjust to user limits in governors"
> > - formatting fixes
> > ---
> > drivers/devfreq/devfreq.c | 137 ++++++++++++++++-------------
> > drivers/devfreq/governor_passive.c | 4 +-
> > include/linux/devfreq.h | 38 +++++---
> > 3 files changed, 103 insertions(+), 76 deletions(-)
> >
>
> (skip)
>
> >
> > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> > index 3bc29acbd54e..e0987c749ec2 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -99,12 +99,12 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> > {
> > int ret;
> >
> > - if (!devfreq->governor)
> > + if (!devfreq->policy.governor)
> > return -EINVAL;
> >
> > mutex_lock_nested(&devfreq->lock, SINGLE_DEPTH_NESTING);
> >
> > - ret = devfreq->governor->get_target_freq(devfreq, &freq);
> > + ret = devfreq->policy.governor->get_target_freq(devfreq, &freq);
> > if (ret < 0)
> > goto out;
> >
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index 3aae5b3af87c..9bf23b976f4d 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -109,6 +109,30 @@ struct devfreq_dev_profile {
> > unsigned int max_state;
> > };
> >
> > +/**
> > + * struct devfreq_freq_limits - Devfreq frequency limits
> > + * @min_freq: minimum frequency
> > + * @max_freq: maximum frequency
> > + */
> > +struct devfreq_freq_limits {
> > + unsigned long min_freq;
> > + unsigned long max_freq;
> > +};
> > +
> > +/**
> > + * struct devfreq_policy - Devfreq policy
> > + * @user: frequency limits requested by the user
> > + * @devinfo: frequency limits of the device (available OPPs)
> > + * @governor: method how to choose frequency based on the usage.
>
> nitpick. remove '.' on the end of line.
Ok
> > + * @governor_name: devfreq governor name for use with this devfreq
> > + */
> > +struct devfreq_policy {
> > + struct devfreq_freq_limits user;
> > + struct devfreq_freq_limits devinfo;
> > + const struct devfreq_governor *governor;
> > + char governor_name[DEVFREQ_NAME_LEN];
> > +};
> > +
> > /**
> > * struct devfreq - Device devfreq structure
> > * @node: list node - contains the devices with devfreq that have been
> > @@ -117,8 +141,6 @@ struct devfreq_dev_profile {
> > * @dev: device registered by devfreq class. dev.parent is the device
> > * using devfreq.
> > * @profile: device-specific devfreq profile
> > - * @governor: method how to choose frequency based on the usage.
> > - * @governor_name: devfreq governor name for use with this devfreq
> > * @nb: notifier block used to notify devfreq object that it should
> > * reevaluate operable frequencies. Devfreq users may use
> > * devfreq.nb to the corresponding register notifier call chain.
> > @@ -126,10 +148,7 @@ struct devfreq_dev_profile {
> > * @previous_freq: previously configured frequency value.
> > * @data: Private data of the governor. The devfreq framework does not
> > * touch this.
> > - * @min_freq: Limit minimum frequency requested by user (0: none)
> > - * @max_freq: Limit maximum frequency requested by user (0: none)
> > - * @scaling_min_freq: Limit minimum frequency requested by OPP interface
> > - * @scaling_max_freq: Limit maximum frequency requested by OPP interface
> > + * @policy: Policy for frequency adjustments
>
> The devfreq_policy contains the range of frequency and governor information.
> But, this description focus on the frequency. You need to explain the more
> correct description of 'policy'.
I wouldn't say that the focus is on 'frequency', but on 'frequency
adjustments', and the governor is an integral part of them.
I can change it to "Policy for frequency adjustments, including
frequency limits and the governor" if you prefer. I'm open to other
suggestions.
> > * @stop_polling: devfreq polling status of a device.
> > * @total_trans: Number of devfreq transitions
> > * @trans_table: Statistics of devfreq transitions
> > @@ -151,8 +170,6 @@ struct devfreq {
> > struct mutex lock;
> > struct device dev;
> > struct devfreq_dev_profile *profile;
> > - const struct devfreq_governor *governor;
> > - char governor_name[DEVFREQ_NAME_LEN];
> > struct notifier_block nb;
> > struct delayed_work work;
> >
> > @@ -161,10 +178,7 @@ struct devfreq {
> >
> > void *data; /* private data for governors */
> >
> > - unsigned long min_freq;
> > - unsigned long max_freq;
> > - unsigned long scaling_min_freq;
> > - unsigned long scaling_max_freq;
> > + struct devfreq_policy policy;
>
> I recommend that you better to move under 'struct devfreq_dev_profile'
> as following:
>
> struct devfreq_dev_profile *profile;
> struct devfreq_policy policy;
Will do
Thanks for the review!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
