Messages in this thread | | | Subject | Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code | From | "Wu, Songjun" <> | Date | Mon, 8 Aug 2016 18:11:24 +0800 |
| |
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@microchip.com> >>> --- >>> >>> 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. > Accept, thank you.
My purpose is to set a default format, if the default format is not set, there will be a fail case when tested by v4l2-tool. I will try to modify the code and test again.
>> >>> + if (ret) > > You also need to power off the sd on error! > Accept, thank you.
>>> + goto unlock; >>> + >>> +unlock: >>> + mutex_unlock(&isc->lock); >>> + return ret; >>> +} >>> + >> > > Regards, > > Hans >
| |