On Tue, Dec 23, 2008 at 3:47 AM, Eric Miao [off-list ref] wrote:

On Sun, Dec 21, 2008 at 6:59 PM, Jaya Kumar [off-list ref] wrote:

quoted

This patch adds support for the AM300 platform driver.

Patch looks good, I feel I'm nit-picking. Anyway , see my comments below:

quoted

+/* register offsets for gpio control */
+#define PWR_GPIO_PIN 16
+#define CFG_GPIO_PIN 17
+#define RDY_GPIO_PIN 32
+#define DC_GPIO_PIN 48
+#define RST_GPIO_PIN 49
+#define LED_GPIO_PIN 51
+#define RD_GPIO_PIN 74
+#define WR_GPIO_PIN 75
+#define CS_GPIO_PIN 76
+#define IRQ_GPIO_PIN 77
+
+/* hdb bus */
+#define DB0_GPIO_PIN 58
+#define DB15_GPIO_PIN 73

indentation?

Ok, will fix.

quoted

+       gpio_set_value(RST_GPIO_PIN, 0);
+       mdelay(10);

which deserves a msleep(10) if the timing is relaxed enough.

True. Will fix.

quoted

+       gpio_set_value(RST_GPIO_PIN, 1);
+       mdelay(10);
+       am300_wait_for_rdy(par);
+
+       return 0;
+
+err_req_gpio:
+       while (i > 0)
+               gpio_free(gpios[i--]);
+
+       return err;
+}
+
+static int am300_init_board(struct broadsheetfb_par *par)
+{
+       int ret;
+
+       /* setup pins */
+       ret = am300_init_gpio_regs(par);
+       if (ret)
+               return ret;
+
+       return 0;
+}

which can be shortened to one statement 'return am300_init_gpio_regs(par)'

Yes, sorry, there was other stuff I had been doing in that function
while trying to trim down the board init to the minimum needed for
things to work. Okay, will fix.

Thanks,
jaya

------------------------------------------------------------------------------