Hi Sylwester Thanks for the review. God bless you! :) >On 03/15/2012 06:21 AM, Sylwester Nawrocki wrote: >Hi Sunyoung, > >thank you for addressing my previous comments. It looks much better now. >I have is a few more comments... > >On 03/14/2012 09:00 AM, Sunyoung Kang wrote: >> This patch adds new rotator driver which is a device for >> rotation on EXYNOS SoCs. >> >> This rotator device supports the belows as key feature. >> 1) Image format >> : RGB565/888, YUV422 1p, YUV420 2p/3p >> 2) rotation >> : 0/90/180/270 and X/Y Flip >> >> Signed-off-by: Sunyoung Kang<sy0816.kang@xxxxxxxxxxx> >> Signed-off-by: Ayoung Sim<a.sim@xxxxxxxxxxx> >> ---NOTE: > >It's almost right;) After the "---" there must be immediately a new >line character. So "NOTE" should be in a new line. > Oh.. ok. Thanks. >> 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 >> >> Changes since v1: >> - address comments from Sylwester Nawrocki >> >> drivers/media/video/exynos/Kconfig | 1 + >> drivers/media/video/exynos/Makefile | 1 + >> drivers/media/video/exynos/rotator/Kconfig | 7 + >> drivers/media/video/exynos/rotator/Makefile | 9 + >> drivers/media/video/exynos/rotator/rotator-core.c | 1344 +++++++++++++++++++++ >> 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 | 286 +++++ >> 8 files changed, 1933 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/rotator/Kconfig b/drivers/media/video/exynos/rotator/Kconfig >> new file mode 100644 >> index 0000000..977423a >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/Kconfig >> @@ -0,0 +1,7 @@ >> +config VIDEO_EXYNOS_ROTATOR >> + bool "EXYNOS Image Rotator Driver" >> + select V4L2_MEM2MEM_DEV >> + select VIDEOBUF2_DMA_CONTIG >> + default n > >No need, the default default already is "n". > OK. >> + ---help--- >> + Support for Image Rotator Driver with v4l2 on EXYNOS SoCs >> 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..75d28f2 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator-core.c >> @@ -0,0 +1,1344 @@ >> +/* >> + * 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" >> + >> +int log_level; >> +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, >> + .num_comp = 1, >> + .bitperpixel = { 16 }, >> + }, { >> + .name = "XRGB-8888, 32 bps", > >What 'bps' stands for ? > Oops.. it should be 'bpp' means bit per pixel.. thanks. >> + .pixelformat = V4L2_PIX_FMT_RGB32, >> + .num_planes = 1, >> + .num_comp = 1, >> + .bitperpixel = { 32 }, >> + }, { >> + .name = "YUV 4:2:2 packed, YCbYCr", >> + .pixelformat = V4L2_PIX_FMT_YUYV, >> + .num_planes = 1, >> + .num_comp = 1, >> + .bitperpixel = { 16 }, >> + }, { >> + .name = "YUV 4:2:0 non-contiguous 2-planar, Y/CbCr", >> + .pixelformat = V4L2_PIX_FMT_NV12M, >> + .num_planes = 2, >> + .num_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, >> + .num_comp = 3, >> + .bitperpixel = { 8, 2, 2 }, >> + }, >> +}; >> + >... >> +static int rot_s_ctrl(struct v4l2_ctrl *ctrl) >> +{ >> + struct rot_ctx *ctx; >> + unsigned long flags; >> + int ret = 0; >> + >> + ctx = container_of(ctrl->handler, struct rot_ctx, ctrl_handler); >> + spin_lock_irqsave(&ctx->slock, flags); >> + >> + rot_dbg("ctrl ID:%d, value:%d\n", ctrl->id, ctrl->val); >> + switch (ctrl->id) { >> + case V4L2_CID_VFLIP: >> + if (ctrl->val) >> + ctx->flip |= ROT_VFLIP; >> + else >> + ctx->flip&= ~ROT_VFLIP; >> + break; >> + case V4L2_CID_HFLIP: >> + if (ctrl->val) >> + ctx->flip |= ROT_HFLIP; >> + else >> + ctx->flip&= ~ROT_HFLIP; >> + break; >> + case V4L2_CID_ROTATE: >> + ctx->rotation = ctrl->val; >> + break; >> + default: >> + v4l2_err(&ctx->rot_dev->m2m.v4l2_dev, "invalid control id\n"); >> + ret = -EINVAL; > >These 3 lines can be removed, rot_s_ctrl can be called only with ctrl->id >from the set of controls added to control handler. > OK. I'll remove it. >> + } >> + >> + spin_unlock_irqrestore(&ctx->slock, flags); >> + >> + return ret; >> +} >> + >> +static const struct v4l2_ctrl_ops rot_ctrl_ops = { >> + .s_ctrl = rot_s_ctrl, >> +}; >> + >> +static int rot_add_ctrls(struct rot_ctx *ctx) >> +{ >> + v4l2_ctrl_handler_init(&ctx->ctrl_handler, 4); > >There are only 3 controls, so s/3/4. > You're right. Just 3 controls are in here. >> + v4l2_ctrl_new_std(&ctx->ctrl_handler,&rot_ctrl_ops, >> + V4L2_CID_VFLIP, 0, 1, 1, 0); >> + v4l2_ctrl_new_std(&ctx->ctrl_handler,&rot_ctrl_ops, >> + V4L2_CID_HFLIP, 0, 1, 1, 0); >> + v4l2_ctrl_new_std(&ctx->ctrl_handler,&rot_ctrl_ops, >> + V4L2_CID_ROTATE, 0, 270, 90, 0); >> + >> + if (ctx->ctrl_handler.error) { >> + int err = ctx->ctrl_handler.error; >> + v4l2_err(&ctx->rot_dev->m2m.v4l2_dev, >> + "v4l2_ctrl_handler_init failed\n"); >> + v4l2_ctrl_handler_free(&ctx->ctrl_handler); >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +static int rot_open(struct file *file) >> +{ >> + struct rot_dev *rot = video_drvdata(file); >> + struct rot_ctx *ctx = NULL; > >No need to initialize here. > OK. >> + int ret; >> + >> + ctx = kzalloc(sizeof *ctx, GFP_KERNEL); >> + if (!ctx) { >> + dev_err(rot->dev, "no memory for open context\n"); >> + return -ENOMEM; >> + } >> + >> + atomic_inc(&rot->m2m.in_use); >> + ctx->rot_dev = rot; >> + >> + v4l2_fh_init(&ctx->fh, rot->m2m.vfd); >> + ret = rot_add_ctrls(ctx); >> + if (ret) >> + goto err_fh; >> + >> + ctx->fh.ctrl_handler =&ctx->ctrl_handler; >> + file->private_data =&ctx->fh; >> + v4l2_fh_add(&ctx->fh); >> + >> + /* Default color format */ >> + ctx->s_frame.rot_fmt =&rot_formats[0]; >> + ctx->d_frame.rot_fmt =&rot_formats[0]; >> + init_waitqueue_head(&rot->irq.wait); >> + 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)) { >> + ret = -EINVAL; >> + goto err_ctx; >> + } >> + >> + return 0; >> + >> +err_ctx: >> + v4l2_fh_del(&ctx->fh); >> +err_fh: >> + v4l2_ctrl_handler_free(&ctx->ctrl_handler); >> + v4l2_fh_exit(&ctx->fh); >> + atomic_dec(&rot->m2m.in_use); >> + kfree(ctx); >> + >> + return ret; >> +} >> + >... >> +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) { >> + dev_err(rot->dev, "####################\n"); >> + dev_err(rot->dev, "set SFR illegally\n"); >> + dev_err(rot->dev, "maybe the result is wrong\n"); >> + dev_err(rot->dev, "####################\n"); >> + rot_dump_register(rot); >> + } > >How about following instead: > > if (WARN_ON(irq_src != ISR_PEND_DONE), > "Illegal SFR configuration, the result might be wrong\n") { > rot_dump_register(rot); > } >? Um.. I think, here doesn't need debugging information. I'd like to just inform the wrong result. But the log message will be changed as your guide. >> + >> + ctx = v4l2_m2m_get_curr_priv(rot->m2m.m2m_dev); >> + if (!ctx || !ctx->m2m_ctx) { >> + dev_err(rot->dev, "current ctx is NULL\n"); >> + goto isr_unlock; >> + } >> + >> + clear_bit(CTX_RUN,&ctx->flags); >> + >> + 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); >> + >> + pm_runtime_put(rot->dev); >> + } else { >> + dev_err(rot->dev, "failed to get the buffer done\n"); >> + } >> + >> +isr_unlock: >> + spin_unlock(&rot->slock); >> + >> + return IRQ_HANDLED; >> +} >> + >... >> +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)) { >> + dev_err(rot->dev, "Rotator is already in progress\n"); >> + goto run_unlock; >> + } >> + >> + if (test_bit(DEV_SUSPEND,&rot->state)) { >> + dev_err(rot->dev, "Rotator 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->dev); >> + >> + 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; > >Do you mean: > swap(d_frame->pix_mp.height, d_frame->pix_mp.width); >? > >Does it mean setting rotation to 90 or 270 deg changes the output (capture) >format ? Maybe you want to do this swapping on temporary variables, that >would be used to configure the hardware ? > >The rotation is a bit tricky, AFAIK the application should swap width/and >height. And the driver should scale an image (change aspect ratio) when >width/height isn't swapped and 90/270 deg rotation is set. Or return an >error when the device doesn't support resizing. > Ok. As you mentioned, if the application should know about width/height for output, driver doesn't need to care about it. I'll remove this code. >> + } >> + >> + 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); >> + >> + 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 int rot_suspend(struct device *dev) >> +{ >> + struct rot_dev *rot; > >How about assigning here directly, > > struct rot_dev *rot = dev_get_drvdata(dev); >? >> + int ret = 0; > >No need to initialize. > OK. I'll change it. >> + >> + rot = dev_get_drvdata(dev); >> + set_bit(DEV_SUSPEND,&rot->state); >> + >> + ret = wait_event_timeout(rot->irq.wait, >> + !test_bit(DEV_RUN,&rot->state), ROT_TIMEOUT); >> + if (ret == 0) >> + dev_err(rot->dev, "wait timeout\n"); >> + >> + return 0; >> +} >> + >> +static int rot_resume(struct device *dev) >> +{ >> + struct rot_dev *rot; >> + >> + rot = dev_get_drvdata(dev); > > struct rot_dev *rot = dev_get_drvdata(dev); > ? > Ditto. >> + clear_bit(DEV_SUSPEND,&rot->state); >> + >> + 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); > > >I think, you've forgotten to update that one? > Yes. :) >> + 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); > >And here (struct rot_dev *rot = dev_get_drvdata(dev);) ? > I missed it. :) >> + 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; >> + >> + dev_info(&pdev->dev, "++%s\n", __func__); >> + drv_data = (struct exynos_rot_driverdata *) >> + platform_get_device_id(pdev)->driver_data; >> + >> + if (pdev->id>= drv_data->nr_dev) { >> + dev_err(&pdev->dev, "Invalid platform device id\n"); >> + return -EINVAL; >> + } > >If there is always only one device, is this needed ? > Now the EXYNOS SoCs have only one rotator device, but the number can be increased in future. So I considered about it. Should this be removed including the code related with this? >> + rot = devm_kzalloc(&pdev->dev, sizeof(struct rot_dev), GFP_KERNEL); >> + if (!rot) { >> + dev_err(&pdev->dev, "no memory for rotator device\n"); >> + return -ENOMEM; >> + } >> + >> + rot->dev =&pdev->dev; >> + 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); >> + rot->regs = devm_request_and_ioremap(&pdev->dev, res); >> + if (rot->regs == NULL) { >> + dev_err(&pdev->dev, "failed to claim register region\n"); >> + return -ENOENT; >> + } >> + >> + /* Get IRQ resource and register IRQ handler. */ >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + if (res == NULL) { >> + dev_err(&pdev->dev, "failed to get IRQ resource\n"); >> + return -ENXIO; >> + } >> + rot->irq.num = res->start; >> + >> + ret = devm_request_irq(&pdev->dev, rot->irq.num, rot_irq_handler, 0, > >I think you can use res->start directly, and remove irq.num, since the interrupt >is now being released dynamically and we don't need to sore the interrupt number. > OK. Thanks. >> + pdev->name, rot); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to install irq\n"); >> + return ret; >> + } >> + >> + atomic_set(&rot->wdt.cnt, 0); >> + setup_timer(&rot->wdt.timer, rot_watchdog, (unsigned long)rot); >> + >> + rot->clock = clk_get(rot->dev, "rotator"); >> + if (IS_ERR(rot->clock)) { >> + dev_err(&pdev->dev, "failed to get clock for rotator\n"); >> + return -ENXIO; >> + } >> + >> + rot->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev); >> + if (IS_ERR(rot->alloc_ctx)) { >> + dev_err(&pdev->dev, "failed to get alloc_ctx\n"); >> + ret = -EPERM; >> + goto err; >> + } >> + >> + ret = rot_register_m2m_device(rot); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register m2m device\n"); >> + ret = -EPERM; >> + goto err; >> + } >> + >> +#ifdef CONFIG_PM_RUNTIME >> + pm_runtime_enable(&pdev->dev); >> +#else >> + rot_runtime_resume(&pdev->dev); >> +#endif > >Hmm, not very pretty. At least it could be simplified to: > > pm_runtime_enable(&pdev->dev); > >#ifndef CONFIG_PM_RUNTIME > rot_runtime_resume(&pdev->dev); >#endif > I'll make more pretty~! >> + dev_info(&pdev->dev, "rotator registered successfully\n"); >> + >> + return 0; >> + >> +err: >> + clk_put(rot->clock); >> + return ret; >> +} >> + >> +static int rot_remove(struct platform_device *pdev) >> +{ >> + struct rot_dev *rot = (struct rot_dev *)platform_get_drvdata(pdev); > >The is no need for casting from void *. > OK. I'll change it. >> + clk_put(rot->clock); >> +#ifdef CONFIG_PM_RUNTIME >> + pm_runtime_disable(&pdev->dev); >> +#else >> + rot_runtime_suspend(&pdev->dev); >> +#endif > >pm_runtime_disable() is a no op when CONFIG_PM_RUNTIME is disabled. >So it could be changed to: > > pm_runtime_disable(&pdev->dev); > >#ifndef CONFIG_PM_RUNTIME > rot_runtime_suspend(&pdev->dev); >#endif > OK. >> + if (timer_pending(&rot->wdt.timer)) >> + del_timer(&rot->wdt.timer); >> + >> + return 0; >> +} >> + >... >> +void rot_dump_register(struct rot_dev *rot) > >rot_dump_registers ? > OK. :) >> +{ >> + 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%08x: 0x%08x", i, tmp); >> + } >> +} >... >> diff --git a/drivers/media/video/exynos/rotator/rotator.h >b/drivers/media/video/exynos/rotator/rotator.h >> new file mode 100644 >> index 0000000..4034383 >> --- /dev/null >> +++ b/drivers/media/video/exynos/rotator/rotator.h >> @@ -0,0 +1,286 @@ >> +/* >> + * 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> > >Probably this one should be included explicitly only in the .c files. > I'll move it into .c file. >> +#include<media/videobuf2-core.h> >> +#include<media/v4l2-ctrls.h> >> +#include<media/v4l2-device.h> >> +#include<media/v4l2-mem2mem.h> >> + >> +#include "rotator-regs.h" >> + >> +extern int log_level; >> + >> +#define rot_dbg(fmt, args...) \ >> + do { \ >> + if (log_level) \ >> + printk(KERN_DEBUG "[%s:%d] " \ >> + fmt, __func__, __LINE__, ##args); \ >> + } while (0) >> + >> +/* Time to wait for frame done interrupt */ >> +#define ROT_TIMEOUT (2 * HZ) >> +#define ROT_WDT_CNT 5 >> +#define MODULE_NAME "exynos-rot" >> +#define ROT_MAX_DEVS 1 > >If there is no more than one hardware instance in each SoC maybe it's worth >to drop ROT_MAX_DEVS and all the code around it ? > >> + >> +/* 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 >> + >> +/* 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_SRC (1<< 4) >> +#define CTX_DST (1<< 5) >> + >> +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; > >Probably not needed. > >> +}; >> + >> +/** >> + * 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 >> + * @num_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 num_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; > >It's also unused, what was this needed for (if at all) ? > Yes. I'll remove it. >> +}; >> + >> +/* >> + * 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 >> + * @in_use: the open count >> + */ >> +struct rot_m2m_device { >> + struct v4l2_device v4l2_dev; >> + struct video_device *vfd; >> + struct v4l2_m2m_dev *m2m_dev; >> + atomic_t in_use; >> +}; >> + >> +/* >> + * struct rot_irq - Rotator irq information >> + * @num: Rotator interrupt number >> + * @wait: interrupt handler waitqueue >> + */ >> +struct rot_irq { >> + int num; > >It's unused, you can remove it. > OK. >> + 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 >> + * @dev: pointer to the Rotator 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 >> + * @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 device *dev; >> + struct exynos_platform_rot *pdata; >> + struct exynos_rot_variant *variant; >> + struct rot_m2m_device m2m; >> + int id; >> + struct clk *clock; >> + void __iomem *regs; >> + struct rot_irq irq; >> + struct work_struct ws; > >irq and ws are not used, are they ? > I missed it and it will be removed. >> + 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 >> + * @ctrl_handler: v4l2 controls handler >> + * @fh: v4l2 file handle >> + * @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; >> + struct v4l2_ctrl_handler ctrl_handler; >> + struct v4l2_fh fh; >> + int rotation; >> + u32 flip; >> + unsigned long flags; >> + spinlock_t slock; >> +}; >... >> +#endif /* ROTATOR__H_ */ > >Otherwise looks good. > >-- > >Regards, >Sylwester -- 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
