Received-SPF: pass (google.com: domain of linux-kernel+bounces-20072-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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>
 <CABGWkvoNuJNmxhrOE6csE2mGwF50ou-jx-kpmH-oQ2zBgzLrSg@mail.gmail.com> <CACRpkdZJF-WE8oTi3RXpX_+W+D6bV_o2Nt1ikRbErR6pBc2OJg@mail.gmail.com>
In-Reply-To: <CACRpkdZJF-WE8oTi3RXpX_+W+D6bV_o2Nt1ikRbErR6pBc2OJg@mail.gmail.com>
From: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Date: Mon, 8 Jan 2024 21:12:37 +0100
Message-ID: <CABGWkvoHuzTPNhh54VihNJ4PtSTF9sRLiup6PRNqG5uoHfJc_A@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 Sun, Jan 7, 2024 at 9:02=E2=80=AFPM Linus Walleij <linus.walleij@linaro.=
org> wrote:
>
> On Sat, Jan 6, 2024 at 12:07=E2=80=AFPM Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
>
> > 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 re=
set 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);
>
>
> This is fine, because we later on toggle reset in nt35510_power_on(),
> this just asserts the reset signal already in probe.
>
> I don't see why this would make a difference though?
>
> Is it to make the reset delete artifacts from the screen?
>
> Just explain it in the commit message.
>
> It is a bit confusing when I look at your DTS patch:
>
> https://lore.kernel.org/linux-arm-kernel/20240104084206.721824-7-dario.bi=
nacchi@amarulasolutions.com/
>
> this doesn't even contain a reset GPIO, so nothing will happen
> at all?
+++ b/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-mb1166-reva09.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 Dario Binacchi <dario.binacchi@amarulasolutions.com>
+ */
+
+#include "stm32f769-disco.dts"
+

The GPIO is contained in the stm32f769-disco.dts:

panel0: panel-dsi@0 {
        compatible =3D "orisetech,otm8009a";
        reg =3D <0>; /* dsi virtual channel (0..3) */
        reset-gpios =3D <&gpioj 15 GPIO_ACTIVE_LOW>;
        power-supply =3D <&vcc_3v3>;
        backlight =3D <&panel_backlight>;
       status =3D "okay";
...
};

>
> > 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-=
reva09.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;
>
> But this achieves the *opposite* of what you do in the
> other patch. This sets the reset line de-asserted since it is
> active low.
>
> Did you add the reset line to your device tree and forgot to
> set it as GPIO_ACTIVE_LOW perhaps?

panel0: panel-dsi@0 {
        compatible =3D "orisetech,otm8009a";
        reg =3D <0>; /* dsi virtual channel (0..3) */
        reset-gpios =3D <&gpioj 15 GPIO_ACTIVE_LOW>;

>
> > --- 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,
>
> To be honest this is what I use a lot of the time, with non-strict
> pin control you can set up default GPIO values using the pin control
> device tree, and it's really simple and easy to read like that since e.g.
> in this case you set it from the panel device node which is what
> is also consuming the GPIO, so very logical. So I
> would certainly remove this .strict setting, but maybe Alexandre
> et al have a strong opinion about it.

I will send a separate RFC PATCH.

Thanks and regards,
Dario Binacchi

>
> > 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?
>
> It's fine either way, but just use GPIOD_OUT_HIGH and I can test
> it on my system as well, I think it's fine.
>
> 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