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@xxxxxxxx> wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> 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
Attachment:
pgpc3pWuIyv5T.pgp
Description: PGP signature
