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