| Message ID | Pine.LNX.4.64.1501021247310.30761@axis700.grange (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Guennadi On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: > All uses of the v4l2_clk API so far only register one clock with a fixed > name. This allows us to get rid of it, which also will make CCF and DT > integration easier. This patch not register a v4l2_clk with name: "mclk". So this will break all the i2c sensors that used the v4l2 clock "mclk". To make those drivers work, we need change all lines from: v4l2_clk_get(&client->dev, "mclk"); to: v4l2_clk_get(&client->dev, NULL); Am I understanding correctly? if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and all code will use: v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID); > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- > drivers/media/usb/em28xx/em28xx-camera.c | 2 +- > drivers/media/v4l2-core/v4l2-clk.c | 24 +++++++++++------------- > include/media/v4l2-clk.h | 7 +++---- > 4 files changed, 18 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c > index f4be2a1..ce192b6 100644 > --- a/drivers/media/platform/soc_camera/soc_camera.c > +++ b/drivers/media/platform/soc_camera/soc_camera.c > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd, > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > shd->i2c_adapter_id, shd->board_info->addr); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici, > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, > snprintf(clk_name, sizeof(clk_name), "of-%s", > of_node_full_name(remote)); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c > index 7be661f..a4b22c2 100644 > --- a/drivers/media/usb/em28xx/em28xx-camera.c > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name), > i2c_adapter_id(adap), client->addr); > - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); > + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); > if (IS_ERR(v4l2->clk)) > return PTR_ERR(v4l2->clk); > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c > index e18cc04..c210906 100644 > --- a/drivers/media/v4l2-core/v4l2-clk.c > +++ b/drivers/media/v4l2-core/v4l2-clk.c > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id) > if (strcmp(dev_id, clk->dev_id)) > continue; > > - if (!id || !clk->id || !strcmp(clk->id, id)) > + if ((!id && !clk->id) || > + (id && clk->id && !strcmp(clk->id, id))) > return clk; > } > > @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > mutex_lock(&clk->lock); > > enable = --clk->enable; > - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, > - clk->dev_id, clk->id)) > + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, > + clk->dev_id)) > clk->enable++; > else if (!enable && clk->ops->disable) > clk->ops->disable(clk); > @@ -181,7 +182,7 @@ 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) > + void *priv) > { > struct v4l2_clk *clk; > int ret; > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > 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) { > + if (!clk->dev_id) { > ret = -ENOMEM; > goto ealloc; > } > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > mutex_init(&clk->lock); > > mutex_lock(&clk_lock); > - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { So the camera sensor driver should request the v4l2 clock with id is NULL? How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like: #define SOC_CAMERA_HOST_CLK_ID NULL then we can rewrite this block code to: clk->ops = ops; clk->priv = priv; + clk->id = SOC_CAMERA_HOST_CLK_ID; atomic_set(&clk->use_count, 0); mutex_init(&clk->lock); mutex_lock(&clk_lock); if (!IS_ERR(v4l2_clk_find(dev_id, id))) { ... ... Best Regards, Josh Wu > mutex_unlock(&clk_lock); > ret = -EEXIST; > goto eexist; > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > > eexist: > ealloc: > - kfree(clk->id); > kfree(clk->dev_id); > kfree(clk); > return ERR_PTR(ret); > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); > void v4l2_clk_unregister(struct v4l2_clk *clk) > { > if (WARN(atomic_read(&clk->use_count), > - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", > - __func__, clk->dev_id, clk->id)) > + "%s(): Refusing to unregister ref-counted %s clock!\n", > + __func__, clk->dev_id)) > return; > > mutex_lock(&clk_lock); > list_del(&clk->list); > mutex_unlock(&clk_lock); > > - kfree(clk->id); > kfree(clk->dev_id); > kfree(clk); > } > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk) > } > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > - const char *id, unsigned long rate, struct module *owner) > + unsigned long rate, struct module *owner) > { > struct v4l2_clk *clk; > struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > priv->ops.get_rate = fixed_get_rate; > priv->ops.owner = owner; > > - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); > + clk = v4l2_clk_register(&priv->ops, dev_id, priv); > if (IS_ERR(clk)) > kfree(priv); > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h > index 0b36cc1..8f06967 100644 > --- a/include/media/v4l2-clk.h > +++ b/include/media/v4l2-clk.h > @@ -43,7 +43,7 @@ struct v4l2_clk_ops { > > struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > const char *dev_name, > - const char *name, void *priv); > + void *priv); > void v4l2_clk_unregister(struct v4l2_clk *clk); > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); > void v4l2_clk_put(struct v4l2_clk *clk); > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate); > struct module; > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > - const char *id, unsigned long rate, struct module *owner); > + unsigned long rate, struct module *owner); > void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); > > static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id, > - const char *id, > unsigned long rate) > { > - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); > + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); > } > > #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \ -- 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 Josh, On Mon, 5 Jan 2015, Josh Wu wrote: > Hi, Guennadi > > On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: > > All uses of the v4l2_clk API so far only register one clock with a fixed > > name. This allows us to get rid of it, which also will make CCF and DT > > integration easier. > This patch not register a v4l2_clk with name: "mclk". So this will break all > the i2c sensors that used the v4l2 clock "mclk". > To make those drivers work, we need change all lines from: > v4l2_clk_get(&client->dev, "mclk"); > to: > v4l2_clk_get(&client->dev, NULL); > > Am I understanding correctly? No, it's the opposite. Clock consumers should request clock names, according to their datasheets. In ov2640 case it's xvclk. For v4l2_clk the name will simply be ignored. But it will be used to try to get a CCF clock before falling back to V4L2 clocks. Thanks Guennadi > if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and all > code will use: > v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID); > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > --- > > drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- > > drivers/media/usb/em28xx/em28xx-camera.c | 2 +- > > drivers/media/v4l2-core/v4l2-clk.c | 24 > > +++++++++++------------- > > include/media/v4l2-clk.h | 7 +++---- > > 4 files changed, 18 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c > > b/drivers/media/platform/soc_camera/soc_camera.c > > index f4be2a1..ce192b6 100644 > > --- a/drivers/media/platform/soc_camera/soc_camera.c > > +++ b/drivers/media/platform/soc_camera/soc_camera.c > > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct > > soc_camera_device *icd, > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > shd->i2c_adapter_id, shd->board_info->addr); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host > > *ici, > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, > > snprintf(clk_name, sizeof(clk_name), "of-%s", > > of_node_full_name(remote)); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c > > b/drivers/media/usb/em28xx/em28xx-camera.c > > index 7be661f..a4b22c2 100644 > > --- a/drivers/media/usb/em28xx/em28xx-camera.c > > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name), > > i2c_adapter_id(adap), client->addr); > > - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); > > + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); > > if (IS_ERR(v4l2->clk)) > > return PTR_ERR(v4l2->clk); > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > b/drivers/media/v4l2-core/v4l2-clk.c > > index e18cc04..c210906 100644 > > --- a/drivers/media/v4l2-core/v4l2-clk.c > > +++ b/drivers/media/v4l2-core/v4l2-clk.c > > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, > > const char *id) > > if (strcmp(dev_id, clk->dev_id)) > > continue; > > - if (!id || !clk->id || !strcmp(clk->id, id)) > > + if ((!id && !clk->id) || > > + (id && clk->id && !strcmp(clk->id, id))) > > return clk; > > } > > @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > mutex_lock(&clk->lock); > > enable = --clk->enable; > > - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, > > - clk->dev_id, clk->id)) > > + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, > > + clk->dev_id)) > > clk->enable++; > > else if (!enable && clk->ops->disable) > > clk->ops->disable(clk); > > @@ -181,7 +182,7 @@ 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) > > + void *priv) > > { > > struct v4l2_clk *clk; > > int ret; > > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > 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) { > > + if (!clk->dev_id) { > > ret = -ENOMEM; > > goto ealloc; > > } > > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > mutex_init(&clk->lock); > > mutex_lock(&clk_lock); > > - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > > + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { > So the camera sensor driver should request the v4l2 clock with id is NULL? > > How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like: > #define SOC_CAMERA_HOST_CLK_ID NULL > > then we can rewrite this block code to: > > clk->ops = ops; > clk->priv = priv; > + clk->id = SOC_CAMERA_HOST_CLK_ID; > atomic_set(&clk->use_count, 0); > mutex_init(&clk->lock); > > mutex_lock(&clk_lock); > if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > ... ... > > Best Regards, > Josh Wu > > > mutex_unlock(&clk_lock); > > ret = -EEXIST; > > goto eexist; > > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > eexist: > > ealloc: > > - kfree(clk->id); > > kfree(clk->dev_id); > > kfree(clk); > > return ERR_PTR(ret); > > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); > > void v4l2_clk_unregister(struct v4l2_clk *clk) > > { > > if (WARN(atomic_read(&clk->use_count), > > - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", > > - __func__, clk->dev_id, clk->id)) > > + "%s(): Refusing to unregister ref-counted %s clock!\n", > > + __func__, clk->dev_id)) > > return; > > mutex_lock(&clk_lock); > > list_del(&clk->list); > > mutex_unlock(&clk_lock); > > - kfree(clk->id); > > kfree(clk->dev_id); > > kfree(clk); > > } > > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk > > *clk) > > } > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > - const char *id, unsigned long rate, struct module *owner) > > + unsigned long rate, struct module *owner) > > { > > struct v4l2_clk *clk; > > struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char > > *dev_id, > > priv->ops.get_rate = fixed_get_rate; > > priv->ops.owner = owner; > > - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); > > + clk = v4l2_clk_register(&priv->ops, dev_id, priv); > > if (IS_ERR(clk)) > > kfree(priv); > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h > > index 0b36cc1..8f06967 100644 > > --- a/include/media/v4l2-clk.h > > +++ b/include/media/v4l2-clk.h > > @@ -43,7 +43,7 @@ struct v4l2_clk_ops { > > struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > > const char *dev_name, > > - const char *name, void *priv); > > + void *priv); > > void v4l2_clk_unregister(struct v4l2_clk *clk); > > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); > > void v4l2_clk_put(struct v4l2_clk *clk); > > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned > > long rate); > > struct module; > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > - const char *id, unsigned long rate, struct module *owner); > > + unsigned long rate, struct module *owner); > > void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); > > static inline struct v4l2_clk *v4l2_clk_register_fixed(const char > > *dev_id, > > - const char *id, > > unsigned long rate) > > { > > - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); > > + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); > > } > > #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, > > \ > -- 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 1/5/2015 5:28 PM, Guennadi Liakhovetski wrote: > Hi Josh, > > On Mon, 5 Jan 2015, Josh Wu wrote: > >> Hi, Guennadi >> >> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: >>> All uses of the v4l2_clk API so far only register one clock with a fixed >>> name. This allows us to get rid of it, which also will make CCF and DT >>> integration easier. >> This patch not register a v4l2_clk with name: "mclk". So this will break all >> the i2c sensors that used the v4l2 clock "mclk". >> To make those drivers work, we need change all lines from: >> v4l2_clk_get(&client->dev, "mclk"); >> to: >> v4l2_clk_get(&client->dev, NULL); >> >> Am I understanding correctly? > No, it's the opposite. Clock consumers should request clock names, > according to their datasheets. In ov2640 case it's xvclk. For v4l2_clk the > name will simply be ignored. But it will be used to try to get a CCF clock > before falling back to V4L2 clocks. Okay, you mean I only need request xvclk clock via call v4l2_clk_get() in ov2640 driver. right? So the question is how do we operation the camera host's .clock_start/stop()? Which is by v4l2_clk_get() old "mclk". hmm, wait. Since this part is not belong to ov2640, I think we should move this part code to soc_camera.c 's function: soc_camera_power_on(). or do I miss anything? > > Thanks > Guennadi Best Regards, Josh Wu >> if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and all >> code will use: >> v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID); >> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> --- >>> drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- >>> drivers/media/usb/em28xx/em28xx-camera.c | 2 +- >>> drivers/media/v4l2-core/v4l2-clk.c | 24 >>> +++++++++++------------- >>> include/media/v4l2-clk.h | 7 +++---- >>> 4 files changed, 18 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c >>> b/drivers/media/platform/soc_camera/soc_camera.c >>> index f4be2a1..ce192b6 100644 >>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct >>> soc_camera_device *icd, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> shd->i2c_adapter_id, shd->board_info->addr); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host >>> *ici, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, >>> snprintf(clk_name, sizeof(clk_name), "of-%s", >>> of_node_full_name(remote)); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c >>> b/drivers/media/usb/em28xx/em28xx-camera.c >>> index 7be661f..a4b22c2 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-camera.c >>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c >>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) >>> v4l2_clk_name_i2c(clk_name, sizeof(clk_name), >>> i2c_adapter_id(adap), client->addr); >>> - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); >>> + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); >>> if (IS_ERR(v4l2->clk)) >>> return PTR_ERR(v4l2->clk); >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c >>> b/drivers/media/v4l2-core/v4l2-clk.c >>> index e18cc04..c210906 100644 >>> --- a/drivers/media/v4l2-core/v4l2-clk.c >>> +++ b/drivers/media/v4l2-core/v4l2-clk.c >>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, >>> const char *id) >>> if (strcmp(dev_id, clk->dev_id)) >>> continue; >>> - if (!id || !clk->id || !strcmp(clk->id, id)) >>> + if ((!id && !clk->id) || >>> + (id && clk->id && !strcmp(clk->id, id))) >>> return clk; >>> } >>> @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) >>> mutex_lock(&clk->lock); >>> enable = --clk->enable; >>> - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, >>> - clk->dev_id, clk->id)) >>> + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, >>> + clk->dev_id)) >>> clk->enable++; >>> else if (!enable && clk->ops->disable) >>> clk->ops->disable(clk); >>> @@ -181,7 +182,7 @@ 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) >>> + void *priv) >>> { >>> struct v4l2_clk *clk; >>> int ret; >>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> 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) { >>> + if (!clk->dev_id) { >>> ret = -ENOMEM; >>> goto ealloc; >>> } >>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> mutex_init(&clk->lock); >>> mutex_lock(&clk_lock); >>> - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { >>> + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { >> So the camera sensor driver should request the v4l2 clock with id is NULL? >> >> How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like: >> #define SOC_CAMERA_HOST_CLK_ID NULL >> >> then we can rewrite this block code to: >> >> clk->ops = ops; >> clk->priv = priv; >> + clk->id = SOC_CAMERA_HOST_CLK_ID; >> atomic_set(&clk->use_count, 0); >> mutex_init(&clk->lock); >> >> mutex_lock(&clk_lock); >> if (!IS_ERR(v4l2_clk_find(dev_id, id))) { >> ... ... >> >> Best Regards, >> Josh Wu >> >>> mutex_unlock(&clk_lock); >>> ret = -EEXIST; >>> goto eexist; >>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> eexist: >>> ealloc: >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> return ERR_PTR(ret); >>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); >>> void v4l2_clk_unregister(struct v4l2_clk *clk) >>> { >>> if (WARN(atomic_read(&clk->use_count), >>> - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", >>> - __func__, clk->dev_id, clk->id)) >>> + "%s(): Refusing to unregister ref-counted %s clock!\n", >>> + __func__, clk->dev_id)) >>> return; >>> mutex_lock(&clk_lock); >>> list_del(&clk->list); >>> mutex_unlock(&clk_lock); >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> } >>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk >>> *clk) >>> } >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner) >>> + unsigned long rate, struct module *owner) >>> { >>> struct v4l2_clk *clk; >>> struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char >>> *dev_id, >>> priv->ops.get_rate = fixed_get_rate; >>> priv->ops.owner = owner; >>> - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); >>> + clk = v4l2_clk_register(&priv->ops, dev_id, priv); >>> if (IS_ERR(clk)) >>> kfree(priv); >>> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h >>> index 0b36cc1..8f06967 100644 >>> --- a/include/media/v4l2-clk.h >>> +++ b/include/media/v4l2-clk.h >>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops { >>> struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, >>> const char *dev_name, >>> - const char *name, void *priv); >>> + void *priv); >>> void v4l2_clk_unregister(struct v4l2_clk *clk); >>> struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); >>> void v4l2_clk_put(struct v4l2_clk *clk); >>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned >>> long rate); >>> struct module; >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner); >>> + unsigned long rate, struct module *owner); >>> void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); >>> static inline struct v4l2_clk *v4l2_clk_register_fixed(const char >>> *dev_id, >>> - const char *id, >>> unsigned long rate) >>> { >>> - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); >>> + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); >>> } >>> #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, >>> \ -- 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 After look deep into this patch, I found you miss one line that should be changed as well. It's In function v4l2_clk_get(), there still has one line code called v4l2_clk_find(dev_id, id). You need to change it to v4l2_clk_find(dev_id, NULL) as well. Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk") cannot acquired the "mclk" clock. After above changes, this patch works for me. On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: > All uses of the v4l2_clk API so far only register one clock with a fixed > name. This allows us to get rid of it, which also will make CCF and DT > integration easier. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- > drivers/media/usb/em28xx/em28xx-camera.c | 2 +- > drivers/media/v4l2-core/v4l2-clk.c | 24 +++++++++++------------- > include/media/v4l2-clk.h | 7 +++---- > 4 files changed, 18 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c > index f4be2a1..ce192b6 100644 > --- a/drivers/media/platform/soc_camera/soc_camera.c > +++ b/drivers/media/platform/soc_camera/soc_camera.c > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd, > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > shd->i2c_adapter_id, shd->board_info->addr); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici, > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, > snprintf(clk_name, sizeof(clk_name), "of-%s", > of_node_full_name(remote)); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > if (IS_ERR(icd->clk)) { > ret = PTR_ERR(icd->clk); > goto eclkreg; > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c > index 7be661f..a4b22c2 100644 > --- a/drivers/media/usb/em28xx/em28xx-camera.c > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name), > i2c_adapter_id(adap), client->addr); > - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); > + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); > if (IS_ERR(v4l2->clk)) > return PTR_ERR(v4l2->clk); > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c > index e18cc04..c210906 100644 > --- a/drivers/media/v4l2-core/v4l2-clk.c > +++ b/drivers/media/v4l2-core/v4l2-clk.c > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id) > if (strcmp(dev_id, clk->dev_id)) > continue; > > - if (!id || !clk->id || !strcmp(clk->id, id)) > + if ((!id && !clk->id) || > + (id && clk->id && !strcmp(clk->id, id))) > return clk; > } > > @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > mutex_lock(&clk->lock); > > enable = --clk->enable; > - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, > - clk->dev_id, clk->id)) > + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, > + clk->dev_id)) > clk->enable++; > else if (!enable && clk->ops->disable) > clk->ops->disable(clk); > @@ -181,7 +182,7 @@ 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) > + void *priv) > { > struct v4l2_clk *clk; > int ret; > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > 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) { > + if (!clk->dev_id) { > ret = -ENOMEM; > goto ealloc; > } > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > mutex_init(&clk->lock); > > mutex_lock(&clk_lock); > - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { like mentioned in the beginning of the mail, you need to do same change in v4l2_clk_get(). Best Regards, Josh Wu > mutex_unlock(&clk_lock); > ret = -EEXIST; > goto eexist; > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > > eexist: > ealloc: > - kfree(clk->id); > kfree(clk->dev_id); > kfree(clk); > return ERR_PTR(ret); > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); > void v4l2_clk_unregister(struct v4l2_clk *clk) > { > if (WARN(atomic_read(&clk->use_count), > - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", > - __func__, clk->dev_id, clk->id)) > + "%s(): Refusing to unregister ref-counted %s clock!\n", > + __func__, clk->dev_id)) > return; > > mutex_lock(&clk_lock); > list_del(&clk->list); > mutex_unlock(&clk_lock); > > - kfree(clk->id); > kfree(clk->dev_id); > kfree(clk); > } > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk) > } > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > - const char *id, unsigned long rate, struct module *owner) > + unsigned long rate, struct module *owner) > { > struct v4l2_clk *clk; > struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > priv->ops.get_rate = fixed_get_rate; > priv->ops.owner = owner; > > - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); > + clk = v4l2_clk_register(&priv->ops, dev_id, priv); > if (IS_ERR(clk)) > kfree(priv); > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h > index 0b36cc1..8f06967 100644 > --- a/include/media/v4l2-clk.h > +++ b/include/media/v4l2-clk.h > @@ -43,7 +43,7 @@ struct v4l2_clk_ops { > > struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > const char *dev_name, > - const char *name, void *priv); > + void *priv); > void v4l2_clk_unregister(struct v4l2_clk *clk); > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); > void v4l2_clk_put(struct v4l2_clk *clk); > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate); > struct module; > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > - const char *id, unsigned long rate, struct module *owner); > + unsigned long rate, struct module *owner); > void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); > > static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id, > - const char *id, > unsigned long rate) > { > - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); > + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); > } > > #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \ -- 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 Josh, On Tue, 6 Jan 2015, Josh Wu wrote: > Hi, Guennadi > > After look deep into this patch, I found you miss one line that should be > changed as well. > It's In function v4l2_clk_get(), there still has one line code called > v4l2_clk_find(dev_id, id). > You need to change it to v4l2_clk_find(dev_id, NULL) as well. > Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk") > cannot acquired the "mclk" clock. > > After above changes, this patch works for me. I think you're right, in fact, since we now don't store CCF-based v4l2_clk wrappers on the list, this can be simplified even further, I'll update the patch. Did you only test this patch or both? Thanks Guennadi > > On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: > > All uses of the v4l2_clk API so far only register one clock with a fixed > > name. This allows us to get rid of it, which also will make CCF and DT > > integration easier. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > --- > > drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- > > drivers/media/usb/em28xx/em28xx-camera.c | 2 +- > > drivers/media/v4l2-core/v4l2-clk.c | 24 > > +++++++++++------------- > > include/media/v4l2-clk.h | 7 +++---- > > 4 files changed, 18 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c > > b/drivers/media/platform/soc_camera/soc_camera.c > > index f4be2a1..ce192b6 100644 > > --- a/drivers/media/platform/soc_camera/soc_camera.c > > +++ b/drivers/media/platform/soc_camera/soc_camera.c > > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct > > soc_camera_device *icd, > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > shd->i2c_adapter_id, shd->board_info->addr); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host > > *ici, > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, > > snprintf(clk_name, sizeof(clk_name), "of-%s", > > of_node_full_name(remote)); > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", > > icd); > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > if (IS_ERR(icd->clk)) { > > ret = PTR_ERR(icd->clk); > > goto eclkreg; > > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c > > b/drivers/media/usb/em28xx/em28xx-camera.c > > index 7be661f..a4b22c2 100644 > > --- a/drivers/media/usb/em28xx/em28xx-camera.c > > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name), > > i2c_adapter_id(adap), client->addr); > > - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); > > + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); > > if (IS_ERR(v4l2->clk)) > > return PTR_ERR(v4l2->clk); > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > b/drivers/media/v4l2-core/v4l2-clk.c > > index e18cc04..c210906 100644 > > --- a/drivers/media/v4l2-core/v4l2-clk.c > > +++ b/drivers/media/v4l2-core/v4l2-clk.c > > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, > > const char *id) > > if (strcmp(dev_id, clk->dev_id)) > > continue; > > - if (!id || !clk->id || !strcmp(clk->id, id)) > > + if ((!id && !clk->id) || > > + (id && clk->id && !strcmp(clk->id, id))) > > return clk; > > } > > @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > mutex_lock(&clk->lock); > > enable = --clk->enable; > > - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, > > - clk->dev_id, clk->id)) > > + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, > > + clk->dev_id)) > > clk->enable++; > > else if (!enable && clk->ops->disable) > > clk->ops->disable(clk); > > @@ -181,7 +182,7 @@ 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) > > + void *priv) > > { > > struct v4l2_clk *clk; > > int ret; > > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > 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) { > > + if (!clk->dev_id) { > > ret = -ENOMEM; > > goto ealloc; > > } > > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > mutex_init(&clk->lock); > > mutex_lock(&clk_lock); > > - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > > + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { > > like mentioned in the beginning of the mail, you need to do same change in > v4l2_clk_get(). > > Best Regards, > Josh Wu > > > > mutex_unlock(&clk_lock); > > ret = -EEXIST; > > goto eexist; > > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct > > v4l2_clk_ops *ops, > > eexist: > > ealloc: > > - kfree(clk->id); > > kfree(clk->dev_id); > > kfree(clk); > > return ERR_PTR(ret); > > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); > > void v4l2_clk_unregister(struct v4l2_clk *clk) > > { > > if (WARN(atomic_read(&clk->use_count), > > - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", > > - __func__, clk->dev_id, clk->id)) > > + "%s(): Refusing to unregister ref-counted %s clock!\n", > > + __func__, clk->dev_id)) > > return; > > mutex_lock(&clk_lock); > > list_del(&clk->list); > > mutex_unlock(&clk_lock); > > - kfree(clk->id); > > kfree(clk->dev_id); > > kfree(clk); > > } > > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk > > *clk) > > } > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > - const char *id, unsigned long rate, struct module *owner) > > + unsigned long rate, struct module *owner) > > { > > struct v4l2_clk *clk; > > struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char > > *dev_id, > > priv->ops.get_rate = fixed_get_rate; > > priv->ops.owner = owner; > > - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); > > + clk = v4l2_clk_register(&priv->ops, dev_id, priv); > > if (IS_ERR(clk)) > > kfree(priv); > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h > > index 0b36cc1..8f06967 100644 > > --- a/include/media/v4l2-clk.h > > +++ b/include/media/v4l2-clk.h > > @@ -43,7 +43,7 @@ struct v4l2_clk_ops { > > struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > > const char *dev_name, > > - const char *name, void *priv); > > + void *priv); > > void v4l2_clk_unregister(struct v4l2_clk *clk); > > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); > > void v4l2_clk_put(struct v4l2_clk *clk); > > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned > > long rate); > > struct module; > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > - const char *id, unsigned long rate, struct module *owner); > > + unsigned long rate, struct module *owner); > > void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); > > static inline struct v4l2_clk *v4l2_clk_register_fixed(const char > > *dev_id, > > - const char *id, > > unsigned long rate) > > { > > - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); > > + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); > > } > > #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, > > \ > -- 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 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: > Hi Josh, > > On Tue, 6 Jan 2015, Josh Wu wrote: > >> Hi, Guennadi >> >> After look deep into this patch, I found you miss one line that should be >> changed as well. >> It's In function v4l2_clk_get(), there still has one line code called >> v4l2_clk_find(dev_id, id). >> You need to change it to v4l2_clk_find(dev_id, NULL) as well. >> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk") >> cannot acquired the "mclk" clock. >> >> After above changes, this patch works for me. > I think you're right, in fact, since we now don't store CCF-based v4l2_clk > wrappers on the list, this can be simplified even further, I'll update the > patch. Did you only test this patch or both? I tested both patches with Atmel-isi driver. For the 2/2 patch I applied the modification Laurent suggested. Those patches works for me. The only concern is in ov2640 I still need to acquired two v4l2 clocks: "xvclk" that will get the xvclk CCF clock directly. "mclk" that make ISI driver call his clock_start()/stop() to enable/disable ISI's peripheral clock. If I only get xvclk clock, then the camera capture will be failed with a ISI timeout error. But I think this is acceptable as we will keep go forward. Finally we'll switch to CCF and removed the v4l2_clock then we will move the clock_start()/stop() caller code to soc_camera.c. Best Regards, Josh Wu > > Thanks > Guennadi > >> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: >>> All uses of the v4l2_clk API so far only register one clock with a fixed >>> name. This allows us to get rid of it, which also will make CCF and DT >>> integration easier. >>> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> --- >>> drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- >>> drivers/media/usb/em28xx/em28xx-camera.c | 2 +- >>> drivers/media/v4l2-core/v4l2-clk.c | 24 >>> +++++++++++------------- >>> include/media/v4l2-clk.h | 7 +++---- >>> 4 files changed, 18 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c >>> b/drivers/media/platform/soc_camera/soc_camera.c >>> index f4be2a1..ce192b6 100644 >>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct >>> soc_camera_device *icd, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> shd->i2c_adapter_id, shd->board_info->addr); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host >>> *ici, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, >>> snprintf(clk_name, sizeof(clk_name), "of-%s", >>> of_node_full_name(remote)); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c >>> b/drivers/media/usb/em28xx/em28xx-camera.c >>> index 7be661f..a4b22c2 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-camera.c >>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c >>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) >>> v4l2_clk_name_i2c(clk_name, sizeof(clk_name), >>> i2c_adapter_id(adap), client->addr); >>> - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); >>> + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); >>> if (IS_ERR(v4l2->clk)) >>> return PTR_ERR(v4l2->clk); >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c >>> b/drivers/media/v4l2-core/v4l2-clk.c >>> index e18cc04..c210906 100644 >>> --- a/drivers/media/v4l2-core/v4l2-clk.c >>> +++ b/drivers/media/v4l2-core/v4l2-clk.c >>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, >>> const char *id) >>> if (strcmp(dev_id, clk->dev_id)) >>> continue; >>> - if (!id || !clk->id || !strcmp(clk->id, id)) >>> + if ((!id && !clk->id) || >>> + (id && clk->id && !strcmp(clk->id, id))) >>> return clk; >>> } >>> @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) >>> mutex_lock(&clk->lock); >>> enable = --clk->enable; >>> - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, >>> - clk->dev_id, clk->id)) >>> + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, >>> + clk->dev_id)) >>> clk->enable++; >>> else if (!enable && clk->ops->disable) >>> clk->ops->disable(clk); >>> @@ -181,7 +182,7 @@ 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) >>> + void *priv) >>> { >>> struct v4l2_clk *clk; >>> int ret; >>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> 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) { >>> + if (!clk->dev_id) { >>> ret = -ENOMEM; >>> goto ealloc; >>> } >>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> mutex_init(&clk->lock); >>> mutex_lock(&clk_lock); >>> - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { >>> + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { >> like mentioned in the beginning of the mail, you need to do same change in >> v4l2_clk_get(). >> >> Best Regards, >> Josh Wu >> >> >>> mutex_unlock(&clk_lock); >>> ret = -EEXIST; >>> goto eexist; >>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> eexist: >>> ealloc: >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> return ERR_PTR(ret); >>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); >>> void v4l2_clk_unregister(struct v4l2_clk *clk) >>> { >>> if (WARN(atomic_read(&clk->use_count), >>> - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", >>> - __func__, clk->dev_id, clk->id)) >>> + "%s(): Refusing to unregister ref-counted %s clock!\n", >>> + __func__, clk->dev_id)) >>> return; >>> mutex_lock(&clk_lock); >>> list_del(&clk->list); >>> mutex_unlock(&clk_lock); >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> } >>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk >>> *clk) >>> } >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner) >>> + unsigned long rate, struct module *owner) >>> { >>> struct v4l2_clk *clk; >>> struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char >>> *dev_id, >>> priv->ops.get_rate = fixed_get_rate; >>> priv->ops.owner = owner; >>> - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); >>> + clk = v4l2_clk_register(&priv->ops, dev_id, priv); >>> if (IS_ERR(clk)) >>> kfree(priv); >>> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h >>> index 0b36cc1..8f06967 100644 >>> --- a/include/media/v4l2-clk.h >>> +++ b/include/media/v4l2-clk.h >>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops { >>> struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, >>> const char *dev_name, >>> - const char *name, void *priv); >>> + void *priv); >>> void v4l2_clk_unregister(struct v4l2_clk *clk); >>> struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); >>> void v4l2_clk_put(struct v4l2_clk *clk); >>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned >>> long rate); >>> struct module; >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner); >>> + unsigned long rate, struct module *owner); >>> void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); >>> static inline struct v4l2_clk *v4l2_clk_register_fixed(const char >>> *dev_id, >>> - const char *id, >>> unsigned long rate) >>> { >>> - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); >>> + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); >>> } >>> #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, >>> \ -- 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 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: > Hi Josh, > > On Tue, 6 Jan 2015, Josh Wu wrote: > >> Hi, Guennadi >> >> After look deep into this patch, I found you miss one line that should be >> changed as well. >> It's In function v4l2_clk_get(), there still has one line code called >> v4l2_clk_find(dev_id, id). >> You need to change it to v4l2_clk_find(dev_id, NULL) as well. >> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk") >> cannot acquired the "mclk" clock. >> >> After above changes, this patch works for me. > I think you're right, in fact, since we now don't store CCF-based v4l2_clk > wrappers on the list, this can be simplified even further, I'll update the > patch. Did you only test this patch or both? I tested both patches with Atmel-isi driver. For the 2/2 patch I applied the modification Laurent suggested. Those patches works for me. The only concern is in ov2640 I still need to acquired two v4l2 clocks: "xvclk" that will get the xvclk CCF clock directly. "mclk" that make ISI driver call his clock_start()/stop() to enable/disable ISI's peripheral clock. If I only get xvclk clock, then the camera capture will be failed with a ISI timeout error. But I think this is acceptable as we will keep go forward. Finally we'll switch to CCF and removed the v4l2_clock then we will move the clock_start()/stop() caller code to soc_camera.c. Best Regards, Josh Wu > > Thanks > Guennadi > >> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: >>> All uses of the v4l2_clk API so far only register one clock with a fixed >>> name. This allows us to get rid of it, which also will make CCF and DT >>> integration easier. >>> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> --- >>> drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- >>> drivers/media/usb/em28xx/em28xx-camera.c | 2 +- >>> drivers/media/v4l2-core/v4l2-clk.c | 24 >>> +++++++++++------------- >>> include/media/v4l2-clk.h | 7 +++---- >>> 4 files changed, 18 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c >>> b/drivers/media/platform/soc_camera/soc_camera.c >>> index f4be2a1..ce192b6 100644 >>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct >>> soc_camera_device *icd, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> shd->i2c_adapter_id, shd->board_info->addr); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host >>> *ici, >>> snprintf(clk_name, sizeof(clk_name), "%d-%04x", >>> sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, >>> snprintf(clk_name, sizeof(clk_name), "of-%s", >>> of_node_full_name(remote)); >>> - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", >>> icd); >>> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); >>> if (IS_ERR(icd->clk)) { >>> ret = PTR_ERR(icd->clk); >>> goto eclkreg; >>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c >>> b/drivers/media/usb/em28xx/em28xx-camera.c >>> index 7be661f..a4b22c2 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-camera.c >>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c >>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) >>> v4l2_clk_name_i2c(clk_name, sizeof(clk_name), >>> i2c_adapter_id(adap), client->addr); >>> - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); >>> + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); >>> if (IS_ERR(v4l2->clk)) >>> return PTR_ERR(v4l2->clk); >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c >>> b/drivers/media/v4l2-core/v4l2-clk.c >>> index e18cc04..c210906 100644 >>> --- a/drivers/media/v4l2-core/v4l2-clk.c >>> +++ b/drivers/media/v4l2-core/v4l2-clk.c >>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, >>> const char *id) >>> if (strcmp(dev_id, clk->dev_id)) >>> continue; >>> - if (!id || !clk->id || !strcmp(clk->id, id)) >>> + if ((!id && !clk->id) || >>> + (id && clk->id && !strcmp(clk->id, id))) >>> return clk; >>> } >>> @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) >>> mutex_lock(&clk->lock); >>> enable = --clk->enable; >>> - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, >>> - clk->dev_id, clk->id)) >>> + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, >>> + clk->dev_id)) >>> clk->enable++; >>> else if (!enable && clk->ops->disable) >>> clk->ops->disable(clk); >>> @@ -181,7 +182,7 @@ 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) >>> + void *priv) >>> { >>> struct v4l2_clk *clk; >>> int ret; >>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> 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) { >>> + if (!clk->dev_id) { >>> ret = -ENOMEM; >>> goto ealloc; >>> } >>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> mutex_init(&clk->lock); >>> mutex_lock(&clk_lock); >>> - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { >>> + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { >> like mentioned in the beginning of the mail, you need to do same change in >> v4l2_clk_get(). >> >> Best Regards, >> Josh Wu >> >> >>> mutex_unlock(&clk_lock); >>> ret = -EEXIST; >>> goto eexist; >>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct >>> v4l2_clk_ops *ops, >>> eexist: >>> ealloc: >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> return ERR_PTR(ret); >>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); >>> void v4l2_clk_unregister(struct v4l2_clk *clk) >>> { >>> if (WARN(atomic_read(&clk->use_count), >>> - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", >>> - __func__, clk->dev_id, clk->id)) >>> + "%s(): Refusing to unregister ref-counted %s clock!\n", >>> + __func__, clk->dev_id)) >>> return; >>> mutex_lock(&clk_lock); >>> list_del(&clk->list); >>> mutex_unlock(&clk_lock); >>> - kfree(clk->id); >>> kfree(clk->dev_id); >>> kfree(clk); >>> } >>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk >>> *clk) >>> } >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner) >>> + unsigned long rate, struct module *owner) >>> { >>> struct v4l2_clk *clk; >>> struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char >>> *dev_id, >>> priv->ops.get_rate = fixed_get_rate; >>> priv->ops.owner = owner; >>> - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); >>> + clk = v4l2_clk_register(&priv->ops, dev_id, priv); >>> if (IS_ERR(clk)) >>> kfree(priv); >>> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h >>> index 0b36cc1..8f06967 100644 >>> --- a/include/media/v4l2-clk.h >>> +++ b/include/media/v4l2-clk.h >>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops { >>> struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, >>> const char *dev_name, >>> - const char *name, void *priv); >>> + void *priv); >>> void v4l2_clk_unregister(struct v4l2_clk *clk); >>> struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); >>> void v4l2_clk_put(struct v4l2_clk *clk); >>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned >>> long rate); >>> struct module; >>> struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, >>> - const char *id, unsigned long rate, struct module *owner); >>> + unsigned long rate, struct module *owner); >>> void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); >>> static inline struct v4l2_clk *v4l2_clk_register_fixed(const char >>> *dev_id, >>> - const char *id, >>> unsigned long rate) >>> { >>> - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); >>> + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); >>> } >>> #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, >>> \ -- 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 Josh, On Wed, 7 Jan 2015, Josh Wu wrote: > Hi, Guennadi > > On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: > > Hi Josh, > > > > On Tue, 6 Jan 2015, Josh Wu wrote: > > > > > Hi, Guennadi > > > > > > After look deep into this patch, I found you miss one line that should be > > > changed as well. > > > It's In function v4l2_clk_get(), there still has one line code called > > > v4l2_clk_find(dev_id, id). > > > You need to change it to v4l2_clk_find(dev_id, NULL) as well. > > > Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, > > > "mclk") > > > cannot acquired the "mclk" clock. > > > > > > After above changes, this patch works for me. > > I think you're right, in fact, since we now don't store CCF-based v4l2_clk > > wrappers on the list, this can be simplified even further, I'll update the > > patch. Did you only test this patch or both? > I tested both patches with Atmel-isi driver. For the 2/2 patch I applied the > modification Laurent suggested. > Those patches works for me. > > The only concern is in ov2640 I still need to acquired two v4l2 clocks: > "xvclk" that will get the xvclk CCF clock directly. > "mclk" that make ISI driver call his clock_start()/stop() to > enable/disable ISI's peripheral clock. > If I only get xvclk clock, then the camera capture will be failed with a ISI > timeout error. No, this doesn't look right to me. The camera sensor has only one clock input, so, it should only request one clock. Where does the clock signal to the camera come from on your system? If it comes from the ISI itself, you don't need to specify the clock in the DT, since the ISI doesn't produce a clock from DT. If you do want to have your clock consumer (ov2640) and the supplier (ISI) properly described in DT, you'll have to teach the ISI to register a CCF clock source, which then will be connected to from the ov2640. If you choose not to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel. If the closk to ov2640 is supplied by a separate clock source, then you v4l2_clk_get() will connect ov2640 to it directly and soc-camera will enable and disable it on power-on / -off as required. From your above description it looks like the clock to ov2640 is supplied by a separate source, but atmel-isi's .clock_start() / .clock_stop() functions still need to be called? By looking at those functions it looks like they turn on and off clocks, supplying the ISI itself... Instead of only turning on and off clocks, provided by the ISI to a camera sensor. If my understanding is right, then this is a bug in atmel-isi and it has to be fixed. Thanks Guennadi > But I think this is acceptable as we will keep go forward. Finally we'll > switch to CCF and removed the v4l2_clock then we will move the > clock_start()/stop() caller code to soc_camera.c. > > Best Regards, > Josh Wu > > > > > Thanks > > Guennadi > > > > > On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote: > > > > All uses of the v4l2_clk API so far only register one clock with a fixed > > > > name. This allows us to get rid of it, which also will make CCF and DT > > > > integration easier. > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > --- > > > > drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- > > > > drivers/media/usb/em28xx/em28xx-camera.c | 2 +- > > > > drivers/media/v4l2-core/v4l2-clk.c | 24 > > > > +++++++++++------------- > > > > include/media/v4l2-clk.h | 7 +++---- > > > > 4 files changed, 18 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c > > > > b/drivers/media/platform/soc_camera/soc_camera.c > > > > index f4be2a1..ce192b6 100644 > > > > --- a/drivers/media/platform/soc_camera/soc_camera.c > > > > +++ b/drivers/media/platform/soc_camera/soc_camera.c > > > > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct > > > > soc_camera_device *icd, > > > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > > > shd->i2c_adapter_id, shd->board_info->addr); > > > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, > > > > "mclk", > > > > icd); > > > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > > > if (IS_ERR(icd->clk)) { > > > > ret = PTR_ERR(icd->clk); > > > > goto eclkreg; > > > > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host > > > > *ici, > > > > snprintf(clk_name, sizeof(clk_name), "%d-%04x", > > > > sasd->asd.match.i2c.adapter_id, > > > > sasd->asd.match.i2c.address); > > > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, > > > > "mclk", > > > > icd); > > > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > > > if (IS_ERR(icd->clk)) { > > > > ret = PTR_ERR(icd->clk); > > > > goto eclkreg; > > > > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host > > > > *ici, > > > > snprintf(clk_name, sizeof(clk_name), "of-%s", > > > > of_node_full_name(remote)); > > > > - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, > > > > "mclk", > > > > icd); > > > > + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); > > > > if (IS_ERR(icd->clk)) { > > > > ret = PTR_ERR(icd->clk); > > > > goto eclkreg; > > > > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c > > > > b/drivers/media/usb/em28xx/em28xx-camera.c > > > > index 7be661f..a4b22c2 100644 > > > > --- a/drivers/media/usb/em28xx/em28xx-camera.c > > > > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > > > > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) > > > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name), > > > > i2c_adapter_id(adap), client->addr); > > > > - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); > > > > + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); > > > > if (IS_ERR(v4l2->clk)) > > > > return PTR_ERR(v4l2->clk); > > > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c > > > > b/drivers/media/v4l2-core/v4l2-clk.c > > > > index e18cc04..c210906 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-clk.c > > > > +++ b/drivers/media/v4l2-core/v4l2-clk.c > > > > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char > > > > *dev_id, > > > > const char *id) > > > > if (strcmp(dev_id, clk->dev_id)) > > > > continue; > > > > - if (!id || !clk->id || !strcmp(clk->id, id)) > > > > + if ((!id && !clk->id) || > > > > + (id && clk->id && !strcmp(clk->id, id))) > > > > return clk; > > > > } > > > > @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) > > > > mutex_lock(&clk->lock); > > > > enable = --clk->enable; > > > > - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, > > > > - clk->dev_id, clk->id)) > > > > + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, > > > > + clk->dev_id)) > > > > clk->enable++; > > > > else if (!enable && clk->ops->disable) > > > > clk->ops->disable(clk); > > > > @@ -181,7 +182,7 @@ 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) > > > > + void *priv) > > > > { > > > > struct v4l2_clk *clk; > > > > int ret; > > > > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct > > > > v4l2_clk_ops *ops, > > > > 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) { > > > > + if (!clk->dev_id) { > > > > ret = -ENOMEM; > > > > goto ealloc; > > > > } > > > > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct > > > > v4l2_clk_ops *ops, > > > > mutex_init(&clk->lock); > > > > mutex_lock(&clk_lock); > > > > - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { > > > > + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { > > > like mentioned in the beginning of the mail, you need to do same change in > > > v4l2_clk_get(). > > > > > > Best Regards, > > > Josh Wu > > > > > > > > > > mutex_unlock(&clk_lock); > > > > ret = -EEXIST; > > > > goto eexist; > > > > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct > > > > v4l2_clk_ops *ops, > > > > eexist: > > > > ealloc: > > > > - kfree(clk->id); > > > > kfree(clk->dev_id); > > > > kfree(clk); > > > > return ERR_PTR(ret); > > > > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); > > > > void v4l2_clk_unregister(struct v4l2_clk *clk) > > > > { > > > > if (WARN(atomic_read(&clk->use_count), > > > > - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", > > > > - __func__, clk->dev_id, clk->id)) > > > > + "%s(): Refusing to unregister ref-counted %s clock!\n", > > > > + __func__, clk->dev_id)) > > > > return; > > > > mutex_lock(&clk_lock); > > > > list_del(&clk->list); > > > > mutex_unlock(&clk_lock); > > > > - kfree(clk->id); > > > > kfree(clk->dev_id); > > > > kfree(clk); > > > > } > > > > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk > > > > *clk) > > > > } > > > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > > > - const char *id, unsigned long rate, struct module *owner) > > > > + unsigned long rate, struct module *owner) > > > > { > > > > struct v4l2_clk *clk; > > > > struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), > > > > GFP_KERNEL); > > > > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const > > > > char > > > > *dev_id, > > > > priv->ops.get_rate = fixed_get_rate; > > > > priv->ops.owner = owner; > > > > - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); > > > > + clk = v4l2_clk_register(&priv->ops, dev_id, priv); > > > > if (IS_ERR(clk)) > > > > kfree(priv); > > > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h > > > > index 0b36cc1..8f06967 100644 > > > > --- a/include/media/v4l2-clk.h > > > > +++ b/include/media/v4l2-clk.h > > > > @@ -43,7 +43,7 @@ struct v4l2_clk_ops { > > > > struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, > > > > const char *dev_name, > > > > - const char *name, void *priv); > > > > + void *priv); > > > > void v4l2_clk_unregister(struct v4l2_clk *clk); > > > > struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); > > > > void v4l2_clk_put(struct v4l2_clk *clk); > > > > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned > > > > long rate); > > > > struct module; > > > > struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, > > > > - const char *id, unsigned long rate, struct module *owner); > > > > + unsigned long rate, struct module *owner); > > > > void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); > > > > static inline struct v4l2_clk *v4l2_clk_register_fixed(const char > > > > *dev_id, > > > > - const char *id, > > > > unsigned long > > > > rate) > > > > { > > > > - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); > > > > + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); > > > > } > > > > #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, > > > > size, > > > > \ > -- 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 and Josh, On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote: > On Wed, 7 Jan 2015, Josh Wu wrote: > > On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: > >> On Tue, 6 Jan 2015, Josh Wu wrote: > >>> Hi, Guennadi > >>> > >>> After look deep into this patch, I found you miss one line that should > >>> be changed as well. > >>> It's In function v4l2_clk_get(), there still has one line code called > >>> v4l2_clk_find(dev_id, id). > >>> You need to change it to v4l2_clk_find(dev_id, NULL) as well. > >>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, > >>> "mclk") cannot acquired the "mclk" clock. > >>> > >>> After above changes, this patch works for me. > >> > >> I think you're right, in fact, since we now don't store CCF-based > >> v4l2_clk wrappers on the list, this can be simplified even further, I'll > >> update the patch. Did you only test this patch or both? > > > > I tested both patches with Atmel-isi driver. For the 2/2 patch I applied > > the modification Laurent suggested. > > Those patches works for me. > > > > The only concern is in ov2640 I still need to acquired two v4l2 clocks: > > "xvclk" that will get the xvclk CCF clock directly. > > "mclk" that make ISI driver call his clock_start()/stop() to > > enable/disable ISI's peripheral clock. > > If I only get xvclk clock, then the camera capture will be failed with a > > ISI timeout error. > > No, this doesn't look right to me. The camera sensor has only one clock > input, so, it should only request one clock. Where does the clock signal > to the camera come from on your system? That's correct, the sensor driver only has one clock input, so it should just request the xvclk clock. > If it comes from the ISI itself, you don't need to specify the clock in > the DT, since the ISI doesn't produce a clock from DT. If you do want to > have your clock consumer (ov2640) and the supplier (ISI) properly > described in DT, you'll have to teach the ISI to register a CCF clock > source, which then will be connected to from the ov2640. If you choose not > to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk") > and it will be handled by v4l2_clk / soc-camera / isi-atmel. > > If the closk to ov2640 is supplied by a separate clock source, then you > v4l2_clk_get() will connect ov2640 to it directly and soc-camera will > enable and disable it on power-on / -off as required. The ISI has no way to supply a sensor clock, the clock is supplied by a separate clock source. > From your above description it looks like the clock to ov2640 is supplied > by a separate source, but atmel-isi's .clock_start() / .clock_stop() > functions still need to be called? By looking at those functions it looks > like they turn on and off clocks, supplying the ISI itself... Instead of > only turning on and off clocks, provided by the ISI to a camera sensor. If > my understanding is right, then this is a bug in atmel-isi and it has to > be fixed. That's correct as well, the ISI driver needs to be fixed.
Hi, Guennadi and Laurent On 1/9/2015 6:47 AM, Laurent Pinchart wrote: > Hi Guennadi and Josh, > > On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote: >> On Wed, 7 Jan 2015, Josh Wu wrote: >>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: >>>> On Tue, 6 Jan 2015, Josh Wu wrote: >>>>> Hi, Guennadi >>>>> >>>>> After look deep into this patch, I found you miss one line that should >>>>> be changed as well. >>>>> It's In function v4l2_clk_get(), there still has one line code called >>>>> v4l2_clk_find(dev_id, id). >>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well. >>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, >>>>> "mclk") cannot acquired the "mclk" clock. >>>>> >>>>> After above changes, this patch works for me. >>>> I think you're right, in fact, since we now don't store CCF-based >>>> v4l2_clk wrappers on the list, this can be simplified even further, I'll >>>> update the patch. Did you only test this patch or both? >>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied >>> the modification Laurent suggested. >>> Those patches works for me. >>> >>> The only concern is in ov2640 I still need to acquired two v4l2 clocks: >>> "xvclk" that will get the xvclk CCF clock directly. >>> "mclk" that make ISI driver call his clock_start()/stop() to >>> enable/disable ISI's peripheral clock. >>> If I only get xvclk clock, then the camera capture will be failed with a >>> ISI timeout error. >> No, this doesn't look right to me. The camera sensor has only one clock >> input, so, it should only request one clock. Where does the clock signal >> to the camera come from on your system? > That's correct, the sensor driver only has one clock input, so it should just > request the xvclk clock. > >> If it comes from the ISI itself, you don't need to specify the clock in >> the DT, since the ISI doesn't produce a clock from DT. If you do want to >> have your clock consumer (ov2640) and the supplier (ISI) properly >> described in DT, you'll have to teach the ISI to register a CCF clock >> source, which then will be connected to from the ov2640. If you choose not >> to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk") >> and it will be handled by v4l2_clk / soc-camera / isi-atmel. >> >> If the closk to ov2640 is supplied by a separate clock source, then you >> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will >> enable and disable it on power-on / -off as required. > The ISI has no way to supply a sensor clock, the clock is supplied by a > separate clock source. > >> From your above description it looks like the clock to ov2640 is supplied >> by a separate source, but atmel-isi's .clock_start() / .clock_stop() >> functions still need to be called? By looking at those functions it looks >> like they turn on and off clocks, supplying the ISI itself... Instead of >> only turning on and off clocks, provided by the ISI to a camera sensor. If >> my understanding is right, then this is a bug in atmel-isi and it has to >> be fixed. > That's correct as well, the ISI driver needs to be fixed. > Thanks both of you for the details. Now I got it. Indeed, I need fix this in atmel-isi driver not in ov2640 driver. So I will send a new patch for this, which should move the ISI peripheral clock enable/disable() from clock_start/stop() to isi_camera_add_device/remove_device(). Best Regards, Josh Wu -- 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 Josh, On Monday 12 January 2015 17:14:33 Josh Wu wrote: > On 1/9/2015 6:47 AM, Laurent Pinchart wrote: > > On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote: > >> On Wed, 7 Jan 2015, Josh Wu wrote: > >>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: > >>>> On Tue, 6 Jan 2015, Josh Wu wrote: > >>>>> Hi, Guennadi > >>>>> > >>>>> After look deep into this patch, I found you miss one line that should > >>>>> be changed as well. > >>>>> It's In function v4l2_clk_get(), there still has one line code called > >>>>> v4l2_clk_find(dev_id, id). > >>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well. > >>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, > >>>>> "mclk") cannot acquired the "mclk" clock. > >>>>> > >>>>> After above changes, this patch works for me. > >>>> > >>>> I think you're right, in fact, since we now don't store CCF-based > >>>> v4l2_clk wrappers on the list, this can be simplified even further, > >>>> I'll update the patch. Did you only test this patch or both? > >>> > >>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied > >>> the modification Laurent suggested. > >>> Those patches works for me. > >>> > >>> The only concern is in ov2640 I still need to acquired two v4l2 clocks: > >>> "xvclk" that will get the xvclk CCF clock directly. > >>> "mclk" that make ISI driver call his clock_start()/stop() to > >>> enable/disable ISI's peripheral clock. > >>> > >>> If I only get xvclk clock, then the camera capture will be failed with a > >>> ISI timeout error. > >> > >> No, this doesn't look right to me. The camera sensor has only one clock > >> input, so, it should only request one clock. Where does the clock signal > >> to the camera come from on your system? > > > > That's correct, the sensor driver only has one clock input, so it should > > just request the xvclk clock. > > > >> If it comes from the ISI itself, you don't need to specify the clock in > >> the DT, since the ISI doesn't produce a clock from DT. If you do want to > >> have your clock consumer (ov2640) and the supplier (ISI) properly > >> described in DT, you'll have to teach the ISI to register a CCF clock > >> source, which then will be connected to from the ov2640. If you choose > >> not to show your clock in the DT, you can just use v4l2_clk_get(dev, > >> "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel. > >> > >> If the closk to ov2640 is supplied by a separate clock source, then you > >> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will > >> enable and disable it on power-on / -off as required. > > > > The ISI has no way to supply a sensor clock, the clock is supplied by a > > separate clock source. > > > >> From your above description it looks like the clock to ov2640 is > >> supplied by a separate source, but atmel-isi's .clock_start() / > >> .clock_stop() functions still need to be called? By looking at those > >> functions it looks like they turn on and off clocks, supplying the ISI > >> itself... Instead of only turning on and off clocks, provided by the ISI > >> to a camera sensor. If my understanding is right, then this is a bug in > >> atmel-isi and it has to be fixed. > > > > That's correct as well, the ISI driver needs to be fixed. > > Thanks both of you for the details. Now I got it. > Indeed, I need fix this in atmel-isi driver not in ov2640 driver. > So I will send a new patch for this, which should move the ISI > peripheral clock enable/disable() from clock_start/stop() to > isi_camera_add_device/remove_device(). Shouldn't you move it to the start_streaming() and stop_streaming() functions instead ? An even better solution would be to use runtime PM to enable/disable the ISI clock in the runtime PM resume and suspend handlers, and call pm_runtime_get_sync() and pm_runtime_put() when you need the ISI to be operational.
On 1/12/2015 6:38 PM, Laurent Pinchart wrote: > Hi Josh, > > On Monday 12 January 2015 17:14:33 Josh Wu wrote: >> On 1/9/2015 6:47 AM, Laurent Pinchart wrote: >>> On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote: >>>> On Wed, 7 Jan 2015, Josh Wu wrote: >>>>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote: >>>>>> On Tue, 6 Jan 2015, Josh Wu wrote: >>>>>>> Hi, Guennadi >>>>>>> >>>>>>> After look deep into this patch, I found you miss one line that should >>>>>>> be changed as well. >>>>>>> It's In function v4l2_clk_get(), there still has one line code called >>>>>>> v4l2_clk_find(dev_id, id). >>>>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well. >>>>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, >>>>>>> "mclk") cannot acquired the "mclk" clock. >>>>>>> >>>>>>> After above changes, this patch works for me. >>>>>> I think you're right, in fact, since we now don't store CCF-based >>>>>> v4l2_clk wrappers on the list, this can be simplified even further, >>>>>> I'll update the patch. Did you only test this patch or both? >>>>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied >>>>> the modification Laurent suggested. >>>>> Those patches works for me. >>>>> >>>>> The only concern is in ov2640 I still need to acquired two v4l2 clocks: >>>>> "xvclk" that will get the xvclk CCF clock directly. >>>>> "mclk" that make ISI driver call his clock_start()/stop() to >>>>> enable/disable ISI's peripheral clock. >>>>> >>>>> If I only get xvclk clock, then the camera capture will be failed with a >>>>> ISI timeout error. >>>> No, this doesn't look right to me. The camera sensor has only one clock >>>> input, so, it should only request one clock. Where does the clock signal >>>> to the camera come from on your system? >>> That's correct, the sensor driver only has one clock input, so it should >>> just request the xvclk clock. >>> >>>> If it comes from the ISI itself, you don't need to specify the clock in >>>> the DT, since the ISI doesn't produce a clock from DT. If you do want to >>>> have your clock consumer (ov2640) and the supplier (ISI) properly >>>> described in DT, you'll have to teach the ISI to register a CCF clock >>>> source, which then will be connected to from the ov2640. If you choose >>>> not to show your clock in the DT, you can just use v4l2_clk_get(dev, >>>> "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel. >>>> >>>> If the closk to ov2640 is supplied by a separate clock source, then you >>>> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will >>>> enable and disable it on power-on / -off as required. >>> The ISI has no way to supply a sensor clock, the clock is supplied by a >>> separate clock source. >>> >>>> From your above description it looks like the clock to ov2640 is >>>> supplied by a separate source, but atmel-isi's .clock_start() / >>>> .clock_stop() functions still need to be called? By looking at those >>>> functions it looks like they turn on and off clocks, supplying the ISI >>>> itself... Instead of only turning on and off clocks, provided by the ISI >>>> to a camera sensor. If my understanding is right, then this is a bug in >>>> atmel-isi and it has to be fixed. >>> That's correct as well, the ISI driver needs to be fixed. >> Thanks both of you for the details. Now I got it. >> Indeed, I need fix this in atmel-isi driver not in ov2640 driver. >> So I will send a new patch for this, which should move the ISI >> peripheral clock enable/disable() from clock_start/stop() to >> isi_camera_add_device/remove_device(). > Shouldn't you move it to the start_streaming() and stop_streaming() functions > instead ? An even better solution would be to use runtime PM to enable/disable > the ISI clock in the runtime PM resume and suspend handlers, and call > pm_runtime_get_sync() and pm_runtime_put() when you need the ISI to be > operational. Okay, I'll try to add the PM functions for atmel-isi in the meantime. Best Regards, Josh Wu > -- 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/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index f4be2a1..ce192b6 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd, snprintf(clk_name, sizeof(clk_name), "%d-%04x", shd->i2c_adapter_id, shd->board_info->addr); - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); if (IS_ERR(icd->clk)) { ret = PTR_ERR(icd->clk); goto eclkreg; @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici, snprintf(clk_name, sizeof(clk_name), "%d-%04x", sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address); - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); if (IS_ERR(icd->clk)) { ret = PTR_ERR(icd->clk); goto eclkreg; @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici, snprintf(clk_name, sizeof(clk_name), "of-%s", of_node_full_name(remote)); - icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd); if (IS_ERR(icd->clk)) { ret = PTR_ERR(icd->clk); goto eclkreg; diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c index 7be661f..a4b22c2 100644 --- a/drivers/media/usb/em28xx/em28xx-camera.c +++ b/drivers/media/usb/em28xx/em28xx-camera.c @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev) v4l2_clk_name_i2c(clk_name, sizeof(clk_name), i2c_adapter_id(adap), client->addr); - v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL); + v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL); if (IS_ERR(v4l2->clk)) return PTR_ERR(v4l2->clk); diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c index e18cc04..c210906 100644 --- a/drivers/media/v4l2-core/v4l2-clk.c +++ b/drivers/media/v4l2-core/v4l2-clk.c @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id) if (strcmp(dev_id, clk->dev_id)) continue; - if (!id || !clk->id || !strcmp(clk->id, id)) + if ((!id && !clk->id) || + (id && clk->id && !strcmp(clk->id, id))) return clk; } @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk) mutex_lock(&clk->lock); enable = --clk->enable; - if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__, - clk->dev_id, clk->id)) + if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__, + clk->dev_id)) clk->enable++; else if (!enable && clk->ops->disable) clk->ops->disable(clk); @@ -181,7 +182,7 @@ 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) + void *priv) { struct v4l2_clk *clk; int ret; @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, 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) { + if (!clk->dev_id) { ret = -ENOMEM; goto ealloc; } @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, mutex_init(&clk->lock); mutex_lock(&clk_lock); - if (!IS_ERR(v4l2_clk_find(dev_id, id))) { + if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) { mutex_unlock(&clk_lock); ret = -EEXIST; goto eexist; @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, eexist: ealloc: - kfree(clk->id); kfree(clk->dev_id); kfree(clk); return ERR_PTR(ret); @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register); void v4l2_clk_unregister(struct v4l2_clk *clk) { if (WARN(atomic_read(&clk->use_count), - "%s(): Refusing to unregister ref-counted %s:%s clock!\n", - __func__, clk->dev_id, clk->id)) + "%s(): Refusing to unregister ref-counted %s clock!\n", + __func__, clk->dev_id)) return; mutex_lock(&clk_lock); list_del(&clk->list); mutex_unlock(&clk_lock); - kfree(clk->id); kfree(clk->dev_id); kfree(clk); } @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk) } struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, - const char *id, unsigned long rate, struct module *owner) + unsigned long rate, struct module *owner) { struct v4l2_clk *clk; struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL); @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, priv->ops.get_rate = fixed_get_rate; priv->ops.owner = owner; - clk = v4l2_clk_register(&priv->ops, dev_id, id, priv); + clk = v4l2_clk_register(&priv->ops, dev_id, priv); if (IS_ERR(clk)) kfree(priv); diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h index 0b36cc1..8f06967 100644 --- a/include/media/v4l2-clk.h +++ b/include/media/v4l2-clk.h @@ -43,7 +43,7 @@ struct v4l2_clk_ops { struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops, const char *dev_name, - const char *name, void *priv); + void *priv); void v4l2_clk_unregister(struct v4l2_clk *clk); struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id); void v4l2_clk_put(struct v4l2_clk *clk); @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate); struct module; struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id, - const char *id, unsigned long rate, struct module *owner); + unsigned long rate, struct module *owner); void v4l2_clk_unregister_fixed(struct v4l2_clk *clk); static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id, - const char *id, unsigned long rate) { - return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE); + return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE); } #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
All uses of the v4l2_clk API so far only register one clock with a fixed name. This allows us to get rid of it, which also will make CCF and DT integration easier. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/media/platform/soc_camera/soc_camera.c | 6 +++--- drivers/media/usb/em28xx/em28xx-camera.c | 2 +- drivers/media/v4l2-core/v4l2-clk.c | 24 +++++++++++------------- include/media/v4l2-clk.h | 7 +++---- 4 files changed, 18 insertions(+), 21 deletions(-)