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@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? > 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@xxxxxxxxxxxxxxx/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@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/ > --- > 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
