[RFC 00/19] Async sub-notifiers and how to use them

[RFC 00/19] Async sub-notifiers and how to use them

[Sakari Ailus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 4782 bytes --]

Hi folks,

This RFC patchset achieves a number of things which I've put to the same
patchset for they need to be show together to demonstrate the use cases.

I don't really intend this to compete with Niklas's patchset but much of
the problem area addressed by the two is the same.

Comments would be welcome.

- Add AS3645A LED flash class driver.

- Add async notifiers (by Niklas).

- V4L2 sub-device node registration is moved to take place at the same time
  with the registration of the sub-device itself. With this change,
  sub-device node registration behaviour is aligned with video node
  registration.

- The former is made possible by moving the bound() callback after
  sub-device registration.

- As all the device node registration and link creation is done as the
  respective devices are probed, there is no longer dependency to the
  notifier complete callback which as itself is seen problematic. The
  complete callback still exists but there's no need to use it, pending
  changes in individual drivers.

  See:
  <URL:http://www.spinics.net/lists/linux-media/msg118323.html>

  As a result, if a part of the media device fails to initialise because it
  is e.g. physically broken, it will be possible to use what works.

- Finally, the use of the async sub-notifier is hidden in the framework and
  all a driver (such as smiapp) needs to do is to call
  v4l2_subdev_fwnode_reference_parse_sensor_common() in its probe()
  function to find out the associated devices (lens and flash). This
  approach makes it possible to later on to rework the sub-notifier
  implementation without touching driver code. Endpoints can be parsed
  similarly by simply calling v4l2_subdev_fwnode_endpoints_parse() for
  driver's probe function.

The patches depend on this branch currently:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=as3645a-leds-base>

It's essentially the V4L2 flash class patches I've posted earlier today and
a stash of fwnode property API improvements.


Niklas Söderlund (1):
  v4l: async: add subnotifier registration for subdevices

Sakari Ailus (18):
  device property: Introduce fwnode_property_get_reference_args
  dt: bindings: Add a binding for flash devices associated to a sensor
  dt: bindings: Add lens-focus binding for image sensors
  leds: as3645a: Add LED flash class driver
  leds: as3645a: Separate flash and indicator LED sub-devices
  v4l: fwnode: Support generic parsing of graph endpoints in V4L2
  arm: dts: omap3: N9/N950: Add AS3645A camera flash
  v4l2-fwnode: Add conveniences function for parsing generic references
  v4l2-fwnode: Add convenience function for parsing common external refs
  v4l2-async: Register sub-devices before calling bound callback
  v4l2-subdev: Support registering V4L2 sub-device nodes one by one
  v4l2-device: Register sub-device nodes at sub-device registration time
  omap3isp: Move sub-device link creation to notifier bound callback
  omap3isp: Initialise "crashed" media entity enum in probe
  omap3isp: Move media device registration to probe
  omap3isp: Drop the async notifier callback
  v4l2-fwnode: Add abstracted sub-device notifiers
  smiapp: Add support for flash and lens devices

 .../devicetree/bindings/media/video-interfaces.txt |  10 +
 Documentation/media/kapi/v4l2-subdev.rst           |  20 +
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/omap3-n9.dts                     |   1 +
 arch/arm/boot/dts/omap3-n950-n9.dtsi               |  14 +
 arch/arm/boot/dts/omap3-n950.dts                   |   1 +
 drivers/acpi/property.c                            |  27 +
 drivers/base/property.c                            |  12 +
 drivers/leds/Kconfig                               |   8 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-as3645a.c                        | 762 +++++++++++++++++++++
 drivers/media/i2c/smiapp/smiapp-core.c             |   5 +
 drivers/media/platform/omap3isp/isp.c              | 144 ++--
 drivers/media/platform/omap3isp/isp.h              |   3 -
 drivers/media/v4l2-core/v4l2-async.c               |  96 ++-
 drivers/media/v4l2-core/v4l2-device.c              | 139 ++--
 drivers/media/v4l2-core/v4l2-fwnode.c              | 174 +++++
 drivers/media/v4l2-core/v4l2-subdev.c              |  44 +-
 drivers/of/property.c                              |  31 +
 include/linux/fwnode.h                             |  19 +
 include/linux/property.h                           |   4 +
 include/media/v4l2-async.h                         |  26 +-
 include/media/v4l2-fwnode.h                        |  21 +
 include/media/v4l2-subdev.h                        |  11 +
 24 files changed, 1388 insertions(+), 191 deletions(-)
 create mode 100644 drivers/leds/leds-as3645a.c

-- 
2.11.0

[RFC 01/19] device property: Introduce fwnode_property_get_reference_args

[Sakari Ailus]

The new fwnode_property_get_reference_args() interface amends the fwnode
property API with the functionality of both of_parse_phandle_with_args()
and __acpi_node_get_property_reference().

The semantics is slightly different: the cells property is ignored on ACPI
as the number of arguments can be explicitly obtained from the firmware
interface.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c  | 27 +++++++++++++++++++++++++++
 drivers/base/property.c  | 12 ++++++++++++
 drivers/of/property.c    | 31 +++++++++++++++++++++++++++++++
 include/linux/fwnode.h   | 19 +++++++++++++++++++
 include/linux/property.h |  4 ++++
 5 files changed, 93 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f8d60051efb8..b39497cd3f7f 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1195,6 +1195,32 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 	return NULL;
 }
 
+static int
+acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
+			       const char *prop, const char *nargs_prop,
+			       unsigned int args_count, unsigned int index,
+			       struct fwnode_reference_args *args)
+{
+	struct acpi_reference_args acpi_args;
+	unsigned int i;
+	int ret;
+
+	ret = __acpi_node_get_property_reference(fwnode, prop, index,
+						 args_count, &acpi_args);
+	if (ret < 0)
+		return ret;
+	if (!args)
+		return 0;
+
+	args->nargs = acpi_args.nargs;
+	args->fwnode = acpi_fwnode_handle(acpi_args.adev);
+
+	for (i = 0; i < NR_OF_FWNODE_REFERENCE_ARGS; i++)
+		args->args[i] = i < acpi_args.nargs ? acpi_args.args[i] : 0;
+
+	return 0;
+}
+
 static struct fwnode_handle *
 acpi_fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 				    struct fwnode_handle *prev)
@@ -1248,6 +1274,7 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 		.get_parent = acpi_node_get_parent,			\
 		.get_next_child_node = acpi_get_next_subnode,		\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
+		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
 			acpi_fwnode_graph_get_next_endpoint,		\
 		.graph_get_remote_endpoint =				\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 673e2353a2fb..369b5471198f 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -665,6 +665,18 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+/**
+ */
+int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
+				       const char *prop, const char *nargs_prop,
+				       unsigned int nargs, unsigned int index,
+				       struct fwnode_reference_args *args)
+{
+	return fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
+				  nargs, index, args);
+}
+EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
+
 static int property_copy_string_array(struct property_entry *dst,
 				      const struct property_entry *src)
 {
diff --git a/drivers/of/property.c b/drivers/of/property.c
index ae46a6f0ea36..2350103f8be6 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -891,6 +891,36 @@ of_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 	return NULL;
 }
 
+static int
+of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
+			     const char *prop, const char *nargs_prop,
+			     unsigned int nargs, unsigned int index,
+			     struct fwnode_reference_args *args)
+{
+	struct of_phandle_args of_args;
+	unsigned int i;
+	int ret;
+
+	if (nargs_prop)
+		ret = of_parse_phandle_with_args(to_of_node(fwnode), prop,
+						 nargs_prop, index, &of_args);
+	else
+		ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop,
+						       nargs, index, &of_args);
+	if (ret < 0)
+		return ret;
+	if (!args)
+		return 0;
+
+	args->nargs = of_args.args_count;
+	args->fwnode = of_fwnode_handle(of_args.np);
+
+	for (i = 0; i < NR_OF_FWNODE_REFERENCE_ARGS; i++)
+		args->args[i] = i < of_args.args_count ? of_args.args[i] : 0;
+
+	return 0;
+}
+
 static struct fwnode_handle *
 of_fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 				  struct fwnode_handle *prev)
@@ -949,6 +979,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
+	.get_reference_args = of_fwnode_get_reference_args,
 	.graph_get_next_endpoint = of_fwnode_graph_get_next_endpoint,
 	.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
 	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 7b50ee4edcfc..218fe03544c9 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -33,6 +33,20 @@ struct fwnode_endpoint {
 	const struct fwnode_handle *local_fwnode;
 };
 
+#define NR_OF_FWNODE_REFERENCE_ARGS	8
+
+/**
+ * struct fwnode_reference_args - Fwnode reference with additional arguments
+ * @fwnode:- A reference to the base fwnode
+ * @nargs: Number of elements in @args array
+ * @args: Integer arguments on the fwnode
+ */
+struct fwnode_reference_args {
+	struct fwnode_handle *fwnode;
+	unsigned int nargs;
+	unsigned int args[NR_OF_FWNODE_REFERENCE_ARGS];
+};
+
 /**
  * struct fwnode_operations - Operations for fwnode interface
  * @get: Get a reference to an fwnode.
@@ -46,6 +60,7 @@ struct fwnode_endpoint {
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
+ * @get_reference_args: Return a reference pointed to by a property, with args
  * @graph_get_next_endpoint: Return an endpoint node in an iteration.
  * @graph_get_remote_endpoint: Return the remote endpoint node of a local
  *			       endpoint node.
@@ -73,6 +88,10 @@ struct fwnode_operations {
 	struct fwnode_handle *
 	(*get_named_child_node)(const struct fwnode_handle *fwnode,
 				const char *name);
+	int (*get_reference_args)(const struct fwnode_handle *fwnode,
+				  const char *prop, const char *nargs_prop,
+				  unsigned int nargs, unsigned int index,
+				  struct fwnode_reference_args *args);
 	struct fwnode_handle *
 	(*graph_get_next_endpoint)(const struct fwnode_handle *fwnode,
 				   struct fwnode_handle *prev);
diff --git a/include/linux/property.h b/include/linux/property.h
index edff3f89e755..6bebee13c5e0 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -73,6 +73,10 @@ int fwnode_property_read_string(const struct fwnode_handle *fwnode,
 				const char *propname, const char **val);
 int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
+int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
+				       const char *prop, const char *nargs_prop,
+				       unsigned int nargs, unsigned int index,
+				       struct fwnode_reference_args *args);
 
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
-- 
2.11.0

[RFC 02/19] v4l: async: add subnotifier registration for subdevices

[Sakari Ailus]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

When the registered() callback of v4l2_subdev_internal_ops is called the
subdevice has access to the master devices v4l2_dev and it's called with
the async frameworks list_lock held. In this context the subdevice can
register its own notifiers to allow for incremental discovery of
subdevices.

The master device registers the subdevices closest to itself in its
notifier while the subdevice(s) register notifiers for their closest
neighboring devices when they are registered. Using this incremental
approach two problems can be solved:

1. The master device no longer has to care how many devices exist in
   the pipeline. It only needs to care about its closest subdevice and
   arbitrary long pipelines can be created without having to adapt the
   master device for each case.

2. Subdevices which are represented as a single DT node but register
   more than one subdevice can use this to improve the pipeline
   discovery, since the subdevice driver is the only one who knows which
   of its subdevices is linked with which subdevice of a neighboring DT
   node.

To enable subdevices to register/unregister notifiers from the
registered()/unregistered() callback v4l2_async_subnotifier_register()
and v4l2_async_subnotifier_unregister() are added. These new notifier
register functions are similar to the master device equivalent functions
but run without taking the v4l2-async list_lock which already is held
when the registered()/unregistered() callbacks are called.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/kapi/v4l2-subdev.rst | 20 ++++++++++
 drivers/media/v4l2-core/v4l2-async.c     | 65 +++++++++++++++++++++++++++-----
 include/media/v4l2-async.h               | 22 +++++++++++
 3 files changed, 98 insertions(+), 9 deletions(-)

diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
index e1f0b726e438..e308f30887a8 100644
--- a/Documentation/media/kapi/v4l2-subdev.rst
+++ b/Documentation/media/kapi/v4l2-subdev.rst
@@ -262,6 +262,26 @@ is called. After all subdevices have been located the .complete() callback is
 called. When a subdevice is removed from the system the .unbind() method is
 called. All three callbacks are optional.
 
+Subdevice drivers might in turn register subnotifier objects with an
+array of other subdevice descriptors that the subdevice needs for its
+own operation. Subnotifiers are an extension of the bridge drivers
+notifier to allow for a incremental registering and matching of
+subdevices. This is useful when a driver only has information about
+which subdevice is closest to itself and would require knowledge from the
+driver of that subdevice to know which other subdevice(s) lie beyond.
+By registering subnotifiers drivers can incrementally move the subdevice
+matching down the chain of drivers. This is performed using the
+:c:func:`v4l2_async_subnotifier_register` call. To unregister the
+subnotifier the driver has to call
+:c:func:`v4l2_async_subnotifier_unregister`. These functions and their
+arguments behave almost the same as the bridge driver notifiers
+described above and are treated equally by the V4L2 core when matching
+asynchronously registered subdevices. The differences are that the
+subnotifier functions act on :c:type:`v4l2_subdev` instead of
+:c:type:`v4l2_device` and that they should be called from the subdevice's
+``.registered()`` and ``.unregistered()``
+:c:type:`v4l2_subdev_internal_ops` callbacks instead of at probe time.
+
 V4L2 sub-device userspace API
 -----------------------------
 
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 268e19724809..d2ce39ac402e 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -163,8 +163,9 @@ 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_do_notifier_register(struct v4l2_device *v4l2_dev,
+					   struct v4l2_async_notifier *notifier,
+					   bool subnotifier)
 {
 	struct v4l2_subdev *sd, *tmp;
 	struct v4l2_async_subdev *asd;
@@ -196,8 +197,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 		list_add_tail(&asd->list, &notifier->waiting);
 	}
 
-	mutex_lock(&list_lock);
+	if (subnotifier)
+		lockdep_assert_held(&list_lock);
+	else
+		mutex_lock(&list_lock);
 
+	/*
+	 * This function can be called recursively so the list
+	 * might be modified in a recursive call. Start from the
+	 * top of the list each iteration.
+	 */
+again:
 	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
 		int ret;
 
@@ -207,21 +217,39 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 
 		ret = v4l2_async_test_notify(notifier, sd, asd);
 		if (ret < 0) {
-			mutex_unlock(&list_lock);
+			if (!subnotifier)
+				mutex_unlock(&list_lock);
 			return ret;
 		}
+		goto again;
 	}
 
 	/* Keep also completed notifiers on the list */
 	list_add(&notifier->list, &notifier_list);
 
-	mutex_unlock(&list_lock);
+	if (!subnotifier)
+		mutex_unlock(&list_lock);
 
 	return 0;
 }
+
+int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
+				    struct v4l2_async_notifier *notifier)
+{
+	return v4l2_async_do_notifier_register(sd->v4l2_dev, notifier, true);
+}
+EXPORT_SYMBOL(v4l2_async_subnotifier_register);
+
+int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
+				 struct v4l2_async_notifier *notifier)
+{
+	return v4l2_async_do_notifier_register(v4l2_dev, notifier, false);
+}
 EXPORT_SYMBOL(v4l2_async_notifier_register);
 
-void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
+static void
+v4l2_async_do_notifier_unregister(struct v4l2_async_notifier *notifier,
+				  bool subnotifier)
 {
 	struct v4l2_subdev *sd, *tmp;
 	unsigned int notif_n_subdev = notifier->num_subdevs;
@@ -238,7 +266,10 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 			"Failed to allocate device cache!\n");
 	}
 
-	mutex_lock(&list_lock);
+	if (subnotifier)
+		lockdep_assert_held(&list_lock);
+	else
+		mutex_lock(&list_lock);
 
 	list_del(&notifier->list);
 
@@ -265,15 +296,20 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 			put_device(d);
 	}
 
-	mutex_unlock(&list_lock);
+	if (!subnotifier)
+		mutex_unlock(&list_lock);
 
 	/*
 	 * Call device_attach() to reprobe devices
 	 *
 	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
 	 * executed.
+	 * TODO: If we are unregistering a subdevice notifier we can't reprobe
+	 * since the lock_list is held by the master device and attaching that
+	 * device would call v4l2_async_register_subdev() and end in a deadlock
+	 * on list_lock.
 	 */
-	while (i--) {
+	while (i-- && !subnotifier) {
 		struct device *d = dev[i];
 
 		if (d && device_attach(d) < 0) {
@@ -297,6 +333,17 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	 * upon notifier registration.
 	 */
 }
+
+void v4l2_async_subnotifier_unregister(struct v4l2_async_notifier *notifier)
+{
+	v4l2_async_do_notifier_unregister(notifier, true);
+}
+EXPORT_SYMBOL(v4l2_async_subnotifier_unregister);
+
+void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
+{
+	v4l2_async_do_notifier_unregister(notifier, false);
+}
 EXPORT_SYMBOL(v4l2_async_notifier_unregister);
 
 int v4l2_async_register_subdev(struct v4l2_subdev *sd)
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 056cae0af2f0..8c7519fce5b9 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -105,6 +105,18 @@ struct v4l2_async_notifier {
 };
 
 /**
+ * v4l2_async_notifier_register - registers a subdevice asynchronous subnotifier
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @notifier: pointer to &struct v4l2_async_notifier
+ *
+ * This function assumes the async list_lock is already locked, allowing it to
+ * be used from the struct v4l2_subdev_internal_ops registered() callback.
+ */
+int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
+				    struct v4l2_async_notifier *notifier);
+
+/**
  * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
  *
  * @v4l2_dev: pointer to &struct v4l2_device
@@ -114,6 +126,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 				 struct v4l2_async_notifier *notifier);
 
 /**
+ * v4l2_async_subnotifier_unregister - unregisters a asynchronous subnotifier
+ *
+ * @notifier: pointer to &struct v4l2_async_notifier
+ *
+ * This function assumes the async list_lock is already locked, allowing it to
+ * be used from the struct v4l2_subdev_internal_ops unregistered() callback.
+ */
+void v4l2_async_subnotifier_unregister(struct v4l2_async_notifier *notifier);
+
+/**
  * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier
  *
  * @notifier: pointer to &struct v4l2_async_notifier
-- 
2.11.0

[RFC 03/19] dt: bindings: Add a binding for flash devices associated to a sensor

[Sakari Ailus]

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>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 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 9cd2a369125d..9723f7e1c7db 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: phandle referring to the flash driver chip. A flash driver may
+  have multiple flashes connected to it.
+
+
 Optional endpoint properties
 ----------------------------
 
-- 
2.11.0

[RFC 04/19] dt: bindings: Add lens-focus binding for image sensors

[Sakari Ailus]

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 9723f7e1c7db..a18d9b2d309f 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -74,6 +74,8 @@ Optional properties
 - flash: phandle referring to the flash driver chip. A flash driver may
   have multiple flashes connected to it.
 
+- lens-focus: A phandle to the node of the focus lens controller.
+
 
 Optional endpoint properties
 ----------------------------
-- 
2.11.0

[RFC 05/19] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
is based on that.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 MAINTAINERS                 |   6 +
 drivers/leds/Kconfig        |   8 +
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-as3645a.c | 742 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 757 insertions(+)
 create mode 100644 drivers/leds/leds-as3645a.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 205d3977ac46..312be8939969 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2106,6 +2106,12 @@ F:	arch/arm64/
 F:	Documentation/arm64/
 
 AS3645A LED FLASH CONTROLLER DRIVER
+M:	Sakari Ailus <sakari.ailus@iki.fi>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+F:	drivers/leds/leds-as3645a.c
+
+AS3645A LED FLASH CONTROLLER DRIVER
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
 T:	git git://linuxtv.org/media_tree.git
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 594b24d410c3..bad3a4098104 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -58,6 +58,14 @@ config LEDS_AAT1290
 	help
 	 This option enables support for the LEDs on the AAT1290.
 
+config LEDS_AS3645A
+	tristate "AS3645A LED flash controller support"
+	depends on I2C && LEDS_CLASS_FLASH
+	help
+	  Enable LED flash class support for AS3645A LED flash
+	  controller. V4L2 flash API is provided as well if
+	  CONFIG_V4L2_FLASH_API is enabled.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 909dae62ba05..7d7b26552923 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 # LED Platform Drivers
 obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
 obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
+obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
new file mode 100644
index 000000000000..b1dc32a3c620
--- /dev/null
+++ b/drivers/leds/leds-as3645a.c
@@ -0,0 +1,742 @@
+/*
+ * drivers/leds/leds-as3645a.c - AS3645A and LM3555 flash controllers driver
+ *
+ * Copyright (C) 2008-2011 Nokia Corporation
+ * Copyright (c) 2011, 2017 Intel Corporation.
+ *
+ * Based on drivers/media/i2c/as3645a.c.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@iki.fi>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/led-class-flash.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include <media/v4l2-flash-led-class.h>
+
+#define AS_TIMER_US_TO_CODE(t)			(((t) / 1000 - 100) / 50)
+#define AS_TIMER_CODE_TO_US(c)			((50 * (c) + 100) * 1000)
+
+/* Register definitions */
+
+/* Read-only Design info register: Reset state: xxxx 0001 */
+#define AS_DESIGN_INFO_REG			0x00
+#define AS_DESIGN_INFO_FACTORY(x)		(((x) >> 4))
+#define AS_DESIGN_INFO_MODEL(x)			((x) & 0x0f)
+
+/* Read-only Version control register: Reset state: 0000 0000
+ * for first engineering samples
+ */
+#define AS_VERSION_CONTROL_REG			0x01
+#define AS_VERSION_CONTROL_RFU(x)		(((x) >> 4))
+#define AS_VERSION_CONTROL_VERSION(x)		((x) & 0x0f)
+
+/* Read / Write	(Indicator and timer register): Reset state: 0000 1111 */
+#define AS_INDICATOR_AND_TIMER_REG		0x02
+#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT	0
+#define AS_INDICATOR_AND_TIMER_VREF_SHIFT	4
+#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT	6
+
+/* Read / Write	(Current set register): Reset state: 0110 1001 */
+#define AS_CURRENT_SET_REG			0x03
+#define AS_CURRENT_ASSIST_LIGHT_SHIFT		0
+#define AS_CURRENT_LED_DET_ON			(1 << 3)
+#define AS_CURRENT_FLASH_CURRENT_SHIFT		4
+
+/* Read / Write	(Control register): Reset state: 1011 0100 */
+#define AS_CONTROL_REG				0x04
+#define AS_CONTROL_MODE_SETTING_SHIFT		0
+#define AS_CONTROL_STROBE_ON			(1 << 2)
+#define AS_CONTROL_OUT_ON			(1 << 3)
+#define AS_CONTROL_EXT_TORCH_ON			(1 << 4)
+#define AS_CONTROL_STROBE_TYPE_EDGE		(0 << 5)
+#define AS_CONTROL_STROBE_TYPE_LEVEL		(1 << 5)
+#define AS_CONTROL_COIL_PEAK_SHIFT		6
+
+/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */
+#define AS_FAULT_INFO_REG			0x05
+#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT	(1 << 1)
+#define AS_FAULT_INFO_INDICATOR_LED		(1 << 2)
+#define AS_FAULT_INFO_LED_AMOUNT		(1 << 3)
+#define AS_FAULT_INFO_TIMEOUT			(1 << 4)
+#define AS_FAULT_INFO_OVER_TEMPERATURE		(1 << 5)
+#define AS_FAULT_INFO_SHORT_CIRCUIT		(1 << 6)
+#define AS_FAULT_INFO_OVER_VOLTAGE		(1 << 7)
+
+/* Boost register */
+#define AS_BOOST_REG				0x0d
+#define AS_BOOST_CURRENT_DISABLE		(0 << 0)
+#define AS_BOOST_CURRENT_ENABLE			(1 << 0)
+
+/* Password register is used to unlock boost register writing */
+#define AS_PASSWORD_REG				0x0f
+#define AS_PASSWORD_UNLOCK_VALUE		0x55
+
+#define AS_NAME					"as3645a"
+#define AS_I2C_ADDR				(0x60 >> 1) /* W:0x60, R:0x61 */
+
+#define AS_FLASH_TIMEOUT_MIN			100000	/* us */
+#define AS_FLASH_TIMEOUT_MAX			850000
+#define AS_FLASH_TIMEOUT_STEP			50000
+
+#define AS_FLASH_INTENSITY_MIN			200000	/* uA */
+#define AS_FLASH_INTENSITY_MAX_1LED		500000
+#define AS_FLASH_INTENSITY_MAX_2LEDS		400000
+#define AS_FLASH_INTENSITY_STEP			20000
+
+#define AS_TORCH_INTENSITY_MIN			20000	/* uA */
+#define AS_TORCH_INTENSITY_MAX			160000
+#define AS_TORCH_INTENSITY_STEP			20000
+
+#define AS_INDICATOR_INTENSITY_MIN		0	/* uA */
+#define AS_INDICATOR_INTENSITY_MAX		10000
+#define AS_INDICATOR_INTENSITY_STEP		2500
+
+#define AS_PEAK_mA_MAX				2000
+#define AS_PEAK_mA_TO_REG(a) \
+	((min_t(u32, AS_PEAK_mA_MAX, a) - 1250) / 250)
+
+enum as_mode {
+	AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT,
+};
+
+struct as3645a_config {
+	u32 flash_timeout_us;
+	u32 flash_max_ua;
+	u32 assist_max_ua;
+	u32 indicator_max_ua;
+	u32 voltage_reference;
+	u32 peak;
+};
+
+struct as3645a {
+	struct i2c_client *client;
+
+	struct mutex mutex;
+
+	struct led_classdev_flash fled;
+	struct led_classdev iled_cdev;
+
+	struct v4l2_flash *vf;
+
+	struct as3645a_config cfg;
+
+	enum as_mode mode;
+	unsigned int timeout;
+	unsigned int flash_current;
+	unsigned int assist_current;
+	unsigned int indicator_current;
+	enum v4l2_flash_strobe_source strobe_source;
+};
+
+#define fled_to_as3645a(__fled) container_of(__fled, struct as3645a, fled)
+#define iled_cdev_to_as3645a(__iled_cdev) \
+	container_of(__iled_cdev, struct as3645a, iled_cdev)
+
+/* Return negative errno else zero on success */
+static int as3645a_write(struct as3645a *flash, u8 addr, u8 val)
+{
+	struct i2c_client *client = flash->client;
+	int rval;
+
+	rval = i2c_smbus_write_byte_data(client, addr, val);
+
+	dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val,
+		rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+/* Return negative errno else a data byte received from the device. */
+static int as3645a_read(struct as3645a *flash, u8 addr)
+{
+	struct i2c_client *client = flash->client;
+	int rval;
+
+	rval = i2c_smbus_read_byte_data(client, addr);
+
+	dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval,
+		rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+/* -----------------------------------------------------------------------------
+ * Hardware configuration and trigger
+ */
+
+/**
+ * as3645a_set_config - Set flash configuration registers
+ * @flash: The flash
+ *
+ * Configure the hardware with flash, assist and indicator currents, as well as
+ * flash timeout.
+ *
+ * Return 0 on success, or a negative error code if an I2C communication error
+ * occurred.
+ */
+static int as3645a_set_current(struct as3645a *flash)
+{
+	u8 val;
+
+	val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT)
+	    | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT)
+	    | AS_CURRENT_LED_DET_ON;
+
+	return as3645a_write(flash, AS_CURRENT_SET_REG, val);
+}
+
+static int as3645a_set_timeout(struct as3645a *flash)
+{
+	u8 val;
+
+	val = flash->timeout << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT;
+
+	val |= (flash->cfg.voltage_reference
+		<< AS_INDICATOR_AND_TIMER_VREF_SHIFT)
+	    |  ((flash->indicator_current ? flash->indicator_current - 1 : 0)
+		 << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT);
+
+	return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val);
+}
+
+/**
+ * as3645a_set_control - Set flash control register
+ * @flash: The flash
+ * @mode: Desired output mode
+ * @on: Desired output state
+ *
+ * Configure the hardware with output mode and state.
+ *
+ * Return 0 on success, or a negative error code if an I2C communication error
+ * occurred.
+ */
+static int
+as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on)
+{
+	u8 reg;
+
+	/* Configure output parameters and operation mode. */
+	reg = (flash->cfg.peak << AS_CONTROL_COIL_PEAK_SHIFT)
+	    | (on ? AS_CONTROL_OUT_ON : 0)
+	    | mode;
+
+	if (mode == AS_MODE_FLASH &&
+	    flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL)
+		reg |= AS_CONTROL_STROBE_TYPE_LEVEL
+		    |  AS_CONTROL_STROBE_ON;
+
+	return as3645a_write(flash, AS_CONTROL_REG, reg);
+}
+
+static int as3645a_get_fault(struct led_classdev_flash *fled, u32 *fault)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+	int rval;
+
+	/* NOTE: reading register clears fault status */
+	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
+	if (rval < 0)
+		return rval;
+
+	if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT)
+		*fault |= LED_FAULT_OVER_CURRENT;
+
+	if (rval & AS_FAULT_INFO_INDICATOR_LED)
+		*fault |= LED_FAULT_INDICATOR;
+
+	dev_dbg(&flash->client->dev, "%u connected LEDs\n",
+		rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1);
+
+	if (rval & AS_FAULT_INFO_TIMEOUT)
+		*fault |= LED_FAULT_TIMEOUT;
+
+	if (rval & AS_FAULT_INFO_OVER_TEMPERATURE)
+		*fault |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (rval & AS_FAULT_INFO_SHORT_CIRCUIT)
+		*fault |= LED_FAULT_OVER_CURRENT;
+
+	if (rval & AS_FAULT_INFO_OVER_VOLTAGE)
+		*fault |= LED_FAULT_INPUT_VOLTAGE;
+
+	return rval;
+}
+
+static unsigned int __as3645a_current_to_reg(unsigned int min, unsigned int max,
+					     unsigned int step,
+					     unsigned int val)
+{
+	if (val < min)
+		val = min;
+
+	if (val > max)
+		val = max;
+
+	return (val - min) / step;
+}
+
+static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
+					   unsigned int ua)
+{
+	if (is_flash)
+		return __as3645a_current_to_reg(AS_TORCH_INTENSITY_MIN,
+						flash->cfg.assist_max_ua,
+						AS_TORCH_INTENSITY_STEP, ua);
+	else
+		return __as3645a_current_to_reg(AS_FLASH_INTENSITY_MIN,
+						flash->cfg.flash_max_ua,
+						AS_FLASH_INTENSITY_STEP, ua);
+}
+
+static int as3645a_set_indicator_brightness(struct led_classdev *iled_cdev,
+					    enum led_brightness brightness)
+{
+	struct as3645a *flash = iled_cdev_to_as3645a(iled_cdev);
+	int rval;
+
+	flash->indicator_current = brightness;
+
+	rval = as3645a_set_timeout(flash);
+	if (rval)
+		return rval;
+
+	return as3645a_set_control(flash, AS_MODE_INDICATOR, brightness);
+}
+
+static int as3645a_set_assist_brightness(struct led_classdev *fled_cdev,
+					 enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled = lcdev_to_flcdev(fled_cdev);
+	struct as3645a *flash = fled_to_as3645a(fled);
+	int rval;
+
+	if (brightness) {
+		/* Register value 0 is 20 mA. */
+		flash->assist_current = brightness - 1;
+
+		rval = as3645a_set_current(flash);
+		if (rval)
+			return rval;
+	}
+
+	return as3645a_set_control(flash, AS_MODE_ASSIST, brightness);
+}
+
+static int as3645a_set_flash_brightness(struct led_classdev_flash *fled,
+					u32 brightness_ua)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	flash->flash_current = as3645a_current_to_reg(flash, true, brightness_ua);
+
+	return as3645a_set_current(flash);
+}
+
+static int as3645a_set_flash_timeout(struct led_classdev_flash *fled,
+				     u32 timeout_us)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	flash->timeout = AS_TIMER_US_TO_CODE(timeout_us);
+
+	return as3645a_set_timeout(flash);
+}
+
+static int as3645a_set_strobe(struct led_classdev_flash *fled, bool state)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	return as3645a_set_control(flash, AS_MODE_FLASH, state);
+}
+
+static const struct led_flash_ops as3645a_led_flash_ops = {
+	.flash_brightness_set = as3645a_set_flash_brightness,
+	.timeout_set = as3645a_set_flash_timeout,
+	.strobe_set = as3645a_set_strobe,
+	.fault_get = as3645a_get_fault,
+};
+
+static int as3645a_setup(struct as3645a *flash)
+{
+	struct device *dev = &flash->client->dev;
+	u32 fault = 0;
+	int rval;
+
+	/* clear errors */
+	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
+	if (rval < 0)
+		return rval;
+
+	dev_dbg(dev, "Fault info: %02x\n", rval);
+
+	rval = as3645a_set_current(flash);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_set_timeout(flash);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_set_control(flash, AS_MODE_INDICATOR, false);
+	if (rval < 0)
+		return rval;
+
+	/* read status */
+	rval = as3645a_get_fault(&flash->fled, &fault);
+	if (rval < 0)
+		return rval;
+
+	dev_dbg(dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n",
+		as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG));
+	dev_dbg(dev, "AS_CURRENT_SET_REG: %02x\n",
+		as3645a_read(flash, AS_CURRENT_SET_REG));
+	dev_dbg(dev, "AS_CONTROL_REG: %02x\n",
+		as3645a_read(flash, AS_CONTROL_REG));
+
+	return rval & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
+}
+
+static int as3645a_detect(struct as3645a *flash)
+{
+	struct device *dev = &flash->client->dev;
+	int rval, man, model, rfu, version;
+	const char *vendor;
+
+	rval = as3645a_read(flash, AS_DESIGN_INFO_REG);
+	if (rval < 0) {
+		dev_err(dev, "can't read design info reg\n");
+		return rval;
+	}
+
+	man = AS_DESIGN_INFO_FACTORY(rval);
+	model = AS_DESIGN_INFO_MODEL(rval);
+
+	rval = as3645a_read(flash, AS_VERSION_CONTROL_REG);
+	if (rval < 0) {
+		dev_err(dev, "can't read version control reg\n");
+		return rval;
+	}
+
+	rfu = AS_VERSION_CONTROL_RFU(rval);
+	version = AS_VERSION_CONTROL_VERSION(rval);
+
+	/* Verify the chip model and version. */
+	if (model != 0x01 || rfu != 0x00) {
+		dev_err(dev, "AS3645A not detected "
+			"(model %d rfu %d)\n", model, rfu);
+		return -ENODEV;
+	}
+
+	switch (man) {
+	case 1:
+		vendor = "AMS, Austria Micro Systems";
+		break;
+	case 2:
+		vendor = "ADI, Analog Devices Inc.";
+		break;
+	case 3:
+		vendor = "NSC, National Semiconductor";
+		break;
+	case 4:
+		vendor = "NXP";
+		break;
+	case 5:
+		vendor = "TI, Texas Instrument";
+		break;
+	default:
+		vendor = "Unknown";
+	}
+
+	dev_info(dev, "Chip vendor: %s (%d) Version: %d\n", vendor,
+		 man, version);
+
+	rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE);
+	if (rval < 0)
+		return rval;
+
+	return as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE);
+}
+
+static __maybe_unused int as3645a_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as3645a *flash = i2c_get_clientdata(client);
+	int rval;
+
+	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
+
+	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
+
+	return rval;
+}
+
+static __maybe_unused int as3645a_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as3645a *flash = i2c_get_clientdata(client);
+	int rval;
+
+	rval = as3645a_setup(flash);
+
+	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+static int as3645a_parse_node(struct as3645a *flash,
+			      struct device_node *node)
+{
+	struct as3645a_config *cfg = &flash->cfg;
+	struct device_node *child;
+	int rval;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child) {
+		dev_err(&flash->client->dev, "can't find flash node\n");
+		return -ENODEV;
+	}
+
+	rval = of_property_read_u32(child, "flash-timeout-us",
+				   &cfg->flash_timeout_us);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read flash-timeout-us property for flash\n");
+		goto out_err;
+	}
+
+	rval = of_property_read_u32(child, "flash-max-microamp",
+				   &cfg->flash_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read flash-max-microamp property for flash\n");
+		goto out_err;
+	}
+
+	rval = of_property_read_u32(child, "led-max-microamp",
+				   &cfg->assist_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read led-max-microamp property for flash\n");
+		goto out_err;
+	}
+
+	of_property_read_u32(child, "voltage-reference",
+			     &cfg->voltage_reference);
+
+	of_property_read_u32(child, "peak-current-limit", &cfg->peak);
+	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
+
+	of_node_put(child);
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child) {
+		dev_warn(&flash->client->dev,
+			 "can't find indicator node\n");
+		return 0;
+	}
+
+	rval = of_property_read_u32(child, "led-max-microamp",
+				   &cfg->indicator_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read led-max-microamp property for indicator\n");
+		goto out_err;
+	}
+
+	of_node_put(child);
+
+	return 0;
+
+out_err:
+	of_node_put(child);
+
+	return rval;
+}
+
+static int as3645a_led_class_setup(struct as3645a *flash)
+{
+	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
+	struct led_classdev *iled_cdev = &flash->iled_cdev;
+	struct led_flash_setting *cfg;
+	int rval;
+
+	iled_cdev->name = "as3645a indicator";
+	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
+	iled_cdev->max_brightness =
+		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
+
+	rval = led_classdev_register(&flash->client->dev, iled_cdev);
+	if (rval < 0)
+		return rval;
+
+	cfg = &flash->fled.brightness;
+	cfg->min = AS_FLASH_INTENSITY_MIN;
+	cfg->max = flash->cfg.flash_max_ua;
+	cfg->step = AS_FLASH_INTENSITY_STEP;
+	cfg->val = flash->cfg.flash_max_ua;
+
+	cfg = &flash->fled.timeout;
+	cfg->min = AS_FLASH_TIMEOUT_MIN;
+	cfg->max = flash->cfg.flash_timeout_us;
+	cfg->step = AS_FLASH_TIMEOUT_STEP;
+	cfg->val = flash->cfg.flash_timeout_us;
+
+	flash->fled.ops = &as3645a_led_flash_ops;
+
+	fled_cdev->name = "as3645a flash";
+	fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness;
+	/* Value 0 is off in LED class. */
+	fled_cdev->max_brightness =
+		as3645a_current_to_reg(flash, false,
+				       flash->cfg.assist_max_ua) + 1;
+	fled_cdev->flags = LED_DEV_CAP_FLASH;
+
+	rval = led_classdev_flash_register(&flash->client->dev, &flash->fled);
+	if (rval) {
+		led_classdev_unregister(iled_cdev);
+		dev_err(&flash->client->dev,
+			"led_classdev_flash_register() failed, error %d\n",
+			rval);
+	}
+
+	return rval;
+}
+
+static int as3645a_v4l2_setup(struct as3645a *flash)
+{
+	struct led_classdev_flash *fled = &flash->fled;
+	struct led_classdev *led = &fled->led_cdev;
+	struct v4l2_flash_config cfg = {
+		.torch_intensity = {
+			.min = AS_TORCH_INTENSITY_MIN,
+			.max = flash->cfg.assist_max_ua,
+			.step = AS_TORCH_INTENSITY_STEP,
+			.val = flash->cfg.assist_max_ua,
+		},
+		.indicator_intensity = {
+			.min = AS_INDICATOR_INTENSITY_MIN,
+			.max = flash->cfg.indicator_max_ua,
+			.step = AS_INDICATOR_INTENSITY_STEP,
+			.val = flash->cfg.indicator_max_ua,
+		},
+	};
+
+	strlcpy(cfg.dev_name, led->name, sizeof(cfg.dev_name));
+
+	flash->vf = v4l2_flash_init(&flash->client->dev, NULL, &flash->fled,
+				    &flash->iled_cdev, NULL, &cfg);
+	if (IS_ERR(flash->vf))
+		return PTR_ERR(flash->vf);
+
+	return 0;
+}
+
+static int as3645a_probe(struct i2c_client *client)
+{
+	struct as3645a *flash;
+	int rval;
+
+	if (client->dev.of_node == NULL)
+		return -ENODEV;
+
+	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
+	if (flash == NULL)
+		return -ENOMEM;
+
+	flash->client = client;
+
+	rval = as3645a_parse_node(flash, client->dev.of_node);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_detect(flash);
+	if (rval < 0)
+		return rval;
+
+	mutex_init(&flash->mutex);
+	i2c_set_clientdata(client, flash);
+
+	rval = as3645a_setup(flash);
+	if (rval)
+		goto out_mutex_destroy;
+
+	rval = as3645a_led_class_setup(flash);
+	if (rval)
+		goto out_mutex_destroy;
+
+	rval = as3645a_v4l2_setup(flash);
+	if (rval)
+		goto out_led_classdev_flash_unregister;
+
+	return 0;
+
+out_led_classdev_flash_unregister:
+	led_classdev_flash_unregister(&flash->fled);
+
+out_mutex_destroy:
+	mutex_destroy(&flash->mutex);
+
+	return rval;
+}
+
+static int as3645a_remove(struct i2c_client *client)
+{
+	struct as3645a *flash = i2c_get_clientdata(client);
+
+	as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
+
+	v4l2_flash_release(flash->vf);
+
+	led_classdev_flash_unregister(&flash->fled);
+	led_classdev_unregister(&flash->iled_cdev);
+
+	mutex_destroy(&flash->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id as3645a_of_table[] = {
+	{ .compatible = "ams,as3645a" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, as3645a_of_table);
+
+SIMPLE_DEV_PM_OPS(as3645a_pm_ops, as3645a_resume, as3645a_suspend);
+
+static struct i2c_driver as3645a_i2c_driver = {
+	.driver	= {
+		.of_match_table = as3645a_of_table,
+		.name = AS_NAME,
+		.pm   = &as3645a_pm_ops,
+	},
+	.probe_new	= as3645a_probe,
+	.remove	= as3645a_remove,
+};
+
+module_i2c_driver(as3645a_i2c_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_AUTHOR("Sakari Ailus <sakari.ailus@iki.fi>");
+MODULE_DESCRIPTION("LED flash driver for AS3645A, LM3555 and their clones");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

[RFC 06/19] leds: as3645a: Separate flash and indicator LED sub-devices

[Sakari Ailus]

To be merged to the as3645a driver patch.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/leds/leds-as3645a.c | 64 +++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
index b1dc32a3c620..9df480cea160 100644
--- a/drivers/leds/leds-as3645a.c
+++ b/drivers/leds/leds-as3645a.c
@@ -138,6 +138,10 @@ struct as3645a {
 	struct led_classdev iled_cdev;
 
 	struct v4l2_flash *vf;
+	struct v4l2_flash *vfind;
+
+	struct device_node *flash_node;
+	struct device_node *indicator_node;
 
 	struct as3645a_config cfg;
 
@@ -508,16 +512,15 @@ static int as3645a_parse_node(struct as3645a *flash,
 			      struct device_node *node)
 {
 	struct as3645a_config *cfg = &flash->cfg;
-	struct device_node *child;
 	int rval;
 
-	child = of_get_child_by_name(node, "flash");
-	if (!child) {
+	flash->flash_node = of_get_child_by_name(node, "flash");
+	if (!flash->flash_node) {
 		dev_err(&flash->client->dev, "can't find flash node\n");
 		return -ENODEV;
 	}
 
-	rval = of_property_read_u32(child, "flash-timeout-us",
+	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
 				   &cfg->flash_timeout_us);
 	if (rval < 0) {
 		dev_err(&flash->client->dev,
@@ -525,7 +528,7 @@ static int as3645a_parse_node(struct as3645a *flash,
 		goto out_err;
 	}
 
-	rval = of_property_read_u32(child, "flash-max-microamp",
+	rval = of_property_read_u32(flash->flash_node, "flash-max-microamp",
 				   &cfg->flash_max_ua);
 	if (rval < 0) {
 		dev_err(&flash->client->dev,
@@ -533,7 +536,7 @@ static int as3645a_parse_node(struct as3645a *flash,
 		goto out_err;
 	}
 
-	rval = of_property_read_u32(child, "led-max-microamp",
+	rval = of_property_read_u32(flash->flash_node, "led-max-microamp",
 				   &cfg->assist_max_ua);
 	if (rval < 0) {
 		dev_err(&flash->client->dev,
@@ -541,22 +544,21 @@ static int as3645a_parse_node(struct as3645a *flash,
 		goto out_err;
 	}
 
-	of_property_read_u32(child, "voltage-reference",
+	of_property_read_u32(flash->flash_node, "voltage-reference",
 			     &cfg->voltage_reference);
 
-	of_property_read_u32(child, "peak-current-limit", &cfg->peak);
+	of_property_read_u32(flash->flash_node, "peak-current-limit",
+			     &cfg->peak);
 	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
 
-	of_node_put(child);
-
-	child = of_get_child_by_name(node, "indicator");
-	if (!child) {
+	flash->indicator_node = of_get_child_by_name(node, "indicator");
+	if (!flash->indicator_node) {
 		dev_warn(&flash->client->dev,
 			 "can't find indicator node\n");
-		return 0;
+		goto out_err;
 	}
 
-	rval = of_property_read_u32(child, "led-max-microamp",
+	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
 				   &cfg->indicator_max_ua);
 	if (rval < 0) {
 		dev_err(&flash->client->dev,
@@ -564,12 +566,11 @@ static int as3645a_parse_node(struct as3645a *flash,
 		goto out_err;
 	}
 
-	of_node_put(child);
-
 	return 0;
 
 out_err:
-	of_node_put(child);
+	of_node_put(flash->flash_node);
+	of_node_put(flash->indicator_node);
 
 	return rval;
 }
@@ -628,13 +629,15 @@ static int as3645a_v4l2_setup(struct as3645a *flash)
 	struct led_classdev_flash *fled = &flash->fled;
 	struct led_classdev *led = &fled->led_cdev;
 	struct v4l2_flash_config cfg = {
-		.torch_intensity = {
+		.intensity = {
 			.min = AS_TORCH_INTENSITY_MIN,
 			.max = flash->cfg.assist_max_ua,
 			.step = AS_TORCH_INTENSITY_STEP,
 			.val = flash->cfg.assist_max_ua,
 		},
-		.indicator_intensity = {
+	};
+	struct v4l2_flash_config cfgind = {
+		.intensity = {
 			.min = AS_INDICATOR_INTENSITY_MIN,
 			.max = flash->cfg.indicator_max_ua,
 			.step = AS_INDICATOR_INTENSITY_STEP,
@@ -643,12 +646,22 @@ static int as3645a_v4l2_setup(struct as3645a *flash)
 	};
 
 	strlcpy(cfg.dev_name, led->name, sizeof(cfg.dev_name));
+	strlcpy(cfgind.dev_name, flash->iled_cdev.name, sizeof(cfg.dev_name));
 
-	flash->vf = v4l2_flash_init(&flash->client->dev, NULL, &flash->fled,
-				    &flash->iled_cdev, NULL, &cfg);
+	flash->vf = v4l2_flash_init(
+		&flash->client->dev, of_fwnode_handle(flash->flash_node),
+		&flash->fled, NULL, &cfg);
 	if (IS_ERR(flash->vf))
 		return PTR_ERR(flash->vf);
 
+	flash->vfind = v4l2_flash_indicator_init(
+		&flash->client->dev, of_fwnode_handle(flash->indicator_node),
+		&flash->iled_cdev, &cfgind);
+	if (IS_ERR(flash->vfind)) {
+		v4l2_flash_release(flash->vf);
+		return PTR_ERR(flash->vfind);
+	}
+
 	return 0;
 }
 
@@ -672,7 +685,7 @@ static int as3645a_probe(struct i2c_client *client)
 
 	rval = as3645a_detect(flash);
 	if (rval < 0)
-		return rval;
+		goto out_put_nodes;
 
 	mutex_init(&flash->mutex);
 	i2c_set_clientdata(client, flash);
@@ -697,6 +710,10 @@ static int as3645a_probe(struct i2c_client *client)
 out_mutex_destroy:
 	mutex_destroy(&flash->mutex);
 
+out_put_nodes:
+	of_node_put(flash->flash_node);
+	of_node_put(flash->indicator_node);
+
 	return rval;
 }
 
@@ -713,6 +730,9 @@ static int as3645a_remove(struct i2c_client *client)
 
 	mutex_destroy(&flash->mutex);
 
+	of_node_put(flash->flash_node);
+	of_node_put(flash->indicator_node);
+
 	return 0;
 }
 
-- 
2.11.0

[RFC 07/19] v4l: fwnode: Support generic parsing of graph endpoints in V4L2

[Sakari Ailus]

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.

Convert the omap3isp as an example.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 88 +++++++++-----------------------
 drivers/media/platform/omap3isp/isp.h |  3 --
 drivers/media/v4l2-core/v4l2-fwnode.c | 94 +++++++++++++++++++++++++++++++++++
 include/media/v4l2-async.h            |  4 +-
 include/media/v4l2-fwnode.h           |  9 ++++
 5 files changed, 129 insertions(+), 69 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 9df64c189883..92245a457d18 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2008,43 +2008,40 @@ 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;
-
-	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
-	if (ret)
-		return ret;
 
 	dev_dbg(dev, "parsing endpoint %s, interface %u\n",
-		to_of_node(fwnode)->full_name, vep.base.port);
+		to_of_node(vep->base.local_fwnode)->full_name, 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);
+			!!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
 		break;
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:
 		/* FIXME: always assume CSI-2 for now. */
-		switch (vep.base.port) {
+		switch (vep->base.port) {
 		case ISP_OF_PHY_CSIPHY1:
 			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
 			break;
@@ -2052,18 +2049,18 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
 			break;
 		}
-		buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
+		buscfg->bus.csi2.lanecfg.clk.pos = 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);
 
 		for (i = 0; i < ISP_CSIPHY2_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,
 				buscfg->bus.csi2.lanecfg.data[i].pos);
@@ -2079,55 +2076,14 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 
 	default:
 		dev_warn(dev, "%s: invalid interface %u\n",
-			 to_of_node(fwnode)->full_name, vep.base.port);
+			 to_of_node(vep->base.local_fwnode)->full_name,
+			 vep->base.port);
 		break;
 	}
 
 	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;
-
-		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
-
-		if (isp_fwnode_parse(dev, fwnode, isd))
-			goto error;
-
-		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_bound(struct v4l2_async_notifier *async,
 				     struct v4l2_subdev *subdev,
 				     struct v4l2_async_subdev *asd)
@@ -2210,7 +2166,9 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
+	ret = v4l2_fwnode_endpoints_parse(
+		&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
+		isp_fwnode_parse);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 2f2ae609c548..a852c1168d20 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -220,9 +220,6 @@ 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 {
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d4d7537011da..c3ad9e31e4cb 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -26,6 +26,7 @@
 #include <linux/string.h>
 #include <linux/types.h>
 
+#include <media/v4l2-async.h>
 #include <media/v4l2-fwnode.h>
 
 static int v4l2_fwnode_endpoint_parse_csi_bus(
@@ -339,6 +340,99 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+static int notifier_realloc(struct device *dev,
+			    struct v4l2_async_notifier *notifier,
+			    unsigned int max_subdevs)
+{
+	struct v4l2_async_subdev **subdevs;
+	unsigned int i;
+
+	if (max_subdevs <= notifier->max_subdevs)
+		return 0;
+
+	subdevs = devm_kcalloc(
+		dev, max_subdevs, sizeof(*notifier->subdevs), GFP_KERNEL);
+	if (!subdevs)
+		return -ENOMEM;
+
+	if (notifier->subdevs) {
+		for (i = 0; i < notifier->num_subdevs; i++)
+			subdevs[i] = notifier->subdevs[i];
+
+		devm_kfree(dev, notifier->subdevs);
+	}
+
+	notifier->subdevs = subdevs;
+	notifier->max_subdevs = max_subdevs;
+
+	return 0;
+}
+
+int v4l2_fwnode_endpoints_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	size_t asd_struct_size,
+	int (*parse_single)(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;
+
+	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
+							fwnode)))
+		max_subdevs++;
+
+	ret = notifier_realloc(dev, notifier, max_subdevs);
+	if (ret)
+		return ret;
+
+	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
+				     dev_fwnode(dev), fwnode)) &&
+		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
+		) {
+		struct v4l2_fwnode_endpoint *vep;
+		struct v4l2_async_subdev *asd;
+
+		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
+		if (!asd) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		notifier->subdevs[notifier->num_subdevs] = asd;
+
+		/* Ignore endpoints the parsing of which failed. */
+		vep = v4l2_fwnode_endpoint_alloc_parse(fwnode);
+		if (IS_ERR(vep))
+			continue;
+
+		ret = parse_single(dev, vep, asd);
+		v4l2_fwnode_endpoint_free(vep);
+		if (ret)
+			goto error;
+
+		asd->match.fwnode.fwnode =
+			fwnode_graph_get_remote_port_parent(fwnode);
+		if (!asd->match.fwnode.fwnode) {
+			dev_warn(dev, "bad remote port parent\n");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+		notifier->num_subdevs++;
+	}
+
+error:
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_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-async.h b/include/media/v4l2-async.h
index 8c7519fce5b9..54eecf9c9d2f 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -78,7 +78,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 subdevs array
+ * @max_subdevs: number of subdevices allocated in 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 +91,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;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index bdbd785c3d38..6ba1a0bbc328 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;
 
 /**
  * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
@@ -101,4 +103,11 @@ int v4l2_fwnode_parse_link(const struct fwnode_handle *fwnode,
 			   struct v4l2_fwnode_link *link);
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+int v4l2_fwnode_endpoints_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd));
+
 #endif /* _V4L2_FWNODE_H */
-- 
2.11.0

[RFC 08/19] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

Add the as3645a flash controller to the DT source as well as the flash
property with the as3645a device phandle to the sensor DT node.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/omap3-n9.dts       |  1 +
 arch/arm/boot/dts/omap3-n950-n9.dtsi | 14 ++++++++++++++
 arch/arm/boot/dts/omap3-n950.dts     |  1 +
 3 files changed, 16 insertions(+)

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 df3366fa5409..e15722b83a70 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -265,6 +265,20 @@
 
 &i2c2 {
 	clock-frequency = <400000>;
+
+	as3645a: flash@30 {
+		reg = <0x30>;
+		compatible = "ams,as3645a";
+		as3645a_flash: flash {
+			flash-timeout-us = <150000>;
+			flash-max-microamp = <320000>;
+			led-max-microamp = <60000>;
+			peak-current-limit = <1750000>;
+		};
+		as3645a_indicator: indicator {
+			led-max-microamp = <10000>;
+		};
+	};
 };
 
 &i2c3 {
diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
index 646601a3ebd8..8fca0384d2df 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>;
 		port {
 			smia_1_1: endpoint {
 				link-frequencies = /bits/ 64 <210000000 333600000 398400000>;
-- 
2.11.0

[RFC 09/19] v4l2-fwnode: Add conveniences function for parsing generic references

[Sakari Ailus]

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 | 53 +++++++++++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  8 ++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index c3ad9e31e4cb..bfc9e38766f3 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -433,6 +433,59 @@ int v4l2_fwnode_endpoints_parse(
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
 
+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 max_subdevs = notifier->max_subdevs;
+	int ret;
+
+	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
+		return -EINVAL;
+
+	while (!fwnode_property_get_reference_args(
+		       dev_fwnode(dev), prop, nargs_prop, nargs,
+		       max_subdevs - notifier->max_subdevs, NULL))
+		max_subdevs++;
+
+	ret = notifier_realloc(dev, notifier, max_subdevs);
+	if (ret)
+		return ret;
+
+	for (ret = -ENOENT; !fwnode_property_get_reference_args(
+				     dev_fwnode(dev), prop, nargs_prop, nargs,
+				     notifier->num_subdevs, &args) &&
+		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
+	     notifier->num_subdevs++) {
+		struct v4l2_async_subdev *asd;
+
+		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
+		if (!asd) {
+			fwnode_handle_put(args.fwnode);
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		ret = parse_single ? parse_single(dev, &args, asd) : 0;
+		fwnode_handle_put(args.fwnode);
+		if (ret)
+			goto error;
+
+		notifier->subdevs[notifier->num_subdevs] = asd;
+		asd->match.fwnode.fwnode = args.fwnode;
+		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+	}
+
+error:
+	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 6ba1a0bbc328..e27526bd744d 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -110,4 +110,12 @@ int v4l2_fwnode_endpoints_parse(
 			    struct v4l2_fwnode_endpoint *vep,
 			    struct v4l2_async_subdev *asd));
 
+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

[RFC 10/19] v4l2-fwnode: Add convenience function for parsing common external refs

[Sakari Ailus]

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 bfc9e38766f3..8671262eb22c 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -486,6 +486,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 e27526bd744d..8cd4f8a75c3d 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -118,4 +118,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

[RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Sakari Ailus]

The async notifier supports three callbacks to the notifier: bound, unbound
and complete. The complete callback has been traditionally used for
creating the sub-device nodes.

This approach has an inherent weakness: if registration of a single
sub-device fails for whatever reason, it renders the entire media device
unusable even if only that piece of hardware is not working. This is a
problem in particular in systems with multiple independent image pipelines
on a single device. We have had such devices (e.g. omap3isp) supported for
a number of years and the problem is growing more pressing as time passes
so there is an incentive to resolve this.

The solution is to register device nodes at the time when the driver of
those devices is complete with initialising the piece of hardware it is
controlling.

This leaves partial pipelines visible to the user. There are two things to
consider here:

1) Registering multiple device nodes was never an atomic operation so the
user space still had to be prepared for partial media graph being visible
and

2) The user space can figure out that a pipeline is not startable from the
fact that there are pads with MEDIA_PAD_FL_MUST_CONNECT flag set without
an (enabled) link.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index d2ce39ac402e..55fa7106345c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -127,19 +127,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 {
 	int ret;
 
-	if (notifier->bound) {
-		ret = notifier->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);
 		return ret;
 	}
 
+	if (notifier->bound) {
+		ret = notifier->bound(notifier, sd, asd);
+		if (ret < 0) {
+			v4l2_device_unregister_subdev(sd);
+			return ret;
+		}
+	}
+
 	/* Remove from the waiting list */
 	list_del(&asd->list);
 	sd->asd = asd;
-- 
2.11.0

[RFC 12/19] v4l2-subdev: Support registering V4L2 sub-device nodes one by one

[Sakari Ailus]

Just like video devices, V4L2 sub-device nodes can and should be registered
as soon as possible. Support this by providing
v4l2_device_register_subdev_node() function.
v4l2_device_register_subdev_nodes() continues to work just as it used to.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 99 ++++++++++++++++++++---------------
 include/media/v4l2-device.h           | 12 +++++
 2 files changed, 70 insertions(+), 41 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 937c6de85606..0c0c4772c00a 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -222,58 +222,75 @@ static void v4l2_device_release_subdev_node(struct video_device *vdev)
 	kfree(vdev);
 }
 
-int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+int v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+				     struct v4l2_subdev *sd)
 {
 	struct video_device *vdev;
-	struct v4l2_subdev *sd;
 	int err;
 
-	/* Register a device node for every subdev marked with the
-	 * V4L2_SUBDEV_FL_HAS_DEVNODE flag.
-	 */
-	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE))
-			continue;
+	/* Bail out if the V4L2_SUBDEV_FL_HAS_DEVNODE flag isn't set. */
+	if (!(sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE))
+		return 0;
 
-		if (sd->devnode)
-			continue;
+	/* Was the device node already registered? If yes, then return here. */
+	if (sd->devnode)
+		return 0;
 
-		vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
-		if (!vdev) {
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	video_set_drvdata(vdev, sd);
+	strlcpy(vdev->name, sd->name, sizeof(vdev->name));
+	vdev->v4l2_dev = v4l2_dev;
+	vdev->fops = &v4l2_subdev_fops;
+	vdev->release = v4l2_device_release_subdev_node;
+	vdev->ctrl_handler = sd->ctrl_handler;
+	err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
+				      sd->owner);
+	if (err < 0)
+		goto clean_up;
+
+	sd->devnode = vdev;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	sd->entity.info.dev.major = VIDEO_MAJOR;
+	sd->entity.info.dev.minor = vdev->minor;
+
+	/* Interface is created by __video_register_device() */
+	if (vdev->v4l2_dev->mdev) {
+		struct media_link *link;
+
+		link = media_create_intf_link(&sd->entity,
+					      &vdev->intf_devnode->intf,
+					      MEDIA_LNK_FL_ENABLED);
+		if (!link) {
 			err = -ENOMEM;
 			goto clean_up;
 		}
+	}
+#endif
 
-		video_set_drvdata(vdev, sd);
-		strlcpy(vdev->name, sd->name, sizeof(vdev->name));
-		vdev->v4l2_dev = v4l2_dev;
-		vdev->fops = &v4l2_subdev_fops;
-		vdev->release = v4l2_device_release_subdev_node;
-		vdev->ctrl_handler = sd->ctrl_handler;
-		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
-					      sd->owner);
-		if (err < 0) {
-			kfree(vdev);
+	return 0;
+
+clean_up:
+	video_unregister_device(vdev);
+	kfree(vdev);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_node);
+
+int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd;
+	int err;
+
+	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
+		int err;
+
+		err = v4l2_device_register_subdev_node(v4l2_dev, sd);
+		if (err < 0)
 			goto clean_up;
-		}
-		sd->devnode = vdev;
-#if defined(CONFIG_MEDIA_CONTROLLER)
-		sd->entity.info.dev.major = VIDEO_MAJOR;
-		sd->entity.info.dev.minor = vdev->minor;
-
-		/* Interface is created by __video_register_device() */
-		if (vdev->v4l2_dev->mdev) {
-			struct media_link *link;
-
-			link = media_create_intf_link(&sd->entity,
-						      &vdev->intf_devnode->intf,
-						      MEDIA_LNK_FL_ENABLED);
-			if (!link) {
-				err = -ENOMEM;
-				goto clean_up;
-			}
-		}
-#endif
 	}
 	return 0;
 
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 8ffa94009d1a..8b19c1f5bacd 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -189,6 +189,18 @@ int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
 
 /**
+ * v4l2_device_register_subdev_node - Registers device nodes for a subdev
+ *	of the v4l2 device. The call is a no-op unless the
+ *	%V4L2_SUBDEV_FL_HAS_DEVNODE subdev flag is set.
+ *
+ * @v4l2_dev: pointer to struct v4l2_device
+ * @sd: pointer to struct v4l2_subdev
+ */
+int __must_check
+v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+				 struct v4l2_subdev *sd);
+
+/**
  * v4l2_device_register_subdev_nodes - Registers device nodes for all subdevs
  *	of the v4l2 device that are marked with
  *	the %V4L2_SUBDEV_FL_HAS_DEVNODE flag.
-- 
2.11.0

[RFC 13/19] v4l2-device: Register sub-device nodes at sub-device registration time

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 146 ++++++++++++++++------------------
 include/media/v4l2-device.h           |  12 ---
 2 files changed, 67 insertions(+), 91 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 0c0c4772c00a..4da8f07fc373 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -151,70 +151,6 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister);
 
-int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
-				struct v4l2_subdev *sd)
-{
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_entity *entity = &sd->entity;
-#endif
-	int err;
-
-	/* Check for valid input */
-	if (!v4l2_dev || !sd || sd->v4l2_dev || !sd->name[0])
-		return -EINVAL;
-
-	/*
-	 * The reason to acquire the module here is to avoid unloading
-	 * a module of sub-device which is registered to a media
-	 * device. To make it possible to unload modules for media
-	 * devices that also register sub-devices, do not
-	 * try_module_get() such sub-device owners.
-	 */
-	sd->owner_v4l2_dev = v4l2_dev->dev && v4l2_dev->dev->driver &&
-		sd->owner == v4l2_dev->dev->driver->owner;
-
-	if (!sd->owner_v4l2_dev && !try_module_get(sd->owner))
-		return -ENODEV;
-
-	sd->v4l2_dev = v4l2_dev;
-	/* This just returns 0 if either of the two args is NULL */
-	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
-	if (err)
-		goto error_module;
-
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	/* Register the entity. */
-	if (v4l2_dev->mdev) {
-		err = media_device_register_entity(v4l2_dev->mdev, entity);
-		if (err < 0)
-			goto error_module;
-	}
-#endif
-
-	if (sd->internal_ops && sd->internal_ops->registered) {
-		err = sd->internal_ops->registered(sd);
-		if (err)
-			goto error_unregister;
-	}
-
-	spin_lock(&v4l2_dev->lock);
-	list_add_tail(&sd->list, &v4l2_dev->subdevs);
-	spin_unlock(&v4l2_dev->lock);
-
-	return 0;
-
-error_unregister:
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	media_device_unregister_entity(entity);
-#endif
-error_module:
-	if (!sd->owner_v4l2_dev)
-		module_put(sd->owner);
-	sd->v4l2_dev = NULL;
-	return err;
-}
-EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
-
 static void v4l2_device_release_subdev_node(struct video_device *vdev)
 {
 	struct v4l2_subdev *sd = video_get_drvdata(vdev);
@@ -222,8 +158,8 @@ static void v4l2_device_release_subdev_node(struct video_device *vdev)
 	kfree(vdev);
 }
 
-int v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
-				     struct v4l2_subdev *sd)
+static int v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+					    struct v4l2_subdev *sd)
 {
 	struct video_device *vdev;
 	int err;
@@ -278,31 +214,83 @@ int v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_node);
 
-int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
+				struct v4l2_subdev *sd)
 {
-	struct v4l2_subdev *sd;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct media_entity *entity = &sd->entity;
+#endif
 	int err;
 
-	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		int err;
+	/* Check for valid input */
+	if (!v4l2_dev || !sd || sd->v4l2_dev || !sd->name[0])
+		return -EINVAL;
 
-		err = v4l2_device_register_subdev_node(v4l2_dev, sd);
+	/*
+	 * The reason to acquire the module here is to avoid unloading
+	 * a module of sub-device which is registered to a media
+	 * device. To make it possible to unload modules for media
+	 * devices that also register sub-devices, do not
+	 * try_module_get() such sub-device owners.
+	 */
+	sd->owner_v4l2_dev = v4l2_dev->dev && v4l2_dev->dev->driver &&
+		sd->owner == v4l2_dev->dev->driver->owner;
+
+	if (!sd->owner_v4l2_dev && !try_module_get(sd->owner))
+		return -ENODEV;
+
+	sd->v4l2_dev = v4l2_dev;
+	/* This just returns 0 if either of the two args is NULL */
+	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
+	if (err)
+		goto error_module;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	/* Register the entity. */
+	if (v4l2_dev->mdev) {
+		err = media_device_register_entity(v4l2_dev->mdev, entity);
 		if (err < 0)
-			goto clean_up;
+			goto error_module;
 	}
-	return 0;
+#endif
 
-clean_up:
-	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		if (!sd->devnode)
-			break;
-		video_unregister_device(sd->devnode);
+	if (sd->internal_ops && sd->internal_ops->registered) {
+		err = sd->internal_ops->registered(sd);
+		if (err)
+			goto error_unregister;
 	}
 
+	err = v4l2_device_register_subdev_node(v4l2_dev, sd);
+	if (err)
+		goto error_subdev_node;
+
+	spin_lock(&v4l2_dev->lock);
+	list_add_tail(&sd->list, &v4l2_dev->subdevs);
+	spin_unlock(&v4l2_dev->lock);
+
+	return 0;
+
+error_subdev_node:
+	if (sd->internal_ops && sd->internal_ops->unregistered)
+		sd->internal_ops->unregistered(sd);
+
+error_unregister:
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	media_device_unregister_entity(entity);
+#endif
+error_module:
+	if (!sd->owner_v4l2_dev)
+		module_put(sd->owner);
+	sd->v4l2_dev = NULL;
 	return err;
 }
+EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
+
+int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+{
+	return 0;
+}
 EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_nodes);
 
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 8b19c1f5bacd..8ffa94009d1a 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -189,18 +189,6 @@ int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
 
 /**
- * v4l2_device_register_subdev_node - Registers device nodes for a subdev
- *	of the v4l2 device. The call is a no-op unless the
- *	%V4L2_SUBDEV_FL_HAS_DEVNODE subdev flag is set.
- *
- * @v4l2_dev: pointer to struct v4l2_device
- * @sd: pointer to struct v4l2_subdev
- */
-int __must_check
-v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
-				 struct v4l2_subdev *sd);
-
-/**
  * v4l2_device_register_subdev_nodes - Registers device nodes for all subdevs
  *	of the v4l2 device that are marked with
  *	the %V4L2_SUBDEV_FL_HAS_DEVNODE flag.
-- 
2.11.0

[RFC 14/19] omap3isp: Move sub-device link creation to notifier bound callback

[Sakari Ailus]

The external sub-device links may well be created from the bound callback.
Don't postpone creation to the complete callback.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 92245a457d18..ef6ce2b214ce 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2085,16 +2085,18 @@ static int isp_fwnode_parse(struct device *dev,
 }
 
 static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
+				     struct v4l2_subdev *sd,
 				     struct v4l2_async_subdev *asd)
 {
+	struct isp_device *isp = container_of(async, struct isp_device,
+					      notifier);
 	struct isp_async_subdev *isd =
 		container_of(asd, struct isp_async_subdev, asd);
 
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
+	isd->sd = sd;
+	sd->host_priv = &isd->bus;
 
-	return 0;
+	return isp_link_entity(isp, &sd->entity, isd->bus.interface);
 }
 
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
@@ -2110,16 +2112,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	if (ret)
 		return ret;
 
-	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
-	}
-
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
 	if (ret < 0)
 		return ret;
-- 
2.11.0

[RFC 15/19] omap3isp: Initialise "crashed" media entity enum in probe

[Sakari Ailus]

Initialise the struct isp_device.crashed media entity enum field when the
ISP's local media entities have been registered, in probe. This is to make
sure that the enumeration is initialised and large enough when the media
device is made visible.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index ef6ce2b214ce..90da8343b3dd 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1592,6 +1592,7 @@ static void isp_pm_complete(struct device *dev)
 
 static void isp_unregister_entities(struct isp_device *isp)
 {
+	media_entity_enum_cleanup(&isp->crashed);
 	omap3isp_csi2_unregister_entities(&isp->isp_csi2a);
 	omap3isp_ccp2_unregister_entities(&isp->isp_ccp2);
 	omap3isp_ccdc_unregister_entities(&isp->isp_ccdc);
@@ -1730,6 +1731,10 @@ static int isp_register_entities(struct isp_device *isp)
 	if (ret < 0)
 		goto done;
 
+	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
+	if (ret)
+		return ret;
+
 done:
 	if (ret < 0)
 		isp_unregister_entities(isp);
@@ -1997,8 +2002,6 @@ static int isp_remove(struct platform_device *pdev)
 	isp_detach_iommu(isp);
 	__omap3isp_put(isp, false);
 
-	media_entity_enum_cleanup(&isp->crashed);
-
 	return 0;
 }
 
@@ -2108,10 +2111,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct isp_bus_cfg *bus;
 	int ret;
 
-	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
-	if (ret)
-		return ret;
-
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
 	if (ret < 0)
 		return ret;
-- 
2.11.0

[RFC 16/19] omap3isp: Move media device registration to probe

[Sakari Ailus]

Register the media device in probe, thus making the omap3isp device usable
once the driver is registered.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 90da8343b3dd..68c02ea1fe6f 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1592,6 +1592,7 @@ static void isp_pm_complete(struct device *dev)
 
 static void isp_unregister_entities(struct isp_device *isp)
 {
+	media_device_unregister(&isp->media_dev);
 	media_entity_enum_cleanup(&isp->crashed);
 	omap3isp_csi2_unregister_entities(&isp->isp_csi2a);
 	omap3isp_ccp2_unregister_entities(&isp->isp_ccp2);
@@ -1603,7 +1604,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
 	v4l2_device_unregister(&isp->v4l2_dev);
-	media_device_unregister(&isp->media_dev);
 	media_device_cleanup(&isp->media_dev);
 }
 
@@ -2111,11 +2111,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct isp_bus_cfg *bus;
 	int ret;
 
-	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
-	if (ret < 0)
-		return ret;
-
-	return media_device_register(&isp->media_dev);
+        return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
 }
 
 /*
@@ -2284,6 +2280,10 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_modules;
 
+	ret = media_device_register(&isp->media_dev);
+	if (ret < 0)
+		goto error_media_device;
+
 	ret = isp_create_links(isp);
 	if (ret < 0)
 		goto error_register_entities;
@@ -2301,6 +2301,8 @@ static int isp_probe(struct platform_device *pdev)
 	return 0;
 
 error_register_entities:
+	media_device_unregister(&isp->media_dev);
+error_media_device:
 	isp_unregister_entities(isp);
 error_modules:
 	isp_cleanup_modules(isp);
-- 
2.11.0

[RFC 17/19] omap3isp: Drop the async notifier callback

[Sakari Ailus]

v4l2_device_register_subdev_nodes() is now nop and can be dropped without
side effects. Do so.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 68c02ea1fe6f..ae867eb3fd15 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2102,18 +2102,6 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
 	return isp_link_entity(isp, &sd->entity, isd->bus.interface);
 }
 
-static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
-{
-	struct isp_device *isp = container_of(async, struct isp_device,
-					      notifier);
-	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
-	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
-	int ret;
-
-        return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
-}
-
 /*
  * isp_probe - Probe ISP platform device
  * @pdev: Pointer to ISP platform device
@@ -2289,7 +2277,6 @@ static int isp_probe(struct platform_device *pdev)
 		goto error_register_entities;
 
 	isp->notifier.bound = isp_subdev_notifier_bound;
-	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
 	if (ret)
-- 
2.11.0

[RFC 18/19] v4l2-fwnode: Add abstracted sub-device notifiers

[Sakari Ailus]

Add notifiers for sub-devices. The notifiers themselves are not visible for
the sub-device drivers but instead are accessed through interface functions
v4l2_subdev_fwnode_endpoints_parse() and
v4l2_subdev_fwnode_reference_parse_sensor_common().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-async.c  | 27 +++++++++++++++++----
 drivers/media/v4l2-core/v4l2-subdev.c | 44 +++++++++++++++++++++++++++++++++--
 include/media/v4l2-fwnode.h           |  1 +
 include/media/v4l2-subdev.h           | 11 +++++++++
 4 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 55fa7106345c..411deadf5d85 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -134,10 +134,14 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 
 	if (notifier->bound) {
 		ret = notifier->bound(notifier, sd, asd);
-		if (ret < 0) {
-			v4l2_device_unregister_subdev(sd);
-			return ret;
-		}
+		if (ret < 0)
+			goto err_unregister;
+	}
+
+	if (sd->subnotifier) {
+		ret = v4l2_async_subnotifier_register(sd, sd->subnotifier);
+		if (ret < 0)
+			goto err_unbind;
 	}
 
 	/* Remove from the waiting list */
@@ -152,6 +156,15 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 		return notifier->complete(notifier);
 
 	return 0;
+
+err_unbind:
+	if (notifier->unbind)
+		notifier->unbind(notifier, sd, asd);
+
+err_unregister:
+	v4l2_device_unregister_subdev(sd);
+
+	return ret;
 }
 
 static void v4l2_async_cleanup(struct v4l2_subdev *sd)
@@ -283,6 +296,9 @@ v4l2_async_do_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 (sd->subnotifier)
+			v4l2_async_subnotifier_unregister(sd->subnotifier);
+
 		if (notifier->unbind)
 			notifier->unbind(notifier, sd, sd->asd);
 
@@ -396,6 +412,9 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 
 	v4l2_async_cleanup(sd);
 
+	if (sd->subnotifier)
+		v4l2_async_subnotifier_unregister(sd->subnotifier);
+
 	if (notifier->unbind)
 		notifier->unbind(notifier, sd, sd->asd);
 
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 43fefa73e0a3..a6976d4a52ac 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -25,9 +25,10 @@
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
-#include <media/v4l2-ioctl.h>
-#include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-ioctl.h>
 
 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
@@ -626,3 +627,42 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
+
+static struct v4l2_async_notifier *v4l2_subdev_get_subnotifier(
+	struct v4l2_subdev *sd)
+{
+	if (sd->subnotifier)
+		return sd->subnotifier;
+
+	return (sd->subnotifier = devm_kzalloc(
+			sd->dev, sizeof(*sd->subnotifier), GFP_KERNEL));
+}
+
+int v4l2_subdev_fwnode_endpoints_parse(
+	struct v4l2_subdev *sd,	size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd))
+{
+	struct v4l2_async_notifier *subnotifier =
+		v4l2_subdev_get_subnotifier(sd);
+
+	if (!subnotifier)
+		return -ENOMEM;
+
+	return v4l2_fwnode_endpoints_parse(sd->dev, subnotifier,
+					   asd_struct_size, parse_single);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_endpoints_parse);
+
+int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd)
+{
+	struct v4l2_async_notifier *subnotifier =
+		v4l2_subdev_get_subnotifier(sd);
+
+	if (!subnotifier)
+		return -ENOMEM;
+
+	return v4l2_fwnode_reference_parse_sensor_common(sd->dev, subnotifier);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_reference_parse_sensor_common);
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 8cd4f8a75c3d..0a3f869ead52 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -27,6 +27,7 @@
 struct fwnode_handle;
 struct v4l2_async_notifier;
 struct v4l2_async_subdev;
+struct v4l2_subdev;
 
 /**
  * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 0f92ebd2d710..e309a2e2030b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -43,6 +43,7 @@ struct v4l2_ctrl_handler;
 struct v4l2_event;
 struct v4l2_event_subscription;
 struct v4l2_fh;
+struct v4l2_fwnode_endpoint;
 struct v4l2_subdev;
 struct v4l2_subdev_fh;
 struct tuner_setup;
@@ -793,6 +794,7 @@ struct v4l2_subdev_platform_data {
  *	list.
  * @asd: Pointer to respective &struct v4l2_async_subdev.
  * @notifier: Pointer to the managing notifier.
+ * @subnotifier: Pointer to the async sub-device notifier.
  * @pdata: common part of subdevice platform data
  *
  * Each instance of a subdev driver should create this struct, either
@@ -823,6 +825,7 @@ struct v4l2_subdev {
 	struct list_head async_list;
 	struct v4l2_async_subdev *asd;
 	struct v4l2_async_notifier *notifier;
+	struct v4l2_async_notifier *subnotifier;
 	struct v4l2_subdev_platform_data *pdata;
 };
 
@@ -1001,4 +1004,12 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
 void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 			      const struct v4l2_event *ev);
 
+int v4l2_subdev_fwnode_endpoints_parse(
+	struct v4l2_subdev *sd,	size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd));
+
+int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd);
+
 #endif
-- 
2.11.0

[RFC 19/19] smiapp: Add support for flash and lens devices

[Sakari Ailus]

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e0b0c032c4ac..ae93d81d179c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2879,6 +2879,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_subdev_fwnode_reference_parse_sensor_common(
+		&sensor->src->sd);
+	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");
-- 
2.11.0

Re: [RFC 08/19] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Sakari Ailus]

On Tue, Jul 18, 2017 at 10:03:50PM +0300, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Add the as3645a flash controller to the DT source as well as the flash
> property with the as3645a device phandle to the sensor DT node.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  arch/arm/boot/dts/omap3-n9.dts       |  1 +
>  arch/arm/boot/dts/omap3-n950-n9.dtsi | 14 ++++++++++++++
>  arch/arm/boot/dts/omap3-n950.dts     |  1 +
>  3 files changed, 16 insertions(+)
> 
> 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 df3366fa5409..e15722b83a70 100644
> --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -265,6 +265,20 @@
>  
>  &i2c2 {
>  	clock-frequency = <400000>;
> +
> +	as3645a: flash@30 {
> +		reg = <0x30>;
> +		compatible = "ams,as3645a";
> +		as3645a_flash: flash {
> +			flash-timeout-us = <150000>;
> +			flash-max-microamp = <320000>;
> +			led-max-microamp = <60000>;
> +			peak-current-limit = <1750000>;
> +		};
> +		as3645a_indicator: indicator {
> +			led-max-microamp = <10000>;
> +		};
> +	};
>  };
>  
>  &i2c3 {
> diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
> index 646601a3ebd8..8fca0384d2df 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>;

This one should have mirrored the N9 configuration; I'll fix that for the
next version.

>  		port {
>  			smia_1_1: endpoint {
>  				link-frequencies = /bits/ 64 <210000000 333600000 398400000>;
> -- 
> 2.11.0
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 18/19] v4l2-fwnode: Add abstracted sub-device notifiers

[Niklas Söderlund]

Hi Sakari,

Thanks for your hard work!

On 2017-07-18 22:04:00 +0300, Sakari Ailus wrote:
> Add notifiers for sub-devices. The notifiers themselves are not visible for
> the sub-device drivers but instead are accessed through interface functions

I might be missing it, but I can't find a interface function to set the 
bound()/unbind() callbacks on a sub-notifiers :-) Is this intentional or 
only not present in this RFC?

> v4l2_subdev_fwnode_endpoints_parse() and
> v4l2_subdev_fwnode_reference_parse_sensor_common().


> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c  | 27 +++++++++++++++++----
>  drivers/media/v4l2-core/v4l2-subdev.c | 44 +++++++++++++++++++++++++++++++++--
>  include/media/v4l2-fwnode.h           |  1 +
>  include/media/v4l2-subdev.h           | 11 +++++++++
>  4 files changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 55fa7106345c..411deadf5d85 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -134,10 +134,14 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>  
>  	if (notifier->bound) {
>  		ret = notifier->bound(notifier, sd, asd);
> -		if (ret < 0) {
> -			v4l2_device_unregister_subdev(sd);
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto err_unregister;
> +	}
> +
> +	if (sd->subnotifier) {
> +		ret = v4l2_async_subnotifier_register(sd, sd->subnotifier);
> +		if (ret < 0)
> +			goto err_unbind;
>  	}
>  
>  	/* Remove from the waiting list */
> @@ -152,6 +156,15 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>  		return notifier->complete(notifier);
>  
>  	return 0;
> +
> +err_unbind:
> +	if (notifier->unbind)
> +		notifier->unbind(notifier, sd, asd);
> +
> +err_unregister:
> +	v4l2_device_unregister_subdev(sd);
> +
> +	return ret;
>  }
>  
>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> @@ -283,6 +296,9 @@ v4l2_async_do_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 (sd->subnotifier)
> +			v4l2_async_subnotifier_unregister(sd->subnotifier);
> +
>  		if (notifier->unbind)
>  			notifier->unbind(notifier, sd, sd->asd);
>  
> @@ -396,6 +412,9 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  
>  	v4l2_async_cleanup(sd);
>  
> +	if (sd->subnotifier)
> +		v4l2_async_subnotifier_unregister(sd->subnotifier);
> +
>  	if (notifier->unbind)
>  		notifier->unbind(notifier, sd, sd->asd);
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 43fefa73e0a3..a6976d4a52ac 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -25,9 +25,10 @@
>  
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> -#include <media/v4l2-ioctl.h>
> -#include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
>  
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
> @@ -626,3 +627,42 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +static struct v4l2_async_notifier *v4l2_subdev_get_subnotifier(
> +	struct v4l2_subdev *sd)
> +{
> +	if (sd->subnotifier)
> +		return sd->subnotifier;
> +
> +	return (sd->subnotifier = devm_kzalloc(
> +			sd->dev, sizeof(*sd->subnotifier), GFP_KERNEL));
> +}
> +
> +int v4l2_subdev_fwnode_endpoints_parse(
> +	struct v4l2_subdev *sd,	size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd))
> +{
> +	struct v4l2_async_notifier *subnotifier =
> +		v4l2_subdev_get_subnotifier(sd);
> +
> +	if (!subnotifier)
> +		return -ENOMEM;
> +
> +	return v4l2_fwnode_endpoints_parse(sd->dev, subnotifier,
> +					   asd_struct_size, parse_single);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_endpoints_parse);
> +
> +int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_async_notifier *subnotifier =
> +		v4l2_subdev_get_subnotifier(sd);
> +
> +	if (!subnotifier)
> +		return -ENOMEM;
> +
> +	return v4l2_fwnode_reference_parse_sensor_common(sd->dev, subnotifier);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_reference_parse_sensor_common);
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 8cd4f8a75c3d..0a3f869ead52 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -27,6 +27,7 @@
>  struct fwnode_handle;
>  struct v4l2_async_notifier;
>  struct v4l2_async_subdev;
> +struct v4l2_subdev;
>  
>  /**
>   * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 0f92ebd2d710..e309a2e2030b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -43,6 +43,7 @@ struct v4l2_ctrl_handler;
>  struct v4l2_event;
>  struct v4l2_event_subscription;
>  struct v4l2_fh;
> +struct v4l2_fwnode_endpoint;
>  struct v4l2_subdev;
>  struct v4l2_subdev_fh;
>  struct tuner_setup;
> @@ -793,6 +794,7 @@ struct v4l2_subdev_platform_data {
>   *	list.
>   * @asd: Pointer to respective &struct v4l2_async_subdev.
>   * @notifier: Pointer to the managing notifier.
> + * @subnotifier: Pointer to the async sub-device notifier.
>   * @pdata: common part of subdevice platform data
>   *
>   * Each instance of a subdev driver should create this struct, either
> @@ -823,6 +825,7 @@ struct v4l2_subdev {
>  	struct list_head async_list;
>  	struct v4l2_async_subdev *asd;
>  	struct v4l2_async_notifier *notifier;
> +	struct v4l2_async_notifier *subnotifier;
>  	struct v4l2_subdev_platform_data *pdata;
>  };
>  
> @@ -1001,4 +1004,12 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>  void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  			      const struct v4l2_event *ev);
>  
> +int v4l2_subdev_fwnode_endpoints_parse(
> +	struct v4l2_subdev *sd,	size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd));
> +
> +int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd);
> +
>  #endif
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC 02/19] v4l: async: add subnotifier registration for subdevices

[Niklas Söderlund]

Hi Sakari,

Just a heads up, I posted a new version of this patch today which 
addresses a few short comings in v4l2_async_notifier_unregister() which 
exists in this patch. With the latest patches the re-probing of 
subdevices, disregarding if they are master or sub notifiers is 
addressed.

Also the latest version do not expose the new equivalent of 
v4l2_async_subnotifier_* outside v4l2-async.c and do similar things as 
you do here by hiding the (un)registration of subnotifiers inside the 
v4l2-async framework. So unfortunately to make use of the fixes in your 
series some rework is needed of my patch(es).

On 2017-07-18 22:03:44 +0300, Sakari Ailus wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> When the registered() callback of v4l2_subdev_internal_ops is called the
> subdevice has access to the master devices v4l2_dev and it's called with
> the async frameworks list_lock held. In this context the subdevice can
> register its own notifiers to allow for incremental discovery of
> subdevices.
> 
> The master device registers the subdevices closest to itself in its
> notifier while the subdevice(s) register notifiers for their closest
> neighboring devices when they are registered. Using this incremental
> approach two problems can be solved:
> 
> 1. The master device no longer has to care how many devices exist in
>    the pipeline. It only needs to care about its closest subdevice and
>    arbitrary long pipelines can be created without having to adapt the
>    master device for each case.
> 
> 2. Subdevices which are represented as a single DT node but register
>    more than one subdevice can use this to improve the pipeline
>    discovery, since the subdevice driver is the only one who knows which
>    of its subdevices is linked with which subdevice of a neighboring DT
>    node.
> 
> To enable subdevices to register/unregister notifiers from the
> registered()/unregistered() callback v4l2_async_subnotifier_register()
> and v4l2_async_subnotifier_unregister() are added. These new notifier
> register functions are similar to the master device equivalent functions
> but run without taking the v4l2-async list_lock which already is held
> when the registered()/unregistered() callbacks are called.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media/kapi/v4l2-subdev.rst | 20 ++++++++++
>  drivers/media/v4l2-core/v4l2-async.c     | 65 +++++++++++++++++++++++++++-----
>  include/media/v4l2-async.h               | 22 +++++++++++
>  3 files changed, 98 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> index e1f0b726e438..e308f30887a8 100644
> --- a/Documentation/media/kapi/v4l2-subdev.rst
> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> @@ -262,6 +262,26 @@ is called. After all subdevices have been located the .complete() callback is
>  called. When a subdevice is removed from the system the .unbind() method is
>  called. All three callbacks are optional.
>  
> +Subdevice drivers might in turn register subnotifier objects with an
> +array of other subdevice descriptors that the subdevice needs for its
> +own operation. Subnotifiers are an extension of the bridge drivers
> +notifier to allow for a incremental registering and matching of
> +subdevices. This is useful when a driver only has information about
> +which subdevice is closest to itself and would require knowledge from the
> +driver of that subdevice to know which other subdevice(s) lie beyond.
> +By registering subnotifiers drivers can incrementally move the subdevice
> +matching down the chain of drivers. This is performed using the
> +:c:func:`v4l2_async_subnotifier_register` call. To unregister the
> +subnotifier the driver has to call
> +:c:func:`v4l2_async_subnotifier_unregister`. These functions and their
> +arguments behave almost the same as the bridge driver notifiers
> +described above and are treated equally by the V4L2 core when matching
> +asynchronously registered subdevices. The differences are that the
> +subnotifier functions act on :c:type:`v4l2_subdev` instead of
> +:c:type:`v4l2_device` and that they should be called from the subdevice's
> +``.registered()`` and ``.unregistered()``
> +:c:type:`v4l2_subdev_internal_ops` callbacks instead of at probe time.
> +
>  V4L2 sub-device userspace API
>  -----------------------------
>  
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 268e19724809..d2ce39ac402e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -163,8 +163,9 @@ 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_do_notifier_register(struct v4l2_device *v4l2_dev,
> +					   struct v4l2_async_notifier *notifier,
> +					   bool subnotifier)
>  {
>  	struct v4l2_subdev *sd, *tmp;
>  	struct v4l2_async_subdev *asd;
> @@ -196,8 +197,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  		list_add_tail(&asd->list, &notifier->waiting);
>  	}
>  
> -	mutex_lock(&list_lock);
> +	if (subnotifier)
> +		lockdep_assert_held(&list_lock);
> +	else
> +		mutex_lock(&list_lock);
>  
> +	/*
> +	 * This function can be called recursively so the list
> +	 * might be modified in a recursive call. Start from the
> +	 * top of the list each iteration.
> +	 */
> +again:
>  	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
>  		int ret;
>  
> @@ -207,21 +217,39 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  
>  		ret = v4l2_async_test_notify(notifier, sd, asd);
>  		if (ret < 0) {
> -			mutex_unlock(&list_lock);
> +			if (!subnotifier)
> +				mutex_unlock(&list_lock);
>  			return ret;
>  		}
> +		goto again;
>  	}
>  
>  	/* Keep also completed notifiers on the list */
>  	list_add(&notifier->list, &notifier_list);
>  
> -	mutex_unlock(&list_lock);
> +	if (!subnotifier)
> +		mutex_unlock(&list_lock);
>  
>  	return 0;
>  }
> +
> +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> +				    struct v4l2_async_notifier *notifier)
> +{
> +	return v4l2_async_do_notifier_register(sd->v4l2_dev, notifier, true);
> +}
> +EXPORT_SYMBOL(v4l2_async_subnotifier_register);
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> +				 struct v4l2_async_notifier *notifier)
> +{
> +	return v4l2_async_do_notifier_register(v4l2_dev, notifier, false);
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>  
> -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +static void
> +v4l2_async_do_notifier_unregister(struct v4l2_async_notifier *notifier,
> +				  bool subnotifier)
>  {
>  	struct v4l2_subdev *sd, *tmp;
>  	unsigned int notif_n_subdev = notifier->num_subdevs;
> @@ -238,7 +266,10 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  			"Failed to allocate device cache!\n");
>  	}
>  
> -	mutex_lock(&list_lock);
> +	if (subnotifier)
> +		lockdep_assert_held(&list_lock);
> +	else
> +		mutex_lock(&list_lock);
>  
>  	list_del(&notifier->list);
>  
> @@ -265,15 +296,20 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  			put_device(d);
>  	}
>  
> -	mutex_unlock(&list_lock);
> +	if (!subnotifier)
> +		mutex_unlock(&list_lock);
>  
>  	/*
>  	 * Call device_attach() to reprobe devices
>  	 *
>  	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
>  	 * executed.
> +	 * TODO: If we are unregistering a subdevice notifier we can't reprobe
> +	 * since the lock_list is held by the master device and attaching that
> +	 * device would call v4l2_async_register_subdev() and end in a deadlock
> +	 * on list_lock.
>  	 */
> -	while (i--) {
> +	while (i-- && !subnotifier) {
>  		struct device *d = dev[i];
>  
>  		if (d && device_attach(d) < 0) {
> @@ -297,6 +333,17 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  	 * upon notifier registration.
>  	 */
>  }
> +
> +void v4l2_async_subnotifier_unregister(struct v4l2_async_notifier *notifier)
> +{
> +	v4l2_async_do_notifier_unregister(notifier, true);
> +}
> +EXPORT_SYMBOL(v4l2_async_subnotifier_unregister);
> +
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +{
> +	v4l2_async_do_notifier_unregister(notifier, false);
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>  
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 056cae0af2f0..8c7519fce5b9 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -105,6 +105,18 @@ struct v4l2_async_notifier {
>  };
>  
>  /**
> + * v4l2_async_notifier_register - registers a subdevice asynchronous subnotifier
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + * @notifier: pointer to &struct v4l2_async_notifier
> + *
> + * This function assumes the async list_lock is already locked, allowing it to
> + * be used from the struct v4l2_subdev_internal_ops registered() callback.
> + */
> +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> +				    struct v4l2_async_notifier *notifier);
> +
> +/**
>   * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
>   *
>   * @v4l2_dev: pointer to &struct v4l2_device
> @@ -114,6 +126,16 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  				 struct v4l2_async_notifier *notifier);
>  
>  /**
> + * v4l2_async_subnotifier_unregister - unregisters a asynchronous subnotifier
> + *
> + * @notifier: pointer to &struct v4l2_async_notifier
> + *
> + * This function assumes the async list_lock is already locked, allowing it to
> + * be used from the struct v4l2_subdev_internal_ops unregistered() callback.
> + */
> +void v4l2_async_subnotifier_unregister(struct v4l2_async_notifier *notifier);
> +
> +/**
>   * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier
>   *
>   * @notifier: pointer to &struct v4l2_async_notifier
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Hans Verkuil]

On 18/07/17 21:03, Sakari Ailus wrote:
> The async notifier supports three callbacks to the notifier: bound, unbound
> and complete. The complete callback has been traditionally used for
> creating the sub-device nodes.
> 
> This approach has an inherent weakness: if registration of a single
> sub-device fails for whatever reason, it renders the entire media device
> unusable even if only that piece of hardware is not working. This is a
> problem in particular in systems with multiple independent image pipelines
> on a single device. We have had such devices (e.g. omap3isp) supported for
> a number of years and the problem is growing more pressing as time passes
> so there is an incentive to resolve this.

I don't think this is a good reason. If one of the subdevices fail, then your
hardware is messed up and there is no point in continuing.

There may be other valid reasons why you would want this (reconfigurable
FPGA, hotplugging of sensors), but the reason you give here doesn't hold
water IMHO.

Regards,

	Hans

> The solution is to register device nodes at the time when the driver of
> those devices is complete with initialising the piece of hardware it is
> controlling.
> 
> This leaves partial pipelines visible to the user. There are two things to
> consider here:
> 
> 1) Registering multiple device nodes was never an atomic operation so the
> user space still had to be prepared for partial media graph being visible
> and
> 
> 2) The user space can figure out that a pipeline is not startable from the
> fact that there are pads with MEDIA_PAD_FL_MUST_CONNECT flag set without
> an (enabled) link.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index d2ce39ac402e..55fa7106345c 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -127,19 +127,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>  {
>  	int ret;
>  
> -	if (notifier->bound) {
> -		ret = notifier->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);
>  		return ret;
>  	}
>  
> +	if (notifier->bound) {
> +		ret = notifier->bound(notifier, sd, asd);
> +		if (ret < 0) {
> +			v4l2_device_unregister_subdev(sd);
> +			return ret;
> +		}
> +	}
> +
>  	/* Remove from the waiting list */
>  	list_del(&asd->list);
>  	sd->asd = asd;
> 

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Hans Verkuil]

On 18/07/17 21:03, Sakari Ailus wrote:
> Hi folks,
> 
> This RFC patchset achieves a number of things which I've put to the same
> patchset for they need to be show together to demonstrate the use cases.
> 
> I don't really intend this to compete with Niklas's patchset but much of
> the problem area addressed by the two is the same.
> 
> Comments would be welcome.
> 
> - Add AS3645A LED flash class driver.
> 
> - Add async notifiers (by Niklas).
> 
> - V4L2 sub-device node registration is moved to take place at the same time
>   with the registration of the sub-device itself. With this change,
>   sub-device node registration behaviour is aligned with video node
>   registration.
> 
> - The former is made possible by moving the bound() callback after
>   sub-device registration.
> 
> - As all the device node registration and link creation is done as the
>   respective devices are probed, there is no longer dependency to the
>   notifier complete callback which as itself is seen problematic. The
>   complete callback still exists but there's no need to use it, pending
>   changes in individual drivers.
> 
>   See:
>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> 
>   As a result, if a part of the media device fails to initialise because it
>   is e.g. physically broken, it will be possible to use what works.

I've got major problems with this from a userspace point of view. In the vast
majority of cases you just want to bail out if one or more subdevs fail.

Unless your device has special requirements (hotpluggable sensors) you just
want to wait until everything has finished loading and initializing and only
then do you start using the device nodes (actually, they only appear if
everything was loaded correctly today).

You do *not* want to put the burden of checking if everything is OK on
userspace. Only if it is known that things may not be there is it something
you can let userspace figure out.

I haven't really thought how this should be done but you can't just move
this problem to userspace.

Unless I misunderstand what is happening here?

A lot of this patch series I like a lot, but this part needs more work.

Regards,

	Hans

> - Finally, the use of the async sub-notifier is hidden in the framework and
>   all a driver (such as smiapp) needs to do is to call
>   v4l2_subdev_fwnode_reference_parse_sensor_common() in its probe()
>   function to find out the associated devices (lens and flash). This
>   approach makes it possible to later on to rework the sub-notifier
>   implementation without touching driver code. Endpoints can be parsed
>   similarly by simply calling v4l2_subdev_fwnode_endpoints_parse() for
>   driver's probe function.
> 
> The patches depend on this branch currently:
> 
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=as3645a-leds-base>
> 
> It's essentially the V4L2 flash class patches I've posted earlier today and
> a stash of fwnode property API improvements.
> 
> 
> Niklas S�derlund (1):
>   v4l: async: add subnotifier registration for subdevices
> 
> Sakari Ailus (18):
>   device property: Introduce fwnode_property_get_reference_args
>   dt: bindings: Add a binding for flash devices associated to a sensor
>   dt: bindings: Add lens-focus binding for image sensors
>   leds: as3645a: Add LED flash class driver
>   leds: as3645a: Separate flash and indicator LED sub-devices
>   v4l: fwnode: Support generic parsing of graph endpoints in V4L2
>   arm: dts: omap3: N9/N950: Add AS3645A camera flash
>   v4l2-fwnode: Add conveniences function for parsing generic references
>   v4l2-fwnode: Add convenience function for parsing common external refs
>   v4l2-async: Register sub-devices before calling bound callback
>   v4l2-subdev: Support registering V4L2 sub-device nodes one by one
>   v4l2-device: Register sub-device nodes at sub-device registration time
>   omap3isp: Move sub-device link creation to notifier bound callback
>   omap3isp: Initialise "crashed" media entity enum in probe
>   omap3isp: Move media device registration to probe
>   omap3isp: Drop the async notifier callback
>   v4l2-fwnode: Add abstracted sub-device notifiers
>   smiapp: Add support for flash and lens devices
> 
>  .../devicetree/bindings/media/video-interfaces.txt |  10 +
>  Documentation/media/kapi/v4l2-subdev.rst           |  20 +
>  MAINTAINERS                                        |   6 +
>  arch/arm/boot/dts/omap3-n9.dts                     |   1 +
>  arch/arm/boot/dts/omap3-n950-n9.dtsi               |  14 +
>  arch/arm/boot/dts/omap3-n950.dts                   |   1 +
>  drivers/acpi/property.c                            |  27 +
>  drivers/base/property.c                            |  12 +
>  drivers/leds/Kconfig                               |   8 +
>  drivers/leds/Makefile                              |   1 +
>  drivers/leds/leds-as3645a.c                        | 762 +++++++++++++++++++++
>  drivers/media/i2c/smiapp/smiapp-core.c             |   5 +
>  drivers/media/platform/omap3isp/isp.c              | 144 ++--
>  drivers/media/platform/omap3isp/isp.h              |   3 -
>  drivers/media/v4l2-core/v4l2-async.c               |  96 ++-
>  drivers/media/v4l2-core/v4l2-device.c              | 139 ++--
>  drivers/media/v4l2-core/v4l2-fwnode.c              | 174 +++++
>  drivers/media/v4l2-core/v4l2-subdev.c              |  44 +-
>  drivers/of/property.c                              |  31 +
>  include/linux/fwnode.h                             |  19 +
>  include/linux/property.h                           |   4 +
>  include/media/v4l2-async.h                         |  26 +-
>  include/media/v4l2-fwnode.h                        |  21 +
>  include/media/v4l2-subdev.h                        |  11 +
>  24 files changed, 1388 insertions(+), 191 deletions(-)
>  create mode 100644 drivers/leds/leds-as3645a.c
> 

Re: [RFC 05/19] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Hi Sakari,

Thanks for the update.

My remarks from [0] related to LED class device naming apply also
to this version of the patch.

[0[ https://www.spinics.net/lists/linux-leds/msg08015.html

Best regards,
Jacek Anaszewski

On 07/18/2017 09:03 PM, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
> driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
> is based on that.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  MAINTAINERS                 |   6 +
>  drivers/leds/Kconfig        |   8 +
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-as3645a.c | 742 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 757 insertions(+)
>  create mode 100644 drivers/leds/leds-as3645a.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 205d3977ac46..312be8939969 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2106,6 +2106,12 @@ F:	arch/arm64/
>  F:	Documentation/arm64/
>  
>  AS3645A LED FLASH CONTROLLER DRIVER
> +M:	Sakari Ailus <sakari.ailus@iki.fi>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	drivers/leds/leds-as3645a.c
> +
> +AS3645A LED FLASH CONTROLLER DRIVER
>  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>  L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 594b24d410c3..bad3a4098104 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -58,6 +58,14 @@ config LEDS_AAT1290
>  	help
>  	 This option enables support for the LEDs on the AAT1290.
>  
> +config LEDS_AS3645A
> +	tristate "AS3645A LED flash controller support"
> +	depends on I2C && LEDS_CLASS_FLASH
> +	help
> +	  Enable LED flash class support for AS3645A LED flash
> +	  controller. V4L2 flash API is provided as well if
> +	  CONFIG_V4L2_FLASH_API is enabled.
> +
>  config LEDS_BCM6328
>  	tristate "LED Support for Broadcom BCM6328"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 909dae62ba05..7d7b26552923 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
>  # LED Platform Drivers
>  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
>  obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
> +obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
>  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
> diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
> new file mode 100644
> index 000000000000..b1dc32a3c620
> --- /dev/null
> +++ b/drivers/leds/leds-as3645a.c
> @@ -0,0 +1,742 @@
> +/*
> + * drivers/leds/leds-as3645a.c - AS3645A and LM3555 flash controllers driver
> + *
> + * Copyright (C) 2008-2011 Nokia Corporation
> + * Copyright (c) 2011, 2017 Intel Corporation.
> + *
> + * Based on drivers/media/i2c/as3645a.c.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@iki.fi>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define AS_TIMER_US_TO_CODE(t)			(((t) / 1000 - 100) / 50)
> +#define AS_TIMER_CODE_TO_US(c)			((50 * (c) + 100) * 1000)
> +
> +/* Register definitions */
> +
> +/* Read-only Design info register: Reset state: xxxx 0001 */
> +#define AS_DESIGN_INFO_REG			0x00
> +#define AS_DESIGN_INFO_FACTORY(x)		(((x) >> 4))
> +#define AS_DESIGN_INFO_MODEL(x)			((x) & 0x0f)
> +
> +/* Read-only Version control register: Reset state: 0000 0000
> + * for first engineering samples
> + */
> +#define AS_VERSION_CONTROL_REG			0x01
> +#define AS_VERSION_CONTROL_RFU(x)		(((x) >> 4))
> +#define AS_VERSION_CONTROL_VERSION(x)		((x) & 0x0f)
> +
> +/* Read / Write	(Indicator and timer register): Reset state: 0000 1111 */
> +#define AS_INDICATOR_AND_TIMER_REG		0x02
> +#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT	0
> +#define AS_INDICATOR_AND_TIMER_VREF_SHIFT	4
> +#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT	6
> +
> +/* Read / Write	(Current set register): Reset state: 0110 1001 */
> +#define AS_CURRENT_SET_REG			0x03
> +#define AS_CURRENT_ASSIST_LIGHT_SHIFT		0
> +#define AS_CURRENT_LED_DET_ON			(1 << 3)
> +#define AS_CURRENT_FLASH_CURRENT_SHIFT		4
> +
> +/* Read / Write	(Control register): Reset state: 1011 0100 */
> +#define AS_CONTROL_REG				0x04
> +#define AS_CONTROL_MODE_SETTING_SHIFT		0
> +#define AS_CONTROL_STROBE_ON			(1 << 2)
> +#define AS_CONTROL_OUT_ON			(1 << 3)
> +#define AS_CONTROL_EXT_TORCH_ON			(1 << 4)
> +#define AS_CONTROL_STROBE_TYPE_EDGE		(0 << 5)
> +#define AS_CONTROL_STROBE_TYPE_LEVEL		(1 << 5)
> +#define AS_CONTROL_COIL_PEAK_SHIFT		6
> +
> +/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */
> +#define AS_FAULT_INFO_REG			0x05
> +#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT	(1 << 1)
> +#define AS_FAULT_INFO_INDICATOR_LED		(1 << 2)
> +#define AS_FAULT_INFO_LED_AMOUNT		(1 << 3)
> +#define AS_FAULT_INFO_TIMEOUT			(1 << 4)
> +#define AS_FAULT_INFO_OVER_TEMPERATURE		(1 << 5)
> +#define AS_FAULT_INFO_SHORT_CIRCUIT		(1 << 6)
> +#define AS_FAULT_INFO_OVER_VOLTAGE		(1 << 7)
> +
> +/* Boost register */
> +#define AS_BOOST_REG				0x0d
> +#define AS_BOOST_CURRENT_DISABLE		(0 << 0)
> +#define AS_BOOST_CURRENT_ENABLE			(1 << 0)
> +
> +/* Password register is used to unlock boost register writing */
> +#define AS_PASSWORD_REG				0x0f
> +#define AS_PASSWORD_UNLOCK_VALUE		0x55
> +
> +#define AS_NAME					"as3645a"
> +#define AS_I2C_ADDR				(0x60 >> 1) /* W:0x60, R:0x61 */
> +
> +#define AS_FLASH_TIMEOUT_MIN			100000	/* us */
> +#define AS_FLASH_TIMEOUT_MAX			850000
> +#define AS_FLASH_TIMEOUT_STEP			50000
> +
> +#define AS_FLASH_INTENSITY_MIN			200000	/* uA */
> +#define AS_FLASH_INTENSITY_MAX_1LED		500000
> +#define AS_FLASH_INTENSITY_MAX_2LEDS		400000
> +#define AS_FLASH_INTENSITY_STEP			20000
> +
> +#define AS_TORCH_INTENSITY_MIN			20000	/* uA */
> +#define AS_TORCH_INTENSITY_MAX			160000
> +#define AS_TORCH_INTENSITY_STEP			20000
> +
> +#define AS_INDICATOR_INTENSITY_MIN		0	/* uA */
> +#define AS_INDICATOR_INTENSITY_MAX		10000
> +#define AS_INDICATOR_INTENSITY_STEP		2500
> +
> +#define AS_PEAK_mA_MAX				2000
> +#define AS_PEAK_mA_TO_REG(a) \
> +	((min_t(u32, AS_PEAK_mA_MAX, a) - 1250) / 250)
> +
> +enum as_mode {
> +	AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT,
> +};
> +
> +struct as3645a_config {
> +	u32 flash_timeout_us;
> +	u32 flash_max_ua;
> +	u32 assist_max_ua;
> +	u32 indicator_max_ua;
> +	u32 voltage_reference;
> +	u32 peak;
> +};
> +
> +struct as3645a {
> +	struct i2c_client *client;
> +
> +	struct mutex mutex;
> +
> +	struct led_classdev_flash fled;
> +	struct led_classdev iled_cdev;
> +
> +	struct v4l2_flash *vf;
> +
> +	struct as3645a_config cfg;
> +
> +	enum as_mode mode;
> +	unsigned int timeout;
> +	unsigned int flash_current;
> +	unsigned int assist_current;
> +	unsigned int indicator_current;
> +	enum v4l2_flash_strobe_source strobe_source;
> +};
> +
> +#define fled_to_as3645a(__fled) container_of(__fled, struct as3645a, fled)
> +#define iled_cdev_to_as3645a(__iled_cdev) \
> +	container_of(__iled_cdev, struct as3645a, iled_cdev)
> +
> +/* Return negative errno else zero on success */
> +static int as3645a_write(struct as3645a *flash, u8 addr, u8 val)
> +{
> +	struct i2c_client *client = flash->client;
> +	int rval;
> +
> +	rval = i2c_smbus_write_byte_data(client, addr, val);
> +
> +	dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val,
> +		rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +/* Return negative errno else a data byte received from the device. */
> +static int as3645a_read(struct as3645a *flash, u8 addr)
> +{
> +	struct i2c_client *client = flash->client;
> +	int rval;
> +
> +	rval = i2c_smbus_read_byte_data(client, addr);
> +
> +	dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval,
> +		rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Hardware configuration and trigger
> + */
> +
> +/**
> + * as3645a_set_config - Set flash configuration registers
> + * @flash: The flash
> + *
> + * Configure the hardware with flash, assist and indicator currents, as well as
> + * flash timeout.
> + *
> + * Return 0 on success, or a negative error code if an I2C communication error
> + * occurred.
> + */
> +static int as3645a_set_current(struct as3645a *flash)
> +{
> +	u8 val;
> +
> +	val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT)
> +	    | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT)
> +	    | AS_CURRENT_LED_DET_ON;
> +
> +	return as3645a_write(flash, AS_CURRENT_SET_REG, val);
> +}
> +
> +static int as3645a_set_timeout(struct as3645a *flash)
> +{
> +	u8 val;
> +
> +	val = flash->timeout << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT;
> +
> +	val |= (flash->cfg.voltage_reference
> +		<< AS_INDICATOR_AND_TIMER_VREF_SHIFT)
> +	    |  ((flash->indicator_current ? flash->indicator_current - 1 : 0)
> +		 << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT);
> +
> +	return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val);
> +}
> +
> +/**
> + * as3645a_set_control - Set flash control register
> + * @flash: The flash
> + * @mode: Desired output mode
> + * @on: Desired output state
> + *
> + * Configure the hardware with output mode and state.
> + *
> + * Return 0 on success, or a negative error code if an I2C communication error
> + * occurred.
> + */
> +static int
> +as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on)
> +{
> +	u8 reg;
> +
> +	/* Configure output parameters and operation mode. */
> +	reg = (flash->cfg.peak << AS_CONTROL_COIL_PEAK_SHIFT)
> +	    | (on ? AS_CONTROL_OUT_ON : 0)
> +	    | mode;
> +
> +	if (mode == AS_MODE_FLASH &&
> +	    flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL)
> +		reg |= AS_CONTROL_STROBE_TYPE_LEVEL
> +		    |  AS_CONTROL_STROBE_ON;
> +
> +	return as3645a_write(flash, AS_CONTROL_REG, reg);
> +}
> +
> +static int as3645a_get_fault(struct led_classdev_flash *fled, u32 *fault)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +	int rval;
> +
> +	/* NOTE: reading register clears fault status */
> +	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
> +	if (rval < 0)
> +		return rval;
> +
> +	if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT)
> +		*fault |= LED_FAULT_OVER_CURRENT;
> +
> +	if (rval & AS_FAULT_INFO_INDICATOR_LED)
> +		*fault |= LED_FAULT_INDICATOR;
> +
> +	dev_dbg(&flash->client->dev, "%u connected LEDs\n",
> +		rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1);
> +
> +	if (rval & AS_FAULT_INFO_TIMEOUT)
> +		*fault |= LED_FAULT_TIMEOUT;
> +
> +	if (rval & AS_FAULT_INFO_OVER_TEMPERATURE)
> +		*fault |= LED_FAULT_OVER_TEMPERATURE;
> +
> +	if (rval & AS_FAULT_INFO_SHORT_CIRCUIT)
> +		*fault |= LED_FAULT_OVER_CURRENT;
> +
> +	if (rval & AS_FAULT_INFO_OVER_VOLTAGE)
> +		*fault |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	return rval;
> +}
> +
> +static unsigned int __as3645a_current_to_reg(unsigned int min, unsigned int max,
> +					     unsigned int step,
> +					     unsigned int val)
> +{
> +	if (val < min)
> +		val = min;
> +
> +	if (val > max)
> +		val = max;
> +
> +	return (val - min) / step;
> +}
> +
> +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> +					   unsigned int ua)
> +{
> +	if (is_flash)
> +		return __as3645a_current_to_reg(AS_TORCH_INTENSITY_MIN,
> +						flash->cfg.assist_max_ua,
> +						AS_TORCH_INTENSITY_STEP, ua);
> +	else
> +		return __as3645a_current_to_reg(AS_FLASH_INTENSITY_MIN,
> +						flash->cfg.flash_max_ua,
> +						AS_FLASH_INTENSITY_STEP, ua);
> +}
> +
> +static int as3645a_set_indicator_brightness(struct led_classdev *iled_cdev,
> +					    enum led_brightness brightness)
> +{
> +	struct as3645a *flash = iled_cdev_to_as3645a(iled_cdev);
> +	int rval;
> +
> +	flash->indicator_current = brightness;
> +
> +	rval = as3645a_set_timeout(flash);
> +	if (rval)
> +		return rval;
> +
> +	return as3645a_set_control(flash, AS_MODE_INDICATOR, brightness);
> +}
> +
> +static int as3645a_set_assist_brightness(struct led_classdev *fled_cdev,
> +					 enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled = lcdev_to_flcdev(fled_cdev);
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +	int rval;
> +
> +	if (brightness) {
> +		/* Register value 0 is 20 mA. */
> +		flash->assist_current = brightness - 1;
> +
> +		rval = as3645a_set_current(flash);
> +		if (rval)
> +			return rval;
> +	}
> +
> +	return as3645a_set_control(flash, AS_MODE_ASSIST, brightness);
> +}
> +
> +static int as3645a_set_flash_brightness(struct led_classdev_flash *fled,
> +					u32 brightness_ua)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	flash->flash_current = as3645a_current_to_reg(flash, true, brightness_ua);
> +
> +	return as3645a_set_current(flash);
> +}
> +
> +static int as3645a_set_flash_timeout(struct led_classdev_flash *fled,
> +				     u32 timeout_us)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	flash->timeout = AS_TIMER_US_TO_CODE(timeout_us);
> +
> +	return as3645a_set_timeout(flash);
> +}
> +
> +static int as3645a_set_strobe(struct led_classdev_flash *fled, bool state)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	return as3645a_set_control(flash, AS_MODE_FLASH, state);
> +}
> +
> +static const struct led_flash_ops as3645a_led_flash_ops = {
> +	.flash_brightness_set = as3645a_set_flash_brightness,
> +	.timeout_set = as3645a_set_flash_timeout,
> +	.strobe_set = as3645a_set_strobe,
> +	.fault_get = as3645a_get_fault,
> +};
> +
> +static int as3645a_setup(struct as3645a *flash)
> +{
> +	struct device *dev = &flash->client->dev;
> +	u32 fault = 0;
> +	int rval;
> +
> +	/* clear errors */
> +	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
> +	if (rval < 0)
> +		return rval;
> +
> +	dev_dbg(dev, "Fault info: %02x\n", rval);
> +
> +	rval = as3645a_set_current(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_set_timeout(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_set_control(flash, AS_MODE_INDICATOR, false);
> +	if (rval < 0)
> +		return rval;
> +
> +	/* read status */
> +	rval = as3645a_get_fault(&flash->fled, &fault);
> +	if (rval < 0)
> +		return rval;
> +
> +	dev_dbg(dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n",
> +		as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG));
> +	dev_dbg(dev, "AS_CURRENT_SET_REG: %02x\n",
> +		as3645a_read(flash, AS_CURRENT_SET_REG));
> +	dev_dbg(dev, "AS_CONTROL_REG: %02x\n",
> +		as3645a_read(flash, AS_CONTROL_REG));
> +
> +	return rval & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
> +}
> +
> +static int as3645a_detect(struct as3645a *flash)
> +{
> +	struct device *dev = &flash->client->dev;
> +	int rval, man, model, rfu, version;
> +	const char *vendor;
> +
> +	rval = as3645a_read(flash, AS_DESIGN_INFO_REG);
> +	if (rval < 0) {
> +		dev_err(dev, "can't read design info reg\n");
> +		return rval;
> +	}
> +
> +	man = AS_DESIGN_INFO_FACTORY(rval);
> +	model = AS_DESIGN_INFO_MODEL(rval);
> +
> +	rval = as3645a_read(flash, AS_VERSION_CONTROL_REG);
> +	if (rval < 0) {
> +		dev_err(dev, "can't read version control reg\n");
> +		return rval;
> +	}
> +
> +	rfu = AS_VERSION_CONTROL_RFU(rval);
> +	version = AS_VERSION_CONTROL_VERSION(rval);
> +
> +	/* Verify the chip model and version. */
> +	if (model != 0x01 || rfu != 0x00) {
> +		dev_err(dev, "AS3645A not detected "
> +			"(model %d rfu %d)\n", model, rfu);
> +		return -ENODEV;
> +	}
> +
> +	switch (man) {
> +	case 1:
> +		vendor = "AMS, Austria Micro Systems";
> +		break;
> +	case 2:
> +		vendor = "ADI, Analog Devices Inc.";
> +		break;
> +	case 3:
> +		vendor = "NSC, National Semiconductor";
> +		break;
> +	case 4:
> +		vendor = "NXP";
> +		break;
> +	case 5:
> +		vendor = "TI, Texas Instrument";
> +		break;
> +	default:
> +		vendor = "Unknown";
> +	}
> +
> +	dev_info(dev, "Chip vendor: %s (%d) Version: %d\n", vendor,
> +		 man, version);
> +
> +	rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE);
> +	if (rval < 0)
> +		return rval;
> +
> +	return as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE);
> +}
> +
> +static __maybe_unused int as3645a_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +	int rval;
> +
> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> +
> +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
> +
> +	return rval;
> +}
> +
> +static __maybe_unused int as3645a_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +	int rval;
> +
> +	rval = as3645a_setup(flash);
> +
> +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +static int as3645a_parse_node(struct as3645a *flash,
> +			      struct device_node *node)
> +{
> +	struct as3645a_config *cfg = &flash->cfg;
> +	struct device_node *child;
> +	int rval;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child) {
> +		dev_err(&flash->client->dev, "can't find flash node\n");
> +		return -ENODEV;
> +	}
> +
> +	rval = of_property_read_u32(child, "flash-timeout-us",
> +				   &cfg->flash_timeout_us);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read flash-timeout-us property for flash\n");
> +		goto out_err;
> +	}
> +
> +	rval = of_property_read_u32(child, "flash-max-microamp",
> +				   &cfg->flash_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read flash-max-microamp property for flash\n");
> +		goto out_err;
> +	}
> +
> +	rval = of_property_read_u32(child, "led-max-microamp",
> +				   &cfg->assist_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read led-max-microamp property for flash\n");
> +		goto out_err;
> +	}
> +
> +	of_property_read_u32(child, "voltage-reference",
> +			     &cfg->voltage_reference);
> +
> +	of_property_read_u32(child, "peak-current-limit", &cfg->peak);
> +	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
> +
> +	of_node_put(child);
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child) {
> +		dev_warn(&flash->client->dev,
> +			 "can't find indicator node\n");
> +		return 0;
> +	}
> +
> +	rval = of_property_read_u32(child, "led-max-microamp",
> +				   &cfg->indicator_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read led-max-microamp property for indicator\n");
> +		goto out_err;
> +	}
> +
> +	of_node_put(child);
> +
> +	return 0;
> +
> +out_err:
> +	of_node_put(child);
> +
> +	return rval;
> +}
> +
> +static int as3645a_led_class_setup(struct as3645a *flash)
> +{
> +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
> +	struct led_classdev *iled_cdev = &flash->iled_cdev;
> +	struct led_flash_setting *cfg;
> +	int rval;
> +
> +	iled_cdev->name = "as3645a indicator";
> +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
> +	iled_cdev->max_brightness =
> +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
> +
> +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
> +	if (rval < 0)
> +		return rval;
> +
> +	cfg = &flash->fled.brightness;
> +	cfg->min = AS_FLASH_INTENSITY_MIN;
> +	cfg->max = flash->cfg.flash_max_ua;
> +	cfg->step = AS_FLASH_INTENSITY_STEP;
> +	cfg->val = flash->cfg.flash_max_ua;
> +
> +	cfg = &flash->fled.timeout;
> +	cfg->min = AS_FLASH_TIMEOUT_MIN;
> +	cfg->max = flash->cfg.flash_timeout_us;
> +	cfg->step = AS_FLASH_TIMEOUT_STEP;
> +	cfg->val = flash->cfg.flash_timeout_us;
> +
> +	flash->fled.ops = &as3645a_led_flash_ops;
> +
> +	fled_cdev->name = "as3645a flash";
> +	fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness;
> +	/* Value 0 is off in LED class. */
> +	fled_cdev->max_brightness =
> +		as3645a_current_to_reg(flash, false,
> +				       flash->cfg.assist_max_ua) + 1;
> +	fled_cdev->flags = LED_DEV_CAP_FLASH;
> +
> +	rval = led_classdev_flash_register(&flash->client->dev, &flash->fled);
> +	if (rval) {
> +		led_classdev_unregister(iled_cdev);
> +		dev_err(&flash->client->dev,
> +			"led_classdev_flash_register() failed, error %d\n",
> +			rval);
> +	}
> +
> +	return rval;
> +}
> +
> +static int as3645a_v4l2_setup(struct as3645a *flash)
> +{
> +	struct led_classdev_flash *fled = &flash->fled;
> +	struct led_classdev *led = &fled->led_cdev;
> +	struct v4l2_flash_config cfg = {
> +		.torch_intensity = {
> +			.min = AS_TORCH_INTENSITY_MIN,
> +			.max = flash->cfg.assist_max_ua,
> +			.step = AS_TORCH_INTENSITY_STEP,
> +			.val = flash->cfg.assist_max_ua,
> +		},
> +		.indicator_intensity = {
> +			.min = AS_INDICATOR_INTENSITY_MIN,
> +			.max = flash->cfg.indicator_max_ua,
> +			.step = AS_INDICATOR_INTENSITY_STEP,
> +			.val = flash->cfg.indicator_max_ua,
> +		},
> +	};
> +
> +	strlcpy(cfg.dev_name, led->name, sizeof(cfg.dev_name));
> +
> +	flash->vf = v4l2_flash_init(&flash->client->dev, NULL, &flash->fled,
> +				    &flash->iled_cdev, NULL, &cfg);
> +	if (IS_ERR(flash->vf))
> +		return PTR_ERR(flash->vf);
> +
> +	return 0;
> +}
> +
> +static int as3645a_probe(struct i2c_client *client)
> +{
> +	struct as3645a *flash;
> +	int rval;
> +
> +	if (client->dev.of_node == NULL)
> +		return -ENODEV;
> +
> +	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> +	if (flash == NULL)
> +		return -ENOMEM;
> +
> +	flash->client = client;
> +
> +	rval = as3645a_parse_node(flash, client->dev.of_node);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_detect(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	mutex_init(&flash->mutex);
> +	i2c_set_clientdata(client, flash);
> +
> +	rval = as3645a_setup(flash);
> +	if (rval)
> +		goto out_mutex_destroy;
> +
> +	rval = as3645a_led_class_setup(flash);
> +	if (rval)
> +		goto out_mutex_destroy;
> +
> +	rval = as3645a_v4l2_setup(flash);
> +	if (rval)
> +		goto out_led_classdev_flash_unregister;
> +
> +	return 0;
> +
> +out_led_classdev_flash_unregister:
> +	led_classdev_flash_unregister(&flash->fled);
> +
> +out_mutex_destroy:
> +	mutex_destroy(&flash->mutex);
> +
> +	return rval;
> +}
> +
> +static int as3645a_remove(struct i2c_client *client)
> +{
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +
> +	as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> +
> +	v4l2_flash_release(flash->vf);
> +
> +	led_classdev_flash_unregister(&flash->fled);
> +	led_classdev_unregister(&flash->iled_cdev);
> +
> +	mutex_destroy(&flash->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id as3645a_of_table[] = {
> +	{ .compatible = "ams,as3645a" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, as3645a_of_table);
> +
> +SIMPLE_DEV_PM_OPS(as3645a_pm_ops, as3645a_resume, as3645a_suspend);
> +
> +static struct i2c_driver as3645a_i2c_driver = {
> +	.driver	= {
> +		.of_match_table = as3645a_of_table,
> +		.name = AS_NAME,
> +		.pm   = &as3645a_pm_ops,
> +	},
> +	.probe_new	= as3645a_probe,
> +	.remove	= as3645a_remove,
> +};
> +
> +module_i2c_driver(as3645a_i2c_driver);
> +
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@iki.fi>");
> +MODULE_DESCRIPTION("LED flash driver for AS3645A, LM3555 and their clones");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC 05/19] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

On Wed, Jul 19, 2017 at 10:17:26PM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the update.
> 
> My remarks from [0] related to LED class device naming apply also
> to this version of the patch.
> 
> [0[ https://www.spinics.net/lists/linux-leds/msg08015.html

Oops. I'll address this in the next version.

Thanks for the reminder!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 18/19] v4l2-fwnode: Add abstracted sub-device notifiers

[Sakari Ailus]

Hi Niklas,

Thanks for the review!

On Tue, Jul 18, 2017 at 11:19:22PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your hard work!
> 
> On 2017-07-18 22:04:00 +0300, Sakari Ailus wrote:
> > Add notifiers for sub-devices. The notifiers themselves are not visible for
> > the sub-device drivers but instead are accessed through interface functions
> 
> I might be missing it, but I can't find a interface function to set the 
> bound()/unbind() callbacks on a sub-notifiers :-) Is this intentional or 
> only not present in this RFC?

It's not present since I didn't need it. :-)

I'll add that for v2.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[RFC 1/1] v4l2-subdev: Add a function to set sub-device notifier callbacks

[Sakari Ailus]

The sub-device's sub-notifier is hidded in the sub-device and not meant to
be accessed directly by drivers. Still the driver may wish to set callbacks
to the notifier. Add a function to do that:
v4l2_subdev_notifier_set_callbacks().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Well, this appears to be quite straightforward. The code is entirely untested
but trivial at the same time. 

 drivers/media/v4l2-core/v4l2-subdev.c | 20 ++++++++++++++++++++
 include/media/v4l2-subdev.h           |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index a6976d4a52ac..8629224bfdba 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -666,3 +666,23 @@ int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd)
 	return v4l2_fwnode_reference_parse_sensor_common(sd->dev, subnotifier);
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_reference_parse_sensor_common);
+
+int v4l2_subdev_notifier_set_callbacks(
+	struct v4l2_subdev *sd,
+	int (*bound)(struct v4l2_async_notifier *notifier,
+		     struct v4l2_subdev *subdev,
+		     struct v4l2_async_subdev *asd),
+	int (*complete)(struct v4l2_async_notifier *notifier))
+{
+	struct v4l2_async_notifier *subnotifier =
+		v4l2_subdev_get_subnotifier(sd);
+
+	if (!subnotifier)
+		return -ENOMEM;
+
+	subnotifier->bound = bound;
+	subnotifier->complete = complete;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_notifier_set_callbacks);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e309a2e2030b..ee85b64ad4f4 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1012,4 +1012,10 @@ int v4l2_subdev_fwnode_endpoints_parse(
 
 int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd);
 
+int v4l2_subdev_notifier_set_callbacks(
+	struct v4l2_subdev *sd,
+	int (*bound)(struct v4l2_async_notifier *notifier,
+		     struct v4l2_subdev *subdev,
+		     struct v4l2_async_subdev *asd),
+	int (*complete)(struct v4l2_async_notifier *notifier));
 #endif
-- 
2.11.0

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Sakari Ailus]

Hi Hans,

On Wed, Jul 19, 2017 at 01:24:54PM +0200, Hans Verkuil wrote:
> On 18/07/17 21:03, Sakari Ailus wrote:
> > The async notifier supports three callbacks to the notifier: bound, unbound
> > and complete. The complete callback has been traditionally used for
> > creating the sub-device nodes.
> > 
> > This approach has an inherent weakness: if registration of a single
> > sub-device fails for whatever reason, it renders the entire media device
> > unusable even if only that piece of hardware is not working. This is a
> > problem in particular in systems with multiple independent image pipelines
> > on a single device. We have had such devices (e.g. omap3isp) supported for
> > a number of years and the problem is growing more pressing as time passes
> > so there is an incentive to resolve this.
> 
> I don't think this is a good reason. If one of the subdevices fail, then your
> hardware is messed up and there is no point in continuing.

That's entirely untrue in general case.

If you have e.g. a mobile phone with a single camera, yes, you're right.
But most mobile phones have two cameras these days. Embedded systems may
have many, think of automotive use cases: you could have five or ten
cameras there.

It is not feasible to prevent the entire system from working if a single
component is at fault --- this is really any component such as a lens
controller.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Sakari Ailus]

Hi Hans,

Thanks for the review.

On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
> On 18/07/17 21:03, Sakari Ailus wrote:
> > Hi folks,
> > 
> > This RFC patchset achieves a number of things which I've put to the same
> > patchset for they need to be show together to demonstrate the use cases.
> > 
> > I don't really intend this to compete with Niklas's patchset but much of
> > the problem area addressed by the two is the same.
> > 
> > Comments would be welcome.
> > 
> > - Add AS3645A LED flash class driver.
> > 
> > - Add async notifiers (by Niklas).
> > 
> > - V4L2 sub-device node registration is moved to take place at the same time
> >   with the registration of the sub-device itself. With this change,
> >   sub-device node registration behaviour is aligned with video node
> >   registration.
> > 
> > - The former is made possible by moving the bound() callback after
> >   sub-device registration.
> > 
> > - As all the device node registration and link creation is done as the
> >   respective devices are probed, there is no longer dependency to the
> >   notifier complete callback which as itself is seen problematic. The
> >   complete callback still exists but there's no need to use it, pending
> >   changes in individual drivers.
> > 
> >   See:
> >   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> > 
> >   As a result, if a part of the media device fails to initialise because it
> >   is e.g. physically broken, it will be possible to use what works.
> 
> I've got major problems with this from a userspace point of view. In the vast
> majority of cases you just want to bail out if one or more subdevs fail.

I admit it's easier for the user space if the device becomes available only
when all its component drivers have registered.

Also remember that video nodes are registered in the file system right on
device probe time. It's only sub-device and media device node registration
that has taken place in the notifier's complete handler.

There are things how we could do this easier, for instance providing events
on the media device on entity registration and possibly registration state
(whether all entities have been registered).

This way the user space can easily choose whether it would like to proceed
using a partially functional device (or not).

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Hans Verkuil]

On 20/07/17 18:09, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Jul 19, 2017 at 01:24:54PM +0200, Hans Verkuil wrote:
>> On 18/07/17 21:03, Sakari Ailus wrote:
>>> The async notifier supports three callbacks to the notifier: bound, unbound
>>> and complete. The complete callback has been traditionally used for
>>> creating the sub-device nodes.
>>>
>>> This approach has an inherent weakness: if registration of a single
>>> sub-device fails for whatever reason, it renders the entire media device
>>> unusable even if only that piece of hardware is not working. This is a
>>> problem in particular in systems with multiple independent image pipelines
>>> on a single device. We have had such devices (e.g. omap3isp) supported for
>>> a number of years and the problem is growing more pressing as time passes
>>> so there is an incentive to resolve this.
>>
>> I don't think this is a good reason. If one of the subdevices fail, then your
>> hardware is messed up and there is no point in continuing.
> 
> That's entirely untrue in general case.
> 
> If you have e.g. a mobile phone with a single camera, yes, you're right.
> But most mobile phones have two cameras these days. Embedded systems may
> have many, think of automotive use cases: you could have five or ten
> cameras there.

These are all very recent developments. Today userspace can safely assume
that either everything would be up and running, or nothing at all.

> It is not feasible to prevent the entire system from working if a single
> component is at fault --- this is really any component such as a lens
> controller.

All I am saying is that there should be a way to indicate that you accept
that parts are faulty, and that you (i.e. userspace) are able to detect
and handle that.

You can't just change the current behavior and expect existing applications
to work. E.g. says a sensor failed. Today the application might detect that
the video node didn't come up, so something is seriously wrong with the hardware
and it shows a message on the display. If this would change and the video node
*would* come up, even though there is no sensor the behavior of the application
would almost certainly change unexpectedly.

How to select which behavior you want isn't easy. The only thing I can come up
with is a module option. Not very elegant, unfortunately. But it doesn't
belong in the DT, and when userspace gets involved it is already too late.

Regards,

	Hans

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Sakari Ailus]

Hi Hans,

On Thu, Jul 20, 2017 at 06:23:38PM +0200, Hans Verkuil wrote:
> On 20/07/17 18:09, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Jul 19, 2017 at 01:24:54PM +0200, Hans Verkuil wrote:
> >> On 18/07/17 21:03, Sakari Ailus wrote:
> >>> The async notifier supports three callbacks to the notifier: bound, unbound
> >>> and complete. The complete callback has been traditionally used for
> >>> creating the sub-device nodes.
> >>>
> >>> This approach has an inherent weakness: if registration of a single
> >>> sub-device fails for whatever reason, it renders the entire media device
> >>> unusable even if only that piece of hardware is not working. This is a
> >>> problem in particular in systems with multiple independent image pipelines
> >>> on a single device. We have had such devices (e.g. omap3isp) supported for
> >>> a number of years and the problem is growing more pressing as time passes
> >>> so there is an incentive to resolve this.
> >>
> >> I don't think this is a good reason. If one of the subdevices fail, then your
> >> hardware is messed up and there is no point in continuing.
> > 
> > That's entirely untrue in general case.
> > 
> > If you have e.g. a mobile phone with a single camera, yes, you're right.
> > But most mobile phones have two cameras these days. Embedded systems may
> > have many, think of automotive use cases: you could have five or ten
> > cameras there.
> 
> These are all very recent developments. Today userspace can safely assume
> that either everything would be up and running, or nothing at all.
> 
> > It is not feasible to prevent the entire system from working if a single
> > component is at fault --- this is really any component such as a lens
> > controller.
> 
> All I am saying is that there should be a way to indicate that you accept
> that parts are faulty, and that you (i.e. userspace) are able to detect
> and handle that.
> 
> You can't just change the current behavior and expect existing applications
> to work. E.g. says a sensor failed. Today the application might detect that
> the video node didn't come up, so something is seriously wrong with the hardware
> and it shows a message on the display. If this would change and the video node
> *would* come up, even though there is no sensor the behavior of the application
> would almost certainly change unexpectedly.
> 
> How to select which behavior you want isn't easy. The only thing I can come up
> with is a module option. Not very elegant, unfortunately. But it doesn't
> belong in the DT, and when userspace gets involved it is already too late.

Module options don't scale if you want to change kernel interface
behaviour. Adding a Kconfig option would. We can neither make this
application specific since the application isn't known by the time the
nodes are created.

Kconfig option (that defaults to no) with the events and media device
status info amended with documentation change would achieve the goal,
although it'd take a lot of time to adjust all the applications before the
Kconfig option can be safely removed. This approach does have the benefit
of being able to provide the feature to those systems that really depend on
it.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Kieran Bingham]

Hi Hans,

On 20/07/17 17:23, Hans Verkuil wrote:
> On 20/07/17 18:09, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Wed, Jul 19, 2017 at 01:24:54PM +0200, Hans Verkuil wrote:
>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>> The async notifier supports three callbacks to the notifier: bound, unbound
>>>> and complete. The complete callback has been traditionally used for
>>>> creating the sub-device nodes.
>>>>
>>>> This approach has an inherent weakness: if registration of a single
>>>> sub-device fails for whatever reason, it renders the entire media device
>>>> unusable even if only that piece of hardware is not working. This is a
>>>> problem in particular in systems with multiple independent image pipelines
>>>> on a single device. We have had such devices (e.g. omap3isp) supported for
>>>> a number of years and the problem is growing more pressing as time passes
>>>> so there is an incentive to resolve this.
>>>
>>> I don't think this is a good reason. If one of the subdevices fail, then your
>>> hardware is messed up and there is no point in continuing.
>>
>> That's entirely untrue in general case.

Adding my 2 cents ... because I'm hitting this ... right now.

>>
>> If you have e.g. a mobile phone with a single camera, yes, you're right.
>> But most mobile phones have two cameras these days. Embedded systems may
>> have many, think of automotive use cases: you could have five or ten
>> cameras there.
I have a MAX9286 which has 4 camera subdevices connected.

Right now, if *ONE* fails - they all fail. - This is very much undesirable
behaviour.

In this instance, when one fails (perhaps I have not connected one camera) then
the remaining camera subdevices all probe successfully, but the complete
callback is never called. Therefore the rest of my pipeline is dead, - But that
could now mean my reversing camera is not working because my wing mirror camera
was 'knocked' off... :-(


> These are all very recent developments. Today userspace can safely assume
> that either everything would be up and running, or nothing at all.

This is a strong point, and I'm struggling to decide if I agree or not :D

There are so many use cases, it's hard to make one statement fit all.

For example - currently - an analogue input source might not be connected - but
userspace may not know that, and would instead capture a black screen.

Maybe that doesn't even match ... I'm tired ;)


>> It is not feasible to prevent the entire system from working if a single
>> component is at fault --- this is really any component such as a lens
>> controller.
> 
> All I am saying is that there should be a way to indicate that you accept
> that parts are faulty, and that you (i.e. userspace) are able to detect
> and handle that.
>
> You can't just change the current behavior and expect existing applications
> to work. E.g. says a sensor failed. Today the application might detect that
> the video node didn't come up, so something is seriously wrong with the hardware
> and it shows a message on the display. If this would change and the video node
> *would* come up, even though there is no sensor the behavior of the application
> would almost certainly change unexpectedly.
> 
> How to select which behavior you want isn't easy. The only thing I can come up
> with is a module option. Not very elegant, unfortunately. But it doesn't
> belong in the DT, and when userspace gets involved it is already too late.

Yes, this sounds nasty - but 3 out of 4 working cameras being disabled / not
coming up because one failed sounds worse to me currently (I would say that ...
my cameras didn't come up) ... :-(

<thinking cap on ... going to bed>

--
Kieran

> Regards,
> 
> 	Hans
> 

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Niklas Söderlund]

Hi Sakari,

On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
> > On 18/07/17 21:03, Sakari Ailus wrote:
> > > Hi folks,
> > > 
> > > This RFC patchset achieves a number of things which I've put to the same
> > > patchset for they need to be show together to demonstrate the use cases.
> > > 
> > > I don't really intend this to compete with Niklas's patchset but much of
> > > the problem area addressed by the two is the same.
> > > 
> > > Comments would be welcome.
> > > 
> > > - Add AS3645A LED flash class driver.
> > > 
> > > - Add async notifiers (by Niklas).
> > > 
> > > - V4L2 sub-device node registration is moved to take place at the same time
> > >   with the registration of the sub-device itself. With this change,
> > >   sub-device node registration behaviour is aligned with video node
> > >   registration.
> > > 
> > > - The former is made possible by moving the bound() callback after
> > >   sub-device registration.
> > > 
> > > - As all the device node registration and link creation is done as the
> > >   respective devices are probed, there is no longer dependency to the
> > >   notifier complete callback which as itself is seen problematic. The
> > >   complete callback still exists but there's no need to use it, pending
> > >   changes in individual drivers.
> > > 
> > >   See:
> > >   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> > > 
> > >   As a result, if a part of the media device fails to initialise because it
> > >   is e.g. physically broken, it will be possible to use what works.
> > 
> > I've got major problems with this from a userspace point of view. In the vast
> > majority of cases you just want to bail out if one or more subdevs fail.
> 
> I admit it's easier for the user space if the device becomes available only
> when all its component drivers have registered.
> 
> Also remember that video nodes are registered in the file system right on
> device probe time. It's only sub-device and media device node registration
> that has taken place in the notifier's complete handler.

Is this always the case? In the R-Car VIN driver I register the video 
devices using video_register_device() in the complete handler. Am I 
doing things wrong in that driver? I had a patch where I moved the 
video_register_device() call to probe time but it got shoot down in 
review and was dropped.

> 
> There are things how we could do this easier, for instance providing events
> on the media device on entity registration and possibly registration state
> (whether all entities have been registered).
> 
> This way the user space can easily choose whether it would like to proceed
> using a partially functional device (or not).
> 
> -- 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

Re: [RFC 11/19] v4l2-async: Register sub-devices before calling bound callback

[Hans Verkuil]

On 20/07/17 21:23, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Jul 20, 2017 at 06:23:38PM +0200, Hans Verkuil wrote:
>> On 20/07/17 18:09, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Jul 19, 2017 at 01:24:54PM +0200, Hans Verkuil wrote:
>>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>>> The async notifier supports three callbacks to the notifier: bound, unbound
>>>>> and complete. The complete callback has been traditionally used for
>>>>> creating the sub-device nodes.
>>>>>
>>>>> This approach has an inherent weakness: if registration of a single
>>>>> sub-device fails for whatever reason, it renders the entire media device
>>>>> unusable even if only that piece of hardware is not working. This is a
>>>>> problem in particular in systems with multiple independent image pipelines
>>>>> on a single device. We have had such devices (e.g. omap3isp) supported for
>>>>> a number of years and the problem is growing more pressing as time passes
>>>>> so there is an incentive to resolve this.
>>>>
>>>> I don't think this is a good reason. If one of the subdevices fail, then your
>>>> hardware is messed up and there is no point in continuing.
>>>
>>> That's entirely untrue in general case.
>>>
>>> If you have e.g. a mobile phone with a single camera, yes, you're right.
>>> But most mobile phones have two cameras these days. Embedded systems may
>>> have many, think of automotive use cases: you could have five or ten
>>> cameras there.
>>
>> These are all very recent developments. Today userspace can safely assume
>> that either everything would be up and running, or nothing at all.
>>
>>> It is not feasible to prevent the entire system from working if a single
>>> component is at fault --- this is really any component such as a lens
>>> controller.
>>
>> All I am saying is that there should be a way to indicate that you accept
>> that parts are faulty, and that you (i.e. userspace) are able to detect
>> and handle that.
>>
>> You can't just change the current behavior and expect existing applications
>> to work. E.g. says a sensor failed. Today the application might detect that
>> the video node didn't come up, so something is seriously wrong with the hardware
>> and it shows a message on the display. If this would change and the video node
>> *would* come up, even though there is no sensor the behavior of the application
>> would almost certainly change unexpectedly.
>>
>> How to select which behavior you want isn't easy. The only thing I can come up
>> with is a module option. Not very elegant, unfortunately. But it doesn't
>> belong in the DT, and when userspace gets involved it is already too late.
> 
> Module options don't scale if you want to change kernel interface
> behaviour. Adding a Kconfig option would. We can neither make this
> application specific since the application isn't known by the time the
> nodes are created.
> 
> Kconfig option (that defaults to no) with the events and media device
> status info amended with documentation change would achieve the goal,
> although it'd take a lot of time to adjust all the applications before the
> Kconfig option can be safely removed. This approach does have the benefit
> of being able to provide the feature to those systems that really depend on
> it.

That sounds like a good idea. I don't think you ever want to remove this
option in the future. Depending on the product I think there is often a very
good case to be made to just fail if one component failed.

You need specialized applications that can handle partially configured systems,
and having to explicitly enable this is a good way to force users to carefully
think about this.

Some notes:

I recommend to first finish this async rework. Adding support for this new
feature should be discussed a bit more before we start work on that.

Some discussion points:

1) What about adding time-out support? Today we wait forever until all components
   are found, but wouldn't it make sense to optionally time-out? And if so, then
   we can keep most of the code the same and decide in the complete() callback
   whether or not we accept missing components. And decide how badly 'impaired'
   the system is. We can also still bring up all the devices in the complete rather
   than one-by-one as you proposed (and which I am not sure I like).

2) This can be hard to test, so perhaps we should support some form of error
   injection to easily test what happens if something doesn't come up.

Regards,

	Hans

Re: [RFC 08/19] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Pavel Machek]

On Tue 2017-07-18 22:03:50, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Add the as3645a flash controller to the DT source as well as the flash
> property with the as3645a device phandle to the sensor DT node.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Acked-by: Pavel Machek <pavel@ucw.cz>

Re: [RFC 1/1] v4l2-subdev: Add a function to set sub-device notifier callbacks

[Mauro Carvalho Chehab]

Em Thu, 20 Jul 2017 01:33:29 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> The sub-device's sub-notifier is hidded in the sub-device and not meant to
> be accessed directly by drivers. Still the driver may wish to set callbacks
> to the notifier. Add a function to do that:
> v4l2_subdev_notifier_set_callbacks().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Well, this appears to be quite straightforward. The code is entirely untested
> but trivial at the same time. 
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 20 ++++++++++++++++++++
>  include/media/v4l2-subdev.h           |  6 ++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index a6976d4a52ac..8629224bfdba 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -666,3 +666,23 @@ int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd)
>  	return v4l2_fwnode_reference_parse_sensor_common(sd->dev, subnotifier);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_fwnode_reference_parse_sensor_common);
> +
> +int v4l2_subdev_notifier_set_callbacks(
> +	struct v4l2_subdev *sd,
> +	int (*bound)(struct v4l2_async_notifier *notifier,
> +		     struct v4l2_subdev *subdev,
> +		     struct v4l2_async_subdev *asd),
> +	int (*complete)(struct v4l2_async_notifier *notifier))
> +{
> +	struct v4l2_async_notifier *subnotifier =
> +		v4l2_subdev_get_subnotifier(sd);
> +
> +	if (!subnotifier)
> +		return -ENOMEM;
> +
> +	subnotifier->bound = bound;
> +	subnotifier->complete = complete;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_notifier_set_callbacks);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e309a2e2030b..ee85b64ad4f4 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1012,4 +1012,10 @@ int v4l2_subdev_fwnode_endpoints_parse(
>  
>  int v4l2_subdev_fwnode_reference_parse_sensor_common(struct v4l2_subdev *sd);
>  
> +int v4l2_subdev_notifier_set_callbacks(
> +	struct v4l2_subdev *sd,
> +	int (*bound)(struct v4l2_async_notifier *notifier,
> +		     struct v4l2_subdev *subdev,
> +		     struct v4l2_async_subdev *asd),
> +	int (*complete)(struct v4l2_async_notifier *notifier));

I guess currently v4l2-subdev.h is not included at the kAPI guide,
but it should (patches documenting it are welcomed!).

Yet, let's try to not increase the documentation gap here. So,
if we're willing to add this upstream, please add the 
kernel-doc macro for such function at the final patch, and test it.

I suspect that the best is to use typedefs for bound and complete,
in order for them to be properly documented and to be parsed by 
kernel-doc/Sphinx.

Regards,

Thanks,
Mauro

Re: [RFC 04/19] dt: bindings: Add lens-focus binding for image sensors

[Hans Verkuil]

Hi Sakari,

Is the lens-focus phandle specific to voice-coil controllers? What about
motor-controlled focus systems? What when there are multiple motors? E.g.
one for the focus, one for the iris control (yes, we have that).

What if you have two sensors (stereoscopic) controlled by one motor?

Just trying to understand this from a bigger perspective. Specifically
how this will scale when more of these supporting devices as added.

Regards,

	Hans

On 07/18/2017 09:03 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.
> 
> 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 9723f7e1c7db..a18d9b2d309f 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -74,6 +74,8 @@ Optional properties
>  - flash: phandle referring to the flash driver chip. A flash driver may
>    have multiple flashes connected to it.
>  
> +- lens-focus: A phandle to the node of the focus lens controller.
> +
>  
>  Optional endpoint properties
>  ----------------------------
> 

Re: [RFC 04/19] dt: bindings: Add lens-focus binding for image sensors

[Pavel Machek]

Hi!

> Is the lens-focus phandle specific to voice-coil controllers? What about
> motor-controlled focus systems? What when there are multiple motors? E.g.
> one for the focus, one for the iris control (yes, we have that).

I'd say motor vs. coil is not important. Iris control should go to
lens-iris: or something.

> What if you have two sensors (stereoscopic) controlled by one motor?

Are there such systems? I'd say lets solve it if we see it...

> >  - flash: phandle referring to the flash driver chip. A flash driver may
> >    have multiple flashes connected to it.
> >  
> > +- lens-focus: A phandle to the node of the focus lens controller.
> > +
> >  

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Sakari Ailus]

Niklas Söderlund wrote:
> Hi Sakari,
> 
> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
>> Hi Hans,
>>
>> Thanks for the review.
>>
>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>> Hi folks,
>>>>
>>>> This RFC patchset achieves a number of things which I've put to the same
>>>> patchset for they need to be show together to demonstrate the use cases.
>>>>
>>>> I don't really intend this to compete with Niklas's patchset but much of
>>>> the problem area addressed by the two is the same.
>>>>
>>>> Comments would be welcome.
>>>>
>>>> - Add AS3645A LED flash class driver.
>>>>
>>>> - Add async notifiers (by Niklas).
>>>>
>>>> - V4L2 sub-device node registration is moved to take place at the same time
>>>>   with the registration of the sub-device itself. With this change,
>>>>   sub-device node registration behaviour is aligned with video node
>>>>   registration.
>>>>
>>>> - The former is made possible by moving the bound() callback after
>>>>   sub-device registration.
>>>>
>>>> - As all the device node registration and link creation is done as the
>>>>   respective devices are probed, there is no longer dependency to the
>>>>   notifier complete callback which as itself is seen problematic. The
>>>>   complete callback still exists but there's no need to use it, pending
>>>>   changes in individual drivers.
>>>>
>>>>   See:
>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
>>>>
>>>>   As a result, if a part of the media device fails to initialise because it
>>>>   is e.g. physically broken, it will be possible to use what works.
>>>
>>> I've got major problems with this from a userspace point of view. In the vast
>>> majority of cases you just want to bail out if one or more subdevs fail.
>>
>> I admit it's easier for the user space if the device becomes available only
>> when all its component drivers have registered.
>>
>> Also remember that video nodes are registered in the file system right on
>> device probe time. It's only sub-device and media device node registration
>> that has taken place in the notifier's complete handler.
> 
> Is this always the case? In the R-Car VIN driver I register the video 
> devices using video_register_device() in the complete handler. Am I 
> doing things wrong in that driver? I had a patch where I moved the 
> video_register_device() call to probe time but it got shoot down in 
> review and was dropped.

I don't think the current implementation is wrong, it's just different
from other drivers; there's really no requirement regarding this AFAIU.
It's one of the things where no attention has been paid I presume.

However doing anything that can fail earlier on would be nicer since
there's no reasonable way to signal an error from complete callback either.

-- 
Regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC 04/19] dt: bindings: Add lens-focus binding for image sensors

[Sakari Ailus]

Hi Hans,

On Fri, Jul 28, 2017 at 10:53:35AM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> Is the lens-focus phandle specific to voice-coil controllers? What about

I think it's not important. Right now the information is used for making
the association only.

> motor-controlled focus systems? What when there are multiple motors? E.g.
> one for the focus, one for the iris control (yes, we have that).

I'd add another property for this purpose.

> 
> What if you have two sensors (stereoscopic) controlled by one motor?

You can refer to the same controller from both, I don't see a problem with
that. The software would need to support that though, I think changes to
the async framework would be needed.

> 
> Just trying to understand this from a bigger perspective. Specifically
> how this will scale when more of these supporting devices as added.
> 
> Regards,
> 
> 	Hans
> 
> On 07/18/2017 09:03 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.
> > 
> > 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 9723f7e1c7db..a18d9b2d309f 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -74,6 +74,8 @@ Optional properties
> >  - flash: phandle referring to the flash driver chip. A flash driver may
> >    have multiple flashes connected to it.
> >  
> > +- lens-focus: A phandle to the node of the focus lens controller.
> > +
> >  
> >  Optional endpoint properties
> >  ----------------------------
> > 
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Hans Verkuil]

On 08/04/17 20:25, Sakari Ailus wrote:
> Niklas Söderlund wrote:
>> Hi Sakari,
>>
>> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the review.
>>>
>>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
>>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>>> Hi folks,
>>>>>
>>>>> This RFC patchset achieves a number of things which I've put to the same
>>>>> patchset for they need to be show together to demonstrate the use cases.
>>>>>
>>>>> I don't really intend this to compete with Niklas's patchset but much of
>>>>> the problem area addressed by the two is the same.
>>>>>
>>>>> Comments would be welcome.
>>>>>
>>>>> - Add AS3645A LED flash class driver.
>>>>>
>>>>> - Add async notifiers (by Niklas).
>>>>>
>>>>> - V4L2 sub-device node registration is moved to take place at the same time
>>>>>   with the registration of the sub-device itself. With this change,
>>>>>   sub-device node registration behaviour is aligned with video node
>>>>>   registration.
>>>>>
>>>>> - The former is made possible by moving the bound() callback after
>>>>>   sub-device registration.
>>>>>
>>>>> - As all the device node registration and link creation is done as the
>>>>>   respective devices are probed, there is no longer dependency to the
>>>>>   notifier complete callback which as itself is seen problematic. The
>>>>>   complete callback still exists but there's no need to use it, pending
>>>>>   changes in individual drivers.
>>>>>
>>>>>   See:
>>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
>>>>>
>>>>>   As a result, if a part of the media device fails to initialise because it
>>>>>   is e.g. physically broken, it will be possible to use what works.
>>>>
>>>> I've got major problems with this from a userspace point of view. In the vast
>>>> majority of cases you just want to bail out if one or more subdevs fail.
>>>
>>> I admit it's easier for the user space if the device becomes available only
>>> when all its component drivers have registered.
>>>
>>> Also remember that video nodes are registered in the file system right on
>>> device probe time. It's only sub-device and media device node registration
>>> that has taken place in the notifier's complete handler.
>>
>> Is this always the case? In the R-Car VIN driver I register the video 
>> devices using video_register_device() in the complete handler. Am I 
>> doing things wrong in that driver? I had a patch where I moved the 
>> video_register_device() call to probe time but it got shoot down in 
>> review and was dropped.
> 
> I don't think the current implementation is wrong, it's just different
> from other drivers; there's really no requirement regarding this AFAIU.
> It's one of the things where no attention has been paid I presume.

It actually is a requirement: when a device node appears applications can
reasonably expect to have a fully functioning device. True for any device
node. You don't want to have to wait until some unspecified time before
the full functionality is there.

I try to pay attention to this when reviewing code, since not following this
rule basically introduces a race condition which is hard to test.

> However doing anything that can fail earlier on would be nicer since
> there's no reasonable way to signal an error from complete callback either.

Right.

Adding support for cases where devices may not be present is very desirable,
but this should go through an RFC process first to hammer out all the details.

Today we do not support this and we have to review code with that in mind.

So the first async subnotifiers implementation should NOT support this (although
it can of course be designed with this in mind). Once it is in we can start
on an RFC on how to support partial pipelines. I have a lot of questions about
that that need to be answered first.

One thing at a time. Trying to do everything at once never works.

Regards,

	Hans

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Hi!

> >> Is this always the case? In the R-Car VIN driver I register the video 
> >> devices using video_register_device() in the complete handler. Am I 
> >> doing things wrong in that driver? I had a patch where I moved the 
> >> video_register_device() call to probe time but it got shoot down in 
> >> review and was dropped.
> > 
> > I don't think the current implementation is wrong, it's just different
> > from other drivers; there's really no requirement regarding this AFAIU.
> > It's one of the things where no attention has been paid I presume.
> 
> It actually is a requirement: when a device node appears applications can
> reasonably expect to have a fully functioning device. True for any device
> node. You don't want to have to wait until some unspecified time before
> the full functionality is there.

Well... /dev/sdb appears, but you still get -ENOMEDIA before user
presses "Turn on USB storage" button on android phone.

So I agree it is not desirable, but it sometimes happens.

									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 --]

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Hans Verkuil]

On 08/23/17 13:34, Pavel Machek wrote:
> Hi!
> 
>>>> Is this always the case? In the R-Car VIN driver I register the video 
>>>> devices using video_register_device() in the complete handler. Am I 
>>>> doing things wrong in that driver? I had a patch where I moved the 
>>>> video_register_device() call to probe time but it got shoot down in 
>>>> review and was dropped.
>>>
>>> I don't think the current implementation is wrong, it's just different
>>> from other drivers; there's really no requirement regarding this AFAIU.
>>> It's one of the things where no attention has been paid I presume.
>>
>> It actually is a requirement: when a device node appears applications can
>> reasonably expect to have a fully functioning device. True for any device
>> node. You don't want to have to wait until some unspecified time before
>> the full functionality is there.
> 
> Well... /dev/sdb appears, but you still get -ENOMEDIA before user
> presses "Turn on USB storage" button on android phone.
> 
> So I agree it is not desirable, but it sometimes happens.

But that is expected behavior. There is nothing wrong with the device.
Just as it is expected behavior that you can't stream from a video node
if there is no HDMI source connected.

That the HDMI receiver or sensor itself is completely missing is quite
another story, though.

Regards,

	Hans

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Laurent Pinchart]

Hi Hans,

On Wednesday, 23 August 2017 12:09:15 EEST Hans Verkuil wrote:
> On 08/04/17 20:25, Sakari Ailus wrote:
> > Niklas Söderlund wrote:
> >> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
> >>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
> >>>> On 18/07/17 21:03, Sakari Ailus wrote:
> >>>>> Hi folks,
> >>>>> 
> >>>>> This RFC patchset achieves a number of things which I've put to the
> >>>>> same patchset for they need to be show together to demonstrate the use
> >>>>> cases.
> >>>>> 
> >>>>> I don't really intend this to compete with Niklas's patchset but much
> >>>>> of the problem area addressed by the two is the same.
> >>>>> 
> >>>>> Comments would be welcome.
> >>>>> 
> >>>>> - Add AS3645A LED flash class driver.
> >>>>> 
> >>>>> - Add async notifiers (by Niklas).
> >>>>> 
> >>>>> - V4L2 sub-device node registration is moved to take place at the same
> >>>>>   time with the registration of the sub-device itself. With this
> >>>>>   change, sub-device node registration behaviour is aligned with video
> >>>>>   node registration.
> >>>>> 
> >>>>> - The former is made possible by moving the bound() callback after
> >>>>>   sub-device registration.
> >>>>> 
> >>>>> - As all the device node registration and link creation is done as the
> >>>>>   respective devices are probed, there is no longer dependency to the
> >>>>>   notifier complete callback which as itself is seen problematic. The
> >>>>>   complete callback still exists but there's no need to use it,
> >>>>>   pending changes in individual drivers.
> >>>>>   
> >>>>>   See:
> >>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> >>>>>   
> >>>>>   As a result, if a part of the media device fails to initialise
> >>>>>   because it is e.g. physically broken, it will be possible to use
> >>>>>   what works.
> >>>> 
> >>>> I've got major problems with this from a userspace point of view. In
> >>>> the vast majority of cases you just want to bail out if one or more
> >>>> subdevs fail.
> >>>
> >>> I admit it's easier for the user space if the device becomes available
> >>> only when all its component drivers have registered.
> >>> 
> >>> Also remember that video nodes are registered in the file system right
> >>> on device probe time. It's only sub-device and media device node
> >>> registration that has taken place in the notifier's complete handler.
> >> 
> >> Is this always the case? In the R-Car VIN driver I register the video
> >> devices using video_register_device() in the complete handler. Am I
> >> doing things wrong in that driver? I had a patch where I moved the
> >> video_register_device() call to probe time but it got shoot down in
> >> review and was dropped.
> > 
> > I don't think the current implementation is wrong, it's just different
> > from other drivers; there's really no requirement regarding this AFAIU.
> > It's one of the things where no attention has been paid I presume.
> 
> It actually is a requirement: when a device node appears applications can
> reasonably expect to have a fully functioning device. True for any device
> node.

Why not ? I'm not aware of any such kernel-wide requirement. 

> You don't want to have to wait until some unspecified time before the full
> functionality is there.

We certainly should specify that time and give userspace a way to find out 
what is usable and when.

> I try to pay attention to this when reviewing code, since not following this
> rule basically introduces a race condition which is hard to test.
> 
> > However doing anything that can fail earlier on would be nicer since
> > there's no reasonable way to signal an error from complete callback
> > either.
> 
> Right.
> 
> Adding support for cases where devices may not be present is very desirable,
> but this should go through an RFC process first to hammer out all the
> details.
> 
> Today we do not support this and we have to review code with that in mind.
> 
> So the first async subnotifiers implementation should NOT support this
> (although it can of course be designed with this in mind).

I very much disagree. The first async subnotifiers implementation (and I still 
believe we don't need subnotifiers, there's nothing "sub" in them) shall 
support this. If it means we first have to hammer out the details of out it 
will work, so be it.

> Once it is in we can start on an RFC on how to support partial pipelines. I
> have a lot of questions about that that need to be answered first.
> 
> One thing at a time. Trying to do everything at once never works.

Sure, so let's start with probe time device node registration, and then move 
on to subnotifiers.

-- 
Regards,

Laurent Pinchart

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Niklas Söderlund]

Hi Hans,

On 2017-08-23 11:09:15 +0200, Hans Verkuil wrote:
> On 08/04/17 20:25, Sakari Ailus wrote:
> > Niklas Söderlund wrote:
> >> Hi Sakari,
> >>
> >> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> Thanks for the review.
> >>>
> >>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
> >>>> On 18/07/17 21:03, Sakari Ailus wrote:
> >>>>> Hi folks,
> >>>>>
> >>>>> This RFC patchset achieves a number of things which I've put to the same
> >>>>> patchset for they need to be show together to demonstrate the use cases.
> >>>>>
> >>>>> I don't really intend this to compete with Niklas's patchset but much of
> >>>>> the problem area addressed by the two is the same.
> >>>>>
> >>>>> Comments would be welcome.
> >>>>>
> >>>>> - Add AS3645A LED flash class driver.
> >>>>>
> >>>>> - Add async notifiers (by Niklas).
> >>>>>
> >>>>> - V4L2 sub-device node registration is moved to take place at the same time
> >>>>>   with the registration of the sub-device itself. With this change,
> >>>>>   sub-device node registration behaviour is aligned with video node
> >>>>>   registration.
> >>>>>
> >>>>> - The former is made possible by moving the bound() callback after
> >>>>>   sub-device registration.
> >>>>>
> >>>>> - As all the device node registration and link creation is done as the
> >>>>>   respective devices are probed, there is no longer dependency to the
> >>>>>   notifier complete callback which as itself is seen problematic. The
> >>>>>   complete callback still exists but there's no need to use it, pending
> >>>>>   changes in individual drivers.
> >>>>>
> >>>>>   See:
> >>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> >>>>>
> >>>>>   As a result, if a part of the media device fails to initialise because it
> >>>>>   is e.g. physically broken, it will be possible to use what works.
> >>>>
> >>>> I've got major problems with this from a userspace point of view. In the vast
> >>>> majority of cases you just want to bail out if one or more subdevs fail.
> >>>
> >>> I admit it's easier for the user space if the device becomes available only
> >>> when all its component drivers have registered.
> >>>
> >>> Also remember that video nodes are registered in the file system right on
> >>> device probe time. It's only sub-device and media device node registration
> >>> that has taken place in the notifier's complete handler.
> >>
> >> Is this always the case? In the R-Car VIN driver I register the video 
> >> devices using video_register_device() in the complete handler. Am I 
> >> doing things wrong in that driver? I had a patch where I moved the 
> >> video_register_device() call to probe time but it got shoot down in 
> >> review and was dropped.
> > 
> > I don't think the current implementation is wrong, it's just different
> > from other drivers; there's really no requirement regarding this AFAIU.
> > It's one of the things where no attention has been paid I presume.
> 
> It actually is a requirement: when a device node appears applications can
> reasonably expect to have a fully functioning device. True for any device
> node. You don't want to have to wait until some unspecified time before
> the full functionality is there.
> 
> I try to pay attention to this when reviewing code, since not following this
> rule basically introduces a race condition which is hard to test.

In the latest version of the R-Car VIN series I have now moved the video 
device registration to happen at probe time... So I think it would be a 
good time to clarify what and what is not the intended way of where this 
can happen. I'm not keen on reworking that series for each time it's 
posted to where the video device is registered :-) I can see both good 
and bad things with both solutions.

If the video device is registered in the complete callback it do make it 
easier to spot some race conditions (my VIN series got review comments 
where I missed this almost instantly). It is also clear to user-space 
when a device is ready to be used. At the same time as Sakari points out 
this prevents partially complete graphs which might contain a valid 
pipeline to be able to function, which of these two behaviors is the 
most opportune I assume differs with use-cases and which one is best 
from a framework point of view I don't know.

But I do know that if a video device is registered from the complete 
callback it's reasonable that it should be unregistered if the unbind 
callback is called, right? Else the same situation as registering it at 
probe time is reached if a subdevice is ever unbound and the driver 
needs to handle the corner cases of both situations. And this use-case 
is today broken in v4l2! If a video device is registered in the complete 
callback, unregistered in the unbind callback and later re-registered in 
the complete callback once the subdevice is re-bound everything blows up 
with

  kobject (eb3be918): tried to init an initialized object, something is seriously wrong.

But yes if the video device is registered at probe time there are more 
races and object life-time issues for the driver to handle, but these 
needs to be considered anyhow if the unbind/re-bind scenario is to be 
fixed, right? So maybe it don't really matter where the video device is 
registered and both methods should be allowed and documented (so all 
drivers returns same -ENOSUBDEVBOUNDYET etc) and leave it up to each 
driver to handle this for how it perceives it primary use-case to be?  
And instead we should talk about how to fix the bind/unbind issues as 
this is where IMHO where the real problem is.

> 
> > However doing anything that can fail earlier on would be nicer since
> > there's no reasonable way to signal an error from complete callback either.
> 
> Right.
> 
> Adding support for cases where devices may not be present is very desirable,
> but this should go through an RFC process first to hammer out all the details.
> 
> Today we do not support this and we have to review code with that in mind.
> 
> So the first async subnotifiers implementation should NOT support this (although
> it can of course be designed with this in mind). Once it is in we can start
> on an RFC on how to support partial pipelines. I have a lot of questions about
> that that need to be answered first.
> 
> One thing at a time. Trying to do everything at once never works.
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Laurent Pinchart]

Hi Niklas,

On Wednesday, 23 August 2017 15:59:10 EEST Niklas Söderlund wrote:
> On 2017-08-23 11:09:15 +0200, Hans Verkuil wrote:
> > On 08/04/17 20:25, Sakari Ailus wrote:
> >> Niklas Söderlund wrote:
> >>> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
> >>>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
> >>>>> On 18/07/17 21:03, Sakari Ailus wrote:
> >>>>>> Hi folks,
> >>>>>> 
> >>>>>> This RFC patchset achieves a number of things which I've put to the
> >>>>>> same patchset for they need to be show together to demonstrate the
> >>>>>> use cases.
> >>>>>> 
> >>>>>> I don't really intend this to compete with Niklas's patchset but
> >>>>>> much of the problem area addressed by the two is the same.
> >>>>>> 
> >>>>>> Comments would be welcome.
> >>>>>> 
> >>>>>> - Add AS3645A LED flash class driver.
> >>>>>> 
> >>>>>> - Add async notifiers (by Niklas).
> >>>>>> 
> >>>>>> - V4L2 sub-device node registration is moved to take place at the
> >>>>>>   same time with the registration of the sub-device itself. With
> >>>>>>   this change, sub-device node registration behaviour is aligned
> >>>>>>   with video node registration.
> >>>>>> 
> >>>>>> - The former is made possible by moving the bound() callback after
> >>>>>>   sub-device registration.
> >>>>>> 
> >>>>>> - As all the device node registration and link creation is done as
> >>>>>>   the respective devices are probed, there is no longer dependency
> >>>>>>   to the notifier complete callback which as itself is seen
> >>>>>>   problematic. The complete callback still exists but there's no
> >>>>>>   need to use it, pending changes in individual drivers.
> >>>>>>   
> >>>>>>   See:
> >>>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
> >>>>>>   
> >>>>>>   As a result, if a part of the media device fails to initialise
> >>>>>>   because it is e.g. physically broken, it will be possible to use
> >>>>>>   what works.
> >>>>> 
> >>>>> I've got major problems with this from a userspace point of view. In
> >>>>> the vast majority of cases you just want to bail out if one or more
> >>>>> subdevs fail.
> >>>>
> >>>> I admit it's easier for the user space if the device becomes available
> >>>> only when all its component drivers have registered.
> >>>> 
> >>>> Also remember that video nodes are registered in the file system right
> >>>> on device probe time. It's only sub-device and media device node
> >>>> registration that has taken place in the notifier's complete handler.
> >>> 
> >>> Is this always the case? In the R-Car VIN driver I register the video
> >>> devices using video_register_device() in the complete handler. Am I
> >>> doing things wrong in that driver? I had a patch where I moved the
> >>> video_register_device() call to probe time but it got shoot down in
> >>> review and was dropped.
> >> 
> >> I don't think the current implementation is wrong, it's just different
> >> from other drivers; there's really no requirement regarding this AFAIU.
> >> It's one of the things where no attention has been paid I presume.
> > 
> > It actually is a requirement: when a device node appears applications can
> > reasonably expect to have a fully functioning device. True for any device
> > node. You don't want to have to wait until some unspecified time before
> > the full functionality is there.
> > 
> > I try to pay attention to this when reviewing code, since not following
> > this rule basically introduces a race condition which is hard to test.
> 
> In the latest version of the R-Car VIN series I have now moved the video
> device registration to happen at probe time... So I think it would be a
> good time to clarify what and what is not the intended way of where this
> can happen. I'm not keen on reworking that series for each time it's
> posted to where the video device is registered :-) I can see both good
> and bad things with both solutions.

I agree, let's get to the bottom of this issue.

> If the video device is registered in the complete callback it do make it
> easier to spot some race conditions (my VIN series got review comments
> where I missed this almost instantly).

I'm not sure I agree with that. It removes several potential race conditions, 
but I don't see how it makes it easier to spot any of them.

> It is also clear to user-space when a device is ready to be used. At the
> same time as Sakari points out this prevents partially complete graphs which
> might contain a valid pipeline to be able to function, which of these two
> behaviors is the most opportune I assume differs with use-cases and which
> one is best from a framework point of view I don't know.

For drivers that support pipelines with multiple sources I don't think there's 
any disagreement. We need to support partial pipelines, and thus need to 
register video nodes at probe time. The case that we're debating here is vdev-
centric drivers that expose a single video node and no subdev-node, and have a 
single source. Is there a general agreement on this ?

> But I do know that if a video device is registered from the complete
> callback it's reasonable that it should be unregistered if the unbind
> callback is called, right?

I don't see any other way it could work in that case, do I'd say yes.

> Else the same situation as registering it at probe time is reached if a
> subdevice is ever unbound and the driver needs to handle the corner cases of
> both situations. And this use-case is today broken in v4l2! If a video
> device is registered in the complete callback, unregistered in the unbind
> callback and later re-registered in the complete callback once the subdevice
> is re-bound everything blows up with
> 
>   kobject (eb3be918): tried to init an initialized object, something is
> seriously wrong.

That's because reusing a struct device is wrong. In this case the video_device 
structure needs to be allocated dynamically at .complete() time, and 
unregistered at .unbind() time. It will then be freed from its .release() 
handler when the last reference will disappear, which could be way after the 
subdev is unbound or even rebound. In the latter case two struct video_device 
will coexist for some time.

> But yes if the video device is registered at probe time there are more
> races and object life-time issues for the driver to handle, but these
> needs to be considered anyhow if the unbind/re-bind scenario is to be
> fixed, right?

I believe so.

> So maybe it don't really matter where the video device is registered and
> both methods should be allowed and documented (so all drivers returns same
> -ENOSUBDEVBOUNDYET etc) and leave it up to each driver to handle this for
> how it perceives it primary use-case to be?

I have no strong opinion on whether the decision should be left to driver 
authors, but I believe we should have at least the option of registering video 
device nodes a probe time.

> And instead we should talk about how to fix the bind/unbind issues as this
> is where IMHO where the real problem is.

That I agree with. There's so many object lifetime management issues in V4L2 
that it's hard not to run into one of them during development (as proved again 
but this mail thread :-)). For me this is the number one priority. I'm working 
on it with Sakari this afternoon.

> >> However doing anything that can fail earlier on would be nicer since
> >> there's no reasonable way to signal an error from complete callback
> >> either.
> > 
> > Right.
> > 
> > Adding support for cases where devices may not be present is very
> > desirable, but this should go through an RFC process first to hammer out
> > all the details.
> > 
> > Today we do not support this and we have to review code with that in mind.
> > 
> > So the first async subnotifiers implementation should NOT support this
> > (although it can of course be designed with this in mind). Once it is in
> > we can start on an RFC on how to support partial pipelines. I have a lot
> > of questions about that that need to be answered first.
> > 
> > One thing at a time. Trying to do everything at once never works.

-- 
Regards,

Laurent Pinchart

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Hans Verkuil]

On 08/23/17 15:32, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Wednesday, 23 August 2017 15:59:10 EEST Niklas Söderlund wrote:
>> On 2017-08-23 11:09:15 +0200, Hans Verkuil wrote:
>>> On 08/04/17 20:25, Sakari Ailus wrote:
>>>> Niklas Söderlund wrote:
>>>>> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
>>>>>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
>>>>>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>>>>>> Hi folks,
>>>>>>>>
>>>>>>>> This RFC patchset achieves a number of things which I've put to the
>>>>>>>> same patchset for they need to be show together to demonstrate the
>>>>>>>> use cases.
>>>>>>>>
>>>>>>>> I don't really intend this to compete with Niklas's patchset but
>>>>>>>> much of the problem area addressed by the two is the same.
>>>>>>>>
>>>>>>>> Comments would be welcome.
>>>>>>>>
>>>>>>>> - Add AS3645A LED flash class driver.
>>>>>>>>
>>>>>>>> - Add async notifiers (by Niklas).
>>>>>>>>
>>>>>>>> - V4L2 sub-device node registration is moved to take place at the
>>>>>>>>   same time with the registration of the sub-device itself. With
>>>>>>>>   this change, sub-device node registration behaviour is aligned
>>>>>>>>   with video node registration.
>>>>>>>>
>>>>>>>> - The former is made possible by moving the bound() callback after
>>>>>>>>   sub-device registration.
>>>>>>>>
>>>>>>>> - As all the device node registration and link creation is done as
>>>>>>>>   the respective devices are probed, there is no longer dependency
>>>>>>>>   to the notifier complete callback which as itself is seen
>>>>>>>>   problematic. The complete callback still exists but there's no
>>>>>>>>   need to use it, pending changes in individual drivers.
>>>>>>>>   
>>>>>>>>   See:
>>>>>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
>>>>>>>>   
>>>>>>>>   As a result, if a part of the media device fails to initialise
>>>>>>>>   because it is e.g. physically broken, it will be possible to use
>>>>>>>>   what works.
>>>>>>>
>>>>>>> I've got major problems with this from a userspace point of view. In
>>>>>>> the vast majority of cases you just want to bail out if one or more
>>>>>>> subdevs fail.
>>>>>>
>>>>>> I admit it's easier for the user space if the device becomes available
>>>>>> only when all its component drivers have registered.
>>>>>>
>>>>>> Also remember that video nodes are registered in the file system right
>>>>>> on device probe time. It's only sub-device and media device node
>>>>>> registration that has taken place in the notifier's complete handler.
>>>>>
>>>>> Is this always the case? In the R-Car VIN driver I register the video
>>>>> devices using video_register_device() in the complete handler. Am I
>>>>> doing things wrong in that driver? I had a patch where I moved the
>>>>> video_register_device() call to probe time but it got shoot down in
>>>>> review and was dropped.
>>>>
>>>> I don't think the current implementation is wrong, it's just different
>>>> from other drivers; there's really no requirement regarding this AFAIU.
>>>> It's one of the things where no attention has been paid I presume.
>>>
>>> It actually is a requirement: when a device node appears applications can
>>> reasonably expect to have a fully functioning device. True for any device
>>> node. You don't want to have to wait until some unspecified time before
>>> the full functionality is there.
>>>
>>> I try to pay attention to this when reviewing code, since not following
>>> this rule basically introduces a race condition which is hard to test.
>>
>> In the latest version of the R-Car VIN series I have now moved the video
>> device registration to happen at probe time... So I think it would be a
>> good time to clarify what and what is not the intended way of where this
>> can happen. I'm not keen on reworking that series for each time it's
>> posted to where the video device is registered :-) I can see both good
>> and bad things with both solutions.
> 
> I agree, let's get to the bottom of this issue.
> 
>> If the video device is registered in the complete callback it do make it
>> easier to spot some race conditions (my VIN series got review comments
>> where I missed this almost instantly).
> 
> I'm not sure I agree with that. It removes several potential race conditions, 
> but I don't see how it makes it easier to spot any of them.
> 
>> It is also clear to user-space when a device is ready to be used. At the
>> same time as Sakari points out this prevents partially complete graphs which
>> might contain a valid pipeline to be able to function, which of these two
>> behaviors is the most opportune I assume differs with use-cases and which
>> one is best from a framework point of view I don't know.
> 
> For drivers that support pipelines with multiple sources I don't think there's 
> any disagreement. We need to support partial pipelines, and thus need to 
> register video nodes at probe time. The case that we're debating here is vdev-
> centric drivers that expose a single video node and no subdev-node, and have a 
> single source. Is there a general agreement on this ?

Regardless of whether this is vdev or mc centric, today userspace expects that
when devices appear, all the HW blocks and corresponding drivers are loaded,
configured and ready for use.

It doesn't matter what we want, we have to deal with what we have today. And
the only way to achieve this is to create the device nodes last, i.e. in the
completion callback.

Mind you, what should be done and what is actually done are two different
things. I suspect many drivers have race conditions relating to this as we
didn't always pay attention to this (I try to, though, because I have had
problems relating to this in the past).

Where things should be done once we allow partial configuration is something
we don't know yet, but personally I think it will still end up in a completion
like callback.

> 
>> But I do know that if a video device is registered from the complete
>> callback it's reasonable that it should be unregistered if the unbind
>> callback is called, right?
> 
> I don't see any other way it could work in that case, do I'd say yes.

I agree.

> 
>> Else the same situation as registering it at probe time is reached if a
>> subdevice is ever unbound and the driver needs to handle the corner cases of
>> both situations. And this use-case is today broken in v4l2! If a video
>> device is registered in the complete callback, unregistered in the unbind
>> callback and later re-registered in the complete callback once the subdevice
>> is re-bound everything blows up with
>>
>>   kobject (eb3be918): tried to init an initialized object, something is
>> seriously wrong.
> 
> That's because reusing a struct device is wrong. In this case the video_device 
> structure needs to be allocated dynamically at .complete() time, and 
> unregistered at .unbind() time. It will then be freed from its .release() 
> handler when the last reference will disappear, which could be way after the 
> subdev is unbound or even rebound. In the latter case two struct video_device 
> will coexist for some time.
> 
>> But yes if the video device is registered at probe time there are more
>> races and object life-time issues for the driver to handle, but these
>> needs to be considered anyhow if the unbind/re-bind scenario is to be
>> fixed, right?
> 
> I believe so.
> 
>> So maybe it don't really matter where the video device is registered and
>> both methods should be allowed and documented (so all drivers returns same
>> -ENOSUBDEVBOUNDYET etc) and leave it up to each driver to handle this for
>> how it perceives it primary use-case to be?
> 
> I have no strong opinion on whether the decision should be left to driver 
> authors, but I believe we should have at least the option of registering video 
> device nodes a probe time.

I think it is way too early to decide this. I for one see no reason whatsoever
why you would want to do that there. We haven't discussed the userspace API
for this at all (i.e. partial pipelines), and I have no idea what that will
look like. Stick to what we have today when reviewing driver code.

>> And instead we should talk about how to fix the bind/unbind issues as this
>> is where IMHO where the real problem is.
> 
> That I agree with. There's so many object lifetime management issues in V4L2 
> that it's hard not to run into one of them during development (as proved again 
> but this mail thread :-)). For me this is the number one priority. I'm working 
> on it with Sakari this afternoon.

Right, today v4l2 drivers don't reliably (or at all) support unbind.

It will be great if this is fixed.

Regards,

	Hans

>>>> However doing anything that can fail earlier on would be nicer since
>>>> there's no reasonable way to signal an error from complete callback
>>>> either.
>>>
>>> Right.
>>>
>>> Adding support for cases where devices may not be present is very
>>> desirable, but this should go through an RFC process first to hammer out
>>> all the details.
>>>
>>> Today we do not support this and we have to review code with that in mind.
>>>
>>> So the first async subnotifiers implementation should NOT support this
>>> (although it can of course be designed with this in mind). Once it is in
>>> we can start on an RFC on how to support partial pipelines. I have a lot
>>> of questions about that that need to be answered first.
>>>
>>> One thing at a time. Trying to do everything at once never works.
> 

Re: [RFC 00/19] Async sub-notifiers and how to use them

[Hans Verkuil]

On 08/23/17 14:29, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wednesday, 23 August 2017 12:09:15 EEST Hans Verkuil wrote:
>> On 08/04/17 20:25, Sakari Ailus wrote:
>>> Niklas Söderlund wrote:
>>>> On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:
>>>>> On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:
>>>>>> On 18/07/17 21:03, Sakari Ailus wrote:
>>>>>>> Hi folks,
>>>>>>>
>>>>>>> This RFC patchset achieves a number of things which I've put to the
>>>>>>> same patchset for they need to be show together to demonstrate the use
>>>>>>> cases.
>>>>>>>
>>>>>>> I don't really intend this to compete with Niklas's patchset but much
>>>>>>> of the problem area addressed by the two is the same.
>>>>>>>
>>>>>>> Comments would be welcome.
>>>>>>>
>>>>>>> - Add AS3645A LED flash class driver.
>>>>>>>
>>>>>>> - Add async notifiers (by Niklas).
>>>>>>>
>>>>>>> - V4L2 sub-device node registration is moved to take place at the same
>>>>>>>   time with the registration of the sub-device itself. With this
>>>>>>>   change, sub-device node registration behaviour is aligned with video
>>>>>>>   node registration.
>>>>>>>
>>>>>>> - The former is made possible by moving the bound() callback after
>>>>>>>   sub-device registration.
>>>>>>>
>>>>>>> - As all the device node registration and link creation is done as the
>>>>>>>   respective devices are probed, there is no longer dependency to the
>>>>>>>   notifier complete callback which as itself is seen problematic. The
>>>>>>>   complete callback still exists but there's no need to use it,
>>>>>>>   pending changes in individual drivers.
>>>>>>>   
>>>>>>>   See:
>>>>>>>   <URL:http://www.spinics.net/lists/linux-media/msg118323.html>
>>>>>>>   
>>>>>>>   As a result, if a part of the media device fails to initialise
>>>>>>>   because it is e.g. physically broken, it will be possible to use
>>>>>>>   what works.
>>>>>>
>>>>>> I've got major problems with this from a userspace point of view. In
>>>>>> the vast majority of cases you just want to bail out if one or more
>>>>>> subdevs fail.
>>>>>
>>>>> I admit it's easier for the user space if the device becomes available
>>>>> only when all its component drivers have registered.
>>>>>
>>>>> Also remember that video nodes are registered in the file system right
>>>>> on device probe time. It's only sub-device and media device node
>>>>> registration that has taken place in the notifier's complete handler.
>>>>
>>>> Is this always the case? In the R-Car VIN driver I register the video
>>>> devices using video_register_device() in the complete handler. Am I
>>>> doing things wrong in that driver? I had a patch where I moved the
>>>> video_register_device() call to probe time but it got shoot down in
>>>> review and was dropped.
>>>
>>> I don't think the current implementation is wrong, it's just different
>>> from other drivers; there's really no requirement regarding this AFAIU.
>>> It's one of the things where no attention has been paid I presume.
>>
>> It actually is a requirement: when a device node appears applications can
>> reasonably expect to have a fully functioning device. True for any device
>> node.
> 
> Why not ? I'm not aware of any such kernel-wide requirement. 
> 
>> You don't want to have to wait until some unspecified time before the full
>> functionality is there.
> 
> We certainly should specify that time and give userspace a way to find out 
> what is usable and when.
> 
>> I try to pay attention to this when reviewing code, since not following this
>> rule basically introduces a race condition which is hard to test.
>>
>>> However doing anything that can fail earlier on would be nicer since
>>> there's no reasonable way to signal an error from complete callback
>>> either.
>>
>> Right.
>>
>> Adding support for cases where devices may not be present is very desirable,
>> but this should go through an RFC process first to hammer out all the
>> details.
>>
>> Today we do not support this and we have to review code with that in mind.
>>
>> So the first async subnotifiers implementation should NOT support this
>> (although it can of course be designed with this in mind).
> 
> I very much disagree. The first async subnotifiers implementation (and I still 
> believe we don't need subnotifiers, there's nothing "sub" in them) shall 
> support this. If it means we first have to hammer out the details of out it 
> will work, so be it.

I see no reason to block the highly desirable async (sub)notifier code
while waiting for us to hammer out the details of something that is for the
most part unrelated to this. That usually takes a lot of time and since
the async (sub)notifier code is an internal API that can be changed later
you really don't want it to be stuck in limbo for all that time.

There are already a few drivers pending on this and, as you said yourself,
it is very unpleasant for driver authors to have their work stuck in the
queue for a long time.

Ditto for postponing work on this until the life-time issues have been
solved. If Niklas can do this work while you and Sakari work on the life-time
issues, then I see nothing wrong with that.

For APIs internal to the kernel you can make gradual improvements. As long
as each change makes things a bit better I happily accept it. Waiting until
everything is perfect and beautiful means nothing ever gets accepted since
it'll never be perfect as there is always something that can be done better...

It's what makes uAPI additions so hard, because you have to try and make it
as close to perfect as you can, and we never quite succeed there either.

Regards,

	Hans

>> Once it is in we can start on an RFC on how to support partial pipelines. I
>> have a lot of questions about that that need to be answered first.
>>
>> One thing at a time. Trying to do everything at once never works.
> 
> Sure, so let's start with probe time device node registration, and then move 
> on to subnotifiers.