mx2_camera changes and corrections
cover mx2_camera changes and corrections
Hi everybody,
this patchseries include little changes, which are necessary for the mx2_camera driver to work properly on i.MX27 baseboards, especially an issue with the emma.
Thanks,
Michael
Michael Grzeschik (5):
mx2_camera: change to register and probe
mx2_camera: remove emma limitation for RGB565
mx2_camera: fix for list bufnum in frame_done_emma
mx2_camera: add rising edge for pixclock
mx2_camera: add informative camera clock frequency printout
drivers/media/video/mx2_camera.c | 19 +++++++++---------- 1 files changed, 9 insertions(+), 10 deletions(-)
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 1/5 mx2_camera: change to register and probe
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.
--- drivers/media/video/mx2_camera.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/media/video/mx2_camera.c +3 -1
@@ -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)
Signature · 1 line
1.7.1
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 3 lines · Michael Grzeschik
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
Quoted 31 lines
Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/media/video/mx2_camera.c +3
-1@@ -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)Signature · 1 line
1.7.1
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 9 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 11 lines · Michael Grzeschik
Quoted 9 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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?
Quoted 7 lines
Yes, when you look at the today posted patch [1] you find the function
pcm970_baseboard_init_lateas anlate_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/
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 21 lines · Guennadi Liakhovetski
Quoted 11 lines · Michael Grzeschik
Quoted 9 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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?
Quoted 7 lines
Yes, when you look at the today posted patch [1] you find the function
pcm970_baseboard_init_lateas anlate_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
Quoted 10 lines
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()andplatform_driver_register()is just that the probe method gets discarded in an__initsection, 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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 41 lines · Sascha Hauer
Quoted 21 lines · Guennadi Liakhovetski
Quoted 11 lines · Michael Grzeschik
Quoted 9 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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?
Quoted 7 lines
Yes, when you look at the today posted patch [1] you find the function
pcm970_baseboard_init_lateas anlate_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.htmlQuoted 10 lines
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()andplatform_driver_register()is just that the probe method gets discarded in an__initsection, 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-onceprobe()infrastructure from memory after the
* driver has bound to the device.
Quoted 5 lines
So
platform_driver_probewill 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 inlate_initcall(to make sure the needed regulators are already there). Duringlate_initcalltime theplatform_driver_probehas 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?
Quoted 3 lines
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/
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 49 lines · Guennadi Liakhovetski
Quoted 41 lines · Sascha Hauer
Quoted 21 lines · Guennadi Liakhovetski
Quoted 11 lines · Michael Grzeschik
Quoted 9 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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?
Quoted 7 lines
Yes, when you look at the today posted patch [1] you find the function
pcm970_baseboard_init_lateas anlate_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.htmlQuoted 10 lines
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()andplatform_driver_register()is just that the probe method gets discarded in an__initsection, 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-onceprobe()infrastructure from memory after the
* driver has bound to the device.Quoted 5 lines
So
platform_driver_probewill 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 inlate_initcall(to make sure the needed regulators are already there). Duringlate_initcalltime theplatform_driver_probehas already run.
Quoted 5 lines
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()fromdevice_initcallfix 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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 4 lines · Michael Grzeschik
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_latecomes 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/
Re: [PATCH 1/5] mx2_camera: change to register and probe
In reply to Michael GrzeschikQuoted 3 lines · Michael Grzeschik
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
Quoted 31 lines
Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/media/video/mx2_camera.c +3
-1@@ -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)Signature · 1 line
1.7.1
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Re: [PATCH 1/5] mx2_camera: change to register and probe
Hi Guennadi,
Quoted 10 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/5] mx2_camera: change to register and probe
Quoted 14 lines · Michael Grzeschik
Hi Guennadi,
Quoted 10 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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.
Quoted 7 lines
The issue is, that we cannot change the platform code from the
late_initcallstructure. 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/
Re: [PATCH 1/5] mx2_camera: change to register and probe
Hey Guennadi,
Quoted 24 lines · Guennadi Liakhovetski
Quoted 14 lines · Michael Grzeschik
Hi Guennadi,
Quoted 10 lines · Guennadi Liakhovetski
Quoted 3 lines · Michael Grzeschik
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.
Quoted 7 lines
The issue is, that we cannot change the platform code from the
late_initcallstructure. 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.
Quoted 7 lines
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 (likegpio_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.
Quoted 2 lines
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
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
PATCH 2/5 mx2_camera: remove emma limitation for RGB565
In reply to Michael GrzeschikIn the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
--- drivers/media/video/mx2_camera.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
drivers/media/video/mx2_camera.c +0 -8
@@ -897,10 +897,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,return -EINVAL ; } -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pix->pixelformat !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; - mf.width = pix->width; mf.height = pix->height; mf.field = pix->field;
@@ -944,10 +940,6 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,/* FIXME: implement MX27 limits */ -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pixfmt !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; -/* limit to MX25 hardware capabilities */ if (cpu_is_mx25 ()) {if (xlate->host_fmt->bits_per_sample <=8 )
Signature · 1 line
1.7.1
Re: [PATCH 2/5] mx2_camera: remove emma limitation for RGB565
Quoted 2 lines · Michael Grzeschik
In the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
I'd like some explanation for this one too, please. What about
+ /*
+ * We only use the EMMA engine to get rid of the broken
+ * DMA Engine. No color space consversion at the moment.
+ * We adjust incoming and outgoing pixelformat to rgb16
+ * and adjust the bytesperline accordingly.
+ */
+ writel(PRP_CNTL_CH1EN |
+ PRP_CNTL_CSIEN |
+ PRP_CNTL_DATA_IN_RGB16 |
+ PRP_CNTL_CH1_OUT_RGB16 |
+ PRP_CNTL_CH1_LEN |
+ PRP_CNTL_CH1BYP |
+ PRP_CNTL_CH1_TSKIP(0) |
+ PRP_CNTL_IN_TSKIP(0),
+ pcdev->base_emma + PRP_CNTL);
+
+ writel(((bytesperline >> 1) << 16) | icd->user_height,
+ pcdev->base_emma + PRP_SRC_FRAME_SIZE);
+ writel(((bytesperline >> 1) << 16) | icd->user_height,
+ pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
+ writel(bytesperline,
+ pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
+ writel(0x2ca00565, /* RGB565 */
+ pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
+ writel(0x2ca00565, /* RGB565 */
+ pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
To me it looks like the eMMA is configured for RGB565. What's the trick?
Thanks
Guennadi
Quoted 36 lines
Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)drivers/media/video/mx2_camera.c +0
-8@@ -897,10 +897,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,return -EINVAL ; } -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pix->pixelformat !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; - mf.width = pix->width; mf.height = pix->height; mf.field = pix->field;@@ -944,10 +940,6 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,/* FIXME: implement MX27 limits */ -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pixfmt !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; -/* limit to MX25 hardware capabilities */ if (cpu_is_mx25 ()) {if (xlate->host_fmt->bits_per_sample <=8 )Signature · 1 line
1.7.1
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Re: [PATCH 2/5] mx2_camera: remove emma limitation for RGB565
Quoted 34 lines · Guennadi Liakhovetski
Quoted 2 lines · Michael Grzeschik
In the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
I'd like some explanation for this one too, please. What about
(quoted diff) +26
-0+ /* + * We only use the EMMA engine to get rid of the broken + * DMA Engine. No color space consversion at the moment. + * We adjust incoming and outgoing pixelformat to rgb16 + * and adjust the bytesperline accordingly. + */ + writel(PRP_CNTL_CH1EN | + PRP_CNTL_CSIEN | + PRP_CNTL_DATA_IN_RGB16 | + PRP_CNTL_CH1_OUT_RGB16 | + PRP_CNTL_CH1_LEN | + PRP_CNTL_CH1BYP | + PRP_CNTL_CH1_TSKIP(0) | + PRP_CNTL_IN_TSKIP(0), + pcdev->base_emma + PRP_CNTL); + + writel(((bytesperline >> 1) << 16) | icd->user_height, + pcdev->base_emma + PRP_SRC_FRAME_SIZE); + writel(((bytesperline >> 1) << 16) | icd->user_height, + pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE); + writel(bytesperline, + pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE); + writel(0x2ca00565, /* RGB565 */ + pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL); + writel(0x2ca00565, /* RGB565 */ + pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
Quoted 2 lines
To me it looks like the eMMA is configured for RGB565. What's the trick?
Yes, it seems to be an indication, but the emma currently does not touch any pixels, since the SRC_PIXEL_FORMAT and CH1_PIXEL_FORMAT are identical. It will be needed in the future when we are going to do some resizing operations with the emma or the SRC_PIXEL_FORMAT will differ to the output channels. But at that time, the simple condition check for RGB565 wouldn't be enough. So we should better remove them now.
Michael
Quoted 37 lines
Quoted 37 lines
Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)drivers/media/video/mx2_camera.c +0
-8@@ -897,10 +897,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,return -EINVAL ; } -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pix->pixelformat !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; - mf.width = pix->width; mf.height = pix->height; mf.field = pix->field;@@ -944,10 +940,6 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,/* FIXME: implement MX27 limits */ -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pixfmt !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; -/* limit to MX25 hardware capabilities */ if (cpu_is_mx25 ()) {if (xlate->host_fmt->bits_per_sample <=8 )Signature · 1 line
1.7.1
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 2/5] mx2_camera: remove emma limitation for RGB565
Quoted 35 lines · Michael Grzeschik
Quoted 34 lines · Guennadi Liakhovetski
Quoted 2 lines · Michael Grzeschik
In the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
I'd like some explanation for this one too, please. What about
(quoted diff) +26
-0+ /* + * We only use the EMMA engine to get rid of the broken + * DMA Engine. No color space consversion at the moment. + * We adjust incoming and outgoing pixelformat to rgb16 + * and adjust the bytesperline accordingly. + */ + writel(PRP_CNTL_CH1EN | + PRP_CNTL_CSIEN | + PRP_CNTL_DATA_IN_RGB16 | + PRP_CNTL_CH1_OUT_RGB16 | + PRP_CNTL_CH1_LEN | + PRP_CNTL_CH1BYP | + PRP_CNTL_CH1_TSKIP(0) | + PRP_CNTL_IN_TSKIP(0), + pcdev->base_emma + PRP_CNTL); + + writel(((bytesperline >> 1) << 16) | icd->user_height, + pcdev->base_emma + PRP_SRC_FRAME_SIZE); + writel(((bytesperline >> 1) << 16) | icd->user_height, + pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE); + writel(bytesperline, + pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE); + writel(0x2ca00565, /* RGB565 */ + pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL); + writel(0x2ca00565, /* RGB565 */ + pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
Quoted 9 lines
Quoted 2 lines
To me it looks like the eMMA is configured for RGB565. What's the trick?
Yes, it seems to be an indication, but the emma currently does not touch any pixels, since the
SRC_PIXEL_FORMATandCH1_PIXEL_FORMATare identical. It will be needed in the future when we are going to do some resizing operations with the emma or theSRC_PIXEL_FORMATwill differ to the output channels. But at that time, the simple condition check for RGB565 wouldn't be enough. So we should better remove them now.
Then at least, please fix the above comment:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ * We adjust incoming and outgoing pixelformat to rgb16 + * and adjust the bytesperline accordingly.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
PATCH v2 1/1 mx2_camera: remove emma limitation for RGB565
X-Uptime: 16:20:11 up 37 days, 5:31, 61 users, load average: 0.68, 0.32,
0.30
In the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
---
v1 -> v2
Changed Comment in emma_buf_init as recommended
drivers/media/video/mx2_camera.c | 15 +++++---------- 1 files changed, 5 insertions(+), 10 deletions(-)
drivers/media/video/mx2_camera.c +5 -10
@@ -712,8 +712,11 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,/* * We only use the EMMA engine to get rid of the broken * DMA Engine. No color space consversion at the moment. -* We adjust incoming and outgoing pixelformat to rgb16 -* and adjust the bytesperline accordingly. +* We set the incomming and outgoing pixelformat to an +* 16 Bit wide format and adjust the bytesperline +* accordingly. With this configuration the inputdata +* will not be changed by the emma and could be any type +* of 16 Bit Pixelformat. */ writel(PRP_CNTL_CH1EN | PRP_CNTL_CSIEN |
@@ -897,10 +900,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,return -EINVAL ; } -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pix->pixelformat !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; - mf.width = pix->width; mf.height = pix->height; mf.field = pix->field;
@@ -944,10 +943,6 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,/* FIXME: implement MX27 limits */ -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pixfmt !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; -/* limit to MX25 hardware capabilities */ if (cpu_is_mx25 ()) {if (xlate->host_fmt->bits_per_sample <=8 )
Signature · 1 line
1.7.1
PATCH RESEND v2 1/1 mx2_camera: remove emma limitation for RGB565
In reply to Guennadi LiakhovetskiIn the current source status the emma has no limitation for any PIXFMT since the data is parsed raw and unprocessed into the memory.
---
v1 -> v2
Changed Comment in emma_buf_init as recommended
drivers/media/video/mx2_camera.c | 15 +++++---------- 1 files changed, 5 insertions(+), 10 deletions(-)
drivers/media/video/mx2_camera.c +5 -10
@@ -712,8 +712,11 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,/* * We only use the EMMA engine to get rid of the broken * DMA Engine. No color space consversion at the moment. -* We adjust incoming and outgoing pixelformat to rgb16 -* and adjust the bytesperline accordingly. +* We set the incomming and outgoing pixelformat to an +* 16 Bit wide format and adjust the bytesperline +* accordingly. With this configuration the inputdata +* will not be changed by the emma and could be any type +* of 16 Bit Pixelformat. */ writel(PRP_CNTL_CH1EN | PRP_CNTL_CSIEN |
@@ -897,10 +900,6 @@ static int mx2_camera_set_fmt(struct soc_camera_device *icd,return -EINVAL ; } -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pix->pixelformat !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; - mf.width = pix->width; mf.height = pix->height; mf.field = pix->field;
@@ -944,10 +943,6 @@ static int mx2_camera_try_fmt(struct soc_camera_device *icd,/* FIXME: implement MX27 limits */ -/* eMMA can only do RGB565 */ -if (mx27_camera_emma (pcdev) && pixfmt !=V4L2_PIX_FMT_RGB565 ) -return -EINVAL ; -/* limit to MX25 hardware capabilities */ if (cpu_is_mx25 ()) {if (xlate->host_fmt->bits_per_sample <=8 )
Signature · 1 line
1.7.1
PATCH 3/5 mx2_camera: fix for list bufnum in frame_done_emma
In reply to Michael GrzeschikThe emma uses bufnum 1 and 0. This patch tells the bufqueue to change the next buffer to the next one and not the current one. Otherwise the BUG_ON above will trigger everytime.
--- drivers/media/video/mx2_camera.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/media/video/mx2_camera.c +1 -1
@@ -1193,7 +1193,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev, buf =list_entry (pcdev->capture.next, struct mx2_buffer, vb.queue); - buf->bufnum = bufnum; + buf->bufnum = !bufnum;list_move_tail (pcdev->capture.next, &pcdev->active_bufs);
Signature · 1 line
1.7.1
PATCH 4/5 mx2_camera: add rising edge for pixclock
In reply to Michael Grzeschik--- drivers/media/video/mx2_camera.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/media/video/mx2_camera.c +2 -0
@@ -785,6 +785,8 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,if (ret <0 )return ret; +if (common_flags &SOCAM_PCLK_SAMPLE_RISING ) + csicr1 |=CSICR1_REDGE ;if (common_flags &SOCAM_PCLK_SAMPLE_FALLING ) csicr1 |=CSICR1_INV_PCLK ;if (common_flags &SOCAM_VSYNC_ACTIVE_HIGH )
Signature · 1 line
1.7.1
PATCH 5/5 mx2_camera: add informative camera clock frequency printout
In reply to Michael Grzeschikported mx27_camera to 2.6.33.2
--- drivers/media/video/mx2_camera.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/media/video/mx2_camera.c +3 -0
@@ -1360,6 +1360,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev) goto exit_dma_free; } +dev_info (&pdev->dev,"Camera clock frequency: %ld\n" , +clk_get_rate (pcdev->clk_csi)); +INIT_LIST_HEAD (&pcdev->capture);INIT_LIST_HEAD (&pcdev->active_bufs);spin_lock_init (&pcdev->lock);
Signature · 1 line
1.7.1
Re: [PATCH 5/5] mx2_camera: add informative camera clock frequency printout
Quoted 1 line · Michael Grzeschik
ported
mx27_camerato 2.6.33.2
Sorry, do not understand what this description has to do with the contents
- adding a printk to a driver? I don't think this is something critical enough to be handled urgently now for 2.6.36, right?
Thanks
Guennadi
Quoted 24 lines
Signed-off-by Teresa Gamez <[email protected]>Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/media/video/mx2_camera.c +3
-0@@ -1360,6 +1360,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev) goto exit_dma_free; } +dev_info (&pdev->dev,"Camera clock frequency: %ld\n" , +clk_get_rate (pcdev->clk_csi)); +INIT_LIST_HEAD (&pcdev->capture);INIT_LIST_HEAD (&pcdev->active_bufs);spin_lock_init (&pcdev->lock);Signature · 1 line
1.7.1
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Re: [PATCH 5/5] mx2_camera: add informative camera clock frequency printout
Quoted 5 lines · Guennadi Liakhovetski
Quoted 1 line · Michael Grzeschik
ported
mx27_camerato 2.6.33.2Sorry, do not understand what this description has to do with the contents
The Description is of topic from a previous patchseries from Teresa Gamez and has nothin to do with the content, right!
Quoted 2 lines
- adding a printk to a driver? I don't think this is something critical enough to be handled urgently now for 2.6.36, right?
Yes you are right, this one isn't urgent.
Michael
Quoted 34 lines
Thanks
GuennadiQuoted 24 lines
Signed-off-by Teresa Gamez <[email protected]>Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/media/video/mx2_camera.c +3
-0@@ -1360,6 +1360,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev) goto exit_dma_free; } +dev_info (&pdev->dev,"Camera clock frequency: %ld\n" , +clk_get_rate (pcdev->clk_csi)); +INIT_LIST_HEAD (&pcdev->capture);INIT_LIST_HEAD (&pcdev->active_bufs);spin_lock_init (&pcdev->lock);Signature · 1 line
1.7.1---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 5/5] mx2_camera: add informative camera clock frequency printout
Quoted 33 lines · Michael Grzeschik
Quoted 5 lines · Guennadi Liakhovetski
Quoted 1 line · Michael Grzeschik
ported
mx27_camerato 2.6.33.2Sorry, do not understand what this description has to do with the contents
The Description is of topic from a previous patchseries from Teresa Gamez and has nothin to do with the content, right!
Quoted 2 lines
- adding a printk to a driver? I don't think this is something critical enough to be handled urgently now for 2.6.36, right?
Yes you are right, this one isn't urgent.
Michael
Quoted 18 lines
Thanks
GuennadiQuoted 14 lines
Signed-off-by Teresa Gamez <[email protected]>Signed-off-by Michael Grzeschik <[email protected]>--- drivers/media/video/mx2_camera.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/media/video/mx2_camera.c +0
-0@@ -1360,6 +1360,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev) goto exit_dma_free; }
Quoted 6 lines
Quoted 6 lines
drivers/media/video/mx2_camera.c static int __devinit mx2_camera_probe(struct platform_device *pdev) · line 1368
Quoted 6 lines
drivers/media/video/mx2_camera.c +3
-0+dev_info (&pdev->dev,"Camera clock frequency: %ld\n" , +clk_get_rate (pcdev->clk_csi)); +INIT_LIST_HEAD (&pcdev->capture);INIT_LIST_HEAD (&pcdev->active_bufs);spin_lock_init (&pcdev->lock);
Well, in mx2_camera_remove() we have a message
dev_info(&pdev->dev, "MX2 Camera driver unloaded\n");
and currently no counterpart in probe. I don't think this "unloaded" message is particularly valuable, but we've already got it. So, we can either remove it or add one more in probe. If you prefer the latter - fine, but (1) I'd put it later - just before "return 0;" where we already know probe will not fail, and (2) make it even more informative like
"MX2 Camera (CSI) driver probed, clock frequency %ld\n"
if you really _do_ think the user is interested to know that;) Otherwise, make this and the "unloaded" dev_dbg().
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
PATCH v2 1/1 mx2_camera: add informative camera clock frequency printout
In reply to Michael Grzeschik--- drivers/media/video/mx2_camera.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/media/video/mx2_camera.c +3 -0
@@ -1415,6 +1415,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)if (err) goto exit_free_emma; +dev_info (&pdev->dev,"MX2 Camera (CSI) driver probed, clock frequency: %ld\n" , +clk_get_rate (pcdev->clk_csi)); +return 0 ; exit_free_emma:
Signature · 1 line
1.7.1