[PM-SR] [PATCH 0/7] Janitor series 2: cleanups

[PM-SR] [PATCH 0/7] Janitor series 2: cleanups

[Nishanth Menon]

More misc cleanups for Sr branch including print cleans, checks etc.
This series probably should be squashed with other patches in pm-sr
branch. note some of these patches address few other comments I had
made in an unrelated patch.

Related comments prior to providing patches:

a) I looked close into sr_device.c, I dont think we should
decide in sr_device.c if SR is enabled by default or not. IMHO, this
should be passed on from board files. it is not the sr_device's call
to make. we have boards which are comfortable with SR and which are not.
example some boards may choose to enable SR late in the game, while
some may choose to enable it by default. we need to cater to both.

e.g. currently 3430 es3.1 would have SR enabled by default. N900 as an
example chooses to allow users to decide what to have and is enabled
late in the game (boots up disabled, gets enabled once userspace starts).


b) It assumes that 3630 have ntargets - this is true from ES1.1 onwards.
the older silicon dont have ntargets fused in them.

How do we want to handle these?

This is the second series independent of series 1:
http://marc.info/?l=linux-omap&m=127741497415352&w=2

Nishanth Menon (7):
  omap3: voltage: cleanup pr_xxxx
  omap3: sr: cleanup pr_xxx
  omap3: sr: enable/disable sr only if required
  omap3: sr: device: cleanup pr_xxx
  omap3: sr: device: add unlikely checks
  omap3: sr: device: check for dev_attr
  omap3: sr: device: fail sr_dev_init should return error

 arch/arm/mach-omap2/smartreflex.c |   24 ++++++++++++++++++------
 arch/arm/mach-omap2/sr_device.c   |   29 +++++++++++++++++++----------
 arch/arm/mach-omap2/voltage.c     |    7 ++++---
 3 files changed, 41 insertions(+), 19 deletions(-)

Regards,
Nishanth Menon

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

[PM-SR] [PATCH 1/7] omap3: voltage: cleanup pr_xxxx

[Nishanth Menon]

few more pr_xxxx need cleanup for printing the function name and
not using multiline prints when c allows us to do "".

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/voltage.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index d289691..963b037 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
 		u32 *option = data;
 		*option = val;
 	} else {
-		pr_notice("DEBUG option not enabled!\n	\
-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
+		pr_notice("%s: DEBUG option not enabled! "
+			"echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
+			__func__);
 	}
 	return 0;
 }
@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
 	u8 vsel;
 
 	vsel = voltage_read_reg(info->vp_offs.voltage_reg);
-	pr_notice("curr_vsel = %x\n", vsel);
+	pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
 	*val = vsel * 12500 + 600000;
 
 	return 0;
-- 
1.6.3.3

[PM-SR] [PATCH 2/7] omap3: sr: cleanup pr_xxx

[Nishanth Menon]

pr_xxx family is not informative for debug unless one decides
to grep the code, instead print the function to help debug
easier on the field.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 57fc9b2..d63691b 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -804,8 +804,9 @@ static int omap_sr_params_store(void *data, u64 val)
 		u32 *option = data;
 		*option = val;
 	} else {
-		pr_notice("DEBUG option not enabled!\n	\
-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
+		pr_notice("%s: DEBUG option not enabled! "
+			"echo 1 > pm_debug/enable_sr_vp_debug to enable\n",
+			__func__);
 	}
 	return 0;
 }
-- 
1.6.3.3

[PM-SR] [PATCH 3/7] omap3: sr: enable/disable sr only if required

[Nishanth Menon]

we dont need to go down the path of enabling/disabling the SR
if we dont need to. do some sanity check and trigger if needed

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index d63691b..9b5a10e 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -778,15 +778,26 @@ static int omap_sr_autocomp_show(void *data, u64 *val)
 static int omap_sr_autocomp_store(void *data, u64 val)
 {
 	struct omap_sr *sr_info = (struct omap_sr *) data;
+	u32 value = (u32) val;
 
 	if (!sr_info) {
 		pr_warning("%s: omap_sr struct for SR not found\n", __func__);
 		return -EINVAL;
 	}
-	if (!val)
-		sr_stop_vddautocomp(sr_info);
-	else
-		sr_start_vddautocomp(sr_info);
+
+	/* Sanity check */
+	if (value && (value != 1)) {
+		pr_err("%s: invalid value %d\n", __func__, value);
+		return -EINVAL;
+	}
+
+	/* change only if needed */
+	if (sr_info->is_autocomp_active ^ value) {
+		if (!val)
+			sr_stop_vddautocomp(sr_info);
+		else
+			sr_start_vddautocomp(sr_info);
+	}
 	return 0;
 }
 
-- 
1.6.3.3

[PM-SR] [PATCH 4/7] omap3: sr: device: cleanup pr_xxx

[Nishanth Menon]

strings in c dont need to be split accross multiple lines with \
. instead they can be put as "abc " "def " and it is equivalent to
"abc def". fix the same

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index dbf7603..7d13704 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -151,8 +151,9 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_dev_data->volts_supported = omap_get_voltage_table(i,
 				&sr_dev_data->volt_data);
 	if (!sr_dev_data->volts_supported) {
-		pr_warning("%s: No Voltage table registerd fo VDD%d.Something \
-				really wrong\n\n", __func__, i + 1);
+		pr_warning("%s: No Voltage table registerd fo VDD%d. "
+				"Something is really wrong\n",
+				__func__, i + 1);
 		i++;
 		kfree(sr_data);
 		return 0;
-- 
1.6.3.3

[PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Nishanth Menon]

Add unlikely checks to better optimize the rare occurrance of
erroneous conditions.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 7d13704..177299a 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
 {
 	int i;
 
-	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
-			!dev_data->efuse_nvalues_offs) {
+	if (unlikely(!dev_data || !dev_data->volts_supported ||
+			!dev_data->volt_data ||
+			!dev_data->efuse_nvalues_offs)) {
 		pr_warning("%s: Bad parameters! dev_data = %x,"
 			"dev_data->volts_supported = %x,"
 			"dev_data->volt_data = %x,"
@@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
 {
 	int i;
 
-	if (!dev_data || !dev_data->volts_supported ||
-			!dev_data->volt_data || !dev_data->test_nvalues) {
+	if (unlikely(!dev_data || !dev_data->volts_supported ||
+			!dev_data->volt_data || !dev_data->test_nvalues)) {
 		pr_warning("%s: Bad parameters! dev_data = %x,"
 			"dev_data->volts_supported = %x,"
 			"dev_data->volt_data = %x,"
@@ -123,7 +124,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	static int i;
 
 	sr_data = kzalloc(sizeof(struct omap_sr_data), GFP_KERNEL);
-	if (!sr_data) {
+	if (unlikely(!sr_data)) {
 		pr_err("%s: Unable to allocate memory for %s sr_data.Error!\n",
 			__func__, oh->name);
 		return -ENOMEM;
@@ -150,7 +151,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_data->device_idle = omap_device_idle;
 	sr_dev_data->volts_supported = omap_get_voltage_table(i,
 				&sr_dev_data->volt_data);
-	if (!sr_dev_data->volts_supported) {
+	if (unlikely(!sr_dev_data->volts_supported)) {
 		pr_warning("%s: No Voltage table registerd fo VDD%d. "
 				"Something is really wrong\n",
 				__func__, i + 1);
@@ -162,7 +163,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
 			       omap_sr_latency,
 			       ARRAY_SIZE(omap_sr_latency), 0);
-	if (IS_ERR(od)) {
+	if (unlikely(IS_ERR(od))) {
 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
 			__func__, name, oh->name);
 		kfree(sr_data);
-- 
1.6.3.3

[PM-SR] [PATCH 6/7] omap3: sr: device: check for dev_attr

[Nishanth Menon]

in the unlikely case that hwmod database is messed up, dont crash
report error and attempt to recover.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 177299a..81d2532 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -131,6 +131,12 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	}
 
 	sr_dev_data = (struct omap_sr_dev_data *)oh->dev_attr;
+	if (unlikely(!sr_dev_data)) {
+		pr_err("%s: Bad oh->dev_attr!\n", __func__);
+		kfree(sr_data);
+		return -EINVAL;
+	}
+
 	/*
 	 * OMAP3430 ES3.1 chips by default come with Efuse burnt
 	 * with parameters required for full functionality of
-- 
1.6.3.3

[PM-SR] [PATCH 7/7] omap3: sr: device: fail sr_dev_init should return error

[Nishanth Menon]

sr_dev_init should return error on error conditions

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 81d2532..945ff45 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -163,7 +163,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 				__func__, i + 1);
 		i++;
 		kfree(sr_data);
-		return 0;
+		return -ENODATA;
 	}
 	sr_set_nvalues(sr_dev_data, sr_data);
 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
@@ -173,6 +173,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
 			__func__, name, oh->name);
 		kfree(sr_data);
+		return PTR_ERR(od);
 	}
 	i++;
 	return 0;
-- 
1.6.3.3

Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Artem Bityutskiy]

On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
> Add unlikely checks to better optimize the rare occurrance of
> erroneous conditions.
> 
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Thara Gopinath <thara@ti.com>
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>

unlikely and friends make sens only in realy hot path places. In other
places like you touch, they are pointless - better let gcc make a choice
of how to arrange code. And they only make the code less readable by
adding extra braces and making ifs longer.

Thus, NACK for this from my side.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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

Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Nishanth Menon]

Artem Bityutskiy had written, on 07/09/2010 08:12 AM, the following:
> On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
>> Add unlikely checks to better optimize the rare occurrance of
>> erroneous conditions.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Cc: Thara Gopinath <thara@ti.com>
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
> 
> unlikely and friends make sens only in realy hot path places. In other
> places like you touch, they are pointless - better let gcc make a choice

> @@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
>  {
>  	int i;
>  
> -	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
> -			!dev_data->efuse_nvalues_offs) {
> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> +			!dev_data->volt_data ||
> +			!dev_data->efuse_nvalues_offs)) {

> @@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
>  {
>  	int i;
>  
> -	if (!dev_data || !dev_data->volts_supported ||
> -			!dev_data->volt_data || !dev_data->test_nvalues) {
> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> +			!dev_data->volt_data || !dev_data->test_nvalues)) {

and other places, why do you think that these are checks that should be 
expected? - would be great if you can explain inline to the patch which 
unlikely checks dont make sense.

static functions such as these are helpers for the maincode, unless 
something horribly wrong occurs within the codepath calling these 
helpers, they are not expected to be invalid parameters. hence the 
rationale for adding unlikely.



> of how to arrange code. And they only make the code less readable by
> adding extra braces and making ifs longer.
> 
> Thus, NACK for this from my side.
> 


-- 
Regards,
Nishanth Menon

Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Artem Bityutskiy]

On Fri, 2010-07-09 at 08:40 -0500, Nishanth Menon wrote:
> Artem Bityutskiy had written, on 07/09/2010 08:12 AM, the following:
> > On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
> >> Add unlikely checks to better optimize the rare occurrance of
> >> erroneous conditions.
> >>
> >> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> >> Cc: Thara Gopinath <thara@ti.com>
> >>
> >> Signed-off-by: Nishanth Menon <nm@ti.com>
> > 
> > unlikely and friends make sens only in realy hot path places. In other
> > places like you touch, they are pointless - better let gcc make a choice
> 
> > @@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
> >  {
> >  	int i;
> >  
> > -	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
> > -			!dev_data->efuse_nvalues_offs) {
> > +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> > +			!dev_data->volt_data ||
> > +			!dev_data->efuse_nvalues_offs)) {
> 
> > @@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
> >  {
> >  	int i;
> >  
> > -	if (!dev_data || !dev_data->volts_supported ||
> > -			!dev_data->volt_data || !dev_data->test_nvalues) {
> > +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> > +			!dev_data->volt_data || !dev_data->test_nvalues)) {
> 
> and other places, why do you think that these are checks that should be 
> expected? - would be great if you can explain inline to the patch which 
> unlikely checks dont make sense.
> 
> static functions such as these are helpers for the maincode, unless 
> something horribly wrong occurs within the codepath calling these 
> helpers, they are not expected to be invalid parameters. hence the 
> rationale for adding unlikely.

Sorry, I do not really understand you. All I said is that
unlikely()/likely() are usually used in hot-paths / tight loops.

unlikely()/likely() are micro optimization. They make no difference when
you use them in initialization paths.

So what I said, that in your patch they will make no difference
performance wise. So no benefits.

But they make if () statements a bit more difficult to read, this is a
drawback.

So your patch introduces no benefits, just a drawback. Thus, it is not
good.

There were many flamewars about unlikely and likely in lkml in the past.
And the outcome was always - do not use them anywhere except of
performance-critical tight loops / hot paths.


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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

Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Nishanth Menon]

Artem Bityutskiy had written, on 07/09/2010 09:15 AM, the following:
> On Fri, 2010-07-09 at 08:40 -0500, Nishanth Menon wrote:
>> Artem Bityutskiy had written, on 07/09/2010 08:12 AM, the following:
>>> On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
>>>> Add unlikely checks to better optimize the rare occurrance of
>>>> erroneous conditions.
>>>>
>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Cc: Thara Gopinath <thara@ti.com>
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> unlikely and friends make sens only in realy hot path places. In other
>>> places like you touch, they are pointless - better let gcc make a choice
>>> @@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
>>>  {
>>>  	int i;
>>>  
>>> -	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
>>> -			!dev_data->efuse_nvalues_offs) {
>>> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
>>> +			!dev_data->volt_data ||
>>> +			!dev_data->efuse_nvalues_offs)) {
>>> @@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
>>>  {
>>>  	int i;
>>>  
>>> -	if (!dev_data || !dev_data->volts_supported ||
>>> -			!dev_data->volt_data || !dev_data->test_nvalues) {
>>> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
>>> +			!dev_data->volt_data || !dev_data->test_nvalues)) {
>> and other places, why do you think that these are checks that should be 
>> expected? - would be great if you can explain inline to the patch which 
>> unlikely checks dont make sense.
>>
>> static functions such as these are helpers for the maincode, unless 
>> something horribly wrong occurs within the codepath calling these 
>> helpers, they are not expected to be invalid parameters. hence the 
>> rationale for adding unlikely.
> 
> Sorry, I do not really understand you. All I said is that
> unlikely()/likely() are usually used in hot-paths / tight loops.
> 
> unlikely()/likely() are micro optimization. They make no difference when
> you use them in initialization paths.
> 
> So what I said, that in your patch they will make no difference
> performance wise. So no benefits.
> 
> But they make if () statements a bit more difficult to read, this is a
> drawback.
> 
> So your patch introduces no benefits, just a drawback. Thus, it is not
> good.
> 
> There were many flamewars about unlikely and likely in lkml in the past.
> And the outcome was always - do not use them anywhere except of
> performance-critical tight loops / hot paths.
> 
> 
alright.. thanks for the review. will drop this patch.

-- 
Regards,
Nishanth Menon