Re: [PATCH v2 2/5] [media] exynos-mscl: Add core functionality for the M-Scaler driver

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

 



On Tue, Aug 20, 2013 at 11:57 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 08/20/2013 07:43 AM, Shaik Ameer Basha wrote:
>> + linux-media, linux-samsung-soc
>>
>> Hi Hans,
>>
>> Thanks for the review.
>> Will address all your comments in v3.
>>
>> I have only one doubt regarding try_ctrl... (addressed inline)
>>
>>
>> On Mon, Aug 19, 2013 at 6:36 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>
>>> On 08/19/2013 12:58 PM, Shaik Ameer Basha wrote:
>>>> This patch adds the core functionality for the M-Scaler driver.
>>>
>>> Some more comments below...
>>>
>>>>
>>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/media/platform/exynos-mscl/mscl-core.c | 1312 ++++++++++++++++++++++++
>>>>  drivers/media/platform/exynos-mscl/mscl-core.h |  549 ++++++++++
>>>>  2 files changed, 1861 insertions(+)
>>>>  create mode 100644 drivers/media/platform/exynos-mscl/mscl-core.c
>>>>  create mode 100644 drivers/media/platform/exynos-mscl/mscl-core.h
>>>>
>>>> diff --git a/drivers/media/platform/exynos-mscl/mscl-core.c b/drivers/media/platform/exynos-mscl/mscl-core.c
>>>> new file mode 100644
>>>> index 0000000..4a3a851
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/exynos-mscl/mscl-core.c
>>>> @@ -0,0 +1,1312 @@
>>>> +/*
>>>> + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.
>>>> + *           http://www.samsung.com
>>>> + *
>>>> + * Samsung EXYNOS5 SoC series M-Scaler driver
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published
>>>> + * by the Free Software Foundation, either version 2 of the License,
>>>> + * or (at your option) any later version.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/interrupt.h>
>>
>> [snip]
>>
>>>> +
>>>> +static int __mscl_s_ctrl(struct mscl_ctx *ctx, struct v4l2_ctrl *ctrl)
>>>> +{
>>>> +     struct mscl_dev *mscl = ctx->mscl_dev;
>>>> +     struct mscl_variant *variant = mscl->variant;
>>>> +     unsigned int flags = MSCL_DST_FMT | MSCL_SRC_FMT;
>>>> +     int ret = 0;
>>>> +
>>>> +     if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
>>>> +             return 0;
>>>
>>> Why would you want to do this check?
>>
>> Will remove this. seems no such check is required for this driver.
>>
>>>
>>>> +
>>>> +     switch (ctrl->id) {
>>>> +     case V4L2_CID_HFLIP:
>>>> +             ctx->hflip = ctrl->val;
>>>> +             break;
>>>> +
>>>> +     case V4L2_CID_VFLIP:
>>>> +             ctx->vflip = ctrl->val;
>>>> +             break;
>>>> +
>>>> +     case V4L2_CID_ROTATE:
>>>> +             if ((ctx->state & flags) == flags) {
>>>> +                     ret = mscl_check_scaler_ratio(variant,
>>>> +                                     ctx->s_frame.crop.width,
>>>> +                                     ctx->s_frame.crop.height,
>>>> +                                     ctx->d_frame.crop.width,
>>>> +                                     ctx->d_frame.crop.height,
>>>> +                                     ctx->ctrls_mscl.rotate->val);
>>>> +
>>>> +                     if (ret)
>>>> +                             return -EINVAL;
>>>> +             }
>>>
>>> I think it would be good if the try_ctrl op is implemented so you can call
>>> VIDIOC_EXT_TRY_CTRLS in the application to check if the ROTATE control can be
>>> set.
>>
>> * @try_ctrl: Test whether the control's value is valid. Only relevant when
>> * the usual min/max/step checks are not sufficient.
>>
>> As we support only 0,90,270 and the min, max and step can address these values,
>> does it really relevant to have try_ctrl op here ???
>
> Well, you seem to have an additional mscl_check_scaler_ratio check here that can
> make it fail, in other words: the min/max/step checks aren't sufficient.

Ok. Thanks for the explanation. Will implement that.

Regards,
Shaik Ameer Basha

>
> Regards,
>
>         Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[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