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

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

[Sakari Ailus]

Hi,

While I was adding a link_frequencies array field to struct
v4l2_of_endpoint, I also realised that the smiapp driver was not reading the
link-frequencies property from the endpoint, but the i2c device node
instead. This is what the second patch addresses.

The third patch does add support for reading the link-frequencies property
to struct v4l2_of_endpoint. There were a few options to consider. I'm not
entirely happy with the solution, but it still appears the best option to
me, all being:

1. Let drivers parse the link-frequencies property. There are three samples
from three different developers (myself, Laurent and Prabhakar) and two of
them seemed to have issues.

2. Reading the contents of the link-frequencies property in
v4l2_of_parse_endpoint() addresses the above issue. It currently has no
cleanup function to release memory reserved for the variable-size
link_frequencies array.

2a. Use a constant size array for link-frequencies. This limits the number
of link-frequencies and wastes memory elsewhere.

2b. Use devm_*() functions to allocate the memory. v4l2_of_parse_endpoint()
would require a device pointer argument in order to read variable size
content from the endpoint. One major issue in this approach is that the
configuration struct is typically short-lived as drivers allocate it in
stack in their DT node parsing function.

2c. Add a function to release resources reserved by
v4l2_of_parse_endpoint(). This is what the third patch does.

Comments are welcome.

-- 
Kind regards,
Sakari

[PATCH 1/3] smiapp: Clean up smiapp_get_pdata()

[Sakari Ailus]

Don't set rval when it's not used (the function returns a pointer to struct
smiapp_platform_data).

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index b1f566b..565a00c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2988,10 +2988,8 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 		return NULL;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata) {
-		rval = -ENOMEM;
+	if (!pdata)
 		goto out_err;
-	}
 
 	v4l2_of_parse_endpoint(ep, &bus_cfg);
 
@@ -3001,7 +2999,6 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 		break;
 		/* FIXME: add CCP2 support. */
 	default:
-		rval = -EINVAL;
 		goto out_err;
 	}
 
-- 
1.7.10.4

[PATCH 2/3] smiapp: Read link-frequencies property from the endpoint node

[Sakari Ailus]

The documentation stated that the link-frequencies property belongs to the
endpoint node, not to the device's of_node. Fix this.

There are no DT board descriptions using the driver yet, so a fix in the
driver is sufficient.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 565a00c..ecae76b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -3022,8 +3022,7 @@ 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(
-		dev->of_node, "link-frequencies", &asize) ? 0 : -ENOENT;
+	rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
 	if (rval) {
 		dev_warn(dev, "can't get link-frequencies array size\n");
 		goto out_err;
@@ -3037,7 +3036,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 
 	asize /= sizeof(*pdata->op_sys_clock);
 	rval = of_property_read_u64_array(
-		dev->of_node, "link-frequencies", pdata->op_sys_clock, asize);
+		ep, "link-frequencies", pdata->op_sys_clock, asize);
 	if (rval) {
 		dev_warn(dev, "can't get link-frequencies\n");
 		goto out_err;
-- 
1.7.10.4

[PATCH 3/3] v4l: of: Add link-frequencies array to struct v4l2_of_endpoint

[Sakari Ailus]

Parse and read the link-frequencies property in v4l2_of_parse_endpoint().
The property is an u64 array of undefined length, thus the memory allocation
may fail, leading

- v4l2_of_parse_endpoint() to return an error in such a case (as well as
  when failing to parse the property) and
- to requiring releasing the memory reserved for the array
  (v4l2_of_release_endpoint()).

If a driver does not need to access properties that require memory
allocation (such as link-frequencies), it may choose to call
v4l2_of_release_endpoint() right after calling v4l2_of_parse_endpoint().

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/i2c/adv7604.c                    |    1 +
 drivers/media/i2c/s5c73m3/s5c73m3-core.c       |    1 +
 drivers/media/i2c/s5k5baf.c                    |    1 +
 drivers/media/i2c/smiapp/smiapp-core.c         |   32 ++++++++--------
 drivers/media/i2c/tvp514x.c                    |    1 +
 drivers/media/i2c/tvp7002.c                    |    1 +
 drivers/media/platform/am437x/am437x-vpfe.c    |    1 +
 drivers/media/platform/exynos4-is/media-dev.c  |    1 +
 drivers/media/platform/exynos4-is/mipi-csis.c  |    1 +
 drivers/media/platform/soc_camera/atmel-isi.c  |    1 +
 drivers/media/platform/soc_camera/pxa_camera.c |    1 +
 drivers/media/platform/soc_camera/rcar_vin.c   |    1 +
 drivers/media/v4l2-core/v4l2-of.c              |   47 +++++++++++++++++++++++-
 include/media/v4l2-of.h                        |    9 +++++
 14 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index aaab9c9..9c6c2a5 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2624,6 +2624,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 		return -EINVAL;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	of_node_put(endpoint);
 
 	flags = bus_cfg.bus.parallel.flags;
diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index ee0f57e..f248a47 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -1624,6 +1624,7 @@ static int s5c73m3_get_platform_data(struct s5c73m3 *state)
 	}
 
 	v4l2_of_parse_endpoint(node_ep, &ep);
+	v4l2_of_release_endpoint(&ep);
 	of_node_put(node_ep);
 
 	if (ep.bus_type != V4L2_MBUS_CSI2) {
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index a3d7d03..222cd62 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -1867,6 +1867,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev)
 	}
 
 	v4l2_of_parse_endpoint(node_ep, &ep);
+	v4l2_of_release_endpoint(&ep);
 	of_node_put(node_ep);
 	state->bus_type = ep.bus_type;
 
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index ecae76b..2b3cd9e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2977,7 +2977,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	struct smiapp_platform_data *pdata;
 	struct v4l2_of_endpoint bus_cfg;
 	struct device_node *ep;
-	uint32_t asize;
+	int i;
 	int rval;
 
 	if (!dev->of_node)
@@ -2987,12 +2987,14 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	if (!ep)
 		return NULL;
 
+	rval = v4l2_of_parse_endpoint(ep, &bus_cfg);
+	if (rval < 0)
+		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) {
 	case V4L2_MBUS_CSI2:
 		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
@@ -3022,34 +3024,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_kmalloc_array(
+		dev, bus_cfg.nr_of_link_frequencies + 1 /* guardian */,
+		sizeof(*bus_cfg.link_frequencies), 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_release_endpoint(&bus_cfg);
 	of_node_put(ep);
 	return pdata;
 
 out_err:
+	v4l2_of_release_endpoint(&bus_cfg);
 	of_node_put(ep);
 	return NULL;
 }
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index c6b3dc5..91d7dc9 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -1078,6 +1078,7 @@ tvp514x_get_pdata(struct i2c_client *client)
 		goto done;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	flags = bus_cfg.bus.parallel.flags;
 
 	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 9233194..70119ea 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -965,6 +965,7 @@ tvp7002_get_pdata(struct i2c_client *client)
 		goto done;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	flags = bus_cfg.bus.parallel.flags;
 
 	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 56a5cb0..19ec20a 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2469,6 +2469,7 @@ vpfe_get_pdata(struct platform_device *pdev)
 		}
 
 		err = v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+		v4l2_of_release_endpoint(&bus_cfg);
 		if (err) {
 			dev_err(&pdev->dev, "Could not parse the endpoint\n");
 			goto done;
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index f315ef9..4d28305 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -340,6 +340,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		return 0;
 
 	v4l2_of_parse_endpoint(ep, &endpoint);
+	v4l2_of_release_endpoint(&endpoint);
 	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS)
 		return -EINVAL;
 
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 2504aa8..03bf468 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -751,6 +751,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 	}
 	/* Get port node and validate MIPI-CSI channel id. */
 	v4l2_of_parse_endpoint(node, &endpoint);
+	v4l2_of_release_endpoint(&endpoint);
 
 	state->index = endpoint.base.port - FIMC_INPUT_MIPI_CSI2_0;
 	if (state->index >= CSIS_MAX_ENTITIES)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 8526bf5..245e3a4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -900,6 +900,7 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi,
 	}
 
 	err = v4l2_of_parse_endpoint(np, &ep);
+	v4l2_of_release_endpoint(&ep);
 	if (err) {
 		dev_err(&pdev->dev, "Could not parse the endpoint\n");
 		goto err_probe_dt;
diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
index 8d6e343..1e37b9b 100644
--- a/drivers/media/platform/soc_camera/pxa_camera.c
+++ b/drivers/media/platform/soc_camera/pxa_camera.c
@@ -1672,6 +1672,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
 	}
 
 	err = v4l2_of_parse_endpoint(np, &ep);
+	v4l2_of_release_endpoint(&ep);
 	if (err) {
 		dev_err(dev, "could not parse endpoint\n");
 		goto out;
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 279ab9f..cf0a328 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1863,6 +1863,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 		}
 
 		ret = v4l2_of_parse_endpoint(np, &ep);
+		v4l2_of_release_endpoint(&ep);
 		if (ret) {
 			dev_err(&pdev->dev, "could not parse endpoint\n");
 			return ret;
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index b4ed9a9..e24610c 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>
 
@@ -109,6 +110,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 }
 
 /**
+ * v4l2_of_release_endpoint() - release resources acquired by
+ * v4l2_of_parse_endpoint()
+ * @endpoint - the endpoint the resources of which are to be released
+ *
+ * It is safe to call this function on an endpoint which is not parsed or
+ * and endpoint the parsing of which failed. However in the former case the
+ * argument must point to a struct the memory of which has been set to zero.
+ *
+ * Values in the struct v4l2_of_endpoint that are not connected to resources
+ * acquired by v4l2_of_parse_endpoint() are guaranteed to remain untouched.
+ */
+void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+	kfree(endpoint->link_frequencies);
+	endpoint->link_frequencies = NULL;
+	endpoint->nr_of_link_frequencies = 0;
+}
+EXPORT_SYMBOL(v4l2_of_parse_endpoint);
+
+/**
  * v4l2_of_parse_endpoint() - parse all endpoint node properties
  * @node: pointer to endpoint device_node
  * @endpoint: pointer to the V4L2 OF endpoint data structure
@@ -122,15 +143,39 @@ 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.
  *
+ * An endpoint parsed using v4l2_of_parse_endpoint() must be released using
+ * v4l2_of_release_endpoint().
+ *
  * Return: 0.
  */
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint)
 {
+	int len;
+
 	of_graph_parse_endpoint(node, &endpoint->base);
 	endpoint->bus_type = 0;
 	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
 
+	if (of_get_property(node, "link-frequencies", &len)) {
+		int rval;
+
+		endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
+		if (!endpoint->link_frequencies)
+			return -ENOMEM;
+
+		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) {
+			v4l2_of_release_endpoint(endpoint);
+			return rval;
+		}
+	}
+
 	v4l2_of_parse_csi_bus(node, endpoint);
 	/*
 	 * Parse the parallel video bus properties only if none
@@ -141,4 +186,4 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 
 	return 0;
 }
-EXPORT_SYMBOL(v4l2_of_parse_endpoint);
+EXPORT_SYMBOL(v4l2_of_release_endpoint);
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 70fa7b7..8c123ff 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -54,6 +54,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
  * @head: list head for this structure
  */
 struct v4l2_of_endpoint {
@@ -63,12 +65,15 @@ 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;
 	struct list_head head;
 };
 
 #ifdef CONFIG_OF
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint);
+void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint);
 #else /* CONFIG_OF */
 
 static inline int v4l2_of_parse_endpoint(const struct device_node *node,
@@ -77,6 +82,10 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
+static void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+}
+
 #endif /* CONFIG_OF */
 
 #endif /* _V4L2_OF_H */
-- 
1.7.10.4

Re: [PATCH 1/3] smiapp: Clean up smiapp_get_pdata()

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 10 March 2015 03:18:00 Sakari Ailus wrote:
> Don't set rval when it's not used (the function returns a pointer to struct
> smiapp_platform_data).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index b1f566b..565a00c 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2988,10 +2988,8 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) return NULL;
> 
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> -	if (!pdata) {
> -		rval = -ENOMEM;
> +	if (!pdata)
>  		goto out_err;
> -	}
> 
>  	v4l2_of_parse_endpoint(ep, &bus_cfg);
> 
> @@ -3001,7 +2999,6 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) break;
>  		/* FIXME: add CCP2 support. */
>  	default:
> -		rval = -EINVAL;
>  		goto out_err;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] smiapp: Read link-frequencies property from the endpoint node

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 10 March 2015 03:18:01 Sakari Ailus wrote:
> The documentation stated that the link-frequencies property belongs to the
> endpoint node, not to the device's of_node. Fix this.
> 
> There are no DT board descriptions using the driver yet, so a fix in the
> driver is sufficient.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 565a00c..ecae76b 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3022,8 +3022,7 @@ 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(
> -		dev->of_node, "link-frequencies", &asize) ? 0 : -ENOENT;
> +	rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
>  	if (rval) {
>  		dev_warn(dev, "can't get link-frequencies array size\n");
>  		goto out_err;
> @@ -3037,7 +3036,7 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev)
> 
>  	asize /= sizeof(*pdata->op_sys_clock);
>  	rval = of_property_read_u64_array(
> -		dev->of_node, "link-frequencies", pdata->op_sys_clock, asize);
> +		ep, "link-frequencies", pdata->op_sys_clock, asize);
>  	if (rval) {
>  		dev_warn(dev, "can't get link-frequencies\n");
>  		goto out_err;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] v4l: of: Add link-frequencies array to struct v4l2_of_endpoint

[Lad, Prabhakar]

Hi Sakari,

Thanks for the patch.

On Tue, Mar 10, 2015 at 1:18 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Parse and read the link-frequencies property in v4l2_of_parse_endpoint().
> The property is an u64 array of undefined length, thus the memory allocation
> may fail, leading
>
> - v4l2_of_parse_endpoint() to return an error in such a case (as well as
>   when failing to parse the property) and
> - to requiring releasing the memory reserved for the array
>   (v4l2_of_release_endpoint()).
>
> If a driver does not need to access properties that require memory
> allocation (such as link-frequencies), it may choose to call
> v4l2_of_release_endpoint() right after calling v4l2_of_parse_endpoint().
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/i2c/adv7604.c                    |    1 +
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c       |    1 +
>  drivers/media/i2c/s5k5baf.c                    |    1 +
>  drivers/media/i2c/smiapp/smiapp-core.c         |   32 ++++++++--------
>  drivers/media/i2c/tvp514x.c                    |    1 +
>  drivers/media/i2c/tvp7002.c                    |    1 +
>  drivers/media/platform/am437x/am437x-vpfe.c    |    1 +
>  drivers/media/platform/exynos4-is/media-dev.c  |    1 +
>  drivers/media/platform/exynos4-is/mipi-csis.c  |    1 +
>  drivers/media/platform/soc_camera/atmel-isi.c  |    1 +
>  drivers/media/platform/soc_camera/pxa_camera.c |    1 +
>  drivers/media/platform/soc_camera/rcar_vin.c   |    1 +
>  drivers/media/v4l2-core/v4l2-of.c              |   47 +++++++++++++++++++++++-
>  include/media/v4l2-of.h                        |    9 +++++
>  14 files changed, 81 insertions(+), 18 deletions(-)
>
[snip]
> diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> index b4ed9a9..e24610c 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>
>
> @@ -109,6 +110,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
>  }
>
>  /**
> + * v4l2_of_release_endpoint() - release resources acquired by
> + * v4l2_of_parse_endpoint()
> + * @endpoint - the endpoint the resources of which are to be released
> + *
> + * It is safe to call this function on an endpoint which is not parsed or
> + * and endpoint the parsing of which failed. However in the former case the
> + * argument must point to a struct the memory of which has been set to zero.
> + *
> + * Values in the struct v4l2_of_endpoint that are not connected to resources
> + * acquired by v4l2_of_parse_endpoint() are guaranteed to remain untouched.
> + */
> +void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +       kfree(endpoint->link_frequencies);
> +       endpoint->link_frequencies = NULL;
> +       endpoint->nr_of_link_frequencies = 0;
> +}
> +EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> +
> +/**
>   * v4l2_of_parse_endpoint() - parse all endpoint node properties
>   * @node: pointer to endpoint device_node
>   * @endpoint: pointer to the V4L2 OF endpoint data structure
> @@ -122,15 +143,39 @@ 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.
>   *
> + * An endpoint parsed using v4l2_of_parse_endpoint() must be released using
> + * v4l2_of_release_endpoint().
> + *
>   * Return: 0.
>   */
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>                            struct v4l2_of_endpoint *endpoint)
>  {
> +       int len;
> +
>         of_graph_parse_endpoint(node, &endpoint->base);
>         endpoint->bus_type = 0;
>         memset(&endpoint->bus, 0, sizeof(endpoint->bus));
>
endpoint->link_frequencies = NULL; required here.

Apart from that patch looks good.

Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

> +       if (of_get_property(node, "link-frequencies", &len)) {
> +               int rval;
> +
> +               endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
> +               if (!endpoint->link_frequencies)
> +                       return -ENOMEM;
> +
> +               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) {
> +                       v4l2_of_release_endpoint(endpoint);
> +                       return rval;
> +               }
> +       }
> +
>         v4l2_of_parse_csi_bus(node, endpoint);
>         /*
>          * Parse the parallel video bus properties only if none
> @@ -141,4 +186,4 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
>
>         return 0;
>  }
> -EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> +EXPORT_SYMBOL(v4l2_of_release_endpoint);
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 70fa7b7..8c123ff 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -54,6 +54,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
>   * @head: list head for this structure
>   */
>  struct v4l2_of_endpoint {
> @@ -63,12 +65,15 @@ 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;
>         struct list_head head;
>  };
>
>  #ifdef CONFIG_OF
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>                            struct v4l2_of_endpoint *endpoint);
> +void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint);
>  #else /* CONFIG_OF */
>
>  static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> @@ -77,6 +82,10 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
>         return -ENOSYS;
>  }
>
> +static void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +}
> +
>  #endif /* CONFIG_OF */
>
>  #endif /* _V4L2_OF_H */
> --
> 1.7.10.4
>

Re: [PATCH 1/3] smiapp: Clean up smiapp_get_pdata()

[Lad, Prabhakar]

Hi Sakari,

Thanks for the patch.

On Tue, Mar 10, 2015 at 1:18 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Don't set rval when it's not used (the function returns a pointer to struct
> smiapp_platform_data).
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index b1f566b..565a00c 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2988,10 +2988,8 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>                 return NULL;
>
>         pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> -       if (!pdata) {
> -               rval = -ENOMEM;
> +       if (!pdata)
>                 goto out_err;
> -       }
>
>         v4l2_of_parse_endpoint(ep, &bus_cfg);
>
> @@ -3001,7 +2999,6 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>                 break;
>                 /* FIXME: add CCP2 support. */
>         default:
> -               rval = -EINVAL;
>                 goto out_err;
>         }
>
> --
> 1.7.10.4
>

Re: [PATCH 2/3] smiapp: Read link-frequencies property from the endpoint node

[Lad, Prabhakar]

Hi Sakari,

Thanks for the patch.

On Tue, Mar 10, 2015 at 1:18 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> The documentation stated that the link-frequencies property belongs to the
> endpoint node, not to the device's of_node. Fix this.
>
> There are no DT board descriptions using the driver yet, so a fix in the
> driver is sufficient.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 565a00c..ecae76b 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3022,8 +3022,7 @@ 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(
> -               dev->of_node, "link-frequencies", &asize) ? 0 : -ENOENT;
> +       rval = of_get_property(ep, "link-frequencies", &asize) ? 0 : -ENOENT;
>         if (rval) {
>                 dev_warn(dev, "can't get link-frequencies array size\n");
>                 goto out_err;
> @@ -3037,7 +3036,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>
>         asize /= sizeof(*pdata->op_sys_clock);
>         rval = of_property_read_u64_array(
> -               dev->of_node, "link-frequencies", pdata->op_sys_clock, asize);
> +               ep, "link-frequencies", pdata->op_sys_clock, asize);
>         if (rval) {
>                 dev_warn(dev, "can't get link-frequencies\n");
>                 goto out_err;
> --
> 1.7.10.4
>

Re: [PATCH 1/3] smiapp: Clean up smiapp_get_pdata()

[Lad, Prabhakar]

On Wed, Mar 11, 2015 at 10:24 PM, Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> Hi Sakari,
>
> Thanks for the patch.
>
> On Tue, Mar 10, 2015 at 1:18 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> Don't set rval when it's not used (the function returns a pointer to struct
>> smiapp_platform_data).
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>
> Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>
I meant :
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

Re: [PATCH 2/3] smiapp: Read link-frequencies property from the endpoint node

[Lad, Prabhakar]

On Wed, Mar 11, 2015 at 10:25 PM, Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> Hi Sakari,
>
> Thanks for the patch.
>
> On Tue, Mar 10, 2015 at 1:18 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> The documentation stated that the link-frequencies property belongs to the
>> endpoint node, not to the device's of_node. Fix this.
>>
>> There are no DT board descriptions using the driver yet, so a fix in the
>> driver is sufficient.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>
> Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>

I meant :
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

[PATCH v1.1 3/3] v4l: of: Add link-frequencies array to struct v4l2_of_endpoint

[Sakari Ailus]

Parse and read the link-frequencies property in v4l2_of_parse_endpoint().
The property is an u64 array of undefined length, thus the memory allocation
may fail, leading

- v4l2_of_parse_endpoint() to return an error in such a case (as well as
  when failing to parse the property) and
- to requiring releasing the memory reserved for the array
  (v4l2_of_release_endpoint()).

If a driver does not need to access properties that require memory
allocation (such as link-frequencies), it may choose to call
v4l2_of_release_endpoint() right after calling v4l2_of_parse_endpoint().

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
since v1:

- Set the link_frequencies pointer to NULL and set nr_of_link_frequencies to
  zero if link-frequencies property cannot be found by changing memset
  arguments. Thanks to Prabhakar for spotting this!

 drivers/media/i2c/adv7604.c                    |    1 +
 drivers/media/i2c/s5c73m3/s5c73m3-core.c       |    1 +
 drivers/media/i2c/s5k5baf.c                    |    1 +
 drivers/media/i2c/smiapp/smiapp-core.c         |   32 +++++++--------
 drivers/media/i2c/tvp514x.c                    |    1 +
 drivers/media/i2c/tvp7002.c                    |    1 +
 drivers/media/platform/am437x/am437x-vpfe.c    |    1 +
 drivers/media/platform/exynos4-is/media-dev.c  |    1 +
 drivers/media/platform/exynos4-is/mipi-csis.c  |    1 +
 drivers/media/platform/soc_camera/atmel-isi.c  |    1 +
 drivers/media/platform/soc_camera/pxa_camera.c |    1 +
 drivers/media/platform/soc_camera/rcar_vin.c   |    1 +
 drivers/media/v4l2-core/v4l2-of.c              |   51 +++++++++++++++++++++++-
 include/media/v4l2-of.h                        |    9 +++++
 14 files changed, 84 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index aaab9c9..9c6c2a5 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2624,6 +2624,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 		return -EINVAL;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	of_node_put(endpoint);
 
 	flags = bus_cfg.bus.parallel.flags;
diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index ee0f57e..f248a47 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -1624,6 +1624,7 @@ static int s5c73m3_get_platform_data(struct s5c73m3 *state)
 	}
 
 	v4l2_of_parse_endpoint(node_ep, &ep);
+	v4l2_of_release_endpoint(&ep);
 	of_node_put(node_ep);
 
 	if (ep.bus_type != V4L2_MBUS_CSI2) {
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index a3d7d03..222cd62 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -1867,6 +1867,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev)
 	}
 
 	v4l2_of_parse_endpoint(node_ep, &ep);
+	v4l2_of_release_endpoint(&ep);
 	of_node_put(node_ep);
 	state->bus_type = ep.bus_type;
 
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index ecae76b..2b3cd9e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2977,7 +2977,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	struct smiapp_platform_data *pdata;
 	struct v4l2_of_endpoint bus_cfg;
 	struct device_node *ep;
-	uint32_t asize;
+	int i;
 	int rval;
 
 	if (!dev->of_node)
@@ -2987,12 +2987,14 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
 	if (!ep)
 		return NULL;
 
+	rval = v4l2_of_parse_endpoint(ep, &bus_cfg);
+	if (rval < 0)
+		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) {
 	case V4L2_MBUS_CSI2:
 		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
@@ -3022,34 +3024,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_kmalloc_array(
+		dev, bus_cfg.nr_of_link_frequencies + 1 /* guardian */,
+		sizeof(*bus_cfg.link_frequencies), 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_release_endpoint(&bus_cfg);
 	of_node_put(ep);
 	return pdata;
 
 out_err:
+	v4l2_of_release_endpoint(&bus_cfg);
 	of_node_put(ep);
 	return NULL;
 }
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index c6b3dc5..91d7dc9 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -1078,6 +1078,7 @@ tvp514x_get_pdata(struct i2c_client *client)
 		goto done;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	flags = bus_cfg.bus.parallel.flags;
 
 	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 9233194..70119ea 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -965,6 +965,7 @@ tvp7002_get_pdata(struct i2c_client *client)
 		goto done;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	v4l2_of_release_endpoint(&bus_cfg);
 	flags = bus_cfg.bus.parallel.flags;
 
 	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 56a5cb0..19ec20a 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2469,6 +2469,7 @@ vpfe_get_pdata(struct platform_device *pdev)
 		}
 
 		err = v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+		v4l2_of_release_endpoint(&bus_cfg);
 		if (err) {
 			dev_err(&pdev->dev, "Could not parse the endpoint\n");
 			goto done;
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index f315ef9..4d28305 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -340,6 +340,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		return 0;
 
 	v4l2_of_parse_endpoint(ep, &endpoint);
+	v4l2_of_release_endpoint(&endpoint);
 	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS)
 		return -EINVAL;
 
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 2504aa8..03bf468 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -751,6 +751,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 	}
 	/* Get port node and validate MIPI-CSI channel id. */
 	v4l2_of_parse_endpoint(node, &endpoint);
+	v4l2_of_release_endpoint(&endpoint);
 
 	state->index = endpoint.base.port - FIMC_INPUT_MIPI_CSI2_0;
 	if (state->index >= CSIS_MAX_ENTITIES)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 8526bf5..245e3a4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -900,6 +900,7 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi,
 	}
 
 	err = v4l2_of_parse_endpoint(np, &ep);
+	v4l2_of_release_endpoint(&ep);
 	if (err) {
 		dev_err(&pdev->dev, "Could not parse the endpoint\n");
 		goto err_probe_dt;
diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
index 8d6e343..1e37b9b 100644
--- a/drivers/media/platform/soc_camera/pxa_camera.c
+++ b/drivers/media/platform/soc_camera/pxa_camera.c
@@ -1672,6 +1672,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
 	}
 
 	err = v4l2_of_parse_endpoint(np, &ep);
+	v4l2_of_release_endpoint(&ep);
 	if (err) {
 		dev_err(dev, "could not parse endpoint\n");
 		goto out;
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 279ab9f..cf0a328 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1863,6 +1863,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 		}
 
 		ret = v4l2_of_parse_endpoint(np, &ep);
+		v4l2_of_release_endpoint(&ep);
 		if (ret) {
 			dev_err(&pdev->dev, "could not parse endpoint\n");
 			return ret;
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index b4ed9a9..9043eea 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>
 
@@ -109,6 +110,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 }
 
 /**
+ * v4l2_of_release_endpoint() - release resources acquired by
+ * v4l2_of_parse_endpoint()
+ * @endpoint - the endpoint the resources of which are to be released
+ *
+ * It is safe to call this function on an endpoint which is not parsed or
+ * and endpoint the parsing of which failed. However in the former case the
+ * argument must point to a struct the memory of which has been set to zero.
+ *
+ * Values in the struct v4l2_of_endpoint that are not connected to resources
+ * acquired by v4l2_of_parse_endpoint() are guaranteed to remain untouched.
+ */
+void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+	kfree(endpoint->link_frequencies);
+	endpoint->link_frequencies = NULL;
+	endpoint->nr_of_link_frequencies = 0;
+}
+EXPORT_SYMBOL(v4l2_of_parse_endpoint);
+
+/**
  * v4l2_of_parse_endpoint() - parse all endpoint node properties
  * @node: pointer to endpoint device_node
  * @endpoint: pointer to the V4L2 OF endpoint data structure
@@ -122,14 +143,40 @@ 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.
  *
+ * An endpoint parsed using v4l2_of_parse_endpoint() must be released using
+ * v4l2_of_release_endpoint().
+ *
  * Return: 0.
  */
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint)
 {
+	int len;
+
 	of_graph_parse_endpoint(node, &endpoint->base);
 	endpoint->bus_type = 0;
-	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
+	/* Zero fields from bus to until head (excluding) */
+	memset(&endpoint->bus, 0, offsetof(typeof(*endpoint), head) -
+	       offsetof(typeof(*endpoint), bus));
+
+	if (of_get_property(node, "link-frequencies", &len)) {
+		int rval;
+
+		endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
+		if (!endpoint->link_frequencies)
+			return -ENOMEM;
+
+		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) {
+			v4l2_of_release_endpoint(endpoint);
+			return rval;
+		}
+	}
 
 	v4l2_of_parse_csi_bus(node, endpoint);
 	/*
@@ -141,4 +188,4 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 
 	return 0;
 }
-EXPORT_SYMBOL(v4l2_of_parse_endpoint);
+EXPORT_SYMBOL(v4l2_of_release_endpoint);
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 70fa7b7..8c123ff 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -54,6 +54,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
  * @head: list head for this structure
  */
 struct v4l2_of_endpoint {
@@ -63,12 +65,15 @@ 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;
 	struct list_head head;
 };
 
 #ifdef CONFIG_OF
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint);
+void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint);
 #else /* CONFIG_OF */
 
 static inline int v4l2_of_parse_endpoint(const struct device_node *node,
@@ -77,6 +82,10 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
+static void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
+{
+}
+
 #endif /* CONFIG_OF */
 
 #endif /* _V4L2_OF_H */
-- 
1.7.10.4

Re: [PATCH v1.1 3/3] v4l: of: Add link-frequencies array to struct v4l2_of_endpoint

[Guennadi Liakhovetski]

Hi Sakari,

Thanks for the patches.

On Thu, 12 Mar 2015, Sakari Ailus wrote:

> Parse and read the link-frequencies property in v4l2_of_parse_endpoint().
> The property is an u64 array of undefined length, thus the memory allocation
> may fail, leading
> 
> - v4l2_of_parse_endpoint() to return an error in such a case (as well as
>   when failing to parse the property) and
> - to requiring releasing the memory reserved for the array
>   (v4l2_of_release_endpoint()).
> 
> If a driver does not need to access properties that require memory
> allocation (such as link-frequencies), it may choose to call
> v4l2_of_release_endpoint() right after calling v4l2_of_parse_endpoint().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
> since v1:
> 
> - Set the link_frequencies pointer to NULL and set nr_of_link_frequencies to
>   zero if link-frequencies property cannot be found by changing memset
>   arguments. Thanks to Prabhakar for spotting this!
> 
>  drivers/media/i2c/adv7604.c                    |    1 +
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c       |    1 +
>  drivers/media/i2c/s5k5baf.c                    |    1 +
>  drivers/media/i2c/smiapp/smiapp-core.c         |   32 +++++++--------
>  drivers/media/i2c/tvp514x.c                    |    1 +
>  drivers/media/i2c/tvp7002.c                    |    1 +
>  drivers/media/platform/am437x/am437x-vpfe.c    |    1 +
>  drivers/media/platform/exynos4-is/media-dev.c  |    1 +
>  drivers/media/platform/exynos4-is/mipi-csis.c  |    1 +
>  drivers/media/platform/soc_camera/atmel-isi.c  |    1 +
>  drivers/media/platform/soc_camera/pxa_camera.c |    1 +
>  drivers/media/platform/soc_camera/rcar_vin.c   |    1 +

You'll find a couple of notes below, but none of them is critical, so you 
can have my

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

for soc-camera drivers above.

>  drivers/media/v4l2-core/v4l2-of.c              |   51 +++++++++++++++++++++++-
>  include/media/v4l2-of.h                        |    9 +++++
>  14 files changed, 84 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index aaab9c9..9c6c2a5 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -2624,6 +2624,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
>  		return -EINVAL;
>  
>  	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
> +	v4l2_of_release_endpoint(&bus_cfg);
>  	of_node_put(endpoint);
>  
>  	flags = bus_cfg.bus.parallel.flags;

This is a general comment - for this and all other "trivial" cases below: 
I understand that none of them use your new dynamically-allocated field, 
but still a sequence like

	release(&x);
	y = x.z;

looks like an invitation for future bugs to me. You can check all patches 
locations and put release() after the last use of the endpoint, but, well, 
I'm not sure how really important this is.

[snip]

> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index ecae76b..2b3cd9e 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2977,7 +2977,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>  	struct smiapp_platform_data *pdata;
>  	struct v4l2_of_endpoint bus_cfg;
>  	struct device_node *ep;
> -	uint32_t asize;
> +	int i;
>  	int rval;
>  
>  	if (!dev->of_node)
> @@ -2987,12 +2987,14 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
>  	if (!ep)
>  		return NULL;
>  
> +	rval = v4l2_of_parse_endpoint(ep, &bus_cfg);
> +	if (rval < 0)
> +		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) {
>  	case V4L2_MBUS_CSI2:
>  		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
> @@ -3022,34 +3024,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_kmalloc_array(
> +		dev, bus_cfg.nr_of_link_frequencies + 1 /* guardian */,
> +		sizeof(*bus_cfg.link_frequencies), GFP_KERNEL);

You probably want "sizeof(*pdata->op_sys_clock)"

>  	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_release_endpoint(&bus_cfg);
>  	of_node_put(ep);
>  	return pdata;
>  
>  out_err:
> +	v4l2_of_release_endpoint(&bus_cfg);
>  	of_node_put(ep);
>  	return NULL;
>  }

[snip]

> diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> index b4ed9a9..9043eea 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>
>  
> @@ -109,6 +110,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
>  }
>  
>  /**
> + * v4l2_of_release_endpoint() - release resources acquired by
> + * v4l2_of_parse_endpoint()
> + * @endpoint - the endpoint the resources of which are to be released
> + *
> + * It is safe to call this function on an endpoint which is not parsed or
> + * and endpoint the parsing of which failed. However in the former case the
> + * argument must point to a struct the memory of which has been set to zero.
> + *
> + * Values in the struct v4l2_of_endpoint that are not connected to resources
> + * acquired by v4l2_of_parse_endpoint() are guaranteed to remain untouched.
> + */
> +void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +	kfree(endpoint->link_frequencies);
> +	endpoint->link_frequencies = NULL;
> +	endpoint->nr_of_link_frequencies = 0;
> +}
> +EXPORT_SYMBOL(v4l2_of_parse_endpoint);

You want to swap "EXPORT_SYMBOL()" calls here and below :)

> +
> +/**
>   * v4l2_of_parse_endpoint() - parse all endpoint node properties
>   * @node: pointer to endpoint device_node
>   * @endpoint: pointer to the V4L2 OF endpoint data structure
> @@ -122,14 +143,40 @@ 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.
>   *
> + * An endpoint parsed using v4l2_of_parse_endpoint() must be released using
> + * v4l2_of_release_endpoint().
> + *
>   * Return: 0.
>   */
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>  			   struct v4l2_of_endpoint *endpoint)
>  {
> +	int len;
> +
>  	of_graph_parse_endpoint(node, &endpoint->base);
>  	endpoint->bus_type = 0;
> -	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
> +	/* Zero fields from bus to until head (excluding) */
> +	memset(&endpoint->bus, 0, offsetof(typeof(*endpoint), head) -
> +	       offsetof(typeof(*endpoint), bus));

You can move fields around in struct v4l2_of_endpoint to avoid this risky 
arithmetic. If you move it to the beginning, you'll only have one 
offsetof(). OTOH, maybe it's better to play safe and zero out each field 
explicitly.

> +
> +	if (of_get_property(node, "link-frequencies", &len)) {
> +		int rval;
> +
> +		endpoint->link_frequencies = kmalloc(len, GFP_KERNEL);
> +		if (!endpoint->link_frequencies)
> +			return -ENOMEM;
> +
> +		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) {
> +			v4l2_of_release_endpoint(endpoint);
> +			return rval;
> +		}
> +	}
>  
>  	v4l2_of_parse_csi_bus(node, endpoint);
>  	/*
> @@ -141,4 +188,4 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> +EXPORT_SYMBOL(v4l2_of_release_endpoint);
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 70fa7b7..8c123ff 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -54,6 +54,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
>   * @head: list head for this structure
>   */
>  struct v4l2_of_endpoint {
> @@ -63,12 +65,15 @@ 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;
>  	struct list_head head;
>  };
>  
>  #ifdef CONFIG_OF
>  int v4l2_of_parse_endpoint(const struct device_node *node,
>  			   struct v4l2_of_endpoint *endpoint);
> +void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint);
>  #else /* CONFIG_OF */
>  
>  static inline int v4l2_of_parse_endpoint(const struct device_node *node,
> @@ -77,6 +82,10 @@ static inline int v4l2_of_parse_endpoint(const struct device_node *node,
>  	return -ENOSYS;
>  }
>  
> +static void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
> +{
> +}
> +
>  #endif /* CONFIG_OF */
>  
>  #endif /* _V4L2_OF_H */
> -- 
> 1.7.10.4

Thanks
Guennadi

Re: [PATCH v1.1 3/3] v4l: of: Add link-frequencies array to struct v4l2_of_endpoint

[Sakari Ailus]

Hi Guennadi,

Many thanks for the comments!

On Sun, Mar 15, 2015 at 06:15:27PM +0100, Guennadi Liakhovetski wrote:
> Hi Sakari,
> 
> Thanks for the patches.
> 
> On Thu, 12 Mar 2015, Sakari Ailus wrote:
> 
> > Parse and read the link-frequencies property in v4l2_of_parse_endpoint().
> > The property is an u64 array of undefined length, thus the memory allocation
> > may fail, leading
> > 
> > - v4l2_of_parse_endpoint() to return an error in such a case (as well as
> >   when failing to parse the property) and
> > - to requiring releasing the memory reserved for the array
> >   (v4l2_of_release_endpoint()).
> > 
> > If a driver does not need to access properties that require memory
> > allocation (such as link-frequencies), it may choose to call
> > v4l2_of_release_endpoint() right after calling v4l2_of_parse_endpoint().
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > ---
> > since v1:
> > 
> > - Set the link_frequencies pointer to NULL and set nr_of_link_frequencies to
> >   zero if link-frequencies property cannot be found by changing memset
> >   arguments. Thanks to Prabhakar for spotting this!
> > 
> >  drivers/media/i2c/adv7604.c                    |    1 +
> >  drivers/media/i2c/s5c73m3/s5c73m3-core.c       |    1 +
> >  drivers/media/i2c/s5k5baf.c                    |    1 +
> >  drivers/media/i2c/smiapp/smiapp-core.c         |   32 +++++++--------
> >  drivers/media/i2c/tvp514x.c                    |    1 +
> >  drivers/media/i2c/tvp7002.c                    |    1 +
> >  drivers/media/platform/am437x/am437x-vpfe.c    |    1 +
> >  drivers/media/platform/exynos4-is/media-dev.c  |    1 +
> >  drivers/media/platform/exynos4-is/mipi-csis.c  |    1 +
> >  drivers/media/platform/soc_camera/atmel-isi.c  |    1 +
> >  drivers/media/platform/soc_camera/pxa_camera.c |    1 +
> >  drivers/media/platform/soc_camera/rcar_vin.c   |    1 +
> 
> You'll find a couple of notes below, but none of them is critical, so you 
> can have my
> 
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> for soc-camera drivers above.

Thanks --- but I think I've somewhat reworked the patch already. I'll send a
new version. I think the changes should answer to at least some of the
concerns you've had.

> >  drivers/media/v4l2-core/v4l2-of.c              |   51 +++++++++++++++++++++++-
> >  include/media/v4l2-of.h                        |    9 +++++
> >  14 files changed, 84 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> > index aaab9c9..9c6c2a5 100644
> > --- a/drivers/media/i2c/adv7604.c
> > +++ b/drivers/media/i2c/adv7604.c
> > @@ -2624,6 +2624,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
> >  		return -EINVAL;
> >  
> >  	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
> > +	v4l2_of_release_endpoint(&bus_cfg);
> >  	of_node_put(endpoint);
> >  
> >  	flags = bus_cfg.bus.parallel.flags;
> 
> This is a general comment - for this and all other "trivial" cases below: 
> I understand that none of them use your new dynamically-allocated field, 
> but still a sequence like
> 
> 	release(&x);
> 	y = x.z;
> 
> looks like an invitation for future bugs to me. You can check all patches 
> locations and put release() after the last use of the endpoint, but, well, 
> I'm not sure how really important this is.

I didn't much like the construct either, I have to admit. Instead, I thought
of adding two more interface functions, v4l2_of_endpoint_get() and
v4l2_of_endpoint_put(). The former is v4l2_of_parse_endpoint() amended with
parsing variable size nodes (without small maximum size), and the latter is
what v4l2_of_release_endpoint() was in this patch.

The naming probably could be improved, but this way the existing users don't
need to start caring about acquiring or releasing resources they do not
need.

> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> > index ecae76b..2b3cd9e 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -2977,7 +2977,7 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
> >  	struct smiapp_platform_data *pdata;
> >  	struct v4l2_of_endpoint bus_cfg;
> >  	struct device_node *ep;
> > -	uint32_t asize;
> > +	int i;
> >  	int rval;
> >  
> >  	if (!dev->of_node)
> > @@ -2987,12 +2987,14 @@ static struct smiapp_platform_data *smiapp_get_pdata(struct device *dev)
> >  	if (!ep)
> >  		return NULL;
> >  
> > +	rval = v4l2_of_parse_endpoint(ep, &bus_cfg);
> > +	if (rval < 0)
> > +		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) {
> >  	case V4L2_MBUS_CSI2:
> >  		pdata->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
> > @@ -3022,34 +3024,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_kmalloc_array(
> > +		dev, bus_cfg.nr_of_link_frequencies + 1 /* guardian */,
> > +		sizeof(*bus_cfg.link_frequencies), GFP_KERNEL);
> 
> You probably want "sizeof(*pdata->op_sys_clock)"

Yes. Fixed.

> >  	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]);
> >  	}

^

This one was missing assigning the value at last index to zero... ouch.

> >  
> > -	for (; asize > 0; asize--)
> > -		dev_dbg(dev, "freq %d: %lld\n", asize - 1,
> > -			pdata->op_sys_clock[asize - 1]);
> > -
> > +	v4l2_of_release_endpoint(&bus_cfg);
> >  	of_node_put(ep);
> >  	return pdata;
> >  
> >  out_err:
> > +	v4l2_of_release_endpoint(&bus_cfg);
> >  	of_node_put(ep);
> >  	return NULL;
> >  }
> 
> [snip]
> 
> > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> > index b4ed9a9..9043eea 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>
> >  
> > @@ -109,6 +110,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
> >  }
> >  
> >  /**
> > + * v4l2_of_release_endpoint() - release resources acquired by
> > + * v4l2_of_parse_endpoint()
> > + * @endpoint - the endpoint the resources of which are to be released
> > + *
> > + * It is safe to call this function on an endpoint which is not parsed or
> > + * and endpoint the parsing of which failed. However in the former case the
> > + * argument must point to a struct the memory of which has been set to zero.
> > + *
> > + * Values in the struct v4l2_of_endpoint that are not connected to resources
> > + * acquired by v4l2_of_parse_endpoint() are guaranteed to remain untouched.
> > + */
> > +void v4l2_of_release_endpoint(struct v4l2_of_endpoint *endpoint)
> > +{
> > +	kfree(endpoint->link_frequencies);
> > +	endpoint->link_frequencies = NULL;
> > +	endpoint->nr_of_link_frequencies = 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_of_parse_endpoint);
> 
> You want to swap "EXPORT_SYMBOL()" calls here and below :)

Fixed.

> > +
> > +/**
> >   * v4l2_of_parse_endpoint() - parse all endpoint node properties
> >   * @node: pointer to endpoint device_node
> >   * @endpoint: pointer to the V4L2 OF endpoint data structure
> > @@ -122,14 +143,40 @@ 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.
> >   *
> > + * An endpoint parsed using v4l2_of_parse_endpoint() must be released using
> > + * v4l2_of_release_endpoint().
> > + *
> >   * Return: 0.
> >   */
> >  int v4l2_of_parse_endpoint(const struct device_node *node,
> >  			   struct v4l2_of_endpoint *endpoint)
> >  {
> > +	int len;
> > +
> >  	of_graph_parse_endpoint(node, &endpoint->base);
> >  	endpoint->bus_type = 0;
> > -	memset(&endpoint->bus, 0, sizeof(endpoint->bus));
> > +	/* Zero fields from bus to until head (excluding) */
> > +	memset(&endpoint->bus, 0, offsetof(typeof(*endpoint), head) -
> > +	       offsetof(typeof(*endpoint), bus));
> 
> You can move fields around in struct v4l2_of_endpoint to avoid this risky 
> arithmetic. If you move it to the beginning, you'll only have one 
> offsetof(). OTOH, maybe it's better to play safe and zero out each field 
> explicitly.

I've thought about different options and I think my preference is to keep it
as-is (or close to that), with perhaps adding a comment on the matter.

When additional fields are added to the struct, they would need to be taken
into account in multiple places. Now there's just where to allocate and
release them (setting pointers to NULL can be achieved by using memset,
too).

I'll resend, please tell me what's your opinion. I'm open to other options,
too.

-- 
Kind regards,

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