| Message ID | 20201023102632.740-1-cw00.choi@samsung.com |
|---|---|
| Headers | show |
| Series | PM / devfreq: Add governor feature and attribute flag | expand |
23.10.2020 13:26, Chanwoo Choi пишет: > @@ -909,6 +915,8 @@ struct devfreq *devfreq_add_device(struct device *dev, > goto err_init; > } > > + create_sysfs_files(devfreq, governor); > + > devfreq->governor = governor; > err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, > NULL); Shouldn't sysfs be created *after* GOV_START? This is inconsistent with governor_store().
23.10.2020 13:26, Chanwoo Choi пишет: > @@ -1401,8 +1423,13 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, > "%s: reverting to Governor %s failed (%d)\n", > __func__, df->governor_name, ret); > df->governor = NULL; > + goto out; > } ... > + create_sysfs_files(df, df->governor); > + goto out; These two lines could be removed. > } > + create_sysfs_files(df, df->governor); > + > out: > mutex_unlock(&devfreq_list_lock); Otherwise looks good to me. Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Tested-by: Dmitry Osipenko <digetx@gmail.com>
On 10/26/20 1:30 AM, Dmitry Osipenko wrote: > 23.10.2020 13:26, Chanwoo Choi пишет: >> @@ -909,6 +915,8 @@ struct devfreq *devfreq_add_device(struct device *dev, >> goto err_init; >> } >> >> + create_sysfs_files(devfreq, governor); >> + >> devfreq->governor = governor; >> err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START, >> NULL); > > Shouldn't sysfs be created *after* GOV_START? This is inconsistent with > governor_store(). > > Good point. Thanks for the review. I'll edit it.
On 10/26/20 1:31 AM, Dmitry Osipenko wrote: > 23.10.2020 13:26, Chanwoo Choi пишет: >> @@ -1401,8 +1423,13 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, >> "%s: reverting to Governor %s failed (%d)\n", >> __func__, df->governor_name, ret); >> df->governor = NULL; >> + goto out; >> } > ... >> + create_sysfs_files(df, df->governor); >> + goto out; > > These two lines could be removed. Right. These are redundant code. It is possible to support with just fallback style. > >> } >> + create_sysfs_files(df, df->governor); >> + >> out: >> mutex_unlock(&devfreq_list_lock); > > Otherwise looks good to me. > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > Tested-by: Dmitry Osipenko <digetx@gmail.com> > > -- Best Regards, Chanwoo Choi Samsung Electronics
Each devfreq governor can have the different sysfs attributes and features. In order to provide the only available sysfs attribute to user-space, add governor attribute flag with DEVFREQ_GOV_ATTR_[attribute name] defintion. Also, each governor is able to have the specific flag in order to support specific feature with DEVFREQ_GOV_FLAG_[feature name] defintion like immutable governor. According to each governor, can initiate the governor feature and attribute flags. [Common sysfs attributes for devfreq class] And all devfreq governors have to support the following common attributes. The common attributes are added to devfreq class by default. - governor - available_governors - available_frequencies - cur_freq - target_freq - min_freq - max_freq - trans_stat [Definition for governor attribute flag] - DEVFREQ_GOV_ATTR_POLLING_INTERVAL to update polling interval for timer. : /sys/class/devfreq/[devfreq dev name]/polling_interval - DEVFREQ_GOV_ATTR_TIMER to change the type of timer on either deferrable or dealyed timer. : /sys/class/devfreq/[devfreq dev name]/timer [Definition for governor feature flag] - DEVFREQ_GOV_FLAG_IMMUTABLE : If immutable flag is set, governor is never changeable to other governors. - DEVFREQ_GOV_FLAG_IRQ_DRIVEN : Devfreq core won't schedule polling work for this governor if value is set. [Table of governor attribute flags for evfreq governors] ----------------------------------------------------------------------------- | simple | perfor | power | user | passive | tegra30 | ondemand | mance | save | space| | ------------------------------------------------------------------------------ governor | O | O | O | O | O | O available_governors | O | O | O | O | O | O available_frequencies | O | O | O | O | O | O cur_freq | O | O | O | O | O | O target_freq | O | O | O | O | O | O min_freq | O | O | O | O | O | O max_freq | O | O | O | O | O | O trans_stat | O | O | O | O | O | O ------------------------------------------------------------------------------ polling_interval | O | X | X | X | X | O timer | O | X | X | X | X | X ------------------------------------------------------------------------------ immutable | X | X | X | X | O | O interrupt_driven | X(polling)| X | X | X | X | O (irq) ------------------------------------------------------------------------------ Changes from v4: - Rename from 'attr' to 'attrs' - Restore the variable name in governor_store because it is enought to explain the previous or new governor with detailed comments instead of variable name changes. Changes from v3: - Fix typo - Rename from 'flag' to 'flags' - Add more exception handling code and add comments on governor_store() Changes from v2: - Hide unsupported sysfs node to user-space instead of checking the permission of sysfs node. Chanwoo Choi (2): PM / devfreq: Add governor feature flag PM / devfreq: Add governor attribute flag for specifc sysfs nodes Documentation/ABI/testing/sysfs-class-devfreq | 54 ++--- drivers/devfreq/devfreq.c | 185 ++++++++++++------ drivers/devfreq/governor.h | 30 ++- drivers/devfreq/governor_passive.c | 2 +- drivers/devfreq/governor_simpleondemand.c | 2 + drivers/devfreq/tegra30-devfreq.c | 5 +- 6 files changed, 186 insertions(+), 92 deletions(-)