On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel@xxxxxxxx> 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.
>
> 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@xxxxxxxxxxx>
>
> [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, FWIW. However,
seems reasonable to keep the device_node instead.
>> + 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.
>> 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
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
