Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code

Hi Songjun,

Some more comments below. Except for one in the open/release functions
it's all small things.

On 07/29/2016 09:54 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu <redacted>
---

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
  in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
  with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
  'isc_queue_setup' function.
- Invoke isc_config to configure register in
  'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
  the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
  in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
  in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
  initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner  = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
  module_platform_driver().

 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    2 +
 drivers/media/platform/atmel/Kconfig          |    9 +
 drivers/media/platform/atmel/Makefile         |    1 +
 drivers/media/platform/atmel/atmel-isc-regs.h |  165 +++
 drivers/media/platform/atmel/atmel-isc.c      | 1611 +++++++++++++++++++++++++
 6 files changed, 1789 insertions(+)
 create mode 100644 drivers/media/platform/atmel/Kconfig
 create mode 100644 drivers/media/platform/atmel/Makefile
 create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
 create mode 100644 drivers/media/platform/atmel/atmel-isc.c

<snip>

<snip>

+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+		 "Sensor is preferred to output the specified format (1-on 0-off) default 1");

I have no idea what this means. Can you elaborate? Why would you want to set this to 0?

<snip>

+static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
+{
+	if ((sensor_preferred && isc_fmt->sd_support) ||
+	    !isc_fmt->isc_support)

I'd just do:

	return (sensor_preferred && isc_fmt->sd_support) ||
	       !isc_fmt->isc_support;

+		return true;
+	else
+		return false;
+}
+

<snip>

+static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
+			struct isc_format **current_fmt, u32 *code)
+{
+	struct isc_format *isc_fmt;
+	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
+	struct v4l2_subdev_format format = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+	};
+	u32 mbus_code;
+	int ret;
+
+	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	isc_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
+	if (!isc_fmt) {
+		v4l2_warn(&isc->v4l2_dev, "Format 0x%x not found\n",
+			  pixfmt->pixelformat);
+		isc_fmt = isc->user_formats[isc->num_user_formats - 1];
+		pixfmt->pixelformat = isc_fmt->fourcc;
+	}
+
+	/* Limit to Atmel ISC hardware capabilities */
+	if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
+		pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
+	if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
+		pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
+
+	if (sensor_is_preferred(isc_fmt))
+		mbus_code = isc_fmt->sd_mbus_code;
+	else
+		mbus_code = isc_fmt->isc_mbus_code;
+
+	v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
+	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
+			       isc->current_subdev->config, &format);
+	if (ret < 0)
+		return ret;
+
+	v4l2_fill_pix_format(pixfmt, &format.format);
+
+	switch (pixfmt->field) {
+	case V4L2_FIELD_ANY:
+	case V4L2_FIELD_NONE:
+		break;
+	default:
+		pixfmt->field = V4L2_FIELD_NONE;
+	}

Drop the switch and just always set field to FIELD_NONE.

+
+	pixfmt->bytesperline = pixfmt->width * isc_fmt->bpp;
+	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
+
+	if (current_fmt)
+		*current_fmt = isc_fmt;
+
+	if (code)
+		*code = mbus_code;
+
+	return 0;
+}

<snip>

+static int isc_set_default_fmt(struct isc_device *isc)
+{
+	u32 index = isc->num_user_formats - 1;
+	struct v4l2_format f = {
+		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		.fmt.pix = {
+			.width		= VGA_WIDTH,
+			.height		= VGA_HEIGHT,
+			.field		= V4L2_FIELD_ANY,

Use FIELD_NONE instead of ANY.

+			.pixelformat	= isc->user_formats[index]->fourcc,
+		},
+	};
+
+	return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+	struct isc_device *isc = video_drvdata(file);
+	struct v4l2_subdev *sd = isc->current_subdev->sd;
+	int ret;
+
+	if (mutex_lock_interruptible(&isc->lock))
+		return -ERESTARTSYS;
+
+	ret = v4l2_fh_open(file);
+	if (ret < 0)
+		goto unlock;
+
+	ret = v4l2_subdev_call(sd, core, s_power, 1);

Only do this if this is the first open...

+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		goto unlock;
+
+	ret = isc_set_default_fmt(isc);
+	if (ret)
+		goto unlock;
+
+unlock:
+	mutex_unlock(&isc->lock);
+	return ret;
+}
+
+static int isc_release(struct file *file)
+{
+	struct isc_device *isc = video_drvdata(file);
+	struct v4l2_subdev *sd = isc->current_subdev->sd;
+	int ret;
+
+	mutex_lock(&isc->lock);
+
+	ret = _vb2_fop_release(file, NULL);
+
+	v4l2_subdev_call(sd, core, s_power, 0);

...and only do this on the last release.

Otherwise a simple 'v4l2-ctl -D' call would turn off the power when it closes its
filehandle, even if another process is streaming.

See drivers/media/platform/am437x/am437x-vpfe.c for a good example on how to handle
this (search for v4l2_fh_is_singular).

+
+	mutex_unlock(&isc->lock);
+
+	return ret;
+}
+
+static const struct v4l2_file_operations isc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= isc_open,
+	.release	= isc_release,
+	.unlocked_ioctl	= video_ioctl2,
+	.read		= vb2_fop_read,
+	.mmap		= vb2_fop_mmap,
+	.poll		= vb2_fop_poll,
+};
+
+static irqreturn_t isc_interrupt(int irq, void *dev_id)
+{
+	struct isc_device *isc = (struct isc_device *)dev_id;
+	struct regmap *regmap = isc->regmap;
+	u32 isc_intsr, isc_intmask, pending;
+	irqreturn_t ret = IRQ_NONE;
+
+	spin_lock(&isc->dma_queue_lock);
+
+	regmap_read(regmap, ISC_INTSR, &isc_intsr);
+	regmap_read(regmap, ISC_INTMASK, &isc_intmask);
+
+	pending = isc_intsr & isc_intmask;
+
+	if (likely(pending & ISC_INT_DDONE)) {
+		if (isc->cur_frm) {
+			struct vb2_v4l2_buffer *vbuf = &isc->cur_frm->vb;
+			struct vb2_buffer *vb = &vbuf->vb2_buf;
+
+			vb->timestamp = ktime_get_ns();
+			vbuf->sequence = isc->sequence++;
+			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+			isc->cur_frm = NULL;
+		}
+
+		if (!list_empty(&isc->dma_queue) && !isc->stop) {
+			isc->cur_frm = list_first_entry(&isc->dma_queue,
+						     struct isc_buffer, list);
+			list_del(&isc->cur_frm->list);
+
+			isc_start_dma(regmap, isc->cur_frm,
+				      isc->current_fmt->reg_dctrl_dview);
+		}
+
+		if (isc->stop)
+			complete(&isc->comp);
+
+		ret = IRQ_HANDLED;
+	}
+
+	spin_unlock(&isc->dma_queue_lock);
+
+	return ret;
+}
+
+static int isc_async_bound(struct v4l2_async_notifier *notifier,
+			    struct v4l2_subdev *subdev,
+			    struct v4l2_async_subdev *asd)
+{
+	struct isc_device *isc = container_of(notifier->v4l2_dev,
+					      struct isc_device, v4l2_dev);
+	struct isc_subdev_entity *subdev_entity =
+		container_of(notifier, struct isc_subdev_entity, notifier);
+
+	if (video_is_registered(&isc->video_dev)) {
+		v4l2_err(&isc->v4l2_dev, "only supports one sub-device.\n");
+		return -EBUSY;
+	}
+
+	subdev_entity->sd = subdev;
+
+	return 0;
+}
+
+static void isc_async_unbind(struct v4l2_async_notifier *notifier,
+			      struct v4l2_subdev *subdev,
+			      struct v4l2_async_subdev *asd)
+{
+	struct isc_device *isc = container_of(notifier->v4l2_dev,
+					      struct isc_device, v4l2_dev);
+
+	video_unregister_device(&isc->video_dev);
+	if (isc->current_subdev->config)
+		v4l2_subdev_free_pad_config(isc->current_subdev->config);
+}
+
+static struct isc_format *find_format_by_code(unsigned int code, int *index)
+{
+	struct isc_format *fmt = &isc_formats[0];
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
+		if (fmt->sd_mbus_code == code) {
+			*index = i;
+			return fmt;
+		}
+
+		fmt++;
+	}
+
+	return NULL;
+}
+
+static int isc_formats_init(struct isc_device *isc)
+{
+	struct isc_format *fmt;
+	struct v4l2_subdev *subdev = isc->current_subdev->sd;
+	int num_fmts, i, j;
+	struct v4l2_subdev_mbus_code_enum mbus_code = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+
+	fmt = &isc_formats[0];
+	for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
+		fmt->isc_support = false;
+		fmt->sd_support = false;
+
+		fmt++;
+	}
+
+	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
+	       NULL, &mbus_code)) {
+		mbus_code.index++;
+		fmt = find_format_by_code(mbus_code.code, &i);
+		if (!fmt)
+			continue;
+
+		fmt->sd_support = true;
+
+		if (i <= RAW_FMT_INDEX_END) {
+			for (j = ISC_FMT_INDEX_START;
+			     j <= ISC_FMT_INDEX_END; j++) {
+				isc_formats[j].isc_support = true;
+				isc_formats[j].isc_mbus_code = mbus_code.code;
+				isc_formats[j].reg_isc_bps = fmt->reg_sd_bps;
+				isc_formats[j].reg_bay_cfg = fmt->reg_bay_cfg;
+			}
+		}
+	}
+
+	fmt = &isc_formats[0];
+	for (i = 0, num_fmts = 0; i < ARRAY_SIZE(isc_formats); i++) {
+		if (fmt->isc_support || fmt->sd_support)
+			num_fmts++;
+
+		fmt++;
+	}
+
+	if (!num_fmts)
+		return -ENXIO;
+
+	isc->num_user_formats = num_fmts;
+	isc->user_formats = devm_kcalloc(isc->dev,
+					 num_fmts, sizeof(struct isc_format *),
+					 GFP_KERNEL);
+	if (!isc->user_formats) {
+		v4l2_err(&isc->v4l2_dev, "could not allocate memory\n");
+		return -ENOMEM;
+	}
+
+	fmt = &isc_formats[0];
+	for (i = 0, j = 0; i < ARRAY_SIZE(isc_formats); i++) {
+		if (fmt->isc_support || fmt->sd_support)
+			isc->user_formats[j++] = fmt;
+
+		fmt++;
+	}
+
+	return 0;
+}
+
+static int isc_async_complete(struct v4l2_async_notifier *notifier)
+{
+	struct isc_device *isc = container_of(notifier->v4l2_dev,
+					      struct isc_device, v4l2_dev);
+	struct isc_subdev_entity *sd_entity;
+	struct video_device *vdev = &isc->video_dev;
+	struct vb2_queue *q = &isc->vb2_vidq;
+	int ret;
+
+	isc->current_subdev = container_of(notifier,
+					   struct isc_subdev_entity, notifier);
+	sd_entity = isc->current_subdev;
+
+	mutex_init(&isc->lock);
+	init_completion(&isc->comp);
+
+	/* Initialize videobuf2 queue */
+	q->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	q->io_modes		= VB2_MMAP;

Is there any reason VB2_DMABUF and VB2_READ are missing here?

+	q->drv_priv		= isc;
+	q->buf_struct_size	= sizeof(struct isc_buffer);
+	q->ops			= &isc_vb2_ops;
+	q->mem_ops		= &vb2_dma_contig_memops;
+	q->timestamp_flags	= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->lock			= &isc->lock;
+	q->min_buffers_needed	= 1;
+	q->dev			= isc->dev;
+
+	ret = vb2_queue_init(q);
+	if (ret < 0) {
+		v4l2_err(&isc->v4l2_dev,
+			 "vb2_queue_init() failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Init video dma queues */
+	INIT_LIST_HEAD(&isc->dma_queue);
+	spin_lock_init(&isc->dma_queue_lock);
+
+	/* Register video device */
+	strlcpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name));
+	vdev->release		= video_device_release_empty;
+	vdev->fops		= &isc_fops;
+	vdev->ioctl_ops		= &isc_ioctl_ops;
+	vdev->v4l2_dev		= &isc->v4l2_dev;
+	vdev->vfl_dir		= VFL_DIR_RX;
+	vdev->queue		= q;
+	vdev->lock		= &isc->lock;
+	vdev->ctrl_handler	= isc->current_subdev->sd->ctrl_handler;
+	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
+	video_set_drvdata(vdev, isc);
+
+	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
+	if (ret < 0) {
+		v4l2_err(&isc->v4l2_dev,
+			 "video_register_device failed: %d\n", ret);
+		return ret;
+	}
+
+	sd_entity->config = v4l2_subdev_alloc_pad_config(sd_entity->sd);
+	if (sd_entity->config == NULL)
+		return -ENOMEM;
+
+	ret = isc_formats_init(isc);
+	if (ret < 0) {
+		v4l2_err(&isc->v4l2_dev,
+			 "Init format failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}

Regards,

	Hans