em28xx + ov2640 and v4l2-clk

em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

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

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

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" <fschaefer.oss@googlemail.com>
To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> 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).

> 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
> Thanks
> Guennadi
>
>
> -----Original Message-----
> From: "Frank Schäfer" <fschaefer.oss@googlemail.com>
> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> 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

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Sun, 18 Aug 2013 13:40:25 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > 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).
> 
> > 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 <g.liakhovetski@gmx.de>
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 <g.liakhovetski@gmx.de>
    Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
    Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>


(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

> 
> Regards,
> Frank
> > Thanks
> > Guennadi
> >
> >
> > -----Original Message-----
> > From: "Frank Schäfer" <fschaefer.oss@googlemail.com>
> > To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> > 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Laurent Pinchart]

Hi Mauro,

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:
> > Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > 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).
> > 
> > > 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 <g.liakhovetski@gmx.de>
> 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 <g.liakhovetski@gmx.de>
>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> (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.

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.

-- 
Regards,

Laurent Pinchart

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Tue, 20 Aug 2013 15:38:57 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> 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:
> > > Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > > 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).
> > > 
> > > > 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 <g.liakhovetski@gmx.de>
> > 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 <g.liakhovetski@gmx.de>
> >     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > 
> > (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.
> 
> 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.

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

> 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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> Hi Mauro,
>
> 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:
>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>> 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).
>>>
>>>> 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 <g.liakhovetski@gmx.de>
>> 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 <g.liakhovetski@gmx.de>
>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>
>> (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.
> 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");
-   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

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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 20.08.2013 17:31, schrieb Mauro Carvalho Chehab:
> Em Tue, 20 Aug 2013 15:38:57 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>
>> Hi Mauro,
>>
>> 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:
>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>> 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).
>>>>
>>>>> 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 <g.liakhovetski@gmx.de>
>>> 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 <g.liakhovetski@gmx.de>
>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>
>>> (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.
>> 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.
>
> 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... ?

>> 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

>
> Cheers,
> Mauro

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 20.08.2013 18:34, schrieb Frank Schäfer:
> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>> Hi Mauro,
>>
>> 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:
>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>> 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).
>>>>
>>>>> 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 <g.liakhovetski@gmx.de>
>>> 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 <g.liakhovetski@gmx.de>
>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>
>>> (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.
>> 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");
> -   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().
Tested and that works.
Patch follows.

> 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

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...

Regards,
Frank

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

Re: em28xx + ov2640 and v4l2-clk

[Sylwester Nawrocki]

Hi Frank,

On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> Am 20.08.2013 18:34, schrieb Frank Schäfer:
>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>>> Hi Mauro,
>>>
>>> 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:
>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>> 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).
>>>>>
>>>>>> 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<g.liakhovetski@gmx.de>
>>>> 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<g.liakhovetski@gmx.de>
>>>>      Acked-by: Hans Verkuil<hans.verkuil@cisco.com>
>>>>      Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>>>>      Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>>>
>>>> (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.
>>> 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");
>> -   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().
>
> 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.

>> 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
>
> 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.

> 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...

Look how soc-camera registers the clock:

  snprintf(clk_name, sizeof(clk_name), "%d-%04x",
	 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);

  icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
  if (IS_ERR(icd->clk)) {
  	 ret = PTR_ERR(icd->clk);
  	 goto eclkreg;
  }

Other bridge drivers need to do something similar. All you need to know is
the I2C adapter id and the sensor's I2C slave address. For em28xx it might
be something along the lines of:

	...
	struct i2c_board_info ov2640_info = {
		.type = "ov2640",
		.flags = I2C_CLIENT_SCCB,
		.addr = dev->i2c_client[dev->def_i2c_bus].addr,
		.platform_data = &camlink,
	};
	...

	//////////////////////////////////////////////////////
	char clk_name[16];
	struct v4l2_clk *clk;

	static const struct v4l2_clk_ops em28xx_sensor_clk_ops = {
		/* empty ops */
	};


	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.

--
Regards,
Sylwester

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Hi Sylwester,

Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> Hi Frank,
>
> On 08/21/2013 10:39 PM, Frank Schäfer wrote:
>> Am 20.08.2013 18:34, schrieb Frank Schäfer:
>>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>>>> Hi Mauro,
>>>>
>>>> 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:
>>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>>> 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).
>>>>>>
>>>>>>> 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<g.liakhovetski@gmx.de>
>>>>> 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<g.liakhovetski@gmx.de>
>>>>>      Acked-by: Hans Verkuil<hans.verkuil@cisco.com>
>>>>>      Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>>>>>      Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>>>>
>>>>> (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.
>>>> 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");
>>> -   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().
>>
>> 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.
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.

>
>>> 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
>>
>> 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.
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.

>
>> 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);
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 ?

>
>     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.
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).

Regards,
Frank

>
> -- 
> Regards,
> Sylwester

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Tue, 20 Aug 2013 18:39:33 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 20.08.2013 17:31, schrieb Mauro Carvalho Chehab:
> > Em Tue, 20 Aug 2013 15:38:57 +0200
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >
> >> Hi Mauro,
> >>
> >> 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:
> >>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>>>> 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).
> >>>>
> >>>>> 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 <g.liakhovetski@gmx.de>
> >>> 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 <g.liakhovetski@gmx.de>
> >>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >>>
> >>> (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.
> >> 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.
> >
> > 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... ?

Currently, gspca doesn't use the sensors drivers (nor uvc). So, very
few places drivers are actually affected. I think only em28xx is affected
by ov2640 changes.

> 
> >> 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
> 
> >
> > Cheers,
> > Mauro
> 


-- 

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Hi Sylwester,
> 
> Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > Hi Frank,
> >
> > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> >>>> Hi Mauro,
> >>>>
> >>>> 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:
> >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>>>>>> 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).
> >>>>>>
> >>>>>>> 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<g.liakhovetski@gmx.de>
> >>>>> 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<g.liakhovetski@gmx.de>
> >>>>>      Acked-by: Hans Verkuil<hans.verkuil@cisco.com>
> >>>>>      Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> >>>>>      Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
> >>>>>
> >>>>> (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.
> >>>> 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");
> >>> -   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().
> >>
> >> 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.
> 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.

> 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.

> >
> >>> 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
> >>
> >> 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.
> 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.

> Anyway, let's focus on the main issue.
> 
> >
> >> 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);
> 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.

> >
> >     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.
> 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.

Regards,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Sylwester Nawrocki]

On 08/24/2013 09:03 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 23 Aug 2013 00:15:52 +0200
> Frank Schäfer<fschaefer.oss@googlemail.com>  escreveu:
>> Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
>>> On 08/21/2013 10:39 PM, Frank Schäfer wrote:
>>>> Am 20.08.2013 18:34, schrieb Frank Schäfer:
>>>>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>>>>>> 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:
>>>>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>>>>> 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).
>>>>>>>>
>>>>>>>>> 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<g.liakhovetski@gmx.de>
>>>>>>> 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<g.liakhovetski@gmx.de>
>>>>>>>       Acked-by: Hans Verkuil<hans.verkuil@cisco.com>
>>>>>>>       Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>>>>>>>       Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>>>>>>
>>>>>>> (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.
>>>>>> 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");
>>>>> -   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().
>>>>
>>>> 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.
>> 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.

Yeah, constructive critics is always welcome ;)

The deferred probing mechanism has been getting more common recently, as
it is difficult to model all dependencies between devices. So if some
_resource_ for a device is missing its probe() is being deferred. It then
gets retried after some other device probed, e.g. the one that provides
the missing resource. So I can't see, what exactly is a misuse here ?

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

Kind of, as I see it we just pass control of one of device's resource to
its particular driver, rather than handling it somewhere else.

Typically image sensors in embedded systems need specific sequence of
voltage supply, GPIO and clock control. And its a driver of the device
that will have coded such sequences. We don't have board files any more
on systems using Device Tree, so any hack that used to live in board
files need to be replaced with proper, i.e. more exact driver/resource
modelling.

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

Yes, I agree. But we need to make it to suck as little as possible...
Not sure if it is relevant, but there has been some works on optional
regulator support: https://lkml.org/lkml/2013/7/30/334.

Presumably we could add a platform data flag indicating not to handle
a clock in subdev drivers. For those used by bridge drivers like em28xx.
On DT systems platform_data is in most cases NULL.

>> 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.

Yes, I saw only patches for those 2 sensors adding v4l2-async support.
Likely this is only what Guennadi could test.

>>>>> 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
>>>>
>>>> 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.
>>
>> Hmm... the common clock API probably needs to be more flexible.
>> An enum would be safer to use, that's my though.

With Device Tree per device clocks can be looked up by index. And mapping
of system clocks for a consumer device is done in the device tree. For
compatibility with original clock API names (character strings) are mapped
to those indexes. Which clock is which is described in a DT binding of
a device. Still drivers reference the clocks by name.
So in case of DT it is actually a mixture of enums/strings currently.

Most of the clock provider drivers expose a table of clocks and mapping
for clock consumers is done using a handle to DT node of the clocks
supplier + (in simplest case) an index to the table.

> 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.
>
>> Anyway, let's focus on the main issue.
>>
>>>> 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);
>>
>> 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 ?

You're right, the drivers should not need to know detailed device names
as created in other subsystems (in this case i2C core).

It's easy to add a helper function to v4l2 core either returning clock
name or entirely registering a clock. However I don't want to see v4l2-clk
API spreading too much, it should rather die soon. We should use the
common clock API. x86 already enables COMMON_CLK.

Anyway, whether we use v4l2-clk or the common clk API is not something
those drivers that want use a dummy clock would need to know, it could
be handled entirely in the core.

And translation from a v4l2-clk user calls to the common clk provider
calls can be done easily in v4l2-clk.

You may ask why do we need all this complexity ? Because we want to have
common subdev drivers across all possible systems ?

> There are some docs describing DT somewhere at Documentation.
> Maybe Documentation/video4linux/v4l2-framework.txt?
>
> Perhaps it is incomplete.
>
>>>
>>>      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.
>> 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.

Yes, I agree with that.

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

I guess it wouldn't be too bad. But if we were to introduce this optionality
into many subdevs just to satisfy a few bridge drivers I guess it's 
better to
make a helper function to create a dummy clock and just use it in those few
bridge drivers. The memory waste argument seems bogus on PC systems. The 
only
problem could be (under-documented ?) bridge-dependent clock frequency
requirements. But currently there is no clock frequency setting in those
affected soc-camera subdevs, just clock gating. It's interesting, who is
supposed to set frequency then ?

--
Regards,
Sylwester

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:

> Em Fri, 23 Aug 2013 00:15:52 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> 
> > Hi Sylwester,
> > 
> > Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > > Hi Frank,
> > >
> > > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> > >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> > >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> > >>>> Hi Mauro,
> > >>>>
> > >>>> 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:
> > >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > >>>>>>> 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).
> > >>>>>>
> > >>>>>>> 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 :)

> > >>>> 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.

> > >>>> 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");
> > >>> -   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().
> > >>
> > >> 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.
> > 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.
> 
> > 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.
> 
> > >
> > >>> 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.

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

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Mon, 26 Aug 2013 15:54:16 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

> On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:
> 
> > Em Fri, 23 Aug 2013 00:15:52 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> > 
> > > Hi Sylwester,
> > > 
> > > Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > > > Hi Frank,
> > > >
> > > > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> > > >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> > > >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> > > >>>> Hi Mauro,
> > > >>>>
> > > >>>> 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:
> > > >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > >>>>>>> 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).
> > > >>>>>>
> > > >>>>>>> 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 :)
> 
> > > >>>> 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.
> 
> > > >>>> 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");
> > > >>> -   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().
> > > >>
> > > >> 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.
> > > 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.
> > 
> > > 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.
> > 
> > > >
> > > >>> 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.

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.

> * 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

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.

> 
> (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.

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

Regards,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Laurent Pinchart]

Hi Mauro,

On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> > On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:
> > > Em Fri, 23 Aug 2013 00:15:52 +0200
> > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> > > > Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > > > > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> > > > >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> > > > >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> > > > >>>> 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:
> > > > >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > > >>>>>>> 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).
> > > > >>>>>> 
> > > > >>>>>>> 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 :)
> > 
> > > > >>>> 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.
> > 
> > > > >>>> 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");
> > > > >>> 
> > > > >>> -   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().
> > > > >> 
> > > > >> 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.
> > > > 
> > > > 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.
> > > 
> > > > 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.
> > > 
> > > > >>> 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.
> 
> 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.

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.

> 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.

> > * 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
> 
> 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.

> 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.
> 
> > (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.
> 
> 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.

-- 
Regards,

Laurent Pinchart

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Tue, 27 Aug 2013 14:52:19 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> > > On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:
> > > > Em Fri, 23 Aug 2013 00:15:52 +0200
> > > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> > > > > Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > > > > > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> > > > > >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> > > > > >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> > > > > >>>> 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:
> > > > > >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > > > >>>>>>> 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).
> > > > > >>>>>> 
> > > > > >>>>>>> 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 :)
> > > 
> > > > > >>>> 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.
> > > 
> > > > > >>>> 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");
> > > > > >>> 
> > > > > >>> -   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().
> > > > > >> 
> > > > > >> 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > > 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.
> > > > 
> > > > > >>> 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.
> > 
> > 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.
> 
> 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.

> > 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.

Yes, we agree to disagree on this. The board layout is not the bridge. 
> 
> 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.

> 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.

> 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.

> 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.

> 
> > > * 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
> > 
> > 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.

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.

> > 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.
> > 
> > > (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.
> > 
> > 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.

-- 

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Laurent Pinchart]

Hi Mauro,

On Tuesday 27 August 2013 11:08:58 Mauro Carvalho Chehab wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

[snip]

> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> > > 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.
> 
> Yes, we agree to disagree on this. The board layout is not the bridge.

Strictly speaking, you're right. However, board layout is information that 
need to be conveyed to software one way or the other. Historically that 
information has been part of the bridge driver. For instance, if you look at 
the bttv driver, board layouts are stored in bttv-cards.c. That's why I 
believe that the em28xx driver should store board layout information as well.

> > 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.
> 
> > 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.
> 
> > 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.

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 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.
> 
> > > > * 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
> > > 
> > > 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.
> 
> Software can't provide clock. Only hardware can do it.

The hardware provides a clock, and the software provides a corresponding clock 
object to access clock properties.

> 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.

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.

> 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.

> 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 ?

> This is wrong on devices where the clock is hardwired.
>
> > > 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.
> > > 
> > > > (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.
> > > 
> > > 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.

-- 
Regards,

Laurent Pinchart

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Tue, 27 Aug 2013 17:27:52 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 27 August 2013 11:08:58 Mauro Carvalho Chehab wrote:
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > > On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
> > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> 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.

Yes, on PCI/USB drivers, we have a board specific setup. On em28xx, it is
at em28xx-cards.c.

Yet, there's no board-specific information in this case: em28xx doesn't
manage clocks. It is always on. No need to add a bit there at the boards
config file to initialize the clock before loading the subdevice, because
the clock is already there.

> 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).

> > 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.

Ok, this is a good point.

> > 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.

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Sylwester Nawrocki]

On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote:
>>> > > 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

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Wed, 28 Aug 2013 11:00:42 +0200
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote:
> >>> > > 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().

On an always on clock, there's no sense on defer probe.
> 
> --
> Regards,
> Sylwester


-- 

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Laurent Pinchart]

Hi Mauro,

On Wednesday 28 August 2013 06:27:52 Mauro Carvalho Chehab wrote:
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> > On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote:
> > >>> > > 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().
> 
> On an always on clock, there's no sense on defer probe.

The point is that the sensor driver doesn't know whether the clock is always 
on or not, so it must defer the probe if the clock object isn't available 
(remember that even for always-on clocks the sensor driver often needs to 
query the clock rate). That won't happen in this case as the sensor device is 
instanciated by the em28xx driver, so the clock object will always be 
available.

-- 
Regards,

Laurent Pinchart

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

Hi Frank,

On Fri, 23 Aug 2013, Frank Schäfer wrote:

> Hi Sylwester,
> 
> Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
> > Hi Frank,
> >
> > On 08/21/2013 10:39 PM, Frank Schäfer wrote:
> >> Am 20.08.2013 18:34, schrieb Frank Schäfer:
> >>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
> >>>> Hi Mauro,
> >>>>
> >>>> 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:
> >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>>>>>> 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).
> >>>>>>
> >>>>>>> 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/

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Hi Guennadi,

Am 30.08.2013 12:30, schrieb Guennadi Liakhovetski:
> Hi Frank,
>
> On Fri, 23 Aug 2013, Frank Schäfer wrote:
>
>> Hi Sylwester,
>>
>> Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
>>> Hi Frank,
>>>
>>> On 08/21/2013 10:39 PM, Frank Schäfer wrote:
>>>> Am 20.08.2013 18:34, schrieb Frank Schäfer:
>>>>> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>>>>>> Hi Mauro,
>>>>>>
>>>>>> 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:
>>>>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>>>>> 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).
>>>>>>>>
>>>>>>>>> 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 for the patches.
Unfortunately, I'm traveling this week and can't test them before
Monday, sorry. :(
But I've put I on my ToDo list. Please be patient.

Regards,
Frank

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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Sorry for the delayed reply.
A few remarks:


Am 27.08.2013 14:52, schrieb Laurent Pinchart:
> ...
>
> 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... :/

>>> * 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
>> 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.

> 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.

> 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:
> Em Tue, 27 Aug 2013 17:27:52 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>
> ...
>> 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.

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.

> 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.

Regards,
Frank

Re: em28xx + ov2640 and v4l2-clk

[Sylwester Nawrocki]

On 09/02/2013 08:30 PM, Frank Schäfer wrote:
> Sorry for the delayed reply.
> A few remarks:
>
> Am 27.08.2013 14:52, schrieb Laurent Pinchart:
>> ...
>>
>> 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 ?

Whatever module that defines configuration of the whole system. In case of
embedded systems it were the board files. Now it can be defined in DT as
a fixed rate clock and is handled by generic code, without a need for any
custom driver.

> Making a clock object mandatory isn't reasonable.

That's a too far generalization. :-)

> 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.

We need to have it modelled like this to support initialization from the
device tree. It has been agreed on the mailing list and on face-to-face
meetings. Let's not go in circles with that.

With device tree the order of initialization of drivers (e.g. I2C client
subdev and the host interface) cannot be determined in advance.

The I2C client subdev is normally a child of I2C bus controller device,
not the video host interface device. So it may happen that either I2C
client or the host initializes first. It cannot be determined in advance.

All resources that are provided to the sensor so it can complete its
initialization (e.g. I2C communication can be performed) need to be
explicitly modelled and the sensor driver needs to be aware when any
of them is missing.

Also a well written sensor driver needs to know exact frequency of the
master clock, so it can, e.g. configure any PLLs inside the sensor in
order to achieve requested frame rates, exposure times, etc.

I hear your argument that for some systems it's the bridge driver that
has some specific requirements for the clock frequency. But it seems
strange, in that particular case of the em28xx, that lower master clock
frequency is needed for higher resolution. Usually it's the opposite.
I'm curious how that strange requirement has been figured out in this
particular case ? I guess it's somehow connected with the pixel clock
signal ?

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

On embedded SoC systems it also often the host that has registers to
control the clock.

> 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.

Not necessarily, power on/off sequences are supposed to be handled by
subdev drivers. Otherwise each bridge driver would need to code power/
clock sequences for each subdev. Do you think that's a good idea ?
N sensors * M bridge devices.

>  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".

Perhaps there are some requirements missed. But we could also say that,
for embedded systems where we in majority of cases know what wiring
and device control registers exactly look like, thus it's clear what
resources and device properties should be associated with what driver,
the em28xx like modules are not needed. Since they were written with
simplified, e.g. due to insufficient access to documentation, view of
the hardware and are difficult to work with.

> 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.

Could be, note that there are many non soc-camera sensor drivers that
behave similarly. And now it is still difficult (not supported) to use
soc-camera sensor with non a soc-camera bridge. But related works are
ongoing AFAIK and it will likely get addressed in not so distant future.

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

IIRC soc-camera already allowed the sensors to control the clocks in the
past, but then it was decided to move away from that. Not sure what were
the reasons, perhaps Guennadi knows the details. Now due to the DT boot
requirements it has been changed again.

>>>> * 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
>>> 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.

If there is really nothing that could be done about those handful of the
old bridge drivers we could probably use a platform data flag at the sensor
subdevs they interwork with, signalling the clock usage is optional. With
device tree platform data is usually NULL so such flag wouldn't be set
automatically.

But that sounds like a wrong approach, given that it would make things
easier for...one? bridge driver and cause more trouble for tens of subdev
drivers that are used on (ARM) embedded systems, that will in near future
support in practice only initialization/booting from the device tree.

>> 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.

Could become slightly more complex once the drivers start using the clock
object to get/configure the clock's frequency.

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

So what's you proposed fixes/improvements ? ;-)

>> 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:
>> Em Tue, 27 Aug 2013 17:27:52 +0200
>> Laurent Pinchart<laurent.pinchart@ideasonboard.com>  escreveu:
>>
>> ...
>>> 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.
>
> 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.

It may be just a matter of passing a GPIO number to a specific driver.
If the GPIO is not used on a specific system we just pass EINVAL and its
handled gracefully in the GPIO API. No changes in a sensor driver needed.

>> 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.

If a sensor has a STANDBY pin natural API to control any device attached
to such a pin is the GPIO API. By introducing various callbacks we are
just tying various elements using v4l2-specific APIs, which makes the
whole thing difficult to change partially.

> 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.

Yes, but that doesn't mean the bridge drivers need to handle power sequence
details for each specific sensor they need to interwork with.

--
Regards,
Sylwester

Re: em28xx + ov2640 and v4l2-clk

[Laurent Pinchart]

Hi Frank,

On Monday 02 September 2013 20:30:42 Frank Schäfer wrote:
> Sorry for the delayed reply.
> A few remarks:
> 
> Am 27.08.2013 14:52, schrieb Laurent Pinchart:
> > ...
> > 
> > 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 ?

Whatever code provides other board information. In this case board information 
is provided by em28xx driver (in em28xx-cards.c), so the clock object should 
be provided by that driver as well.

> Making a clock object mandatory isn't reasonable.

The point could be debated further, but you will need to bring that to LKML 
and convince the core CCF (common clock framework) developers.

> 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... :/

Sensor drivers are modeled around the assumption that the host needs to 
control the sensor. If the bridge performs half of the job and relies on the 
host performing the other half, no API generalization is possible. If the half 
that is incumbant upon us matches the current sensor API, great. Otherwise we 
might need custom sensor drivers. There's not much that can be done in terms 
of software APIs generalization when the device defines the boundary between 
software and hardware on a per-device basis.

> >>> * 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
> >> 
> >> 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.
> 
> > 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.

They might be simple, but they need to be duplicated all over the place, 
resulting in code bloat and bugs.

> Of course, the async device registration mechanism needs to be
> fixed/improved.
> 
> > That's something we wanted to avoid, as it would result in code bloat.
> 
> Forcing drivers to implement pseudo/fake clocks is code bloat.

There's no fake clock. There's a real hardware clock, which we model as a 
software clock object.

> Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab:
> > Em Tue, 27 Aug 2013 17:27:52 +0200
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > 
> > ...
> > 
> >> 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.
> 
> 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.
> 
> > 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.

-- 
Regards,

Laurent Pinchart

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> Em Sun, 18 Aug 2013 13:40:25 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>> 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).
>>
>>> 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 <g.liakhovetski@gmx.de>
> 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 <g.liakhovetski@gmx.de>
>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
>
> (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

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

Regards,
Frank

>> Regards,
>> Frank
>>> Thanks
>>> Guennadi
>>>
>>>
>>> -----Original Message-----
>>> From: "Frank Schäfer" <fschaefer.oss@googlemail.com>
>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
>>> 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
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

Hi Frank,

On Tue, 8 Oct 2013, Frank SchÀfer wrote:

> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> > Em Sun, 18 Aug 2013 13:40:25 +0200
> > Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>> 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).
> >>
> >>> 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 <g.liakhovetski@gmx.de>
> > 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 <g.liakhovetski@gmx.de>
> >     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >
> >
> > (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
> 
> 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

> >>> -----Original Message-----
> >>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
> >>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> >>> 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

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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
> Hi Frank,
>
> On Tue, 8 Oct 2013, Frank SchÀfer wrote:
>
>> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 18 Aug 2013 13:40:25 +0200
>>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>> 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).
>>>>
>>>>> 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 <g.liakhovetski@gmx.de>
>>> 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 <g.liakhovetski@gmx.de>
>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>
>>>
>>> (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
>> 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.

> 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

> 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
>
>>>>> -----Original Message-----
>>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
>>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
>>>>> 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
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

Hi Frank,

On Thu, 10 Oct 2013, Frank Schäfer wrote:

> Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
> > Hi Frank,
> >
> > On Tue, 8 Oct 2013, Frank SchÀfer wrote:
> >
> >> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> >>> Em Sun, 18 Aug 2013 13:40:25 +0200
> >>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>>>> 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).
> >>>>
> >>>>> 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 <g.liakhovetski@gmx.de>
> >>> 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 <g.liakhovetski@gmx.de>
> >>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >>>
> >>>
> >>> (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
> >> 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?

> > 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!

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

This is still under discussion:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html

> And what about the em28xx v4l2-clk patches ?

Their acceptance is related to the above.

Thanks
Guennadi

> 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
> 
> > 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
> >
> >>>>> -----Original Message-----
> >>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
> >>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> >>>>> 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
> > ---
> > 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 majordomo@vger.kernel.org
> > 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/

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 10.10.2013 15:50, schrieb Guennadi Liakhovetski:
> Hi Frank,
>
> On Thu, 10 Oct 2013, Frank Schäfer wrote:
>
>> Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
>>> Hi Frank,
>>>
>>> On Tue, 8 Oct 2013, Frank SchÀfer wrote:
>>>
>>>> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
>>>>> Em Sun, 18 Aug 2013 13:40:25 +0200
>>>>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>>> 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).
>>>>>>
>>>>>>> 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 <g.liakhovetski@gmx.de>
>>>>> 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 <g.liakhovetski@gmx.de>
>>>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>>>
>>>>>
>>>>> (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
>>>> 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.

> "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.
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.

> "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.

> 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 ? ;)

>>> 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!
>
>> 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.

> http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html
>
>> 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

> Thanks
> Guennadi
>
>> 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
>>
>>> 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
>>>
>>>>>>> -----Original Message-----
>>>>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
>>>>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
>>>>>>> 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
>>> ---
>>> 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 majordomo@vger.kernel.org
>>> 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/

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

On Thu, 10 Oct 2013, Frank Schäfer wrote:

> Am 10.10.2013 15:50, schrieb Guennadi Liakhovetski:
> > Hi Frank,
> >
> > On Thu, 10 Oct 2013, Frank Schäfer wrote:
> >
> >> Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
> >>> Hi Frank,
> >>>
> >>> On Tue, 8 Oct 2013, Frank SchÀfer wrote:
> >>>
> >>>> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> >>>>> Em Sun, 18 Aug 2013 13:40:25 +0200
> >>>>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
> >>>>>
> >>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> >>>>>>> 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).
> >>>>>>
> >>>>>>> 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 <g.liakhovetski@gmx.de>
> >>>>> 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 <g.liakhovetski@gmx.de>
> >>>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >>>>>
> >>>>>
> >>>>> (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
> >>>> 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.

> It's a proposal.
> 
> > "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.
> 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.

> And it substantiates what Mauro tries to explain you.

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

> > "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.

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.

> > 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.

> 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.

> >>> 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!
> >
> >> 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.

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.

> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html
> >
> >> 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

> 
> Regards,
> Frank
> 
> > Thanks
> > Guennadi
> >
> >> 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
> >>
> >>> 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
> >>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
> >>>>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> >>>>>>> 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
> >>> ---
> >>> 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 majordomo@vger.kernel.org
> >>> 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/

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 10.10.2013 19:50, schrieb Guennadi Liakhovetski:
> On Thu, 10 Oct 2013, Frank Schäfer wrote:
>
>> Am 10.10.2013 15:50, schrieb Guennadi Liakhovetski:
>>> Hi Frank,
>>>
>>> On Thu, 10 Oct 2013, Frank Schäfer wrote:
>>>
>>>> Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
>>>>> Hi Frank,
>>>>>
>>>>> On Tue, 8 Oct 2013, Frank SchÀfer wrote:
>>>>>
>>>>>> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
>>>>>>> Em Sun, 18 Aug 2013 13:40:25 +0200
>>>>>>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>>>
>>>>>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>>>>>> 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).
>>>>>>>>
>>>>>>>>> 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 <g.liakhovetski@gmx.de>
>>>>>>> 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 <g.liakhovetski@gmx.de>
>>>>>>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>>>>>
>>>>>>>
>>>>>>> (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
>>>>>> 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.
>
>> It's a proposal.
>>
>>> "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.
>> 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.
>
>> And it substantiates what Mauro tries to explain you.
> I think I mostly understand what Mauro is explaining to me.
>
>>> "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.
> 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.

>
>>> 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.
I'm fine with this approach, but please, come to a decision soon.

>
>> 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.
>
>>>>> 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!
>>>
>>>> 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.
> 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.
True. It would be nice to get further feedback and clear decisions from
the maintainer.

Anyway, if a regression can't be fixed in a reasonable time (at least
provisional), the commit that introduced this regression should be reverted.


>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html
>>>
>>>> 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.
Well, these patches at least make the device usable again.
A working device which pollutes the log is IMHO much better than a
device which isn't working at all.
AFAICS, s_power balancing also isn't related to the code sections
modified by these patches.

Regards,
Frank

>
> Remember, in the end it's not me who's going to send these patches to 
> Linus.
>
> Thanks
> Guennadi
>
>> Regards,
>> Frank
>>
>>> Thanks
>>> Guennadi
>>>
>>>> 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
>>>>
>>>>> 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
>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
>>>>>>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
>>>>>>>>> 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
>>>>> ---
>>>>> 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 majordomo@vger.kernel.org
>>>>> 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/

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 10.10.2013 20:38, schrieb Frank Schäfer:

[...]
>>>> "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.
>> 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.
Looking into the code again, I think there are even more things which
need to be fixed. :(
Will try to send a patch tomorrow.

Regards,
Frank

Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab]

Em Thu, 10 Oct 2013 15:50:15 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

> Hi Frank,
> 
> On Thu, 10 Oct 2013, Frank Schäfer wrote:
> 
> > Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
> > > Hi Frank,
> > >
> > > On Tue, 8 Oct 2013, Frank SchÀfer wrote:
> > >
> > >> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> > >>> Em Sun, 18 Aug 2013 13:40:25 +0200
> > >>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
> > >>>
> > >>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > >>>>> 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).
> > >>>>
> > >>>>> 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 <g.liakhovetski@gmx.de>
> > >>> 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 <g.liakhovetski@gmx.de>
> > >>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > >>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > >>>
> > >>>
> > >>> (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
> > >> 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.

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.

> 
> "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?
> 
> > > 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!
> 
> > What about Mauros an my concerns with regards to all other em28xx devices ?
> 
> This is still under discussion:
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html
> 
> > And what about the em28xx v4l2-clk patches ?
> 
> Their acceptance is related to the above.
> 
> Thanks
> Guennadi
> 
> > 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
> > 
> > > 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
> > >
> > >>>>> -----Original Message-----
> > >>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
> > >>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> > >>>>> 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
> > > ---
> > > 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 majordomo@vger.kernel.org
> > > 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/


-- 

Cheers,
Mauro

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

[snip]

Am 12.10.2013 05:45, schrieb Mauro Carvalho Chehab:
>
> 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.

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.

> 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

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

Hi Mauro,

On Sat, 12 Oct 2013, Mauro Carvalho Chehab wrote:

> Em Thu, 10 Oct 2013 15:50:15 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> 
> > Hi Frank,
> > 
> > On Thu, 10 Oct 2013, Frank SchÀfer wrote:
> > 
> > > Am 08.10.2013 18:38, schrieb Guennadi Liakhovetski:
> > > > Hi Frank,
> > > >
> > > > On Tue, 8 Oct 2013, Frank SchÀfer wrote:
> > > >
> > > >> Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab:
> > > >>> Em Sun, 18 Aug 2013 13:40:25 +0200
> > > >>> Frank SchÀfer <fschaefer.oss@googlemail.com> escreveu:
> > > >>>
> > > >>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
> > > >>>>> 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).
> > > >>>>
> > > >>>>> 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 <g.liakhovetski@gmx.de>
> > > >>> 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 <g.liakhovetski@gmx.de>
> > > >>>     Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > >>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > > >>>
> > > >>>
> > > >>> (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
> > > >> 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.
> 
> 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.

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).

Thanks
Guennadi

> > "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?
> > 
> > > > 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!
> > 
> > > What about Mauros an my concerns with regards to all other em28xx devices ?
> > 
> > This is still under discussion:
> > 
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg66566.html
> > 
> > > And what about the em28xx v4l2-clk patches ?
> > 
> > Their acceptance is related to the above.
> > 
> > Thanks
> > Guennadi
> > 
> > > 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
> > > 
> > > > 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
> > > >
> > > >>>>> -----Original Message-----
> > > >>>>> From: "Frank SchÀfer" <fschaefer.oss@googlemail.com>
> > > >>>>> To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Linux Media Mailing List <linux-media@vger.kernel.org>
> > > >>>>> 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

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

Re: em28xx + ov2640 and v4l2-clk

[Guennadi Liakhovetski]

On Tue, 15 Oct 2013, Guennadi Liakhovetski wrote:

> 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).

Sorry, I posted it in a different thread:

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/69003

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

Re: em28xx + ov2640 and v4l2-clk

[Frank Schäfer]

Am 13.10.2013 16:00, schrieb Frank Schäfer:
> [snip]
>
> Am 12.10.2013 05:45, schrieb Mauro Carvalho Chehab:
>> 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.
> 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.
>
>> 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

Ok, I've spent some time digging deeper into the code, checking em28xx,
the subdev drivers (msp3400, saa7115_auto, tvp5150, tvaudio, tuner,
mt9v011, ov2640) an and all the places where we're applying GPIO-sequences.

The em28xx driver is already at least partially aware of the problem,
that GPIO-sequences might change the power states of the subdevices or
reset them.
That's why em28xx_wake_i2c() is called in several places after GPIO
sequences have been applied (see code comments).
As the name already implies, these are places where we want to make sure
that the subdevices are in power-on state.
So adding a (s_power, 1) call at the beginning of this function would be
a good starting point. AFAICS, this can't break things.
This would also make sure that (s_power, 1) is called before the first
(s_power, 0) call and silence the warning about unbalanced
v4l2_clk_disable() calls.

However, I doubt we'll ever get the s_power calls really balanced.
Due to the GPIO-problems, there will likely always be more power-on
calls than power-off calls and hence more v4l2_clk_enable() than
v4l2_clk_disable() calls.

Regards,
Frank