On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:
> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > - struct drm_device *dev; > + struct device *dev;
Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083
I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.
Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.
Thierry?
Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists.
I don't understand. Panels are global resources that get registered by some driver that isn't tied to the DRM device until attached. It can't be in a per-DRM device list, because it's external to the device.
And yes, they can go away when a driver is unloaded, though it's not the typical use-case.
This bridge code here though suffers from the same. So to me this looks rather fishy.
Well, this version of the DRM bridge support was written to be close to DRM panel, so it isn't really surprising that it's similar =), but like I said, I don't really understand what you think is wrong with it.
You have a mutex to protect the global list of bridges/panels. But if you do a lookup you don't grab a reference count on the panel, so the moment you drop the list mutex the panel/bridge can go away.
Yes usually you don't unload a driver on a soc but soc isn't everything and designing new stuff to not be hotunplug save isn't great either.
Yeah, I certainly agree that adding proper reference counting would be a good thing. I think perhaps we could just take a reference on the struct device * to prevent it from disappearing.
Although perhaps I misunderstand what you mean by "go away".
Thierry