* [PATCH] drm/panel: Flesh out kerneldoc
@ 2014-11-04 16:26 Thierry Reding
2014-11-05 12:55 ` Daniel Vetter
0 siblings, 1 reply; 2+ messages in thread
From: Thierry Reding @ 2014-11-04 16:26 UTC (permalink / raw)
To: Thierry Reding; +Cc: Daniel Vetter, dri-devel
From: Thierry Reding <treding@nvidia.com>
Write more complete kerneldoc comments for the DRM panel API and
integrate the helpers in the DRM DocBook reference.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Documentation/DocBook/drm.tmpl | 6 +++++
drivers/gpu/drm/drm_panel.c | 61 ++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_panel.h | 59 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index d89ca5830697..8737f03d9a75 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2379,6 +2379,12 @@ void intel_crt_init(struct drm_device *dev)
<title id="drm-kms-planehelpers">Plane Helper Reference</title>
!Edrivers/gpu/drm/drm_plane_helper.c Plane Helpers
</sect2>
+ <sect2>
+ <title>Panel Helper Reference</title>
+!Iinclude/drm/drm_panel.h
+!Edrivers/gpu/drm/drm_panel.c
+!Pdrivers/gpu/drm/drm_panel.c drm panel
+ </sect2>
</sect1>
<!-- Internals: kms properties -->
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e037b7..3dfe3c886502 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -30,12 +30,36 @@
static DEFINE_MUTEX(panel_lock);
static LIST_HEAD(panel_list);
+/**
+ * DOC: drm panel
+ *
+ * The DRM panel helpers allow drivers to register panel objects with a
+ * central registry and provide functions to retrieve those panels in display
+ * drivers.
+ */
+
+/**
+ * drm_panel_init - initialize a panel
+ * @panel: DRM panel
+ *
+ * Sets up internal fields of the panel so that it can subsequently be added
+ * to the registry.
+ */
void drm_panel_init(struct drm_panel *panel)
{
INIT_LIST_HEAD(&panel->list);
}
EXPORT_SYMBOL(drm_panel_init);
+/**
+ * drm_panel_add - add a panel to the global registry
+ * @panel: panel to add
+ *
+ * Add a panel to the global registry so that it can be looked up by display
+ * drivers.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
int drm_panel_add(struct drm_panel *panel)
{
mutex_lock(&panel_lock);
@@ -46,6 +70,12 @@ int drm_panel_add(struct drm_panel *panel)
}
EXPORT_SYMBOL(drm_panel_add);
+/**
+ * drm_panel_remove - remove a panel from the global registry
+ * @panel: DRM panel
+ *
+ * Removes a panel from the global registry.
+ */
void drm_panel_remove(struct drm_panel *panel)
{
mutex_lock(&panel_lock);
@@ -54,6 +84,18 @@ void drm_panel_remove(struct drm_panel *panel)
}
EXPORT_SYMBOL(drm_panel_remove);
+/**
+ * drm_panel_attach - attach a panel to a connector
+ * @panel: DRM panel
+ * @connector: DRM connector
+ *
+ * After obtaining a pointer to a DRM panel a display driver calls this
+ * function to attach a panel to a connector.
+ *
+ * An error is returned if the panel is already attached to another connector.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
{
if (panel->connector)
@@ -66,6 +108,15 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
}
EXPORT_SYMBOL(drm_panel_attach);
+/**
+ * drm_panel_detach - detach a panel from a connector
+ * @panel: DRM panel
+ *
+ * Detaches a panel from the connector it is attached to. If a panel is not
+ * attached to any connector this is effectively a no-op.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
int drm_panel_detach(struct drm_panel *panel)
{
panel->connector = NULL;
@@ -76,6 +127,16 @@ int drm_panel_detach(struct drm_panel *panel)
EXPORT_SYMBOL(drm_panel_detach);
#ifdef CONFIG_OF
+/**
+ * of_drm_find_panel - look up a panel using a device tree node
+ * @np: device tree node of the panel
+ *
+ * Searches the set of registered panels for one that matches the given device
+ * tree node. If a matching panel is found, return a pointer to it.
+ *
+ * Return: A pointer to the panel registered for the specified device tree
+ * node or NULL if no panel matching the device tree node can be found.
+ */
struct drm_panel *of_drm_find_panel(struct device_node *np)
{
struct drm_panel *panel;
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 1fbcc96063a7..b88b4dcd698b 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -70,6 +70,14 @@ struct drm_panel_funcs {
int (*get_modes)(struct drm_panel *panel);
};
+/**
+ * struct drm_panel - DRM panel object
+ * @drm: DRM device owning the panel
+ * @connector: DRM connector that the panel is attached to
+ * @dev: parent device of the panel
+ * @funcs: operations that can be performed on the panel
+ * @list: panel entry in registry
+ */
struct drm_panel {
struct drm_device *drm;
struct drm_connector *connector;
@@ -80,6 +88,17 @@ struct drm_panel {
struct list_head list;
};
+/**
+ * drm_disable_unprepare - power off a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will completely power off a panel (assert the panel's
+ * reset, turn off power supplies, ...). After this function has completed, it
+ * is usually no longer possible to communicate with the panel until another
+ * call to drm_panel_prepare().
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
static inline int drm_panel_unprepare(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->unprepare)
@@ -88,6 +107,16 @@ static inline int drm_panel_unprepare(struct drm_panel *panel)
return panel ? -ENOSYS : -EINVAL;
}
+/**
+ * drm_panel_disable - disable a panel
+ * @panel: DRM panel
+ *
+ * This will typically turn off the panel's backlight or disable the display
+ * drivers. For smart panels it should still be possible to communicate with
+ * the integrated circuitry via any command bus after this call.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
static inline int drm_panel_disable(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->disable)
@@ -96,6 +125,16 @@ static inline int drm_panel_disable(struct drm_panel *panel)
return panel ? -ENOSYS : -EINVAL;
}
+/**
+ * drm_panel_prepare - power on a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will enable power and deassert any reset signals to
+ * the panel. After this has completed it is possible to communicate with any
+ * integrated circuitry via a command bus.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
static inline int drm_panel_prepare(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->prepare)
@@ -104,6 +143,16 @@ static inline int drm_panel_prepare(struct drm_panel *panel)
return panel ? -ENOSYS : -EINVAL;
}
+/**
+ * drm_panel_enable - enable a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will cause the panel display drivers to be turned on
+ * and the backlight to be enabled. Content will be visible on screen after
+ * this call completes.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
static inline int drm_panel_enable(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->enable)
@@ -112,6 +161,16 @@ static inline int drm_panel_enable(struct drm_panel *panel)
return panel ? -ENOSYS : -EINVAL;
}
+/**
+ * drm_panel_get_modes - probe the available display modes of a panel
+ * @panel: DRM panel
+ *
+ * The modes probed from the panel are automatically added to the connector
+ * that the panel is attached to.
+ *
+ * Return: The number of modes available from the panel on success or a
+ * negative error code on failure.
+ */
static inline int drm_panel_get_modes(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->get_modes)
--
2.1.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] drm/panel: Flesh out kerneldoc
2014-11-04 16:26 [PATCH] drm/panel: Flesh out kerneldoc Thierry Reding
@ 2014-11-05 12:55 ` Daniel Vetter
0 siblings, 0 replies; 2+ messages in thread
From: Daniel Vetter @ 2014-11-05 12:55 UTC (permalink / raw)
To: Thierry Reding; +Cc: Daniel Vetter, dri-devel
On Tue, Nov 04, 2014 at 05:26:13PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Write more complete kerneldoc comments for the DRM panel API and
> integrate the helpers in the DRM DocBook reference.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
A few comments below. With thos addressed this is
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
And feel free the ignore the bikeshed one of course ;-)
Cheers, Daniel
> ---
> Documentation/DocBook/drm.tmpl | 6 +++++
> drivers/gpu/drm/drm_panel.c | 61 ++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_panel.h | 59 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 126 insertions(+)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index d89ca5830697..8737f03d9a75 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2379,6 +2379,12 @@ void intel_crt_init(struct drm_device *dev)
> <title id="drm-kms-planehelpers">Plane Helper Reference</title>
> !Edrivers/gpu/drm/drm_plane_helper.c Plane Helpers
> </sect2>
> + <sect2>
> + <title>Panel Helper Reference</title>
> +!Iinclude/drm/drm_panel.h
> +!Edrivers/gpu/drm/drm_panel.c
> +!Pdrivers/gpu/drm/drm_panel.c drm panel
> + </sect2>
> </sect1>
>
> <!-- Internals: kms properties -->
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 2ef988e037b7..3dfe3c886502 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -30,12 +30,36 @@
> static DEFINE_MUTEX(panel_lock);
> static LIST_HEAD(panel_list);
>
> +/**
> + * DOC: drm panel
> + *
> + * The DRM panel helpers allow drivers to register panel objects with a
> + * central registry and provide functions to retrieve those panels in display
> + * drivers.
I think a few more words here would be useful.
"The goal is to be able to abstract away panel specific quirks and then
share panel drivers between different display drivers.""
> + */
> +
> +/**
> + * drm_panel_init - initialize a panel
> + * @panel: DRM panel
> + *
> + * Sets up internal fields of the panel so that it can subsequently be added
> + * to the registry.
> + */
> void drm_panel_init(struct drm_panel *panel)
> {
> INIT_LIST_HEAD(&panel->list);
> }
> EXPORT_SYMBOL(drm_panel_init);
>
> +/**
> + * drm_panel_add - add a panel to the global registry
> + * @panel: panel to add
> + *
> + * Add a panel to the global registry so that it can be looked up by display
> + * drivers.
> + *
> + * Return: 0 on success or a negative error code on failure.
Bikeshed: I think the usual layout for return value sections in drm is
* Returns:
* blabla
i.e. with s at the end and newline before the blabla.
> + */
> int drm_panel_add(struct drm_panel *panel)
> {
> mutex_lock(&panel_lock);
> @@ -46,6 +70,12 @@ int drm_panel_add(struct drm_panel *panel)
> }
> EXPORT_SYMBOL(drm_panel_add);
>
> +/**
> + * drm_panel_remove - remove a panel from the global registry
> + * @panel: DRM panel
> + *
> + * Removes a panel from the global registry.
> + */
> void drm_panel_remove(struct drm_panel *panel)
> {
> mutex_lock(&panel_lock);
> @@ -54,6 +84,18 @@ void drm_panel_remove(struct drm_panel *panel)
> }
> EXPORT_SYMBOL(drm_panel_remove);
>
> +/**
> + * drm_panel_attach - attach a panel to a connector
> + * @panel: DRM panel
> + * @connector: DRM connector
> + *
> + * After obtaining a pointer to a DRM panel a display driver calls this
> + * function to attach a panel to a connector.
> + *
> + * An error is returned if the panel is already attached to another connector.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> {
> if (panel->connector)
> @@ -66,6 +108,15 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> }
> EXPORT_SYMBOL(drm_panel_attach);
>
> +/**
> + * drm_panel_detach - detach a panel from a connector
> + * @panel: DRM panel
> + *
> + * Detaches a panel from the connector it is attached to. If a panel is not
> + * attached to any connector this is effectively a no-op.
> + *
> + * Return: 0 on success or a negative error code on failure.
This never fails. I think it's better to switch the return value to void
instead of documenting something that never actually happens. This kind of
interface polish is one of the main reasons I think doing kerneldoc is
useful
> + */
> int drm_panel_detach(struct drm_panel *panel)
> {
> panel->connector = NULL;
> @@ -76,6 +127,16 @@ int drm_panel_detach(struct drm_panel *panel)
> EXPORT_SYMBOL(drm_panel_detach);
>
> #ifdef CONFIG_OF
> +/**
> + * of_drm_find_panel - look up a panel using a device tree node
> + * @np: device tree node of the panel
> + *
> + * Searches the set of registered panels for one that matches the given device
> + * tree node. If a matching panel is found, return a pointer to it.
> + *
> + * Return: A pointer to the panel registered for the specified device tree
> + * node or NULL if no panel matching the device tree node can be found.
> + */
> struct drm_panel *of_drm_find_panel(struct device_node *np)
> {
> struct drm_panel *panel;
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 1fbcc96063a7..b88b4dcd698b 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -70,6 +70,14 @@ struct drm_panel_funcs {
> int (*get_modes)(struct drm_panel *panel);
> };
>
> +/**
> + * struct drm_panel - DRM panel object
> + * @drm: DRM device owning the panel
> + * @connector: DRM connector that the panel is attached to
> + * @dev: parent device of the panel
> + * @funcs: operations that can be performed on the panel
> + * @list: panel entry in registry
> + */
> struct drm_panel {
> struct drm_device *drm;
> struct drm_connector *connector;
> @@ -80,6 +88,17 @@ struct drm_panel {
> struct list_head list;
> };
>
> +/**
> + * drm_disable_unprepare - power off a panel
> + * @panel: DRM panel
I think all these static inline wrappers should start with something along
the lines of
"This function calls the ->foo() panel callback if provided."
> + *
> + * Calling this function will completely power off a panel (assert the panel's
> + * reset, turn off power supplies, ...). After this function has completed, it
> + * is usually no longer possible to communicate with the panel until another
> + * call to drm_panel_prepare().
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
But thinking about the semantics of these functions I have a few
questions:
- panel seems to be optional, which doesn't make a lot of sense imo.
Should we WARN_ON if it's not there.
- the vfuncs are all optional, which imo makes sense (I don't expect all
panels to e.g. have a prepare). But if the vfunc isnt' there the
function still fails. Usually no vfunc means silent success. But if it's
an implementation error on the panel driver's side then I think there
should be a WARN_ON both here and in the panel registration function.
> static inline int drm_panel_unprepare(struct drm_panel *panel)
> {
> if (panel && panel->funcs && panel->funcs->unprepare)
> @@ -88,6 +107,16 @@ static inline int drm_panel_unprepare(struct drm_panel *panel)
> return panel ? -ENOSYS : -EINVAL;
> }
>
> +/**
> + * drm_panel_disable - disable a panel
> + * @panel: DRM panel
> + *
> + * This will typically turn off the panel's backlight or disable the display
> + * drivers. For smart panels it should still be possible to communicate with
> + * the integrated circuitry via any command bus after this call.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> static inline int drm_panel_disable(struct drm_panel *panel)
> {
> if (panel && panel->funcs && panel->funcs->disable)
> @@ -96,6 +125,16 @@ static inline int drm_panel_disable(struct drm_panel *panel)
> return panel ? -ENOSYS : -EINVAL;
> }
>
> +/**
> + * drm_panel_prepare - power on a panel
> + * @panel: DRM panel
> + *
> + * Calling this function will enable power and deassert any reset signals to
> + * the panel. After this has completed it is possible to communicate with any
> + * integrated circuitry via a command bus.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> static inline int drm_panel_prepare(struct drm_panel *panel)
> {
> if (panel && panel->funcs && panel->funcs->prepare)
> @@ -104,6 +143,16 @@ static inline int drm_panel_prepare(struct drm_panel *panel)
> return panel ? -ENOSYS : -EINVAL;
> }
>
> +/**
> + * drm_panel_enable - enable a panel
> + * @panel: DRM panel
> + *
> + * Calling this function will cause the panel display drivers to be turned on
> + * and the backlight to be enabled. Content will be visible on screen after
> + * this call completes.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> static inline int drm_panel_enable(struct drm_panel *panel)
> {
> if (panel && panel->funcs && panel->funcs->enable)
> @@ -112,6 +161,16 @@ static inline int drm_panel_enable(struct drm_panel *panel)
> return panel ? -ENOSYS : -EINVAL;
> }
>
> +/**
> + * drm_panel_get_modes - probe the available display modes of a panel
> + * @panel: DRM panel
> + *
> + * The modes probed from the panel are automatically added to the connector
> + * that the panel is attached to.
> + *
> + * Return: The number of modes available from the panel on success or a
> + * negative error code on failure.
> + */
> static inline int drm_panel_get_modes(struct drm_panel *panel)
> {
> if (panel && panel->funcs && panel->funcs->get_modes)
> --
> 2.1.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-11-05 12:54 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-04 16:26 [PATCH] drm/panel: Flesh out kerneldoc Thierry Reding
2014-11-05 12:55 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox