* [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-11 3:40 ` Barry Song
2013-04-08 15:05 ` [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration Guennadi Liakhovetski
` (7 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Typical video devices like camera sensors require an external clock source.
Many such devices cannot even access their hardware registers without a
running clock. These clock sources should be controlled by their consumers.
This should be performed, using the generic clock framework. Unfortunately
so far only very few systems have been ported to that framework. This patch
adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
Platforms, adopting the clock API, should switch to using it. Eventually
this temporary API should be removed.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
v8: Updated both (C) dates
drivers/media/v4l2-core/Makefile | 2 +-
drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
include/media/v4l2-clk.h | 54 +++++++++++
3 files changed, 232 insertions(+), 1 deletions(-)
create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
create mode 100644 include/media/v4l2-clk.h
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index aa50c46..628c630 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -5,7 +5,7 @@
tuner-objs := tuner-core.o
videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
- v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
+ v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
ifeq ($(CONFIG_COMPAT),y)
videodev-objs += v4l2-compat-ioctl32.o
endif
diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
new file mode 100644
index 0000000..d7cc13e
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -0,0 +1,177 @@
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/atomic.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include <media/v4l2-clk.h>
+#include <media/v4l2-subdev.h>
+
+static DEFINE_MUTEX(clk_lock);
+static LIST_HEAD(clk_list);
+
+static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
+{
+ struct v4l2_clk *clk;
+
+ list_for_each_entry(clk, &clk_list, list) {
+ if (strcmp(dev_id, clk->dev_id))
+ continue;
+
+ if (!id || !clk->id || !strcmp(clk->id, id))
+ return clk;
+ }
+
+ return ERR_PTR(-ENODEV);
+}
+
+struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
+{
+ struct v4l2_clk *clk;
+
+ mutex_lock(&clk_lock);
+ clk = v4l2_clk_find(dev_name(dev), id);
+
+ if (!IS_ERR(clk) && !try_module_get(clk->ops->owner))
+ clk = ERR_PTR(-ENODEV);
+ mutex_unlock(&clk_lock);
+
+ if (!IS_ERR(clk))
+ atomic_inc(&clk->use_count);
+
+ return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_get);
+
+void v4l2_clk_put(struct v4l2_clk *clk)
+{
+ if (!IS_ERR(clk)) {
+ atomic_dec(&clk->use_count);
+ module_put(clk->ops->owner);
+ }
+}
+EXPORT_SYMBOL(v4l2_clk_put);
+
+int v4l2_clk_enable(struct v4l2_clk *clk)
+{
+ int ret;
+ mutex_lock(&clk->lock);
+ if (++clk->enable = 1 && clk->ops->enable) {
+ ret = clk->ops->enable(clk);
+ if (ret < 0)
+ clk->enable--;
+ } else {
+ ret = 0;
+ }
+ mutex_unlock(&clk->lock);
+ return ret;
+}
+EXPORT_SYMBOL(v4l2_clk_enable);
+
+void v4l2_clk_disable(struct v4l2_clk *clk)
+{
+ int enable;
+
+ mutex_lock(&clk->lock);
+ enable = --clk->enable;
+ if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
+ clk->dev_id, clk->id))
+ clk->enable++;
+ else if (!enable && clk->ops->disable)
+ clk->ops->disable(clk);
+ mutex_unlock(&clk->lock);
+}
+EXPORT_SYMBOL(v4l2_clk_disable);
+
+unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
+{
+ if (!clk->ops->get_rate)
+ return -ENOSYS;
+
+ return clk->ops->get_rate(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_get_rate);
+
+int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
+{
+ if (!clk->ops->set_rate)
+ return -ENOSYS;
+
+ return clk->ops->set_rate(clk, rate);
+}
+EXPORT_SYMBOL(v4l2_clk_set_rate);
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+ const char *dev_id,
+ const char *id, void *priv)
+{
+ struct v4l2_clk *clk;
+ int ret;
+
+ if (!ops || !dev_id)
+ return ERR_PTR(-EINVAL);
+
+ clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+ if (!clk)
+ return ERR_PTR(-ENOMEM);
+
+ clk->id = kstrdup(id, GFP_KERNEL);
+ clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
+ if ((id && !clk->id) || !clk->dev_id) {
+ ret = -ENOMEM;
+ goto ealloc;
+ }
+ clk->ops = ops;
+ clk->priv = priv;
+ atomic_set(&clk->use_count, 0);
+ mutex_init(&clk->lock);
+
+ mutex_lock(&clk_lock);
+ if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
+ mutex_unlock(&clk_lock);
+ ret = -EEXIST;
+ goto eexist;
+ }
+ list_add_tail(&clk->list, &clk_list);
+ mutex_unlock(&clk_lock);
+
+ return clk;
+
+eexist:
+ealloc:
+ kfree(clk->id);
+ kfree(clk->dev_id);
+ kfree(clk);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(v4l2_clk_register);
+
+void v4l2_clk_unregister(struct v4l2_clk *clk)
+{
+ if (WARN(atomic_read(&clk->use_count),
+ "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
+ __func__, clk->dev_id, clk->id))
+ return;
+
+ mutex_lock(&clk_lock);
+ list_del(&clk->list);
+ mutex_unlock(&clk_lock);
+
+ kfree(clk->id);
+ kfree(clk->dev_id);
+ kfree(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_unregister);
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
new file mode 100644
index 0000000..89efbd7
--- /dev/null
+++ b/include/media/v4l2-clk.h
@@ -0,0 +1,54 @@
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * ATTENTION: This is a temporary API and it shall be replaced by the generic
+ * clock API, when the latter becomes widely available.
+ */
+
+#ifndef MEDIA_V4L2_CLK_H
+#define MEDIA_V4L2_CLK_H
+
+#include <linux/atomic.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+struct module;
+struct device;
+
+struct v4l2_clk {
+ struct list_head list;
+ const struct v4l2_clk_ops *ops;
+ const char *dev_id;
+ const char *id;
+ int enable;
+ struct mutex lock; /* Protect the enable count */
+ atomic_t use_count;
+ void *priv;
+};
+
+struct v4l2_clk_ops {
+ struct module *owner;
+ int (*enable)(struct v4l2_clk *clk);
+ void (*disable)(struct v4l2_clk *clk);
+ unsigned long (*get_rate)(struct v4l2_clk *clk);
+ int (*set_rate)(struct v4l2_clk *clk, unsigned long);
+};
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+ const char *dev_name,
+ const char *name, void *priv);
+void v4l2_clk_unregister(struct v4l2_clk *clk);
+struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
+void v4l2_clk_put(struct v4l2_clk *clk);
+int v4l2_clk_enable(struct v4l2_clk *clk);
+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);
+
+#endif
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-08 15:05 ` [PATCH v8 1/7] media: V4L2: add temporary clock helpers Guennadi Liakhovetski
@ 2013-04-11 3:40 ` Barry Song
2013-04-11 7:22 ` Guennadi Liakhovetski
0 siblings, 1 reply; 27+ messages in thread
From: Barry Song @ 2013-04-11 3:40 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Sylwester Nawrocki, Mark Brown, linux-media, linux-sh,
Magnus Damm, Hans Verkuil, Laurent Pinchart, renwei.wu,
DL-SHA-WorkGroupLinux, xiaomeng.hou, zilong.wu
Hi Guennadi,
> Typical video devices like camera sensors require an external clock source.
> Many such devices cannot even access their hardware registers without a
> running clock. These clock sources should be controlled by their consumers.
> This should be performed, using the generic clock framework. Unfortunately
> so far only very few systems have been ported to that framework. This patch
> adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> Platforms, adopting the clock API, should switch to using it. Eventually
> this temporary API should be removed.
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> ---
for your patch 1/8 and 3/8, i think it makes a lot of senses to let
the object manages its own clock by itself.
is it possible for us to implement v4l2-clk.c directly as an instance
of standard clk driver for those systems which don't have generic
clock, and remove the V4L2 clock APIs like v4l2_clk_get,
v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
and fake clock controller driver. finally, after people have
generically clk, remove it.
> v8: Updated both (C) dates
> drivers/media/v4l2-core/Makefile | 2 +-
> drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
> include/media/v4l2-clk.h | 54 +++++++++++
> 3 files changed, 232 insertions(+), 1 deletions(-)
> create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> create mode 100644 include/media/v4l2-clk.h
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index aa50c46..628c630 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -5,7 +5,7 @@
> tuner-objs := tuner-core.o
> videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> ifeq ($(CONFIG_COMPAT),y)
> videodev-objs += v4l2-compat-ioctl32.o
> endif
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> new file mode 100644
> index 0000000..d7cc13e
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,177 @@
-barry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 3:40 ` Barry Song
@ 2013-04-11 7:22 ` Guennadi Liakhovetski
2013-04-11 8:22 ` Barry Song
0 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-11 7:22 UTC (permalink / raw)
To: Barry Song
Cc: Sylwester Nawrocki, Mark Brown, linux-media, linux-sh,
Magnus Damm, Hans Verkuil, Laurent Pinchart, renwei.wu,
DL-SHA-WorkGroupLinux, xiaomeng.hou, zilong.wu
Hi Barry
On Thu, 11 Apr 2013, Barry Song wrote:
> Hi Guennadi,
>
> > Typical video devices like camera sensors require an external clock source.
> > Many such devices cannot even access their hardware registers without a
> > running clock. These clock sources should be controlled by their consumers.
> > This should be performed, using the generic clock framework. Unfortunately
> > so far only very few systems have been ported to that framework. This patch
> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> > Platforms, adopting the clock API, should switch to using it. Eventually
> > this temporary API should be removed.
>
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > ---
>
> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> the object manages its own clock by itself.
> is it possible for us to implement v4l2-clk.c directly as an instance
> of standard clk driver for those systems which don't have generic
> clock, and remove the V4L2 clock APIs like v4l2_clk_get,
> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> and fake clock controller driver. finally, after people have
> generically clk, remove it.
I don't think you can force-enable the CFF on systems, that don't support
it, e.g. PXA.
Thanks
Guennadi
> > v8: Updated both (C) dates
>
> > drivers/media/v4l2-core/Makefile | 2 +-
> > drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
> > include/media/v4l2-clk.h | 54 +++++++++++
> > 3 files changed, 232 insertions(+), 1 deletions(-)
> > create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> > create mode 100644 include/media/v4l2-clk.h
>
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index aa50c46..628c630 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -5,7 +5,7 @@
> > tuner-objs := tuner-core.o
>
> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> > - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> > + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> > ifeq ($(CONFIG_COMPAT),y)
> > videodev-objs += v4l2-compat-ioctl32.o
> > endif
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > new file mode 100644
> > index 0000000..d7cc13e
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -0,0 +1,177 @@
>
> -barry
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 7:22 ` Guennadi Liakhovetski
@ 2013-04-11 8:22 ` Barry Song
2013-04-11 8:36 ` Guennadi Liakhovetski
0 siblings, 1 reply; 27+ messages in thread
From: Barry Song @ 2013-04-11 8:22 UTC (permalink / raw)
To: linux-arm-kernel
2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Barry
>
> On Thu, 11 Apr 2013, Barry Song wrote:
>
>> Hi Guennadi,
>>
>> > Typical video devices like camera sensors require an external clock source.
>> > Many such devices cannot even access their hardware registers without a
>> > running clock. These clock sources should be controlled by their consumers.
>> > This should be performed, using the generic clock framework. Unfortunately
>> > so far only very few systems have been ported to that framework. This patch
>> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
>> > Platforms, adopting the clock API, should switch to using it. Eventually
>> > this temporary API should be removed.
>>
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>> > ---
>>
>> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
>> the object manages its own clock by itself.
>> is it possible for us to implement v4l2-clk.c directly as an instance
>> of standard clk driver for those systems which don't have generic
>> clock, and remove the V4L2 clock APIs like v4l2_clk_get,
>> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
>> and fake clock controller driver. finally, after people have
>> generically clk, remove it.
>
> I don't think you can force-enable the CFF on systems, that don't support
> it, e.g. PXA.
yes. we can. clock is only a framework, has it any limitation to
implement a driver instance on any platform?
people have tried to move to common clk and generic framework for a
long time, now you still try to provide a v4l2 specific clock APIs, it
just makes v4l2 unacceptable and much complex.
>
> Thanks
> Guennadi
>
>> > v8: Updated both (C) dates
>>
>> > drivers/media/v4l2-core/Makefile | 2 +-
>> > drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
>> > include/media/v4l2-clk.h | 54 +++++++++++
>> > 3 files changed, 232 insertions(+), 1 deletions(-)
>> > create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>> > create mode 100644 include/media/v4l2-clk.h
>>
>> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> > index aa50c46..628c630 100644
>> > --- a/drivers/media/v4l2-core/Makefile
>> > +++ b/drivers/media/v4l2-core/Makefile
>> > @@ -5,7 +5,7 @@
>> > tuner-objs := tuner-core.o
>>
>> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> > - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
>> > + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>> > ifeq ($(CONFIG_COMPAT),y)
>> > videodev-objs += v4l2-compat-ioctl32.o
>> > endif
>> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
>> > new file mode 100644
>> > index 0000000..d7cc13e
>> > --- /dev/null
>> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
>> > @@ -0,0 +1,177 @@
>>
>> -barry
-barry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 8:22 ` Barry Song
@ 2013-04-11 8:36 ` Guennadi Liakhovetski
2013-04-11 8:59 ` Barry Song
0 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-11 8:36 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 11 Apr 2013, Barry Song wrote:
> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > Hi Barry
> >
> > On Thu, 11 Apr 2013, Barry Song wrote:
> >
> >> Hi Guennadi,
> >>
> >> > Typical video devices like camera sensors require an external clock source.
> >> > Many such devices cannot even access their hardware registers without a
> >> > running clock. These clock sources should be controlled by their consumers.
> >> > This should be performed, using the generic clock framework. Unfortunately
> >> > so far only very few systems have been ported to that framework. This patch
> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> >> > Platforms, adopting the clock API, should switch to using it. Eventually
> >> > this temporary API should be removed.
> >>
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> > ---
> >>
> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> >> the object manages its own clock by itself.
> >> is it possible for us to implement v4l2-clk.c directly as an instance
> >> of standard clk driver for those systems which don't have generic
> >> clock, and remove the V4L2 clock APIs like v4l2_clk_get,
> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> >> and fake clock controller driver. finally, after people have
> >> generically clk, remove it.
> >
> > I don't think you can force-enable the CFF on systems, that don't support
> > it, e.g. PXA.
>
> yes. we can. clock is only a framework, has it any limitation to
> implement a driver instance on any platform?
So, you enable CFF, it provides its own clk_* implementation like
clk_get_rate() etc. Now, PXA already has it defined in
arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
Thanks
Guennadi
> people have tried to move to common clk and generic framework for a
> long time, now you still try to provide a v4l2 specific clock APIs, it
> just makes v4l2 unacceptable and much complex.
>
> >
> > Thanks
> > Guennadi
> >
> >> > v8: Updated both (C) dates
> >>
> >> > drivers/media/v4l2-core/Makefile | 2 +-
> >> > drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
> >> > include/media/v4l2-clk.h | 54 +++++++++++
> >> > 3 files changed, 232 insertions(+), 1 deletions(-)
> >> > create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >> > create mode 100644 include/media/v4l2-clk.h
> >>
> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> >> > index aa50c46..628c630 100644
> >> > --- a/drivers/media/v4l2-core/Makefile
> >> > +++ b/drivers/media/v4l2-core/Makefile
> >> > @@ -5,7 +5,7 @@
> >> > tuner-objs := tuner-core.o
> >>
> >> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> >> > - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> >> > + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> >> > ifeq ($(CONFIG_COMPAT),y)
> >> > videodev-objs += v4l2-compat-ioctl32.o
> >> > endif
> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> >> > new file mode 100644
> >> > index 0000000..d7cc13e
> >> > --- /dev/null
> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> >> > @@ -0,0 +1,177 @@
> >>
> >> -barry
>
> -barry
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 8:36 ` Guennadi Liakhovetski
@ 2013-04-11 8:59 ` Barry Song
2013-04-11 18:52 ` Mike Turquette
0 siblings, 1 reply; 27+ messages in thread
From: Barry Song @ 2013-04-11 8:59 UTC (permalink / raw)
To: linux-arm-kernel
2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Thu, 11 Apr 2013, Barry Song wrote:
>
>> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> > Hi Barry
>> >
>> > On Thu, 11 Apr 2013, Barry Song wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> > Typical video devices like camera sensors require an external clock source.
>> >> > Many such devices cannot even access their hardware registers without a
>> >> > running clock. These clock sources should be controlled by their consumers.
>> >> > This should be performed, using the generic clock framework. Unfortunately
>> >> > so far only very few systems have been ported to that framework. This patch
>> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
>> >> > Platforms, adopting the clock API, should switch to using it. Eventually
>> >> > this temporary API should be removed.
>> >>
>> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>> >> > ---
>> >>
>> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
>> >> the object manages its own clock by itself.
>> >> is it possible for us to implement v4l2-clk.c directly as an instance
>> >> of standard clk driver for those systems which don't have generic
>> >> clock, and remove the V4L2 clock APIs like v4l2_clk_get,
>> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
>> >> and fake clock controller driver. finally, after people have
>> >> generically clk, remove it.
>> >
>> > I don't think you can force-enable the CFF on systems, that don't support
>> > it, e.g. PXA.
>>
>> yes. we can. clock is only a framework, has it any limitation to
>> implement a driver instance on any platform?
>
> So, you enable CFF, it provides its own clk_* implementation like
> clk_get_rate() etc. Now, PXA already has it defined in
> arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
agree.
>
> Thanks
> Guennadi
>
>> people have tried to move to common clk and generic framework for a
>> long time, now you still try to provide a v4l2 specific clock APIs, it
>> just makes v4l2 unacceptable and much complex.
>>
>> >
>> > Thanks
>> > Guennadi
>> >
>> >> > v8: Updated both (C) dates
>> >>
>> >> > drivers/media/v4l2-core/Makefile | 2 +-
>> >> > drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
>> >> > include/media/v4l2-clk.h | 54 +++++++++++
>> >> > 3 files changed, 232 insertions(+), 1 deletions(-)
>> >> > create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>> >> > create mode 100644 include/media/v4l2-clk.h
>> >>
>> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> >> > index aa50c46..628c630 100644
>> >> > --- a/drivers/media/v4l2-core/Makefile
>> >> > +++ b/drivers/media/v4l2-core/Makefile
>> >> > @@ -5,7 +5,7 @@
>> >> > tuner-objs := tuner-core.o
>> >>
>> >> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> >> > - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
>> >> > + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>> >> > ifeq ($(CONFIG_COMPAT),y)
>> >> > videodev-objs += v4l2-compat-ioctl32.o
>> >> > endif
>> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
>> >> > new file mode 100644
>> >> > index 0000000..d7cc13e
>> >> > --- /dev/null
>> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
>> >> > @@ -0,0 +1,177 @@
>> >>
>> >> -barry
-barry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 8:59 ` Barry Song
@ 2013-04-11 18:52 ` Mike Turquette
2013-04-11 20:14 ` Sylwester Nawrocki
2013-04-11 22:35 ` Laurent Pinchart
0 siblings, 2 replies; 27+ messages in thread
From: Mike Turquette @ 2013-04-11 18:52 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Barry Song (2013-04-11 01:59:28)
> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Thu, 11 Apr 2013, Barry Song wrote:
> >
> >> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> >> > Hi Barry
> >> >
> >> > On Thu, 11 Apr 2013, Barry Song wrote:
> >> >
> >> >> Hi Guennadi,
> >> >>
> >> >> > Typical video devices like camera sensors require an external clock source.
> >> >> > Many such devices cannot even access their hardware registers without a
> >> >> > running clock. These clock sources should be controlled by their consumers.
> >> >> > This should be performed, using the generic clock framework. Unfortunately
> >> >> > so far only very few systems have been ported to that framework. This patch
> >> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> >> >> > Platforms, adopting the clock API, should switch to using it. Eventually
> >> >> > this temporary API should be removed.
> >> >>
> >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> >> > ---
> >> >>
> >> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> >> >> the object manages its own clock by itself.
> >> >> is it possible for us to implement v4l2-clk.c directly as an instance
> >> >> of standard clk driver for those systems which don't have generic
> >> >> clock, and remove the V4L2 clock APIs like v4l2_clk_get,
> >> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> >> >> and fake clock controller driver. finally, after people have
> >> >> generically clk, remove it.
> >> >
> >> > I don't think you can force-enable the CFF on systems, that don't support
> >> > it, e.g. PXA.
> >>
> >> yes. we can. clock is only a framework, has it any limitation to
> >> implement a driver instance on any platform?
> >
> > So, you enable CFF, it provides its own clk_* implementation like
> > clk_get_rate() etc. Now, PXA already has it defined in
> > arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
>
> agree.
>
Hi,
I came into this thread late and don't have the actual patches in my
inbox for review. That said, I don't understand why V4L2 cares about
the clk framework *implementation*? The clk.h api is the same for
platforms using the common struct clk and those still using the legacy
method of defining their own struct clk. If drivers are only consumers
of the clk.h api then the implementation underneath should not matter.
Regards,
Mike
> >
> > Thanks
> > Guennadi
> >
> >> people have tried to move to common clk and generic framework for a
> >> long time, now you still try to provide a v4l2 specific clock APIs, it
> >> just makes v4l2 unacceptable and much complex.
> >>
> >> >
> >> > Thanks
> >> > Guennadi
> >> >
> >> >> > v8: Updated both (C) dates
> >> >>
> >> >> > drivers/media/v4l2-core/Makefile | 2 +-
> >> >> > drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++++++++++++++++++++++++++++++++
> >> >> > include/media/v4l2-clk.h | 54 +++++++++++
> >> >> > 3 files changed, 232 insertions(+), 1 deletions(-)
> >> >> > create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >> >> > create mode 100644 include/media/v4l2-clk.h
> >> >>
> >> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> >> >> > index aa50c46..628c630 100644
> >> >> > --- a/drivers/media/v4l2-core/Makefile
> >> >> > +++ b/drivers/media/v4l2-core/Makefile
> >> >> > @@ -5,7 +5,7 @@
> >> >> > tuner-objs := tuner-core.o
> >> >>
> >> >> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> >> >> > - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> >> >> > + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> >> >> > ifeq ($(CONFIG_COMPAT),y)
> >> >> > videodev-objs += v4l2-compat-ioctl32.o
> >> >> > endif
> >> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> >> >> > new file mode 100644
> >> >> > index 0000000..d7cc13e
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> >> >> > @@ -0,0 +1,177 @@
> >> >>
> >> >> -barry
>
> -barry
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 18:52 ` Mike Turquette
@ 2013-04-11 20:14 ` Sylwester Nawrocki
2013-04-11 22:35 ` Laurent Pinchart
1 sibling, 0 replies; 27+ messages in thread
From: Sylwester Nawrocki @ 2013-04-11 20:14 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 04/11/2013 08:52 PM, Mike Turquette wrote:
[...]
>>> So, you enable CFF, it provides its own clk_* implementation like
>>> clk_get_rate() etc. Now, PXA already has it defined in
>>> arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
>>
>> agree.
>
> Hi,
>
> I came into this thread late and don't have the actual patches in my
> inbox for review. That said, I don't understand why V4L2 cares about
> the clk framework *implementation*? The clk.h api is the same for
> platforms using the common struct clk and those still using the legacy
> method of defining their own struct clk. If drivers are only consumers
> of the clk.h api then the implementation underneath should not matter.
I came to similar conclusions previously, but in case when one of the two
drivers is the clock provider I think there is still an issue there.
The drivers are supposed to be platform agnostic, but the clock provider
would have to include mach specific declarations of struct clk, wouldn't
it ?
Regards,
Sylwester
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers
2013-04-11 18:52 ` Mike Turquette
2013-04-11 20:14 ` Sylwester Nawrocki
@ 2013-04-11 22:35 ` Laurent Pinchart
[not found] ` <20130411231923.7915.17215@quantum>
1 sibling, 1 reply; 27+ messages in thread
From: Laurent Pinchart @ 2013-04-11 22:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mike,
On Thursday 11 April 2013 11:52:58 Mike Turquette wrote:
> Quoting Barry Song (2013-04-11 01:59:28)
> > 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > > On Thu, 11 Apr 2013, Barry Song wrote:
> > >> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > >> > On Thu, 11 Apr 2013, Barry Song wrote:
> > >> >> Hi Guennadi,
> > >> >>
> > >> >> > Typical video devices like camera sensors require an external
> > >> >> > clock source. Many such devices cannot even access their hardware
> > >> >> > registers without a running clock. These clock sources should be
> > >> >> > controlled by their consumers. This should be performed, using the
> > >> >> > generic clock framework. Unfortunately so far only very few
> > >> >> > systems have been ported to that framework. This patch adds a set
> > >> >> > of temporary helpers, mimicking the generic clock API, to V4L2.
> > >> >> > Platforms, adopting the clock API, should switch to using it.
> > >> >> > Eventually this temporary API should be removed.
> > >> >> >
> > >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > >> >> > ---
> > >> >>
> > >> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> > >> >> the object manages its own clock by itself.
> > >> >> is it possible for us to implement v4l2-clk.c directly as an
> > >> >> instance of standard clk driver for those systems which don't have
> > >> >> generic clock, and remove the V4L2 clock APIs like v4l2_clk_get,
> > >> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> > >> >> and fake clock controller driver. finally, after people have
> > >> >> generically clk, remove it.
> > >> >
> > >> > I don't think you can force-enable the CFF on systems, that don't
> > >> > support it, e.g. PXA.
> > >>
> > >> yes. we can. clock is only a framework, has it any limitation to
> > >> implement a driver instance on any platform?
> > >
> > > So, you enable CFF, it provides its own clk_* implementation like
> > > clk_get_rate() etc. Now, PXA already has it defined in
> > > arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
> >
> > agree.
>
> I came into this thread late and don't have the actual patches in my inbox
> for review. That said, I don't understand why V4L2 cares about the clk
> framework *implementation*? The clk.h api is the same for platforms using
> the common struct clk and those still using the legacy method of defining
> their own struct clk. If drivers are only consumers of the clk.h api then
> the implementation underneath should not matter.
The issue on non-CCF systems is that devices usually can't register clocks
dynamically. (Most of) those systems provide system clocks only through their
clock API, without a way for the camera IP core to hook up the clock(s) it can
provide to the camera sensor. On the consumer side we don't care much about
the clock framework implementation, but on the provider side we need a
framework that allows registering non-system clocks at runtime.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 1/7] media: V4L2: add temporary clock helpers Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-12 12:09 ` Laurent Pinchart
2013-04-08 15:05 ` [PATCH v8 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk Guennadi Liakhovetski
` (6 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Currently bridge device drivers register devices for all subdevices
synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor
is attached to a video bridge device, the bridge driver will create an I2C
device and wait for the respective I2C driver to probe. This makes linking
of devices straight forward, but this approach cannot be used with
intrinsically asynchronous and unordered device registration systems like
the Flattened Device Tree. To support such systems this patch adds an
asynchronous subdevice registration framework to V4L2. To use it respective
(e.g. I2C) subdevice drivers must register themselves with the framework.
A bridge driver on the other hand must register notification callbacks,
that will be called upon various related events.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
v8: Really renamed V4L2_ASYNC_BUS_SPECIAL to V4L2_ASYNC_BUS_CUSTOM
drivers/media/v4l2-core/Makefile | 3 +-
drivers/media/v4l2-core/v4l2-async.c | 262 ++++++++++++++++++++++++++++++++++
include/media/v4l2-async.h | 104 ++++++++++++++
3 files changed, 368 insertions(+), 1 deletions(-)
create mode 100644 drivers/media/v4l2-core/v4l2-async.c
create mode 100644 include/media/v4l2-async.h
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 628c630..4c33b8d6 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -5,7 +5,8 @@
tuner-objs := tuner-core.o
videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
- v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
+ v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
+ v4l2-async.o
ifeq ($(CONFIG_COMPAT),y)
videodev-objs += v4l2-compat-ioctl32.o
endif
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
new file mode 100644
index 0000000..4cc56ad
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -0,0 +1,262 @@
+/*
+ * V4L2 asynchronous subdevice registration API
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <media/v4l2-async.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+static bool match_i2c(struct device *dev, struct v4l2_async_hw_device *hw_dev)
+{
+ struct i2c_client *client = i2c_verify_client(dev);
+ return client &&
+ hw_dev->bus_type = V4L2_ASYNC_BUS_I2C &&
+ hw_dev->match.i2c.adapter_id = client->adapter->nr &&
+ hw_dev->match.i2c.address = client->addr;
+}
+
+static bool match_platform(struct device *dev, struct v4l2_async_hw_device *hw_dev)
+{
+ return hw_dev->bus_type = V4L2_ASYNC_BUS_PLATFORM &&
+ !strcmp(hw_dev->match.platform.name, dev_name(dev));
+}
+
+static LIST_HEAD(subdev_list);
+static LIST_HEAD(notifier_list);
+static DEFINE_MUTEX(list_lock);
+
+static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl)
+{
+ struct v4l2_async_subdev *asd = NULL;
+ bool (*match)(struct device *,
+ struct v4l2_async_hw_device *);
+
+ list_for_each_entry (asd, ¬ifier->waiting, list) {
+ struct v4l2_async_hw_device *hw = &asd->hw;
+ switch (hw->bus_type) {
+ case V4L2_ASYNC_BUS_CUSTOM:
+ match = hw->match.special.match;
+ if (!match)
+ /* Match always */
+ return asd;
+ break;
+ case V4L2_ASYNC_BUS_PLATFORM:
+ match = match_platform;
+ break;
+ case V4L2_ASYNC_BUS_I2C:
+ match = match_i2c;
+ break;
+ default:
+ /* Oops */
+ match = NULL;
+ dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL,
+ "Invalid bus-type %u on %p\n", hw->bus_type, asd);
+ }
+
+ if (match && match(asdl->dev, hw))
+ break;
+ }
+
+ return asd;
+}
+
+static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl,
+ struct v4l2_async_subdev *asd)
+{
+ int ret;
+
+ /* Remove from the waiting list */
+ list_del(&asd->list);
+ asdl->asd = asd;
+ asdl->notifier = notifier;
+
+ if (notifier->bound) {
+ ret = notifier->bound(notifier, asdl);
+ if (ret < 0)
+ return ret;
+ }
+ /* Move from the global subdevice list to notifier's done */
+ list_move(&asdl->list, ¬ifier->done);
+
+ ret = v4l2_device_register_subdev(notifier->v4l2_dev,
+ asdl->subdev);
+ if (ret < 0) {
+ if (notifier->unbind)
+ notifier->unbind(notifier, asdl);
+ return ret;
+ }
+
+ if (list_empty(¬ifier->waiting) && notifier->complete)
+ return notifier->complete(notifier);
+
+ return 0;
+}
+
+static void v4l2_async_cleanup(struct v4l2_async_subdev_list *asdl)
+{
+ v4l2_device_unregister_subdev(asdl->subdev);
+ /* Subdevice driver will reprobe and put asdl back onto the list */
+ list_del_init(&asdl->list);
+ asdl->asd = NULL;
+ asdl->dev = NULL;
+}
+
+static struct device *v4l2_async_unregister(struct v4l2_async_subdev_list *asdl)
+{
+ struct device *dev = asdl->dev;
+
+ v4l2_async_cleanup(asdl);
+
+ /* If we handled USB devices, we'd have to lock the parent too */
+ device_release_driver(dev);
+ return dev;
+}
+
+int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
+ struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_async_subdev_list *asdl, *tmp;
+ int i;
+
+ notifier->v4l2_dev = v4l2_dev;
+ INIT_LIST_HEAD(¬ifier->waiting);
+ INIT_LIST_HEAD(¬ifier->done);
+
+ for (i = 0; i < notifier->subdev_num; i++)
+ list_add_tail(¬ifier->subdev[i]->list, ¬ifier->waiting);
+
+ mutex_lock(&list_lock);
+
+ /* Keep also completed notifiers on the list */
+ list_add(¬ifier->list, ¬ifier_list);
+
+ list_for_each_entry_safe(asdl, tmp, &subdev_list, list) {
+ struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl);
+ int ret;
+
+ if (!asd)
+ continue;
+
+ ret = v4l2_async_test_notify(notifier, asdl, asd);
+ if (ret < 0) {
+ mutex_unlock(&list_lock);
+ return ret;
+ }
+ }
+
+ mutex_unlock(&list_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(v4l2_async_notifier_register);
+
+void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_async_subdev_list *asdl, *tmp;
+ int i = 0;
+ struct device **dev = kcalloc(notifier->subdev_num,
+ sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ dev_err(notifier->v4l2_dev->dev,
+ "Failed to allocate device cache!\n");
+
+ mutex_lock(&list_lock);
+
+ list_del(¬ifier->list);
+
+ list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) {
+ if (dev)
+ dev[i++] = get_device(asdl->dev);
+ v4l2_async_unregister(asdl);
+
+ if (notifier->unbind)
+ notifier->unbind(notifier, asdl);
+ }
+
+ mutex_unlock(&list_lock);
+
+ if (dev) {
+ while (i--) {
+ if (dev[i] && device_attach(dev[i]) < 0)
+ dev_err(dev[i], "Failed to re-probe to %s\n",
+ dev[i]->driver ? dev[i]->driver->name : "(none)");
+ put_device(dev[i]);
+ }
+ kfree(dev);
+ }
+ /*
+ * Don't care about the waiting list, it is initialised and populated
+ * upon notifier registration.
+ */
+}
+EXPORT_SYMBOL(v4l2_async_notifier_unregister);
+
+int v4l2_async_register_subdev(struct v4l2_async_subdev_list *asdl)
+{
+ struct v4l2_async_notifier *notifier;
+
+ mutex_lock(&list_lock);
+
+ INIT_LIST_HEAD(&asdl->list);
+
+ list_for_each_entry(notifier, ¬ifier_list, list) {
+ struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl);
+ if (asd) {
+ int ret = v4l2_async_test_notify(notifier, asdl, asd);
+ mutex_unlock(&list_lock);
+ return ret;
+ }
+ }
+
+ /* None matched, wait for hot-plugging */
+ list_add(&asdl->list, &subdev_list);
+
+ mutex_unlock(&list_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(v4l2_async_register_subdev);
+
+void v4l2_async_unregister_subdev(struct v4l2_async_subdev_list *asdl)
+{
+ struct v4l2_async_notifier *notifier = asdl->notifier;
+ struct device *dev;
+
+ if (!asdl->asd) {
+ if (!list_empty(&asdl->list))
+ v4l2_async_cleanup(asdl);
+ return;
+ }
+
+ mutex_lock(&list_lock);
+
+ dev = asdl->dev;
+
+ list_add(&asdl->asd->list, ¬ifier->waiting);
+
+ v4l2_async_cleanup(asdl);
+
+ if (notifier->unbind)
+ notifier->unbind(notifier, asdl);
+
+ mutex_unlock(&list_lock);
+}
+EXPORT_SYMBOL(v4l2_async_unregister_subdev);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
new file mode 100644
index 0000000..c0470c6
--- /dev/null
+++ b/include/media/v4l2-async.h
@@ -0,0 +1,104 @@
+/*
+ * V4L2 asynchronous subdevice registration API
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef V4L2_ASYNC_H
+#define V4L2_ASYNC_H
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+#include <media/v4l2-subdev.h>
+
+struct device;
+struct v4l2_device;
+struct v4l2_async_notifier;
+
+enum v4l2_async_bus_type {
+ V4L2_ASYNC_BUS_CUSTOM,
+ V4L2_ASYNC_BUS_PLATFORM,
+ V4L2_ASYNC_BUS_I2C,
+};
+
+struct v4l2_async_hw_device {
+ enum v4l2_async_bus_type bus_type;
+ union {
+ struct {
+ const char *name;
+ } platform;
+ struct {
+ int adapter_id;
+ unsigned short address;
+ } i2c;
+ struct {
+ bool (*match)(struct device *,
+ struct v4l2_async_hw_device *);
+ void *priv;
+ } special;
+ } match;
+};
+
+/**
+ * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
+ * @hw: this device descriptor
+ * @list: member in a list of subdevices
+ */
+struct v4l2_async_subdev {
+ struct v4l2_async_hw_device hw;
+ struct list_head list;
+};
+
+/**
+ * v4l2_async_subdev_list - provided by subdevices
+ * @list: member in a list of subdevices
+ * @dev: hardware device
+ * @subdev: V4L2 subdevice
+ * @asd: pointer to respective struct v4l2_async_subdev
+ * @notifier: pointer to managing notifier
+ */
+struct v4l2_async_subdev_list {
+ struct list_head list;
+ struct device *dev;
+ struct v4l2_subdev *subdev;
+ struct v4l2_async_subdev *asd;
+ struct v4l2_async_notifier *notifier;
+};
+
+/**
+ * v4l2_async_notifier - provided by bridges
+ * @subdev_num: number of subdevices
+ * @subdev: array of pointers to subdevices
+ * @v4l2_dev: pointer to sruct v4l2_device
+ * @waiting: list of subdevices, waiting for their drivers
+ * @done: list of subdevices, already probed
+ * @list: member in a global list of notifiers
+ * @bound: a subdevice driver has successfully probed one of subdevices
+ * @complete: all subdevices have been probed successfully
+ * @unbind: a subdevice is leaving
+ */
+struct v4l2_async_notifier {
+ int subdev_num;
+ struct v4l2_async_subdev **subdev;
+ struct v4l2_device *v4l2_dev;
+ struct list_head waiting;
+ struct list_head done;
+ struct list_head list;
+ int (*bound)(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl);
+ int (*complete)(struct v4l2_async_notifier *notifier);
+ void (*unbind)(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl);
+};
+
+int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
+ struct v4l2_async_notifier *notifier);
+void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
+int v4l2_async_register_subdev(struct v4l2_async_subdev_list *asdl);
+void v4l2_async_unregister_subdev(struct v4l2_async_subdev_list *asdl);
+#endif
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration
2013-04-08 15:05 ` [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration Guennadi Liakhovetski
@ 2013-04-12 12:09 ` Laurent Pinchart
2013-04-12 14:29 ` Guennadi Liakhovetski
0 siblings, 1 reply; 27+ messages in thread
From: Laurent Pinchart @ 2013-04-12 12:09 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Hans Verkuil, Sylwester Nawrocki, Sylwester Nawrocki,
linux-sh, Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi Guennadi,
Thanks for the patch.
On Monday 08 April 2013 17:05:33 Guennadi Liakhovetski wrote:
> Currently bridge device drivers register devices for all subdevices
> synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor
> is attached to a video bridge device, the bridge driver will create an I2C
> device and wait for the respective I2C driver to probe. This makes linking
> of devices straight forward, but this approach cannot be used with
> intrinsically asynchronous and unordered device registration systems like
> the Flattened Device Tree. To support such systems this patch adds an
> asynchronous subdevice registration framework to V4L2. To use it respective
> (e.g. I2C) subdevice drivers must register themselves with the framework.
> A bridge driver on the other hand must register notification callbacks,
> that will be called upon various related events.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> v8: Really renamed V4L2_ASYNC_BUS_SPECIAL to V4L2_ASYNC_BUS_CUSTOM
Each iteration gets better :-)
> drivers/media/v4l2-core/Makefile | 3 +-
> drivers/media/v4l2-core/v4l2-async.c | 262 +++++++++++++++++++++++++++++++
> include/media/v4l2-async.h | 104 ++++++++++++++
> 3 files changed, 368 insertions(+), 1 deletions(-)
> create mode 100644 drivers/media/v4l2-core/v4l2-async.c
> create mode 100644 include/media/v4l2-async.h
[snip]
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c new file mode 100644
> index 0000000..4cc56ad
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -0,0 +1,262 @@
> +/*
> + * V4L2 asynchronous subdevice registration API
> + *
> + * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +
> +static bool match_i2c(struct device *dev, struct v4l2_async_hw_device
> *hw_dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + return client &&
> + hw_dev->bus_type = V4L2_ASYNC_BUS_I2C &&
> + hw_dev->match.i2c.adapter_id = client->adapter->nr &&
> + hw_dev->match.i2c.address = client->addr;
> +}
> +
> +static bool match_platform(struct device *dev, struct v4l2_async_hw_device
> *hw_dev)
> +{
> + return hw_dev->bus_type = V4L2_ASYNC_BUS_PLATFORM &&
> + !strcmp(hw_dev->match.platform.name, dev_name(dev));
> +}
> +
> +static LIST_HEAD(subdev_list);
> +static LIST_HEAD(notifier_list);
> +static DEFINE_MUTEX(list_lock);
> +
> +static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl)
> +{
> + struct v4l2_async_subdev *asd = NULL;
> + bool (*match)(struct device *,
> + struct v4l2_async_hw_device *);
> +
> + list_for_each_entry (asd, ¬ifier->waiting, list) {
> + struct v4l2_async_hw_device *hw = &asd->hw;
> + switch (hw->bus_type) {
> + case V4L2_ASYNC_BUS_CUSTOM:
> + match = hw->match.special.match;
> + if (!match)
> + /* Match always */
> + return asd;
> + break;
> + case V4L2_ASYNC_BUS_PLATFORM:
> + match = match_platform;
> + break;
> + case V4L2_ASYNC_BUS_I2C:
> + match = match_i2c;
> + break;
> + default:
> + /* Oops */
> + match = NULL;
> + dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL,
> + "Invalid bus-type %u on %p\n", hw->bus_type, asd);
An invalid hw->bus_type value is a driver (or board code) bug. Could you move
this check to v4l2_async_notifier_register() when building the subdev list and
return an error ?
> + }
> +
> + if (match && match(asdl->dev, hw))
> + break;
> + }
> +
> + return asd;
> +}
> +
> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl,
> + struct v4l2_async_subdev *asd)
> +{
> + int ret;
> +
> + /* Remove from the waiting list */
> + list_del(&asd->list);
> + asdl->asd = asd;
> + asdl->notifier = notifier;
> +
> + if (notifier->bound) {
> + ret = notifier->bound(notifier, asdl);
> + if (ret < 0)
> + return ret;
> + }
> + /* Move from the global subdevice list to notifier's done */
> + list_move(&asdl->list, ¬ifier->done);
> +
> + ret = v4l2_device_register_subdev(notifier->v4l2_dev,
> + asdl->subdev);
> + if (ret < 0) {
> + if (notifier->unbind)
> + notifier->unbind(notifier, asdl);
> + return ret;
> + }
> +
> + if (list_empty(¬ifier->waiting) && notifier->complete)
> + return notifier->complete(notifier);
> +
> + return 0;
> +}
> +
> +static void v4l2_async_cleanup(struct v4l2_async_subdev_list *asdl)
> +{
> + v4l2_device_unregister_subdev(asdl->subdev);
> + /* Subdevice driver will reprobe and put asdl back onto the list */
> + list_del_init(&asdl->list);
> + asdl->asd = NULL;
> + asdl->dev = NULL;
> +}
> +
> +static struct device *v4l2_async_unregister(struct v4l2_async_subdev_list
> *asdl)
> +{
> + struct device *dev = asdl->dev;
> +
> + v4l2_async_cleanup(asdl);
> +
> + /* If we handled USB devices, we'd have to lock the parent too */
> + device_release_driver(dev);
> + return dev;
This function is called from a single location and the return value is unused,
it could just return void.
> +}
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_subdev_list *asdl, *tmp;
> + int i;
Could this be unsigned (please see below for a similar comment about notifier-
>subdev_num as well) ?
> + notifier->v4l2_dev = v4l2_dev;
> + INIT_LIST_HEAD(¬ifier->waiting);
> + INIT_LIST_HEAD(¬ifier->done);
> +
> + for (i = 0; i < notifier->subdev_num; i++)
> + list_add_tail(¬ifier->subdev[i]->list, ¬ifier->waiting);
> +
> + mutex_lock(&list_lock);
> +
> + /* Keep also completed notifiers on the list */
> + list_add(¬ifier->list, ¬ifier_list);
> +
> + list_for_each_entry_safe(asdl, tmp, &subdev_list, list) {
> + struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl);
> + int ret;
> +
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_test_notify(notifier, asdl, asd);
> + if (ret < 0) {
> + mutex_unlock(&list_lock);
> + return ret;
> + }
> + }
> +
> + mutex_unlock(&list_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_async_notifier_register);
> +
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_subdev_list *asdl, *tmp;
> + int i = 0;
i can't be negative, could it then be unsiged ?
> + struct device **dev = kcalloc(notifier->subdev_num,
> + sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + dev_err(notifier->v4l2_dev->dev,
> + "Failed to allocate device cache!\n");
> +
> + mutex_lock(&list_lock);
> +
> + list_del(¬ifier->list);
> +
> + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) {
> + if (dev)
> + dev[i++] = get_device(asdl->dev);
> + v4l2_async_unregister(asdl);
> +
> + if (notifier->unbind)
> + notifier->unbind(notifier, asdl);
> + }
> +
> + mutex_unlock(&list_lock);
> +
> + if (dev) {
> + while (i--) {
> + if (dev[i] && device_attach(dev[i]) < 0)
I'm still pretty uneasy about the device_attach() and device_release_driver()
calls, I'll read your reply to Sylwester's comments and I'll answer there.
> + dev_err(dev[i], "Failed to re-probe to %s\n",
> + dev[i]->driver ? dev[i]->driver->name : "(none)");
> + put_device(dev[i]);
> + }
> + kfree(dev);
> + }
> + /*
> + * Don't care about the waiting list, it is initialised and populated
> + * upon notifier registration.
> + */
> +}
> +EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> +
> +int v4l2_async_register_subdev(struct v4l2_async_subdev_list *asdl)
> +{
> + struct v4l2_async_notifier *notifier;
> +
> + mutex_lock(&list_lock);
> +
> + INIT_LIST_HEAD(&asdl->list);
> +
> + list_for_each_entry(notifier, ¬ifier_list, list) {
> + struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl);
> + if (asd) {
> + int ret = v4l2_async_test_notify(notifier, asdl, asd);
> + mutex_unlock(&list_lock);
> + return ret;
> + }
> + }
> +
> + /* None matched, wait for hot-plugging */
> + list_add(&asdl->list, &subdev_list);
> +
> + mutex_unlock(&list_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_async_register_subdev);
> +
> +void v4l2_async_unregister_subdev(struct v4l2_async_subdev_list *asdl)
> +{
> + struct v4l2_async_notifier *notifier = asdl->notifier;
> + struct device *dev;
> +
> + if (!asdl->asd) {
> + if (!list_empty(&asdl->list))
> + v4l2_async_cleanup(asdl);
> + return;
> + }
> +
> + mutex_lock(&list_lock);
> +
> + dev = asdl->dev;
> +
> + list_add(&asdl->asd->list, ¬ifier->waiting);
> +
> + v4l2_async_cleanup(asdl);
> +
> + if (notifier->unbind)
> + notifier->unbind(notifier, asdl);
> +
> + mutex_unlock(&list_lock);
> +}
> +EXPORT_SYMBOL(v4l2_async_unregister_subdev);
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> new file mode 100644
> index 0000000..c0470c6
> --- /dev/null
> +++ b/include/media/v4l2-async.h
> @@ -0,0 +1,104 @@
> +/*
> + * V4L2 asynchronous subdevice registration API
> + *
> + * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef V4L2_ASYNC_H
> +#define V4L2_ASYNC_H
> +
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +#include <media/v4l2-subdev.h>
> +
> +struct device;
> +struct v4l2_device;
> +struct v4l2_async_notifier;
> +
> +enum v4l2_async_bus_type {
> + V4L2_ASYNC_BUS_CUSTOM,
> + V4L2_ASYNC_BUS_PLATFORM,
> + V4L2_ASYNC_BUS_I2C,
> +};
> +
> +struct v4l2_async_hw_device {
> + enum v4l2_async_bus_type bus_type;
> + union {
> + struct {
> + const char *name;
> + } platform;
> + struct {
> + int adapter_id;
> + unsigned short address;
> + } i2c;
Do you think it would make sense to match I2C devices by name as well (using
dev_name(dev)) ?
> + struct {
> + bool (*match)(struct device *,
> + struct v4l2_async_hw_device *);
> + void *priv;
> + } special;
> + } match;
> +};
This isn't really a device, what about renaming it to v4l2_async_device_info,
v4l2_async_hw_info, v4l2_async_dev_info, ... (or s/info/desc/) ?
> +/**
> + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> + * @hw: this device descriptor
> + * @list: member in a list of subdevices
> + */
> +struct v4l2_async_subdev {
> + struct v4l2_async_hw_device hw;
> + struct list_head list;
> +};
I was wondering whether this structure couldn't be made private (and thus
dynamically allocated), but that might be overkill.
The structure isn't not part of the public API, except to access the hw field.
Maybe the bound and unbind notifiers could get a pointer to the hw field
directly to avoid going through v4l2_async_subdev ? We could then add a
comment to the structure definition to warn that the structure must not be
touched by subdev drivers at any time, and by bridge drivers in the notifier
callbacks (bridge drivers will still need to create and initialize the
v4l2_async_subdev instances passed to v4l2_async_notifier_register()).
v4l2_async_subdev could even be merged with struct v4l2_async_hw_device.
> +/**
> + * v4l2_async_subdev_list - provided by subdevices
> + * @list: member in a list of subdevices
> + * @dev: hardware device
> + * @subdev: V4L2 subdevice
> + * @asd: pointer to respective struct v4l2_async_subdev
> + * @notifier: pointer to managing notifier
> + */
> +struct v4l2_async_subdev_list {
> + struct list_head list;
> + struct device *dev;
> + struct v4l2_subdev *subdev;
> + struct v4l2_async_subdev *asd;
> + struct v4l2_async_notifier *notifier;
> +};
I don't think this structure is needed, at least not in a public header file.
Its fields could be moved to struct v4l2_subdev (which would also get rid of
the subdev field) or, alternatively, a pointer to v4l2_async_subdev_list could
be added to struct v4l2_subdev and allocated dynamically.
This would simplify the registration process for subdev drivers. They would
only need to call v4l2_async_register_subdev() with a pointer to the subdev,
without being required to instantiate a struct v4l2_async_subdev_list.
Obviously the dev pointer will still be needed. It could be passed to
v4l2_async_register_subdev(), but my personal preference for now would be to
add the struct device pointer to struct v4l2_subdev and let subdev drivers set
it before registering the subdev.
> +/**
> + * v4l2_async_notifier - provided by bridges
> + * @subdev_num: number of subdevices
> + * @subdev: array of pointers to subdevices
> + * @v4l2_dev: pointer to sruct v4l2_device
Typo, s/sruct/struct/
> + * @waiting: list of subdevices, waiting for their drivers
> + * @done: list of subdevices, already probed
> + * @list: member in a global list of notifiers
> + * @bound: a subdevice driver has successfully probed one of subdevices
> + * @complete: all subdevices have been probed successfully
> + * @unbind: a subdevice is leaving
> + */
> +struct v4l2_async_notifier {
> + int subdev_num;
The number of subdevs can't be negative, could this be unsigned ?
> + struct v4l2_async_subdev **subdev;
> + struct v4l2_device *v4l2_dev;
> + struct list_head waiting;
> + struct list_head done;
> + struct list_head list;
> + int (*bound)(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl);
> + int (*complete)(struct v4l2_async_notifier *notifier);
> + void (*unbind)(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl);
> +};
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> + struct v4l2_async_notifier *notifier);
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> +int v4l2_async_register_subdev(struct v4l2_async_subdev_list *asdl);
> +void v4l2_async_unregister_subdev(struct v4l2_async_subdev_list *asdl);
Renaming v4l2_async_(un)register_subdev to v4l2_(un)register_subdev might be a
good idea at some point, we can fix that later.
> +#endif
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration
2013-04-12 12:09 ` Laurent Pinchart
@ 2013-04-12 14:29 ` Guennadi Liakhovetski
0 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-12 14:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Hans Verkuil, Sylwester Nawrocki, Sylwester Nawrocki,
linux-sh, Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi Laurent
Thanks for the review.
On Fri, 12 Apr 2013, Laurent Pinchart wrote:
[snip]
> > + switch (hw->bus_type) {
> > + case V4L2_ASYNC_BUS_CUSTOM:
> > + match = hw->match.special.match;
> > + if (!match)
> > + /* Match always */
> > + return asd;
> > + break;
> > + case V4L2_ASYNC_BUS_PLATFORM:
> > + match = match_platform;
> > + break;
> > + case V4L2_ASYNC_BUS_I2C:
> > + match = match_i2c;
> > + break;
> > + default:
> > + /* Oops */
> > + match = NULL;
> > + dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL,
> > + "Invalid bus-type %u on %p\n", hw->bus_type, asd);
>
> An invalid hw->bus_type value is a driver (or board code) bug. Could you move
> this check to v4l2_async_notifier_register() when building the subdev list and
> return an error ?
agree.
> > +static struct device *v4l2_async_unregister(struct v4l2_async_subdev_list
> > *asdl)
> > +{
> > + struct device *dev = asdl->dev;
> > +
> > + v4l2_async_cleanup(asdl);
> > +
> > + /* If we handled USB devices, we'd have to lock the parent too */
> > + device_release_driver(dev);
> > + return dev;
>
> This function is called from a single location and the return value is unused,
> it could just return void.
will be changed
> > +}
> > +
> > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > + struct v4l2_async_notifier *notifier)
> > +{
> > + struct v4l2_async_subdev_list *asdl, *tmp;
> > + int i;
>
> Could this be unsigned (please see below for a similar comment about notifier-
> >subdev_num as well) ?
I like it how clearly we can separate in our reviews suggestions for
technical quality improvements from personal opinions and preferences ;-)
> > +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > +{
> > + struct v4l2_async_subdev_list *asdl, *tmp;
> > + int i = 0;
>
> i can't be negative, could it then be unsiged ?
Ditto :)
> > + struct device **dev = kcalloc(notifier->subdev_num,
> > + sizeof(*dev), GFP_KERNEL);
> > + if (!dev)
> > + dev_err(notifier->v4l2_dev->dev,
> > + "Failed to allocate device cache!\n");
> > +
> > + mutex_lock(&list_lock);
> > +
> > + list_del(¬ifier->list);
> > +
> > + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) {
> > + if (dev)
> > + dev[i++] = get_device(asdl->dev);
> > + v4l2_async_unregister(asdl);
> > +
> > + if (notifier->unbind)
> > + notifier->unbind(notifier, asdl);
> > + }
> > +
> > + mutex_unlock(&list_lock);
> > +
> > + if (dev) {
> > + while (i--) {
> > + if (dev[i] && device_attach(dev[i]) < 0)
>
> I'm still pretty uneasy about the device_attach() and device_release_driver()
> calls, I'll read your reply to Sylwester's comments and I'll answer there.
Maybe the following will help: we also discussed this with Greg K-H, he
also initially was of the opinion, that these calls shouldn't be needed in
_device_ drivers. They are to be used by bus drivers. Then I explained,
that we are indeed dealing with a media bus here. He didn't reply any
more :-)
> > +struct v4l2_async_hw_device {
> > + enum v4l2_async_bus_type bus_type;
> > + union {
> > + struct {
> > + const char *name;
> > + } platform;
> > + struct {
> > + int adapter_id;
> > + unsigned short address;
> > + } i2c;
>
> Do you think it would make sense to match I2C devices by name as well (using
> dev_name(dev)) ?
not necessarily... it would make it uniform, yes, but code authors would
have to hard-code device names, that are otherwise created by the I2C core.
Are we sure those never change? Don't think we're supposed to rely on them.
> > + struct {
> > + bool (*match)(struct device *,
> > + struct v4l2_async_hw_device *);
> > + void *priv;
> > + } special;
> > + } match;
> > +};
>
> This isn't really a device, what about renaming it to v4l2_async_device_info,
> v4l2_async_hw_info, v4l2_async_dev_info, ... (or s/info/desc/) ?
ok
> > +/**
> > + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> > + * @hw: this device descriptor
> > + * @list: member in a list of subdevices
> > + */
> > +struct v4l2_async_subdev {
> > + struct v4l2_async_hw_device hw;
> > + struct list_head list;
> > +};
>
> I was wondering whether this structure couldn't be made private (and thus
> dynamically allocated), but that might be overkill.
seems an overkill to me too.
> The structure isn't not part of the public API, except to access the hw field.
> Maybe the bound and unbind notifiers could get a pointer to the hw field
> directly to avoid going through v4l2_async_subdev ? We could then add a
> comment to the structure definition to warn that the structure must not be
> touched by subdev drivers at any time, and by bridge drivers in the notifier
> callbacks (bridge drivers will still need to create and initialize the
> v4l2_async_subdev instances passed to v4l2_async_notifier_register()).
> v4l2_async_subdev could even be merged with struct v4l2_async_hw_device.
>
> > +/**
> > + * v4l2_async_subdev_list - provided by subdevices
> > + * @list: member in a list of subdevices
> > + * @dev: hardware device
> > + * @subdev: V4L2 subdevice
> > + * @asd: pointer to respective struct v4l2_async_subdev
> > + * @notifier: pointer to managing notifier
> > + */
> > +struct v4l2_async_subdev_list {
> > + struct list_head list;
> > + struct device *dev;
> > + struct v4l2_subdev *subdev;
> > + struct v4l2_async_subdev *asd;
> > + struct v4l2_async_notifier *notifier;
> > +};
>
> I don't think this structure is needed, at least not in a public header file.
> Its fields could be moved to struct v4l2_subdev (which would also get rid of
> the subdev field) or, alternatively, a pointer to v4l2_async_subdev_list could
> be added to struct v4l2_subdev and allocated dynamically.
I'll merge it into v4l2_dubdev
> This would simplify the registration process for subdev drivers. They would
> only need to call v4l2_async_register_subdev() with a pointer to the subdev,
> without being required to instantiate a struct v4l2_async_subdev_list.
>
> Obviously the dev pointer will still be needed. It could be passed to
> v4l2_async_register_subdev(), but my personal preference for now would be to
> add the struct device pointer to struct v4l2_subdev and let subdev drivers set
> it before registering the subdev.
>
> > +/**
> > + * v4l2_async_notifier - provided by bridges
> > + * @subdev_num: number of subdevices
> > + * @subdev: array of pointers to subdevices
> > + * @v4l2_dev: pointer to sruct v4l2_device
>
> Typo, s/sruct/struct/
thanks
> > + * @waiting: list of subdevices, waiting for their drivers
> > + * @done: list of subdevices, already probed
> > + * @list: member in a global list of notifiers
> > + * @bound: a subdevice driver has successfully probed one of subdevices
> > + * @complete: all subdevices have been probed successfully
> > + * @unbind: a subdevice is leaving
> > + */
> > +struct v4l2_async_notifier {
> > + int subdev_num;
>
> The number of subdevs can't be negative, could this be unsigned ?
yes, here it makes sense. a simple
int i;
for (i = 0; i < N; i++)
...
is just not worth it imho :)
> > + struct v4l2_async_subdev **subdev;
> > + struct v4l2_device *v4l2_dev;
> > + struct list_head waiting;
> > + struct list_head done;
> > + struct list_head list;
> > + int (*bound)(struct v4l2_async_notifier *notifier,
> > + struct v4l2_async_subdev_list *asdl);
> > + int (*complete)(struct v4l2_async_notifier *notifier);
> > + void (*unbind)(struct v4l2_async_notifier *notifier,
> > + struct v4l2_async_subdev_list *asdl);
> > +};
> > +
> > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > + struct v4l2_async_notifier *notifier);
> > +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> > +int v4l2_async_register_subdev(struct v4l2_async_subdev_list *asdl);
> > +void v4l2_async_unregister_subdev(struct v4l2_async_subdev_list *asdl);
>
> Renaming v4l2_async_(un)register_subdev to v4l2_(un)register_subdev might be a
> good idea at some point, we can fix that later.
maybe
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 1/7] media: V4L2: add temporary clock helpers Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 2/7] media: V4L2: support asynchronous subdevice registration Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 4/7] soc-camera: add V4L2-async support Guennadi Liakhovetski
` (5 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/media/i2c/soc_camera/imx074.c | 18 ++-
drivers/media/i2c/soc_camera/mt9m001.c | 17 ++-
drivers/media/i2c/soc_camera/mt9m111.c | 20 ++-
drivers/media/i2c/soc_camera/mt9t031.c | 19 ++-
drivers/media/i2c/soc_camera/mt9t112.c | 19 ++-
drivers/media/i2c/soc_camera/mt9v022.c | 17 ++-
drivers/media/i2c/soc_camera/ov2640.c | 19 ++-
drivers/media/i2c/soc_camera/ov5642.c | 20 ++-
drivers/media/i2c/soc_camera/ov6650.c | 17 ++-
drivers/media/i2c/soc_camera/ov772x.c | 15 ++-
drivers/media/i2c/soc_camera/ov9640.c | 17 ++-
drivers/media/i2c/soc_camera/ov9640.h | 1 +
drivers/media/i2c/soc_camera/ov9740.c | 18 ++-
drivers/media/i2c/soc_camera/rj54n1cb0c.c | 17 ++-
drivers/media/i2c/soc_camera/tw9910.c | 18 ++-
drivers/media/platform/soc_camera/soc_camera.c | 162 +++++++++++++++-----
.../platform/soc_camera/soc_camera_platform.c | 2 +-
include/media/soc_camera.h | 13 +-
18 files changed, 350 insertions(+), 79 deletions(-)
diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
index a2a5cbb..a6a5060 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <media/soc_camera.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
@@ -77,6 +78,7 @@ struct imx074_datafmt {
struct imx074 {
struct v4l2_subdev subdev;
const struct imx074_datafmt *fmt;
+ struct v4l2_clk *clk;
};
static const struct imx074_datafmt imx074_colour_fmts[] = {
@@ -272,8 +274,9 @@ static int imx074_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
static int imx074_g_mbus_config(struct v4l2_subdev *sd,
@@ -431,6 +434,7 @@ static int imx074_probe(struct i2c_client *client,
struct imx074 *priv;
struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(&client->dev, "IMX074: missing platform data!\n");
@@ -451,13 +455,23 @@ static int imx074_probe(struct i2c_client *client,
priv->fmt = &imx074_colour_fmts[0];
- return imx074_video_probe(client);
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ ret = imx074_video_probe(client);
+ if (ret < 0)
+ v4l2_clk_put(priv->clk);
+
+ return ret;
}
static int imx074_remove(struct i2c_client *client)
{
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
+ v4l2_clk_put(priv->clk);
if (ssdd->free_bus)
ssdd->free_bus(ssdd);
diff --git a/drivers/media/i2c/soc_camera/mt9m001.c b/drivers/media/i2c/soc_camera/mt9m001.c
index dd90898..620f9df 100644
--- a/drivers/media/i2c/soc_camera/mt9m001.c
+++ b/drivers/media/i2c/soc_camera/mt9m001.c
@@ -16,6 +16,7 @@
#include <media/soc_camera.h>
#include <media/soc_mediabus.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-ctrls.h>
@@ -94,6 +95,7 @@ struct mt9m001 {
struct v4l2_ctrl *exposure;
};
struct v4l2_rect rect; /* Sensor window */
+ struct v4l2_clk *clk;
const struct mt9m001_datafmt *fmt;
const struct mt9m001_datafmt *fmts;
int num_fmts;
@@ -381,8 +383,9 @@ static int mt9m001_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct mt9m001 *mt9m001 = to_mt9m001(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, mt9m001->clk, on);
}
static int mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -710,9 +713,18 @@ static int mt9m001_probe(struct i2c_client *client,
mt9m001->rect.width = MT9M001_MAX_WIDTH;
mt9m001->rect.height = MT9M001_MAX_HEIGHT;
+ mt9m001->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(mt9m001->clk)) {
+ ret = PTR_ERR(mt9m001->clk);
+ goto eclkget;
+ }
+
ret = mt9m001_video_probe(ssdd, client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(mt9m001->clk);
+eclkget:
v4l2_ctrl_handler_free(&mt9m001->hdl);
+ }
return ret;
}
@@ -722,6 +734,7 @@ static int mt9m001_remove(struct i2c_client *client)
struct mt9m001 *mt9m001 = to_mt9m001(client);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ v4l2_clk_put(mt9m001->clk);
v4l2_device_unregister_subdev(&mt9m001->subdev);
v4l2_ctrl_handler_free(&mt9m001->hdl);
mt9m001_video_remove(ssdd);
diff --git a/drivers/media/i2c/soc_camera/mt9m111.c b/drivers/media/i2c/soc_camera/mt9m111.c
index 8bd4e0d..5fa9372 100644
--- a/drivers/media/i2c/soc_camera/mt9m111.c
+++ b/drivers/media/i2c/soc_camera/mt9m111.c
@@ -17,6 +17,7 @@
#include <linux/module.h>
#include <media/soc_camera.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-chip-ident.h>
@@ -209,6 +210,7 @@ struct mt9m111 {
* from v4l2-chip-ident.h */
struct mt9m111_context *ctx;
struct v4l2_rect rect; /* cropping rectangle */
+ struct v4l2_clk *clk;
int width; /* output */
int height; /* sizes */
struct mutex power_lock; /* lock to protect power_count */
@@ -801,14 +803,14 @@ static int mt9m111_power_on(struct mt9m111 *mt9m111)
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
int ret;
- ret = soc_camera_power_on(&client->dev, ssdd);
+ ret = soc_camera_power_on(&client->dev, ssdd, mt9m111->clk);
if (ret < 0)
return ret;
ret = mt9m111_resume(mt9m111);
if (ret < 0) {
dev_err(&client->dev, "Failed to resume the sensor: %d\n", ret);
- soc_camera_power_off(&client->dev, ssdd);
+ soc_camera_power_off(&client->dev, ssdd, mt9m111->clk);
}
return ret;
@@ -820,7 +822,7 @@ static void mt9m111_power_off(struct mt9m111 *mt9m111)
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
mt9m111_suspend(mt9m111);
- soc_camera_power_off(&client->dev, ssdd);
+ soc_camera_power_off(&client->dev, ssdd, mt9m111->clk);
}
static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
@@ -1002,9 +1004,18 @@ static int mt9m111_probe(struct i2c_client *client,
mt9m111->lastpage = -1;
mutex_init(&mt9m111->power_lock);
+ mt9m111->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(mt9m111->clk)) {
+ ret = PTR_ERR(mt9m111->clk);
+ goto eclkget;
+ }
+
ret = mt9m111_video_probe(client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(mt9m111->clk);
+eclkget:
v4l2_ctrl_handler_free(&mt9m111->hdl);
+ }
return ret;
}
@@ -1013,6 +1024,7 @@ static int mt9m111_remove(struct i2c_client *client)
{
struct mt9m111 *mt9m111 = to_mt9m111(client);
+ v4l2_clk_put(mt9m111->clk);
v4l2_device_unregister_subdev(&mt9m111->subdev);
v4l2_ctrl_handler_free(&mt9m111->hdl);
diff --git a/drivers/media/i2c/soc_camera/mt9t031.c b/drivers/media/i2c/soc_camera/mt9t031.c
index 26a15b8..ea791e3 100644
--- a/drivers/media/i2c/soc_camera/mt9t031.c
+++ b/drivers/media/i2c/soc_camera/mt9t031.c
@@ -19,6 +19,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-ctrls.h>
@@ -76,6 +77,7 @@ struct mt9t031 {
struct v4l2_ctrl *exposure;
};
struct v4l2_rect rect; /* Sensor window */
+ struct v4l2_clk *clk;
int model; /* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
u16 xskip;
u16 yskip;
@@ -610,16 +612,17 @@ static int mt9t031_s_power(struct v4l2_subdev *sd, int on)
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
struct video_device *vdev = soc_camera_i2c_to_vdev(client);
+ struct mt9t031 *mt9t031 = to_mt9t031(client);
int ret;
if (on) {
- ret = soc_camera_power_on(&client->dev, ssdd);
+ ret = soc_camera_power_on(&client->dev, ssdd, mt9t031->clk);
if (ret < 0)
return ret;
vdev->dev.type = &mt9t031_dev_type;
} else {
vdev->dev.type = NULL;
- soc_camera_power_off(&client->dev, ssdd);
+ soc_camera_power_off(&client->dev, ssdd, mt9t031->clk);
}
return 0;
@@ -812,9 +815,18 @@ static int mt9t031_probe(struct i2c_client *client,
mt9t031->xskip = 1;
mt9t031->yskip = 1;
+ mt9t031->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(mt9t031->clk)) {
+ ret = PTR_ERR(mt9t031->clk);
+ goto eclkget;
+ }
+
ret = mt9t031_video_probe(client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(mt9t031->clk);
+eclkget:
v4l2_ctrl_handler_free(&mt9t031->hdl);
+ }
return ret;
}
@@ -823,6 +835,7 @@ static int mt9t031_remove(struct i2c_client *client)
{
struct mt9t031 *mt9t031 = to_mt9t031(client);
+ v4l2_clk_put(mt9t031->clk);
v4l2_device_unregister_subdev(&mt9t031->subdev);
v4l2_ctrl_handler_free(&mt9t031->hdl);
diff --git a/drivers/media/i2c/soc_camera/mt9t112.c b/drivers/media/i2c/soc_camera/mt9t112.c
index a7256b7..577cdea 100644
--- a/drivers/media/i2c/soc_camera/mt9t112.c
+++ b/drivers/media/i2c/soc_camera/mt9t112.c
@@ -28,6 +28,7 @@
#include <media/mt9t112.h>
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-common.h>
/* you can check PLL/clock info */
@@ -90,6 +91,7 @@ struct mt9t112_priv {
struct mt9t112_camera_info *info;
struct i2c_client *client;
struct v4l2_rect frame;
+ struct v4l2_clk *clk;
const struct mt9t112_format *format;
int model;
int num_formats;
@@ -781,8 +783,9 @@ static int mt9t112_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct mt9t112_priv *priv = to_mt9t112(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
static struct v4l2_subdev_core_ops mt9t112_subdev_core_ops = {
@@ -1108,18 +1111,26 @@ static int mt9t112_probe(struct i2c_client *client,
v4l2_i2c_subdev_init(&priv->subdev, client, &mt9t112_subdev_ops);
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
ret = mt9t112_camera_probe(client);
- if (ret)
- return ret;
/* Cannot fail: using the default supported pixel code */
- mt9t112_set_params(priv, &rect, V4L2_MBUS_FMT_UYVY8_2X8);
+ if (!ret)
+ mt9t112_set_params(priv, &rect, V4L2_MBUS_FMT_UYVY8_2X8);
+ else
+ v4l2_clk_put(priv->clk);
return ret;
}
static int mt9t112_remove(struct i2c_client *client)
{
+ struct mt9t112_priv *priv = to_mt9t112(client);
+
+ v4l2_clk_put(priv->clk);
return 0;
}
diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index a295e59..3164f56 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -20,6 +20,7 @@
#include <media/soc_mediabus.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-ctrls.h>
/*
@@ -149,6 +150,7 @@ struct mt9v022 {
struct v4l2_ctrl *hblank;
struct v4l2_ctrl *vblank;
struct v4l2_rect rect; /* Sensor window */
+ struct v4l2_clk *clk;
const struct mt9v022_datafmt *fmt;
const struct mt9v022_datafmt *fmts;
const struct mt9v02x_register *reg;
@@ -518,8 +520,9 @@ static int mt9v022_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct mt9v022 *mt9v022 = to_mt9v022(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, mt9v022->clk, on);
}
static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -957,9 +960,18 @@ static int mt9v022_probe(struct i2c_client *client,
mt9v022->rect.width = MT9V022_MAX_WIDTH;
mt9v022->rect.height = MT9V022_MAX_HEIGHT;
+ mt9v022->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(mt9v022->clk)) {
+ ret = PTR_ERR(mt9v022->clk);
+ goto eclkget;
+ }
+
ret = mt9v022_video_probe(client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(mt9v022->clk);
+eclkget:
v4l2_ctrl_handler_free(&mt9v022->hdl);
+ }
return ret;
}
@@ -969,6 +981,7 @@ static int mt9v022_remove(struct i2c_client *client)
struct mt9v022 *mt9v022 = to_mt9v022(client);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ v4l2_clk_put(mt9v022->clk);
v4l2_device_unregister_subdev(&mt9v022->subdev);
if (ssdd->free_bus)
ssdd->free_bus(ssdd);
diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index e316842..0c97b8c 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -23,6 +23,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-ctrls.h>
@@ -303,6 +304,7 @@ struct ov2640_priv {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
enum v4l2_mbus_pixelcode cfmt_code;
+ struct v4l2_clk *clk;
const struct ov2640_win_size *win;
int model;
};
@@ -772,8 +774,9 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov2640_priv *priv = to_ov2640(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
/* Select the nearest higher resolution for capture */
@@ -1113,11 +1116,20 @@ static int ov2640_probe(struct i2c_client *client,
if (priv->hdl.error)
return priv->hdl.error;
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto eclkget;
+ }
+
ret = ov2640_video_probe(client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(priv->clk);
+eclkget:
v4l2_ctrl_handler_free(&priv->hdl);
- else
+ } else {
dev_info(&adapter->dev, "OV2640 Probed\n");
+ }
return ret;
}
@@ -1126,6 +1138,7 @@ static int ov2640_remove(struct i2c_client *client)
{
struct ov2640_priv *priv = to_ov2640(client);
+ v4l2_clk_put(priv->clk);
v4l2_device_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
diff --git a/drivers/media/i2c/soc_camera/ov5642.c b/drivers/media/i2c/soc_camera/ov5642.c
index 9aa56de..8ffc8f9 100644
--- a/drivers/media/i2c/soc_camera/ov5642.c
+++ b/drivers/media/i2c/soc_camera/ov5642.c
@@ -25,6 +25,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
/* OV5642 registers */
@@ -610,6 +611,7 @@ struct ov5642 {
struct v4l2_subdev subdev;
const struct ov5642_datafmt *fmt;
struct v4l2_rect crop_rect;
+ struct v4l2_clk *clk;
/* blanking information */
int total_width;
@@ -935,12 +937,13 @@ static int ov5642_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov5642 *priv = to_ov5642(client);
int ret;
if (!on)
- return soc_camera_power_off(&client->dev, ssdd);
+ return soc_camera_power_off(&client->dev, ssdd, priv->clk);
- ret = soc_camera_power_on(&client->dev, ssdd);
+ ret = soc_camera_power_on(&client->dev, ssdd, priv->clk);
if (ret < 0)
return ret;
@@ -1021,6 +1024,7 @@ static int ov5642_probe(struct i2c_client *client,
{
struct ov5642 *priv;
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(&client->dev, "OV5642: missing platform data!\n");
@@ -1042,13 +1046,23 @@ static int ov5642_probe(struct i2c_client *client,
priv->total_width = OV5642_DEFAULT_WIDTH + BLANKING_EXTRA_WIDTH;
priv->total_height = BLANKING_MIN_HEIGHT;
- return ov5642_video_probe(client);
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ ret = ov5642_video_probe(client);
+ if (ret < 0)
+ v4l2_clk_put(priv->clk);
+
+ return ret;
}
static int ov5642_remove(struct i2c_client *client)
{
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov5642 *priv = to_ov5642(client);
+ v4l2_clk_put(priv->clk);
if (ssdd->free_bus)
ssdd->free_bus(ssdd);
diff --git a/drivers/media/i2c/soc_camera/ov6650.c b/drivers/media/i2c/soc_camera/ov6650.c
index 991202d..2389aa2 100644
--- a/drivers/media/i2c/soc_camera/ov6650.c
+++ b/drivers/media/i2c/soc_camera/ov6650.c
@@ -33,6 +33,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-ctrls.h>
/* Register definitions */
@@ -196,6 +197,7 @@ struct ov6650 {
struct v4l2_ctrl *blue;
struct v4l2_ctrl *red;
};
+ struct v4l2_clk *clk;
bool half_scale; /* scale down output by 2 */
struct v4l2_rect rect; /* sensor cropping window */
unsigned long pclk_limit; /* from host */
@@ -436,8 +438,9 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov6650 *priv = to_ov6650(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
static int ov6650_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
@@ -1025,9 +1028,18 @@ static int ov6650_probe(struct i2c_client *client,
priv->code = V4L2_MBUS_FMT_YUYV8_2X8;
priv->colorspace = V4L2_COLORSPACE_JPEG;
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto eclkget;
+ }
+
ret = ov6650_video_probe(client);
- if (ret)
+ if (ret) {
+ v4l2_clk_put(priv->clk);
+eclkget:
v4l2_ctrl_handler_free(&priv->hdl);
+ }
return ret;
}
@@ -1036,6 +1048,7 @@ static int ov6650_remove(struct i2c_client *client)
{
struct ov6650 *priv = to_ov6650(client);
+ v4l2_clk_put(priv->clk);
v4l2_device_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
diff --git a/drivers/media/i2c/soc_camera/ov772x.c b/drivers/media/i2c/soc_camera/ov772x.c
index 713d62e..d643b69 100644
--- a/drivers/media/i2c/soc_camera/ov772x.c
+++ b/drivers/media/i2c/soc_camera/ov772x.c
@@ -26,6 +26,7 @@
#include <media/ov772x.h>
#include <media/soc_camera.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-subdev.h>
@@ -396,6 +397,7 @@ struct ov772x_win_size {
struct ov772x_priv {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
+ struct v4l2_clk *clk;
struct ov772x_camera_info *info;
const struct ov772x_color_format *cfmt;
const struct ov772x_win_size *win;
@@ -668,8 +670,9 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov772x_priv *priv = to_ov772x(sd);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
@@ -1088,13 +1091,22 @@ static int ov772x_probe(struct i2c_client *client,
if (priv->hdl.error)
return priv->hdl.error;
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto eclkget;
+ }
+
ret = ov772x_video_probe(priv);
if (ret < 0) {
+ v4l2_clk_put(priv->clk);
+eclkget:
v4l2_ctrl_handler_free(&priv->hdl);
} else {
priv->cfmt = &ov772x_cfmts[0];
priv->win = &ov772x_win_sizes[0];
}
+
return ret;
}
@@ -1102,6 +1114,7 @@ static int ov772x_remove(struct i2c_client *client)
{
struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
+ v4l2_clk_put(priv->clk);
v4l2_device_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
diff --git a/drivers/media/i2c/soc_camera/ov9640.c b/drivers/media/i2c/soc_camera/ov9640.c
index 20ca62d..f4d09f7 100644
--- a/drivers/media/i2c/soc_camera/ov9640.c
+++ b/drivers/media/i2c/soc_camera/ov9640.c
@@ -29,6 +29,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ctrls.h>
@@ -337,8 +338,9 @@ static int ov9640_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
/* select nearest higher resolution for capture */
@@ -716,10 +718,18 @@ static int ov9640_probe(struct i2c_client *client,
if (priv->hdl.error)
return priv->hdl.error;
- ret = ov9640_video_probe(client);
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto eclkget;
+ }
- if (ret)
+ ret = ov9640_video_probe(client);
+ if (ret) {
+ v4l2_clk_put(priv->clk);
+eclkget:
v4l2_ctrl_handler_free(&priv->hdl);
+ }
return ret;
}
@@ -729,6 +739,7 @@ static int ov9640_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct ov9640_priv *priv = to_ov9640_sensor(sd);
+ v4l2_clk_put(priv->clk);
v4l2_device_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
diff --git a/drivers/media/i2c/soc_camera/ov9640.h b/drivers/media/i2c/soc_camera/ov9640.h
index 6b33a97..65d13ff 100644
--- a/drivers/media/i2c/soc_camera/ov9640.h
+++ b/drivers/media/i2c/soc_camera/ov9640.h
@@ -199,6 +199,7 @@ struct ov9640_reg {
struct ov9640_priv {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
+ struct v4l2_clk *clk;
int model;
int revision;
diff --git a/drivers/media/i2c/soc_camera/ov9740.c b/drivers/media/i2c/soc_camera/ov9740.c
index 012bd62..6a5e164 100644
--- a/drivers/media/i2c/soc_camera/ov9740.c
+++ b/drivers/media/i2c/soc_camera/ov9740.c
@@ -18,6 +18,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-ctrls.h>
#define to_ov9740(sd) container_of(sd, struct ov9740_priv, subdev)
@@ -196,6 +197,7 @@ struct ov9740_reg {
struct ov9740_priv {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
+ struct v4l2_clk *clk;
int ident;
u16 model;
@@ -792,7 +794,7 @@ static int ov9740_s_power(struct v4l2_subdev *sd, int on)
int ret;
if (on) {
- ret = soc_camera_power_on(&client->dev, ssdd);
+ ret = soc_camera_power_on(&client->dev, ssdd, priv->clk);
if (ret < 0)
return ret;
@@ -806,7 +808,7 @@ static int ov9740_s_power(struct v4l2_subdev *sd, int on)
priv->current_enable = true;
}
- soc_camera_power_off(&client->dev, ssdd);
+ soc_camera_power_off(&client->dev, ssdd, priv->clk);
}
return 0;
@@ -975,9 +977,18 @@ static int ov9740_probe(struct i2c_client *client,
if (priv->hdl.error)
return priv->hdl.error;
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ goto eclkget;
+ }
+
ret = ov9740_video_probe(client);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_clk_put(priv->clk);
+eclkget:
v4l2_ctrl_handler_free(&priv->hdl);
+ }
return ret;
}
@@ -986,6 +997,7 @@ static int ov9740_remove(struct i2c_client *client)
{
struct ov9740_priv *priv = i2c_get_clientdata(client);
+ v4l2_clk_put(priv->clk);
v4l2_device_unregister_subdev(&priv->subdev);
v4l2_ctrl_handler_free(&priv->hdl);
return 0;
diff --git a/drivers/media/i2c/soc_camera/rj54n1cb0c.c b/drivers/media/i2c/soc_camera/rj54n1cb0c.c
index 1f9ec3b..93db388 100644
--- a/drivers/media/i2c/soc_camera/rj54n1cb0c.c
+++ b/drivers/media/i2c/soc_camera/rj54n1cb0c.c
@@ -17,6 +17,7 @@
#include <media/rj54n1cb0c.h>
#include <media/soc_camera.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-ctrls.h>
@@ -151,6 +152,7 @@ struct rj54n1_clock_div {
struct rj54n1 {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
+ struct v4l2_clk *clk;
struct rj54n1_clock_div clk_div;
const struct rj54n1_datafmt *fmt;
struct v4l2_rect rect; /* Sensor window */
@@ -1184,8 +1186,9 @@ static int rj54n1_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct rj54n1 *rj54n1 = to_rj54n1(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, rj54n1->clk, on);
}
static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -1382,9 +1385,18 @@ static int rj54n1_probe(struct i2c_client *client,
rj54n1->tgclk_mhz = (rj54n1_priv->mclk_freq / PLL_L * PLL_N) /
(clk_div.ratio_tg + 1) / (clk_div.ratio_t + 1);
+ rj54n1->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(rj54n1->clk)) {
+ ret = PTR_ERR(rj54n1->clk);
+ goto eclkget;
+ }
+
ret = rj54n1_video_probe(client, rj54n1_priv);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_clk_put(rj54n1->clk);
+eclkget:
v4l2_ctrl_handler_free(&rj54n1->hdl);
+ }
return ret;
}
@@ -1394,6 +1406,7 @@ static int rj54n1_remove(struct i2c_client *client)
struct rj54n1 *rj54n1 = to_rj54n1(client);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ v4l2_clk_put(rj54n1->clk);
v4l2_device_unregister_subdev(&rj54n1->subdev);
if (ssdd->free_bus)
ssdd->free_bus(ssdd);
diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
index bad90b1..fe43591 100644
--- a/drivers/media/i2c/soc_camera/tw9910.c
+++ b/drivers/media/i2c/soc_camera/tw9910.c
@@ -28,6 +28,7 @@
#include <media/soc_camera.h>
#include <media/tw9910.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#define GET_ID(val) ((val & 0xF8) >> 3)
@@ -228,6 +229,7 @@ struct tw9910_scale_ctrl {
struct tw9910_priv {
struct v4l2_subdev subdev;
+ struct v4l2_clk *clk;
struct tw9910_video_info *info;
const struct tw9910_scale_ctrl *scale;
v4l2_std_id norm;
@@ -570,8 +572,9 @@ static int tw9910_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct tw9910_priv *priv = to_tw9910(client);
- return soc_camera_set_power(&client->dev, ssdd, on);
+ return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
}
static int tw9910_set_frame(struct v4l2_subdev *sd, u32 *width, u32 *height)
@@ -912,6 +915,7 @@ static int tw9910_probe(struct i2c_client *client,
struct i2c_adapter *adapter to_i2c_adapter(client->dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd || !ssdd->drv_priv) {
dev_err(&client->dev, "TW9910: missing platform data!\n");
@@ -935,11 +939,21 @@ static int tw9910_probe(struct i2c_client *client,
v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops);
- return tw9910_video_probe(client);
+ priv->clk = v4l2_clk_get(&client->dev, "mclk");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ ret = tw9910_video_probe(client);
+ if (ret < 0)
+ v4l2_clk_put(priv->clk);
+
+ return ret;
}
static int tw9910_remove(struct i2c_client *client)
{
+ struct tw9910_priv *priv = to_tw9910(client);
+ v4l2_clk_put(priv->clk);
return 0;
}
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f693817..cfcdabe 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -30,6 +30,7 @@
#include <linux/vmalloc.h>
#include <media/soc_camera.h>
+#include <media/v4l2-clk.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-dev.h>
@@ -50,13 +51,19 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
-int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd)
+int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
+ struct v4l2_clk *clk)
{
- int ret = regulator_bulk_enable(ssdd->num_regulators,
+ int ret = clk ? v4l2_clk_enable(clk) : 0;
+ if (ret < 0) {
+ dev_err(dev, "Cannot enable clock\n");
+ return ret;
+ }
+ ret = regulator_bulk_enable(ssdd->num_regulators,
ssdd->regulators);
if (ret < 0) {
dev_err(dev, "Cannot enable regulators\n");
- return ret;
+ goto eregenable;;
}
if (ssdd->power) {
@@ -64,16 +71,25 @@ int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd)
if (ret < 0) {
dev_err(dev,
"Platform failed to power-on the camera.\n");
- regulator_bulk_disable(ssdd->num_regulators,
- ssdd->regulators);
+ goto epwron;
}
}
+ return 0;
+
+epwron:
+ regulator_bulk_disable(ssdd->num_regulators,
+ ssdd->regulators);
+eregenable:
+ if (clk)
+ v4l2_clk_disable(clk);
+
return ret;
}
EXPORT_SYMBOL(soc_camera_power_on);
-int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd)
+int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd,
+ struct v4l2_clk *clk)
{
int ret = 0;
int err;
@@ -94,6 +110,9 @@ int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd
ret = ret ? : err;
}
+ if (clk)
+ v4l2_clk_disable(clk);
+
return ret;
}
EXPORT_SYMBOL(soc_camera_power_off);
@@ -513,9 +532,11 @@ static int soc_camera_add_device(struct soc_camera_device *icd)
if (ici->icd)
return -EBUSY;
- ret = ici->ops->clock_start(ici);
- if (ret < 0)
- return ret;
+ if (!icd->clk) {
+ ret = ici->ops->clock_start(ici);
+ if (ret < 0)
+ return ret;
+ }
if (ici->ops->add) {
ret = ici->ops->add(icd);
@@ -528,7 +549,8 @@ static int soc_camera_add_device(struct soc_camera_device *icd)
return 0;
eadd:
- ici->ops->clock_stop(ici);
+ if (!icd->clk)
+ ici->ops->clock_stop(ici);
return ret;
}
@@ -541,7 +563,8 @@ static void soc_camera_remove_device(struct soc_camera_device *icd)
if (ici->ops->remove)
ici->ops->remove(icd);
- ici->ops->clock_stop(ici);
+ if (!icd->clk)
+ ici->ops->clock_stop(ici);
ici->icd = NULL;
}
@@ -1124,6 +1147,57 @@ static void scan_add_host(struct soc_camera_host *ici)
mutex_unlock(&list_lock);
}
+/*
+ * It is invalid to call v4l2_clk_enable() after a successful probing
+ * asynchronously outside of V4L2 operations, i.e. with .host_lock not held.
+ */
+static int soc_camera_clk_enable(struct v4l2_clk *clk)
+{
+ struct soc_camera_device *icd = clk->priv;
+ struct soc_camera_host *ici;
+
+ if (!icd || !icd->parent)
+ return -ENODEV;
+
+ ici = to_soc_camera_host(icd->parent);
+
+ if (!try_module_get(ici->ops->owner))
+ return -ENODEV;
+
+ /*
+ * If a different client is currently being probed, the host will tell
+ * you to go
+ */
+ return ici->ops->clock_start(ici);
+}
+
+static void soc_camera_clk_disable(struct v4l2_clk *clk)
+{
+ struct soc_camera_device *icd = clk->priv;
+ struct soc_camera_host *ici;
+
+ if (!icd || !icd->parent)
+ return;
+
+ ici = to_soc_camera_host(icd->parent);
+
+ ici->ops->clock_stop(ici);
+
+ module_put(ici->ops->owner);
+}
+
+/*
+ * Eventually, it would be more logical to make the respective host the clock
+ * owner, but then we would have to copy this struct for each ici. Besides, it
+ * would introduce the circular dependency problem, unless we port all client
+ * drivers to release the clock, when not in use.
+ */
+static const struct v4l2_clk_ops soc_camera_clk_ops = {
+ .owner = THIS_MODULE,
+ .enable = soc_camera_clk_enable,
+ .disable = soc_camera_clk_disable,
+};
+
#ifdef CONFIG_I2C_BOARDINFO
static int soc_camera_init_i2c(struct soc_camera_device *icd,
struct soc_camera_desc *sdesc)
@@ -1133,19 +1207,32 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
struct soc_camera_host_desc *shd = &sdesc->host_desc;
struct i2c_adapter *adap = i2c_get_adapter(shd->i2c_adapter_id);
struct v4l2_subdev *subdev;
+ char clk_name[V4L2_SUBDEV_NAME_SIZE];
+ int ret;
if (!adap) {
dev_err(icd->pdev, "Cannot get I2C adapter #%d. No driver?\n",
shd->i2c_adapter_id);
- goto ei2cga;
+ return -ENODEV;
}
shd->board_info->platform_data = &sdesc->subdev_desc;
+ snprintf(clk_name, sizeof(clk_name), "%d-%04x",
+ shd->i2c_adapter_id, shd->board_info->addr);
+
+ icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
+ if (IS_ERR(icd->clk)) {
+ ret = PTR_ERR(icd->clk);
+ goto eclkreg;
+ }
+
subdev = v4l2_i2c_new_subdev_board(&ici->v4l2_dev, adap,
shd->board_info, NULL);
- if (!subdev)
+ if (!subdev) {
+ ret = -ENODEV;
goto ei2cnd;
+ }
client = v4l2_get_subdevdata(subdev);
@@ -1154,9 +1241,11 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
return 0;
ei2cnd:
+ v4l2_clk_unregister(icd->clk);
+ icd->clk = NULL;
+eclkreg:
i2c_put_adapter(adap);
-ei2cga:
- return -ENODEV;
+ return ret;
}
static void soc_camera_free_i2c(struct soc_camera_device *icd)
@@ -1169,6 +1258,8 @@ static void soc_camera_free_i2c(struct soc_camera_device *icd)
v4l2_device_unregister_subdev(i2c_get_clientdata(client));
i2c_unregister_device(client);
i2c_put_adapter(adap);
+ v4l2_clk_unregister(icd->clk);
+ icd->clk = NULL;
}
#else
#define soc_camera_init_i2c(icd, sdesc) (-ENODEV)
@@ -1206,26 +1297,31 @@ static int soc_camera_probe(struct soc_camera_device *icd)
if (ssdd->reset)
ssdd->reset(icd->pdev);
- mutex_lock(&ici->host_lock);
- ret = ici->ops->clock_start(ici);
- mutex_unlock(&ici->host_lock);
- if (ret < 0)
- goto eadd;
-
/* Must have icd->vdev before registering the device */
ret = video_dev_create(icd);
if (ret < 0)
goto evdc;
+ /*
+ * ..._video_start() will create a device node, video_register_device()
+ * itself is protected against concurrent open() calls, but we also have
+ * to protect our data also during client probing.
+ */
+ mutex_lock(&ici->host_lock);
+
/* Non-i2c cameras, e.g., soc_camera_platform, have no board_info */
if (shd->board_info) {
ret = soc_camera_init_i2c(icd, sdesc);
if (ret < 0)
- goto eadddev;
+ goto eadd;
} else if (!shd->add_device || !shd->del_device) {
ret = -EINVAL;
- goto eadddev;
+ goto eadd;
} else {
+ ret = ici->ops->clock_start(ici);
+ if (ret < 0)
+ goto eadd;
+
if (shd->module_name)
ret = request_module(shd->module_name);
@@ -1261,13 +1357,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
icd->field = V4L2_FIELD_ANY;
- /*
- * ..._video_start() will create a device node, video_register_device()
- * itself is protected against concurrent open() calls, but we also have
- * to protect our data.
- */
- mutex_lock(&ici->host_lock);
-
ret = soc_camera_video_start(icd);
if (ret < 0)
goto evidstart;
@@ -1280,14 +1369,14 @@ static int soc_camera_probe(struct soc_camera_device *icd)
icd->field = mf.field;
}
- ici->ops->clock_stop(ici);
+ if (!shd->board_info)
+ ici->ops->clock_stop(ici);
mutex_unlock(&ici->host_lock);
return 0;
evidstart:
- mutex_unlock(&ici->host_lock);
soc_camera_free_user_formats(icd);
eiufmt:
ectrl:
@@ -1296,16 +1385,15 @@ ectrl:
} else {
shd->del_device(icd);
module_put(control->driver->owner);
- }
enodrv:
eadddev:
+ ici->ops->clock_stop(ici);
+ }
+eadd:
video_device_release(icd->vdev);
icd->vdev = NULL;
-evdc:
- mutex_lock(&ici->host_lock);
- ici->ops->clock_stop(ici);
mutex_unlock(&ici->host_lock);
-eadd:
+evdc:
v4l2_ctrl_handler_free(&icd->ctrl_handler);
return ret;
}
diff --git a/drivers/media/platform/soc_camera/soc_camera_platform.c b/drivers/media/platform/soc_camera/soc_camera_platform.c
index 1b7a88c..8b7ae85 100644
--- a/drivers/media/platform/soc_camera/soc_camera_platform.c
+++ b/drivers/media/platform/soc_camera/soc_camera_platform.c
@@ -54,7 +54,7 @@ static int soc_camera_platform_s_power(struct v4l2_subdev *sd, int on)
{
struct soc_camera_platform_info *p = v4l2_get_subdevdata(sd);
- return soc_camera_set_power(p->icd->control, &p->icd->sdesc->subdev_desc, on);
+ return soc_camera_set_power(p->icd->control, &p->icd->sdesc->subdev_desc, NULL, on);
}
static struct v4l2_subdev_core_ops platform_subdev_core_ops = {
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 64415ee..f8c1ffe 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -49,6 +49,7 @@ struct soc_camera_device {
/* soc_camera.c private count. Only accessed with .host_lock held */
int use_count;
struct file *streamer; /* stream owner */
+ struct v4l2_clk *clk;
union {
struct videobuf_queue vb_vidq;
struct vb2_queue vb2_vidq;
@@ -325,14 +326,16 @@ static inline void soc_camera_limit_side(int *start, int *length,
unsigned long soc_camera_apply_board_flags(struct soc_camera_subdev_desc *ssdd,
const struct v4l2_mbus_config *cfg);
-int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd);
-int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd);
+int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
+ struct v4l2_clk *clk);
+int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd,
+ struct v4l2_clk *clk);
static inline int soc_camera_set_power(struct device *dev,
- struct soc_camera_subdev_desc *ssdd, bool on)
+ struct soc_camera_subdev_desc *ssdd, struct v4l2_clk *clk, bool on)
{
- return on ? soc_camera_power_on(dev, ssdd)
- : soc_camera_power_off(dev, ssdd);
+ return on ? soc_camera_power_on(dev, ssdd, clk)
+ : soc_camera_power_off(dev, ssdd, clk);
}
/* This is only temporary here - until v4l2-subdev begins to link to video_device */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 4/7] soc-camera: add V4L2-async support
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (2 preceding siblings ...)
2013-04-08 15:05 ` [PATCH v8 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 5/7] sh_mobile_ceu_camera: add asynchronous subdevice probing support Guennadi Liakhovetski
` (4 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Add support for asynchronous subdevice probing, using the v4l2-async API.
The legacy synchronous mode is still supported too, which allows to
gradually update drivers and platforms. The selected approach adds a
notifier for each struct soc_camera_device instance, i.e. for each video
device node, even when there are multiple such instances registered with a
single soc-camera host simultaneously.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/media/platform/soc_camera/soc_camera.c | 526 ++++++++++++++++++++----
include/media/soc_camera.h | 23 +-
2 files changed, 463 insertions(+), 86 deletions(-)
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index cfcdabe..abc1b7e 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -21,22 +21,23 @@
#include <linux/i2c.h>
#include <linux/init.h>
#include <linux/list.h>
-#include <linux/mutex.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
-#include <linux/pm_runtime.h>
#include <linux/vmalloc.h>
#include <media/soc_camera.h>
+#include <media/soc_mediabus.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-clk.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-dev.h>
#include <media/videobuf-core.h>
#include <media/videobuf2-core.h>
-#include <media/soc_mediabus.h>
/* Default to VGA resolution */
#define DEFAULT_WIDTH 640
@@ -47,23 +48,39 @@
(icd)->vb_vidq.streaming : \
vb2_is_streaming(&(icd)->vb2_vidq))
+#define MAP_MAX_NUM 32
+static DECLARE_BITMAP(device_map, MAP_MAX_NUM);
static LIST_HEAD(hosts);
static LIST_HEAD(devices);
-static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
+/*
+ * Protects lists and bitmaps of hosts and devices.
+ * Lock nesting: Ok to take ->host_lock under list_lock.
+ */
+static DEFINE_MUTEX(list_lock);
+
+struct soc_camera_async_client {
+ struct v4l2_async_subdev *sensor;
+ struct v4l2_async_notifier notifier;
+ struct platform_device *pdev;
+ struct list_head list; /* needed for clean up */
+};
+
+static int soc_camera_video_start(struct soc_camera_device *icd);
+static int video_dev_create(struct soc_camera_device *icd);
int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
struct v4l2_clk *clk)
{
int ret = clk ? v4l2_clk_enable(clk) : 0;
if (ret < 0) {
- dev_err(dev, "Cannot enable clock\n");
+ dev_err(dev, "Cannot enable clock: %d\n", ret);
return ret;
}
ret = regulator_bulk_enable(ssdd->num_regulators,
ssdd->regulators);
if (ret < 0) {
dev_err(dev, "Cannot enable regulators\n");
- goto eregenable;;
+ goto eregenable;
}
if (ssdd->power) {
@@ -117,6 +134,14 @@ int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd
}
EXPORT_SYMBOL(soc_camera_power_off);
+int soc_camera_power_init(struct device *dev, struct soc_camera_subdev_desc *ssdd)
+{
+
+ return devm_regulator_bulk_get(dev, ssdd->num_regulators,
+ ssdd->regulators);
+}
+EXPORT_SYMBOL(soc_camera_power_init);
+
static int __soc_camera_power_on(struct soc_camera_device *icd)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
@@ -533,7 +558,9 @@ static int soc_camera_add_device(struct soc_camera_device *icd)
return -EBUSY;
if (!icd->clk) {
+ mutex_lock(&ici->clk_lock);
ret = ici->ops->clock_start(ici);
+ mutex_unlock(&ici->clk_lock);
if (ret < 0)
return ret;
}
@@ -549,8 +576,11 @@ static int soc_camera_add_device(struct soc_camera_device *icd)
return 0;
eadd:
- if (!icd->clk)
+ if (!icd->clk) {
+ mutex_lock(&ici->clk_lock);
ici->ops->clock_stop(ici);
+ mutex_unlock(&ici->clk_lock);
+ }
return ret;
}
@@ -563,8 +593,11 @@ static void soc_camera_remove_device(struct soc_camera_device *icd)
if (ici->ops->remove)
ici->ops->remove(icd);
- if (!icd->clk)
+ if (!icd->clk) {
+ mutex_lock(&ici->clk_lock);
ici->ops->clock_stop(ici);
+ mutex_unlock(&ici->clk_lock);
+ }
ici->icd = NULL;
}
@@ -673,8 +706,8 @@ static int soc_camera_open(struct file *file)
return 0;
/*
- * First four errors are entered with the .host_lock held
- * and use_count = 1
+ * All errors are entered with the .host_lock held, first four also
+ * with use_count = 1
*/
einitvb:
esfmt:
@@ -1128,7 +1161,8 @@ static int soc_camera_s_register(struct file *file, void *fh,
}
#endif
-static int soc_camera_probe(struct soc_camera_device *icd);
+static int soc_camera_probe(struct soc_camera_host *ici,
+ struct soc_camera_device *icd);
/* So far this function cannot fail */
static void scan_add_host(struct soc_camera_host *ici)
@@ -1137,12 +1171,20 @@ static void scan_add_host(struct soc_camera_host *ici)
mutex_lock(&list_lock);
- list_for_each_entry(icd, &devices, list) {
+ list_for_each_entry(icd, &devices, list)
if (icd->iface = ici->nr) {
+ struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
+ struct soc_camera_subdev_desc *ssdd = &sdesc->subdev_desc;
+
+ /* The camera could have been already on, try to reset */
+ if (ssdd->reset)
+ ssdd->reset(icd->pdev);
+
icd->parent = ici->v4l2_dev.dev;
- soc_camera_probe(icd);
+
+ /* Ignore errors */
+ soc_camera_probe(ici, icd);
}
- }
mutex_unlock(&list_lock);
}
@@ -1155,6 +1197,7 @@ static int soc_camera_clk_enable(struct v4l2_clk *clk)
{
struct soc_camera_device *icd = clk->priv;
struct soc_camera_host *ici;
+ int ret;
if (!icd || !icd->parent)
return -ENODEV;
@@ -1168,7 +1211,10 @@ static int soc_camera_clk_enable(struct v4l2_clk *clk)
* If a different client is currently being probed, the host will tell
* you to go
*/
- return ici->ops->clock_start(ici);
+ mutex_lock(&ici->clk_lock);
+ ret = ici->ops->clock_start(ici);
+ mutex_unlock(&ici->clk_lock);
+ return ret;
}
static void soc_camera_clk_disable(struct v4l2_clk *clk)
@@ -1181,7 +1227,9 @@ static void soc_camera_clk_disable(struct v4l2_clk *clk)
ici = to_soc_camera_host(icd->parent);
+ mutex_lock(&ici->clk_lock);
ici->ops->clock_stop(ici);
+ mutex_unlock(&ici->clk_lock);
module_put(ici->ops->owner);
}
@@ -1198,18 +1246,117 @@ static const struct v4l2_clk_ops soc_camera_clk_ops = {
.disable = soc_camera_clk_disable,
};
+static int soc_camera_dyn_pdev(struct soc_camera_desc *sdesc,
+ struct soc_camera_async_client *sasc)
+{
+ struct platform_device *pdev;
+ int ret, i;
+
+ mutex_lock(&list_lock);
+ i = find_first_zero_bit(device_map, MAP_MAX_NUM);
+ if (i < MAP_MAX_NUM)
+ set_bit(i, device_map);
+ mutex_unlock(&list_lock);
+ if (i >= MAP_MAX_NUM)
+ return -ENOMEM;
+
+ pdev = platform_device_alloc("soc-camera-pdrv", i);
+ if (!pdev)
+ return -ENOMEM;
+
+ ret = platform_device_add_data(pdev, sdesc, sizeof(*sdesc));
+ if (ret < 0) {
+ platform_device_put(pdev);
+ return ret;
+ }
+
+ sasc->pdev = pdev;
+
+ return 0;
+}
+
+static struct soc_camera_device *soc_camera_add_pdev(struct soc_camera_async_client *sasc)
+{
+ struct platform_device *pdev = sasc->pdev;
+ int ret;
+
+ ret = platform_device_add(pdev);
+ if (ret < 0 || !pdev->dev.driver)
+ return NULL;
+
+ return platform_get_drvdata(pdev);
+}
+
+/* Locking: called with .host_lock held */
+static int soc_camera_probe_finish(struct soc_camera_device *icd)
+{
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ struct v4l2_mbus_framefmt mf;
+ int ret;
+
+ sd->grp_id = soc_camera_grp_id(icd);
+ v4l2_set_subdev_hostdata(sd, icd);
+
+ ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL);
+ if (ret < 0)
+ return ret;
+
+ ret = soc_camera_add_device(icd);
+ if (ret < 0) {
+ dev_err(icd->pdev, "Couldn't activate the camera: %d\n", ret);
+ return ret;
+ }
+
+ /* At this point client .probe() should have run already */
+ ret = soc_camera_init_user_formats(icd);
+ if (ret < 0)
+ goto eusrfmt;
+
+ icd->field = V4L2_FIELD_ANY;
+
+ ret = soc_camera_video_start(icd);
+ if (ret < 0)
+ goto evidstart;
+
+ /* Try to improve our guess of a reasonable window format */
+ if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
+ icd->user_width = mf.width;
+ icd->user_height = mf.height;
+ icd->colorspace = mf.colorspace;
+ icd->field = mf.field;
+ }
+ soc_camera_remove_device(icd);
+
+ return 0;
+
+evidstart:
+ soc_camera_free_user_formats(icd);
+eusrfmt:
+ soc_camera_remove_device(icd);
+
+ return ret;
+}
+
#ifdef CONFIG_I2C_BOARDINFO
-static int soc_camera_init_i2c(struct soc_camera_device *icd,
+static int soc_camera_i2c_init(struct soc_camera_device *icd,
struct soc_camera_desc *sdesc)
{
struct i2c_client *client;
- struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+ struct soc_camera_host *ici;
struct soc_camera_host_desc *shd = &sdesc->host_desc;
- struct i2c_adapter *adap = i2c_get_adapter(shd->i2c_adapter_id);
+ struct i2c_adapter *adap;
struct v4l2_subdev *subdev;
char clk_name[V4L2_SUBDEV_NAME_SIZE];
int ret;
+ /* First find out how we link the main client */
+ if (icd->sasc) {
+ /* Async non-OF probing handled by the subdevice list */
+ return -EPROBE_DEFER;
+ }
+
+ ici = to_soc_camera_host(icd->parent);
+ adap = i2c_get_adapter(shd->i2c_adapter_id);
if (!adap) {
dev_err(icd->pdev, "Cannot get I2C adapter #%d. No driver?\n",
shd->i2c_adapter_id);
@@ -1242,42 +1389,201 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
return 0;
ei2cnd:
v4l2_clk_unregister(icd->clk);
- icd->clk = NULL;
eclkreg:
+ icd->clk = NULL;
i2c_put_adapter(adap);
return ret;
}
-static void soc_camera_free_i2c(struct soc_camera_device *icd)
+static void soc_camera_i2c_free(struct soc_camera_device *icd)
{
struct i2c_client *client to_i2c_client(to_soc_camera_control(icd));
- struct i2c_adapter *adap = client->adapter;
+ struct i2c_adapter *adap;
icd->control = NULL;
+ if (icd->sasc)
+ return;
+
+ adap = client->adapter;
v4l2_device_unregister_subdev(i2c_get_clientdata(client));
i2c_unregister_device(client);
i2c_put_adapter(adap);
v4l2_clk_unregister(icd->clk);
icd->clk = NULL;
}
+
+/*
+ * V4L2 asynchronous notifier callbacks. They are all called under a v4l2-async
+ * internal global mutex, therefore cannot race against other asynchronous
+ * events. Until notifier->complete() (soc_camera_async_complete()) is called,
+ * the video device node is not registered and no V4L fops can occur. Unloading
+ * of the host driver also calls a v4l2-async function, so also there we're
+ * protected.
+ */
+static int soc_camera_async_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl)
+{
+ struct soc_camera_async_client *sasc = container_of(notifier,
+ struct soc_camera_async_client, notifier);
+ struct soc_camera_device *icd = platform_get_drvdata(sasc->pdev);
+
+ if (asdl->asd = sasc->sensor && !WARN_ON(icd->control)) {
+ struct v4l2_subdev *sd = asdl->subdev;
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ /*
+ * Only now we get subdevice-specific information like
+ * regulators, flags, callbacks, etc.
+ */
+ if (client) {
+ struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
+ struct soc_camera_subdev_desc *ssdd + soc_camera_i2c_to_desc(client);
+ if (ssdd) {
+ memcpy(&sdesc->subdev_desc, ssdd,
+ sizeof(sdesc->subdev_desc));
+ if (ssdd->reset)
+ ssdd->reset(icd->pdev);
+ }
+
+ icd->control = &client->dev;
+ }
+ }
+
+ return 0;
+}
+
+static void soc_camera_async_unbind(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl)
+{
+ struct soc_camera_async_client *sasc = container_of(notifier,
+ struct soc_camera_async_client, notifier);
+ struct soc_camera_device *icd = platform_get_drvdata(sasc->pdev);
+
+ if (icd->clk) {
+ v4l2_clk_unregister(icd->clk);
+ icd->clk = NULL;
+ }
+}
+
+static int soc_camera_async_complete(struct v4l2_async_notifier *notifier)
+{
+ struct soc_camera_async_client *sasc = container_of(notifier,
+ struct soc_camera_async_client, notifier);
+ struct soc_camera_device *icd = platform_get_drvdata(sasc->pdev);
+
+ if (to_soc_camera_control(icd)) {
+ struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+ int ret;
+
+ mutex_lock(&list_lock);
+ ret = soc_camera_probe(ici, icd);
+ mutex_unlock(&list_lock);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int scan_async_group(struct soc_camera_host *ici,
+ struct v4l2_async_subdev **asd, int size)
+{
+ struct soc_camera_async_subdev *sasd;
+ struct soc_camera_async_client *sasc;
+ struct soc_camera_device *icd;
+ struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
+ char clk_name[V4L2_SUBDEV_NAME_SIZE];
+ int ret, i;
+
+ /* First look for a sensor */
+ for (i = 0; i < size; i++) {
+ sasd = container_of(asd[i], struct soc_camera_async_subdev, asd);
+ if (sasd->role = SOCAM_SUBDEV_DATA_SOURCE)
+ break;
+ }
+
+ if (i = size || asd[i]->hw.bus_type != V4L2_ASYNC_BUS_I2C) {
+ /* All useless */
+ dev_err(ici->v4l2_dev.dev, "No I2C data source found!\n");
+ return -ENODEV;
+ }
+
+ /* Or shall this be managed by the soc-camera device? */
+ sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
+ if (!sasc)
+ return -ENOMEM;
+
+ /* HACK: just need a != NULL */
+ sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
+
+ ret = soc_camera_dyn_pdev(&sdesc, sasc);
+ if (ret < 0)
+ return ret;
+
+ sasc->sensor = &sasd->asd;
+
+ icd = soc_camera_add_pdev(sasc);
+ if (!icd) {
+ platform_device_put(sasc->pdev);
+ return -ENOMEM;
+ }
+
+ sasc->notifier.subdev = asd;
+ sasc->notifier.subdev_num = size;
+ sasc->notifier.bound = soc_camera_async_bound;
+ sasc->notifier.unbind = soc_camera_async_unbind;
+ sasc->notifier.complete = soc_camera_async_complete;
+
+ icd->sasc = sasc;
+ icd->parent = ici->v4l2_dev.dev;
+
+ snprintf(clk_name, sizeof(clk_name), "%d-%04x",
+ sasd->asd.hw.match.i2c.adapter_id, sasd->asd.hw.match.i2c.address);
+
+ icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
+ if (IS_ERR(icd->clk)) {
+ ret = PTR_ERR(icd->clk);
+ goto eclkreg;
+ }
+
+ ret = v4l2_async_notifier_register(&ici->v4l2_dev, &sasc->notifier);
+ if (!ret)
+ return 0;
+
+ v4l2_clk_unregister(icd->clk);
+eclkreg:
+ icd->clk = NULL;
+ platform_device_unregister(sasc->pdev);
+ dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
+
+ return ret;
+}
+
+static void scan_async_host(struct soc_camera_host *ici)
+{
+ struct v4l2_async_subdev **asd;
+ int j;
+
+ for (j = 0, asd = ici->asd; ici->asd_sizes[j]; j++) {
+ scan_async_group(ici, asd, ici->asd_sizes[j]);
+ asd += ici->asd_sizes[j];
+ }
+}
#else
-#define soc_camera_init_i2c(icd, sdesc) (-ENODEV)
-#define soc_camera_free_i2c(icd) do {} while (0)
+#define soc_camera_i2c_init(icd, sdesc) (-ENODEV)
+#define soc_camera_i2c_free(icd) do {} while (0)
+#define scan_async_host(ici) do {} while (0)
#endif
-static int soc_camera_video_start(struct soc_camera_device *icd);
-static int video_dev_create(struct soc_camera_device *icd);
/* Called during host-driver probe */
-static int soc_camera_probe(struct soc_camera_device *icd)
+static int soc_camera_probe(struct soc_camera_host *ici,
+ struct soc_camera_device *icd)
{
- struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
struct soc_camera_host_desc *shd = &sdesc->host_desc;
- struct soc_camera_subdev_desc *ssdd = &sdesc->subdev_desc;
struct device *control = NULL;
- struct v4l2_subdev *sd;
- struct v4l2_mbus_framefmt mf;
int ret;
dev_info(icd->pdev, "Probing %s\n", dev_name(icd->pdev));
@@ -1293,10 +1599,6 @@ static int soc_camera_probe(struct soc_camera_device *icd)
if (ret < 0)
return ret;
- /* The camera could have been already on, try to reset */
- if (ssdd->reset)
- ssdd->reset(icd->pdev);
-
/* Must have icd->vdev before registering the device */
ret = video_dev_create(icd);
if (ret < 0)
@@ -1307,18 +1609,19 @@ static int soc_camera_probe(struct soc_camera_device *icd)
* itself is protected against concurrent open() calls, but we also have
* to protect our data also during client probing.
*/
- mutex_lock(&ici->host_lock);
/* Non-i2c cameras, e.g., soc_camera_platform, have no board_info */
if (shd->board_info) {
- ret = soc_camera_init_i2c(icd, sdesc);
- if (ret < 0)
+ ret = soc_camera_i2c_init(icd, sdesc);
+ if (ret < 0 && ret != -EPROBE_DEFER)
goto eadd;
} else if (!shd->add_device || !shd->del_device) {
ret = -EINVAL;
goto eadd;
} else {
+ mutex_lock(&ici->clk_lock);
ret = ici->ops->clock_start(ici);
+ mutex_unlock(&ici->clk_lock);
if (ret < 0)
goto eadd;
@@ -1342,57 +1645,33 @@ static int soc_camera_probe(struct soc_camera_device *icd)
}
}
- sd = soc_camera_to_subdev(icd);
- sd->grp_id = soc_camera_grp_id(icd);
- v4l2_set_subdev_hostdata(sd, icd);
-
- ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL);
- if (ret < 0)
- goto ectrl;
-
- /* At this point client .probe() should have run already */
- ret = soc_camera_init_user_formats(icd);
- if (ret < 0)
- goto eiufmt;
-
- icd->field = V4L2_FIELD_ANY;
-
- ret = soc_camera_video_start(icd);
- if (ret < 0)
- goto evidstart;
-
- /* Try to improve our guess of a reasonable window format */
- if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
- icd->user_width = mf.width;
- icd->user_height = mf.height;
- icd->colorspace = mf.colorspace;
- icd->field = mf.field;
- }
-
- if (!shd->board_info)
- ici->ops->clock_stop(ici);
-
+ mutex_lock(&ici->host_lock);
+ ret = soc_camera_probe_finish(icd);
mutex_unlock(&ici->host_lock);
+ if (ret < 0)
+ goto efinish;
return 0;
-evidstart:
- soc_camera_free_user_formats(icd);
-eiufmt:
-ectrl:
+efinish:
if (shd->board_info) {
- soc_camera_free_i2c(icd);
+ soc_camera_i2c_free(icd);
} else {
shd->del_device(icd);
module_put(control->driver->owner);
enodrv:
eadddev:
+ mutex_lock(&ici->clk_lock);
ici->ops->clock_stop(ici);
+ mutex_unlock(&ici->clk_lock);
}
eadd:
video_device_release(icd->vdev);
icd->vdev = NULL;
- mutex_unlock(&ici->host_lock);
+ if (icd->vdev) {
+ video_device_release(icd->vdev);
+ icd->vdev = NULL;
+ }
evdc:
v4l2_ctrl_handler_free(&icd->ctrl_handler);
return ret;
@@ -1400,15 +1679,15 @@ evdc:
/*
* This is called on device_unregister, which only means we have to disconnect
- * from the host, but not remove ourselves from the device list
+ * from the host, but not remove ourselves from the device list. With
+ * asynchronous client probing this can also be called without
+ * soc_camera_probe_finish() having run. Careful with clean up.
*/
static int soc_camera_remove(struct soc_camera_device *icd)
{
struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
struct video_device *vdev = icd->vdev;
- BUG_ON(!icd->parent);
-
v4l2_ctrl_handler_free(&icd->ctrl_handler);
if (vdev) {
video_unregister_device(vdev);
@@ -1416,15 +1695,27 @@ static int soc_camera_remove(struct soc_camera_device *icd)
}
if (sdesc->host_desc.board_info) {
- soc_camera_free_i2c(icd);
+ soc_camera_i2c_free(icd);
} else {
- struct device_driver *drv = to_soc_camera_control(icd)->driver;
+ struct device *dev = to_soc_camera_control(icd);
+ struct device_driver *drv = dev ? dev->driver : NULL;
if (drv) {
sdesc->host_desc.del_device(icd);
module_put(drv->owner);
}
}
- soc_camera_free_user_formats(icd);
+
+ if (icd->num_user_formats)
+ soc_camera_free_user_formats(icd);
+
+ if (icd->clk) {
+ /* For the synchronous case */
+ v4l2_clk_unregister(icd->clk);
+ icd->clk = NULL;
+ }
+
+ if (icd->sasc)
+ platform_device_unregister(icd->sasc->pdev);
return 0;
}
@@ -1535,7 +1826,18 @@ int soc_camera_host_register(struct soc_camera_host *ici)
mutex_unlock(&list_lock);
mutex_init(&ici->host_lock);
- scan_add_host(ici);
+ mutex_init(&ici->clk_lock);
+
+ if (ici->asd_sizes)
+ /*
+ * No OF, host with a list of subdevices. Don't try to mix
+ * modes by initialising some groups statically and some
+ * dynamically!
+ */
+ scan_async_host(ici);
+ else
+ /* Legacy: static platform devices from board data */
+ scan_add_host(ici);
return 0;
@@ -1548,13 +1850,30 @@ EXPORT_SYMBOL(soc_camera_host_register);
/* Unregister all clients! */
void soc_camera_host_unregister(struct soc_camera_host *ici)
{
- struct soc_camera_device *icd;
+ struct soc_camera_device *icd, *tmp;
+ struct soc_camera_async_client *sasc;
+ LIST_HEAD(notifiers);
mutex_lock(&list_lock);
-
list_del(&ici->list);
list_for_each_entry(icd, &devices, list)
- if (icd->iface = ici->nr && to_soc_camera_control(icd))
+ if (icd->iface = ici->nr && icd->sasc) {
+ /* as long as we hold the device, sasc won't be freed */
+ get_device(icd->pdev);
+ list_add(&icd->sasc->list, ¬ifiers);
+ }
+ mutex_unlock(&list_lock);
+
+ list_for_each_entry(sasc, ¬ifiers, list) {
+ /* Must call unlocked to avoid AB-BA dead-lock */
+ v4l2_async_notifier_unregister(&sasc->notifier);
+ put_device(&sasc->pdev->dev);
+ }
+
+ mutex_lock(&list_lock);
+
+ list_for_each_entry_safe(icd, tmp, &devices, list)
+ if (icd->iface = ici->nr)
soc_camera_remove(icd);
mutex_unlock(&list_lock);
@@ -1569,6 +1888,7 @@ static int soc_camera_device_register(struct soc_camera_device *icd)
struct soc_camera_device *ix;
int num = -1, i;
+ mutex_lock(&list_lock);
for (i = 0; i < 256 && num < 0; i++) {
num = i;
/* Check if this index is available on this interface */
@@ -1580,18 +1900,34 @@ static int soc_camera_device_register(struct soc_camera_device *icd)
}
}
- if (num < 0)
+ if (num < 0) {
/*
* ok, we have 256 cameras on this host...
* man, stay reasonable...
*/
+ mutex_unlock(&list_lock);
return -ENOMEM;
+ }
icd->devnum = num;
icd->use_count = 0;
icd->host_priv = NULL;
+ /*
+ * Dynamically allocated devices set the bit earlier, but it doesn't hurt setting
+ * it again
+ */
+ i = to_platform_device(icd->pdev)->id;
+ if (i < 0)
+ /* One static (legacy) soc-camera platform device */
+ i = 0;
+ if (i >= MAP_MAX_NUM) {
+ mutex_unlock(&list_lock);
+ return -EBUSY;
+ }
+ set_bit(i, device_map);
list_add_tail(&icd->list, &devices);
+ mutex_unlock(&list_lock);
return 0;
}
@@ -1691,6 +2027,12 @@ static int soc_camera_pdrv_probe(struct platform_device *pdev)
if (!icd)
return -ENOMEM;
+ /*
+ * In the asynchronous case ssdd->num_regulators = 0 yet, so, the below
+ * regulator allocation is a dummy. They will be really requested later
+ * in soc_camera_async_bind(). Also note, that in that case regulators
+ * are attached to the I2C device and not to the camera platform device.
+ */
ret = devm_regulator_bulk_get(&pdev->dev, ssdd->num_regulators,
ssdd->regulators);
if (ret < 0)
@@ -1715,11 +2057,25 @@ static int soc_camera_pdrv_probe(struct platform_device *pdev)
static int soc_camera_pdrv_remove(struct platform_device *pdev)
{
struct soc_camera_device *icd = platform_get_drvdata(pdev);
+ int i;
if (!icd)
return -EINVAL;
- list_del(&icd->list);
+ i = pdev->id;
+ if (i < 0)
+ i = 0;
+
+ /*
+ * In synchronous mode with static platform devices this is called in a
+ * loop from drivers/base/dd.c::driver_detach(), no parallel execution,
+ * no need to lock. In asynchronous case the caller -
+ * soc_camera_host_unregister() - already holds the lock
+ */
+ if (test_bit(i, device_map)) {
+ clear_bit(i, device_map);
+ list_del(&icd->list);
+ }
return 0;
}
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index f8c1ffe..2d3c939 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -19,11 +19,13 @@
#include <linux/videodev2.h>
#include <media/videobuf-core.h>
#include <media/videobuf2-core.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
struct file;
struct soc_camera_desc;
+struct soc_camera_async_client;
struct soc_camera_device {
struct list_head list; /* list of all registered devices */
@@ -50,6 +52,9 @@ struct soc_camera_device {
int use_count;
struct file *streamer; /* stream owner */
struct v4l2_clk *clk;
+ /* Asynchronous subdevice management */
+ struct soc_camera_async_client *sasc;
+ /* video buffer queue */
union {
struct videobuf_queue vb_vidq;
struct vb2_queue vb2_vidq;
@@ -59,16 +64,30 @@ struct soc_camera_device {
/* Host supports programmable stride */
#define SOCAM_HOST_CAP_STRIDE (1 << 0)
+enum soc_camera_subdev_role {
+ SOCAM_SUBDEV_DATA_SOURCE = 1,
+ SOCAM_SUBDEV_DATA_SINK,
+ SOCAM_SUBDEV_DATA_PROCESSOR,
+};
+
+struct soc_camera_async_subdev {
+ struct v4l2_async_subdev asd;
+ enum soc_camera_subdev_role role;
+};
+
struct soc_camera_host {
struct v4l2_device v4l2_dev;
struct list_head list;
- struct mutex host_lock; /* Protect pipeline modifications */
+ struct mutex host_lock; /* Main synchronisation lock */
+ struct mutex clk_lock; /* Protect pipeline modifications */
unsigned char nr; /* Host number */
u32 capabilities;
struct soc_camera_device *icd; /* Currently attached client */
void *priv;
const char *drv_name;
struct soc_camera_host_ops *ops;
+ struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
+ int *asd_sizes; /* 0-terminated array of asd group sizes */
};
struct soc_camera_host_ops {
@@ -161,6 +180,7 @@ struct soc_camera_host_desc {
};
/*
+ * Platform data for "soc-camera-pdrv"
* This MUST be kept binary-identical to struct soc_camera_link below, until
* it is completely replaced by this one, after which we can split it into its
* two components.
@@ -326,6 +346,7 @@ static inline void soc_camera_limit_side(int *start, int *length,
unsigned long soc_camera_apply_board_flags(struct soc_camera_subdev_desc *ssdd,
const struct v4l2_mbus_config *cfg);
+int soc_camera_power_init(struct device *dev, struct soc_camera_subdev_desc *ssdd);
int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
struct v4l2_clk *clk);
int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 5/7] sh_mobile_ceu_camera: add asynchronous subdevice probing support
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (3 preceding siblings ...)
2013-04-08 15:05 ` [PATCH v8 4/7] soc-camera: add V4L2-async support Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 6/7] imx074: support asynchronous probing Guennadi Liakhovetski
` (3 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Use the v4l2-async API to support asynchronous subdevice probing,
including the CSI2 subdevice. Synchronous probing is still supported too.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
.../platform/soc_camera/sh_mobile_ceu_camera.c | 134 ++++++++++++-----
drivers/media/platform/soc_camera/sh_mobile_csi2.c | 163 +++++++++++--------
include/media/sh_mobile_ceu.h | 2 +
include/media/sh_mobile_csi2.h | 2 +-
4 files changed, 194 insertions(+), 107 deletions(-)
diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index b0f0995..99d9029 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -36,6 +36,7 @@
#include <linux/pm_runtime.h>
#include <linux/sched.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-common.h>
#include <media/v4l2-dev.h>
#include <media/soc_camera.h>
@@ -96,6 +97,10 @@ struct sh_mobile_ceu_buffer {
struct sh_mobile_ceu_dev {
struct soc_camera_host ici;
+ /* Asynchronous CSI2 linking */
+ struct v4l2_async_subdev *csi2_asd;
+ struct v4l2_subdev *csi2_sd;
+ /* Synchronous probing compatibility */
struct platform_device *csi2_pdev;
unsigned int irq;
@@ -185,7 +190,6 @@ static int sh_mobile_ceu_soft_reset(struct sh_mobile_ceu_dev *pcdev)
udelay(1);
}
-
if (2 != success) {
dev_warn(pcdev->ici.v4l2_dev.dev, "soft reset time out\n");
return -EIO;
@@ -534,16 +538,29 @@ static struct v4l2_subdev *find_csi2(struct sh_mobile_ceu_dev *pcdev)
{
struct v4l2_subdev *sd;
- if (!pcdev->csi2_pdev)
- return NULL;
+ if (pcdev->csi2_sd)
+ return pcdev->csi2_sd;
- v4l2_device_for_each_subdev(sd, &pcdev->ici.v4l2_dev)
- if (&pcdev->csi2_pdev->dev = v4l2_get_subdevdata(sd))
- return sd;
+ if (pcdev->csi2_asd) {
+ char name[] = "sh-mobile-csi2";
+ v4l2_device_for_each_subdev(sd, &pcdev->ici.v4l2_dev)
+ if (!strncmp(name, sd->name, sizeof(name) - 1)) {
+ pcdev->csi2_sd = sd;
+ return sd;
+ }
+ }
return NULL;
}
+static struct v4l2_subdev *csi2_subdev(struct sh_mobile_ceu_dev *pcdev,
+ struct soc_camera_device *icd)
+{
+ struct v4l2_subdev *sd = pcdev->csi2_sd;
+
+ return sd && sd->grp_id = soc_camera_grp_id(icd) ? sd : NULL;
+}
+
static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
{
struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
@@ -564,12 +581,12 @@ static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
* -ENODEV is special: either csi2_sd = NULL or the CSI-2 driver
* has not found this soc-camera device among its clients
*/
- if (ret = -ENODEV && csi2_sd)
+ if (csi2_sd && ret = -ENODEV)
csi2_sd->grp_id = 0;
dev_info(icd->parent,
- "SuperH Mobile CEU driver attached to camera %d\n",
- icd->devnum);
+ "SuperH Mobile CEU%s driver attached to camera %d\n",
+ csi2_sd && csi2_sd->grp_id ? "/CSI-2" : "", icd->devnum);
return 0;
}
@@ -585,8 +602,6 @@ static void sh_mobile_ceu_remove_device(struct soc_camera_device *icd)
icd->devnum);
v4l2_subdev_call(csi2_sd, core, s_power, 0);
- if (csi2_sd)
- csi2_sd->grp_id = 0;
}
/* Called with .host_lock held */
@@ -708,7 +723,7 @@ static void sh_mobile_ceu_set_rect(struct soc_camera_device *icd)
}
/* CSI2 special configuration */
- if (pcdev->csi2_pdev) {
+ if (csi2_subdev(pcdev, icd)) {
in_width = ((in_width - 2) * 2);
left_offset *= 2;
}
@@ -765,13 +780,7 @@ static void capture_restore(struct sh_mobile_ceu_dev *pcdev, u32 capsr)
static struct v4l2_subdev *find_bus_subdev(struct sh_mobile_ceu_dev *pcdev,
struct soc_camera_device *icd)
{
- if (pcdev->csi2_pdev) {
- struct v4l2_subdev *csi2_sd = find_csi2(pcdev);
- if (csi2_sd && csi2_sd->grp_id = soc_camera_grp_id(icd))
- return csi2_sd;
- }
-
- return soc_camera_to_subdev(icd);
+ return csi2_subdev(pcdev, icd) ? : soc_camera_to_subdev(icd);
}
#define CEU_BUS_FLAGS (V4L2_MBUS_MASTER | \
@@ -875,7 +884,7 @@ static int sh_mobile_ceu_set_bus_param(struct soc_camera_device *icd)
value |= common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW ? 1 << 1 : 0;
value |= common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW ? 1 << 0 : 0;
- if (pcdev->csi2_pdev) /* CSI2 mode */
+ if (csi2_subdev(pcdev, icd)) /* CSI2 mode */
value |= 3 << 12;
else if (pcdev->is_16bit)
value |= 1 << 12;
@@ -1054,7 +1063,7 @@ static int sh_mobile_ceu_get_formats(struct soc_camera_device *icd, unsigned int
return 0;
}
- if (!pcdev->pdata || !pcdev->pdata->csi2) {
+ if (!csi2_subdev(pcdev, icd)) {
/* Are there any restrictions in the CSI-2 case? */
ret = sh_mobile_ceu_try_bus_param(icd, fmt->bits_per_sample);
if (ret < 0)
@@ -2084,7 +2093,7 @@ static int sh_mobile_ceu_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *base;
unsigned int irq;
- int err = 0;
+ int err, i;
struct bus_wait wait = {
.completion = COMPLETION_INITIALIZER_ONSTACK(wait.completion),
.notifier.notifier_call = bus_notify,
@@ -2188,31 +2197,60 @@ static int sh_mobile_ceu_probe(struct platform_device *pdev)
goto exit_free_clk;
}
- err = soc_camera_host_register(&pcdev->ici);
- if (err)
- goto exit_free_ctx;
+ if (pcdev->pdata && pcdev->pdata->asd_sizes) {
+ struct v4l2_async_subdev **asd;
+ char name[] = "sh-mobile-csi2";
+ int j;
+
+ /*
+ * CSI2 interfacing: several groups can use CSI2, pick up the
+ * first one
+ */
+ asd = pcdev->pdata->asd;
+ for (j = 0; pcdev->pdata->asd_sizes[j]; j++) {
+ for (i = 0; i < pcdev->pdata->asd_sizes[j]; i++, asd++) {
+ dev_dbg(&pdev->dev, "%s(): subdev #%d, type %u\n",
+ __func__, i, (*asd)->hw.bus_type);
+ if ((*asd)->hw.bus_type = V4L2_ASYNC_BUS_PLATFORM &&
+ !strncmp(name, (*asd)->hw.match.platform.name,
+ sizeof(name) - 1)) {
+ pcdev->csi2_asd = *asd;
+ break;
+ }
+ }
+ if (pcdev->csi2_asd)
+ break;
+ }
+
+ pcdev->ici.asd = pcdev->pdata->asd;
+ pcdev->ici.asd_sizes = pcdev->pdata->asd_sizes;
+ }
- /* CSI2 interfacing */
+ /* Legacy CSI2 interfacing */
csi2 = pcdev->pdata ? pcdev->pdata->csi2 : NULL;
if (csi2) {
+ /*
+ * TODO: remove this once all users are converted to
+ * asynchronous CSI2 probing. If it has to be kept, csi2
+ * platform device resources have to be added, using
+ * platform_device_add_resources()
+ */
struct platform_device *csi2_pdev platform_device_alloc("sh-mobile-csi2", csi2->id);
struct sh_csi2_pdata *csi2_pdata = csi2->platform_data;
if (!csi2_pdev) {
err = -ENOMEM;
- goto exit_host_unregister;
+ goto exit_free_ctx;
}
pcdev->csi2_pdev = csi2_pdev;
- err = platform_device_add_data(csi2_pdev, csi2_pdata, sizeof(*csi2_pdata));
+ err = platform_device_add_data(csi2_pdev, csi2_pdata,
+ sizeof(*csi2_pdata));
if (err < 0)
goto exit_pdev_put;
- csi2_pdata = csi2_pdev->dev.platform_data;
- csi2_pdata->v4l2_dev = &pcdev->ici.v4l2_dev;
-
csi2_pdev->resource = csi2->resource;
csi2_pdev->num_resources = csi2->num_resources;
@@ -2254,17 +2292,38 @@ static int sh_mobile_ceu_probe(struct platform_device *pdev)
err = -ENODEV;
goto exit_pdev_unregister;
}
+
+ pcdev->csi2_sd = platform_get_drvdata(csi2_pdev);
+ }
+
+ err = soc_camera_host_register(&pcdev->ici);
+ if (err)
+ goto exit_csi2_unregister;
+
+ if (csi2) {
+ err = v4l2_device_register_subdev(&pcdev->ici.v4l2_dev,
+ pcdev->csi2_sd);
+ dev_dbg(&pdev->dev, "%s(): ret(register_subdev) = %d\n",
+ __func__, err);
+ if (err < 0)
+ goto exit_host_unregister;
+ /* v4l2_device_register_subdev() took a reference too */
+ module_put(pcdev->csi2_sd->owner);
}
return 0;
-exit_pdev_unregister:
- platform_device_del(pcdev->csi2_pdev);
-exit_pdev_put:
- pcdev->csi2_pdev->resource = NULL;
- platform_device_put(pcdev->csi2_pdev);
exit_host_unregister:
soc_camera_host_unregister(&pcdev->ici);
+exit_csi2_unregister:
+ if (csi2) {
+ module_put(pcdev->csi2_pdev->dev.driver->owner);
+exit_pdev_unregister:
+ platform_device_del(pcdev->csi2_pdev);
+exit_pdev_put:
+ pcdev->csi2_pdev->resource = NULL;
+ platform_device_put(pcdev->csi2_pdev);
+ }
exit_free_ctx:
vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
exit_free_clk:
@@ -2324,6 +2383,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_ceu_of_match);
static struct platform_driver sh_mobile_ceu_driver = {
.driver = {
.name = "sh_mobile_ceu",
+ .owner = THIS_MODULE,
.pm = &sh_mobile_ceu_dev_pm_ops,
.of_match_table = sh_mobile_ceu_of_match,
},
@@ -2349,5 +2409,5 @@ module_exit(sh_mobile_ceu_exit);
MODULE_DESCRIPTION("SuperH Mobile CEU driver");
MODULE_AUTHOR("Magnus Damm");
MODULE_LICENSE("GPL");
-MODULE_VERSION("0.0.6");
+MODULE_VERSION("0.1.0");
MODULE_ALIAS("platform:sh_mobile_ceu");
diff --git a/drivers/media/platform/soc_camera/sh_mobile_csi2.c b/drivers/media/platform/soc_camera/sh_mobile_csi2.c
index 09cb4fc..1764b34 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_csi2.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_csi2.c
@@ -36,14 +36,16 @@
struct sh_csi2 {
struct v4l2_subdev subdev;
- struct list_head list;
unsigned int irq;
unsigned long mipi_flags;
void __iomem *base;
struct platform_device *pdev;
struct sh_csi2_client_config *client;
+ struct v4l2_async_subdev_list asdl;
};
+static void sh_csi2_hwinit(struct sh_csi2 *priv);
+
static int sh_csi2_try_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *mf)
{
@@ -132,10 +134,58 @@ static int sh_csi2_s_fmt(struct v4l2_subdev *sd,
static int sh_csi2_g_mbus_config(struct v4l2_subdev *sd,
struct v4l2_mbus_config *cfg)
{
- cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING |
- V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
- V4L2_MBUS_MASTER | V4L2_MBUS_DATA_ACTIVE_HIGH;
- cfg->type = V4L2_MBUS_PARALLEL;
+ struct sh_csi2 *priv = container_of(sd, struct sh_csi2, subdev);
+
+ if (!priv->mipi_flags) {
+ struct soc_camera_device *icd = v4l2_get_subdev_hostdata(sd);
+ struct v4l2_subdev *client_sd = soc_camera_to_subdev(icd);
+ struct sh_csi2_pdata *pdata = priv->pdev->dev.platform_data;
+ unsigned long common_flags, csi2_flags;
+ struct v4l2_mbus_config client_cfg = {.type = V4L2_MBUS_CSI2,};
+ int ret;
+
+ /* Check if we can support this camera */
+ csi2_flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK |
+ V4L2_MBUS_CSI2_1_LANE;
+
+ switch (pdata->type) {
+ case SH_CSI2C:
+ if (priv->client->lanes != 1)
+ csi2_flags |= V4L2_MBUS_CSI2_2_LANE;
+ break;
+ case SH_CSI2I:
+ switch (priv->client->lanes) {
+ default:
+ csi2_flags |= V4L2_MBUS_CSI2_4_LANE;
+ case 3:
+ csi2_flags |= V4L2_MBUS_CSI2_3_LANE;
+ case 2:
+ csi2_flags |= V4L2_MBUS_CSI2_2_LANE;
+ }
+ }
+
+ ret = v4l2_subdev_call(client_sd, video, g_mbus_config, &client_cfg);
+ if (ret = -ENOIOCTLCMD)
+ common_flags = csi2_flags;
+ else if (!ret)
+ common_flags = soc_mbus_config_compatible(&client_cfg,
+ csi2_flags);
+ else
+ common_flags = 0;
+
+ if (!common_flags)
+ return -EINVAL;
+
+ /* All good: camera MIPI configuration supported */
+ priv->mipi_flags = common_flags;
+ }
+
+ if (cfg) {
+ cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING |
+ V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
+ V4L2_MBUS_MASTER | V4L2_MBUS_DATA_ACTIVE_HIGH;
+ cfg->type = V4L2_MBUS_PARALLEL;
+ }
return 0;
}
@@ -146,8 +196,17 @@ static int sh_csi2_s_mbus_config(struct v4l2_subdev *sd,
struct sh_csi2 *priv = container_of(sd, struct sh_csi2, subdev);
struct soc_camera_device *icd = v4l2_get_subdev_hostdata(sd);
struct v4l2_subdev *client_sd = soc_camera_to_subdev(icd);
- struct v4l2_mbus_config client_cfg = {.type = V4L2_MBUS_CSI2,
- .flags = priv->mipi_flags};
+ struct v4l2_mbus_config client_cfg = {.type = V4L2_MBUS_CSI2,};
+ int ret = sh_csi2_g_mbus_config(sd, NULL);
+
+ if (ret < 0)
+ return ret;
+
+ pm_runtime_get_sync(&priv->pdev->dev);
+
+ sh_csi2_hwinit(priv);
+
+ client_cfg.flags = priv->mipi_flags;
return v4l2_subdev_call(client_sd, video, s_mbus_config, &client_cfg);
}
@@ -202,19 +261,19 @@ static void sh_csi2_hwinit(struct sh_csi2 *priv)
static int sh_csi2_client_connect(struct sh_csi2 *priv)
{
- struct sh_csi2_pdata *pdata = priv->pdev->dev.platform_data;
- struct soc_camera_device *icd = v4l2_get_subdev_hostdata(&priv->subdev);
- struct v4l2_subdev *client_sd = soc_camera_to_subdev(icd);
struct device *dev = v4l2_get_subdevdata(&priv->subdev);
- struct v4l2_mbus_config cfg;
- unsigned long common_flags, csi2_flags;
- int i, ret;
+ struct sh_csi2_pdata *pdata = dev->platform_data;
+ struct soc_camera_device *icd = v4l2_get_subdev_hostdata(&priv->subdev);
+ int i;
if (priv->client)
return -EBUSY;
for (i = 0; i < pdata->num_clients; i++)
- if (&pdata->clients[i].pdev->dev = icd->pdev)
+ if ((pdata->clients[i].pdev &&
+ &pdata->clients[i].pdev->dev = icd->pdev) ||
+ (icd->control &&
+ strcmp(pdata->clients[i].name, dev_name(icd->control))))
break;
dev_dbg(dev, "%s(%p): found #%d\n", __func__, dev, i);
@@ -222,46 +281,8 @@ static int sh_csi2_client_connect(struct sh_csi2 *priv)
if (i = pdata->num_clients)
return -ENODEV;
- /* Check if we can support this camera */
- csi2_flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK | V4L2_MBUS_CSI2_1_LANE;
-
- switch (pdata->type) {
- case SH_CSI2C:
- if (pdata->clients[i].lanes != 1)
- csi2_flags |= V4L2_MBUS_CSI2_2_LANE;
- break;
- case SH_CSI2I:
- switch (pdata->clients[i].lanes) {
- default:
- csi2_flags |= V4L2_MBUS_CSI2_4_LANE;
- case 3:
- csi2_flags |= V4L2_MBUS_CSI2_3_LANE;
- case 2:
- csi2_flags |= V4L2_MBUS_CSI2_2_LANE;
- }
- }
-
- cfg.type = V4L2_MBUS_CSI2;
- ret = v4l2_subdev_call(client_sd, video, g_mbus_config, &cfg);
- if (ret = -ENOIOCTLCMD)
- common_flags = csi2_flags;
- else if (!ret)
- common_flags = soc_mbus_config_compatible(&cfg,
- csi2_flags);
- else
- common_flags = 0;
-
- if (!common_flags)
- return -EINVAL;
-
- /* All good: camera MIPI configuration supported */
- priv->mipi_flags = common_flags;
priv->client = pdata->clients + i;
- pm_runtime_get_sync(dev);
-
- sh_csi2_hwinit(priv);
-
return 0;
}
@@ -304,11 +325,21 @@ static int sh_csi2_probe(struct platform_device *pdev)
/* Platform data specify the PHY, lanes, ECC, CRC */
struct sh_csi2_pdata *pdata = pdev->dev.platform_data;
+ if (!pdata)
+ return -EINVAL;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(struct sh_csi2), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->asdl.subdev = &priv->subdev;
+ priv->asdl.dev = &pdev->dev;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
/* Interrupt unused so far */
irq = platform_get_irq(pdev, 0);
- if (!res || (int)irq <= 0 || !pdata) {
+ if (!res || (int)irq <= 0) {
dev_err(&pdev->dev, "Not enough CSI2 platform resources.\n");
return -ENODEV;
}
@@ -319,10 +350,6 @@ static int sh_csi2_probe(struct platform_device *pdev)
return -EINVAL;
}
- priv = devm_kzalloc(&pdev->dev, sizeof(struct sh_csi2), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
priv->irq = irq;
priv->base = devm_ioremap_resource(&pdev->dev, res);
@@ -330,35 +357,33 @@ static int sh_csi2_probe(struct platform_device *pdev)
return PTR_ERR(priv->base);
priv->pdev = pdev;
- platform_set_drvdata(pdev, priv);
+ priv->subdev.owner = THIS_MODULE;
+ platform_set_drvdata(pdev, &priv->subdev);
v4l2_subdev_init(&priv->subdev, &sh_csi2_subdev_ops);
v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.mipi-csi",
- dev_name(pdata->v4l2_dev->dev));
- ret = v4l2_device_register_subdev(pdata->v4l2_dev, &priv->subdev);
- dev_dbg(&pdev->dev, "%s(%p): ret(register_subdev) = %d\n", __func__, priv, ret);
+ dev_name(&pdev->dev));
+
+ ret = v4l2_async_register_subdev(&priv->asdl);
if (ret < 0)
- goto esdreg;
+ return ret;
pm_runtime_enable(&pdev->dev);
dev_dbg(&pdev->dev, "CSI2 probed.\n");
return 0;
-
-esdreg:
- platform_set_drvdata(pdev, NULL);
-
- return ret;
}
static int sh_csi2_remove(struct platform_device *pdev)
{
- struct sh_csi2 *priv = platform_get_drvdata(pdev);
+ struct v4l2_subdev *subdev = platform_get_drvdata(pdev);
+ struct sh_csi2 *priv = container_of(subdev, struct sh_csi2, subdev);
- v4l2_device_unregister_subdev(&priv->subdev);
+ v4l2_async_unregister_subdev(&priv->asdl);
+ v4l2_device_unregister_subdev(subdev);
pm_runtime_disable(&pdev->dev);
platform_set_drvdata(pdev, NULL);
diff --git a/include/media/sh_mobile_ceu.h b/include/media/sh_mobile_ceu.h
index 6fdb6ad..8937241 100644
--- a/include/media/sh_mobile_ceu.h
+++ b/include/media/sh_mobile_ceu.h
@@ -22,6 +22,8 @@ struct sh_mobile_ceu_info {
int max_width;
int max_height;
struct sh_mobile_ceu_companion *csi2;
+ struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
+ int *asd_sizes; /* 0-terminated array pf asd group sizes */
};
#endif /* __ASM_SH_MOBILE_CEU_H__ */
diff --git a/include/media/sh_mobile_csi2.h b/include/media/sh_mobile_csi2.h
index c586c4f..14030db 100644
--- a/include/media/sh_mobile_csi2.h
+++ b/include/media/sh_mobile_csi2.h
@@ -33,6 +33,7 @@ struct sh_csi2_client_config {
unsigned char lanes; /* bitmask[3:0] */
unsigned char channel; /* 0..3 */
struct platform_device *pdev; /* client platform device */
+ const char *name; /* async matching: client name */
};
struct v4l2_device;
@@ -42,7 +43,6 @@ struct sh_csi2_pdata {
unsigned int flags;
struct sh_csi2_client_config *clients;
int num_clients;
- struct v4l2_device *v4l2_dev;
};
#endif
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 6/7] imx074: support asynchronous probing
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (4 preceding siblings ...)
2013-04-08 15:05 ` [PATCH v8 5/7] sh_mobile_ceu_camera: add asynchronous subdevice probing support Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-08 15:05 ` [PATCH v8 7/7] ARM: shmobile: convert ap4evb to asynchronously register camera subdevices Guennadi Liakhovetski
` (2 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Both synchronous and asynchronous imx074 subdevice probing is supported by
this patch.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/media/i2c/soc_camera/imx074.c | 24 +++++++++++++++++++++---
1 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
index a6a5060..624d7c7 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <media/soc_camera.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-clk.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
@@ -79,6 +80,7 @@ struct imx074 {
struct v4l2_subdev subdev;
const struct imx074_datafmt *fmt;
struct v4l2_clk *clk;
+ struct v4l2_async_subdev_list asdl;
};
static const struct imx074_datafmt imx074_colour_fmts[] = {
@@ -455,14 +457,28 @@ static int imx074_probe(struct i2c_client *client,
priv->fmt = &imx074_colour_fmts[0];
+ priv->asdl.subdev = &priv->subdev;
+ priv->asdl.dev = &client->dev;
+
priv->clk = v4l2_clk_get(&client->dev, "mclk");
- if (IS_ERR(priv->clk))
- return PTR_ERR(priv->clk);
+ if (IS_ERR(priv->clk)) {
+ dev_info(&client->dev, "Error %ld getting clock\n", PTR_ERR(priv->clk));
+ return -EPROBE_DEFER;
+ }
+
+ ret = soc_camera_power_init(&client->dev, ssdd);
+ if (ret < 0)
+ goto epwrinit;
ret = imx074_video_probe(client);
if (ret < 0)
- v4l2_clk_put(priv->clk);
+ goto eprobe;
+ return v4l2_async_register_subdev(&priv->asdl);
+
+epwrinit:
+eprobe:
+ v4l2_clk_put(priv->clk);
return ret;
}
@@ -471,7 +487,9 @@ static int imx074_remove(struct i2c_client *client)
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
struct imx074 *priv = to_imx074(client);
+ v4l2_async_unregister_subdev(&priv->asdl);
v4l2_clk_put(priv->clk);
+
if (ssdd->free_bus)
ssdd->free_bus(ssdd);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 7/7] ARM: shmobile: convert ap4evb to asynchronously register camera subdevices
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (5 preceding siblings ...)
2013-04-08 15:05 ` [PATCH v8 6/7] imx074: support asynchronous probing Guennadi Liakhovetski
@ 2013-04-08 15:05 ` Guennadi Liakhovetski
2013-04-09 8:07 ` [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Simon Horman
2013-04-11 9:59 ` Guennadi Liakhovetski
8 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-08 15:05 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad, Guennadi Liakhovetski
Register the imx074 camera I2C and the CSI-2 platform devices directly
in board platform data instead of letting the sh_mobile_ceu_camera driver
and the soc-camera framework register them at their run-time. This uses
the V4L2 asynchronous subdevice probing capability.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
arch/arm/mach-shmobile/board-ap4evb.c | 103 +++++++++++++++++++-------------
arch/arm/mach-shmobile/clock-sh7372.c | 1 +
2 files changed, 62 insertions(+), 42 deletions(-)
diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index 38f1259..450e06b 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -50,6 +50,7 @@
#include <media/sh_mobile_ceu.h>
#include <media/sh_mobile_csi2.h>
#include <media/soc_camera.h>
+#include <media/v4l2-async.h>
#include <sound/sh_fsi.h>
#include <sound/simple_card.h>
@@ -871,22 +872,32 @@ static struct platform_device leds_device = {
},
};
-static struct i2c_board_info imx074_info = {
- I2C_BOARD_INFO("imx074", 0x1a),
+/* I2C */
+static struct soc_camera_subdev_desc imx074_desc;
+static struct i2c_board_info i2c0_devices[] = {
+ {
+ I2C_BOARD_INFO("ak4643", 0x13),
+ }, {
+ I2C_BOARD_INFO("imx074", 0x1a),
+ .platform_data = &imx074_desc,
+ },
};
-static struct soc_camera_link imx074_link = {
- .bus_id = 0,
- .board_info = &imx074_info,
- .i2c_adapter_id = 0,
- .module_name = "imx074",
+static struct i2c_board_info i2c1_devices[] = {
+ {
+ I2C_BOARD_INFO("r2025sd", 0x32),
+ },
};
-static struct platform_device ap4evb_camera = {
- .name = "soc-camera-pdrv",
- .id = 0,
- .dev = {
- .platform_data = &imx074_link,
+static struct resource csi2_resources[] = {
+ {
+ .name = "CSI2",
+ .start = 0xffc90000,
+ .end = 0xffc90fff,
+ .flags = IORESOURCE_MEM,
+ }, {
+ .start = intcs_evt2irq(0x17a0),
+ .flags = IORESOURCE_IRQ,
},
};
@@ -895,7 +906,7 @@ static struct sh_csi2_client_config csi2_clients[] = {
.phy = SH_CSI2_PHY_MAIN,
.lanes = 0, /* default: 2 lanes */
.channel = 0,
- .pdev = &ap4evb_camera,
+ .name = "imx074",
},
};
@@ -906,31 +917,50 @@ static struct sh_csi2_pdata csi2_info = {
.flags = SH_CSI2_ECC | SH_CSI2_CRC,
};
-static struct resource csi2_resources[] = {
- [0] = {
- .name = "CSI2",
- .start = 0xffc90000,
- .end = 0xffc90fff,
- .flags = IORESOURCE_MEM,
+static struct platform_device csi2_device = {
+ .name = "sh-mobile-csi2",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(csi2_resources),
+ .resource = csi2_resources,
+ .dev = {
+ .platform_data = &csi2_info,
},
- [1] = {
- .start = intcs_evt2irq(0x17a0),
- .flags = IORESOURCE_IRQ,
+};
+
+static struct soc_camera_async_subdev csi2_sd = {
+ .asd.hw = {
+ .bus_type = V4L2_ASYNC_BUS_PLATFORM,
+ .match.platform.name = "sh-mobile-csi2.0",
},
+ .role = SOCAM_SUBDEV_DATA_PROCESSOR,
};
-static struct sh_mobile_ceu_companion csi2 = {
- .id = 0,
- .num_resources = ARRAY_SIZE(csi2_resources),
- .resource = csi2_resources,
- .platform_data = &csi2_info,
+static struct soc_camera_async_subdev imx074_sd = {
+ .asd.hw = {
+ .bus_type = V4L2_ASYNC_BUS_I2C,
+ .match.i2c = {
+ .adapter_id = 0,
+ .address = 0x1a,
+ },
+ },
+ .role = SOCAM_SUBDEV_DATA_SOURCE,
};
+static struct v4l2_async_subdev *ceu_subdevs[] = {
+ /* Single 2-element group */
+ &csi2_sd.asd,
+ &imx074_sd.asd,
+};
+
+/* 0-terminated array of group-sizes */
+static int ceu_subdev_sizes[] = {ARRAY_SIZE(ceu_subdevs), 0};
+
static struct sh_mobile_ceu_info sh_mobile_ceu_info = {
.flags = SH_CEU_FLAG_USE_8BIT_BUS,
.max_width = 8188,
.max_height = 8188,
- .csi2 = &csi2,
+ .asd = ceu_subdevs,
+ .asd_sizes = ceu_subdev_sizes,
};
static struct resource ceu_resources[] = {
@@ -975,7 +1005,7 @@ static struct platform_device *ap4evb_devices[] __initdata = {
&lcdc_device,
&lcdc1_device,
&ceu_device,
- &ap4evb_camera,
+ &csi2_device,
&meram_device,
};
@@ -1070,19 +1100,6 @@ static struct i2c_board_info tsc_device = {
/*.irq is selected on ap4evb_init */
};
-/* I2C */
-static struct i2c_board_info i2c0_devices[] = {
- {
- I2C_BOARD_INFO("ak4643", 0x13),
- },
-};
-
-static struct i2c_board_info i2c1_devices[] = {
- {
- I2C_BOARD_INFO("r2025sd", 0x32),
- },
-};
-
#define GPIO_PORT9CR IOMEM(0xE6051009)
#define GPIO_PORT10CR IOMEM(0xE605100A)
@@ -1097,6 +1114,7 @@ static void __init ap4evb_init(void)
{ "A3SP", &sdhi0_device, },
{ "A3SP", &sdhi1_device, },
{ "A4R", &ceu_device, },
+ { "A4R", &csi2_device, },
};
u32 srcr4;
struct clk *clk;
@@ -1324,6 +1342,7 @@ static void __init ap4evb_init(void)
sh7372_pm_init();
pm_clk_add(&fsi_device.dev, "spu2");
pm_clk_add(&lcdc1_device.dev, "hdmi");
+ pm_clk_add(&csi2_device.dev, "csir");
}
MACHINE_START(AP4EVB, "ap4evb")
diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
index 45d21fe..2e8cb42 100644
--- a/arch/arm/mach-shmobile/clock-sh7372.c
+++ b/arch/arm/mach-shmobile/clock-sh7372.c
@@ -617,6 +617,7 @@ static struct clk_lookup lookups[] = {
CLKDEV_ICK_ID("divb", "sh_fsi2", &fsidivs[FSIDIV_B]),
CLKDEV_ICK_ID("xcka", "sh_fsi2", &fsiack_clk),
CLKDEV_ICK_ID("xckb", "sh_fsi2", &fsibck_clk),
+ CLKDEV_ICK_ID("csir", "sh-mobile-csi2.0", &div4_clks[DIV4_CSIR]),
};
void __init sh7372_clock_init(void)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (6 preceding siblings ...)
2013-04-08 15:05 ` [PATCH v8 7/7] ARM: shmobile: convert ap4evb to asynchronously register camera subdevices Guennadi Liakhovetski
@ 2013-04-09 8:07 ` Simon Horman
2013-04-09 8:14 ` Guennadi Liakhovetski
2013-04-11 9:59 ` Guennadi Liakhovetski
8 siblings, 1 reply; 27+ messages in thread
From: Simon Horman @ 2013-04-09 8:07 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad
On Mon, Apr 08, 2013 at 05:05:31PM +0200, Guennadi Liakhovetski wrote:
> Mostly just a re-spin of v7 with minor modifications.
>
> Guennadi Liakhovetski (7):
> media: V4L2: add temporary clock helpers
> media: V4L2: support asynchronous subdevice registration
> media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
> soc-camera: add V4L2-async support
> sh_mobile_ceu_camera: add asynchronous subdevice probing support
> imx074: support asynchronous probing
> ARM: shmobile: convert ap4evb to asynchronously register camera
> subdevices
Hi Guennadi,
can the last patch be applied (to my tree) independently
of the other patches (applied elsewhere) ?
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-09 8:07 ` [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Simon Horman
@ 2013-04-09 8:14 ` Guennadi Liakhovetski
2013-04-09 9:01 ` Simon Horman
0 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-09 8:14 UTC (permalink / raw)
To: Simon Horman
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad
Hi Simon
On Tue, 9 Apr 2013, Simon Horman wrote:
> On Mon, Apr 08, 2013 at 05:05:31PM +0200, Guennadi Liakhovetski wrote:
> > Mostly just a re-spin of v7 with minor modifications.
> >
> > Guennadi Liakhovetski (7):
> > media: V4L2: add temporary clock helpers
> > media: V4L2: support asynchronous subdevice registration
> > media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
> > soc-camera: add V4L2-async support
> > sh_mobile_ceu_camera: add asynchronous subdevice probing support
> > imx074: support asynchronous probing
> > ARM: shmobile: convert ap4evb to asynchronously register camera
> > subdevices
>
> Hi Guennadi,
>
> can the last patch be applied (to my tree) independently
> of the other patches (applied elsewhere) ?
No, please, wait. It strongly depends on the whole patch series.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-09 8:14 ` Guennadi Liakhovetski
@ 2013-04-09 9:01 ` Simon Horman
0 siblings, 0 replies; 27+ messages in thread
From: Simon Horman @ 2013-04-09 9:01 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad
On Tue, Apr 09, 2013 at 10:14:08AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
>
> On Tue, 9 Apr 2013, Simon Horman wrote:
>
> > On Mon, Apr 08, 2013 at 05:05:31PM +0200, Guennadi Liakhovetski wrote:
> > > Mostly just a re-spin of v7 with minor modifications.
> > >
> > > Guennadi Liakhovetski (7):
> > > media: V4L2: add temporary clock helpers
> > > media: V4L2: support asynchronous subdevice registration
> > > media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
> > > soc-camera: add V4L2-async support
> > > sh_mobile_ceu_camera: add asynchronous subdevice probing support
> > > imx074: support asynchronous probing
> > > ARM: shmobile: convert ap4evb to asynchronously register camera
> > > subdevices
> >
> > Hi Guennadi,
> >
> > can the last patch be applied (to my tree) independently
> > of the other patches (applied elsewhere) ?
>
> No, please, wait. It strongly depends on the whole patch series.
Thanks, understood.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-08 15:05 [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Guennadi Liakhovetski
` (7 preceding siblings ...)
2013-04-09 8:07 ` [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example Simon Horman
@ 2013-04-11 9:59 ` Guennadi Liakhovetski
2013-04-11 19:58 ` Sylwester Nawrocki
8 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-11 9:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
Sylwester Nawrocki, linux-sh, Magnus Damm, Sakari Ailus,
Prabhakar Lad
Hi all
On Mon, 8 Apr 2013, Guennadi Liakhovetski wrote:
> Mostly just a re-spin of v7 with minor modifications.
>
> Guennadi Liakhovetski (7):
> media: V4L2: add temporary clock helpers
> media: V4L2: support asynchronous subdevice registration
> media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
> soc-camera: add V4L2-async support
> sh_mobile_ceu_camera: add asynchronous subdevice probing support
> imx074: support asynchronous probing
> ARM: shmobile: convert ap4evb to asynchronously register camera
> subdevices
So far there haven't been any comments to this, and Mauro asked to push
all non-fixes to him by tomorrow. So, if at least the API is now ok, we
could push this to 3.10, at least the core patches 1 and 2. Then during
3.10 we could look at porting individual drivers on top of this.
Thanks
Guennadi
> arch/arm/mach-shmobile/board-ap4evb.c | 103 ++--
> arch/arm/mach-shmobile/clock-sh7372.c | 1 +
> drivers/media/i2c/soc_camera/imx074.c | 36 +-
> drivers/media/i2c/soc_camera/mt9m001.c | 17 +-
> drivers/media/i2c/soc_camera/mt9m111.c | 20 +-
> drivers/media/i2c/soc_camera/mt9t031.c | 19 +-
> drivers/media/i2c/soc_camera/mt9t112.c | 19 +-
> drivers/media/i2c/soc_camera/mt9v022.c | 17 +-
> drivers/media/i2c/soc_camera/ov2640.c | 19 +-
> drivers/media/i2c/soc_camera/ov5642.c | 20 +-
> drivers/media/i2c/soc_camera/ov6650.c | 17 +-
> drivers/media/i2c/soc_camera/ov772x.c | 15 +-
> drivers/media/i2c/soc_camera/ov9640.c | 17 +-
> drivers/media/i2c/soc_camera/ov9640.h | 1 +
> drivers/media/i2c/soc_camera/ov9740.c | 18 +-
> drivers/media/i2c/soc_camera/rj54n1cb0c.c | 17 +-
> drivers/media/i2c/soc_camera/tw9910.c | 18 +-
> .../platform/soc_camera/sh_mobile_ceu_camera.c | 134 +++--
> drivers/media/platform/soc_camera/sh_mobile_csi2.c | 163 +++--
> drivers/media/platform/soc_camera/soc_camera.c | 666 ++++++++++++++++----
> .../platform/soc_camera/soc_camera_platform.c | 2 +-
> drivers/media/v4l2-core/Makefile | 3 +-
> drivers/media/v4l2-core/v4l2-async.c | 262 ++++++++
> drivers/media/v4l2-core/v4l2-clk.c | 177 ++++++
> include/media/sh_mobile_ceu.h | 2 +
> include/media/sh_mobile_csi2.h | 2 +-
> include/media/soc_camera.h | 36 +-
> include/media/v4l2-async.h | 104 +++
> include/media/v4l2-clk.h | 54 ++
> 29 files changed, 1675 insertions(+), 304 deletions(-)
> create mode 100644 drivers/media/v4l2-core/v4l2-async.c
> create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> create mode 100644 include/media/v4l2-async.h
> create mode 100644 include/media/v4l2-clk.h
>
> --
> 1.7.2.5
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-11 9:59 ` Guennadi Liakhovetski
@ 2013-04-11 19:58 ` Sylwester Nawrocki
2013-04-12 6:13 ` Guennadi Liakhovetski
0 siblings, 1 reply; 27+ messages in thread
From: Sylwester Nawrocki @ 2013-04-11 19:58 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
linux-sh, Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi Guennadi,
On 04/11/2013 11:59 AM, Guennadi Liakhovetski wrote:
> Hi all
>
> On Mon, 8 Apr 2013, Guennadi Liakhovetski wrote:
>
>> > Mostly just a re-spin of v7 with minor modifications.
>> >
>> > Guennadi Liakhovetski (7):
>> > media: V4L2: add temporary clock helpers
>> > media: V4L2: support asynchronous subdevice registration
>> > media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
>> > soc-camera: add V4L2-async support
>> > sh_mobile_ceu_camera: add asynchronous subdevice probing support
>> > imx074: support asynchronous probing
>> > ARM: shmobile: convert ap4evb to asynchronously register camera
>> > subdevices
>
> So far there haven't been any comments to this, and Mauro asked to push
> all non-fixes to him by tomorrow. So, if at least the API is now ok, we
> could push this to 3.10, at least the core patches 1 and 2. Then during
> 3.10 we could look at porting individual drivers on top of this.
This patch series has significantly improved over time but I'm not sure
it is all ready to merge it at this moment. At least it doesn't make sense
to me to merge it without any users.
The purpose of an introduction of this whole asynchronous probing concept
was to add support for the device tree based systems. However there is no
patch in this series that would be adding device tree support to some V4L2
driver. That's a minor issue though I think.
A significant blocking point IMHO is that this API is bound to the circular
dependency issue between a sub-device and the host driver. I think we
should
have at least some specific ideas on how to resolve it before pushing the
API upstream. Or are there any already ?
One of the ideas I had was to make a sub-device driver drop the reference
it has to the clock provider module (the host) as soon as it gets registered
to it. But it doesn't seem straightforward with the common clock API.
Other option is a sysfs attribute at a host driver that would allow to
release its sub-device(s). But it sounds a bit strange to me to require
userspace to touch some sysfs attributes before being able to remove some
modules.
Something probably needs to be changed at the high level design to avoid
this circular dependency.
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-11 19:58 ` Sylwester Nawrocki
@ 2013-04-12 6:13 ` Guennadi Liakhovetski
2013-04-12 12:48 ` Sylwester Nawrocki
0 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2013-04-12 6:13 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
linux-sh, Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi Sylwester
On Thu, 11 Apr 2013, Sylwester Nawrocki wrote:
> Hi Guennadi,
>
> On 04/11/2013 11:59 AM, Guennadi Liakhovetski wrote:
> > Hi all
> >
> > On Mon, 8 Apr 2013, Guennadi Liakhovetski wrote:
> >
> > > > Mostly just a re-spin of v7 with minor modifications.
> > > >
> > > > Guennadi Liakhovetski (7):
> > > > media: V4L2: add temporary clock helpers
> > > > media: V4L2: support asynchronous subdevice registration
> > > > media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
> > > > soc-camera: add V4L2-async support
> > > > sh_mobile_ceu_camera: add asynchronous subdevice probing support
> > > > imx074: support asynchronous probing
> > > > ARM: shmobile: convert ap4evb to asynchronously register camera
> > > > subdevices
> >
> > So far there haven't been any comments to this, and Mauro asked to push
> > all non-fixes to him by tomorrow. So, if at least the API is now ok, we
> > could push this to 3.10, at least the core patches 1 and 2. Then during
> > 3.10 we could look at porting individual drivers on top of this.
>
> This patch series has significantly improved over time but I'm not sure
> it is all ready to merge it at this moment. At least it doesn't make sense
> to me to merge it without any users.
I wouldn't be too scared to also push my soc-camera patches from the same
patch-series. But our V4L2 OF patches don't have many users so far either,
right? And they aren't likely to get any until asynchronous probing is
supported.
> The purpose of an introduction of this whole asynchronous probing concept
> was to add support for the device tree based systems. However there is no
> patch in this series that would be adding device tree support to some V4L2
> driver. That's a minor issue though I think.
It is indeed. And I did have such patches in the past, but I dropped them
on purpose for now. It was too much work to update them all for each
iteration, so, I picked up a testable minimum.
> A significant blocking point IMHO is that this API is bound to the circular
> dependency issue between a sub-device and the host driver. I think we should
> have at least some specific ideas on how to resolve it before pushing the
> API upstream. Or are there any already ?
Of course there is at least one. I wouldn't propose (soc-camera) patches,
that lock modules hard into memory, once probing is complete.
> One of the ideas I had was to make a sub-device driver drop the reference
> it has to the clock provider module (the host) as soon as it gets registered
> to it. But it doesn't seem straightforward with the common clock API.
It isn't.
> Other option is a sysfs attribute at a host driver that would allow to
> release its sub-device(s). But it sounds a bit strange to me to require
> userspace to touch some sysfs attributes before being able to remove some
> modules.
>
> Something probably needs to be changed at the high level design to avoid
> this circular dependency.
Here's what I do in my soc-camera patches atm: holding a reference to a
(V4L2) clock doesn't increment bridge driver's use-count (for this
discussion I describe the combined soc-camera host and soc-camera core
functionality as a bridge driver, because that's what most non soc-camera
drivers will look like). So, it can be unloaded. Once unloaded, it
unregisters its V4L2 async notifier. Inside that the v4l2-async framework
first detaches the subdevice driver, then calls the notifier's .unbind()
method, which should now unregister the clock. Then, back in
v4l2_async_notifier_unregister() the subdevice driver is re-probed, this
time with no clock available, so, it re-enters the deferred probing state.
BTW, a bit of self-advertisement: most soc-camera host drivers will hardly
need any modifications to support asynchronous subdevice probing. It's all
hidden in the soc-camera core. The sh-mobile CEU driver had to be modified
because it supports one more subdevice - a CSI-2 interface.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-12 6:13 ` Guennadi Liakhovetski
@ 2013-04-12 12:48 ` Sylwester Nawrocki
2013-04-15 10:32 ` Laurent Pinchart
0 siblings, 1 reply; 27+ messages in thread
From: Sylwester Nawrocki @ 2013-04-12 12:48 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Laurent Pinchart, Hans Verkuil, Sylwester Nawrocki,
linux-sh, Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi Guennadi,
On 04/12/2013 08:13 AM, Guennadi Liakhovetski wrote:
> On Thu, 11 Apr 2013, Sylwester Nawrocki wrote:
>> On 04/11/2013 11:59 AM, Guennadi Liakhovetski wrote:
>>> On Mon, 8 Apr 2013, Guennadi Liakhovetski wrote:
>>>
>>>>> Mostly just a re-spin of v7 with minor modifications.
>>>>>
>>>>> Guennadi Liakhovetski (7):
>>>>> media: V4L2: add temporary clock helpers
>>>>> media: V4L2: support asynchronous subdevice registration
>>>>> media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
>>>>> soc-camera: add V4L2-async support
>>>>> sh_mobile_ceu_camera: add asynchronous subdevice probing support
>>>>> imx074: support asynchronous probing
>>>>> ARM: shmobile: convert ap4evb to asynchronously register camera
>>>>> subdevices
>>>
>>> So far there haven't been any comments to this, and Mauro asked to push
>>> all non-fixes to him by tomorrow. So, if at least the API is now ok, we
>>> could push this to 3.10, at least the core patches 1 and 2. Then during
>>> 3.10 we could look at porting individual drivers on top of this.
>>
>> This patch series has significantly improved over time but I'm not sure
>> it is all ready to merge it at this moment. At least it doesn't make sense
>> to me to merge it without any users.
>
> I wouldn't be too scared to also push my soc-camera patches from the same
> patch-series. But our V4L2 OF patches don't have many users so far either,
> right? And they aren't likely to get any until asynchronous probing is
> supported.
Arguably there is not many users, only Samsung H/W - couple drivers and boards.
I didn't push patches for the sensors that add device tree support, and are
moving any H/W access to the subdev's .registered callback. Since it is not
preferred way as we all agreed. But since I needed to focus on the Exynos ISP
driver I chose this method as an interim solution. In fact it works well
without many changes in original initialization sequence that need to be
carefully verified.
Anyway, my plan is to focus more on the asynchronous registration stuff to
possibly have it ready for 3.11. Now when the Exynos camera subsystem is still
supported after this platform becomes dt-only.
>> The purpose of an introduction of this whole asynchronous probing concept
>> was to add support for the device tree based systems. However there is no
>> patch in this series that would be adding device tree support to some V4L2
>> driver. That's a minor issue though I think.
>
> It is indeed. And I did have such patches in the past, but I dropped them
> on purpose for now. It was too much work to update them all for each
> iteration, so, I picked up a testable minimum.
Fair enough.
>> A significant blocking point IMHO is that this API is bound to the circular
>> dependency issue between a sub-device and the host driver. I think we should
>> have at least some specific ideas on how to resolve it before pushing the
>> API upstream. Or are there any already ?
>
> Of course there is at least one. I wouldn't propose (soc-camera) patches,
> that lock modules hard into memory, once probing is complete.
Alright then, maybe I should have more carefully analysed you last patch
series.
>> One of the ideas I had was to make a sub-device driver drop the reference
>> it has to the clock provider module (the host) as soon as it gets registered
>> to it. But it doesn't seem straightforward with the common clock API.
>
> It isn't.
>
>> Other option is a sysfs attribute at a host driver that would allow to
>> release its sub-device(s). But it sounds a bit strange to me to require
>> userspace to touch some sysfs attributes before being able to remove some
>> modules.
>>
>> Something probably needs to be changed at the high level design to avoid
>> this circular dependency.
>
> Here's what I do in my soc-camera patches atm: holding a reference to a
> (V4L2) clock doesn't increment bridge driver's use-count (for this
> discussion I describe the combined soc-camera host and soc-camera core
> functionality as a bridge driver, because that's what most non soc-camera
> drivers will look like). So, it can be unloaded. Once unloaded, it
> unregisters its V4L2 async notifier. Inside that the v4l2-async framework
> first detaches the subdevice driver, then calls the notifier's .unbind()
> method, which should now unregister the clock. Then, back in
> v4l2_async_notifier_unregister() the subdevice driver is re-probed, this
> time with no clock available, so, it re-enters the deferred probing state.
Ok, it looks better than I thought initially.. :)
Still, aren't there races possible, when the host driver gets unregistered
while subdev holds a reference to the clock, and before it gets registered
to the host ? The likelihood of that seems very low, but I fail to find
any prove it can't happen either.
> BTW, a bit of self-advertisement: most soc-camera host drivers will hardly
> need any modifications to support asynchronous subdevice probing. It's all
> hidden in the soc-camera core. The sh-mobile CEU driver had to be modified
> because it supports one more subdevice - a CSI-2 interface.
OK, sounds good!
I just noticed Laurent's review of this patch series. I'd like to add couple
remarks too. But I pretty much agree to Laurent's comments, especially about
integrating some the new data structures with struct v4l2_subdev.
Regards,
Sylwester
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/7] V4L2 clock and async patches and soc-camera example
2013-04-12 12:48 ` Sylwester Nawrocki
@ 2013-04-15 10:32 ` Laurent Pinchart
0 siblings, 0 replies; 27+ messages in thread
From: Laurent Pinchart @ 2013-04-15 10:32 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Guennadi Liakhovetski, linux-media, Hans Verkuil, linux-sh,
Magnus Damm, Sakari Ailus, Prabhakar Lad
Hi,
On Friday 12 April 2013 14:48:17 Sylwester Nawrocki wrote:
> On 04/12/2013 08:13 AM, Guennadi Liakhovetski wrote:
> > On Thu, 11 Apr 2013, Sylwester Nawrocki wrote:
> >> On 04/11/2013 11:59 AM, Guennadi Liakhovetski wrote:
> >>> On Mon, 8 Apr 2013, Guennadi Liakhovetski wrote:
[snip]
> >> A significant blocking point IMHO is that this API is bound to the
> >> circular dependency issue between a sub-device and the host driver. I
> >> think we should have at least some specific ideas on how to resolve it
> >> before pushing the API upstream. Or are there any already ?
> >
> > Of course there is at least one. I wouldn't propose (soc-camera) patches,
> > that lock modules hard into memory, once probing is complete.
>
> Alright then, maybe I should have more carefully analysed you last patch
> series.
>
> >> One of the ideas I had was to make a sub-device driver drop the reference
> >> it has to the clock provider module (the host) as soon as it gets
> >> registered to it. But it doesn't seem straightforward with the common
> >> clock API.
> >
> > It isn't.
> >
> >> Other option is a sysfs attribute at a host driver that would allow to
> >> release its sub-device(s). But it sounds a bit strange to me to require
> >> userspace to touch some sysfs attributes before being able to remove some
> >> modules.
> >>
> >> Something probably needs to be changed at the high level design to avoid
> >> this circular dependency.
> >
> > Here's what I do in my soc-camera patches atm: holding a reference to a
> > (V4L2) clock doesn't increment bridge driver's use-count (for this
> > discussion I describe the combined soc-camera host and soc-camera core
> > functionality as a bridge driver, because that's what most non soc-camera
> > drivers will look like). So, it can be unloaded. Once unloaded, it
> > unregisters its V4L2 async notifier. Inside that the v4l2-async framework
> > first detaches the subdevice driver, then calls the notifier's .unbind()
> > method, which should now unregister the clock. Then, back in
> > v4l2_async_notifier_unregister() the subdevice driver is re-probed, this
> > time with no clock available, so, it re-enters the deferred probing state.
>
> Ok, it looks better than I thought initially.. :)
>
> Still, aren't there races possible, when the host driver gets unregistered
> while subdev holds a reference to the clock, and before it gets registered
> to the host ? The likelihood of that seems very low, but I fail to find
> any prove it can't happen either.
That was the concern I was about to raise as well, before reading your e-mail.
Holding a reference to an object that can disappear at any time is asking for
trouble. The method currently implemented should work, but is racy in my
opinion. The bridge module could be unloaded after the subdev gets a reference
to the clock but before it registers itself with v4l2_async_register_subdev().
The clock would then be freed by the bridge, resulting in a crash.
I'm not sure if the circular dependency problem can be solved without an
explicit way to break the dependency, possibly from userspace (although I'm
not sure if that's the best solution).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 27+ messages in thread