Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Laurent, On Tue, Sep 19, 2017 at 02:14:39PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tuesday, 19 September 2017 14:10:37 EEST Sakari Ailus wrote: > > On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote: > > > On Friday, 15 September 2017 17:17:00 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 consequently 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 > > >> Reviewed-by: Niklas Söderlund > > >> Acked-by: Hans Verkuil > > >> Acked-by: Pavel Machek > > > > > > I'm still very opposed to this. In addition to increasing the risk of > > > documentation becoming stale, it also makes review more difficult. I'm > > > reviewing patch 05/25 of this series and I have to jump around the patch > > > to verify that the documentation matches the implementation, it's really > > > annoying. > > > > I'd like to have this discussion separately from the patchset, which is > > right now in its 13th version. This patch simply makes the current state > > consistent; V4L2 async was the only part of V4L2 with KernelDoc > > documentation in .c files. > > But there's no need to move the documentation at this point until we reach an > agreement, is there ? The status quo has is that the KernelDoc is in headers. Generally, if you change parts that appear to lack framework-wide changes already done, you do those changes before making other changes since it's a no-brainer. Which is what this patch represents. If we end up moving the KernelDoc to .c files moving this back could result into an extra patch. I'm not too worried about that frankly. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Hans, On Tue, Sep 19, 2017 at 01:04:36PM +0200, Hans Verkuil wrote: > On 09/19/17 12:48, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Friday, 15 September 2017 17:17:00 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 consequently 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 > >> Reviewed-by: Niklas Söderlund > >> Acked-by: Hans Verkuil > >> Acked-by: Pavel Machek > > > > I'm still very opposed to this. In addition to increasing the risk of > > documentation becoming stale, it also makes review more difficult. I'm > > reviewing patch 05/25 of this series and I have to jump around the patch to > > verify that the documentation matches the implementation, it's really > > annoying. > > > > We should instead move all function documentation from header files to > > source > > files. > > I disagree with this. Yes, it makes reviewing harder, but when you have to > *use* these functions as e.g. a driver developer, then having it in the > header is much more convenient. For developers writing a driver and _not_ using e.g. the HTML documentation, programs like cscope point the user to the implementation of the function --- which is in the .c file, not the header. This is what I personally tend to do at least; for most of the time I ignore where exactly a given function is implemented (this is actually not self-evident in V4L2 outside async / fwnode). The rest of the kernel appears to generally have the KernelDoc in .c files, for a reason or another: 14:05:15 nauris sailus [~/scratch/src/linux]git grep '/\*\*$' -- include/|wc -l 6997 14:14:46 nauris sailus [~/scratch/src/linux]git grep '/\*\*$' -- drivers/ net/ mm/ lib/ kernel/ fs/ firmware/ init/ ipc/ block/ crypto/ |wc -l 44756 I think I'm slightly leaning towards moving it: having the documentation where the implementation is does help keeping it up-to-date. It's currently all too easy to change a function without realising it was actually documented somewhere. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Sakari, On Tuesday, 19 September 2017 14:10:37 EEST Sakari Ailus wrote: > On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote: > > On Friday, 15 September 2017 17:17:00 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 consequently 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 > >> Reviewed-by: Niklas Söderlund > >> Acked-by: Hans Verkuil > >> Acked-by: Pavel Machek > > > > I'm still very opposed to this. In addition to increasing the risk of > > documentation becoming stale, it also makes review more difficult. I'm > > reviewing patch 05/25 of this series and I have to jump around the patch > > to verify that the documentation matches the implementation, it's really > > annoying. > > I'd like to have this discussion separately from the patchset, which is > right now in its 13th version. This patch simply makes the current state > consistent; V4L2 async was the only part of V4L2 with KernelDoc > documentation in .c files. But there's no need to move the documentation at this point until we reach an agreement, is there ? -- Regards, Laurent Pinchart
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Laurent, On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Friday, 15 September 2017 17:17:00 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 consequently 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 > > Reviewed-by: Niklas Söderlund > > Acked-by: Hans Verkuil > > Acked-by: Pavel Machek > > I'm still very opposed to this. In addition to increasing the risk of > documentation becoming stale, it also makes review more difficult. I'm > reviewing patch 05/25 of this series and I have to jump around the patch to > verify that the documentation matches the implementation, it's really > annoying. I'd like to have this discussion separately from the patchset, which is right now in its 13th version. This patch simply makes the current state consistent; V4L2 async was the only part of V4L2 with KernelDoc documentation in .c files. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Hans, On Tuesday, 19 September 2017 14:04:36 EEST Hans Verkuil wrote: > On 09/19/17 12:48, Laurent Pinchart wrote: > > On Friday, 15 September 2017 17:17:00 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 consequently 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 > >> Reviewed-by: Niklas Söderlund > >> Acked-by: Hans Verkuil > >> Acked-by: Pavel Machek > > > > I'm still very opposed to this. In addition to increasing the risk of > > documentation becoming stale, it also makes review more difficult. I'm > > reviewing patch 05/25 of this series and I have to jump around the patch > > to verify that the documentation matches the implementation, it's really > > annoying. > > > > We should instead move all function documentation from header files to > > source files. > > I disagree with this. Yes, it makes reviewing harder, but when you have to > *use* these functions as e.g. a driver developer, then having it in the > header is much more convenient. When writing a driver you can use the compiled documentation. We're lacking reviewers in V4L2, we should make their life easier if we want to attract more. Furthermore, if documentation becomes stale, it will become useless for driver authors, regardless of where it's stored. > >> --- > >> > >> drivers/media/v4l2-core/v4l2-fwnode.c | 75 - > >> include/media/v4l2-fwnode.h | 81 +++- > >> 2 files changed, 80 insertions(+), 76 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
On 09/19/17 12:48, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Friday, 15 September 2017 17:17:00 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 consequently 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 >> Reviewed-by: Niklas Söderlund >> Acked-by: Hans Verkuil >> Acked-by: Pavel Machek > > I'm still very opposed to this. In addition to increasing the risk of > documentation becoming stale, it also makes review more difficult. I'm > reviewing patch 05/25 of this series and I have to jump around the patch to > verify that the documentation matches the implementation, it's really > annoying. > > We should instead move all function documentation from header files to source > files. I disagree with this. Yes, it makes reviewing harder, but when you have to *use* these functions as e.g. a driver developer, then having it in the header is much more convenient. Regards, Hans > >> --- >> drivers/media/v4l2-core/v4l2-fwnode.c | 75 >> include/media/v4l2-fwnode.h| 81 +++- >> 2 files changed, 80 insertions(+), 76 deletions(-) > > [snip] >
Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
Hi Sakari, Thank you for the patch. On Friday, 15 September 2017 17:17:00 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 consequently 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 > Reviewed-by: Niklas Söderlund > Acked-by: Hans Verkuil > Acked-by: Pavel Machek I'm still very opposed to this. In addition to increasing the risk of documentation becoming stale, it also makes review more difficult. I'm reviewing patch 05/25 of this series and I have to jump around the patch to verify that the documentation matches the implementation, it's really annoying. We should instead move all function documentation from header files to source files. > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 75 > include/media/v4l2-fwnode.h| 81 +++- > 2 files changed, 80 insertions(+), 76 deletions(-) [snip] -- Regards, Laurent Pinchart
[PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header
In V4L2 the practice is to have the KernelDoc documentation in the header
and not in .c source code files. This consequently 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
Reviewed-by: Niklas Söderlund
Acked-by: Hans Verkuil
Acked-by: Pavel Machek
---
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_p
