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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3655 bytes --]

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@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?
> > 
> > The struct device * is in DRM panel because there's nothing device tree
> > specific about the concept. Having a struct device_node * instead would
> > indicate that it can only be used with a device tree, whereas the
> > framework doesn't care the tiniest bit what type of device we have.
> > 
> > While the trend clearly is to use more device tree, I don't think we
> > should make it impossible for anybody else to use these frameworks.
> > 
> > There are other advantages to keeping a struct device *, like having
> > access to the proper device and its name. Also you get access to the
> > device_node * via dev->of_node anyway. I don't see any advantage in
> > switching to just a struct device_node *, only disadvantages.
> 
> Well the idea is to make the lookup key specific, and conditional on
> #CONFIG_OF. If there's going to be another neat way to enumerate platform
> devices then I think we should add that, too. Or maybe we should have a
> void *platform_data or so.
> 
> The reason I really don't want a struct device * in core drm structures is
> that two releases down the road people will have found tons of really
> great ways to abuse them and re-create a midlayer. DRM core really should
> only care about the sw objects and not be hw specific at all. Heck there's
> not even an requirement to have any piece of actual hw, you could write a
> completely fake drm driver (for e.g. testing like the new v4l driver).
> 
> Tbh I wonder a bit why we even have this registery embedded into the core
> drm objects. Essentially the only thing you're doing is a list that maps
> some platform specific key onto some subsystem specific driver structure
> or fails the lookup. So instead of putting all these low-level details
> into drm core structures can't we just have a generic hashtable/list for
> this, plus some static inline helpers that cast the void * you get into
> the one you want?
> 
> I also get the feeling that this really should be in the driver core (like
> the component helpers), and that we should think a lot harder about
> lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel