Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp

[Nishanth Menon <nm@ti.com>]

Gopinath, Thara had written, on 07/13/2010 09:32 AM, the following:
> 
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Tuesday, July 13, 2010 5:53 PM
>>> To: Gopinath, Thara
>>> Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com; paul@pwsan.com; tony@atomide.com;
>>> Cousson, Benoit; Sawant, Anand; Sripathy, Vishwanath; Premi, Sanjeev
>>> Subject: Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp
>>>
>>> Gopinath, Thara had written, on 07/13/2010 12:47 AM, the following:
>>>> Earlier we were checking on !dev_opp where as find_device_opp returns
>>>> a error pointer in case of error. So correcting the check as in the earlier
>>>> code even if find_device_opp returns an error opp_init_cpufreq_table
>>>> was not exiting.
>>>>
>>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>>> ---
>>>>  arch/arm/plat-omap/opp.c |    4 +++-
>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>>>> index a06b88d..d88a2e0 100644
>>>> --- a/arch/arm/plat-omap/opp.c
>>>> +++ b/arch/arm/plat-omap/opp.c
>>>> @@ -457,8 +457,10 @@ void opp_init_cpufreq_table(struct device *dev,
>>>>  	int i = 0;
>>>>
>>>>  	dev_opp = find_device_opp(dev);
>>>> -	if (WARN_ON(!dev_opp))
>>>> +	if (IS_ERR(dev_opp)) {
>>>> +		WARN_ON(1);
>>> could we use pr_warning here instead of WARN_ON?
> The WARN_ON was already in the code. Added by you or Kevin. I just corrected the error checking.

:) using a WARN_ON() for an operational code is not a good idea - who 
ever added it (git annotate can help you there). WARN_ON(1) give no 
useful information when debugging in the field - just gives a line 
number and function name which changes btw.. useful info is something like:
pr_warning("%s: failed to find device\n", __func__);

useful when someone posts a log and we need to debug such logic.. esp 
useful after a period of time.

> 
> Regards
> Thara
>>>>  		return;
>>>> +	}
>>>>
>>>>  	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
>>>>  			     (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);
>>>
>>> --
>>> Regards,
>>> Nishanth Menon


-- 
Regards,
Nishanth Menon