Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel