[U-Boot] [PATCH 8/9 v2] video: add a i.MX31 framebuffer driver

[Anatolij Gustschin]

Guennadi Liakhovetski wrote:
> Add a driver for the Synchronous Display Controller and the Display
> Interface on i.MX31, using IPU for DMA channel setup. So far only
> displaying of bitmaps is supported, no text output.
> 
> Signed-off-by: Guennadi Liakhovetski <lg at denx.de>

mostly looks good, please see some comments below.

> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> new file mode 100644
> index 0000000..d9b673e
> --- /dev/null
> +++ b/drivers/video/mx3fb.c
<snip>
> +/**
> + * sdc_init_panel() - initialize a synchronous LCD panel.
> + * @mx3fb:		mx3fb context.
> + * @panel:		panel type.
> + * @pixel_clk:		desired pixel clock frequency in Hz.
> + * @width:		width of panel in pixels.
> + * @height:		height of panel in pixels.
> + * @pixel_fmt:		pixel format of buffer as FOURCC ASCII code.
> + * @h_start_width:	number of pixel clocks between the HSYNC signal pulse
> + *			and the start of valid data.
> + * @h_sync_width:	width of the HSYNC signal in units of pixel clocks.
> + * @h_end_width:	number of pixel clocks between the end of valid data
> + *			and the HSYNC signal for next line.
> + * @v_start_width:	number of lines between the VSYNC signal pulse and the
> + *			start of valid data.
> + * @v_sync_width:	width of the VSYNC signal in units of lines
> + * @v_end_width:	number of lines between the end of valid data and the
> + *			VSYNC signal for next frame.
> + * @sig:		bitfield of signal polarities for LCD interface.
> + * @return:		0 on success or negative error code on failure.
> + */
> +static int sdc_init_panel(u16 width, u16 height, enum pixel_fmt pixel_fmt)

function parameters description is from the Linux driver,
however most of them are not present here in U-Boot driver.
Is there any reason you leave the original description here?
Also please replace starting '/**' with '/*'.

<snip>
> +static void ipu_ch_param_set_size(union chan_param_mem *params,
> +				  uint32_t pixel_fmt, uint16_t width,
> +				  uint16_t height, uint16_t stride)
> +{
> +	params->pp.fw		= width - 1;
> +	params->pp.fh_l		= height - 1;
> +	params->pp.fh_h		= (height - 1) >> 8;
> +	params->pp.sl		= stride - 1;
> +
> +	/* See above, for further formats see tge Linux driver */

s/tge/the/
?

<snip>
> +/**
> + * ipu_enable_channel() - enable an IPU channel.
> + * @channel:	channel ID.
> + * @return:	0 on success or negative error code on failure.
> + */
> +static int ipu_enable_channel(enum ipu_channel channel)

just use '/*' here, too.

<snip>
> +/**
> + * mx3fb_set_par() - set framebuffer parameters and change the operating mode.
> + * @fbi:	framebuffer information pointer.
> + * @return:	0 on success or negative error code on failure.
> + */
> +static int mx3fb_set_par(void *fbmem)

use '/*' here, too. Thanks!
Also @fbi description doesn't apply here, 'fbmem' is frame buffer
memory pointer.

Best regards,
Anatolij