On Tue, Oct 28, 2014 at 10:19 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > 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 ;-) Heh, sorry about that. Sean > -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