Hi Sakari,
On Mon, Aug 27, 2018 at 12:29:44PM +0300, Sakari Ailus wrote:
> Instead of allocating the V4L2 fwnode endpoint in
> v4l2_fwnode_endpoint_alloc_parse, let the caller to do this. This allows
> setting default parameters for the endpoint which is a very common need
> for drivers.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/ov2659.c | 14 +++++-----
> drivers/media/i2c/smiapp/smiapp-core.c | 26 +++++++++---------
> drivers/media/i2c/tc358743.c | 26 +++++++++---------
> drivers/media/v4l2-core/v4l2-fwnode.c | 49 +++++++++++++---------------------
> include/media/v4l2-fwnode.h | 10 ++++---
> 5 files changed, 60 insertions(+), 65 deletions(-)
>
[snip]
> -struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> - struct fwnode_handle *fwnode)
> +int v4l2_fwnode_endpoint_alloc_parse(
> + struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep)
Looking at the resulting implementation of
"v4l2_fwnode_endpoint_alloc_parse" and "v4l2_fwnode_endpoint_parse" I
wonder if there's still value in keeping them separate... Now that in
both cases the caller has to provide an v4l2_fwnode_endpoint, isn't it
worth making a single function out of them, that behaves like
"alloc_parse" is doing nowadays (allocates vep->link_frequencies
conditionally on the presence of the "link-frequencies" property) ?
Or is the size of the allocated vep relevant in the async subdevice
matching or registration process? I guess not, but I might be missing
something...
Thanks
j
> {
> - struct v4l2_fwnode_endpoint *vep;
> int rval;
>
> - vep = kzalloc(sizeof(*vep), GFP_KERNEL);
> - if (!vep)
> - return ERR_PTR(-ENOMEM);
> -
> rval = __v4l2_fwnode_endpoint_parse(fwnode, vep);
> if (rval < 0)
> - goto out_err;
> + return rval;
>
> rval = fwnode_property_read_u64_array(fwnode, "link-frequencies",
> NULL, 0);
> @@ -316,18 +310,18 @@ struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> vep->link_frequencies =
> kmalloc_array(rval, sizeof(*vep->link_frequencies),
> GFP_KERNEL);
> - if (!vep->link_frequencies) {
> - rval = -ENOMEM;
> - goto out_err;
> - }
> + if (!vep->link_frequencies)
> + return -ENOMEM;
>
> vep->nr_of_link_frequencies = rval;
>
> rval = fwnode_property_read_u64_array(
> fwnode, "link-frequencies", vep->link_frequencies,
> vep->nr_of_link_frequencies);
> - if (rval < 0)
> - goto out_err;
> + if (rval < 0) {
> + v4l2_fwnode_endpoint_free(vep);
> + return rval;
> + }
>
> for (i = 0; i < vep->nr_of_link_frequencies; i++)
> pr_info("link-frequencies %u value %llu\n", i,
> @@ -336,11 +330,7 @@ struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
>
> pr_debug("===== end V4L2 endpoint properties\n");
>
> - return vep;
> -
> -out_err:
> - v4l2_fwnode_endpoint_free(vep);
> - return ERR_PTR(rval);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_alloc_parse);
>
> @@ -392,9 +382,9 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd))
> {
> + struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_UNKNOWN };
> struct v4l2_async_subdev *asd;
> - struct v4l2_fwnode_endpoint *vep;
> - int ret = 0;
> + int ret;
>
> asd = kzalloc(asd_struct_size, GFP_KERNEL);
> if (!asd)
> @@ -409,23 +399,22 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> goto out_err;
> }
>
> - vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> - if (IS_ERR(vep)) {
> - ret = PTR_ERR(vep);
> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &vep);
> + if (ret) {
> dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> ret);
> goto out_err;
> }
>
> - ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> + ret = parse_endpoint ? parse_endpoint(dev, &vep, asd) : 0;
> if (ret == -ENOTCONN)
> - dev_dbg(dev, "ignoring port@%u/endpoint@%u\n", vep->base.port,
> - vep->base.id);
> + dev_dbg(dev, "ignoring port@%u/endpoint@%u\n", vep.base.port,
> + vep.base.id);
> else if (ret < 0)
> dev_warn(dev,
> "driver could not parse port@%u/endpoint@%u (%d)\n",
> - vep->base.port, vep->base.id, ret);
> - v4l2_fwnode_endpoint_free(vep);
> + vep.base.port, vep.base.id, ret);
> + v4l2_fwnode_endpoint_free(&vep);
> if (ret < 0)
> goto out_err;
>
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 8b4873c37098..4a371c3ad86c 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -161,6 +161,7 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
> /**
> * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> * @fwnode: pointer to the endpoint's fwnode handle
> + * @vep: pointer to the V4L2 fwnode data structure
> *
> * 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
> @@ -170,6 +171,8 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
> * set the V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a
> * reference to @fwnode.
> *
> + * The caller must set the bus_type field of @vep to zero.
> + *
> * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> * v4l2_fwnode_endpoint_parse():
> *
> @@ -178,11 +181,10 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
> * 2. The memory it has allocated to store the variable size data must be freed
> * using v4l2_fwnode_endpoint_free() when no longer needed.
> *
> - * Return: Pointer to v4l2_fwnode_endpoint if successful, on an error pointer
> - * on error.
> + * Return: 0 on success or a negative error code on failure.
> */
> -struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> - struct fwnode_handle *fwnode);
> +int v4l2_fwnode_endpoint_alloc_parse(
> + struct fwnode_handle *fwnode, struct v4l2_fwnode_endpoint *vep);
>
> /**
> * v4l2_fwnode_parse_link() - parse a link between two endpoints
> --
> 2.11.0
>
Attachment:
signature.asc
Description: PGP signature
