Received-SPF: pass (google.com: domain of linux-kernel+bounces-18973-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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>
In-Reply-To: <CABGWkvoNuJNmxhrOE6csE2mGwF50ou-jx-kpmH-oQ2zBgzLrSg@mail.gmail.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Sun, 7 Jan 2024 21:01:49 +0100
Message-ID: <CACRpkdZJF-WE8oTi3RXpX_+W+D6bV_o2Nt1ikRbErR6pBc2OJg@mail.gmail.com>
Subject: Re: [PATCH v4 8/8] drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK
To: Dario Binacchi <dario.binacchi@amarulasolutions.com>
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 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 rese=
t 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 *ds=
i)
>         if (ret)
>                 return ret;
>
> -       nt->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_AS=
IS);
> +       nt->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OU=
T_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.bina=
cchi@amarulasolutions.com/

this doesn't even contain a reset GPIO, so nothing will happen
at all?

> 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-re=
va09.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.dt=
s
> +++ b/arch/arm/boot/dts/st/stm32f769-disco-mb1225-revb03-mb1166-reva09.dt=
s
> @@ -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?

> --- 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.

> 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