[PATCH 1/5] Add HP iPAQ RX1950 machine support

[anarsoul@gmail.com (Vasily Khoruzhick)]

? ????????? ?? 11 ??? 2010 03:15:42 ????? Ben Dooks ???????:
> 
> firstly, do you need this mapping for anything and secondly can
> you check the indentation on it if it is staying.

Ok, I don't need them.
> > +static struct s3c2410fb_display rx1950_display __initdata = {
> 
> don't think this is allowed to be __initdata at the moment.

Ok
> > +		/* Wait a bit here... */
> > +		mdelay(100);
> 
> would msleep() be a better thing to do herer.

Nope, I prefer to use mdelay here, msleep doesn't do well for
some reason (there's a garbage on screen in case of msleep usage).
And actually wait for hsync/vsync should replace
this mdelay, but it isn't implemented at the moment.
 
> 
> i think S3C_GPIO_SFN(2) would do for all of these, and would allow
> you to loop over the range.

Ok

> 
> at-least get a space between { and KEY_
> would prefer to see properly named initialisers.

Ok
 
> > +	s3c_irq_wake(IRQ_EINT0, 1);
> > +}
> 
> surely this should be propertyu of a key in the keys table?

Ok
 
> > +	gpio_request(S3C2410_GPJ(1), "MMC power");
> > +	gpio_direction_output(S3C2410_GPJ(1), 0);
> 
> you might want to WARN_ON() the result of gpio_request().

Ok

Thanks for review.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100511/75e3d1ce/attachment.sig>