Re: em28xx + ov2640 and v4l2-clk
Am 13.10.2013 16:00, schrieb Frank Schäfer: [snip] Am 12.10.2013 05:45, schrieb Mauro Carvalho Chehab: 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. The main problem is, that since commit 622b828ab7 (v4l2_subdev: rename tuner s_standby operation to core s_power) all subdevices are powered down, not only the tuners. But other subdevices may not wake up automatically when needed, so this commit should also have introduced (s_power, 1) calls. At least for em28xx it seems that this din't cause any regressions up to now, because none of the subdevs used by this driver did anything essential in its s_power callbacks (most of them don't support it at all). But it's of course a ticking bomb. The introduction of v4l2-clk enabling/disabling in the sensor subdevs' (soc_cameras') s_power callbacks now let this bomb in em28xx explode. This time, it only caused scary warnings/traces, but it could be non-working (never waking up) devices next time. 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. Or, if we want to keep the API as is, we have to introduce (s_power, 1) calls in the affected drivers to avoid regressions due to future subdev changes. Regards, Frank Ok, I've spent some time digging deeper into the code, checking em28xx, the subdev drivers (msp3400, saa7115_auto, tvp5150, tvaudio, tuner, mt9v011, ov2640) an and all the places where we're applying GPIO-sequences. The em28xx driver is already at least partially aware of the problem, that GPIO-sequences might change the power states of the subdevices or reset them. That's why em28xx_wake_i2c() is called in several places after GPIO sequences have been applied (see code comments). As the name already implies, these are places where we want to make sure that the subdevices are in power-on state. So adding a (s_power, 1) call at the beginning of this function would be a good starting point. AFAICS, this can't break things. This would also make sure that (s_power, 1) is called before the first (s_power, 0) call and silence the warning about unbalanced v4l2_clk_disable() calls. However, I doubt we'll ever get the s_power calls really balanced. Due to the GPIO-problems, there will likely always be more power-on calls than power-off calls and hence more v4l2_clk_enable() than v4l2_clk_disable() calls. Regards, Frank -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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@vger.kernel.org/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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
On Tue, 15 Oct 2013, Guennadi Liakhovetski wrote: 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). Sorry, I posted it in a different thread: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/69003 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
[snip] Am 12.10.2013 05:45, schrieb Mauro Carvalho Chehab: 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. The main problem is, that since commit 622b828ab7 (v4l2_subdev: rename tuner s_standby operation to core s_power) all subdevices are powered down, not only the tuners. But other subdevices may not wake up automatically when needed, so this commit should also have introduced (s_power, 1) calls. At least for em28xx it seems that this din't cause any regressions up to now, because none of the subdevs used by this driver did anything essential in its s_power callbacks (most of them don't support it at all). But it's of course a ticking bomb. The introduction of v4l2-clk enabling/disabling in the sensor subdevs' (soc_cameras') s_power callbacks now let this bomb in em28xx explode. This time, it only caused scary warnings/traces, but it could be non-working (never waking up) devices next time. 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. Or, if we want to keep the API as is, we have to introduce (s_power, 1) calls in the affected drivers to avoid regressions due to future subdev changes. Regards, Frank -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
an my concerns with regards to all other em28xx devices ? This is still under discussion: http://www.mail-archive.com/linux-media@vger.kernel.org/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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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@googlemail.com 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.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. Could you test, please? Yes, this patch will make the warnings disappear and works at least for my em28xx+ov2640 device. What about Mauros an my concerns with regards to all other em28xx devices ? And what about the em28xx v4l2-clk patches ? 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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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@googlemail.com 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.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. 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@vger.kernel.org/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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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
Re: em28xx + ov2640 and v4l2-clk
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. 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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org 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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
? 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 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. http://www.mail-archive.com/linux-media@vger.kernel.org/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. 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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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@vger.kernel.org/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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Am 10.10.2013 20:38, schrieb Frank Schäfer: [...] 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. Looking into the code again, I think there are even more things which need to be fixed. :( Will try to send a patch tomorrow. Regards, Frank -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Am 18.08.2013 17:20, schrieb Mauro Carvalho Chehab: Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer fschaefer@googlemail.com 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.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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 ? Regards, Frank Regards, Frank Thanks Guennadi -Original Message- From: Frank Schäfer fschaefer@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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@googlemail.com 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.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. Could you test, please? 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@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Sorry for the delayed reply. A few remarks: Am 27.08.2013 14:52, schrieb Laurent Pinchart: ... Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere. As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ? Making a clock object mandatory isn't reasonable. And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them. At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around. The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc. From an em28xx bridges point of view, the whole soc_camera module isn't needed and the sensor driver design appears to be upside-down. The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others. Changing that will be a not-so-trivial long-term task... :/ * 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. No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant. IMHO it is relevant. And (as mentioned above) if the sensor _should_be_allowed_ to control the clock is also relevant. Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not. That's indeed what the drivers should do. What's so unusual about it ? soc_camera already does it. ;) As you can see from the code (and also my RFC patch), these code paths are basically pretty simple. Of course, the async device registration mechanism needs to be fixed/improved. That's something we wanted to avoid, as it would result in code bloat. Forcing drivers to implement pseudo/fake clocks is code bloat. Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab: Em Tue, 27 Aug 2013 17:27:52 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: ... The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers. On em28xx devices, power saving is done by enabling reset pin. On several hardware, doing that internally disables the clock line. I'm not sure if ov2640 supports this mode (Frank may know better how power saving is done with those cameras). Other devices have an special pin for power off or power saving. The EM25xx describes a standard mapping of GPIO pins to several functionalities (LEDs, buttons, sensor power on/off, ...). But hardware manufacturers can of course build circuits differently. The VAD Laplace webcam for example doesn't use the dedicated GPIO-pin for sensor power on/off. It's sensor seems to be powered all the time. Anyway, that rises an interesting question: on devices with wired clocks, the power saving mode should not be provided via clock API abstraction layer, but via a callback to the bridge (as the bridge knows the GPIO register/bit that corresponds to device reset and/or power off pin). Well, you can add power on/off callbacks to struct soc_camera_link for this. But - same question as above: who controls who ? ;) IMHO, it's the em28xx bridge that controls the sensor and decides when
Re: em28xx + ov2640 and v4l2-clk
On 09/02/2013 08:30 PM, Frank Schäfer wrote: Sorry for the delayed reply. A few remarks: Am 27.08.2013 14:52, schrieb Laurent Pinchart: ... Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere. As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ? Whatever module that defines configuration of the whole system. In case of embedded systems it were the board files. Now it can be defined in DT as a fixed rate clock and is handled by generic code, without a need for any custom driver. Making a clock object mandatory isn't reasonable. That's a too far generalization. :-) And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them. We need to have it modelled like this to support initialization from the device tree. It has been agreed on the mailing list and on face-to-face meetings. Let's not go in circles with that. With device tree the order of initialization of drivers (e.g. I2C client subdev and the host interface) cannot be determined in advance. The I2C client subdev is normally a child of I2C bus controller device, not the video host interface device. So it may happen that either I2C client or the host initializes first. It cannot be determined in advance. All resources that are provided to the sensor so it can complete its initialization (e.g. I2C communication can be performed) need to be explicitly modelled and the sensor driver needs to be aware when any of them is missing. Also a well written sensor driver needs to know exact frequency of the master clock, so it can, e.g. configure any PLLs inside the sensor in order to achieve requested frame rates, exposure times, etc. I hear your argument that for some systems it's the bridge driver that has some specific requirements for the clock frequency. But it seems strange, in that particular case of the em28xx, that lower master clock frequency is needed for higher resolution. Usually it's the opposite. I'm curious how that strange requirement has been figured out in this particular case ? I guess it's somehow connected with the pixel clock signal ? At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around. On embedded SoC systems it also often the host that has registers to control the clock. The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc. Not necessarily, power on/off sequences are supposed to be handled by subdev drivers. Otherwise each bridge driver would need to code power/ clock sequences for each subdev. Do you think that's a good idea ? N sensors * M bridge devices. From an em28xx bridges point of view, the whole soc_camera module isn't needed and the sensor driver design appears to be upside-down. Perhaps there are some requirements missed. But we could also say that, for embedded systems where we in majority of cases know what wiring and device control registers exactly look like, thus it's clear what resources and device properties should be associated with what driver, the em28xx like modules are not needed. Since they were written with simplified, e.g. due to insufficient access to documentation, view of the hardware and are difficult to work with. The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others. Could be, note that there are many non soc-camera sensor drivers that behave similarly. And now it is still difficult (not supported) to use soc-camera sensor with non a soc-camera bridge. But related works are ongoing AFAIK and it will likely get addressed in not so distant future. Changing that will be a not-so-trivial long-term task... :/ IIRC soc-camera already allowed the sensors to control the clocks in the past, but then it was decided to move away from that. Not sure what were the reasons, perhaps Guennadi knows the details. Now due to the DT boot requirements it has been changed again. * 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:
Re: em28xx + ov2640 and v4l2-clk
Hi Frank, On Monday 02 September 2013 20:30:42 Frank Schäfer wrote: Sorry for the delayed reply. A few remarks: Am 27.08.2013 14:52, schrieb Laurent Pinchart: ... Even if the bridge doesn't control the clock, it provides a clock to the sensor. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere. As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ? Whatever code provides other board information. In this case board information is provided by em28xx driver (in em28xx-cards.c), so the clock object should be provided by that driver as well. Making a clock object mandatory isn't reasonable. The point could be debated further, but you will need to bring that to LKML and convince the core CCF (common clock framework) developers. And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them. At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around. The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc. From an em28xx bridges point of view, the whole soc_camera module isn't needed and the sensor driver design appears to be upside-down. The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others. Changing that will be a not-so-trivial long-term task... :/ Sensor drivers are modeled around the assumption that the host needs to control the sensor. If the bridge performs half of the job and relies on the host performing the other half, no API generalization is possible. If the half that is incumbant upon us matches the current sensor API, great. Otherwise we might need custom sensor drivers. There's not much that can be done in terms of software APIs generalization when the device defines the boundary between software and hardware on a per-device basis. * 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. No, it's a matter of providing a clock to a chip that needs one. If the chip needs a clock, it must get one. Whether the clock can be controlled or not is not relevant. IMHO it is relevant. And (as mentioned above) if the sensor _should_be_allowed_ to control the clock is also relevant. Otherwise all clock users would need to implement several code paths depending on whether the clock is controllable or not. That's indeed what the drivers should do. What's so unusual about it ? soc_camera already does it. ;) As you can see from the code (and also my RFC patch), these code paths are basically pretty simple. They might be simple, but they need to be duplicated all over the place, resulting in code bloat and bugs. Of course, the async device registration mechanism needs to be fixed/improved. That's something we wanted to avoid, as it would result in code bloat. Forcing drivers to implement pseudo/fake clocks is code bloat. There's no fake clock. There's a real hardware clock, which we model as a software clock object. Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab: Em Tue, 27 Aug 2013 17:27:52 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: ... The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any
Re: em28xx + ov2640 and v4l2-clk
Hi Frank, On Fri, 23 Aug 2013, Frank Schäfer wrote: 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. Could you please test this patch series http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68510 to see whether it fixes this your problem? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Hi Guennadi, Am 30.08.2013 12:30, schrieb Guennadi Liakhovetski: Hi Frank, On Fri, 23 Aug 2013, Frank Schäfer wrote: 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. Could you please test this patch series http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68510 to see whether it fixes this your problem? Thanks for the patches. Unfortunately, I'm traveling this week and can't test them before Monday, sorry. :( But I've put I on my ToDo list. Please be patient. Regards, Frank 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote: The thing is that you're wanting to use the clock register as a way to detect that the device got initialized. I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ? As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready. Hmm, actually there are two drivers here - the sensor driver defers its probing() when a clock provided by the bridge driver is missing. Thus let's not misunderstand it that missing clock is used as an indication of the sensor not being ready. It merely means that the clock provider (which in this case is the bridge driver) has not initialized yet. It's pretty standard situation, the sensor doesn't know who provides the clock but it knows it needs the clock and when that's missing it defers its probe(). -- Regards, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Em Wed, 28 Aug 2013 11:00:42 +0200 Sylwester Nawrocki s.nawro...@samsung.com escreveu: On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote: The thing is that you're wanting to use the clock register as a way to detect that the device got initialized. I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ? As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready. Hmm, actually there are two drivers here - the sensor driver defers its probing() when a clock provided by the bridge driver is missing. Thus let's not misunderstand it that missing clock is used as an indication of the sensor not being ready. It merely means that the clock provider (which in this case is the bridge driver) has not initialized yet. It's pretty standard situation, the sensor doesn't know who provides the clock but it knows it needs the clock and when that's missing it defers its probe(). On an always on clock, there's no sense on defer probe. -- Regards, Sylwester -- Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Hi Mauro, On Wednesday 28 August 2013 06:27:52 Mauro Carvalho Chehab wrote: Sylwester Nawrocki s.nawro...@samsung.com escreveu: On 08/27/2013 06:00 PM, Mauro Carvalho Chehab wrote: The thing is that you're wanting to use the clock register as a way to detect that the device got initialized. I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ? As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready. Hmm, actually there are two drivers here - the sensor driver defers its probing() when a clock provided by the bridge driver is missing. Thus let's not misunderstand it that missing clock is used as an indication of the sensor not being ready. It merely means that the clock provider (which in this case is the bridge driver) has not initialized yet. It's pretty standard situation, the sensor doesn't know who provides the clock but it knows it needs the clock and when that's missing it defers its probe(). On an always on clock, there's no sense on defer probe. The point is that the sensor driver doesn't know whether the clock is always on or not, so it must defer the probe if the clock object isn't available (remember that even for always-on clocks the sensor driver often needs to query the clock rate). That won't happen in this case as the sensor device is instanciated by the em28xx driver, so the clock object will always be available. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Hi Mauro,
On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
Guennadi Liakhovetski g.liakhovet...@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@googlemail.com escreveu:
Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
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:
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
Re: em28xx + ov2640 and v4l2-clk
Em Tue, 27 Aug 2013 14:52:19 +0200
Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu:
Hi Mauro,
On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote:
Guennadi Liakhovetski g.liakhovet...@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@googlemail.com escreveu:
Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
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:
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
Re: em28xx + ov2640 and v4l2-clk
Hi Mauro, On Tuesday 27 August 2013 11:08:58 Mauro Carvalho Chehab wrote: Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote: Guennadi Liakhovetski g.liakhovet...@gmx.de escreveu: [snip] 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. Possibly. Or the em28xx controls the clock transparently. We will probably never know, and it doesn't matter much at the end of the day. We know that the clock is on whenever we access the sensor, so we can consider that clock as an always-on clock for all practical matters. Yes. 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. That's where I don't agree. Here we need to think about the bridge as the combination of the bridge chip and the board on which it's soldered, as the board itself isn't modelled separately. Yes, we agree to disagree on this. The board layout is not the bridge. Strictly speaking, you're right. However, board layout is information that need to be conveyed to software one way or the other. Historically that information has been part of the bridge driver. For instance, if you look at the bttv driver, board layouts are stored in bttv-cards.c. That's why I believe that the em28xx driver should store board layout information as well. Even if the bridge doesn't control the clock, it provides a clock to the sensor. That highly depends on how it is wired. The clock could be provided by some independent circuit, or could be driven from the bridge clock. It is very common on those em28xx sticks to see two or even more xtals on it. As such, it's the responsibility of the bridge driver to provide the clock to the sensor driver. The sensor driver knows that the sensor needs a clock, and must thus get a clock object from somewhere. Somewhere doesn't mean that it comes from em28xx. This is a fundamental principle of the Linux clock framework and regulator framework. For fixed-frequency always-on clocks, as well as for fixed-voltage always-on regulators, the clock and/or regulator provider just needs to register a fixed clock or regulator, which is very easy to do. Ok, but the voltage and clock regulators are not mapped, on embedded devices, as part of the USB or PCI bus bridge device (except, of course, when the voltage/clocks are needed by the bridge device itself). It is mapped elsewhere, at DT. Or in a C code board file, depending on the platform. DT or board files are more or less equivalent, both of them store information about the board. For PCI and USB devices we need to store that information somewhere as well. As the em28xx driver already stores board layout information in em28xx-cards.c, we could store clock information there as well (I haven't checked whether that's the best place to store that information in the driver). I don't see why storing board-specific clock information (there's a fixed-frequency clock with this frequency and this name on the board) in the driver is a different issue than storing other kind of board information in the em28xx_board structure. The v4l2-clock API has been designed to mimic the clock API to ease the transition to the clock API at a later time (the v4l2-clock API is meant to be temporary only). It doesn't offer all the helper functions available in the clock API and should thus be improved, as Guennadi pointed out. That argument I understand: it should mimic the clock API, to avoid rework. * possible fixes: several fixes have been
Re: em28xx + ov2640 and v4l2-clk
Em Tue, 27 Aug 2013 17:27:52 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: Hi Mauro, On Tuesday 27 August 2013 11:08:58 Mauro Carvalho Chehab wrote: Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: On Monday 26 August 2013 11:09:33 Mauro Carvalho Chehab wrote: Guennadi Liakhovetski g.liakhovet...@gmx.de escreveu: Ok, but the voltage and clock regulators are not mapped, on embedded devices, as part of the USB or PCI bus bridge device (except, of course, when the voltage/clocks are needed by the bridge device itself). It is mapped elsewhere, at DT. Or in a C code board file, depending on the platform. DT or board files are more or less equivalent, both of them store information about the board. For PCI and USB devices we need to store that information somewhere as well. As the em28xx driver already stores board layout information in em28xx-cards.c, we could store clock information there as well (I haven't checked whether that's the best place to store that information in the driver). I don't see why storing board-specific clock information (there's a fixed-frequency clock with this frequency and this name on the board) in the driver is a different issue than storing other kind of board information in the em28xx_board structure. Yes, on PCI/USB drivers, we have a board specific setup. On em28xx, it is at em28xx-cards.c. Yet, there's no board-specific information in this case: em28xx doesn't manage clocks. It is always on. No need to add a bit there at the boards config file to initialize the clock before loading the subdevice, because the clock is already there. The point is that the client driver knows that it needs a clock, and knows how to use it (for instance it knows that it should turn the clock on at least 100ms before sending the first I2C command). However, the client should not know how the clock is provided. That's the clock API abstraction layer. The client will request the clock and turn it on/off when it needs to, and if the clock source is a crystal it will always be on. On platforms where the clock can be controlled we will thus save power by disabling the clock when it's not used, and on other platforms the clock will just always be on, without any need to code this explictly in all client drivers. On em28xx devices, power saving is done by enabling reset pin. On several hardware, doing that internally disables the clock line. I'm not sure if ov2640 supports this mode (Frank may know better how power saving is done with those cameras). Other devices have an special pin for power off or power saving. Anyway, that rises an interesting question: on devices with wired clocks, the power saving mode should not be provided via clock API abstraction layer, but via a callback to the bridge (as the bridge knows the GPIO register/bit that corresponds to device reset and/or power off pin). So, the only sense on having a clock API is when the hardware allows some control on it. So, if the hardware can't be controlled and it is always on, it makes no sense to register a clock. Please also note that, even if the clock can't be controlled, the sensor might need to query the clock frequency for instance to adjust its PLL parameters. The clk_get_rate() call is used for such a purpose, and requires a clock object. Ok, this is a good point. The thing is that you're wanting to use the clock register as a way to detect that the device got initialized. I'm not sure to follow you there, I don't think that's how I want to use the clock. Could you please elaborate ? As Sylwester pointed, the lack of clock register makes ov2640 to defer probing, as it assumes that the sensor is not ready. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
On Sat, 24 Aug 2013, Mauro Carvalho Chehab wrote:
Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer fschaefer@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.
* 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
Re: em28xx + ov2640 and v4l2-clk
Em Mon, 26 Aug 2013 15:54:16 +0200 (CEST)
Guennadi Liakhovetski g.liakhovet...@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@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
Re: em28xx + ov2640 and v4l2-clk
Em Tue, 20 Aug 2013 18:39:33 +0200 Frank Schäfer fschaefer@googlemail.com escreveu: Am 20.08.2013 17:31, schrieb Mauro Carvalho Chehab: Em Tue, 20 Aug 2013 15:38:57 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: 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 ! commit 9aea470b399d797e88be08985c489855759c6c60 Author: Guennadi Liakhovetski g.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them. Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? Well, it is simple: we should either revert the patch(es) that broke it or someone should fix it at em28xx. If nobody could fix it, I'll just revert the patches that broke it and ask -stable to do the same. Btw, 3.10 is a long term stable, so, it is not too late for fixes there. AFAICS, 3.10 should be fine. It should be possible to fix em28xx before Linus releases 3.11, but what about other drivers ? It seems like a v4l2-clock has been made mandatory for all sensor drivers (not only ov2640). I don't know if there are any other users of these drivers apart from soc_camera and em28xx... ? Currently, gspca doesn't use the sensors drivers (nor uvc). So, very few places drivers are actually affected. I think only em28xx is affected by ov2640 changes. The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation. Ok, but this doesn't solve one issue: who would do it and when. I can spend some time on em28xx tomorrow evening. Regards, Frank Cheers, Mauro -- Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäfer fschaefer@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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetskig.liakhovet...@gmx.de
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 Liakhovetskig.liakhovet...@gmx.de
Acked-by: Hans Verkuilhans.verk...@cisco.com
Acked-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when
making such a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be
too late to
fix the issue (given that 3.10 is already broken) ? The fix
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 ?
For UXGA for example, the clock needs to be switched to 12MHz, while
24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the
switch (to define fixed bindings between resoultions and clock
frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the
clock to the next nearest supported frequency ?
Regards,
Frank
I
Re: em28xx + ov2640 and v4l2-clk
On 08/24/2013 09:03 PM, Mauro Carvalho Chehab wrote:
Em Fri, 23 Aug 2013 00:15:52 +0200
Frank Schäferfschaefer@googlemail.com escreveu:
Am 21.08.2013 23:42, schrieb Sylwester Nawrocki:
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:
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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetskig.liakhovet...@gmx.de
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 Liakhovetskig.liakhovet...@gmx.de
Acked-by: Hans Verkuilhans.verk...@cisco.com
Acked-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when
making such a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be
too late to
fix the issue (given that 3.10 is already broken) ? The fix
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.
Yeah, constructive critics is always welcome ;)
The deferred probing mechanism has been getting more common recently, as
it is difficult to model all dependencies between devices. So if some
_resource_ for a device is missing its probe() is being deferred. It then
gets retried after some other device probed, e.g. the one that provides
the missing resource. So I can't see, what exactly is a misuse here ?
The interface was written to mimic what OF does with clock.
Kind of, as I see it we just pass control of one of device's resource to
its particular driver, rather than handling it somewhere else.
Typically image sensors in embedded systems need specific sequence of
voltage supply, GPIO and clock control. And its a driver of the device
that will have coded such sequences. We don't have board files any more
on systems using Device Tree, so any hack that used to live in board
files need to be replaced with proper, i.e. more exact driver/resource
modelling.
Yeah, I agree that this sucks for non OF drivers.
Yes, I agree. But we need to make it to suck
Re: em28xx + ov2640 and v4l2-clk
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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetskig.liakhovet...@gmx.de
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 Liakhovetskig.liakhovet...@gmx.de
Acked-by: Hans Verkuilhans.verk...@cisco.com
Acked-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when
making such a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be
too late to
fix the issue (given that 3.10 is already broken) ? The fix
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.
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.
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 ?
For UXGA for example, the clock needs to be switched to 12MHz, while
24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the
switch (to define fixed bindings between resoultions and clock
frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the
clock to the next nearest supported frequency ?
Regards,
Frank
I tried to implement a v4l2_clk for the em28xx driver (not yet beeing
sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have
specific expectations for the names of the clock.
The name must me mclk and dev_name must be the device name of the i2c
client device.
Is mclk supposed to be a clock type ?
Re: em28xx + ov2640 and v4l2-clk
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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetski g.liakhovet...@gmx.de
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.liakhovet...@gmx.de
Acked-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when making such
a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be too late
to
fix the issue (given that 3.10 is already broken) ? The fix 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.
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 ?
For UXGA for example, the clock needs to be switched to 12MHz, while
24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the
switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the
clock to the next nearest supported frequency ?
Regards,
Frank
I tried to implement a v4l2_clk for the em28xx driver (not yet beeing
sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have
specific expectations for the names of the clock.
The name must me mclk and dev_name must be the device name of the i2c
client device.
Is mclk supposed to be a clock type ? Wouldn't an enum be a better
choice in this case ?
Anyway, the sensor subdevices are registered using
v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the
module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is
found during sensor probing), but at this point the device name of the
i2c_client isn't known yet...
Regards,
Frank
v4l2-clk.c should provide a helper function to do so as that will be a
pretty
common operation.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetskig.liakhovet...@gmx.de
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 Liakhovetskig.liakhovet...@gmx.de
Acked-by: Hans Verkuilhans.verk...@cisco.com
Acked-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when making such a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be too late to
fix the issue (given that 3.10 is already broken) ? The fix 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.
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 ?
For UXGA for example, the clock needs to be switched to 12MHz, while
24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the
switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the
clock to the next nearest supported frequency ?
Regards,
Frank
I tried to implement a v4l2_clk for the em28xx driver (not yet beeing
sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have
specific expectations for the names of the clock.
The name must me mclk and dev_name must be the device name of the i2c
client device.
Is mclk supposed to be a clock type ? Wouldn't an enum be a better
choice in this case ?
This is made similar to the common clock API, a string is an identifier
of a clock for the device. I can't see anything unusual in that, it will
also make it easier to phase out the v4l2-clk API and replace it with
the common clock API once that is more widely available.
The name is supposed to come from the datasheet and usually be different
for each sensor, but since we mostly deal with just one clock a common
mclk name was chosen for simplicity.
Anyway, the sensor subdevices are registered using
v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the
module and returns v4l2_subdev.
The
Re: em28xx + ov2640 and v4l2-clk
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 ! commit 9aea470b399d797e88be08985c489855759c6c60 Author: Guennadi Liakhovetski g.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them. Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Em Tue, 20 Aug 2013 15:38:57 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: 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 ! commit 9aea470b399d797e88be08985c489855759c6c60 Author: Guennadi Liakhovetski g.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them. Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? Well, it is simple: we should either revert the patch(es) that broke it or someone should fix it at em28xx. If nobody could fix it, I'll just revert the patches that broke it and ask -stable to do the same. Btw, 3.10 is a long term stable, so, it is not too late for fixes there. The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation. Ok, but this doesn't solve one issue: who would do it and when. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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 !
commit 9aea470b399d797e88be08985c489855759c6c60
Author: Guennadi Liakhovetski g.liakhovet...@gmx.de
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.liakhovet...@gmx.de
Acked-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
(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.
We definitely need to check all users of our sensor drivers when making such
a
change. Mistakes happen, so let's fix them.
Guennadi is on holidays until the end of this week. Would that be too late to
fix the issue (given that 3.10 is already broken) ? The fix 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().
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 ?
For UXGA for example, the clock needs to be switched to 12MHz, while
24MHz is used for smaller reolutions.
But I'm not sure if it is a good idea to let the sensor driver do the
switch (to define fixed bindings between resoultions and clock frequencies).
Btw, what if a frequency is requested which isn't supported ? Set the
clock to the next nearest supported frequency ?
Regards,
Frank
v4l2-clk.c should provide a helper function to do so as that will be a pretty
common operation.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Am 20.08.2013 17:31, schrieb Mauro Carvalho Chehab: Em Tue, 20 Aug 2013 15:38:57 +0200 Laurent Pinchart laurent.pinch...@ideasonboard.com escreveu: 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 ! commit 9aea470b399d797e88be08985c489855759c6c60 Author: Guennadi Liakhovetski g.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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. We definitely need to check all users of our sensor drivers when making such a change. Mistakes happen, so let's fix them. Guennadi is on holidays until the end of this week. Would that be too late to fix the issue (given that 3.10 is already broken) ? Well, it is simple: we should either revert the patch(es) that broke it or someone should fix it at em28xx. If nobody could fix it, I'll just revert the patches that broke it and ask -stable to do the same. Btw, 3.10 is a long term stable, so, it is not too late for fixes there. AFAICS, 3.10 should be fine. It should be possible to fix em28xx before Linus releases 3.11, but what about other drivers ? It seems like a v4l2-clock has been made mandatory for all sensor drivers (not only ov2640). I don't know if there are any other users of these drivers apart from soc_camera and em28xx... ? The fix shouldn't be too complex, registering a dummy V4L2 clock in the em28xx driver should be enough. v4l2-clk.c should provide a helper function to do so as that will be a pretty common operation. Ok, but this doesn't solve one issue: who would do it and when. I can spend some time on em28xx tomorrow evening. Regards, Frank Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
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 ! Regards, Frank Thanks Guennadi -Original Message- From: Frank Schäfer fschaefer@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer fschaefer@googlemail.com 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.liakhovet...@gmx.de 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.liakhovet...@gmx.de Acked-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com (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 Regards, Frank Thanks Guennadi -Original Message- From: Frank Schäfer fschaefer@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx + ov2640 and v4l2-clk
Hi Frank, As I mentioned on the list, I'm currently on a holiday, so, replying briefly. 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. Thanks Guennadi -Original Message- From: Frank Schäfer fschaefer@googlemail.com To: Guennadi Liakhovetski g.liakhovet...@gmx.de, Linux Media Mailing List linux-media@vger.kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
