On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:
> Em Fri, 23 Aug 2013 00:15:52 +0200
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
