Hi Daniel and Sean,
Thanks for the comments!
On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanpaul@chromium.org wrote:
On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter daniel@ffwll.ch wrote:
So don't ask why but I accidentally ended up in a branch looking at this patch and didn't like it. So very quick&grumpy review.
First, please make the patch subject more descriptive: I'd expect a helper function scaffolding like the various crtc/probe/dp ... helpers we already have. You instead add code to untangle the probe ordering. Please say so.
Sure. I will reword it properly.
More comments below.
On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.
The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".
The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.
The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.
drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
[snip]
@@ -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,
Right, The entire rework is based on how drm_panel framework is structured.
FWIW. However, seems reasonable to keep the device_node instead.
Yes, its visible that just device_node would be sufficient. This will save us from renaming drm_device as well.
struct drm_device *drm;struct drm_encoder *encoder;This breaks bridge->bridge chaining (if we ever get there). It seems pretty much unused anyway except for an EBUSY check. Can't you use bridge->dev for that?
Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach and leave it up to the caller to establish the proper chain.
Ok. I will use drm_device pointer directly instead of passing encoder pointer.
struct list_head head;
struct list_head list;These lists need better names. I know that the "head" is really awful, especially since it's actually not the head of the list but just an element.
I think we can just rip bridge_list out of mode_config if we're going to keep track of bridges elsewhere. So we can nuke "head" and keep "list". This also means that bridge->destroy() goes away, but that's probably Ok if everything converts to the standalone driver model where we have driver->remove()
Sean
Great! Thierry actually mentioned about this once, and we have the confirmation now.
struct drm_mode_object base;Aside: I've noticed all this trying to update the kerneldoc for struct drm_bridge, which just showed that this patch makes inconsistent changes. Trying to write kerneldoc is a really great way to come up with better interfaces imo.
Cheers, Daniel
I din't get this actually. You want me to create Doc../drm_bridge.txt or something similar?
Ajay