Re: em28xx + ov2640 and v4l2-clk

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

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