Hi Mauro,
On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@xxxxxx> escreveu:
>
> > V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> > have to be supplied by a different V4L2 driver, they can also be
> > supplied by an independent source. In this case the standart kernel
> > clock API should be used to handle such clocks. This patch adds support
> > for such cases.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> >
> > v4: sizeof(*clk) :)
> >
> > drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
> > include/media/v4l2-clk.h | 2 ++
> > 2 files changed, 47 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > index 3ff0b00..9f8cb20 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -9,6 +9,7 @@
> > */
> >
> > #include <linux/atomic.h>
> > +#include <linux/clk.h>
> > #include <linux/device.h>
> > #include <linux/errno.h>
> > #include <linux/list.h>
> > @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id)
> > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
> > {
> > struct v4l2_clk *clk;
> > + struct clk *ccf_clk = clk_get(dev, id);
> > +
> > + if (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> > + return ERR_PTR(-EPROBE_DEFER);
>
> Why not do just:
> return ccf_clk;
I find the explicit error slightly more readable, but that's a matter of taste.
> > +
> > + if (!IS_ERR_OR_NULL(ccf_clk)) {
> > + clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > + if (!clk) {
> > + clk_put(ccf_clk);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + clk->clk = ccf_clk;
> > +
> > + return clk;
> > + }
>
> The error condition here looks a little weird to me. I mean, if the
> CCF clock returns an error, shouldn't it fail instead of silently
> run some logic to find another clock source? Isn't it risky on getting
> a wrong value?
The idea is that, in the long term, everything should use CCF directly. However, we have clock providers on platforms where CCF isn't avalaible. V4L2 clock has been introduced as a single API usable by V4L2 clock users allowing them to retrieve and use clocks regardless of whether the provider uses CCF or not. Internally it first tries CCF, and then falls back to the non-CCF implementation in case of failure.
> If the above code is right, please add a comment there explaining
> why it is safe to discard the CCF clock error.
>
> >
> > mutex_lock(&clk_lock);
> > clk = v4l2_clk_find(dev_name(dev));
> > @@ -56,6 +72,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
> > if (IS_ERR(clk))
> > return;
> >
> > + if (clk->clk) {
> > + clk_put(clk->clk);
> > + kfree(clk);
> > + return;
> > + }
> > +
> > mutex_lock(&clk_lock);
> >
> > list_for_each_entry(tmp, &clk_list, list)
> > @@ -93,8 +115,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
> >
> > int v4l2_clk_enable(struct v4l2_clk *clk)
> > {
> > - int ret = v4l2_clk_lock_driver(clk);
> > + int ret;
> >
> > + if (clk->clk)
> > + return clk_prepare_enable(clk->clk);
> > +
> > + ret = v4l2_clk_lock_driver(clk);
> > if (ret < 0)
> > return ret;
> >
> > @@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> > {
> > int enable;
> >
> > + if (clk->clk)
> > + return clk_disable_unprepare(clk->clk);
> > +
> > mutex_lock(&clk->lock);
> >
> > enable = --clk->enable;
> > @@ -137,8 +166,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> >
> > unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> > {
> > - int ret = v4l2_clk_lock_driver(clk);
> > + int ret;
> > +
> > + if (clk->clk)
> > + return clk_get_rate(clk->clk);
> >
> > + ret = v4l2_clk_lock_driver(clk);
> > if (ret < 0)
> > return ret;
> >
> > @@ -157,7 +190,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> >
> > int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> > {
> > - int ret = v4l2_clk_lock_driver(clk);
> > + int ret;
> > +
> > + if (clk->clk) {
> > + long r = clk_round_rate(clk->clk, rate);
> > + if (r < 0)
> > + return r;
> > + return clk_set_rate(clk->clk, r);
> > + }
> > +
> > + ret = v4l2_clk_lock_driver(clk);
> >
> > if (ret < 0)
> > return ret;
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 928045f..3ef6e3d 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -22,6 +22,7 @@
> > struct module;
> > struct device;
> >
> > +struct clk;
> > struct v4l2_clk {
> > struct list_head list;
> > const struct v4l2_clk_ops *ops;
> > @@ -29,6 +30,7 @@ struct v4l2_clk {
> > int enable;
> > struct mutex lock; /* Protect the enable count */
> > atomic_t use_count;
> > + struct clk *clk;
> > void *priv;
> > };
> >
>��.n��������+%������w��{.n�����{��g����^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
