Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200822163250.63664-1-paul@crapouillou.net> <20200822163250.63664-7-paul@crapouillou.net>
In-Reply-To: <20200822163250.63664-7-paul@crapouillou.net>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Wed, 9 Sep 2020 13:38:25 +0200
Message-ID: <CACRpkdY8GnpK_Xiz6zRtj0G_LPKpLiopMsHR=9M3DU56eZMVzA@mail.gmail.com>
Subject: Re: [PATCH v2 6/6] drm/panel: Add Ilitek ILI9341 DBI panel driver
To:     Paul Cercueil <paul@crapouillou.net>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Noralf Tronnes <noralf@tronnes.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        od@zcrc.me,
        "open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Paul,

just a drive-by comment:

On Sat, Aug 22, 2020 at 6:33 PM Paul Cercueil <paul@crapouillou.net> wrote:

> +       gpiod_set_value_cansleep(priv->reset_gpiod, 0);
> +       usleep_range(20, 1000);
> +       gpiod_set_value_cansleep(priv->reset_gpiod, 1);

This implies that the reset line is active low.

I would specify in the DT GPIO handle that it is active low
and invert the above.

So:

reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;

gpiod_set_value_cansleep(priv->reset_gpiod, 1);
usleep_range(20, 1000);
gpiod_set_value_cansleep(priv->reset_gpiod, 0);

> +       priv->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(priv->reset_gpiod)) {
> +               dev_err(dev, "Couldn't get our reset GPIO\n");
> +               return PTR_ERR(priv->reset_gpiod);
> +       }

This would then fetch the GPIO as asserted (device in reset)
unless changed, but that may be the right thing to do actually.

> +static const struct ili9341_pdata yx240qv29_pdata = {
> +       .mode = { DRM_SIMPLE_MODE(240, 320, 37, 49) },
> +       .width_mm = 0, // TODO
> +       .height_mm = 0, // TODO

When nothing else works and data sheets are incomplete I
just take out a ruler and measure on the actual device.

Yours,
Linus Walleij