[PM-WIP-OPP] [PATCH 2/2]: Change return value from ERR_PTR(..) to NULL in opp layer

[PM-WIP-OPP] [PATCH 2/2]: Change return value from ERR_PTR(..) to NULL in opp layer

[Romit Dasgupta]

Returning NULL pointer from the OPP APIs instead of ERR_PTR where
return struct omap_opp *. This is because there is no inherent value in
returning ERR_PTR from the opp layer. Returning NULL serves the purpose.

Signed-off-by: Romit Dasgupta <romit@ti.com>
---

diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index 5ec072e..9572062 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -202,7 +202,7 @@ static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,
 
 	BUG_ON(opp_t >= OPP_TYPES_MAX);
 	opp = opp_find_freq_ceil(opp_t, &freq);
-	if (IS_ERR(opp))
+	if (!opp)
 		return -EINVAL;
 	*opp_id = opp_get_opp_id(opp);
 	return 0;
diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index 8fd9366..7835b5d 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -129,7 +129,7 @@ struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
 
 	if (unlikely(opp_t >= OPP_TYPES_MAX)) {
 		pr_err("%s: Invalid parameters being passed\n", __func__);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 
 	oppl = _opp_list[opp_t];
@@ -143,7 +143,7 @@ struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
 		oppl++;
 	}
 
-	return OPP_TERM(oppl) ? ERR_PTR(-ENOENT) : oppl;
+	return OPP_TERM(oppl) ? NULL : oppl;
 }
 
 struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
@@ -153,7 +153,7 @@ struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
 	if (unlikely(opp_t >= OPP_TYPES_MAX || !freq ||
 		 IS_ERR(freq))) {
 		pr_err("%s: Invalid parameters being passed\n", __func__);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 
 	oppl = _opp_list[opp_t];
@@ -169,7 +169,7 @@ struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
 	}
 
 	if (OPP_TERM(oppl))
-		return ERR_PTR(-ENOENT);
+		return NULL;
 
 	*freq = oppl->rate;
 
@@ -183,7 +183,7 @@ struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
 	if (unlikely(opp_t >= OPP_TYPES_MAX || !freq ||
 		 IS_ERR(freq))) {
 		pr_err("%s: Invalid parameters being passed\n", __func__);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 	oppl = prev_opp = _opp_list[opp_t];
 
@@ -202,7 +202,7 @@ struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
 	}
 
 	if (prev_opp->rate > *freq)
-		return ERR_PTR(-ENOENT);
+		return NULL;
 
 	*freq = prev_opp->rate;
 

Re: [PM-WIP-OPP] [PATCH 2/2]: Change return value from ERR_PTR(..) to NULL in opp layer

[Kevin Hilman]

Romit Dasgupta <romit@ti.com> writes:

> Returning NULL pointer from the OPP APIs instead of ERR_PTR where
> return struct omap_opp *. This is because there is no inherent value in
> returning ERR_PTR from the opp layer. Returning NULL serves the purpose.

NAK.

Using ERR_PTR allows returning different types of error conditions,
and is common practice across the kernel.

Kevin

> Signed-off-by: Romit Dasgupta <romit@ti.com>
> ---
>
> diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
> index 5ec072e..9572062 100644
> --- a/arch/arm/mach-omap2/resource34xx.c
> +++ b/arch/arm/mach-omap2/resource34xx.c
> @@ -202,7 +202,7 @@ static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,
>  
>  	BUG_ON(opp_t >= OPP_TYPES_MAX);
>  	opp = opp_find_freq_ceil(opp_t, &freq);
> -	if (IS_ERR(opp))
> +	if (!opp)
>  		return -EINVAL;
>  	*opp_id = opp_get_opp_id(opp);
>  	return 0;
> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> index 8fd9366..7835b5d 100644
> --- a/arch/arm/plat-omap/opp.c
> +++ b/arch/arm/plat-omap/opp.c
> @@ -129,7 +129,7 @@ struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
>  
>  	if (unlikely(opp_t >= OPP_TYPES_MAX)) {
>  		pr_err("%s: Invalid parameters being passed\n", __func__);
> -		return ERR_PTR(-EINVAL);
> +		return NULL;
>  	}
>  
>  	oppl = _opp_list[opp_t];
> @@ -143,7 +143,7 @@ struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
>  		oppl++;
>  	}
>  
> -	return OPP_TERM(oppl) ? ERR_PTR(-ENOENT) : oppl;
> +	return OPP_TERM(oppl) ? NULL : oppl;
>  }
>  
>  struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
> @@ -153,7 +153,7 @@ struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
>  	if (unlikely(opp_t >= OPP_TYPES_MAX || !freq ||
>  		 IS_ERR(freq))) {
>  		pr_err("%s: Invalid parameters being passed\n", __func__);
> -		return ERR_PTR(-EINVAL);
> +		return NULL;
>  	}
>  
>  	oppl = _opp_list[opp_t];
> @@ -169,7 +169,7 @@ struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
>  	}
>  
>  	if (OPP_TERM(oppl))
> -		return ERR_PTR(-ENOENT);
> +		return NULL;
>  
>  	*freq = oppl->rate;
>  
> @@ -183,7 +183,7 @@ struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
>  	if (unlikely(opp_t >= OPP_TYPES_MAX || !freq ||
>  		 IS_ERR(freq))) {
>  		pr_err("%s: Invalid parameters being passed\n", __func__);
> -		return ERR_PTR(-EINVAL);
> +		return NULL;
>  	}
>  	oppl = prev_opp = _opp_list[opp_t];
>  
> @@ -202,7 +202,7 @@ struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
>  	}
>  
>  	if (prev_opp->rate > *freq)
> -		return ERR_PTR(-ENOENT);
> +		return NULL;
>  
>  	*freq = prev_opp->rate;
>  

RE: [PM-WIP-OPP] [PATCH 2/2]: Change return value from ERR_PTR(..) to NULL in opp layer

[Dasgupta, Romit]

>> Returning NULL pointer from the OPP APIs instead of ERR_PTR where
>> return struct omap_opp *. This is because there is no inherent value in
>> returning ERR_PTR from the opp layer. Returning NULL serves the purpose.

>NAK.

>Using ERR_PTR allows returning different types of error conditions,
>and is common practice across the kernel.
Yes, it is true that it is a common practice but in case of the OPP layer I did not 
see any purpose. The caller of the APIs returning struct omap_opp * can just
check for NULL value for failure.

Re: [PM-WIP-OPP] [PATCH 2/2]: Change return value from ERR_PTR(..) to NULL in opp layer

[Nishanth Menon]

Dasgupta, Romit had written, on 01/15/2010 08:36 PM, the following:
>>> Returning NULL pointer from the OPP APIs instead of ERR_PTR where
>>> return struct omap_opp *. This is because there is no inherent value in
>>> returning ERR_PTR from the opp layer. Returning NULL serves the purpose.
> 
>> NAK.
> 
>> Using ERR_PTR allows returning different types of error conditions,
>> and is common practice across the kernel.
> Yes, it is true that it is a common practice but in case of the OPP layer I did not 
> see any purpose. The caller of the APIs returning struct omap_opp * can just
> check for NULL value for failure.
My initial intention of introducing ERR_PTR had the objective of being 
flexible:
I could not predict how each function would develop into - e.g. lists 
etc. They may prefer to return error values which could be independently 
handled. allow future flexibility. let me illustrate it - now that we 
are aligned that we are moving to enums:
The caller does not have an idea if mpu_opps was initialized or not.

opp_find_freq_exact can return: (examples ofcourse)
   -EDATA to say that the domain requested was not initialized
or
   -EAGAIN once we introduce locks to say that it is locked 
(non-blocking implementation)
or
   -ERANGE to say that the caller is asking for a frequency beyond the 
supported range.

Another example: opp_enable can now return -EEXIST to say that the the 
opp was already enabled etc..

The benefit I definitely see is that with an previously placed single 
pr_err of the return value by the caller, I can remotely debug an issue 
in code instead of having the developer to add printks/use lauterbach to 
debug.

ok, I might not have selected the best of return values, but I hope the 
idea is clear. NAK from myside too.

Regards,
Nishanth Menon