Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
On Wed, 27 Mar 2013, Laurent Pinchart wrote:
Hi Guennadi,
Thanks for the patch.
On Friday 15 March 2013 22:27:49 Guennadi Liakhovetski wrote:
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
[snip]
diff --git a/drivers/media/platform/soc_camera/soc_camera.c
b/drivers/media/platform/soc_camera/soc_camera.c index 4e626a6..01cd5a0
100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -30,6 +30,7 @@
#include linux/vmalloc.h
#include media/soc_camera.h
+#include media/v4l2-clk.h
#include media/v4l2-common.h
#include media/v4l2-ioctl.h
#include media/v4l2-dev.h
@@ -50,13 +51,19 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock);/* Protects the list of hosts */
-int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc
*ssdd)
+int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc
*ssdd,
+ struct v4l2_clk *clk)
{
- int ret = regulator_bulk_enable(ssdd-num_regulators,
+ int ret = clk ? v4l2_clk_enable(clk) : 0;
+ if (ret 0) {
+ dev_err(dev, Cannot enable clock\n);
+ return ret;
+ }
Will that work for all devices ? Aren't there devices that would need the
clock to be turned on after the power supply is stable ?
Swapping the order would be a functionality change. Let's not do that
unless proven to be needed.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
Hi Guennadi,
Thanks for the patch.
On Friday 15 March 2013 22:27:49 Guennadi Liakhovetski wrote:
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
[snip]
diff --git a/drivers/media/platform/soc_camera/soc_camera.c
b/drivers/media/platform/soc_camera/soc_camera.c index 4e626a6..01cd5a0
100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -30,6 +30,7 @@
#include linux/vmalloc.h
#include media/soc_camera.h
+#include media/v4l2-clk.h
#include media/v4l2-common.h
#include media/v4l2-ioctl.h
#include media/v4l2-dev.h
@@ -50,13 +51,19 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
-int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc
*ssdd)
+int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc
*ssdd,
+ struct v4l2_clk *clk)
{
- int ret = regulator_bulk_enable(ssdd-num_regulators,
+ int ret = clk ? v4l2_clk_enable(clk) : 0;
+ if (ret 0) {
+ dev_err(dev, Cannot enable clock\n);
+ return ret;
+ }
Will that work for all devices ? Aren't there devices that would need the
clock to be turned on after the power supply is stable ?
+ ret = regulator_bulk_enable(ssdd-num_regulators,
ssdd-regulators);
if (ret 0) {
dev_err(dev, Cannot enable regulators\n);
- return ret;
+ goto eregenable;;
}
if (ssdd-power) {
@@ -64,16 +71,25 @@ int soc_camera_power_on(struct device *dev, struct
soc_camera_subdev_desc *ssdd) if (ret 0) {
dev_err(dev,
Platform failed to power-on the camera.\n);
- regulator_bulk_disable(ssdd-num_regulators,
-ssdd-regulators);
+ goto epwron;
}
}
+ return 0;
+
+epwron:
+ regulator_bulk_disable(ssdd-num_regulators,
+ssdd-regulators);
+eregenable:
+ if (clk)
+ v4l2_clk_disable(clk);
+
return ret;
}
EXPORT_SYMBOL(soc_camera_power_on);
-int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc
*ssdd)
+int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc
*ssdd,
+ struct v4l2_clk *clk)
{
int ret = 0;
int err;
@@ -94,28 +110,44 @@ int soc_camera_power_off(struct device *dev, struct
soc_camera_subdev_desc *ssdd ret = ret ? : err;
}
+ if (clk)
+ v4l2_clk_disable(clk);
+
return ret;
}
EXPORT_SYMBOL(soc_camera_power_off);
static int __soc_camera_power_on(struct soc_camera_device *icd)
{
+ struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
int ret;
+ if (!icd-clk) {
+ ret = ici-ops-add(icd);
+ if (ret 0)
+ return ret;
+ }
+
ret = v4l2_subdev_call(sd, core, s_power, 1);
- if (ret 0 ret != -ENOIOCTLCMD ret != -ENODEV)
+ if (ret 0 ret != -ENOIOCTLCMD ret != -ENODEV) {
+ if (!icd-clk)
+ ici-ops-remove(icd);
return ret;
+ }
return 0;
}
static int __soc_camera_power_off(struct soc_camera_device *icd)
{
+ struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
int ret;
ret = v4l2_subdev_call(sd, core, s_power, 0);
+ if (!icd-clk)
+ ici-ops-remove(icd);
if (ret 0 ret != -ENOIOCTLCMD ret != -ENODEV)
return ret;
@@ -563,12 +595,6 @@ static int soc_camera_open(struct file *file)
if (sdesc-subdev_desc.reset)
sdesc-subdev_desc.reset(icd-pdev);
- ret = ici-ops-add(icd);
- if (ret 0) {
- dev_err(icd-pdev, Couldn't activate the camera:
%d\n, ret);
- goto eiciadd;
- }
-
ret = __soc_camera_power_on(icd);
if (ret 0)
goto epower;
@@ -614,8 +640,6 @@ esfmt:
eresume:
__soc_camera_power_off(icd);
epower:
- ici-ops-remove(icd);
-eiciadd:
icd-use_count--;
mutex_unlock(ici-host_lock);
elockhost:
@@ -638,7 +662,6 @@ static int soc_camera_close(struct file *file)
if (ici-ops-init_videobuf2)
vb2_queue_release(icd-vb2_vidq);
- ici-ops-remove(icd);
Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
On Fri March 15 2013 22:27:49 Guennadi Liakhovetski wrote:
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
v6: clock name update
drivers/media/i2c/soc_camera/imx074.c | 18 ++-
drivers/media/i2c/soc_camera/mt9m001.c | 17 ++-
drivers/media/i2c/soc_camera/mt9m111.c | 20 ++-
drivers/media/i2c/soc_camera/mt9t031.c | 19 ++-
drivers/media/i2c/soc_camera/mt9t112.c | 19 ++-
drivers/media/i2c/soc_camera/mt9v022.c | 17 ++-
drivers/media/i2c/soc_camera/ov2640.c | 19 ++-
drivers/media/i2c/soc_camera/ov5642.c | 20 ++-
drivers/media/i2c/soc_camera/ov6650.c | 17 ++-
drivers/media/i2c/soc_camera/ov772x.c | 15 ++-
drivers/media/i2c/soc_camera/ov9640.c | 17 ++-
drivers/media/i2c/soc_camera/ov9640.h |1 +
drivers/media/i2c/soc_camera/ov9740.c | 18 ++-
drivers/media/i2c/soc_camera/rj54n1cb0c.c | 17 ++-
drivers/media/i2c/soc_camera/tw9910.c | 18 ++-
drivers/media/platform/soc_camera/soc_camera.c | 172
+++-
.../platform/soc_camera/soc_camera_platform.c |2 +-
include/media/soc_camera.h | 13 +-
18 files changed, 355 insertions(+), 84 deletions(-)
diff --git a/drivers/media/i2c/soc_camera/imx074.c
b/drivers/media/i2c/soc_camera/imx074.c
index a2a5cbb..cee5345 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -18,6 +18,7 @@
#include linux/module.h
#include media/soc_camera.h
+#include media/v4l2-clk.h
#include media/v4l2-subdev.h
#include media/v4l2-chip-ident.h
@@ -77,6 +78,7 @@ struct imx074_datafmt {
struct imx074 {
struct v4l2_subdev subdev;
const struct imx074_datafmt *fmt;
+ struct v4l2_clk *clk;
};
static const struct imx074_datafmt imx074_colour_fmts[] = {
@@ -272,8 +274,9 @@ static int imx074_s_power(struct v4l2_subdev *sd, int on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
- return soc_camera_set_power(client-dev, ssdd, on);
+ return soc_camera_set_power(client-dev, ssdd, priv-clk, on);
}
static int imx074_g_mbus_config(struct v4l2_subdev *sd,
@@ -431,6 +434,7 @@ static int imx074_probe(struct i2c_client *client,
struct imx074 *priv;
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(client-dev, IMX074: missing platform data!\n);
@@ -451,13 +455,23 @@ static int imx074_probe(struct i2c_client *client,
priv-fmt = imx074_colour_fmts[0];
- return imx074_video_probe(client);
+ priv-clk = v4l2_clk_get(priv-subdev, mclk);
+ if (IS_ERR(priv-clk))
+ return PTR_ERR(priv-clk);
+
+ ret = imx074_video_probe(client);
+ if (ret 0)
+ v4l2_clk_put(priv-clk);
+
I feel uneasy about this. It's not the clock part as such but the fact that
assumptions are made about the usage of this sensor driver. It basically
comes down to the fact that these drivers are *still* tied to the soc-camera
framework. I think I am going to work on this in a few weeks time to cut
these drivers loose from soc-camera. We discussed how to do that in the past.
The whole point of the subdev API is to make drivers independent of bridge
drivers, and these soc-camera subdev drivers are the big exception and they
stick out like a sore thumb.
Anyway, w.r.t. the clock use: what happens if these drivers are used in e.g.
a USB webcam driver? In that case there probably won't be a clock involved
(well, there is one, but that is likely to be setup by the firmware/hardware
itself).
Wouldn't it be better if the clock name is passed on through the platform data
(or device tree)? And if no clock name was specified, then there is no need to
get a clock either and the driver can assume that it will always have a clock.
That would solve this problem when this sensor driver is no longer soc-camera
dependent.
Sorry if this was discussed in earlier patches, I haven't been following this
very closely before.
Regards,
Hans
+ return ret;
}
static int imx074_remove(struct i2c_client *client)
{
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
+ v4l2_clk_put(priv-clk);
if (ssdd-free_bus)
Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
On Mon, 18 Mar 2013, Hans Verkuil wrote:
On Fri March 15 2013 22:27:49 Guennadi Liakhovetski wrote:
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
v6: clock name update
drivers/media/i2c/soc_camera/imx074.c | 18 ++-
drivers/media/i2c/soc_camera/mt9m001.c | 17 ++-
drivers/media/i2c/soc_camera/mt9m111.c | 20 ++-
drivers/media/i2c/soc_camera/mt9t031.c | 19 ++-
drivers/media/i2c/soc_camera/mt9t112.c | 19 ++-
drivers/media/i2c/soc_camera/mt9v022.c | 17 ++-
drivers/media/i2c/soc_camera/ov2640.c | 19 ++-
drivers/media/i2c/soc_camera/ov5642.c | 20 ++-
drivers/media/i2c/soc_camera/ov6650.c | 17 ++-
drivers/media/i2c/soc_camera/ov772x.c | 15 ++-
drivers/media/i2c/soc_camera/ov9640.c | 17 ++-
drivers/media/i2c/soc_camera/ov9640.h |1 +
drivers/media/i2c/soc_camera/ov9740.c | 18 ++-
drivers/media/i2c/soc_camera/rj54n1cb0c.c | 17 ++-
drivers/media/i2c/soc_camera/tw9910.c | 18 ++-
drivers/media/platform/soc_camera/soc_camera.c | 172
+++-
.../platform/soc_camera/soc_camera_platform.c |2 +-
include/media/soc_camera.h | 13 +-
18 files changed, 355 insertions(+), 84 deletions(-)
diff --git a/drivers/media/i2c/soc_camera/imx074.c
b/drivers/media/i2c/soc_camera/imx074.c
index a2a5cbb..cee5345 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -18,6 +18,7 @@
#include linux/module.h
#include media/soc_camera.h
+#include media/v4l2-clk.h
#include media/v4l2-subdev.h
#include media/v4l2-chip-ident.h
@@ -77,6 +78,7 @@ struct imx074_datafmt {
struct imx074 {
struct v4l2_subdev subdev;
const struct imx074_datafmt *fmt;
+ struct v4l2_clk *clk;
};
static const struct imx074_datafmt imx074_colour_fmts[] = {
@@ -272,8 +274,9 @@ static int imx074_s_power(struct v4l2_subdev *sd, int
on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
- return soc_camera_set_power(client-dev, ssdd, on);
+ return soc_camera_set_power(client-dev, ssdd, priv-clk, on);
}
static int imx074_g_mbus_config(struct v4l2_subdev *sd,
@@ -431,6 +434,7 @@ static int imx074_probe(struct i2c_client *client,
struct imx074 *priv;
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(client-dev, IMX074: missing platform data!\n);
@@ -451,13 +455,23 @@ static int imx074_probe(struct i2c_client *client,
priv-fmt = imx074_colour_fmts[0];
- return imx074_video_probe(client);
+ priv-clk = v4l2_clk_get(priv-subdev, mclk);
+ if (IS_ERR(priv-clk))
+ return PTR_ERR(priv-clk);
+
+ ret = imx074_video_probe(client);
+ if (ret 0)
+ v4l2_clk_put(priv-clk);
+
I feel uneasy about this. It's not the clock part as such but the fact that
assumptions are made about the usage of this sensor driver. It basically
comes down to the fact that these drivers are *still* tied to the soc-camera
framework. I think I am going to work on this in a few weeks time to cut
these drivers loose from soc-camera. We discussed how to do that in the past.
Sorry, not sure I understand. This is a generic (V4L2) clock, it has
nothing specific to soc-camera.
The whole point of the subdev API is to make drivers independent of bridge
drivers, and these soc-camera subdev drivers are the big exception and they
stick out like a sore thumb.
We are moving towards complete driver independency from the soc-camera
framework, and, afaics, there's not much left. Simply noone is interested
enough to do the work or to pay for it, noone has a really burning
use-case. And without one it's not very easy to implement things with no
test case. But sure, you're most welcome to work on this :)
Anyway, w.r.t. the clock use: what happens if these drivers are used in e.g.
a USB webcam driver? In that case there probably won't be a clock involved
(well, there is one, but that is likely to be setup by the firmware/hardware
itself).
Well, from the sensor driver PoV if the sensor needs a clock it seems
logical for the driver to request it and to fail if it's not available.
USB cameras could provide a dummy clock, or we could
Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
On Mon March 18 2013 11:08:16 Guennadi Liakhovetski wrote:
On Mon, 18 Mar 2013, Hans Verkuil wrote:
On Fri March 15 2013 22:27:49 Guennadi Liakhovetski wrote:
Instead of centrally enabling and disabling subdevice master clocks in
soc-camera core, let subdevice drivers do that themselves, using the
V4L2 clock API and soc-camera convenience wrappers.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
v6: clock name update
drivers/media/i2c/soc_camera/imx074.c | 18 ++-
drivers/media/i2c/soc_camera/mt9m001.c | 17 ++-
drivers/media/i2c/soc_camera/mt9m111.c | 20 ++-
drivers/media/i2c/soc_camera/mt9t031.c | 19 ++-
drivers/media/i2c/soc_camera/mt9t112.c | 19 ++-
drivers/media/i2c/soc_camera/mt9v022.c | 17 ++-
drivers/media/i2c/soc_camera/ov2640.c | 19 ++-
drivers/media/i2c/soc_camera/ov5642.c | 20 ++-
drivers/media/i2c/soc_camera/ov6650.c | 17 ++-
drivers/media/i2c/soc_camera/ov772x.c | 15 ++-
drivers/media/i2c/soc_camera/ov9640.c | 17 ++-
drivers/media/i2c/soc_camera/ov9640.h |1 +
drivers/media/i2c/soc_camera/ov9740.c | 18 ++-
drivers/media/i2c/soc_camera/rj54n1cb0c.c | 17 ++-
drivers/media/i2c/soc_camera/tw9910.c | 18 ++-
drivers/media/platform/soc_camera/soc_camera.c | 172
+++-
.../platform/soc_camera/soc_camera_platform.c |2 +-
include/media/soc_camera.h | 13 +-
18 files changed, 355 insertions(+), 84 deletions(-)
diff --git a/drivers/media/i2c/soc_camera/imx074.c
b/drivers/media/i2c/soc_camera/imx074.c
index a2a5cbb..cee5345 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -18,6 +18,7 @@
#include linux/module.h
#include media/soc_camera.h
+#include media/v4l2-clk.h
#include media/v4l2-subdev.h
#include media/v4l2-chip-ident.h
@@ -77,6 +78,7 @@ struct imx074_datafmt {
struct imx074 {
struct v4l2_subdev subdev;
const struct imx074_datafmt *fmt;
+ struct v4l2_clk *clk;
};
static const struct imx074_datafmt imx074_colour_fmts[] = {
@@ -272,8 +274,9 @@ static int imx074_s_power(struct v4l2_subdev *sd, int
on)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ struct imx074 *priv = to_imx074(client);
- return soc_camera_set_power(client-dev, ssdd, on);
+ return soc_camera_set_power(client-dev, ssdd, priv-clk, on);
}
static int imx074_g_mbus_config(struct v4l2_subdev *sd,
@@ -431,6 +434,7 @@ static int imx074_probe(struct i2c_client *client,
struct imx074 *priv;
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(client-dev, IMX074: missing platform data!\n);
@@ -451,13 +455,23 @@ static int imx074_probe(struct i2c_client *client,
priv-fmt = imx074_colour_fmts[0];
- return imx074_video_probe(client);
+ priv-clk = v4l2_clk_get(priv-subdev, mclk);
+ if (IS_ERR(priv-clk))
+ return PTR_ERR(priv-clk);
+
+ ret = imx074_video_probe(client);
+ if (ret 0)
+ v4l2_clk_put(priv-clk);
+
I feel uneasy about this. It's not the clock part as such but the fact that
assumptions are made about the usage of this sensor driver. It basically
comes down to the fact that these drivers are *still* tied to the soc-camera
framework. I think I am going to work on this in a few weeks time to cut
these drivers loose from soc-camera. We discussed how to do that in the
past.
Sorry, not sure I understand. This is a generic (V4L2) clock, it has
nothing specific to soc-camera.
The assumption that there is a clock that needs to be set up is soc_camera
specific IMHO.
The whole point of the subdev API is to make drivers independent of bridge
drivers, and these soc-camera subdev drivers are the big exception and they
stick out like a sore thumb.
We are moving towards complete driver independency from the soc-camera
framework, and, afaics, there's not much left. Simply noone is interested
enough to do the work or to pay for it, noone has a really burning
use-case. And without one it's not very easy to implement things with no
test case. But sure, you're most welcome to work on this :)
I'll see what I can do since I am interested in doing this :-)
Anyway, w.r.t. the clock use: what happens if these drivers are used in e.g.
a USB webcam driver? In that case there probably won't be a clock involved
(well,
Re: [PATCH v6 3/7] media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
On 03/18/2013 11:23 AM, Hans Verkuil wrote:
On Mon March 18 2013 11:08:16 Guennadi Liakhovetski wrote:
On Mon, 18 Mar 2013, Hans Verkuil wrote:
On Fri March 15 2013 22:27:49 Guennadi Liakhovetski wrote:
[...]
@@ -431,6 +434,7 @@ static int imx074_probe(struct i2c_client *client,
struct imx074 *priv;
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+ int ret;
if (!ssdd) {
dev_err(client-dev, IMX074: missing platform data!\n);
@@ -451,13 +455,23 @@ static int imx074_probe(struct i2c_client *client,
priv-fmt=imx074_colour_fmts[0];
- return imx074_video_probe(client);
+ priv-clk = v4l2_clk_get(priv-subdev, mclk);
+ if (IS_ERR(priv-clk))
+ return PTR_ERR(priv-clk);
+
+ ret = imx074_video_probe(client);
+ if (ret 0)
+ v4l2_clk_put(priv-clk);
+
I feel uneasy about this. It's not the clock part as such but the fact that
assumptions are made about the usage of this sensor driver. It basically
comes down to the fact that these drivers are *still* tied to the soc-camera
framework. I think I am going to work on this in a few weeks time to cut
these drivers loose from soc-camera. We discussed how to do that in the past.
Sorry, not sure I understand. This is a generic (V4L2) clock, it has
nothing specific to soc-camera.
The assumption that there is a clock that needs to be set up is soc_camera
specific IMHO.
I don't think this is really soc-camera specific. This is specific to most
SoC camera subsystems, also those not using soc-camera framework (even
though
there might be not that many of them ;)).
The whole point of the subdev API is to make drivers independent of bridge
drivers, and these soc-camera subdev drivers are the big exception and they
stick out like a sore thumb.
We are moving towards complete driver independency from the soc-camera
framework, and, afaics, there's not much left. Simply noone is interested
enough to do the work or to pay for it, noone has a really burning
use-case. And without one it's not very easy to implement things with no
test case. But sure, you're most welcome to work on this :)
I'll see what I can do since I am interested in doing this :-)
Anyway, w.r.t. the clock use: what happens if these drivers are used in e.g.
a USB webcam driver? In that case there probably won't be a clock involved
(well, there is one, but that is likely to be setup by the firmware/hardware
itself).
Well, from the sensor driver PoV if the sensor needs a clock it seems
logical for the driver to request it and to fail if it's not available.
USB cameras could provide a dummy clock, or we could implement one
centrally, however, this will lead to an undesirable result, that everyone
will just use that dummy clock... If we make clock support optional the
same thing will happen - noone will implement them. BTW, you're looking at
an intermediate patch in this series, which only adds clock support. In a
later patch the return error code for missing clock will be replaced with
-EPROBE_DEFER which serves as a sign, that no bridge driver is available
yes and _is_ required to support asynchronous probing.
Creating a dummy clock in a USB device would work, I agree.
Forget my other remarks: I hadn't realized that the global list of clocks
(clk_list) is unique per device (i2c adapter-i2c addr), so you can add
multiple clocks with the same name (mclk) and still match them to the correct
device.
That makes it all work as it should.
Regards,
Hans
Wouldn't it be better if the clock name is passed on through the platform data
(or device tree)? And if no clock name was specified, then there is no need to
get a clock either and the driver can assume that it will always have a clock.
That would solve this problem when this sensor driver is no longer soc-camera
dependent.
No. Yes, this has been discussed many times - in the context of the
generic clock API. I also proposed a patch, that did such a thing and was
kindly explained, why that wasn't a good idea :-) Clock names are names
of clock _inputs_ on the consumer. I.e. a sensor driver should request a
clock according to its datasheet. For the clock provider it's different,
say, a bridge driver cannot know what sensor will be connected to it and
clock it will be expecting. That's why we have clock lookup tables, that
connect physical clock objects (providers) with consumer clock names in
platform data (perhaps, a similar thing is done in DT, haven't looked
yet). I think, we could accept a compromise by using a common name for all
clocks with the same function. I'm using mclk as an abbreviation for
master clock.
I guess using a common mclk name would work. It's easy to specify this
in the device tree. In the clock consumer device's node an exact clock
name, according to the DT binding, can be specified in the 'clock-names'
