Re: [PATCHv5 07/16] atmel-isi: remove dependency of the soc-camera framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/12/2017 05:44 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> Thanks for the patch. Why hasn't this patch been CCed to Josh Wu? Is he 
> still at Atmel? Adding to CC to check.

To the best of my knowledge Josh no longer works for Atmel/Microchip, and Songjun
took over.

> 
> A general comment: this patch doesn't only remove soc-camera dependencies, 
> it also changes the code and the behaviour. I would prefer to break that 
> down in multiple patches, or remove this driver completely and add a new 
> one. I'll provide some comments, but of course I cannot make an extensive 
> review of a 1200-line of change patch without knowing the hardware and the 
> set ups well enough.
> 
> On Sat, 11 Mar 2017, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> This patch converts the atmel-isi driver from a soc-camera driver to a driver
>> that is stand-alone.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>>  drivers/media/platform/soc_camera/Kconfig     |    3 +-
>>  drivers/media/platform/soc_camera/atmel-isi.c | 1209 +++++++++++++++----------
>>  2 files changed, 714 insertions(+), 498 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/Kconfig b/drivers/media/platform/soc_camera/Kconfig
>> index 86d74788544f..a37ec91b026e 100644
>> --- a/drivers/media/platform/soc_camera/Kconfig
>> +++ b/drivers/media/platform/soc_camera/Kconfig
>> @@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
>>  
>>  config VIDEO_ATMEL_ISI
>>  	tristate "ATMEL Image Sensor Interface (ISI) support"
>> -	depends on VIDEO_DEV && SOC_CAMERA
>> +	depends on VIDEO_V4L2 && OF && HAS_DMA
>>  	depends on ARCH_AT91 || COMPILE_TEST
>> -	depends on HAS_DMA
>>  	select VIDEOBUF2_DMA_CONTIG
>>  	---help---
>>  	  This module makes the ATMEL Image Sensor Interface available
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 46de657c3e6d..a6d60c2e207d 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -22,18 +22,22 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/slab.h>
>> -
>> -#include <media/soc_camera.h>
>> -#include <media/drv-intf/soc_mediabus.h>
>> +#include <linux/of.h>
>> +
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-dev.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-event.h>
>>  #include <media/v4l2-of.h>
>>  #include <media/videobuf2-dma-contig.h>
>> +#include <media/v4l2-image-sizes.h>
>>  
>>  #include "atmel-isi.h"
>>  
>> -#define MAX_BUFFER_NUM			32
>>  #define MAX_SUPPORT_WIDTH		2048
>>  #define MAX_SUPPORT_HEIGHT		2048
>> -#define VID_LIMIT_BYTES			(16 * 1024 * 1024)
>>  #define MIN_FRAME_RATE			15
>>  #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)
>>  
>> @@ -65,9 +69,37 @@ struct frame_buffer {
>>  	struct list_head list;
>>  };
>>  
>> +struct isi_graph_entity {
>> +	struct device_node *node;
>> +
>> +	struct v4l2_async_subdev asd;
>> +	struct v4l2_subdev *subdev;
>> +};
>> +
>> +/*
>> + * struct isi_format - ISI media bus format information
>> + * @fourcc:		Fourcc code for this format
>> + * @mbus_code:		V4L2 media bus format code.
>> + * @bpp:		Bytes per pixel (when stored in memory)
>> + * @swap:		Byte swap configuration value
>> + * @support:		Indicates format supported by subdev
>> + * @skip:		Skip duplicate format supported by subdev
>> + */
>> +struct isi_format {
>> +	u32	fourcc;
>> +	u32	mbus_code;
>> +	u8	bpp;
>> +	u32	swap;
>> +
>> +	bool	support;
>> +	bool	skip;
>> +};
>> +
>> +
>>  struct atmel_isi {
>>  	/* Protects the access of variables shared with the ISR */
>> -	spinlock_t			lock;
>> +	spinlock_t			irqlock;
>> +	struct device			*dev;
>>  	void __iomem			*regs;
>>  
>>  	int				sequence;
>> @@ -76,7 +108,7 @@ struct atmel_isi {
>>  	struct fbd			*p_fb_descriptors;
>>  	dma_addr_t			fb_descriptors_phys;
>>  	struct				list_head dma_desc_head;
>> -	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
>> +	struct isi_dma_desc		dma_desc[VIDEO_MAX_FRAME];
>>  	bool				enable_preview_path;
>>  
>>  	struct completion		complete;
>> @@ -90,9 +122,22 @@ struct atmel_isi {
>>  	struct list_head		video_buffer_list;
>>  	struct frame_buffer		*active;
>>  
>> -	struct soc_camera_host		soc_host;
>> +	struct v4l2_device		v4l2_dev;
>> +	struct video_device		*vdev;
>> +	struct v4l2_async_notifier	notifier;
>> +	struct isi_graph_entity		entity;
>> +	struct v4l2_format		fmt;
>> +
>> +	struct isi_format		**user_formats;
>> +	unsigned int			num_user_formats;
>> +	const struct isi_format		*current_fmt;
>> +
>> +	struct mutex			lock;
>> +	struct vb2_queue		queue;
>>  };
>>  
>> +#define notifier_to_isi(n) container_of(n, struct atmel_isi, notifier)
>> +
>>  static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
>>  {
>>  	writel(val, isi->regs + reg);
>> @@ -102,107 +147,46 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>  	return readl(isi->regs + reg);
>>  }
>>  
>> -static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>> -		const struct soc_camera_format_xlate *xlate)
>> -{
>> -	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
>> -		/* all convert to YUYV */
>> -		switch (xlate->code) {
>> -		case MEDIA_BUS_FMT_VYUY8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_3;
>> -		case MEDIA_BUS_FMT_UYVY8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_2;
>> -		case MEDIA_BUS_FMT_YVYU8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_1;
>> -		}
>> -	} else if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_RGB565) {
>> -		/*
>> -		 * Preview path is enabled, it will convert UYVY to RGB format.
>> -		 * But if sensor output format is not UYVY, we need to set
>> -		 * YCC_SWAP_MODE to convert it as UYVY.
>> -		 */
>> -		switch (xlate->code) {
>> -		case MEDIA_BUS_FMT_VYUY8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_1;
>> -		case MEDIA_BUS_FMT_YUYV8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_2;
>> -		case MEDIA_BUS_FMT_YVYU8_2X8:
>> -			return ISI_CFG2_YCC_SWAP_MODE_3;
>> -		}
>> -	}
>> -
>> -	/*
>> -	 * By default, no swap for the codec path of Atmel ISI. So codec
>> -	 * output is same as sensor's output.
>> -	 * For instance, if sensor's output is YUYV, then codec outputs YUYV.
>> -	 * And if sensor's output is UYVY, then codec outputs UYVY.
>> -	 */
>> -	return ISI_CFG2_YCC_SWAP_DEFAULT;
>> -}
>> +static struct isi_format isi_formats[] = {
> 
> This isn't a const array, you're modifying it during initialisation. Are 
> we sure there aren't going to be any SoCs with more than one ISI?

When that happens, this should be changed at that time. I think it is unlikely
since as I understand it ISI has been replaced by ISC (atmel-isc).

> 
>> +	{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8,
>> +	  2, ISI_CFG2_YCC_SWAP_DEFAULT, false },
> 
> This struct has 6 elements and is defined almost a 100 lines above this 
> initialisation. I'd find using explicit field names easier to read, 

Moved down and use field names.

> especially since you only initialise 5 out of 6 fields explicitly. You 
> also explicitly set the fifth field everywhere to false and leave the 
> sixth field implicitly to false.
> 
>> +	{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YVYU8_2X8,
>> +	  2, ISI_CFG2_YCC_SWAP_MODE_1, false },
>> +	{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_UYVY8_2X8,
>> +	  2, ISI_CFG2_YCC_SWAP_MODE_2, false },
>> +	{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_VYUY8_2X8,
>> +	  2, ISI_CFG2_YCC_SWAP_MODE_3, false },
>> +	{ V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_YUYV8_2X8,
>> +	  2, ISI_CFG2_YCC_SWAP_MODE_2, false },
> 
> Why are you dropping other conversions to RGB565? Were they wrong?

There was a reason, but I have forgotten it :-(

I'll have to retest the other three YUYV combinations.

> 
>> +};
>>  
>> -static void configure_geometry(struct atmel_isi *isi, u32 width,
>> -		u32 height, const struct soc_camera_format_xlate *xlate)
>> +static void configure_geometry(struct atmel_isi *isi)
>>  {
>> -	u32 cfg2, psize;
>> -	u32 fourcc = xlate->host_fmt->fourcc;
>> +	u32 cfg2 = 0, psize;
> 
> Superfluous initialisation of cfg2.

Fixed.

> 
>> +	u32 fourcc = isi->current_fmt->fourcc;
>>  
>>  	isi->enable_preview_path = fourcc == V4L2_PIX_FMT_RGB565 ||
>>  				   fourcc == V4L2_PIX_FMT_RGB32;
>>  
>>  	/* According to sensor's output format to set cfg2 */
>> -	switch (xlate->code) {
>> -	default:
>> -	/* Grey */
>> -	case MEDIA_BUS_FMT_Y8_1X8:
>> -		cfg2 = ISI_CFG2_GRAYSCALE | ISI_CFG2_COL_SPACE_YCbCr;
>> -		break;
>> -	/* YUV */
>> -	case MEDIA_BUS_FMT_VYUY8_2X8:
>> -	case MEDIA_BUS_FMT_UYVY8_2X8:
>> -	case MEDIA_BUS_FMT_YVYU8_2X8:
>> -	case MEDIA_BUS_FMT_YUYV8_2X8:
>> -		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
>> -				setup_cfg2_yuv_swap(isi, xlate);
>> -		break;
>> -	/* RGB, TODO */
>> -	}
>> +	cfg2 = isi->current_fmt->swap;
>>  
>>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>  	/* Set width */
>> -	cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>> +	cfg2 |= ((isi->fmt.fmt.pix.width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>>  			ISI_CFG2_IM_HSIZE_MASK;
>>  	/* Set height */
>> -	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>> +	cfg2 |= ((isi->fmt.fmt.pix.height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>>  			& ISI_CFG2_IM_VSIZE_MASK;
>>  	isi_writel(isi, ISI_CFG2, cfg2);
>>  
>>  	/* No down sampling, preview size equal to sensor output size */
>> -	psize = ((width - 1) << ISI_PSIZE_PREV_HSIZE_OFFSET) &
>> +	psize = ((isi->fmt.fmt.pix.width - 1) << ISI_PSIZE_PREV_HSIZE_OFFSET) &
>>  		ISI_PSIZE_PREV_HSIZE_MASK;
>> -	psize |= ((height - 1) << ISI_PSIZE_PREV_VSIZE_OFFSET) &
>> +	psize |= ((isi->fmt.fmt.pix.height - 1) << ISI_PSIZE_PREV_VSIZE_OFFSET) &
>>  		ISI_PSIZE_PREV_VSIZE_MASK;
>>  	isi_writel(isi, ISI_PSIZE, psize);
>>  	isi_writel(isi, ISI_PDECF, ISI_PDECF_NO_SAMPLING);
>> -
>> -	return;
>> -}
>> -
>> -static bool is_supported(struct soc_camera_device *icd,
>> -		const u32 pixformat)
>> -{
>> -	switch (pixformat) {
>> -	/* YUV, including grey */
>> -	case V4L2_PIX_FMT_GREY:
>> -	case V4L2_PIX_FMT_YUYV:
>> -	case V4L2_PIX_FMT_UYVY:
>> -	case V4L2_PIX_FMT_YVYU:
>> -	case V4L2_PIX_FMT_VYUY:
>> -	/* RGB */
>> -	case V4L2_PIX_FMT_RGB565:
>> -		return true;
>> -	default:
>> -		return false;
>> -	}
>>  }
>>  
>>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -214,6 +198,7 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>>  		list_del_init(&buf->list);
>>  		vbuf->vb2_buf.timestamp = ktime_get_ns();
>>  		vbuf->sequence = isi->sequence++;
>> +		vbuf->field = V4L2_FIELD_NONE;
>>  		vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_DONE);
>>  	}
>>  
>> @@ -247,7 +232,7 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
>>  	u32 status, mask, pending;
>>  	irqreturn_t ret = IRQ_NONE;
>>  
>> -	spin_lock(&isi->lock);
>> +	spin_lock(&isi->irqlock);
>>  
>>  	status = isi_readl(isi, ISI_STATUS);
>>  	mask = isi_readl(isi, ISI_INTMASK);
>> @@ -267,7 +252,7 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
>>  			ret = atmel_isi_handle_streaming(isi);
>>  	}
>>  
>> -	spin_unlock(&isi->lock);
>> +	spin_unlock(&isi->irqlock);
>>  	return ret;
>>  }
>>  
>> @@ -305,26 +290,21 @@ static int queue_setup(struct vb2_queue *vq,
>>  				unsigned int *nbuffers, unsigned int *nplanes,
>>  				unsigned int sizes[], struct device *alloc_devs[])
>>  {
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vq);
>>  	unsigned long size;
>>  
>> -	size = icd->sizeimage;
>> +	size = isi->fmt.fmt.pix.sizeimage;
>>  
>> -	if (!*nbuffers || *nbuffers > MAX_BUFFER_NUM)
>> -		*nbuffers = MAX_BUFFER_NUM;
>> -
>> -	if (size * *nbuffers > VID_LIMIT_BYTES)
>> -		*nbuffers = VID_LIMIT_BYTES / size;
>> +	/* Make sure the image size is large enough. */
>> +	if (*nplanes)
>> +		return sizes[0] < size ? -EINVAL : 0;
>>  
>>  	*nplanes = 1;
>>  	sizes[0] = size;
>>  
>> -	isi->sequence = 0;
>>  	isi->active = NULL;
>>  
>> -	dev_dbg(icd->parent, "%s, count=%d, size=%ld\n", __func__,
>> +	dev_dbg(isi->dev, "%s, count=%d, size=%ld\n", __func__,
>>  		*nbuffers, size);
>>  
>>  	return 0;
>> @@ -344,17 +324,15 @@ static int buffer_init(struct vb2_buffer *vb)
>>  static int buffer_prepare(struct vb2_buffer *vb)
>>  {
>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>>  	struct frame_buffer *buf = container_of(vbuf, struct frame_buffer, vb);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vb->vb2_queue);
>>  	unsigned long size;
>>  	struct isi_dma_desc *desc;
>>  
>> -	size = icd->sizeimage;
>> +	size = isi->fmt.fmt.pix.sizeimage;
>>  
>>  	if (vb2_plane_size(vb, 0) < size) {
>> -		dev_err(icd->parent, "%s data will not fit into plane (%lu < %lu)\n",
>> +		dev_err(isi->dev, "%s data will not fit into plane (%lu < %lu)\n",
>>  				__func__, vb2_plane_size(vb, 0), size);
>>  		return -EINVAL;
>>  	}
>> @@ -363,7 +341,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
>>  
>>  	if (!buf->p_dma_desc) {
>>  		if (list_empty(&isi->dma_desc_head)) {
>> -			dev_err(icd->parent, "Not enough dma descriptors.\n");
>> +			dev_err(isi->dev, "Not enough dma descriptors.\n");
>>  			return -EINVAL;
>>  		} else {
>>  			/* Get an available descriptor */
>> @@ -387,9 +365,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
>>  static void buffer_cleanup(struct vb2_buffer *vb)
>>  {
>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vb->vb2_queue);
>>  	struct frame_buffer *buf = container_of(vbuf, struct frame_buffer, vb);
>>  
>>  	/* This descriptor is available now and we add to head list */
>> @@ -409,7 +385,7 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
>>  	/* Check if already in a frame */
>>  	if (!isi->enable_preview_path) {
>>  		if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
>> -			dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
>> +			dev_err(isi->dev, "Already in frame handling.\n");
>>  			return;
>>  		}
>>  
>> @@ -443,13 +419,11 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
>>  static void buffer_queue(struct vb2_buffer *vb)
>>  {
>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vb->vb2_queue);
>>  	struct frame_buffer *buf = container_of(vbuf, struct frame_buffer, vb);
>>  	unsigned long flags = 0;
>>  
>> -	spin_lock_irqsave(&isi->lock, flags);
>> +	spin_lock_irqsave(&isi->irqlock, flags);
>>  	list_add_tail(&buf->list, &isi->video_buffer_list);
>>  
>>  	if (isi->active == NULL) {
>> @@ -457,60 +431,83 @@ static void buffer_queue(struct vb2_buffer *vb)
>>  		if (vb2_is_streaming(vb->vb2_queue))
>>  			start_dma(isi, buf);
>>  	}
>> -	spin_unlock_irqrestore(&isi->lock, flags);
>> +	spin_unlock_irqrestore(&isi->irqlock, flags);
>>  }
>>  
>>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>  {
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vq);
>> +	struct frame_buffer *buf, *node;
>>  	int ret;
>>  
>> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
>> +	/* Enable stream on the sub device */
>> +	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>> +	if (ret && ret != -ENOIOCTLCMD) {
>> +		dev_err(isi->dev, "stream on failed in subdev\n");
>> +		goto err_start_stream;
>> +	}
>> +
>> +	pm_runtime_get_sync(isi->dev);
>>  
>>  	/* Reset ISI */
>>  	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>>  	if (ret < 0) {
>> -		dev_err(icd->parent, "Reset ISI timed out\n");
>> -		pm_runtime_put(ici->v4l2_dev.dev);
>> -		return ret;
>> +		dev_err(isi->dev, "Reset ISI timed out\n");
>> +		goto err_reset;
>>  	}
>>  	/* Disable all interrupts */
>>  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>  
>> -	configure_geometry(isi, icd->user_width, icd->user_height,
>> -				icd->current_fmt);
>> +	isi->sequence = 0;
>> +	configure_geometry(isi);
>>  
>> -	spin_lock_irq(&isi->lock);
>> +	spin_lock_irq(&isi->irqlock);
>>  	/* Clear any pending interrupt */
>>  	isi_readl(isi, ISI_STATUS);
>>  
>> -	if (count)
>> -		start_dma(isi, isi->active);
>> -	spin_unlock_irq(&isi->lock);
>> +	start_dma(isi, isi->active);
> 
> Isn't this also a change in behaviour? Starting DMA also if no buffers 
> have been queued yet?

I now set 'q->min_buffers_needed = 2' so this can never be called with less
than 2 buffers queued. 'min_buffers_needed' didn't exist when this code was
written, but now we can use it.

> 
>> +	spin_unlock_irq(&isi->irqlock);
>>  
>>  	return 0;
>> +
>> +err_reset:
>> +	pm_runtime_put(isi->dev);
>> +	v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);
>> +
>> +err_start_stream:
>> +	spin_lock_irq(&isi->irqlock);
>> +	isi->active = NULL;
>> +	/* Release all active buffers */
>> +	list_for_each_entry_safe(buf, node, &isi->video_buffer_list, list) {
>> +		list_del_init(&buf->list);
>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
>> +	}
>> +	spin_unlock_irq(&isi->irqlock);
>> +
>> +	return ret;
>>  }
>>  
>>  /* abort streaming and wait for last buffer */
>>  static void stop_streaming(struct vb2_queue *vq)
>>  {
>> -	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> +	struct atmel_isi *isi = vb2_get_drv_priv(vq);
>>  	struct frame_buffer *buf, *node;
>>  	int ret = 0;
>>  	unsigned long timeout;
>>  
>> -	spin_lock_irq(&isi->lock);
>> +	/* Disable stream on the sub device */
>> +	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);
>> +	if (ret && ret != -ENOIOCTLCMD)
>> +		dev_err(isi->dev, "stream off failed in subdev\n");
>> +
>> +	spin_lock_irq(&isi->irqlock);
>>  	isi->active = NULL;
>>  	/* Release all active buffers */
>>  	list_for_each_entry_safe(buf, node, &isi->video_buffer_list, list) {
>>  		list_del_init(&buf->list);
>>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>  	}
>> -	spin_unlock_irq(&isi->lock);
>> +	spin_unlock_irq(&isi->irqlock);
>>  
>>  	if (!isi->enable_preview_path) {
>>  		timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
>> @@ -520,7 +517,7 @@ static void stop_streaming(struct vb2_queue *vq)
>>  			msleep(1);
>>  
>>  		if (time_after(jiffies, timeout))
>> -			dev_err(icd->parent,
>> +			dev_err(isi->dev,
>>  				"Timeout waiting for finishing codec request\n");
>>  	}
>>  
>> @@ -531,9 +528,9 @@ static void stop_streaming(struct vb2_queue *vq)
>>  	/* Disable ISI and wait for it is done */
>>  	ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>>  	if (ret < 0)
>> -		dev_err(icd->parent, "Disable ISI timed out\n");
>> +		dev_err(isi->dev, "Disable ISI timed out\n");
>>  
>> -	pm_runtime_put(ici->v4l2_dev.dev);
>> +	pm_runtime_put(isi->dev);
>>  }
>>  
>>  static const struct vb2_ops isi_video_qops = {
>> @@ -548,380 +545,257 @@ static const struct vb2_ops isi_video_qops = {
>>  	.wait_finish		= vb2_ops_wait_finish,
>>  };
>>  
>> -/* ------------------------------------------------------------------
>> -	SOC camera operations for the device
>> -   ------------------------------------------------------------------*/
>> -static int isi_camera_init_videobuf(struct vb2_queue *q,
>> -				     struct soc_camera_device *icd)
>> +static int isi_g_fmt_vid_cap(struct file *file, void *priv,
>> +			      struct v4l2_format *fmt)
>>  {
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> +	struct atmel_isi *isi = video_drvdata(file);
>>  
>> -	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -	q->io_modes = VB2_MMAP;
>> -	q->drv_priv = icd;
>> -	q->buf_struct_size = sizeof(struct frame_buffer);
>> -	q->ops = &isi_video_qops;
>> -	q->mem_ops = &vb2_dma_contig_memops;
>> -	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> -	q->lock = &ici->host_lock;
>> -	q->dev = ici->v4l2_dev.dev;
>> +	*fmt = isi->fmt;
>>  
>> -	return vb2_queue_init(q);
>> +	return 0;
>>  }
>>  
>> -static int isi_camera_set_fmt(struct soc_camera_device *icd,
>> -			      struct v4l2_format *f)
>> +static struct isi_format *find_format_by_fourcc(struct atmel_isi *isi,
>> +						 unsigned int fourcc)
>>  {
>> -	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	const struct soc_camera_format_xlate *xlate;
>> -	struct v4l2_pix_format *pix = &f->fmt.pix;
>> +	unsigned int num_formats = isi->num_user_formats;
>> +	struct isi_format *fmt;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < num_formats; i++) {
>> +		fmt = isi->user_formats[i];
>> +		if (fmt->fourcc == fourcc)
>> +			return fmt;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>> +			struct isi_format **current_fmt)
>> +{
>> +	struct isi_format *isi_fmt;
>> +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
>> +	struct v4l2_subdev_pad_config pad_cfg;
>>  	struct v4l2_subdev_format format = {
>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> +		.which = V4L2_SUBDEV_FORMAT_TRY,
>>  	};
>> -	struct v4l2_mbus_framefmt *mf = &format.format;
>>  	int ret;
>>  
>> -	/* check with atmel-isi support format, if not support use YUYV */
>> -	if (!is_supported(icd, pix->pixelformat))
>> -		pix->pixelformat = V4L2_PIX_FMT_YUYV;
>> -
>> -	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>> -	if (!xlate) {
>> -		dev_warn(icd->parent, "Format %x not found\n",
>> -			 pix->pixelformat);
>> +	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>  		return -EINVAL;
>> -	}
>>  
>> -	dev_dbg(icd->parent, "Plan to set format %dx%d\n",
>> -			pix->width, pix->height);
>> +	isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);
>> +	if (!isi_fmt) {
>> +		isi_fmt = isi->user_formats[isi->num_user_formats - 1];
>> +		pixfmt->pixelformat = isi_fmt->fourcc;
>> +	}
>>  
>> -	mf->width	= pix->width;
>> -	mf->height	= pix->height;
>> -	mf->field	= pix->field;
>> -	mf->colorspace	= pix->colorspace;
>> -	mf->code	= xlate->code;
>> +	/* Limit to Atmel ISC hardware capabilities */
>> +	if (pixfmt->width > MAX_SUPPORT_WIDTH)
>> +		pixfmt->width = MAX_SUPPORT_WIDTH;
>> +	if (pixfmt->height > MAX_SUPPORT_HEIGHT)
>> +		pixfmt->height = MAX_SUPPORT_HEIGHT;
>>  
>> -	ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &format);
>> +	v4l2_fill_mbus_format(&format.format, pixfmt, isi_fmt->mbus_code);
>> +	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>> +			       &pad_cfg, &format);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	if (mf->code != xlate->code)
>> -		return -EINVAL;
>> +	v4l2_fill_pix_format(pixfmt, &format.format);
>>  
>> -	pix->width		= mf->width;
>> -	pix->height		= mf->height;
>> -	pix->field		= mf->field;
>> -	pix->colorspace		= mf->colorspace;
>> -	icd->current_fmt	= xlate;
>> +	pixfmt->field = V4L2_FIELD_NONE;
>> +	pixfmt->bytesperline = pixfmt->width * isi_fmt->bpp;
>> +	pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
>>  
>> -	dev_dbg(icd->parent, "Finally set format %dx%d\n",
>> -		pix->width, pix->height);
>> +	if (current_fmt)
>> +		*current_fmt = isi_fmt;
>>  
>> -	return ret;
>> +	return 0;
>>  }
>>  
>> -static int isi_camera_try_fmt(struct soc_camera_device *icd,
>> -			      struct v4l2_format *f)
>> +static int isi_set_fmt(struct atmel_isi *isi, struct v4l2_format *f)
>>  {
>> -	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	const struct soc_camera_format_xlate *xlate;
>> -	struct v4l2_pix_format *pix = &f->fmt.pix;
>> -	struct v4l2_subdev_pad_config pad_cfg;
>>  	struct v4l2_subdev_format format = {
>> -		.which = V4L2_SUBDEV_FORMAT_TRY,
>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>  	};
>> -	struct v4l2_mbus_framefmt *mf = &format.format;
>> -	u32 pixfmt = pix->pixelformat;
>> +	struct isi_format *current_fmt;
>>  	int ret;
>>  
>> -	/* check with atmel-isi support format, if not support use YUYV */
>> -	if (!is_supported(icd, pix->pixelformat))
>> -		pix->pixelformat = V4L2_PIX_FMT_YUYV;
>> -
>> -	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>> -	if (pixfmt && !xlate) {
>> -		dev_warn(icd->parent, "Format %x not found\n", pixfmt);
>> -		return -EINVAL;
>> -	}
>> -
>> -	/* limit to Atmel ISI hardware capabilities */
>> -	if (pix->height > MAX_SUPPORT_HEIGHT)
>> -		pix->height = MAX_SUPPORT_HEIGHT;
>> -	if (pix->width > MAX_SUPPORT_WIDTH)
>> -		pix->width = MAX_SUPPORT_WIDTH;
>> -
>> -	/* limit to sensor capabilities */
>> -	mf->width	= pix->width;
>> -	mf->height	= pix->height;
>> -	mf->field	= pix->field;
>> -	mf->colorspace	= pix->colorspace;
>> -	mf->code	= xlate->code;
>> +	ret = isi_try_fmt(isi, f, &current_fmt);
>> +	if (ret)
>> +		return ret;
>>  
>> -	ret = v4l2_subdev_call(sd, pad, set_fmt, &pad_cfg, &format);
>> +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
>> +			      current_fmt->mbus_code);
>> +	ret = v4l2_subdev_call(isi->entity.subdev, pad,
>> +			       set_fmt, NULL, &format);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	pix->width	= mf->width;
>> -	pix->height	= mf->height;
>> -	pix->colorspace	= mf->colorspace;
>> +	isi->fmt = *f;
>> +	isi->current_fmt = current_fmt;
>>  
>> -	switch (mf->field) {
>> -	case V4L2_FIELD_ANY:
>> -		pix->field = V4L2_FIELD_NONE;
>> -		break;
>> -	case V4L2_FIELD_NONE:
>> -		break;
>> -	default:
>> -		dev_err(icd->parent, "Field type %d unsupported.\n",
>> -			mf->field);
>> -		ret = -EINVAL;
>> -	}
>> -
>> -	return ret;
>> +	return 0;
>>  }
>>  
>> -static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
>> -	{
>> -		.fourcc			= V4L2_PIX_FMT_YUYV,
>> -		.name			= "Packed YUV422 16 bit",
>> -		.bits_per_sample	= 8,
>> -		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
>> -		.order			= SOC_MBUS_ORDER_LE,
>> -		.layout			= SOC_MBUS_LAYOUT_PACKED,
>> -	},
>> -	{
>> -		.fourcc			= V4L2_PIX_FMT_RGB565,
>> -		.name			= "RGB565",
>> -		.bits_per_sample	= 8,
>> -		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
>> -		.order			= SOC_MBUS_ORDER_LE,
>> -		.layout			= SOC_MBUS_LAYOUT_PACKED,
>> -	},
>> -};
>> -
>> -/* This will be corrected as we get more formats */
>> -static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
>> +static int isi_s_fmt_vid_cap(struct file *file, void *priv,
>> +			      struct v4l2_format *f)
>>  {
>> -	return	fmt->packing == SOC_MBUS_PACKING_NONE ||
>> -		(fmt->bits_per_sample == 8 &&
>> -		 fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
>> -		(fmt->bits_per_sample > 8 &&
>> -		 fmt->packing == SOC_MBUS_PACKING_EXTEND16);
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +
>> +	if (vb2_is_streaming(&isi->queue))
>> +		return -EBUSY;
>> +
>> +	return isi_set_fmt(isi, f);
>>  }
>>  
>> -#define ISI_BUS_PARAM (V4L2_MBUS_MASTER |	\
>> -		V4L2_MBUS_HSYNC_ACTIVE_HIGH |	\
>> -		V4L2_MBUS_HSYNC_ACTIVE_LOW |	\
>> -		V4L2_MBUS_VSYNC_ACTIVE_HIGH |	\
>> -		V4L2_MBUS_VSYNC_ACTIVE_LOW |	\
>> -		V4L2_MBUS_PCLK_SAMPLE_RISING |	\
>> -		V4L2_MBUS_PCLK_SAMPLE_FALLING |	\
>> -		V4L2_MBUS_DATA_ACTIVE_HIGH)
>> -
>> -static int isi_camera_try_bus_param(struct soc_camera_device *icd,
>> -				    unsigned char buswidth)
>> +static int isi_try_fmt_vid_cap(struct file *file, void *priv,
>> +				struct v4l2_format *f)
>>  {
>> -	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> -	struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
>> -	unsigned long common_flags;
>> -	int ret;
>> -
>> -	ret = v4l2_subdev_call(sd, video, g_mbus_config, &cfg);
>> -	if (!ret) {
>> -		common_flags = soc_mbus_config_compatible(&cfg,
>> -							  ISI_BUS_PARAM);
> 
> As far as I understand the bus configuration selection is now performed 
> based on fixed platform data or device tree parameters.

Correct.

> 
>> -		if (!common_flags) {
>> -			dev_warn(icd->parent,
>> -				 "Flags incompatible: camera 0x%x, host 0x%x\n",
>> -				 cfg.flags, ISI_BUS_PARAM);
>> -			return -EINVAL;
>> -		}
>> -	} else if (ret != -ENOIOCTLCMD) {
>> -		return ret;
>> -	}
>> +	struct atmel_isi *isi = video_drvdata(file);
>>  
>> -	if ((1 << (buswidth - 1)) & isi->width_flags)
>> -		return 0;
>> -	return -EINVAL;
>> +	return isi_try_fmt(isi, f, NULL);
>>  }
>>  
>> -
>> -static int isi_camera_get_formats(struct soc_camera_device *icd,
>> -				  unsigned int idx,
>> -				  struct soc_camera_format_xlate *xlate)
>> +static int isi_enum_fmt_vid_cap(struct file *file, void  *priv,
>> +				struct v4l2_fmtdesc *f)
>>  {
>> -	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	int formats = 0, ret, i, n;
>> -	/* sensor format */
>> -	struct v4l2_subdev_mbus_code_enum code = {
>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> -		.index = idx,
>> -	};
>> -	/* soc camera host format */
>> -	const struct soc_mbus_pixelfmt *fmt;
>> +	struct atmel_isi *isi = video_drvdata(file);
>>  
>> -	ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
>> -	if (ret < 0)
>> -		/* No more formats */
>> -		return 0;
>> -
>> -	fmt = soc_mbus_get_fmtdesc(code.code);
>> -	if (!fmt) {
>> -		dev_err(icd->parent,
>> -			"Invalid format code #%u: %d\n", idx, code.code);
>> -		return 0;
>> -	}
>> +	if (f->index >= isi->num_user_formats)
>> +		return -EINVAL;
>>  
>> -	/* This also checks support for the requested bits-per-sample */
>> -	ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
>> -	if (ret < 0) {
>> -		dev_err(icd->parent,
>> -			"Fail to try the bus parameters.\n");
>> -		return 0;
>> -	}
>> +	f->pixelformat = isi->user_formats[f->index]->fourcc;
>> +	return 0;
>> +}
>>  
>> -	switch (code.code) {
>> -	case MEDIA_BUS_FMT_UYVY8_2X8:
>> -	case MEDIA_BUS_FMT_VYUY8_2X8:
>> -	case MEDIA_BUS_FMT_YUYV8_2X8:
>> -	case MEDIA_BUS_FMT_YVYU8_2X8:
>> -		n = ARRAY_SIZE(isi_camera_formats);
>> -		formats += n;
>> -		for (i = 0; xlate && i < n; i++, xlate++) {
>> -			xlate->host_fmt	= &isi_camera_formats[i];
>> -			xlate->code	= code.code;
>> -			dev_dbg(icd->parent, "Providing format %s using code %d\n",
>> -				xlate->host_fmt->name, xlate->code);
>> -		}
>> -		break;
>> -	default:
>> -		if (!isi_camera_packing_supported(fmt))
>> -			return 0;
>> -		if (xlate)
>> -			dev_dbg(icd->parent,
>> -				"Providing format %s in pass-through mode\n",
>> -				fmt->name);
>> -	}
>> +static int isi_querycap(struct file *file, void *priv,
>> +			struct v4l2_capability *cap)
>> +{
>> +	strlcpy(cap->driver, "atmel-isi", sizeof(cap->driver));
>> +	strlcpy(cap->card, "Atmel Image Sensor Interface", sizeof(cap->card));
>> +	strlcpy(cap->bus_info, "platform:isi", sizeof(cap->bus_info));
>> +	return 0;
>> +}
>>  
>> -	/* Generic pass-through */
>> -	formats++;
>> -	if (xlate) {
>> -		xlate->host_fmt	= fmt;
>> -		xlate->code	= code.code;
>> -		xlate++;
>> -	}
>> +static int isi_enum_input(struct file *file, void *priv,
>> +			   struct v4l2_input *i)
>> +{
>> +	if (i->index != 0)
>> +		return -EINVAL;
>>  
>> -	return formats;
>> +	i->type = V4L2_INPUT_TYPE_CAMERA;
>> +	strlcpy(i->name, "Camera", sizeof(i->name));
>> +	return 0;
>>  }
>>  
>> -static int isi_camera_add_device(struct soc_camera_device *icd)
>> +static int isi_g_input(struct file *file, void *priv, unsigned int *i)
>>  {
>> -	dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
>> -		 icd->devnum);
>> +	*i = 0;
>> +	return 0;
>> +}
>>  
>> +static int isi_s_input(struct file *file, void *priv, unsigned int i)
>> +{
>> +	if (i > 0)
>> +		return -EINVAL;
>>  	return 0;
>>  }
>>  
>> -static void isi_camera_remove_device(struct soc_camera_device *icd)
>> +static int isi_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>  {
>> -	dev_dbg(icd->parent, "Atmel ISI Camera driver detached from camera %d\n",
>> -		 icd->devnum);
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +
>> +	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	a->parm.capture.readbuffers = 2;
>> +	return v4l2_subdev_call(isi->entity.subdev, video, g_parm, a);
>>  }
>>  
>> -static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
>> +static int isi_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>  {
>> -	struct soc_camera_device *icd = file->private_data;
>> +	struct atmel_isi *isi = video_drvdata(file);
>>  
>> -	return vb2_poll(&icd->vb2_vidq, file, pt);
>> +	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	a->parm.capture.readbuffers = 2;
>> +	return v4l2_subdev_call(isi->entity.subdev, video, s_parm, a);
>>  }
>>  
>> -static int isi_camera_querycap(struct soc_camera_host *ici,
>> -			       struct v4l2_capability *cap)
>> +static int isi_enum_framesizes(struct file *file, void *fh,
>> +			       struct v4l2_frmsizeenum *fsize)
>>  {
>> -	strcpy(cap->driver, "atmel-isi");
>> -	strcpy(cap->card, "Atmel Image Sensor Interface");
>> -	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>> -	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +	const struct isi_format *isi_fmt;
>> +	struct v4l2_subdev_frame_size_enum fse = {
>> +		.index = fsize->index,
>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> +	};
>> +	int ret;
>> +
>> +	isi_fmt = find_format_by_fourcc(isi, fsize->pixel_format);
>> +	if (!isi_fmt)
>> +		return -EINVAL;
>> +
>> +	fse.code = isi_fmt->mbus_code;
>> +
>> +	ret = v4l2_subdev_call(isi->entity.subdev, pad, enum_frame_size,
>> +			       NULL, &fse);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>> +	fsize->discrete.width = fse.max_width;
>> +	fsize->discrete.height = fse.max_height;
>>  
>>  	return 0;
>>  }
>>  
>> -static int isi_camera_set_bus_param(struct soc_camera_device *icd)
>> +static int isi_enum_frameintervals(struct file *file, void *fh,
>> +				    struct v4l2_frmivalenum *fival)
>>  {
>> -	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> -	struct atmel_isi *isi = ici->priv;
>> -	struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
>> -	unsigned long common_flags;
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +	const struct isi_format *isi_fmt;
>> +	struct v4l2_subdev_frame_interval_enum fie = {
>> +		.index = fival->index,
>> +		.width = fival->width,
>> +		.height = fival->height,
>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> +	};
>>  	int ret;
>> -	u32 cfg1 = 0;
>>  
>> -	ret = v4l2_subdev_call(sd, video, g_mbus_config, &cfg);
>> -	if (!ret) {
>> -		common_flags = soc_mbus_config_compatible(&cfg,
>> -							  ISI_BUS_PARAM);
>> -		if (!common_flags) {
>> -			dev_warn(icd->parent,
>> -				 "Flags incompatible: camera 0x%x, host 0x%x\n",
>> -				 cfg.flags, ISI_BUS_PARAM);
>> -			return -EINVAL;
>> -		}
>> -	} else if (ret != -ENOIOCTLCMD) {
>> +	isi_fmt = find_format_by_fourcc(isi, fival->pixel_format);
>> +	if (!isi_fmt)
>> +		return -EINVAL;
>> +
>> +	fie.code = isi_fmt->mbus_code;
>> +
>> +	ret = v4l2_subdev_call(isi->entity.subdev, pad,
>> +			       enum_frame_interval, NULL, &fie);
>> +	if (ret)
>>  		return ret;
>> -	} else {
>> -		common_flags = ISI_BUS_PARAM;
>> -	}
>> -	dev_dbg(icd->parent, "Flags cam: 0x%x host: 0x%x common: 0x%lx\n",
>> -		cfg.flags, ISI_BUS_PARAM, common_flags);
>> -
>> -	/* Make choises, based on platform preferences */
>> -	if ((common_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) &&
>> -	    (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) {
>> -		if (isi->pdata.hsync_act_low)
>> -			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_HIGH;
>> -		else
>> -			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_LOW;
>> -	}
>>  
>> -	if ((common_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) &&
>> -	    (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) {
>> -		if (isi->pdata.vsync_act_low)
>> -			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_HIGH;
>> -		else
>> -			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_LOW;
>> -	}
>> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>> +	fival->discrete = fie.interval;
>>  
>> -	if ((common_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) &&
>> -	    (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)) {
>> -		if (isi->pdata.pclk_act_falling)
>> -			common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_RISING;
>> -		else
>> -			common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_FALLING;
>> -	}
>> +	return 0;
>> +}
>>  
>> -	cfg.flags = common_flags;
>> -	ret = v4l2_subdev_call(sd, video, s_mbus_config, &cfg);
>> -	if (ret < 0 && ret != -ENOIOCTLCMD) {
>> -		dev_dbg(icd->parent, "camera s_mbus_config(0x%lx) returned %d\n",
>> -			common_flags, ret);
>> -		return ret;
>> -	}
>> +static void isi_camera_set_bus_param(struct atmel_isi *isi)
>> +{
>> +	u32 cfg1 = 0;
>>  
>>  	/* set bus param for ISI */
>> -	if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
>> +	if (isi->pdata.hsync_act_low)
>>  		cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
>> -	if (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
>> +	if (isi->pdata.vsync_act_low)
>>  		cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
>> -	if (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
>> +	if (isi->pdata.pclk_act_falling)
>>  		cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
>> -
>> -	dev_dbg(icd->parent, "vsync active %s, hsync active %s, sampling on pix clock %s edge\n",
>> -		common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW ? "low" : "high",
>> -		common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW ? "low" : "high",
>> -		common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING ? "falling" : "rising");
>> -
>>  	if (isi->pdata.has_emb_sync)
>>  		cfg1 |= ISI_CFG1_EMB_SYNC;
>>  	if (isi->pdata.full_mode)
>> @@ -930,50 +804,19 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
>>  	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>>  
>>  	/* Enable PM and peripheral clock before operate isi registers */
>> -	pm_runtime_get_sync(ici->v4l2_dev.dev);
>> +	pm_runtime_get_sync(isi->dev);
>>  
>>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>  	isi_writel(isi, ISI_CFG1, cfg1);
>>  
>> -	pm_runtime_put(ici->v4l2_dev.dev);
>> -
>> -	return 0;
>> +	pm_runtime_put(isi->dev);
>>  }
>>  
>> -static struct soc_camera_host_ops isi_soc_camera_host_ops = {
>> -	.owner		= THIS_MODULE,
>> -	.add		= isi_camera_add_device,
>> -	.remove		= isi_camera_remove_device,
>> -	.set_fmt	= isi_camera_set_fmt,
>> -	.try_fmt	= isi_camera_try_fmt,
>> -	.get_formats	= isi_camera_get_formats,
>> -	.init_videobuf2	= isi_camera_init_videobuf,
>> -	.poll		= isi_camera_poll,
>> -	.querycap	= isi_camera_querycap,
>> -	.set_bus_param	= isi_camera_set_bus_param,
>> -};
>> -
>>  /* -----------------------------------------------------------------------*/
>> -static int atmel_isi_remove(struct platform_device *pdev)
>> -{
>> -	struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>> -	struct atmel_isi *isi = container_of(soc_host,
>> -					struct atmel_isi, soc_host);
>> -
>> -	soc_camera_host_unregister(soc_host);
>> -	dma_free_coherent(&pdev->dev,
>> -			sizeof(struct fbd) * MAX_BUFFER_NUM,
>> -			isi->p_fb_descriptors,
>> -			isi->fb_descriptors_phys);
>> -	pm_runtime_disable(&pdev->dev);
>> -
>> -	return 0;
>> -}
>> -
>>  static int atmel_isi_parse_dt(struct atmel_isi *isi,
>>  			struct platform_device *pdev)
>>  {
>> -	struct device_node *np= pdev->dev.of_node;
>> +	struct device_node *np = pdev->dev.of_node;
>>  	struct v4l2_of_endpoint ep;
>>  	int err;
>>  
>> @@ -1021,13 +864,335 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
>>  	return 0;
>>  }
>>  
>> +static int isi_open(struct file *file)
>> +{
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +	struct v4l2_subdev *sd = isi->entity.subdev;
>> +	int ret;
>> +
>> +	if (mutex_lock_interruptible(&isi->lock))
>> +		return -ERESTARTSYS;
>> +
>> +	ret = v4l2_fh_open(file);
>> +	if (ret < 0)
>> +		goto unlock;
>> +
>> +	if (!v4l2_fh_is_singular_file(file))
>> +		goto fh_rel;
>> +
>> +	ret = v4l2_subdev_call(sd, core, s_power, 1);
>> +	if (ret < 0 && ret != -ENOIOCTLCMD)
>> +		goto fh_rel;
>> +
>> +	ret = isi_set_fmt(isi, &isi->fmt);
>> +	if (ret)
>> +		v4l2_subdev_call(sd, core, s_power, 0);
>> +fh_rel:
>> +	if (ret)
>> +		v4l2_fh_release(file);
>> +unlock:
>> +	mutex_unlock(&isi->lock);
>> +	return ret;
>> +}
>> +
>> +static int isi_release(struct file *file)
>> +{
>> +	struct atmel_isi *isi = video_drvdata(file);
>> +	struct v4l2_subdev *sd = isi->entity.subdev;
>> +	bool fh_singular;
>> +	int ret;
>> +
>> +	mutex_lock(&isi->lock);
>> +
>> +	fh_singular = v4l2_fh_is_singular_file(file);
>> +
>> +	ret = _vb2_fop_release(file, NULL);
>> +
>> +	if (fh_singular)
>> +		v4l2_subdev_call(sd, core, s_power, 0);
>> +
>> +	mutex_unlock(&isi->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_ioctl_ops isi_ioctl_ops = {
>> +	.vidioc_querycap		= isi_querycap,
>> +
>> +	.vidioc_try_fmt_vid_cap		= isi_try_fmt_vid_cap,
>> +	.vidioc_g_fmt_vid_cap		= isi_g_fmt_vid_cap,
>> +	.vidioc_s_fmt_vid_cap		= isi_s_fmt_vid_cap,
>> +	.vidioc_enum_fmt_vid_cap	= isi_enum_fmt_vid_cap,
>> +
>> +	.vidioc_enum_input		= isi_enum_input,
>> +	.vidioc_g_input			= isi_g_input,
>> +	.vidioc_s_input			= isi_s_input,
>> +
>> +	.vidioc_g_parm			= isi_g_parm,
>> +	.vidioc_s_parm			= isi_s_parm,
>> +	.vidioc_enum_framesizes		= isi_enum_framesizes,
>> +	.vidioc_enum_frameintervals	= isi_enum_frameintervals,
>> +
>> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
>> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
>> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
>> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
>> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
>> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
>> +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
>> +	.vidioc_streamon		= vb2_ioctl_streamon,
>> +	.vidioc_streamoff		= vb2_ioctl_streamoff,
>> +
>> +	.vidioc_log_status		= v4l2_ctrl_log_status,
>> +	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
>> +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
>> +};
>> +
>> +static const struct v4l2_file_operations isi_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= video_ioctl2,
>> +	.open		= isi_open,
>> +	.release	= isi_release,
>> +	.poll		= vb2_fop_poll,
>> +	.mmap		= vb2_fop_mmap,
>> +	.read		= vb2_fop_read,
>> +};
>> +
>> +static int isi_set_default_fmt(struct atmel_isi *isi)
>> +{
>> +	struct v4l2_format f = {
>> +		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>> +		.fmt.pix = {
>> +			.width		= VGA_WIDTH,
>> +			.height		= VGA_HEIGHT,
>> +			.field		= V4L2_FIELD_NONE,
>> +			.pixelformat	= isi->user_formats[0]->fourcc,
>> +		},
>> +	};
>> +	int ret;
>> +
>> +	ret = isi_try_fmt(isi, &f, NULL);
>> +	if (ret)
>> +		return ret;
>> +	isi->current_fmt = isi->user_formats[0];
>> +	isi->fmt = f;
>> +	return 0;
>> +}
>> +
>> +static struct isi_format *find_format_by_code(unsigned int code, int *index)
>> +{
>> +	struct isi_format *fmt = &isi_formats[0];
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(isi_formats); i++) {
>> +		if (fmt->mbus_code == code && !fmt->support && !fmt->skip) {
>> +			*index = i;
>> +			return fmt;
>> +		}
>> +
>> +		fmt++;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int isi_formats_init(struct atmel_isi *isi)
>> +{
>> +	struct isi_format *fmt;
>> +	struct v4l2_subdev *subdev = isi->entity.subdev;
>> +	int num_fmts = 0, i, j;
>> +	struct v4l2_subdev_mbus_code_enum mbus_code = {
>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> +	};
>> +
>> +	fmt = &isi_formats[0];
> 
> Superfluous too.

Fixed.

> 
>> +
>> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
>> +	       NULL, &mbus_code)) {
>> +		fmt = find_format_by_code(mbus_code.code, &i);
> 
> find_format_by_code() assigns i, but it is never used.
> 
>> +		if (!fmt) {
>> +			mbus_code.index++;
>> +			continue;
>> +		}
>> +
>> +		fmt->support = true;
>> +		for (i = 0; i < ARRAY_SIZE(isi_formats); i++)
>> +			if (isi_formats[i].fourcc == fmt->fourcc &&
>> +			    !isi_formats[i].support)
>> +				isi_formats[i].skip = true;
>> +		num_fmts++;
>> +	}
> 
> Hm, how about
> 
> +static int isi_formats_init(struct atmel_isi *isi)
> +{
> +	struct isi_format *isi_fmts[ARRAY_SIZE(isi_formats)];
> +	unsigned int n_fmts = 0, i, j;
> ...
> +	
> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
> +		NULL, &mbus_code)) {
> +		for (i = 0; i < ARRAY_SIZE(isi_formats); i++)
> +			if (isi_formats[i].mbus_code == mbus_code.code) {
> +				/* Code supported, have we got this fourcc yet? */
> +				for (j = 0; j < n_fmts; j++)
> +					if (isi_fmts[j]->fourcc == 
> +						isi_formats[i].fourcc)
> +						/* Already available */
> +						break;
> +				if (j == n_fmts)
> +					/* new */
> +					isi_fmts[n_fmts++] = isi_formats + i;
> +			}
> +		mbus_code.index++;
> +	}
> +
> +	devm_kcalloc(dev, n_fmts, ...);
> ...
> 
> Without calling .enum_mbus_code() multiple times for the same index, 
> without .skip, without .support, without overwriting static data...

Yes, much better. Thanks! This change supercedes some of my comments I made
above since they no longer apply.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux