| Message ID | Pine.LNX.4.64.1501022107370.3028@axis700.grange (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Guennadi, Thank you for the patch. On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote: > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > Date: Fri, 2 Jan 2015 12:26:41 +0100 > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API > > 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@gmx.de> > --- > > Hi Laurent, > Thanks for the comment. The idea of allocating a new object for each "get" > operation seems a bit weird to me, and completely trusting the user is a > bit scary... :) But yes, it can work this way too, I think, and the user > can screw either way too, anyway. The user needs to get it right for when we'll replace v4l2_clk_get() with clk_get(), so I'd rather force it to get it right now. > So, here comes a v2. Something like this? > > v2: don't add CCF-related clocks on the global list, just allocate a new > instance on each v4l2_clk_get() > > drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++--- > include/media/v4l2-clk.h | 2 ++ > 2 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 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> > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, > const char *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 (!IS_ERR(ccf_clk)) { > + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); > + if (!clk) { > + clk_put(ccf_clk); > + return ERR_PTR(-ENOMEM); > + } > + clk->clk = ccf_clk; > + > + return clk; > + } If clk_get() returns -EPROBE_DEFER I think you should return the error code to the user instead of falling back to the v4l2 clocks. > mutex_lock(&clk_lock); > clk = v4l2_clk_find(dev_name(dev), id); > @@ -61,6 +74,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) > @@ -98,8 +117,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_enable(clk->clk); Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call before enabling the clock. > + > + ret = v4l2_clk_lock_driver(clk); > if (ret < 0) > return ret; > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > { > int enable; > > + if (clk->clk) > + return clk_disable(clk->clk); Likewise, clk_disable_unprepare() ? > + > mutex_lock(&clk->lock); > > enable = --clk->enable; > @@ -142,8 +168,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; > > @@ -162,7 +192,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 8f06967..4402b2d 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; > @@ -30,6 +31,7 @@ struct v4l2_clk { > int enable; > struct mutex lock; /* Protect the enable count */ > atomic_t use_count; > + struct clk *clk; > void *priv; > };
On Sun, 4 Jan 2015, Laurent Pinchart wrote: > Hi Guennadi, > > Thank you for the patch. > > On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote: > > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 > > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > Date: Fri, 2 Jan 2015 12:26:41 +0100 > > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API > > > > 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@gmx.de> > > --- > > > > Hi Laurent, > > Thanks for the comment. The idea of allocating a new object for each "get" > > operation seems a bit weird to me, and completely trusting the user is a > > bit scary... :) But yes, it can work this way too, I think, and the user > > can screw either way too, anyway. > > The user needs to get it right for when we'll replace v4l2_clk_get() with > clk_get(), so I'd rather force it to get it right now. > > > So, here comes a v2. Something like this? > > > > v2: don't add CCF-related clocks on the global list, just allocate a new > > instance on each v4l2_clk_get() > > > > drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++--- > > include/media/v4l2-clk.h | 2 ++ > > 2 files changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 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> > > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, > > const char *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 (!IS_ERR(ccf_clk)) { > > + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); > > + if (!clk) { > > + clk_put(ccf_clk); > > + return ERR_PTR(-ENOMEM); > > + } > > + clk->clk = ccf_clk; > > + > > + return clk; > > + } > > If clk_get() returns -EPROBE_DEFER I think you should return the error code to > the user instead of falling back to the v4l2 clocks. This is the case, when a clock is correctly set up in DT, but no suitable provider is registered yet, right? I think such a case isn't very likely, but yes, can do, makes more sense, perhaps. > > mutex_lock(&clk_lock); > > clk = v4l2_clk_find(dev_name(dev), id); > > @@ -61,6 +74,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) > > @@ -98,8 +117,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_enable(clk->clk); > > Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call before > enabling the clock. Right, my fault, thanks for pointing out. > > + > > + ret = v4l2_clk_lock_driver(clk); > > if (ret < 0) > > return ret; > > > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > { > > int enable; > > > > + if (clk->clk) > > + return clk_disable(clk->clk); > > Likewise, clk_disable_unprepare() ? Sure. Ok, these are trivial changes, perhaps we can wait for Josh's test results, then I can submit a v3 with (hopefully) all required changes. Thanks Guennadi > > + > > mutex_lock(&clk->lock); > > > > enable = --clk->enable; > > @@ -142,8 +168,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; > > > > @@ -162,7 +192,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 8f06967..4402b2d 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; > > @@ -30,6 +31,7 @@ struct v4l2_clk { > > int enable; > > struct mutex lock; /* Protect the enable count */ > > atomic_t use_count; > > + struct clk *clk; > > void *priv; > > }; > > -- > Regards, > > Laurent Pinchart > -- 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
Hi Guennadi, On Sunday 04 January 2015 17:45:04 Guennadi Liakhovetski wrote: > On Sun, 4 Jan 2015, Laurent Pinchart wrote: > > On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote: > > > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 > > > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > Date: Fri, 2 Jan 2015 12:26:41 +0100 > > > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API > > > > > > 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@gmx.de> > > > --- > > > > > > Hi Laurent, > > > Thanks for the comment. The idea of allocating a new object for each > > > "get" operation seems a bit weird to me, and completely trusting the > > > user is a bit scary... :) But yes, it can work this way too, I think, > > > and the user can screw either way too, anyway. > > > > The user needs to get it right for when we'll replace v4l2_clk_get() with > > clk_get(), so I'd rather force it to get it right now. > > > > > So, here comes a v2. Something like this? > > > > > > v2: don't add CCF-related clocks on the global list, just allocate a new > > > instance on each v4l2_clk_get() > > > > > > drivers/media/v4l2-core/v4l2-clk.c | 45 ++++++++++++++++++++++++++++--- > > > include/media/v4l2-clk.h | 2 ++ > > > 2 files changed, 44 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > > b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 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> > > > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char > > > *dev_id, const char *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 (!IS_ERR(ccf_clk)) { > > > + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); > > > + if (!clk) { > > > + clk_put(ccf_clk); > > > + return ERR_PTR(-ENOMEM); > > > + } > > > + clk->clk = ccf_clk; > > > + > > > + return clk; > > > + } > > > > If clk_get() returns -EPROBE_DEFER I think you should return the error > > code to the user instead of falling back to the v4l2 clocks. > > This is the case, when a clock is correctly set up in DT, but no suitable > provider is registered yet, right? I think such a case isn't very likely, > but yes, can do, makes more sense, perhaps. Yes, that's the use case. Even if it's not that likely I think it should still be implemented, if only to mimic the CCF behaviour and prepare drivers for the switch away from v4l2_clk_get(). > > > mutex_lock(&clk_lock); > > > clk = v4l2_clk_find(dev_name(dev), id); > > > > > > @@ -61,6 +74,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) > > > > > > @@ -98,8 +117,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_enable(clk->clk); > > > > Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call > > before enabling the clock. > > Right, my fault, thanks for pointing out. > > > > + > > > + ret = v4l2_clk_lock_driver(clk); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > > > > > { > > > > > > int enable; > > > > > > + if (clk->clk) > > > + return clk_disable(clk->clk); > > > > Likewise, clk_disable_unprepare() ? > > Sure. Ok, these are trivial changes, perhaps we can wait for Josh's test > results, then I can submit a v3 with (hopefully) all required changes. Sure. CCF will trigger warnings due to clk_prepare() not being called though. > > > + > > > > > > mutex_lock(&clk->lock); > > > > > > enable = --clk->enable; > > > > > > @@ -142,8 +168,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; > > > > > > @@ -162,7 +192,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 8f06967..4402b2d 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; > > > > > > @@ -30,6 +31,7 @@ struct v4l2_clk { > > > > > > int enable; > > > struct mutex lock; /* Protect the enable count */ > > > atomic_t use_count; > > > > > > + struct clk *clk; > > > > > > void *priv; > > > > > > };
Hi, Guennadi On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote: > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > Date: Fri, 2 Jan 2015 12:26:41 +0100 > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API > > 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@gmx.de> > --- > > Hi Laurent, > Thanks for the comment. The idea of allocating a new object for each "get" > operation seems a bit weird to me, and completely trusting the user is a > bit scary... :) But yes, it can work this way too, I think, and the user > can screw either way too, anyway. So, here comes a v2. Something like > this? > > v2: don't add CCF-related clocks on the global list, just allocate a new > instance on each v4l2_clk_get() > > drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++++++--- > include/media/v4l2-clk.h | 2 ++ > 2 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c > index c210906..f5d1688 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> > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *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); We need to check whether the 'id' is NULL, otherwise there is no error reported if we use IS_ERR(ccf_clk). Best Regards, Josh Wu > + > + if (!IS_ERR(ccf_clk)) { > + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); > + if (!clk) { > + clk_put(ccf_clk); > + return ERR_PTR(-ENOMEM); > + } > + clk->clk = ccf_clk; > + > + return clk; > + } > > mutex_lock(&clk_lock); > clk = v4l2_clk_find(dev_name(dev), id); > @@ -61,6 +74,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) > @@ -98,8 +117,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_enable(clk->clk); > + > + ret = v4l2_clk_lock_driver(clk); > if (ret < 0) > return ret; > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > { > int enable; > > + if (clk->clk) > + return clk_disable(clk->clk); > + > mutex_lock(&clk->lock); > > enable = --clk->enable; > @@ -142,8 +168,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; > > @@ -162,7 +192,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 8f06967..4402b2d 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; > @@ -30,6 +31,7 @@ struct v4l2_clk { > int enable; > struct mutex lock; /* Protect the enable count */ > atomic_t use_count; > + struct clk *clk; > void *priv; > }; > -- 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 Mon, 5 Jan 2015, Josh Wu wrote: > Hi, Guennadi > > On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote: > > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 > > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > Date: Fri, 2 Jan 2015 12:26:41 +0100 > > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API > > > > 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@gmx.de> > > --- > > > > Hi Laurent, > > Thanks for the comment. The idea of allocating a new object for each "get" > > operation seems a bit weird to me, and completely trusting the user is a > > bit scary... :) But yes, it can work this way too, I think, and the user > > can screw either way too, anyway. So, here comes a v2. Something like > > this? > > > > v2: don't add CCF-related clocks on the global list, just allocate a new > > instance on each v4l2_clk_get() > > > > drivers/media/v4l2-core/v4l2-clk.c | 45 > > +++++++++++++++++++++++++++++++++++--- > > include/media/v4l2-clk.h | 2 ++ > > 2 files changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > b/drivers/media/v4l2-core/v4l2-clk.c > > index c210906..f5d1688 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> > > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, > > const char *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); > We need to check whether the 'id' is NULL, otherwise there is no error > reported if we use IS_ERR(ccf_clk). ? Sorry, don't understand. AFAIK, id = NULL is absolutely valid, no need to check for it explicitly. However, it just occurs to me, that we probably need to change the below test > Best Regards, > Josh Wu > > > + > > + if (!IS_ERR(ccf_clk)) { to > > + if (!IS_ERR_OR_NULL(ccf_clk)) { to cover the case when CCF isn't built at all and a stub is used. In that case it's probably better to fall back to the V4L2 clock emulation. Thanks Guennadi > > + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); > > + if (!clk) { > > + clk_put(ccf_clk); > > + return ERR_PTR(-ENOMEM); > > + } > > + clk->clk = ccf_clk; > > + > > + return clk; > > + } > > mutex_lock(&clk_lock); > > clk = v4l2_clk_find(dev_name(dev), id); > > @@ -61,6 +74,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) > > @@ -98,8 +117,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_enable(clk->clk); > > + > > + ret = v4l2_clk_lock_driver(clk); > > if (ret < 0) > > return ret; > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > { > > int enable; > > + if (clk->clk) > > + return clk_disable(clk->clk); > > + > > mutex_lock(&clk->lock); > > enable = --clk->enable; > > @@ -142,8 +168,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; > > @@ -162,7 +192,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 8f06967..4402b2d 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; > > @@ -30,6 +31,7 @@ struct v4l2_clk { > > int enable; > > struct mutex lock; /* Protect the enable count */ > > atomic_t use_count; > > + struct clk *clk; > > void *priv; > > }; > > -- 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 1/5/2015 5:36 PM, Guennadi Liakhovetski wrote: > On Mon, 5 Jan 2015, Josh Wu wrote: > >> Hi, Guennadi >> >> On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote: >>> From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001 >>> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> Date: Fri, 2 Jan 2015 12:26:41 +0100 >>> Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API >>> >>> 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@gmx.de> >>> --- >>> >>> Hi Laurent, >>> Thanks for the comment. The idea of allocating a new object for each "get" >>> operation seems a bit weird to me, and completely trusting the user is a >>> bit scary... :) But yes, it can work this way too, I think, and the user >>> can screw either way too, anyway. So, here comes a v2. Something like >>> this? >>> >>> v2: don't add CCF-related clocks on the global list, just allocate a new >>> instance on each v4l2_clk_get() >>> >>> drivers/media/v4l2-core/v4l2-clk.c | 45 >>> +++++++++++++++++++++++++++++++++++--- >>> include/media/v4l2-clk.h | 2 ++ >>> 2 files changed, 44 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c >>> b/drivers/media/v4l2-core/v4l2-clk.c >>> index c210906..f5d1688 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> >>> @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, >>> const char *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); >> We need to check whether the 'id' is NULL, otherwise there is no error >> reported if we use IS_ERR(ccf_clk). > ? Sorry, don't understand. AFAIK, id = NULL is absolutely valid, no need > to check for it explicitly. However, it just occurs to me, that we > probably need to change the below test > >> Best Regards, >> Josh Wu >> >>> + >>> + if (!IS_ERR(ccf_clk)) { > to > >>> + if (!IS_ERR_OR_NULL(ccf_clk)) { > to cover the case when CCF isn't built at all and a stub is used. In that > case it's probably better to fall back to the V4L2 clock emulation. yes, this's what I mean. but I don't know the function IS_ERR_OR_NULL(). ;-) Best Regards, Josh Wu > > Thanks > Guennadi > >>> + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); >>> + if (!clk) { >>> + clk_put(ccf_clk); >>> + return ERR_PTR(-ENOMEM); >>> + } >>> + clk->clk = ccf_clk; >>> + >>> + return clk; >>> + } >>> mutex_lock(&clk_lock); >>> clk = v4l2_clk_find(dev_name(dev), id); >>> @@ -61,6 +74,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) >>> @@ -98,8 +117,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_enable(clk->clk); >>> + >>> + ret = v4l2_clk_lock_driver(clk); >>> if (ret < 0) >>> return ret; >>> @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) >>> { >>> int enable; >>> + if (clk->clk) >>> + return clk_disable(clk->clk); >>> + >>> mutex_lock(&clk->lock); >>> enable = --clk->enable; >>> @@ -142,8 +168,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; >>> @@ -162,7 +192,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 8f06967..4402b2d 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; >>> @@ -30,6 +31,7 @@ struct v4l2_clk { >>> int enable; >>> struct mutex lock; /* Protect the enable count */ >>> atomic_t use_count; >>> + struct clk *clk; >>> void *priv; >>> }; >>> -- 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 --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 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> @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *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 (!IS_ERR(ccf_clk)) { + clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL); + if (!clk) { + clk_put(ccf_clk); + return ERR_PTR(-ENOMEM); + } + clk->clk = ccf_clk; + + return clk; + } mutex_lock(&clk_lock); clk = v4l2_clk_find(dev_name(dev), id); @@ -61,6 +74,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) @@ -98,8 +117,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_enable(clk->clk); + + ret = v4l2_clk_lock_driver(clk); if (ret < 0) return ret; @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk) { int enable; + if (clk->clk) + return clk_disable(clk->clk); + mutex_lock(&clk->lock); enable = --clk->enable; @@ -142,8 +168,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; @@ -162,7 +192,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 8f06967..4402b2d 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; @@ -30,6 +31,7 @@ struct v4l2_clk { int enable; struct mutex lock; /* Protect the enable count */ atomic_t use_count; + struct clk *clk; void *priv; };