[U-Boot] [PATCH V2 5/7] cm-t35: add support for dvi displays

Wolfgang Denk
2013-01-29T14:00:13Z

Dear Nikita Kiryanov,

In message <1359463349-11649-6-git-send-email-nikita@compulab.co.il> you wrote:
> Add support for dvi displays with user selectable dvi presets.
...
> --- a/board/cm_t35/cm_t35.c
> +++ b/board/cm_t35/cm_t35.c
> @@ -216,6 +216,9 @@ static void cm_t3x_set_common_muxconf(void)
> /* SB-T35 Ethernet */
> MUX_VAL(CP(GPMC_NCS4), (IEN | PTU | EN | M0)); /*GPMC_nCS4*/
>
> + /* DVI enable */
> + MUX_VAL(CP(GPMC_NCS3), (IDIS | PTU | DIS | M4));/*GPMC_nCS3*/

Is it intentional and correct to always enable DVI, even when this is
not configured by the user?

> +/*
> + * The frame buffer is allocated before we have the chance to parse user input.

This seems broken to me.

Please explain why you think so?

> + * vl_{col | row} to the maximal resolution supported by OMAP3.
> + */
> +vidinfo_t panel_info = {
> + .vl_col = 1400,
> + .vl_row = 1050,
> + .vl_bpix = LCD_BPP,
> + .cmap = (ushort *)0x80100000,

Can we please avoid such hard coded magic numbers?

> +/*
> + * env_parse_displaytype() - parse display type.
> + *
> + * Parses the environment variable "displaytype", which contains the
> + * name of the display type or preset, in which case it applies its
> + * configurations.

So we have yet another custom implementation for setting the display
type? Can we please agree on using common standard methods? thanks.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A father doesn't destroy his children.
-- Lt. Carolyn Palamas, "Who Mourns for Adonais?",
stardate 3468.1.