[PATCH 3/5] powerpc/mpc5121: shared DIU framebuffer support

[Timur Tabi]

On Fri, Apr 30, 2010 at 5:19 AM, Anatolij Gustschin <agust at denx.de> wrote:

>> How about just doing this?
>>
>> .init_early             = mpc512x_init_diu,
>
> I thought it should be prepared for adding other code here.
> mpc5121_ads_init_early() is generic and could contain other
> things as well. I would vote for current version.

Do you have any plans to add any additional code?   If not, then I say
skip the middle-man.  If someone ever needs to do more, he can always
put that function back.

>> I'm pretty sure the compiler will optimize this to:
>>
>>     temp = (1000000000000UL / pixclock);
>>
>> so you may as well do it that way.
>
> ??
> 1000000000000 is _not_ UL, but UUL.

That's what I meant.  Actually, I think it's ULL.  Regardless, I think
the compiler will see the  "1000000000 ... * 1000" and just combine
them together.  You're not actually outsmarting the compiler.

>> > +       err = 100000000;
>>
>> Why do you assign err to this arbitrary value?
>
> Dunno. It is Freescale's code and I do not have time to check
> and understand each bit of it and to explain it.

*sigh*  You're not the first person to modify the DIU driver without
understanding what it all does.  I suspect that the original author
should have done this:

    err = -1;

because he wanted it to be the largest possible integer.

>> Do you really need to use reserve_bootmem?  Have you tried kmalloc or
>> alloc_pages_exact()?
>
> Yes. No, it is too early to use them here.

There was a recent change in the kernel that allows kmalloc to work
earlier than before.  Take a look at commit
85355bb272db31a3f2dd99d547eef794805e1319 ("powerpc: Fix mpic alloc
warning").

>> > +       mode = in_be32(diu_reg + 0x1c);
>> > +       if (mode != 1) {
>>
>> How can in_be32() return a -1?
>
> It is a 1, not -1. I will use appropriate macro here and also
> change to use a struct instead of adding offset to register base.

Sorry, I misread the code.  I could have sworn it read "mode != -1"

-- 
Timur Tabi
Linux kernel developer at Freescale