diff mbox

[v3] media: V4L2: add temporary clock helpers

Message ID Pine.LNX.4.64.1212041136250.26918@axis700.grange (mailing list archive)
State New, archived
Headers show

Commit Message

Guennadi Liakhovetski Dec. 4, 2012, 10:42 a.m. UTC 
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>
---

v3: (thanks for all the comments)

1. add and use a mutex to protect the enable counter instead of an atomic 
variable
2. add a use-count
3. add "const" to dev_id
4. move allocations outside the mutex
5. allow rate reading and setting on disabled clock
6. rename the clock list head:-)

 drivers/media/v4l2-core/Makefile   |    2 +-
 drivers/media/v4l2-core/v4l2-clk.c |  176 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-clk.h           |   54 +++++++++++
 3 files changed, 231 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 create mode 100644 include/media/v4l2-clk.h

Comments

Laurent Pinchart Dec. 6, 2012, 1:24 a.m. UTC | #1 
Hi Guennadi,

Thanks for the patch.

On Tuesday 04 December 2012 11:42:15 Guennadi Liakhovetski wrote:
> 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.

As discussed on Jabber, I think we should make the clock helpers use the 
common clock framework when available, to avoid pushing support for the two 
APIs to all sensor drivers. Do you plan to include that in v4 ? :-)
Guennadi Liakhovetski Dec. 6, 2012, 7:41 a.m. UTC | #2 
Hi Laurent

On Thu, 6 Dec 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the patch.
> 
> On Tuesday 04 December 2012 11:42:15 Guennadi Liakhovetski wrote:
> > 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.
> 
> As discussed on Jabber, I think we should make the clock helpers use the 
> common clock framework when available, to avoid pushing support for the two 
> APIs to all sensor drivers. Do you plan to include that in v4 ? :-)

AAMOF, no, I don't. Originally I planned to add this only when the first 
user appears. We can also add it earlier - a test case could be hacked up 
pretty quickly. But in either case I'd prefer to have it as a separate 
patch.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Dec. 6, 2012, 10:13 a.m. UTC | #3 
Hi Guennadi,

On Thursday 06 December 2012 08:41:41 Guennadi Liakhovetski wrote:
> On Thu, 6 Dec 2012, Laurent Pinchart wrote:
> > On Tuesday 04 December 2012 11:42:15 Guennadi Liakhovetski wrote:
> > > 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.
> > 
> > As discussed on Jabber, I think we should make the clock helpers use the
> > common clock framework when available, to avoid pushing support for the
> > two APIs to all sensor drivers. Do you plan to include that in v4 ? :-)
> 
> AAMOF, no, I don't. Originally I planned to add this only when the first
> user appears. We can also add it earlier - a test case could be hacked up
> pretty quickly. But in either case I'd prefer to have it as a separate
> patch.

OK, I'm fine with that.
Sylwester Nawrocki Jan. 8, 2013, 6:59 p.m. UTC | #4 
Hi Guennadi,

Just few minor remarks below...

On 12/04/2012 11:42 AM, Guennadi Liakhovetski wrote:
> +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:

These multiple labels could be avoided by naming labels after what
happens on next lines, rather than after the location we start from.

> +	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 (unlikely(atomic_read(&clk->use_count))) {

I don't think unlikely() is significant here, it doesn't seem to be really
a fast path.

> +		pr_err("%s(): Unregistering ref-counted %s:%s clock!\n",
> +		       __func__, clk->dev_id, clk->id);
> +		BUG();

Hmm, I wouldn't certainly like, e.g. my phone to crash completely only
because camera drivers are buggy. Camera clocks likely aren't essential
resources for system operation... I would just use WARN() here and return
without actually freeing the clock. Not sure if changing signature of
this function and returning an error would be any useful.

Is it indeed such an unrecoverable error we need to resort to BUG() ?

And here is Linus' opinion on how many BUG_ON()s we have in the kernel:
https://lkml.org/lkml/2012/9/27/461
http://permalink.gmane.org/gmane.linux.kernel/1347333

:)

> +	}
> +
> +	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);

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index c2d61d4..d065c01 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..2225081
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -0,0 +1,176 @@ 
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012, 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/errno.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.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 v4l2_subdev *sd, const char *id)
+{
+	struct v4l2_clk *clk;
+
+	mutex_lock(&clk_lock);
+	clk = v4l2_clk_find(sd->name, 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 (unlikely(atomic_read(&clk->use_count))) {
+		pr_err("%s(): Unregistering ref-counted %s:%s clock!\n",
+		       __func__, clk->dev_id, clk->id);
+		BUG();
+	}
+
+	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..9b67472
--- /dev/null
+++ b/include/media/v4l2-clk.h
@@ -0,0 +1,54 @@ 
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012, 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 v4l2_subdev;
+
+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 v4l2_subdev *sd, 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