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

From: Daniel Vetter <daniel@ffwll.ch>
To: Jyri Sarha <jsarha@ti.com>
Cc: airlied@linux.ie, dri-devel@lists.freedesktop.org,
	tomi.valkeinen@ti.com, Thierry Reding <thierry.reding@gmail.com>
Subject: Re: [PATCH v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives
Date: Tue, 6 Mar 2018 11:03:43 +0100	[thread overview]
Message-ID: <20180306100343.GA22212@phenom.ffwll.local> (raw)
In-Reply-To: <10b9e149-01f2-3e3c-622a-434b0136cac1@ti.com>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel