diff mbox

[1/2] media: V4L2: add temporary clock helpers

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

Commit Message

Guennadi Liakhovetski Oct. 19, 2012, 10:20 p.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>
---
 drivers/media/v4l2-core/Makefile   |    2 +-
 drivers/media/v4l2-core/v4l2-clk.c |  126 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-clk.h           |   48 ++++++++++++++
 3 files changed, 175 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 create mode 100644 include/media/v4l2-clk.h

Comments

Sylwester Nawrocki Oct. 21, 2012, 6:52 p.m. UTC | #1
Hi Guennadi,

On 10/20/2012 12:20 AM, 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.

So I gave this patch a try this weekend. I would have a few comments/
questions. Thank you for sharing this!

> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> ---
>   drivers/media/v4l2-core/Makefile   |    2 +-
>   drivers/media/v4l2-core/v4l2-clk.c |  126 ++++++++++++++++++++++++++++++++++++
>   include/media/v4l2-clk.h           |   48 ++++++++++++++
>   3 files changed, 175 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 00f64d6..cb5fede 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..7d457e4
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,126 @@
> +/*
> + * V4L2 clock service

A like the name :-D

> + *
> + * 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/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(v4l2_clk);

nit: how about naming this lists v4l2_clks ?

> +
> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> +{
> +	struct v4l2_clk *clk = NULL;
> +
> +	mutex_lock(&clk_lock);
> +	if (!id) {
> +		if (list_is_singular(&v4l2_clk)) {

Hmm, the clock list is global, why should we assume there will be only one
entry with NULL v4l2_clk::id ? It would be useful to not provide the per 
subdev clock id when there is only one clock used per a sub-device, which 
is a majority of cases AFAICT.


> +			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
> +			if (!strstr(sd->name, clk->dev_id))

Ok, then clk->dev_id is supposed to be a sub-string of sd->name,
looks good...

> +				clk = ERR_PTR(-ENODEV);
> +		} else {
> +			clk = ERR_PTR(-EINVAL);
> +		}
> +	} else {
> +		list_for_each_entry(clk,&v4l2_clk, list) {
> +			if (!strcmp(id, clk->id)&&
> +			    !strcmp(sd->name, clk->dev_id))

but why we are doing a "strong" check here ? Couldn't the second strcmp() 
be just strstr() ?

> +				break;
> +		}
> +		if (&clk->list ==&v4l2_clk)
> +			clk = ERR_PTR(-ENODEV);
> +	}
> +	mutex_unlock(&clk_lock);
> +
> +	if (!IS_ERR(clk)&&
> +	    !try_module_get(clk->ops->owner))
> +		clk = ERR_PTR(-ENODEV);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_get);
>
> +void v4l2_clk_put(struct v4l2_clk *clk)
> +{
> +	if (!IS_ERR(clk))
> +		module_put(clk->ops->owner);
> +}
> +EXPORT_SYMBOL(v4l2_clk_put);
> +
> +int v4l2_clk_enable(struct v4l2_clk *clk)
> +{
> +	if (!clk->ops->enable)
> +		return -ENOSYS;
> +	return clk->ops->enable(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_enable);
> +
> +void v4l2_clk_disable(struct v4l2_clk *clk)
> +{
> +	if (clk->ops->disable)
> +		clk->ops->disable(clk);
> +}
> +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_name,
> +				   const char *name)
> +{
> +	struct v4l2_clk *clk;
> +
> +	if (!ops || !ops->owner || (!list_empty(&v4l2_clk)&&  !name))

ops->owner can be null when the clock provider module is built-in, not 
a loadable module. I actually hit this problem. ops->owner needs to be 
removed and I think the clocks list check could be removed as well,
please see my comment above.

Also it might be useful to check if a particular clocks is already
registered, to make this more foolproof and easier to debug.

> +		return ERR_PTR(-EINVAL);
> +
> +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +	if (!clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk->ops = ops;
> +	clk->id = name;
> +	clk->dev_id = dev_name;
> +
> +	mutex_lock(&clk_lock);
> +	list_add_tail(&clk->list,&v4l2_clk);
> +	mutex_unlock(&clk_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_register);
> +
> +void v4l2_clk_unregister(struct v4l2_clk *clk)
> +{
> +	mutex_lock(&clk_lock);
> +	list_del(&clk->list);
> +	mutex_unlock(&clk_lock);
> +
> +	kfree(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_unregister);

I have reworked some of the functions found here while trying to use your 
work with s3c-camif and ov9650 sensor drivers [1]. Please feel free to 
take (part of) these changes, if there are any you agree with.

I planned to also rework s3c-camif and add asynchronous subdev registration 
to it, but didn't quite managed to do it yet, it's going to be next step.

> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> new file mode 100644
> index 0000000..0c05ab3
> --- /dev/null
> +++ b/include/media/v4l2-clk.h
> @@ -0,0 +1,48 @@
> +/*
> + * 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/list.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;

I've found it helpful to add a

	void *priv;

field here, so the clock provider module can use it as a cookie, 
which can be passed in a call to v4l2_clk_register() and then 
retrieved in the clock ops. I'm not sure if this could be replaced 
with some container_of() magic.

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

[1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif-devel

Regards,
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
Guennadi Liakhovetski Oct. 22, 2012, 9:14 a.m. UTC | #2
Hi Sylwester

On Sun, 21 Oct 2012, Sylwester Nawrocki wrote:

> Hi Guennadi,
> 
> On 10/20/2012 12:20 AM, 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.
> 
> So I gave this patch a try this weekend. I would have a few comments/
> questions. Thank you for sharing this!

You mean you actually tried to use it? Wow, impressive! :-)

> > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> > ---
> >   drivers/media/v4l2-core/Makefile   |    2 +-
> >   drivers/media/v4l2-core/v4l2-clk.c |  126 ++++++++++++++++++++++++++++++++++++
> >   include/media/v4l2-clk.h           |   48 ++++++++++++++
> >   3 files changed, 175 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 00f64d6..cb5fede 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..7d457e4
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -0,0 +1,126 @@
> > +/*
> > + * V4L2 clock service
> 
> A like the name :-D
> 
> > + *
> > + * 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/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(v4l2_clk);
> 
> nit: how about naming this lists v4l2_clks ?
> 
> > +
> > +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> > +{
> > +	struct v4l2_clk *clk = NULL;
> > +
> > +	mutex_lock(&clk_lock);
> > +	if (!id) {
> > +		if (list_is_singular(&v4l2_clk)) {
> 
> Hmm, the clock list is global, why should we assume there will be only one
> entry with NULL v4l2_clk::id ?

This is testing for a case, when the user is trying to obtain a clock 
without providing an ID, similar to how with real clocks you can do

	clk = clk_get(dev, NULL);

> It would be useful to not provide the per 
> subdev clock id when there is only one clock used per a sub-device, which 
> is a majority of cases AFAICT.
> 
> 
> > +			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
> > +			if (!strstr(sd->name, clk->dev_id))
> 
> Ok, then clk->dev_id is supposed to be a sub-string of sd->name,
> looks good...

Looks like the no-ID case hasn't been tested... Make it

+			clk = list_first_entry(&v4l2_clk, struct v4l2_clk, list);
+			if (strcmp(sd->name, clk->dev_id))

> 
> > +				clk = ERR_PTR(-ENODEV);
> > +		} else {
> > +			clk = ERR_PTR(-EINVAL);
> > +		}
> > +	} else {
> > +		list_for_each_entry(clk,&v4l2_clk, list) {
> > +			if (!strcmp(id, clk->id)&&
> > +			    !strcmp(sd->name, clk->dev_id))
> 
> but why we are doing a "strong" check here ? Couldn't the second strcmp() 
> be just strstr() ?

I prefer both to be strcmp to avoid degenerate cases with just one letter 
etc.

> > +				break;
> > +		}
> > +		if (&clk->list ==&v4l2_clk)
> > +			clk = ERR_PTR(-ENODEV);
> > +	}
> > +	mutex_unlock(&clk_lock);
> > +
> > +	if (!IS_ERR(clk)&&
> > +	    !try_module_get(clk->ops->owner))
> > +		clk = ERR_PTR(-ENODEV);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_get);
> >
> > +void v4l2_clk_put(struct v4l2_clk *clk)
> > +{
> > +	if (!IS_ERR(clk))
> > +		module_put(clk->ops->owner);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_put);
> > +
> > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > +{
> > +	if (!clk->ops->enable)
> > +		return -ENOSYS;
> > +	return clk->ops->enable(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_enable);
> > +
> > +void v4l2_clk_disable(struct v4l2_clk *clk)
> > +{
> > +	if (clk->ops->disable)
> > +		clk->ops->disable(clk);
> > +}
> > +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_name,
> > +				   const char *name)
> > +{
> > +	struct v4l2_clk *clk;
> > +
> > +	if (!ops || !ops->owner || (!list_empty(&v4l2_clk)&&  !name))
> 
> ops->owner can be null when the clock provider module is built-in, not 
> a loadable module. I actually hit this problem. ops->owner needs to be 

Ah, good point, thanks!

> removed and I think the clocks list check could be removed as well,
> please see my comment above.

Not sure what you mean here, in fact, what the second test is supposed to 
do is allow a (simple) system to register a single V4L2 clock with a NULL 
ID. But if you want to register multiple clocks, you better use names. 
You're right it might not be needed strictly speaking, we could allow 
clocks with different device IDs with NULL IDs, but I preferred to make it 
a bit simpler. In fact, the dev_name has to be checked here.

> Also it might be useful to check if a particular clocks is already
> registered, to make this more foolproof and easier to debug.

Could do, yes, then we could support multiple clocks with NULL names.

> 
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> > +	if (!clk)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	clk->ops = ops;
> > +	clk->id = name;
> > +	clk->dev_id = dev_name;
> > +
> > +	mutex_lock(&clk_lock);
> > +	list_add_tail(&clk->list,&v4l2_clk);
> > +	mutex_unlock(&clk_lock);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_register);
> > +
> > +void v4l2_clk_unregister(struct v4l2_clk *clk)
> > +{
> > +	mutex_lock(&clk_lock);
> > +	list_del(&clk->list);
> > +	mutex_unlock(&clk_lock);
> > +
> > +	kfree(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_unregister);
> 
> I have reworked some of the functions found here while trying to use your 
> work with s3c-camif and ov9650 sensor drivers [1]. Please feel free to 
> take (part of) these changes, if there are any you agree with.

Nice, thanks, I'll have a look!

> I planned to also rework s3c-camif and add asynchronous subdev registration 
> to it, but didn't quite managed to do it yet, it's going to be next step.
> 
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > new file mode 100644
> > index 0000000..0c05ab3
> > --- /dev/null
> > +++ b/include/media/v4l2-clk.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * 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/list.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;
> 
> I've found it helpful to add a
> 
> 	void *priv;
> 
> field here, so the clock provider module can use it as a cookie, 
> which can be passed in a call to v4l2_clk_register() and then 
> retrieved in the clock ops. I'm not sure if this could be replaced 
> with some container_of() magic.

Yes, in soc-camera dev_id is a string, allocated with the private data, 
so, we can use container_of(clk->dev_id, struct mystruct, clk_name);

Thanks
Guennadi

> > +};
> > +
> > +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 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
> 
> [1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif-devel
> 
> Regards,
> Sylwester
> 

---
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
On 10/22/2012 11:14 AM, Guennadi Liakhovetski wrote:
> On Sun, 21 Oct 2012, Sylwester Nawrocki wrote:
>> On 10/20/2012 12:20 AM, 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.
>>
>> So I gave this patch a try this weekend. I would have a few comments/
>> questions. Thank you for sharing this!
> 
> You mean you actually tried to use it? Wow, impressive! :-)
> 
>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>> ---
>>>   drivers/media/v4l2-core/Makefile   |    2 +-
>>>   drivers/media/v4l2-core/v4l2-clk.c |  126 ++++++++++++++++++++++++++++++++++++
>>>   include/media/v4l2-clk.h           |   48 ++++++++++++++
>>>   3 files changed, 175 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 00f64d6..cb5fede 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..7d457e4
>>> --- /dev/null
>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
>>> @@ -0,0 +1,126 @@
>>> +/*
>>> + * V4L2 clock service
>>
>> A like the name :-D
>>
>>> + *
>>> + * 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/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(v4l2_clk);
>>
>> nit: how about naming this lists v4l2_clks ?
>>
>>> +
>>> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
>>> +{
>>> +	struct v4l2_clk *clk = NULL;
>>> +
>>> +	mutex_lock(&clk_lock);
>>> +	if (!id) {
>>> +		if (list_is_singular(&v4l2_clk)) {
>>
>> Hmm, the clock list is global, why should we assume there will be only one
>> entry with NULL v4l2_clk::id ?
> 
> This is testing for a case, when the user is trying to obtain a clock 
> without providing an ID, similar to how with real clocks you can do
> 
> 	clk = clk_get(dev, NULL);

Right, but there may be a need to handle more than one clock like this.
In Samsung Exynos SoC there are two clocks, for each physical video input
bus. I will like not use this temporary clock API, as I seem to have common
clock framework and DT support available there (just need to sort out a few
crashes yet ;). But still why not to allow more than one clock ?

>> It would be useful to not provide the per 
>> subdev clock id when there is only one clock used per a sub-device, which 
>> is a majority of cases AFAICT.
>>
>>
>>> +			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
>>> +			if (!strstr(sd->name, clk->dev_id))
>>
>> Ok, then clk->dev_id is supposed to be a sub-string of sd->name,
>> looks good...
> 
> Looks like the no-ID case hasn't been tested... Make it
> 
> +			clk = list_first_entry(&v4l2_clk, struct v4l2_clk, list);
> +			if (strcmp(sd->name, clk->dev_id))

Right, I noticed it too, and fixed this list handling temporarily like that.

As for sd->name, this is supposed to be subdev's name as created by subdev 
driver during its' probing ? And the clock may need to be registered before
subdev has been probed or the host gets hold of the subdev ?

How would we make sure the host knows _subdev's_, i.e. not it's driver's
name ? 

I know there are standard functions like v4l2_i2c_subdev_init() and 
v4l2_spi_subdev_init() that initialize sd->name using standar pattern, but
subdev driver can overwrite the name. I.e. to avoid I2C adapter and I2C 
slave address numbers creeping in into subdev names, which are then exposed 
to user space through Media Controller API.

>>
>>> +				clk = ERR_PTR(-ENODEV);
>>> +		} else {
>>> +			clk = ERR_PTR(-EINVAL);
>>> +		}
>>> +	} else {
>>> +		list_for_each_entry(clk,&v4l2_clk, list) {
>>> +			if (!strcmp(id, clk->id)&&
>>> +			    !strcmp(sd->name, clk->dev_id))
>>
>> but why we are doing a "strong" check here ? Couldn't the second strcmp() 
>> be just strstr() ?
> 
> I prefer both to be strcmp to avoid degenerate cases with just one letter 
> etc.
> 
...
>>> +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>> +				   const char *dev_name,
>>> +				   const char *name)
>>> +{
>>> +	struct v4l2_clk *clk;
>>> +
>>> +	if (!ops || !ops->owner || (!list_empty(&v4l2_clk)&&  !name))
>>
>> ops->owner can be null when the clock provider module is built-in, not 
>> a loadable module. I actually hit this problem. ops->owner needs to be 
> 
> Ah, good point, thanks!
> 
>> removed and I think the clocks list check could be removed as well,
>> please see my comment above.
> 
> Not sure what you mean here, in fact, what the second test is supposed to 
> do is allow a (simple) system to register a single V4L2 clock with a NULL 
> ID. But if you want to register multiple clocks, you better use names. 
> You're right it might not be needed strictly speaking, we could allow 
> clocks with different device IDs with NULL IDs, but I preferred to make it 
> a bit simpler. In fact, the dev_name has to be checked here.

I meant in order to support multiple clocks with null ID.
Yes, would be good to verify dev_name at this point.

>> Also it might be useful to check if a particular clocks is already
>> registered, to make this more foolproof and easier to debug.
> 
> Could do, yes, then we could support multiple clocks with NULL names.
 
I'm not quite sure if anyone is ever going to use multiple clocks. But it 
looks not much effort is needed to support this right away.

>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
>>> +	if (!clk)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	clk->ops = ops;
>>> +	clk->id = name;
>>> +	clk->dev_id = dev_name;
>>> +
>>> +	mutex_lock(&clk_lock);
>>> +	list_add_tail(&clk->list,&v4l2_clk);
>>> +	mutex_unlock(&clk_lock);
>>> +
>>> +	return clk;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_clk_register);
>>> +
>>> +void v4l2_clk_unregister(struct v4l2_clk *clk)
>>> +{
>>> +	mutex_lock(&clk_lock);
>>> +	list_del(&clk->list);
>>> +	mutex_unlock(&clk_lock);
>>> +
>>> +	kfree(clk);
>>> +}
>>> +EXPORT_SYMBOL(v4l2_clk_unregister);
>>
>> I have reworked some of the functions found here while trying to use your 
>> work with s3c-camif and ov9650 sensor drivers [1]. Please feel free to 
>> take (part of) these changes, if there are any you agree with.
> 
> Nice, thanks, I'll have a look!
> 
>> I planned to also rework s3c-camif and add asynchronous subdev registration 
>> to it, but didn't quite managed to do it yet, it's going to be next step.
>>
>>> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
>>> new file mode 100644
>>> index 0000000..0c05ab3
>>> --- /dev/null
>>> +++ b/include/media/v4l2-clk.h
>>> @@ -0,0 +1,48 @@
>>> +/*
>>> + * 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/list.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;
>>
>> I've found it helpful to add a
>>
>> 	void *priv;
>>
>> field here, so the clock provider module can use it as a cookie, 
>> which can be passed in a call to v4l2_clk_register() and then 
>> retrieved in the clock ops. I'm not sure if this could be replaced 
>> with some container_of() magic.
> 
> Yes, in soc-camera dev_id is a string, allocated with the private data, 
> so, we can use container_of(clk->dev_id, struct mystruct, clk_name);

OK. Might be a bit difficult to use it like this in some cases though.
This one data field could probably gain us shorter code paths to get into
a required struct within the clock op. callbacks.

> Thanks
> Guennadi


Regards,
Laurent Pinchart Oct. 22, 2012, 12:55 p.m. UTC | #4
Hi Guennadi,

Thanks for the patch. Here are a few comments in addition to what Sylwester 
already mentioned.

On Saturday 20 October 2012 00:20:20 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.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/v4l2-core/Makefile   |    2 +-
>  drivers/media/v4l2-core/v4l2-clk.c |  126 +++++++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h           |   48 ++++++++++++++
>  3 files changed, 175 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 00f64d6..cb5fede 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..7d457e4
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,126 @@
> +/*
> + * 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/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(v4l2_clk);
> +
> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> +{
> +	struct v4l2_clk *clk = NULL;
> +
> +	mutex_lock(&clk_lock);
> +	if (!id) {
> +		if (list_is_singular(&v4l2_clk)) {
> +			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
> +			if (!strstr(sd->name, clk->dev_id))
> +				clk = ERR_PTR(-ENODEV);
> +		} else {
> +			clk = ERR_PTR(-EINVAL);
> +		}
> +	} else {
> +		list_for_each_entry(clk, &v4l2_clk, list) {
> +			if (!strcmp(id, clk->id) &&
> +			    !strcmp(sd->name, clk->dev_id))
> +				break;
> +		}
> +		if (&clk->list == &v4l2_clk)
> +			clk = ERR_PTR(-ENODEV);
> +	}
> +	mutex_unlock(&clk_lock);
> +
> +	if (!IS_ERR(clk) &&
> +	    !try_module_get(clk->ops->owner))
> +		clk = ERR_PTR(-ENODEV);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_get);
> +
> +void v4l2_clk_put(struct v4l2_clk *clk)
> +{
> +	if (!IS_ERR(clk))
> +		module_put(clk->ops->owner);
> +}
> +EXPORT_SYMBOL(v4l2_clk_put);
> +
> +int v4l2_clk_enable(struct v4l2_clk *clk)
> +{
> +	if (!clk->ops->enable)
> +		return -ENOSYS;
> +	return clk->ops->enable(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_enable);
> +
> +void v4l2_clk_disable(struct v4l2_clk *clk)
> +{
> +	if (clk->ops->disable)
> +		clk->ops->disable(clk);
> +}
> +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_name,
> +				   const char *name)
> +{
> +	struct v4l2_clk *clk;
> +
> +	if (!ops || !ops->owner || (!list_empty(&v4l2_clk) && !name))
> +		return ERR_PTR(-EINVAL);
> +
> +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +	if (!clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk->ops = ops;
> +	clk->id = name;
> +	clk->dev_id = dev_name;

What about kstrdup() ing name and dev_name here ? Otherwise callers would need 
to make sure that names are not stored in temporary locations (on the stack 
for instance), leading to possibly difficult to debug crashes.

We would then need a priv field in struct v4l2_clk/

> +
> +	mutex_lock(&clk_lock);
> +	list_add_tail(&clk->list, &v4l2_clk);
> +	mutex_unlock(&clk_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_register);
> +
> +void v4l2_clk_unregister(struct v4l2_clk *clk)
> +{
> +	mutex_lock(&clk_lock);
> +	list_del(&clk->list);
> +	mutex_unlock(&clk_lock);
> +
> +	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..0c05ab3
> --- /dev/null
> +++ b/include/media/v4l2-clk.h
> @@ -0,0 +1,48 @@
> +/*
> + * 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/list.h>
> +
> +struct module;
> +struct v4l2_subdev;
> +
> +struct v4l2_clk {
> +	struct list_head list;
> +	const struct v4l2_clk_ops *ops;

You should forward-declare struct v4l2_clk_ops.

> +	const char *dev_id;
> +	const char *id;
> +};
> +
> +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 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
Laurent Pinchart Oct. 22, 2012, 12:59 p.m. UTC | #5
Hi Guennadi,

A couple more comments.

On Saturday 20 October 2012 00:20:20 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.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/v4l2-core/Makefile   |    2 +-
>  drivers/media/v4l2-core/v4l2-clk.c |  126 +++++++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h           |   48 ++++++++++++++
>  3 files changed, 175 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 00f64d6..cb5fede 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..7d457e4
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,126 @@
> +/*
> + * 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/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(v4l2_clk);
> +
> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> +{
> +	struct v4l2_clk *clk = NULL;
> +
> +	mutex_lock(&clk_lock);
> +	if (!id) {
> +		if (list_is_singular(&v4l2_clk)) {
> +			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
> +			if (!strstr(sd->name, clk->dev_id))
> +				clk = ERR_PTR(-ENODEV);
> +		} else {
> +			clk = ERR_PTR(-EINVAL);
> +		}
> +	} else {
> +		list_for_each_entry(clk, &v4l2_clk, list) {
> +			if (!strcmp(id, clk->id) &&
> +			    !strcmp(sd->name, clk->dev_id))
> +				break;
> +		}
> +		if (&clk->list == &v4l2_clk)
> +			clk = ERR_PTR(-ENODEV);
> +	}
> +	mutex_unlock(&clk_lock);
> +
> +	if (!IS_ERR(clk) &&
> +	    !try_module_get(clk->ops->owner))
> +		clk = ERR_PTR(-ENODEV);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_get);
> +
> +void v4l2_clk_put(struct v4l2_clk *clk)
> +{
> +	if (!IS_ERR(clk))
> +		module_put(clk->ops->owner);
> +}
> +EXPORT_SYMBOL(v4l2_clk_put);
> +
> +int v4l2_clk_enable(struct v4l2_clk *clk)
> +{
> +	if (!clk->ops->enable)
> +		return -ENOSYS;

Are enable/disable supposed to be refcounted ? What about adding the refcount 
(or a boolean enable field) to struct v4l2_clk and handling it here and in 
v4l2_clk_disable() ?

> +	return clk->ops->enable(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_enable);
> +
> +void v4l2_clk_disable(struct v4l2_clk *clk)
> +{
> +	if (clk->ops->disable)
> +		clk->ops->disable(clk);
> +}
> +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);

What's the expected behaviour when calling v4l2_clk_get/set_rate on a disabled 
clock ?

> +}
> +EXPORT_SYMBOL(v4l2_clk_set_rate);
> +
> +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> +				   const char *dev_name,
> +				   const char *name)
> +{
> +	struct v4l2_clk *clk;
> +
> +	if (!ops || !ops->owner || (!list_empty(&v4l2_clk) && !name))
> +		return ERR_PTR(-EINVAL);
> +
> +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +	if (!clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk->ops = ops;
> +	clk->id = name;
> +	clk->dev_id = dev_name;
> +
> +	mutex_lock(&clk_lock);
> +	list_add_tail(&clk->list, &v4l2_clk);
> +	mutex_unlock(&clk_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_register);
> +
> +void v4l2_clk_unregister(struct v4l2_clk *clk)
> +{
> +	mutex_lock(&clk_lock);
> +	list_del(&clk->list);
> +	mutex_unlock(&clk_lock);
> +
> +	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..0c05ab3
> --- /dev/null
> +++ b/include/media/v4l2-clk.h
> @@ -0,0 +1,48 @@
> +/*
> + * 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/list.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;
> +};
> +
> +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 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
Laurent Pinchart Oct. 26, 2012, 2:05 a.m. UTC | #6
Hello,

On Sunday 21 October 2012 20:52:21 Sylwester Nawrocki wrote:
> On 10/20/2012 12:20 AM, 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.
> 
> So I gave this patch a try this weekend. I would have a few comments/
> questions. Thank you for sharing this!

I've finally found time to give it a try, and I can report successful results.

My development target here is a Beagleboard-xM with an MT9P031 sensor. With 
this patch and Sylwester's additional patches [1], I've been able to remove 
the board code callback from the mt9p031 driver platform data, as well as the 
last omap3-isp platform callback.

The result is available in the devel/v4l2-clock branch of 
http://git.linuxtv.org/pinchartl/media.git. Sylwester, that branch includes a 
minor fix titled "v4l2-clk: Fix clock id matching" for your "v4l2-clk: Rework 
to accept more than one clock with null clock id" patch. Could you please have 
a look at it ?

On the downside, there's now a circular dependency between the mt9p031 and 
omap3-isp drivers, so neither of them can be removed. That will need to be 
fixed.

[1] http://git.linuxtv.org/snawrocki/media.git/shortlog/refs/heads/s3c-camif-
devel
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 00f64d6..cb5fede 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..7d457e4
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -0,0 +1,126 @@ 
+/*
+ * 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/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(v4l2_clk);
+
+struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
+{
+	struct v4l2_clk *clk = NULL;
+
+	mutex_lock(&clk_lock);
+	if (!id) {
+		if (list_is_singular(&v4l2_clk)) {
+			clk = list_entry(&v4l2_clk, struct v4l2_clk, list);
+			if (!strstr(sd->name, clk->dev_id))
+				clk = ERR_PTR(-ENODEV);
+		} else {
+			clk = ERR_PTR(-EINVAL);
+		}
+	} else {
+		list_for_each_entry(clk, &v4l2_clk, list) {
+			if (!strcmp(id, clk->id) &&
+			    !strcmp(sd->name, clk->dev_id))
+				break;
+		}
+		if (&clk->list == &v4l2_clk)
+			clk = ERR_PTR(-ENODEV);
+	}
+	mutex_unlock(&clk_lock);
+
+	if (!IS_ERR(clk) &&
+	    !try_module_get(clk->ops->owner))
+		clk = ERR_PTR(-ENODEV);
+
+	return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_get);
+
+void v4l2_clk_put(struct v4l2_clk *clk)
+{
+	if (!IS_ERR(clk))
+		module_put(clk->ops->owner);
+}
+EXPORT_SYMBOL(v4l2_clk_put);
+
+int v4l2_clk_enable(struct v4l2_clk *clk)
+{
+	if (!clk->ops->enable)
+		return -ENOSYS;
+	return clk->ops->enable(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_enable);
+
+void v4l2_clk_disable(struct v4l2_clk *clk)
+{
+	if (clk->ops->disable)
+		clk->ops->disable(clk);
+}
+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_name,
+				   const char *name)
+{
+	struct v4l2_clk *clk;
+
+	if (!ops || !ops->owner || (!list_empty(&v4l2_clk) && !name))
+		return ERR_PTR(-EINVAL);
+
+	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->ops = ops;
+	clk->id = name;
+	clk->dev_id = dev_name;
+
+	mutex_lock(&clk_lock);
+	list_add_tail(&clk->list, &v4l2_clk);
+	mutex_unlock(&clk_lock);
+
+	return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_register);
+
+void v4l2_clk_unregister(struct v4l2_clk *clk)
+{
+	mutex_lock(&clk_lock);
+	list_del(&clk->list);
+	mutex_unlock(&clk_lock);
+
+	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..0c05ab3
--- /dev/null
+++ b/include/media/v4l2-clk.h
@@ -0,0 +1,48 @@ 
+/*
+ * 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/list.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;
+};
+
+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 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