Hi Laurent,
On 01/19/2017 12:43 PM, Todor Tomov wrote:
> Hi Laurent,
>
> Thank you for the detailed review.
>
> On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> Thank you for the patch.
>>
>> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>
>> camss is a quite generic, I'm a bit concerned about claiming that acronym in
>> the global kernel namespace. Would it be too long if we prefixed symbols with
>> msm_camss instead ?
>
> Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() or
> you have something else in mind too?
Could you please add more details about this?
>
>>
>>> Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx>
>>> ---
>>> drivers/media/platform/qcom/camss-8x16/video.c | 597 ++++++++++++++++++++++
>>> drivers/media/platform/qcom/camss-8x16/video.h | 67 +++
>>> 2 files changed, 664 insertions(+)
>>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>>> index 0000000..0bf8ea9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
<snip>
>>> +/* ------------------------------------------------------------------------
>>> + * V4L2 file operations
>>> + */
>>> +
>>> +/*
>>> + * video_init_format - Helper function to initialize format
>>> + *
>>> + * Initialize all pad formats with default values.
>>> + */
>>> +static int video_init_format(struct file *file, void *fh)
>>> +{
>>> + struct v4l2_format format;
>>> +
>>> + memset(&format, 0, sizeof(format));
>>> + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +
>>> + return video_s_fmt(file, fh, &format);
>>
>> This will set the active format every time you open the device node, I don't
>> think that's what you want.
>
> Well, actually this is what I wanted. I wanted to keep in sync the pixel format
> on the video node and the media bus format on the subdev node (i.e. the pixel
> format will be always correct for the current media bus format). For the current
> version there is a direct correspondence between the pixel format and the media
> format so this will work I think. For the future there might be multiple pixel
> formats for one media bus format and a second open of the video node could reset
> the pixel format to unwanted value so this will need a change. I'm wondering about
> (and still not able to find) a good moment/event when to perform the initialization
> of the format on the video node. As it gets the current format from the subdev
> node, the moment of the registration will be too early as the media link is still
> not created. But after that I couldn't find a suitable callback/event where to do
> it. If you can share any idea about this, please do :)
I still haven't found a better solution for this. If you have something in mind,
please share.
>
>>
>>> +}
>>> +
>>> +static int video_open(struct file *file)
>>> +{
>>> + struct video_device *vdev = video_devdata(file);
>>> + struct camss_video *video = video_drvdata(file);
>>> + struct camss_video_fh *handle;
>>> + int ret;
>>> +
>>> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>>> + if (handle == NULL)
>>> + return -ENOMEM;
>>> +
>>> + v4l2_fh_init(&handle->vfh, video->vdev);
>>> + v4l2_fh_add(&handle->vfh);
>>> +
>>> + handle->video = video;
>>> + file->private_data = &handle->vfh;
>>> +
>>> + ret = v4l2_pipeline_pm_use(&vdev->entity, 1);
>>> + if (ret < 0) {
>>> + dev_err(video->camss->dev, "Failed to power up pipeline\n");
>>> + goto error_pm_use;
>>> + }
>>> +
>>> + ret = video_init_format(file, &handle->vfh);
>>> + if (ret < 0) {
>>> + dev_err(video->camss->dev, "Failed to init format\n");
>>> + goto error_init_format;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +error_init_format:
>>> + v4l2_pipeline_pm_use(&vdev->entity, 0);
>>> +
>>> +error_pm_use:
>>> + v4l2_fh_del(&handle->vfh);
>>> + kfree(handle);
>>> +
>>> + return ret;
>>> +}
--
Best regards,
Todor Tomov
