| Message ID | 1514469681-15602-10-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacopo, Thank you for the patch. On Thursday, 28 December 2017 16:01:21 EET Jacopo Mondi wrote: > Remove soc_camera framework dependencies from tw9910 sensor driver. > - Handle clock and gpios > - Register async subdevice > - Remove soc_camera specific g/s_mbus_config operations > - 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 | 9 +++ > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/tw9910.c | 158 ++++++++++++++++++++++++++--------------- > include/media/i2c/tw9910.h | 9 +++ > 4 files changed, 116 insertions(+), 61 deletions(-) [snip] > diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c > index bdb5e0a..efbdebe 100644 > --- a/drivers/media/i2c/tw9910.c > +++ b/drivers/media/i2c/tw9910.c [snip] > @@ -799,8 +848,8 @@ static int tw9910_video_probe(struct i2c_client *client) > /* > * tw9910 only use 8 or 16 bit bus width > */ > - if (SOCAM_DATAWIDTH_16 != priv->info->buswidth && > - SOCAM_DATAWIDTH_8 != priv->info->buswidth) { > + if (priv->info->buswidth != 16 && > + priv->info->buswidth != 8) { No need for a line break. > dev_err(&client->dev, "bus width error\n"); > return -ENODEV; > } [snip] > @@ -959,13 +966,37 @@ static int tw9910_probe(struct i2c_client *client, > > v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops); > > - priv->clk = v4l2_clk_get(&client->dev, "mclk"); > - if (IS_ERR(priv->clk)) > + priv->clk = clk_get(&client->dev, "mclk"); The clock signal is called XTI (see page 60 of http://www.tecworth.com/ administrator/upload/200956419240864.pdf). You should add a clock alias as for the ov7725. > + if (PTR_ERR(priv->clk) == -ENOENT) { > + priv->clk = NULL; > + } else if (IS_ERR(priv->clk)) { > + dev_err(&client->dev, "Unable to get mclk clock\n"); > return PTR_ERR(priv->clk); > + } > + > + priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn", > + GPIOD_OUT_HIGH); > + if (IS_ERR(priv->pdn_gpio)) { > + dev_info(&client->dev, "Unable to get GPIO \"pdn\""); > + ret = PTR_ERR(priv->pdn_gpio); > + goto error_clk_put; > + } > > ret = tw9910_video_probe(client); > if (ret < 0) > - v4l2_clk_put(priv->clk); > + goto error_gpio_put; > + > + ret = v4l2_async_register_subdev(&priv->subdev); > + if (ret) > + goto error_gpio_put; > + > + return ret; > + > +error_gpio_put: > + if (priv->pdn_gpio) > + gpiod_put(priv->pdn_gpio); > +error_clk_put: > + clk_put(priv->clk); > > return ret; > } [snip] With these small issues fixed, and the comments to ov7725 that apply to tw9910 addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Hi Laurent, On Tue, Jan 02, 2018 at 05:50:38PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thursday, 28 December 2017 16:01:21 EET Jacopo Mondi wrote: > > - priv->clk = v4l2_clk_get(&client->dev, "mclk"); > > - if (IS_ERR(priv->clk)) > > + priv->clk = clk_get(&client->dev, "mclk"); > > The clock signal is called XTI (see page 60 of http://www.tecworth.com/ > administrator/upload/200956419240864.pdf). You should add a clock alias as for > the ov7725. I don't think Migo-R board code should provide any alias for XTI clock, as the clock is generated by an on-board oscillator and not from the SoC. Also, changing the name in the driver seems safe, as `git grep tw9910 arch/` returns that only mach-ecovec uses TW9910 (Migo-R apart) and it seems like also in that case the clock comes from an oscillator and it is not provided by the SoC. > > > + if (PTR_ERR(priv->clk) == -ENOENT) { > > + priv->clk = NULL; > > + } else if (IS_ERR(priv->clk)) { > > + dev_err(&client->dev, "Unable to get mclk clock\n"); > > return PTR_ERR(priv->clk); > > + } > > + > > + priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(priv->pdn_gpio)) { > > + dev_info(&client->dev, "Unable to get GPIO \"pdn\""); > > + ret = PTR_ERR(priv->pdn_gpio); > > + goto error_clk_put; > > + } > > > > ret = tw9910_video_probe(client); > > if (ret < 0) > > - v4l2_clk_put(priv->clk); > > + goto error_gpio_put; > > + > > + ret = v4l2_async_register_subdev(&priv->subdev); > > + if (ret) > > + goto error_gpio_put; > > + > > + return ret; > > + > > +error_gpio_put: > > + if (priv->pdn_gpio) > > + gpiod_put(priv->pdn_gpio); > > +error_clk_put: > > + clk_put(priv->clk); > > > > return ret; > > } > > [snip] > > With these small issues fixed, and the comments to ov7725 that apply to tw9910 > addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks j > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacopo, On Thu, Dec 28, 2017 at 12:01 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > + 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); This seems to be inverted. Consider you have an active low GPIO reset. In order to reset it: Put the GPIO to logic level 0 Wait some time Put the GPIO to logic level 1 gpiod_set_value(priv->rstb_gpio, 1), means the GPIO in the active state (0 in this example). , so this should be: gpiod_set_value(priv->rstb_gpio, 1); usleep_range(500, 1000); gpiod_set_value(priv->rstb_gpio, 0); -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fabio, On Wed, Jan 03, 2018 at 02:41:20PM -0200, Fabio Estevam wrote: > Hi Jacopo, > > On Thu, Dec 28, 2017 at 12:01 PM, Jacopo Mondi > <jacopo+renesas@jmondi.org> wrote: > > > + 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); > > This seems to be inverted. > > Consider you have an active low GPIO reset. > > In order to reset it: > > Put the GPIO to logic level 0 > Wait some time > Put the GPIO to logic level 1 > > gpiod_set_value(priv->rstb_gpio, 1), means the GPIO in the active > state (0 in this example). > > , so this should be: > > gpiod_set_value(priv->rstb_gpio, 1); > usleep_range(500, 1000); > gpiod_set_value(priv->rstb_gpio, 0); That would be true if I would have declared the GPIO to be ACTIVE_LOW. See patch [5/9] in this series and search for "rstb". The GPIO (which is shared between two devices) is said to be active high... Are you maybe suggesting I should declare it ACTIVE_LOW in board setup code and invert the set_value() order in the driver as you proposed? Don't you think it would be more natural for the driver to follow the current sequence, as the reset GPIO is effectively active low (so setting it to 0 put the video decoder in reset state and setting it to 1 wakes the video decoder up)? I can maybe agree with you for the PDN GPIO instead. That's said to be active high, so setting it to 1 disables the video decoder, and yes, it feels unnatural in the driver's power up routines to set PDN GPIO to 0 and set it to 1 when putting the device to sleep... Thanks j -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacopo, On Wed, Jan 3, 2018 at 3:13 PM, jacopo mondi <jacopo@jmondi.org> wrote: > That would be true if I would have declared the GPIO to be ACTIVE_LOW. > See patch [5/9] in this series and search for "rstb". The GPIO (which > is shared between two devices) is said to be active high... Just looked at your patch 5/9 and it seems it only works because you made two inversions :-) Initially the rest GPIO was doing: - gpio_set_value(GPIO_PTT3, 0); - mdelay(10); - gpio_set_value(GPIO_PTT3, 1); - mdelay(10); /* wait to let chip come out of reset */ So this is an active low reset. So you should have converted it to: GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW), and then in this patch you should do as I said earlier: gpiod_set_value(priv->rstb_gpio, 1); usleep_range(500, 1000); gpiod_set_value(priv->rstb_gpio, 0); -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fabio, On Wed, Jan 03, 2018 at 03:27:53PM -0200, Fabio Estevam wrote: > Hi Jacopo, > > On Wed, Jan 3, 2018 at 3:13 PM, jacopo mondi <jacopo@jmondi.org> wrote: > > > That would be true if I would have declared the GPIO to be ACTIVE_LOW. > > See patch [5/9] in this series and search for "rstb". The GPIO (which > > is shared between two devices) is said to be active high... > > Just looked at your patch 5/9 and it seems it only works because you > made two inversions :-) > > Initially the rest GPIO was doing: > > - gpio_set_value(GPIO_PTT3, 0); > - mdelay(10); > - gpio_set_value(GPIO_PTT3, 1); > - mdelay(10); /* wait to let chip come out of reset */ And that's what my driver code does :) > > So this is an active low reset. > Indeed > So you should have converted it to: > > GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW), > > and then in this patch you should do as I said earlier: > > gpiod_set_value(priv->rstb_gpio, 1); > usleep_range(500, 1000); > gpiod_set_value(priv->rstb_gpio, 0); My point is that if I read the manual and I see an active low gpio (0 is reset state) then the driver code uses it as and active high one (1 is the reset state), that would be weird to me. But maybe that's just me, and if that's common practice, I'll happly change this! Thanks j -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 3, 2018 at 3:37 PM, jacopo mondi <jacopo@jmondi.org> wrote: >> Initially the rest GPIO was doing: >> >> - gpio_set_value(GPIO_PTT3, 0); >> - mdelay(10); >> - gpio_set_value(GPIO_PTT3, 1); >> - mdelay(10); /* wait to let chip come out of reset */ > > And that's what my driver code does :) No, on 5/9 you converted the original code to: GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_HIGH) It should be GPIO_ACTIVE_LOW instead. > My point is that if I read the manual and I see an active low gpio (0 > is reset state) then the driver code uses it as and active high one (1 > is the reset state), that would be weird to me. Then on this patch you should do: gpiod_set_value(priv->rstb_gpio, 1); ---> This tells the GPIO to go to its active state (In this case active == logic level 0) usleep_range(500, 1000); gpiod_set_value(priv->rstb_gpio, 0); ---> This tells the GPIO to go to its inactive state (In this case inactive == logic level 1) You can also look at Documentation/gpio/consumer.txt where the usage of the gpiod_xxx API is described. It seems you are confusing it with the legacy gpio_set_value() API (Documentation/gpio/gpio-legacy.txt) Hope this helps. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fabio, On Wed, Jan 03, 2018 at 04:14:35PM -0200, Fabio Estevam wrote: > On Wed, Jan 3, 2018 at 3:37 PM, jacopo mondi <jacopo@jmondi.org> wrote: > > >> Initially the rest GPIO was doing: > >> > >> - gpio_set_value(GPIO_PTT3, 0); > >> - mdelay(10); > >> - gpio_set_value(GPIO_PTT3, 1); > >> - mdelay(10); /* wait to let chip come out of reset */ > > > > And that's what my driver code does :) > > No, on 5/9 you converted the original code to: > > GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_HIGH) > > It should be GPIO_ACTIVE_LOW instead. > > > My point is that if I read the manual and I see an active low gpio (0 > > is reset state) then the driver code uses it as and active high one (1 > > is the reset state), that would be weird to me. > > Then on this patch you should do: > > gpiod_set_value(priv->rstb_gpio, 1); ---> This tells the GPIO to go > to its active state (In this case active == logic level 0) > usleep_range(500, 1000); > gpiod_set_value(priv->rstb_gpio, 0); ---> This tells the GPIO to go to > its inactive state (In this case inactive == logic level 1) > > You can also look at Documentation/gpio/consumer.txt where the usage > of the gpiod_xxx API is described. > > It seems you are confusing it with the legacy gpio_set_value() API > (Documentation/gpio/gpio-legacy.txt) It took you 3 email messages, but maybe I finally got it. So, 1 and 0 do not actually represent the line level but the active or inactive states, that's fine. This seems to me a bit inconsistent with the existence of flags like GPIOD_OUT_HIGH/LOW meant to be used at gpiod_get() time, where the actual line level has to be used instead, but that's a discussion surely not pertinent to this series. Thanks for your patience. j > > Hope this helps. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index a61d7f4..804a1bf 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -423,6 +423,15 @@ config VIDEO_TW9906 To compile this driver as a module, choose M here: the module will be called tw9906. +config VIDEO_TW9910 + tristate "Techwell TW9910 video decoder" + depends on VIDEO_V4L2 && I2C + ---help--- + Support for Techwell TW9910 NTSC/PAL/SECAM video decoder. + + To compile this driver as a module, choose M here: the + module will be called tw9910. + config VIDEO_VPX3220 tristate "vpx3220a, vpx3216b & vpx3214c video decoders" depends on VIDEO_V4L2 && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index fb99293..e26544f 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_VIDEO_TVP7002) += tvp7002.o obj-$(CONFIG_VIDEO_TW2804) += tw2804.o obj-$(CONFIG_VIDEO_TW9903) += tw9903.o obj-$(CONFIG_VIDEO_TW9906) += tw9906.o +obj-$(CONFIG_VIDEO_TW9910) += tw9910.o obj-$(CONFIG_VIDEO_CS3308) += cs3308.o obj-$(CONFIG_VIDEO_CS5345) += cs5345.o obj-$(CONFIG_VIDEO_CS53L32A) += cs53l32a.o diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c index bdb5e0a..efbdebe 100644 --- a/drivers/media/i2c/tw9910.c +++ b/drivers/media/i2c/tw9910.c @@ -1,6 +1,9 @@ +// SPDX-License-Identifier: GPL-2.0+ /* * tw9910 Video Driver * + * Copyright (C) 2017 Jacopo Mondi <jacopo+renesas@jmondi.org> + * * Copyright (C) 2008 Renesas Solutions Corp. * Kuninori Morimoto <morimoto.kuninori@renesas.com> * @@ -10,12 +13,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/module.h> #include <linux/i2c.h> @@ -25,9 +26,7 @@ #include <linux/v4l2-mediabus.h> #include <linux/videodev2.h> -#include <media/soc_camera.h> #include <media/i2c/tw9910.h> -#include <media/v4l2-clk.h> #include <media/v4l2-subdev.h> #define GET_ID(val) ((val & 0xF8) >> 3) @@ -228,8 +227,10 @@ struct tw9910_scale_ctrl { struct tw9910_priv { struct v4l2_subdev subdev; - struct v4l2_clk *clk; + struct clk *clk; struct tw9910_video_info *info; + struct gpio_desc *pdn_gpio; + struct gpio_desc *rstb_gpio; const struct tw9910_scale_ctrl *scale; v4l2_std_id norm; u32 revision; @@ -582,13 +583,61 @@ static int tw9910_s_register(struct v4l2_subdev *sd, } #endif +static int tw9910_power_on(struct tw9910_priv *priv) +{ + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev); + int ret; + + if (priv->clk) { + ret = clk_prepare_enable(priv->clk); + if (ret) + return ret; + } + + if (priv->pdn_gpio) { + gpiod_set_value(priv->pdn_gpio, 0); + 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 tw9910_power_off(struct tw9910_priv *priv) +{ + clk_disable_unprepare(priv->clk); + + if (priv->pdn_gpio) { + gpiod_set_value(priv->pdn_gpio, 1); + usleep_range(500, 1000); + } + + return 0; +} + static int tw9910_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 tw9910_priv *priv = to_tw9910(client); - return soc_camera_set_power(&client->dev, ssdd, priv->clk, on); + return on ? tw9910_power_on(priv) : + tw9910_power_off(priv); } static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height) @@ -614,7 +663,7 @@ static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height) * set bus width */ val = 0x00; - if (SOCAM_DATAWIDTH_16 == priv->info->buswidth) + if (priv->info->buswidth == 16) val = LEN; ret = tw9910_mask_set(client, OPFORM, LEN, val); @@ -799,8 +848,8 @@ static int tw9910_video_probe(struct i2c_client *client) /* * tw9910 only use 8 or 16 bit bus width */ - if (SOCAM_DATAWIDTH_16 != priv->info->buswidth && - SOCAM_DATAWIDTH_8 != priv->info->buswidth) { + if (priv->info->buswidth != 16 && + priv->info->buswidth != 8) { dev_err(&client->dev, "bus width error\n"); return -ENODEV; } @@ -856,45 +905,6 @@ static int tw9910_enum_mbus_code(struct v4l2_subdev *sd, return 0; } -static int tw9910_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_VSYNC_ACTIVE_LOW | - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW | - V4L2_MBUS_DATA_ACTIVE_HIGH; - cfg->type = V4L2_MBUS_PARALLEL; - cfg->flags = soc_camera_apply_board_flags(ssdd, cfg); - - return 0; -} - -static int tw9910_s_mbus_config(struct v4l2_subdev *sd, - const 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); - u8 val = VSSL_VVALID | HSSL_DVALID; - unsigned long flags = soc_camera_apply_board_flags(ssdd, cfg); - - /* - * set OUTCTR1 - * - * We use VVALID and DVALID signals to control VSYNC and HSYNC - * outputs, in this mode their polarity is inverted. - */ - if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) - val |= HSP_HI; - - if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) - val |= VSP_HI; - - return i2c_smbus_write_byte_data(client, OUTCTR1, val); -} - static int tw9910_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm) { *norm = V4L2_STD_NTSC | V4L2_STD_PAL; @@ -905,8 +915,6 @@ static const struct v4l2_subdev_video_ops tw9910_subdev_video_ops = { .s_std = tw9910_s_std, .g_std = tw9910_g_std, .s_stream = tw9910_s_stream, - .g_mbus_config = tw9910_g_mbus_config, - .s_mbus_config = tw9910_s_mbus_config, .g_tvnorms = tw9910_g_tvnorms, }; @@ -935,15 +943,14 @@ static int tw9910_probe(struct i2c_client *client, struct tw9910_video_info *info; struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); - struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client); int ret; - if (!ssdd || !ssdd->drv_priv) { + if (!client->dev.platform_data) { dev_err(&client->dev, "TW9910: missing platform data!\n"); return -EINVAL; } - info = ssdd->drv_priv; + info = client->dev.platform_data; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { dev_err(&client->dev, @@ -959,13 +966,37 @@ static int tw9910_probe(struct i2c_client *client, v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops); - priv->clk = v4l2_clk_get(&client->dev, "mclk"); - if (IS_ERR(priv->clk)) + priv->clk = clk_get(&client->dev, "mclk"); + if (PTR_ERR(priv->clk) == -ENOENT) { + priv->clk = NULL; + } else if (IS_ERR(priv->clk)) { + dev_err(&client->dev, "Unable to get mclk clock\n"); return PTR_ERR(priv->clk); + } + + priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn", + GPIOD_OUT_HIGH); + if (IS_ERR(priv->pdn_gpio)) { + dev_info(&client->dev, "Unable to get GPIO \"pdn\""); + ret = PTR_ERR(priv->pdn_gpio); + goto error_clk_put; + } ret = tw9910_video_probe(client); if (ret < 0) - v4l2_clk_put(priv->clk); + goto error_gpio_put; + + ret = v4l2_async_register_subdev(&priv->subdev); + if (ret) + goto error_gpio_put; + + return ret; + +error_gpio_put: + if (priv->pdn_gpio) + gpiod_put(priv->pdn_gpio); +error_clk_put: + clk_put(priv->clk); return ret; } @@ -973,7 +1004,12 @@ static int tw9910_probe(struct i2c_client *client, static int tw9910_remove(struct i2c_client *client) { struct tw9910_priv *priv = to_tw9910(client); - v4l2_clk_put(priv->clk); + + if (priv->pdn_gpio) + gpiod_put(priv->pdn_gpio); + clk_put(priv->clk); + v4l2_device_unregister_subdev(&priv->subdev); + return 0; } @@ -994,6 +1030,6 @@ static struct i2c_driver tw9910_i2c_driver = { module_i2c_driver(tw9910_i2c_driver); -MODULE_DESCRIPTION("SoC Camera driver for tw9910"); +MODULE_DESCRIPTION("V4L2 driver for TW9910 video decoder"); MODULE_AUTHOR("Kuninori Morimoto"); MODULE_LICENSE("GPL v2"); diff --git a/include/media/i2c/tw9910.h b/include/media/i2c/tw9910.h index 90bcf1f..bec8f7b 100644 --- a/include/media/i2c/tw9910.h +++ b/include/media/i2c/tw9910.h @@ -18,6 +18,9 @@ #include <media/soc_camera.h> +/** + * tw9910_mpout_pin - MPOUT (multi-purpose output) pin functions + */ enum tw9910_mpout_pin { TW9910_MPO_VLOSS, TW9910_MPO_HLOCK, @@ -29,6 +32,12 @@ enum tw9910_mpout_pin { TW9910_MPO_RTCO, }; +/** + * tw9910_video_info - tw9910 driver interface structure + * @buswidth: Parallel data bus width (8 or 16). + * @mpout: Selected function of MPOUT (multi-purpose output) pin. + * See &enum tw9910_mpout_pin + */ struct tw9910_video_info { unsigned long buswidth; enum tw9910_mpout_pin mpout;
Remove soc_camera framework dependencies from tw9910 sensor driver. - Handle clock and gpios - Register async subdevice - Remove soc_camera specific g/s_mbus_config operations - 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 | 9 +++ drivers/media/i2c/Makefile | 1 + drivers/media/i2c/tw9910.c | 158 ++++++++++++++++++++++++++++----------------- include/media/i2c/tw9910.h | 9 +++ 4 files changed, 116 insertions(+), 61 deletions(-)