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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> + *
> + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
