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

[Timur Tabi <timur.tabi@gmail.com>]

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

>> How about just doing this?
>>
>> .init_early =A0 =A0 =A0 =A0 =A0 =A0 =3D 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:
>>
>> =A0 =A0 temp =3D (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.

>> > + =A0 =A0 =A0 err =3D 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 =3D -1;

because he wanted it to be the largest possible integer.

>> Do you really need to use reserve_bootmem? =A0Have 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").

>> > + =A0 =A0 =A0 mode =3D in_be32(diu_reg + 0x1c);
>> > + =A0 =A0 =A0 if (mode !=3D 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 !=3D -1"

--=20
Timur Tabi
Linux kernel developer at Freescale