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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |