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

[Hans Verkuil <hverkuil@xs4all.nl>]

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@xs4all.nl> 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@samsung.com>
>>> ---
>>>  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.

Regards,

	Hans