| Message ID | 1514469681-15602-8-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Jacopo, Thank you for the patch. On Thursday, 28 December 2017 16:01:19 EET Jacopo Mondi wrote: > Remove soc_camera framework dependencies from ov772x sensor driver. > - Handle clock and gpios > - Register async subdevice > - Remove soc_camera specific g/s_mbus_config operations > - Change image format colorspace to SRGB Could you explain the rationale for this ? > - Remove sizes crop from get_selection as driver can't scale > - Add kernel doc to driver interface header file > - Adjust build system That's a lot for a single patch. On the other hand I don't think splitting this in 7 patches would be a good idea either. If you can find a better granularity, go for it, otherwise keep it as-is. Same comment for the tw9910 driver. > This commit does not remove the original soc_camera based driver as long > as other platforms depends on soc_camera-based CEU driver. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/Kconfig | 11 +++ > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov772x.c | 169 ++++++++++++++++++++++++++++------------- > include/media/i2c/ov772x.h | 8 ++- > 4 files changed, 130 insertions(+), 59 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index cb5d7ff..a61d7f4 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig [snip] > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 8063835..f7b293f 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -1,6 +1,9 @@ [snip] > @@ -25,8 +26,8 @@ > #include <linux/videodev2.h> > > #include <media/i2c/ov772x.h> > -#include <media/soc_camera.h> > -#include <media/v4l2-clk.h> > + > +#include <media/v4l2-device.h> > #include <media/v4l2-ctrls.h> I think C comes before D. > #include <media/v4l2-subdev.h> > #include <media/v4l2-image-sizes.h> [snip] > @@ -650,13 +653,63 @@ static int ov772x_s_register(struct v4l2_subdev *sd, > } > #endif > > +static int ov772x_power_on(struct ov772x_priv *priv) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev); > + int ret; > + > + if (priv->info->xclk_rate) > + ret = clk_set_rate(priv->clk, priv->info->xclk_rate); The return value is then ignored. I wonder whether the clk_set_rate() call shouldn't be kept in board code as it's a board-specific frequency. DT platforms would use the assigned-clock- rates property that doesn't require any explicit handling in the driver. > + if (priv->clk) { > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + } > + > + if (priv->pwdn_gpio) { > + gpiod_set_value(priv->pwdn_gpio, 1); > + usleep_range(500, 1000); > + } > + > + /* Reset GPIOs are shared in some platforms. */ I'd make this a FIXME comment as this is really a hack. /* * FIXME: The reset signal is connected to a shared GPIO on some * platforms (namely the SuperH Migo-R). Until a framework becomes * available to handle this cleanly, request the GPIO temporarily * only to avoid conflicts. */ Same for the tw9910 driver. > + priv->rstb_gpio = gpiod_get_optional(&client->dev, "rstb", > + GPIOD_OUT_LOW); > + if (IS_ERR(priv->rstb_gpio)) { > + dev_info(&client->dev, "Unable to get GPIO \"rstb\""); > + return PTR_ERR(priv->rstb_gpio); > + } > + > + if (priv->rstb_gpio) { > + gpiod_set_value(priv->rstb_gpio, 0); > + usleep_range(500, 1000); > + gpiod_set_value(priv->rstb_gpio, 1); > + usleep_range(500, 1000); > + > + gpiod_put(priv->rstb_gpio); > + } > + > + return 0; > +} [snip] -- Regards, Laurent Pinchart
Hi Laurent, On Tue, Jan 02, 2018 at 05:44:03PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thursday, 28 December 2017 16:01:19 EET Jacopo Mondi wrote: > > Remove soc_camera framework dependencies from ov772x sensor driver. > > - Handle clock and gpios > > - Register async subdevice > > - Remove soc_camera specific g/s_mbus_config operations > > - Change image format colorspace to SRGB > > Could you explain the rationale for this ? Hans suggested this, and I assume it is beacause COLORSPACE_JPEG == (COLORSPACE_SRGB + assumptions on quantization ranges) which does not apply to the sensor. > > > - Remove sizes crop from get_selection as driver can't scale > > - Add kernel doc to driver interface header file > > - Adjust build system > > That's a lot for a single patch. On the other hand I don't think splitting > this in 7 patches would be a good idea either. If you can find a better > granularity, go for it, otherwise keep it as-is. Same comment for the tw9910 > driver. I know. I would have kept changes down to the minimum required to remove soc_camera dependencies, but I received comments on other parts of the driver not directly soc_camera specific. I understand this, since I'm touching the driver it is maybe worth changing some parts of it that needs updates.. > > > This commit does not remove the original soc_camera based driver as long > > as other platforms depends on soc_camera-based CEU driver. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/Kconfig | 11 +++ > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov772x.c | 169 ++++++++++++++++++++++++++++------------- > > include/media/i2c/ov772x.h | 8 ++- > > 4 files changed, 130 insertions(+), 59 deletions(-) > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index cb5d7ff..a61d7f4 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > [snip] > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > > index 8063835..f7b293f 100644 > > --- a/drivers/media/i2c/ov772x.c > > +++ b/drivers/media/i2c/ov772x.c > > @@ -1,6 +1,9 @@ > > [snip] > > > @@ -25,8 +26,8 @@ > > #include <linux/videodev2.h> > > > > #include <media/i2c/ov772x.h> > > -#include <media/soc_camera.h> > > -#include <media/v4l2-clk.h> > > + > > +#include <media/v4l2-device.h> > > #include <media/v4l2-ctrls.h> > > I think C comes before D. > > > #include <media/v4l2-subdev.h> > > #include <media/v4l2-image-sizes.h> > > [snip] > > > @@ -650,13 +653,63 @@ static int ov772x_s_register(struct v4l2_subdev *sd, > > } > > #endif > > > > +static int ov772x_power_on(struct ov772x_priv *priv) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev); > > + int ret; > > + > > + if (priv->info->xclk_rate) > > + ret = clk_set_rate(priv->clk, priv->info->xclk_rate); > > The return value is then ignored. > > I wonder whether the clk_set_rate() call shouldn't be kept in board code as > it's a board-specific frequency. DT platforms would use the assigned-clock- > rates property that doesn't require any explicit handling in the driver. > DT based platforms won't have any info->xlkc_rate, so they should be fine. I wonder how could I set rate in board code, as I'm just registering an alias for the clock there... > > + if (priv->clk) { > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) > > + return ret; > > + } > > + > > + if (priv->pwdn_gpio) { > > + gpiod_set_value(priv->pwdn_gpio, 1); > > + usleep_range(500, 1000); > > + } > > + > > + /* Reset GPIOs are shared in some platforms. */ > > I'd make this a FIXME comment as this is really a hack. > > /* > * FIXME: The reset signal is connected to a shared GPIO on some > * platforms (namely the SuperH Migo-R). Until a framework becomes > * available to handle this cleanly, request the GPIO temporarily > * only to avoid conflicts. > */ > > Same for the tw9910 driver. Ack. Thanks j
Hi Jacopo, On Wednesday, 3 January 2018 17:44:58 EET jacopo mondi wrote: > On Tue, Jan 02, 2018 at 05:44:03PM +0200, Laurent Pinchart wrote: > > On Thursday, 28 December 2017 16:01:19 EET Jacopo Mondi wrote: > >> Remove soc_camera framework dependencies from ov772x sensor driver. > >> - Handle clock and gpios > >> - Register async subdevice > >> - Remove soc_camera specific g/s_mbus_config operations > >> - Change image format colorspace to SRGB > > > > Could you explain the rationale for this ? > > Hans suggested this, and I assume it is beacause COLORSPACE_JPEG == > (COLORSPACE_SRGB + assumptions on quantization ranges) which does not > apply to the sensor. Could you capture it in the commit message ? :-) > >> - Remove sizes crop from get_selection as driver can't scale > >> - Add kernel doc to driver interface header file > >> - Adjust build system > > > > That's a lot for a single patch. On the other hand I don't think splitting > > this in 7 patches would be a good idea either. If you can find a better > > granularity, go for it, otherwise keep it as-is. Same comment for the > > tw9910 driver. > > I know. > I would have kept changes down to the minimum required to remove > soc_camera dependencies, but I received comments on other parts of the > driver not directly soc_camera specific. I understand this, since I'm > touching the driver it is maybe worth changing some parts of it that > needs updates.. > > >> This commit does not remove the original soc_camera based driver as long > >> as other platforms depends on soc_camera-based CEU driver. > >> > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >> --- > >> > >> drivers/media/i2c/Kconfig | 11 +++ > >> drivers/media/i2c/Makefile | 1 + > >> drivers/media/i2c/ov772x.c | 169 +++++++++++++++++++++++++------------- > >> include/media/i2c/ov772x.h | 8 ++- > >> 4 files changed, 130 insertions(+), 59 deletions(-) > >> > >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > >> index cb5d7ff..a61d7f4 100644 > >> --- a/drivers/media/i2c/Kconfig > >> +++ b/drivers/media/i2c/Kconfig > > > > [snip] > > > >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > >> index 8063835..f7b293f 100644 > >> --- a/drivers/media/i2c/ov772x.c > >> +++ b/drivers/media/i2c/ov772x.c > >> @@ -1,6 +1,9 @@ > > > > [snip] > > > >> @@ -25,8 +26,8 @@ > >> > >> #include <linux/videodev2.h> > >> > >> #include <media/i2c/ov772x.h> > > > > > > -#include <media/soc_camera.h> > > > -#include <media/v4l2-clk.h> > > > + > > > +#include <media/v4l2-device.h> > > > > > > #include <media/v4l2-ctrls.h> > > > > I think C comes before D. > > > > > #include <media/v4l2-subdev.h> > > > #include <media/v4l2-image-sizes.h> > > > > [snip] > > > >> @@ -650,13 +653,63 @@ static int ov772x_s_register(struct v4l2_subdev > >> *sd, > >> } > >> #endif > >> > >> +static int ov772x_power_on(struct ov772x_priv *priv) > >> +{ > >> + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev); > >> + int ret; > >> + > >> + if (priv->info->xclk_rate) > >> + ret = clk_set_rate(priv->clk, priv->info->xclk_rate); > > > > The return value is then ignored. > > > > I wonder whether the clk_set_rate() call shouldn't be kept in board code > > as it's a board-specific frequency. DT platforms would use the > > assigned-clock-rates property that doesn't require any explicit handling > > in the driver. > > DT based platforms won't have any info->xlkc_rate, so they should be > fine. I wonder how could I set rate in board code, as I'm just > registering an alias for the clock there... Exactly as done by the current code, get the clock and set the rate :) You can do that at initialization time, when you register the alias. Don't forget to put the clock too. > >> + if (priv->clk) { > >> + ret = clk_prepare_enable(priv->clk); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + if (priv->pwdn_gpio) { > >> + gpiod_set_value(priv->pwdn_gpio, 1); > >> + usleep_range(500, 1000); > >> + } > >> + > >> + /* Reset GPIOs are shared in some platforms. */ > > > > I'd make this a FIXME comment as this is really a hack. > > > > /* > > * FIXME: The reset signal is connected to a shared GPIO on some > > * platforms (namely the SuperH Migo-R). Until a framework becomes > > * available to handle this cleanly, request the GPIO temporarily > > * only to avoid conflicts. > > */ > > > > Same for the tw9910 driver. > > Ack.
Hi Laurent, On Wed, Jan 03, 2018 at 05:49:55PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > > > > DT based platforms won't have any info->xlkc_rate, so they should be > > fine. I wonder how could I set rate in board code, as I'm just > > registering an alias for the clock there... > > Exactly as done by the current code, get the clock and set the rate :) You can > do that at initialization time, when you register the alias. Don't forget to > put the clock too. Interesting. I had no idea a clock rate is retained between get/put cycles. I'll set the rate in board code (and now that I'm looking at other SH boards code, it seems pretty common to do as a thing). Thanks j > > > >> + if (priv->clk) { > > >> + ret = clk_prepare_enable(priv->clk); > > >> + if (ret) > > >> + return ret; > > >> + } > > >> + > > >> + if (priv->pwdn_gpio) { > > >> + gpiod_set_value(priv->pwdn_gpio, 1); > > >> + usleep_range(500, 1000); > > >> + } > > >> + > > >> + /* Reset GPIOs are shared in some platforms. */ > > > > > > I'd make this a FIXME comment as this is really a hack. > > > > > > /* > > > * FIXME: The reset signal is connected to a shared GPIO on some > > > * platforms (namely the SuperH Migo-R). Until a framework becomes > > > * available to handle this cleanly, request the GPIO temporarily > > > * only to avoid conflicts. > > > */ > > > > > > Same for the tw9910 driver. > > > > Ack. > > -- > Regards, > > Laurent Pinchart >
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index cb5d7ff..a61d7f4 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -645,6 +645,17 @@ config VIDEO_OV5670 To compile this driver as a module, choose M here: the module will be called ov5670. +config VIDEO_OV772X + tristate "OmniVision OV772x sensor support" + depends on I2C && VIDEO_V4L2 + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor-level driver for the OmniVision + OV772x camera. + + To compile this driver as a module, choose M here: the + module will be called ov772x. + config VIDEO_OV7640 tristate "OmniVision OV7640 sensor support" depends on I2C && VIDEO_V4L2 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 548a9ef..fb99293 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_VIDEO_OV5645) += ov5645.o obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o +obj-$(CONFIG_VIDEO_OV772X) += ov772x.o obj-$(CONFIG_VIDEO_OV7640) += ov7640.o obj-$(CONFIG_VIDEO_OV7670) += ov7670.o obj-$(CONFIG_VIDEO_OV9650) += ov9650.o diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c index 8063835..f7b293f 100644 --- a/drivers/media/i2c/ov772x.c +++ b/drivers/media/i2c/ov772x.c @@ -1,6 +1,9 @@ +// SPDX-License-Identifier: GPL-2.0+ /* * ov772x Camera Driver * + * Copyright (C) 2017 Jacopo Mondi <jacopo+renesas@jmondi.org> + * * Copyright (C) 2008 Renesas Solutions Corp. * Kuninori Morimoto <morimoto.kuninori@renesas.com> * @@ -9,12 +12,10 @@ * Copyright 2006-7 Jonathan Corbet <corbet@lwn.net> * Copyright (C) 2008 Magnus Damm * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> - * - * 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/clk.h> +#include <linux/gpio/consumer.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h> @@ -25,8 +26,8 @@ #include <linux/videodev2.h> #include <media/i2c/ov772x.h> -#include <media/soc_camera.h> -#include <media/v4l2-clk.h> + +#include <media/v4l2-device.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-subdev.h> #include <media/v4l2-image-sizes.h> @@ -393,8 +394,10 @@ struct ov772x_win_size { struct ov772x_priv { struct v4l2_subdev subdev; struct v4l2_ctrl_handler hdl; - struct v4l2_clk *clk; + struct clk *clk; struct ov772x_camera_info *info; + struct gpio_desc *pwdn_gpio; + struct gpio_desc *rstb_gpio; const struct ov772x_color_format *cfmt; const struct ov772x_win_size *win; unsigned short flag_vflip:1; @@ -409,7 +412,7 @@ struct ov772x_priv { static const struct ov772x_color_format ov772x_cfmts[] = { { .code = MEDIA_BUS_FMT_YUYV8_2X8, - .colorspace = V4L2_COLORSPACE_JPEG, + .colorspace = V4L2_COLORSPACE_SRGB, .dsp3 = 0x0, .dsp4 = DSP_OFMT_YUV, .com3 = SWAP_YUV, @@ -417,7 +420,7 @@ static const struct ov772x_color_format ov772x_cfmts[] = { }, { .code = MEDIA_BUS_FMT_YVYU8_2X8, - .colorspace = V4L2_COLORSPACE_JPEG, + .colorspace = V4L2_COLORSPACE_SRGB, .dsp3 = UV_ON, .dsp4 = DSP_OFMT_YUV, .com3 = SWAP_YUV, @@ -425,7 +428,7 @@ static const struct ov772x_color_format ov772x_cfmts[] = { }, { .code = MEDIA_BUS_FMT_UYVY8_2X8, - .colorspace = V4L2_COLORSPACE_JPEG, + .colorspace = V4L2_COLORSPACE_SRGB, .dsp3 = 0x0, .dsp4 = DSP_OFMT_YUV, .com3 = 0x0, @@ -550,7 +553,7 @@ static int ov772x_reset(struct i2c_client *client) } /* - * soc_camera_ops function + * subdev ops */ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable) @@ -650,13 +653,63 @@ static int ov772x_s_register(struct v4l2_subdev *sd, } #endif +static int ov772x_power_on(struct ov772x_priv *priv) +{ + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev); + int ret; + + if (priv->info->xclk_rate) + ret = clk_set_rate(priv->clk, priv->info->xclk_rate); + + if (priv->clk) { + ret = clk_prepare_enable(priv->clk); + if (ret) + return ret; + } + + if (priv->pwdn_gpio) { + gpiod_set_value(priv->pwdn_gpio, 1); + usleep_range(500, 1000); + } + + /* Reset GPIOs are shared in some platforms. */ + priv->rstb_gpio = gpiod_get_optional(&client->dev, "rstb", + GPIOD_OUT_LOW); + if (IS_ERR(priv->rstb_gpio)) { + dev_info(&client->dev, "Unable to get GPIO \"rstb\""); + return PTR_ERR(priv->rstb_gpio); + } + + if (priv->rstb_gpio) { + gpiod_set_value(priv->rstb_gpio, 0); + usleep_range(500, 1000); + gpiod_set_value(priv->rstb_gpio, 1); + usleep_range(500, 1000); + + gpiod_put(priv->rstb_gpio); + } + + return 0; +} + +static int ov772x_power_off(struct ov772x_priv *priv) +{ + clk_disable_unprepare(priv->clk); + + if (priv->pwdn_gpio) { + gpiod_set_value(priv->pwdn_gpio, 0); + usleep_range(500, 1000); + } + + return 0; +} + static int ov772x_s_power(struct v4l2_subdev *sd, int on) { - struct i2c_client *client = v4l2_get_subdevdata(sd); - struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client); struct ov772x_priv *priv = to_ov772x(sd); - return soc_camera_set_power(&client->dev, ssdd, priv->clk, on); + return on ? ov772x_power_on(priv) : + ov772x_power_off(priv); } static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height) @@ -855,24 +908,21 @@ static int ov772x_get_selection(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_selection *sel) { + struct ov772x_priv *priv = to_ov772x(sd); + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) return -EINVAL; - sel->r.left = 0; - sel->r.top = 0; switch (sel->target) { case V4L2_SEL_TGT_CROP_BOUNDS: case V4L2_SEL_TGT_CROP_DEFAULT: - sel->r.width = OV772X_MAX_WIDTH; - sel->r.height = OV772X_MAX_HEIGHT; - return 0; case V4L2_SEL_TGT_CROP: - sel->r.width = VGA_WIDTH; - sel->r.height = VGA_HEIGHT; - return 0; - default: - return -EINVAL; + sel->r.width = priv->win->rect.width; + sel->r.height = priv->win->rect.height; + break; } + + return 0; } static int ov772x_get_fmt(struct v4l2_subdev *sd, @@ -997,24 +1047,8 @@ static int ov772x_enum_mbus_code(struct v4l2_subdev *sd, return 0; } -static int ov772x_g_mbus_config(struct v4l2_subdev *sd, - struct v4l2_mbus_config *cfg) -{ - struct i2c_client *client = v4l2_get_subdevdata(sd); - struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client); - - cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER | - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH | - V4L2_MBUS_DATA_ACTIVE_HIGH; - cfg->type = V4L2_MBUS_PARALLEL; - cfg->flags = soc_camera_apply_board_flags(ssdd, cfg); - - return 0; -} - static const struct v4l2_subdev_video_ops ov772x_subdev_video_ops = { .s_stream = ov772x_s_stream, - .g_mbus_config = ov772x_g_mbus_config, }; static const struct v4l2_subdev_pad_ops ov772x_subdev_pad_ops = { @@ -1038,12 +1072,11 @@ static int ov772x_probe(struct i2c_client *client, const struct i2c_device_id *did) { struct ov772x_priv *priv; - struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client); - struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); + struct i2c_adapter *adapter = client->adapter; int ret; - if (!ssdd || !ssdd->drv_priv) { - dev_err(&client->dev, "OV772X: missing platform data!\n"); + if (!client->dev.platform_data) { + dev_err(&client->dev, "Missing OV7725 platform data\n"); return -EINVAL; } @@ -1059,7 +1092,7 @@ static int ov772x_probe(struct i2c_client *client, if (!priv) return -ENOMEM; - priv->info = ssdd->drv_priv; + priv->info = client->dev.platform_data; v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops); v4l2_ctrl_handler_init(&priv->hdl, 3); @@ -1073,22 +1106,42 @@ static int ov772x_probe(struct i2c_client *client, if (priv->hdl.error) return priv->hdl.error; - priv->clk = v4l2_clk_get(&client->dev, "mclk"); + priv->clk = clk_get(&client->dev, "xclk"); if (IS_ERR(priv->clk)) { + dev_err(&client->dev, "Unable to get xclk clock\n"); ret = PTR_ERR(priv->clk); - goto eclkget; + goto error_ctrl_free; } - ret = ov772x_video_probe(priv); - if (ret < 0) { - v4l2_clk_put(priv->clk); -eclkget: - v4l2_ctrl_handler_free(&priv->hdl); - } else { - priv->cfmt = &ov772x_cfmts[0]; - priv->win = &ov772x_win_sizes[0]; + priv->pwdn_gpio = gpiod_get_optional(&client->dev, "pwdn", + GPIOD_OUT_LOW); + if (IS_ERR(priv->pwdn_gpio)) { + dev_info(&client->dev, "Unable to get GPIO \"pwdn\""); + ret = PTR_ERR(priv->pwdn_gpio); + goto error_clk_put; } + ret = ov772x_video_probe(priv); + if (ret < 0) + goto error_gpio_put; + + priv->cfmt = &ov772x_cfmts[0]; + priv->win = &ov772x_win_sizes[0]; + + ret = v4l2_async_register_subdev(&priv->subdev); + if (ret) + goto error_gpio_put; + + return 0; + +error_gpio_put: + if (priv->pwdn_gpio) + gpiod_put(priv->pwdn_gpio); +error_clk_put: + clk_put(priv->clk); +error_ctrl_free: + v4l2_ctrl_handler_free(&priv->hdl); + return ret; } @@ -1096,7 +1149,9 @@ static int ov772x_remove(struct i2c_client *client) { struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client)); - v4l2_clk_put(priv->clk); + clk_put(priv->clk); + if (priv->pwdn_gpio) + gpiod_put(priv->pwdn_gpio); v4l2_device_unregister_subdev(&priv->subdev); v4l2_ctrl_handler_free(&priv->hdl); return 0; @@ -1119,6 +1174,6 @@ static struct i2c_driver ov772x_i2c_driver = { module_i2c_driver(ov772x_i2c_driver); -MODULE_DESCRIPTION("SoC Camera driver for ov772x"); +MODULE_DESCRIPTION("V4L2 driver for OV772x image sensor"); MODULE_AUTHOR("Kuninori Morimoto"); MODULE_LICENSE("GPL v2"); diff --git a/include/media/i2c/ov772x.h b/include/media/i2c/ov772x.h index 00dbb7c..101f9fe 100644 --- a/include/media/i2c/ov772x.h +++ b/include/media/i2c/ov772x.h @@ -48,12 +48,16 @@ struct ov772x_edge_ctrl { .threshold = (t & OV772X_EDGE_THRESHOLD_MASK), \ } -/* - * ov772x camera info +/** + * ov772x_camera_info - ov772x driver interface structure + * @flags: Sensor configuration flags + * @edgectrl: Sensor edge control + * @xclk_rate: xclk clock frequency (in Hz) */ struct ov772x_camera_info { unsigned long flags; struct ov772x_edge_ctrl edgectrl; + unsigned int xclk_rate; }; #endif /* __OV772X_H__ */
Remove soc_camera framework dependencies from ov772x sensor driver. - Handle clock and gpios - Register async subdevice - Remove soc_camera specific g/s_mbus_config operations - Change image format colorspace to SRGB - Remove sizes crop from get_selection as driver can't scale - Add kernel doc to driver interface header file - Adjust build system This commit does not remove the original soc_camera based driver as long as other platforms depends on soc_camera-based CEU driver. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/Kconfig | 11 +++ drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov772x.c | 169 ++++++++++++++++++++++++++++++--------------- include/media/i2c/ov772x.h | 8 ++- 4 files changed, 130 insertions(+), 59 deletions(-)