* [PATCH] drm: panel: simple-panel: get the enable gpio as-is @ 2016-11-06 11:09 Icenowy Zheng 2016-11-07 10:12 ` Chen-Yu Tsai 0 siblings, 1 reply; 8+ messages in thread From: Icenowy Zheng @ 2016-11-06 11:09 UTC (permalink / raw) To: Thierry Reding, David Airlie Cc: Chen-Yu Tsai, linux-kernel, dri-devel, Icenowy Zheng The enable gpio of simple-panel may be used by a simplefb or other driver on the panel's display before the KMS driver get load. Get the GPIO as-is, so the panel won't be disabled, and the simplefb can work. Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> --- drivers/gpu/drm/panel/panel-simple.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 113db3c..ccee4c1 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) return PTR_ERR(panel->supply); panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", - GPIOD_OUT_LOW); + GPIOD_ASIS); if (IS_ERR(panel->enable_gpio)) { err = PTR_ERR(panel->enable_gpio); dev_err(dev, "failed to request GPIO: %d\n", err); -- 2.10.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is 2016-11-06 11:09 [PATCH] drm: panel: simple-panel: get the enable gpio as-is Icenowy Zheng @ 2016-11-07 10:12 ` Chen-Yu Tsai 2016-11-07 13:17 ` Thierry Reding 0 siblings, 1 reply; 8+ messages in thread From: Chen-Yu Tsai @ 2016-11-07 10:12 UTC (permalink / raw) To: Icenowy Zheng Cc: Thierry Reding, David Airlie, Chen-Yu Tsai, dri-devel, linux-kernel On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > The enable gpio of simple-panel may be used by a simplefb or other > driver on the panel's display before the KMS driver get load. > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > can work. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > --- > drivers/gpu/drm/panel/panel-simple.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 113db3c..ccee4c1 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > return PTR_ERR(panel->supply); > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > - GPIOD_OUT_LOW); > + GPIOD_ASIS); The GPIO requested as-is might be in input mode. You should change the gpiod_set_value calls to gpiod_direction_output calls. The later also allows you to give an initial value. Not sure if it checks for cansleep like the set_value calls though. Regards ChenYu > if (IS_ERR(panel->enable_gpio)) { > err = PTR_ERR(panel->enable_gpio); > dev_err(dev, "failed to request GPIO: %d\n", err); > -- > 2.10.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is 2016-11-07 10:12 ` Chen-Yu Tsai @ 2016-11-07 13:17 ` Thierry Reding 0 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2016-11-07 13:17 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: Icenowy Zheng, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1902 bytes --] On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > The enable gpio of simple-panel may be used by a simplefb or other > > driver on the panel's display before the KMS driver get load. > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > can work. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > --- > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > index 113db3c..ccee4c1 100644 > > --- a/drivers/gpu/drm/panel/panel-simple.c > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > return PTR_ERR(panel->supply); > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > - GPIOD_OUT_LOW); > > + GPIOD_ASIS); > > The GPIO requested as-is might be in input mode. You should change the > gpiod_set_value calls to gpiod_direction_output calls. The later also > allows you to give an initial value. Not sure if it checks for cansleep > like the set_value calls though. I'd prefer not to add gpiod_direction_output() calls outside of ->probe(). Instead, could we make this patch be smart about taking over from an earlier user? Could it read the current direction and value of the GPIO and not disable it if it had previously been enabled? And even if we go this extra mile there's a possibility that the GPIO was just left dangling by earlier software (or hardware) and leaving it on would actually be worse than turning the panel off. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is @ 2016-11-07 13:17 ` Thierry Reding 0 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2016-11-07 13:17 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: Icenowy Zheng, David Airlie, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1902 bytes --] On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > The enable gpio of simple-panel may be used by a simplefb or other > > driver on the panel's display before the KMS driver get load. > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > can work. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > --- > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > index 113db3c..ccee4c1 100644 > > --- a/drivers/gpu/drm/panel/panel-simple.c > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > return PTR_ERR(panel->supply); > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > - GPIOD_OUT_LOW); > > + GPIOD_ASIS); > > The GPIO requested as-is might be in input mode. You should change the > gpiod_set_value calls to gpiod_direction_output calls. The later also > allows you to give an initial value. Not sure if it checks for cansleep > like the set_value calls though. I'd prefer not to add gpiod_direction_output() calls outside of ->probe(). Instead, could we make this patch be smart about taking over from an earlier user? Could it read the current direction and value of the GPIO and not disable it if it had previously been enabled? And even if we go this extra mile there's a possibility that the GPIO was just left dangling by earlier software (or hardware) and leaving it on would actually be worse than turning the panel off. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is 2016-11-07 13:17 ` Thierry Reding @ 2016-11-07 14:26 ` Philipp Zabel -1 siblings, 0 replies; 8+ messages in thread From: Philipp Zabel @ 2016-11-07 14:26 UTC (permalink / raw) To: Thierry Reding; +Cc: Chen-Yu Tsai, Icenowy Zheng, dri-devel, linux-kernel Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > The enable gpio of simple-panel may be used by a simplefb or other > > > driver on the panel's display before the KMS driver get load. > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > can work. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > > --- > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > index 113db3c..ccee4c1 100644 > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > return PTR_ERR(panel->supply); > > > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > > - GPIOD_OUT_LOW); > > > + GPIOD_ASIS); > > > > The GPIO requested as-is might be in input mode. You should change the > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > allows you to give an initial value. Not sure if it checks for cansleep > > like the set_value calls though. > > I'd prefer not to add gpiod_direction_output() calls outside of > ->probe(). Instead, could we make this patch be smart about taking over > from an earlier user? Could it read the current direction and value of > the GPIO and not disable it if it had previously been enabled? Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c already does a similar thing. > And even if we go this extra mile there's a possibility that the GPIO > was just left dangling by earlier software (or hardware) and leaving it > on would actually be worse than turning the panel off. Is this something the encoder driver should communicate to the panel? That one will know whether its atomic_reset state is enabled or disabled. regards Philipp _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is @ 2016-11-07 14:26 ` Philipp Zabel 0 siblings, 0 replies; 8+ messages in thread From: Philipp Zabel @ 2016-11-07 14:26 UTC (permalink / raw) To: Thierry Reding; +Cc: Chen-Yu Tsai, Icenowy Zheng, dri-devel, linux-kernel Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > The enable gpio of simple-panel may be used by a simplefb or other > > > driver on the panel's display before the KMS driver get load. > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > can work. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > > --- > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > index 113db3c..ccee4c1 100644 > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > return PTR_ERR(panel->supply); > > > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > > - GPIOD_OUT_LOW); > > > + GPIOD_ASIS); > > > > The GPIO requested as-is might be in input mode. You should change the > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > allows you to give an initial value. Not sure if it checks for cansleep > > like the set_value calls though. > > I'd prefer not to add gpiod_direction_output() calls outside of > ->probe(). Instead, could we make this patch be smart about taking over > from an earlier user? Could it read the current direction and value of > the GPIO and not disable it if it had previously been enabled? Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c already does a similar thing. > And even if we go this extra mile there's a possibility that the GPIO > was just left dangling by earlier software (or hardware) and leaving it > on would actually be worse than turning the panel off. Is this something the encoder driver should communicate to the panel? That one will know whether its atomic_reset state is enabled or disabled. regards Philipp ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is 2016-11-07 14:26 ` Philipp Zabel @ 2016-11-08 11:30 ` Thierry Reding -1 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2016-11-08 11:30 UTC (permalink / raw) To: Philipp Zabel; +Cc: Chen-Yu Tsai, Icenowy Zheng, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3191 bytes --] On Mon, Nov 07, 2016 at 03:26:56PM +0100, Philipp Zabel wrote: > Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > > The enable gpio of simple-panel may be used by a simplefb or other > > > > driver on the panel's display before the KMS driver get load. > > > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > > can work. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > > > --- > > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > > index 113db3c..ccee4c1 100644 > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > > return PTR_ERR(panel->supply); > > > > > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > > > - GPIOD_OUT_LOW); > > > > + GPIOD_ASIS); > > > > > > The GPIO requested as-is might be in input mode. You should change the > > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > > allows you to give an initial value. Not sure if it checks for cansleep > > > like the set_value calls though. > > > > I'd prefer not to add gpiod_direction_output() calls outside of > > ->probe(). Instead, could we make this patch be smart about taking over > > from an earlier user? Could it read the current direction and value of > > the GPIO and not disable it if it had previously been enabled? > > Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c > already does a similar thing. > > > And even if we go this extra mile there's a possibility that the GPIO > > was just left dangling by earlier software (or hardware) and leaving it > > on would actually be worse than turning the panel off. > > Is this something the encoder driver should communicate to the panel? > That one will know whether its atomic_reset state is enabled or > disabled. At that point it's already too late. Configuration of the GPIO here is within the panel's ->probe() implementation, which is before it can ever even get attached to an encoder/connector. I can't think of a proper solution for this, but perhaps the best heuristic would be to request with GPIOD_ASIS and then query the direction. If it's configured as output we could assume that somebody's configured it explicitly and it has the correct value. That could still break existing use-cases, but it would at least allow basic hand-over. Also, if leaving the GPIO configured as-is causes glitches or other issues, then these are observable with the current code as well, until the panel driver takes over and disables everything. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is @ 2016-11-08 11:30 ` Thierry Reding 0 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2016-11-08 11:30 UTC (permalink / raw) To: Philipp Zabel; +Cc: Chen-Yu Tsai, Icenowy Zheng, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3191 bytes --] On Mon, Nov 07, 2016 at 03:26:56PM +0100, Philipp Zabel wrote: > Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > > The enable gpio of simple-panel may be used by a simplefb or other > > > > driver on the panel's display before the KMS driver get load. > > > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > > can work. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > > > --- > > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > > > > index 113db3c..ccee4c1 100644 > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > > > return PTR_ERR(panel->supply); > > > > > > > > panel->enable_gpio = devm_gpiod_get_optional(dev, "enable", > > > > - GPIOD_OUT_LOW); > > > > + GPIOD_ASIS); > > > > > > The GPIO requested as-is might be in input mode. You should change the > > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > > allows you to give an initial value. Not sure if it checks for cansleep > > > like the set_value calls though. > > > > I'd prefer not to add gpiod_direction_output() calls outside of > > ->probe(). Instead, could we make this patch be smart about taking over > > from an earlier user? Could it read the current direction and value of > > the GPIO and not disable it if it had previously been enabled? > > Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c > already does a similar thing. > > > And even if we go this extra mile there's a possibility that the GPIO > > was just left dangling by earlier software (or hardware) and leaving it > > on would actually be worse than turning the panel off. > > Is this something the encoder driver should communicate to the panel? > That one will know whether its atomic_reset state is enabled or > disabled. At that point it's already too late. Configuration of the GPIO here is within the panel's ->probe() implementation, which is before it can ever even get attached to an encoder/connector. I can't think of a proper solution for this, but perhaps the best heuristic would be to request with GPIOD_ASIS and then query the direction. If it's configured as output we could assume that somebody's configured it explicitly and it has the correct value. That could still break existing use-cases, but it would at least allow basic hand-over. Also, if leaving the GPIO configured as-is causes glitches or other issues, then these are observable with the current code as well, until the panel driver takes over and disables everything. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-08 11:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-06 11:09 [PATCH] drm: panel: simple-panel: get the enable gpio as-is Icenowy Zheng 2016-11-07 10:12 ` Chen-Yu Tsai 2016-11-07 13:17 ` Thierry Reding 2016-11-07 13:17 ` Thierry Reding 2016-11-07 14:26 ` Philipp Zabel 2016-11-07 14:26 ` Philipp Zabel 2016-11-08 11:30 ` Thierry Reding 2016-11-08 11:30 ` Thierry Reding
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.