On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
>>> Hi Daniel and Sean,
>>>
>>> Thanks for the comments!
>>>
>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>>>> 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.
>>> 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@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,
>>> 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.
> Are you talking about using struct device_node instead of struct device?
> I guess you have misplaced the comment under the wrong section!
Yeah, that should have been one up ;-)
>>>>>> 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.
> Can you send a link for that?
> And, is there any problem if the doc comes later?
Since quite a while we've asked for the kerneldoc polish as part of
each drm core patch series. It's just that drm_bridge/panel kinda have
flown under the radar of the people usually asking for docs ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
