From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nikita Kiryanov <nikita@compulab.co.il>
Date: Tue, 29 Jan 2013 17:04:55 +0200
Subject: [U-Boot] [PATCH V2 5/7] cm-t35: add support for dvi displays
In-Reply-To: <20130129140013.3496E2A08AC@gemini.denx.de>
References: <1359463349-11649-1-git-send-email-nikita@compulab.co.il>
	<1359463349-11649-6-git-send-email-nikita@compulab.co.il>
	<20130129140013.3496E2A08AC@gemini.denx.de>
Message-ID: <5107E517.20106@compulab.co.il>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Wolfgang Denk,

On 01/29/2013 04:00 PM, Wolfgang Denk wrote:
> 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?

This simply prepares the mux that makes it possible to enable DVI.
The actual enabling of DVI is indeed conditional on user setup.

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

Agreed.

>
> Please explain why you think so?

This isn't just my impression. A patch was submitted by Stephen Warren
to address this issue (http://patchwork.ozlabs.org/patch/212378/), but
it isn't in mainline yet.

>
>> + * 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?

OK.

>
>
>> +/*
>> + * 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.

Most of what is custom in this patchset is parsing of environment
variables. For actually setting up the DSS I do use common code
(specifically omap3_dss_panel_config() and related structs/defines).

>
>
> Best regards,
>
> Wolfgang Denk
>


-- 
Regards,
Nikita.