Re: [PATCH 2/4] PM / OPP: fix warning in of_free_opp_table

[Nishanth Menon <nm@ti.com>]

$subject:
PM / OPP: Do not warn when no OPP was ever registered in of_free_opp_table

might be more appropriate?

On 12/16/2014 10:28 PM, Viresh Kumar wrote:
> On 17 December 2014 at 04:39, Dmitry Torokhov <dtor@chromium.org> wrote:
>> Not having OPP defined for a device is not a crime, we should not splat
>> warning in this case. Also, it seems that we are ready to accept invalid
>> dev (find_device_opp will return ERR_PTR(-EINVAL) then) so let's not
>> crash in dev_name() in such case.
>>
>> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
>> ---
>>  drivers/base/power/opp.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index b78c14d..413c7fe 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -799,9 +799,15 @@ void of_free_opp_table(struct device *dev)
>>
>>         /* Check for existing list for 'dev' */
>>         dev_opp = find_device_opp(dev);
>> -       if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev),
>> -                PTR_ERR(dev_opp)))
>> +       if (IS_ERR(dev_opp)) {
>> +               int error = PTR_ERR(dev_opp);
>> +               if (error != -ENODEV)
>> +                       WARN(1, "%s: dev_opp: %ld\n",
>> +                            IS_ERR_OR_NULL(dev) ?
>> +                                       "Invalid device" : dev_name(dev),
>> +                            error);
>>                 return;
> 
> What about this:
> 
>         if (IS_ERR(dev_opp)) {
>                 int error = PTR_ERR(dev_opp);
>                 WARN(error != -ENODEV, "%s: dev_opp: %ld\n",
>                      IS_ERR_OR_NULL(dev) ?  "Invalid device" : dev_name(dev),
>                      error);
>                 return;
>         }
Adds the following build warning: I suggest changing the %ld to %d

"warning: format ‘%ld’ expects argument of type ‘long int’, but
argument 5 has type ‘int’ [-Wformat]"


This also fixes the warning I got on AM437x based platforms. test results:
1: am437x-sk: BOOT: PASS: crit=2 err=13 warn=25, CPUFreq: N/A,
CPUIdle: N/A: http://slexy.org/raw/s20MAtNEbt
 2: am437x-sk-before: BOOT: PASS: crit=2 err=13 warn=57, CPUFreq: N/A,
CPUIdle: N/A: http://slexy.org/raw/s21rZCo4cD

With the suggested changes,

Acked-by: Nishanth Menon <nm@ti.com>

> 
> We can get rid of the extra indentation level and an extra comparison check.
> 
> Otherwise:
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>


PS: I am sure you have already done some level of checks, but I
suggest using something like aiaiai or
https://github.com/nmenon/kernel_patch_verify or other solutions like
those discussed in
https://plus.google.com/112464029509057661457/posts/5LTuHHMyXLT to
help do some initial clean up of the patches..


-- 
Regards,
Nishanth Menon