Received-SPF: pass (google.com: domain of linux-kernel+bounces-18557-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240104084206.721824-1-dario.binacchi@amarulasolutions.com>
 <20240104084206.721824-9-dario.binacchi@amarulasolutions.com> <CACRpkda+=Zq+v-505O3pHazKzukSifBnNx_CPKbKd2JH-KYpYg@mail.gmail.com>
In-Reply-To: <CACRpkda+=Zq+v-505O3pHazKzukSifBnNx_CPKbKd2JH-KYpYg@mail.gmail.com>
From: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date: Sat, 6 Jan 2024 12:07:00 +0100
Message-ID: <CABGWkvoNuJNmxhrOE6csE2mGwF50ou-jx-kpmH-oQ2zBgzLrSg@mail.gmail.com>
Subject: Re: [PATCH v4 8/8] drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK
To: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-amarula@amarulasolutions.com, 
	Alexandre Torgue <alexandre.torgue@foss.st.com>, Daniel Vetter <daniel@ffwll.ch>, 
	David Airlie <airlied@gmail.com>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Sam Ravnborg <sam@ravnborg.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 5, 2024 at 8:09=E2=80=AFPM Linus Walleij <linus.walleij@linaro.=
org> wrote:
>
> On Thu, Jan 4, 2024 at 9:42=E2=80=AFAM Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
>
> > The initialization commands are taken from the STMicroelectronics drive=
r
> > found at [1].
> > To ensure backward compatibility, flags have been added to enable gamma
> > correction setting and display control. In other cases, registers have
> > been set to their default values according to the specifications found
> > in the datasheet.
> >
> > [1] https://github.com/STMicroelectronics/STM32CubeF7/blob/master/Drive=
rs/BSP/Components/nt35510/
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > ---
> >
> > (no changes since v2)
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> (also tested to not regress my hardware)

After submitting v4, I tested the driver under different conditions,
i. e. without enabling display support in
U-Boot (I also implemented a version for U-Boot, which I will send
only after this series is merged into
the Linux kernel). In that condition I encountered an issue with the reset =
pin.

The minimal fix, would be this:

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
index c85dd0d0829d..89ba99763468 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
@@ -1133,7 +1133,7 @@ static int nt35510_probe(struct mipi_dsi_device *dsi)
        if (ret)
                return ret;

-       nt->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS=
);
+       nt->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_=
HIGH);
        if (IS_ERR(nt->reset_gpio)) {
                dev_err(dev, "error getting RESET GPIO\n");
                return PTR_ERR(nt->reset_gpio);

With the doubt that this might cause a regression in your use case.

I then tried modifying the device tree instead of the nt35510 driver.
In the end, I arrived
at this patch that leaves me with some doubts, especially regarding
the strict option.

diff --git a/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-mb1166-reva=
09.dts
b/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-m>
index 014cac192375..32ed420a6cbf 100644
--- a/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-mb1166-reva09.dts
+++ b/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-mb1166-reva09.dts
@@ -13,6 +13,17 @@ &panel0 {
        compatible =3D "frida,frd400b25025", "novatek,nt35510";
        vddi-supply =3D <&vcc_3v3>;
        vdd-supply =3D <&vcc_3v3>;
+       pinctrl-0 =3D <&panel_reset>;
+       pinctrl-names =3D "default";
        /delete-property/backlight;
        /delete-property/power-supply;
 };
+
+&pinctrl {
+       panel_reset: panel-reset {
+               pins1 {
+                       pinmux =3D <STM32_PINMUX('J', 15, GPIO)>;
+                       output-high;
+               };
+       };
+};

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c
b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 346a31f31bba..6f055f7f96a2 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -895,7 +895,6 @@ static const struct pinmux_ops stm32_pmx_ops =3D {
        .set_mux                =3D stm32_pmx_set_mux,
        .gpio_set_direction     =3D stm32_pmx_gpio_set_direction,
        .request                =3D stm32_pmx_request,
-       .strict                 =3D true,
 };

Another option to fix my use case without introducing regressions
could be to add a
new property to the device tree that suggests whether to call
devm_gpiod_get_optional()
with the GPIOD_ASIS or GPIOD_OUT_HIGH parameter.

What is your opinion?

Thanks and regards,
Dario Binacchi

>
> Yours,
> Linus Walleij


--=20

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com