All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] V4L2: fix em28xx ov2640 support
@ 2013-08-28 13:28 Guennadi Liakhovetski
  2013-08-28 13:28 ` [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock Guennadi Liakhovetski
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 13:28 UTC (permalink / raw)
  To: linux-media
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

This patch series adds a V4L2 clock support to em28xx with an ov2640 
sensor. Only compile tested, might need fixing, please, test.

Guennadi Liakhovetski (3):
  V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
    clock
  V4L2: add a v4l2-clk helper macro to produce an I2C device ID
  V4L2: em28xx: register a V4L2 clock source

 drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
 drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
 drivers/media/usb/em28xx/em28xx.h        |    1 +
 drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
 include/media/v4l2-clk.h                 |   17 ++++++++++++
 5 files changed, 91 insertions(+), 10 deletions(-)

-- 
1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock
  2013-08-28 13:28 [PATCH 0/3] V4L2: fix em28xx ov2640 support Guennadi Liakhovetski
@ 2013-08-28 13:28 ` Guennadi Liakhovetski
  2013-08-28 13:33   ` Laurent Pinchart
  2013-08-28 13:28 ` [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID Guennadi Liakhovetski
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 13:28 UTC (permalink / raw)
  To: linux-media
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

Many bridges and video host controllers supply fixed rate always on clocks
to their I2C devices. This patch adds two simple helpers to register and
unregister such a clock.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/v4l2-clk.c |   39 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-clk.h           |   14 ++++++++++++
 2 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index b67de86..e18cc04 100644
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -240,3 +240,42 @@ void v4l2_clk_unregister(struct v4l2_clk *clk)
 	kfree(clk);
 }
 EXPORT_SYMBOL(v4l2_clk_unregister);
+
+struct v4l2_clk_fixed {
+	unsigned long rate;
+	struct v4l2_clk_ops ops;
+};
+
+static unsigned long fixed_get_rate(struct v4l2_clk *clk)
+{
+	struct v4l2_clk_fixed *priv = clk->priv;
+	return priv->rate;
+}
+
+struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
+		const char *id, unsigned long rate, struct module *owner)
+{
+	struct v4l2_clk *clk;
+	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	priv->rate = rate;
+	priv->ops.get_rate = fixed_get_rate;
+	priv->ops.owner = owner;
+
+	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
+	if (IS_ERR(clk))
+		kfree(priv);
+
+	return clk;
+}
+EXPORT_SYMBOL(__v4l2_clk_register_fixed);
+
+void v4l2_clk_unregister_fixed(struct v4l2_clk *clk)
+{
+	kfree(clk->priv);
+	v4l2_clk_unregister(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_unregister_fixed);
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
index 0503a90..a354a9d 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -15,6 +15,7 @@
 #define MEDIA_V4L2_CLK_H
 
 #include <linux/atomic.h>
+#include <linux/export.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
@@ -51,4 +52,17 @@ void v4l2_clk_disable(struct v4l2_clk *clk);
 unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
 int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
 
+struct module;
+
+struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
+		const char *id, unsigned long rate, struct module *owner);
+void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
+
+static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
+							const char *id,
+							unsigned long rate)
+{
+	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
+}
+
 #endif
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID
  2013-08-28 13:28 [PATCH 0/3] V4L2: fix em28xx ov2640 support Guennadi Liakhovetski
  2013-08-28 13:28 ` [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock Guennadi Liakhovetski
@ 2013-08-28 13:28 ` Guennadi Liakhovetski
  2013-08-28 13:35   ` Laurent Pinchart
  2013-08-28 13:28 ` [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source Guennadi Liakhovetski
  2013-09-02 18:40 ` [PATCH 0/3] V4L2: fix em28xx ov2640 support Frank Schäfer
  3 siblings, 1 reply; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 13:28 UTC (permalink / raw)
  To: linux-media
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

To obtain a clock reference consumers supply their device object to the
V4L2 clock framework. The latter then uses the consumer device name to
find a matching clock. For that to work V4L2 clock providers have to
provide the same device name, when registering clocks. This patch adds
a helper macro to generate a suitable device name for I2C devices.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
V4L2 clocks use device ID matching, which in case of I2C devices involves
comparing a specially constructed from an I2C adapter number and a device
address
---
 include/media/v4l2-clk.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
index a354a9d..0b36cc1 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -65,4 +65,7 @@ static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
 	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
 }
 
+#define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
+			  "%d-%04x", adap, client)
+
 #endif
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source
  2013-08-28 13:28 [PATCH 0/3] V4L2: fix em28xx ov2640 support Guennadi Liakhovetski
  2013-08-28 13:28 ` [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock Guennadi Liakhovetski
  2013-08-28 13:28 ` [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID Guennadi Liakhovetski
@ 2013-08-28 13:28 ` Guennadi Liakhovetski
  2013-08-28 21:54   ` Sylwester Nawrocki
  2013-09-02 18:40 ` [PATCH 0/3] V4L2: fix em28xx ov2640 support Frank Schäfer
  3 siblings, 1 reply; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 13:28 UTC (permalink / raw)
  To: linux-media
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

Camera sensors usually require a master clock for data sampling. This patch
registers such a clock source for em28xx cameras. This fixes the currently
broken em28xx ov2640 camera support and can also be used by other camera
sensors.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Actually after thinking a bit more, it'd probably be better to register a 
clock only for the ov2640 based camera, not for all cameras. If all agree, 
I'll redo this.

 drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
 drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
 drivers/media/usb/em28xx/em28xx.h        |    1 +
 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 73cc50a..2f451e4 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -22,6 +22,7 @@
 #include <linux/i2c.h>
 #include <media/soc_camera.h>
 #include <media/mt9v011.h>
+#include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 
 #include "em28xx.h"
@@ -325,13 +326,24 @@ int em28xx_detect_sensor(struct em28xx *dev)
 
 int em28xx_init_camera(struct em28xx *dev)
 {
+	char clk_name[V4L2_SUBDEV_NAME_SIZE];
+	struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
+	struct i2c_adapter *adap = &dev->i2c_adap[dev->def_i2c_bus];
+	int ret = 0;
+
+	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
+			  i2c_adapter_id(adap), client->addr);
+	dev->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
 	switch (dev->em28xx_sensor) {
 	case EM28XX_MT9V011:
 	{
 		struct mt9v011_platform_data pdata;
 		struct i2c_board_info mt9v011_info = {
 			.type = "mt9v011",
-			.addr = dev->i2c_client[dev->def_i2c_bus].addr,
+			.addr = client->addr,
 			.platform_data = &pdata,
 		};
 
@@ -352,10 +364,11 @@ int em28xx_init_camera(struct em28xx *dev)
 		dev->sensor_xtal = 4300000;
 		pdata.xtal = dev->sensor_xtal;
 		if (NULL ==
-		    v4l2_i2c_new_subdev_board(&dev->v4l2_dev,
-					      &dev->i2c_adap[dev->def_i2c_bus],
-					      &mt9v011_info, NULL))
-			return -ENODEV;
+		    v4l2_i2c_new_subdev_board(&dev->v4l2_dev, adap,
+					      &mt9v011_info, NULL)) {
+			ret = -ENODEV;
+			break;
+		}
 		/* probably means GRGB 16 bit bayer */
 		dev->vinmode = 0x0d;
 		dev->vinctl = 0x00;
@@ -391,7 +404,7 @@ int em28xx_init_camera(struct em28xx *dev)
 		struct i2c_board_info ov2640_info = {
 			.type = "ov2640",
 			.flags = I2C_CLIENT_SCCB,
-			.addr = dev->i2c_client[dev->def_i2c_bus].addr,
+			.addr = client->addr,
 			.platform_data = &camlink,
 		};
 		struct v4l2_mbus_framefmt fmt;
@@ -408,9 +421,12 @@ int em28xx_init_camera(struct em28xx *dev)
 		dev->sensor_yres = 480;
 
 		subdev =
-		     v4l2_i2c_new_subdev_board(&dev->v4l2_dev,
-					       &dev->i2c_adap[dev->def_i2c_bus],
+		     v4l2_i2c_new_subdev_board(&dev->v4l2_dev, adap,
 					       &ov2640_info, NULL);
+		if (NULL == subdev) {
+			ret = -ENODEV;
+			break;
+		}
 
 		fmt.code = V4L2_MBUS_FMT_YUYV8_2X8;
 		fmt.width = 640;
@@ -427,8 +443,13 @@ int em28xx_init_camera(struct em28xx *dev)
 	}
 	case EM28XX_NOSENSOR:
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	if (ret < 0) {
+		v4l2_clk_unregister_fixed(dev->clk);
+		dev->clk = NULL;
+	}
+
+	return ret;
 }
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index dc65742..7c0afa2 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -36,6 +36,7 @@
 #include <media/tvaudio.h>
 #include <media/i2c-addr.h>
 #include <media/tveeprom.h>
+#include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 
 #include "em28xx.h"
@@ -2857,6 +2858,8 @@ void em28xx_release_resources(struct em28xx *dev)
 	if (dev->def_i2c_bus)
 		em28xx_i2c_unregister(dev, 1);
 	em28xx_i2c_unregister(dev, 0);
+	if (dev->clk)
+		v4l2_clk_unregister_fixed(dev->clk);
 
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
 
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 205e903..c9ebe19 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -492,6 +492,7 @@ struct em28xx {
 
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
+	struct v4l2_clk *clk;
 	struct em28xx_board board;
 
 	/* Webcam specific fields */
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock
  2013-08-28 13:28 ` [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock Guennadi Liakhovetski
@ 2013-08-28 13:33   ` Laurent Pinchart
  2013-08-28 14:49     ` Guennadi Liakhovetski
  0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2013-08-28 13:33 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: linux-media, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

Hi Guennadi,

Thank you for the patch.

On Wednesday 28 August 2013 15:28:26 Guennadi Liakhovetski wrote:
> Many bridges and video host controllers supply fixed rate always on clocks
> to their I2C devices. This patch adds two simple helpers to register and
> unregister such a clock.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/v4l2-core/v4l2-clk.c |   39 +++++++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h           |   14 ++++++++++++
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> b/drivers/media/v4l2-core/v4l2-clk.c index b67de86..e18cc04 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -240,3 +240,42 @@ void v4l2_clk_unregister(struct v4l2_clk *clk)
>  	kfree(clk);
>  }
>  EXPORT_SYMBOL(v4l2_clk_unregister);
> +
> +struct v4l2_clk_fixed {
> +	unsigned long rate;
> +	struct v4l2_clk_ops ops;
> +};
> +
> +static unsigned long fixed_get_rate(struct v4l2_clk *clk)
> +{
> +	struct v4l2_clk_fixed *priv = clk->priv;
> +	return priv->rate;
> +}
> +
> +struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> +		const char *id, unsigned long rate, struct module *owner)
> +{
> +	struct v4l2_clk *clk;
> +	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->rate = rate;
> +	priv->ops.get_rate = fixed_get_rate;
> +	priv->ops.owner = owner;

The ops owner is v4l2-clk.c, shouldn't you use THIS_MODULE here instead of the 
caller's THIS_MODULE ?

> +
> +	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> +	if (IS_ERR(clk))
> +		kfree(priv);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(__v4l2_clk_register_fixed);
> +
> +void v4l2_clk_unregister_fixed(struct v4l2_clk *clk)
> +{
> +	kfree(clk->priv);
> +	v4l2_clk_unregister(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_unregister_fixed);
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 0503a90..a354a9d 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -15,6 +15,7 @@
>  #define MEDIA_V4L2_CLK_H
> 
>  #include <linux/atomic.h>
> +#include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> 
> @@ -51,4 +52,17 @@ void v4l2_clk_disable(struct v4l2_clk *clk);
>  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
>  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
> 
> +struct module;
> +
> +struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> +		const char *id, unsigned long rate, struct module *owner);
> +void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
> +
> +static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
> +							const char *id,
> +							unsigned long rate)
> +{
> +	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> +}
> +
>  #endif
-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID
  2013-08-28 13:28 ` [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID Guennadi Liakhovetski
@ 2013-08-28 13:35   ` Laurent Pinchart
  2013-08-28 13:42     ` Guennadi Liakhovetski
  0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2013-08-28 13:35 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: linux-media, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

Hi Guennadi,

Thank you for the patch.

On Wednesday 28 August 2013 15:28:27 Guennadi Liakhovetski wrote:
> To obtain a clock reference consumers supply their device object to the
> V4L2 clock framework. The latter then uses the consumer device name to
> find a matching clock. For that to work V4L2 clock providers have to
> provide the same device name, when registering clocks. This patch adds
> a helper macro to generate a suitable device name for I2C devices.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> V4L2 clocks use device ID matching, which in case of I2C devices involves
> comparing a specially constructed from an I2C adapter number and a device
> address

Is this text placed below the SoB on purpose ?

> ---
>  include/media/v4l2-clk.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index a354a9d..0b36cc1 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -65,4 +65,7 @@ static inline struct v4l2_clk
> *v4l2_clk_register_fixed(const char *dev_id, return
> __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); }
> 
> +#define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
> +			  "%d-%04x", adap, client)
> +

I would have made this a static inline but I have to confess I don't know why 
:-)

>  #endif
-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID
  2013-08-28 13:35   ` Laurent Pinchart
@ 2013-08-28 13:42     ` Guennadi Liakhovetski
  0 siblings, 0 replies; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 13:42 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

On Wed, 28 Aug 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Wednesday 28 August 2013 15:28:27 Guennadi Liakhovetski wrote:
> > To obtain a clock reference consumers supply their device object to the
> > V4L2 clock framework. The latter then uses the consumer device name to
> > find a matching clock. For that to work V4L2 clock providers have to
> > provide the same device name, when registering clocks. This patch adds
> > a helper macro to generate a suitable device name for I2C devices.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > V4L2 clocks use device ID matching, which in case of I2C devices involves
> > comparing a specially constructed from an I2C adapter number and a device
> > address
> 
> Is this text placed below the SoB on purpose ?

Errm, it should have been deleted :) sorry.

Thanks
Guennadi

> 
> > ---
> >  include/media/v4l2-clk.h |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index a354a9d..0b36cc1 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -65,4 +65,7 @@ static inline struct v4l2_clk
> > *v4l2_clk_register_fixed(const char *dev_id, return
> > __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); }
> > 
> > +#define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
> > +			  "%d-%04x", adap, client)
> > +
> 
> I would have made this a static inline but I have to confess I don't know why 
> :-)
> 
> >  #endif
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock
  2013-08-28 13:33   ` Laurent Pinchart
@ 2013-08-28 14:49     ` Guennadi Liakhovetski
  0 siblings, 0 replies; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-08-28 14:49 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Mauro Carvalho Chehab, Sylwester Nawrocki,
	Frank Schäfer, Hans Verkuil

On Wed, 28 Aug 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Wednesday 28 August 2013 15:28:26 Guennadi Liakhovetski wrote:
> > Many bridges and video host controllers supply fixed rate always on clocks
> > to their I2C devices. This patch adds two simple helpers to register and
> > unregister such a clock.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/media/v4l2-core/v4l2-clk.c |   39 +++++++++++++++++++++++++++++++++
> >  include/media/v4l2-clk.h           |   14 ++++++++++++
> >  2 files changed, 53 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c index b67de86..e18cc04 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -240,3 +240,42 @@ void v4l2_clk_unregister(struct v4l2_clk *clk)
> >  	kfree(clk);
> >  }
> >  EXPORT_SYMBOL(v4l2_clk_unregister);
> > +
> > +struct v4l2_clk_fixed {
> > +	unsigned long rate;
> > +	struct v4l2_clk_ops ops;
> > +};
> > +
> > +static unsigned long fixed_get_rate(struct v4l2_clk *clk)
> > +{
> > +	struct v4l2_clk_fixed *priv = clk->priv;
> > +	return priv->rate;
> > +}
> > +
> > +struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > +		const char *id, unsigned long rate, struct module *owner)
> > +{
> > +	struct v4l2_clk *clk;
> > +	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +
> > +	if (!priv)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	priv->rate = rate;
> > +	priv->ops.get_rate = fixed_get_rate;
> > +	priv->ops.owner = owner;
> 
> The ops owner is v4l2-clk.c, shouldn't you use THIS_MODULE here instead of the 
> caller's THIS_MODULE ?

Actually I don't think so. Making THIS_MODULE the owner wouldn't make any 
sense, IMHO, the module cannot be unloaded anyway as long as any V4L 
activity is taking place. If there is anything you want to lock in for as 
long as the clock is used, then it's the bridge / camera host driver, 
which is exactly what's done here.

Thanks
Guennadi

> 
> > +
> > +	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> > +	if (IS_ERR(clk))
> > +		kfree(priv);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(__v4l2_clk_register_fixed);
> > +
> > +void v4l2_clk_unregister_fixed(struct v4l2_clk *clk)
> > +{
> > +	kfree(clk->priv);
> > +	v4l2_clk_unregister(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_unregister_fixed);
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 0503a90..a354a9d 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -15,6 +15,7 @@
> >  #define MEDIA_V4L2_CLK_H
> > 
> >  #include <linux/atomic.h>
> > +#include <linux/export.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> > 
> > @@ -51,4 +52,17 @@ void v4l2_clk_disable(struct v4l2_clk *clk);
> >  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
> >  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
> > 
> > +struct module;
> > +
> > +struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > +		const char *id, unsigned long rate, struct module *owner);
> > +void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
> > +
> > +static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
> > +							const char *id,
> > +							unsigned long rate)
> > +{
> > +	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> > +}
> > +
> >  #endif
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source
  2013-08-28 13:28 ` [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source Guennadi Liakhovetski
@ 2013-08-28 21:54   ` Sylwester Nawrocki
  0 siblings, 0 replies; 17+ messages in thread
From: Sylwester Nawrocki @ 2013-08-28 21:54 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: linux-media, Laurent Pinchart, Mauro Carvalho Chehab,
	Sylwester Nawrocki, Frank Schäfer, Hans Verkuil

Hi Guennadi,

On 08/28/2013 03:28 PM, Guennadi Liakhovetski wrote:
> Camera sensors usually require a master clock for data sampling. This patch
> registers such a clock source for em28xx cameras. This fixes the currently
> broken em28xx ov2640 camera support and can also be used by other camera
> sensors.
>
> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> ---
>
> Actually after thinking a bit more, it'd probably be better to register a
> clock only for the ov2640 based camera, not for all cameras. If all agree,
> I'll redo this.

Not sure, I'd assume it's better to have this clock for all the subdevs.
Currently there are only two sensors handled through regular subdev drivers
though.

>   drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
>   drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
>   drivers/media/usb/em28xx/em28xx.h        |    1 +
>   3 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index 73cc50a..2f451e4 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -22,6 +22,7 @@
>   #include<linux/i2c.h>
>   #include<media/soc_camera.h>
>   #include<media/mt9v011.h>
> +#include<media/v4l2-clk.h>
>   #include<media/v4l2-common.h>
>
>   #include "em28xx.h"
> @@ -325,13 +326,24 @@ int em28xx_detect_sensor(struct em28xx *dev)
>
>   int em28xx_init_camera(struct em28xx *dev)
>   {
> +	char clk_name[V4L2_SUBDEV_NAME_SIZE];
> +	struct i2c_client *client =&dev->i2c_client[dev->def_i2c_bus];
> +	struct i2c_adapter *adap =&dev->i2c_adap[dev->def_i2c_bus];
> +	int ret = 0;
> +
> +	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> +			  i2c_adapter_id(adap), client->addr);
> +	dev->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);

Or maybe we could also create even more easy to use helper function, e.g.

v4l2_clk_register_i2c_fixed(int adap_id, unsigned int i2c_addr,
				char *clk_name);

Then clk_name (hmm, actually it's the device name ?) would be filled
inside the helper ?

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-08-28 13:28 [PATCH 0/3] V4L2: fix em28xx ov2640 support Guennadi Liakhovetski
                   ` (2 preceding siblings ...)
  2013-08-28 13:28 ` [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source Guennadi Liakhovetski
@ 2013-09-02 18:40 ` Frank Schäfer
  2013-09-03  6:34   ` Guennadi Liakhovetski
  3 siblings, 1 reply; 17+ messages in thread
From: Frank Schäfer @ 2013-09-02 18:40 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: linux-media, Laurent Pinchart, Mauro Carvalho Chehab,
	Sylwester Nawrocki, Hans Verkuil

Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
> This patch series adds a V4L2 clock support to em28xx with an ov2640 
> sensor. Only compile tested, might need fixing, please, test.
>
> Guennadi Liakhovetski (3):
>   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
>     clock
>   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
>   V4L2: em28xx: register a V4L2 clock source
>
>  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
>  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
>  drivers/media/usb/em28xx/em28xx.h        |    1 +
>  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h                 |   17 ++++++++++++
>  5 files changed, 91 insertions(+), 10 deletions(-)
>

Tested a few minutes ago:

...
[  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
[  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
max is 2
[  103.678559] usb 1-8: config 1 has no interface number 2
[  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
[  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
SerialNumber=0
[  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
[  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
[  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
class 3)
[  103.797066] em28xx: Video interface 3 found: bulk
[  103.933941] em28xx: chip ID is em2765
[  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
fa 02 d2 0a 32 02
[  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
00 4b 12 0e 1f d2
[  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
00 4b 02 0e 1f 80
[  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
84 22 02 0c 78 00
[  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
0c 44 02 00 03 a2
[  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
04 90 00 00 00 00
[  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
00 20 01 01 00 01
[  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00
[  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
[  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
[  104.043922] em2765 #0: EEPROM info:
[  104.043924] em2765 #0:       microcode start address = 0x0004, boot
configuration = 0x01
[  104.069818] em2765 #0:       no hardware configuration dataset found
in eeprom
[  104.080693] em2765 #0: sensor OV2640 detected
[  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
Laplace webcam (card=90)
[  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
[  104.173836] i2c i2c-11: OV2640 Probed
[  104.306698] em2765 #0: Config register raw data: 0x00
[  104.306717] em2765 #0: v4l2 driver version 0.2.0
[  104.321152] em2765 #0: V4L2 video device registered as video1
[  104.321167] ------------[ cut here ]------------
[  104.321216] WARNING: CPU: 0 PID: 517 at
drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
[videodev]()
[  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
[  104.321226] Modules linked in: ov2640 soc_camera soc_mediabus
em28xx(+) videobuf2_core videobuf2_vmalloc videobuf2_memops xt_tcpudp
xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy autofs4
radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
ata_generic pata_amd pata_jmicron sata_nv
[  104.321353] CPU: 0 PID: 517 Comm: udevd Not tainted
3.11.0-rc2-0.1-desktop+ #19
[  104.321358] Hardware name: System manufacturer System Product
Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
[  104.321363]  00000000 00000000 f52dbb70 c0743834 f52dbbb0 f52dbba0
c023f5ef f7bde565
[  104.321375]  f52dbbcc 00000205 f7bdf620 00000083 f7bd9f03 f7bd9f03
eb112c40 eb112c58
[  104.321386]  00000000 f52dbbb8 c023f68e 00000009 f52dbbb0 f7bde565
f52dbbcc f52dbbe0
[  104.321397] Call Trace:
[  104.321411]  [<c0743834>] dump_stack+0x4b/0x72
[  104.321420]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
[  104.321446]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
[  104.321466]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
[  104.321473]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
[  104.321494]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
[  104.321511]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
[  104.321530]  [<f8d46f13>] ?
em28xx_register_analog_devices+0x653/0x6c0 [em28xx]
[  104.321539]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
[  104.321555]  [<f8d492c2>] em28xx_usb_probe+0xef2/0x1330 [em28xx]
[  104.321564]  [<c03a31f5>] ? __sysfs_add_one+0x55/0xf0
[  104.321574]  [<c057f5f8>] ? __pm_runtime_set_status+0xf8/0x210
[  104.321581]  [<c057f4b1>] ? __pm_runtime_resume+0x41/0x50
[  104.321590]  [<c05df477>] usb_probe_interface+0x187/0x2c0
[  104.321598]  [<c05745aa>] ? driver_sysfs_add+0x6a/0x90
[  104.321604]  [<c0574a44>] driver_probe_device+0x74/0x360
[  104.321610]  [<c05ded31>] ? usb_match_id+0x41/0x60
[  104.321617]  [<c05ded9e>] ? usb_device_match+0x4e/0x90
[  104.321623]  [<c0574db9>] __driver_attach+0x89/0x90
[  104.321630]  [<c0574d30>] ? driver_probe_device+0x360/0x360
[  104.321639]  [<c0572f92>] bus_for_each_dev+0x42/0x80
[  104.321645]  [<c0574539>] driver_attach+0x19/0x20
[  104.321652]  [<c0574d30>] ? driver_probe_device+0x360/0x360
[  104.321658]  [<c05740c4>] bus_add_driver+0xe4/0x260
[  104.321665]  [<c0575375>] driver_register+0x65/0x160
[  104.321673]  [<c029beed>] ? smp_call_function+0x2d/0x50
[  104.321681]  [<c0236870>] ? __cpa_process_fault+0x80/0x80
[  104.321688]  [<c05ddf22>] usb_register_driver+0x62/0x150
[  104.321695]  [<c023752a>] ? change_page_attr_set_clr+0x2da/0x380
[  104.321707]  [<f8d5c000>] ? 0xf8d5bfff
[  104.321724]  [<f8d5c017>] em28xx_usb_driver_init+0x17/0x1000 [em28xx]
[  104.321732]  [<c02003fa>] do_one_initcall+0xaa/0x160
[  104.321741]  [<c02d0ab4>] ? tracepoint_module_notify+0xc4/0x180
[  104.321752]  [<f8d5c000>] ? 0xf8d5bfff
[  104.321758]  [<c0237797>] ? set_memory_nx+0x57/0x60
[  104.321772]  [<c0740c14>] ? set_section_ro_nx+0x4f/0x54
[  104.321781]  [<c02a1388>] load_module+0x1ba8/0x2510
[  104.321792]  [<c02a1d87>] SyS_init_module+0x97/0x100
[  104.321801]  [<c030f463>] ? vm_mmap_pgoff+0x83/0xb0
[  104.321811]  [<c074fc3a>] sysenter_do_call+0x12/0x22
[  104.321816] ---[ end trace ce95bae000cad89a ]---
[  104.321823] em2765 #0: analog set to bulk mode.
[  104.322214] usbcore: registered new interface driver em28xx
[  104.373835] ------------[ cut here ]------------
[  104.373886] WARNING: CPU: 0 PID: 2087 at
drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
[videodev]()
[  104.373892] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
[  104.373896] Modules linked in: snd_rawmidi ov2640 soc_camera
soc_mediabus em28xx videobuf2_core videobuf2_vmalloc videobuf2_memops
xt_tcpudp xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy
autofs4 radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
ata_generic pata_amd pata_jmicron sata_nv
[  104.374088] CPU: 0 PID: 2087 Comm: v4l_id Tainted: G        W   
3.11.0-rc2-0.1-desktop+ #19
[  104.374093] Hardware name: System manufacturer System Product
Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
[  104.374098]  00000000 00000000 eb247e74 c0743834 eb247eb4 eb247ea4
c023f5ef f7bde565
[  104.374110]  eb247ed0 00000827 f7bdf620 00000083 f7bd9f03 f7bd9f03
eb112c40 eb112c58
[  104.374121]  00000000 eb247ebc c023f68e 00000009 eb247eb4 f7bde565
eb247ed0 eb247ee4
[  104.374132] Call Trace:
[  104.374145]  [<c0743834>] dump_stack+0x4b/0x72
[  104.374178]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
[  104.374208]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
[  104.374228]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
[  104.374242]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
[  104.374266]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
[  104.374284]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
[  104.374293]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
[  104.374308]  [<f8d44c16>] em28xx_v4l2_close+0x86/0x150 [em28xx]
[  104.374326]  [<f7bcf02e>] v4l2_release+0x2e/0x70 [videodev]
[  104.374335]  [<c034558f>] __fput+0xaf/0x1d0
[  104.374342]  [<c03456e8>] ____fput+0x8/0x10
[  104.374352]  [<c025d189>] task_work_run+0x79/0x90
[  104.374359]  [<c0202071>] do_notify_resume+0x41/0x70
[  104.374368]  [<c0749a8f>] work_notifysig+0x24/0x29
[  104.374374] ---[ end trace ce95bae000cad89b ]---
[  104.440959] usbcore: registered new interface driver snd-usb-audio


Regards,
Frank


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-02 18:40 ` [PATCH 0/3] V4L2: fix em28xx ov2640 support Frank Schäfer
@ 2013-09-03  6:34   ` Guennadi Liakhovetski
  2013-09-05 13:32     ` Guennadi Liakhovetski
  2013-09-05 15:22     ` Frank Schäfer
  0 siblings, 2 replies; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-09-03  6:34 UTC (permalink / raw)
  To: Frank Schäfer
  Cc: linux-media, Laurent Pinchart, Mauro Carvalho Chehab,
	Sylwester Nawrocki, Hans Verkuil

Hi Frank

Thanks for testing! Let's have a look then:

On Mon, 2 Sep 2013, Frank Schäfer wrote:

> Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
> > This patch series adds a V4L2 clock support to em28xx with an ov2640 
> > sensor. Only compile tested, might need fixing, please, test.
> >
> > Guennadi Liakhovetski (3):
> >   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
> >     clock
> >   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
> >   V4L2: em28xx: register a V4L2 clock source
> >
> >  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
> >  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
> >  drivers/media/usb/em28xx/em28xx.h        |    1 +
> >  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
> >  include/media/v4l2-clk.h                 |   17 ++++++++++++
> >  5 files changed, 91 insertions(+), 10 deletions(-)
> >
> 
> Tested a few minutes ago:
> 
> ...
> [  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
> [  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
> max is 2
> [  103.678559] usb 1-8: config 1 has no interface number 2
> [  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
> [  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
> SerialNumber=0
> [  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
> [  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
> [  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
> class 3)
> [  103.797066] em28xx: Video interface 3 found: bulk
> [  103.933941] em28xx: chip ID is em2765
> [  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
> fa 02 d2 0a 32 02
> [  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
> 00 4b 12 0e 1f d2
> [  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
> 00 4b 02 0e 1f 80
> [  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
> 84 22 02 0c 78 00
> [  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
> 0c 44 02 00 03 a2
> [  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
> 04 90 00 00 00 00
> [  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
> 00 20 01 01 00 01
> [  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00
> [  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
> [  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
> [  104.043922] em2765 #0: EEPROM info:
> [  104.043924] em2765 #0:       microcode start address = 0x0004, boot
> configuration = 0x01
> [  104.069818] em2765 #0:       no hardware configuration dataset found
> in eeprom
> [  104.080693] em2765 #0: sensor OV2640 detected
> [  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
> Laplace webcam (card=90)
> [  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
> [  104.173836] i2c i2c-11: OV2640 Probed

I presume, this is good.

> [  104.306698] em2765 #0: Config register raw data: 0x00
> [  104.306717] em2765 #0: v4l2 driver version 0.2.0
> [  104.321152] em2765 #0: V4L2 video device registered as video1

This is good too.

> [  104.321167] ------------[ cut here ]------------
> [  104.321216] WARNING: CPU: 0 PID: 517 at
> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> [videodev]()
> [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!

Ok, this is because em28xx_init_dev() calls

	/* Save some power by putting tuner to sleep */
	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);

without turning the subdevice on before. Are those subdevices on by 
default? In principle, this warning is harmless and it should still work 
afterwards, but we should still clean this up - by either removing the 
warning, or adding a power-on before a power-off in em28xx_init_dev(), or 
somehow else. In fact, I think, this should indeed be done: 
em28xx_card_setup() performs i2c accesses, right? So, we have to power up 
the subdev before that.

> [  104.321226] Modules linked in: ov2640 soc_camera soc_mediabus
> em28xx(+) videobuf2_core videobuf2_vmalloc videobuf2_memops xt_tcpudp
> xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy autofs4
> radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
> ata_generic pata_amd pata_jmicron sata_nv
> [  104.321353] CPU: 0 PID: 517 Comm: udevd Not tainted
> 3.11.0-rc2-0.1-desktop+ #19
> [  104.321358] Hardware name: System manufacturer System Product
> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
> [  104.321363]  00000000 00000000 f52dbb70 c0743834 f52dbbb0 f52dbba0
> c023f5ef f7bde565
> [  104.321375]  f52dbbcc 00000205 f7bdf620 00000083 f7bd9f03 f7bd9f03
> eb112c40 eb112c58
> [  104.321386]  00000000 f52dbbb8 c023f68e 00000009 f52dbbb0 f7bde565
> f52dbbcc f52dbbe0
> [  104.321397] Call Trace:
> [  104.321411]  [<c0743834>] dump_stack+0x4b/0x72
> [  104.321420]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
> [  104.321446]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.321466]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.321473]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
> [  104.321494]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.321511]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
> [  104.321530]  [<f8d46f13>] ?
> em28xx_register_analog_devices+0x653/0x6c0 [em28xx]
> [  104.321539]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
> [  104.321555]  [<f8d492c2>] em28xx_usb_probe+0xef2/0x1330 [em28xx]
> [  104.321564]  [<c03a31f5>] ? __sysfs_add_one+0x55/0xf0
> [  104.321574]  [<c057f5f8>] ? __pm_runtime_set_status+0xf8/0x210
> [  104.321581]  [<c057f4b1>] ? __pm_runtime_resume+0x41/0x50
> [  104.321590]  [<c05df477>] usb_probe_interface+0x187/0x2c0
> [  104.321598]  [<c05745aa>] ? driver_sysfs_add+0x6a/0x90
> [  104.321604]  [<c0574a44>] driver_probe_device+0x74/0x360
> [  104.321610]  [<c05ded31>] ? usb_match_id+0x41/0x60
> [  104.321617]  [<c05ded9e>] ? usb_device_match+0x4e/0x90
> [  104.321623]  [<c0574db9>] __driver_attach+0x89/0x90
> [  104.321630]  [<c0574d30>] ? driver_probe_device+0x360/0x360
> [  104.321639]  [<c0572f92>] bus_for_each_dev+0x42/0x80
> [  104.321645]  [<c0574539>] driver_attach+0x19/0x20
> [  104.321652]  [<c0574d30>] ? driver_probe_device+0x360/0x360
> [  104.321658]  [<c05740c4>] bus_add_driver+0xe4/0x260
> [  104.321665]  [<c0575375>] driver_register+0x65/0x160
> [  104.321673]  [<c029beed>] ? smp_call_function+0x2d/0x50
> [  104.321681]  [<c0236870>] ? __cpa_process_fault+0x80/0x80
> [  104.321688]  [<c05ddf22>] usb_register_driver+0x62/0x150
> [  104.321695]  [<c023752a>] ? change_page_attr_set_clr+0x2da/0x380
> [  104.321707]  [<f8d5c000>] ? 0xf8d5bfff
> [  104.321724]  [<f8d5c017>] em28xx_usb_driver_init+0x17/0x1000 [em28xx]
> [  104.321732]  [<c02003fa>] do_one_initcall+0xaa/0x160
> [  104.321741]  [<c02d0ab4>] ? tracepoint_module_notify+0xc4/0x180
> [  104.321752]  [<f8d5c000>] ? 0xf8d5bfff
> [  104.321758]  [<c0237797>] ? set_memory_nx+0x57/0x60
> [  104.321772]  [<c0740c14>] ? set_section_ro_nx+0x4f/0x54
> [  104.321781]  [<c02a1388>] load_module+0x1ba8/0x2510
> [  104.321792]  [<c02a1d87>] SyS_init_module+0x97/0x100
> [  104.321801]  [<c030f463>] ? vm_mmap_pgoff+0x83/0xb0
> [  104.321811]  [<c074fc3a>] sysenter_do_call+0x12/0x22
> [  104.321816] ---[ end trace ce95bae000cad89a ]---
> [  104.321823] em2765 #0: analog set to bulk mode.
> [  104.322214] usbcore: registered new interface driver em28xx
> [  104.373835] ------------[ cut here ]------------
> [  104.373886] WARNING: CPU: 0 PID: 2087 at
> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> [videodev]()
> [  104.373892] Unbalanced v4l2_clk_disable() on 11-0030:mclk!

Here's another one of the same kind.

> [  104.373896] Modules linked in: snd_rawmidi ov2640 soc_camera
> soc_mediabus em28xx videobuf2_core videobuf2_vmalloc videobuf2_memops
> xt_tcpudp xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy
> autofs4 radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
> ata_generic pata_amd pata_jmicron sata_nv
> [  104.374088] CPU: 0 PID: 2087 Comm: v4l_id Tainted: G        W   
> 3.11.0-rc2-0.1-desktop+ #19
> [  104.374093] Hardware name: System manufacturer System Product
> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
> [  104.374098]  00000000 00000000 eb247e74 c0743834 eb247eb4 eb247ea4
> c023f5ef f7bde565
> [  104.374110]  eb247ed0 00000827 f7bdf620 00000083 f7bd9f03 f7bd9f03
> eb112c40 eb112c58
> [  104.374121]  00000000 eb247ebc c023f68e 00000009 eb247eb4 f7bde565
> eb247ed0 eb247ee4
> [  104.374132] Call Trace:
> [  104.374145]  [<c0743834>] dump_stack+0x4b/0x72
> [  104.374178]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
> [  104.374208]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.374228]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.374242]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
> [  104.374266]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
> [  104.374284]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
> [  104.374293]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
> [  104.374308]  [<f8d44c16>] em28xx_v4l2_close+0x86/0x150 [em28xx]
> [  104.374326]  [<f7bcf02e>] v4l2_release+0x2e/0x70 [videodev]
> [  104.374335]  [<c034558f>] __fput+0xaf/0x1d0
> [  104.374342]  [<c03456e8>] ____fput+0x8/0x10
> [  104.374352]  [<c025d189>] task_work_run+0x79/0x90
> [  104.374359]  [<c0202071>] do_notify_resume+0x41/0x70
> [  104.374368]  [<c0749a8f>] work_notifysig+0x24/0x29
> [  104.374374] ---[ end trace ce95bae000cad89b ]---
> [  104.440959] usbcore: registered new interface driver snd-usb-audio

So, above I didn't see anything bad, does the camera actually work with 
this?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-03  6:34   ` Guennadi Liakhovetski
@ 2013-09-05 13:32     ` Guennadi Liakhovetski
  2013-09-05 15:22     ` Frank Schäfer
  1 sibling, 0 replies; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-09-05 13:32 UTC (permalink / raw)
  To: Frank Schäfer
  Cc: linux-media, Laurent Pinchart, Mauro Carvalho Chehab,
	Sylwester Nawrocki, Hans Verkuil

Hi Frank,

On Tue, 3 Sep 2013, Guennadi Liakhovetski wrote:

> Hi Frank
> 
> Thanks for testing! Let's have a look then:
> 
> On Mon, 2 Sep 2013, Frank Schäfer wrote:
> 
> > Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
> > > This patch series adds a V4L2 clock support to em28xx with an ov2640 
> > > sensor. Only compile tested, might need fixing, please, test.
> > >
> > > Guennadi Liakhovetski (3):
> > >   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
> > >     clock
> > >   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
> > >   V4L2: em28xx: register a V4L2 clock source
> > >
> > >  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
> > >  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
> > >  drivers/media/usb/em28xx/em28xx.h        |    1 +
> > >  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
> > >  include/media/v4l2-clk.h                 |   17 ++++++++++++
> > >  5 files changed, 91 insertions(+), 10 deletions(-)
> > >
> > 
> > Tested a few minutes ago:

[snip]

> > [  104.321167] ------------[ cut here ]------------
> > [  104.321216] WARNING: CPU: 0 PID: 517 at
> > drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> > [videodev]()
> > [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> 
> Ok, this is because em28xx_init_dev() calls
> 
> 	/* Save some power by putting tuner to sleep */
> 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> 
> without turning the subdevice on before. Are those subdevices on by 
> default? In principle, this warning is harmless and it should still work 
> afterwards, but we should still clean this up - by either removing the 
> warning, or adding a power-on before a power-off in em28xx_init_dev(), or 
> somehow else. In fact, I think, this should indeed be done: 
> em28xx_card_setup() performs i2c accesses, right? So, we have to power up 
> the subdev before that.

Could you re-test with the .s_power() fixup patch I've sent several 
minutes ago?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-03  6:34   ` Guennadi Liakhovetski
  2013-09-05 13:32     ` Guennadi Liakhovetski
@ 2013-09-05 15:22     ` Frank Schäfer
  2013-09-05 15:41       ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 17+ messages in thread
From: Frank Schäfer @ 2013-09-05 15:22 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: linux-media, Laurent Pinchart, Mauro Carvalho Chehab,
	Sylwester Nawrocki, Hans Verkuil

Hi Guennadi,

sorry for delayed replies, I'm currently burried under lots of stuff
with a higher priority...

Am 03.09.2013 08:34, schrieb Guennadi Liakhovetski:
> Hi Frank
>
> Thanks for testing! Let's have a look then:
>
> On Mon, 2 Sep 2013, Frank Schäfer wrote:
>
>> Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
>>> This patch series adds a V4L2 clock support to em28xx with an ov2640 
>>> sensor. Only compile tested, might need fixing, please, test.
>>>
>>> Guennadi Liakhovetski (3):
>>>   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
>>>     clock
>>>   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
>>>   V4L2: em28xx: register a V4L2 clock source
>>>
>>>  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
>>>  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
>>>  drivers/media/usb/em28xx/em28xx.h        |    1 +
>>>  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
>>>  include/media/v4l2-clk.h                 |   17 ++++++++++++
>>>  5 files changed, 91 insertions(+), 10 deletions(-)
>>>
>> Tested a few minutes ago:
>>
>> ...
>> [  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
>> [  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
>> max is 2
>> [  103.678559] usb 1-8: config 1 has no interface number 2
>> [  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
>> [  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
>> SerialNumber=0
>> [  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
>> [  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
>> [  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
>> class 3)
>> [  103.797066] em28xx: Video interface 3 found: bulk
>> [  103.933941] em28xx: chip ID is em2765
>> [  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
>> fa 02 d2 0a 32 02
>> [  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
>> 00 4b 12 0e 1f d2
>> [  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
>> 00 4b 02 0e 1f 80
>> [  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
>> 84 22 02 0c 78 00
>> [  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
>> 0c 44 02 00 03 a2
>> [  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
>> 04 90 00 00 00 00
>> [  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
>> 00 20 01 01 00 01
>> [  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00
>> [  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
>> [  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
>> [  104.043922] em2765 #0: EEPROM info:
>> [  104.043924] em2765 #0:       microcode start address = 0x0004, boot
>> configuration = 0x01
>> [  104.069818] em2765 #0:       no hardware configuration dataset found
>> in eeprom
>> [  104.080693] em2765 #0: sensor OV2640 detected
>> [  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
>> Laplace webcam (card=90)
>> [  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
>> [  104.173836] i2c i2c-11: OV2640 Probed
> I presume, this is good.
>
>> [  104.306698] em2765 #0: Config register raw data: 0x00
>> [  104.306717] em2765 #0: v4l2 driver version 0.2.0
>> [  104.321152] em2765 #0: V4L2 video device registered as video1
> This is good too.
>
>> [  104.321167] ------------[ cut here ]------------
>> [  104.321216] WARNING: CPU: 0 PID: 517 at
>> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
>> [videodev]()
>> [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> Ok, this is because em28xx_init_dev() calls
>
> 	/* Save some power by putting tuner to sleep */
> 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>
> without turning the subdevice on before. Are those subdevices on by 
> default? 
I don't know. We have numerous magic GPIO sequences in the em28xx
driver... ;)
It has at least been working so far without a (s_power, 1) call. ;)

> In principle, this warning is harmless and it should still work 
> afterwards, but we should still clean this up - by either removing the 
> warning, or adding a power-on before a power-off in em28xx_init_dev(), or 
> somehow else. In fact, I think, this should indeed be done: 
> em28xx_card_setup() performs i2c accesses, right? So, we have to power up 
> the subdev before that.

I agree, but I also think the warning should be removed.
The log looks scary and multiple/unbalanced s_power calls are nothing
unusual.

>> [  104.321226] Modules linked in: ov2640 soc_camera soc_mediabus
>> em28xx(+) videobuf2_core videobuf2_vmalloc videobuf2_memops xt_tcpudp
>> xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
>> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
>> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
>> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
>> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
>> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
>> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
>> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
>> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
>> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
>> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
>> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
>> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy autofs4
>> radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
>> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
>> ata_generic pata_amd pata_jmicron sata_nv
>> [  104.321353] CPU: 0 PID: 517 Comm: udevd Not tainted
>> 3.11.0-rc2-0.1-desktop+ #19
>> [  104.321358] Hardware name: System manufacturer System Product
>> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
>> [  104.321363]  00000000 00000000 f52dbb70 c0743834 f52dbbb0 f52dbba0
>> c023f5ef f7bde565
>> [  104.321375]  f52dbbcc 00000205 f7bdf620 00000083 f7bd9f03 f7bd9f03
>> eb112c40 eb112c58
>> [  104.321386]  00000000 f52dbbb8 c023f68e 00000009 f52dbbb0 f7bde565
>> f52dbbcc f52dbbe0
>> [  104.321397] Call Trace:
>> [  104.321411]  [<c0743834>] dump_stack+0x4b/0x72
>> [  104.321420]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
>> [  104.321446]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.321466]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.321473]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
>> [  104.321494]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.321511]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
>> [  104.321530]  [<f8d46f13>] ?
>> em28xx_register_analog_devices+0x653/0x6c0 [em28xx]
>> [  104.321539]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
>> [  104.321555]  [<f8d492c2>] em28xx_usb_probe+0xef2/0x1330 [em28xx]
>> [  104.321564]  [<c03a31f5>] ? __sysfs_add_one+0x55/0xf0
>> [  104.321574]  [<c057f5f8>] ? __pm_runtime_set_status+0xf8/0x210
>> [  104.321581]  [<c057f4b1>] ? __pm_runtime_resume+0x41/0x50
>> [  104.321590]  [<c05df477>] usb_probe_interface+0x187/0x2c0
>> [  104.321598]  [<c05745aa>] ? driver_sysfs_add+0x6a/0x90
>> [  104.321604]  [<c0574a44>] driver_probe_device+0x74/0x360
>> [  104.321610]  [<c05ded31>] ? usb_match_id+0x41/0x60
>> [  104.321617]  [<c05ded9e>] ? usb_device_match+0x4e/0x90
>> [  104.321623]  [<c0574db9>] __driver_attach+0x89/0x90
>> [  104.321630]  [<c0574d30>] ? driver_probe_device+0x360/0x360
>> [  104.321639]  [<c0572f92>] bus_for_each_dev+0x42/0x80
>> [  104.321645]  [<c0574539>] driver_attach+0x19/0x20
>> [  104.321652]  [<c0574d30>] ? driver_probe_device+0x360/0x360
>> [  104.321658]  [<c05740c4>] bus_add_driver+0xe4/0x260
>> [  104.321665]  [<c0575375>] driver_register+0x65/0x160
>> [  104.321673]  [<c029beed>] ? smp_call_function+0x2d/0x50
>> [  104.321681]  [<c0236870>] ? __cpa_process_fault+0x80/0x80
>> [  104.321688]  [<c05ddf22>] usb_register_driver+0x62/0x150
>> [  104.321695]  [<c023752a>] ? change_page_attr_set_clr+0x2da/0x380
>> [  104.321707]  [<f8d5c000>] ? 0xf8d5bfff
>> [  104.321724]  [<f8d5c017>] em28xx_usb_driver_init+0x17/0x1000 [em28xx]
>> [  104.321732]  [<c02003fa>] do_one_initcall+0xaa/0x160
>> [  104.321741]  [<c02d0ab4>] ? tracepoint_module_notify+0xc4/0x180
>> [  104.321752]  [<f8d5c000>] ? 0xf8d5bfff
>> [  104.321758]  [<c0237797>] ? set_memory_nx+0x57/0x60
>> [  104.321772]  [<c0740c14>] ? set_section_ro_nx+0x4f/0x54
>> [  104.321781]  [<c02a1388>] load_module+0x1ba8/0x2510
>> [  104.321792]  [<c02a1d87>] SyS_init_module+0x97/0x100
>> [  104.321801]  [<c030f463>] ? vm_mmap_pgoff+0x83/0xb0
>> [  104.321811]  [<c074fc3a>] sysenter_do_call+0x12/0x22
>> [  104.321816] ---[ end trace ce95bae000cad89a ]---
>> [  104.321823] em2765 #0: analog set to bulk mode.
>> [  104.322214] usbcore: registered new interface driver em28xx
>> [  104.373835] ------------[ cut here ]------------
>> [  104.373886] WARNING: CPU: 0 PID: 2087 at
>> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
>> [videodev]()
>> [  104.373892] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> Here's another one of the same kind.
>
>> [  104.373896] Modules linked in: snd_rawmidi ov2640 soc_camera
>> soc_mediabus em28xx videobuf2_core videobuf2_vmalloc videobuf2_memops
>> xt_tcpudp xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
>> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
>> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
>> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
>> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
>> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
>> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
>> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
>> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
>> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
>> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
>> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
>> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy
>> autofs4 radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
>> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
>> ata_generic pata_amd pata_jmicron sata_nv
>> [  104.374088] CPU: 0 PID: 2087 Comm: v4l_id Tainted: G        W   
>> 3.11.0-rc2-0.1-desktop+ #19
>> [  104.374093] Hardware name: System manufacturer System Product
>> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
>> [  104.374098]  00000000 00000000 eb247e74 c0743834 eb247eb4 eb247ea4
>> c023f5ef f7bde565
>> [  104.374110]  eb247ed0 00000827 f7bdf620 00000083 f7bd9f03 f7bd9f03
>> eb112c40 eb112c58
>> [  104.374121]  00000000 eb247ebc c023f68e 00000009 eb247eb4 f7bde565
>> eb247ed0 eb247ee4
>> [  104.374132] Call Trace:
>> [  104.374145]  [<c0743834>] dump_stack+0x4b/0x72
>> [  104.374178]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
>> [  104.374208]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.374228]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.374242]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
>> [  104.374266]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
>> [  104.374284]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
>> [  104.374293]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
>> [  104.374308]  [<f8d44c16>] em28xx_v4l2_close+0x86/0x150 [em28xx]
>> [  104.374326]  [<f7bcf02e>] v4l2_release+0x2e/0x70 [videodev]
>> [  104.374335]  [<c034558f>] __fput+0xaf/0x1d0
>> [  104.374342]  [<c03456e8>] ____fput+0x8/0x10
>> [  104.374352]  [<c025d189>] task_work_run+0x79/0x90
>> [  104.374359]  [<c0202071>] do_notify_resume+0x41/0x70
>> [  104.374368]  [<c0749a8f>] work_notifysig+0x24/0x29
>> [  104.374374] ---[ end trace ce95bae000cad89b ]---
>> [  104.440959] usbcore: registered new interface driver snd-usb-audio
> So, above I didn't see anything bad, does the camera actually work with 
> this?
Yes, it works.

Regards,
Frank

> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-05 15:22     ` Frank Schäfer
@ 2013-09-05 15:41       ` Mauro Carvalho Chehab
  2013-09-05 15:57         ` Guennadi Liakhovetski
  0 siblings, 1 reply; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2013-09-05 15:41 UTC (permalink / raw)
  To: Frank Schäfer
  Cc: Guennadi Liakhovetski, linux-media, Laurent Pinchart,
	Sylwester Nawrocki, Hans Verkuil

Em Thu, 05 Sep 2013 17:22:36 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Hi Guennadi,
> 
> sorry for delayed replies, I'm currently burried under lots of stuff
> with a higher priority...
> 
> Am 03.09.2013 08:34, schrieb Guennadi Liakhovetski:
> > Hi Frank
> >
> > Thanks for testing! Let's have a look then:
> >
> > On Mon, 2 Sep 2013, Frank Schäfer wrote:
> >
> >> Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
> >>> This patch series adds a V4L2 clock support to em28xx with an ov2640 
> >>> sensor. Only compile tested, might need fixing, please, test.
> >>>
> >>> Guennadi Liakhovetski (3):
> >>>   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
> >>>     clock
> >>>   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
> >>>   V4L2: em28xx: register a V4L2 clock source
> >>>
> >>>  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
> >>>  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
> >>>  drivers/media/usb/em28xx/em28xx.h        |    1 +
> >>>  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
> >>>  include/media/v4l2-clk.h                 |   17 ++++++++++++
> >>>  5 files changed, 91 insertions(+), 10 deletions(-)
> >>>
> >> Tested a few minutes ago:
> >>
> >> ...
> >> [  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
> >> [  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
> >> max is 2
> >> [  103.678559] usb 1-8: config 1 has no interface number 2
> >> [  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
> >> [  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
> >> SerialNumber=0
> >> [  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
> >> [  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
> >> [  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
> >> class 3)
> >> [  103.797066] em28xx: Video interface 3 found: bulk
> >> [  103.933941] em28xx: chip ID is em2765
> >> [  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
> >> fa 02 d2 0a 32 02
> >> [  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
> >> 00 4b 12 0e 1f d2
> >> [  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
> >> 00 4b 02 0e 1f 80
> >> [  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
> >> 84 22 02 0c 78 00
> >> [  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
> >> 0c 44 02 00 03 a2
> >> [  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
> >> 04 90 00 00 00 00
> >> [  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
> >> 00 20 01 01 00 01
> >> [  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00
> >> [  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
> >> [  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
> >> [  104.043922] em2765 #0: EEPROM info:
> >> [  104.043924] em2765 #0:       microcode start address = 0x0004, boot
> >> configuration = 0x01
> >> [  104.069818] em2765 #0:       no hardware configuration dataset found
> >> in eeprom
> >> [  104.080693] em2765 #0: sensor OV2640 detected
> >> [  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
> >> Laplace webcam (card=90)
> >> [  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
> >> [  104.173836] i2c i2c-11: OV2640 Probed
> > I presume, this is good.
> >
> >> [  104.306698] em2765 #0: Config register raw data: 0x00
> >> [  104.306717] em2765 #0: v4l2 driver version 0.2.0
> >> [  104.321152] em2765 #0: V4L2 video device registered as video1
> > This is good too.
> >
> >> [  104.321167] ------------[ cut here ]------------
> >> [  104.321216] WARNING: CPU: 0 PID: 517 at
> >> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> >> [videodev]()
> >> [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> > Ok, this is because em28xx_init_dev() calls
> >
> > 	/* Save some power by putting tuner to sleep */
> > 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> >
> > without turning the subdevice on before. Are those subdevices on by 
> > default? 
> I don't know. We have numerous magic GPIO sequences in the em28xx
> driver... ;)
> It has at least been working so far without a (s_power, 1) call. ;)

On em28xx (as on almost all PCI/USB drivers), the devices are powered
on via GPIO, before loading the I2C drivers. This is needed, otherwise
several I2C drivers will refuse to load, as their synchronous
initialization procedures validate if the device is really present.

So, the power default state is ON.

Please notice that changing it is not an option, as there are several
cases where the very same device ID can have different I2C chips,
and the bridge driver probe() routine uses the subdevice probes to
try other possible subdevices.

Rewriting that part of the code would require to test the changes on
several hundreds of different devices, and even if you find someone
with all those devices, I doubt that he would have enough time to
re-test everything.

So, either the above unbalance check should be removed, or its behavior
should be changed to assume that the devices are ON at probe() time,
as it used to be before the async patches.

> > In principle, this warning is harmless and it should still work 
> > afterwards, but we should still clean this up - by either removing the 
> > warning, or adding a power-on before a power-off in em28xx_init_dev(), or 
> > somehow else. In fact, I think, this should indeed be done: 
> > em28xx_card_setup() performs i2c accesses, right? So, we have to power up 
> > the subdev before that.
> 
> I agree, but I also think the warning should be removed.
> The log looks scary and multiple/unbalanced s_power calls are nothing
> unusual.

Agreed: those messages only cause confusion and should not happen when
the device is using a synchronous probing method.

> 
> >> [  104.321226] Modules linked in: ov2640 soc_camera soc_mediabus
> >> em28xx(+) videobuf2_core videobuf2_vmalloc videobuf2_memops xt_tcpudp
> >> xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
> >> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
> >> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
> >> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
> >> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
> >> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
> >> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
> >> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
> >> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
> >> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
> >> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
> >> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
> >> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy autofs4
> >> radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
> >> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
> >> ata_generic pata_amd pata_jmicron sata_nv
> >> [  104.321353] CPU: 0 PID: 517 Comm: udevd Not tainted
> >> 3.11.0-rc2-0.1-desktop+ #19
> >> [  104.321358] Hardware name: System manufacturer System Product
> >> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
> >> [  104.321363]  00000000 00000000 f52dbb70 c0743834 f52dbbb0 f52dbba0
> >> c023f5ef f7bde565
> >> [  104.321375]  f52dbbcc 00000205 f7bdf620 00000083 f7bd9f03 f7bd9f03
> >> eb112c40 eb112c58
> >> [  104.321386]  00000000 f52dbbb8 c023f68e 00000009 f52dbbb0 f7bde565
> >> f52dbbcc f52dbbe0
> >> [  104.321397] Call Trace:
> >> [  104.321411]  [<c0743834>] dump_stack+0x4b/0x72
> >> [  104.321420]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
> >> [  104.321446]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.321466]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.321473]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
> >> [  104.321494]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.321511]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
> >> [  104.321530]  [<f8d46f13>] ?
> >> em28xx_register_analog_devices+0x653/0x6c0 [em28xx]
> >> [  104.321539]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
> >> [  104.321555]  [<f8d492c2>] em28xx_usb_probe+0xef2/0x1330 [em28xx]
> >> [  104.321564]  [<c03a31f5>] ? __sysfs_add_one+0x55/0xf0
> >> [  104.321574]  [<c057f5f8>] ? __pm_runtime_set_status+0xf8/0x210
> >> [  104.321581]  [<c057f4b1>] ? __pm_runtime_resume+0x41/0x50
> >> [  104.321590]  [<c05df477>] usb_probe_interface+0x187/0x2c0
> >> [  104.321598]  [<c05745aa>] ? driver_sysfs_add+0x6a/0x90
> >> [  104.321604]  [<c0574a44>] driver_probe_device+0x74/0x360
> >> [  104.321610]  [<c05ded31>] ? usb_match_id+0x41/0x60
> >> [  104.321617]  [<c05ded9e>] ? usb_device_match+0x4e/0x90
> >> [  104.321623]  [<c0574db9>] __driver_attach+0x89/0x90
> >> [  104.321630]  [<c0574d30>] ? driver_probe_device+0x360/0x360
> >> [  104.321639]  [<c0572f92>] bus_for_each_dev+0x42/0x80
> >> [  104.321645]  [<c0574539>] driver_attach+0x19/0x20
> >> [  104.321652]  [<c0574d30>] ? driver_probe_device+0x360/0x360
> >> [  104.321658]  [<c05740c4>] bus_add_driver+0xe4/0x260
> >> [  104.321665]  [<c0575375>] driver_register+0x65/0x160
> >> [  104.321673]  [<c029beed>] ? smp_call_function+0x2d/0x50
> >> [  104.321681]  [<c0236870>] ? __cpa_process_fault+0x80/0x80
> >> [  104.321688]  [<c05ddf22>] usb_register_driver+0x62/0x150
> >> [  104.321695]  [<c023752a>] ? change_page_attr_set_clr+0x2da/0x380
> >> [  104.321707]  [<f8d5c000>] ? 0xf8d5bfff
> >> [  104.321724]  [<f8d5c017>] em28xx_usb_driver_init+0x17/0x1000 [em28xx]
> >> [  104.321732]  [<c02003fa>] do_one_initcall+0xaa/0x160
> >> [  104.321741]  [<c02d0ab4>] ? tracepoint_module_notify+0xc4/0x180
> >> [  104.321752]  [<f8d5c000>] ? 0xf8d5bfff
> >> [  104.321758]  [<c0237797>] ? set_memory_nx+0x57/0x60
> >> [  104.321772]  [<c0740c14>] ? set_section_ro_nx+0x4f/0x54
> >> [  104.321781]  [<c02a1388>] load_module+0x1ba8/0x2510
> >> [  104.321792]  [<c02a1d87>] SyS_init_module+0x97/0x100
> >> [  104.321801]  [<c030f463>] ? vm_mmap_pgoff+0x83/0xb0
> >> [  104.321811]  [<c074fc3a>] sysenter_do_call+0x12/0x22
> >> [  104.321816] ---[ end trace ce95bae000cad89a ]---
> >> [  104.321823] em2765 #0: analog set to bulk mode.
> >> [  104.322214] usbcore: registered new interface driver em28xx
> >> [  104.373835] ------------[ cut here ]------------
> >> [  104.373886] WARNING: CPU: 0 PID: 2087 at
> >> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> >> [videodev]()
> >> [  104.373892] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> > Here's another one of the same kind.
> >
> >> [  104.373896] Modules linked in: snd_rawmidi ov2640 soc_camera
> >> soc_mediabus em28xx videobuf2_core videobuf2_vmalloc videobuf2_memops
> >> xt_tcpudp xt_pkttype xt_LOG af_packet xt_limit snd_hda_codec_hdmi
> >> snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
> >> ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT
> >> iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
> >> nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
> >> xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables fuse
> >> snd_seq arc4 rtl8187 rc_hauppauge ir_kbd_i2c tuner_simple tuner_types
> >> mac80211 tda9887 snd_timer tda8290 tuner cfg80211 snd_seq_device msp3400
> >> snd bttv usb_storage usblp firewire_ohci shpchp sg firewire_core sr_mod
> >> rfkill v4l2_common eeprom_93cx6 videodev crc_itu_t ppdev pci_hotplug
> >> serio_raw videobuf_dma_sg soundcore videobuf_core parport_pc parport
> >> i2c_nforce2 forcedeth k8temp snd_page_alloc btcx_risc rc_core tveeprom
> >> mperf asus_atk0110 pcspkr powernow_k8 button cdrom floppy
> >> autofs4 radeon ttm drm_kms_helper drm i2c_algo_bit thermal fan processor
> >> thermal_sys scsi_dh_alua scsi_dh_hp_sw scsi_dh_emc scsi_dh_rdac scsi_dh
> >> ata_generic pata_amd pata_jmicron sata_nv
> >> [  104.374088] CPU: 0 PID: 2087 Comm: v4l_id Tainted: G        W   
> >> 3.11.0-rc2-0.1-desktop+ #19
> >> [  104.374093] Hardware name: System manufacturer System Product
> >> Name/M2N-VM DH, BIOS ASUS M2N-VM DH ACPI BIOS Revision 1503 09/16/2010
> >> [  104.374098]  00000000 00000000 eb247e74 c0743834 eb247eb4 eb247ea4
> >> c023f5ef f7bde565
> >> [  104.374110]  eb247ed0 00000827 f7bdf620 00000083 f7bd9f03 f7bd9f03
> >> eb112c40 eb112c58
> >> [  104.374121]  00000000 eb247ebc c023f68e 00000009 eb247eb4 f7bde565
> >> eb247ed0 eb247ee4
> >> [  104.374132] Call Trace:
> >> [  104.374145]  [<c0743834>] dump_stack+0x4b/0x72
> >> [  104.374178]  [<c023f5ef>] warn_slowpath_common+0x7f/0xa0
> >> [  104.374208]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.374228]  [<f7bd9f03>] ? v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.374242]  [<c023f68e>] warn_slowpath_fmt+0x2e/0x30
> >> [  104.374266]  [<f7bd9f03>] v4l2_clk_disable+0x83/0x90 [videodev]
> >> [  104.374284]  [<f8d2b821>] soc_camera_power_off+0x31/0x60 [soc_camera]
> >> [  104.374293]  [<f8d33284>] ov2640_s_power+0x34/0x60 [ov2640]
> >> [  104.374308]  [<f8d44c16>] em28xx_v4l2_close+0x86/0x150 [em28xx]
> >> [  104.374326]  [<f7bcf02e>] v4l2_release+0x2e/0x70 [videodev]
> >> [  104.374335]  [<c034558f>] __fput+0xaf/0x1d0
> >> [  104.374342]  [<c03456e8>] ____fput+0x8/0x10
> >> [  104.374352]  [<c025d189>] task_work_run+0x79/0x90
> >> [  104.374359]  [<c0202071>] do_notify_resume+0x41/0x70
> >> [  104.374368]  [<c0749a8f>] work_notifysig+0x24/0x29
> >> [  104.374374] ---[ end trace ce95bae000cad89b ]---
> >> [  104.440959] usbcore: registered new interface driver snd-usb-audio
> > So, above I didn't see anything bad, does the camera actually work with 
> > this?
> Yes, it works.
> 
> Regards,
> Frank
> 
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> 


-- 

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-05 15:41       ` Mauro Carvalho Chehab
@ 2013-09-05 15:57         ` Guennadi Liakhovetski
  2013-09-09 17:27           ` Frank Schäfer
  0 siblings, 1 reply; 17+ messages in thread
From: Guennadi Liakhovetski @ 2013-09-05 15:57 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Frank Schäfer, linux-media, Laurent Pinchart,
	Sylwester Nawrocki, Hans Verkuil

On Thu, 5 Sep 2013, Mauro Carvalho Chehab wrote:

> Em Thu, 05 Sep 2013 17:22:36 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> 
> > Hi Guennadi,
> > 
> > sorry for delayed replies, I'm currently burried under lots of stuff
> > with a higher priority...
> > 
> > Am 03.09.2013 08:34, schrieb Guennadi Liakhovetski:
> > > Hi Frank
> > >
> > > Thanks for testing! Let's have a look then:
> > >
> > > On Mon, 2 Sep 2013, Frank Schäfer wrote:
> > >
> > >> Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
> > >>> This patch series adds a V4L2 clock support to em28xx with an ov2640 
> > >>> sensor. Only compile tested, might need fixing, please, test.
> > >>>
> > >>> Guennadi Liakhovetski (3):
> > >>>   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
> > >>>     clock
> > >>>   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
> > >>>   V4L2: em28xx: register a V4L2 clock source
> > >>>
> > >>>  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
> > >>>  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
> > >>>  drivers/media/usb/em28xx/em28xx.h        |    1 +
> > >>>  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
> > >>>  include/media/v4l2-clk.h                 |   17 ++++++++++++
> > >>>  5 files changed, 91 insertions(+), 10 deletions(-)
> > >>>
> > >> Tested a few minutes ago:
> > >>
> > >> ...
> > >> [  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
> > >> [  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
> > >> max is 2
> > >> [  103.678559] usb 1-8: config 1 has no interface number 2
> > >> [  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
> > >> [  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
> > >> SerialNumber=0
> > >> [  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
> > >> [  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
> > >> [  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
> > >> class 3)
> > >> [  103.797066] em28xx: Video interface 3 found: bulk
> > >> [  103.933941] em28xx: chip ID is em2765
> > >> [  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
> > >> fa 02 d2 0a 32 02
> > >> [  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
> > >> 00 4b 12 0e 1f d2
> > >> [  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
> > >> 00 4b 02 0e 1f 80
> > >> [  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
> > >> 84 22 02 0c 78 00
> > >> [  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
> > >> 0c 44 02 00 03 a2
> > >> [  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
> > >> 04 90 00 00 00 00
> > >> [  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
> > >> 00 20 01 01 00 01
> > >> [  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
> > >> 00 00 00 00 00 00
> > >> [  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
> > >> [  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
> > >> [  104.043922] em2765 #0: EEPROM info:
> > >> [  104.043924] em2765 #0:       microcode start address = 0x0004, boot
> > >> configuration = 0x01
> > >> [  104.069818] em2765 #0:       no hardware configuration dataset found
> > >> in eeprom
> > >> [  104.080693] em2765 #0: sensor OV2640 detected
> > >> [  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
> > >> Laplace webcam (card=90)
> > >> [  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
> > >> [  104.173836] i2c i2c-11: OV2640 Probed
> > > I presume, this is good.
> > >
> > >> [  104.306698] em2765 #0: Config register raw data: 0x00
> > >> [  104.306717] em2765 #0: v4l2 driver version 0.2.0
> > >> [  104.321152] em2765 #0: V4L2 video device registered as video1
> > > This is good too.
> > >
> > >> [  104.321167] ------------[ cut here ]------------
> > >> [  104.321216] WARNING: CPU: 0 PID: 517 at
> > >> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
> > >> [videodev]()
> > >> [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
> > > Ok, this is because em28xx_init_dev() calls
> > >
> > > 	/* Save some power by putting tuner to sleep */
> > > 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> > >
> > > without turning the subdevice on before. Are those subdevices on by 
> > > default? 
> > I don't know. We have numerous magic GPIO sequences in the em28xx
> > driver... ;)
> > It has at least been working so far without a (s_power, 1) call. ;)
> 
> On em28xx (as on almost all PCI/USB drivers), the devices are powered
> on via GPIO, before loading the I2C drivers. This is needed, otherwise
> several I2C drivers will refuse to load, as their synchronous
> initialization procedures validate if the device is really present.
> 
> So, the power default state is ON.
> 
> Please notice that changing it is not an option, as there are several
> cases where the very same device ID can have different I2C chips,
> and the bridge driver probe() routine uses the subdevice probes to
> try other possible subdevices.
> 
> Rewriting that part of the code would require to test the changes on
> several hundreds of different devices, and even if you find someone
> with all those devices, I doubt that he would have enough time to
> re-test everything.
> 
> So, either the above unbalance check should be removed, or its behavior
> should be changed to assume that the devices are ON at probe() time,
> as it used to be before the async patches.

Ok, we can certainly do any of the above, but just for understanding - how 
does it actually work now? I mean - ok, I can accept, that the default 
reset state is power on. But the driver then forcedly powers all 
subdevices off upon close() or in the end of initialisation, performed 
during probing - and _never_ explicitly powers them on! That doesn't seem 
right to me. Even if it happens to work.

Further, I grepped em28xx for s_power - only callers have those hooks, I 
didn't find any subdevices with them actually implemented. ov2640 has it 
and it calls soc_camera internal methods, which in the em28xx case also 
end up doing nothing. So, how and which subdevices actually save power 
there and how are they turned back on?

I'll try to look at external subdevice drivers, that are used by em28xx, 
but any hints would be appreciated.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-05 15:57         ` Guennadi Liakhovetski
@ 2013-09-09 17:27           ` Frank Schäfer
  2013-09-09 17:30             ` Frank Schäfer
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Schäfer @ 2013-09-09 17:27 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: Mauro Carvalho Chehab, linux-media, Laurent Pinchart,
	Sylwester Nawrocki, Hans Verkuil

Am 05.09.2013 17:57, schrieb Guennadi Liakhovetski:
> On Thu, 5 Sep 2013, Mauro Carvalho Chehab wrote:
>
>> Em Thu, 05 Sep 2013 17:22:36 +0200
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>> Hi Guennadi,
>>>
>>> sorry for delayed replies, I'm currently burried under lots of stuff
>>> with a higher priority...
>>>
>>> Am 03.09.2013 08:34, schrieb Guennadi Liakhovetski:
>>>> Hi Frank
>>>>
>>>> Thanks for testing! Let's have a look then:
>>>>
>>>> On Mon, 2 Sep 2013, Frank Schäfer wrote:
>>>>
>>>>> Am 28.08.2013 15:28, schrieb Guennadi Liakhovetski:
>>>>>> This patch series adds a V4L2 clock support to em28xx with an ov2640 
>>>>>> sensor. Only compile tested, might need fixing, please, test.
>>>>>>
>>>>>> Guennadi Liakhovetski (3):
>>>>>>   V4L2: add v4l2-clock helpers to register and unregister a fixed-rate
>>>>>>     clock
>>>>>>   V4L2: add a v4l2-clk helper macro to produce an I2C device ID
>>>>>>   V4L2: em28xx: register a V4L2 clock source
>>>>>>
>>>>>>  drivers/media/usb/em28xx/em28xx-camera.c |   41 ++++++++++++++++++++++-------
>>>>>>  drivers/media/usb/em28xx/em28xx-cards.c  |    3 ++
>>>>>>  drivers/media/usb/em28xx/em28xx.h        |    1 +
>>>>>>  drivers/media/v4l2-core/v4l2-clk.c       |   39 ++++++++++++++++++++++++++++
>>>>>>  include/media/v4l2-clk.h                 |   17 ++++++++++++
>>>>>>  5 files changed, 91 insertions(+), 10 deletions(-)
>>>>>>
>>>>> Tested a few minutes ago:
>>>>>
>>>>> ...
>>>>> [  103.564065] usb 1-8: new high-speed USB device number 10 using ehci-pci
>>>>> [  103.678554] usb 1-8: config 1 has an invalid interface number: 3 but
>>>>> max is 2
>>>>> [  103.678559] usb 1-8: config 1 has no interface number 2
>>>>> [  103.678566] usb 1-8: New USB device found, idVendor=1ae7, idProduct=9004
>>>>> [  103.678569] usb 1-8: New USB device strings: Mfr=0, Product=0,
>>>>> SerialNumber=0
>>>>> [  103.797040] em28xx audio device (1ae7:9004): interface 0, class 1
>>>>> [  103.797054] em28xx audio device (1ae7:9004): interface 1, class 1
>>>>> [  103.797064] em28xx: New device   @ 480 Mbps (1ae7:9004, interface 3,
>>>>> class 3)
>>>>> [  103.797066] em28xx: Video interface 3 found: bulk
>>>>> [  103.933941] em28xx: chip ID is em2765
>>>>> [  104.043811] em2765 #0: i2c eeprom 0000: 26 00 01 00 02 0d ea c2 ee 30
>>>>> fa 02 d2 0a 32 02
>>>>> [  104.043821] em2765 #0: i2c eeprom 0010: 0d c3 c2 04 12 00 33 c2 04 12
>>>>> 00 4b 12 0e 1f d2
>>>>> [  104.043828] em2765 #0: i2c eeprom 0020: 04 12 00 33 12 0e 1f d2 04 12
>>>>> 00 4b 02 0e 1f 80
>>>>> [  104.043835] em2765 #0: i2c eeprom 0030: 00 a2 85 22 02 0b cb a2 04 92
>>>>> 84 22 02 0c 78 00
>>>>> [  104.043841] em2765 #0: i2c eeprom 0040: 02 0d 69 7b 1f 7d 40 7f 32 02
>>>>> 0c 44 02 00 03 a2
>>>>> [  104.043847] em2765 #0: i2c eeprom 0050: 04 92 85 22 00 00 00 00 e7 1a
>>>>> 04 90 00 00 00 00
>>>>> [  104.043854] em2765 #0: i2c eeprom 0060: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043860] em2765 #0: i2c eeprom 0070: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043866] em2765 #0: i2c eeprom 0080: 00 00 00 00 00 00 1e 40 1e 72
>>>>> 00 20 01 01 00 01
>>>>> [  104.043873] em2765 #0: i2c eeprom 0090: 01 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043879] em2765 #0: i2c eeprom 00a0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043885] em2765 #0: i2c eeprom 00b0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043891] em2765 #0: i2c eeprom 00c0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043898] em2765 #0: i2c eeprom 00d0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043904] em2765 #0: i2c eeprom 00e0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043910] em2765 #0: i2c eeprom 00f0: 00 00 00 00 00 00 00 00 00 00
>>>>> 00 00 00 00 00 00
>>>>> [  104.043917] em2765 #0: i2c eeprom 0100: ... (skipped)
>>>>> [  104.043921] em2765 #0: EEPROM ID = 26 00 01 00, EEPROM hash = 0x5c22c624
>>>>> [  104.043922] em2765 #0: EEPROM info:
>>>>> [  104.043924] em2765 #0:       microcode start address = 0x0004, boot
>>>>> configuration = 0x01
>>>>> [  104.069818] em2765 #0:       no hardware configuration dataset found
>>>>> in eeprom
>>>>> [  104.080693] em2765 #0: sensor OV2640 detected
>>>>> [  104.080715] em2765 #0: Identified as SpeedLink Vicious And Devine
>>>>> Laplace webcam (card=90)
>>>>> [  104.159699] ov2640 11-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2
>>>>> [  104.173836] i2c i2c-11: OV2640 Probed
>>>> I presume, this is good.
>>>>
>>>>> [  104.306698] em2765 #0: Config register raw data: 0x00
>>>>> [  104.306717] em2765 #0: v4l2 driver version 0.2.0
>>>>> [  104.321152] em2765 #0: V4L2 video device registered as video1
>>>> This is good too.
>>>>
>>>>> [  104.321167] ------------[ cut here ]------------
>>>>> [  104.321216] WARNING: CPU: 0 PID: 517 at
>>>>> drivers/media/v4l2-core/v4l2-clk.c:131 v4l2_clk_disable+0x83/0x90
>>>>> [videodev]()
>>>>> [  104.321221] Unbalanced v4l2_clk_disable() on 11-0030:mclk!
>>>> Ok, this is because em28xx_init_dev() calls
>>>>
>>>> 	/* Save some power by putting tuner to sleep */
>>>> 	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>>>>
>>>> without turning the subdevice on before. Are those subdevices on by 
>>>> default? 
>>> I don't know. We have numerous magic GPIO sequences in the em28xx
>>> driver... ;)
>>> It has at least been working so far without a (s_power, 1) call. ;)
>> On em28xx (as on almost all PCI/USB drivers), the devices are powered
>> on via GPIO, before loading the I2C drivers. This is needed, otherwise
>> several I2C drivers will refuse to load, as their synchronous
>> initialization procedures validate if the device is really present.
>>
>> So, the power default state is ON.
>>
>> Please notice that changing it is not an option, as there are several
>> cases where the very same device ID can have different I2C chips,
>> and the bridge driver probe() routine uses the subdevice probes to
>> try other possible subdevices.
True, but how could an additional (s_power, 1) call break this ?

>>
>> Rewriting that part of the code would require to test the changes on
>> several hundreds of different devices, and even if you find someone
>> with all those devices, I doubt that he would have enough time to
>> re-test everything.
>>
>> So, either the above unbalance check should be removed, or its behavior
>> should be changed to assume that the devices are ON at probe() time,
>> as it used to be before the async patches.
> Ok, we can certainly do any of the above, but just for understanding - how 
> does it actually work now? I mean - ok, I can accept, that the default 
> reset state is power on. But the driver then forcedly powers all 
> subdevices off upon close() or in the end of initialisation, performed 
> during probing - and _never_ explicitly powers them on! That doesn't seem 
> right to me. Even if it happens to work.
>
> Further, I grepped em28xx for s_power - only callers have those hooks, I 
> didn't find any subdevices with them actually implemented. ov2640 has it 
> and it calls soc_camera internal methods, which in the em28xx case also 
> end up doing nothing. So, how and which subdevices actually save power 
> there and how are they turned back on?
>
> I'll try to look at external subdevice drivers, that are used by em28xx, 
> but any hints would be appreciated.
Let's have a look at the commit that introduced the s_power calls:

commit 622b828ab795580903e79acb33fb44f5c9ce7b0f
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Mon Oct 5 10:48:17 2009 -0300

    V4L/DVB (13238): v4l2_subdev: rename tuner s_standby operation to
core s_power
   
    Upcoming I2C v4l2_subdev drivers need a way to control the subdevice
    power state from the core. This use case is already partially covered by
    the tuner s_standby operation, but no way to explicitly come back from
    the standby state is available.
   
    Rename the tuner s_standby operation to core s_power, and fix tuner
    drivers accordingly. The tuner core will call s_power(0) instead of
    s_standby(). No explicit call to s_power(1) is required for tuners as
    they are supposed to wake up from standby automatically.
   
    [mchehab@redhat.com: CodingStyle fix]
    Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>



So at least tuners are supposed to wake up automatically.
Yet another reason why these warnings about unbalanced s_power should be
removed.
The problem is, that since this commit ALL subdevices (supporting it)
are put into standby mode, not only the tuners.
Hopefully, this didn't cause any regressions.

Regards,
Frank

>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] V4L2: fix em28xx ov2640 support
  2013-09-09 17:27           ` Frank Schäfer
@ 2013-09-09 17:30             ` Frank Schäfer
  0 siblings, 0 replies; 17+ messages in thread
From: Frank Schäfer @ 2013-09-09 17:30 UTC (permalink / raw)
  To: Guennadi Liakhovetski
  Cc: Mauro Carvalho Chehab, linux-media, Laurent Pinchart,
	Sylwester Nawrocki, Hans Verkuil

Am 09.09.2013 19:27, schrieb Frank Schäfer:
> Am 05.09.2013 17:57, schrieb Guennadi Liakhovetski:
>> On Thu, 5 Sep 2013, Mauro Carvalho Chehab wrote:
>>> Rewriting that part of the code would require to test the changes on
>>> several hundreds of different devices, and even if you find someone
>>> with all those devices, I doubt that he would have enough time to
>>> re-test everything.
>>>
>>> So, either the above unbalance check should be removed, or its behavior
>>> should be changed to assume that the devices are ON at probe() time,
>>> as it used to be before the async patches.
>> Ok, we can certainly do any of the above, but just for understanding - how 
>> does it actually work now? I mean - ok, I can accept, that the default 
>> reset state is power on. But the driver then forcedly powers all 
>> subdevices off upon close() or in the end of initialisation, performed 
>> during probing - and _never_ explicitly powers them on! That doesn't seem 
>> right to me. Even if it happens to work.
>>
>> Further, I grepped em28xx for s_power - only callers have those hooks, I 
>> didn't find any subdevices with them actually implemented. ov2640 has it 
>> and it calls soc_camera internal methods, which in the em28xx case also 
>> end up doing nothing. So, how and which subdevices actually save power 
>> there and how are they turned back on?
>>
>> I'll try to look at external subdevice drivers, that are used by em28xx, 
>> but any hints would be appreciated.
> Let's have a look at the commit that introduced the s_power calls:
>
> commit 622b828ab795580903e79acb33fb44f5c9ce7b0f
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Mon Oct 5 10:48:17 2009 -0300
>
>     V4L/DVB (13238): v4l2_subdev: rename tuner s_standby operation to
> core s_power
>    
>     Upcoming I2C v4l2_subdev drivers need a way to control the subdevice
>     power state from the core. This use case is already partially covered by
>     the tuner s_standby operation, but no way to explicitly come back from
>     the standby state is available.
>    
>     Rename the tuner s_standby operation to core s_power, and fix tuner
>     drivers accordingly. The tuner core will call s_power(0) instead of
>     s_standby(). No explicit call to s_power(1) is required for tuners as
>     they are supposed to wake up from standby automatically.
>    
>     [mchehab@redhat.com: CodingStyle fix]
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
>
>
> So at least tuners are supposed to wake up automatically.
> Yet another reason why these warnings about unbalanced s_power should be
> removed.
> The problem is, that since this commit ALL subdevices (supporting it)
> are put into standby mode, not only the tuners.
Hmm... wait.
What's s_power supposed to do ? Put the device into stand-by mode or
switch power off ?
Are we talking about subdevice register operations or can it also call
power callbacks in the parent driver (like soc_camera does) ?

Regards,
Frank

> Hopefully, this didn't cause any regressions.
>
> Regards,
> Frank
>
>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/


^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-09-09 17:30 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-28 13:28 [PATCH 0/3] V4L2: fix em28xx ov2640 support Guennadi Liakhovetski
2013-08-28 13:28 ` [PATCH 1/3] V4L2: add v4l2-clock helpers to register and unregister a fixed-rate clock Guennadi Liakhovetski
2013-08-28 13:33   ` Laurent Pinchart
2013-08-28 14:49     ` Guennadi Liakhovetski
2013-08-28 13:28 ` [PATCH 2/3] V4L2: add a v4l2-clk helper macro to produce an I2C device ID Guennadi Liakhovetski
2013-08-28 13:35   ` Laurent Pinchart
2013-08-28 13:42     ` Guennadi Liakhovetski
2013-08-28 13:28 ` [PATCH 3/3] V4L2: em28xx: register a V4L2 clock source Guennadi Liakhovetski
2013-08-28 21:54   ` Sylwester Nawrocki
2013-09-02 18:40 ` [PATCH 0/3] V4L2: fix em28xx ov2640 support Frank Schäfer
2013-09-03  6:34   ` Guennadi Liakhovetski
2013-09-05 13:32     ` Guennadi Liakhovetski
2013-09-05 15:22     ` Frank Schäfer
2013-09-05 15:41       ` Mauro Carvalho Chehab
2013-09-05 15:57         ` Guennadi Liakhovetski
2013-09-09 17:27           ` Frank Schäfer
2013-09-09 17:30             ` Frank Schäfer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.