em28xx + ov2640 and v4l2-clk

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Thanks
Guennadi

-----Original Message-----
From: "Frank Schäfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the
em28xx/ov2640/soc-camera interaction this week.
Have a nice holiday !

Regards,
Frank

Quoted 12 lines

Thanks
Guennadi

-----Original Message-----
From: "Frank Schäfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 7 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 8 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Hi Mauro,

Quoted 43 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 7 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 8 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Quoted 2 lines

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ?

The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation.

Hi Mauro,

Quoted 40 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 6 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 7 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Quoted 2 lines

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation.

Em Tue, 20 Aug 2013 15:38:57 +0200
Laurent Pinchart <[email protected]> escreveu:

Quoted 48 lines

Hi Mauro,

Quoted 34 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 6 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 7 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

Quoted 8 lines

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Quoted 2 lines

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ?

Well, it is simple: we should either revert the patch(es) that broke it or someone should fix it at em28xx. If nobody could fix it, I'll just revert the patches that broke it and ask -stable to do the same.

Btw, 3.10 is a long term stable, so, it is not too late for fixes there.

Quoted 4 lines

The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation.

Ok, but this doesn't solve one issue: who would do it and when.

Cheers,
Mauro

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 43 lines

Hi Mauro,

Quoted 40 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 6 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 7 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Quoted 8 lines

Quoted 2 lines

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

    priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation like em28xx+ov2640 ?
For UXGA for example, the clock needs to be switched to 12MHz, while 24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the switch (to define fixed bindings between resoultions and clock frequencies). Btw, what if a frequency is requested which isn't supported ? Set the clock to the next nearest supported frequency ?

Regards,
Frank

Quoted 5 lines

v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation.

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 66 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 43 lines

Hi Mauro,

Quoted 40 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 6 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 7 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski<[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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<[email protected]>

Acked-by Hans Verkuil<[email protected]>

Acked-by Laurent Pinchart<[email protected]>

Signed-off-by Mauro Carvalho Chehab<[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Quoted 8 lines

Quoted 2 lines

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

Quoted 10 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation like em28xx+ov2640 ?
For UXGA for example, the clock needs to be switched to 12MHz, while 24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the switch (to define fixed bindings between resoultions and clock frequencies). Btw, what if a frequency is requested which isn't supported ? Set the clock to the next nearest supported frequency ?

Quoted 2 lines

Regards,
Frank

I tried to implement a v4l2_clk for the em28xx driver (not yet beeing sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have specific expectations for the names of the clock.
The name must me "mclk" and dev_name must be the device name of the i2c client device.
Is "mclk" supposed to be a clock type ? Wouldn't an enum be a better choice in this case ?

Anyway, the sensor subdevices are registered using v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is found during sensor probing), but at this point the device name of the i2c_client isn't known yet...

Hi Frank,

Quoted 78 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 77 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 56 lines

Hi Mauro,

Quoted 44 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 12 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Quoted 4 lines

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski<[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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<[email protected]>

Acked-by Hans Verkuil<[email protected]>

Acked-by Laurent Pinchart<[email protected]>

Signed-off-by Mauro Carvalho Chehab<[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx
cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Quoted 5 lines

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera
and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a
change. Mistakes happen, so let's fix them.

Quoted 6 lines

Guennadi is on holidays until the end of this week. Would that be too late to
fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Quoted 16 lines

Quoted 13 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation
like em28xx+ov2640 ?
For UXGA for example, the clock needs to be switched to 12MHz, while 24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the clock to the next nearest supported frequency ?

Quoted 2 lines

Regards,
Frank

Quoted 8 lines

I tried to implement a v4l2_clk for the em28xx driver (not yet beeing sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have specific expectations for the names of the clock.
The name must me "mclk" and dev_name must be the device name of the i2c client device.
Is "mclk" supposed to be a clock type ? Wouldn't an enum be a better choice in this case ?

This is made similar to the common clock API, a string is an identifier of a clock for the device. I can't see anything unusual in that, it will also make it easier to phase out the v4l2-clk API and replace it with the common clock API once that is more widely available.

The name is supposed to come from the datasheet and usually be different for each sensor, but since we mostly deal with just one clock a common "mclk" name was chosen for simplicity.

Quoted 7 lines

Anyway, the sensor subdevices are registered using v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is found during sensor probing), but at this point the device name of the i2c_client isn't known yet...

    snprintf(clk_name, sizeof(clk_name), "%d-%04x",
         dev->i2c_adap[dev->def_i2c_bus].nr, ov2640_info.addr);

    clk = v4l2_clk_register(&em28xx_sensor_clk_ops, clk_name, "mclk",
icd);
    if (IS_ERR(icd->clk)) {
        ...
    }
    //////////////////////////////////////////////////////

    subdev =
         v4l2_i2c_new_subdev_board(&dev->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       &ov2640_info, NULL);
    ...

Alternatively all sensors modified by changeset 9aea470b399d797e88be "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" could receive a platform data flag that would be set for drivers like em28xx, and which would be indicating if the clock should be used or not. It probably should has been done originally if it wasn't clear which bridge drivers use that modified sensors and that regressions were possible.

Signature · 3 lines

-- 
Regards,
Sylwester

Am 20.08.2013 17:31, schrieb Mauro Carvalho Chehab:

Quoted 57 lines

Em Tue, 20 Aug 2013 15:38:57 +0200
Laurent Pinchart <[email protected]> escreveu:

Quoted 43 lines

Hi Mauro,

Quoted 40 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 6 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 7 lines

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Quoted 7 lines

Quoted 2 lines

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them.

Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ?

Well, it is simple: we should either revert the patch(es) that broke it or someone should fix it at em28xx. If nobody could fix it, I'll just revert the patches that broke it and ask -stable to do the same.

Quoted 1 line

Btw, 3.10 is a long term stable, so, it is not too late for fixes there.

AFAICS, 3.10 should be fine.
It should be possible to fix em28xx before Linus releases 3.11, but what about other drivers ?
It seems like a v4l2-clock has been made mandatory for all sensor drivers (not only ov2640).
I don't know if there are any other users of these drivers apart from soc_camera and em28xx... ?

Quoted 6 lines

Quoted 5 lines

The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation.

Ok, but this doesn't solve one issue: who would do it and when.

I can spend some time on em28xx tomorrow evening.

Regards,
Frank

Quoted 3 lines

Cheers,
Mauro

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 89 lines

Hi Frank,

Quoted 78 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 77 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 56 lines

Hi Mauro,

Quoted 44 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 12 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Quoted 4 lines

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski<[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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<[email protected]>

Acked-by Hans Verkuil<[email protected]>

Acked-by Laurent Pinchart<[email protected]>

Signed-off-by Mauro Carvalho Chehab<[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx
cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Quoted 5 lines

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera
and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a
change. Mistakes happen, so let's fix them.

Quoted 6 lines

Guennadi is on holidays until the end of this week. Would that be too late to
fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ?
Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Quoted 30 lines

Quoted 16 lines

Quoted 13 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation
like em28xx+ov2640 ?
For UXGA for example, the clock needs to be switched to 12MHz, while 24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the clock to the next nearest supported frequency ?

Quoted 2 lines

Regards,
Frank

Quoted 8 lines

I tried to implement a v4l2_clk for the em28xx driver (not yet beeing sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have specific expectations for the names of the clock.
The name must me "mclk" and dev_name must be the device name of the i2c client device.
Is "mclk" supposed to be a clock type ? Wouldn't an enum be a better choice in this case ?

This is made similar to the common clock API, a string is an identifier of a clock for the device. I can't see anything unusual in that, it will also make it easier to phase out the v4l2-clk API and replace it with the common clock API once that is more widely available.

Quoted 3 lines

The name is supposed to come from the datasheet and usually be different for each sensor, but since we mostly deal with just one clock a common "mclk" name was chosen for simplicity.

Hmm... the common clock API probably needs to be more flexible. An enum would be safer to use, that's my though.

Anyway, let's focus on the main issue.

Quoted 8 lines

Quoted 7 lines

Anyway, the sensor subdevices are registered using v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is found during sensor probing), but at this point the device name of the i2c_client isn't known yet...

[...]

Quoted 3 lines

    snprintf(clk_name, sizeof(clk_name), "%d-%04x",
         dev->i2c_adap[dev->def_i2c_bus].nr, ov2640_info.addr);

That's a joke, isn't it ? ;)
So people have to grub through half of the kernel to figure out which specific string the subdev driver expects and duplicate the code that creates this compound string to be able to use the driver ?

Quoted 14 lines

    clk = v4l2_clk_register(&em28xx_sensor_clk_ops, clk_name, "mclk",
icd);
    if (IS_ERR(icd->clk)) {
        ...
    }
    //////////////////////////////////////////////////////

    subdev =
         v4l2_i2c_new_subdev_board(&dev->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       &ov2640_info, NULL);
    ...

Quoted 6 lines

Alternatively all sensors modified by changeset 9aea470b399d797e88be "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" could receive a platform data flag that would be set for drivers like em28xx, and which would be indicating if the clock should be used or not. It probably should has been done originally if it wasn't clear which bridge drivers use that modified sensors and that regressions were possible.

Yes, but not using a clock would still break the async subdevice registration (although drivers like the em28xx likely don't need/use it at the moment).

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer<[email protected]> escreveu:

Quoted 101 lines

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 85 lines · Frank Schäfer

Quoted 76 lines

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 75 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 54 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Quoted 44 lines

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 12 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Quoted 4 lines

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski<[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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<[email protected]>

Acked-by Hans Verkuil<[email protected]>

Acked-by Laurent Pinchart<[email protected]>

Signed-off-by Mauro Carvalho Chehab<[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx
cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Quoted 5 lines

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera
and apply the fixes where needed, as no regressions are allowed.

We definitely need to check all users of our sensor drivers when making such a
change. Mistakes happen, so let's fix them.

Quoted 6 lines

Guennadi is on holidays until the end of this week. Would that be too late to
fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

      priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ?
Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

The interface was written to mimic what OF does with clock.

Yeah, I agree that this sucks for non OF drivers.

Quoted 3 lines

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Probably because they weren't converted yet to the new way.

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

Quoted 12 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation
like em28xx+ov2640 ?
For UXGA for example, the clock needs to be switched to 12MHz, while 24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the clock to the next nearest supported frequency ?

Quoted 2 lines

Regards,
Frank

Quoted 8 lines

I tried to implement a v4l2_clk for the em28xx driver (not yet beeing sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have specific expectations for the names of the clock.
The name must me "mclk" and dev_name must be the device name of the i2c client device.
Is "mclk" supposed to be a clock type ? Wouldn't an enum be a better choice in this case ?

This is made similar to the common clock API, a string is an identifier of a clock for the device. I can't see anything unusual in that, it will also make it easier to phase out the v4l2-clk API and replace it with the common clock API once that is more widely available.

Quoted 6 lines

Quoted 3 lines

The name is supposed to come from the datasheet and usually be different for each sensor, but since we mostly deal with just one clock a common "mclk" name was chosen for simplicity.

Hmm... the common clock API probably needs to be more flexible. An enum would be safer to use, that's my though.

Again, this is due to OF/DT. Open Firmware/Device Tree wants to have a flexible, OS-independent way to specify clock sources, device parameters, etc.

Quoted 18 lines

Anyway, let's focus on the main issue.

Quoted 7 lines

Quoted 6 lines

Anyway, the sensor subdevices are registered using v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is found during sensor probing), but at this point the device name of the i2c_client isn't known yet...

[...]

Quoted 3 lines

     snprintf(clk_name, sizeof(clk_name), "%d-%04x",
          dev->i2c_adap[dev->def_i2c_bus].nr, ov2640_info.addr);

That's a joke, isn't it ? ;)
So people have to grub through half of the kernel to figure out which specific string the subdev driver expects and duplicate the code that creates this compound string to be able to use the driver ?

There are some docs describing DT somewhere at Documentation. Maybe Documentation/video4linux/v4l2-framework.txt?

Perhaps it is incomplete.

Quoted 20 lines

Quoted 14 lines

     clk = v4l2_clk_register(&em28xx_sensor_clk_ops, clk_name, "mclk",
icd);
     if (IS_ERR(icd->clk)) {
         ...
     }
     //////////////////////////////////////////////////////

     subdev =
          v4l2_i2c_new_subdev_board(&dev->v4l2_dev,
                        &dev->i2c_adap[dev->def_i2c_bus],
                        &ov2640_info, NULL);
     ...

Quoted 6 lines

Alternatively all sensors modified by changeset 9aea470b399d797e88be "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" could receive a platform data flag that would be set for drivers like em28xx, and which would be indicating if the clock should be used or not. It probably should has been done originally if it wasn't clear which bridge drivers use that modified sensors and that regressions were possible.

Quoted 3 lines

Yes, but not using a clock would still break the async subdevice registration (although drivers like the em28xx likely don't need/use it at the moment).

Well, async subdev only makes sense for Device Tree.

A synchronous init works a way better, as everything initializes at the same order, every time. Allowing subdevs to initialize on a random order is a mess, and should be avoided if possible, as we have already enough complexity to handle. Let's not introduce more if not needed.

It seems fine to only use the a new "dont_use_clk_api" flag on synchronous init.

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 28 lines

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 25 lines

Hi Frank,

Quoted 22 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 21 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 20 lines

Hi Mauro,

Quoted 17 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 16 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

too late to
fix the issue (given that 3.10 is already broken) ? The fix

Quoted 33 lines

Quoted 25 lines

Quoted 17 lines

Quoted 17 lines

Quoted 3 lines

shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Quoted 7 lines

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ?
Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

The interface was written to mimic what OF does with clock.

Yeah, I agree that this sucks for non OF drivers.

Quoted 3 lines

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Probably because they weren't converted yet to the new way.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation
like em28xx+ov2640 ?

Quoted 32 lines

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 28 lines

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 25 lines

Hi Frank,

Quoted 22 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 21 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 20 lines

Hi Mauro,

Quoted 17 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 16 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

Thanks, it was nice indeed :)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

too late to
fix the issue (given that 3.10 is already broken) ? The fix

Don't think it is, "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" only appeared in v3.11-rc1.

Quoted 43 lines

Quoted 33 lines

Quoted 25 lines

Quoted 17 lines

Quoted 17 lines

Quoted 3 lines

shouldn't be too
complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Quoted 7 lines

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ?
Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

The interface was written to mimic what OF does with clock.

Quoted 13 lines

Yeah, I agree that this sucks for non OF drivers.

Quoted 3 lines

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Probably because they weren't converted yet to the new way.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation
like em28xx+ov2640 ?

Ok, let's try to summerise:

* background: many camera sensors do not react to I2C commands as long as no master clock is supplied. Therefore for _those_ sensors making a clock availability seems logical to me. And since it's the sensor driver, that knows what that clock is used for, when it is needed and - eventually - what rate is required - it's the sensor driver, that should manipulate it.
Example: some camera sensor drivers write sensor configuration directly to the hardware in each ioctl() possibly without storing the state internally. Such drivers will need a clock running all the time to keep register values. Other drivers might only store configuration internally and only send it to the hardware when streaming is enabled. Those drivers can keep the clock disabled until that time then.

* problem: em28xx USB camera driver uses the ov2640 camera sensor driver and doesn't supply a clock. But ov2640 sensors do need a clock, so, we have to assume it is supplied internally in the camera. Presumably, it is always on and its rate cannot be adjusted either.

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.
Pro: logically correct - a clock is indeed present, local - no core
changes are needed
Contra: presumably relatively many devices will have such static
always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

(b) make clocks optional in all subdevice drivers
Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

(c) add a global flag to indicate, that the use of clocks on this device is optional
Pro: easy to support in drivers
Contra: as in (b) above

(d) a variant of (a), but with a helper function in V4L2 clock core to implement such a static always-on clock
Pro: simple to support in host / bridge drivers
Contra: adds bloat to V4L2 clock helper layer, which we want to keep
small and remove eventually.

Have I missed anything? Of the above I would go with (d). I could try to code the required always-on clock helpers.

Guennadi Liakhovetski <[email protected]> escreveu:

Quoted 105 lines · Mauro Carvalho Chehab

Quoted 23 lines

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 21 lines

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 20 lines · Frank Schäfer

Quoted 19 lines

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 18 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 17 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Quoted 16 lines

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 15 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

Thanks, it was nice indeed :)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

too late to fix the issue (given that 3.10 is already broken) ? The fix

Don't think it is, "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" only appeared in v3.11-rc1.

Quoted 46 lines

Quoted 37 lines

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

Quoted 2 lines

shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct

v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Quoted 6 lines

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ? Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

The interface was written to mimic what OF does with clock.

Quoted 11 lines

Yeah, I agree that this sucks for non OF drivers.

Quoted 3 lines

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Probably because they weren't converted yet to the new way.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation like em28xx+ov2640 ?

Ok, let's try to summerise:

* background: many camera sensors do not react to I2C commands as long as no master clock is supplied. Therefore for _those_ sensors making a clock availability seems logical to me. And since it's the sensor driver, that knows what that clock is used for, when it is needed and - eventually - what rate is required - it's the sensor driver, that should manipulate it.
Example: some camera sensor drivers write sensor configuration directly to the hardware in each ioctl() possibly without storing the state internally. Such drivers will need a clock running all the time to keep register values. Other drivers might only store configuration internally and only send it to the hardware when streaming is enabled. Those drivers can keep the clock disabled until that time then.

Quoted 4 lines

* problem: em28xx USB camera driver uses the ov2640 camera sensor driver and doesn't supply a clock. But ov2640 sensors do need a clock, so, we have to assume it is supplied internally in the camera. Presumably, it is always on and its rate cannot be adjusted either.

Guennadi,

I don't have the schematics of those cameras, but I suspect that the clock for the sensor is hardwired, e. g. probably em28xx can't enable or disable it. This is the usual solution on non-embedded hardware.

That's why, IMHO, putting anything at the USB bridge driver (em28xx) makes no sense: the bridge doesn't have any control over the clock.

Quoted 11 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core
changes are needed
Contra: presumably relatively many devices will have such static

always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

Quoted 4 lines

(b) make clocks optional in all subdevice drivers

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

If the clock can't be controlled via software, there's no sense on adding control stuff for it: it will just add extra code for no good reason.

Quoted 12 lines

(c) add a global flag to indicate, that the use of clocks on this device

is optional
Pro: easy to support in drivers
Contra: as in (b) above

(d) a variant of (a), but with a helper function in V4L2 clock core to

implement such a static always-on clock
Pro: simple to support in host / bridge drivers
Contra: adds bloat to V4L2 clock helper layer, which we want to keep

Quoted 4 lines

small and remove eventually.

Have I missed anything? Of the above I would go with (d). I could try to code the required always-on clock helpers.

I prefer to have some solution that won't add any extra code if the clock is always on and can't be controlled.

Hi Mauro,

Quoted 113 lines · On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:

Guennadi Liakhovetski <[email protected]> escreveu:

Quoted 105 lines · Mauro Carvalho Chehab

Quoted 23 lines

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 21 lines

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 20 lines · Frank Schäfer

Quoted 19 lines

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 18 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 17 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Quoted 16 lines

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 15 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 3 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 3 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

Thanks, it was nice indeed :)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

too late to fix the issue (given that 3.10 is already broken) ? The fix

Don't think it is, "[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk" only appeared in v3.11-rc1.

Quoted 46 lines

Quoted 37 lines

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

Quoted 2 lines

shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough.

I would prefer either a) making the clock optional in the senor driver(s) or b) implementing a real V4L2 clock.

Reading the soc-camera code, it looks like NULL-pointers for struct

v4l2_clk are handled correctly. so a) should be pretty simple:

     priv->clk = v4l2_clk_get(&client->dev, "mclk");

(quoted diff) +2 -4

-   if (IS_ERR(priv->clk)) {
-       ret = PTR_ERR(priv->clk);
-       goto eclkget;
-   }
+   if (IS_ERR(priv->clk))
+       priv->clk = NULL;

Quoted 5 lines

Quoted 2 lines

Some additional NULL-pointer checks might be necessary, e.g. before calling v4l2_clk_put().

Tested and that works.
Patch follows.

That patch breaks subdevs registration through the v4l2-async. See commit

Quoted 8 lines

ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
[media] V4L2: mt9m111: switch to asynchronous subdevice probing

Sensor probe() callback must return EPROBE_DEFER when the clock is not found. This cause the sensor's probe() callback to be called again by the driver core after some other driver has probed, e.g. the one that registers v4l2_clk. If specific error code is not returned from probe() the whole registration process breaks.

Quoted 6 lines

Urgh... great. :/
So the presence of a clock is used as indicator if the device is ready ? Honestly, that sounds like a misuse... Is there no other way to check if the device is ready ? Please don't get me wrong, I noticed you've been working on the async subdevice registration patches for quite a long time and I'm sure it wasn't an easy task.

The interface was written to mimic what OF does with clock.

Quoted 11 lines

Yeah, I agree that this sucks for non OF drivers.

Quoted 3 lines

Btw: only 2 of the 14 drivers return -EPROBE_DEFER when no clock is
found: imx074, mt9m111m.
All others return the error code from v4l2_clk_get(), usually -ENODEV.

Probably because they weren't converted yet to the new way.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Concerning b): I'm not yet sure if it is really needed/makes sense... Who is supposed to configure/enable/disable the clock in a constellation like em28xx+ov2640 ?

Ok, let's try to summerise:

* background: many camera sensors do not react to I2C commands as long as no master clock is supplied. Therefore for _those_ sensors making a clock availability seems logical to me. And since it's the sensor driver, that knows what that clock is used for, when it is needed and - eventually - what rate is required - it's the sensor driver, that should manipulate it.
Example: some camera sensor drivers write sensor configuration directly to the hardware in each ioctl() possibly without storing the state internally. Such drivers will need a clock running all the time to keep register values. Other drivers might only store configuration internally and only send it to the hardware when streaming is enabled. Those drivers can keep the clock disabled until that time then.

Quoted 4 lines

* problem: em28xx USB camera driver uses the ov2640 camera sensor driver and doesn't supply a clock. But ov2640 sensors do need a clock, so, we have to assume it is supplied internally in the camera. Presumably, it is always on and its rate cannot be adjusted either.

Guennadi,

Quoted 3 lines

I don't have the schematics of those cameras, but I suspect that the clock for the sensor is hardwired, e. g. probably em28xx can't enable or disable it. This is the usual solution on non-embedded hardware.

Possibly. Or the em28xx controls the clock transparently. We will probably never know, and it doesn't matter much at the end of the day. We know that the clock is on whenever we access the sensor, so we can consider that clock as an always-on clock for all practical matters.

Quoted 2 lines

That's why, IMHO, putting anything at the USB bridge driver (em28xx) makes no sense: the bridge doesn't have any control over the clock.

That's where I don't agree. Here we need to think about the bridge as the combination of the bridge chip and the board on which it's soldered, as the board itself isn't modelled separately.

Even if the bridge doesn't control the clock, it provides a clock to the sensor.

As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere.

This is a fundamental principle of the Linux clock framework and regulator framework. For fixed-frequency always-on clocks, as well as for fixed-voltage always-on regulators, the clock and/or regulator provider just needs to register a fixed clock or regulator, which is very easy to do.

The v4l2-clock API has been designed to mimic the clock API to ease the transition to the clock API at a later time (the v4l2-clock API is meant to be temporary only). It doesn't offer all the helper functions available in the clock API and should thus be improved, as Guennadi pointed out.

Quoted 12 lines

Quoted 12 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core
changes are needed
Contra: presumably relatively many devices will have such static

always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

Quoted 7 lines

Quoted 4 lines

(b) make clocks optional in all subdevice drivers

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant. Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not. That's something we wanted to avoid, as it would result in code bloat. We've instead pushed all that common code to the core, with a requirement for clock providers to register a clock, even if it can't be controlled.

Quoted 20 lines

If the clock can't be controlled via software, there's no sense on adding control stuff for it: it will just add extra code for no good reason.

Quoted 16 lines

(c) add a global flag to indicate, that the use of clocks on this device

is optional
Pro: easy to support in drivers
Contra: as in (b) above

(d) a variant of (a), but with a helper function in V4L2 clock core to

implement such a static always-on clock
Pro: simple to support in host / bridge drivers
Contra: adds bloat to V4L2 clock helper layer, which we want to keep

small and remove eventually.

Have I missed anything? Of the above I would go with (d). I could try to code the required always-on clock helpers.

Quoted 2 lines

I prefer to have some solution that won't add any extra code if the clock is always on and can't be controlled.

But that's not how the common clock framework works. Sure, we could implement that right now in the v4l2-clock API, but we will need to register a fixed- clock in the em28xx driver when moving to the clock API anyway. Let's not make the transition more complex than it should be.

Laurent Pinchart <[email protected]> escreveu:

Quoted 2 lines · On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:

Quoted 1 line

Guennadi Liakhovetski <[email protected]> escreveu:

Quoted 22 lines

Quoted 22 lines

Quoted 15 lines

Ok, let's try to summerise:

* background: many camera sensors do not react to I2C commands as long as no master clock is supplied. Therefore for _those_ sensors making a clock availability seems logical to me. And since it's the sensor driver, that knows what that clock is used for, when it is needed and
- eventually - what rate is required - it's the sensor driver, that should manipulate it.
Example: some camera sensor drivers write sensor configuration directly to the hardware in each ioctl() possibly without storing the state internally. Such drivers will need a clock running all the time to keep register values. Other drivers might only store configuration internally and only send it to the hardware when streaming is enabled. Those drivers can keep the clock disabled until that time then.

Quoted 4 lines

* problem: em28xx USB camera driver uses the ov2640 camera sensor driver and doesn't supply a clock. But ov2640 sensors do need a clock, so, we have to assume it is supplied internally in the camera. Presumably, it is always on and its rate cannot be adjusted either.

Guennadi,

Quoted 8 lines

Quoted 3 lines

I don't have the schematics of those cameras, but I suspect that the clock for the sensor is hardwired, e. g. probably em28xx can't enable or disable it. This is the usual solution on non-embedded hardware.

Possibly. Or the em28xx controls the clock transparently. We will probably never know, and it doesn't matter much at the end of the day. We know that the clock is on whenever we access the sensor, so we can consider that clock as an always-on clock for all practical matters.

Yes.

Quoted 3 lines

Quoted 2 lines

That's why, IMHO, putting anything at the USB bridge driver (em28xx) makes no sense: the bridge doesn't have any control over the clock.

Quoted 4 lines

That's where I don't agree. Here we need to think about the bridge as the combination of the bridge chip and the board on which it's soldered, as the
board itself isn't modelled separately.

Yes, we agree to disagree on this. The board layout is not the bridge.

Quoted 2 lines

Even if the bridge doesn't control the clock, it provides a clock to the sensor.

That highly depends on how it is wired. The clock could be provided by some independent circuit, or could be driven from the bridge clock. It is very common on those em28xx sticks to see two or even more xtals on it.

Quoted 3 lines

As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere.

"Somewhere" doesn't mean that it comes from em28xx.

Quoted 5 lines

This is a fundamental principle of the Linux clock framework and regulator framework. For fixed-frequency always-on clocks, as well as for fixed-voltage always-on regulators, the clock and/or regulator provider just needs to register a fixed clock or regulator, which is very easy to do.

Ok, but the voltage and clock regulators are not mapped, on embedded devices, as part of the USB or PCI bus bridge device (except, of course, when the voltage/clocks are needed by the bridge device itself). It is mapped elsewhere, at DT.

Quoted 5 lines

The v4l2-clock API has been designed to mimic the clock API to ease the transition to the clock API at a later time (the v4l2-clock API is meant to be temporary only). It doesn't offer all the helper functions available in the clock API and should thus be improved, as Guennadi pointed out.

That argument I understand: it should mimic the clock API, to avoid rework.

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core changes are needed
Contra: presumably relatively many devices will have such static

always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

Quoted 7 lines

Quoted 4 lines

(b) make clocks optional in all subdevice drivers

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

Quoted 7 lines

No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant. Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not. That's something we wanted to avoid, as it would result in code bloat. We've instead pushed all that common code to the core, with a requirement for clock providers to register a clock, even if it can't be controlled.

Software can't provide clock. Only hardware can do it.

If the hardware already provides it, it doesn't make any sense to add an API to control something that can't be controlled at all.

So, the only sense on having a clock API is when the hardware allows some control on it.

So, if the hardware can't be controlled and it is always on, it makes no sense to register a clock.

The thing is that you're wanting to use the clock register as a way to detect that the device got initialized.

This is wrong on devices where the clock is hardwired.

Quoted 23 lines

Quoted 23 lines

If the clock can't be controlled via software, there's no sense on adding control stuff for it: it will just add extra code for no good reason.

Quoted 18 lines

(c) add a global flag to indicate, that the use of clocks on this device

is optional
Pro: easy to support in drivers
Contra: as in (b) above

(d) a variant of (a), but with a helper function in V4L2 clock core to

implement such a static always-on clock
Pro: simple to support in host / bridge drivers
Contra: adds bloat to V4L2 clock helper layer, which we want to keep

small and remove eventually.

Have I missed anything? Of the above I would go with (d). I could try to code the required always-on clock helpers.

Quoted 7 lines

Quoted 2 lines

I prefer to have some solution that won't add any extra code if the clock is always on and can't be controlled.

But that's not how the common clock framework works. Sure, we could implement that right now in the v4l2-clock API, but we will need to register a fixed- clock in the em28xx driver when moving to the clock API anyway. Let's not make the transition more complex than it should be.

Or to fix the clock API to not over-design on devices where the clock can't be controlled.

Hi Mauro,

Quoted 9 lines · On Tuesday 27 August 2013 11:08:58 Mauro Carvalho Chehab wrote:

Laurent Pinchart <[email protected]> escreveu:

Quoted 2 lines · On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:

Quoted 1 line

Guennadi Liakhovetski <[email protected]> escreveu:

Ok, but the voltage and clock regulators are not mapped, on embedded devices, as part of the USB or PCI bus bridge device (except, of course, when the voltage/clocks are needed by the bridge device itself). It is mapped elsewhere, at DT.

Or in a C code board file, depending on the platform. DT or board files are more or less equivalent, both of them store information about the board. For PCI and USB devices we need to store that information somewhere as well. As the em28xx driver already stores board layout information in em28xx-cards.c, we could store clock information there as well (I haven't checked whether that's the best place to store that information in the driver). I don't see why storing board-specific clock information ("there's a fixed-frequency clock with this frequency and this name on the board") in the driver is a different issue than storing other kind of board information in the em28xx_board structure.

The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers.

Quoted 5 lines

So, the only sense on having a clock API is when the hardware allows some control on it.

So, if the hardware can't be controlled and it is always on, it makes no sense to register a clock.

Please also note that, even if the clock can't be controlled, the sensor might need to query the clock frequency for instance to adjust its PLL parameters. The clk_get_rate() call is used for such a purpose, and requires a clock object.

Quoted 2 lines

The thing is that you're wanting to use the clock register as a way to detect that the device got initialized.

I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ?

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The thing is that you're wanting to use the clock register as a way to detect that the device got initialized.

I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ?

As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The thing is that you're wanting to use the clock register as a way to detect that the device got initialized.

I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ?

As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready.

Hmm, actually there are two drivers here - the sensor driver defers its probing() when a clock provided by the bridge driver is missing. Thus let's not misunderstand it that missing clock is used as an indication of the sensor not being ready. It merely means that the clock provider (which in this case is the bridge driver) has not initialized yet. It's pretty standard situation, the sensor doesn't know who provides the clock but it knows it needs the clock and when that's missing it defers its probe().

--
Regards,
Sylwester

Sylwester Nawrocki <[email protected]> escreveu:

Quoted 10 lines · Mauro Carvalho Chehab

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The thing is that you're wanting to use the clock register as a way to detect that the device got initialized.

I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ?

As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready.

Quoted 8 lines

Hmm, actually there are two drivers here - the sensor driver defers its probing() when a clock provided by the bridge driver is missing. Thus let's not misunderstand it that missing clock is used as an indication of the sensor not being ready. It merely means that the clock provider (which in this case is the bridge driver) has not initialized yet. It's pretty standard situation, the sensor doesn't know who provides the clock but it knows it needs the clock and when that's missing it defers its probe().

On an always on clock, there's no sense on defer probe.

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 21 lines

Hi Frank,

Quoted 18 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 17 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 16 lines

Hi Mauro,

Quoted 13 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 12 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Hi Frank,

Quoted 19 lines · Frank Schäfer

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:

Quoted 16 lines

Hi Frank,

Quoted 13 lines · Frank Schäfer

Am 20.08.2013 18:34, schrieb Frank Schäfer:

Quoted 12 lines

Am 20.08.2013 15:38, schrieb Laurent Pinchart:

Quoted 11 lines

Hi Mauro,

Quoted 8 lines · On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:

Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:

Quoted 7 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 4 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying
briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Since em28xx is a USB device, I conclude, that it's supplying clock to
its components including the ov2640 sensor. So, yes, I think the driver
should export a V4L2 clock.

Could you please test this patch series
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68510
to see whether it fixes this your problem?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

...

Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere.

From an em28xx bridges point of view, the whole soc_camera module isn't

Quoted 13 lines

Quoted 13 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core
changes are needed
Contra: presumably relatively many devices will have such static

always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

(b) make clocks optional in all subdevice drivers

Quoted 4 lines

Quoted 2 lines

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant.

Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not.

That's something we wanted to avoid, as it would result in code bloat.

Em Tue, 27 Aug 2013 17:27:52 +0200
Laurent Pinchart <[email protected]> escreveu:

...

Quoted 9 lines

The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers.

On em28xx devices, power saving is done by enabling reset pin. On several hardware, doing that internally disables the clock line. I'm not sure if ov2640 supports this mode (Frank may know better how power saving is done with those cameras). Other devices have an special pin for power off or power saving.

Anyway, that rises an interesting question: on devices with wired clocks, the power saving mode should not be provided via clock API abstraction layer, but via a callback to the bridge (as the bridge knows the GPIO register/bit that corresponds to device reset and/or power off pin).

Sorry for the delayed reply.
A few remarks:

Am 27.08.2013 14:52, schrieb Laurent Pinchart:

Quoted 2 lines

...

Quoted 4 lines

Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere.

As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ?

Making a clock object mandatory isn't reasonable.

And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them.

At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around.

The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc.

From an em28xx bridges point of view, the whole soc_camera module isn't
needed and the sensor driver design appears to be "upside-down".

The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others.

Changing that will be a not-so-trivial long-term task... :/

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core
changes are needed
Contra: presumably relatively many devices will have such static

Quoted 8 lines

always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

(b) make clocks optional in all subdevice drivers

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

Quoted 3 lines

No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant.

IMHO it is relevant. And (as mentioned above) if the sensor _should_be_allowed_ to control the clock is also relevant.

Quoted 2 lines

Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not.

That's indeed what the drivers should do.
What's so unusual about it ? soc_camera already does it. ;) As you can see from the code (and also my RFC patch), these "code paths" are basically pretty simple.

Of course, the async device registration mechanism needs to be fixed/improved.

Quoted 1 line

That's something we wanted to avoid, as it would result in code bloat.

Forcing drivers to implement pseudo/fake clocks is code bloat.

Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab:

Quoted 14 lines

Em Tue, 27 Aug 2013 17:27:52 +0200
Laurent Pinchart<[email protected]> escreveu:

...

Quoted 9 lines

The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers.

Quoted 5 lines

On em28xx devices, power saving is done by enabling reset pin. On several hardware, doing that internally disables the clock line. I'm not sure if ov2640 supports this mode (Frank may know better how power saving is done with those cameras). Other devices have an special pin for power off or power saving.

The EM25xx describes a standard mapping of GPIO pins to several functionalities (LEDs, buttons, sensor power on/off, ...). But hardware manufacturers can of course build circuits differently. The VAD Laplace webcam for example doesn't use the dedicated GPIO-pin for sensor power on/off. It's sensor seems to be powered all the time.

Quoted 4 lines

Anyway, that rises an interesting question: on devices with wired clocks, the power saving mode should not be provided via clock API abstraction layer, but via a callback to the bridge (as the bridge knows the GPIO register/bit that corresponds to device reset and/or power off pin).

Well, you can add power on/off callbacks to struct soc_camera_link for this.

But - same question as above: who controls who ? ;)
IMHO, it's the em28xx bridge that controls the sensor and decides when the sensor power needs to be switched on/off.

Sorry for the delayed reply.
A few remarks:

Am 27.08.2013 14:52, schrieb Laurent Pinchart:

Quoted 2 lines

...

Quoted 4 lines

Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere.

As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ?

Making a clock object mandatory isn't reasonable.

And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them. At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around.
The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc. From an em28xx bridges point of view, the whole soc_camera module isn't needed and the sensor driver design appears to be "upside-down".

The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others. Changing that will be a not-so-trivial long-term task... :/

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

* possible fixes: several fixes have been proposed, e.g.
(a) implement a V4L2 clock in em28xx.

Pro: logically correct - a clock is indeed present, local - no core
changes are needed

Contra: presumably relatively many devices will have such static
always-on clocks. Implementing them in each of those drivers will add copied code. Besides creating a clock name from I2C bus and device numbers is ugly (a helper is needed).

(b) make clocks optional in all subdevice drivers

Pro: host / bridge drivers or core don't have to be modified
Contra: wrong in principle - those clocks are indeed compulsory

Quoted 6 lines

Quoted 2 lines

I don't think that (b) is wrong: it is not a matter or clocks being compulsory or not. It is a matter of being able to be controlled or not.

No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant.

IMHO it is relevant. And (as mentioned above) if the sensor _should_be_allowed_ to control the clock is also relevant.

Quoted 2 lines

Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not.

That's indeed what the drivers should do.
What's so unusual about it ? soc_camera already does it. ;) As you can see from the code (and also my RFC patch), these "code paths" are basically pretty simple.

Of course, the async device registration mechanism needs to be fixed/improved.

Quoted 1 line

That's something we wanted to avoid, as it would result in code bloat.

Forcing drivers to implement pseudo/fake clocks is code bloat.

Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab:

Quoted 16 lines

Em Tue, 27 Aug 2013 17:27:52 +0200
Laurent Pinchart <[email protected]> escreveu:

...

Quoted 10 lines

The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers.

Quoted 5 lines

On em28xx devices, power saving is done by enabling reset pin. On several hardware, doing that internally disables the clock line. I'm not sure if ov2640 supports this mode (Frank may know better how power saving is done with those cameras). Other devices have an special pin for power off or power saving.

The EM25xx describes a standard mapping of GPIO pins to several functionalities (LEDs, buttons, sensor power on/off, ...). But hardware manufacturers can of course build circuits differently. The VAD Laplace webcam for example doesn't use the dedicated GPIO-pin for sensor power on/off. It's sensor seems to be powered all the time.

Quoted 4 lines

Anyway, that rises an interesting question: on devices with wired clocks, the power saving mode should not be provided via clock API abstraction layer, but via a callback to the bridge (as the bridge knows the GPIO register/bit that corresponds to device reset and/or power off pin).

Well, you can add power on/off callbacks to struct soc_camera_link for this.
But - same question as above: who controls who ? ;)
IMHO, it's the em28xx bridge that controls the sensor and decides when the sensor power needs to be switched on/off.

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank Schäfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the
em28xx/ov2640/soc-camera interaction this week.
Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 26 lines

Regards,
Frank

Quoted 12 lines

Thanks
Guennadi

-----Original Message-----
From: "Frank Schäfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 43 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Quoted 2 lines

Regards,
Mauro

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

Hi Frank,

Quoted 44 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 43 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Quoted 5 lines

Quoted 2 lines

Regards,
Mauro

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Could you test, please?

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

What about Mauros an my concerns with regards to all other em28xx devices ?

And what about the em28xx v4l2-clk patches ?

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Regards,
Frank

Quoted 31 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Frank,

Quoted 51 lines · Frank Schäfer

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Firstly, I did say "IMHO," didn't I? Secondly, sure, let's have a look at them:

"I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c()."

Is this an essential comment? Is it essential where to put an operation after a function or after it?

"em28xx_set_mode() calls em28xx_gpio_set(dev, INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable subdevice power again..."

You aren't sure about that. Me neither, so, there's no evidence whatsoever. This is just a guess. And I would consider switching subdevice power in a *_set_mode() function by explicitly toggling a GPIO in presence of proper APIs... not the best design perhaps. I consider this comment non-essential too then.

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Right, my patch doesn't change this, so, this is unrelated.

Have I missed anything?

Quoted 3 lines

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

Good, thanks for testing!

Quoted 1 line

What about Mauros an my concerns with regards to all other em28xx devices ?

This is still under discussion:

http://www.mail-archive.com/[email protected]/msg66566.html

Quoted 1 line

And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Thanks
Guennadi

Quoted 7 lines

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Quoted 34 lines

Regards,
Frank

Quoted 31 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Am 10.10.2013 15:50, schrieb Guennadi Liakhovetski:

Quoted 69 lines

Hi Frank,

Quoted 51 lines · Frank Schäfer

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Firstly, I did say "IMHO," didn't I? Secondly, sure, let's have a look at them:

"I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c()."

Is this an essential comment? Is it essential where to put an operation after a function or after it?

It's a proposal.

Quoted 4 lines

"em28xx_set_mode() calls em28xx_gpio_set(dev, INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable subdevice power again..."

Quoted 5 lines

You aren't sure about that. Me neither, so, there's no evidence whatsoever. This is just a guess. And I would consider switching subdevice power in a *_set_mode() function by explicitly toggling a GPIO in presence of proper APIs... not the best design perhaps. I consider this comment non-essential too then.

It's a warning/indicator that the whole s_power thing is very dangerous because of the various GPIO-sequences used everywhere in the driver.

And it substantiates what Mauro tries to explain you.

Quoted 5 lines

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Quoted 1 line

Right, my patch doesn't change this, so, this is unrelated.

Ok, I have to admit that I wasn't clear enough in this case:
IMHO these are bugs that should be fixed, but I'm not 100% sure. In that case, there is no need to split the if-caluse containing the V4L2_BUF_TYPE_VIDEO_CAPTURE check, just remove this check while you're at it.

Quoted 1 line

Have I missed anything?

It seems we have a different understanding about the meaning of the word "(non)-essential".

I wouldn't use it in the context of review feedbacks.
I'm not in the position to force you to consider my remarks, so from your point of view they are certainly "optional" (and that's no problem for me).
Maybe that's what you mean with non-essential ? ;)

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

Good, thanks for testing!

Quoted 2 lines

Quoted 1 line

What about Mauros an my concerns with regards to all other em28xx devices ?

This is still under discussion:

How long are you going to discuss this ?
We are not talking about a new feature or improvement/extension. This is about fixing a regression, which should always have highest priority.
8 weeks IMHO should be more than enough.

Quoted 4 lines

http://www.mail-archive.com/[email protected]/msg66566.html

Quoted 1 line

And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Why ?
The 3 patches patches you've sent implement support for fixed (dummy) clocks an makes the em28xx driver using them.
Whether on/off switches should be forced to be balanced or not is AFAICS a separate issue.

Regards,
Frank

Quoted 48 lines

Thanks
Guennadi

Quoted 7 lines

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Quoted 34 lines

Regards,
Frank

Quoted 31 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Quoted 71 lines

Am 10.10.2013 15:50, schrieb Guennadi Liakhovetski:

Quoted 69 lines

Hi Frank,

Quoted 51 lines · Frank Schäfer

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Firstly, I did say "IMHO," didn't I? Secondly, sure, let's have a look at them:

"I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c()."

Is this an essential comment? Is it essential where to put an operation after a function or after it?

It should've been "after a function or inside it" of course.

Quoted 6 lines

It's a proposal.

Quoted 4 lines

"em28xx_set_mode() calls em28xx_gpio_set(dev, INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable subdevice power again..."

Quoted 7 lines

Quoted 5 lines

You aren't sure about that. Me neither, so, there's no evidence whatsoever. This is just a guess. And I would consider switching subdevice power in a *_set_mode() function by explicitly toggling a GPIO in presence of proper APIs... not the best design perhaps. I consider this comment non-essential too then.

It's a warning/indicator that the whole s_power thing is very dangerous because of the various GPIO-sequences used everywhere in the driver.

It is important, sure. But let's try to understand again what my patch is doing. It is adding 2 calls to .s_power(1) without changing anything else, or at least that's what it should be doing. is *_set_mode() was problematic, I'm not changing anything about it. In a different thread I offered to review .s_power() methods of all em28xx subdevice drivers to see, what harm turning power on could do. A first quick glance didn't reveal any dangerous locations. Nothing else matters.

Again: the only change should be adding .s_power(1) and it's only those functions that we should care about.

Quoted 1 line

And it substantiates what Mauro tries to explain you.

I think I mostly understand what Mauro is explaining to me.

Quoted 5 lines

Quoted 5 lines

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Quoted 6 lines

Quoted 1 line

Right, my patch doesn't change this, so, this is unrelated.

Ok, I have to admit that I wasn't clear enough in this case:
IMHO these are bugs that should be fixed, but I'm not 100% sure. In that case, there is no need to split the if-caluse containing the V4L2_BUF_TYPE_VIDEO_CAPTURE check, just remove this check while you're at it.

No! It shouldn't be changed "while at it." If it should be changed, it _certainly_ has to be a separate patch! And it is unrelated.

Quoted 4 lines

Quoted 2 lines

Have I missed anything?

It seems we have a different understanding about the meaning of the word "(non)-essential".

Maybe it's not the best wording in _this_ situation, sorry if so. Here's a better one:

I think, your review addresses issues like _how_ this patch should be improved. Whereas the most important question now is - _whether_ this approach should be used in any form, whether or not we should add power-on calls. _After_ this has been decided and _if_ we want to use it, then we can discuss details.

Quoted 6 lines

I wouldn't use it in the context of review feedbacks.
I'm not in the position to force you to consider my remarks, so from your point of view they are certainly "optional" (and that's no problem for me).
Maybe that's what you mean with non-essential ? ;)

No, see above.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

Good, thanks for testing!

Quoted 1 line

What about Mauros an my concerns with regards to all other em28xx devices ?

This is still under discussion:

Quoted 5 lines

How long are you going to discuss this ?
We are not talking about a new feature or improvement/extension. This is about fixing a regression, which should always have highest priority.
8 weeks IMHO should be more than enough.

Thanks. I'm doing what I can. I submitted patches and started a discussion. However I cannot force people to spend time reviewing them and replying immediately. We all have other things to do too.

Quoted 9 lines

Quoted 4 lines

http://www.mail-archive.com/[email protected]/msg66566.html

Quoted 1 line

And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Why ?
The 3 patches patches you've sent implement support for fixed (dummy) clocks an makes the em28xx driver using them.
Whether on/off switches should be forced to be balanced or not is AFAICS a separate issue.

Don't think so. The first 3 patches fix the problem, but they pollute logs with warnings, which isn't a good thing to do. However, if the maintainer decides to apply only them to win some time for the balancing discussion - I'm fine with that too.

Remember, in the end it's not me who's going to send these patches to Linus.

Thanks
Guennadi

Quoted 51 lines

Regards,
Frank

Quoted 48 lines

Thanks
Guennadi

Quoted 7 lines

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Quoted 34 lines

Regards,
Frank

Quoted 31 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Right, my patch doesn't change this, so, this is unrelated.

Ok, I have to admit that I wasn't clear enough in this case:
IMHO these are bugs that should be fixed, but I'm not 100% sure. In that case, there is no need to split the if-caluse containing the V4L2_BUF_TYPE_VIDEO_CAPTURE check, just remove this check while you're at it.

Quoted 2 lines

No! It shouldn't be changed "while at it." If it should be changed, it _certainly_ has to be a separate patch! And it is unrelated.

If you want the fix as a separate patch, then it would make sense to do this before the s_power change.
IMHO it doesn't make sense to complicate the code just to keep a bug which can be fixed easily.

Hi Frank,

Quoted 51 lines · Frank Schäfer

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Firstly, I did say "IMHO," didn't I? Secondly, sure, let's have a look at them:

"I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c()."

Is this an essential comment? Is it essential where to put an operation after a function or after it?

"em28xx_set_mode() calls em28xx_gpio_set(dev, INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable subdevice power again..."

You aren't sure about that. Me neither, so, there's no evidence whatsoever. This is just a guess. And I would consider switching subdevice power in a *_set_mode() function by explicitly toggling a GPIO in presence of proper APIs... not the best design perhaps. I consider this comment non-essential too then.

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Right, my patch doesn't change this, so, this is unrelated.

Have I missed anything?

Quoted 4 lines

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

Good, thanks for testing!

Quoted 1 line

What about Mauros an my concerns with regards to all other em28xx devices ?

This is still under discussion:

http://www.mail-archive.com/[email protected]/msg66566.html

Quoted 1 line

And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Thanks
Guennadi

Quoted 7 lines

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Quoted 35 lines

Regards,
Frank

Quoted 31 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Changing the input will likely power on the device. The design of the old suspend callback were to call it when the device is not being used. Any try to use the device makes it to wake up, as it makes no sense to use a device in standby state.

Also, changing the power states is a requirement, when switching the mode between analog, digital TV (or capture without tuner - although I think em28xx will turn the analog tuner on in this case, even not being required).

The patches that just rename the previous standby callback to s_power callback did a crap job, as it didn't consider the nuances of the API used on that time nor they didn't change the drivers to move the GPIO bits into s_power().

Looking with today's view, it would likely be better if those patches were just adding a power callback without touching the standby callback.

I suspect that the solution would be to fork s_power into two different
callbacks: one asymetric to just put the device into suspend mode (as before), and another symmetric one, where the device needs to be explicitly enabled before its usage and disabled at suspend or driver exit.

Em Thu, 10 Oct 2013 15:50:15 +0200 (CEST) Guennadi Liakhovetski <[email protected]> escreveu:

Quoted 76 lines

Hi Frank,

Quoted 51 lines · Frank SchÀfer

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:

Quoted 50 lines

Hi Frank,

Quoted 46 lines · Frank SchÀfer

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:

Quoted 45 lines

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank SchÀfer <[email protected]> escreveu:

Quoted 10 lines

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:

Quoted 2 lines

Hi Frank,
As I mentioned on the list, I'm currently on a holiday, so, replying briefly.

Sorry, I missed that (can't read all mails on the list).

Quoted 1 line

Since em28xx is a USB device, I conclude, that it's supplying clock to its components including the ov2640 sensor. So, yes, I think the driver should export a V4L2 clock.

Ok, so it's mandatory on purpose ?
I'll take a deeper into the v4l2-clk code and the em28xx/ov2640/soc-camera interaction this week. Have a nice holiday !

commit 9aea470b399d797e88be08985c489855759c6c60

Author: Guennadi Liakhovetski <[email protected]>
Date:   Fri Dec 21 13:01:55 2012 -0300

[media] soc-camera: switch I2C subdevice drivers to use v4l2-clk

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 <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(c/c the ones that acked with this broken changeset)

We need to fix it ASAP or to revert the ov2640 changes, as some em28xx cameras are currently broken on 3.10.

I'll also reject other ports to the async API if the drivers are used outside an embedded driver, as no PC driver currently defines any clock source. The same applies to regulators.

Guennadi,

Next time, please check if the i2c drivers are used outside soc_camera and apply the fixes where needed, as no regressions are allowed.

Regards,
Mauro

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

FYI: 8 weeks have passed by now and this regression has still not been fixed.
Does anybody care about it ? WONTFIX ?

You replied to my patch "em28xx: balance subdevice power-off calls" with a few non-essential IMHO comments but you didn't test it.

Non-essential comments ?
Maybe you disagree or don't care about them, but that's something different.

Firstly, I did say "IMHO," didn't I? Secondly, sure, let's have a look at them:

"I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c()."

Is this an essential comment? Is it essential where to put an operation after a function or after it?

"em28xx_set_mode() calls em28xx_gpio_set(dev, INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable subdevice power again..."

Quoted 5 lines

You aren't sure about that. Me neither, so, there's no evidence whatsoever. This is just a guess. And I would consider switching subdevice power in a *_set_mode() function by explicitly toggling a GPIO in presence of proper APIs... not the best design perhaps. I consider this comment non-essential too then.

Changing the input will likely power on the device. The design of the old suspend callback were to call it when the device is not being used. Any try to use the device makes it to wake up, as it makes no sense to use a device in standby state.

Also, changing the power states is a requirement, when switching the mode between analog, digital TV (or capture without tuner - although I think em28xx will turn the analog tuner on in this case, even not being required).

The patches that just rename the previous standby callback to s_power callback did a crap job, as it didn't consider the nuances of the API used on that time nor they didn't change the drivers to move the GPIO bits into s_power().

Looking with today's view, it would likely be better if those patches were just adding a power callback without touching the standby callback.

I suspect that the solution would be to fork s_power into two different
callbacks: one asymetric to just put the device into suspend mode (as before), and another symmetric one, where the device needs to be explicitly enabled before its usage and disabled at suspend or driver exit.

Quoted 54 lines

"Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ? Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode..."

Right, my patch doesn't change this, so, this is unrelated.

Have I missed anything?

Quoted 4 lines

Quoted 1 line

Could you test, please?

Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device.

Good, thanks for testing!

Quoted 1 line

What about Mauros an my concerns with regards to all other em28xx devices ?

This is still under discussion:

http://www.mail-archive.com/[email protected]/msg66566.html

Quoted 1 line

And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Thanks
Guennadi

Quoted 25 lines

It's pretty simple: someone (usually the maintainer ;) ) needs to decide which way to go.
Either accept and apply the existing patches or request new ones with changes.
But IMHO doing nothing for 2 months isn't the right way to handle regressions.

Regards,
Frank

Quoted 15 lines

In the meantime I'm still waiting for more comments to my "[RFD] use-counting V4L2 clocks" mail, so far only Sylwester has replied. Without all these we don't seem to progress very well.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

-----Original Message-----
From: "Frank SchÀfer" <[email protected]>
To: Guennadi Liakhovetski <[email protected]>, Linux Media Mailing List <[email protected]>
Sent: Fr., 16 Aug 2013 21:03
Subject: em28xx + ov2640 and v4l2-clk

Hi Guennadi,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

since commit 9aea470b399d797e88be08985c489855759c6c60 "soc-camera:
switch I2C subdevice drivers to use v4l2-clk", the em28xx driver fails to register the ov2640 subdevice (if needed).
The reason is that v4l2_clk_get() fails in ov2640_probe(). Does the em28xx driver have to register a (pseudo ?) clock first ?

Regards,
Frank

Well, yes, the idea is not bad, FWIW I could live with it. Doing this wouldn't be very simple though, I guess. E.g. em28xx would have to do both
- call balanced .s_power() for camera sensors etc. and call .suspend() for tuners or whatever... But please also see my other reply in this thread (to be posted shortly).

[snip]

Am 12.10.2013 05:45, schrieb Mauro Carvalho Chehab:

Quoted 10 lines

Changing the input will likely power on the device. The design of the old suspend callback were to call it when the device is not being used. Any try to use the device makes it to wake up, as it makes no sense to use a device in standby state.

Also, changing the power states is a requirement, when switching the mode between analog, digital TV (or capture without tuner - although I think em28xx will turn the analog tuner on in this case, even not being required).

Quoted 7 lines

The patches that just rename the previous standby callback to s_power callback did a crap job, as it didn't consider the nuances of the API used on that time nor they didn't change the drivers to move the GPIO bits into s_power().

Looking with today's view, it would likely be better if those patches were just adding a power callback without touching the standby callback.

The main problem is, that since commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") all subdevices are powered down, not only the tuners.
But other subdevices may not wake up automatically when needed, so this commit should also have introduced (s_power, 1) calls.
At least for em28xx it seems that this din't cause any regressions up to now, because none of the subdevs used by this driver did anything essential in its s_power callbacks (most of them don't support it at all). But it's of course a ticking bomb.
The introduction of v4l2-clk enabling/disabling in the sensor subdevs' (soc_cameras') s_power callbacks now let this bomb in em28xx explode. This time, it only caused scary warnings/traces, but it could be non-working (never waking up) devices next time.

Quoted 5 lines

I suspect that the solution would be to fork s_power into two different
callbacks: one asymetric to just put the device into suspend mode (as before), and another symmetric one, where the device needs to be explicitly enabled before its usage and disabled at suspend or driver exit.

Or, if we want to keep the API as is, we have to introduce (s_power, 1) calls in the affected drivers to avoid regressions due to future subdev changes.

Regards,
Frank