Re: [PATCH v2 07/23] v4l: fwnode: Let the caller provide V4L2 fwnode endpoint

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 6155 bytes --]

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@linux.intel.com>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]