Re: [PATCH 2/5] v4l2-fwnode: v4l2_fwnode_endpoint_parse caller must init vep argument

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, jmondi@jmondi.org
Subject: Re: [PATCH 2/5] v4l2-fwnode: v4l2_fwnode_endpoint_parse caller must init vep argument
Date: Sun, 4 Oct 2020 11:35:33 +0300	[thread overview]
Message-ID: <20201004083533.GC3938@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200930144811.16612-3-sakari.ailus@linux.intel.com>

Hi Sakari,

Thank you for the patch.

On Wed, Sep 30, 2020 at 05:48:08PM +0300, Sakari Ailus wrote:
> Document that the caller of v4l2_fwnode_endpoint_parse() must init the
> fields of struct v4l2_fwnode_endpoint (vep argument) fields.
> 
> It used to be that the fields were zeroed by v4l2_fwnode_endpoint_parse
> when bus type was set to V4L2_MBUS_UNKNOWN but with commit bb4bba9232fc
> that no longer makes sense.
> 
> Fixes: bb4bba9232fc ("media: v4l2-fwnode: Make bus configuration a struct")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-fwnode.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index c09074276543..0f9a768b1a8d 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -231,6 +231,8 @@ struct v4l2_fwnode_connector {
>   * guessing @vep.bus_type between CSI-2 D-PHY, parallel and BT.656 busses is
>   * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
>   *
> + * The caller is required to initialise all fields of @vep.

Would it make sense to explicitly state that fields should be zeroed if
no specific value is desired ? Maybe

 * The caller is required to initialise all fields of @vep, either with
 * explicitly values, or by zeroing them.

Apart from that,

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

> + *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *
>   * NOTE: This function does not parse properties the size of which is variable
> @@ -273,6 +275,8 @@ void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep);
>   * guessing @vep.bus_type between CSI-2 D-PHY, parallel and BT.656 busses is
>   * supported. NEVER RELY ON GUESSING @vep.bus_type IN NEW DRIVERS!
>   *
> + * The caller is required to initialise all fields of @vep.
> + *
>   * The function does not change the V4L2 fwnode endpoint state if it fails.
>   *
>   * v4l2_fwnode_endpoint_alloc_parse() has two important differences to

-- 
Regards,

Laurent Pinchart