Re: [PATCH v2 0/2] drm/panel: Add device link in drm_panel_attach()

From: Jyri Sarha <jsarha@ti.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: airlied@linux.ie, tomi.valkeinen@ti.com,
	thierry.reding@gmail.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 0/2] drm/panel: Add device link in drm_panel_attach()
Date: Thu, 1 Mar 2018 19:42:53 +0200	[thread overview]
Message-ID: <d1dd3a8e-e5d5-b5a9-7692-644ae1c136f0@ti.com> (raw)
In-Reply-To: <20180228194749.GC27565@wunner.de>

On 28/02/18 21:47, Lukas Wunner wrote:
> On Wed, Feb 28, 2018 at 01:09:28PM +0200, Jyri Sarha wrote:
>> The device_link_del() is still there in drm_panel_detach(), despite
>> Lukas Wunner's comment[1]. In the usual (currently all) cases things
>> would work perfectly without the call too, because
>> device_links_driver_cleanup() will eventually remove all orphaned
>> links. However, this would cause an error in the situation where a drm
>> device would like to detach a panel but remain operational, since the
>> drm device would be unbound for no good reason if the detached panel
>> is later unbound.
> 
> Okay, in that case I'd suggest dropping the DL_FLAG_AUTOREMOVE flag
> and keep the device_link_del().  That gives you the flexibility to
> detach a panel at runtime and drop the device link, but also have
> the DRM driver unbound once the panel driver is unbound.
> 
> If you have things like optional panels that can be detached without
> the necessity to unbind the DRM driver, you need something else instead
> of or on top of device links.  Perhaps some kind of notifier block.
> And perhaps two drm_panel_attach/detach() helpers in the DRM library,
> one with device link and one with notifier.
> 
> As stated in the device links documentation, optional dependencies
> are "beyond the scope of device links."
> 

I think the "optional panel" usage pattern is quite unlikely to ever
exist, but still it sound wrong to me to leave the links behind when
there is an apparent symmetry of detach and attach functions.

The situation would be different if we would get rid of the detach call
all together. After all the function in its current form is pretty
useless. The only purpose for its existence is marking the panel
available for other drm devices to use, which would suggest that the
"optional panel"-pattern is supported.

I think my current approach is fine (after removing the
DL_FLAG_AUTOREMOVE, I had not really understood its purpose before) if
we accept that the device link is there only as a precaution. But I am
also fine with removing the drm_panel_detach() function and laeving the
DL_FLAG_AUTOREMOVE flag there.

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