* [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 15:49 ` Pavel Machek
2017-09-03 17:49 ` [PATCH v7 02/18] v4l: async: Add V4L2 async documentation to the documentation build Sakari Ailus
` (17 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
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>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
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 */
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header
2017-09-03 17:49 ` [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header Sakari Ailus
@ 2017-09-04 15:49 ` Pavel Machek
2017-09-05 14:53 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Pavel Machek @ 2017-09-04 15:49 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, niklas.soderlund, robh, hverkuil, laurent.pinchart,
devicetree, sre
[-- Attachment #1: Type: text/plain, Size: 770 bytes --]
On Sun 2017-09-03 20:49:41, 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
consequientally: spelling?
> 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>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Something funny going on with utf-8 here.
Acked-by: Pavel Machek <pavel@ucw.cz>
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header
2017-09-04 15:49 ` Pavel Machek
@ 2017-09-05 14:53 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-05 14:53 UTC (permalink / raw)
To: Pavel Machek
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh, hverkuil,
laurent.pinchart, devicetree, sre
On Mon, Sep 04, 2017 at 05:49:40PM +0200, Pavel Machek wrote:
> On Sun 2017-09-03 20:49:41, 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
>
> consequientally: spelling?
>
> > 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>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Something funny going on with utf-8 here.
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
Thanks! Will fix for v9.
I had bad commit message encoding in .gitconfig, that's now fixed.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 02/18] v4l: async: Add V4L2 async documentation to the documentation build
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 03/18] docs-rst: v4l: Include Qualcomm CAMSS in " Sakari Ailus
` (16 subsequent siblings)
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
The V4L2 async wasn't part of the documentation build. Fix this.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/media/kapi/v4l2-async.rst | 3 +++
Documentation/media/kapi/v4l2-core.rst | 1 +
2 files changed, 4 insertions(+)
create mode 100644 Documentation/media/kapi/v4l2-async.rst
diff --git a/Documentation/media/kapi/v4l2-async.rst b/Documentation/media/kapi/v4l2-async.rst
new file mode 100644
index 000000000000..523ff9eb09a0
--- /dev/null
+++ b/Documentation/media/kapi/v4l2-async.rst
@@ -0,0 +1,3 @@
+V4L2 async kAPI
+^^^^^^^^^^^^^^^
+.. kernel-doc:: include/media/v4l2-async.h
diff --git a/Documentation/media/kapi/v4l2-core.rst b/Documentation/media/kapi/v4l2-core.rst
index c7434f38fd9c..5cf292037a48 100644
--- a/Documentation/media/kapi/v4l2-core.rst
+++ b/Documentation/media/kapi/v4l2-core.rst
@@ -19,6 +19,7 @@ Video4Linux devices
v4l2-mc
v4l2-mediabus
v4l2-mem2mem
+ v4l2-async
v4l2-fwnode
v4l2-rect
v4l2-tuner
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 03/18] docs-rst: v4l: Include Qualcomm CAMSS in documentation build
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 02/18] v4l: async: Add V4L2 async documentation to the documentation build Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device Sakari Ailus
` (15 subsequent siblings)
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Qualcomm CAMSS was left out from documentation build. Fix this.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/media/v4l-drivers/index.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/media/v4l-drivers/index.rst b/Documentation/media/v4l-drivers/index.rst
index 10f2ce42ece2..5c202e23616b 100644
--- a/Documentation/media/v4l-drivers/index.rst
+++ b/Documentation/media/v4l-drivers/index.rst
@@ -50,6 +50,7 @@ For more details see the file COPYING in the source distribution of Linux.
philips
pvrusb2
pxa_camera
+ qcom_camss
radiotrack
rcar-fdp1
saa7134
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (2 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 03/18] docs-rst: v4l: Include Qualcomm CAMSS in " Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 13:19 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 05/18] omap3isp: Use generic parser for parsing fwnode endpoints Sakari Ailus
` (14 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
The current practice is that drivers iterate over their endpoints and
parse each endpoint separately. This is very similar in a number of
drivers, implement a generic function for the job. Driver specific matters
can be taken into account in the driver specific callback.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 16 ++++
drivers/media/v4l2-core/v4l2-fwnode.c | 136 ++++++++++++++++++++++++++++++++++
include/media/v4l2-async.h | 24 +++++-
include/media/v4l2-fwnode.h | 53 +++++++++++++
4 files changed, 227 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 851f128eba22..6740a241d4a1 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -22,6 +22,7 @@
#include <media/v4l2-async.h>
#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
@@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
}
EXPORT_SYMBOL(v4l2_async_notifier_unregister);
+void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
+{
+ unsigned int i;
+
+ for (i = 0; i < notifier->num_subdevs; i++)
+ kfree(notifier->subdevs[i]);
+
+ notifier->max_subdevs = 0;
+ notifier->num_subdevs = 0;
+
+ kvfree(notifier->subdevs);
+ notifier->subdevs = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
+
int v4l2_async_register_subdev(struct v4l2_subdev *sd)
{
struct v4l2_async_notifier *notifier;
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 706f9e7b90f1..f8c7a9bc6a58 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -19,6 +19,7 @@
*/
#include <linux/acpi.h>
#include <linux/kernel.h>
+#include <linux/mm.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/property.h>
@@ -26,6 +27,7 @@
#include <linux/string.h>
#include <linux/types.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-fwnode.h>
enum v4l2_fwnode_bus_type {
@@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
}
EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
+static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
+ unsigned int max_subdevs)
+{
+ struct v4l2_async_subdev **subdevs;
+
+ if (max_subdevs <= notifier->max_subdevs)
+ return 0;
+
+ subdevs = kvmalloc_array(
+ max_subdevs, sizeof(*notifier->subdevs),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!subdevs)
+ return -ENOMEM;
+
+ if (notifier->subdevs) {
+ memcpy(subdevs, notifier->subdevs,
+ sizeof(*subdevs) * notifier->num_subdevs);
+
+ kvfree(notifier->subdevs);
+ }
+
+ notifier->subdevs = subdevs;
+ notifier->max_subdevs = max_subdevs;
+
+ return 0;
+}
+
+static int v4l2_async_notifier_fwnode_parse_endpoint(
+ struct device *dev, struct v4l2_async_notifier *notifier,
+ struct fwnode_handle *endpoint, unsigned int asd_struct_size,
+ int (*parse_endpoint)(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd))
+{
+ struct v4l2_async_subdev *asd;
+ struct v4l2_fwnode_endpoint *vep;
+ int ret = 0;
+
+ asd = kzalloc(asd_struct_size, GFP_KERNEL);
+ if (!asd)
+ return -ENOMEM;
+
+ asd->match.fwnode.fwnode =
+ fwnode_graph_get_remote_port_parent(endpoint);
+ if (!asd->match.fwnode.fwnode) {
+ dev_warn(dev, "bad remote port parent\n");
+ ret = -EINVAL;
+ goto out_err;
+ }
+
+ /* Ignore endpoints the parsing of which failed. */
+ vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
+ if (IS_ERR(vep)) {
+ ret = PTR_ERR(vep);
+ dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
+ ret);
+ goto out_err;
+ }
+
+ ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
+ v4l2_fwnode_endpoint_free(vep);
+ if (ret == -ENOTCONN) {
+ dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
+ vep->base.id);
+ kfree(asd);
+ return 0;
+ } else if (ret < 0) {
+ dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
+ vep->base.port, vep->base.id, ret);
+ goto out_err;
+ }
+
+ asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+ notifier->subdevs[notifier->num_subdevs] = asd;
+ notifier->num_subdevs++;
+
+ return 0;
+
+out_err:
+ fwnode_handle_put(asd->match.fwnode.fwnode);
+ kfree(asd);
+
+ return ret;
+}
+
+int v4l2_async_notifier_parse_fwnode_endpoints(
+ struct device *dev, struct v4l2_async_notifier *notifier,
+ size_t asd_struct_size,
+ int (*parse_endpoint)(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd))
+{
+ struct fwnode_handle *fwnode = NULL;
+ unsigned int max_subdevs = notifier->max_subdevs;
+ int ret;
+
+ if (asd_struct_size < sizeof(struct v4l2_async_subdev))
+ return -EINVAL;
+
+ for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
+ dev_fwnode(dev), fwnode)); )
+ if (fwnode_device_is_available(
+ fwnode_graph_get_port_parent(fwnode)))
+ max_subdevs++;
+
+ /* No subdevs to add? Return here. */
+ if (max_subdevs == notifier->max_subdevs)
+ return 0;
+
+ ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
+ if (ret)
+ return ret;
+
+ for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
+ dev_fwnode(dev), fwnode)); ) {
+ if (!fwnode_device_is_available(
+ fwnode_graph_get_port_parent(fwnode)))
+ continue;
+
+ if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
+ break;
+
+ ret = v4l2_async_notifier_fwnode_parse_endpoint(
+ dev, notifier, fwnode, asd_struct_size, parse_endpoint);
+ if (ret < 0)
+ break;
+ }
+
+ fwnode_handle_put(fwnode);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index c69d8c8a66d0..96fa1afc00dd 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -18,7 +18,6 @@ struct device;
struct device_node;
struct v4l2_device;
struct v4l2_subdev;
-struct v4l2_async_notifier;
/* A random max subdevice number, used to allocate an array on stack */
#define V4L2_MAX_SUBDEVS 128U
@@ -50,6 +49,10 @@ enum v4l2_async_match_type {
* @match: union of per-bus type matching data sets
* @list: used to link struct v4l2_async_subdev objects, waiting to be
* probed, to a notifier->waiting list
+ *
+ * When this struct is used as a member in a driver specific struct,
+ * the driver specific struct shall contain the @struct
+ * v4l2_async_subdev as its first member.
*/
struct v4l2_async_subdev {
enum v4l2_async_match_type match_type;
@@ -78,7 +81,8 @@ struct v4l2_async_subdev {
/**
* struct v4l2_async_notifier - v4l2_device notifier data
*
- * @num_subdevs: number of subdevices
+ * @num_subdevs: number of subdevices used in the subdevs array
+ * @max_subdevs: number of subdevices allocated in the subdevs array
* @subdevs: array of pointers to subdevice descriptors
* @v4l2_dev: pointer to struct v4l2_device
* @waiting: list of struct v4l2_async_subdev, waiting for their drivers
@@ -90,6 +94,7 @@ struct v4l2_async_subdev {
*/
struct v4l2_async_notifier {
unsigned int num_subdevs;
+ unsigned int max_subdevs;
struct v4l2_async_subdev **subdevs;
struct v4l2_device *v4l2_dev;
struct list_head waiting;
@@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
/**
+ * v4l2_async_notifier_release - release notifier resources
+ * @notifier: the notifier the resources of which are to be released
+ *
+ * Release memory resources related to a notifier, including the async
+ * sub-devices allocated for the purposes of the notifier. The user is
+ * responsible for releasing the notifier's resources after calling
+ * @v4l2_async_notifier_parse_fwnode_endpoints.
+ *
+ * There is no harm from calling v4l2_async_notifier_release in other
+ * cases as long as its memory has been zeroed after it has been
+ * allocated.
+ */
+void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);
+
+/**
* v4l2_async_register_subdev - registers a sub-device to the asynchronous
* subdevice framework
*
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 68eb22ba571b..6d125f26ec84 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -25,6 +25,8 @@
#include <media/v4l2-mediabus.h>
struct fwnode_handle;
+struct v4l2_async_notifier;
+struct v4l2_async_subdev;
#define V4L2_FWNODE_CSI2_MAX_DATA_LANES 4
@@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
*/
void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
+/**
+ * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
+ * device node
+ * @dev: the device the endpoints of which are to be parsed
+ * @notifier: notifier for @dev
+ * @asd_struct_size: size of the driver's async sub-device struct, including
+ * sizeof(struct v4l2_async_subdev). The &struct
+ * v4l2_async_subdev shall be the first member of
+ * the driver's async sub-device struct, i.e. both
+ * begin at the same memory address.
+ * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
+ * endpoint. Optional.
+ * Return: %0 on success
+ * %-ENOTCONN if the endpoint is to be skipped but this
+ * should not be considered as an error
+ * %-EINVAL if the endpoint configuration is invalid
+ *
+ * Parse the fwnode endpoints of the @dev device and populate the async sub-
+ * devices array of the notifier. The @parse_endpoint callback function is
+ * called for each endpoint with the corresponding async sub-device pointer to
+ * let the caller initialize the driver-specific part of the async sub-device
+ * structure.
+ *
+ * The notifier memory shall be zeroed before this function is called on the
+ * notifier.
+ *
+ * This function may not be called on a registered notifier and may be called on
+ * a notifier only once. When using this function, the user may not access the
+ * notifier's subdevs array nor change notifier's num_subdevs field, these are
+ * reserved for the framework's internal use only.
+ *
+ * The @struct v4l2_fwnode_endpoint passed to the callback function
+ * @parse_endpoint is released once the function is finished. If there is a need
+ * to retain that configuration, the user needs to allocate memory for it.
+ *
+ * Any notifier populated using this function must be released with a call to
+ * v4l2_async_notifier_release() after it has been unregistered and the async
+ * sub-devices are no longer in use.
+ *
+ * Return: %0 on success, including when no async sub-devices are found
+ * %-ENOMEM if memory allocation failed
+ * %-EINVAL if graph or endpoint parsing failed
+ * Other error codes as returned by @parse_endpoint
+ */
+int v4l2_async_notifier_parse_fwnode_endpoints(
+ struct device *dev, struct v4l2_async_notifier *notifier,
+ size_t asd_struct_size,
+ int (*parse_endpoint)(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd));
+
#endif /* _V4L2_FWNODE_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device
2017-09-03 17:49 ` [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device Sakari Ailus
@ 2017-09-04 13:19 ` Hans Verkuil
2017-09-04 15:54 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 13:19 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> The current practice is that drivers iterate over their endpoints and
> parse each endpoint separately. This is very similar in a number of
> drivers, implement a generic function for the job. Driver specific matters
> can be taken into account in the driver specific callback.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 16 ++++
> drivers/media/v4l2-core/v4l2-fwnode.c | 136 ++++++++++++++++++++++++++++++++++
> include/media/v4l2-async.h | 24 +++++-
> include/media/v4l2-fwnode.h | 53 +++++++++++++
> 4 files changed, 227 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 851f128eba22..6740a241d4a1 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,6 +22,7 @@
>
> #include <media/v4l2-async.h>
> #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> }
> EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>
> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < notifier->num_subdevs; i++)
> + kfree(notifier->subdevs[i]);
> +
> + notifier->max_subdevs = 0;
> + notifier->num_subdevs = 0;
> +
> + kvfree(notifier->subdevs);
> + notifier->subdevs = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> +
> int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> {
> struct v4l2_async_notifier *notifier;
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 706f9e7b90f1..f8c7a9bc6a58 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -19,6 +19,7 @@
> */
> #include <linux/acpi.h>
> #include <linux/kernel.h>
> +#include <linux/mm.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/property.h>
> @@ -26,6 +27,7 @@
> #include <linux/string.h>
> #include <linux/types.h>
>
> +#include <media/v4l2-async.h>
> #include <media/v4l2-fwnode.h>
>
> enum v4l2_fwnode_bus_type {
> @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> }
> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>
> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
> + unsigned int max_subdevs)
> +{
> + struct v4l2_async_subdev **subdevs;
> +
> + if (max_subdevs <= notifier->max_subdevs)
> + return 0;
> +
> + subdevs = kvmalloc_array(
> + max_subdevs, sizeof(*notifier->subdevs),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!subdevs)
> + return -ENOMEM;
> +
> + if (notifier->subdevs) {
> + memcpy(subdevs, notifier->subdevs,
> + sizeof(*subdevs) * notifier->num_subdevs);
> +
> + kvfree(notifier->subdevs);
> + }
> +
> + notifier->subdevs = subdevs;
> + notifier->max_subdevs = max_subdevs;
> +
> + return 0;
> +}
> +
> +static int v4l2_async_notifier_fwnode_parse_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct v4l2_async_subdev *asd;
> + struct v4l2_fwnode_endpoint *vep;
> + int ret = 0;
> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd)
> + return -ENOMEM;
> +
> + asd->match.fwnode.fwnode =
> + fwnode_graph_get_remote_port_parent(endpoint);
> + if (!asd->match.fwnode.fwnode) {
> + dev_warn(dev, "bad remote port parent\n");
> + ret = -EINVAL;
> + goto out_err;
> + }
> +
> + /* Ignore endpoints the parsing of which failed. */
> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> + if (IS_ERR(vep)) {
> + ret = PTR_ERR(vep);
> + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> + ret);
> + goto out_err;
> + }
> +
> + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
How likely is it that parse_endpoint will be NULL? I ask because if it is
common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
'if (parse_endpoint) {'.
The disadvantage is that you won't see parse errors, but on the other hand
nobody apparently needs it, so why bother. I'm not sure what is the right thing
to do here.
> + v4l2_fwnode_endpoint_free(vep);
Here vep is freed,
> + if (ret == -ENOTCONN) {
> + dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> + vep->base.id);
but still used here
> + kfree(asd);
> + return 0;
> + } else if (ret < 0) {
> + dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
> + vep->base.port, vep->base.id, ret);
and here.
> + goto out_err;
> + }
> +
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->subdevs[notifier->num_subdevs] = asd;
> + notifier->num_subdevs++;
> +
> + return 0;
> +
> +out_err:
> + fwnode_handle_put(asd->match.fwnode.fwnode);
> + kfree(asd);
> +
> + return ret;
> +}
> +
> +int v4l2_async_notifier_parse_fwnode_endpoints(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + size_t asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_handle *fwnode = NULL;
> + unsigned int max_subdevs = notifier->max_subdevs;
> + int ret;
> +
> + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> + return -EINVAL;
> +
> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> + dev_fwnode(dev), fwnode)); )
> + if (fwnode_device_is_available(
> + fwnode_graph_get_port_parent(fwnode)))
> + max_subdevs++;
> +
> + /* No subdevs to add? Return here. */
> + if (max_subdevs == notifier->max_subdevs)
> + return 0;
> +
> + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> + if (ret)
> + return ret;
> +
> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> + dev_fwnode(dev), fwnode)); ) {
> + if (!fwnode_device_is_available(
> + fwnode_graph_get_port_parent(fwnode)))
> + continue;
> +
> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> + break;
I think you should return an error here. I feel that if this happens, then
something very strange is going on and you are better off returning -EINVAL
or something like that.
> +
> + ret = v4l2_async_notifier_fwnode_parse_endpoint(
> + dev, notifier, fwnode, asd_struct_size, parse_endpoint);
> + if (ret < 0)
> + break;
> + }
> +
> + fwnode_handle_put(fwnode);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index c69d8c8a66d0..96fa1afc00dd 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -18,7 +18,6 @@ struct device;
> struct device_node;
> struct v4l2_device;
> struct v4l2_subdev;
> -struct v4l2_async_notifier;
>
> /* A random max subdevice number, used to allocate an array on stack */
> #define V4L2_MAX_SUBDEVS 128U
> @@ -50,6 +49,10 @@ enum v4l2_async_match_type {
> * @match: union of per-bus type matching data sets
> * @list: used to link struct v4l2_async_subdev objects, waiting to be
> * probed, to a notifier->waiting list
> + *
> + * When this struct is used as a member in a driver specific struct,
> + * the driver specific struct shall contain the @struct
> + * v4l2_async_subdev as its first member.
> */
> struct v4l2_async_subdev {
> enum v4l2_async_match_type match_type;
> @@ -78,7 +81,8 @@ struct v4l2_async_subdev {
> /**
> * struct v4l2_async_notifier - v4l2_device notifier data
> *
> - * @num_subdevs: number of subdevices
> + * @num_subdevs: number of subdevices used in the subdevs array
> + * @max_subdevs: number of subdevices allocated in the subdevs array
> * @subdevs: array of pointers to subdevice descriptors
> * @v4l2_dev: pointer to struct v4l2_device
> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> @@ -90,6 +94,7 @@ struct v4l2_async_subdev {
> */
> struct v4l2_async_notifier {
> unsigned int num_subdevs;
> + unsigned int max_subdevs;
> struct v4l2_async_subdev **subdevs;
> struct v4l2_device *v4l2_dev;
> struct list_head waiting;
> @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
>
> /**
> + * v4l2_async_notifier_release - release notifier resources
> + * @notifier: the notifier the resources of which are to be released
> + *
> + * Release memory resources related to a notifier, including the async
> + * sub-devices allocated for the purposes of the notifier. The user is
> + * responsible for releasing the notifier's resources after calling
> + * @v4l2_async_notifier_parse_fwnode_endpoints.
> + *
> + * There is no harm from calling v4l2_async_notifier_release in other
> + * cases as long as its memory has been zeroed after it has been
> + * allocated.
> + */
> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);
> +
> +/**
> * v4l2_async_register_subdev - registers a sub-device to the asynchronous
> * subdevice framework
> *
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 68eb22ba571b..6d125f26ec84 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -25,6 +25,8 @@
> #include <media/v4l2-mediabus.h>
>
> struct fwnode_handle;
> +struct v4l2_async_notifier;
> +struct v4l2_async_subdev;
>
> #define V4L2_FWNODE_CSI2_MAX_DATA_LANES 4
>
> @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> */
> void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>
> +/**
> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
> + * device node
> + * @dev: the device the endpoints of which are to be parsed
> + * @notifier: notifier for @dev
> + * @asd_struct_size: size of the driver's async sub-device struct, including
> + * sizeof(struct v4l2_async_subdev). The &struct
> + * v4l2_async_subdev shall be the first member of
> + * the driver's async sub-device struct, i.e. both
> + * begin at the same memory address.
> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> + * endpoint. Optional.
> + * Return: %0 on success
> + * %-ENOTCONN if the endpoint is to be skipped but this
> + * should not be considered as an error
> + * %-EINVAL if the endpoint configuration is invalid
> + *
> + * Parse the fwnode endpoints of the @dev device and populate the async sub-
> + * devices array of the notifier. The @parse_endpoint callback function is
> + * called for each endpoint with the corresponding async sub-device pointer to
> + * let the caller initialize the driver-specific part of the async sub-device
> + * structure.
> + *
> + * The notifier memory shall be zeroed before this function is called on the
> + * notifier.
> + *
> + * This function may not be called on a registered notifier and may be called on
> + * a notifier only once. When using this function, the user may not access the
> + * notifier's subdevs array nor change notifier's num_subdevs field, these are
> + * reserved for the framework's internal use only.
I don't think the sentence "When using...only" makes any sense. How would you
even be able to access any of the notifier fields? You don't have access to it
from the parse_endpoint callback.
I think it can just be dropped.
> + *
> + * The @struct v4l2_fwnode_endpoint passed to the callback function
> + * @parse_endpoint is released once the function is finished. If there is a need
> + * to retain that configuration, the user needs to allocate memory for it.
> + *
> + * Any notifier populated using this function must be released with a call to
> + * v4l2_async_notifier_release() after it has been unregistered and the async
> + * sub-devices are no longer in use.
> + *
> + * Return: %0 on success, including when no async sub-devices are found
> + * %-ENOMEM if memory allocation failed
> + * %-EINVAL if graph or endpoint parsing failed
> + * Other error codes as returned by @parse_endpoint
> + */
> +int v4l2_async_notifier_parse_fwnode_endpoints(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + size_t asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd));
> +
> #endif /* _V4L2_FWNODE_H */
>
Regards,
Hans
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device
2017-09-04 13:19 ` Hans Verkuil
@ 2017-09-04 15:54 ` Sakari Ailus
2017-09-04 17:37 ` Hans Verkuil
0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 15:54 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
Hi Hans,
On Mon, Sep 04, 2017 at 03:19:10PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > The current practice is that drivers iterate over their endpoints and
> > parse each endpoint separately. This is very similar in a number of
> > drivers, implement a generic function for the job. Driver specific matters
> > can be taken into account in the driver specific callback.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 16 ++++
> > drivers/media/v4l2-core/v4l2-fwnode.c | 136 ++++++++++++++++++++++++++++++++++
> > include/media/v4l2-async.h | 24 +++++-
> > include/media/v4l2-fwnode.h | 53 +++++++++++++
> > 4 files changed, 227 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 851f128eba22..6740a241d4a1 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -22,6 +22,7 @@
> >
> > #include <media/v4l2-async.h>
> > #include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> > #include <media/v4l2-subdev.h>
> >
> > static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > }
> > EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> >
> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < notifier->num_subdevs; i++)
> > + kfree(notifier->subdevs[i]);
> > +
> > + notifier->max_subdevs = 0;
> > + notifier->num_subdevs = 0;
> > +
> > + kvfree(notifier->subdevs);
> > + notifier->subdevs = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> > +
> > int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > {
> > struct v4l2_async_notifier *notifier;
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 706f9e7b90f1..f8c7a9bc6a58 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -19,6 +19,7 @@
> > */
> > #include <linux/acpi.h>
> > #include <linux/kernel.h>
> > +#include <linux/mm.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/property.h>
> > @@ -26,6 +27,7 @@
> > #include <linux/string.h>
> > #include <linux/types.h>
> >
> > +#include <media/v4l2-async.h>
> > #include <media/v4l2-fwnode.h>
> >
> > enum v4l2_fwnode_bus_type {
> > @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> > }
> > EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
> > + unsigned int max_subdevs)
> > +{
> > + struct v4l2_async_subdev **subdevs;
> > +
> > + if (max_subdevs <= notifier->max_subdevs)
> > + return 0;
> > +
> > + subdevs = kvmalloc_array(
> > + max_subdevs, sizeof(*notifier->subdevs),
> > + GFP_KERNEL | __GFP_ZERO);
> > + if (!subdevs)
> > + return -ENOMEM;
> > +
> > + if (notifier->subdevs) {
> > + memcpy(subdevs, notifier->subdevs,
> > + sizeof(*subdevs) * notifier->num_subdevs);
> > +
> > + kvfree(notifier->subdevs);
> > + }
> > +
> > + notifier->subdevs = subdevs;
> > + notifier->max_subdevs = max_subdevs;
> > +
> > + return 0;
> > +}
> > +
> > +static int v4l2_async_notifier_fwnode_parse_endpoint(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> > + int (*parse_endpoint)(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd))
> > +{
> > + struct v4l2_async_subdev *asd;
> > + struct v4l2_fwnode_endpoint *vep;
> > + int ret = 0;
> > +
> > + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > + if (!asd)
> > + return -ENOMEM;
> > +
> > + asd->match.fwnode.fwnode =
> > + fwnode_graph_get_remote_port_parent(endpoint);
> > + if (!asd->match.fwnode.fwnode) {
> > + dev_warn(dev, "bad remote port parent\n");
> > + ret = -EINVAL;
> > + goto out_err;
> > + }
> > +
> > + /* Ignore endpoints the parsing of which failed. */
> > + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > + if (IS_ERR(vep)) {
> > + ret = PTR_ERR(vep);
> > + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> > + ret);
> > + goto out_err;
> > + }
> > +
> > + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
>
> How likely is it that parse_endpoint will be NULL? I ask because if it is
> common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
> until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
> 'if (parse_endpoint) {'.
>
> The disadvantage is that you won't see parse errors, but on the other hand
> nobody apparently needs it, so why bother. I'm not sure what is the right thing
> to do here.
If a driver later on is amended with that parse_endpoint callback for a
reason or another, then a range of boards could suddenly stop working. I'd
keep it for validation purposes at least.
I can't think this could ever a performance concern.
>
> > + v4l2_fwnode_endpoint_free(vep);
>
> Here vep is freed,
Oops. I still thought it actually did release only the variable length
fields. Of course it no longer did. Will fix.
>
> > + if (ret == -ENOTCONN) {
> > + dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> > + vep->base.id);
>
> but still used here
>
> > + kfree(asd);
> > + return 0;
> > + } else if (ret < 0) {
> > + dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
> > + vep->base.port, vep->base.id, ret);
>
> and here.
>
> > + goto out_err;
> > + }
> > +
> > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > + notifier->subdevs[notifier->num_subdevs] = asd;
> > + notifier->num_subdevs++;
> > +
> > + return 0;
> > +
> > +out_err:
> > + fwnode_handle_put(asd->match.fwnode.fwnode);
> > + kfree(asd);
> > +
> > + return ret;
> > +}
> > +
> > +int v4l2_async_notifier_parse_fwnode_endpoints(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + int (*parse_endpoint)(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd))
> > +{
> > + struct fwnode_handle *fwnode = NULL;
> > + unsigned int max_subdevs = notifier->max_subdevs;
> > + int ret;
> > +
> > + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > + return -EINVAL;
> > +
> > + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(dev), fwnode)); )
> > + if (fwnode_device_is_available(
> > + fwnode_graph_get_port_parent(fwnode)))
> > + max_subdevs++;
> > +
> > + /* No subdevs to add? Return here. */
> > + if (max_subdevs == notifier->max_subdevs)
> > + return 0;
> > +
> > + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> > + if (ret)
> > + return ret;
> > +
> > + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(dev), fwnode)); ) {
> > + if (!fwnode_device_is_available(
> > + fwnode_graph_get_port_parent(fwnode)))
> > + continue;
> > +
> > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> > + break;
>
> I think you should return an error here. I feel that if this happens, then
> something very strange is going on and you are better off returning -EINVAL
> or something like that.
Yeah, I'll change it.
>
> > +
> > + ret = v4l2_async_notifier_fwnode_parse_endpoint(
> > + dev, notifier, fwnode, asd_struct_size, parse_endpoint);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + fwnode_handle_put(fwnode);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> > +
> > MODULE_LICENSE("GPL");
> > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index c69d8c8a66d0..96fa1afc00dd 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -18,7 +18,6 @@ struct device;
> > struct device_node;
> > struct v4l2_device;
> > struct v4l2_subdev;
> > -struct v4l2_async_notifier;
> >
> > /* A random max subdevice number, used to allocate an array on stack */
> > #define V4L2_MAX_SUBDEVS 128U
> > @@ -50,6 +49,10 @@ enum v4l2_async_match_type {
> > * @match: union of per-bus type matching data sets
> > * @list: used to link struct v4l2_async_subdev objects, waiting to be
> > * probed, to a notifier->waiting list
> > + *
> > + * When this struct is used as a member in a driver specific struct,
> > + * the driver specific struct shall contain the @struct
> > + * v4l2_async_subdev as its first member.
> > */
> > struct v4l2_async_subdev {
> > enum v4l2_async_match_type match_type;
> > @@ -78,7 +81,8 @@ struct v4l2_async_subdev {
> > /**
> > * struct v4l2_async_notifier - v4l2_device notifier data
> > *
> > - * @num_subdevs: number of subdevices
> > + * @num_subdevs: number of subdevices used in the subdevs array
> > + * @max_subdevs: number of subdevices allocated in the subdevs array
> > * @subdevs: array of pointers to subdevice descriptors
> > * @v4l2_dev: pointer to struct v4l2_device
> > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> > @@ -90,6 +94,7 @@ struct v4l2_async_subdev {
> > */
> > struct v4l2_async_notifier {
> > unsigned int num_subdevs;
> > + unsigned int max_subdevs;
> > struct v4l2_async_subdev **subdevs;
> > struct v4l2_device *v4l2_dev;
> > struct list_head waiting;
> > @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> >
> > /**
> > + * v4l2_async_notifier_release - release notifier resources
> > + * @notifier: the notifier the resources of which are to be released
> > + *
> > + * Release memory resources related to a notifier, including the async
> > + * sub-devices allocated for the purposes of the notifier. The user is
> > + * responsible for releasing the notifier's resources after calling
> > + * @v4l2_async_notifier_parse_fwnode_endpoints.
> > + *
> > + * There is no harm from calling v4l2_async_notifier_release in other
> > + * cases as long as its memory has been zeroed after it has been
> > + * allocated.
> > + */
> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);
> > +
> > +/**
> > * v4l2_async_register_subdev - registers a sub-device to the asynchronous
> > * subdevice framework
> > *
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 68eb22ba571b..6d125f26ec84 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> > #include <media/v4l2-mediabus.h>
> >
> > struct fwnode_handle;
> > +struct v4l2_async_notifier;
> > +struct v4l2_async_subdev;
> >
> > #define V4L2_FWNODE_CSI2_MAX_DATA_LANES 4
> >
> > @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> > */
> > void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
> > + * device node
> > + * @dev: the device the endpoints of which are to be parsed
> > + * @notifier: notifier for @dev
> > + * @asd_struct_size: size of the driver's async sub-device struct, including
> > + * sizeof(struct v4l2_async_subdev). The &struct
> > + * v4l2_async_subdev shall be the first member of
> > + * the driver's async sub-device struct, i.e. both
> > + * begin at the same memory address.
> > + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> > + * endpoint. Optional.
> > + * Return: %0 on success
> > + * %-ENOTCONN if the endpoint is to be skipped but this
> > + * should not be considered as an error
> > + * %-EINVAL if the endpoint configuration is invalid
> > + *
> > + * Parse the fwnode endpoints of the @dev device and populate the async sub-
> > + * devices array of the notifier. The @parse_endpoint callback function is
> > + * called for each endpoint with the corresponding async sub-device pointer to
> > + * let the caller initialize the driver-specific part of the async sub-device
> > + * structure.
> > + *
> > + * The notifier memory shall be zeroed before this function is called on the
> > + * notifier.
> > + *
> > + * This function may not be called on a registered notifier and may be called on
> > + * a notifier only once. When using this function, the user may not access the
> > + * notifier's subdevs array nor change notifier's num_subdevs field, these are
> > + * reserved for the framework's internal use only.
>
> I don't think the sentence "When using...only" makes any sense. How would you
> even be able to access any of the notifier fields? You don't have access to it
> from the parse_endpoint callback.
Not from the parse_endpoint callback. The notifier is first set up by the
driver, and this text applies to that --- whether or not parse_endpoint is
given.
>
> I think it can just be dropped.
>
> > + *
> > + * The @struct v4l2_fwnode_endpoint passed to the callback function
> > + * @parse_endpoint is released once the function is finished. If there is a need
> > + * to retain that configuration, the user needs to allocate memory for it.
> > + *
> > + * Any notifier populated using this function must be released with a call to
> > + * v4l2_async_notifier_release() after it has been unregistered and the async
> > + * sub-devices are no longer in use.
> > + *
> > + * Return: %0 on success, including when no async sub-devices are found
> > + * %-ENOMEM if memory allocation failed
> > + * %-EINVAL if graph or endpoint parsing failed
> > + * Other error codes as returned by @parse_endpoint
> > + */
> > +int v4l2_async_notifier_parse_fwnode_endpoints(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + size_t asd_struct_size,
> > + int (*parse_endpoint)(struct device *dev,
> > + struct v4l2_fwnode_endpoint *vep,
> > + struct v4l2_async_subdev *asd));
> > +
> > #endif /* _V4L2_FWNODE_H */
> >
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device
2017-09-04 15:54 ` Sakari Ailus
@ 2017-09-04 17:37 ` Hans Verkuil
2017-09-04 20:30 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 17:37 UTC (permalink / raw)
To: Sakari Ailus
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
On 09/04/2017 05:54 PM, Sakari Ailus wrote:
>>> +/**
>>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
>>> + * device node
>>> + * @dev: the device the endpoints of which are to be parsed
>>> + * @notifier: notifier for @dev
>>> + * @asd_struct_size: size of the driver's async sub-device struct, including
>>> + * sizeof(struct v4l2_async_subdev). The &struct
>>> + * v4l2_async_subdev shall be the first member of
>>> + * the driver's async sub-device struct, i.e. both
>>> + * begin at the same memory address.
>>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
>>> + * endpoint. Optional.
>>> + * Return: %0 on success
>>> + * %-ENOTCONN if the endpoint is to be skipped but this
>>> + * should not be considered as an error
>>> + * %-EINVAL if the endpoint configuration is invalid
>>> + *
>>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-
>>> + * devices array of the notifier. The @parse_endpoint callback function is
>>> + * called for each endpoint with the corresponding async sub-device pointer to
>>> + * let the caller initialize the driver-specific part of the async sub-device
>>> + * structure.
>>> + *
>>> + * The notifier memory shall be zeroed before this function is called on the
>>> + * notifier.
>>> + *
>>> + * This function may not be called on a registered notifier and may be called on
>>> + * a notifier only once. When using this function, the user may not access the
>>> + * notifier's subdevs array nor change notifier's num_subdevs field, these are
>>> + * reserved for the framework's internal use only.
>>
>> I don't think the sentence "When using...only" makes any sense. How would you
>> even be able to access any of the notifier fields? You don't have access to it
>> from the parse_endpoint callback.
>
> Not from the parse_endpoint callback. The notifier is first set up by the
> driver, and this text applies to that --- whether or not parse_endpoint is
> given.
What you mean is "After calling this function..." since v4l2_async_notifier_release()
needs this to release all the memory.
I'll take another look at this text when I see v8.
Regards,
Hans
>
>>
>> I think it can just be dropped.
>>
>>> + *
>>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
>>> + * @parse_endpoint is released once the function is finished. If there is a need
>>> + * to retain that configuration, the user needs to allocate memory for it.
>>> + *
>>> + * Any notifier populated using this function must be released with a call to
>>> + * v4l2_async_notifier_release() after it has been unregistered and the async
>>> + * sub-devices are no longer in use.
>>> + *
>>> + * Return: %0 on success, including when no async sub-devices are found
>>> + * %-ENOMEM if memory allocation failed
>>> + * %-EINVAL if graph or endpoint parsing failed
>>> + * Other error codes as returned by @parse_endpoint
>>> + */
>>> +int v4l2_async_notifier_parse_fwnode_endpoints(
>>> + struct device *dev, struct v4l2_async_notifier *notifier,
>>> + size_t asd_struct_size,
>>> + int (*parse_endpoint)(struct device *dev,
>>> + struct v4l2_fwnode_endpoint *vep,
>>> + struct v4l2_async_subdev *asd));
>>> +
>>> #endif /* _V4L2_FWNODE_H */
>>>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device
2017-09-04 17:37 ` Hans Verkuil
@ 2017-09-04 20:30 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 20:30 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
On Mon, Sep 04, 2017 at 07:37:09PM +0200, Hans Verkuil wrote:
> On 09/04/2017 05:54 PM, Sakari Ailus wrote:
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
> >>> + * device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, including
> >>> + * sizeof(struct v4l2_async_subdev). The &struct
> >>> + * v4l2_async_subdev shall be the first member of
> >>> + * the driver's async sub-device struct, i.e. both
> >>> + * begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + * endpoint. Optional.
> >>> + * Return: %0 on success
> >>> + * %-ENOTCONN if the endpoint is to be skipped but this
> >>> + * should not be considered as an error
> >>> + * %-EINVAL if the endpoint configuration is invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function is
> >>> + * called for each endpoint with the corresponding async sub-device pointer to
> >>> + * let the caller initialize the driver-specific part of the async sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be called on
> >>> + * a notifier only once. When using this function, the user may not access the
> >>> + * notifier's subdevs array nor change notifier's num_subdevs field, these are
> >>> + * reserved for the framework's internal use only.
> >>
> >> I don't think the sentence "When using...only" makes any sense. How would you
> >> even be able to access any of the notifier fields? You don't have access to it
> >> from the parse_endpoint callback.
> >
> > Not from the parse_endpoint callback. The notifier is first set up by the
> > driver, and this text applies to that --- whether or not parse_endpoint is
> > given.
>
> What you mean is "After calling this function..." since v4l2_async_notifier_release()
> needs this to release all the memory.
Right, that's a good point.
notifier->subdevs may be allocated by the driver as well, so
v4l2_async_notifier_release() must take that into account.
notifier->max_subdevs should be good for that.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 05/18] omap3isp: Use generic parser for parsing fwnode endpoints
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (3 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 06/18] rcar-vin: " Sakari Ailus
` (13 subsequent siblings)
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Instead of using driver implementation, use
v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
of the device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 115 +++++++++++-----------------------
drivers/media/platform/omap3isp/isp.h | 5 +-
2 files changed, 37 insertions(+), 83 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 1a428fe9f070..a546cf774d40 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2001,6 +2001,7 @@ static int isp_remove(struct platform_device *pdev)
__omap3isp_put(isp, false);
media_entity_enum_cleanup(&isp->crashed);
+ v4l2_async_notifier_release(&isp->notifier);
return 0;
}
@@ -2011,44 +2012,41 @@ enum isp_of_phy {
ISP_OF_PHY_CSIPHY2,
};
-static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
- struct isp_async_subdev *isd)
+static int isp_fwnode_parse(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd)
{
+ struct isp_async_subdev *isd =
+ container_of(asd, struct isp_async_subdev, asd);
struct isp_bus_cfg *buscfg = &isd->bus;
- struct v4l2_fwnode_endpoint vep;
- unsigned int i;
- int ret;
bool csi1 = false;
-
- ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
- if (ret)
- return ret;
+ unsigned int i;
dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
- to_of_node(fwnode), vep.base.port);
+ to_of_node(vep->base.local_fwnode), vep->base.port);
- switch (vep.base.port) {
+ switch (vep->base.port) {
case ISP_OF_PHY_PARALLEL:
buscfg->interface = ISP_INTERFACE_PARALLEL;
buscfg->bus.parallel.data_lane_shift =
- vep.bus.parallel.data_shift;
+ vep->bus.parallel.data_shift;
buscfg->bus.parallel.clk_pol =
- !!(vep.bus.parallel.flags
+ !!(vep->bus.parallel.flags
& V4L2_MBUS_PCLK_SAMPLE_FALLING);
buscfg->bus.parallel.hs_pol =
- !!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
+ !!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
buscfg->bus.parallel.vs_pol =
- !!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
+ !!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
buscfg->bus.parallel.fld_pol =
- !!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
+ !!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
buscfg->bus.parallel.data_pol =
- !!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
- buscfg->bus.parallel.bt656 = vep.bus_type == V4L2_MBUS_BT656;
+ !!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
+ buscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;
break;
case ISP_OF_PHY_CSIPHY1:
case ISP_OF_PHY_CSIPHY2:
- switch (vep.bus_type) {
+ switch (vep->bus_type) {
case V4L2_MBUS_CCP2:
case V4L2_MBUS_CSI1:
dev_dbg(dev, "CSI-1/CCP-2 configuration\n");
@@ -2060,11 +2058,11 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
break;
default:
dev_err(dev, "unsupported bus type %u\n",
- vep.bus_type);
+ vep->bus_type);
return -EINVAL;
}
- switch (vep.base.port) {
+ switch (vep->base.port) {
case ISP_OF_PHY_CSIPHY1:
if (csi1)
buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
@@ -2080,47 +2078,47 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
}
if (csi1) {
buscfg->bus.ccp2.lanecfg.clk.pos =
- vep.bus.mipi_csi1.clock_lane;
+ vep->bus.mipi_csi1.clock_lane;
buscfg->bus.ccp2.lanecfg.clk.pol =
- vep.bus.mipi_csi1.lane_polarity[0];
+ vep->bus.mipi_csi1.lane_polarity[0];
dev_dbg(dev, "clock lane polarity %u, pos %u\n",
buscfg->bus.ccp2.lanecfg.clk.pol,
buscfg->bus.ccp2.lanecfg.clk.pos);
buscfg->bus.ccp2.lanecfg.data[0].pos =
- vep.bus.mipi_csi1.data_lane;
+ vep->bus.mipi_csi1.data_lane;
buscfg->bus.ccp2.lanecfg.data[0].pol =
- vep.bus.mipi_csi1.lane_polarity[1];
+ vep->bus.mipi_csi1.lane_polarity[1];
dev_dbg(dev, "data lane polarity %u, pos %u\n",
buscfg->bus.ccp2.lanecfg.data[0].pol,
buscfg->bus.ccp2.lanecfg.data[0].pos);
buscfg->bus.ccp2.strobe_clk_pol =
- vep.bus.mipi_csi1.clock_inv;
- buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;
+ vep->bus.mipi_csi1.clock_inv;
+ buscfg->bus.ccp2.phy_layer = vep->bus.mipi_csi1.strobe;
buscfg->bus.ccp2.ccp2_mode =
- vep.bus_type == V4L2_MBUS_CCP2;
+ vep->bus_type == V4L2_MBUS_CCP2;
buscfg->bus.ccp2.vp_clk_pol = 1;
buscfg->bus.ccp2.crc = 1;
} else {
buscfg->bus.csi2.lanecfg.clk.pos =
- vep.bus.mipi_csi2.clock_lane;
+ vep->bus.mipi_csi2.clock_lane;
buscfg->bus.csi2.lanecfg.clk.pol =
- vep.bus.mipi_csi2.lane_polarities[0];
+ vep->bus.mipi_csi2.lane_polarities[0];
dev_dbg(dev, "clock lane polarity %u, pos %u\n",
buscfg->bus.csi2.lanecfg.clk.pol,
buscfg->bus.csi2.lanecfg.clk.pos);
buscfg->bus.csi2.num_data_lanes =
- vep.bus.mipi_csi2.num_data_lanes;
+ vep->bus.mipi_csi2.num_data_lanes;
for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
buscfg->bus.csi2.lanecfg.data[i].pos =
- vep.bus.mipi_csi2.data_lanes[i];
+ vep->bus.mipi_csi2.data_lanes[i];
buscfg->bus.csi2.lanecfg.data[i].pol =
- vep.bus.mipi_csi2.lane_polarities[i + 1];
+ vep->bus.mipi_csi2.lane_polarities[i + 1];
dev_dbg(dev,
"data lane %u polarity %u, pos %u\n", i,
buscfg->bus.csi2.lanecfg.data[i].pol,
@@ -2137,57 +2135,13 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
default:
dev_warn(dev, "%pOF: invalid interface %u\n",
- to_of_node(fwnode), vep.base.port);
+ to_of_node(vep->base.local_fwnode), vep->base.port);
return -EINVAL;
}
return 0;
}
-static int isp_fwnodes_parse(struct device *dev,
- struct v4l2_async_notifier *notifier)
-{
- struct fwnode_handle *fwnode = NULL;
-
- notifier->subdevs = devm_kcalloc(
- dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
- if (!notifier->subdevs)
- return -ENOMEM;
-
- while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
- (fwnode = fwnode_graph_get_next_endpoint(
- of_fwnode_handle(dev->of_node), fwnode))) {
- struct isp_async_subdev *isd;
-
- isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
- if (!isd)
- goto error;
-
- if (isp_fwnode_parse(dev, fwnode, isd)) {
- devm_kfree(dev, isd);
- continue;
- }
-
- notifier->subdevs[notifier->num_subdevs] = &isd->asd;
-
- isd->asd.match.fwnode.fwnode =
- fwnode_graph_get_remote_port_parent(fwnode);
- if (!isd->asd.match.fwnode.fwnode) {
- dev_warn(dev, "bad remote port parent\n");
- goto error;
- }
-
- isd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
- notifier->num_subdevs++;
- }
-
- return notifier->num_subdevs;
-
-error:
- fwnode_handle_put(fwnode);
- return -EINVAL;
-}
-
static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
{
struct isp_device *isp = container_of(async, struct isp_device,
@@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
+ ret = v4l2_async_notifier_parse_fwnode_endpoints(
+ &pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
+ isp_fwnode_parse);
if (ret < 0)
return ret;
@@ -2407,6 +2363,7 @@ static int isp_probe(struct platform_device *pdev)
__omap3isp_put(isp, false);
error:
mutex_destroy(&isp->isp_mutex);
+ v4l2_async_notifier_release(&isp->notifier);
return ret;
}
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index e528df6efc09..8b9043db94b3 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -220,14 +220,11 @@ struct isp_device {
unsigned int sbl_resources;
unsigned int subclk_resources;
-
-#define ISP_MAX_SUBDEVS 8
- struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
};
struct isp_async_subdev {
- struct isp_bus_cfg bus;
struct v4l2_async_subdev asd;
+ struct isp_bus_cfg bus;
};
#define v4l2_subdev_to_bus_cfg(sd) \
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 06/18] rcar-vin: Use generic parser for parsing fwnode endpoints
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (4 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 05/18] omap3isp: Use generic parser for parsing fwnode endpoints Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices Sakari Ailus
` (12 subsequent siblings)
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Instead of using driver implementation, use
v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
of the device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/rcar-vin/rcar-core.c | 112 +++++++++-------------------
drivers/media/platform/rcar-vin/rcar-dma.c | 10 +--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 ++--
drivers/media/platform/rcar-vin/rcar-vin.h | 4 +-
4 files changed, 48 insertions(+), 92 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 142de447aaaa..bd551f0be213 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -21,6 +21,7 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-fwnode.h>
#include "rcar-vin.h"
@@ -77,14 +78,14 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
int ret;
/* Verify subdevices mbus format */
- if (!rvin_mbus_supported(&vin->digital)) {
+ if (!rvin_mbus_supported(vin->digital)) {
vin_err(vin, "Unsupported media bus format for %s\n",
- vin->digital.subdev->name);
+ vin->digital->subdev->name);
return -EINVAL;
}
vin_dbg(vin, "Found media bus format for %s: %d\n",
- vin->digital.subdev->name, vin->digital.code);
+ vin->digital->subdev->name, vin->digital->code);
ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
if (ret < 0) {
@@ -103,7 +104,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
rvin_v4l2_remove(vin);
- vin->digital.subdev = NULL;
+ vin->digital->subdev = NULL;
}
static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
@@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
if (ret < 0)
return ret;
- vin->digital.source_pad = ret;
+ vin->digital->source_pad = ret;
ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
- vin->digital.sink_pad = ret < 0 ? 0 : ret;
+ vin->digital->sink_pad = ret < 0 ? 0 : ret;
- vin->digital.subdev = subdev;
+ vin->digital->subdev = subdev;
vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
- subdev->name, vin->digital.source_pad,
- vin->digital.sink_pad);
+ subdev->name, vin->digital->source_pad,
+ vin->digital->sink_pad);
return 0;
}
-static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
- struct device_node *ep,
- struct v4l2_mbus_config *mbus_cfg)
+static int rvin_digital_parse_v4l2(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd)
{
- struct v4l2_fwnode_endpoint v4l2_ep;
- int ret;
+ struct rvin_dev *vin = dev_get_drvdata(dev);
+ struct rvin_graph_entity *rvge =
+ container_of(asd, struct rvin_graph_entity, asd);
- ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
- if (ret) {
- vin_err(vin, "Could not parse v4l2 endpoint\n");
- return -EINVAL;
- }
+ if (vep->base.port || vep->base.id)
+ return -EPERM;
- mbus_cfg->type = v4l2_ep.bus_type;
+ rvge->mbus_cfg.type = vep->bus_type;
- switch (mbus_cfg->type) {
+ switch (rvge->mbus_cfg.type) {
case V4L2_MBUS_PARALLEL:
vin_dbg(vin, "Found PARALLEL media bus\n");
- mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
+ rvge->mbus_cfg.flags = vep->bus.parallel.flags;
break;
case V4L2_MBUS_BT656:
vin_dbg(vin, "Found BT656 media bus\n");
- mbus_cfg->flags = 0;
+ rvge->mbus_cfg.flags = 0;
break;
default:
vin_err(vin, "Unknown media bus type\n");
return -EINVAL;
}
- return 0;
-}
-
-static int rvin_digital_graph_parse(struct rvin_dev *vin)
-{
- struct device_node *ep, *np;
- int ret;
-
- vin->digital.asd.match.fwnode.fwnode = NULL;
- vin->digital.subdev = NULL;
-
- /*
- * Port 0 id 0 is local digital input, try to get it.
- * Not all instances can or will have this, that is OK
- */
- ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
- if (!ep)
- return 0;
-
- np = of_graph_get_remote_port_parent(ep);
- if (!np) {
- vin_err(vin, "No remote parent for digital input\n");
- of_node_put(ep);
- return -EINVAL;
- }
- of_node_put(np);
-
- ret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg);
- of_node_put(ep);
- if (ret)
- return ret;
-
- vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
- vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+ vin->digital = rvge;
return 0;
}
static int rvin_digital_graph_init(struct rvin_dev *vin)
{
- struct v4l2_async_subdev **subdevs = NULL;
int ret;
- ret = rvin_digital_graph_parse(vin);
+ ret = v4l2_async_notifier_parse_fwnode_endpoints(
+ vin->dev, &vin->notifier,
+ sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
if (ret)
return ret;
- if (!vin->digital.asd.match.fwnode.fwnode) {
- vin_dbg(vin, "No digital subdevice found\n");
- return -ENODEV;
- }
-
- /* Register the subdevices notifier. */
- subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
- if (subdevs == NULL)
- return -ENOMEM;
-
- subdevs[0] = &vin->digital.asd;
-
- vin_dbg(vin, "Found digital subdevice %pOF\n",
- to_of_node(subdevs[0]->match.fwnode.fwnode));
+ if (vin->notifier.num_subdevs > 0)
+ vin_dbg(vin, "Found digital subdevice %pOF\n",
+ to_of_node(
+ vin->notifier.subdevs[0]->match.fwnode.fwnode));
- vin->notifier.num_subdevs = 1;
- vin->notifier.subdevs = subdevs;
vin->notifier.bound = rvin_digital_notify_bound;
vin->notifier.unbind = rvin_digital_notify_unbind;
vin->notifier.complete = rvin_digital_notify_complete;
-
ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
if (ret < 0) {
vin_err(vin, "Notifier registration failed\n");
@@ -290,6 +244,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
if (ret)
return ret;
+ platform_set_drvdata(pdev, vin);
+
ret = rvin_digital_graph_init(vin);
if (ret < 0)
goto error;
@@ -297,11 +253,10 @@ static int rcar_vin_probe(struct platform_device *pdev)
pm_suspend_ignore_children(&pdev->dev, true);
pm_runtime_enable(&pdev->dev);
- platform_set_drvdata(pdev, vin);
-
return 0;
error:
rvin_dma_remove(vin);
+ v4l2_async_notifier_release(&vin->notifier);
return ret;
}
@@ -313,6 +268,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
v4l2_async_notifier_unregister(&vin->notifier);
+ v4l2_async_notifier_release(&vin->notifier);
rvin_dma_remove(vin);
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b136844499f6..23fdff7a7370 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -183,7 +183,7 @@ static int rvin_setup(struct rvin_dev *vin)
/*
* Input interface
*/
- switch (vin->digital.code) {
+ switch (vin->digital->code) {
case MEDIA_BUS_FMT_YUYV8_1X16:
/* BT.601/BT.1358 16bit YCbCr422 */
vnmc |= VNMC_INF_YUV16;
@@ -191,7 +191,7 @@ static int rvin_setup(struct rvin_dev *vin)
break;
case MEDIA_BUS_FMT_UYVY8_2X8:
/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
- vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
+ vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
input_is_yuv = true;
break;
@@ -200,7 +200,7 @@ static int rvin_setup(struct rvin_dev *vin)
break;
case MEDIA_BUS_FMT_UYVY10_2X10:
/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
- vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
+ vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
input_is_yuv = true;
break;
@@ -212,11 +212,11 @@ static int rvin_setup(struct rvin_dev *vin)
dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
/* Hsync Signal Polarity Select */
- if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+ if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
dmr2 |= VNDMR2_HPS;
/* Vsync Signal Polarity Select */
- if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+ if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
dmr2 |= VNDMR2_VPS;
/*
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index dd37ea811680..b479b882da12 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
struct v4l2_mbus_framefmt *mf = &fmt.format;
int ret;
- fmt.pad = vin->digital.source_pad;
+ fmt.pad = vin->digital->source_pad;
ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
if (ret)
@@ -172,13 +172,13 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
sd = vin_to_source(vin);
- v4l2_fill_mbus_format(&format.format, pix, vin->digital.code);
+ v4l2_fill_mbus_format(&format.format, pix, vin->digital->code);
pad_cfg = v4l2_subdev_alloc_pad_config(sd);
if (pad_cfg == NULL)
return -ENOMEM;
- format.pad = vin->digital.source_pad;
+ format.pad = vin->digital->source_pad;
field = pix->field;
@@ -555,7 +555,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
if (timings->pad)
return -EINVAL;
- timings->pad = vin->digital.sink_pad;
+ timings->pad = vin->digital->sink_pad;
ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
@@ -607,7 +607,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
if (cap->pad)
return -EINVAL;
- cap->pad = vin->digital.sink_pad;
+ cap->pad = vin->digital->sink_pad;
ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
@@ -625,7 +625,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
if (edid->pad)
return -EINVAL;
- edid->pad = vin->digital.sink_pad;
+ edid->pad = vin->digital->sink_pad;
ret = v4l2_subdev_call(sd, pad, get_edid, edid);
@@ -643,7 +643,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
if (edid->pad)
return -EINVAL;
- edid->pad = vin->digital.sink_pad;
+ edid->pad = vin->digital->sink_pad;
ret = v4l2_subdev_call(sd, pad, set_edid, edid);
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 9bfb5a7c4dc4..5382078143fb 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -126,7 +126,7 @@ struct rvin_dev {
struct v4l2_device v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;
struct v4l2_async_notifier notifier;
- struct rvin_graph_entity digital;
+ struct rvin_graph_entity *digital;
struct mutex lock;
struct vb2_queue queue;
@@ -145,7 +145,7 @@ struct rvin_dev {
struct v4l2_rect compose;
};
-#define vin_to_source(vin) vin->digital.subdev
+#define vin_to_source(vin) ((vin)->digital->subdev)
/* Debug */
#define vin_dbg(d, fmt, arg...) dev_dbg(d->dev, fmt, ##arg)
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (5 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 06/18] rcar-vin: " Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 13:28 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 08/18] omap3isp: Print the name of the entity where no source pads could be found Sakari Ailus
` (11 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
We only want to link sub-devices that were bound to the async notifier the
isp driver registered but there may be other sub-devices in the
v4l2_device as well. Check for the correct async notifier.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index a546cf774d40..3b1a9cd0e591 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2155,7 +2155,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
return ret;
list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
- if (!sd->asd)
+ if (sd->notifier != &isp->notifier)
continue;
ret = isp_link_entity(isp, &sd->entity,
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices
2017-09-03 17:49 ` [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices Sakari Ailus
@ 2017-09-04 13:28 ` Hans Verkuil
2017-09-04 16:07 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 13:28 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> We only want to link sub-devices that were bound to the async notifier the
> isp driver registered but there may be other sub-devices in the
> v4l2_device as well. Check for the correct async notifier.
Just to be sure I understand this correctly: the original code wasn't wrong as such,
but this new test is just more precise.
Right?
Hans
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index a546cf774d40..3b1a9cd0e591 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2155,7 +2155,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> return ret;
>
> list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> - if (!sd->asd)
> + if (sd->notifier != &isp->notifier)
> continue;
>
> ret = isp_link_entity(isp, &sd->entity,
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices
2017-09-04 13:28 ` Hans Verkuil
@ 2017-09-04 16:07 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:07 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
On Mon, Sep 04, 2017 at 03:28:04PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > We only want to link sub-devices that were bound to the async notifier the
> > isp driver registered but there may be other sub-devices in the
> > v4l2_device as well. Check for the correct async notifier.
>
> Just to be sure I understand this correctly: the original code wasn't wrong as such,
> but this new test is just more precise.
Well, it would be wrong very soon. :-)
So yes. As long as there's just a single user of the async notifiers in for
a V4L2 device, what used to be there works. The other drivers don't seem to
be affected.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 08/18] omap3isp: Print the name of the entity where no source pads could be found
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (6 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure Sakari Ailus
` (10 subsequent siblings)
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
If no source pads are found in an entity, print the name of the entity.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 3b1a9cd0e591..9a694924e46e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1669,8 +1669,8 @@ static int isp_link_entity(
break;
}
if (i == entity->num_pads) {
- dev_err(isp->dev, "%s: no source pad in external entity\n",
- __func__);
+ dev_err(isp->dev, "%s: no source pad in external entity %s\n",
+ __func__, entity->name);
return -EINVAL;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (7 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 08/18] omap3isp: Print the name of the entity where no source pads could be found Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 13:48 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks Sakari Ailus
` (9 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The async subdev notifier .bound(), .unbind() and .complete() operations
are function pointers stored directly in the v4l2_async_subdev
structure. As the structure isn't immutable, this creates a potential
security risk as the function pointers are mutable.
To fix this, move the function pointers to a new
v4l2_async_subdev_operations structure that can be made const in
drivers.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/am437x/am437x-vpfe.c | 8 +++++--
drivers/media/platform/atmel/atmel-isc.c | 10 ++++++---
drivers/media/platform/atmel/atmel-isi.c | 10 ++++++---
drivers/media/platform/davinci/vpif_capture.c | 8 +++++--
drivers/media/platform/davinci/vpif_display.c | 8 +++++--
drivers/media/platform/exynos4-is/media-dev.c | 8 +++++--
drivers/media/platform/omap3isp/isp.c | 6 +++++-
drivers/media/platform/pxa_camera.c | 8 +++++--
drivers/media/platform/qcom/camss-8x16/camss.c | 8 +++++--
drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++---
drivers/media/platform/rcar_drif.c | 10 ++++++---
drivers/media/platform/soc_camera/soc_camera.c | 14 +++++++------
drivers/media/platform/stm32/stm32-dcmi.c | 10 ++++++---
drivers/media/platform/ti-vpe/cal.c | 8 +++++--
drivers/media/platform/xilinx/xilinx-vipp.c | 8 +++++--
drivers/media/v4l2-core/v4l2-async.c | 20 +++++++++---------
drivers/staging/media/imx/imx-media-dev.c | 8 +++++--
include/media/v4l2-async.h | 29 +++++++++++++++++---------
18 files changed, 131 insertions(+), 60 deletions(-)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index dfcc484cab89..0997c640191d 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2417,6 +2417,11 @@ static int vpfe_async_complete(struct v4l2_async_notifier *notifier)
return vpfe_probe_complete(vpfe);
}
+static const struct v4l2_async_notifier_operations vpfe_async_ops = {
+ .bound = vpfe_async_bound,
+ .complete = vpfe_async_complete,
+};
+
static struct vpfe_config *
vpfe_get_pdata(struct platform_device *pdev)
{
@@ -2590,8 +2595,7 @@ static int vpfe_probe(struct platform_device *pdev)
vpfe->notifier.subdevs = vpfe->cfg->asd;
vpfe->notifier.num_subdevs = ARRAY_SIZE(vpfe->cfg->asd);
- vpfe->notifier.bound = vpfe_async_bound;
- vpfe->notifier.complete = vpfe_async_complete;
+ vpfe->notifier.ops = &vpfe_async_ops;
ret = v4l2_async_notifier_register(&vpfe->v4l2_dev,
&vpfe->notifier);
if (ret) {
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index d7103c5f92c3..48544c4137cb 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1639,6 +1639,12 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
return 0;
}
+static const struct v4l2_async_notifier_operations isc_async_ops = {
+ .bound = isc_async_bound,
+ .unbind = isc_async_unbind,
+ .complete = isc_async_complete,
+};
+
static void isc_subdev_cleanup(struct isc_device *isc)
{
struct isc_subdev_entity *subdev_entity;
@@ -1851,9 +1857,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
subdev_entity->notifier.subdevs = &subdev_entity->asd;
subdev_entity->notifier.num_subdevs = 1;
- subdev_entity->notifier.bound = isc_async_bound;
- subdev_entity->notifier.unbind = isc_async_unbind;
- subdev_entity->notifier.complete = isc_async_complete;
+ subdev_entity->notifier.ops = &isc_async_ops;
ret = v4l2_async_notifier_register(&isc->v4l2_dev,
&subdev_entity->notifier);
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 891fa2505efa..eadbf9def358 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1105,6 +1105,12 @@ static int isi_graph_notify_bound(struct v4l2_async_notifier *notifier,
return 0;
}
+static const struct v4l2_async_notifier_operations isi_graph_notify_ops = {
+ .bound = isi_graph_notify_bound,
+ .unbind = isi_graph_notify_unbind,
+ .complete = isi_graph_notify_complete,
+};
+
static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
{
struct device_node *ep = NULL;
@@ -1152,9 +1158,7 @@ static int isi_graph_init(struct atmel_isi *isi)
isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
- isi->notifier.bound = isi_graph_notify_bound;
- isi->notifier.unbind = isi_graph_notify_unbind;
- isi->notifier.complete = isi_graph_notify_complete;
+ isi->notifier.ops = &isi_graph_notify_ops;
ret = v4l2_async_notifier_register(&isi->v4l2_dev, &isi->notifier);
if (ret < 0) {
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 0ef36cec21d1..a89367ab1e06 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1500,6 +1500,11 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
return vpif_probe_complete();
}
+static const struct v4l2_async_notifier_operations vpif_async_ops = {
+ .bound = vpif_async_bound,
+ .complete = vpif_async_complete,
+};
+
static struct vpif_capture_config *
vpif_capture_get_pdata(struct platform_device *pdev)
{
@@ -1691,8 +1696,7 @@ static __init int vpif_probe(struct platform_device *pdev)
} else {
vpif_obj.notifier.subdevs = vpif_obj.config->asd;
vpif_obj.notifier.num_subdevs = vpif_obj.config->asd_sizes[0];
- vpif_obj.notifier.bound = vpif_async_bound;
- vpif_obj.notifier.complete = vpif_async_complete;
+ vpif_obj.notifier.ops = &vpif_async_ops;
err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
&vpif_obj.notifier);
if (err) {
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index 56fe4e5b396e..ff2f75a328c9 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -1232,6 +1232,11 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
return vpif_probe_complete();
}
+static const struct v4l2_async_notifier_operations vpif_async_ops = {
+ .bound = vpif_async_bound,
+ .complete = vpif_async_complete,
+};
+
/*
* vpif_probe: This function creates device entries by register itself to the
* V4L2 driver and initializes fields of each channel objects
@@ -1313,8 +1318,7 @@ static __init int vpif_probe(struct platform_device *pdev)
} else {
vpif_obj.notifier.subdevs = vpif_obj.config->asd;
vpif_obj.notifier.num_subdevs = vpif_obj.config->asd_sizes[0];
- vpif_obj.notifier.bound = vpif_async_bound;
- vpif_obj.notifier.complete = vpif_async_complete;
+ vpif_obj.notifier.ops = &vpif_async_ops;
err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
&vpif_obj.notifier);
if (err) {
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index d4656d5175d7..c15596b56dc9 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1405,6 +1405,11 @@ static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
return media_device_register(&fmd->media_dev);
}
+static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
+ .bound = subdev_notifier_bound,
+ .complete = subdev_notifier_complete,
+};
+
static int fimc_md_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1479,8 +1484,7 @@ static int fimc_md_probe(struct platform_device *pdev)
if (fmd->num_sensors > 0) {
fmd->subdev_notifier.subdevs = fmd->async_subdevs;
fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
- fmd->subdev_notifier.bound = subdev_notifier_bound;
- fmd->subdev_notifier.complete = subdev_notifier_complete;
+ fmd->subdev_notifier.ops = &subdev_notifier_ops;
fmd->num_sensors = 0;
ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 9a694924e46e..0f08d602f756 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2171,6 +2171,10 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
return media_device_register(&isp->media_dev);
}
+static const struct v4l2_async_notifier_operations isp_subdev_notifier_ops = {
+ .complete = isp_subdev_notifier_complete,
+};
+
/*
* isp_probe - Probe ISP platform device
* @pdev: Pointer to ISP platform device
@@ -2341,7 +2345,7 @@ static int isp_probe(struct platform_device *pdev)
if (ret < 0)
goto error_register_entities;
- isp->notifier.complete = isp_subdev_notifier_complete;
+ isp->notifier.ops = &isp_subdev_notifier_ops;
ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
if (ret)
diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index edca993c2b1f..9d3f0cb1d95a 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2221,6 +2221,11 @@ static void pxa_camera_sensor_unbind(struct v4l2_async_notifier *notifier,
mutex_unlock(&pcdev->mlock);
}
+static const struct v4l2_async_notifier_operations pxa_camera_sensor_ops = {
+ .bound = pxa_camera_sensor_bound,
+ .unbind = pxa_camera_sensor_unbind,
+};
+
/*
* Driver probe, remove, suspend and resume operations
*/
@@ -2489,8 +2494,7 @@ static int pxa_camera_probe(struct platform_device *pdev)
pcdev->asds[0] = &pcdev->asd;
pcdev->notifier.subdevs = pcdev->asds;
pcdev->notifier.num_subdevs = 1;
- pcdev->notifier.bound = pxa_camera_sensor_bound;
- pcdev->notifier.unbind = pxa_camera_sensor_unbind;
+ pcdev->notifier.ops = &pxa_camera_sensor_ops;
if (!of_have_populated_dt())
pcdev->asd.match_type = V4L2_ASYNC_MATCH_I2C;
diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
index a3760b5dd1d1..390a42c17b66 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss.c
@@ -601,6 +601,11 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
return media_device_register(&camss->media_dev);
}
+static const struct v4l2_async_notifier_operations camss_subdev_notifier_ops = {
+ .bound = camss_subdev_notifier_bound,
+ .complete = camss_subdev_notifier_complete,
+};
+
static const struct media_device_ops camss_media_ops = {
.link_notify = v4l2_pipeline_link_notify,
};
@@ -655,8 +660,7 @@ static int camss_probe(struct platform_device *pdev)
goto err_register_entities;
if (camss->notifier.num_subdevs) {
- camss->notifier.bound = camss_subdev_notifier_bound;
- camss->notifier.complete = camss_subdev_notifier_complete;
+ camss->notifier.ops = &camss_subdev_notifier_ops;
ret = v4l2_async_notifier_register(&camss->v4l2_dev,
&camss->notifier);
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index bd551f0be213..f55b0ef29a56 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -134,6 +134,12 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
return 0;
}
+static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
+ .bound = rvin_digital_notify_bound,
+ .unbind = rvin_digital_notify_unbind,
+ .complete = rvin_digital_notify_complete,
+};
+
static int rvin_digital_parse_v4l2(struct device *dev,
struct v4l2_fwnode_endpoint *vep,
@@ -182,9 +188,7 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
to_of_node(
vin->notifier.subdevs[0]->match.fwnode.fwnode));
- vin->notifier.bound = rvin_digital_notify_bound;
- vin->notifier.unbind = rvin_digital_notify_unbind;
- vin->notifier.complete = rvin_digital_notify_complete;
+ vin->notifier.ops = &rvin_digital_notify_ops;
ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
if (ret < 0) {
vin_err(vin, "Notifier registration failed\n");
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 522364ff0d5d..0b2214d6d621 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1185,6 +1185,12 @@ static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
return ret;
}
+static const struct v4l2_async_notifier_operations rcar_drif_notify_ops = {
+ .bound = rcar_drif_notify_bound,
+ .unbind = rcar_drif_notify_unbind,
+ .complete = rcar_drif_notify_complete,
+};
+
/* Read endpoint properties */
static void rcar_drif_get_ep_properties(struct rcar_drif_sdr *sdr,
struct fwnode_handle *fwnode)
@@ -1347,9 +1353,7 @@ static int rcar_drif_sdr_probe(struct rcar_drif_sdr *sdr)
if (ret)
goto error;
- sdr->notifier.bound = rcar_drif_notify_bound;
- sdr->notifier.unbind = rcar_drif_notify_unbind;
- sdr->notifier.complete = rcar_drif_notify_complete;
+ sdr->notifier.ops = &rcar_drif_notify_ops;
/* Register notifier */
ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 1f3c450c7a69..916ff68b73d4 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1391,6 +1391,12 @@ static int soc_camera_async_complete(struct v4l2_async_notifier *notifier)
return 0;
}
+static const struct v4l2_async_notifier_operations soc_camera_async_ops = {
+ .bound = soc_camera_async_bound,
+ .unbind = soc_camera_async_unbind,
+ .complete = soc_camera_async_complete,
+};
+
static int scan_async_group(struct soc_camera_host *ici,
struct v4l2_async_subdev **asd, unsigned int size)
{
@@ -1437,9 +1443,7 @@ static int scan_async_group(struct soc_camera_host *ici,
sasc->notifier.subdevs = asd;
sasc->notifier.num_subdevs = size;
- sasc->notifier.bound = soc_camera_async_bound;
- sasc->notifier.unbind = soc_camera_async_unbind;
- sasc->notifier.complete = soc_camera_async_complete;
+ sasc->notifier.ops = &soc_camera_async_ops;
icd->sasc = sasc;
icd->parent = ici->v4l2_dev.dev;
@@ -1537,9 +1541,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
sasc->notifier.subdevs = &info->subdev;
sasc->notifier.num_subdevs = 1;
- sasc->notifier.bound = soc_camera_async_bound;
- sasc->notifier.unbind = soc_camera_async_unbind;
- sasc->notifier.complete = soc_camera_async_complete;
+ sasc->notifier.ops = &soc_camera_async_ops;
icd->sasc = sasc;
icd->parent = ici->v4l2_dev.dev;
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 35ba6f211b79..ac4c450a6c7d 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1495,6 +1495,12 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
return 0;
}
+static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
+ .bound = dcmi_graph_notify_bound,
+ .unbind = dcmi_graph_notify_unbind,
+ .complete = dcmi_graph_notify_complete,
+};
+
static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
{
struct device_node *ep = NULL;
@@ -1542,9 +1548,7 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
dcmi->notifier.subdevs = subdevs;
dcmi->notifier.num_subdevs = 1;
- dcmi->notifier.bound = dcmi_graph_notify_bound;
- dcmi->notifier.unbind = dcmi_graph_notify_unbind;
- dcmi->notifier.complete = dcmi_graph_notify_complete;
+ dcmi->notifier.ops = &dcmi_graph_notify_ops;
ret = v4l2_async_notifier_register(&dcmi->v4l2_dev, &dcmi->notifier);
if (ret < 0) {
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 42e383a48ffe..8b586c864524 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1522,6 +1522,11 @@ static int cal_async_complete(struct v4l2_async_notifier *notifier)
return 0;
}
+static const struct v4l2_async_notifier_operations cal_async_ops = {
+ .bound = cal_async_bound,
+ .complete = cal_async_complete,
+};
+
static int cal_complete_ctx(struct cal_ctx *ctx)
{
struct video_device *vfd;
@@ -1736,8 +1741,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
ctx->asd_list[0] = asd;
ctx->notifier.subdevs = ctx->asd_list;
ctx->notifier.num_subdevs = 1;
- ctx->notifier.bound = cal_async_bound;
- ctx->notifier.complete = cal_async_complete;
+ ctx->notifier.ops = &cal_async_ops;
ret = v4l2_async_notifier_register(&ctx->v4l2_dev,
&ctx->notifier);
if (ret) {
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index ebfdf334d99c..d881cf09876d 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -351,6 +351,11 @@ static int xvip_graph_notify_bound(struct v4l2_async_notifier *notifier,
return -EINVAL;
}
+static const struct v4l2_async_notifier_operations xvip_graph_notify_ops = {
+ .bound = xvip_graph_notify_bound,
+ .complete = xvip_graph_notify_complete,
+};
+
static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
struct device_node *node)
{
@@ -548,8 +553,7 @@ static int xvip_graph_init(struct xvip_composite_device *xdev)
xdev->notifier.subdevs = subdevs;
xdev->notifier.num_subdevs = num_subdevs;
- xdev->notifier.bound = xvip_graph_notify_bound;
- xdev->notifier.complete = xvip_graph_notify_complete;
+ xdev->notifier.ops = &xvip_graph_notify_ops;
ret = v4l2_async_notifier_register(&xdev->v4l2_dev, &xdev->notifier);
if (ret < 0) {
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 6740a241d4a1..810f5e0273dc 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -107,16 +107,16 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
{
int ret;
- if (notifier->bound) {
- ret = notifier->bound(notifier, sd, asd);
+ if (notifier->ops->bound) {
+ ret = notifier->ops->bound(notifier, sd, asd);
if (ret < 0)
return ret;
}
ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
if (ret < 0) {
- if (notifier->unbind)
- notifier->unbind(notifier, sd, asd);
+ if (notifier->ops->unbind)
+ notifier->ops->unbind(notifier, sd, asd);
return ret;
}
@@ -128,8 +128,8 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
/* Move from the global subdevice list to notifier's done */
list_move(&sd->async_list, ¬ifier->done);
- if (list_empty(¬ifier->waiting) && notifier->complete)
- return notifier->complete(notifier);
+ if (list_empty(¬ifier->waiting) && notifier->ops->complete)
+ return notifier->ops->complete(notifier);
return 0;
}
@@ -232,8 +232,8 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
/* If we handled USB devices, we'd have to lock the parent too */
device_release_driver(d);
- if (notifier->unbind)
- notifier->unbind(notifier, sd, sd->asd);
+ if (notifier->ops->unbind)
+ notifier->ops->unbind(notifier, sd, sd->asd);
/*
* Store device at the device cache, in order to call
@@ -344,8 +344,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
v4l2_async_cleanup(sd);
- if (notifier->unbind)
- notifier->unbind(notifier, sd, sd->asd);
+ if (notifier->ops->unbind)
+ notifier->ops->unbind(notifier, sd, sd->asd);
mutex_unlock(&list_lock);
}
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index d96f4512224f..e8dc2c93658a 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -440,6 +440,11 @@ static int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
return media_device_register(&imxmd->md);
}
+static const struct v4l2_async_notifier_operations imx_media_subdev_ops = {
+ .bound = imx_media_subdev_bound,
+ .complete = imx_media_probe_complete,
+};
+
/*
* adds controls to a video device from an entity subdevice.
* Continues upstream from the entity's sink pads.
@@ -608,8 +613,7 @@ static int imx_media_probe(struct platform_device *pdev)
/* prepare the async subdev notifier and register it */
imxmd->subdev_notifier.subdevs = imxmd->async_ptrs;
- imxmd->subdev_notifier.bound = imx_media_subdev_bound;
- imxmd->subdev_notifier.complete = imx_media_probe_complete;
+ imxmd->subdev_notifier.ops = &imx_media_subdev_ops;
ret = v4l2_async_notifier_register(&imxmd->v4l2_dev,
&imxmd->subdev_notifier);
if (ret) {
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 96fa1afc00dd..3c48f8b66d12 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -18,6 +18,7 @@ struct device;
struct device_node;
struct v4l2_device;
struct v4l2_subdev;
+struct v4l2_async_notifier;
/* A random max subdevice number, used to allocate an array on stack */
#define V4L2_MAX_SUBDEVS 128U
@@ -79,8 +80,25 @@ struct v4l2_async_subdev {
};
/**
+ * struct v4l2_async_notifier_operations - Asynchronous V4L2 notifier operations
+ * @bound: a subdevice driver has successfully probed one of the subdevices
+ * @complete: all subdevices have been probed successfully
+ * @unbind: a subdevice is leaving
+ */
+struct v4l2_async_notifier_operations {
+ int (*bound)(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd);
+ int (*complete)(struct v4l2_async_notifier *notifier);
+ void (*unbind)(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd);
+};
+
+/**
* struct v4l2_async_notifier - v4l2_device notifier data
*
+ * @ops: notifier operations
* @num_subdevs: number of subdevices used in the subdevs array
* @max_subdevs: number of subdevices allocated in the subdevs array
* @subdevs: array of pointers to subdevice descriptors
@@ -88,11 +106,9 @@ struct v4l2_async_subdev {
* @waiting: list of struct v4l2_async_subdev, waiting for their drivers
* @done: list of struct v4l2_subdev, already probed
* @list: member in a global list of notifiers
- * @bound: a subdevice driver has successfully probed one of subdevices
- * @complete: all subdevices have been probed successfully
- * @unbind: a subdevice is leaving
*/
struct v4l2_async_notifier {
+ const struct v4l2_async_notifier_operations *ops;
unsigned int num_subdevs;
unsigned int max_subdevs;
struct v4l2_async_subdev **subdevs;
@@ -100,13 +116,6 @@ struct v4l2_async_notifier {
struct list_head waiting;
struct list_head done;
struct list_head list;
- int (*bound)(struct v4l2_async_notifier *notifier,
- struct v4l2_subdev *subdev,
- struct v4l2_async_subdev *asd);
- int (*complete)(struct v4l2_async_notifier *notifier);
- void (*unbind)(struct v4l2_async_notifier *notifier,
- struct v4l2_subdev *subdev,
- struct v4l2_async_subdev *asd);
};
/**
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure
2017-09-03 17:49 ` [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure Sakari Ailus
@ 2017-09-04 13:48 ` Hans Verkuil
0 siblings, 0 replies; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 13:48 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The async subdev notifier .bound(), .unbind() and .complete() operations
> are function pointers stored directly in the v4l2_async_subdev
> structure. As the structure isn't immutable, this creates a potential
> security risk as the function pointers are mutable.
>
> To fix this, move the function pointers to a new
> v4l2_async_subdev_operations structure that can be made const in
> drivers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> ---
> drivers/media/platform/am437x/am437x-vpfe.c | 8 +++++--
> drivers/media/platform/atmel/atmel-isc.c | 10 ++++++---
> drivers/media/platform/atmel/atmel-isi.c | 10 ++++++---
> drivers/media/platform/davinci/vpif_capture.c | 8 +++++--
> drivers/media/platform/davinci/vpif_display.c | 8 +++++--
> drivers/media/platform/exynos4-is/media-dev.c | 8 +++++--
> drivers/media/platform/omap3isp/isp.c | 6 +++++-
> drivers/media/platform/pxa_camera.c | 8 +++++--
> drivers/media/platform/qcom/camss-8x16/camss.c | 8 +++++--
> drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++---
> drivers/media/platform/rcar_drif.c | 10 ++++++---
> drivers/media/platform/soc_camera/soc_camera.c | 14 +++++++------
> drivers/media/platform/stm32/stm32-dcmi.c | 10 ++++++---
> drivers/media/platform/ti-vpe/cal.c | 8 +++++--
> drivers/media/platform/xilinx/xilinx-vipp.c | 8 +++++--
> drivers/media/v4l2-core/v4l2-async.c | 20 +++++++++---------
> drivers/staging/media/imx/imx-media-dev.c | 8 +++++--
> include/media/v4l2-async.h | 29 +++++++++++++++++---------
> 18 files changed, 131 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index dfcc484cab89..0997c640191d 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2417,6 +2417,11 @@ static int vpfe_async_complete(struct v4l2_async_notifier *notifier)
> return vpfe_probe_complete(vpfe);
> }
>
> +static const struct v4l2_async_notifier_operations vpfe_async_ops = {
> + .bound = vpfe_async_bound,
> + .complete = vpfe_async_complete,
> +};
> +
> static struct vpfe_config *
> vpfe_get_pdata(struct platform_device *pdev)
> {
> @@ -2590,8 +2595,7 @@ static int vpfe_probe(struct platform_device *pdev)
>
> vpfe->notifier.subdevs = vpfe->cfg->asd;
> vpfe->notifier.num_subdevs = ARRAY_SIZE(vpfe->cfg->asd);
> - vpfe->notifier.bound = vpfe_async_bound;
> - vpfe->notifier.complete = vpfe_async_complete;
> + vpfe->notifier.ops = &vpfe_async_ops;
> ret = v4l2_async_notifier_register(&vpfe->v4l2_dev,
> &vpfe->notifier);
> if (ret) {
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index d7103c5f92c3..48544c4137cb 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -1639,6 +1639,12 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
> return 0;
> }
>
> +static const struct v4l2_async_notifier_operations isc_async_ops = {
> + .bound = isc_async_bound,
> + .unbind = isc_async_unbind,
> + .complete = isc_async_complete,
> +};
> +
> static void isc_subdev_cleanup(struct isc_device *isc)
> {
> struct isc_subdev_entity *subdev_entity;
> @@ -1851,9 +1857,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
> list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
> subdev_entity->notifier.subdevs = &subdev_entity->asd;
> subdev_entity->notifier.num_subdevs = 1;
> - subdev_entity->notifier.bound = isc_async_bound;
> - subdev_entity->notifier.unbind = isc_async_unbind;
> - subdev_entity->notifier.complete = isc_async_complete;
> + subdev_entity->notifier.ops = &isc_async_ops;
>
> ret = v4l2_async_notifier_register(&isc->v4l2_dev,
> &subdev_entity->notifier);
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 891fa2505efa..eadbf9def358 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1105,6 +1105,12 @@ static int isi_graph_notify_bound(struct v4l2_async_notifier *notifier,
> return 0;
> }
>
> +static const struct v4l2_async_notifier_operations isi_graph_notify_ops = {
> + .bound = isi_graph_notify_bound,
> + .unbind = isi_graph_notify_unbind,
> + .complete = isi_graph_notify_complete,
> +};
> +
> static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> {
> struct device_node *ep = NULL;
> @@ -1152,9 +1158,7 @@ static int isi_graph_init(struct atmel_isi *isi)
>
> isi->notifier.subdevs = subdevs;
> isi->notifier.num_subdevs = 1;
> - isi->notifier.bound = isi_graph_notify_bound;
> - isi->notifier.unbind = isi_graph_notify_unbind;
> - isi->notifier.complete = isi_graph_notify_complete;
> + isi->notifier.ops = &isi_graph_notify_ops;
>
> ret = v4l2_async_notifier_register(&isi->v4l2_dev, &isi->notifier);
> if (ret < 0) {
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 0ef36cec21d1..a89367ab1e06 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1500,6 +1500,11 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
> return vpif_probe_complete();
> }
>
> +static const struct v4l2_async_notifier_operations vpif_async_ops = {
> + .bound = vpif_async_bound,
> + .complete = vpif_async_complete,
> +};
> +
> static struct vpif_capture_config *
> vpif_capture_get_pdata(struct platform_device *pdev)
> {
> @@ -1691,8 +1696,7 @@ static __init int vpif_probe(struct platform_device *pdev)
> } else {
> vpif_obj.notifier.subdevs = vpif_obj.config->asd;
> vpif_obj.notifier.num_subdevs = vpif_obj.config->asd_sizes[0];
> - vpif_obj.notifier.bound = vpif_async_bound;
> - vpif_obj.notifier.complete = vpif_async_complete;
> + vpif_obj.notifier.ops = &vpif_async_ops;
> err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
> &vpif_obj.notifier);
> if (err) {
> diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
> index 56fe4e5b396e..ff2f75a328c9 100644
> --- a/drivers/media/platform/davinci/vpif_display.c
> +++ b/drivers/media/platform/davinci/vpif_display.c
> @@ -1232,6 +1232,11 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
> return vpif_probe_complete();
> }
>
> +static const struct v4l2_async_notifier_operations vpif_async_ops = {
> + .bound = vpif_async_bound,
> + .complete = vpif_async_complete,
> +};
> +
> /*
> * vpif_probe: This function creates device entries by register itself to the
> * V4L2 driver and initializes fields of each channel objects
> @@ -1313,8 +1318,7 @@ static __init int vpif_probe(struct platform_device *pdev)
> } else {
> vpif_obj.notifier.subdevs = vpif_obj.config->asd;
> vpif_obj.notifier.num_subdevs = vpif_obj.config->asd_sizes[0];
> - vpif_obj.notifier.bound = vpif_async_bound;
> - vpif_obj.notifier.complete = vpif_async_complete;
> + vpif_obj.notifier.ops = &vpif_async_ops;
> err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
> &vpif_obj.notifier);
> if (err) {
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index d4656d5175d7..c15596b56dc9 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -1405,6 +1405,11 @@ static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> return media_device_register(&fmd->media_dev);
> }
>
> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> + .bound = subdev_notifier_bound,
> + .complete = subdev_notifier_complete,
> +};
> +
> static int fimc_md_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1479,8 +1484,7 @@ static int fimc_md_probe(struct platform_device *pdev)
> if (fmd->num_sensors > 0) {
> fmd->subdev_notifier.subdevs = fmd->async_subdevs;
> fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
> - fmd->subdev_notifier.bound = subdev_notifier_bound;
> - fmd->subdev_notifier.complete = subdev_notifier_complete;
> + fmd->subdev_notifier.ops = &subdev_notifier_ops;
> fmd->num_sensors = 0;
>
> ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 9a694924e46e..0f08d602f756 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2171,6 +2171,10 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> return media_device_register(&isp->media_dev);
> }
>
> +static const struct v4l2_async_notifier_operations isp_subdev_notifier_ops = {
> + .complete = isp_subdev_notifier_complete,
> +};
> +
> /*
> * isp_probe - Probe ISP platform device
> * @pdev: Pointer to ISP platform device
> @@ -2341,7 +2345,7 @@ static int isp_probe(struct platform_device *pdev)
> if (ret < 0)
> goto error_register_entities;
>
> - isp->notifier.complete = isp_subdev_notifier_complete;
> + isp->notifier.ops = &isp_subdev_notifier_ops;
>
> ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
> if (ret)
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index edca993c2b1f..9d3f0cb1d95a 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2221,6 +2221,11 @@ static void pxa_camera_sensor_unbind(struct v4l2_async_notifier *notifier,
> mutex_unlock(&pcdev->mlock);
> }
>
> +static const struct v4l2_async_notifier_operations pxa_camera_sensor_ops = {
> + .bound = pxa_camera_sensor_bound,
> + .unbind = pxa_camera_sensor_unbind,
> +};
> +
> /*
> * Driver probe, remove, suspend and resume operations
> */
> @@ -2489,8 +2494,7 @@ static int pxa_camera_probe(struct platform_device *pdev)
> pcdev->asds[0] = &pcdev->asd;
> pcdev->notifier.subdevs = pcdev->asds;
> pcdev->notifier.num_subdevs = 1;
> - pcdev->notifier.bound = pxa_camera_sensor_bound;
> - pcdev->notifier.unbind = pxa_camera_sensor_unbind;
> + pcdev->notifier.ops = &pxa_camera_sensor_ops;
>
> if (!of_have_populated_dt())
> pcdev->asd.match_type = V4L2_ASYNC_MATCH_I2C;
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> index a3760b5dd1d1..390a42c17b66 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> @@ -601,6 +601,11 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
> return media_device_register(&camss->media_dev);
> }
>
> +static const struct v4l2_async_notifier_operations camss_subdev_notifier_ops = {
> + .bound = camss_subdev_notifier_bound,
> + .complete = camss_subdev_notifier_complete,
> +};
> +
> static const struct media_device_ops camss_media_ops = {
> .link_notify = v4l2_pipeline_link_notify,
> };
> @@ -655,8 +660,7 @@ static int camss_probe(struct platform_device *pdev)
> goto err_register_entities;
>
> if (camss->notifier.num_subdevs) {
> - camss->notifier.bound = camss_subdev_notifier_bound;
> - camss->notifier.complete = camss_subdev_notifier_complete;
> + camss->notifier.ops = &camss_subdev_notifier_ops;
>
> ret = v4l2_async_notifier_register(&camss->v4l2_dev,
> &camss->notifier);
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index bd551f0be213..f55b0ef29a56 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -134,6 +134,12 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>
> return 0;
> }
> +static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
> + .bound = rvin_digital_notify_bound,
> + .unbind = rvin_digital_notify_unbind,
> + .complete = rvin_digital_notify_complete,
> +};
> +
>
> static int rvin_digital_parse_v4l2(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> @@ -182,9 +188,7 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> to_of_node(
> vin->notifier.subdevs[0]->match.fwnode.fwnode));
>
> - vin->notifier.bound = rvin_digital_notify_bound;
> - vin->notifier.unbind = rvin_digital_notify_unbind;
> - vin->notifier.complete = rvin_digital_notify_complete;
> + vin->notifier.ops = &rvin_digital_notify_ops;
> ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> if (ret < 0) {
> vin_err(vin, "Notifier registration failed\n");
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 522364ff0d5d..0b2214d6d621 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1185,6 +1185,12 @@ static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
> return ret;
> }
>
> +static const struct v4l2_async_notifier_operations rcar_drif_notify_ops = {
> + .bound = rcar_drif_notify_bound,
> + .unbind = rcar_drif_notify_unbind,
> + .complete = rcar_drif_notify_complete,
> +};
> +
> /* Read endpoint properties */
> static void rcar_drif_get_ep_properties(struct rcar_drif_sdr *sdr,
> struct fwnode_handle *fwnode)
> @@ -1347,9 +1353,7 @@ static int rcar_drif_sdr_probe(struct rcar_drif_sdr *sdr)
> if (ret)
> goto error;
>
> - sdr->notifier.bound = rcar_drif_notify_bound;
> - sdr->notifier.unbind = rcar_drif_notify_unbind;
> - sdr->notifier.complete = rcar_drif_notify_complete;
> + sdr->notifier.ops = &rcar_drif_notify_ops;
>
> /* Register notifier */
> ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index 1f3c450c7a69..916ff68b73d4 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1391,6 +1391,12 @@ static int soc_camera_async_complete(struct v4l2_async_notifier *notifier)
> return 0;
> }
>
> +static const struct v4l2_async_notifier_operations soc_camera_async_ops = {
> + .bound = soc_camera_async_bound,
> + .unbind = soc_camera_async_unbind,
> + .complete = soc_camera_async_complete,
> +};
> +
> static int scan_async_group(struct soc_camera_host *ici,
> struct v4l2_async_subdev **asd, unsigned int size)
> {
> @@ -1437,9 +1443,7 @@ static int scan_async_group(struct soc_camera_host *ici,
>
> sasc->notifier.subdevs = asd;
> sasc->notifier.num_subdevs = size;
> - sasc->notifier.bound = soc_camera_async_bound;
> - sasc->notifier.unbind = soc_camera_async_unbind;
> - sasc->notifier.complete = soc_camera_async_complete;
> + sasc->notifier.ops = &soc_camera_async_ops;
>
> icd->sasc = sasc;
> icd->parent = ici->v4l2_dev.dev;
> @@ -1537,9 +1541,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>
> sasc->notifier.subdevs = &info->subdev;
> sasc->notifier.num_subdevs = 1;
> - sasc->notifier.bound = soc_camera_async_bound;
> - sasc->notifier.unbind = soc_camera_async_unbind;
> - sasc->notifier.complete = soc_camera_async_complete;
> + sasc->notifier.ops = &soc_camera_async_ops;
>
> icd->sasc = sasc;
> icd->parent = ici->v4l2_dev.dev;
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 35ba6f211b79..ac4c450a6c7d 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1495,6 +1495,12 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
> return 0;
> }
>
> +static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
> + .bound = dcmi_graph_notify_bound,
> + .unbind = dcmi_graph_notify_unbind,
> + .complete = dcmi_graph_notify_complete,
> +};
> +
> static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> {
> struct device_node *ep = NULL;
> @@ -1542,9 +1548,7 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>
> dcmi->notifier.subdevs = subdevs;
> dcmi->notifier.num_subdevs = 1;
> - dcmi->notifier.bound = dcmi_graph_notify_bound;
> - dcmi->notifier.unbind = dcmi_graph_notify_unbind;
> - dcmi->notifier.complete = dcmi_graph_notify_complete;
> + dcmi->notifier.ops = &dcmi_graph_notify_ops;
>
> ret = v4l2_async_notifier_register(&dcmi->v4l2_dev, &dcmi->notifier);
> if (ret < 0) {
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 42e383a48ffe..8b586c864524 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1522,6 +1522,11 @@ static int cal_async_complete(struct v4l2_async_notifier *notifier)
> return 0;
> }
>
> +static const struct v4l2_async_notifier_operations cal_async_ops = {
> + .bound = cal_async_bound,
> + .complete = cal_async_complete,
> +};
> +
> static int cal_complete_ctx(struct cal_ctx *ctx)
> {
> struct video_device *vfd;
> @@ -1736,8 +1741,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> ctx->asd_list[0] = asd;
> ctx->notifier.subdevs = ctx->asd_list;
> ctx->notifier.num_subdevs = 1;
> - ctx->notifier.bound = cal_async_bound;
> - ctx->notifier.complete = cal_async_complete;
> + ctx->notifier.ops = &cal_async_ops;
> ret = v4l2_async_notifier_register(&ctx->v4l2_dev,
> &ctx->notifier);
> if (ret) {
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index ebfdf334d99c..d881cf09876d 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -351,6 +351,11 @@ static int xvip_graph_notify_bound(struct v4l2_async_notifier *notifier,
> return -EINVAL;
> }
>
> +static const struct v4l2_async_notifier_operations xvip_graph_notify_ops = {
> + .bound = xvip_graph_notify_bound,
> + .complete = xvip_graph_notify_complete,
> +};
> +
> static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> struct device_node *node)
> {
> @@ -548,8 +553,7 @@ static int xvip_graph_init(struct xvip_composite_device *xdev)
>
> xdev->notifier.subdevs = subdevs;
> xdev->notifier.num_subdevs = num_subdevs;
> - xdev->notifier.bound = xvip_graph_notify_bound;
> - xdev->notifier.complete = xvip_graph_notify_complete;
> + xdev->notifier.ops = &xvip_graph_notify_ops;
>
> ret = v4l2_async_notifier_register(&xdev->v4l2_dev, &xdev->notifier);
> if (ret < 0) {
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 6740a241d4a1..810f5e0273dc 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -107,16 +107,16 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> {
> int ret;
>
> - if (notifier->bound) {
> - ret = notifier->bound(notifier, sd, asd);
> + if (notifier->ops->bound) {
> + ret = notifier->ops->bound(notifier, sd, asd);
> if (ret < 0)
> return ret;
> }
>
> ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> if (ret < 0) {
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, asd);
> + if (notifier->ops->unbind)
> + notifier->ops->unbind(notifier, sd, asd);
> return ret;
> }
>
> @@ -128,8 +128,8 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> /* Move from the global subdevice list to notifier's done */
> list_move(&sd->async_list, ¬ifier->done);
>
> - if (list_empty(¬ifier->waiting) && notifier->complete)
> - return notifier->complete(notifier);
> + if (list_empty(¬ifier->waiting) && notifier->ops->complete)
> + return notifier->ops->complete(notifier);
>
> return 0;
> }
> @@ -232,8 +232,8 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> /* If we handled USB devices, we'd have to lock the parent too */
> device_release_driver(d);
>
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, sd->asd);
> + if (notifier->ops->unbind)
> + notifier->ops->unbind(notifier, sd, sd->asd);
>
> /*
> * Store device at the device cache, in order to call
> @@ -344,8 +344,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>
> v4l2_async_cleanup(sd);
>
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, sd->asd);
> + if (notifier->ops->unbind)
> + notifier->ops->unbind(notifier, sd, sd->asd);
>
> mutex_unlock(&list_lock);
> }
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index d96f4512224f..e8dc2c93658a 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -440,6 +440,11 @@ static int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
> return media_device_register(&imxmd->md);
> }
>
> +static const struct v4l2_async_notifier_operations imx_media_subdev_ops = {
> + .bound = imx_media_subdev_bound,
> + .complete = imx_media_probe_complete,
> +};
> +
> /*
> * adds controls to a video device from an entity subdevice.
> * Continues upstream from the entity's sink pads.
> @@ -608,8 +613,7 @@ static int imx_media_probe(struct platform_device *pdev)
>
> /* prepare the async subdev notifier and register it */
> imxmd->subdev_notifier.subdevs = imxmd->async_ptrs;
> - imxmd->subdev_notifier.bound = imx_media_subdev_bound;
> - imxmd->subdev_notifier.complete = imx_media_probe_complete;
> + imxmd->subdev_notifier.ops = &imx_media_subdev_ops;
> ret = v4l2_async_notifier_register(&imxmd->v4l2_dev,
> &imxmd->subdev_notifier);
> if (ret) {
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 96fa1afc00dd..3c48f8b66d12 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -18,6 +18,7 @@ struct device;
> struct device_node;
> struct v4l2_device;
> struct v4l2_subdev;
> +struct v4l2_async_notifier;
>
> /* A random max subdevice number, used to allocate an array on stack */
> #define V4L2_MAX_SUBDEVS 128U
> @@ -79,8 +80,25 @@ struct v4l2_async_subdev {
> };
>
> /**
> + * struct v4l2_async_notifier_operations - Asynchronous V4L2 notifier operations
> + * @bound: a subdevice driver has successfully probed one of the subdevices
> + * @complete: all subdevices have been probed successfully
> + * @unbind: a subdevice is leaving
> + */
> +struct v4l2_async_notifier_operations {
> + int (*bound)(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd);
> + int (*complete)(struct v4l2_async_notifier *notifier);
> + void (*unbind)(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd);
> +};
> +
> +/**
> * struct v4l2_async_notifier - v4l2_device notifier data
> *
> + * @ops: notifier operations
> * @num_subdevs: number of subdevices used in the subdevs array
> * @max_subdevs: number of subdevices allocated in the subdevs array
> * @subdevs: array of pointers to subdevice descriptors
> @@ -88,11 +106,9 @@ struct v4l2_async_subdev {
> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> * @done: list of struct v4l2_subdev, already probed
> * @list: member in a global list of notifiers
> - * @bound: a subdevice driver has successfully probed one of subdevices
> - * @complete: all subdevices have been probed successfully
> - * @unbind: a subdevice is leaving
> */
> struct v4l2_async_notifier {
> + const struct v4l2_async_notifier_operations *ops;
> unsigned int num_subdevs;
> unsigned int max_subdevs;
> struct v4l2_async_subdev **subdevs;
> @@ -100,13 +116,6 @@ struct v4l2_async_notifier {
> struct list_head waiting;
> struct list_head done;
> struct list_head list;
> - int (*bound)(struct v4l2_async_notifier *notifier,
> - struct v4l2_subdev *subdev,
> - struct v4l2_async_subdev *asd);
> - int (*complete)(struct v4l2_async_notifier *notifier);
> - void (*unbind)(struct v4l2_async_notifier *notifier,
> - struct v4l2_subdev *subdev,
> - struct v4l2_async_subdev *asd);
> };
>
> /**
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (8 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 13:50 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 11/18] v4l: async: Register sub-devices before calling bound callback Sakari Ailus
` (8 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Add two macros to call async operations callbacks. Besides simplifying
callbacks, this allows async notifiers to have no ops set, i.e. it can be
left NULL.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 19 +++++++------------
include/media/v4l2-async.h | 8 ++++++++
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 810f5e0273dc..91d04f00b4e4 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -107,16 +107,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
{
int ret;
- if (notifier->ops->bound) {
- ret = notifier->ops->bound(notifier, sd, asd);
- if (ret < 0)
- return ret;
- }
+ ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
+ if (ret < 0)
+ return ret;
ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
if (ret < 0) {
- if (notifier->ops->unbind)
- notifier->ops->unbind(notifier, sd, asd);
+ v4l2_async_notifier_call_void_op(notifier, unbind, sd, asd);
return ret;
}
@@ -129,7 +126,7 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
list_move(&sd->async_list, ¬ifier->done);
if (list_empty(¬ifier->waiting) && notifier->ops->complete)
- return notifier->ops->complete(notifier);
+ return v4l2_async_notifier_call_int_op(notifier, complete);
return 0;
}
@@ -232,8 +229,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
/* If we handled USB devices, we'd have to lock the parent too */
device_release_driver(d);
- if (notifier->ops->unbind)
- notifier->ops->unbind(notifier, sd, sd->asd);
+ v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
/*
* Store device at the device cache, in order to call
@@ -344,8 +340,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
v4l2_async_cleanup(sd);
- if (notifier->ops->unbind)
- notifier->ops->unbind(notifier, sd, sd->asd);
+ v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
mutex_unlock(&list_lock);
}
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 3c48f8b66d12..c3e001e0d1f1 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -95,6 +95,14 @@ struct v4l2_async_notifier_operations {
struct v4l2_async_subdev *asd);
};
+#define v4l2_async_notifier_call_int_op(n, op, ...) \
+ (((n)->ops && (n)->ops->op) ? (n)->ops->op(n, ## __VA_ARGS__) : 0)
+#define v4l2_async_notifier_call_void_op(n, op, ...) \
+ do { \
+ if ((n)->ops && (n)->ops->op) \
+ (n)->ops->op(n, ## __VA_ARGS__); \
+ } while (false)
+
/**
* struct v4l2_async_notifier - v4l2_device notifier data
*
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks
2017-09-03 17:49 ` [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks Sakari Ailus
@ 2017-09-04 13:50 ` Hans Verkuil
2017-09-04 16:09 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 13:50 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add two macros to call async operations callbacks. Besides simplifying
> callbacks, this allows async notifiers to have no ops set, i.e. it can be
> left NULL.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 19 +++++++------------
> include/media/v4l2-async.h | 8 ++++++++
> 2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 810f5e0273dc..91d04f00b4e4 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -107,16 +107,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> {
> int ret;
>
> - if (notifier->ops->bound) {
> - ret = notifier->ops->bound(notifier, sd, asd);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
Hmm, I think this is rather ugly. We only have three ops, so why not make
three macros:
v4l2_async_notifier_call_bound/unbind/complete?
Much cleaner than _int_op(...bound...).
Regards,
Hans
> + if (ret < 0)
> + return ret;
>
> ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> if (ret < 0) {
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, asd);
> return ret;
> }
>
> @@ -129,7 +126,7 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> list_move(&sd->async_list, ¬ifier->done);
>
> if (list_empty(¬ifier->waiting) && notifier->ops->complete)
> - return notifier->ops->complete(notifier);
> + return v4l2_async_notifier_call_int_op(notifier, complete);
>
> return 0;
> }
> @@ -232,8 +229,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> /* If we handled USB devices, we'd have to lock the parent too */
> device_release_driver(d);
>
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
>
> /*
> * Store device at the device cache, in order to call
> @@ -344,8 +340,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>
> v4l2_async_cleanup(sd);
>
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
>
> mutex_unlock(&list_lock);
> }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 3c48f8b66d12..c3e001e0d1f1 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -95,6 +95,14 @@ struct v4l2_async_notifier_operations {
> struct v4l2_async_subdev *asd);
> };
>
> +#define v4l2_async_notifier_call_int_op(n, op, ...) \
> + (((n)->ops && (n)->ops->op) ? (n)->ops->op(n, ## __VA_ARGS__) : 0)
> +#define v4l2_async_notifier_call_void_op(n, op, ...) \
> + do { \
> + if ((n)->ops && (n)->ops->op) \
> + (n)->ops->op(n, ## __VA_ARGS__); \
> + } while (false)
> +
> /**
> * struct v4l2_async_notifier - v4l2_device notifier data
> *
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks
2017-09-04 13:50 ` Hans Verkuil
@ 2017-09-04 16:09 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:09 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
Hi Hans,
On Mon, Sep 04, 2017 at 03:50:52PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Add two macros to call async operations callbacks. Besides simplifying
> > callbacks, this allows async notifiers to have no ops set, i.e. it can be
> > left NULL.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 19 +++++++------------
> > include/media/v4l2-async.h | 8 ++++++++
> > 2 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 810f5e0273dc..91d04f00b4e4 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -107,16 +107,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> > {
> > int ret;
> >
> > - if (notifier->ops->bound) {
> > - ret = notifier->ops->bound(notifier, sd, asd);
> > - if (ret < 0)
> > - return ret;
> > - }
> > + ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
>
> Hmm, I think this is rather ugly. We only have three ops, so why not make
> three macros:
>
> v4l2_async_notifier_call_bound/unbind/complete?
>
> Much cleaner than _int_op(...bound...).
Works for me.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 11/18] v4l: async: Register sub-devices before calling bound callback
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (9 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-08 12:28 ` Pavel Machek
2017-09-03 17:49 ` [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices Sakari Ailus
` (7 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Register the sub-device before calling the notifier's bound callback.
Doing this the other way around is problematic as the struct v4l2_device
has not assigned for the sub-device yet and may be required by the bound
callback.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 91d04f00b4e4..70d02378b48f 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -107,13 +107,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
{
int ret;
- ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
+ ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
if (ret < 0)
return ret;
- ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
+ ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
if (ret < 0) {
- v4l2_async_notifier_call_void_op(notifier, unbind, sd, asd);
+ v4l2_device_unregister_subdev(sd);
return ret;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 11/18] v4l: async: Register sub-devices before calling bound callback
2017-09-03 17:49 ` [PATCH v7 11/18] v4l: async: Register sub-devices before calling bound callback Sakari Ailus
@ 2017-09-08 12:28 ` Pavel Machek
0 siblings, 0 replies; 46+ messages in thread
From: Pavel Machek @ 2017-09-08 12:28 UTC (permalink / raw)
To: Sakari Ailus, Hans Verkuil, Sakari Ailus
Cc: linux-media, niklas.soderlund, robh, laurent.pinchart, devicetree, sre
[-- Attachment #1: Type: text/plain, Size: 537 bytes --]
On Sun 2017-09-03 20:49:51, Sakari Ailus wrote:
> Register the sub-device before calling the notifier's bound callback.
> Doing this the other way around is problematic as the struct v4l2_device
> has not assigned for the sub-device yet and may be required by the bound
> callback.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (10 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 11/18] v4l: async: Register sub-devices before calling bound callback Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 14:56 ` Hans Verkuil
2017-09-05 6:49 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor Sakari Ailus
` (6 subsequent siblings)
18 siblings, 2 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Registering a notifier has required the knowledge of struct v4l2_device
for the reason that sub-devices generally are registered to the
v4l2_device (as well as the media device, also available through
v4l2_device).
This information is not available for sub-device drivers at probe time.
What this patch does is that it allows registering notifiers without
having v4l2_device around. Instead the sub-device pointer is stored to the
notifier. Once the sub-device of the driver that registered the notifier
is registered, the notifier will gain the knowledge of the v4l2_device,
and the binding of async sub-devices from the sub-device driver's notifier
may proceed.
The master notifier's complete callback is only called when all sub-device
notifiers are completed.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
include/media/v4l2-async.h | 19 ++++-
2 files changed, 146 insertions(+), 26 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 70d02378b48f..55d7886103d2 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -25,6 +25,10 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
+static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *sd,
+ struct v4l2_async_subdev *asd);
+
static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
{
#if IS_ENABLED(CONFIG_I2C)
@@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
return NULL;
}
+static bool v4l2_async_subdev_notifiers_complete(
+ struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_async_notifier *n;
+
+ list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
+ if (!n->master)
+ return false;
+ }
+
+ return true;
+}
+
+#define notifier_v4l2_dev(n) \
+ (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
+ !!(n)->master ? (n)->master->v4l2_dev : NULL)
+
+static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
+ struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
+{
+ struct v4l2_async_notifier *n;
+
+ list_for_each_entry(n, ¬ifier_list, list) {
+ if (n->sd == sd)
+ return n;
+ }
+
+ return NULL;
+}
+
+static int v4l2_async_notifier_test_all_subdevs(
+ struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_subdev *sd, *tmp;
+
+ if (!notifier_v4l2_dev(notifier))
+ return 0;
+
+ list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
+ struct v4l2_async_subdev *asd;
+ int ret;
+
+ asd = v4l2_async_belongs(notifier, sd);
+ if (!asd)
+ continue;
+
+ ret = v4l2_async_test_notify(notifier, sd, asd);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
struct v4l2_subdev *sd,
struct v4l2_async_subdev *asd)
{
+ struct v4l2_async_notifier *subdev_notifier;
int ret;
- ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
- if (ret < 0)
+ ret = v4l2_device_register_subdev(notifier_v4l2_dev(notifier), sd);
+ if (ret)
return ret;
ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
@@ -125,8 +184,26 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
/* Move from the global subdevice list to notifier's done */
list_move(&sd->async_list, ¬ifier->done);
- if (list_empty(¬ifier->waiting) && notifier->ops->complete)
- return v4l2_async_notifier_call_int_op(notifier, complete);
+ subdev_notifier = v4l2_async_get_subdev_notifier(notifier, sd);
+ if (subdev_notifier && !subdev_notifier->master) {
+ subdev_notifier->master = notifier;
+ list_add(&subdev_notifier->notifiers, ¬ifier->notifiers);
+ ret = v4l2_async_notifier_test_all_subdevs(subdev_notifier);
+ if (ret)
+ return ret;
+ }
+
+ if (list_empty(¬ifier->waiting) &&
+ v4l2_async_subdev_notifiers_complete(notifier)) {
+ ret = v4l2_async_notifier_call_int_op(notifier, complete);
+ if (ret)
+ return ret;
+ }
+
+ if (notifier->master && list_empty(¬ifier->master->waiting) &&
+ v4l2_async_subdev_notifiers_complete(notifier->master))
+ return v4l2_async_notifier_call_int_op(notifier->master,
+ complete);
return 0;
}
@@ -140,18 +217,17 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
sd->dev = NULL;
}
-int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
- struct v4l2_async_notifier *notifier)
+static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
{
- struct v4l2_subdev *sd, *tmp;
struct v4l2_async_subdev *asd;
+ int ret;
int i;
- if (!v4l2_dev || !notifier->num_subdevs ||
+ if (!!notifier->v4l2_dev == !!notifier->sd || !notifier->num_subdevs ||
notifier->num_subdevs > V4L2_MAX_SUBDEVS)
return -EINVAL;
- notifier->v4l2_dev = v4l2_dev;
+ INIT_LIST_HEAD(¬ifier->notifiers);
INIT_LIST_HEAD(¬ifier->waiting);
INIT_LIST_HEAD(¬ifier->done);
@@ -175,18 +251,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
mutex_lock(&list_lock);
- list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
- int ret;
-
- asd = v4l2_async_belongs(notifier, sd);
- if (!asd)
- continue;
-
- ret = v4l2_async_test_notify(notifier, sd, asd);
- if (ret < 0) {
- mutex_unlock(&list_lock);
- return ret;
- }
+ ret = v4l2_async_notifier_test_all_subdevs(notifier);
+ if (ret) {
+ mutex_unlock(&list_lock);
+ return ret;
}
/* Keep also completed notifiers on the list */
@@ -196,27 +264,62 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
return 0;
}
+
+int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
+ struct v4l2_async_notifier *notifier)
+{
+ notifier->v4l2_dev = v4l2_dev;
+
+ return __v4l2_async_notifier_register(notifier);
+}
EXPORT_SYMBOL(v4l2_async_notifier_register);
+int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
+ struct v4l2_async_notifier *notifier)
+{
+ notifier->sd = sd;
+
+ return __v4l2_async_notifier_register(notifier);
+}
+EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
+
void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
{
+ struct v4l2_async_notifier *notifier_iter, *notifier_tmp;
struct v4l2_subdev *sd, *tmp;
unsigned int notif_n_subdev = notifier->num_subdevs;
unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
struct device **dev;
int i = 0;
- if (!notifier->v4l2_dev)
+ if (!notifier->v4l2_dev && !notifier->sd)
return;
dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
if (!dev) {
- dev_err(notifier->v4l2_dev->dev,
+ dev_err(notifier->v4l2_dev ?
+ notifier->v4l2_dev->dev : notifier->sd->dev,
"Failed to allocate device cache!\n");
}
mutex_lock(&list_lock);
+ if (notifier->v4l2_dev) {
+ /* Remove all subdev notifiers from the master's list */
+ list_for_each_entry_safe(notifier_iter, notifier_tmp,
+ ¬ifier->notifiers, notifiers) {
+ list_del_init(¬ifier_iter->notifiers);
+ WARN_ON(notifier_iter->master != notifier);
+ notifier_iter->master = NULL;
+ }
+ notifier->v4l2_dev = NULL;
+ } else {
+ /* Remove subdev notifier from the master's list */
+ list_del_init(¬ifier->notifiers);
+ notifier->master = NULL;
+ notifier->sd = NULL;
+ }
+
list_del(¬ifier->list);
list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
@@ -266,8 +369,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
}
kvfree(dev);
- notifier->v4l2_dev = NULL;
-
/*
* Don't care about the waiting list, it is initialised and populated
* upon notifier registration.
@@ -287,6 +388,8 @@ void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
kvfree(notifier->subdevs);
notifier->subdevs = NULL;
+
+ WARN_ON(!list_empty(¬ifier->notifiers));
}
EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index c3e001e0d1f1..a5c123ac2873 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -110,7 +110,11 @@ struct v4l2_async_notifier_operations {
* @num_subdevs: number of subdevices used in the subdevs array
* @max_subdevs: number of subdevices allocated in the subdevs array
* @subdevs: array of pointers to subdevice descriptors
- * @v4l2_dev: pointer to struct v4l2_device
+ * @v4l2_dev: v4l2_device of the master, for subdev notifiers NULL
+ * @sd: sub-device that registered the notifier, NULL otherwise
+ * @notifiers: list of struct v4l2_async_notifier, notifiers linked to this
+ * notifier
+ * @master: master notifier carrying @v4l2_dev
* @waiting: list of struct v4l2_async_subdev, waiting for their drivers
* @done: list of struct v4l2_subdev, already probed
* @list: member in a global list of notifiers
@@ -121,6 +125,9 @@ struct v4l2_async_notifier {
unsigned int max_subdevs;
struct v4l2_async_subdev **subdevs;
struct v4l2_device *v4l2_dev;
+ struct v4l2_subdev *sd;
+ struct list_head notifiers;
+ struct v4l2_async_notifier *master;
struct list_head waiting;
struct list_head done;
struct list_head list;
@@ -136,6 +143,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
struct v4l2_async_notifier *notifier);
/**
+ * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous
+ * notifier for a sub-device
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @notifier: pointer to &struct v4l2_async_notifier
+ */
+int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
+ struct v4l2_async_notifier *notifier);
+
+/**
* v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier
*
* @notifier: pointer to &struct v4l2_async_notifier
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices
2017-09-03 17:49 ` [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices Sakari Ailus
@ 2017-09-04 14:56 ` Hans Verkuil
2017-09-04 16:42 ` Sakari Ailus
2017-09-05 6:49 ` Hans Verkuil
1 sibling, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 14:56 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Registering a notifier has required the knowledge of struct v4l2_device
> for the reason that sub-devices generally are registered to the
> v4l2_device (as well as the media device, also available through
> v4l2_device).
>
> This information is not available for sub-device drivers at probe time.
>
> What this patch does is that it allows registering notifiers without
> having v4l2_device around. Instead the sub-device pointer is stored to the
> notifier. Once the sub-device of the driver that registered the notifier
> is registered, the notifier will gain the knowledge of the v4l2_device,
> and the binding of async sub-devices from the sub-device driver's notifier
> may proceed.
>
> The master notifier's complete callback is only called when all sub-device
> notifiers are completed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
> include/media/v4l2-async.h | 19 ++++-
> 2 files changed, 146 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 70d02378b48f..55d7886103d2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,10 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd);
> +
> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> #if IS_ENABLED(CONFIG_I2C)
> @@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
> return NULL;
> }
>
> +static bool v4l2_async_subdev_notifiers_complete(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
> + if (!n->master)
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#define notifier_v4l2_dev(n) \
> + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
> + !!(n)->master ? (n)->master->v4l2_dev : NULL)
Why '!!'?
> +
> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, ¬ifier_list, list) {
> + if (n->sd == sd)
> + return n;
> + }
> +
> + return NULL;
> +}
> +
> +static int v4l2_async_notifier_test_all_subdevs(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd, *tmp;
> +
> + if (!notifier_v4l2_dev(notifier))
> + return 0;
> +
> + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> + struct v4l2_async_subdev *asd;
> + int ret;
> +
> + asd = v4l2_async_belongs(notifier, sd);
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_test_notify(notifier, sd, asd);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> struct v4l2_subdev *sd,
> struct v4l2_async_subdev *asd)
A general note (not specific to this patch series): I really don't like
this function name. v4l2_async_match_notify is a much better name.
With 'test' I get association with 'testing something' and not that it is
a match.
I have a similar problem with v4l2_async_belongs: v4l2_async_find_match
makes a lot more sense.
> {
> + struct v4l2_async_notifier *subdev_notifier;
> int ret;
>
> - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> - if (ret < 0)
> + ret = v4l2_device_register_subdev(notifier_v4l2_dev(notifier), sd);
> + if (ret)
> return ret;
>
> ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
> @@ -125,8 +184,26 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> /* Move from the global subdevice list to notifier's done */
> list_move(&sd->async_list, ¬ifier->done);
>
> - if (list_empty(¬ifier->waiting) && notifier->ops->complete)
> - return v4l2_async_notifier_call_int_op(notifier, complete);
> + subdev_notifier = v4l2_async_get_subdev_notifier(notifier, sd);
> + if (subdev_notifier && !subdev_notifier->master) {
> + subdev_notifier->master = notifier;
> + list_add(&subdev_notifier->notifiers, ¬ifier->notifiers);
> + ret = v4l2_async_notifier_test_all_subdevs(subdev_notifier);
> + if (ret)
> + return ret;
> + }
> +
> + if (list_empty(¬ifier->waiting) &&
> + v4l2_async_subdev_notifiers_complete(notifier)) {
> + ret = v4l2_async_notifier_call_int_op(notifier, complete);
> + if (ret)
> + return ret;
> + }
> +
> + if (notifier->master && list_empty(¬ifier->master->waiting) &&
> + v4l2_async_subdev_notifiers_complete(notifier->master))
> + return v4l2_async_notifier_call_int_op(notifier->master,
> + complete);
>
> return 0;
> }
> @@ -140,18 +217,17 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> sd->dev = NULL;
> }
>
> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> - struct v4l2_async_notifier *notifier)
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> {
> - struct v4l2_subdev *sd, *tmp;
> struct v4l2_async_subdev *asd;
> + int ret;
> int i;
>
> - if (!v4l2_dev || !notifier->num_subdevs ||
> + if (!!notifier->v4l2_dev == !!notifier->sd || !notifier->num_subdevs ||
'!!notifier->v4l2_dev == !!notifier->sd'???
This is '(notifier->v4l2_dev && notifier->sd) || (!notifier->v4l2_dev && !notifier->sd)'
but it took me a bit of time to parse that.
A 10 for creativity, but not so much for readability :-)
I suspect it can be simplified as well, or some of these tests can be moved to
the two functions that call this one. I think that might be best, in fact.
> notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> return -EINVAL;
>
> - notifier->v4l2_dev = v4l2_dev;
> + INIT_LIST_HEAD(¬ifier->notifiers);
> INIT_LIST_HEAD(¬ifier->waiting);
> INIT_LIST_HEAD(¬ifier->done);
>
> @@ -175,18 +251,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>
> mutex_lock(&list_lock);
>
> - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> - int ret;
> -
> - asd = v4l2_async_belongs(notifier, sd);
> - if (!asd)
> - continue;
> -
> - ret = v4l2_async_test_notify(notifier, sd, asd);
> - if (ret < 0) {
> - mutex_unlock(&list_lock);
> - return ret;
> - }
> + ret = v4l2_async_notifier_test_all_subdevs(notifier);
> + if (ret) {
> + mutex_unlock(&list_lock);
> + return ret;
> }
>
> /* Keep also completed notifiers on the list */
> @@ -196,27 +264,62 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>
> return 0;
> }
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> + struct v4l2_async_notifier *notifier)
> +{
> + notifier->v4l2_dev = v4l2_dev;
> +
> + return __v4l2_async_notifier_register(notifier);
> +}
> EXPORT_SYMBOL(v4l2_async_notifier_register);
>
> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> + struct v4l2_async_notifier *notifier)
> +{
> + notifier->sd = sd;
> +
> + return __v4l2_async_notifier_register(notifier);
> +}
> +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
> +
> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> {
> + struct v4l2_async_notifier *notifier_iter, *notifier_tmp;
> struct v4l2_subdev *sd, *tmp;
> unsigned int notif_n_subdev = notifier->num_subdevs;
> unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> struct device **dev;
> int i = 0;
>
> - if (!notifier->v4l2_dev)
> + if (!notifier->v4l2_dev && !notifier->sd)
> return;
>
> dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> - dev_err(notifier->v4l2_dev->dev,
> + dev_err(notifier->v4l2_dev ?
> + notifier->v4l2_dev->dev : notifier->sd->dev,
> "Failed to allocate device cache!\n");
> }
>
> mutex_lock(&list_lock);
>
> + if (notifier->v4l2_dev) {
> + /* Remove all subdev notifiers from the master's list */
> + list_for_each_entry_safe(notifier_iter, notifier_tmp,
> + ¬ifier->notifiers, notifiers) {
> + list_del_init(¬ifier_iter->notifiers);
> + WARN_ON(notifier_iter->master != notifier);
> + notifier_iter->master = NULL;
> + }
> + notifier->v4l2_dev = NULL;
> + } else {
> + /* Remove subdev notifier from the master's list */
> + list_del_init(¬ifier->notifiers);
> + notifier->master = NULL;
> + notifier->sd = NULL;
> + }
> +
> list_del(¬ifier->list);
>
> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> @@ -266,8 +369,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> }
> kvfree(dev);
>
> - notifier->v4l2_dev = NULL;
> -
> /*
> * Don't care about the waiting list, it is initialised and populated
> * upon notifier registration.
> @@ -287,6 +388,8 @@ void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
>
> kvfree(notifier->subdevs);
> notifier->subdevs = NULL;
> +
> + WARN_ON(!list_empty(¬ifier->notifiers));
> }
> EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index c3e001e0d1f1..a5c123ac2873 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -110,7 +110,11 @@ struct v4l2_async_notifier_operations {
> * @num_subdevs: number of subdevices used in the subdevs array
> * @max_subdevs: number of subdevices allocated in the subdevs array
> * @subdevs: array of pointers to subdevice descriptors
> - * @v4l2_dev: pointer to struct v4l2_device
> + * @v4l2_dev: v4l2_device of the master, for subdev notifiers NULL
> + * @sd: sub-device that registered the notifier, NULL otherwise
> + * @notifiers: list of struct v4l2_async_notifier, notifiers linked to this
> + * notifier
> + * @master: master notifier carrying @v4l2_dev
> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> * @done: list of struct v4l2_subdev, already probed
> * @list: member in a global list of notifiers
> @@ -121,6 +125,9 @@ struct v4l2_async_notifier {
> unsigned int max_subdevs;
> struct v4l2_async_subdev **subdevs;
> struct v4l2_device *v4l2_dev;
> + struct v4l2_subdev *sd;
> + struct list_head notifiers;
> + struct v4l2_async_notifier *master;
> struct list_head waiting;
> struct list_head done;
> struct list_head list;
> @@ -136,6 +143,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> struct v4l2_async_notifier *notifier);
>
> /**
> + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous
> + * notifier for a sub-device
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + * @notifier: pointer to &struct v4l2_async_notifier
> + */
> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> + struct v4l2_async_notifier *notifier);
> +
> +/**
> * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier
> *
> * @notifier: pointer to &struct v4l2_async_notifier
>
Do you have a git tree with this patch series? I think I need to look at this
in the final version, not just the patch.
Regards,
Hans
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices
2017-09-04 14:56 ` Hans Verkuil
@ 2017-09-04 16:42 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:42 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
Hi Hans,
On Mon, Sep 04, 2017 at 04:56:29PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Registering a notifier has required the knowledge of struct v4l2_device
> > for the reason that sub-devices generally are registered to the
> > v4l2_device (as well as the media device, also available through
> > v4l2_device).
> >
> > This information is not available for sub-device drivers at probe time.
> >
> > What this patch does is that it allows registering notifiers without
> > having v4l2_device around. Instead the sub-device pointer is stored to the
> > notifier. Once the sub-device of the driver that registered the notifier
> > is registered, the notifier will gain the knowledge of the v4l2_device,
> > and the binding of async sub-devices from the sub-device driver's notifier
> > may proceed.
> >
> > The master notifier's complete callback is only called when all sub-device
> > notifiers are completed.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
> > include/media/v4l2-async.h | 19 ++++-
> > 2 files changed, 146 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 70d02378b48f..55d7886103d2 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -25,6 +25,10 @@
> > #include <media/v4l2-fwnode.h>
> > #include <media/v4l2-subdev.h>
> >
> > +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *sd,
> > + struct v4l2_async_subdev *asd);
> > +
> > static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > {
> > #if IS_ENABLED(CONFIG_I2C)
> > @@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
> > return NULL;
> > }
> >
> > +static bool v4l2_async_subdev_notifiers_complete(
> > + struct v4l2_async_notifier *notifier)
> > +{
> > + struct v4l2_async_notifier *n;
> > +
> > + list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
> > + if (!n->master)
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +#define notifier_v4l2_dev(n) \
> > + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
> > + !!(n)->master ? (n)->master->v4l2_dev : NULL)
>
> Why '!!'?
I've since replaced this by a function.
My understanding is GCC 7 doesn't like a ? x : y construct where a is a
non-boolean. This will be effectively the same, but a boolean.
See e.g.
commit da48c948c263c9d87dfc64566b3373a858cc8aa2
Author: Arnd Bergmann <arnd@arndb.de>
Date: Wed Jul 19 15:23:27 2017 -0400
media: fix warning on v4l2_subdev_call() result interpreted as bool
v4l2_subdev_call is a macro returning whatever the callback return
type is, usually 'int'. With gcc-7 and ccache, this can lead to
many wanings like:
media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
The problem here is that after preprocessing, we the compiler
sees a variation of
if (a ? 0 : 2)
that it thinks is suspicious.
This replaces the ?: operator with an different expression that
does the same thing in a more easily readable way that cannot
tigger the warning
Link: https://lkml.org/lkml/2017/7/14/156
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> > +
> > +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
> > + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> > +{
> > + struct v4l2_async_notifier *n;
> > +
> > + list_for_each_entry(n, ¬ifier_list, list) {
> > + if (n->sd == sd)
> > + return n;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int v4l2_async_notifier_test_all_subdevs(
> > + struct v4l2_async_notifier *notifier)
> > +{
> > + struct v4l2_subdev *sd, *tmp;
> > +
> > + if (!notifier_v4l2_dev(notifier))
> > + return 0;
> > +
> > + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > + struct v4l2_async_subdev *asd;
> > + int ret;
> > +
> > + asd = v4l2_async_belongs(notifier, sd);
> > + if (!asd)
> > + continue;
> > +
> > + ret = v4l2_async_test_notify(notifier, sd, asd);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> > struct v4l2_subdev *sd,
> > struct v4l2_async_subdev *asd)
>
> A general note (not specific to this patch series): I really don't like
> this function name. v4l2_async_match_notify is a much better name.
>
> With 'test' I get association with 'testing something' and not that it is
> a match.
>
> I have a similar problem with v4l2_async_belongs: v4l2_async_find_match
> makes a lot more sense.
I can prepend the set with a patch renaming them.
>
> > {
> > + struct v4l2_async_notifier *subdev_notifier;
> > int ret;
> >
> > - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> > - if (ret < 0)
> > + ret = v4l2_device_register_subdev(notifier_v4l2_dev(notifier), sd);
> > + if (ret)
> > return ret;
> >
> > ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
> > @@ -125,8 +184,26 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> > /* Move from the global subdevice list to notifier's done */
> > list_move(&sd->async_list, ¬ifier->done);
> >
> > - if (list_empty(¬ifier->waiting) && notifier->ops->complete)
> > - return v4l2_async_notifier_call_int_op(notifier, complete);
> > + subdev_notifier = v4l2_async_get_subdev_notifier(notifier, sd);
> > + if (subdev_notifier && !subdev_notifier->master) {
> > + subdev_notifier->master = notifier;
> > + list_add(&subdev_notifier->notifiers, ¬ifier->notifiers);
> > + ret = v4l2_async_notifier_test_all_subdevs(subdev_notifier);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (list_empty(¬ifier->waiting) &&
> > + v4l2_async_subdev_notifiers_complete(notifier)) {
> > + ret = v4l2_async_notifier_call_int_op(notifier, complete);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (notifier->master && list_empty(¬ifier->master->waiting) &&
> > + v4l2_async_subdev_notifiers_complete(notifier->master))
> > + return v4l2_async_notifier_call_int_op(notifier->master,
> > + complete);
> >
> > return 0;
> > }
> > @@ -140,18 +217,17 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> > sd->dev = NULL;
> > }
> >
> > -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > - struct v4l2_async_notifier *notifier)
> > +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> > {
> > - struct v4l2_subdev *sd, *tmp;
> > struct v4l2_async_subdev *asd;
> > + int ret;
> > int i;
> >
> > - if (!v4l2_dev || !notifier->num_subdevs ||
> > + if (!!notifier->v4l2_dev == !!notifier->sd || !notifier->num_subdevs ||
>
> '!!notifier->v4l2_dev == !!notifier->sd'???
>
> This is '(notifier->v4l2_dev && notifier->sd) || (!notifier->v4l2_dev && !notifier->sd)'
> but it took me a bit of time to parse that.
>
> A 10 for creativity, but not so much for readability :-)
:-D
>
> I suspect it can be simplified as well, or some of these tests can be moved to
> the two functions that call this one. I think that might be best, in fact.
A single ! would be actually enough. What would you think of that? A bit
less too loud? :-) It should be easies to grasp: both cannot be NULL or
non-NULL.
I've moved the check for v4l2_dev or sd to the appropriate functions. Still
it could be worth checking for this just in case.
>
> > notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > return -EINVAL;
> >
> > - notifier->v4l2_dev = v4l2_dev;
> > + INIT_LIST_HEAD(¬ifier->notifiers);
> > INIT_LIST_HEAD(¬ifier->waiting);
> > INIT_LIST_HEAD(¬ifier->done);
> >
> > @@ -175,18 +251,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> >
> > mutex_lock(&list_lock);
> >
> > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > - int ret;
> > -
> > - asd = v4l2_async_belongs(notifier, sd);
> > - if (!asd)
> > - continue;
> > -
> > - ret = v4l2_async_test_notify(notifier, sd, asd);
> > - if (ret < 0) {
> > - mutex_unlock(&list_lock);
> > - return ret;
> > - }
> > + ret = v4l2_async_notifier_test_all_subdevs(notifier);
> > + if (ret) {
> > + mutex_unlock(&list_lock);
> > + return ret;
> > }
> >
> > /* Keep also completed notifiers on the list */
> > @@ -196,27 +264,62 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> >
> > return 0;
> > }
> > +
> > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > + struct v4l2_async_notifier *notifier)
> > +{
> > + notifier->v4l2_dev = v4l2_dev;
> > +
> > + return __v4l2_async_notifier_register(notifier);
> > +}
> > EXPORT_SYMBOL(v4l2_async_notifier_register);
> >
> > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> > + struct v4l2_async_notifier *notifier)
> > +{
> > + notifier->sd = sd;
> > +
> > + return __v4l2_async_notifier_register(notifier);
> > +}
> > +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
> > +
> > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > {
> > + struct v4l2_async_notifier *notifier_iter, *notifier_tmp;
> > struct v4l2_subdev *sd, *tmp;
> > unsigned int notif_n_subdev = notifier->num_subdevs;
> > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> > struct device **dev;
> > int i = 0;
> >
> > - if (!notifier->v4l2_dev)
> > + if (!notifier->v4l2_dev && !notifier->sd)
> > return;
> >
> > dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> > if (!dev) {
> > - dev_err(notifier->v4l2_dev->dev,
> > + dev_err(notifier->v4l2_dev ?
> > + notifier->v4l2_dev->dev : notifier->sd->dev,
> > "Failed to allocate device cache!\n");
> > }
> >
> > mutex_lock(&list_lock);
> >
> > + if (notifier->v4l2_dev) {
> > + /* Remove all subdev notifiers from the master's list */
> > + list_for_each_entry_safe(notifier_iter, notifier_tmp,
> > + ¬ifier->notifiers, notifiers) {
> > + list_del_init(¬ifier_iter->notifiers);
> > + WARN_ON(notifier_iter->master != notifier);
> > + notifier_iter->master = NULL;
> > + }
> > + notifier->v4l2_dev = NULL;
> > + } else {
> > + /* Remove subdev notifier from the master's list */
> > + list_del_init(¬ifier->notifiers);
> > + notifier->master = NULL;
> > + notifier->sd = NULL;
> > + }
> > +
> > list_del(¬ifier->list);
> >
> > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> > @@ -266,8 +369,6 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > }
> > kvfree(dev);
> >
> > - notifier->v4l2_dev = NULL;
> > -
> > /*
> > * Don't care about the waiting list, it is initialised and populated
> > * upon notifier registration.
> > @@ -287,6 +388,8 @@ void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> >
> > kvfree(notifier->subdevs);
> > notifier->subdevs = NULL;
> > +
> > + WARN_ON(!list_empty(¬ifier->notifiers));
> > }
> > EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> >
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index c3e001e0d1f1..a5c123ac2873 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -110,7 +110,11 @@ struct v4l2_async_notifier_operations {
> > * @num_subdevs: number of subdevices used in the subdevs array
> > * @max_subdevs: number of subdevices allocated in the subdevs array
> > * @subdevs: array of pointers to subdevice descriptors
> > - * @v4l2_dev: pointer to struct v4l2_device
> > + * @v4l2_dev: v4l2_device of the master, for subdev notifiers NULL
> > + * @sd: sub-device that registered the notifier, NULL otherwise
> > + * @notifiers: list of struct v4l2_async_notifier, notifiers linked to this
> > + * notifier
> > + * @master: master notifier carrying @v4l2_dev
> > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> > * @done: list of struct v4l2_subdev, already probed
> > * @list: member in a global list of notifiers
> > @@ -121,6 +125,9 @@ struct v4l2_async_notifier {
> > unsigned int max_subdevs;
> > struct v4l2_async_subdev **subdevs;
> > struct v4l2_device *v4l2_dev;
> > + struct v4l2_subdev *sd;
> > + struct list_head notifiers;
> > + struct v4l2_async_notifier *master;
> > struct list_head waiting;
> > struct list_head done;
> > struct list_head list;
> > @@ -136,6 +143,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > struct v4l2_async_notifier *notifier);
> >
> > /**
> > + * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous
> > + * notifier for a sub-device
> > + *
> > + * @sd: pointer to &struct v4l2_subdev
> > + * @notifier: pointer to &struct v4l2_async_notifier
> > + */
> > +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> > + struct v4l2_async_notifier *notifier);
> > +
> > +/**
> > * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier
> > *
> > * @notifier: pointer to &struct v4l2_async_notifier
> >
>
> Do you have a git tree with this patch series? I think I need to look at this
> in the final version, not just the patch.
I'll upload the patches after making the latest changes.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices
2017-09-03 17:49 ` [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices Sakari Ailus
2017-09-04 14:56 ` Hans Verkuil
@ 2017-09-05 6:49 ` Hans Verkuil
2017-09-05 8:36 ` Sakari Ailus
1 sibling, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-05 6:49 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Registering a notifier has required the knowledge of struct v4l2_device
> for the reason that sub-devices generally are registered to the
> v4l2_device (as well as the media device, also available through
> v4l2_device).
>
> This information is not available for sub-device drivers at probe time.
>
> What this patch does is that it allows registering notifiers without
> having v4l2_device around. Instead the sub-device pointer is stored to the
> notifier. Once the sub-device of the driver that registered the notifier
> is registered, the notifier will gain the knowledge of the v4l2_device,
> and the binding of async sub-devices from the sub-device driver's notifier
> may proceed.
>
> The master notifier's complete callback is only called when all sub-device
> notifiers are completed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
> include/media/v4l2-async.h | 19 ++++-
> 2 files changed, 146 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 70d02378b48f..55d7886103d2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,10 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd);
> +
> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> #if IS_ENABLED(CONFIG_I2C)
> @@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
> return NULL;
> }
>
> +static bool v4l2_async_subdev_notifiers_complete(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
> + if (!n->master)
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#define notifier_v4l2_dev(n) \
> + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
> + !!(n)->master ? (n)->master->v4l2_dev : NULL)
> +
> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
Why pass the notifier argument when it is not actually used in the function?
Is this function needed at all? As far as I can see the sd always belongs to
the given notifier, otherwise the v4l2_async_belongs() call would fail.
And v4l2_async_belongs() is always called before v4l2_async_test_notify().
This could all do with some more code comments. I'm having a difficult time
understanding it all.
I'll wait for v8 before continuing this.
Regards,
Hans
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, ¬ifier_list, list) {
> + if (n->sd == sd)
> + return n;
> + }
> +
> + return NULL;
> +}
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices
2017-09-05 6:49 ` Hans Verkuil
@ 2017-09-05 8:36 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-05 8:36 UTC (permalink / raw)
To: Hans Verkuil, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
Hi Hans,
Thanks for the review!
On 09/05/17 09:49, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
>> Registering a notifier has required the knowledge of struct v4l2_device
>> for the reason that sub-devices generally are registered to the
>> v4l2_device (as well as the media device, also available through
>> v4l2_device).
>>
>> This information is not available for sub-device drivers at probe time.
>>
>> What this patch does is that it allows registering notifiers without
>> having v4l2_device around. Instead the sub-device pointer is stored to the
>> notifier. Once the sub-device of the driver that registered the notifier
>> is registered, the notifier will gain the knowledge of the v4l2_device,
>> and the binding of async sub-devices from the sub-device driver's notifier
>> may proceed.
>>
>> The master notifier's complete callback is only called when all sub-device
>> notifiers are completed.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
>> include/media/v4l2-async.h | 19 ++++-
>> 2 files changed, 146 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 70d02378b48f..55d7886103d2 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -25,6 +25,10 @@
>> #include <media/v4l2-fwnode.h>
>> #include <media/v4l2-subdev.h>
>>
>> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>> + struct v4l2_subdev *sd,
>> + struct v4l2_async_subdev *asd);
>> +
>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>> {
>> #if IS_ENABLED(CONFIG_I2C)
>> @@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
>> return NULL;
>> }
>>
>> +static bool v4l2_async_subdev_notifiers_complete(
>> + struct v4l2_async_notifier *notifier)
>> +{
>> + struct v4l2_async_notifier *n;
>> +
>> + list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
>> + if (!n->master)
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +#define notifier_v4l2_dev(n) \
>> + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
>> + !!(n)->master ? (n)->master->v4l2_dev : NULL)
>> +
>> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
>> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>
> Why pass the notifier argument when it is not actually used in the function?
>
> Is this function needed at all? As far as I can see the sd always belongs to
> the given notifier, otherwise the v4l2_async_belongs() call would fail.
> And v4l2_async_belongs() is always called before v4l2_async_test_notify().
The function gets a notifier which the sub-device may have registered,
it's not the same notifier that was used in registering the sub-device
itself.
I'll remove the other argument as well.
>
> This could all do with some more code comments. I'm having a difficult time
> understanding it all.
Yes, I'm adding comments to v8.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (11 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 14:31 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors Sakari Ailus
` (5 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Camera flash drivers (and LEDs) are separate from the sensor devices in
DT. In order to make an association between the two, provide the
association information to the software.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 852041a7480c..fee73cf2a714 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -67,6 +67,14 @@ are required in a relevant parent node:
identifier, should be 1.
- #size-cells : should be zero.
+
+Optional properties
+-------------------
+
+- flash: An array of phandles referring to the flash LED, a sub-node
+ of the LED driver device node.
+
+
Optional endpoint properties
----------------------------
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor
2017-09-03 17:49 ` [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor Sakari Ailus
@ 2017-09-04 14:31 ` Hans Verkuil
2017-09-04 16:27 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 14:31 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 852041a7480c..fee73cf2a714 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -67,6 +67,14 @@ are required in a relevant parent node:
> identifier, should be 1.
> - #size-cells : should be zero.
>
> +
> +Optional properties
> +-------------------
> +
> +- flash: An array of phandles referring to the flash LED, a sub-node
> + of the LED driver device node.
If it is an array, then I guess it should say: "An array of phandles, each referring to
a flash LED,"
Regards,
Hans
> +
> +
> Optional endpoint properties
> ----------------------------
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor
2017-09-04 14:31 ` Hans Verkuil
@ 2017-09-04 16:27 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:27 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
On Mon, Sep 04, 2017 at 04:31:51PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Camera flash drivers (and LEDs) are separate from the sensor devices in
> > DT. In order to make an association between the two, provide the
> > association information to the software.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > ---
> > Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index 852041a7480c..fee73cf2a714 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -67,6 +67,14 @@ are required in a relevant parent node:
> > identifier, should be 1.
> > - #size-cells : should be zero.
> >
> > +
> > +Optional properties
> > +-------------------
> > +
> > +- flash: An array of phandles referring to the flash LED, a sub-node
> > + of the LED driver device node.
>
> If it is an array, then I guess it should say: "An array of phandles, each referring to
> a flash LED,"
Sounds good, I'll use that in v8.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (12 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 14:37 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references Sakari Ailus
` (4 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
The lens-focus property contains a phandle to the lens voice coil driver
that is associated to the sensor; typically both are contained in the same
camera module.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index fee73cf2a714..73d045127dcf 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -74,6 +74,8 @@ Optional properties
- flash: An array of phandles referring to the flash LED, a sub-node
of the LED driver device node.
+- lens-focus: A phandle to the node of the focus lens controller.
+
Optional endpoint properties
----------------------------
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors
2017-09-03 17:49 ` [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors Sakari Ailus
@ 2017-09-04 14:37 ` Hans Verkuil
2017-09-04 16:29 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 14:37 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> The lens-focus property contains a phandle to the lens voice coil driver
> that is associated to the sensor; typically both are contained in the same
> camera module.
Just to be certain: this lens-focus phandle should also work fine for a motor
driver, right?
We (Cisco) also have a camera that has an iris motor, but since nothing upstream
uses that I'm not sure if we should bother adding that as well.
Regards,
Hans
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index fee73cf2a714..73d045127dcf 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -74,6 +74,8 @@ Optional properties
> - flash: An array of phandles referring to the flash LED, a sub-node
> of the LED driver device node.
>
> +- lens-focus: A phandle to the node of the focus lens controller.
> +
>
> Optional endpoint properties
> ----------------------------
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors
2017-09-04 14:37 ` Hans Verkuil
@ 2017-09-04 16:29 ` Sakari Ailus
0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:29 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
Hi Hans,
On Mon, Sep 04, 2017 at 04:37:29PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > The lens-focus property contains a phandle to the lens voice coil driver
> > that is associated to the sensor; typically both are contained in the same
> > camera module.
>
> Just to be certain: this lens-focus phandle should also work fine for a motor
> driver, right?
Yes; the commit message specifies what this is used right now but nothing
prevents using this for different lens control technologies either. The
property itself does not mention voice coil modules.
>
> We (Cisco) also have a camera that has an iris motor, but since nothing upstream
> uses that I'm not sure if we should bother adding that as well.
Do you have one for focussing only or for zooming as well?
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (13 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 14:44 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs Sakari Ailus
` (3 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Add function v4l2_fwnode_reference_count() for counting external
references and v4l2_fwnode_reference_parse() for parsing them as async
sub-devices.
This can be done on e.g. flash or lens async sub-devices that are not part
of but are associated with a sensor.
struct v4l2_async_notifier.max_subdevs field is added to contain the
maximum number of sub-devices in a notifier to reflect the memory
allocated for the subdevs array.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-fwnode.c | 81 +++++++++++++++++++++++++++++++++++
include/media/v4l2-fwnode.h | 28 ++++++++++++
2 files changed, 109 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index f8c7a9bc6a58..24f8020caf28 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
}
EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
+static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
+{
+ unsigned int i;
+
+ for (i = 0; i < args->nargs; i++) {
+ pr_cont(" %u", args->args[i]);
+ if (i + 1 < args->nargs)
+ pr_cont(",");
+ }
+}
+
+int v4l2_fwnode_reference_parse(
+ struct device *dev, struct v4l2_async_notifier *notifier,
+ const char *prop, const char *nargs_prop, unsigned int nargs,
+ size_t asd_struct_size,
+ int (*parse_single)(struct device *dev,
+ struct fwnode_reference_args *args,
+ struct v4l2_async_subdev *asd))
+{
+ struct fwnode_reference_args args;
+ unsigned int index = 0;
+ int ret = -ENOENT;
+
+ if (asd_struct_size < sizeof(struct v4l2_async_subdev))
+ return -EINVAL;
+
+ for (; !fwnode_property_get_reference_args(
+ dev_fwnode(dev), prop, nargs_prop, nargs,
+ index, &args); index++)
+ fwnode_handle_put(args.fwnode);
+
+ ret = v4l2_async_notifier_realloc(notifier,
+ notifier->num_subdevs + index);
+ if (ret)
+ return -ENOMEM;
+
+ for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
+ dev_fwnode(dev), prop, nargs_prop, nargs,
+ index, &args); index++) {
+ struct v4l2_async_subdev *asd;
+
+ if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
+ break;
+
+ asd = kzalloc(asd_struct_size, GFP_KERNEL);
+ if (!asd) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ ret = parse_single ? parse_single(dev, &args, asd) : 0;
+ if (ret == -ENOTCONN) {
+ dev_dbg(dev,
+ "ignoring reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
+ prop, nargs_prop, nargs, index);
+ v4l2_fwnode_print_args(&args);
+ pr_cont("\n");
+ continue;
+ } else if (ret < 0) {
+ dev_warn(dev,
+ "driver could not parse reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
+ prop, nargs_prop, nargs, index);
+ v4l2_fwnode_print_args(&args);
+ pr_cont("\n");
+ goto error;
+ }
+
+ notifier->subdevs[notifier->num_subdevs] = asd;
+ asd->match.fwnode.fwnode = args.fwnode;
+ asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+ notifier->num_subdevs++;
+ }
+
+ return 0;
+
+error:
+ fwnode_handle_put(args.fwnode);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 6d125f26ec84..52f528162818 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
struct v4l2_fwnode_endpoint *vep,
struct v4l2_async_subdev *asd));
+/**
+ * v4l2_fwnode_reference_parse - parse references for async sub-devices
+ * @dev: the device node the properties of which are parsed for references
+ * @notifier: the async notifier where the async subdevs will be added
+ * @prop: the name of the property
+ * @nargs_prop: the name of the property in the remote node that specifies the
+ * number of integer arguments (may be NULL, in that case nargs
+ * will be used).
+ * @nargs: the number of integer arguments after the reference
+ * @asd_struct_size: the size of the driver's async sub-device struct, including
+ * @struct v4l2_async_subdev
+ * @parse_single: Driver's callback function for parsing a reference. Optional.
+ * Return: 0 on success
+ * %-ENOTCONN if the reference is to be skipped but this
+ * should not be considered as an error
+ *
+ * Return: 0 on success
+ * -ENOMEM if memory allocation failed
+ * -EINVAL if property parsing failed
+ */
+int v4l2_fwnode_reference_parse(
+ struct device *dev, struct v4l2_async_notifier *notifier,
+ const char *prop, const char *nargs_prop, unsigned int nargs,
+ size_t asd_struct_size,
+ int (*parse_single)(struct device *dev,
+ struct fwnode_reference_args *args,
+ struct v4l2_async_subdev *asd));
+
#endif /* _V4L2_FWNODE_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references
2017-09-03 17:49 ` [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references Sakari Ailus
@ 2017-09-04 14:44 ` Hans Verkuil
2017-09-04 16:34 ` Sakari Ailus
0 siblings, 1 reply; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 14:44 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add function v4l2_fwnode_reference_count() for counting external
> references and v4l2_fwnode_reference_parse() for parsing them as async
> sub-devices.
>
> This can be done on e.g. flash or lens async sub-devices that are not part
> of but are associated with a sensor.
>
> struct v4l2_async_notifier.max_subdevs field is added to contain the
> maximum number of sub-devices in a notifier to reflect the memory
> allocated for the subdevs array.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 81 +++++++++++++++++++++++++++++++++++
> include/media/v4l2-fwnode.h | 28 ++++++++++++
> 2 files changed, 109 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index f8c7a9bc6a58..24f8020caf28 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> }
> EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>
> +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < args->nargs; i++) {
> + pr_cont(" %u", args->args[i]);
> + if (i + 1 < args->nargs)
> + pr_cont(",");
> + }
> +}
> +
> +int v4l2_fwnode_reference_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *prop, const char *nargs_prop, unsigned int nargs,
> + size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct fwnode_reference_args *args,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_reference_args args;
> + unsigned int index = 0;
> + int ret = -ENOENT;
> +
> + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> + return -EINVAL;
> +
> + for (; !fwnode_property_get_reference_args(
> + dev_fwnode(dev), prop, nargs_prop, nargs,
> + index, &args); index++)
> + fwnode_handle_put(args.fwnode);
> +
> + ret = v4l2_async_notifier_realloc(notifier,
> + notifier->num_subdevs + index);
> + if (ret)
> + return -ENOMEM;
> +
> + for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> + dev_fwnode(dev), prop, nargs_prop, nargs,
> + index, &args); index++) {
> + struct v4l2_async_subdev *asd;
> +
> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> + break;
As mentioned elsewhere: I think this should return an error.
> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + ret = parse_single ? parse_single(dev, &args, asd) : 0;
> + if (ret == -ENOTCONN) {
> + dev_dbg(dev,
> + "ignoring reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
> + prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args(&args);
> + pr_cont("\n");
asd isn't freed.
> + continue;
> + } else if (ret < 0) {
> + dev_warn(dev,
> + "driver could not parse reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
> + prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args(&args);
> + pr_cont("\n");
Ditto.
> + goto error;
> + }
> +
> + notifier->subdevs[notifier->num_subdevs] = asd;
> + asd->match.fwnode.fwnode = args.fwnode;
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->num_subdevs++;
> + }
> +
> + return 0;
> +
> +error:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 6d125f26ec84..52f528162818 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd));
>
> +/**
> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> + * @dev: the device node the properties of which are parsed for references
the device node whose properties are...
> + * @notifier: the async notifier where the async subdevs will be added
> + * @prop: the name of the property
> + * @nargs_prop: the name of the property in the remote node that specifies the
> + * number of integer arguments (may be NULL, in that case nargs
> + * will be used).
> + * @nargs: the number of integer arguments after the reference
> + * @asd_struct_size: the size of the driver's async sub-device struct, including
> + * @struct v4l2_async_subdev
> + * @parse_single: Driver's callback function for parsing a reference. Optional.
Driver's -> driver's
> + * Return: 0 on success
> + * %-ENOTCONN if the reference is to be skipped but this
> + * should not be considered as an error
skipped but this -> skipped. This
> + *
> + * Return: 0 on success
> + * -ENOMEM if memory allocation failed
> + * -EINVAL if property parsing failed
> + */
> +int v4l2_fwnode_reference_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *prop, const char *nargs_prop, unsigned int nargs,
> + size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct fwnode_reference_args *args,
> + struct v4l2_async_subdev *asd));
> +
> #endif /* _V4L2_FWNODE_H */
>
Regards,
Hans
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references
2017-09-04 14:44 ` Hans Verkuil
@ 2017-09-04 16:34 ` Sakari Ailus
2017-09-04 17:38 ` Hans Verkuil
0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 16:34 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
Hi Hans,
On Mon, Sep 04, 2017 at 04:44:48PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Add function v4l2_fwnode_reference_count() for counting external
> > references and v4l2_fwnode_reference_parse() for parsing them as async
> > sub-devices.
> >
> > This can be done on e.g. flash or lens async sub-devices that are not part
> > of but are associated with a sensor.
> >
> > struct v4l2_async_notifier.max_subdevs field is added to contain the
> > maximum number of sub-devices in a notifier to reflect the memory
> > allocated for the subdevs array.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 81 +++++++++++++++++++++++++++++++++++
> > include/media/v4l2-fwnode.h | 28 ++++++++++++
> > 2 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index f8c7a9bc6a58..24f8020caf28 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> > }
> > EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> >
> > +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < args->nargs; i++) {
> > + pr_cont(" %u", args->args[i]);
> > + if (i + 1 < args->nargs)
> > + pr_cont(",");
> > + }
> > +}
> > +
> > +int v4l2_fwnode_reference_parse(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + const char *prop, const char *nargs_prop, unsigned int nargs,
> > + size_t asd_struct_size,
> > + int (*parse_single)(struct device *dev,
> > + struct fwnode_reference_args *args,
> > + struct v4l2_async_subdev *asd))
> > +{
> > + struct fwnode_reference_args args;
> > + unsigned int index = 0;
> > + int ret = -ENOENT;
> > +
> > + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > + return -EINVAL;
> > +
> > + for (; !fwnode_property_get_reference_args(
> > + dev_fwnode(dev), prop, nargs_prop, nargs,
> > + index, &args); index++)
> > + fwnode_handle_put(args.fwnode);
> > +
> > + ret = v4l2_async_notifier_realloc(notifier,
> > + notifier->num_subdevs + index);
> > + if (ret)
> > + return -ENOMEM;
> > +
> > + for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> > + dev_fwnode(dev), prop, nargs_prop, nargs,
> > + index, &args); index++) {
> > + struct v4l2_async_subdev *asd;
> > +
> > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> > + break;
>
> As mentioned elsewhere: I think this should return an error.
I'll use -EINVAL and put the fwnode as well, i.e. goto error.
>
> > +
> > + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > + if (!asd) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + ret = parse_single ? parse_single(dev, &args, asd) : 0;
> > + if (ret == -ENOTCONN) {
> > + dev_dbg(dev,
> > + "ignoring reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
> > + prop, nargs_prop, nargs, index);
> > + v4l2_fwnode_print_args(&args);
> > + pr_cont("\n");
>
> asd isn't freed.
Will fix both.
>
> > + continue;
> > + } else if (ret < 0) {
> > + dev_warn(dev,
> > + "driver could not parse reference prop \"%s\", nargs_prop \"%s\", nargs %u, index %u",
> > + prop, nargs_prop, nargs, index);
> > + v4l2_fwnode_print_args(&args);
> > + pr_cont("\n");
>
> Ditto.
>
> > + goto error;
> > + }
> > +
> > + notifier->subdevs[notifier->num_subdevs] = asd;
> > + asd->match.fwnode.fwnode = args.fwnode;
> > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > + notifier->num_subdevs++;
> > + }
> > +
> > + return 0;
> > +
> > +error:
> > + fwnode_handle_put(args.fwnode);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> > +
> > MODULE_LICENSE("GPL");
> > MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> > MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 6d125f26ec84..52f528162818 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> > struct v4l2_fwnode_endpoint *vep,
> > struct v4l2_async_subdev *asd));
> >
> > +/**
> > + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> > + * @dev: the device node the properties of which are parsed for references
>
> the device node whose properties are...
Both mean the same thing.
>
> > + * @notifier: the async notifier where the async subdevs will be added
> > + * @prop: the name of the property
> > + * @nargs_prop: the name of the property in the remote node that specifies the
> > + * number of integer arguments (may be NULL, in that case nargs
> > + * will be used).
> > + * @nargs: the number of integer arguments after the reference
> > + * @asd_struct_size: the size of the driver's async sub-device struct, including
> > + * @struct v4l2_async_subdev
> > + * @parse_single: Driver's callback function for parsing a reference. Optional.
>
> Driver's -> driver's
Fixed.
>
> > + * Return: 0 on success
> > + * %-ENOTCONN if the reference is to be skipped but this
> > + * should not be considered as an error
>
> skipped but this -> skipped. This
Also should -> will.
>
> > + *
> > + * Return: 0 on success
> > + * -ENOMEM if memory allocation failed
> > + * -EINVAL if property parsing failed
> > + */
> > +int v4l2_fwnode_reference_parse(
> > + struct device *dev, struct v4l2_async_notifier *notifier,
> > + const char *prop, const char *nargs_prop, unsigned int nargs,
> > + size_t asd_struct_size,
> > + int (*parse_single)(struct device *dev,
> > + struct fwnode_reference_args *args,
> > + struct v4l2_async_subdev *asd));
> > +
> > #endif /* _V4L2_FWNODE_H */
> >
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references
2017-09-04 16:34 ` Sakari Ailus
@ 2017-09-04 17:38 ` Hans Verkuil
0 siblings, 0 replies; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 17:38 UTC (permalink / raw)
To: Sakari Ailus
Cc: Sakari Ailus, linux-media, niklas.soderlund, robh,
laurent.pinchart, devicetree, pavel, sre
On 09/04/2017 06:34 PM, Sakari Ailus wrote:
>>> MODULE_LICENSE("GPL");
>>> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
>>> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>> index 6d125f26ec84..52f528162818 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
>>> struct v4l2_fwnode_endpoint *vep,
>>> struct v4l2_async_subdev *asd));
>>>
>>> +/**
>>> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
>>> + * @dev: the device node the properties of which are parsed for references
>>
>> the device node whose properties are...
>
> Both mean the same thing.
Yes, but I think your sentence is a bit awkward. Just my opinion, though.
Regards,
Hans
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (14 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 14:58 ` Hans Verkuil
2017-09-03 17:49 ` [PATCH v7 17/18] smiapp: Add support for flash and lens devices Sakari Ailus
` (2 subsequent siblings)
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Add v4l2_fwnode_parse_reference_sensor_common for parsing common
sensor properties that refer to adjacent devices such as flash or lens
driver chips.
As this is an association only, there's little a regular driver needs to
know about these devices as such.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-fwnode.c | 27 +++++++++++++++++++++++++++
include/media/v4l2-fwnode.h | 3 +++
2 files changed, 30 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 24f8020caf28..f28be3b751d3 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -530,6 +530,33 @@ int v4l2_fwnode_reference_parse(
}
EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
+int v4l2_fwnode_reference_parse_sensor_common(
+ struct device *dev, struct v4l2_async_notifier *notifier)
+{
+ static const struct {
+ char *name;
+ char *cells;
+ unsigned int nr_cells;
+ } props[] = {
+ { "flash", NULL, 0 },
+ { "lens-focus", NULL, 0 },
+ };
+ unsigned int i;
+ int rval;
+
+ for (i = 0; i < ARRAY_SIZE(props); i++) {
+ rval = v4l2_fwnode_reference_parse(
+ dev, notifier, props[i].name, props[i].cells,
+ props[i].nr_cells, sizeof(struct v4l2_async_subdev),
+ NULL);
+ if (rval < 0 && rval != -ENOENT)
+ return rval;
+ }
+
+ return rval;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse_sensor_common);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 52f528162818..fd14f1d38966 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -282,4 +282,7 @@ int v4l2_fwnode_reference_parse(
struct fwnode_reference_args *args,
struct v4l2_async_subdev *asd));
+int v4l2_fwnode_reference_parse_sensor_common(
+ struct device *dev, struct v4l2_async_notifier *notifier);
+
#endif /* _V4L2_FWNODE_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs
2017-09-03 17:49 ` [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs Sakari Ailus
@ 2017-09-04 14:58 ` Hans Verkuil
0 siblings, 0 replies; 46+ messages in thread
From: Hans Verkuil @ 2017-09-04 14:58 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: niklas.soderlund, robh, laurent.pinchart, devicetree, pavel, sre
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add v4l2_fwnode_parse_reference_sensor_common for parsing common
> sensor properties that refer to adjacent devices such as flash or lens
> driver chips.
>
> As this is an association only, there's little a regular driver needs to
> know about these devices as such.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-fwnode.c | 27 +++++++++++++++++++++++++++
> include/media/v4l2-fwnode.h | 3 +++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 24f8020caf28..f28be3b751d3 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -530,6 +530,33 @@ int v4l2_fwnode_reference_parse(
> }
> EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
>
> +int v4l2_fwnode_reference_parse_sensor_common(
> + struct device *dev, struct v4l2_async_notifier *notifier)
> +{
> + static const struct {
> + char *name;
> + char *cells;
> + unsigned int nr_cells;
> + } props[] = {
> + { "flash", NULL, 0 },
> + { "lens-focus", NULL, 0 },
> + };
> + unsigned int i;
> + int rval;
> +
> + for (i = 0; i < ARRAY_SIZE(props); i++) {
> + rval = v4l2_fwnode_reference_parse(
> + dev, notifier, props[i].name, props[i].cells,
> + props[i].nr_cells, sizeof(struct v4l2_async_subdev),
> + NULL);
> + if (rval < 0 && rval != -ENOENT)
> + return rval;
> + }
> +
> + return rval;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse_sensor_common);
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 52f528162818..fd14f1d38966 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -282,4 +282,7 @@ int v4l2_fwnode_reference_parse(
> struct fwnode_reference_args *args,
> struct v4l2_async_subdev *asd));
>
> +int v4l2_fwnode_reference_parse_sensor_common(
> + struct device *dev, struct v4l2_async_notifier *notifier);
> +
> #endif /* _V4L2_FWNODE_H */
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 17/18] smiapp: Add support for flash and lens devices
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (15 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-03 20:18 ` [PATCH v7 1/1] dt: bindings: smiapp: Document lens-focus and flash properties Sakari Ailus
2017-09-03 17:49 ` [PATCH v7 18/18] arm: dts: omap3: N9/N950: Add flash references to the camera Sakari Ailus
2017-09-04 7:47 ` [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
18 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Parse async sub-devices by using
v4l2_subdev_fwnode_reference_parse_sensor_common().
These types devices aren't directly related to the sensor, but are
nevertheless handled by the smiapp driver due to the relationship of these
component to the main part of the camera module --- the sensor.
This does not yet address providing the user space with information on how
to associate the sensor or lens devices but the kernel now has the
necessary information to do that.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/smiapp/smiapp-core.c | 29 +++++++++++++++++++++++++++--
drivers/media/i2c/smiapp/smiapp.h | 4 +++-
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 700f433261d0..34fb5a2f63ce 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -31,7 +31,7 @@
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/smiapp.h>
-#include <linux/v4l2-mediabus.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-fwnode.h>
#include <media/v4l2-device.h>
@@ -2784,6 +2784,16 @@ static int __maybe_unused smiapp_resume(struct device *dev)
return rval;
}
+static int smiapp_async_complete(struct v4l2_async_notifier *notifier)
+{
+ return v4l2_device_register_subdev_nodes(
+ notifier->master->v4l2_dev);
+}
+
+static const struct v4l2_async_notifier_operations smiapp_async_notifier_ops = {
+ .complete = smiapp_async_complete,
+};
+
static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
{
struct smiapp_hwconfig *hwcfg;
@@ -2887,6 +2897,11 @@ static int smiapp_probe(struct i2c_client *client,
v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
+ rval = v4l2_fwnode_reference_parse_sensor_common(
+ &client->dev, &sensor->notifier);
+ if (rval < 0 && rval != -ENOENT)
+ return rval;
+
sensor->vana = devm_regulator_get(&client->dev, "vana");
if (IS_ERR(sensor->vana)) {
dev_err(&client->dev, "could not get regulator for vana\n");
@@ -3092,9 +3107,15 @@ static int smiapp_probe(struct i2c_client *client,
if (rval < 0)
goto out_media_entity_cleanup;
+ sensor->notifier.ops = &smiapp_async_notifier_ops;
+ rval = v4l2_async_subdev_notifier_register(&sensor->src->sd,
+ &sensor->notifier);
+ if (rval)
+ goto out_media_entity_cleanup;
+
rval = v4l2_async_register_subdev(&sensor->src->sd);
if (rval < 0)
- goto out_media_entity_cleanup;
+ goto out_unregister_async_notifier;
pm_runtime_set_active(&client->dev);
pm_runtime_get_noresume(&client->dev);
@@ -3105,6 +3126,9 @@ static int smiapp_probe(struct i2c_client *client,
return 0;
+out_unregister_async_notifier:
+ v4l2_async_notifier_unregister(&sensor->notifier);
+
out_media_entity_cleanup:
media_entity_cleanup(&sensor->src->sd.entity);
@@ -3124,6 +3148,7 @@ static int smiapp_remove(struct i2c_client *client)
unsigned int i;
v4l2_async_unregister_subdev(subdev);
+ v4l2_async_notifier_unregister(&sensor->notifier);
pm_runtime_disable(&client->dev);
if (!pm_runtime_status_suspended(&client->dev))
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index f74d695018b9..be92cb5713f4 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -20,9 +20,10 @@
#define __SMIAPP_PRIV_H_
#include <linux/mutex.h>
+#include <media/i2c/smiapp.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-subdev.h>
-#include <media/i2c/smiapp.h>
#include "smiapp-pll.h"
#include "smiapp-reg.h"
@@ -172,6 +173,7 @@ struct smiapp_subdev {
* struct smiapp_sensor - Main device structure
*/
struct smiapp_sensor {
+ struct v4l2_async_notifier notifier;
/*
* "mutex" is used to serialise access to all fields here
* except v4l2_ctrls at the end of the struct. "mutex" is also
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v7 1/1] dt: bindings: smiapp: Document lens-focus and flash properties
2017-09-03 17:49 ` [PATCH v7 17/18] smiapp: Add support for flash and lens devices Sakari Ailus
@ 2017-09-03 20:18 ` Sakari Ailus
2017-09-08 12:28 ` Pavel Machek
0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 20:18 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Document optional lens-focus and flash properties for the smiapp driver.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi all,
This should precede the smiapp driver change.
Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
index 855e1faf73e2..f02178eef84d 100644
--- a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
+++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
@@ -27,6 +27,8 @@ Optional properties
- nokia,nvm-size: The size of the NVM, in bytes. If the size is not given,
the NVM contents will not be read.
- reset-gpios: XSHUTDOWN GPIO
+- flash: One or more phandles to refer to flash LEDs
+- lens-focus: Phandle for lens focus
Endpoint node mandatory properties
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v7 1/1] dt: bindings: smiapp: Document lens-focus and flash properties
2017-09-03 20:18 ` [PATCH v7 1/1] dt: bindings: smiapp: Document lens-focus and flash properties Sakari Ailus
@ 2017-09-08 12:28 ` Pavel Machek
0 siblings, 0 replies; 46+ messages in thread
From: Pavel Machek @ 2017-09-08 12:28 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, niklas.soderlund, robh, hverkuil, laurent.pinchart,
devicetree, sre
[-- Attachment #1: Type: text/plain, Size: 373 bytes --]
On Sun 2017-09-03 23:18:05, Sakari Ailus wrote:
> Document optional lens-focus and flash properties for the smiapp driver.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v7 18/18] arm: dts: omap3: N9/N950: Add flash references to the camera
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (16 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 17/18] smiapp: Add support for flash and lens devices Sakari Ailus
@ 2017-09-03 17:49 ` Sakari Ailus
2017-09-04 7:47 ` [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-03 17:49 UTC (permalink / raw)
To: linux-media
Cc: niklas.soderlund, robh, hverkuil, laurent.pinchart, devicetree,
pavel, sre
Add flash and indicator LED phandles to the sensor node.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
arch/arm/boot/dts/omap3-n9.dts | 1 +
arch/arm/boot/dts/omap3-n950-n9.dtsi | 4 ++--
arch/arm/boot/dts/omap3-n950.dts | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
index b9e58c536afd..a2944010f62f 100644
--- a/arch/arm/boot/dts/omap3-n9.dts
+++ b/arch/arm/boot/dts/omap3-n9.dts
@@ -26,6 +26,7 @@
clocks = <&isp 0>;
clock-frequency = <9600000>;
nokia,nvm-size = <(16 * 64)>;
+ flash = <&as3645a_flash &as3645a_indicator>;
port {
smia_1_1: endpoint {
link-frequencies = /bits/ 64 <199200000 210000000 499200000>;
diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
index cb47ae79a5f9..92c1a1da28d3 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -269,13 +269,13 @@
as3645a@30 {
reg = <0x30>;
compatible = "ams,as3645a";
- flash {
+ as3645a_flash: flash {
flash-timeout-us = <150000>;
flash-max-microamp = <320000>;
led-max-microamp = <60000>;
peak-current-limit = <1750000>;
};
- indicator {
+ as3645a_indicator: indicator {
led-max-microamp = <10000>;
};
};
diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
index 646601a3ebd8..bba5c5a6950c 100644
--- a/arch/arm/boot/dts/omap3-n950.dts
+++ b/arch/arm/boot/dts/omap3-n950.dts
@@ -60,6 +60,7 @@
clocks = <&isp 0>;
clock-frequency = <9600000>;
nokia,nvm-size = <(16 * 64)>;
+ flash = <&as3645a_flash &as3645a_indicator>;
port {
smia_1_1: endpoint {
link-frequencies = /bits/ 64 <210000000 333600000 398400000>;
--
2.11.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS
2017-09-03 17:49 [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS Sakari Ailus
` (17 preceding siblings ...)
2017-09-03 17:49 ` [PATCH v7 18/18] arm: dts: omap3: N9/N950: Add flash references to the camera Sakari Ailus
@ 2017-09-04 7:47 ` Sakari Ailus
18 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2017-09-04 7:47 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, niklas.soderlund, robh, hverkuil, laurent.pinchart,
devicetree, pavel, sre
Oh, and this set depends on this patch, on its way to 4.14-rc1 I believe:
<URL:https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=device-properties&id=3e3119d3088f41106f3581d39e7694a50ca3fc02>
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 46+ messages in thread