Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452305 > unrolled thread

[PATCH v7 0/2] [media] atmel-isc: add driver for Atmel ISC

Started bySongjun Wu <songjun.wu@microchip.com>
First post2016-07-29 10:10 +0200
Last post2016-08-02 10:00 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 0/2] [media] atmel-isc: add driver for Atmel ISC Songjun Wu <songjun.wu@microchip.com> - 2016-07-29 10:10 +0200
    Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller  code Hans Verkuil <hverkuil@xs4all.nl> - 2016-08-01 11:50 +0200
      Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller  code "Wu, Songjun" <Songjun.Wu@microchip.com> - 2016-08-02 08:30 +0200
        Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller  code Hans Verkuil <hverkuil@xs4all.nl> - 2016-08-02 09:40 +0200
          Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller  code "Wu, Songjun" <Songjun.Wu@microchip.com> - 2016-08-02 10:00 +0200

#1452305 — [PATCH v7 0/2] [media] atmel-isc: add driver for Atmel ISC

FromSongjun Wu <songjun.wu@microchip.com>
Date2016-07-29 10:10 +0200
Subject[PATCH v7 0/2] [media] atmel-isc: add driver for Atmel ISC
Message-ID<s0b1M-20c-7@gated-at.bofh.it>
The Image Sensor Controller driver includes two parts.
1) Driver code to implement the ISC function.
2) Device tree binding documentation, it describes how
   to add the ISC in device tree.

Test result with v4l-utils.
v4l2-compliance SHA   : not available

Driver Info:
        Driver name   : atmel_isc
        Card type     : Atmel Image Sensor Controller
        Bus info      : platform:atmel_isc f0008000.isc
        Driver version: 4.7.0
        Capabilities  : 0x84200001
                Video Capture
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps   : 0x04200001
                Video Capture
                Streaming
                Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second video open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

        Control ioctls:
                test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
                test VIDIOC_QUERYCTRL: OK (Not Supported)
                test VIDIOC_G/S_CTRL: OK (Not Supported)
                test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
                test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
                test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
                Standard Controls: 0 Private Controls: 0

        Format ioctls:
                test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
                test VIDIOC_G/S_PARM: OK
                test VIDIOC_G_FBUF: OK (Not Supported)
                test VIDIOC_G_FMT: OK
                test VIDIOC_TRY_FMT: OK
                test VIDIOC_S_FMT: OK
                test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
                test Cropping: OK (Not Supported)
                test Composing: OK (Not Supported)
                test Scaling: OK (Not Supported)

        Codec ioctls:
                test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
                test VIDIOC_G_ENC_INDEX: OK (Not Supported)
                test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

        Buffer ioctls:
                test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
                test VIDIOC_EXPBUF: OK

Test input 0:

Stream using all formats:
        test MMAP for Format BA81, Frame Size 640x480@60.00 Hz:
                Stride 640, Field None: OK
        test MMAP for Format YUYV, Frame Size 640x480@60.00 Hz:
                Stride 1280, Field None: OK

Total: 45, Succeeded: 45, Failed: 0, Warnings: 0

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:
- Add "iscck" and "gck" to clock-names.

Changes in v5:
- Modify the macro definition and the related code.
- Add clock-output-names.

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

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.

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().
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.

Songjun Wu (2):
  [media] atmel-isc: add the Image Sensor Controller code
  [media] atmel-isc: DT binding for Image Sensor Controller driver

 .../devicetree/bindings/media/atmel-isc.txt        |   65 +
 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 ++++++++++++++++++++
 7 files changed, 1854 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
 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

-- 
2.7.4

[toc] | [next] | [standalone]

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

FromHans Verkuil <hverkuil@xs4all.nl>
Date2016-08-01 11:50 +0200
SubjectRe: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
Message-ID<s1i1c-4yM-31@gated-at.bofh.it>
In reply to#1452305
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 <songjun.wu@microchip.com>
> ---
> 
> 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>

> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> new file mode 100644
> index 0000000..f2ef664
> --- /dev/null
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -0,0 +1,1611 @@

<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

[toc] | [prev] | [next] | [standalone]

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

From"Wu, Songjun" <Songjun.Wu@microchip.com>
Date2016-08-02 08:30 +0200
SubjectRe: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
Message-ID<s1Bnb-Io-11@gated-at.bofh.it>
In reply to#1453083

On 8/1/2016 17:47, Hans Verkuil wrote:
> 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 <songjun.wu@microchip.com>
>> ---
>>
>> 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>
>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>> new file mode 100644
>> index 0000000..f2ef664
>> --- /dev/null
>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>> @@ -0,0 +1,1611 @@
>
> <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?
>
ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the 
sensor output YUYV format, ISC bypass the data to the memory, the other 
is the sensor output raw format, ISC convert raw format to YUYV.

So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set 
this parameter to '1', not '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;
>
Accept, thank you.

>> +		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.
>
Accept, thank you.

>> +
>> +	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.
>
Accept, thank you.

>> +			.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...
>
Accept, I will add the v4l2_fh_is_singular to judge the first open.
Thank you.

>> +	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).
>
Accept, thank you.

>> +
>> +	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?
>
VB2_DMABUF and VB2_READ are missed out, they should be added.
Thank you.

>> +	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
>

[toc] | [prev] | [next] | [standalone]

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

FromHans Verkuil <hverkuil@xs4all.nl>
Date2016-08-02 09:40 +0200
SubjectRe: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
Message-ID<s1CsV-1on-5@gated-at.bofh.it>
In reply to#1453616

On 08/02/2016 08:20 AM, Wu, Songjun wrote:
>>> +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?
>>
> ISC can convert the raw format to the other format, e.g. YUYV.
> If we want to output YUYV format, there are two choices, one is the 
> sensor output YUYV format, ISC bypass the data to the memory, the other 
> is the sensor output raw format, ISC convert raw format to YUYV.
> 
> So I provide a module parameter to user to select.
> I prefer to select the sensor to output the specified format, then I set 
> this parameter to '1', not '0'.

Does this only apply to YUYV?

The reason I am hesitant about this option is that I am not convinced you need
it. The default (sensor preferred) makes sense and that's what other drivers
do as well. Unless you know of a real use-case where you want to set this to 0,
I would just drop this option.

If there *is* a real use-case, then split off adding this module option into a
separate patch so we can discuss it more without blocking getting this driver
into mainline. I don't like the way this is done here.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

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

From"Wu, Songjun" <Songjun.Wu@microchip.com>
Date2016-08-02 10:00 +0200
SubjectRe: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
Message-ID<s1CMh-1vQ-15@gated-at.bofh.it>
In reply to#1453643

On 8/2/2016 15:32, Hans Verkuil wrote:
>
>
> On 08/02/2016 08:20 AM, Wu, Songjun wrote:
>>>> +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?
>>>
>> ISC can convert the raw format to the other format, e.g. YUYV.
>> If we want to output YUYV format, there are two choices, one is the
>> sensor output YUYV format, ISC bypass the data to the memory, the other
>> is the sensor output raw format, ISC convert raw format to YUYV.
>>
>> So I provide a module parameter to user to select.
>> I prefer to select the sensor to output the specified format, then I set
>> this parameter to '1', not '0'.
>
> Does this only apply to YUYV?
>
> The reason I am hesitant about this option is that I am not convinced you need
> it. The default (sensor preferred) makes sense and that's what other drivers
> do as well. Unless you know of a real use-case where you want to set this to 0,
> I would just drop this option.
>
> If there *is* a real use-case, then split off adding this module option into a
> separate patch so we can discuss it more without blocking getting this driver
> into mainline. I don't like the way this is done here.
>
This does not only apply to YUYV, ISC IP defines some formats that can 
be converted from raw format.
In some scenarios, ISC can extend the formats, e.g. if the sensor does 
not support YUYV, but raw format, the ISC can output YUYV.

OK, I will remove the related code.
After this driver is accepted, I will submit another patch to add this 
feature.

Thank you.

> Regards,
>
> 	Hans
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web