[Patch 0/6] media: i2c: ov2659: maintenance series

[Patch 0/6] media: i2c: ov2659: maintenance series

[Benoit Parrot]

This patch series is a collection of patches we have been carrying for a
while.

It includes a few sensor register fixes which would cause visual
artifacts at lower resolution and also at 720p.

Also on some board the 'powerdown' is not tied to always on so we add
support for an optional powerdown gpio.

Since these camera are removable on some board we alos need the driver
to actually fail when there is no hardware present so the driver is
actually removed.

Finally, we update the licensing statement to use SPDX licensing.

Benoit Parrot (6):
  media: i2c: ov2659: Fix for image wrap-around in lower resolution
  media: i2c: ov2659: Fix sensor detection to actually fail when device
    is not present
  media: dt-bindings: ov2659: add powerdown-gpios optional property
  media: i2c: ov2659: Add optional powerdown gpio handling
  media: i2c: ov2659: Fix missing 720p register config
  media: i2c: ov2659: Switch to SPDX Licensing

 .../devicetree/bindings/media/i2c/ov2659.txt  |  6 +++
 drivers/media/i2c/Kconfig                     |  2 +-
 drivers/media/i2c/ov2659.c                    | 40 +++++++++++--------
 3 files changed, 30 insertions(+), 18 deletions(-)

-- 
2.17.1

[Patch 1/6] media: i2c: ov2659: Fix for image wrap-around in lower resolution

[Benoit Parrot]

Based on recently found sensor configuration examples, it was
discovered that when scaling and binning are used for the lower
resolutions (i.e. 640x480, 320x240) the read offset has to be
increased otherwise the image appears to be wrapped around.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/media/i2c/ov2659.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index f4ded0669ff9..17573257097d 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -661,7 +661,7 @@ static struct sensor_register ov2659_vga[] = {
 	{ REG_TIMING_HORIZ_FORMAT, 0x01 },
 	{ 0x370a, 0x52 },
 	{ REG_VFIFO_READ_START_H, 0x00 },
-	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_VFIFO_READ_START_L, 0xa0 },
 	{ REG_ISP_CTRL02, 0x10 },
 	{ REG_NULL, 0x00 },
 };
@@ -709,7 +709,7 @@ static  struct sensor_register ov2659_qvga[] = {
 	{ REG_TIMING_HORIZ_FORMAT, 0x01 },
 	{ 0x370a, 0x52 },
 	{ REG_VFIFO_READ_START_H, 0x00 },
-	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_VFIFO_READ_START_L, 0xa0 },
 	{ REG_ISP_CTRL02, 0x10 },
 	{ REG_NULL, 0x00 },
 };
-- 
2.17.1

[Patch 2/6] media: i2c: ov2659: Fix sensor detection to actually fail when device is not present

[Benoit Parrot]

Make sure that if the expected sensor device id register
is not recognized properly the failure is propagated
up so devices are not left partially initialized.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/media/i2c/ov2659.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index 17573257097d..efbe6dc720e2 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1330,11 +1330,12 @@ static int ov2659_detect(struct v4l2_subdev *sd)
 		unsigned short id;
 
 		id = OV265X_ID(pid, ver);
-		if (id != OV2659_ID)
+		if (id != OV2659_ID) {
 			dev_err(&client->dev,
 				"Sensor detection failed (%04X, %d)\n",
 				id, ret);
-		else {
+			ret = -ENODEV;
+		} else {
 			dev_info(&client->dev, "Found OV%04X sensor\n", id);
 			ret = ov2659_init(sd, 0);
 		}
-- 
2.17.1

[Patch 3/6] media: dt-bindings: ov2659: add powerdown-gpios optional property

[Benoit Parrot]

Add powerdown-gpios to the list of optional properties for the OV2659
camera sensor.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 Documentation/devicetree/bindings/media/i2c/ov2659.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
index cabc7d827dfb..f55204cce0cd 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
@@ -12,6 +12,10 @@ Required Properties:
 - clock-names: should be "xvclk".
 - link-frequencies: target pixel clock frequency.
 
+Optional Properties:
+- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
+  Active is low.
+
 For further reading on port node refer to
 Documentation/devicetree/bindings/media/video-interfaces.txt.
 
@@ -27,6 +31,8 @@ Example:
 			clocks = <&clk_ov2659 0>;
 			clock-names = "xvclk";
 
+			powerdown-gpios = <&gpio6 14 GPIO_ACTIVE_LOW>;
+
 			port {
 				ov2659_0: endpoint {
 					remote-endpoint = <&vpfe_ep>;
-- 
2.17.1

[Patch 4/6] media: i2c: ov2659: Add optional powerdown gpio handling

[Benoit Parrot]

On some board it is possible that the sensor 'powerdown'
pin might be controlled with a gpio instead of being
tied to always powered.

This patch add support to specify an optional gpio
which will be set at probe time and remained on.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ov2659.c | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7eee1812bba3..315c1d8bdb7b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -634,7 +634,7 @@ config VIDEO_OV2640
 config VIDEO_OV2659
 	tristate "OmniVision OV2659 sensor support"
 	depends on VIDEO_V4L2 && I2C
-	depends on MEDIA_CAMERA_SUPPORT
+	depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
 	select V4L2_FWNODE
 	help
 	  This is a Video4Linux2 sensor driver for the OmniVision
diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index efbe6dc720e2..c64f73bef336 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -32,6 +32,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/videodev2.h>
@@ -232,6 +234,8 @@ struct ov2659 {
 	struct sensor_register *format_ctrl_regs;
 	struct ov2659_pll_ctrl pll;
 	int streaming;
+	/* used to control the sensor powerdownN pin */
+	struct gpio_desc *pwrdn_gpio;
 };
 
 static const struct sensor_register ov2659_init_regs[] = {
@@ -1391,6 +1395,7 @@ static int ov2659_probe(struct i2c_client *client)
 	struct v4l2_subdev *sd;
 	struct ov2659 *ov2659;
 	struct clk *clk;
+	struct gpio_desc *gpio;
 	int ret;
 
 	if (!pdata) {
@@ -1414,6 +1419,14 @@ static int ov2659_probe(struct i2c_client *client)
 	    ov2659->xvclk_frequency > 27000000)
 		return -EINVAL;
 
+	/* Optional gpio don't fail if not present */
+	gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
+				       GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+
+	ov2659->pwrdn_gpio = gpio;
+
 	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
 	ov2659->link_frequency =
 			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
-- 
2.17.1

[Patch 5/6] media: i2c: ov2659: Fix missing 720p register config

[Benoit Parrot]

The initial registers sequence is only loaded at probe
time. Afterward only the resolution and format specific
register are modified. Care must be taken to make sure
registers modified by one resolution setting are reverted
back when another resolution is programmed.

This was not done properly for the 720p case.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/ov2659.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index c64f73bef336..fbe624457fbc 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -423,10 +423,14 @@ static struct sensor_register ov2659_720p[] = {
 	{ REG_TIMING_YINC, 0x11 },
 	{ REG_TIMING_VERT_FORMAT, 0x80 },
 	{ REG_TIMING_HORIZ_FORMAT, 0x00 },
+	{ 0x370a, 0x12 },
 	{ 0x3a03, 0xe8 },
 	{ 0x3a09, 0x6f },
 	{ 0x3a0b, 0x5d },
 	{ 0x3a15, 0x9a },
+	{ REG_VFIFO_READ_START_H, 0x00 },
+	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_ISP_CTRL02, 0x00 },
 	{ REG_NULL, 0x00 },
 };
 
-- 
2.17.1

[Patch 6/6] media: i2c: ov2659: Switch to SPDX Licensing

[Benoit Parrot]

Switch to SPDX licensing and drop the redundant GPL text.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/ov2659.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index fbe624457fbc..946bed29900a 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Omnivision OV2659 CMOS Image Sensor driver
  *
@@ -5,19 +6,6 @@
  *
  * Benoit Parrot <bparrot@ti.com>
  * Lad, Prabhakar <prabhakar.csengg@gmail.com>
- *
- * This program is free software; you may redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 2 of the License.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
  */
 
 #include <linux/clk.h>
-- 
2.17.1

Re: [Patch 3/6] media: dt-bindings: ov2659: add powerdown-gpios optional property

[Lad, Prabhakar]

Hi Benoit,

Thank you for the patch.

On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote:
>
> Add powerdown-gpios to the list of optional properties for the OV2659
> camera sensor.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov2659.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> index cabc7d827dfb..f55204cce0cd 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> @@ -12,6 +12,10 @@ Required Properties:
>  - clock-names: should be "xvclk".
>  - link-frequencies: target pixel clock frequency.
>
> +Optional Properties:
> +- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +  Active is low.
> +
as per the datasheet this should be active high with  pull-down resistor.

Cheers,
--Prabhakar Lad

Re: [Patch 4/6] media: i2c: ov2659: Add optional powerdown gpio handling

[Lad, Prabhakar]

Hi Benoit,

On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote:
>
> On some board it is possible that the sensor 'powerdown'
> pin might be controlled with a gpio instead of being
> tied to always powered.
>
> This patch add support to specify an optional gpio
> which will be set at probe time and remained on.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/i2c/Kconfig  |  2 +-
>  drivers/media/i2c/ov2659.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 7eee1812bba3..315c1d8bdb7b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -634,7 +634,7 @@ config VIDEO_OV2640
>  config VIDEO_OV2659
>         tristate "OmniVision OV2659 sensor support"
>         depends on VIDEO_V4L2 && I2C
> -       depends on MEDIA_CAMERA_SUPPORT
> +       depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
>         select V4L2_FWNODE
>         help
>           This is a Video4Linux2 sensor driver for the OmniVision
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> index efbe6dc720e2..c64f73bef336 100644
> --- a/drivers/media/i2c/ov2659.c
> +++ b/drivers/media/i2c/ov2659.c
> @@ -32,6 +32,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/videodev2.h>
> @@ -232,6 +234,8 @@ struct ov2659 {
>         struct sensor_register *format_ctrl_regs;
>         struct ov2659_pll_ctrl pll;
>         int streaming;
> +       /* used to control the sensor powerdownN pin */
> +       struct gpio_desc *pwrdn_gpio;
>  };
>
>  static const struct sensor_register ov2659_init_regs[] = {
> @@ -1391,6 +1395,7 @@ static int ov2659_probe(struct i2c_client *client)
>         struct v4l2_subdev *sd;
>         struct ov2659 *ov2659;
>         struct clk *clk;
> +       struct gpio_desc *gpio;

you don't need the local var here you can just assign it directly to pwrdn_gpio.

>         int ret;
>
>         if (!pdata) {
> @@ -1414,6 +1419,14 @@ static int ov2659_probe(struct i2c_client *client)
>             ov2659->xvclk_frequency > 27000000)
>                 return -EINVAL;
>
> +       /* Optional gpio don't fail if not present */
> +       gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> +                                      GPIOD_OUT_HIGH);
> +       if (IS_ERR(gpio))
> +               return PTR_ERR(gpio);
> +
> +       ov2659->pwrdn_gpio = gpio;
> +
apart from assigning it you don't actually use it.

you will also have to read the reset gpio pin and implement
ov2659_set_power() and
call it in appropriate places/ s_power ?

Cheers,
--Prabhakar Lad

Re: [Patch 3/6] media: dt-bindings: ov2659: add powerdown-gpios optional property

[Benoit Parrot]

Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote on Sat [2019-Sep-14 11:11:02 +0100]:
> Hi Benoit,
> 
> Thank you for the patch.
> 
> On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote:
> >
> > Add powerdown-gpios to the list of optional properties for the OV2659
> > camera sensor.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ov2659.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > index cabc7d827dfb..f55204cce0cd 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > @@ -12,6 +12,10 @@ Required Properties:
> >  - clock-names: should be "xvclk".
> >  - link-frequencies: target pixel clock frequency.
> >
> > +Optional Properties:
> > +- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
> > +  Active is low.
> > +
> as per the datasheet this should be active high with  pull-down resistor.

Ahhh, yes I'll fix that.

> 
> Cheers,
> --Prabhakar Lad

Re: [Patch 4/6] media: i2c: ov2659: Add optional powerdown gpio handling

[Benoit Parrot]

Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote on Sat [2019-Sep-14 11:33:42 +0100]:
> Hi Benoit,
> 
> On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote:
> >
> > On some board it is possible that the sensor 'powerdown'
> > pin might be controlled with a gpio instead of being
> > tied to always powered.
> >
> > This patch add support to specify an optional gpio
> > which will be set at probe time and remained on.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/i2c/Kconfig  |  2 +-
> >  drivers/media/i2c/ov2659.c | 13 +++++++++++++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 7eee1812bba3..315c1d8bdb7b 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -634,7 +634,7 @@ config VIDEO_OV2640
> >  config VIDEO_OV2659
> >         tristate "OmniVision OV2659 sensor support"
> >         depends on VIDEO_V4L2 && I2C
> > -       depends on MEDIA_CAMERA_SUPPORT
> > +       depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
> >         select V4L2_FWNODE
> >         help
> >           This is a Video4Linux2 sensor driver for the OmniVision
> > diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> > index efbe6dc720e2..c64f73bef336 100644
> > --- a/drivers/media/i2c/ov2659.c
> > +++ b/drivers/media/i2c/ov2659.c
> > @@ -32,6 +32,8 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_graph.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/videodev2.h>
> > @@ -232,6 +234,8 @@ struct ov2659 {
> >         struct sensor_register *format_ctrl_regs;
> >         struct ov2659_pll_ctrl pll;
> >         int streaming;
> > +       /* used to control the sensor powerdownN pin */
> > +       struct gpio_desc *pwrdn_gpio;
> >  };
> >
> >  static const struct sensor_register ov2659_init_regs[] = {
> > @@ -1391,6 +1395,7 @@ static int ov2659_probe(struct i2c_client *client)
> >         struct v4l2_subdev *sd;
> >         struct ov2659 *ov2659;
> >         struct clk *clk;
> > +       struct gpio_desc *gpio;
> 
> you don't need the local var here you can just assign it directly to pwrdn_gpio.

Ok.

> 
> >         int ret;
> >
> >         if (!pdata) {
> > @@ -1414,6 +1419,14 @@ static int ov2659_probe(struct i2c_client *client)
> >             ov2659->xvclk_frequency > 27000000)
> >                 return -EINVAL;
> >
> > +       /* Optional gpio don't fail if not present */
> > +       gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > +                                      GPIOD_OUT_HIGH);
> > +       if (IS_ERR(gpio))
> > +               return PTR_ERR(gpio);
> > +
> > +       ov2659->pwrdn_gpio = gpio;
> > +
> apart from assigning it you don't actually use it.
> 
> you will also have to read the reset gpio pin and implement
> ov2659_set_power() and
> call it in appropriate places/ s_power ?

Well I am not sure I want to go that far.
On most board I have the sensor is always powered as soon as the board gets
powered. Which is why we go through a S/W reset before starting a stream.

I didn't want to change the logic here too much.

I'll check this out a little more.

Benoit

> 
> Cheers,
> --Prabhakar Lad