Re: [PATCH v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives
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 > >> --- > >> 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
Re: [PATCH v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives
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 >> --- >> 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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives
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 > --- > 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 signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
