diff mbox

[v2,1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives

Message ID 94922d2b5feed117354b5c0a42b4b56f643f73d9.1519815150.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Feb. 28, 2018, 11:09 a.m. UTC
Setting the connector and drm to NULL when the drm panel device is
going away hardly serves any purpose. Usually the the whole memory
stucture is freed right after the remove call.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
 drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
 drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
 drivers/gpu/drm/panel/panel-simple.c                 | 1 -
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
 9 files changed, 9 deletions(-)

Comments

Thierry Reding Feb. 28, 2018, 6:53 p.m. UTC | #1
On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
> Setting the connector and drm to NULL when the drm panel device is
> going away hardly serves any purpose. Usually the the whole memory
> stucture is freed right after the remove call.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
>  drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
>  drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
>  drivers/gpu/drm/panel/panel-simple.c                 | 1 -
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
>  9 files changed, 9 deletions(-)

I don't understand the purpose of this patch. I'll grant you that the
current implementation of drm_panel_detach() is not very useful, but
then you add code to drm_panel_detach() in the next patch and mention
in the commit message that panel drivers should be calling the
drm_panel_detach() function to remove the link.

This is confusing. Can you clarify?

Thierry
Jyri Sarha Feb. 28, 2018, 9:31 p.m. UTC | #2
On 28/02/18 20:53, Thierry Reding wrote:
> On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
>> Setting the connector and drm to NULL when the drm panel device is
>> going away hardly serves any purpose. Usually the the whole memory
>> stucture is freed right after the remove call.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
>>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
>>  drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
>>  drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
>>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
>>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
>>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
>>  drivers/gpu/drm/panel/panel-simple.c                 | 1 -
>>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
>>  9 files changed, 9 deletions(-)
> 
> I don't understand the purpose of this patch. I'll grant you that the
> current implementation of drm_panel_detach() is not very useful, but
> then you add code to drm_panel_detach() in the next patch and mention
> in the commit message that panel drivers should be calling the
> drm_panel_detach() function to remove the link.
> 
> This is confusing. Can you clarify?
> 

When looking at the current implementation it does not make any sense to
me to call drm_panel_detach() from the panel driver itself. However, it
makes perfect sense calling it from drm driver. Setting panel->connector
= NULL marks it free and attachable to other devices, but the panel
driver that the passive element in the picture should not go and mark
itself available on its own.

But now that I take the steps to make the drm_panel_detach() to be
called only from drm device I should at least document it too.

Also in general I think it is hard to come up with a detach
implementation that would work from both panel and the drm device.

Best regards,
Jyri
Daniel Vetter March 6, 2018, 10:03 a.m. UTC | #3
On Wed, Feb 28, 2018 at 11:31:53PM +0200, Jyri Sarha wrote:
> On 28/02/18 20:53, Thierry Reding wrote:
> > On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
> >> Setting the connector and drm to NULL when the drm panel device is
> >> going away hardly serves any purpose. Usually the the whole memory
> >> stucture is freed right after the remove call.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>  drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
> >>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
> >>  drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
> >>  drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
> >>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
> >>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
> >>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
> >>  drivers/gpu/drm/panel/panel-simple.c                 | 1 -
> >>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
> >>  9 files changed, 9 deletions(-)
> > 
> > I don't understand the purpose of this patch. I'll grant you that the
> > current implementation of drm_panel_detach() is not very useful, but
> > then you add code to drm_panel_detach() in the next patch and mention
> > in the commit message that panel drivers should be calling the
> > drm_panel_detach() function to remove the link.
> > 
> > This is confusing. Can you clarify?
> > 
> 
> When looking at the current implementation it does not make any sense to
> me to call drm_panel_detach() from the panel driver itself. However, it
> makes perfect sense calling it from drm driver. Setting panel->connector
> = NULL marks it free and attachable to other devices, but the panel
> driver that the passive element in the picture should not go and mark
> itself available on its own.
> 
> But now that I take the steps to make the drm_panel_detach() to be
> called only from drm device I should at least document it too.
> 
> Also in general I think it is hard to come up with a detach
> implementation that would work from both panel and the drm device.

I think we first need a series which changes drm_panel_detach to be called
by drm drivers (not panel drivers), and have a drm_panel_remove of similar
(like we do with bridges) to remove the panel driver.

Then I think this series here makes a lot more sense as a follow-up.
Otherwise it's indeed rather confusing.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 57df39b..bb53e08 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -292,7 +292,6 @@  static int innolux_panel_remove(struct mipi_dsi_device *dsi)
 		DRM_DEV_ERROR(&dsi->dev, "failed to detach from DSI host: %d\n",
 			      err);
 
-	drm_panel_detach(&innolux->base);
 	innolux_panel_del(innolux);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 0a94ab7..99caa78 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -500,7 +500,6 @@  static int jdi_panel_remove(struct mipi_dsi_device *dsi)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n",
 			ret);
 
-	drm_panel_detach(&jdi->base);
 	jdi_panel_del(jdi);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index b5e3994..e8bc356 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -282,7 +282,6 @@  static int panel_lvds_remove(struct platform_device *pdev)
 {
 	struct panel_lvds *lvds = dev_get_drvdata(&pdev->dev);
 
-	drm_panel_detach(&lvds->panel);
 	drm_panel_remove(&lvds->panel);
 
 	panel_lvds_disable(&lvds->panel);
diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
index 74a8061..cb4dfb9 100644
--- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
+++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
@@ -299,7 +299,6 @@  static int wuxga_nt_panel_remove(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
 
-	drm_panel_detach(&wuxga_nt->base);
 	wuxga_nt_panel_del(wuxga_nt);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
index 71c09ed..75f9253 100644
--- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
+++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
@@ -292,7 +292,6 @@  static int seiko_panel_remove(struct platform_device *pdev)
 {
 	struct seiko_panel *panel = dev_get_drvdata(&pdev->dev);
 
-	drm_panel_detach(&panel->base);
 	drm_panel_remove(&panel->base);
 
 	seiko_panel_disable(&panel->base);
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 6bf8730..02fc0f5 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -418,7 +418,6 @@  static int sharp_panel_remove(struct mipi_dsi_device *dsi)
 	if (err < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
 
-	drm_panel_detach(&sharp->base);
 	sharp_panel_del(sharp);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 494aa9b..e5cae00 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -327,7 +327,6 @@  static int sharp_nt_panel_remove(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
 
-	drm_panel_detach(&sharp_nt->base);
 	sharp_nt_panel_del(sharp_nt);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 3b0ba9f..5aa736c 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -364,7 +364,6 @@  static int panel_simple_remove(struct device *dev)
 {
 	struct panel_simple *panel = dev_get_drvdata(dev);
 
-	drm_panel_detach(&panel->base);
 	drm_panel_remove(&panel->base);
 
 	panel_simple_disable(&panel->base);
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 358c64e..74284e5 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -419,7 +419,6 @@  static int st7789v_remove(struct spi_device *spi)
 {
 	struct st7789v *ctx = spi_get_drvdata(spi);
 
-	drm_panel_detach(&ctx->panel);
 	drm_panel_remove(&ctx->panel);
 
 	if (ctx->backlight)