[6/8] WmT: adv7604 driver compatibility

  Add 'struct media_pad pad' member and suitable glue code, so that
soc_camera/rcar_vin can become agnostic to whether an old or new-
style driver (wrt pad API use) can sit underneath

This version has been reworked to include appropriate constant and
datatype names for kernel v3.18
---
 drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
 drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
 include/media/soc_camera.h                         |    1 +
 3 files changed, 182 insertions(+), 10 deletions(-)
  

Hi Wills,

Thanks for the patch. First and foremost, the title of the patch is wrong. 
This patch does more than just adding some "adv7604 compatibility." It's 
adding pad-level API to soc-camera.

This is just a rough review. I'm not an expert in media-controller / 
pad-level API, I hope someone with a better knowledge of those areas will 
help me reviewing this.

Another general comment: it has been discussed since a long time, whether 
a wrapper wouldn't be desired to enable a seamless use of both subdev 
drivers using and not using the pad-level API. Maybe it's the right time 
now?..

On Thu, 29 Jan 2015, William Towle wrote:

> Add 'struct media_pad pad' member and suitable glue code, so that
> soc_camera/rcar_vin can become agnostic to whether an old or new-
> style driver (wrt pad API use) can sit underneath
> 
> This version has been reworked to include appropriate constant and
> datatype names for kernel v3.18
> ---
>  drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
>  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
>  include/media/soc_camera.h                         |    1 +
>  3 files changed, 182 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index f4be2a1..efc20bf 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -37,8 +37,11 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-of.h>
> +#if 0
>  #include <media/videobuf-core.h>
>  #include <media/videobuf2-core.h>
> +#endif

No. These headers are needed even if the code can be compiled without 
them.

> +#include <media/v4l2-mediabus.h>

Well, maybe. This header is included indirectly via soc_mediabus.h, but 
yes, as I just said above, headers, whose defines, structs etc. are used, 
should be encluded directly. Further, you'll need more headers, e.g. 
media-entity.h, maybe some more.

>  /* Default to VGA resolution */
>  #define DEFAULT_WIDTH	640
> @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file, void *priv,
>  		return vb2_expbuf(&icd->vb2_vidq, p);
>  }
>  
> +static int soc_camera_init_user_formats_pad(struct soc_camera_device *icd, int src_pad_idx)
> +{
> +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> +	struct v4l2_subdev_mbus_code_enum code;
> +	int fmts= 0, raw_fmts, i, ret;

Please, run this patch through checkpatch.pl. It will tell you to add a 
Signed-off-by line, (hopefully) to add spaces before "=" in multiple 
places, to place braces correctly, to not use C++-style comments etc. Only 
feel free to ignore 80-character warnings.

> +
> +	code.pad= src_pad_idx;
> +	code.index= 0;
> +
> +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {

This function is called only once below and only after the above test has 
already returned success. Looks like you don't need it here again and the 
below "else" branch can be dropped completely?

> +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> +			code.index++;
> +	} else {
> +		u32 pixcode;
> +
> +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode))
> +		{
> +			code.code= pixcode;
> +			code.index++;
> +		}
> +	}
> +	raw_fmts= code.index;
> +
> +	if (!ici->ops->get_formats) {
> +		/*
> +		 * Fallback mode - the host will have to serve all
> +		 * sensor-provided formats one-to-one to the user
> +		 */
> +		fmts = raw_fmts;
> +	}
> +	else {
> +		/*
> +		 * First pass - only count formats this host-sensor
> +		 * configuration can provide
> +		 */
> +		for (i = 0; i < raw_fmts; i++) {
> +			int ret = ici->ops->get_formats(icd, i, NULL);
> +			if (ret < 0)
> +				return ret;
> +			fmts += ret;
> +		}
> +	}
> +
> +	if (!fmts)
> +		return -ENXIO;
> +
> +	icd->user_formats =
> +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> +	if (!icd->user_formats)
> +		return -ENOMEM;
> +
> +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> +
> +	/* Second pass - actually fill data formats */
> +	fmts = 0;
> +	for (i = 0; i < raw_fmts; i++) {
> +		if (!ici->ops->get_formats) {
> +			code.index= i;
> +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {

Same test again?? Or am I missing something? If indeed these tests are 
redundant, after you remove them this function will become very similar to 
the original soc_camera_init_user_formats(), so, maybe some code reuse 
will become possible.

> +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> +			} else {
> +				u32 pixcode;
> +
> +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode);
> +				code.code= pixcode;
> +			}
> +			icd->user_formats[fmts].host_fmt =
> +				soc_mbus_get_fmtdesc(code.code);
> +			if (icd->user_formats[fmts].host_fmt)
> +				icd->user_formats[fmts++].code = code.code;
> +		} else {
> +			ret = ici->ops->get_formats(icd, i,
> +						    &icd->user_formats[fmts]);
> +			if (ret < 0)
> +				goto egfmt;
> +			fmts += ret;
> +		}
> +	}
> +
> +	icd->num_user_formats = fmts;
> +	icd->current_fmt = &icd->user_formats[0];
> +
> +	return 0;
> +
> +egfmt:
> +	vfree(icd->user_formats);
> +	return ret;
> +}
> +
>  /* Always entered with .host_lock held */
>  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  {
> @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  {
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>  	struct v4l2_mbus_framefmt mf;
> +	int src_pad_idx= -1;
>  	int ret;
>  
>  	sd->grp_id = soc_camera_grp_id(icd);
> @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  	}
>  
>  	/* At this point client .probe() should have run already */
> -	ret = soc_camera_init_user_formats(icd);
> +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))

This is the test, that I meant above.

> +		ret = soc_camera_init_user_formats(icd);
> +	else {
> +		ret = media_entity_init(&icd->vdev->entity, 1,
> +					&icd->pad, 0);

Ok, maybe this hard-coded 1 pad with no extras is justified here, but 
let's here what others say.

> +		if (!ret) {
> +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads; src_pad_idx++)
> +				if (sd->entity.pads[src_pad_idx].flags == MEDIA_PAD_FL_SOURCE)
> +					break;
> +
> +			if (src_pad_idx < sd->entity.num_pads) {
> +				ret = media_entity_create_link(
> +					&icd->vdev->entity, 0,
> +					&sd->entity, src_pad_idx,
> +					MEDIA_LNK_FL_IMMUTABLE |
> +					MEDIA_LNK_FL_ENABLED);

Let's try to preserve the style. I normally try to avoid splitting the 
line after "f(" and adding at least the first function parameter above 
will not make that line longer, than the ones above. So, let's do that.

> +			}
> +		}
> +
> +		if (!ret)
> +			ret = soc_camera_init_user_formats_pad(icd,
> +							src_pad_idx);

Probably no need to break the line here either.

> +	}
>  	if (ret < 0)
>  		goto eusrfmt;
>  
> @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  		goto evidstart;
>  
>  	/* Try to improve our guess of a reasonable window format */
> -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> -		icd->user_width		= mf.width;
> -		icd->user_height	= mf.height;
> -		icd->colorspace		= mf.colorspace;
> -		icd->field		= mf.field;
> +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> +		&& src_pad_idx != -1) {

The rest of the file puts operations after the first argument, not before 
the second one when breaking the line. Let's do that here too.

Thanks
Guennadi

> +		struct v4l2_subdev_format sd_format;
> +
> +		sd_format.pad= src_pad_idx;
> +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> +			icd->user_width		= sd_format.format.width;
> +			icd->user_height	= sd_format.format.height;
> +			icd->colorspace		= sd_format.format.colorspace;
> +			icd->field		= sd_format.format.field;
> +		}
> +	} else {
> +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> +			icd->user_width		= mf.width;
> +			icd->user_height	= mf.height;
> +			icd->colorspace		= mf.colorspace;
> +			icd->field		= mf.field;
> +		}
>  	}
>  	soc_camera_remove_device(icd);
>  
> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
> index 8e74fb7..8a1ca05 100644
> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device *icd,
>  	bool host_1to1;
>  	int ret;
>  
> -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> -					 soc_camera_grp_id(icd), video,
> -					 s_mbus_fmt, mf);
> +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> +		struct v4l2_subdev_format sd_format;
> +		struct media_pad *remote_pad;
> +
> +		remote_pad= media_entity_remote_pad(
> +			&icd->vdev->entity.pads[0]);
> +		sd_format.pad = remote_pad->index;
> +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +		sd_format.format= *mf;
> +
> +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> +			&sd_format);
> +
> +		mf->width = sd_format.format.width;
> +		mf->height = sd_format.format.height;
> +	} else {
> +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
> @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device *icd,
>  		tmp_h = min(2 * tmp_h, max_height);
>  		mf->width = tmp_w;
>  		mf->height = tmp_h;
> -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> +			struct v4l2_subdev_format sd_format;
> +			struct media_pad *remote_pad;
> +
> +			remote_pad= media_entity_remote_pad(
> +				&icd->vdev->entity.pads[0]);
> +			sd_format.pad = remote_pad->index;
> +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +			sd_format.format= *mf;
> +
> +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +					soc_camera_grp_id(icd),
> +					pad, set_fmt, NULL,
> +					&sd_format);
> +		} else {
> +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
>  					soc_camera_grp_id(icd), video,
>  					s_mbus_fmt, mf);
> +		}
>  		dev_geo(dev, "Camera scaled to %ux%u\n",
>  			mf->width, mf->height);
>  		if (ret < 0) {
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 2f6261f..f0c5238 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -42,6 +42,7 @@ struct soc_camera_device {
>  	unsigned char devnum;		/* Device number per host */
>  	struct soc_camera_sense *sense;	/* See comment in struct definition */
>  	struct video_device *vdev;
> +	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	const struct soc_camera_format_xlate *current_fmt;
>  	struct soc_camera_format_xlate *user_formats;
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On a second thought:

On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:

> Hi Wills,
> 
> Thanks for the patch. First and foremost, the title of the patch is wrong. 
> This patch does more than just adding some "adv7604 compatibility." It's 
> adding pad-level API to soc-camera.
> 
> This is just a rough review. I'm not an expert in media-controller / 
> pad-level API, I hope someone with a better knowledge of those areas will 
> help me reviewing this.
> 
> Another general comment: it has been discussed since a long time, whether 
> a wrapper wouldn't be desired to enable a seamless use of both subdev 
> drivers using and not using the pad-level API. Maybe it's the right time 
> now?..

This would be a considerable change and would most probably take a rather 
long time, given how busy everyone is. I personally would be fine with a 
(yet another) intermittent solution, whereby we create a 
soc_camera_subdev.c file, in which we collect all those function to call 
either a video or a pad subdev operation, depending on whether the latter 
is available. E.g.

int soc_camera_sd_enum_mbus_fmt(sd, code)
{
	int ret;

	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, code);
	} else {
		u32 pixcode;

		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, code->index, &pixcode);
		if (!ret)
			code->code= pixcode;
	}

	return ret;
}

Similarly for other ops. 

Thanks
Guennadi

> 
> On Thu, 29 Jan 2015, William Towle wrote:
> 
> > Add 'struct media_pad pad' member and suitable glue code, so that
> > soc_camera/rcar_vin can become agnostic to whether an old or new-
> > style driver (wrt pad API use) can sit underneath
> > 
> > This version has been reworked to include appropriate constant and
> > datatype names for kernel v3.18
> > ---
> >  drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
> >  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
> >  include/media/soc_camera.h                         |    1 +
> >  3 files changed, 182 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> > index f4be2a1..efc20bf 100644
> > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > @@ -37,8 +37,11 @@
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-dev.h>
> >  #include <media/v4l2-of.h>
> > +#if 0
> >  #include <media/videobuf-core.h>
> >  #include <media/videobuf2-core.h>
> > +#endif
> 
> No. These headers are needed even if the code can be compiled without 
> them.
> 
> > +#include <media/v4l2-mediabus.h>
> 
> Well, maybe. This header is included indirectly via soc_mediabus.h, but 
> yes, as I just said above, headers, whose defines, structs etc. are used, 
> should be encluded directly. Further, you'll need more headers, e.g. 
> media-entity.h, maybe some more.
> 
> >  /* Default to VGA resolution */
> >  #define DEFAULT_WIDTH	640
> > @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file, void *priv,
> >  		return vb2_expbuf(&icd->vb2_vidq, p);
> >  }
> >  
> > +static int soc_camera_init_user_formats_pad(struct soc_camera_device *icd, int src_pad_idx)
> > +{
> > +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > +	struct v4l2_subdev_mbus_code_enum code;
> > +	int fmts= 0, raw_fmts, i, ret;
> 
> Please, run this patch through checkpatch.pl. It will tell you to add a 
> Signed-off-by line, (hopefully) to add spaces before "=" in multiple 
> places, to place braces correctly, to not use C++-style comments etc. Only 
> feel free to ignore 80-character warnings.
> 
> > +
> > +	code.pad= src_pad_idx;
> > +	code.index= 0;
> > +
> > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 
> This function is called only once below and only after the above test has 
> already returned success. Looks like you don't need it here again and the 
> below "else" branch can be dropped completely?
> 
> > +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> > +			code.index++;
> > +	} else {
> > +		u32 pixcode;
> > +
> > +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode))
> > +		{
> > +			code.code= pixcode;
> > +			code.index++;
> > +		}
> > +	}
> > +	raw_fmts= code.index;
> > +
> > +	if (!ici->ops->get_formats) {
> > +		/*
> > +		 * Fallback mode - the host will have to serve all
> > +		 * sensor-provided formats one-to-one to the user
> > +		 */
> > +		fmts = raw_fmts;
> > +	}
> > +	else {
> > +		/*
> > +		 * First pass - only count formats this host-sensor
> > +		 * configuration can provide
> > +		 */
> > +		for (i = 0; i < raw_fmts; i++) {
> > +			int ret = ici->ops->get_formats(icd, i, NULL);
> > +			if (ret < 0)
> > +				return ret;
> > +			fmts += ret;
> > +		}
> > +	}
> > +
> > +	if (!fmts)
> > +		return -ENXIO;
> > +
> > +	icd->user_formats =
> > +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> > +	if (!icd->user_formats)
> > +		return -ENOMEM;
> > +
> > +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> > +
> > +	/* Second pass - actually fill data formats */
> > +	fmts = 0;
> > +	for (i = 0; i < raw_fmts; i++) {
> > +		if (!ici->ops->get_formats) {
> > +			code.index= i;
> > +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 
> Same test again?? Or am I missing something? If indeed these tests are 
> redundant, after you remove them this function will become very similar to 
> the original soc_camera_init_user_formats(), so, maybe some code reuse 
> will become possible.
> 
> > +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> > +			} else {
> > +				u32 pixcode;
> > +
> > +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode);
> > +				code.code= pixcode;
> > +			}
> > +			icd->user_formats[fmts].host_fmt =
> > +				soc_mbus_get_fmtdesc(code.code);
> > +			if (icd->user_formats[fmts].host_fmt)
> > +				icd->user_formats[fmts++].code = code.code;
> > +		} else {
> > +			ret = ici->ops->get_formats(icd, i,
> > +						    &icd->user_formats[fmts]);
> > +			if (ret < 0)
> > +				goto egfmt;
> > +			fmts += ret;
> > +		}
> > +	}
> > +
> > +	icd->num_user_formats = fmts;
> > +	icd->current_fmt = &icd->user_formats[0];
> > +
> > +	return 0;
> > +
> > +egfmt:
> > +	vfree(icd->user_formats);
> > +	return ret;
> > +}
> > +
> >  /* Always entered with .host_lock held */
> >  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> >  {
> > @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  {
> >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> >  	struct v4l2_mbus_framefmt mf;
> > +	int src_pad_idx= -1;
> >  	int ret;
> >  
> >  	sd->grp_id = soc_camera_grp_id(icd);
> > @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  	}
> >  
> >  	/* At this point client .probe() should have run already */
> > -	ret = soc_camera_init_user_formats(icd);
> > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
> 
> This is the test, that I meant above.
> 
> > +		ret = soc_camera_init_user_formats(icd);
> > +	else {
> > +		ret = media_entity_init(&icd->vdev->entity, 1,
> > +					&icd->pad, 0);
> 
> Ok, maybe this hard-coded 1 pad with no extras is justified here, but 
> let's here what others say.
> 
> > +		if (!ret) {
> > +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads; src_pad_idx++)
> > +				if (sd->entity.pads[src_pad_idx].flags == MEDIA_PAD_FL_SOURCE)
> > +					break;
> > +
> > +			if (src_pad_idx < sd->entity.num_pads) {
> > +				ret = media_entity_create_link(
> > +					&icd->vdev->entity, 0,
> > +					&sd->entity, src_pad_idx,
> > +					MEDIA_LNK_FL_IMMUTABLE |
> > +					MEDIA_LNK_FL_ENABLED);
> 
> Let's try to preserve the style. I normally try to avoid splitting the 
> line after "f(" and adding at least the first function parameter above 
> will not make that line longer, than the ones above. So, let's do that.
> 
> > +			}
> > +		}
> > +
> > +		if (!ret)
> > +			ret = soc_camera_init_user_formats_pad(icd,
> > +							src_pad_idx);
> 
> Probably no need to break the line here either.
> 
> > +	}
> >  	if (ret < 0)
> >  		goto eusrfmt;
> >  
> > @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  		goto evidstart;
> >  
> >  	/* Try to improve our guess of a reasonable window format */
> > -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > -		icd->user_width		= mf.width;
> > -		icd->user_height	= mf.height;
> > -		icd->colorspace		= mf.colorspace;
> > -		icd->field		= mf.field;
> > +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> > +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> > +		&& src_pad_idx != -1) {
> 
> The rest of the file puts operations after the first argument, not before 
> the second one when breaking the line. Let's do that here too.
> 
> Thanks
> Guennadi
> 
> > +		struct v4l2_subdev_format sd_format;
> > +
> > +		sd_format.pad= src_pad_idx;
> > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +
> > +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> > +			icd->user_width		= sd_format.format.width;
> > +			icd->user_height	= sd_format.format.height;
> > +			icd->colorspace		= sd_format.format.colorspace;
> > +			icd->field		= sd_format.format.field;
> > +		}
> > +	} else {
> > +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > +			icd->user_width		= mf.width;
> > +			icd->user_height	= mf.height;
> > +			icd->colorspace		= mf.colorspace;
> > +			icd->field		= mf.field;
> > +		}
> >  	}
> >  	soc_camera_remove_device(icd);
> >  
> > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > index 8e74fb7..8a1ca05 100644
> > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device *icd,
> >  	bool host_1to1;
> >  	int ret;
> >  
> > -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > -					 soc_camera_grp_id(icd), video,
> > -					 s_mbus_fmt, mf);
> > +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > +		struct v4l2_subdev_format sd_format;
> > +		struct media_pad *remote_pad;
> > +
> > +		remote_pad= media_entity_remote_pad(
> > +			&icd->vdev->entity.pads[0]);
> > +		sd_format.pad = remote_pad->index;
> > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +		sd_format.format= *mf;
> > +
> > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> > +			&sd_format);
> > +
> > +		mf->width = sd_format.format.width;
> > +		mf->height = sd_format.format.height;
> > +	} else {
> > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> > +	}
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device *icd,
> >  		tmp_h = min(2 * tmp_h, max_height);
> >  		mf->width = tmp_w;
> >  		mf->height = tmp_h;
> > -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> > +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > +			struct v4l2_subdev_format sd_format;
> > +			struct media_pad *remote_pad;
> > +
> > +			remote_pad= media_entity_remote_pad(
> > +				&icd->vdev->entity.pads[0]);
> > +			sd_format.pad = remote_pad->index;
> > +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +			sd_format.format= *mf;
> > +
> > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +					soc_camera_grp_id(icd),
> > +					pad, set_fmt, NULL,
> > +					&sd_format);
> > +		} else {
> > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> >  					soc_camera_grp_id(icd), video,
> >  					s_mbus_fmt, mf);
> > +		}
> >  		dev_geo(dev, "Camera scaled to %ux%u\n",
> >  			mf->width, mf->height);
> >  		if (ret < 0) {
> > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > index 2f6261f..f0c5238 100644
> > --- a/include/media/soc_camera.h
> > +++ b/include/media/soc_camera.h
> > @@ -42,6 +42,7 @@ struct soc_camera_device {
> >  	unsigned char devnum;		/* Device number per host */
> >  	struct soc_camera_sense *sense;	/* See comment in struct definition */
> >  	struct video_device *vdev;
> > +	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	const struct soc_camera_format_xlate *current_fmt;
> >  	struct soc_camera_format_xlate *user_formats;
> > -- 
> > 1.7.10.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Guennadi,

On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> On a second thought:
> 
> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> > Hi Wills,
> > 
> > Thanks for the patch. First and foremost, the title of the patch is wrong.
> > This patch does more than just adding some "adv7604 compatibility." It's
> > adding pad-level API to soc-camera.
> > 
> > This is just a rough review. I'm not an expert in media-controller /
> > pad-level API, I hope someone with a better knowledge of those areas will
> > help me reviewing this.
> > 
> > Another general comment: it has been discussed since a long time, whether
> > a wrapper wouldn't be desired to enable a seamless use of both subdev
> > drivers using and not using the pad-level API. Maybe it's the right time
> > now?..
> 
> This would be a considerable change and would most probably take a rather
> long time, given how busy everyone is.

If I understood correctly Hans Verkuil told me over the weekend that he wanted 
to address this problem in the near future. Hans, could you detail your plans 
?

> I personally would be fine with a (yet another) intermittent solution,
> whereby we create a soc_camera_subdev.c file, in which we collect all those
> function to call either a video or a pad subdev operation, depending on
> whether the latter is available. E.g.
> 
> int soc_camera_sd_enum_mbus_fmt(sd, code)
> {
> 	int ret;
> 
> 	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, code);
> 	} else {
> 		u32 pixcode;
> 
> 		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, code->index, 
&pixcode);
> 		if (!ret)
> 			code->code= pixcode;
> 	}
> 
> 	return ret;
> }
> 
> Similarly for other ops.
> 
> Thanks
> Guennadi
> 
> > On Thu, 29 Jan 2015, William Towle wrote:
> > > Add 'struct media_pad pad' member and suitable glue code, so that
> > > soc_camera/rcar_vin can become agnostic to whether an old or new-
> > > style driver (wrt pad API use) can sit underneath
> > > 
> > > This version has been reworked to include appropriate constant and
> > > datatype names for kernel v3.18
> > > ---
> > > 
> > >  drivers/media/platform/soc_camera/soc_camera.c     |  148
> > >  +++++++++++++++++++-
> > >  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
> > >  include/media/soc_camera.h                         |    1 +
> > >  3 files changed, 182 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > > b/drivers/media/platform/soc_camera/soc_camera.c index f4be2a1..efc20bf
> > > 100644
> > > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > > @@ -37,8 +37,11 @@
> > > 
> > >  #include <media/v4l2-ioctl.h>
> > >  #include <media/v4l2-dev.h>
> > >  #include <media/v4l2-of.h>
> > > 
> > > +#if 0
> > > 
> > >  #include <media/videobuf-core.h>
> > >  #include <media/videobuf2-core.h>
> > > 
> > > +#endif
> > 
> > No. These headers are needed even if the code can be compiled without
> > them.
> > 
> > > +#include <media/v4l2-mediabus.h>
> > 
> > Well, maybe. This header is included indirectly via soc_mediabus.h, but
> > yes, as I just said above, headers, whose defines, structs etc. are used,
> > should be encluded directly. Further, you'll need more headers, e.g.
> > media-entity.h, maybe some more.
> > 
> > >  /* Default to VGA resolution */
> > >  #define DEFAULT_WIDTH	640
> > > 
> > > @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file,
> > > void *priv,> > 
> > >  		return vb2_expbuf(&icd->vb2_vidq, p);
> > >  
> > >  }
> > > 
> > > +static int soc_camera_init_user_formats_pad(struct soc_camera_device
> > > *icd, int src_pad_idx) +{
> > > +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > +	struct v4l2_subdev_mbus_code_enum code;
> > > +	int fmts= 0, raw_fmts, i, ret;
> > 
> > Please, run this patch through checkpatch.pl. It will tell you to add a
> > Signed-off-by line, (hopefully) to add spaces before "=" in multiple
> > places, to place braces correctly, to not use C++-style comments etc. Only
> > feel free to ignore 80-character warnings.
> > 
> > > +
> > > +	code.pad= src_pad_idx;
> > > +	code.index= 0;
> > > +
> > > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> > 
> > This function is called only once below and only after the above test has
> > already returned success. Looks like you don't need it here again and the
> > below "else" branch can be dropped completely?
> > 
> > > +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> > > +			code.index++;
> > > +	} else {
> > > +		u32 pixcode;
> > > +
> > > +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index,
> > > &pixcode)) +		{
> > > +			code.code= pixcode;
> > > +			code.index++;
> > > +		}
> > > +	}
> > > +	raw_fmts= code.index;
> > > +
> > > +	if (!ici->ops->get_formats) {
> > > +		/*
> > > +		 * Fallback mode - the host will have to serve all
> > > +		 * sensor-provided formats one-to-one to the user
> > > +		 */
> > > +		fmts = raw_fmts;
> > > +	}
> > > +	else {
> > > +		/*
> > > +		 * First pass - only count formats this host-sensor
> > > +		 * configuration can provide
> > > +		 */
> > > +		for (i = 0; i < raw_fmts; i++) {
> > > +			int ret = ici->ops->get_formats(icd, i, NULL);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +			fmts += ret;
> > > +		}
> > > +	}
> > > +
> > > +	if (!fmts)
> > > +		return -ENXIO;
> > > +
> > > +	icd->user_formats =
> > > +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> > > +	if (!icd->user_formats)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> > > +
> > > +	/* Second pass - actually fill data formats */
> > > +	fmts = 0;
> > > +	for (i = 0; i < raw_fmts; i++) {
> > > +		if (!ici->ops->get_formats) {
> > > +			code.index= i;
> > > +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> > 
> > Same test again?? Or am I missing something? If indeed these tests are
> > redundant, after you remove them this function will become very similar to
> > the original soc_camera_init_user_formats(), so, maybe some code reuse
> > will become possible.
> > 
> > > +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> > > +			} else {
> > > +				u32 pixcode;
> > > +
> > > +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, 
&pixcode);
> > > +				code.code= pixcode;
> > > +			}
> > > +			icd->user_formats[fmts].host_fmt =
> > > +				soc_mbus_get_fmtdesc(code.code);
> > > +			if (icd->user_formats[fmts].host_fmt)
> > > +				icd->user_formats[fmts++].code = code.code;
> > > +		} else {
> > > +			ret = ici->ops->get_formats(icd, i,
> > > +						    &icd->user_formats[fmts]);
> > > +			if (ret < 0)
> > > +				goto egfmt;
> > > +			fmts += ret;
> > > +		}
> > > +	}
> > > +
> > > +	icd->num_user_formats = fmts;
> > > +	icd->current_fmt = &icd->user_formats[0];
> > > +
> > > +	return 0;
> > > +
> > > +egfmt:
> > > +	vfree(icd->user_formats);
> > > +	return ret;
> > > +}
> > > +
> > > 
> > >  /* Always entered with .host_lock held */
> > >  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> > >  {
> > > 
> > > @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  {
> > >  
> > >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > >  	struct v4l2_mbus_framefmt mf;
> > > 
> > > +	int src_pad_idx= -1;
> > > 
> > >  	int ret;
> > >  	
> > >  	sd->grp_id = soc_camera_grp_id(icd);
> > > 
> > > @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  	}
> > >  	
> > >  	/* At this point client .probe() should have run already */
> > > 
> > > -	ret = soc_camera_init_user_formats(icd);
> > > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
> > 
> > This is the test, that I meant above.
> > 
> > > +		ret = soc_camera_init_user_formats(icd);
> > > +	else {
> > > +		ret = media_entity_init(&icd->vdev->entity, 1,
> > > +					&icd->pad, 0);
> > 
> > Ok, maybe this hard-coded 1 pad with no extras is justified here, but
> > let's here what others say.
> > 
> > > +		if (!ret) {
> > > +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads;
> > > src_pad_idx++)
> > > +				if (sd->entity.pads[src_pad_idx].flags == 
MEDIA_PAD_FL_SOURCE)
> > > +					break;
> > > +
> > > +			if (src_pad_idx < sd->entity.num_pads) {
> > > +				ret = media_entity_create_link(
> > > +					&icd->vdev->entity, 0,
> > > +					&sd->entity, src_pad_idx,
> > > +					MEDIA_LNK_FL_IMMUTABLE |
> > > +					MEDIA_LNK_FL_ENABLED);
> > 
> > Let's try to preserve the style. I normally try to avoid splitting the
> > line after "f(" and adding at least the first function parameter above
> > will not make that line longer, than the ones above. So, let's do that.
> > 
> > > +			}
> > > +		}
> > > +
> > > +		if (!ret)
> > > +			ret = soc_camera_init_user_formats_pad(icd,
> > > +							src_pad_idx);
> > 
> > Probably no need to break the line here either.
> > 
> > > +	}
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		goto eusrfmt;
> > > 
> > > @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  		goto evidstart;
> > >  	
> > >  	/* Try to improve our guess of a reasonable window format */
> > > 
> > > -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > -		icd->user_width		= mf.width;
> > > -		icd->user_height	= mf.height;
> > > -		icd->colorspace		= mf.colorspace;
> > > -		icd->field		= mf.field;
> > > +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> > > +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> > > +		&& src_pad_idx != -1) {
> > 
> > The rest of the file puts operations after the first argument, not before
> > the second one when breaking the line. Let's do that here too.
> > 
> > Thanks
> > Guennadi
> > 
> > > +		struct v4l2_subdev_format sd_format;
> > > +
> > > +		sd_format.pad= src_pad_idx;
> > > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +
> > > +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> > > +			icd->user_width		= sd_format.format.width;
> > > +			icd->user_height	= sd_format.format.height;
> > > +			icd->colorspace		= sd_format.format.colorspace;
> > > +			icd->field		= sd_format.format.field;
> > > +		}
> > > +	} else {
> > > +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > +			icd->user_width		= mf.width;
> > > +			icd->user_height	= mf.height;
> > > +			icd->colorspace		= mf.colorspace;
> > > +			icd->field		= mf.field;
> > > +		}
> > > 
> > >  	}
> > >  	soc_camera_remove_device(icd);
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> > > 8e74fb7..8a1ca05 100644
> > > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > > 
> > >  	bool host_1to1;
> > >  	int ret;
> > > 
> > > -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > -					 soc_camera_grp_id(icd), video,
> > > -					 s_mbus_fmt, mf);
> > > +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > +		struct v4l2_subdev_format sd_format;
> > > +		struct media_pad *remote_pad;
> > > +
> > > +		remote_pad= media_entity_remote_pad(
> > > +			&icd->vdev->entity.pads[0]);
> > > +		sd_format.pad = remote_pad->index;
> > > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +		sd_format.format= *mf;
> > > +
> > > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> > > +			&sd_format);
> > > +
> > > +		mf->width = sd_format.format.width;
> > > +		mf->height = sd_format.format.height;
> > > +	} else {
> > > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> > > +	}
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > > 
> > >  		tmp_h = min(2 * tmp_h, max_height);
> > >  		mf->width = tmp_w;
> > >  		mf->height = tmp_h;
> > > 
> > > -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> > > +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > +			struct v4l2_subdev_format sd_format;
> > > +			struct media_pad *remote_pad;
> > > +
> > > +			remote_pad= media_entity_remote_pad(
> > > +				&icd->vdev->entity.pads[0]);
> > > +			sd_format.pad = remote_pad->index;
> > > +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +			sd_format.format= *mf;
> > > +
> > > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +					soc_camera_grp_id(icd),
> > > +					pad, set_fmt, NULL,
> > > +					&sd_format);
> > > +		} else {
> > > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > 
> > >  					soc_camera_grp_id(icd), video,
> > >  					s_mbus_fmt, mf);
> > > 
> > > +		}
> > > 
> > >  		dev_geo(dev, "Camera scaled to %ux%u\n",
> > >  		
> > >  			mf->width, mf->height);
> > >  		
> > >  		if (ret < 0) {
> > > 
> > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > > index 2f6261f..f0c5238 100644
> > > --- a/include/media/soc_camera.h
> > > +++ b/include/media/soc_camera.h
> > > @@ -42,6 +42,7 @@ struct soc_camera_device {
> > > 
> > >  	unsigned char devnum;		/* Device number per host */
> > >  	struct soc_camera_sense *sense;	/* See comment in struct definition 
*/
> > >  	struct video_device *vdev;
> > > 
> > > +	struct media_pad pad;
> > > 
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >  	const struct soc_camera_format_xlate *current_fmt;
> > >  	struct soc_camera_format_xlate *user_formats;
  

On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> Hi Guennadi,
> 
> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>> On a second thought:
>>
>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>> Hi Wills,
>>>
>>> Thanks for the patch. First and foremost, the title of the patch is wrong.
>>> This patch does more than just adding some "adv7604 compatibility." It's
>>> adding pad-level API to soc-camera.
>>>
>>> This is just a rough review. I'm not an expert in media-controller /
>>> pad-level API, I hope someone with a better knowledge of those areas will
>>> help me reviewing this.
>>>
>>> Another general comment: it has been discussed since a long time, whether
>>> a wrapper wouldn't be desired to enable a seamless use of both subdev
>>> drivers using and not using the pad-level API. Maybe it's the right time
>>> now?..
>>
>> This would be a considerable change and would most probably take a rather
>> long time, given how busy everyone is.
> 
> If I understood correctly Hans Verkuil told me over the weekend that he wanted 
> to address this problem in the near future. Hans, could you detail your plans 
> ?

That's correct. This patch series makes all the necessary changes.

https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html

Patches 1-4 have been merged already, but I need to do more testing for the
remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
can't get it to work with the current kernel.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Hans,

On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> > On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> >> On a second thought:
> >> 
> >> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> >>> Hi Wills,
> >>> 
> >>> Thanks for the patch. First and foremost, the title of the patch is
> >>> wrong. This patch does more than just adding some "adv7604
> >>> compatibility." It's adding pad-level API to soc-camera.
> >>> 
> >>> This is just a rough review. I'm not an expert in media-controller /
> >>> pad-level API, I hope someone with a better knowledge of those areas
> >>> will help me reviewing this.
> >>> 
> >>> Another general comment: it has been discussed since a long time,
> >>> whether a wrapper wouldn't be desired to enable a seamless use of both
> >>> subdev drivers using and not using the pad-level API. Maybe it's the
> >>> right time now?..
> >> 
> >> This would be a considerable change and would most probably take a rather
> >> long time, given how busy everyone is.
> > 
> > If I understood correctly Hans Verkuil told me over the weekend that he
> > wanted to address this problem in the near future. Hans, could you detail
> > your plans ?
> 
> That's correct. This patch series makes all the necessary changes.
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
> 
> Patches 1-4 have been merged already, but I need to do more testing for the
> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
> can't get it to work with the current kernel.

I can't help you much with that, but I could test changes using the rcar-vin 
driver with the adv7180 if needed (does the adv7180 generate an image if no 
analog source is connected ?). That will need to wait for two weeks though as 
I don't have access to the hardware right now.
  

On 02/03/15 16:22, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
>> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
>>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>>>> On a second thought:
>>>>
>>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>>>> Hi Wills,
>>>>>
>>>>> Thanks for the patch. First and foremost, the title of the patch is
>>>>> wrong. This patch does more than just adding some "adv7604
>>>>> compatibility." It's adding pad-level API to soc-camera.
>>>>>
>>>>> This is just a rough review. I'm not an expert in media-controller /
>>>>> pad-level API, I hope someone with a better knowledge of those areas
>>>>> will help me reviewing this.
>>>>>
>>>>> Another general comment: it has been discussed since a long time,
>>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
>>>>> subdev drivers using and not using the pad-level API. Maybe it's the
>>>>> right time now?..
>>>>
>>>> This would be a considerable change and would most probably take a rather
>>>> long time, given how busy everyone is.
>>>
>>> If I understood correctly Hans Verkuil told me over the weekend that he
>>> wanted to address this problem in the near future. Hans, could you detail
>>> your plans ?
>>
>> That's correct. This patch series makes all the necessary changes.
>>
>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
>>
>> Patches 1-4 have been merged already, but I need to do more testing for the
>> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
>> can't get it to work with the current kernel.
> 
> I can't help you much with that, but I could test changes using the rcar-vin 
> driver with the adv7180 if needed (does the adv7180 generate an image if no 
> analog source is connected ?). 

I expect so, most SDTV receivers do that.

> That will need to wait for two weeks though as 
> I don't have access to the hardware right now.
> 

It's certainly appreciated (I'll see if I can rebase it), but I am not
worried about that driver. It's soc_camera that is affected the most.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 02/03/2015 04:24 PM, Hans Verkuil wrote:
> On 02/03/15 16:22, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
>>> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
>>>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>>>>> On a second thought:
>>>>>
>>>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>>>>> Hi Wills,
>>>>>>
>>>>>> Thanks for the patch. First and foremost, the title of the patch is
>>>>>> wrong. This patch does more than just adding some "adv7604
>>>>>> compatibility." It's adding pad-level API to soc-camera.
>>>>>>
>>>>>> This is just a rough review. I'm not an expert in media-controller /
>>>>>> pad-level API, I hope someone with a better knowledge of those areas
>>>>>> will help me reviewing this.
>>>>>>
>>>>>> Another general comment: it has been discussed since a long time,
>>>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
>>>>>> subdev drivers using and not using the pad-level API. Maybe it's the
>>>>>> right time now?..
>>>>>
>>>>> This would be a considerable change and would most probably take a rather
>>>>> long time, given how busy everyone is.
>>>>
>>>> If I understood correctly Hans Verkuil told me over the weekend that he
>>>> wanted to address this problem in the near future. Hans, could you detail
>>>> your plans ?
>>>
>>> That's correct. This patch series makes all the necessary changes.
>>>
>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
>>>
>>> Patches 1-4 have been merged already, but I need to do more testing for the
>>> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
>>> can't get it to work with the current kernel.
>>
>> I can't help you much with that, but I could test changes using the rcar-vin
>> driver with the adv7180 if needed (does the adv7180 generate an image if no
>> analog source is connected ?).
>
> I expect so, most SDTV receivers do that.

It has a freerun mode, in which it can output various kinds of patterns, see 
https://patchwork.linuxtv.org/patch/27894/

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Hans,

On Tuesday 03 February 2015 16:24:19 Hans Verkuil wrote:
> On 02/03/15 16:22, Laurent Pinchart wrote:
> > On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
> >> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> >>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> >>>> On a second thought:
> >>>> 
> >>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> >>>>> Hi Wills,
> >>>>> 
> >>>>> Thanks for the patch. First and foremost, the title of the patch is
> >>>>> wrong. This patch does more than just adding some "adv7604
> >>>>> compatibility." It's adding pad-level API to soc-camera.
> >>>>> 
> >>>>> This is just a rough review. I'm not an expert in media-controller /
> >>>>> pad-level API, I hope someone with a better knowledge of those areas
> >>>>> will help me reviewing this.
> >>>>> 
> >>>>> Another general comment: it has been discussed since a long time,
> >>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
> >>>>> subdev drivers using and not using the pad-level API. Maybe it's the
> >>>>> right time now?..
> >>>> 
> >>>> This would be a considerable change and would most probably take a
> >>>> rather long time, given how busy everyone is.
> >>> 
> >>> If I understood correctly Hans Verkuil told me over the weekend that he
> >>> wanted to address this problem in the near future. Hans, could you
> >>> detail your plans ?
> >> 
> >> That's correct. This patch series makes all the necessary changes.
> >> 
> >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
> >> 
> >> Patches 1-4 have been merged already, but I need to do more testing for
> >> the remainder. The Renesas SH7724 board is ideal for that, but
> >> unfortunately I can't get it to work with the current kernel.
> > 
> > I can't help you much with that, but I could test changes using the
> > rcar-vin driver with the adv7180 if needed (does the adv7180 generate an
> > image if no analog source is connected ?).
> 
> I expect so, most SDTV receivers do that.
> 
> > That will need to wait for two weeks though as
> > I don't have access to the hardware right now.
> 
> It's certainly appreciated (I'll see if I can rebase it), but I am not
> worried about that driver. It's soc_camera that is affected the most.

rcar-vin is an soc-camera host driver, isn't it ?
  

On Tuesday 03 February 2015 16:29:57 Lars-Peter Clausen wrote:
> On 02/03/2015 04:24 PM, Hans Verkuil wrote:
> > On 02/03/15 16:22, Laurent Pinchart wrote:

[snip]

> >> I can't help you much with that, but I could test changes using the
> >> rcar-vin driver with the adv7180 if needed (does the adv7180 generate an
> >> image if no analog source is connected ?).
> > 
> > I expect so, most SDTV receivers do that.
> 
> It has a freerun mode, in which it can output various kinds of patterns, see
> https://patchwork.linuxtv.org/patch/27894/

Thank you.