From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sylwester Nawrocki Subject: Re: [RFC/PATCH] v4l: Add S5P MIPI-CSI2 interface subdev driver Date: Sat, 25 Dec 2010 22:22:17 +0100 Message-ID: <4D166089.8050908@gmail.com> References: <1293132375-26342-1-git-send-email-s.nawrocki@samsung.com> <201012241154.35159.hverkuil@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <201012241154.35159.hverkuil@xs4all.nl> Sender: linux-media-owner@vger.kernel.org To: Hans Verkuil Cc: Sylwester Nawrocki , linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org, kyungmin.park@samsung.com, m.szyprowski@samsung.com List-Id: linux-samsung-soc@vger.kernel.org Hi Hans, On 12/24/2010 11:54 AM, Hans Verkuil wrote: > On Thursday, December 23, 2010 20:26:15 Sylwester Nawrocki wrote: >> Add the v4l2 subdev/platform device driver for S5P SoC MIPI-CSI2 >> slave devices. >> >> Signed-off-by: Sylwester Nawrocki >> Signed-off-by: Kyungmin Park >> >> --- >> >> Hello, >> >> this patch adds a v4l2 subdev/platform device driver for S5P SoC MIPI-CSI2 >> slave devices.On S5PVXXX SoC there might be up two MIPI-CSI channels. >> This driver creates for each channel a subdev instance which is intended >> to interwork with the s5pc-fimc host driver. I am going to post >> the patches for the s5p fimc driver in near future so it can be seen how >> the subdevs are used. >> We are looking forward to use the media controller framework >> with S5P SoC for camera devices control, since the SoC architecture >> is relatively complex and flexible in configuration. >> There are up to 4 camera host interfaces with scaler, rotator >> and color format converter, up to 2 MIPI-CSI2 interfaces and >> there are also available datapaths to and from up to 2 LCD controllers. >> >> Regards, >> -- >> Sylwester Nawrocki >> Samsung Poland R&D Center >> >> --- >> drivers/media/video/Kconfig | 8 + >> drivers/media/video/Makefile | 1 + >> drivers/media/video/s5p-mipi_csis.c | 592 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 601 insertions(+), 0 deletions(-) >> create mode 100644 drivers/media/video/s5p-mipi_csis.c >> >> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig >> index da08267..04441aa 100644 >> --- a/drivers/media/video/Kconfig >> +++ b/drivers/media/video/Kconfig >> @@ -732,6 +732,13 @@ config VIDEO_VIA_CAMERA >> Chrome9 chipsets. Currently only tested on OLPC xo-1.5 systems >> with ov7670 sensors. >> >> +config VIDEO_S5P_MIPI_CSIS >> + bool "S5P MIPI-CSI2 slave iterface support" > > Typo: 'interface' Yes, I just noticed this too right after posting.. > >> + depends on VIDEO_V4L2 >> + ---help--- >> + This is video4linux driver for MIPI CSI2 interfaces >> + in Samsung S5P SoCs. >> + >> config SOC_CAMERA >> tristate "SoC camera support" >> depends on VIDEO_V4L2&& HAS_DMA&& I2C >> @@ -1000,6 +1007,7 @@ config VIDEO_SAMSUNG_S5P_FIMC >> depends on VIDEO_DEV&& VIDEO_V4L2&& PLAT_S5P >> select VIDEOBUF_DMA_CONTIG >> select V4L2_MEM2MEM_DEV >> + select VIDEO_SAMSUNG_MIPI_CSIS >> help >> This is a v4l2 driver for the S5P camera interface >> (video postprocessor) >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile >> index 482f14b..8cc093f 100644 >> --- a/drivers/media/video/Makefile >> +++ b/drivers/media/video/Makefile >> @@ -164,6 +164,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o >> obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o >> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o >> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o >> +obj-$(CONFIG_VIDEO_S5P_MIPI_CSIS) += s5p-mipi_csis.o >> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/ >> >> obj-$(CONFIG_ARCH_DAVINCI) += davinci/ >> diff --git a/drivers/media/video/s5p-mipi_csis.c b/drivers/media/video/s5p-mipi_csis.c >> new file mode 100644 >> index 0000000..823f49f >> --- /dev/null >> +++ b/drivers/media/video/s5p-mipi_csis.c >> @@ -0,0 +1,592 @@ >> +/* >> + * Samsung S5P SoC series MIPI-CSI2 slave interface driver >> + * >> + * Copyright (c) 2010 Samsung Electronics Co., Ltd >> + * Contact: Sylwester Nawrocki, >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +static int debug; >> +module_param(debug, int, 0644); >> +MODULE_PARM_DESC(debug, "Enable module debug trace. Set to 1 to enable."); >> + >> +#define MODULE_NAME "s5p-mipi-csis" >> + >> +/* Register map definition */ >> + >> +/* CSI global control. */ >> +#define S5P_CSIS_CTRL 0x00 >> +#define S5P_CSIS_CTRL_DPDN_DEFAULT (0<< 31) >> +#define S5P_CSIS_CTRL_DPDN_SWAP (1<< 31) >> +#define S5P_CSIS_CTRL_ALIGN_32BIT (1<< 20) >> +#define S5P_CSIS_CTRL_UPDATE_SHADOW (1<< 16) >> +#define S5P_CSIS_CTRL_WCLK_EXTCLK (1<< 8) >> +#define S5P_CSIS_CTRL_RESET (1<< 4) >> +#define S5P_CSIS_CTRL_ENABLE (1<< 0) >> + >> +/* D-PHY control. */ >> +#define S5P_CSIS_DPHYCTRL 0x04 >> +#define S5P_CSIS_DPHYCTRL_HSS_MASK (0x1F<< 27) >> +#define S5P_CSIS_DPHYCTRL_ENABLE (0x1F<< 0) >> + >> +#define S5P_CSIS_CONFIG 0x08 >> +#define S5P_CSIS_CFG_FMT_YCBCR422_8BIT (0x1E<< 2) >> +#define S5P_CSIS_CFG_FMT_RAW8 (0x2A<< 2) >> +#define S5P_CSIS_CFG_FMT_RAW10 (0x2B<< 2) >> +#define S5P_CSIS_CFG_FMT_RAW12 (0x2C<< 2) >> +#define S5P_CSIS_CFG_FMT_USER1 (0x30<< 2) >> +#define S5P_CSIS_CFG_FMT_MASK (0x3F<< 2) >> +#define S5P_CSIS_CFG_NR_LANE_MASK 3 >> + >> +/* Interrupt mask. */ >> +#define S5P_CSIS_INTMSK 0x10 >> +#define S5P_CSIS_INTMSK_EN_ALL 0xF000003F >> +#define S5P_CSIS_INTSRC 0x14 >> + >> +/* Pixel resolution. */ >> +#define S5P_CSIS_RESOL 0x2C >> +#define CSIS_MAX_PIX_WIDTH 0xFFFF >> +#define CSIS_MAX_PIX_HEIGHT 0xFFFF >> + >> +enum { >> + CSIS_CLK_BUS, >> + CSIS_CLK_GATE, >> +}; >> + >> +static char *csi_clock_name[] = { >> + [CSIS_CLK_BUS] = "sclk_csis", >> + [CSIS_CLK_GATE] = "csis", >> +}; >> + >> +#define NUM_CSIS_CLOCKS ARRAY_SIZE(csi_clock_name) >> + >> +struct s5p_csis_state { >> + struct v4l2_subdev sd; >> + struct v4l2_mbus_framefmt fmt; >> + struct platform_device *pdev; >> + struct resource *regs_res; >> + void __iomem *regs; >> + int irq; >> + struct clk *clock[NUM_CSIS_CLOCKS]; >> + struct regulator *supply; >> +}; >> + >> +struct s5p_csis_color_format { >> + enum v4l2_mbus_pixelcode code; >> + u32 fmt_reg; >> + u16 pix_hor_align; >> +}; >> + >> +static const struct s5p_csis_color_format s5p_csis_formats[] = { >> + { >> + .code = V4L2_MBUS_FMT_VYUY8_2X8, >> + .fmt_reg = S5P_CSIS_CFG_FMT_YCBCR422_8BIT, >> + .pix_hor_align = 1, >> + }, >> + /* TODO: Add S5P_CSIS_CFG_FMT_USER1 for JPEG format. */ >> +}; >> + >> +static struct s5p_csis_state *to_s5p_csis_state(struct v4l2_subdev *sdev) >> +{ >> + return container_of(sdev, struct s5p_csis_state, sd); >> +} >> + >> + >> +static void s5p_csis_enable_interrupts(struct s5p_csis_state *state, int on) >> +{ >> + u32 cfg = readl(state->regs + S5P_CSIS_CTRL); >> + >> + if (on) >> + cfg |= S5P_CSIS_INTMSK_EN_ALL; >> + else >> + cfg&= ~S5P_CSIS_INTMSK_EN_ALL; >> + writel(cfg, state->regs + S5P_CSIS_INTMSK); >> +} >> + >> +static void s5p_csis_reset(struct s5p_csis_state *state) >> +{ >> + u32 cfg = readl(state->regs + S5P_CSIS_CTRL); >> + >> + writel(cfg | S5P_CSIS_CTRL_RESET, state->regs + S5P_CSIS_CTRL); >> + udelay(10); >> +} >> + >> +static void s5p_csis_system_enable(struct s5p_csis_state *state, int on) >> +{ >> + u32 cfg; >> + >> + cfg = readl(state->regs + S5P_CSIS_CTRL); >> + if (on) >> + cfg |= S5P_CSIS_CTRL_ENABLE; >> + else >> + cfg&= ~S5P_CSIS_CTRL_ENABLE; >> + writel(cfg, state->regs + S5P_CSIS_CTRL); >> + >> + cfg = readl(state->regs + S5P_CSIS_DPHYCTRL); >> + if (on) >> + cfg |= S5P_CSIS_DPHYCTRL_ENABLE; >> + else >> + cfg&= ~S5P_CSIS_DPHYCTRL_ENABLE; >> + writel(cfg, state->regs + S5P_CSIS_DPHYCTRL); >> + >> + s5p_csis_enable_interrupts(state, on); >> +} >> + >> + >> +static int s5p_csis_set_format(struct s5p_csis_state *state) >> +{ >> + u32 cfg; >> + int i = ARRAY_SIZE(s5p_csis_formats); >> + >> + v4l2_dbg(1, debug,&state->sd, "fmt: %d, %d x %d\n", >> + state->fmt.code, state->fmt.width, state->fmt.height); >> + >> + /* Color format */ >> + cfg = readl(state->regs + S5P_CSIS_CONFIG); >> + cfg&= ~S5P_CSIS_CFG_FMT_MASK; >> + >> + while (i--) >> + if (state->fmt.code == s5p_csis_formats[i].code) >> + break; >> + >> + if (i>= ARRAY_SIZE(s5p_csis_formats)) >> + return -EINVAL; >> + >> + writel(cfg | s5p_csis_formats[i].fmt_reg, >> + state->regs + S5P_CSIS_CONFIG); >> + >> + /* Pixel resolution */ >> + cfg = (state->fmt.width<< 16) | state->fmt.height; >> + writel(cfg, state->regs + S5P_CSIS_RESOL); >> + >> + return 0; >> +} >> + >> +static void s5p_csis_set_hsync_settle(struct s5p_csis_state *state, int settle) >> +{ >> + u32 cfg = readl(state->regs + S5P_CSIS_DPHYCTRL); >> + >> + cfg&= ~S5P_CSIS_DPHYCTRL_HSS_MASK; >> + cfg |= (settle<< 27); >> + writel(cfg, state->regs + S5P_CSIS_DPHYCTRL); >> +} >> + >> +static void s5p_csis_set_params(struct s5p_csis_state *state) >> +{ >> + struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data; >> + u32 cfg, tmp; >> + >> + /* Number of MIPI lanes used */ >> + cfg = readl(state->regs + S5P_CSIS_CONFIG); >> + cfg&= ~S5P_CSIS_CFG_NR_LANE_MASK; >> + tmp = (pdata->lanes - 1)& 0x3; >> + writel(cfg | tmp, state->regs + S5P_CSIS_CONFIG); >> + >> + s5p_csis_set_format(state); >> + >> + s5p_csis_set_hsync_settle(state, pdata->hs_settle); >> + >> + /* CSI bus data alignment. */ >> + cfg = readl(state->regs + S5P_CSIS_CTRL); >> + >> + if (pdata->alignment == 32) >> + cfg |= S5P_CSIS_CTRL_ALIGN_32BIT; >> + else /* 24-bits */ >> + cfg&= ~S5P_CSIS_CTRL_ALIGN_32BIT; >> + >> + /* Not using external clock. */ >> + cfg&= ~S5P_CSIS_CTRL_WCLK_EXTCLK; >> + >> + writel(cfg, state->regs + S5P_CSIS_CTRL); >> + >> + /* Update the shadow register. */ >> + cfg = readl(state->regs + S5P_CSIS_CTRL); >> + writel(cfg | S5P_CSIS_CTRL_UPDATE_SHADOW, state->regs + S5P_CSIS_CTRL); >> +} >> + >> +static void s5p_csis_clk_enable(struct s5p_csis_state *state, bool enable) >> +{ >> + int i; >> + >> + for (i = 0; i< NUM_CSIS_CLOCKS; i++) { >> + if (!state->clock[i]) >> + continue; >> + if (enable) >> + clk_enable(state->clock[i]); >> + else >> + clk_disable(state->clock[i]); >> + } >> + >> + v4l2_dbg(1, debug,&state->sd, "%s: clocks %sabled\n", >> + __func__, enable ? "en" : "dis"); >> +} >> + >> +static void s5p_csis_clk_put(struct s5p_csis_state *state) >> +{ >> + int i = NUM_CSIS_CLOCKS - 1; >> + >> + while (i-->= 0) { >> + if (!state->clock[i]) >> + continue; >> + clk_disable(state->clock[i]); >> + clk_put(state->clock[i]); >> + } >> +} >> + >> +static int s5p_csis_clk_get(struct s5p_csis_state *state) >> +{ >> + struct device *dev =&state->pdev->dev; >> + int i; >> + >> + for (i = 0; i< NUM_CSIS_CLOCKS; i++) { >> + state->clock[i] = clk_get(dev, csi_clock_name[i]); >> + >> + if (IS_ERR(state->clock[i])) { >> + dev_err(dev, "failed to get clock: %s\n", >> + csi_clock_name[i]); >> + return -ENXIO; >> + } >> + clk_enable(state->clock[i]); >> + } >> + return 0; >> +} >> + >> +static int mipi_csis_power(struct v4l2_subdev *sd, int on) >> +{ >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data; >> + int ret; >> + >> + if (on) { >> + ret = regulator_enable(state->supply); >> + if (pdata->phy_enable) >> + ret = pdata->phy_enable(state->pdev, true); >> + } else { >> + if (pdata->phy_enable) { >> + ret = pdata->phy_enable(state->pdev, false); >> + if (ret) >> + return ret; >> + } >> + ret = regulator_disable(state->supply); >> + } >> + >> + if (!ret) >> + v4l2_dbg(1, debug, sd, "%s: regulator is %s\n", >> + __func__, on ? "on" : "off"); >> + return ret; >> +} >> + >> +static int s5p_csis_s_power(struct v4l2_subdev *sd, int on) >> +{ >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + int ret; >> + >> + v4l2_dbg(1, debug, sd, "%s: on= %d\n", __func__, on); >> + >> + if (on) { >> + ret = mipi_csis_power(sd, on); >> + if (ret) >> + return ret; >> + s5p_csis_clk_enable(state, on); >> + s5p_csis_reset(state); >> + } else { >> + s5p_csis_enable_interrupts(state, false); >> + s5p_csis_system_enable(state, false); >> + s5p_csis_clk_enable(state, false); >> + ret = mipi_csis_power(sd, on); >> + } >> + >> + return ret; >> +} >> + >> +static inline struct s5p_csis_color_format const *find_csis_format( >> + struct v4l2_mbus_framefmt *mf) >> +{ >> + int i = ARRAY_SIZE(s5p_csis_formats); >> + >> + while (i--) >> + if (mf->code == s5p_csis_formats[i].code) >> + break; >> + >> + if (i>= ARRAY_SIZE(s5p_csis_formats)) >> + return NULL; >> + >> + return&s5p_csis_formats[i]; >> +} >> + >> +static int s5p_csis_try_fmt(struct v4l2_subdev *sd, >> + struct v4l2_mbus_framefmt *mf) >> +{ >> + struct s5p_csis_color_format const *csis_fmt; >> + >> + if (!sd || !mf) >> + return -EINVAL; >> + >> + csis_fmt = find_csis_format(mf); >> + >> + if (csis_fmt == NULL) >> + csis_fmt =&s5p_csis_formats[0]; >> + >> + mf->code = csis_fmt->code; >> + >> + /* Adjust pixel size if required so it fits in the supported range >> + and meets the aligment requirements. */ >> + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, >> + csis_fmt->pix_hor_align, >> + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, >> + 0); >> + return 0; >> +} >> + >> +static int s5p_csis_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) >> +{ >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + struct s5p_csis_color_format const *csis_fmt = find_csis_format(mf); >> + >> + v4l2_dbg(1, debug, sd, "%s: w: %d, h: %d\n", __func__, >> + mf->width, mf->height); >> + >> + if (csis_fmt == NULL || >> + mf->width> CSIS_MAX_PIX_WIDTH || >> + mf->height> CSIS_MAX_PIX_WIDTH || >> + mf->width& (u32)(csis_fmt->pix_hor_align - 1)) >> + return -EINVAL; >> + >> + state->fmt = *mf; >> + >> + return 0; >> +} >> + >> +static int s5p_csis_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) >> +{ >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + >> + *mf = state->fmt; >> + return 0; >> +} >> + >> +static int s5p_csis_s_stream(struct v4l2_subdev *sd, int enable) >> +{ >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + >> + v4l2_dbg(1, debug, sd, "%s: arg: %d\n", __func__, enable); >> + >> + if (enable) { >> + s5p_csis_clk_enable(state, enable); >> + s5p_csis_reset(state); >> + s5p_csis_set_params(state); >> + s5p_csis_system_enable(state, enable); >> + } else { >> + s5p_csis_enable_interrupts(state, enable); >> + s5p_csis_system_enable(state, enable); >> + s5p_csis_clk_enable(state, enable); >> + } >> + >> + return 0; >> +} >> + >> +static struct v4l2_subdev_core_ops s5p_csis_core_ops = { >> + .s_power = s5p_csis_s_power, >> +}; >> + >> +static struct v4l2_subdev_video_ops s5p_csis_video_ops = { >> + .try_mbus_fmt = s5p_csis_try_fmt, >> + .g_mbus_fmt = s5p_csis_g_fmt, >> + .s_mbus_fmt = s5p_csis_s_fmt, >> + .s_stream = s5p_csis_s_stream, >> +}; >> + >> +static struct v4l2_subdev_ops s5p_csis_subdev_ops = { >> + .core =&s5p_csis_core_ops, >> + .video =&s5p_csis_video_ops, >> +}; >> + >> +static irqreturn_t s5p_csis_isr(int irq, void *dev_id) >> +{ >> + struct s5p_csis_state *state = dev_id; >> + u32 cfg; >> + >> + /* Just clear the interrupt pending bits. */ >> + cfg = readl(state->regs + S5P_CSIS_INTSRC); >> + writel(cfg, state->regs + S5P_CSIS_INTSRC); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int s5p_csis_probe(struct platform_device *pdev) >> +{ >> + struct s5p_platform_mipi_csis *pdata; >> + struct resource *mem_res; >> + struct resource *regs_res; >> + struct s5p_csis_state *state; >> + int ret = -ENODEV; >> + >> + state = kzalloc(sizeof(*state), GFP_KERNEL); >> + if (!state) >> + return -ENOMEM; >> + >> + state->pdev = pdev; >> + state->irq = -ENXIO; > > Huh? Ups, that really seems a bit pointless, will remove that line. > >> + >> + pdata = pdev->dev.platform_data; >> + if (!pdata) { >> + dev_err(&pdev->dev, "Platform data not set\n"); >> + goto p_err1; >> + } >> + >> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!mem_res) { >> + dev_err(&pdev->dev, "Failed to get io memory region\n"); >> + goto p_err1; >> + } >> + >> + regs_res = request_mem_region(mem_res->start, >> + mem_res->end - mem_res->start + 1, pdev->name); >> + if (!regs_res) { >> + dev_err(&pdev->dev, "Failed to request io memory region\n"); >> + goto p_err1; >> + } >> + state->regs_res = regs_res; >> + >> + state->regs = ioremap(mem_res->start, >> + mem_res->end - mem_res->start + 1); >> + if (!state->regs) { >> + dev_err(&pdev->dev, "Failed to remap io region\n"); >> + goto p_err2; >> + } >> + >> + if (s5p_csis_clk_get(state)) >> + goto p_err3; >> + >> + if (pdata->clk_rate) >> + clk_set_rate(state->clock[CSIS_CLK_BUS], pdata->clk_rate); >> + >> + if (pdata->phy_enable) >> + pdata->phy_enable(state->pdev, false); >> + >> + state->irq = platform_get_irq(pdev, 0); >> + if (state->irq< 0) { >> + dev_err(&pdev->dev, "failed to get irq\n"); > > Missing: ret = state->irq; Yes, obviously more appropriate than returning -ENODEV. > >> + goto p_err4; >> + } >> + >> + ret = request_irq(state->irq, s5p_csis_isr, 0, MODULE_NAME, state); >> + if (ret) { >> + dev_err(&pdev->dev, "request_irq failed\n"); >> + goto p_err4; >> + } >> + >> + state->supply = regulator_get(&pdev->dev, "mipi_csi"); >> + if (IS_ERR(state->supply)) { >> + state->supply = NULL; > > Shouldn't there be a 'ret = PTR_ERR(state->supply)' here? Yes, that's indeed my omission. Will change, thanks. > >> + goto p_err5; >> + } >> + >> + v4l2_subdev_init(&state->sd,&s5p_csis_subdev_ops); >> + state->sd.owner = THIS_MODULE; >> + strcpy(state->sd.name, MODULE_NAME); >> + >> + >> + /* This allows to retrieve the platform device id by the host driver */ >> + v4l2_set_subdevdata(&state->sd, pdev); >> + >> + /* .. and a pointer to the subdev. */ >> + platform_set_drvdata(pdev,&state->sd); >> + >> + return 0; >> + >> +p_err5: >> + free_irq(state->irq, state); >> +p_err4: >> + s5p_csis_clk_put(state); >> +p_err3: >> + iounmap(state->regs); >> +p_err2: >> + release_mem_region(regs_res->start, >> + regs_res->end - regs_res->start + 1); >> +p_err1: >> + kfree(state); >> + return ret; >> +} >> + >> +static int s5p_csis_remove(struct platform_device *pdev) >> +{ >> + struct v4l2_subdev *sd = platform_get_drvdata(pdev); >> + struct s5p_csis_state *state = to_s5p_csis_state(sd); >> + struct resource *res = state->regs_res; >> + >> + s5p_csis_clk_put(state); >> + s5p_csis_s_power(&state->sd, 0); >> + >> + free_irq(state->irq, state); >> + iounmap(state->regs); >> + release_mem_region(res->start, res->end - res->start + 1); >> + kfree(state); >> + return 0; >> +} >> + >> +static int s5p_csis_suspend(struct platform_device *pdev, pm_message_t msg) >> +{ >> + struct v4l2_subdev *sd = platform_get_drvdata(pdev); >> + >> + s5p_csis_clk_enable(to_s5p_csis_state(sd), false); >> + return 0; >> +} >> + >> +static int s5p_csis_resume(struct platform_device *pdev) >> +{ >> + struct v4l2_subdev *sd = platform_get_drvdata(pdev); >> + >> + s5p_csis_clk_enable(to_s5p_csis_state(sd), true); >> + return 0; >> +} >> + >> +static struct platform_driver s5p_csis_driver = { >> + .probe = s5p_csis_probe, >> + .remove = s5p_csis_remove, >> + .suspend = s5p_csis_suspend, >> + .resume = s5p_csis_resume, >> + .driver = { >> + .name = MODULE_NAME, >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +static int __init s5p_csis_register(void) >> +{ >> + return platform_driver_register(&s5p_csis_driver); >> +} >> + >> +static void __exit s5p_csis_unregister(void) >> +{ >> + platform_driver_unregister(&s5p_csis_driver); >> +} >> + >> +module_init(s5p_csis_register); >> +module_exit(s5p_csis_unregister); >> + >> +MODULE_AUTHOR("Sylwester Nawrocki,"); >> +MODULE_DESCRIPTION("S5P MIPI-CSI2 slave interface driver"); >> +MODULE_LICENSE("GPL"); >> > > Looks good otherwise. Thank you for the review. I am going to incorporate your comments along with fixes for the few minor issues I've found. Merry Christmas! Sylwester