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.
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
- 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?
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.
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
@@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev);
+extern int drm_bridge_add(struct drm_bridge *bridge); +extern void drm_bridge_remove(struct drm_bridge *bridge); +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); +extern int drm_bridge_attach(struct drm_bridge *bridge,
struct drm_encoder *encoder);extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.7.9.5