Re: [Linux-fbdev-devel] [PATCH v2 1/2] ep93xx video platform support

[Ryan M. <ry...@bl...>]

H Hartley Sweeten wrote:
> On Sunday, July 19, 2009 4:56 AM, Ryan Mallon wrote:
>   
>> Updated version of the ep93xx video driver platform support patch with
>> fixes suggested by Hartley.
>>
>> Signed-off-by: Ryan Mallon <ry...@bl...>
>>     
>
> A couple more formatting issue comments below.
>
>   
>>  static struct clk clk_uart1 = {
>>  	.sw_locked	= 1,
>> @@ -73,6 +73,13 @@ static struct clk clk_keypad = {
>>  	.set_rate	= set_keytchclk_rate,
>>  };
>>  
>> +static struct clk clk_video = {
>>     
>
> Unnecessary blank line.
>   
I like having blank lines between structures :-). I can remove it if you
want.
>>  
>> +static unsigned long calc_clk_div(unsigned long rate, int *psel, int *esel, 
>> +				  int *pdiv, int *div)
>> +{
>> +	unsigned long max_rate, best_rate = 0, 
>> +		actual_rate = 0, mclk_rate = 0, rate_err = -1;
>> +	int i, found = 0, __div = 0, __pdiv = 0;
>> +	
>> +	/* Don't exceed the maximum rate */
>> +	max_rate = max(max(clk_pll1.rate / 4, clk_pll2.rate / 4),
>> +		       (unsigned long)EP93XX_EXT_CLK_RATE / 4);
>> +	rate = min(rate, max_rate);
>> +	
>> +	/*
>> +	 * Try the two pll's and the external clock
>> +	 * Because the valid predividers are 2, 2.5 and 3, we multiply
>> +	 * all the clocks by 2 to avoid floating point math.
>> +	 *
>> +	 * This is based on the algorithm in the ep93xx raster guide:
>> +	 * http://be-a-maverick.com/en/pubs/appNote/AN269REV1.pdf
>> +	 *
>>     
>
> Unnecessary extra '*'.
>   
Again, I think this is okay, it spaces the comment out a bit and makes
it more readable (IMHO).
> Looks good other than that.
>
> I'm not sure which tag is appropriate for this patch.  So here's both.
>
> Signed-off-by: H Hartley Sweeten <hsw...@vi...>
>   
Sorry, your signed-off-by should have been there from the start.
> Acked-by: H Hartley Sweeten <hsw...@vi...>
>   
I'm not sure if you can have both a signed-off-by and acked-by tag on
the same patch. Russell?

I'm also putting together a short document on how to use the driver
since Daniele was having trouble getting it going. I would like to wait
until he has it fully working before this gets applied since that gives
us a third independent person who can confirm it all works.

Russell, will this eventually be okay to go through the ARM patch
system, or does it need to go via the framebuffer people?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

       Ryan Mallon                              Unit 5, Amuri Park
       Phone: +64 3 3779127                     404 Barbadoes St
       Fax:   +64 3 3779135                     PO Box 13 889
       Email: ry...@bl...             Christchurch, 8013
       Web:   http://www.bluewatersys.com       New Zealand
       Freecall Australia  1800 148 751         USA 1800 261 2934