Re: [PATCH v12 01/38] drm/doc: document recommended component helper usage

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@intel.com>
> 
> 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@armlinux.org.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx