[PATCH] drm/panel: simple: Support reset GPIOs

[PATCH] drm/panel: simple: Support reset GPIOs

[Miquel Raynal]

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(-)

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;
 	}
 
-- 
2.20.1

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Sam Ravnborg]

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?

	Sam


> ---
>  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;
> +	}
> +
>  	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;
>  	}
>  
> -- 
> 2.20.1

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Miquel Raynal]

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

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Miquel Raynal]

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

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/panel: simple: Support reset GPIOs

[Miquel Raynal]

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