Hi Sylwester Thank you for your comment and sorry for late answer. > >Hi Sunyoung, > >Please see my comments below (just a quick review)... > >On 02/29/2012 01:41 PM, Sunyoung Kang wrote: >> >> This patch adds support image rotator driver for EXYNOS >> SoCs and this is including following: >> 1) Image format >> : RGB565/888, YUV422 1p, YUV420 2p/3p >> 2) Rotation >> : 0/90/180/270 degree and X/Y Flip >> >> Signed-off-by: Ayoung Sim<a.sim@xxxxxxxxxxx> >> Signed-off-by: Sunyoung Kang<sy0816.kang@xxxxxxxxxxx> >> --- >> NOTE: >> This patch has been created based on following >> - media: media-dev: Add media devices for EXYNOS5 by Sungchun Kang >> - media: fimc-lite: Add new driver for camera interface by Sungchun Kang >> >> Dear Mauro, >> >> I couldn't find your review on Sungchun Kang's patches has been submitted 2weeks ago. >> Since this is based on them, we _really_ need your comments on that. > >I'm going to review those patches, just need to find time for that. >I have some serious doubts to your high level driver design, plus we will need >to agree on the code reuse with the s5p-fimc driver, since some devices are >common for exynos4 and exynos5. And you seem to just have ignored that fact. > >It is always better to consult as early as possible, to avoid significant time >waste, when lot's of code has to be rewritten. > And we created exynos directory for EXYNOS IPs including new one like g-scaler and rotator, and put this rotator into it. Do you have any idea for this new rotator driver location? >> If any problems/comments, please kindly let us know. >> >> Thanks. >> Sunyoung. >> >> drivers/media/video/exynos/Kconfig | 1 + >> drivers/media/video/exynos/Makefile | 1 + >> drivers/media/video/exynos/rotator/Kconfig | 12 + >> drivers/media/video/exynos/rotator/Makefile | 9 + >> drivers/media/video/exynos/rotator/rotator-core.c | 1490 +++++++++++++++++++++ >> drivers/media/video/exynos/rotator/rotator-regs.c | 215 +++ >> drivers/media/video/exynos/rotator/rotator-regs.h | 70 + >> drivers/media/video/exynos/rotator/rotator.h | 325 +++++ >> 8 files changed, 2123 insertions(+), 0 deletions(-) >> create mode 100644 drivers/media/video/exynos/rotator/Kconfig >> create mode 100644 drivers/media/video/exynos/rotator/Makefile >> create mode 100644 drivers/media/video/exynos/rotator/rotator-core.c >> create mode 100644 drivers/media/video/exynos/rotator/rotator-regs.c >> create mode 100644 drivers/media/video/exynos/rotator/rotator-regs.h >> create mode 100644 drivers/media/video/exynos/rotator/rotator.h >> >> diff --git a/drivers/media/video/exynos/Kconfig b/drivers/media/video/exynos/Kconfig >> index a84097d..38a885d 100644 >> --- a/drivers/media/video/exynos/Kconfig >> +++ b/drivers/media/video/exynos/Kconfig >> @@ -12,6 +12,7 @@ config VIDEO_EXYNOS >> if VIDEO_EXYNOS >> source "drivers/media/video/exynos/mdev/Kconfig" >> source "drivers/media/video/exynos/fimc-lite/Kconfig" >> + source "drivers/media/video/exynos/rotator/Kconfig" > >Shouldn't you just include here > > source "drivers/media/video/exynos/Kconfig" > >and handle individual devices under drivers/media/video/exynos/Kconfig ? > We found that Kconfig uses like this in almost of sub-directory structure. >> endif >> >> config MEDIA_EXYNOS >> diff --git a/drivers/media/video/exynos/Makefile b/drivers/media/video/exynos/Makefile >> index 56cb7b2..24f19c5 100644 >> --- a/drivers/media/video/exynos/Makefile >> +++ b/drivers/media/video/exynos/Makefile >> @@ -1,4 +1,5 @@ >> obj-$(CONFIG_EXYNOS_MEDIA_DEVICE) += mdev/ >> obj-$(CONFIG_VIDEO_EXYNOS_FIMC_LITE) += fimc-lite/ >> +obj-$(CONFIG_VIDEO_EXYNOS_ROTATOR) += rotator/ >> >> EXTRA_CLAGS += -Idrivers/media/video >> diff --git a/drivers/media/video/exynos/rotator/Kconfig b/drivers/media/video/exynos/rotator/Kconfig >> new file mode 100644 >> index 0000000..332a997 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/Kconfig >> @@ -0,0 +1,12 @@ >> +config VIDEO_EXYNOS_ROTATOR >> + bool "EXYNOS Image Rotator Driver" >> + select V4L2_MEM2MEM_DEV >> + select VIDEOBUF2_DMA_CONTIG >> + default n >> + ---help--- >> + Support for Image Rotator Driver with v4l2 on EXYNOS SoCs >> + >> +config VIDEO_SAMSUNG_MEMSIZE_ROT >> + int "Memory size in kbytes for ROTATOR" >> + depends on VIDEO_EXYNOS_ROTATOR >> + default "9216" > >I think this should be removed, where VIDEO_SAMSUNG_MEMSIZE_ROT is used ? > OK. I will remove it. :) >> diff --git a/drivers/media/video/exynos/rotator/Makefile >b/drivers/media/video/exynos/rotator/Makefile >> new file mode 100644 >> index 0000000..6f74403 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/Makefile >> @@ -0,0 +1,9 @@ >> +# >> +# Copyright (c) 2012 Samsung Electronics Co., Ltd. >> +# http://www.samsung.com >> +# >> +# Licensed under GPLv2 >> +# >> + >> +rotator-objs := rotator-core.o rotator-regs.o >> +obj-$(CONFIG_VIDEO_EXYNOS_ROTATOR) += rotator.o >> diff --git a/drivers/media/video/exynos/rotator/rotator-core.c >b/drivers/media/video/exynos/rotator/rotator-core.c >> new file mode 100644 >> index 0000000..0c91196 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator-core.c >> @@ -0,0 +1,1490 @@ >> +/* >> + * Copyright (c) 2012 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Core file for Samsung EXYNOS Image Rotator driver >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> +*/ >> + >> +#include<linux/module.h> >> +#include<linux/kernel.h> >> +#include<linux/version.h> >> +#include<linux/platform_device.h> >> +#include<linux/interrupt.h> >> +#include<linux/clk.h> >> +#include<linux/slab.h> >> + >> +#include<media/v4l2-ioctl.h> >> +#include<media/videobuf2-dma-contig.h> >> + >> +#include "rotator.h" >> + >> +module_param_named(log_level, log_level, uint, 0644); >> + >> +static struct rot_fmt rot_formats[] = { >> + { >> + .name = "RGB565", >> + .pixelformat = V4L2_PIX_FMT_RGB565, >> + .num_planes = 1, >> + .nr_comp = 1, > >Isn't num_comp better, to be consistent with num_planes ? I agree it. It will be changed to num_planes. > >> + .bitperpixel = { 16 }, >> + }, { >> + .name = "XRGB-8888, 32 bps", >> + .pixelformat = V4L2_PIX_FMT_RGB32, >> + .num_planes = 1, >> + .nr_comp = 1, >> + .bitperpixel = { 32 }, >> + }, { >> + .name = "YUV 4:2:2 packed, YCbYCr", >> + .pixelformat = V4L2_PIX_FMT_YUYV, >> + .num_planes = 1, >> + .nr_comp = 1, >> + .bitperpixel = { 16 }, >> + }, { >> + .name = "YUV 4:2:0 non-contiguous 2-planar, Y/CbCr", >> + .pixelformat = V4L2_PIX_FMT_NV12M, >> + .num_planes = 2, >> + .nr_comp = 2, >> + .bitperpixel = { 8, 4 }, >> + }, { >> + .name = "YUV 4:2:0 non-contiguous 3-planar, Y/Cb/Cr", >> + .pixelformat = V4L2_PIX_FMT_YUV420M, >> + .num_planes = 3, >> + .nr_comp = 3, >> + .bitperpixel = { 8, 2, 2 }, >> + }, >> +}; >> + >> +static struct v4l2_queryctrl rot_ctrls[] = { >> + { >> + .id = V4L2_CID_HFLIP, >> + .type = V4L2_CTRL_TYPE_BOOLEAN, >> + .name = "Horizontal flip", >> + .minimum = 0, >> + .maximum = 1, >> + .default_value = 0, >> + }, { >> + .id = V4L2_CID_VFLIP, >> + .type = V4L2_CTRL_TYPE_BOOLEAN, >> + .name = "Vertical flip", >> + .minimum = 0, >> + .maximum = 1, >> + .default_value = 0, >> + }, { >> + .id = V4L2_CID_ROTATE, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .name = "Rotation", >> + .minimum = 0, >> + .maximum = 270, >> + .step = 90, >> + .default_value = 0, >> + }, >> +}; > >Please rework the driver to use the control framework, >Documentation/video4linux/v4l2-controls.txt > I forgot to mention about it. I've planned to use the control framework and it will be implemented soon. >> + >> +/* Find the matches format */ >> +static struct rot_fmt *rot_find_format(struct v4l2_format *f) >> +{ >> + struct rot_fmt *rot_fmt; >> + unsigned int i; >> + >> + for (i = 0; i< ARRAY_SIZE(rot_formats); ++i) { >> + rot_fmt =&rot_formats[i]; >> + if (rot_fmt->pixelformat == f->fmt.pix_mp.pixelformat) >> + return&rot_formats[i]; >> + } >> + >> + return NULL; >> +} >> + >> +static struct v4l2_queryctrl *rot_find_ctrl(int id) >> +{ >> + int i; >> + >> + for (i = 0; i< ARRAY_SIZE(rot_ctrls); ++i) { >> + if (id == rot_ctrls[i].id) >> + return&rot_ctrls[i]; >> + } >> + >> + return NULL; >> +} > >This function can be removed when you switch to the control framework. > OK. Use control framework instead of this. >> + >> +static void rot_bound_align_image(struct rot_ctx *ctx, struct rot_fmt *rot_fmt, >> + u32 *width, u32 *height) >> +{ >> + struct exynos_rot_variant *variant = ctx->rot_dev->variant; >> + struct exynos_rot_size_limit *limit = NULL; >> + >> + switch (rot_fmt->pixelformat) { >> + case V4L2_PIX_FMT_YUV420M: >> + limit =&variant->limit_yuv420_3p; >> + break; >> + case V4L2_PIX_FMT_NV12M: >> + limit =&variant->limit_yuv420_2p; >> + break; >> + case V4L2_PIX_FMT_YUYV: >> + limit =&variant->limit_yuv422; >> + break; >> + case V4L2_PIX_FMT_RGB565: >> + limit = &variant->limit_rgb565; >> + break; >> + case V4L2_PIX_FMT_RGB32: >> + limit =&variant->limit_rgb888; >> + break; >> + default: >> + rot_err("not supported format values\n"); > >Could please try to use the v4l2_* log helpers where possible, i.e. v4l2_err() >in this case ? > OK. I'll remove rot_err() and use v4l2_err() or dev_err(). >> + return; >> + } >> + >> + /* Bound an image to have width and height in limit */ >> + v4l_bound_align_image(width, limit->min_x, limit->max_x, >> + limit->align, height, limit->min_y, >> + limit->max_y, limit->align, 0); >> +} >> + >> +static void rot_adjust_pixminfo(struct rot_ctx *ctx, struct rot_frame *frame, >> + struct v4l2_pix_format_mplane *pixm) >> +{ >> + struct rot_frame *rot_frame; >> + >> + if (frame ==&ctx->s_frame) { >> + if (test_bit(CTX_DST,&ctx->flags)) { >> + rot_frame =&ctx->d_frame; >> + pixm->pixelformat = rot_frame->rot_fmt->pixelformat; >> + } >> + set_bit(CTX_SRC,&ctx->flags); >> + } else if (frame ==&ctx->d_frame) { >> + if (test_bit(CTX_SRC,&ctx->flags)) { >> + rot_frame =&ctx->s_frame; >> + pixm->pixelformat = rot_frame->rot_fmt->pixelformat; >> + } >> + set_bit(CTX_DST,&ctx->flags); >> + } >> +} >> + >> +static void rot_adjust_cropinfo(struct rot_ctx *ctx, struct rot_frame *frame, >> + struct v4l2_rect *crop) >> +{ >> + struct rot_frame *rot_frame; >> + >> + if (frame ==&ctx->s_frame) { >> + if (test_bit(CTX_DST,&ctx->flags)) { >> + rot_frame =&ctx->d_frame; >> + crop->width = rot_frame->crop.height; >> + crop->height = rot_frame->crop.width; >> + } >> + set_bit(CTX_SRC,&ctx->flags); >> + } else if (frame ==&ctx->d_frame) { >> + if (test_bit(CTX_SRC,&ctx->flags)) { >> + rot_frame =&ctx->s_frame; >> + crop->width = rot_frame->crop.height; >> + crop->height = rot_frame->crop.width; >> + } >> + set_bit(CTX_DST,&ctx->flags); >> + } >> +} >> + >> +static int rot_v4l2_querycap(struct file *file, void *priv, >> + struct v4l2_capability *cap) >> +{ >> + struct rot_ctx *ctx = file->private_data; >> + struct rot_dev *rot = ctx->rot_dev; >> + >> + strncpy(cap->driver, rot->pdev->name, sizeof(cap->driver) - 1); >> + strncpy(cap->card, rot->pdev->name, sizeof(cap->card) - 1); >> + >> + cap->bus_info[0] = 0; >> + cap->version = KERNEL_VERSION(MAJOR_VERSION, >> + MINOR_VERSION, >> + RELEASE_VERSION); > >You can remove this assignment, version is already initialized in v4l2-ioctl.c >to LINUX_KERNEL_VERSION. > OK. I'll remove it. >> + cap->capabilities = V4L2_CAP_STREAMING | >> + V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT_MPLANE; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_enum_fmt_mplane(struct file *file, void *priv, >> + struct v4l2_fmtdesc *f) >> +{ >> + struct rot_fmt *rot_fmt; >> + >> + if (f->index>= ARRAY_SIZE(rot_formats)) { >> + rot_err("invalid number of format\n"); > >Just simply return -EINVAL here, this will error log will be triggered >every time when application enumerates all formats. It's not any serious >sort of an error. > Yes, You're right, this log can be printed whenever enum_fmt is called. It'll be removed. >> + return -EINVAL; >> + } >> + >> + rot_fmt =&rot_formats[f->index]; >> + strncpy(f->description, rot_fmt->name, sizeof(f->description) - 1); >> + f->pixelformat = rot_fmt->pixelformat; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_g_fmt_mplane(struct file *file, void *priv, >> + struct v4l2_format *f) >> +{ >> + struct rot_ctx *ctx = priv; >> + struct rot_fmt *rot_fmt; >> + struct rot_frame *frame; >> + struct v4l2_pix_format_mplane *pixm =&f->fmt.pix_mp; >> + int i; >> + >> + frame = ctx_get_frame(ctx, f->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + rot_fmt = frame->rot_fmt; >> + >> + pixm->width = frame->pix_mp.width; >> + pixm->height = frame->pix_mp.height; >> + pixm->pixelformat = frame->pix_mp.pixelformat; >> + pixm->field = V4L2_FIELD_NONE; >> + pixm->num_planes = frame->rot_fmt->num_planes; >> + pixm->colorspace = 0; >> + >> + for (i = 0; i< pixm->num_planes; ++i) { >> + pixm->plane_fmt[i].bytesperline = (pixm->width * >> + rot_fmt->bitperpixel[i])>> 3; >> + pixm->plane_fmt[i].sizeimage = pixm->plane_fmt[i].bytesperline >> + * pixm->height; >> + >> + rot_dbg("[%d] plane: bytesperline %d, sizeimage %d\n", i, >> + pixm->plane_fmt[i].bytesperline, >> + pixm->plane_fmt[i].sizeimage); >> + } >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_try_fmt_mplane(struct file *file, void *priv, >> + struct v4l2_format *f) >> +{ >> + struct rot_ctx *ctx = priv; >> + struct rot_fmt *rot_fmt; >> + struct v4l2_pix_format_mplane *pixm =&f->fmt.pix_mp; >> + int i; >> + >> + if (!V4L2_TYPE_IS_MULTIPLANAR(f->type)) { >> + rot_err("not supported format type\n"); >> + return -EINVAL; >> + } >> + >> + rot_fmt = rot_find_format(f); >> + if (!rot_fmt) { >> + rot_err("not supported format values\n"); >> + return -EINVAL; >> + } >> + >> + rot_bound_align_image(ctx, rot_fmt,&pixm->width,&pixm->height); >> + >> + pixm->num_planes = rot_fmt->num_planes; >> + pixm->colorspace = 0; >> + >> + for (i = 0; i< pixm->num_planes; ++i) { >> + pixm->plane_fmt[i].bytesperline = (pixm->width * >> + rot_fmt->bitperpixel[i])>> 3; >> + pixm->plane_fmt[i].sizeimage = pixm->plane_fmt[i].bytesperline >> + * pixm->height; >> + >> + rot_dbg("[%d] plane: bytesperline %d, sizeimage %d\n", i, > >v4l2_dbg() ? > Um... Is v4l2_dbg() better than rot_dbg()? OK.. I'll change it to v4l2_dbg(). >> + pixm->plane_fmt[i].bytesperline, >> + pixm->plane_fmt[i].sizeimage); >> + } >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_s_fmt_mplane(struct file *file, void *priv, >> + struct v4l2_format *f) >> +{ >> + struct rot_ctx *ctx = priv; >> + struct vb2_queue *vq; > >How about assigning directly, > >struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); > >? OK. >> + struct rot_frame *frame; >> + struct v4l2_pix_format_mplane *pixm =&f->fmt.pix_mp; >> + int i, ret = 0; >> + >> + vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); >> + >> + if (vb2_is_streaming(vq)) { >> + rot_err("device is busy\n"); >> + return -EBUSY; >> + } >> + >> + ret = rot_v4l2_try_fmt_mplane(file, priv, f); >> + if (ret< 0) >> + return ret; >> + >> + frame = ctx_get_frame(ctx, f->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + set_bit(CTX_PARAMS,&ctx->flags); >> + >> + frame->rot_fmt = rot_find_format(f); >> + if (!frame->rot_fmt) { >> + rot_err("not supported format values\n"); >> + return -EINVAL; >> + } >> + >> + rot_adjust_pixminfo(ctx, frame, pixm); >> + >> + frame->pix_mp.pixelformat = pixm->pixelformat; >> + frame->pix_mp.width = pixm->width; >> + frame->pix_mp.height = pixm->height; >> + >> + /* >> + * Shouldn't call s_crop or g_crop before called g_fmt or s_fmt. >> + * Let's assume that we can keep the order. >> + */ >> + frame->crop.width = pixm->width; >> + frame->crop.height = pixm->height; >> + >> + for (i = 0; i< frame->rot_fmt->num_planes; ++i) >> + frame->bytesused[i] = (pixm->width * pixm->height * >> + frame->rot_fmt->bitperpixel[i])>> 3; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_reqbufs(struct file *file, void *priv, >> + struct v4l2_requestbuffers *reqbufs) >> +{ >> + struct rot_ctx *ctx = priv; >> + struct rot_frame *frame; >> + >> + frame = ctx_get_frame(ctx, reqbufs->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + if (frame ==&ctx->s_frame) >> + clear_bit(CTX_SRC,&ctx->flags); >> + else if (frame ==&ctx->d_frame) >> + clear_bit(CTX_DST,&ctx->flags); >> + >> + return v4l2_m2m_reqbufs(file, ctx->m2m_ctx, reqbufs); >> +} >> + >> +static int rot_v4l2_querybuf(struct file *file, void *priv, >> + struct v4l2_buffer *buf) >> +{ >> + struct rot_ctx *ctx = priv; >> + return v4l2_m2m_querybuf(file, ctx->m2m_ctx, buf); >> +} >> + >> +static int rot_v4l2_qbuf(struct file *file, void *priv, >> + struct v4l2_buffer *buf) >> +{ >> + struct rot_ctx *ctx = priv; >> + return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf); >> +} >> + >> +static int rot_v4l2_dqbuf(struct file *file, void *priv, >> + struct v4l2_buffer *buf) >> +{ >> + struct rot_ctx *ctx = priv; >> + return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf); >> +} >> + >> +static int rot_v4l2_streamon(struct file *file, void *priv, >> + enum v4l2_buf_type type) >> +{ >> + struct rot_ctx *ctx = priv; >> + return v4l2_m2m_streamon(file, ctx->m2m_ctx, type); >> +} >> + >> +static int rot_v4l2_streamoff(struct file *file, void *priv, >> + enum v4l2_buf_type type) >> +{ >> + struct rot_ctx *ctx = priv; >> + return v4l2_m2m_streamoff(file, ctx->m2m_ctx, type); >> +} >> + >> +static int rot_v4l2_queryctrl(struct file *file, void *priv, >> + struct v4l2_queryctrl *qc) >> +{ >> + struct v4l2_queryctrl *c; >> + >> + c = rot_find_ctrl(qc->id); >> + if (!c) { >> + rot_err("not supported control id\n"); >> + return -EINVAL; >> + } >> + *qc = *c; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_g_ctrl(struct file *file, void *priv, >> + struct v4l2_control *ctrl) >> +{ >> + struct rot_ctx *ctx = priv; >> + >> + switch (ctrl->id) { >> + case V4L2_CID_VFLIP: >> + ctrl->value = (ROT_VFLIP& ctx->flip) ? 1 : 0; >> + break; >> + case V4L2_CID_HFLIP: >> + ctrl->value = (ROT_HFLIP& ctx->flip) ? 1 : 0; >> + break; >> + case V4L2_CID_ROTATE: >> + ctrl->value = ctx->rotation; >> + break; >> + default: >> + rot_err("invalid control id\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int rot_check_ctrl_val(struct rot_ctx *ctx, struct v4l2_control *ctrl) >> +{ >> + struct v4l2_queryctrl *c; >> + >> + c = rot_find_ctrl(ctrl->id); >> + if (!c) { >> + rot_err("not supported control id\n"); >> + return -EINVAL; >> + } >> + >> + if (ctrl->value< c->minimum || ctrl->value> c->maximum >> + || ((c->step != 0)&& (ctrl->value % c->step != 0))) { >> + rot_err("not supported control value\n"); >> + return -ERANGE; >> + } >> + >> + return 0; >> +} > >These three functions are also not needed when you switch to the control framework. > OK. It'll be removed. >> + >> +static int rot_v4l2_s_ctrl(struct file *file, void *priv, >> + struct v4l2_control *ctrl) >> +{ >> + struct rot_ctx *ctx = file->private_data; >> + int ret; >> + >> + ret = rot_check_ctrl_val(ctx, ctrl); >> + if (ret) >> + return ret; >> + >> + switch (ctrl->id) { >> + case V4L2_CID_VFLIP: >> + if (ctrl->value) >> + ctx->flip |= ROT_VFLIP; >> + else >> + ctx->flip&= ~ROT_VFLIP; >> + break; >> + case V4L2_CID_HFLIP: >> + if (ctrl->value) >> + ctx->flip |= ROT_HFLIP; >> + else >> + ctx->flip&= ~ROT_HFLIP; >> + break; >> + case V4L2_CID_ROTATE: >> + ctx->rotation = ctrl->value; >> + break; >> + default: >> + rot_err("invalid control id\n"); >> + return -EINVAL; >> + } > >No need to protect ctx->flip, ctx->hflip, ctx->rotation with the spinlock ? > OK. I'll use spin_lock in this case. >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_cropcap(struct file *file, void *fh, >> + struct v4l2_cropcap *cr) >> +{ >> + struct rot_ctx *ctx = fh; >> + struct rot_frame *frame; >> + >> + frame = ctx_get_frame(ctx, cr->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + cr->bounds.left = 0; >> + cr->bounds.top = 0; >> + cr->bounds.width = frame->pix_mp.width; >> + cr->bounds.height = frame->pix_mp.height; >> + cr->defrect = cr->bounds; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_g_crop(struct file *file, void *fh, struct v4l2_crop *cr) >> +{ >> + struct rot_ctx *ctx = fh; >> + struct rot_frame *frame; >> + >> + frame = ctx_get_frame(ctx, cr->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + cr->c = frame->crop; >> + >> + return 0; >> +} >> + >> +static int rot_v4l2_s_crop(struct file *file, void *fh, struct v4l2_crop *cr) >> +{ >> + struct rot_ctx *ctx = fh; >> + struct rot_frame *frame; >> + struct v4l2_pix_format_mplane *pixm; >> + int i; >> + >> + frame = ctx_get_frame(ctx, cr->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + if (!test_bit(CTX_PARAMS,&ctx->flags)) { >> + rot_err("color format is not set\n"); >> + return -EINVAL; >> + } >> + >> + if (cr->c.left< 0 || cr->c.top< 0 || >> + cr->c.width< 0 || cr->c.height< 0) { >> + rot_err("crop value is negative\n"); >> + return -EINVAL; >> + } >> + >> + pixm =&frame->pix_mp; >> + rot_adjust_cropinfo(ctx, frame,&cr->c); >> + rot_bound_align_image(ctx, frame->rot_fmt,&cr->c.width,&cr->c.height); >> + >> + /* Adjust left/top if cropping rectangle is out of bounds */ >> + if (cr->c.left + cr->c.width> pixm->width) { >> + rot_warn("out of bound left cropping size:left %d, width %d\n", >> + cr->c.left, cr->c.width); >> + cr->c.left = pixm->width - cr->c.width; >> + } >> + if (cr->c.top + cr->c.height> pixm->height) { >> + rot_warn("out of bound top cropping size:top %d, height %d\n", >> + cr->c.top, cr->c.height); >> + cr->c.top = pixm->height - cr->c.height; >> + } >> + >> + frame->crop = cr->c; >> + >> + for (i = 0; i< frame->rot_fmt->num_planes; ++i) >> + frame->bytesused[i] = (cr->c.width * cr->c.height * >> + frame->rot_fmt->bitperpixel[i])>> 3; >> + >> + return 0; >> +} > >It would be nice to use to the selection API right away instead, here is some >example for a capture device: > >http://git.infradead.org/users/kmpark/linux-2.6- >samsung/commitdiff/06a208bf5925df449a79b600bd33954e1d31a1d3 > >I'm not quite sure right now if it is mandatory. > Um.. Currently it seems not mandatory and I think rotator doesn't need to it yet. >> + >> +static const struct v4l2_ioctl_ops rot_v4l2_ioctl_ops = { >> + .vidioc_querycap = rot_v4l2_querycap, >> + >> + .vidioc_enum_fmt_vid_cap_mplane = rot_v4l2_enum_fmt_mplane, >> + .vidioc_enum_fmt_vid_out_mplane = rot_v4l2_enum_fmt_mplane, >> + >> + .vidioc_g_fmt_vid_cap_mplane = rot_v4l2_g_fmt_mplane, >> + .vidioc_g_fmt_vid_out_mplane = rot_v4l2_g_fmt_mplane, >> + >> + .vidioc_try_fmt_vid_cap_mplane = rot_v4l2_try_fmt_mplane, >> + .vidioc_try_fmt_vid_out_mplane = rot_v4l2_try_fmt_mplane, >> + >> + .vidioc_s_fmt_vid_cap_mplane = rot_v4l2_s_fmt_mplane, >> + .vidioc_s_fmt_vid_out_mplane = rot_v4l2_s_fmt_mplane, >> + >> + .vidioc_reqbufs = rot_v4l2_reqbufs, >> + .vidioc_querybuf = rot_v4l2_querybuf, >> + >> + .vidioc_qbuf = rot_v4l2_qbuf, >> + .vidioc_dqbuf = rot_v4l2_dqbuf, >> + >> + .vidioc_streamon = rot_v4l2_streamon, >> + .vidioc_streamoff = rot_v4l2_streamoff, >> + >> + .vidioc_queryctrl = rot_v4l2_queryctrl, >> + .vidioc_g_ctrl = rot_v4l2_g_ctrl, >> + .vidioc_s_ctrl = rot_v4l2_s_ctrl, >> + >> + .vidioc_g_crop = rot_v4l2_g_crop, >> + .vidioc_s_crop = rot_v4l2_s_crop, >> + .vidioc_cropcap = rot_v4l2_cropcap >> +}; >> + >> +static int rot_ctx_stop_req(struct rot_ctx *ctx) >> +{ >> + struct rot_ctx *curr_ctx; >> + struct rot_dev *rot = ctx->rot_dev; >> + int ret = 0; >> + >> + curr_ctx = v4l2_m2m_get_curr_priv(rot->m2m.m2m_dev); >> + if (!test_bit(CTX_RUN,&ctx->flags) || (curr_ctx != ctx)) >> + return 0; >> + >> + set_bit(CTX_ABORT,&ctx->flags); >> + >> + ret = wait_event_timeout(rot->irq.wait, >> + !test_bit(CTX_RUN,&ctx->flags), ROT_TIMEOUT); >> + >> + /* TODO: How to handle case of timeout event */ >> + if (!ret) { >> + rot_err("device failed to stop request\n"); >> + ret = -EBUSY; >> + } >> + >> + return ret; >> +} >> + >> +static int rot_vb2_queue_setup(struct vb2_queue *vq, >> + const struct v4l2_format *fmt, unsigned int *num_buffers, >> + unsigned int *num_planes, unsigned int sizes[], >> + void *allocators[]) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vq); >> + struct rot_frame *frame; >> + int i; >> + >> + frame = ctx_get_frame(ctx, vq->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + /* Get number of planes from format_list in driver */ >> + *num_planes = frame->rot_fmt->num_planes; >> + for (i = 0; i< frame->rot_fmt->num_planes; i++) { >> + sizes[i] = (frame->pix_mp.width * frame->pix_mp.height * >> + frame->rot_fmt->bitperpixel[i])>> 3; >> + allocators[i] = ctx->rot_dev->alloc_ctx; >> + } >> + >> + return 0; >> +} >> + >> +static int rot_vb2_buf_prepare(struct vb2_buffer *vb) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); >> + struct rot_frame *frame; >> + int i; >> + >> + frame = ctx_get_frame(ctx, vb->vb2_queue->type); >> + if (IS_ERR(frame)) >> + return PTR_ERR(frame); >> + >> + if (!V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type)) { >> + for (i = 0; i< frame->rot_fmt->num_planes; i++) >> + vb2_set_plane_payload(vb, i, frame->bytesused[i]); >> + } >> + >> + return 0; >> +} >> + >> +static void rot_vb2_buf_queue(struct vb2_buffer *vb) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); >> + >> + if (ctx->m2m_ctx) >> + v4l2_m2m_buf_queue(ctx->m2m_ctx, vb); >> +} >> + >> +static void rot_vb2_lock(struct vb2_queue *vq) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vq); >> + mutex_lock(&ctx->rot_dev->lock); >> +} >> + >> +static void rot_vb2_unlock(struct vb2_queue *vq) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vq); >> + mutex_unlock(&ctx->rot_dev->lock); >> +} >> + >> +static int rot_vb2_start_streaming(struct vb2_queue *vq, unsigned int count) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vq); >> + set_bit(CTX_STREAMING,&ctx->flags); >> + >> + return 0; >> +} >> + >> +static int rot_vb2_stop_streaming(struct vb2_queue *vq) >> +{ >> + struct rot_ctx *ctx = vb2_get_drv_priv(vq); >> + int ret; >> + >> + ret = rot_ctx_stop_req(ctx); >> + if (ret< 0) >> + rot_err("wait timeout\n"); >> + >> + clear_bit(CTX_STREAMING,&ctx->flags); >> + >> + return ret; >> +} >> + >> +static struct vb2_ops rot_vb2_ops = { >> + .queue_setup = rot_vb2_queue_setup, >> + .buf_prepare = rot_vb2_buf_prepare, >> + .buf_queue = rot_vb2_buf_queue, >> + .wait_finish = rot_vb2_lock, >> + .wait_prepare = rot_vb2_unlock, >> + .start_streaming = rot_vb2_start_streaming, >> + .stop_streaming = rot_vb2_stop_streaming, >> +}; >> + >> +static int queue_init(void *priv, struct vb2_queue *src_vq, >> + struct vb2_queue *dst_vq) >> +{ >> + struct rot_ctx *ctx = priv; >> + int ret; >> + >> + memset(src_vq, 0, sizeof(*src_vq)); >> + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; >> + src_vq->io_modes = VB2_MMAP | VB2_USERPTR; >> + src_vq->ops =&rot_vb2_ops; >> + src_vq->mem_ops =&vb2_dma_contig_memops; >> + src_vq->drv_priv = ctx; >> + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); >> + >> + ret = vb2_queue_init(src_vq); >> + if (ret) >> + return ret; >> + >> + memset(dst_vq, 0, sizeof(*dst_vq)); >> + dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> + dst_vq->io_modes = VB2_MMAP | VB2_USERPTR; >> + dst_vq->ops =&rot_vb2_ops; >> + dst_vq->mem_ops =&vb2_dma_contig_memops; >> + dst_vq->drv_priv = ctx; >> + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); >> + >> + return vb2_queue_init(dst_vq); >> +} >> + >> +static int rot_open(struct file *file) >> +{ >> + struct rot_dev *rot = video_drvdata(file); >> + struct rot_ctx *ctx = NULL; >> + >> + ctx = kzalloc(sizeof *ctx, GFP_KERNEL); >> + if (!ctx) { >> + rot_err("no memory for open context\n"); >> + return -ENOMEM; >> + } >> + >> + atomic_inc(&rot->m2m.in_use); >> + >> + file->private_data = ctx; >> + ctx->rot_dev = rot; >> + >> + /* Default color format */ >> + ctx->s_frame.rot_fmt =&rot_formats[0]; >> + ctx->d_frame.rot_fmt =&rot_formats[0]; >> + spin_lock_init(&ctx->slock); >> + >> + /* Setup the device context for mem2mem mode. */ >> + ctx->m2m_ctx = v4l2_m2m_ctx_init(rot->m2m.m2m_dev, ctx, queue_init); >> + if (IS_ERR(ctx->m2m_ctx)) { >> + kfree(ctx); >> + atomic_dec(&rot->m2m.in_use); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int rot_release(struct file *file) >> +{ >> + struct rot_ctx *ctx = file->private_data; >> + struct rot_dev *rot = ctx->rot_dev; >> + >> + rot_dbg("refcnt= %d", atomic_read(&rot->m2m.in_use)); >> + >> + v4l2_m2m_ctx_release(ctx->m2m_ctx); >> + kfree(ctx); >> + >> + atomic_dec(&rot->m2m.in_use); >> + >> + return 0; >> +} >> + >> +static unsigned int rot_poll(struct file *file, >> + struct poll_table_struct *wait) >> +{ >> + struct rot_ctx *ctx = file->private_data; >> + >> + return v4l2_m2m_poll(file, ctx->m2m_ctx, wait); >> +} >> + >> +static int rot_mmap(struct file *file, struct vm_area_struct *vma) >> +{ >> + struct rot_ctx *ctx = file->private_data; >> + >> + return v4l2_m2m_mmap(file, ctx->m2m_ctx, vma); >> +} >> + >> +static const struct v4l2_file_operations rot_v4l2_fops = { >> + .owner = THIS_MODULE, >> + .open = rot_open, >> + .release = rot_release, >> + .poll = rot_poll, >> + .unlocked_ioctl = video_ioctl2, >> + .mmap = rot_mmap, >> +}; >> + >> +static void rot_work(struct work_struct *work) >> +{ >> + struct rot_dev *rot = container_of(work, struct rot_dev, ws); >> + struct rot_ctx *ctx; >> + unsigned long flags; >> + struct vb2_buffer *src_vb, *dst_vb; >> + >> + spin_lock_irqsave(&rot->slock, flags); >> + >> + if (atomic_read(&rot->wdt.cnt)>= ROT_WDT_CNT) { >> + rot_dbg("wakeup blocked process\n"); >> + ctx = v4l2_m2m_get_curr_priv(rot->m2m.m2m_dev); >> + if (!ctx || !ctx->m2m_ctx) { >> + rot_err("current ctx is NULL\n"); >> + goto wq_unlock; >> + } >> + src_vb = v4l2_m2m_src_buf_remove(ctx->m2m_ctx); >> + dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx); >> + >> + if (src_vb&& dst_vb) { >> + v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_ERROR); >> + v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_ERROR); >> + >> + v4l2_m2m_job_finish(rot->m2m.m2m_dev, ctx->m2m_ctx); >> + } >> + rot->m2m.ctx = NULL; >> + atomic_set(&rot->wdt.cnt, 0); >> + clear_bit(DEV_RUN,&rot->state); >> + clear_bit(CTX_RUN,&ctx->flags); >> + } >> + >> +wq_unlock: >> + spin_unlock_irqrestore(&rot->slock, flags); >> + >> + pm_runtime_put(&rot->pdev->dev); >> +} >> + >> +static void rot_watchdog(unsigned long arg) >> +{ >> + struct rot_dev *rot = (struct rot_dev *)arg; >> + >> + rot_dbg("timeout watchdog\n"); >> + if (atomic_read(&rot->wdt.cnt)>= ROT_WDT_CNT) { >> + queue_work(rot->wq,&rot->ws); >> + return; >> + } >> + >> + if (test_bit(DEV_RUN,&rot->state)) { >> + atomic_inc(&rot->wdt.cnt); >> + rot_err("rotator is still running\n"); >> + rot->wdt.timer.expires = jiffies + ROT_TIMEOUT; >> + add_timer(&rot->wdt.timer); >> + } else { >> + rot_dbg("rotator finished job\n"); >> + } >> +} >> + >> +static irqreturn_t rot_irq_handler(int irq, void *priv) >> +{ >> + struct rot_dev *rot = priv; >> + struct rot_ctx *ctx; >> + struct vb2_buffer *src_vb, *dst_vb; >> + unsigned int irq_src; >> + >> + spin_lock(&rot->slock); >> + >> + clear_bit(DEV_RUN,&rot->state); >> + if (timer_pending(&rot->wdt.timer)) >> + del_timer(&rot->wdt.timer); >> + >> + rot_hwget_irq_src(rot,&irq_src); >> + rot_hwset_irq_clear(rot,&irq_src); >> + >> + if (irq_src != ISR_PEND_DONE) { >> + rot_err("####################\n"); >> + rot_err("set SFR illegally\n"); >> + rot_err("maybe the result is wrong\n"); >> + rot_err("####################\n"); >> + rot_dump_register(rot); >> + } >> + >> + ctx = v4l2_m2m_get_curr_priv(rot->m2m.m2m_dev); >> + if (!ctx || !ctx->m2m_ctx) { >> + rot_err("current ctx is NULL\n"); >> + goto isr_unlock; >> + } >> + >> + clear_bit(CTX_RUN,&ctx->flags); >> + rot->m2m.ctx = NULL; >> + >> + src_vb = v4l2_m2m_src_buf_remove(ctx->m2m_ctx); >> + dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx); >> + >> + if (src_vb&& dst_vb) { >> + v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE); >> + v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_DONE); >> + >> + if (test_bit(DEV_SUSPEND,&rot->state)) { >> + rot_dbg("wake up blocked process by suspend\n"); >> + wake_up(&rot->irq.wait); >> + } else { >> + v4l2_m2m_job_finish(rot->m2m.m2m_dev, ctx->m2m_ctx); >> + } >> + >> + /* Wake up from CTX_ABORT state */ >> + if (test_and_clear_bit(CTX_ABORT,&ctx->flags)) >> + wake_up(&rot->irq.wait); >> + >> + queue_work(rot->wq,&rot->ws); >> + } else { >> + rot_err("failed to get the buffer done\n"); >> + } >> + >> +isr_unlock: >> + spin_unlock(&rot->slock); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static void rot_get_bufaddr(struct rot_dev *rot, struct vb2_buffer *vb, >> + struct rot_frame *frame, struct rot_addr *addr) >> +{ >> + unsigned int pix_size; >> + >> + pix_size = frame->pix_mp.width * frame->pix_mp.height; >> + >> + addr->y = vb2_dma_contig_plane_dma_addr(vb, 0); >> + addr->cb = 0; >> + addr->cr = 0; >> + >> + switch (frame->rot_fmt->nr_comp) { >> + case 2: >> + if (frame->rot_fmt->num_planes == 1) >> + addr->cb = addr->y + pix_size; >> + else if (frame->rot_fmt->num_planes == 2) >> + addr->cb = vb2_dma_contig_plane_dma_addr(vb, 1); >> + break; >> + case 3: >> + if (frame->rot_fmt->num_planes == 3) { >> + addr->cb = vb2_dma_contig_plane_dma_addr(vb, 1); >> + addr->cr = vb2_dma_contig_plane_dma_addr(vb, 2); >> + } >> + break; >> + default: >> + break; >> + } >> +} >> + >> +static void rot_set_frame_addr(struct rot_ctx *ctx) >> +{ >> + struct vb2_buffer *vb; >> + struct rot_frame *s_frame, *d_frame; >> + struct rot_dev *rot = ctx->rot_dev; > >Didn't you consider putting the local variable declarations in descending >line length order ? It usually looks better. > I didn't care about it :) but I'll try it later.. >> + >> + s_frame =&ctx->s_frame; >> + d_frame =&ctx->d_frame; >> + >> + /* set source buffer address */ >> + vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx); >> + rot_get_bufaddr(rot, vb, s_frame,&s_frame->addr); >> + >> + rot_hwset_src_addr(rot, s_frame->addr.y, ROT_ADDR_Y); >> + rot_hwset_src_addr(rot, s_frame->addr.cb, ROT_ADDR_CB); >> + rot_hwset_src_addr(rot, s_frame->addr.cr, ROT_ADDR_CR); >> + >> + /* set destination buffer address */ >> + vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx); >> + rot_get_bufaddr(rot, vb, d_frame,&d_frame->addr); >> + >> + rot_hwset_dst_addr(rot, d_frame->addr.y, ROT_ADDR_Y); >> + rot_hwset_dst_addr(rot, d_frame->addr.cb, ROT_ADDR_CB); >> + rot_hwset_dst_addr(rot, d_frame->addr.cr, ROT_ADDR_CR); >> +} >> + >> +static void rot_mapping_flip(struct rot_ctx *ctx, u32 *degree, u32 *flip) >> +{ >> + *degree = ctx->rotation; >> + *flip = ctx->flip; >> + >> + if (ctx->flip == (ROT_VFLIP | ROT_HFLIP)) { >> + *flip = ROT_NOFLIP; >> + switch (ctx->rotation) { >> + case 0: >> + *degree = 180; >> + break; >> + case 90: >> + *degree = 270; >> + break; >> + case 180: >> + *degree = 0; >> + break; >> + case 270: >> + *degree = 90; >> + break; >> + } >> + } >> +} >> + >> +static void rot_m2m_device_run(void *priv) >> +{ >> + struct rot_ctx *ctx = priv; >> + struct rot_frame *s_frame, *d_frame; >> + struct rot_dev *rot; >> + unsigned long flags, tmp; >> + u32 degree = 0, flip = 0; >> + >> + spin_lock_irqsave(&ctx->slock, flags); >> + >> + rot = ctx->rot_dev; >> + >> + if (test_bit(DEV_RUN,&rot->state)) { >> + rot_err("Rotate is already in progress\n"); >> + goto run_unlock; >> + } >> + >> + if (test_bit(DEV_SUSPEND,&rot->state)) { >> + rot_err("Rotate is in suspend state\n"); >> + goto run_unlock; >> + } >> + >> + if (test_bit(CTX_ABORT,&ctx->flags)) { >> + rot_dbg("aborted rot device run\n"); >> + goto run_unlock; >> + } >> + >> + pm_runtime_get_sync(&ctx->rot_dev->pdev->dev); >> + >> + if (rot->m2m.ctx != ctx) > >What is this check for ? It doesn't make sense when all you do in >rot->m2m.ctx != ctx case is assigning ctx's value to rot->m2m.ctx. > OK. I'll remove it. >> + rot->m2m.ctx = ctx; >> + >> + s_frame =&ctx->s_frame; >> + d_frame =&ctx->d_frame; >> + >> + /* Configuration rotator registers */ >> + rot_hwset_image_format(rot, s_frame->rot_fmt->pixelformat); >> + rot_mapping_flip(ctx,°ree,&flip); >> + rot_hwset_flip(rot, flip); >> + rot_hwset_rotation(rot, degree); >> + >> + if (ctx->rotation == 90 || ctx->rotation == 270) { >> + tmp = d_frame->pix_mp.height; >> + d_frame->pix_mp.height = d_frame->pix_mp.width; >> + d_frame->pix_mp.width = tmp; >> + } >> + >> + rot_hwset_src_imgsize(rot, s_frame); >> + rot_hwset_dst_imgsize(rot, d_frame); >> + >> + rot_hwset_src_crop(rot,&s_frame->crop); >> + rot_hwset_dst_crop(rot,&d_frame->crop); >> + >> + rot_set_frame_addr(ctx); >> + >> + /* Enable rotator interrupt */ >> + rot_hwset_irq_frame_done(rot, 1); >> + rot_hwset_irq_illegal_config(rot, 1); > >w00t ? :-) Why are you setting illegal configuration ? ;-) > It means enabling interrupt (irq) for illegal configuration, not setting illegal config. :) >> + >> + set_bit(DEV_RUN,&rot->state); >> + set_bit(CTX_RUN,&ctx->flags); >> + >> + /* Start rotate operation */ >> + rot_hwset_start(rot); >> + >> + /* Start watchdog timer */ >> + rot->wdt.timer.expires = jiffies + ROT_TIMEOUT; >> + if (timer_pending(&rot->wdt.timer) == 0) >> + add_timer(&rot->wdt.timer); >> + else >> + mod_timer(&rot->wdt.timer, rot->wdt.timer.expires); >> + >> +run_unlock: >> + spin_unlock_irqrestore(&ctx->slock, flags); >> +} >> + >> +static void rot_m2m_job_abort(void *priv) >> +{ >> + struct rot_ctx *ctx = priv; >> + int ret; >> + >> + ret = rot_ctx_stop_req(ctx); >> + if (ret< 0) >> + rot_err("wait timeout\n"); >> +} >> + >> +static struct v4l2_m2m_ops rot_m2m_ops = { >> + .device_run = rot_m2m_device_run, >> + .job_abort = rot_m2m_job_abort, >> +}; >> + >> +static int rot_register_m2m_device(struct rot_dev *rot) >> +{ >> + struct v4l2_device *v4l2_dev; >> + struct platform_device *pdev; >> + struct video_device *vfd; >> + int ret = 0; >> + >> + if (!rot) >> + return -ENODEV; >> + >> + pdev = rot->pdev; >> + v4l2_dev =&rot->m2m.v4l2_dev; >> + >> + /* Set name to "device name.m2m" if it is empty */ >> + if (!v4l2_dev->name[0]) > >Please drop this check, it's useless. > >> + snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), >> + "%s.m2m", dev_name(&pdev->dev)); > >We will be in trouble using dev_name(&pdev->dev) when trying to migrate >to the device tree. Maybe it's better to use some driver hard coded name >right away, just only parametrized with the rotator hardware instance index. > Ok.. I'll try to use hard coded name for future. >> + >> + ret = v4l2_device_register(&pdev->dev, v4l2_dev); >> + if (ret) { >> + rot_err("failed to register v4l2 device\n"); >> + return ret; >> + } >> + >> + vfd = video_device_alloc(); >> + if (!vfd) { >> + rot_err("failed to allocate video device\n"); >> + goto err_v4l2_dev; >> + } >> + >> + vfd->fops =&rot_v4l2_fops; >> + vfd->ioctl_ops =&rot_v4l2_ioctl_ops; >> + vfd->release = video_device_release; >> + >> + video_set_drvdata(vfd, rot); >> + platform_set_drvdata(pdev, rot); >> + >> + rot->m2m.vfd = vfd; >> + rot->m2m.m2m_dev = v4l2_m2m_init(&rot_m2m_ops); >> + if (IS_ERR(rot->m2m.m2m_dev)) { >> + rot_err("failed to initialize v4l2-m2m device\n"); >> + ret = PTR_ERR(rot->m2m.m2m_dev); >> + goto err_dev_alloc; >> + } >> + >> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1); >> + if (ret) { >> + rot_err("failed to register video device\n"); >> + goto err_m2m_dev; >> + } >> + >> + return 0; >> + >> +err_m2m_dev: >> + v4l2_m2m_release(rot->m2m.m2m_dev); >> +err_dev_alloc: >> + video_device_release(rot->m2m.vfd); >> +err_v4l2_dev: >> + v4l2_device_unregister(v4l2_dev); >> + >> + return ret; >> +} >> + >> +static int rot_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev; >> + struct rot_dev *rot; >> + struct rot_ctx *ctx; >> + int ret = 0; >> + >> + pdev = to_platform_device(dev); >> + rot = (struct rot_dev *)platform_get_drvdata(pdev); > >You can replace that with > > rot = dev_get_drvdata(dev); > >and remove pdev completely. > It's so simple and pdev will be removed. >> + >> + set_bit(DEV_SUSPEND,&rot->state); >> + >> + ret = wait_event_timeout(rot->irq.wait, >> + !test_bit(DEV_RUN,&rot->state), >> + ROT_TIMEOUT); >> + if (!ret) >> + rot_err("wait timeout\n"); >> + >> + ctx = rot->m2m.ctx; >> + if (ctx != NULL) >> + set_bit(CTX_SUSPEND,&ctx->flags); >> + >> + return ret; >> +} >> + >> +static int rot_resume(struct device *dev) >> +{ >> + struct platform_device *pdev; >> + struct rot_dev *rot; >> + struct rot_ctx *ctx; >> + >> + pdev = to_platform_device(dev); >> + rot = (struct rot_dev *)platform_get_drvdata(pdev); > >Ditto. > OK. >> + >> + clear_bit(DEV_SUSPEND,&rot->state); >> + >> + ctx = rot->m2m.ctx; >> + if (ctx != NULL) { >> + clear_bit(CTX_SUSPEND,&ctx->flags); >> + rot->m2m.ctx = NULL; >> + v4l2_m2m_job_finish(rot->m2m.m2m_dev, ctx->m2m_ctx); >> + } >> + >> + return 0; >> +} >> + >> +static int rot_runtime_suspend(struct device *dev) >> +{ >> + struct rot_dev *rot; >> + struct platform_device *pdev; >> + >> + pdev = to_platform_device(dev); >> + rot = (struct rot_dev *)platform_get_drvdata(pdev); >> + >> + clk_disable(rot->clock); >> + >> + return 0; >> +} >> + >> +static int rot_runtime_resume(struct device *dev) >> +{ >> + struct rot_dev *rot; >> + struct platform_device *pdev; >> + >> + pdev = to_platform_device(dev); >> + rot = (struct rot_dev *)platform_get_drvdata(pdev); >> + >> + clk_enable(rot->clock); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops rot_pm_ops = { >> + .suspend = rot_suspend, >> + .resume = rot_resume, >> + .runtime_suspend = rot_runtime_suspend, >> + .runtime_resume = rot_runtime_resume, >> +}; >> + >> +static int rot_probe(struct platform_device *pdev) >> +{ >> + struct exynos_rot_driverdata *drv_data; >> + struct rot_dev *rot; >> + struct resource *res; >> + int variant_num, ret = 0; >> + >> + printk(KERN_INFO "++%s\n", __func__); > >dev_info(), if you really need that. > OK.. I'll change it. >> + >> + drv_data = (struct exynos_rot_driverdata *) >> + platform_get_device_id(pdev)->driver_data; >> + >> + if (pdev->id>= drv_data->nr_dev) { >> + pr_err("Invalid platform device id\n"); >> + return -EINVAL; >> + } >> + >> + rot = kzalloc(sizeof(struct rot_dev), GFP_KERNEL); > >You can use device managed resources, it will significantly simplify your >error handling and overall code below. Here is some example: > >http://git.infradead.org/users/kmpark/linux-2.6- >samsung/commitdiff/154ae8a99869da241128139e004bdec60b190b43 > It's good for error handling as you mentioned. Thanks for your guide. >> + if (!rot) { >> + pr_err("no memory for rotator device\n"); >> + return -ENOMEM; >> + } >> + >> + rot->pdev = pdev; >> + rot->id = pdev->id; >> + variant_num = (rot->id< 0) ? 0 : rot->id; >> + rot->variant = drv_data->variant[variant_num]; >> + >> + spin_lock_init(&rot->slock); >> + >> + /* Get memory resource and map SFR region. */ >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + rot_err("failed to find the registers\n"); >> + ret = -ENOENT; >> + goto err_dev; >> + } >> + >> + rot->regs_res = request_mem_region(res->start, resource_size(res), >> + dev_name(&pdev->dev)); >> + if (!rot->regs_res) { >> + rot_err("failed to claim register region\n"); >> + ret = -ENOENT; >> + goto err_dev; >> + } >> + >> + rot->regs = ioremap(res->start, resource_size(res)); >> + if (!rot->regs) { >> + rot_err("failed to map register\n"); >> + ret = -ENXIO; >> + goto err_req_region; >> + } >> + >> + /* Get IRQ resource and register IRQ handler. */ >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + if (!res) { >> + rot_err("failed to get IRQ resource\n"); >> + ret = -ENXIO; >> + goto err_ioremap; >> + } >> + rot->irq.num = res->start; >> + >> + ret = request_irq(rot->irq.num, rot_irq_handler, 0, >> + pdev->name, rot); >> + if (ret) { >> + rot_err("failed to install irq(%d)\n", ret); >> + goto err_ioremap; >> + } >> + >> + rot->wq = create_singlethread_workqueue(MODULE_NAME); > >I'm curious, why do you need a workqueue and timers together ? > It needs the workqueue to call runtime_pm_put_sync() shared in both interrupt handler and timeout by timer. But I'll remove the workqueue and call runtime_pm_put() directly. >> + if (rot->wq == NULL) { >> + rot_err("failed to create workqueue for rotator\n"); >> + ret = -EPERM; >> + goto err_irq; >> + } >> + INIT_WORK(&rot->ws, rot_work); >> + >> + atomic_set(&rot->wdt.cnt, 0); >> + setup_timer(&rot->wdt.timer, rot_watchdog, (unsigned long)rot); >> + >> + rot->clock = clk_get(&rot->pdev->dev, "rotator"); >> + if (IS_ERR(rot->clock)) { >> + rot_err("failed to get clock for rotator\n"); >> + ret = -ENXIO; >> + goto err_wq; >> + } >> + >> + rot->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev); >> + if (IS_ERR(rot->alloc_ctx)) { >> + rot_err("failed to get alloc_ctx\n"); >> + ret = -EPERM; >> + goto err_clk; >> + } >> + >> + ret = rot_register_m2m_device(rot); >> + if (ret) { >> + rot_err("failed to register m2m device\n"); >> + ret = -EPERM; >> + goto err_clk; >> + } >> + >> +#ifdef CONFIG_PM_RUNTIME >> + pm_runtime_enable(&pdev->dev); >> +#else >> + rot_runtime_resume(&pdev->dev); >> +#endif >> + >> + rot_info("rotator registered successfully\n"); > >dev_info() > OK. >> + >> + return 0; >> + >> +err_clk: >> + clk_put(rot->clock); >> +err_wq: >> + destroy_workqueue(rot->wq); >> +err_irq: >> + free_irq(rot->irq.num, rot); >> +err_ioremap: >> + iounmap(rot->regs); >> +err_req_region: >> + release_mem_region(rot->regs_res->start, >> + resource_size(rot->regs_res)); >> +err_dev: >> + kfree(rot); >> + >> + return ret; >> +} >> + >> +static int rot_remove(struct platform_device *pdev) >> +{ >> + struct rot_dev *rot = (struct rot_dev *)platform_get_drvdata(pdev); >> + >> + free_irq(rot->irq.num, rot); >> + clk_put(rot->clock); >> +#ifdef CONFIG_PM_RUNTIME >> + pm_runtime_disable(&pdev->dev); >> +#else >> + rot_runtime_suspend(&pdev->dev); >> +#endif >> + >> + if (timer_pending(&rot->wdt.timer)) >> + del_timer(&rot->wdt.timer); >> + >> + destroy_workqueue(rot->wq); >> + iounmap(rot->regs); >> + release_mem_region(rot->regs_res->start, >> + resource_size(rot->regs_res)); >> + >> + kfree(rot); >> + >> + return 0; >> +} >> + >> +static struct exynos_rot_variant rot_variant_exynos = { >> + .limit_rgb565 = { >> + .min_x = 16, >> + .min_y = 16, >> + .max_x = SZ_16K, >> + .max_y = SZ_16K, >> + .align = 2, >> + }, >> + .limit_rgb888 = { >> + .min_x = 8, >> + .min_y = 8, >> + .max_x = SZ_8K, >> + .max_y = SZ_8K, >> + .align = 2, >> + }, >> + .limit_yuv422 = { >> + .min_x = 16, >> + .min_y = 16, >> + .max_x = SZ_16K, >> + .max_y = SZ_16K, >> + .align = 2, >> + }, >> + .limit_yuv420_2p = { >> + .min_x = 32, >> + .min_y = 32, >> + .max_x = SZ_32K, >> + .max_y = SZ_32K, >> + .align = 3, >> + }, >> + .limit_yuv420_3p = { >> + .min_x = 64, >> + .min_y = 32, >> + .max_x = SZ_32K, >> + .max_y = SZ_32K, >> + .align = 4, >> + }, >> +}; >> + >> +static struct exynos_rot_driverdata rot_drvdata_exynos = { >> + .variant = { >> + [0] =&rot_variant_exynos, >> + }, >> + .nr_dev = 1, >> +}; >> + >> +static struct platform_device_id rot_driver_ids[] = { >> + { >> + .name = "exynos-rot", >> + .driver_data = (unsigned long)&rot_drvdata_exynos, >> + }, >> + {}, >> +}; >> + >> +static struct platform_driver rot_driver = { >> + .probe = rot_probe, >> + .remove = rot_remove, >> + .id_table = rot_driver_ids, >> + .driver = { >> + .name = MODULE_NAME, >> + .owner = THIS_MODULE, >> + .pm =&rot_pm_ops, >> + } >> +}; >> + >> +static int __init rot_init(void) >> +{ >> + return platform_driver_register(&rot_driver); >> +} >> + >> +static void __exit rot_exit(void) >> +{ >> + platform_driver_unregister(&rot_driver); >> +} >> + >> +module_init(rot_init); >> +module_exit(rot_exit); > >module_platform_driver() > OK. >> + >> +MODULE_AUTHOR("Sunyoung, Kang<sy0816.kang@xxxxxxxxxxx>"); >> +MODULE_AUTHOR("Ayoung, Sim<a.sim@xxxxxxxxxxx>"); >> +MODULE_DESCRIPTION("Exynos Image Rotator driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/media/video/exynos/rotator/rotator-regs.c >b/drivers/media/video/exynos/rotator/rotator-regs.c >> new file mode 100644 >> index 0000000..8eadad2 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator-regs.c >> @@ -0,0 +1,215 @@ >> +/* >> + * Copyright (c) 2012 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Register interface file for Exynos Rotator driver >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> +*/ >> + >> +#include "rotator.h" >> + >> +void rot_hwset_irq_frame_done(struct rot_dev *rot, u32 enable) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONFIG); >> + >> + if (enable) >> + cfg |= ROTATOR_CONFIG_IRQ_DONE; >> + else >> + cfg&= ~ROTATOR_CONFIG_IRQ_DONE; >> + >> + writel(cfg, rot->regs + ROTATOR_CONFIG); >> +} >> + >> +void rot_hwset_irq_illegal_config(struct rot_dev *rot, u32 enable) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONFIG); >> + >> + if (enable) >> + cfg |= ROTATOR_CONFIG_IRQ_ILLEGAL; >> + else >> + cfg&= ~ROTATOR_CONFIG_IRQ_ILLEGAL; >> + >> + writel(cfg, rot->regs + ROTATOR_CONFIG); >> +} >> + >> +int rot_hwset_image_format(struct rot_dev *rot, u32 pixelformat) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONTROL); >> + cfg&= ~ROTATOR_CONTROL_FMT_MASK; >> + >> + switch (pixelformat) { >> + case V4L2_PIX_FMT_YUV420M: >> + cfg |= ROTATOR_CONTROL_FMT_YCBCR420_3P; >> + break; >> + case V4L2_PIX_FMT_NV12M: >> + cfg |= ROTATOR_CONTROL_FMT_YCBCR420_2P; >> + break; >> + case V4L2_PIX_FMT_YUYV: >> + cfg |= ROTATOR_CONTROL_FMT_YCBCR422; >> + break; >> + case V4L2_PIX_FMT_RGB565: >> + cfg |= ROTATOR_CONTROL_FMT_RGB565; >> + break; >> + case V4L2_PIX_FMT_RGB32: >> + cfg |= ROTATOR_CONTROL_FMT_RGB888; >> + break; >> + default: >> + rot_err("invalid pixelformat type\n"); >> + return -EINVAL; >> + } >> + writel(cfg, rot->regs + ROTATOR_CONTROL); >> + return 0; >> +} >> + >> +void rot_hwset_flip(struct rot_dev *rot, u32 direction) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONTROL); >> + cfg&= ~ROTATOR_CONTROL_FLIP_MASK; >> + >> + if (direction == ROT_VFLIP) >> + cfg |= ROTATOR_CONTROL_FLIP_V; >> + else if (direction == ROT_HFLIP) >> + cfg |= ROTATOR_CONTROL_FLIP_H; >> + >> + writel(cfg, rot->regs + ROTATOR_CONTROL); >> +} >> + >> +void rot_hwset_rotation(struct rot_dev *rot, int degree) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONTROL); >> + cfg&= ~ROTATOR_CONTROL_ROT_MASK; >> + >> + if (degree == 90) >> + cfg |= ROTATOR_CONTROL_ROT_90; >> + else if (degree == 180) >> + cfg |= ROTATOR_CONTROL_ROT_180; >> + else if (degree == 270) >> + cfg |= ROTATOR_CONTROL_ROT_270; >> + >> + writel(cfg, rot->regs + ROTATOR_CONTROL); >> +} >> + >> +void rot_hwset_start(struct rot_dev *rot) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_CONTROL); >> + >> + cfg |= ROTATOR_CONTROL_START; >> + >> + writel(cfg, rot->regs + ROTATOR_CONTROL); >> +} >> + >> +void rot_hwset_src_addr(struct rot_dev *rot, dma_addr_t addr, u32 comp) >> +{ >> + writel(addr, rot->regs + ROTATOR_SRC_IMG_ADDR(comp)); >> +} >> + >> +void rot_hwset_dst_addr(struct rot_dev *rot, dma_addr_t addr, u32 comp) >> +{ >> + writel(addr, rot->regs + ROTATOR_DST_IMG_ADDR(comp)); >> +} >> + >> +void rot_hwset_src_imgsize(struct rot_dev *rot, struct rot_frame *frame) >> +{ >> + unsigned long cfg; >> + >> + cfg = ROTATOR_SRCIMG_YSIZE(frame->pix_mp.height) | >> + ROTATOR_SRCIMG_XSIZE(frame->pix_mp.width); >> + >> + writel(cfg, rot->regs + ROTATOR_SRCIMG); >> + >> + cfg = ROTATOR_SRCROT_YSIZE(frame->pix_mp.height) | >> + ROTATOR_SRCROT_XSIZE(frame->pix_mp.width); >> + >> + writel(cfg, rot->regs + ROTATOR_SRCROT); >> +} >> + >> +void rot_hwset_src_crop(struct rot_dev *rot, struct v4l2_rect *rect) >> +{ >> + unsigned long cfg; >> + >> + cfg = ROTATOR_SRC_Y(rect->top) | >> + ROTATOR_SRC_X(rect->left); >> + >> + writel(cfg, rot->regs + ROTATOR_SRC); >> + >> + cfg = ROTATOR_SRCROT_YSIZE(rect->height) | >> + ROTATOR_SRCROT_XSIZE(rect->width); >> + >> + writel(cfg, rot->regs + ROTATOR_SRCROT); >> +} >> + >> +void rot_hwset_dst_imgsize(struct rot_dev *rot, struct rot_frame *frame) >> +{ >> + unsigned long cfg; >> + >> + cfg = ROTATOR_DSTIMG_YSIZE(frame->pix_mp.height) | >> + ROTATOR_DSTIMG_XSIZE(frame->pix_mp.width); >> + >> + writel(cfg, rot->regs + ROTATOR_DSTIMG); >> +} >> + >> +void rot_hwset_dst_crop(struct rot_dev *rot, struct v4l2_rect *rect) >> +{ >> + unsigned long cfg; >> + >> + cfg = ROTATOR_DST_Y(rect->top) | >> + ROTATOR_DST_X(rect->left); >> + >> + writel(cfg, rot->regs + ROTATOR_DST); >> +} >> + >> +void rot_hwget_irq_src(struct rot_dev *rot, enum rot_irq_src *irq) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_STATUS); >> + cfg = ROTATOR_STATUS_IRQ(cfg); >> + >> + if (cfg == 1) >> + *irq = ISR_PEND_DONE; >> + else if (cfg == 2) >> + *irq = ISR_PEND_ILLEGAL; >> +} >> + >> +void rot_hwset_irq_clear(struct rot_dev *rot, enum rot_irq_src *irq) >> +{ >> + unsigned long cfg = readl(rot->regs + ROTATOR_STATUS); >> + cfg |= ROTATOR_STATUS_IRQ_PENDING((u32)irq); >> + >> + writel(cfg, rot->regs + ROTATOR_STATUS); >> +} >> + >> +void rot_hwget_status(struct rot_dev *rot, enum rot_status *state) >> +{ >> + unsigned long cfg; >> + >> + cfg = readl(rot->regs + ROTATOR_STATUS); >> + cfg&= ROTATOR_STATUS_MASK; >> + >> + switch (cfg) { >> + case 0: >> + *state = ROT_IDLE; >> + return; > >Plase just use 'break' here, or make the function return enum rot_status. > OK. 'break' is better. >> + case 1: >> + *state = ROT_RESERVED; >> + return; >> + case 2: >> + *state = ROT_RUNNING; >> + return; >> + case 3: >> + *state = ROT_RUNNING_REMAIN; >> + return; >> + }; >> +} >> + >> +void rot_dump_register(struct rot_dev *rot) >> +{ >> + unsigned int tmp, i; >> + >> + rot_dbg("dump rotator registers\n"); >> + for (i = 0; i<= ROTATOR_DST; i += 0x4) { >> + tmp = readl(rot->regs + i); >> + rot_dbg("+0x%x: 0x%x", i, tmp); > >rot_dbg("0x%08x: 0x%08x", i, tmp); might be better. > OK. Good. >> + } >> +} >> diff --git a/drivers/media/video/exynos/rotator/rotator-regs.h >b/drivers/media/video/exynos/rotator/rotator-regs.h >> new file mode 100644 >> index 0000000..a603417 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator-regs.h >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright (c) 2012 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Register header file for Exynos Rotator driver >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> +*/ >> + >> +/* Configuration */ >> +#define ROTATOR_CONFIG 0x00 >> +#define ROTATOR_CONFIG_IRQ_ILLEGAL (1<< 9) >> +#define ROTATOR_CONFIG_IRQ_DONE (1<< 8) >> + >> +/* Image0 Control */ >> +#define ROTATOR_CONTROL 0x10 >> +#define ROTATOR_CONTROL_PATTERN_WRITE (1<< 16) >> +#define ROTATOR_CONTROL_FMT_YCBCR420_3P (0<< 8) >> +#define ROTATOR_CONTROL_FMT_YCBCR420_2P (1<< 8) >> +#define ROTATOR_CONTROL_FMT_YCBCR422 (3<< 8) >> +#define ROTATOR_CONTROL_FMT_RGB565 (4<< 8) >> +#define ROTATOR_CONTROL_FMT_RGB888 (6<< 8) >> +#define ROTATOR_CONTROL_FMT_MASK (7<< 8) >> +#define ROTATOR_CONTROL_FLIP_V (2<< 6) >> +#define ROTATOR_CONTROL_FLIP_H (3<< 6) >> +#define ROTATOR_CONTROL_FLIP_MASK (3<< 6) >> +#define ROTATOR_CONTROL_ROT_90 (1<< 4) >> +#define ROTATOR_CONTROL_ROT_180 (2<< 4) >> +#define ROTATOR_CONTROL_ROT_270 (3<< 4) >> +#define ROTATOR_CONTROL_ROT_MASK (3<< 4) >> +#define ROTATOR_CONTROL_START (1<< 0) >> + >> +/* Status */ >> +#define ROTATOR_STATUS 0x20 >> +#define ROTATOR_STATUS_IRQ_PENDING(x) (1<< (x)) >> +#define ROTATOR_STATUS_IRQ(x) (((x)>> 8)& 0x3) >> +#define ROTATOR_STATUS_MASK (3<< 0) >> + >> +/* Sourc Image Base Address */ >> +#define ROTATOR_SRC_IMG_ADDR(n) (0x30 + ((n)<< 2)) >> + >> +/* Source Image X,Y Size */ >> +#define ROTATOR_SRCIMG 0x3c >> +#define ROTATOR_SRCIMG_YSIZE(x) ((x)<< 16) >> +#define ROTATOR_SRCIMG_XSIZE(x) ((x)<< 0) >> + >> +/* Source Image X,Y Coordinates */ >> +#define ROTATOR_SRC 0x40 >> +#define ROTATOR_SRC_Y(x) ((x)<< 16) >> +#define ROTATOR_SRC_X(x) ((x)<< 0) >> + >> +/* Source Image Rotation Size */ >> +#define ROTATOR_SRCROT 0x44 >> +#define ROTATOR_SRCROT_YSIZE(x) ((x)<< 16) >> +#define ROTATOR_SRCROT_XSIZE(x) ((x)<< 0) >> + >> +/* Destination Image Base Address */ >> +#define ROTATOR_DST_IMG_ADDR(n) (0x50 + ((n)<< 2)) >> + >> +/* Destination Image X,Y Size */ >> +#define ROTATOR_DSTIMG 0x5c >> +#define ROTATOR_DSTIMG_YSIZE(x) ((x)<< 16) >> +#define ROTATOR_DSTIMG_XSIZE(x) ((x)<< 0) >> + >> +/* Destination Image X,Y Coordinates */ >> +#define ROTATOR_DST 0x60 >> +#define ROTATOR_DST_Y(x) ((x)<< 16) >> +#define ROTATOR_DST_X(x) ((x)<< 0) >> diff --git a/drivers/media/video/exynos/rotator/rotator.h >b/drivers/media/video/exynos/rotator/rotator.h >> new file mode 100644 >> index 0000000..67b5152 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator.h >> @@ -0,0 +1,325 @@ >> +/* >> + * Copyright (c) 2012 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Header file for Exynos Rotator driver >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> +*/ >> + >> +#ifndef ROTATOR__H_ >> +#define ROTATOR__H_ >> + >> +#include<linux/delay.h> >> +#include<linux/sched.h> >> +#include<linux/spinlock.h> >> +#include<linux/types.h> >> +#include<linux/videodev2.h> >> +#include<linux/io.h> >> +#include<linux/pm_runtime.h> >> +#include<media/videobuf2-core.h> >> +#include<media/v4l2-device.h> >> +#include<media/v4l2-mem2mem.h> >> +#include<media/v4l2-mediabus.h> >> + >> +#include "rotator-regs.h" >> + >> +/* debug macro */ >> +enum rot_log { >> + ROT_LOG_DEBUG = 1000, >> + ROT_LOG_INFO = 100, >> + ROT_LOG_WARN = 10, >> + ROT_LOG_ERR = 1, > >Should't these be hexadecimal values ? > You're right. my mistake :) >> +}; >> + >> +#define ROT_LOG_DEFAULT (ROT_LOG_INFO | ROT_LOG_WARN | ROT_LOG_ERR) >> +static enum rot_log log_level = ROT_LOG_DEFAULT; >> + >> +#define ROT_DEBUG(fmt, ...) \ >> + do { \ >> + if (log_level& ROT_LOG_DEBUG) \ >> + printk(KERN_DEBUG "[%s:%d] " \ > >pr_debug, if you relly need that, > Prefer dev_dbg instead pr_debug in printk.h Which is better among the printk, pr_debug and dev_dbg ? >> + fmt, __func__, __LINE__, ##__VA_ARGS__); \ >> + } while (0) >> + >> +#define ROT_INFO(fmt, ...) \ >> + do { \ >> + if (log_level& ROT_LOG_INFO) \ >> + pr_info("[%s:%d] " \ >> + fmt, __func__, __LINE__ , ##__VA_ARGS__); \ >> + } while (0) >> + >> +#define ROT_WARN(fmt, ...) \ >> + do { \ >> + if (log_level& ROT_LOG_WARN) \ >> + pr_warn("[%s:%d] " \ >> + fmt, __func__, __LINE__ , ##__VA_ARGS__); \ >> + } while (0) >> + >> +#define ROT_ERR(fmt, ...) \ >> + do { \ >> + if (log_level& ROT_LOG_ERR) \ >> + pr_err("[%s:%d] " \ >> + fmt, __func__, __LINE__ , ##__VA_ARGS__); \ >> + } while (0) >> + >> +#define rot_dbg(fmt, ...) ROT_DEBUG(fmt, ##__VA_ARGS__) >> +#define rot_info(fmt, ...) ROT_INFO(fmt, ##__VA_ARGS__) >> +#define rot_warn(fmt, ...) ROT_WARN(fmt, ##__VA_ARGS__) >> +#define rot_err(fmt, ...) ROT_ERR(fmt, ##__VA_ARGS__) > >You probably could do without those macros. Alternatively you could just add > >#define pr_fmt(fmt) "[%s:%d] " fmt, __func__, __LINE__ > >line on top of this file and drop each ""[%s:%d] " __func__, __LINE__" above. > >> + >> +/* Time to wait for frame done interrupt */ >> +#define ROT_TIMEOUT (2 * HZ) >> +#define ROT_WDT_CNT 5 >> +#define MODULE_NAME "rotator" >> +#define ROT_MAX_DEVS 1 >> + >> +/* Address index */ >> +#define ROT_ADDR_RGB 0 >> +#define ROT_ADDR_Y 0 >> +#define ROT_ADDR_CB 1 >> +#define ROT_ADDR_CBCR 1 >> +#define ROT_ADDR_CR 2 >> + >> +/* Driver version */ >> +#define MAJOR_VERSION 0 >> +#define MINOR_VERSION 1 >> +#define RELEASE_VERSION 1 > >Just ditch that. > I'll remove it. >> +/* Rotator flip direction */ >> +#define ROT_NOFLIP (1<< 0) >> +#define ROT_VFLIP (1<< 1) >> +#define ROT_HFLIP (1<< 2) >> + >> +/* Rotator hardware device state */ >> +#define DEV_RUN (1<< 0) >> +#define DEV_SUSPEND (1<< 1) >> + >> +/* Rotator m2m context state */ >> +#define CTX_PARAMS (1<< 0) >> +#define CTX_STREAMING (1<< 1) >> +#define CTX_RUN (1<< 2) >> +#define CTX_ABORT (1<< 3) >> +#define CTX_SUSPEND (1<< 4) >> +#define CTX_SRC (1<< 5) >> +#define CTX_DST (1<< 6) >> + >> +enum rot_irq_src { >> + ISR_PEND_DONE = 8, >> + ISR_PEND_ILLEGAL = 9, >> +}; >> + >> +enum rot_status { >> + ROT_IDLE, >> + ROT_RESERVED, >> + ROT_RUNNING, >> + ROT_RUNNING_REMAIN, >> +}; >> + >> +/* >> + * struct exynos_rot_size_limit - Rotator variant size information >> + * >> + * @min_x: minimum pixel x size >> + * @min_y: minimum pixel y size >> + * @max_x: maximum pixel x size >> + * @max_y: maximum pixel y size >> + */ >> +struct exynos_rot_size_limit { >> + u32 min_x; >> + u32 min_y; >> + u32 max_x; >> + u32 max_y; >> + u32 align; >> +}; >> + >> +struct exynos_rot_variant { >> + struct exynos_rot_size_limit limit_rgb565; >> + struct exynos_rot_size_limit limit_rgb888; >> + struct exynos_rot_size_limit limit_yuv422; >> + struct exynos_rot_size_limit limit_yuv420_2p; >> + struct exynos_rot_size_limit limit_yuv420_3p; >> +}; >> + >> +/* >> + * struct exynos_rot_driverdata - per device type driver data for init time. >> + * >> + * @variant: the variant information for this driver. >> + * @nr_dev: number of devices available in SoC >> + */ >> +struct exynos_rot_driverdata { >> + struct exynos_rot_variant *variant[ROT_MAX_DEVS]; >> + int nr_dev; >> +}; >> + >> +/** >> + * struct rot_fmt - the driver's internal color format data >> + * @name: format description >> + * @pixelformat: the fourcc code for this format, 0 if not applicable >> + * @num_planes: number of physically non-contiguous data planes >> + * @nr_comp: number of color components(ex. RGB, Y, Cb, Cr) >> + * @bitperpixel: bits per pixel >> + */ >> +struct rot_fmt { >> + char *name; >> + u32 pixelformat; >> + u16 num_planes; >> + u16 nr_comp; >> + u32 bitperpixel[VIDEO_MAX_PLANES]; >> +}; >> + >> +struct rot_addr { >> + dma_addr_t y; >> + dma_addr_t cb; >> + dma_addr_t cr; >> +}; >> + >> +/* >> + * struct rot_frame - source/target frame properties >> + * @fmt: buffer format(like virtual screen) >> + * @crop: image size / position >> + * @addr: buffer start address(access using ROT_ADDR_XXX) >> + * @bytesused: image size in bytes (w x h x bpp) >> + * @cacheable: cache property for image address >> + */ >> +struct rot_frame { >> + struct rot_fmt *rot_fmt; >> + struct v4l2_pix_format_mplane pix_mp; >> + struct v4l2_rect crop; >> + struct rot_addr addr; >> + unsigned long bytesused[VIDEO_MAX_PLANES]; >> + bool cacheable; >> +}; >> + >> +/* >> + * struct rot_m2m_device - v4l2 memory-to-memory device data >> + * @v4l2_dev: v4l2 device >> + * @vfd: the video device node >> + * @m2m_dev: v4l2 memory-to-memory device data >> + * @ctx: hardware context data >> + * @in_use: the open count >> + */ >> +struct rot_m2m_device { >> + struct v4l2_device v4l2_dev; >> + struct video_device *vfd; >> + struct v4l2_m2m_dev *m2m_dev; >> + struct rot_ctx *ctx; >> + atomic_t in_use; >> +}; >> + >> +/* >> + * struct rot_irq - Rotator irq information >> + * @num: Rotator interrupt number >> + * @wait: interrupt handler waitqueue >> + */ >> +struct rot_irq { >> + int num; >> + wait_queue_head_t wait; >> +}; >> + >> +struct rot_wdt { >> + struct timer_list timer; >> + atomic_t cnt; >> +}; >> + >> +struct rot_ctx; >> +struct rot_vb2; >> + >> +/* >> + * struct rot_dev - the abstraction for Rotator device >> + * @pdev: pointer to the Rotator platform device >> + * @pdata: pointer to the device platform data >> + * @variant: the IP variant information >> + * @m2m: memory-to-memory V4L2 device information >> + * @id: Rotator device index (0..ROT_MAX_DEVS) >> + * @clock: clock required for Rotator operation >> + * @regs: the mapped hardware registers >> + * @regs_res: the resource claimed for IO registers >> + * @irq: irq information >> + * @ws: work struct >> + * @wq: workqueue >> + * @state: device state flags >> + * @alloc_ctx: videobuf2 memory allocator context >> + * @rot_vb2: videobuf2 memory allocator callbacks >> + * @slock: the spinlock protecting this data structure >> + * @lock: the mutex protecting this data structure >> + * @wdt: watchdog timer information >> + */ >> +struct rot_dev { >> + struct platform_device *pdev; >> + struct exynos_platform_rot *pdata; >> + struct exynos_rot_variant *variant; >> + struct rot_m2m_device m2m; >> + int id; >> + struct clk *clock; >> + void __iomem *regs; >> + struct resource *regs_res; >> + struct rot_irq irq; >> + struct work_struct ws; >> + struct workqueue_struct *wq; >> + unsigned long state; >> + struct vb2_alloc_ctx *alloc_ctx; >> + const struct rot_vb2 *vb2; >> + spinlock_t slock; >> + struct mutex lock; >> + struct rot_wdt wdt; >> +}; >> + >> +/* >> + * rot_ctx - the abstration for Rotator open context >> + * @rot_dev: the Rotator device this context applies to >> + * @m2m_ctx: memory-to-memory device context >> + * @frame: source frame properties >> + * @rotation: image clockwise rotation in degrees >> + * @flip: image flip mode >> + * @state: context state flags >> + * @slock: spinlock protecting this data structure >> + */ >> +struct rot_ctx { >> + struct rot_dev *rot_dev; >> + struct v4l2_m2m_ctx *m2m_ctx; >> + struct rot_frame s_frame; >> + struct rot_frame d_frame; >> + int rotation; >> + u32 flip; >> + unsigned long flags; >> + spinlock_t slock; >> +}; >> + >> +static inline struct rot_frame *ctx_get_frame(struct rot_ctx *ctx, >> + enum v4l2_buf_type type) >> +{ >> + struct rot_frame *frame; >> + >> + if (V4L2_TYPE_IS_MULTIPLANAR(type)) { >> + if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> + frame =&ctx->s_frame; >> + else >> + frame =&ctx->d_frame; >> + } else { >> + rot_err("Wrong V4L2 buffer type %d\n", type); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + return frame; >> +} >> + >> +void rot_hwset_irq_frame_done(struct rot_dev *rot, u32 enable); >> +void rot_hwset_irq_illegal_config(struct rot_dev *rot, u32 enable); >> +int rot_hwset_image_format(struct rot_dev *rot, u32 pixelformat); >> +void rot_hwset_flip(struct rot_dev *rot, u32 direction); >> +void rot_hwset_rotation(struct rot_dev *rot, int degree); >> +void rot_hwset_start(struct rot_dev *rot); >> +void rot_hwset_src_addr(struct rot_dev *rot, dma_addr_t addr, u32 comp); >> +void rot_hwset_dst_addr(struct rot_dev *rot, dma_addr_t addr, u32 comp); >> +void rot_hwset_src_imgsize(struct rot_dev *rot, struct rot_frame *frame); >> +void rot_hwset_src_crop(struct rot_dev *rot, struct v4l2_rect *rect); >> +void rot_hwset_dst_imgsize(struct rot_dev *rot, struct rot_frame *frame); >> +void rot_hwset_dst_crop(struct rot_dev *rot, struct v4l2_rect *rect); >> +void rot_hwget_irq_src(struct rot_dev *rot, enum rot_irq_src *irq); >> +void rot_hwset_irq_clear(struct rot_dev *rot, enum rot_irq_src *irq); >> +void rot_hwget_status(struct rot_dev *rot, enum rot_status *state); >> +void rot_dump_register(struct rot_dev *rot); >> + >> +#endif /* ROTATOR__H_ */ > >--- > >Thank you, > >Sylwester > Thank you, Sunyoung -- 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
