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@xxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxx> > > > >>> 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@xxxxxx> > > > >>> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > >>> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> > > > >>> > > > >>> > > > >>> (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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxx> > > > >>>>> To: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>, Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx> > > > >>>>> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
