Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

2020-10-19 Thread Chanwoo Choi 
On 10/20/20 7:39 AM, Dmitry Osipenko wrote:
> 19.10.2020 06:53, Chanwoo Choi пишет:
> ...
 +  const u64 flag;
>>> A plural form of flag(s) is more common, IMO.
>>
>> When need to add more feature flag, I prefer to add
>> the definition instead of changing the structure.
>> I think it is better.
> 
> I meant to rename the new member "flag" to "flags".

I misunderstood. I'll edit it.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

2020-10-19 Thread Dmitry Osipenko 
19.10.2020 06:53, Chanwoo Choi пишет:
...
>>> +   const u64 flag;
>> A plural form of flag(s) is more common, IMO.
> 
> When need to add more feature flag, I prefer to add
> the definition instead of changing the structure.
> I think it is better.

I meant to rename the new member "flag" to "flags".

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

2020-10-18 Thread Chanwoo Choi 
On 10/19/20 9:57 AM, Dmitry Osipenko wrote:
> 07.10.2020 08:07, Chanwoo Choi пишет:
>> The devfreq governor is able to have the specific flag as follows
>> in order to implement the specific feature. For example, devfreq allows
>> user to change the governors on runtime via sysfs interface.
>> But, if devfreq device uses 'passive' governor, don't allow user to change
>> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
> 
> s/DEVFREQ_GOV_FLAT/DEVFREQ_GOV_FLAG/
> 
> ...
>>  /**
>>   * struct devfreq_governor - Devfreq policy governor
>>   * @node:   list node - contains registered devfreq governors
>>   * @name:   Governor's name
>> - * @immutable:  Immutable flag for governor. If the value is 1,
>> - *  this governor is never changeable to other governor.
>> - * @interrupt_driven:   Devfreq core won't schedule polling work for 
>> this
>> - *  governor if value is set to 1.
>> + * @flag:   Governor's feature flag
>>   * @get_target_freq:Returns desired operating frequency for the 
>> device.
>>   *  Basically, get_target_freq will run
>>   *  devfreq_dev_profile.get_dev_status() to get the
>> @@ -50,8 +57,7 @@ struct devfreq_governor {
>>  struct list_head node;
>>  
>>  const char name[DEVFREQ_NAME_LEN];
>> -const unsigned int immutable;
>> -const unsigned int interrupt_driven;
>> +const u64 flag;
> A plural form of flag(s) is more common, IMO.

When need to add more feature flag, I prefer to add
the definition instead of changing the structure.
I think it is better.

> 
> It's also possible to use a single bit:1 for the struct members. Thus,
> could you please explain what are the benefits of the "flag"?

I think that anyone might add the some optional
feature. So, I used 'flag' for the extensibility.



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 1/2] PM / devfreq: Add governor feature flag

2020-10-18 Thread Dmitry Osipenko 
07.10.2020 08:07, Chanwoo Choi пишет:
> The devfreq governor is able to have the specific flag as follows
> in order to implement the specific feature. For example, devfreq allows
> user to change the governors on runtime via sysfs interface.
> But, if devfreq device uses 'passive' governor, don't allow user to change
> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE

s/DEVFREQ_GOV_FLAT/DEVFREQ_GOV_FLAG/

...
>  /**
>   * struct devfreq_governor - Devfreq policy governor
>   * @node:list node - contains registered devfreq governors
>   * @name:Governor's name
> - * @immutable:   Immutable flag for governor. If the value is 1,
> - *   this governor is never changeable to other governor.
> - * @interrupt_driven:Devfreq core won't schedule polling work for 
> this
> - *   governor if value is set to 1.
> + * @flag:Governor's feature flag
>   * @get_target_freq: Returns desired operating frequency for the device.
>   *   Basically, get_target_freq will run
>   *   devfreq_dev_profile.get_dev_status() to get the
> @@ -50,8 +57,7 @@ struct devfreq_governor {
>   struct list_head node;
>  
>   const char name[DEVFREQ_NAME_LEN];
> - const unsigned int immutable;
> - const unsigned int interrupt_driven;
> + const u64 flag;
A plural form of flag(s) is more common, IMO.

It's also possible to use a single bit:1 for the struct members. Thus,
could you please explain what are the benefits of the "flag"?

[PATCH v3 1/2] PM / devfreq: Add governor feature flag

2020-10-06 Thread Chanwoo Choi 
The devfreq governor is able to have the specific flag as follows
in order to implement the specific feature. For example, devfreq allows
user to change the governors on runtime via sysfs interface.
But, if devfreq device uses 'passive' governor, don't allow user to change
the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
and set it to flag of passive governor.

[Definition for governor 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 flag for devfreq governors]
--
  | simple| perfor | power | user | passive | tegra30
  | ondemand  | mance  | save  | space| |
--
immutable | X | X  | X | X| O   | O
interrupt_driven  | X(polling)| X  | X | X| X   | O (irq)
--

Signed-off-by: Chanwoo Choi 
---
 drivers/devfreq/devfreq.c  | 18 ++
 drivers/devfreq/governor.h | 18 --
 drivers/devfreq/governor_passive.c |  2 +-
 drivers/devfreq/tegra30-devfreq.c  |  4 ++--
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 861c100f9fac..ce793fc9a558 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -31,6 +31,7 @@
 #define CREATE_TRACE_POINTS
 #include 
 
+#define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : 
false)
 #define HZ_PER_KHZ 1000
 
 static struct class *devfreq_class;
@@ -456,7 +457,7 @@ static void devfreq_monitor(struct work_struct *work)
  */
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
-   if (devfreq->governor->interrupt_driven)
+   if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
return;
 
switch (devfreq->profile->timer) {
@@ -486,7 +487,7 @@ EXPORT_SYMBOL(devfreq_monitor_start);
  */
 void devfreq_monitor_stop(struct devfreq *devfreq)
 {
-   if (devfreq->governor->interrupt_driven)
+   if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
return;
 
cancel_delayed_work_sync(>work);
@@ -517,7 +518,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
devfreq->stop_polling = true;
mutex_unlock(>lock);
 
-   if (devfreq->governor->interrupt_driven)
+   if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
return;
 
cancel_delayed_work_sync(>work);
@@ -540,7 +541,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
if (!devfreq->stop_polling)
goto out;
 
-   if (devfreq->governor->interrupt_driven)
+   if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
goto out_update;
 
if (!delayed_work_pending(>work) &&
@@ -580,7 +581,7 @@ void devfreq_update_interval(struct devfreq *devfreq, 
unsigned int *delay)
if (devfreq->stop_polling)
goto out;
 
-   if (devfreq->governor->interrupt_driven)
+   if (IS_SUPPORTED_FLAG(devfreq->governor->flag, IRQ_DRIVEN))
goto out;
 
/* if new delay is zero, stop polling */
@@ -1347,7 +1348,8 @@ static ssize_t governor_store(struct device *dev, struct 
device_attribute *attr,
if (df->governor == governor) {
ret = 0;
goto out;
-   } else if (df->governor->immutable || governor->immutable) {
+   } else if (IS_SUPPORTED_FLAG(df->governor->flag, IMMUTABLE)
+   || IS_SUPPORTED_FLAG(governor->flag, IMMUTABLE)) {
ret = -EINVAL;
goto out;
}
@@ -1402,7 +1404,7 @@ static ssize_t available_governors_show(struct device *d,
 * The devfreq with immutable governor (e.g., passive) shows
 * only own governor.
 */
-   if (df->governor->immutable) {
+   if (IS_SUPPORTED_FLAG(df->governor->flag, IMMUTABLE)) {
count = scnprintf([count], DEVFREQ_NAME_LEN,
  "%s ", df->governor_name);
/*
@@ -1413,7 +1415,7 @@ static ssize_t available_governors_show(struct device *d,
struct devfreq_governor *governor;
 
list_for_each_entry(governor, _governor_list, node) {
-   if (governor->immutable)
+   if (IS_SUPPORTED_FLAG(governor->flag, IMMUTABLE))
continue;
count += scnprintf([count], (PAGE_SIZE - count - 2),
   "%s ", governor->name);
diff --git a/drivers/devfreq/governor.h