Re: [PATCH v2 1/2] ep93xx video platform support

On Monday, July 20, 2009 1:55 PM, Ryan Mallon wrote:

quoted

quoted

Updated version of the ep93xx video driver platform support patch with
fixes suggested by Hartley.

Signed-off-by: Ryan Mallon <redacted>
    

A couple more formatting issue comments below.

  

quoted

 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.

quoted

quoted

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

quoted

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 <hsweeten@visionengravers.com>
  

Sorry, your signed-off-by should have been there from the start.

quoted

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
  

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

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge