Hi Daniel, Thank you for the patch. On Sat, Feb 09, 2019 at 12:42:30PM +0530, Ramalingam C wrote: > From: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Now that component has docs it's worth spending a few words and > hyperlinks on recommended best practices in drm. > > Cc: Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > Documentation/driver-api/component.rst | 2 ++ > Documentation/gpu/drm-internals.rst | 5 +++++ > drivers/gpu/drm/drm_drv.c | 14 ++++++++++++++ > 3 files changed, 21 insertions(+) > > diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst > index 2da4a8f20607..57e37590733f 100644 > --- a/Documentation/driver-api/component.rst > +++ b/Documentation/driver-api/component.rst > @@ -1,3 +1,5 @@ > +.. _component: > + > ====================================== > Component Helper for Aggregate Drivers > ====================================== > diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst > index 3ae23a5454ac..966bd2d9f0cc 100644 > --- a/Documentation/gpu/drm-internals.rst > +++ b/Documentation/gpu/drm-internals.rst > @@ -93,6 +93,11 @@ Device Instance and Driver Handling > Driver Load > ----------- > > +Component Helper Usage > +~~~~~~~~~~~~~~~~~~~~~~ > + > +.. kernel-doc:: drivers/gpu/drm/drm_drv.c > + :doc: component helper usage recommendations > > IRQ Helper Library > ~~~~~~~~~~~~~~~~~~ > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 381581b01d48..aae413003705 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -457,6 +457,20 @@ static void drm_fs_inode_free(struct inode *inode) > } > > /** > + * DOC: component helper usage recommendations > + * > + * DRM drivers that drive hardware where a logical device consists of a pile of > + * independent hardware blocks are recommended to use the :ref:`component helper > + * library<component>`. That's the first recommendation I would challenge :-) When using drm_bridge and drm_panel, the component framework is not strictly needed. The DRM/KMS probe function can defer probe when a bridge or panel is missing. Only when two-way dependencies exist between the display controller and the external components is the component framework required. > The entire device initialization procedure should be run > + * from the &component_master_ops.master_bind callback, starting with > + * drm_dev_init(), then binding all components with component_bind_all() and > + * finishing with drm_dev_register(). The omapdss driver, for instance, performs hardware-specific initialization of the DSS in the probe() handler itself (DT-related parsing, memory mapping, IRQ registration, ...). I don't see why this would need to move to the .master_bind() callback. > For consistency and easier sharing of > + * components across drivers the opaque pointer passed to all components through > + * component_bind_all() should point at &struct drm_device of the device > + * instance, not some driver specific private structure. I'd say "shall" instead of "should" to make this a hard requirement. The opaque pointer is something that I have never really liked in the component framework as it hinders reusability. I have a few ideas on how to fix that but haven't had time to try them out yet as the changes would be quite intrusive. Long term major changes will be needed there anyway as it makes no sense to have to frameworks with similar purposes in DRM/KMS and V4L2. > + */ > + > +/** > * drm_dev_init - Initialise new DRM device > * @dev: DRM device > * @driver: DRM driver -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
