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

[Ajay kumar]

Hi Daniel and Sean,

Thanks for the comments!

On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul at chromium.org> wrote:
> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <daniel at 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 at 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