Re: [PATCH V2 4/4] cpufreq: Get rid of "struct global_attr"

2013-02-21 Thread Viresh Kumar
On 22 February 2013 08:03, Rafael J. Wysocki  wrote:
> On Friday, February 22, 2013 07:47:44 AM Viresh Kumar wrote:
>> On 22 February 2013 05:15, Rafael J. Wysocki  wrote:
>> > Why did you change all of the lines of this macro instead of changing just 
>> > the
>> > one line you needed to change?
>>
>> I didn't like the indentation used within the macro. So did it.
>
> In general, things like that are for separate cleanup patches.  If you mix
> functional changes with cleanups, poeple get confused and it's difficult to 
> see
> what's needed and what's "optional".
>
> I know it's tempting to fix stuff like that along with doing functional
> changes and I do that sometimes.  Not very often, though, and with care.

Even i give similar comments sometimes but forget these while writing my
patches :)

Anyway, fixup:

commit b1bbb99467d56140cf3a8a2b70e61b456aa46e48
Author: Viresh Kumar 
Date:   Fri Feb 22 07:59:20 2013 +0530

fixup! cpufreq: Get rid of "struct global_attr"
---
 drivers/cpufreq/intel_pstate.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index e795134..49846b9 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -273,12 +273,12 @@ static void intel_pstate_debug_expose_params(void)
 /** debugfs end /

 /** sysfs begin /
-#define show_one(file_name, object)\
-static ssize_t show_##file_name\
-(struct cpufreq_policy *policy, char *buf) \
-{  \
-   return sprintf(buf, "%u\n", limits.object); \
-}
+#define show_one(file_name, object)\
+   static ssize_t show_##file_name \
+   (struct cpufreq_policy *policy, char *buf)  \
+   {   \
+   return sprintf(buf, "%u\n", limits.object); \
+   }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V2 4/4] cpufreq: Get rid of "struct global_attr"

2013-02-21 Thread Rafael J. Wysocki
On Friday, February 22, 2013 07:47:44 AM Viresh Kumar wrote:
> On 22 February 2013 05:15, Rafael J. Wysocki  wrote:
> > Why did you change all of the lines of this macro instead of changing just 
> > the
> > one line you needed to change?
> 
> I didn't like the indentation used within the macro. So did it.

In general, things like that are for separate cleanup patches.  If you mix
functional changes with cleanups, poeple get confused and it's difficult to see
what's needed and what's "optional".

I know it's tempting to fix stuff like that along with doing functional
changes and I do that sometimes.  Not very often, though, and with care.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V2 4/4] cpufreq: Get rid of "struct global_attr"

2013-02-21 Thread Viresh Kumar
On 22 February 2013 05:15, Rafael J. Wysocki  wrote:
> Why did you change all of the lines of this macro instead of changing just the
> one line you needed to change?

I didn't like the indentation used within the macro. So did it.

> Please don't do that.

Okay.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V2 4/4] cpufreq: Get rid of "struct global_attr"

2013-02-21 Thread Rafael J. Wysocki
On Monday, February 11, 2013 01:20:03 PM Viresh Kumar wrote:
> We don't need to keep two structures for file attributes, global_attr and
> freq_attr. Lets use freq_attr only for cpufreq core and drivers.
> 
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/cpufreq/acpi-cpufreq.c |  9 -
>  drivers/cpufreq/intel_pstate.c | 30 +++---
>  include/linux/cpufreq.h| 16 
>  3 files changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 937bc28..14219c3 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -160,19 +160,18 @@ static ssize_t _store_boost(const char *buf, size_t 
> count)
>   return count;
>  }
>  
> -static ssize_t store_global_boost(struct kobject *kobj, struct attribute 
> *attr,
> -   const char *buf, size_t count)
> +static ssize_t store_global_boost(struct cpufreq_policy *policy,
> + const char *buf, size_t count)
>  {
>   return _store_boost(buf, count);
>  }
>  
> -static ssize_t show_global_boost(struct kobject *kobj,
> -  struct attribute *attr, char *buf)
> +static ssize_t show_global_boost(struct cpufreq_policy *policy, char *buf)
>  {
>   return sprintf(buf, "%u\n", boost_enabled);
>  }
>  
> -static struct global_attr global_boost = __ATTR(boost, 0644,
> +static struct freq_attr global_boost = __ATTR(boost, 0644,
>   show_global_boost,
>   store_global_boost);
>  
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 86ad482..0f2d6a0 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -273,15 +273,15 @@ static void intel_pstate_debug_expose_params(void)
>  /** debugfs end /
>  
>  /** sysfs begin /
> -#define show_one(file_name, object)  \
> - static ssize_t show_##file_name \
> - (struct kobject *kobj, struct attribute *attr, char *buf)   \
> - {   \
> - return sprintf(buf, "%u\n", limits.object); \
> - }
> +#define show_one(file_name, object)  \
> +static ssize_t show_##file_name  \
> +(struct cpufreq_policy *policy, char *buf)   \
> +{\
> + return sprintf(buf, "%u\n", limits.object); \
> +}

Why did you change all of the lines of this macro instead of changing just the
one line you needed to change?

Please don't do that.

> -static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
> - const char *buf, size_t count)
> +static ssize_t store_no_turbo(struct cpufreq_policy *policy, const char *buf,
> + size_t count)
>  {
>   unsigned int input;
>   int ret;
> @@ -293,8 +293,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct 
> attribute *b,
>   return count;
>  }
>  
> -static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
> - const char *buf, size_t count)
> +static ssize_t store_max_perf_pct(struct cpufreq_policy *policy,
> + const char *buf, size_t count)
>  {
>   unsigned int input;
>   int ret;
> @@ -307,8 +307,8 @@ static ssize_t store_max_perf_pct(struct kobject *a, 
> struct attribute *b,
>   return count;
>  }
>  
> -static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
> - const char *buf, size_t count)
> +static ssize_t store_min_perf_pct(struct cpufreq_policy *policy,
> + const char *buf, size_t count)
>  {
>   unsigned int input;
>   int ret;
> @@ -325,9 +325,9 @@ show_one(no_turbo, no_turbo);
>  show_one(max_perf_pct, max_perf_pct);
>  show_one(min_perf_pct, min_perf_pct);
>  
> -define_one_global_rw(no_turbo);
> -define_one_global_rw(max_perf_pct);
> -define_one_global_rw(min_perf_pct);
> +cpufreq_freq_attr_rw(no_turbo);
> +cpufreq_freq_attr_rw(max_perf_pct);
> +cpufreq_freq_attr_rw(min_perf_pct);
>  
>  static struct attribute *intel_pstate_attributes[] = {
>   _turbo.attr,
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 0d84bfa..a092fcb 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -324,22 +324,6 @@ __ATTR(_name, _perm, show_##_name, NULL)
>  static struct freq_attr _name =  \
>  __ATTR(_name, 0644, show_##_name, store_##_name)
>  
> -struct global_attr {
> - struct attribute attr;
> - ssize_t (*show)(struct kobject *kobj,
> - struct attribute 

Re: [PATCH V2 4/4] cpufreq: Get rid of struct global_attr

2013-02-21 Thread Rafael J. Wysocki
On Monday, February 11, 2013 01:20:03 PM Viresh Kumar wrote:
 We don't need to keep two structures for file attributes, global_attr and
 freq_attr. Lets use freq_attr only for cpufreq core and drivers.
 
 Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
 ---
  drivers/cpufreq/acpi-cpufreq.c |  9 -
  drivers/cpufreq/intel_pstate.c | 30 +++---
  include/linux/cpufreq.h| 16 
  3 files changed, 19 insertions(+), 36 deletions(-)
 
 diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
 index 937bc28..14219c3 100644
 --- a/drivers/cpufreq/acpi-cpufreq.c
 +++ b/drivers/cpufreq/acpi-cpufreq.c
 @@ -160,19 +160,18 @@ static ssize_t _store_boost(const char *buf, size_t 
 count)
   return count;
  }
  
 -static ssize_t store_global_boost(struct kobject *kobj, struct attribute 
 *attr,
 -   const char *buf, size_t count)
 +static ssize_t store_global_boost(struct cpufreq_policy *policy,
 + const char *buf, size_t count)
  {
   return _store_boost(buf, count);
  }
  
 -static ssize_t show_global_boost(struct kobject *kobj,
 -  struct attribute *attr, char *buf)
 +static ssize_t show_global_boost(struct cpufreq_policy *policy, char *buf)
  {
   return sprintf(buf, %u\n, boost_enabled);
  }
  
 -static struct global_attr global_boost = __ATTR(boost, 0644,
 +static struct freq_attr global_boost = __ATTR(boost, 0644,
   show_global_boost,
   store_global_boost);
  
 diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
 index 86ad482..0f2d6a0 100644
 --- a/drivers/cpufreq/intel_pstate.c
 +++ b/drivers/cpufreq/intel_pstate.c
 @@ -273,15 +273,15 @@ static void intel_pstate_debug_expose_params(void)
  /** debugfs end /
  
  /** sysfs begin /
 -#define show_one(file_name, object)  \
 - static ssize_t show_##file_name \
 - (struct kobject *kobj, struct attribute *attr, char *buf)   \
 - {   \
 - return sprintf(buf, %u\n, limits.object); \
 - }
 +#define show_one(file_name, object)  \
 +static ssize_t show_##file_name  \
 +(struct cpufreq_policy *policy, char *buf)   \
 +{\
 + return sprintf(buf, %u\n, limits.object); \
 +}

Why did you change all of the lines of this macro instead of changing just the
one line you needed to change?

Please don't do that.

 -static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
 - const char *buf, size_t count)
 +static ssize_t store_no_turbo(struct cpufreq_policy *policy, const char *buf,
 + size_t count)
  {
   unsigned int input;
   int ret;
 @@ -293,8 +293,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct 
 attribute *b,
   return count;
  }
  
 -static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
 - const char *buf, size_t count)
 +static ssize_t store_max_perf_pct(struct cpufreq_policy *policy,
 + const char *buf, size_t count)
  {
   unsigned int input;
   int ret;
 @@ -307,8 +307,8 @@ static ssize_t store_max_perf_pct(struct kobject *a, 
 struct attribute *b,
   return count;
  }
  
 -static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
 - const char *buf, size_t count)
 +static ssize_t store_min_perf_pct(struct cpufreq_policy *policy,
 + const char *buf, size_t count)
  {
   unsigned int input;
   int ret;
 @@ -325,9 +325,9 @@ show_one(no_turbo, no_turbo);
  show_one(max_perf_pct, max_perf_pct);
  show_one(min_perf_pct, min_perf_pct);
  
 -define_one_global_rw(no_turbo);
 -define_one_global_rw(max_perf_pct);
 -define_one_global_rw(min_perf_pct);
 +cpufreq_freq_attr_rw(no_turbo);
 +cpufreq_freq_attr_rw(max_perf_pct);
 +cpufreq_freq_attr_rw(min_perf_pct);
  
  static struct attribute *intel_pstate_attributes[] = {
   no_turbo.attr,
 diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
 index 0d84bfa..a092fcb 100644
 --- a/include/linux/cpufreq.h
 +++ b/include/linux/cpufreq.h
 @@ -324,22 +324,6 @@ __ATTR(_name, _perm, show_##_name, NULL)
  static struct freq_attr _name =  \
  __ATTR(_name, 0644, show_##_name, store_##_name)
  
 -struct global_attr {
 - struct attribute attr;
 - ssize_t (*show)(struct kobject *kobj,
 - struct attribute *attr, char *buf);
 - ssize_t (*store)(struct kobject *a, struct attribute *b,
 - 

Re: [PATCH V2 4/4] cpufreq: Get rid of struct global_attr

2013-02-21 Thread Viresh Kumar
On 22 February 2013 05:15, Rafael J. Wysocki r...@sisk.pl wrote:
 Why did you change all of the lines of this macro instead of changing just the
 one line you needed to change?

I didn't like the indentation used within the macro. So did it.

 Please don't do that.

Okay.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V2 4/4] cpufreq: Get rid of struct global_attr

2013-02-21 Thread Rafael J. Wysocki
On Friday, February 22, 2013 07:47:44 AM Viresh Kumar wrote:
 On 22 February 2013 05:15, Rafael J. Wysocki r...@sisk.pl wrote:
  Why did you change all of the lines of this macro instead of changing just 
  the
  one line you needed to change?
 
 I didn't like the indentation used within the macro. So did it.

In general, things like that are for separate cleanup patches.  If you mix
functional changes with cleanups, poeple get confused and it's difficult to see
what's needed and what's optional.

I know it's tempting to fix stuff like that along with doing functional
changes and I do that sometimes.  Not very often, though, and with care.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V2 4/4] cpufreq: Get rid of struct global_attr

2013-02-21 Thread Viresh Kumar
On 22 February 2013 08:03, Rafael J. Wysocki r...@sisk.pl wrote:
 On Friday, February 22, 2013 07:47:44 AM Viresh Kumar wrote:
 On 22 February 2013 05:15, Rafael J. Wysocki r...@sisk.pl wrote:
  Why did you change all of the lines of this macro instead of changing just 
  the
  one line you needed to change?

 I didn't like the indentation used within the macro. So did it.

 In general, things like that are for separate cleanup patches.  If you mix
 functional changes with cleanups, poeple get confused and it's difficult to 
 see
 what's needed and what's optional.

 I know it's tempting to fix stuff like that along with doing functional
 changes and I do that sometimes.  Not very often, though, and with care.

Even i give similar comments sometimes but forget these while writing my
patches :)

Anyway, fixup:

commit b1bbb99467d56140cf3a8a2b70e61b456aa46e48
Author: Viresh Kumar viresh.ku...@linaro.org
Date:   Fri Feb 22 07:59:20 2013 +0530

fixup! cpufreq: Get rid of struct global_attr
---
 drivers/cpufreq/intel_pstate.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index e795134..49846b9 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -273,12 +273,12 @@ static void intel_pstate_debug_expose_params(void)
 /** debugfs end /

 /** sysfs begin /
-#define show_one(file_name, object)\
-static ssize_t show_##file_name\
-(struct cpufreq_policy *policy, char *buf) \
-{  \
-   return sprintf(buf, %u\n, limits.object); \
-}
+#define show_one(file_name, object)\
+   static ssize_t show_##file_name \
+   (struct cpufreq_policy *policy, char *buf)  \
+   {   \
+   return sprintf(buf, %u\n, limits.object); \
+   }
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/