[PATCH 3/5] powerpc/mpc5121: shared DIU framebuffer support
Timur Tabi
timur.tabi at gmail.com
Sat May 1 01:08:45 EST 2010
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
More information about the devicetree-discuss
mailing list