[PATCH 0/4] Elida KD35T133 Panel Improvements

[PATCH 0/4] Elida KD35T133 Panel Improvements

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Fix a few bugs and clean up no longer needed code on the Elida KD35T133
DSI panel, as used in devices such as the Odroid Go Advance and the
Anbernic RG351M.

Chris Morgan (4):
  drm/panel-elida-kd35t133: trival: update panel size from 5.5 to 3.5
  drm/panel-elida-kd35t133: hold panel in reset for unprepare
  drm/panel-elida-kd35t133: drop
    drm_connector_set_orientation_from_panel
  drm/panel-elida-kd35t133: Drop prepare/unprepare logic

 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 37 ++------------------
 1 file changed, 3 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH 1/4] drm/panel-elida-kd35t133: trival: update panel size from 5.5 to 3.5

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

The comments at the top of the driver state the panel size incorrectly
as 5.5" instead of 3.5".

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index e7be15b68102..6cd8536c09ff 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Elida kd35t133 5.5" MIPI-DSI panel driver
+ * Elida kd35t133 3.5" MIPI-DSI panel driver
  * Copyright (C) 2020 Theobroma Systems Design und Consulting GmbH
  *
  * based on
-- 
2.34.1

Re: [PATCH 1/4] drm/panel-elida-kd35t133: trival: update panel size from 5.5 to 3.5

[Jessica Zhang]

On 11/15/2023 7:26 AM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> The comments at the top of the driver state the panel size incorrectly
> as 5.5" instead of 3.5".
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> index e7be15b68102..6cd8536c09ff 100644
> --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * Elida kd35t133 5.5" MIPI-DSI panel driver
> + * Elida kd35t133 3.5" MIPI-DSI panel driver
>    * Copyright (C) 2020 Theobroma Systems Design und Consulting GmbH
>    *
>    * based on
> -- 
> 2.34.1
> 

[PATCH 2/4] drm/panel-elida-kd35t133: hold panel in reset for unprepare

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

For devices like the Anbernic RG351M and RG351P the panel is wired to
an always on regulator. When the device suspends and wakes up, there
are some slight artifacts on the screen that go away over time. If
instead we hold the panel in reset status after it is unprepared,
this does not happen.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index 6cd8536c09ff..f1fc4a26f447 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -104,6 +104,8 @@ static int kd35t133_unprepare(struct drm_panel *panel)
 		return ret;
 	}
 
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+
 	regulator_disable(ctx->iovcc);
 	regulator_disable(ctx->vdd);
 
-- 
2.34.1

Re: [PATCH 2/4] drm/panel-elida-kd35t133: hold panel in reset for unprepare

[Jessica Zhang]

On 11/15/2023 7:26 AM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> For devices like the Anbernic RG351M and RG351P the panel is wired to
> an always on regulator. When the device suspends and wakes up, there
> are some slight artifacts on the screen that go away over time. If
> instead we hold the panel in reset status after it is unprepared,
> this does not happen.

Hi Chris,

LGTM, though if this is fixing something maybe we can add

Fixes: 5b6603360c12 ("drm/panel: add panel driver for Elida KD35T133 
panels")

> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thanks,

Jessica Zhang

> ---
>   drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> index 6cd8536c09ff..f1fc4a26f447 100644
> --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> @@ -104,6 +104,8 @@ static int kd35t133_unprepare(struct drm_panel *panel)
>   		return ret;
>   	}
>   
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +
>   	regulator_disable(ctx->iovcc);
>   	regulator_disable(ctx->vdd);
>   
> -- 
> 2.34.1
> 

Re: [PATCH 2/4] drm/panel-elida-kd35t133: hold panel in reset for unprepare

[Chris Morgan]

On Wed, Nov 15, 2023 at 01:38:11PM -0800, Jessica Zhang wrote:
> 
> 
> On 11/15/2023 7:26 AM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > For devices like the Anbernic RG351M and RG351P the panel is wired to
> > an always on regulator. When the device suspends and wakes up, there
> > are some slight artifacts on the screen that go away over time. If
> > instead we hold the panel in reset status after it is unprepared,
> > this does not happen.
> 
> Hi Chris,
> 
> LGTM, though if this is fixing something maybe we can add
> 
> Fixes: 5b6603360c12 ("drm/panel: add panel driver for Elida KD35T133
> panels")

It *may* fix an intermittent issue with the panel resuming from
suspend, but it really more solves an issue with devices for which
we're still adding support.

Should I still tag it as a "fixes" then?

Thank you.

> 
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> 
> Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Thanks,
> 
> Jessica Zhang
> 
> > ---
> >   drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> > index 6cd8536c09ff..f1fc4a26f447 100644
> > --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> > +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> > @@ -104,6 +104,8 @@ static int kd35t133_unprepare(struct drm_panel *panel)
> >   		return ret;
> >   	}
> > +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> > +
> >   	regulator_disable(ctx->iovcc);
> >   	regulator_disable(ctx->vdd);
> > -- 
> > 2.34.1
> > 

Re: [PATCH 2/4] drm/panel-elida-kd35t133: hold panel in reset for unprepare

[Jessica Zhang]

On 11/16/2023 7:17 AM, Chris Morgan wrote:
> On Wed, Nov 15, 2023 at 01:38:11PM -0800, Jessica Zhang wrote:
>>
>>
>> On 11/15/2023 7:26 AM, Chris Morgan wrote:
>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>
>>> For devices like the Anbernic RG351M and RG351P the panel is wired to
>>> an always on regulator. When the device suspends and wakes up, there
>>> are some slight artifacts on the screen that go away over time. If
>>> instead we hold the panel in reset status after it is unprepared,
>>> this does not happen.
>>
>> Hi Chris,
>>
>> LGTM, though if this is fixing something maybe we can add
>>
>> Fixes: 5b6603360c12 ("drm/panel: add panel driver for Elida KD35T133
>> panels")
> 
> It *may* fix an intermittent issue with the panel resuming from
> suspend, but it really more solves an issue with devices for which
> we're still adding support.
> 
> Should I still tag it as a "fixes" then?

 From my understanding of the submitting patches docs [1], I think this 
warrants a "Fixes" since it's fixing an issue you're seeing with the 
original driver.

Thanks,

Jessica Zhang

[1] 
https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

> 
> Thank you.
> 
>>
>>>
>>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>>
>> Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>> ---
>>>    drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
>>> index 6cd8536c09ff..f1fc4a26f447 100644
>>> --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
>>> +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
>>> @@ -104,6 +104,8 @@ static int kd35t133_unprepare(struct drm_panel *panel)
>>>    		return ret;
>>>    	}
>>> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>> +
>>>    	regulator_disable(ctx->iovcc);
>>>    	regulator_disable(ctx->vdd);
>>> -- 
>>> 2.34.1
>>>

[PATCH 3/4] drm/panel-elida-kd35t133: drop drm_connector_set_orientation_from_panel

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Stop calling drm_connector_set_orientation_from_panel() as its now
called by the panel bridge directly when it is initialized.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index f1fc4a26f447..29b4ee63d83b 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -211,11 +211,6 @@ static int kd35t133_get_modes(struct drm_panel *panel,
 	connector->display_info.width_mm = mode->width_mm;
 	connector->display_info.height_mm = mode->height_mm;
 	drm_mode_probed_add(connector, mode);
-	/*
-	 * TODO: Remove once all drm drivers call
-	 * drm_connector_set_orientation_from_panel()
-	 */
-	drm_connector_set_panel_orientation(connector, ctx->orientation);
 
 	return 1;
 }
-- 
2.34.1

Re: [PATCH 3/4] drm/panel-elida-kd35t133: drop drm_connector_set_orientation_from_panel

[Jessica Zhang]

On 11/15/2023 7:26 AM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Stop calling drm_connector_set_orientation_from_panel() as its now
> called by the panel bridge directly when it is initialized.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/panel/panel-elida-kd35t133.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> index f1fc4a26f447..29b4ee63d83b 100644
> --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> @@ -211,11 +211,6 @@ static int kd35t133_get_modes(struct drm_panel *panel,
>   	connector->display_info.width_mm = mode->width_mm;
>   	connector->display_info.height_mm = mode->height_mm;
>   	drm_mode_probed_add(connector, mode);
> -	/*
> -	 * TODO: Remove once all drm drivers call
> -	 * drm_connector_set_orientation_from_panel()
> -	 */
> -	drm_connector_set_panel_orientation(connector, ctx->orientation);
>   
>   	return 1;
>   }
> -- 
> 2.34.1
> 

[PATCH 4/4] drm/panel-elida-kd35t133: Drop prepare/unprepare logic

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Drop the prepare/unprepare logic, as this is now tracked elsewhere.
Additionally, the driver shutdown is also duplicate as it calls
drm_unprepare and drm_disable which are called anyway when
associated drivers are shutdown/removed.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 28 --------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index 29b4ee63d83b..00791ea81e90 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -43,7 +43,6 @@ struct kd35t133 {
 	struct regulator *vdd;
 	struct regulator *iovcc;
 	enum drm_panel_orientation orientation;
-	bool prepared;
 };
 
 static inline struct kd35t133 *panel_to_kd35t133(struct drm_panel *panel)
@@ -91,9 +90,6 @@ static int kd35t133_unprepare(struct drm_panel *panel)
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	int ret;
 
-	if (!ctx->prepared)
-		return 0;
-
 	ret = mipi_dsi_dcs_set_display_off(dsi);
 	if (ret < 0)
 		dev_err(ctx->dev, "failed to set display off: %d\n", ret);
@@ -109,8 +105,6 @@ static int kd35t133_unprepare(struct drm_panel *panel)
 	regulator_disable(ctx->iovcc);
 	regulator_disable(ctx->vdd);
 
-	ctx->prepared = false;
-
 	return 0;
 }
 
@@ -120,9 +114,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	int ret;
 
-	if (ctx->prepared)
-		return 0;
-
 	dev_dbg(ctx->dev, "Resetting the panel\n");
 	ret = regulator_enable(ctx->vdd);
 	if (ret < 0) {
@@ -166,8 +157,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
 
 	msleep(50);
 
-	ctx->prepared = true;
-
 	return 0;
 
 disable_iovcc:
@@ -296,27 +285,11 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
 	return 0;
 }
 
-static void kd35t133_shutdown(struct mipi_dsi_device *dsi)
-{
-	struct kd35t133 *ctx = mipi_dsi_get_drvdata(dsi);
-	int ret;
-
-	ret = drm_panel_unprepare(&ctx->panel);
-	if (ret < 0)
-		dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
-
-	ret = drm_panel_disable(&ctx->panel);
-	if (ret < 0)
-		dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
-}
-
 static void kd35t133_remove(struct mipi_dsi_device *dsi)
 {
 	struct kd35t133 *ctx = mipi_dsi_get_drvdata(dsi);
 	int ret;
 
-	kd35t133_shutdown(dsi);
-
 	ret = mipi_dsi_detach(dsi);
 	if (ret < 0)
 		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
@@ -337,7 +310,6 @@ static struct mipi_dsi_driver kd35t133_driver = {
 	},
 	.probe	= kd35t133_probe,
 	.remove = kd35t133_remove,
-	.shutdown = kd35t133_shutdown,
 };
 module_mipi_dsi_driver(kd35t133_driver);
 
-- 
2.34.1

Re: [PATCH 4/4] drm/panel-elida-kd35t133: Drop prepare/unprepare logic

[Jessica Zhang]

On 11/15/2023 7:26 AM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Drop the prepare/unprepare logic, as this is now tracked elsewhere.
> Additionally, the driver shutdown is also duplicate as it calls
> drm_unprepare and drm_disable which are called anyway when
> associated drivers are shutdown/removed.

Hi Chris,

In the commit message, can you mention that this [1] was the change that 
made tracking the prepared flag common?

Also, it seems to me that this patch squashes 2 different changes 
together. Maybe we can split this into 2 patches -- one to drop the 
prepared flag checks and another to drop shutdown().

Thanks,

Jessica Zhang

[1] d2aacaf07395 ("drm/panel: Check for already prepared/enabled in 
drm_panel")

> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>   drivers/gpu/drm/panel/panel-elida-kd35t133.c | 28 --------------------
>   1 file changed, 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> index 29b4ee63d83b..00791ea81e90 100644
> --- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> +++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
> @@ -43,7 +43,6 @@ struct kd35t133 {
>   	struct regulator *vdd;
>   	struct regulator *iovcc;
>   	enum drm_panel_orientation orientation;
> -	bool prepared;
>   };
>   
>   static inline struct kd35t133 *panel_to_kd35t133(struct drm_panel *panel)
> @@ -91,9 +90,6 @@ static int kd35t133_unprepare(struct drm_panel *panel)
>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>   	int ret;
>   
> -	if (!ctx->prepared)
> -		return 0;
> -
>   	ret = mipi_dsi_dcs_set_display_off(dsi);
>   	if (ret < 0)
>   		dev_err(ctx->dev, "failed to set display off: %d\n", ret);
> @@ -109,8 +105,6 @@ static int kd35t133_unprepare(struct drm_panel *panel)
>   	regulator_disable(ctx->iovcc);
>   	regulator_disable(ctx->vdd);
>   
> -	ctx->prepared = false;
> -
>   	return 0;
>   }
>   
> @@ -120,9 +114,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>   	int ret;
>   
> -	if (ctx->prepared)
> -		return 0;
> -
>   	dev_dbg(ctx->dev, "Resetting the panel\n");
>   	ret = regulator_enable(ctx->vdd);
>   	if (ret < 0) {
> @@ -166,8 +157,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
>   
>   	msleep(50);
>   
> -	ctx->prepared = true;
> -
>   	return 0;
>   
>   disable_iovcc:
> @@ -296,27 +285,11 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
>   	return 0;
>   }
>   
> -static void kd35t133_shutdown(struct mipi_dsi_device *dsi)
> -{
> -	struct kd35t133 *ctx = mipi_dsi_get_drvdata(dsi);
> -	int ret;
> -
> -	ret = drm_panel_unprepare(&ctx->panel);
> -	if (ret < 0)
> -		dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
> -
> -	ret = drm_panel_disable(&ctx->panel);
> -	if (ret < 0)
> -		dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
> -}
> -
>   static void kd35t133_remove(struct mipi_dsi_device *dsi)
>   {
>   	struct kd35t133 *ctx = mipi_dsi_get_drvdata(dsi);
>   	int ret;
>   
> -	kd35t133_shutdown(dsi);
> -
>   	ret = mipi_dsi_detach(dsi);
>   	if (ret < 0)
>   		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
> @@ -337,7 +310,6 @@ static struct mipi_dsi_driver kd35t133_driver = {
>   	},
>   	.probe	= kd35t133_probe,
>   	.remove = kd35t133_remove,
> -	.shutdown = kd35t133_shutdown,
>   };
>   module_mipi_dsi_driver(kd35t133_driver);
>   
> -- 
> 2.34.1
>