[U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes

[Helmut Raiger <helmut.raiger@hale.at>]

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.