Hi Sakari, Laurent, all..

The more I look at this patch, the less I like it, to be honest...

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;
> +};
> +
> +/**
> + * 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;

I had a brief look at how v4l2_mbus_frame_desc evolved, and how Sakari
has extended it with stream and CSI-2 specific information in the
v4l-multiplexed series[1] before I've thrown the phy configuration part
in the mix with this patch, and my feeling is that we're maybe bending
the original v4l2_mbus_frame_desc purpose to our needs a bit...

What I'm questioning is if stream and media bus configuration actually
belong here. Sure we could bend this to match our purposes, but
'struct v4l2_mbus_frame_desc' is actually only used in mainline to
transport informations about binary streams, and it fits the purpose,
which is, by the way, also intended in the structure name.

What I think would be more appropriate would be a 'streaming session'
descriptor, that contains entries for up to 4 streams and one phy
configuration part. I would even say that, as this changes are intended
to make frame_desc usable to negotiate multiplexed streams parameters
that are only available on CSI-2 (VC and data type at the protocol level,
and since this patch, a few D-PHY parameters at the phy level) this
could actually be (at least initially) a new CSI-2 specific
structure with a new set of operations associated, so that we can leave
frame_desc alone to be used for binary blob description?

What do you think? Are you comfortable with this change?

Thanks
   j


[1]
https://patchwork.kernel.org/patch/10875871/
https://patchwork.kernel.org/patch/10875873/
https://patchwork.kernel.org/patch/10875879/

From the same series, here it is how frame_desc is now used:
https://patchwork.kernel.org/patch/10875911/
https://patchwork.kernel.org/patch/10875911/

And from this RFC series how it would be used to expose phy
parameters:
https://patchwork.kernel.org/patch/10855937/



>  	unsigned short num_entries;
>  };
>
> --
> 2.21.0
>