On 08/22/2011 07:13 PM, Stefano Babic wrote:
> I see you updated the code synchronizing it with the linux driver. Add
> to the commit message the kernel version (better: the commit id) of the
> kernel you used as base for your changes, so that everybody in future
> can know where it comes from.
Ok.
>> +static struct ctfb_res_modes *mode;
>> +static struct ctfb_res_modes var_mode;
>> +
>> +
> One newline should be enough.
Sure.
>> - * @pixel_fmt: pixel format of buffer as FOURCC ASCII code.
> pixel_fmt is still in the parameter list, and di_panel should be added
> to the description.
I'll update it.
>> + reg = width + mode->left_margin + mode->right_margin - 1;
>> + if (reg> 1023) {
>> + debug("display width too large, coerced to 1023!");
>> + reg = 1023;
> I do not if it is better to try to adapt the values if the caller pass
> to the function wrong parameters. Probably it does not work. I think in
> these case it is better to output an error (print instead of debug) and
> return without with an error. What do you think ?
>
I put that code in as I tried to adjust the porches (left and right
margin) for our display.
If it is coerced the way I did, you'll never overwrite other bits in the
same register field, so
you can still see the effect it has. I preferred it during display
configuration, so I left it in.
We could output an error (not only during debug builds) but write the
registers anyway.
>> - switch (PANEL_TYPE) {
>> + switch (di_panel) {
>> case IPU_PANEL_SHARP_TFT:
>> reg_write(0x00FD0102L, SDC_SHARP_CONF_1);
>> reg_write(0x00F500F4L, SDC_SHARP_CONF_2);
>> reg_write(SDC_COM_SHARP | SDC_COM_TFT_COLOR, SDC_COM_CONF);
>> + /* TODO: probably IF_CONF must be adapted (see below)! */
> I do not understand this comment.
Each display specific define found an equivalent in the ctfb_res_modes
struct.
IF_CONF however is currently always 0, but might need adaption for SHARP
TFT panels, which
I could not test.
>> /* Init clocking */
>>
>> - /*
>> - * Calculate divider: fractional part is 4 bits so simply multiple by
>> - * 2^4 to get fractional part, as long as we stay under ~250MHz and on
>> - * i.MX31 it (HSP_CLK) is<= 178MHz. Currently 128.267MHz
>> + /* Calculate divider: the IPU receives its clock from the hsp divder */
>> + /* fractional part is 4 bits so simply multiple by 2^4 to get it
> Multiline comments, you should use the same style as in the removed lines.
Ok.
>> + div = ((mxc_get_clock(MXC_IPU_CLK)/1024)*(mode->pixclock/128))/476837;
> I try to understand this line. pixclock is in ps, as in kernel. I am
> missing something. I am expecting to have the same formula as in kernel,
> where I can read:
>
> div = clk_get_rate(ipu_clk) * 16 / pixel_clk;
>
> Where does 476837 come from ?
Well I already thought that might need another line of comment. In the
kernel the pixel_clk really
is a clock value, while it is a time (in pico seconds) in this driver. I
could have calculated the
pixel clock from the pixel time value first:
pixel_clk = 1e12 / mode->pixclock;
div = ipu_clk * 16 / pixel_clk;
I simply put that into one calculation:
div = ipu_clk * 16 / (1e12 / mode->pixclock)
or
div = ipu_clk * mode->pixclock / ~60e6;
but this would provoke an overflow problem during the multiplication, so
I split the division to
1024, 128 and 476837 which exactly gives 1e12 / 16 (~60e6). So I have 2
shifts a multiplication and a division.
Probably I simply put the 2 code lines above into a comment. The name
'pixel_clk' is actually misleading, but it sat there already. We could
use 'pixel_ps' in ctfb_res_modes instead?
>> +/*
>> + * The current implementation is only tested for GDF_16BIT_565RGB!
>> + * It was switched from the original CONFIG_LCD setup to CONFIG_VIDEO,
>> + * because the lcd code seemed loaded with color table stuff, that
>> + * does not relate to most modern TFTs. cfb_console.c looks more
>> + * straight forward.
>> + * This is the environment setting for the original setup
>> + "unknown=video=ctfb:x:240,y:320,depth:16,mode:0,pclk:185925,le:9,ri:17,
>> + up:7,lo:10,hs:1,vs:1,sync:100663296,vmode:0"
>> + "videomode=unknown"
> Multiline comment. As "original setup" you mean the setup if not
> CONFIG_DISPLAY_* was set, right ?
I'll fix the comment and yes you're right.
>> +void *video_hw_init(void)
>> {
>> - return ((panel_info.vl_col * panel_info.vl_row *
>> - NBITS(panel_info.vl_bpix)) / 8) + PAGE_SIZE;
>> + char *penv;
>> + u32 memsize;
>> + unsigned long t1, hsynch, vsynch;
>> + int bits_per_pixel, i, tmp, vesa_idx = 0, videomode;
>> +
>> + tmp = 0;
>> +
>> + videomode = CONFIG_SYS_DEFAULT_VIDEO_MODE;
> Ok. This is a way to fix qong/phycore after merging these patches, and
> avoid that they do not work anymore if the videomode variable is not
> set. I write it down...
>
Perfect. I could have done that already, but lacking hardware to test
with ...
> Anatolij, should be this code not general ? It is not strictly related
> to the i.MX. Should we put it in another place ?
>
I thought of that as-well.
>> +
>> + /* fill in Graphic device struct */
>> + sprintf(panel.modeIdent, "%dx%dx%d %ldkHz %ldHz",
>> + mode->xres, mode->yres,
>> + bits_per_pixel, (hsynch / 1000), (vsynch / 1000));
>> + printf("%s\n", panel.modeIdent);
> Should we not use "debug" instead ?
The kernel driver also outputs this during probe, but debug is fine with me.
Thanks for the review, I'll give it a few more days to settle and
deliver a V2 then.
Helmut
--
Scanned by MailScanner.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
