Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: laurent.pinchart@ideasonboard.com,
	niklas.soderlund+renesas@ragnatech.se,
	kieran.bingham@ideasonboard.com, linux-media@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org,
	dave.stevenson@raspberrypi.org
Subject: Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc
Date: Sat, 16 Mar 2019 19:51:21 +0200	[thread overview]
Message-ID: <20190316175121.wdek74c7tfpmrhde@kekkonen.localdomain> (raw)
In-Reply-To: <20190316154801.20460-2-jacopo+renesas@jmondi.org>

Hi Jacopo,

On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6311f670de3c..eca9633c83bf 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>  };
>  
> +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES	4
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> + *
> + * @clock_lane:		physical lane index of the clock lane
> + * @data_lanes:		an array of physical data lane indexes
> + * @num_data_lanes:	number of data lanes
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> +	u8 clock_lane;
> +	u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> +	u8 num_data_lanes;

Do you need more than the number of the data lanes? I'd expect few devices
to be able to do more than that. The PHY type comes already from the
firmware but I guess it's good to do the separation here as well.

Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I
think it'd be good to stick to a single definition.

> +};
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> +	/* TODO */
> +};
> +
>  /**
>   * struct v4l2_mbus_frame_desc_entry_csi2
>   *
> - * @channel: CSI-2 virtual channel
> - * @data_type: CSI-2 data type ID
> + * @channel:	CSI-2 virtual channel
> + * @data_type:	CSI-2 data type ID
>   */
>  struct v4l2_mbus_frame_desc_entry_csi2 {
>  	u8 channel;
> @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
>  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
>  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
>  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
>  };
>  
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
> - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> + * @entry:		frame descriptors array
> + * @phy:		PHY specific parameters
> + * @phy.dphy:		MIPI D-PHY specific bus configurations
> + * @phy.cphy:		MIPI C-PHY specific bus configurations
> + * @num_entries:	number of entries in @entry array
>   */
>  struct v4l2_mbus_frame_desc {
>  	enum v4l2_mbus_frame_desc_type type;
>  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> +	union {
> +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> +	} phy;
>  	unsigned short num_entries;
>  };
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com