From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Wed, 11 Jul 2012 06:56:00 +0200
Subject: [U-Boot] [PATCH v2 10/19] tegra: Add LCD driver
In-Reply-To: <4FDA7794.3020006@wwwdotorg.org>
References: <1339604395-6621-1-git-send-email-sjg@chromium.org>
	<1339604395-6621-11-git-send-email-sjg@chromium.org>
	<4FDA7794.3020006@wwwdotorg.org>
Message-ID: <CAPnjgZ16PRB04dQPdKfzH19Sv5V5oSbpNpAZgj=t6UkQ9rYp2A@mail.gmail.com>
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 Stephen,

On Fri, Jun 15, 2012 at 1:45 AM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 06/13/2012 10:19 AM, Simon Glass wrote:
> > This driver supports driving a single LCD and providing a U-Boot console
> > on it.
>
> > +int fdt_decode_lcd(const void *blob, struct fdt_lcd *config)
>
> > +     fdtdec_decode_gpio(blob, display_node,
> "nvidia,backlight-vdd-gpios",
> > +                        &config->backlight_vdd);
>
> Given that's a power supply control, I expect this to be represented as
> a regulator in the DT, not as a simple GPIO (it could be controlled by a
> register bit in a PMU on some boards).
>

Hmm we don't have regulator support in U-Boot as yet. Any ideas? It is
possible that it might come towards the end of the year as part of some
other upstreaming work, but I am not sure. At lot of these sorts of things
are really hard in U-Boot until we have a device model.


> > +void lcd_enable(void)
> > +{
> > +     /*
> > +      * Backlight and power init will be done separately in
> > +      * tegra_lcd_check_next_stage(), which should be called in
> > +      * board_late_init().
> > +      *
> > +      * U-Boot code supports only colour depth, selected at compile
> time.
> > +      * The device tree setting should match this. Otherwise the display
> > +      * will not look right, and U-Boot may crash.
> > +      */
> > +     if (config.log2_bpp != LCD_BPP) {
> > +             printf("%s: Error: LCD depth configured in FDT (%d =
> %dbpp)"
> > +                     " must match setting of LCD_BPP (%d)\n", __func__,
> > +                    config.log2_bpp, config.bpp, LCD_BPP);
> > +     }
> > +}
>
> I wonder why even read the configuration from DT if the value can't be
> used?
>

The conflict is in one place and has a clear warning, so if U-Boot does
support this in the future, we can adjust it.


>
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>
> > @@ -43,6 +43,7 @@ static const char * const compat_names[COMPAT_COUNT] =
> {
>
> > +     COMPAT(NVIDIA_TEGRA20_DISPLAY, "nvidia,tegra20-display"),
>
> Surely all the DT plumbing for the display controller itself should be
> in the previous patch that adds the display controller driver.
>
> The LCD stuff that sits on top of that seems like it should be a
> separate node in DT, and the two patches kept much more separate.
>

I have made the LCD a subnode of the display controller. Let's see what
Thierry says about the kernel binding.

I will move this compatible string to the previous patch.

Regards,
Simon