On Sun, Jan 7, 2024 at 9:02 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > On Sat, Jan 6, 2024 at 12:07 PM Dario Binacchi > <dario.binacchi@xxxxxxxxxxxxxxxxxxxx> 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 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 = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS); > > + nt->reset_gpio = 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.binacchi@xxxxxxxxxxxxxxxxxxxx/ > > 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@xxxxxxxxxxxxxxxxxxxx> + */ + +#include "stm32f769-disco.dts" + The GPIO is contained in the stm32f769-disco.dts: panel0: panel-dsi@0 { compatible = "orisetech,otm8009a"; reg = <0>; /* dsi virtual channel (0..3) */ reset-gpios = <&gpioj 15 GPIO_ACTIVE_LOW>; power-supply = <&vcc_3v3>; backlight = <&panel_backlight>; status = "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 = "frida,frd400b25025", "novatek,nt35510"; > > vddi-supply = <&vcc_3v3>; > > vdd-supply = <&vcc_3v3>; > > + pinctrl-0 = <&panel_reset>; > > + pinctrl-names = "default"; > > /delete-property/backlight; > > /delete-property/power-supply; > > }; > > + > > +&pinctrl { > > + panel_reset: panel-reset { > > + pins1 { > > + pinmux = <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 = "orisetech,otm8009a"; reg = <0>; /* dsi virtual channel (0..3) */ reset-gpios = <&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 = { > > .set_mux = stm32_pmx_set_mux, > > .gpio_set_direction = stm32_pmx_gpio_set_direction, > > .request = stm32_pmx_request, > > - .strict = 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 -- Dario Binacchi Senior Embedded Linux Developer dario.binacchi@xxxxxxxxxxxxxxxxxxxx __________________________________ Amarula Solutions SRL Via Le Canevare 30, 31100 Treviso, Veneto, IT T. +39 042 243 5310 info@xxxxxxxxxxxxxxxxxxxx www.amarulasolutions.com
