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