All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, niklas.soderlund@ragnatech.se,
	robh@kernel.org, hverkuil@xs4all.nl, devicetree@vger.kernel.org
Subject: Re: [PATCH v5 1/5] v4l: fwnode: Move KernelDoc documentation to the header
Date: Tue, 29 Aug 2017 16:15:22 +0300	[thread overview]
Message-ID: <2676284.rKR023OhTM@avalon> (raw)
In-Reply-To: <20170829110313.19538-2-sakari.ailus@linux.intel.com>

Hi Sakari,

Thank you for the patch.

On Tuesday, 29 August 2017 14:03:09 EEST Sakari Ailus wrote:
> In V4L2 the practice is to have the KernelDoc documentation in the header
> and not in .c source code files. This consequientally makes the V4L2
> fwnode function documentation part of the Media documentation build.
> 
> Also correct the link related function and argument naming in
> documentation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

I prefer documenting functions in the C file. Documentation in header files 
will get out-of-sync with the implementation much more easily.

> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 75 --------------------------------
> include/media/v4l2-fwnode.h           | 81 ++++++++++++++++++++++++++++++++-
> 2 files changed, 80 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index 40b2fbfe8865..706f9e7b90f1
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -181,25 +181,6 @@ v4l2_fwnode_endpoint_parse_csi1_bus(struct
> fwnode_handle *fwnode, vep->bus_type = V4L2_MBUS_CSI1;
>  }
> 
> -/**
> - * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> - * @fwnode: pointer to the endpoint's fwnode handle
> - * @vep: pointer to the V4L2 fwnode data structure
> - *
> - * All properties are optional. If none are found, we don't set any flags.
> This - * means the port has a static configuration and no properties have
> to be - * specified explicitly. If any properties that identify the bus as
> parallel - * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER.
> Similarly, if - * we recognise the bus as serial CSI-2 and
> clock-noncontinuous isn't set, we - * set the
> V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a - *
> reference to @fwnode.
> - *
> - * NOTE: This function does not parse properties the size of which is
> variable - * without a low fixed limit. Please use
> v4l2_fwnode_endpoint_alloc_parse() in - * new drivers instead.
> - *
> - * Return: 0 on success or a negative error code on failure.
> - */
>  int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwnode,
>  			       struct v4l2_fwnode_endpoint *vep)
>  {
> @@ -239,14 +220,6 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle
> *fwnode, }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_parse);
> 
> -/*
> - * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> - * v4l2_fwnode_endpoint_alloc_parse()
> - * @vep - the V4L2 fwnode the resources of which are to be released
> - *
> - * It is safe to call this function with NULL argument or on a V4L2 fwnode
> the - * parsing of which failed.
> - */
>  void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep)
>  {
>  	if (IS_ERR_OR_NULL(vep))
> @@ -257,29 +230,6 @@ void v4l2_fwnode_endpoint_free(struct
> v4l2_fwnode_endpoint *vep) }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_free);
> 
> -/**
> - * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> - * @fwnode: pointer to the endpoint's fwnode handle
> - *
> - * All properties are optional. If none are found, we don't set any flags.
> This - * means the port has a static configuration and no properties have
> to be - * specified explicitly. If any properties that identify the bus as
> parallel - * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER.
> Similarly, if - * we recognise the bus as serial CSI-2 and
> clock-noncontinuous isn't set, we - * set the
> V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a - *
> reference to @fwnode.
> - *
> - * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> - * v4l2_fwnode_endpoint_parse():
> - *
> - * 1. It also parses variable size data.
> - *
> - * 2. The memory it has allocated to store the variable size data must be
> freed - *    using v4l2_fwnode_endpoint_free() when no longer needed.
> - *
> - * Return: Pointer to v4l2_fwnode_endpoint if successful, on an error
> pointer - * on error.
> - */
>  struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
>  	struct fwnode_handle *fwnode)
>  {
> @@ -322,24 +272,6 @@ struct v4l2_fwnode_endpoint
> *v4l2_fwnode_endpoint_alloc_parse( }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_alloc_parse);
> 
> -/**
> - * v4l2_fwnode_endpoint_parse_link() - parse a link between two endpoints
> - * @__fwnode: pointer to the endpoint's fwnode at the local end of the link
> - * @link: pointer to the V4L2 fwnode link data structure
> - *
> - * Fill the link structure with the local and remote nodes and port
> numbers. - * The local_node and remote_node fields are set to point to the
> local and - * remote port's parent nodes respectively (the port parent node
> being the - * parent node of the port node if that node isn't a 'ports'
> node, or the - * grand-parent node of the port node otherwise).
> - *
> - * A reference is taken to both the local and remote nodes, the caller must
> use - * v4l2_fwnode_endpoint_put_link() to drop the references when done
> with the - * link.
> - *
> - * Return: 0 on success, or -ENOLINK if the remote endpoint fwnode can't be
> - * found.
> - */
>  int v4l2_fwnode_parse_link(struct fwnode_handle *__fwnode,
>  			   struct v4l2_fwnode_link *link)
>  {
> @@ -374,13 +306,6 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> *__fwnode, }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_link);
> 
> -/**
> - * v4l2_fwnode_put_link() - drop references to nodes in a link
> - * @link: pointer to the V4L2 fwnode link data structure
> - *
> - * Drop references to the local and remote nodes in the link. This function
> - * must be called on every link parsed with v4l2_fwnode_parse_link(). - */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  {
>  	fwnode_handle_put(link->local_node);
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 7adec9851d9e..68eb22ba571b 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -113,13 +113,92 @@ struct v4l2_fwnode_link {
>  	unsigned int remote_port;
>  };
> 
> +/**
> + * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> + * @fwnode: pointer to the endpoint's fwnode handle
> + * @vep: pointer to the V4L2 fwnode data structure
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> This + * means the port has a static configuration and no properties have
> to be + * specified explicitly. If any properties that identify the bus as
> parallel + * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER.
> Similarly, if + * we recognise the bus as serial CSI-2 and
> clock-noncontinuous isn't set, we + * set the
> V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a + *
> reference to @fwnode.
> + *
> + * NOTE: This function does not parse properties the size of which is
> variable + * without a low fixed limit. Please use
> v4l2_fwnode_endpoint_alloc_parse() in + * new drivers instead.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
>  int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwnode,
>  			       struct v4l2_fwnode_endpoint *vep);
> +
> +/*
> + * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> + * v4l2_fwnode_endpoint_alloc_parse()
> + * @vep - the V4L2 fwnode the resources of which are to be released
> + *
> + * It is safe to call this function with NULL argument or on a V4L2 fwnode
> the + * parsing of which failed.
> + */
> +void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
> +
> +/**
> + * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> + * @fwnode: pointer to the endpoint's fwnode handle
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> This + * means the port has a static configuration and no properties have
> to be + * specified explicitly. If any properties that identify the bus as
> parallel + * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER.
> Similarly, if + * we recognise the bus as serial CSI-2 and
> clock-noncontinuous isn't set, we + * set the
> V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a + *
> reference to @fwnode.
> + *
> + * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> + * v4l2_fwnode_endpoint_parse():
> + *
> + * 1. It also parses variable size data.
> + *
> + * 2. The memory it has allocated to store the variable size data must be
> freed + *    using v4l2_fwnode_endpoint_free() when no longer needed.
> + *
> + * Return: Pointer to v4l2_fwnode_endpoint if successful, on an error
> pointer + * on error.
> + */
>  struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
>  	struct fwnode_handle *fwnode);
> -void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
> +
> +/**
> + * v4l2_fwnode_parse_link() - parse a link between two endpoints
> + * @fwnode: pointer to the endpoint's fwnode at the local end of the link
> + * @link: pointer to the V4L2 fwnode link data structure
> + *
> + * Fill the link structure with the local and remote nodes and port
> numbers. + * The local_node and remote_node fields are set to point to the
> local and + * remote port's parent nodes respectively (the port parent node
> being the + * parent node of the port node if that node isn't a 'ports'
> node, or the + * grand-parent node of the port node otherwise).
> + *
> + * A reference is taken to both the local and remote nodes, the caller must
> use + * v4l2_fwnode_put_link() to drop the references when done with the +
> * link.
> + *
> + * Return: 0 on success, or -ENOLINK if the remote endpoint fwnode can't be
> + * found.
> + */
>  int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>  			   struct v4l2_fwnode_link *link);
> +
> +/**
> + * v4l2_fwnode_put_link() - drop references to nodes in a link
> + * @link: pointer to the V4L2 fwnode link data structure
> + *
> + * Drop references to the local and remote nodes in the link. This function
> + * must be called on every link parsed with v4l2_fwnode_parse_link(). + */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> 
>  #endif /* _V4L2_FWNODE_H */


-- 
Regards,

Laurent Pinchart

  parent reply	other threads:[~2017-08-29 13:15 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-29 11:03 [PATCH v5 0/5] Unified fwnode endpoint parser Sakari Ailus
2017-08-29 11:03 ` Sakari Ailus
     [not found] ` <20170829110313.19538-1-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-08-29 11:03   ` [PATCH v5 1/5] v4l: fwnode: Move KernelDoc documentation to the header Sakari Ailus
2017-08-29 11:03     ` Sakari Ailus
     [not found]     ` <20170829110313.19538-2-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-08-29 12:50       ` Niklas Söderlund
2017-08-29 12:50         ` Niklas Söderlund
2017-08-29 13:15     ` Laurent Pinchart [this message]
2017-08-29 13:20       ` Sakari Ailus
2017-08-29 13:20         ` Sakari Ailus
2017-08-29 13:22         ` Sakari Ailus
2017-08-29 11:03   ` [PATCH v5 2/5] v4l: async: Add V4L2 async documentation to the documentation build Sakari Ailus
2017-08-29 11:03     ` Sakari Ailus
2017-08-29 12:52     ` Niklas Söderlund
2017-08-29 11:03   ` [PATCH v5 4/5] v4l: fwnode: Support generic parsing of graph endpoints in a device Sakari Ailus
2017-08-29 11:03     ` Sakari Ailus
     [not found]     ` <20170829110313.19538-5-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-08-29 14:02       ` Laurent Pinchart
2017-08-29 14:02         ` Laurent Pinchart
2017-08-29 14:31         ` Sakari Ailus
2017-08-29 14:31           ` Sakari Ailus
     [not found]           ` <20170829143121.6sjdx3lgcoxm6mva-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
2017-09-01  8:55             ` Laurent Pinchart
2017-09-01  8:55               ` Laurent Pinchart
2017-09-01  9:04               ` Sakari Ailus
2017-08-29 11:03 ` [PATCH v5 3/5] docs-rst: v4l: Include Qualcomm CAMSS in documentation build Sakari Ailus
     [not found]   ` <20170829110313.19538-4-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-08-29 13:32     ` Laurent Pinchart
2017-08-29 13:32       ` Laurent Pinchart
2017-08-29 11:03 ` [PATCH v5 5/5] v4l: fwnode: Support generic parsing of graph endpoints in a single port Sakari Ailus
2017-08-29 12:57   ` Niklas Söderlund

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2676284.rKR023OhTM@avalon \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-media@vger.kernel.org \
    --cc=niklas.soderlund@ragnatech.se \
    --cc=robh@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.