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

[Hans Verkuil]

On 08/10/2016 07:36 AM, Wu, Songjun wrote:
> 
> 
> On 8/8/2016 17:56, Hans Verkuil wrote:
>> On 08/08/2016 11:37 AM, Hans Verkuil wrote:
>>> On 08/03/2016 10:08 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@xxxxxxxxxxxxx>
>>>> ---
>>>>
>>>> Changes in v8:
>>>> - Power on the sensor on the first open in function
>>>>   'isc_open'.
>>>> - Power off the sensor on the last release in function
>>>>   'isc_release'.
>>>> - Remove the switch of the pipeline.
>>>>
>>>> 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      | 1503 +++++++++++++++++++++++++
>>>>  6 files changed, 1681 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..d99d4a5
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>>
>>> <snip>
>>>
>>>> +static int isc_set_default_fmt(struct isc_device *isc)
>>>> +{
>>>> +	u32 index = isc->num_user_formats - 1;
>>>
>>> Why pick the last format? Strictly speaking it doesn't matter, but in practice
>>> the most common formats tend to be at the beginning of the format list.
>>>
>>>> +	struct v4l2_format f = {
>>>> +		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>>>> +		.fmt.pix = {
>>>> +			.width		= VGA_WIDTH,
>>>> +			.height		= VGA_HEIGHT,
>>>> +			.field		= V4L2_FIELD_NONE,
>>>> +			.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;
>>>> +
>>>> +	if (!v4l2_fh_is_singular_file(file))
>>>> +		goto unlock;
>>>> +
>>>> +	ret = v4l2_subdev_call(sd, core, s_power, 1);
>>>> +	if (ret < 0 && ret != -ENOIOCTLCMD)
>>>> +		goto unlock;
>>>> +
>>>> +	ret = isc_set_default_fmt(isc);
>>>
>>> This doesn't belong here, this needs to be done in isc_async_complete().
>>>
>>> Having the code here means that every time you open the device, the format
>>> changes back to the default. That's not what you want.
>>
>> Actually, you do need to set the format here since here is where you turn on
>> the sensor power, but it should be the current format, not the default format.
>>
>> And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
>> in order to let the subdev adjust the proposed default format. The 'try' doesn't
>> need to power on the sensor.
>>
> I replace the 'set fmt' with 'try fmt', then test with v4l2-compliance, 
> but there are tree fail cases. I think it's reasonable that setting a 
> default fmt on the first open. When app open the device on the first 
> open, it should set a fmt, or the default fmt will be set.

It's not reasonable :-). In V4L2 all format (or any other for that matter) settings
are persistent.

Anyway, take a look at my atmel-isi driver:

https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel-isi.c?h=sama5d3

There I use try_fmt as well, and that passes the compliance test just fine.

Regards,

	Hans