[RFC PATCH v1 0/3] drm/panel: mantix panel reset fixes

[RFC PATCH v1 0/3] drm/panel: mantix panel reset fixes

[Guido Günther]

Posting as RFC since I'm not sure how to handle the bindings, please see below.

The first patch in this series fixes dereferencing a NULL mode in the error
path. The second one extends the resets to not only reset RESX but also TP_RSTN
since otherwise the display will stay completely blank. I didn't spot that
before initial submission since the reset line was bound to the touch
controller and although that failed to probe it came far enough to deassert the
reset line (at an arbitrary point in time during boot) and hence we got a
picture. Since touch and panel are on the same IC they're not completely
independent and i might have to turn the whole thing into an MFD at some point
but this series gets the panel to a reliably working state on boot and on fb
blank/unblank.

Since the reset-gpios are active low we can deassert in prepare and assert in
unprepare simplifying the code making sure lines are kept low when the
panel is off.

As for the bindings it seems I can't override the maxItems of reset-gpios since
that is already set in bindings/display/panel/panel-common.yaml to maxItems:
1`.  Is there a way to do that instead of side stepping the issue and using
a different name or using enable-gpios for one of them or forcing everyone else
to set `maxItems: 1`?

The binding were not part of a stable kernel so I hope it's okay to not worry
about backward compatibility.

Guido Günther (3):
  drm/panel: mantix: Don't dereference NULL mode
  drm/panel: mantix: Fix panel reset
  dt-binding: display: Require two rests on mantix panel

 .../display/panel/mantix,mlaf057we51-x.yaml   |  7 +++-
 .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 39 ++++++++++++-------
 2 files changed, 30 insertions(+), 16 deletions(-)

-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH v1 1/3] drm/panel: mantix: Don't dereference NULL mode

[Guido Günther]

Don't dereference mode which was just NULL checked.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
index 3482e28e30fc..4a7fbf64bb7a 100644
--- a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
+++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
@@ -204,7 +204,7 @@ static int mantix_get_modes(struct drm_panel *panel,
 	if (!mode) {
 		dev_err(ctx->dev, "Failed to add mode %ux%u@%u\n",
 			default_mode.hdisplay, default_mode.vdisplay,
-			drm_mode_vrefresh(mode));
+			drm_mode_vrefresh(&default_mode));
 		return -ENOMEM;
 	}
 
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH v1 2/3] drm/panel: mantix: Fix panel reset

[Guido Günther]

The mantix panel needs two reset lines (RESX and TP_RSTN) deasserted to
output an image. Only deasserting RESX is not enough and the display
will stay blank. Deassert in prepare() and assert in unprepare() to keep
device held in reset when off.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 37 ++++++++++++-------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
index 4a7fbf64bb7a..29d28c63bb72 100644
--- a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
+++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
@@ -23,10 +23,13 @@
 #define MANTIX_CMD_OTP_STOP_RELOAD_MIPI 0x41
 #define MANTIX_CMD_INT_CANCEL           0x4C
 
+#define MANTIX_NUM_RESETS 2
+
 struct mantix {
 	struct device *dev;
 	struct drm_panel panel;
-	struct gpio_desc *reset_gpio;
+	/* RESX and TP_RSTN */
+	struct gpio_descs *reset_gpios;
 
 	struct regulator *avdd;
 	struct regulator *avee;
@@ -122,8 +125,13 @@ static int mantix_disable(struct drm_panel *panel)
 
 static int mantix_unprepare(struct drm_panel *panel)
 {
+	DECLARE_BITMAP(asserted_values, BITS_PER_TYPE(1));
 	struct mantix *ctx = panel_to_mantix(panel);
 
+	gpiod_set_array_value_cansleep(ctx->reset_gpios->ndescs,
+				       ctx->reset_gpios->desc,
+				       ctx->reset_gpios->info, asserted_values);
+
 	regulator_disable(ctx->avee);
 	regulator_disable(ctx->avdd);
 	/* T11 */
@@ -137,6 +145,7 @@ static int mantix_unprepare(struct drm_panel *panel)
 
 static int mantix_prepare(struct drm_panel *panel)
 {
+	DECLARE_BITMAP(deasserted_values, BITS_PER_TYPE(0));
 	struct mantix *ctx = panel_to_mantix(panel);
 	int ret;
 
@@ -165,14 +174,11 @@ static int mantix_prepare(struct drm_panel *panel)
 		return ret;
 	}
 
-	/* T3+T5 */
-	usleep_range(10000, 12000);
-
-	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
-	usleep_range(5150, 7000);
-
-	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
-
+	/* T3 + T4 + time for voltage to become stable: */
+	usleep_range(6000, 7000);
+	gpiod_set_array_value_cansleep(ctx->reset_gpios->ndescs,
+				       ctx->reset_gpios->desc,
+				       ctx->reset_gpios->info, deasserted_values);
 	/* T6 */
 	msleep(50);
 
@@ -236,10 +242,15 @@ static int mantix_probe(struct mipi_dsi_device *dsi)
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(ctx->reset_gpio)) {
-		dev_err(dev, "cannot get reset gpio\n");
-		return PTR_ERR(ctx->reset_gpio);
+	ctx->reset_gpios = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ctx->reset_gpios)) {
+		dev_err(dev, "cannot get reset gpios\n");
+		return PTR_ERR(ctx->reset_gpios);
+	}
+
+	if (ctx->reset_gpios->ndescs != MANTIX_NUM_RESETS) {
+		dev_err(dev, "Need exactly %d reset-gpios\n", MANTIX_NUM_RESETS);
+		return -EINVAL;
 	}
 
 	mipi_dsi_set_drvdata(dsi, ctx);
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Guido Günther]

We need to reset both for the panel to show an image.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../bindings/display/panel/mantix,mlaf057we51-x.yaml       | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
index 937323cc9aaa..ba5a18fac9f9 100644
--- a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
+++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
@@ -35,7 +35,9 @@ properties:
   vddi-supply:
     description: 1.8V I/O voltage supply
 
-  reset-gpios: true
+  reset-gpios:
+    minItems: 2
+    maxItems: 2
 
   backlight: true
 
@@ -62,7 +64,8 @@ examples:
             avdd-supply = <&reg_avdd>;
             avee-supply = <&reg_avee>;
             vddi-supply = <&reg_1v8_p>;
-            reset-gpios = <&gpio1 29 GPIO_ACTIVE_LOW>;
+            reset-gpios = <&gpio1 29 GPIO_ACTIVE_LOW>,
+                          <&gpio1 24 GPIO_ACTIVE_LOW>;
             backlight = <&backlight>;
         };
     };
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Fabio Estevam]

Hi Guido,

On Mon, Sep 21, 2020 at 1:56 PM Guido Günther <agx@sigxcpu.org> wrote:
>
> We need to reset both for the panel to show an image.

There is a typo in "resets" in the Subject line.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Guido Günther]

Hi,
On Mon, Sep 21, 2020 at 02:05:55PM -0300, Fabio Estevam wrote:
> Hi Guido,
> 
> On Mon, Sep 21, 2020 at 1:56 PM Guido Günther <agx@sigxcpu.org> wrote:
> >
> > We need to reset both for the panel to show an image.
> 
> There is a typo in "resets" in the Subject line.

Fix queued for v2. Thanks!
 -- Guido
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Sam Ravnborg]

Hi Guido.

On Mon, Sep 21, 2020 at 06:55:52PM +0200, Guido Günther wrote:
> We need to reset both for the panel to show an image.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  .../bindings/display/panel/mantix,mlaf057we51-x.yaml       | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> index 937323cc9aaa..ba5a18fac9f9 100644
> --- a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> @@ -35,7 +35,9 @@ properties:
>    vddi-supply:
>      description: 1.8V I/O voltage supply
>  
> -  reset-gpios: true
> +  reset-gpios:
> +    minItems: 2
> +    maxItems: 2

reset-gpios is, as you already wrote, defined in panel-common.yaml.
Do not try to change it here.
It would be much better, I think, to introduce a mantix,reset-gpios
property.

This would avoid that we had two different reset-gpios definitions.

	Sam

>  
>    backlight: true
>  
> @@ -62,7 +64,8 @@ examples:
>              avdd-supply = <&reg_avdd>;
>              avee-supply = <&reg_avee>;
>              vddi-supply = <&reg_1v8_p>;
> -            reset-gpios = <&gpio1 29 GPIO_ACTIVE_LOW>;
> +            reset-gpios = <&gpio1 29 GPIO_ACTIVE_LOW>,
> +                          <&gpio1 24 GPIO_ACTIVE_LOW>;
>              backlight = <&backlight>;
>          };
>      };
> -- 
> 2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1671 bytes --]

On Thu, Sep 24, 2020 at 09:38:07PM +0200, Sam Ravnborg wrote:
> Hi Guido.
> 
> On Mon, Sep 21, 2020 at 06:55:52PM +0200, Guido Günther wrote:
> > We need to reset both for the panel to show an image.
> > 
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > ---
> >  .../bindings/display/panel/mantix,mlaf057we51-x.yaml       | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > index 937323cc9aaa..ba5a18fac9f9 100644
> > --- a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > @@ -35,7 +35,9 @@ properties:
> >    vddi-supply:
> >      description: 1.8V I/O voltage supply
> >  
> > -  reset-gpios: true
> > +  reset-gpios:
> > +    minItems: 2
> > +    maxItems: 2
> 
> reset-gpios is, as you already wrote, defined in panel-common.yaml.
> Do not try to change it here.
> It would be much better, I think, to introduce a mantix,reset-gpios
> property.
> 
> This would avoid that we had two different reset-gpios definitions.

My understanding is that this will only override the defaults defined in
panel-common.yaml. So everything else about the
panel-common.yaml#/properties/reset-gpios definition remains valid and
all this does is say that when this matched on "mantix,mlaf057we51-x",
then the "reset-gpios" property is expected to contain two entries as
opposed to the single entry that is expected by default.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Guido Günther]

Hi Thierry,
On Mon, Sep 28, 2020 at 08:50:39AM +0200, Thierry Reding wrote:
> On Thu, Sep 24, 2020 at 09:38:07PM +0200, Sam Ravnborg wrote:
> > Hi Guido.
> > 
> > On Mon, Sep 21, 2020 at 06:55:52PM +0200, Guido Günther wrote:
> > > We need to reset both for the panel to show an image.
> > > 
> > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > ---
> > >  .../bindings/display/panel/mantix,mlaf057we51-x.yaml       | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > > index 937323cc9aaa..ba5a18fac9f9 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > > @@ -35,7 +35,9 @@ properties:
> > >    vddi-supply:
> > >      description: 1.8V I/O voltage supply
> > >  
> > > -  reset-gpios: true
> > > +  reset-gpios:
> > > +    minItems: 2
> > > +    maxItems: 2
> > 
> > reset-gpios is, as you already wrote, defined in panel-common.yaml.
> > Do not try to change it here.
> > It would be much better, I think, to introduce a mantix,reset-gpios
> > property.
> > 
> > This would avoid that we had two different reset-gpios definitions.
> 
> My understanding is that this will only override the defaults defined in
> panel-common.yaml. So everything else about the
> panel-common.yaml#/properties/reset-gpios definition remains valid and
> all this does is say that when this matched on "mantix,mlaf057we51-x",
> then the "reset-gpios" property is expected to contain two entries as
> opposed to the single entry that is expected by default.

I thought so too but I get:

linux/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.example.dt.yaml: panel@0: reset-gpios: [[4294967295, 29, 1], [4294967295, 24, 1]] is too long
	From schema: /.../linux/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml

It seems to bepossible to *extend* reset-gpios though because when i use
an example like:

            reset-gpios = <&gpio1 29 GPIO_ACTIVE_LOW>,
                          <&gpio1 24 GPIO_ACTIVE_LOW>;

i correctly get

linux/linux/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.example.dt.yaml: panel@0: reset-gpios: [[4294967295, 29, 1]] is too short
	From schema: /.../linux/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
 
so the `minItems: 2` is being picked up. This mens i can add
properties but i can't overwrite the existing `maxItems: 2` - that's why
i was wondering how that is supposed to work (if properties should be
overrideable from common files or not).
Cheers,
 -- Guido

> 
> Thierry


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH v1 3/3] dt-binding: display: Require two rests on mantix panel

[Rob Herring]

On Thu, Sep 24, 2020 at 09:38:07PM +0200, Sam Ravnborg wrote:
> Hi Guido.
> 
> On Mon, Sep 21, 2020 at 06:55:52PM +0200, Guido Günther wrote:
> > We need to reset both for the panel to show an image.
> > 
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > ---
> >  .../bindings/display/panel/mantix,mlaf057we51-x.yaml       | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > index 937323cc9aaa..ba5a18fac9f9 100644
> > --- a/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > @@ -35,7 +35,9 @@ properties:
> >    vddi-supply:
> >      description: 1.8V I/O voltage supply
> >  
> > -  reset-gpios: true
> > +  reset-gpios:
> > +    minItems: 2
> > +    maxItems: 2
> 
> reset-gpios is, as you already wrote, defined in panel-common.yaml.
> Do not try to change it here.
> It would be much better, I think, to introduce a mantix,reset-gpios
> property.

Yes.

You also need to define what each reset entry corresponds to as the 
assertion/deassertion order could be important. You might just do 2 
properties with <pinname>-gpios.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel