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

[H H. S. <har...@vi...>]

On Monday, July 20, 2009 1:55 PM, 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.

Only because all the other clk's don't have a blank line separating them.

>>>  
>>> +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).

That's fine.

>> 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 guess it should be a Signed-off-by, you will need an ack from someone
else (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.

I was going to suggest that.  Or you could submit a patch for the
snappercl15 showing an example usage.

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

I think Russell could ok this part of the support.  The actual driver
will probably need to go via the framebuffer people.

I'm reviewing the driver right now.  You should have my Signed-off-by
later today.

Regards,
Hartley