| Message ID | 20191213181325.26228-1-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: simple: Support reset GPIOs | expand |
Hi Sam, Sam Ravnborg <sam@ravnborg.org> wrote on Sat, 14 Dec 2019 11:23:54 +0100: > Hi Miquel. > > On Fri, Dec 13, 2019 at 07:13:25PM +0100, Miquel Raynal wrote: > > The panel common bindings provide a gpios-reset property which is > > active low by default. Let's support it in the simple driver. > > > > De-asserting the reset pin implies a physical high, which in turns is > > a logic low. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Code looks fine - but I fail to see why simple panels would require a > reset pin. > > Do you have any simple panels that requires this, or did you add it > because you saw it in the panel-common.yaml file? My hardware is: LVDS IP <----------> LVDS to RGB bridge <------------> Panel While there is a simple "RGB to LVDS" bridge driver, there is none doing the work the other way around. In my case, the bridge has a reset pin. As until now there is no way to represent the "LVDS to RGB" bridge and because the bindings already document such reset pin, I decided to add support for it in the simple panel driver. Thanks, Miquèl
Hi, On Fri, Dec 13, 2019 at 07:13:25PM +0100, Miquel Raynal wrote: > The panel common bindings provide a gpios-reset property which is > active low by default. Let's support it in the simple driver. > > De-asserting the reset pin implies a physical high, which in turns is > a logic low. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> The GPIOd API asks for a logical state, so it doesn't really matter what the polarity of the GPIO, OUT_LOW will always mean that the reset is deasserted (and thus, it will work even if the reset pin is active high). > --- > drivers/gpu/drm/panel/panel-simple.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 5d487686d25c..15dd495c347d 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -110,6 +110,7 @@ struct panel_simple { > struct i2c_adapter *ddc; > > struct gpio_desc *enable_gpio; > + struct gpio_desc *reset_gpio; > > struct drm_display_mode override_mode; > }; > @@ -433,12 +434,21 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > if (IS_ERR(panel->supply)) > return PTR_ERR(panel->supply); > > + panel->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(panel->reset_gpio)) { > + err = PTR_ERR(panel->reset_gpio); > + if (err != -EPROBE_DEFER) > + dev_err(dev, "failed to request reset pin: %d\n", err); > + return err; > + } > + However, I'm wondering if it wouldn't be better to just have the device maintained in reset at probe (so OUT_HIGH) and moved out of reset during either the prepare or enable callbacks. This is pretty much what is happening with the enable-gpios already. Also, panels usually need to wait for a minimum time after you deassert the reset line. How is that dealt with? I guess a good way to do that would be to add that duration to the panel description, since this is pretty much device specific. Thanks! Maxime
Hi Maxime, Maxime Ripard <maxime@cerno.tech> wrote on Mon, 16 Dec 2019 14:06:15 +0100: > Hi, > > On Fri, Dec 13, 2019 at 07:13:25PM +0100, Miquel Raynal wrote: > > The panel common bindings provide a gpios-reset property which is > > active low by default. Let's support it in the simple driver. > > > > De-asserting the reset pin implies a physical high, which in turns is > > a logic low. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > The GPIOd API asks for a logical state, so it doesn't really matter > what the polarity of the GPIO, OUT_LOW will always mean that the reset > is deasserted (and thus, it will work even if the reset pin is active > high). > > > --- > > drivers/gpu/drm/panel/panel-simple.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > index 5d487686d25c..15dd495c347d 100644 > > --- a/drivers/gpu/drm/panel/panel-simple.c > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > @@ -110,6 +110,7 @@ struct panel_simple { > > struct i2c_adapter *ddc; > > > > struct gpio_desc *enable_gpio; > > + struct gpio_desc *reset_gpio; > > > > struct drm_display_mode override_mode; > > }; > > @@ -433,12 +434,21 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > if (IS_ERR(panel->supply)) > > return PTR_ERR(panel->supply); > > > > + panel->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(panel->reset_gpio)) { > > + err = PTR_ERR(panel->reset_gpio); > > + if (err != -EPROBE_DEFER) > > + dev_err(dev, "failed to request reset pin: %d\n", err); > > + return err; > > + } > > + > > However, I'm wondering if it wouldn't be better to just have the > device maintained in reset at probe (so OUT_HIGH) and moved out of > reset during either the prepare or enable callbacks. > > This is pretty much what is happening with the enable-gpios already. > > Also, panels usually need to wait for a minimum time after you > deassert the reset line. How is that dealt with? > > I guess a good way to do that would be to add that duration to the > panel description, since this is pretty much device specific. What about the case were your Bootloader displays something and you don't want the panel to blink ? Right now I am just forcing the reset to be deasserted. Thanks, Miquèl
On Mon, Dec 16, 2019 at 02:10:36PM +0100, Miquel Raynal wrote: > > > drivers/gpu/drm/panel/panel-simple.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > index 5d487686d25c..15dd495c347d 100644 > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > @@ -110,6 +110,7 @@ struct panel_simple { > > > struct i2c_adapter *ddc; > > > > > > struct gpio_desc *enable_gpio; > > > + struct gpio_desc *reset_gpio; > > > > > > struct drm_display_mode override_mode; > > > }; > > > @@ -433,12 +434,21 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > if (IS_ERR(panel->supply)) > > > return PTR_ERR(panel->supply); > > > > > > + panel->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > > + GPIOD_OUT_LOW); > > > + if (IS_ERR(panel->reset_gpio)) { > > > + err = PTR_ERR(panel->reset_gpio); > > > + if (err != -EPROBE_DEFER) > > > + dev_err(dev, "failed to request reset pin: %d\n", err); > > > + return err; > > > + } > > > + > > > > However, I'm wondering if it wouldn't be better to just have the > > device maintained in reset at probe (so OUT_HIGH) and moved out of > > reset during either the prepare or enable callbacks. > > > > This is pretty much what is happening with the enable-gpios already. > > > > Also, panels usually need to wait for a minimum time after you > > deassert the reset line. How is that dealt with? > > > > I guess a good way to do that would be to add that duration to the > > panel description, since this is pretty much device specific. > > What about the case were your Bootloader displays something and you > don't want the panel to blink ? The Bootloader to Linux transition will make the panel blink already, since the display engine is going to be reset / reconfigured during the transition. The only way to implement this would be to implement properly the reset callbacks in all you display drivers to recreate the DRM state from the hardware state, and then you'll be able to just switch to the new buffer. Only Intel does this at the time though, and that's way outside of the scope of this patch... > Right now I am just forcing the reset to be deasserted. ... Especially since the very next line after your patch forces the panel to be disabled. Maxime
Hi Sam, Maxime Ripard <maxime@cerno.tech> wrote on Mon, 16 Dec 2019 14:27:32 +0100: > On Mon, Dec 16, 2019 at 02:10:36PM +0100, Miquel Raynal wrote: > > > > drivers/gpu/drm/panel/panel-simple.c | 12 +++++++++++- > > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > > index 5d487686d25c..15dd495c347d 100644 > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > @@ -110,6 +110,7 @@ struct panel_simple { > > > > struct i2c_adapter *ddc; > > > > > > > > struct gpio_desc *enable_gpio; > > > > + struct gpio_desc *reset_gpio; > > > > > > > > struct drm_display_mode override_mode; > > > > }; > > > > @@ -433,12 +434,21 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > > if (IS_ERR(panel->supply)) > > > > return PTR_ERR(panel->supply); > > > > > > > > + panel->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > > > + GPIOD_OUT_LOW); > > > > + if (IS_ERR(panel->reset_gpio)) { > > > > + err = PTR_ERR(panel->reset_gpio); > > > > + if (err != -EPROBE_DEFER) > > > > + dev_err(dev, "failed to request reset pin: %d\n", err); > > > > + return err; > > > > + } > > > > + > > > > > > However, I'm wondering if it wouldn't be better to just have the > > > device maintained in reset at probe (so OUT_HIGH) and moved out of > > > reset during either the prepare or enable callbacks. > > > > > > This is pretty much what is happening with the enable-gpios already. > > > > > > Also, panels usually need to wait for a minimum time after you > > > deassert the reset line. How is that dealt with? > > > > > > I guess a good way to do that would be to add that duration to the > > > panel description, since this is pretty much device specific. > > > > What about the case were your Bootloader displays something and you > > don't want the panel to blink ? > > The Bootloader to Linux transition will make the panel blink already, > since the display engine is going to be reset / reconfigured during > the transition. > > The only way to implement this would be to implement properly the > reset callbacks in all you display drivers to recreate the DRM state > from the hardware state, and then you'll be able to just switch to the > new buffer. > > Only Intel does this at the time though, and that's way outside of the > scope of this patch... > > > Right now I am just forcing the reset to be deasserted. > > ... Especially since the very next line after your patch forces the > panel to be disabled. > Is the addition of the reset support (as proposed by Maxime) interesting from your point of view? As you answered that you were not understanding the needs for such a change, I prefer to ask before spending more time on it. Thanks, Miquèl
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 5d487686d25c..15dd495c347d 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -110,6 +110,7 @@ struct panel_simple { struct i2c_adapter *ddc; struct gpio_desc *enable_gpio; + struct gpio_desc *reset_gpio; struct drm_display_mode override_mode; }; @@ -433,12 +434,21 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) if (IS_ERR(panel->supply)) return PTR_ERR(panel->supply); + panel->reset_gpio = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_LOW); + if (IS_ERR(panel->reset_gpio)) { + err = PTR_ERR(panel->reset_gpio); + if (err != -EPROBE_DEFER) + dev_err(dev, "failed to request reset pin: %d\n", err); + return err; + } + panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(panel->enable_gpio)) { err = PTR_ERR(panel->enable_gpio); if (err != -EPROBE_DEFER) - dev_err(dev, "failed to request GPIO: %d\n", err); + dev_err(dev, "failed to request enable pin: %d\n", err); return err; }
The panel common bindings provide a gpios-reset property which is active low by default. Let's support it in the simple driver. De-asserting the reset pin implies a physical high, which in turns is a logic low. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/gpu/drm/panel/panel-simple.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)