Re: [PATCH v8 35/38] media: ov2740: Add support for embedded data

[Julien Massot <julien.massot@collabora.com>]

Hi Sakari,

On 3/13/24 08:25, Sakari Ailus wrote:
> Add support for embedded data. This introduces two internal pads for pixel
> and embedded data streams. As the driver supports a single mode only,
> there's no need for backward compatibility in mode selection.
> 
> The embedded data is configured to be placed before the image data whereas
> after the image data is the default.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>   drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
>   1 file changed, 137 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index df57f0096e98..7488b2535071 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/nvmem-provider.h>
>   #include <linux/regmap.h>
> +#include <media/mipi-csi2.h>
>   #include <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-fwnode.h>
> @@ -71,11 +72,31 @@
>   #define OV2740_REG_ISP_CTRL00		0x5000
>   /* ISP CTRL01 */
>   #define OV2740_REG_ISP_CTRL01		0x5001
> +
> +/* Embedded data line location control */
> +#define OV2740_REG_EMBEDDED_FLAG	0x5a08
> +#define OV2740_EMBEDDED_FLAG_FOOTER	BIT(2) /* otherwise it's in header */
> +#define OV2740_EMBEDDED_FLAG_MYSTERY	BIT(1)
>   /* Customer Addresses: 0x7010 - 0x710F */
>   #define CUSTOMER_USE_OTP_SIZE		0x100
>   /* OTP registers from sensor */
>   #define OV2740_REG_OTP_CUSTOMER		0x7010
>   
> +enum {
> +	OV2740_PAD_SOURCE,
> +	OV2740_PAD_PIXEL,
> +	OV2740_PAD_META,
> +	OV2740_NUM_PADS,
> +};
> +
> +enum {
> +	OV2740_STREAM_PIXEL,
> +	OV2740_STREAM_META,
> +};
> +
> +#define OV2740_META_WIDTH		100U /* 97 bytes of actual data */
> +#define OV2740_META_HEIGHT		1U
> +
>   struct nvm_data {
>   	struct nvmem_device *nvmem;
>   	struct regmap *regmap;
> @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
>   
>   struct ov2740 {
>   	struct v4l2_subdev sd;
> -	struct media_pad pad;
> +	struct media_pad pads[OV2740_NUM_PADS];
>   	struct v4l2_ctrl_handler ctrl_handler;
>   
>   	/* V4L2 Controls */
> @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
>   	if (ret)
>   		goto out_pm_put;
>   
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> +			       OV2740_EMBEDDED_FLAG_MYSTERY);
> +	if (ret)
> +		return ret;
> +
>   	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>   			       OV2740_MODE_STREAMING);
>   	if (ret) {
> @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
>   	return ret;
>   }
>   
> -static int ov2740_set_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *fmt)
> +static int __ov2740_set_format(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *sd_state,
> +			       struct v4l2_mbus_framefmt *format,
> +			       enum v4l2_subdev_format_whence which,
> +			       unsigned int pad, unsigned int stream)
>   {
> +	struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> +		*meta_fmt;
>   	struct ov2740 *ov2740 = to_ov2740(sd);
>   	const struct ov2740_mode *mode;
>   	s32 vblank_def, h_blank;
>   
> +	/*
> +	 * Allow setting format on internal pixel pad as well as the source
> +	 * pad's pixel stream (for compatibility).
> +	 */
> +	if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> +	    stream == OV2740_STREAM_META) {
This is equivalent to
if (pad != OV2740_PAD_PIXEL)
Correct me if I'm wrong but this code doesn't allow to set the format on 
the source pad.

Should it be:
if ((pad == OV2740_PAD_SOURCE && stream == OV2740_STREAM_META) ||
	pad == OV2740_PAD_META) {


> +		*format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> +		return 0;
> +	}
> +
> +	pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> +	meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> +	src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						   OV2740_STREAM_PIXEL);
> +	src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> +						    OV2740_STREAM_META);
> +
>   	mode = v4l2_find_nearest_size(ov2740->supported_modes,
>   				      ov2740->supported_modes_count,
>   				      width, height,
> -				      fmt->format.width, fmt->format.height);
> +				      format->width, format->height);
> +	ov2740_update_pad_format(mode, pix_fmt);
> +	*format = *src_pix_fmt = *pix_fmt;
>   
> -	ov2740_update_pad_format(mode, &fmt->format);
> -	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +	meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> +	meta_fmt->width = OV2740_META_WIDTH;
> +	meta_fmt->height = OV2740_META_HEIGHT;
> +	*src_meta_fmt = *meta_fmt;
> +	src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
>   
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
>   		return 0;
>   
>   	ov2740->cur_mode = mode;
Regards,
-- 
Julien Massot
Senior Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718