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

[Thierry Reding]

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul at chromium.org> wrote:
> >>>> @@ -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.
> >
> > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > drm_crtc drop the struct device and go directly to a struct
> > device_node. Since we don't really need the sturct device, the only
> > thing we care about is the of_node. For added bonus wrap an #ifdef
> > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > Should be all fairly simple to pull off with cocci.
> >
> > Thierry?
> 
> Looking at the of_drm_find_panel function I actually wonder how that
> works - the drm_panel doesn't really need to stick around afaics.
> After all panel_list is global so some other driver can unload.
> Russell's of support for possible_crtcs code works differently since
> it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by
some driver that isn't tied to the DRM device until attached. It can't
be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the
typical use-case.

> This bridge code here though suffers from the same. So to me this
> looks rather fishy.

Well, this version of the DRM bridge support was written to be close to
DRM panel, so it isn't really surprising that it's similar =), but like
I said, I don't really understand what you think is wrong with it.

> It doesn't help that we have drm_of.[hc] around but not all the of
> code is in there. Adding Russell too.

DRM panel and DRM bridge aren't just OF helpers. They can be used with
whatever type of device you want.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141028/c55c18b0/attachment.sig>