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. > 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". > + ---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 ? > + .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. > + } > + > + 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. > + 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. > + 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); } ? > + > + 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. > + } > + > + 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. > + > + 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); ? > + 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? > + 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);) ? > + 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 ? > + 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. > + 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 > + 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 *. > + 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 > + if (timer_pending(&rot->wdt.timer)) > + del_timer(&rot->wdt.timer); > + > + return 0; > +} > + ... > +void rot_dump_register(struct rot_dev *rot) rot_dump_registers ? > +{ > + 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. > +#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) ? > +}; > + > +/* > + * 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. > + 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 ? > + 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
