Re: [Linux-fbdev-devel] [REVIEW PATCH 9/9] DSS: support for OMAP3 SDP board

[Jarkko N. <jar...@no...>]

On Tue, 04 Nov 2008 18:10:45 +0200
"ext Tomi Valkeinen" <tom...@no...> wrote:

> Signed-off-by: Tomi Valkeinen <tom...@no...>
> ---
> 
>  arch/arm/mach-omap2/board-3430sdp.c |  234 +++++++++++++++++++++++++++++++++--
>  drivers/video/omap2/Kconfig         |    7 +
>  drivers/video/omap2/Makefile        |    1 
>  drivers/video/omap2/panel-sdp3430.c |  110 ++++++++++++++++
>  4 files changed, 340 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/video/omap2/panel-sdp3430.c
> 

> +static void __init display_init(void)
>
I would use unique 3430sdp_display_init even the function is static.
Cleaner symbol names and cleaner oops dumps.

> +{
> +	int r;
> +
> +	enable_gpio    = SDP3430_LCD_PANEL_ENABLE_GPIO;
> +	backlight_gpio = SDP3430_LCD_PANEL_BACKLIGHT_GPIO;
> +
> +	r = gpio_request(enable_gpio, "OMAP SDP LCD Reset GPIO");
> +	if (r) {
> +		printk(KERN_ERR "failed to get LCD reset GPIO\n");
> +		goto err0;
> +	}
> +
> +	r = gpio_request(backlight_gpio, "OMAP SDP LCD Backlight GPIO");
> +	if (r) {

How about labels "LCD reset" and "LCD backlight" (actually bit too
long).

> +static int panel_enable_lcd(struct omap_display *display)
> +static void panel_disable_lcd(struct omap_display *display)
> +static int panel_enable_dvi(struct omap_display *display)

Would it better to rename these as sdp3430_enable_lcd etc since
otherwise looks somewhat confusing to see them in board-3430sdp.c where
there is also panel-sdp3430.c?


Jarkko