| Message ID | 1428614706-8367-4-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/04/15 23:25, 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> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> It's a bit sad we need to introduce the ERR_PTR() patter, nevertheless I can't see a better alternative now. I think in long term we need to get rid of v4l2_of_parse_endpoint() function. Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Hi Sakari, Thanks for the patch. On Thu, Apr 9, 2015 at 10:25 PM, Sakari Ailus <sakari.ailus@iki.fi> 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> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Cheers, --Prabhakar Lad > --- > drivers/media/v4l2-core/v4l2-of.c | 87 +++++++++++++++++++++++++++++++++++++ > include/media/v4l2-of.h | 17 ++++++++ > 2 files changed, 104 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c > index 3ac6348..c52fb96 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,88 @@ int v4l2_of_parse_endpoint(const struct device_node *node, > } > EXPORT_SYMBOL(v4l2_of_parse_endpoint); > > +/* > + * v4l2_of_free_endpoint() - free the endpoint 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 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 it has allocated to store the variable size data must > + * be freed 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 6c85c07..241e98a 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c index 3ac6348..c52fb96 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,88 @@ int v4l2_of_parse_endpoint(const struct device_node *node, } EXPORT_SYMBOL(v4l2_of_parse_endpoint); +/* + * v4l2_of_free_endpoint() - free the endpoint 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 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 it has allocated to store the variable size data must + * be freed 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 6c85c07..241e98a 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) {