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. > > "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 > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ -- Cheers, Mauro -- 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
