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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
