Message ID | 1280828276-483-2-git-send-email-m.grzeschik@pengutronix.de (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Tue, 03 Aug 2010 09:38:49 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Tue, 03 Aug 2010 09:28:08 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1OgDxQ-0004Ee-RJ; Tue, 03 Aug 2010 09:38:49 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755813Ab0HCJim (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Tue, 3 Aug 2010 05:38:42 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:58896 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755784Ab0HCJik (ORCPT <rfc822; linux-media@vger.kernel.org>); Tue, 3 Aug 2010 05:38:40 -0400 Received: from octopus.hi.pengutronix.de ([2001:6f8:1178:2:215:17ff:fe12:23b0]) by metis.ext.pengutronix.de with esmtp (Exim 4.71) (envelope-from <mgr@pengutronix.de>) id 1OgDxH-0000zl-06; Tue, 03 Aug 2010 11:38:39 +0200 Received: from mgr by octopus.hi.pengutronix.de with local (Exim 4.69) (envelope-from <mgr@pengutronix.de>) id 1OgDxG-0000MC-Oh; Tue, 03 Aug 2010 11:38:38 +0200 From: Michael Grzeschik <m.grzeschik@pengutronix.de> To: linux-media@vger.kernel.org Cc: baruch@tkos.co.il, g.liakhovetski@gmx.de, s.hauer@pengutronix.de, Michael Grzeschik <m.grzeschik@pengutronix.de> Subject: [PATCH 1/5] mx2_camera: change to register and probe Date: Tue, 3 Aug 2010 11:37:52 +0200 Message-Id: <1280828276-483-2-git-send-email-m.grzeschik@pengutronix.de> X-Mailer: git-send-email 1.7.1 In-Reply-To: <1280828276-483-1-git-send-email-m.grzeschik@pengutronix.de> References: <1280828276-483-1-git-send-email-m.grzeschik@pengutronix.de> X-SA-Exim-Connect-IP: 2001:6f8:1178:2:215:17ff:fe12:23b0 X-SA-Exim-Mail-From: mgr@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-media@vger.kernel.org Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Michael Grzeschik
Aug. 3, 2010, 9:37 a.m. UTC
change this driver back to register and probe, since some platforms
first have to initialize an already registered power regulator to switch
on the camera.
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
drivers/media/video/mx2_camera.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
Comments
On Tue, 3 Aug 2010, Michael Grzeschik wrote: > change this driver back to register and probe, since some platforms > first have to initialize an already registered power regulator to switch > on the camera. Sorry, don't see a difference. Can you give an example of two call sequences, where this change changes the behaviour? Thanks Guennadi > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/media/video/mx2_camera.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c > index 98c93fa..c77a673 100644 > --- a/drivers/media/video/mx2_camera.c > +++ b/drivers/media/video/mx2_camera.c > @@ -1491,13 +1491,15 @@ static struct platform_driver mx2_camera_driver = { > .driver = { > .name = MX2_CAM_DRV_NAME, > }, > + > + .probe = mx2_camera_probe, > .remove = __devexit_p(mx2_camera_remove), > }; > > > static int __init mx2_camera_init(void) > { > - return platform_driver_probe(&mx2_camera_driver, &mx2_camera_probe); > + return platform_driver_register(&mx2_camera_driver); > } > > static void __exit mx2_camera_exit(void) > -- > 1.7.1 > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > change this driver back to register and probe, since some platforms > > first have to initialize an already registered power regulator to switch > > on the camera. > > Sorry, don't see a difference. Can you give an example of two call > sequences, where this change changes the behaviour? > Yes, when you look at the today posted patch [1] you find the function pcm970_baseboard_init_late as an late_initcall. It uses an already registred regulator device to turn on the power of the camera before the cameras device registration. [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html Thanks, Michael
On Tue, 3 Aug 2010, Michael Grzeschik wrote: > On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > change this driver back to register and probe, since some platforms > > > first have to initialize an already registered power regulator to switch > > > on the camera. > > > > Sorry, don't see a difference. Can you give an example of two call > > sequences, where this change changes the behaviour? > > > > Yes, when you look at the today posted patch [1] you find the function > pcm970_baseboard_init_late as an late_initcall. It uses an already > registred regulator device to turn on the power of the camera before the > cameras device registration. > > [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html Sorry again, still don't understand. What I mean is the following: take two cases - before and after your patch. What is the difference? As far as I know, the difference between platform_driver_probe() and platform_driver_register() is just that the probe method gets discarded in an __init section, which is suitable for non hotpluggable devices. I don't know what the difference this should make for call order. So, that's what I am asking about. Can you explain, how this patch changes the call order in your case? Can you tell, that in the unpatches case the probe is called at that moment, and in the patched case it is called at a different point of time and that fixes the 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 04, 2010 at 01:01:34AM +0200, Guennadi Liakhovetski wrote: > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > change this driver back to register and probe, since some platforms > > > > first have to initialize an already registered power regulator to switch > > > > on the camera. > > > > > > Sorry, don't see a difference. Can you give an example of two call > > > sequences, where this change changes the behaviour? > > > > > > > Yes, when you look at the today posted patch [1] you find the function > > pcm970_baseboard_init_late as an late_initcall. It uses an already > > registred regulator device to turn on the power of the camera before the > > cameras device registration. > > > > [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html > > Sorry again, still don't understand. What I mean is the following: take > two cases - before and after your patch. What is the difference? As far as > I know, the difference between platform_driver_probe() and > platform_driver_register() is just that the probe method gets discarded in > an __init section, which is suitable for non hotpluggable devices. I don't > know what the difference this should make for call order. So, that's what > I am asking about. Can you explain, how this patch changes the call order > in your case? Can you tell, that in the unpatches case the probe is called > at that moment, and in the patched case it is called at a different point > of time and that fixes the problem. The following is above platform_driver_probe: * Use this instead of platform_driver_register() when you know the device * is not hotpluggable and has already been registered, and you want to * remove its run-once probe() infrastructure from memory after the * driver has bound to the device. So platform_driver_probe will only call the probe function when the device is already there when this function runs. This is not the case on our board. We have to register the camera in late_initcall (to make sure the needed regulators are already there). During late_initcall time the platform_driver_probe has already run. I don't really like the trend to platform_driver_probe, because this makes cases like camera needs regulator which in turn needs SPI even more complicated. Sascha
On Wed, 4 Aug 2010, Sascha Hauer wrote: > On Wed, Aug 04, 2010 at 01:01:34AM +0200, Guennadi Liakhovetski wrote: > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > > > change this driver back to register and probe, since some platforms > > > > > first have to initialize an already registered power regulator to switch > > > > > on the camera. > > > > > > > > Sorry, don't see a difference. Can you give an example of two call > > > > sequences, where this change changes the behaviour? > > > > > > > > > > Yes, when you look at the today posted patch [1] you find the function > > > pcm970_baseboard_init_late as an late_initcall. It uses an already > > > registred regulator device to turn on the power of the camera before the > > > cameras device registration. > > > > > > [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html > > > > Sorry again, still don't understand. What I mean is the following: take > > two cases - before and after your patch. What is the difference? As far as > > I know, the difference between platform_driver_probe() and > > platform_driver_register() is just that the probe method gets discarded in > > an __init section, which is suitable for non hotpluggable devices. I don't > > know what the difference this should make for call order. So, that's what > > I am asking about. Can you explain, how this patch changes the call order > > in your case? Can you tell, that in the unpatches case the probe is called > > at that moment, and in the patched case it is called at a different point > > of time and that fixes the problem. > > > The following is above platform_driver_probe: > > * Use this instead of platform_driver_register() when you know the device > * is not hotpluggable and has already been registered, and you want to > * remove its run-once probe() infrastructure from memory after the > * driver has bound to the device. > > So platform_driver_probe will only call the probe function when the device > is already there when this function runs. This is not the case on our board. > We have to register the camera in late_initcall (to make sure the needed > regulators are already there). During late_initcall time the > platform_driver_probe has already run. Ok, now I see. I missed the key-phrase: "before the cameras device registration." Ok, in this case, it's certainly a valid reason for the change. Just one more question: wouldn't calling pcm970_baseboard_init_late() from device_initcall fix the problem without requiring to change the driver? > I don't really like the trend to platform_driver_probe, because this > makes cases like camera needs regulator which in turn needs SPI even > more complicated. Well, you can always change to using the platform_driver_register() if platform_driver_probe() causes problems, otherwise it does have its advantages, as described in the comment, you quoted above. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 04, 2010 at 10:24:50AM +0200, Guennadi Liakhovetski wrote: > On Wed, 4 Aug 2010, Sascha Hauer wrote: > > > On Wed, Aug 04, 2010 at 01:01:34AM +0200, Guennadi Liakhovetski wrote: > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > On Tue, Aug 03, 2010 at 08:22:13PM +0200, Guennadi Liakhovetski wrote: > > > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > > > > > change this driver back to register and probe, since some platforms > > > > > > first have to initialize an already registered power regulator to switch > > > > > > on the camera. > > > > > > > > > > Sorry, don't see a difference. Can you give an example of two call > > > > > sequences, where this change changes the behaviour? > > > > > > > > > > > > > Yes, when you look at the today posted patch [1] you find the function > > > > pcm970_baseboard_init_late as an late_initcall. It uses an already > > > > registred regulator device to turn on the power of the camera before the > > > > cameras device registration. > > > > > > > > [1] [PATCH 1/2] ARM: i.MX27 pcm970: Add camera support > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022317.html > > > > > > Sorry again, still don't understand. What I mean is the following: take > > > two cases - before and after your patch. What is the difference? As far as > > > I know, the difference between platform_driver_probe() and > > > platform_driver_register() is just that the probe method gets discarded in > > > an __init section, which is suitable for non hotpluggable devices. I don't > > > know what the difference this should make for call order. So, that's what > > > I am asking about. Can you explain, how this patch changes the call order > > > in your case? Can you tell, that in the unpatches case the probe is called > > > at that moment, and in the patched case it is called at a different point > > > of time and that fixes the problem. > > > > > > The following is above platform_driver_probe: > > > > * Use this instead of platform_driver_register() when you know the device > > * is not hotpluggable and has already been registered, and you want to > > * remove its run-once probe() infrastructure from memory after the > > * driver has bound to the device. > > > > So platform_driver_probe will only call the probe function when the device > > is already there when this function runs. This is not the case on our board. > > We have to register the camera in late_initcall (to make sure the needed > > regulators are already there). During late_initcall time the > > platform_driver_probe has already run. > > Ok, now I see. I missed the key-phrase: "before the cameras device > registration." Ok, in this case, it's certainly a valid reason for the > change. Just one more question: wouldn't calling > pcm970_baseboard_init_late() from device_initcall fix the problem without > requiring to change the driver? No, sorry but this doesn't solve the problem. I tested it and get an "unable to get regulator: -19" when i hit on that. The problem is the device init order. The pcm970_baseboard_init_late comes first and then the regulator. So i think we should keep that patch. Michael
On Wed, 4 Aug 2010, Michael Grzeschik wrote: > No, sorry but this doesn't solve the problem. I tested it and get an > "unable to get regulator: -19" when i hit on that. The problem is the > device init order. The pcm970_baseboard_init_late comes first and > then the regulator. So i think we should keep that patch. Ok, you could register a bus-notifier on the soc-camera bus (http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/21364/focus=8520), watching out for a BUS_NOTIFY_ADD_DEVICE event. I think, that would be a more elegant solution, with it we still preserve the ability to clean up the probe function. Although, for that, I think, we'd need to make it __init instead of __devinit. In any case, I would prefer that solution, however, if for some reason you cannot or do not want to do it, I'll take this patch. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 3 Aug 2010, Michael Grzeschik wrote: > change this driver back to register and probe, since some platforms > first have to initialize an already registered power regulator to switch > on the camera. I shall be preparing a pull-request for 2.6.36-rc1 #2, but since we haven't finished discussing this and when this is ready, this will be a fix - without this your platform doesn't work, right? So, we can push it after rc1. Thanks Guennadi > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/media/video/mx2_camera.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c > index 98c93fa..c77a673 100644 > --- a/drivers/media/video/mx2_camera.c > +++ b/drivers/media/video/mx2_camera.c > @@ -1491,13 +1491,15 @@ static struct platform_driver mx2_camera_driver = { > .driver = { > .name = MX2_CAM_DRV_NAME, > }, > + > + .probe = mx2_camera_probe, > .remove = __devexit_p(mx2_camera_remove), > }; > > > static int __init mx2_camera_init(void) > { > - return platform_driver_probe(&mx2_camera_driver, &mx2_camera_probe); > + return platform_driver_register(&mx2_camera_driver); > } > > static void __exit mx2_camera_exit(void) > -- > 1.7.1 > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Thu, Aug 05, 2010 at 10:17:11PM +0200, Guennadi Liakhovetski wrote: > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > change this driver back to register and probe, since some platforms > > first have to initialize an already registered power regulator to switch > > on the camera. > > I shall be preparing a pull-request for 2.6.36-rc1 #2, but since we > haven't finished discussing this and when this is ready, this will be a > fix - without this your platform doesn't work, right? So, we can push it > after rc1. The issue is, that we cannot change the platform code from the late_initcall structure. For me there is no other solution than that, because we have to enable the regulator before the camera chip to communicate over i2c. If we would move to the notify way we would first listen for the i2c enabled clients but for that we would still have to first enable the regulator. At this moment i don't see a solution in this way. The safest way would still be to use the patch as is. Thanks, Michael
On Tue, 10 Aug 2010, Michael Grzeschik wrote: > Hi Guennadi, > > On Thu, Aug 05, 2010 at 10:17:11PM +0200, Guennadi Liakhovetski wrote: > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > change this driver back to register and probe, since some platforms > > > first have to initialize an already registered power regulator to switch > > > on the camera. > > > > I shall be preparing a pull-request for 2.6.36-rc1 #2, but since we > > haven't finished discussing this and when this is ready, this will be a > > fix - without this your platform doesn't work, right? So, we can push it > > after rc1. > > The issue is, that we cannot change the platform code from the > late_initcall structure. For me there is no other solution than that, > because we have to enable the regulator before the camera chip to > communicate over i2c. If we would move to the notify way we would > first listen for the i2c enabled clients but for that we would still > have to first enable the regulator. At this moment i don't see a > solution in this way. Hm, I think, there is an easier way to do this: just use the .power() callback from struct soc_camera_link. It is called for the first time before the camera is added to the i2c bus, so, before any IO is taking place. Just be careful to make sure you don't call one-time init actions (like gpio_request()) multiple times - .power is called also later again upon each open / close. So, you'll need some flag to detect the very first power-on. Sorry, for keeping on my attempts to avoid your patch - it really seems to me, a better solution is possible. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Guennadi, On Tue, Aug 10, 2010 at 09:08:11PM +0200, Guennadi Liakhovetski wrote: > On Tue, 10 Aug 2010, Michael Grzeschik wrote: > > > Hi Guennadi, > > > > On Thu, Aug 05, 2010 at 10:17:11PM +0200, Guennadi Liakhovetski wrote: > > > On Tue, 3 Aug 2010, Michael Grzeschik wrote: > > > > > > > change this driver back to register and probe, since some platforms > > > > first have to initialize an already registered power regulator to switch > > > > on the camera. > > > > > > I shall be preparing a pull-request for 2.6.36-rc1 #2, but since we > > > haven't finished discussing this and when this is ready, this will be a > > > fix - without this your platform doesn't work, right? So, we can push it > > > after rc1. > > > > The issue is, that we cannot change the platform code from the > > late_initcall structure. For me there is no other solution than that, > > because we have to enable the regulator before the camera chip to > > communicate over i2c. If we would move to the notify way we would > > first listen for the i2c enabled clients but for that we would still > > have to first enable the regulator. At this moment i don't see a > > solution in this way. > > Hm, I think, there is an easier way to do this: just use the .power() > callback from struct soc_camera_link. It is called for the first time > before the camera is added to the i2c bus, so, before any IO is taking > place. Just be careful to make sure you don't call one-time init actions > (like gpio_request()) multiple times - .power is called also later again > upon each open / close. So, you'll need some flag to detect the very first > power-on. this seems for me to be the best solution so far. At this time i have a patched version (v2) for my pcm970-baseboard.c glue code. I will send it ASOP, so this "change back to probe and register patch" is not needed anymore. > Sorry, for keeping on my attempts to avoid your patch - it really seems to > me, a better solution is possible. Good thoughts! Thanks for the hints, Michael
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c index 98c93fa..c77a673 100644 --- a/drivers/media/video/mx2_camera.c +++ b/drivers/media/video/mx2_camera.c @@ -1491,13 +1491,15 @@ static struct platform_driver mx2_camera_driver = { .driver = { .name = MX2_CAM_DRV_NAME, }, + + .probe = mx2_camera_probe, .remove = __devexit_p(mx2_camera_remove), }; static int __init mx2_camera_init(void) { - return platform_driver_probe(&mx2_camera_driver, &mx2_camera_probe); + return platform_driver_register(&mx2_camera_driver); } static void __exit mx2_camera_exit(void)