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

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
> 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.

Hm, if you do this can you pls also update drm_panel accordingly? It
shouldn't be a lot of fuzz and would make things around drm+dt more
consistent.

>
>>>>       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?

If you want to document drm_bridge then I recomment to sprinkle proper
kerneldoc over drm_bridge.c and pull it all into the drm DocBook
template. That way all the drm documentation is in one place. I've
done that for drm_crtc.h in an unrelated patch series (but based upon
a branch with your patch here included) and there's struct drm_bridge*
in there. Hence why I've noticed.

If you do kerneldoc/DocBook integration for drm_bridge it's probably
best to also do it for drm_panel and use the opportunity to
review/rework the interfaces a bit for consistency. E.g. move dt
related stuff into drm_of.c and have that in a separate section in the
docs.
-Daniel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch