Re: [PATCH v5 27/35] omap3isp: Introduce isp_video_check_external_subdevs()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thanks for the patch.

On Tuesday 06 March 2012 18:33:08 Sakari Ailus wrote:
> isp_video_check_external_subdevs() will retrieve external subdev's
> bits-per-pixel and pixel rate for the use of other ISP subdevs at streamon
> time. isp_video_check_external_subdevs() is called after pipeline
> validation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/omap3isp/ispvideo.c |   75 ++++++++++++++++++++++++++++
>  1 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index 4bc9cca..ef5c770 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -934,6 +934,77 @@ isp_video_dqbuf(struct file *file, void *fh, struct
> v4l2_buffer *b) file->f_flags & O_NONBLOCK);
>  }
> 
> +static int isp_video_check_external_subdevs(struct isp_pipeline *pipe)
> +{
> +	struct isp_device *isp =
> +		container_of(pipe, struct isp_video, pipe)->isp;

Any reason not to pass isp_device * from the caller to this function ?

> +	struct media_entity *ents[] = {
> +		&isp->isp_csi2a.subdev.entity,
> +		&isp->isp_csi2c.subdev.entity,
> +		&isp->isp_ccp2.subdev.entity,
> +		&isp->isp_ccdc.subdev.entity
> +	};
> +	struct media_pad *source_pad;
> +	struct media_entity *source = NULL;
> +	struct media_entity *sink;
> +	struct v4l2_subdev_format fmt;
> +	struct v4l2_ext_controls ctrls;
> +	struct v4l2_ext_control ctrl;
> +	int i;

i is allowed to be unsigned in this driver as well ;-)

> +	int ret = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(ents); i++) {
> +		/* Is the entity part of the pipeline? */
> +		if (!(pipe->entities & (1 << ents[i]->id)))
> +			continue;
> +
> +		/* ISP entities have always sink pad == 0. Find source. */
> +		source_pad = media_entity_remote_source(&ents[i]->pads[0]);
> +

Don't you usually avoid blank lines between a variable assignment and checking 
it for an error condition ?

> +		if (source_pad == NULL)
> +			continue;
> +
> +		source = source_pad->entity;
> +		sink = ents[i];
> +		break;
> +	}
> +
> +	if (!source || media_entity_type(source) != MEDIA_ENT_T_V4L2_SUBDEV)
> +		return 0;
> +
> +	pipe->external = media_entity_to_v4l2_subdev(source);
> +
> +	fmt.pad = source_pad->index;
> +	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(sink),
> +			       pad, get_fmt, NULL, &fmt);
> +	BUG_ON(ret < 0);

That's a bit harsh. I'd rather return an error.

> +
> +	pipe->external_bpp = omap3isp_video_format_info(
> +		fmt.format.code)->bpp;

Maybe you could teachs emacs that 78 characters fit in a 80-columns line ? :-)

> +
> +	memset(&ctrls, 0, sizeof(ctrls));
> +	memset(&ctrl, 0, sizeof(ctrl));
> +
> +	ctrl.id = V4L2_CID_PIXEL_RATE;
> +
> +	ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);

You can leave ctrl_class to 0.

> +	ctrls.count = 1;
> +	ctrls.controls = &ctrl;
> +
> +	ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &ctrls);
> +	if (ret < 0) {
> +		dev_warn(isp->dev,
> +			 "no pixel rate control in subdev %s\n",

No need to split this either.

> +			 pipe->external->name);
> +		return ret;
> +	}
> +
> +	pipe->external_rate = ctrl.value64;
> +
> +	return 0;
> +}
> +
>  /*
>   * Stream management
>   *
> @@ -1010,6 +1081,10 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) while ((entity = media_entity_graph_walk_next(&graph)))
>  		pipe->entities |= 1 << entity->id;
> 
> +	ret = isp_video_check_external_subdevs(pipe);
> +	if (ret < 0)
> +		goto err_check_format;
> +
>  	/* Verify that the currently configured format matches the output of
>  	 * the connected subdev.
>  	 */
-- 
Regards,

Laurent Pinchart