Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

2009-10-08 Thread Kevin Hilman 
Reddy, Teerth tee...@ti.com writes:

 From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
 From: Teerth Reddy tee...@ti.com
 Date: Wed, 9 Sep 2009 11:01:04 +0530
 Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue

 This patch fixes the VDD2 OPP1 issue. The patch has change
 which does not allow VDD2 OPP setting to 1.VDD2 should not be put 
 at OPP1 as this is not a supported OPP for VDD2

Patch looks fine, but shortlog (subject) and changelog should be more
clear.  These should be written for people who are not as familiar
with the code.  For example, seeing this shortlog in a git history
would not be helpful as the irst thing one would ask is what OPP1
issue?

How about something like this:

Subject: OMAP3: PM: do not allow OPP1 allow for VDD2

Since OPP1 is not a supported OPP for VDD2, do not allow it to be
changed using the sysfs interface.

Kevin


 Signed-off-by: Teerth Reddy tee...@ti.com
 ---
  arch/arm/mach-omap2/pm.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
 index fec7d00..d0e03c4 100644
 --- a/arch/arm/mach-omap2/pm.c
 +++ b/arch/arm/mach-omap2/pm.c
 @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct 
 kobj_attribute *attr,
   }
   resource_set_opp_level(VDD1_OPP, value, flags);
   } else if (attr == vdd2_opp_attr) {
 - if (value  1 || value  3) {
 + if (value  2 || value  3) {
   printk(KERN_ERR vdd_opp_store: Invalid value\n);
   return -EINVAL;
   }
 -- 
 1.5.4.7
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

2009-10-08 Thread Nishanth Menon 

Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:

From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
From: Teerth Reddy tee...@ti.com
Date: Wed, 9 Sep 2009 11:01:04 +0530
Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue

This patch fixes the VDD2 OPP1 issue. The patch has change
which does not allow VDD2 OPP setting to 1.VDD2 should not be put 
at OPP1 as this is not a supported OPP for VDD2


Signed-off-by: Teerth Reddy tee...@ti.com
---
 arch/arm/mach-omap2/pm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index fec7d00..d0e03c4 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct 
kobj_attribute *attr,
}
resource_set_opp_level(VDD1_OPP, value, flags);
} else if (attr == vdd2_opp_attr) {
-   if (value  1 || value  3) {
+   if (value  2 || value  3) {
printk(KERN_ERR vdd_opp_store: Invalid value\n);
return -EINVAL;
}
this is not scalable. we should be able to disable OPPs for each OPP 
from the OPP array. with different silicons, we could have the same OPP 
enabled/disabled. NAK - need to handle based on mpu_opps[vale].rate ==0


--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

2009-10-08 Thread Kevin Hilman 
Nishanth Menon n...@ti.com writes:

 Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:
 From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
 From: Teerth Reddy tee...@ti.com
 Date: Wed, 9 Sep 2009 11:01:04 +0530
 Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue

 This patch fixes the VDD2 OPP1 issue. The patch has change
 which does not allow VDD2 OPP setting to 1.VDD2 should not be put at
 OPP1 as this is not a supported OPP for VDD2

 Signed-off-by: Teerth Reddy tee...@ti.com
 ---
  arch/arm/mach-omap2/pm.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
 index fec7d00..d0e03c4 100644
 --- a/arch/arm/mach-omap2/pm.c
 +++ b/arch/arm/mach-omap2/pm.c
 @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj, 
 struct kobj_attribute *attr,
  }
  resource_set_opp_level(VDD1_OPP, value, flags);
  } else if (attr == vdd2_opp_attr) {
 -if (value  1 || value  3) {
 +if (value  2 || value  3) {
  printk(KERN_ERR vdd_opp_store: Invalid value\n);
  return -EINVAL;
  }
 this is not scalable. we should be able to disable OPPs for each OPP
 from the OPP array. with different silicons, we could have the same
 OPP enabled/disabled. NAK - need to handle based on
 mpu_opps[vale].rate ==0

Agreed that the current code is not scalable, but that was a problem
before Teerth's fix.

The proposed patch is a bugfix to existing code and should be merged
after my comments are addressed.

Then, the scalability issues can be addressed separately.

Kevin

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue

2009-10-08 Thread Menon, Nishanth 
 -Original Message-
 From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
 Sent: Thursday, October 08, 2009 9:58 AM
 To: Menon, Nishanth
 Cc: Reddy, Teerth; linux-omap@vger.kernel.org
 Subject: Re: [PATCH] OMAP3: PM: Fix VDD2 OPP1 issue
 
 Nishanth Menon n...@ti.com writes:
 
  Reddy, Teerth had written, on 10/08/2009 04:21 AM, the following:
  From 144669d941a432875db37ae9431847f6753e566e Mon Sep 17 00:00:00 2001
  From: Teerth Reddy tee...@ti.com
  Date: Wed, 9 Sep 2009 11:01:04 +0530
  Subject: [PATCH] ARM: OMAP3: PM: Fix VDD2 OPP1 issue
 
  This patch fixes the VDD2 OPP1 issue. The patch has change
  which does not allow VDD2 OPP setting to 1.VDD2 should not be put at
  OPP1 as this is not a supported OPP for VDD2
 
  Signed-off-by: Teerth Reddy tee...@ti.com
  ---
   arch/arm/mach-omap2/pm.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
  index fec7d00..d0e03c4 100644
  --- a/arch/arm/mach-omap2/pm.c
  +++ b/arch/arm/mach-omap2/pm.c
  @@ -195,7 +195,7 @@ static ssize_t vdd_opp_store(struct kobject *kobj,
 struct kobj_attribute *attr,
 }
 resource_set_opp_level(VDD1_OPP, value, flags);
 } else if (attr == vdd2_opp_attr) {
  -  if (value  1 || value  3) {
  +  if (value  2 || value  3) {
 printk(KERN_ERR vdd_opp_store: Invalid value\n);
 return -EINVAL;
 }
  this is not scalable. we should be able to disable OPPs for each OPP
  from the OPP array. with different silicons, we could have the same
  OPP enabled/disabled. NAK - need to handle based on
  mpu_opps[vale].rate ==0
 
 Agreed that the current code is not scalable, but that was a problem
 before Teerth's fix.
 
 The proposed patch is a bugfix to existing code and should be merged
 after my comments are addressed.
 
 Then, the scalability issues can be addressed separately.
 
Ok, reverting my objection.

Will try to do a follow up patch - essentially let program_opp make that 
decision IMHO.


Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html