From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:54264 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754831AbbBBKAt (ORCPT ); Mon, 2 Feb 2015 05:00:49 -0500 From: Laurent Pinchart To: Guennadi Liakhovetski Cc: William Towle , linux-kernel@lists.codethink.co.uk, Linux Media Mailing List , Sergei Shtylyov , Hans Verkuil Subject: Re: [PATCH 6/8] WmT: adv7604 driver compatibility Date: Mon, 02 Feb 2015 12:01:05 +0200 Message-ID: <2552213.h99FiuUI04@avalon> In-Reply-To: References: <1422548388-28861-1-git-send-email-william.towle@codethink.co.uk> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: 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 > > > #include > > > #include > > > > > > +#if 0 > > > > > > #include > > > #include > > > > > > +#endif > > > > No. These headers are needed even if the code can be compiled without > > them. > > > > > +#include > > > > 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; -- Regards, Laurent Pinchart