[PATCH v3 0/4] Add link-frequencies to struct v4l2_of_endpoint

[PATCH v3 0/4] Add link-frequencies to struct v4l2_of_endpoint

[Sakari Ailus]

Hi folks,

This set changes the interface which is used to parse the properties of the
endpoint for media devices.

changes since v2:

- Rebased on current media-tree

v2 can be found here:

http://www.spinics.net/lists/linux-media/msg88058.html

-- 
Regards,
Sakari

[PATCH v3 1/4] v4l: of: Remove the head field in struct v4l2_of_endpoint

[Sakari Ailus]

The field is unused. Remove it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 include/media/v4l2-of.h |    2 --
 1 file changed, 2 deletions(-)

diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index f831c9c..f66b92c 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -57,7 +57,6 @@ struct v4l2_of_bus_parallel {
  * @base: struct of_endpoint containing port, id, and local of_node
  * @bus_type: bus type
  * @bus: bus configuration data structure
- * @head: list head for this structure
  */
 struct v4l2_of_endpoint {
 	struct of_endpoint base;
@@ -66,7 +65,6 @@ struct v4l2_of_endpoint {
 		struct v4l2_of_bus_parallel parallel;
 		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
 	} bus;
-	struct list_head head;
 };
 
 /**
-- 
1.7.10.4

Re: [PATCH v3 1/4] v4l: of: Remove the head field in struct v4l2_of_endpoint

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 07 April 2015 01:57:29 Sakari Ailus wrote:
> The field is unused. Remove it.

Do you know what the field was added for in the first place ?

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  include/media/v4l2-of.h |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index f831c9c..f66b92c 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -57,7 +57,6 @@ struct v4l2_of_bus_parallel {
>   * @base: struct of_endpoint containing port, id, and local of_node
>   * @bus_type: bus type
>   * @bus: bus configuration data structure
> - * @head: list head for this structure
>   */
>  struct v4l2_of_endpoint {
>  	struct of_endpoint base;
> @@ -66,7 +65,6 @@ struct v4l2_of_endpoint {
>  		struct v4l2_of_bus_parallel parallel;
>  		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
>  	} bus;
> -	struct list_head head;
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/4] v4l: of: Remove the head field in struct v4l2_of_endpoint

[Sakari Ailus]

Hi Laurent,

On Tue, Apr 07, 2015 at 01:11:34PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tuesday 07 April 2015 01:57:29 Sakari Ailus wrote:
> > The field is unused. Remove it.
> 
> Do you know what the field was added for in the first place ?

Frankly I have to admit I have no idea. It's part of the original patch
which adds V4L2 OF support:

---
commit 99fd133f907afdb430942d8d2ae53faa438adfe8
Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date:   Wed Sep 26 05:24:03 2012 -0300

    [media] Add a V4L2 OF parser
    
    Add a V4L2 OF parser, implementing bindings documented in
    Documentation/devicetree/bindings/media/video-interfaces.txt.
    [s.nawrocki@samsung.com: various corrections and improvements
    since the initial version]
    
    Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
    Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---

It looks like the intent has been that the field is used in order to keep a
list of structs of this kind, but no-one is using it for that purpose at the
moment.

-- 
Regards,

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

[PATCH v3 2/4] v4l: of: Instead of zeroing bus_type and bus field separately, unify this

[Sakari Ailus]

Clean the entire struct starting from bus_type. As more fields are added, no
changes will be needed in the function to reset their value explicitly.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/v4l2-core/v4l2-of.c |    5 +++--
 include/media/v4l2-of.h           |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index 83143d3..3ac6348 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -149,8 +149,9 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 	int rval;
 
 	of_graph_parse_endpoint(node, &endpoint->base);
-	endpoint->bus_type = 0;
-	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
+	/* Zero fields from bus_type to until the end */
+	memset(&endpoint->bus_type, 0, sizeof(*endpoint) -
+	       offsetof(typeof(*endpoint), bus_type));
 
 	rval = v4l2_of_parse_csi_bus(node, endpoint);
 	if (rval)
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index f66b92c..5bbdfbf 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -60,6 +60,7 @@ struct v4l2_of_bus_parallel {
  */
 struct v4l2_of_endpoint {
 	struct of_endpoint base;
+	/* Fields below this line will be cleaned by v4l2_of_parse_endpoint() */
 	enum v4l2_mbus_type bus_type;
 	union {
 		struct v4l2_of_bus_parallel parallel;
-- 
1.7.10.4

Re: [PATCH v3 2/4] v4l: of: Instead of zeroing bus_type and bus field separately, unify this

[Laurent Pinchart]

Hello Sakari,

Thank you for the patch.

On Tuesday 07 April 2015 01:57:30 Sakari Ailus wrote:
> Clean the entire struct starting from bus_type. As more fields are added, no
> changes will be needed in the function to reset their value explicitly.

I would s/Clean/Clear/ or s/Clean/Zero/. Same for the comment in the code. 
Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/v4l2-core/v4l2-of.c |    5 +++--
>  include/media/v4l2-of.h           |    1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-of.c
> b/drivers/media/v4l2-core/v4l2-of.c index 83143d3..3ac6348 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -149,8 +149,9 @@ int v4l2_of_parse_endpoint(const struct device_node
> *node, int rval;
> 
>  	of_graph_parse_endpoint(node, &endpoint->base);
> -	endpoint->bus_type = 0;
> -	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
> +	/* Zero fields from bus_type to until the end */
> +	memset(&endpoint->bus_type, 0, sizeof(*endpoint) -
> +	       offsetof(typeof(*endpoint), bus_type));
> 
>  	rval = v4l2_of_parse_csi_bus(node, endpoint);
>  	if (rval)
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index f66b92c..5bbdfbf 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -60,6 +60,7 @@ struct v4l2_of_bus_parallel {
>   */
>  struct v4l2_of_endpoint {
>  	struct of_endpoint base;
> +	/* Fields below this line will be cleaned by v4l2_of_parse_endpoint() */
>  	enum v4l2_mbus_type bus_type;
>  	union {
>  		struct v4l2_of_bus_parallel parallel;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/4] v4l: of: Instead of zeroing bus_type and bus field separately, unify this

[Sakari Ailus]

On Tue, Apr 07, 2015 at 12:47:56PM +0300, Laurent Pinchart wrote:
> Hello Sakari,
> 
> Thank you for the patch.
> 
> On Tuesday 07 April 2015 01:57:30 Sakari Ailus wrote:
> > Clean the entire struct starting from bus_type. As more fields are added, no
> > changes will be needed in the function to reset their value explicitly.
> 
> I would s/Clean/Clear/ or s/Clean/Zero/. Same for the comment in the code. 
> Apart from that,

I'll use zero. It's clearer. :-)

> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!!

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

[PATCH v3 3/4] v4l: of: Parse variable length properties --- link-frequencies

[Sakari Ailus]

The link-frequencies property is a variable length array of link frequencies
in an endpoint. The array is needed by an increasing number of drivers, so
it makes sense to add it to struct v4l2_of_endpoint.

However, the length of the array is variable and the size of struct
v4l2_of_endpoint is fixed since it is allocated by the caller. The options
here are

1. to define a fixed maximum limit of link frequencies that has to be the
global maximum of all boards. This is seen as problematic since the maximum
could be largish, and everyone hitting the problem would need to submit a
patch to fix it, or

2. parse the property in every driver. This doesn't sound appealing as two
of the three implementations submitted to linux-media were wrong, and one of
them was even merged before this was noticed, or

3. change the interface so that allocating and releasing memory according to
the size of the array is possible. This is what the patch does.

v4l2_of_alloc_parse_endpoint() is just like v4l2_of_parse_endpoint(), but it
will allocate the memory resources needed to store struct v4l2_of_endpoint
and the additional arrays pointed to by this struct. A corresponding release
function v4l2_of_free_endpoint() is provided to release the memory allocated
by v4l2_of_alloc_parse_endpoint().

In addition to this, the link-frequencies property is parsed as well, and
the result is stored to struct v4l2_of_endpoint field link_frequencies.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/v4l2-core/v4l2-of.c |   88 +++++++++++++++++++++++++++++++++++++
 include/media/v4l2-of.h           |   17 +++++++
 2 files changed, 105 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index 3ac6348..9810cc6 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/types.h>
 
@@ -141,6 +142,10 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
  * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
  * The caller should hold a reference to @node.
  *
+ * NOTE: This function does not parse properties the size of which is
+ * variable without a low fixed limit. Please use
+ * v4l2_of_alloc_parse_endpoint() in new drivers instead.
+ *
  * Return: 0.
  */
 int v4l2_of_parse_endpoint(const struct device_node *node,
@@ -167,6 +172,89 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 }
 EXPORT_SYMBOL(v4l2_of_parse_endpoint);
 
+/*
+ * v4l2_of_free_endpoint() - release resources acquired by
+ * v4l2_of_alloc_parse_endpoint()
+ * @endpoint - the endpoint the resources of which are to be released
+ *
+ * It is safe to call this function with NULL argument or on and
+ * endpoint the parsing of which failed.
+ */
+void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+	if (IS_ERR_OR_NULL(endpoint))
+		return;
+
+	kfree(endpoint->link_frequencies);
+	kfree(endpoint);
+}
+EXPORT_SYMBOL(v4l2_of_free_endpoint);
+
+/**
+ * v4l2_of_alloc_parse_endpoint() - parse all endpoint node properties
+ * @node: pointer to endpoint device_node
+ *
+ * All properties are optional. If none are found, we don't set any flags.
+ * This means the port has a static configuration and no properties have
+ * to be specified explicitly.
+ * If any properties that identify the bus as parallel are found and
+ * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise
+ * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
+ * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
+ * The caller should hold a reference to @node.
+ *
+ * v4l2_of_alloc_parse_endpoint() has two important differences to
+ * v4l2_of_parse_endpoint():
+ *
+ * 1. It also parses variable size data and
+ *
+ * 2. The memory resources it has acquired to store the variable size
+ *    data must be released using v4l2_of_free_endpoint() when no longer
+ *    needed.
+ *
+ * Return: Pointer to v4l2_of_endpoint if successful, on error a
+ * negative error code.
+ */
+struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
+	const struct device_node *node)
+{
+	struct v4l2_of_endpoint *endpoint;
+	int len;
+	int rval;
+
+	endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
+	if (!endpoint)
+		return ERR_PTR(-ENOMEM);
+
+	rval = v4l2_of_parse_endpoint(node, endpoint);
+	if (rval < 0)
+		goto out_err;
+
+	if (of_get_property(node, "link-frequencies", &len)) {
+		endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
+		if (!endpoint->link_frequencies) {
+			rval = -ENOMEM;
+			goto out_err;
+		}
+
+		endpoint->nr_of_link_frequencies =
+			len / sizeof(*endpoint->link_frequencies);
+
+		rval = of_property_read_u64_array(
+			node, "link-frequencies", endpoint->link_frequencies,
+			endpoint->nr_of_link_frequencies);
+		if (rval < 0)
+			goto out_err;
+	}
+
+	return endpoint;
+
+out_err:
+	v4l2_of_free_endpoint(endpoint);
+	return ERR_PTR(rval);
+}
+EXPORT_SYMBOL(v4l2_of_alloc_parse_endpoint);
+
 /**
  * v4l2_of_parse_link() - parse a link between two endpoints
  * @node: pointer to the endpoint at the local end of the link
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 5bbdfbf..88eb572 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -57,6 +57,8 @@ struct v4l2_of_bus_parallel {
  * @base: struct of_endpoint containing port, id, and local of_node
  * @bus_type: bus type
  * @bus: bus configuration data structure
+ * @link_frequencies: array of supported link frequencies
+ * @nr_of_link_frequencies: number of elements in link_frequenccies array
  */
 struct v4l2_of_endpoint {
 	struct of_endpoint base;
@@ -66,6 +68,8 @@ struct v4l2_of_endpoint {
 		struct v4l2_of_bus_parallel parallel;
 		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
 	} bus;
+	u64 *link_frequencies;
+	unsigned int nr_of_link_frequencies;
 };
 
 /**
@@ -85,6 +89,9 @@ struct v4l2_of_link {
 #ifdef CONFIG_OF
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint);
+struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
+	const struct device_node *node);
+void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint);
 int v4l2_of_parse_link(const struct device_node *node,
 		       struct v4l2_of_link *link);
 void v4l2_of_put_link(struct v4l2_of_link *link);
@@ -96,6 +103,16 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
+struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
+	const struct device_node *node)
+{
+	return NULL;
+}
+
+static void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+}
+
 static inline int v4l2_of_parse_link(const struct device_node *node,
 				     struct v4l2_of_link *link)
 {
-- 
1.7.10.4

Re: [PATCH v3 3/4] v4l: of: Parse variable length properties --- link-frequencies

[Laurent Pinchart]

Hello Sakari,

Thank you for the patch.

On Tuesday 07 April 2015 01:57:31 Sakari Ailus wrote:
> The link-frequencies property is a variable length array of link frequencies
> in an endpoint. The array is needed by an increasing number of drivers, so
> it makes sense to add it to struct v4l2_of_endpoint.
> 
> However, the length of the array is variable and the size of struct
> v4l2_of_endpoint is fixed since it is allocated by the caller. The options
> here are
> 
> 1. to define a fixed maximum limit of link frequencies that has to be the
> global maximum of all boards. This is seen as problematic since the maximum
> could be largish, and everyone hitting the problem would need to submit a
> patch to fix it, or
> 
> 2. parse the property in every driver. This doesn't sound appealing as two
> of the three implementations submitted to linux-media were wrong, and one of
> them was even merged before this was noticed, or
> 
> 3. change the interface so that allocating and releasing memory according to
> the size of the array is possible. This is what the patch does.
> 
> v4l2_of_alloc_parse_endpoint() is just like v4l2_of_parse_endpoint(), but it
> will allocate the memory resources needed to store struct v4l2_of_endpoint
> and the additional arrays pointed to by this struct. A corresponding
> release function v4l2_of_free_endpoint() is provided to release the memory
> allocated by v4l2_of_alloc_parse_endpoint().
> 
> In addition to this, the link-frequencies property is parsed as well, and
> the result is stored to struct v4l2_of_endpoint field link_frequencies.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/v4l2-core/v4l2-of.c |   88 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-of.h           |   17 +++++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-of.c
> b/drivers/media/v4l2-core/v4l2-of.c index 3ac6348..9810cc6 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -14,6 +14,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> 
> @@ -141,6 +142,10 @@ static void v4l2_of_parse_parallel_bus(const struct
> device_node *node, * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
>   * The caller should hold a reference to @node.
>   *
> + * NOTE: This function does not parse properties the size of which is
> + * variable without a low fixed limit. Please use
> + * v4l2_of_alloc_parse_endpoint() in new drivers instead.
> + *
>   * Return: 0.
>   */
>  int v4l2_of_parse_endpoint(const struct device_node *node,
> @@ -167,6 +172,89 @@ int v4l2_of_parse_endpoint(const struct device_node
> *node, }
>  EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> 
> +/*
> + * v4l2_of_free_endpoint() - release resources acquired by
> + * v4l2_of_alloc_parse_endpoint()

I would say "free the endpoint allocated by v4l2_of_alloc_parse_endpoint()".

> + * @endpoint - the endpoint the resources of which are to be released
> + *
> + * It is safe to call this function with NULL argument or on and

s/and/an/

> + * endpoint the parsing of which failed.
> + */
> +void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +	if (IS_ERR_OR_NULL(endpoint))
> +		return;
> +
> +	kfree(endpoint->link_frequencies);
> +	kfree(endpoint);
> +}
> +EXPORT_SYMBOL(v4l2_of_free_endpoint);
> +
> +/**
> + * v4l2_of_alloc_parse_endpoint() - parse all endpoint node properties
> + * @node: pointer to endpoint device_node
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.
> + * If any properties that identify the bus as parallel are found and
> + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> recognise
> + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> + * The caller should hold a reference to @node.
> + *
> + * v4l2_of_alloc_parse_endpoint() has two important differences to
> + * v4l2_of_parse_endpoint():
> + *
> + * 1. It also parses variable size data and
> + *
> + * 2. The memory resources it has acquired to store the variable size
> + *    data must be released using v4l2_of_free_endpoint() when no longer
> + *    needed.

I would s/resources it has acquired/it has allocated/ and s/released/freed/.

Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + *
> + * Return: Pointer to v4l2_of_endpoint if successful, on error a
> + * negative error code.
> + */
> +struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
> +	const struct device_node *node)
> +{
> +	struct v4l2_of_endpoint *endpoint;
> +	int len;
> +	int rval;
> +
> +	endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
> +	if (!endpoint)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rval = v4l2_of_parse_endpoint(node, endpoint);
> +	if (rval < 0)
> +		goto out_err;
> +
> +	if (of_get_property(node, "link-frequencies", &len)) {
> +		endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
> +		if (!endpoint->link_frequencies) {
> +			rval = -ENOMEM;
> +			goto out_err;
> +		}
> +
> +		endpoint->nr_of_link_frequencies =
> +			len / sizeof(*endpoint->link_frequencies);
> +
> +		rval = of_property_read_u64_array(
> +			node, "link-frequencies", endpoint->link_frequencies,
> +			endpoint->nr_of_link_frequencies);
> +		if (rval < 0)
> +			goto out_err;
> +	}
> +
> +	return endpoint;
> +
> +out_err:
> +	v4l2_of_free_endpoint(endpoint);
> +	return ERR_PTR(rval);
> +}
> +EXPORT_SYMBOL(v4l2_of_alloc_parse_endpoint);
> +
>  /**
>   * v4l2_of_parse_link() - parse a link between two endpoints
>   * @node: pointer to the endpoint at the local end of the link
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 5bbdfbf..88eb572 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -57,6 +57,8 @@ struct v4l2_of_bus_parallel {
>   * @base: struct of_endpoint containing port, id, and local of_node
>   * @bus_type: bus type
>   * @bus: bus configuration data structure
> + * @link_frequencies: array of supported link frequencies
> + * @nr_of_link_frequencies: number of elements in link_frequenccies array
>   */
>  struct v4l2_of_endpoint {
>  	struct of_endpoint base;
> @@ -66,6 +68,8 @@ struct v4l2_of_endpoint {
>  		struct v4l2_of_bus_parallel parallel;
>  		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
>  	} bus;
> +	u64 *link_frequencies;
> +	unsigned int nr_of_link_frequencies;
>  };
> 
>  /**
> @@ -85,6 +89,9 @@ struct v4l2_of_link {
>  #ifdef CONFIG_OF
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>  			   struct v4l2_of_endpoint *endpoint);
> +struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
> +	const struct device_node *node);
> +void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint);
>  int v4l2_of_parse_link(const struct device_node *node,
>  		       struct v4l2_of_link *link);
>  void v4l2_of_put_link(struct v4l2_of_link *link);
> @@ -96,6 +103,16 @@ static inline int v4l2_of_parse_endpoint(const struct
> device_node *node, return -ENOSYS;
>  }
> 
> +struct v4l2_of_endpoint *v4l2_of_alloc_parse_endpoint(
> +	const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
> +static void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +}
> +
>  static inline int v4l2_of_parse_link(const struct device_node *node,
>  				     struct v4l2_of_link *link)
>  {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/4] v4l: of: Parse variable length properties --- link-frequencies

[Sakari Ailus]

Hi Laurent,

On Tue, Apr 07, 2015 at 01:02:31PM +0300, Laurent Pinchart wrote:
> Hello Sakari,
> 
> Thank you for the patch.

Thanks for the review!

> On Tuesday 07 April 2015 01:57:31 Sakari Ailus wrote:
> > +/*
> > + * v4l2_of_free_endpoint() - release resources acquired by
> > + * v4l2_of_alloc_parse_endpoint()
> 
> I would say "free the endpoint allocated by v4l2_of_alloc_parse_endpoint()".
> 
> > + * @endpoint - the endpoint the resources of which are to be released
> > + *
> > + * It is safe to call this function with NULL argument or on and
> 
> s/and/an/

Fixed both.

> > + * endpoint the parsing of which failed.
> > + */
> > +void v4l2_of_free_endpoint(struct v4l2_of_endpoint *endpoint)
> > +{
> > +	if (IS_ERR_OR_NULL(endpoint))
> > +		return;
> > +
> > +	kfree(endpoint->link_frequencies);
> > +	kfree(endpoint);
> > +}
> > +EXPORT_SYMBOL(v4l2_of_free_endpoint);
> > +
> > +/**
> > + * v4l2_of_alloc_parse_endpoint() - parse all endpoint node properties
> > + * @node: pointer to endpoint device_node
> > + *
> > + * All properties are optional. If none are found, we don't set any flags.
> > + * This means the port has a static configuration and no properties have
> > + * to be specified explicitly.
> > + * If any properties that identify the bus as parallel are found and
> > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> > recognise
> > + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> > + * The caller should hold a reference to @node.
> > + *
> > + * v4l2_of_alloc_parse_endpoint() has two important differences to
> > + * v4l2_of_parse_endpoint():
> > + *
> > + * 1. It also parses variable size data and
> > + *
> > + * 2. The memory resources it has acquired to store the variable size
> > + *    data must be released using v4l2_of_free_endpoint() when no longer
> > + *    needed.
> 
> I would s/resources it has acquired/it has allocated/ and s/released/freed/.

Fixed.

> Apart from that,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

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

[PATCH v3 4/4] smiapp: Use v4l2_of_alloc_parse_endpoint()

[Sakari Ailus]

Instead of parsing the link-frequencies property in the driver, let
v4l2_of_alloc_parse_endpoint() do it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   40 ++++++++++++++++----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 557f25d..4a2e8d3 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2975,9 +2975,9 @@ static int smiapp_resume(struct device *dev)
 static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 {
 	struct smiapp_platform_data *pdata;
-	struct v4l2_of_endpoint bus_cfg;
+	struct v4l2_of_endpoint *bus_cfg;
 	struct device_node *ep;
-	uint32_t asize;
+	int i;
 	int rval;
 
 	if (!dev->of_node)
@@ -2987,13 +2987,17 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	if (!ep)
 		return NULL;
 
+	bus_cfg = v4l2_of_alloc_parse_endpoint(ep);
+	if (IS_ERR(bus_cfg)) {
+		rval = PTR_ERR(bus_cfg);
+		goto out_err;
+	}
+
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		goto out_err;
 
-	v4l2_of_parse_endpoint(ep, &bus_cfg);
-
-	switch (bus_cfg.bus_type) {
+	switch (bus_cfg->bus_type) {
 	case V4L2_MBUS_CSI2:
 		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
 		break;
@@ -3002,7 +3006,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 		goto out_err;
 	}
 
-	pdata->lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
+	pdata->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes;
 	dev_dbg(dev, "lanes %u\n", pdata->lanes);
 
 	/* xshutdown GPIO is optional */
@@ -3022,34 +3026,30 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	dev_dbg(dev, "reset %d, nvm %d, clk %d, csi %d\n", pdata->xshutdown,
 		pdata->nvm_size, pdata->ext_clk, pdata->csi_signalling_mode);
 
-	rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
-	if (rval) {
-		dev_warn(dev, "can't get link-frequencies array size\n");
+	if (!bus_cfg->nr_of_link_frequencies) {
+		dev_warn(dev, "no link frequencies defined\n");
 		goto out_err;
 	}
 
-	pdata->op_sys_clock = devm_kzalloc(dev, asize, GFP_KERNEL);
+	pdata->op_sys_clock = devm_kcalloc(
+		dev, bus_cfg->nr_of_link_frequencies + 1 /* guardian */,
+		sizeof(*pdata->op_sys_clock), GFP_KERNEL);
 	if (!pdata->op_sys_clock) {
 		rval = -ENOMEM;
 		goto out_err;
 	}
 
-	asize /= sizeof(*pdata->op_sys_clock);
-	rval = of_property_read_u64_array(
-		ep, "link-frequencies", pdata->op_sys_clock, asize);
-	if (rval) {
-		dev_warn(dev, "can't get link-frequencies\n");
-		goto out_err;
+	for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
+		pdata->op_sys_clock[i] = bus_cfg->link_frequencies[i];
+		dev_dbg(dev, "freq %d: %lld\n", i, pdata->op_sys_clock[i]);
 	}
 
-	for (; asize > 0; asize--)
-		dev_dbg(dev, "freq %d: %lld\n", asize - 1,
-			pdata->op_sys_clock[asize - 1]);
-
+	v4l2_of_free_endpoint(bus_cfg);
 	of_node_put(ep);
 	return pdata;
 
 out_err:
+	v4l2_of_free_endpoint(bus_cfg);
 	of_node_put(ep);
 	return NULL;
 }
-- 
1.7.10.4

Re: [PATCH v3 4/4] smiapp: Use v4l2_of_alloc_parse_endpoint()

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 07 April 2015 01:57:32 Sakari Ailus wrote:
> Instead of parsing the link-frequencies property in the driver, let
> v4l2_of_alloc_parse_endpoint() do it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |   40 +++++++++++++++--------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 557f25d..4a2e8d3 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2975,9 +2975,9 @@ static int smiapp_resume(struct device *dev)
>  static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>  {
>  	struct smiapp_platform_data *pdata;
> -	struct v4l2_of_endpoint bus_cfg;
> +	struct v4l2_of_endpoint *bus_cfg;
>  	struct device_node *ep;
> -	uint32_t asize;
> +	int i;
>  	int rval;
> 
>  	if (!dev->of_node)
> @@ -2987,13 +2987,17 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) if (!ep)
>  		return NULL;
> 
> +	bus_cfg = v4l2_of_alloc_parse_endpoint(ep);
> +	if (IS_ERR(bus_cfg)) {
> +		rval = PTR_ERR(bus_cfg);
> +		goto out_err;
> +	}
> +
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		goto out_err;
> 
> -	v4l2_of_parse_endpoint(ep, &bus_cfg);
> -
> -	switch (bus_cfg.bus_type) {
> +	switch (bus_cfg->bus_type) {
>  	case V4L2_MBUS_CSI2:
>  		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
>  		break;
> @@ -3002,7 +3006,7 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) goto out_err;
>  	}
> 
> -	pdata->lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
> +	pdata->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes;
>  	dev_dbg(dev, "lanes %u\n", pdata->lanes);
> 
>  	/* xshutdown GPIO is optional */
> @@ -3022,34 +3026,30 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) dev_dbg(dev, "reset %d, nvm %d, clk
> %d, csi %d\n", pdata->xshutdown, pdata->nvm_size, pdata->ext_clk,
> pdata->csi_signalling_mode);
> 
> -	rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
> -	if (rval) {
> -		dev_warn(dev, "can't get link-frequencies array size\n");
> +	if (!bus_cfg->nr_of_link_frequencies) {

Now that I see it being used, nr_of_link_frequencies feels a bit long. 
num_link_freqs could be an alternative. I'll let you decide. But for this 
patch,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		dev_warn(dev, "no link frequencies defined\n");
>  		goto out_err;
>  	}
> 
> -	pdata->op_sys_clock = devm_kzalloc(dev, asize, GFP_KERNEL);
> +	pdata->op_sys_clock = devm_kcalloc(
> +		dev, bus_cfg->nr_of_link_frequencies + 1 /* guardian */,
> +		sizeof(*pdata->op_sys_clock), GFP_KERNEL);
>  	if (!pdata->op_sys_clock) {
>  		rval = -ENOMEM;
>  		goto out_err;
>  	}
> 
> -	asize /= sizeof(*pdata->op_sys_clock);
> -	rval = of_property_read_u64_array(
> -		ep, "link-frequencies", pdata->op_sys_clock, asize);
> -	if (rval) {
> -		dev_warn(dev, "can't get link-frequencies\n");
> -		goto out_err;
> +	for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> +		pdata->op_sys_clock[i] = bus_cfg->link_frequencies[i];
> +		dev_dbg(dev, "freq %d: %lld\n", i, pdata->op_sys_clock[i]);
>  	}
> 
> -	for (; asize > 0; asize--)
> -		dev_dbg(dev, "freq %d: %lld\n", asize - 1,
> -			pdata->op_sys_clock[asize - 1]);
> -
> +	v4l2_of_free_endpoint(bus_cfg);
>  	of_node_put(ep);
>  	return pdata;
> 
>  out_err:
> +	v4l2_of_free_endpoint(bus_cfg);
>  	of_node_put(ep);
>  	return NULL;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/4] smiapp: Use v4l2_of_alloc_parse_endpoint()

[Sakari Ailus]

Hi Laurent,

On Tue, Apr 07, 2015 at 01:10:20PM +0300, Laurent Pinchart wrote:
> > @@ -3022,34 +3026,30 @@ static struct smiapp_platform_data
> > *smiapp_get_pdata(struct device *dev) dev_dbg(dev, "reset %d, nvm %d, clk
> > %d, csi %d\n", pdata->xshutdown, pdata->nvm_size, pdata->ext_clk,
> > pdata->csi_signalling_mode);
> > 
> > -	rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
> > -	if (rval) {
> > -		dev_warn(dev, "can't get link-frequencies array size\n");
> > +	if (!bus_cfg->nr_of_link_frequencies) {
> 
> Now that I see it being used, nr_of_link_frequencies feels a bit long. 
> num_link_freqs could be an alternative. I'll let you decide. But for this 
> patch,

It's long, I agree, but still used in only a small number of places in
drivers. I'd prefer to keep it as-is also as the name matches the name of
the property.

> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

-- 
Kind regards,

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