[PATCH] Fix suspend/resume of pxa_camera driver

[PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

PXA suspend switches off DMA core, which looses all context
of previously assigned descriptors. As pxa_camera driver
relies on DMA transfers, setup the lost descriptors on
resume.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index efb2d19..0cacf16 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -1017,6 +1017,16 @@ static struct soc_camera_host pxa_soc_camera_host = {
 	.ops			= &pxa_soc_camera_host_ops,
 };
 
+static int pxa_camera_resume(struct platform_device *pdev)
+{
+	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
+
+	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
+	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
+	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
+	return 0;
+}
+
 static int pxa_camera_probe(struct platform_device *pdev)
 {
 	struct pxa_camera_dev *pcdev;
@@ -1188,6 +1198,7 @@ static struct platform_driver pxa_camera_driver = {
 	},
 	.probe		= pxa_camera_probe,
 	.remove		= __exit_p(pxa_camera_remove),
+	.resume		= pxa_camera_resume,
 };
 
 
-- 
1.5.5.3

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Hi Guennadi,

I forgot to join you in the patch submission, hence this mail, as you wrote
pxa_camera in the first place.

Have a nice day.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sun, 27 Jul 2008, Robert Jarzmik wrote:

> PXA suspend switches off DMA core, which looses all context
> of previously assigned descriptors. As pxa_camera driver
> relies on DMA transfers, setup the lost descriptors on
> resume.

Hm, is this really enough? How have you tested it - with a complete STR 
with powering the CPU and all peripherals down and only keeping the SDRAM 
in self-refresh or with some sort of a low-power "standby" mode? I think, 
when the CPU is really powered off, and this is what the suspend / resume 
callbacks should support, something like calling pxa_camera_deactivate() / 
pxa_camera_activate() should be done, i.e., disabling / enabling of 
clocks, configuring clock modes on the camera interface, resetting and 
powering down and up the camera. Please also notice, there're patches in 
work (ping?) to move ->power and ->reset callbacks from per camera host to 
per camera sensor. But you probably just can ignore those plans, even if 
you do conflict with them we'll resolve the conflict later.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 27 Jul 2008, Robert Jarzmik wrote:
>
>> PXA suspend switches off DMA core, which looses all context
>> of previously assigned descriptors. As pxa_camera driver
>> relies on DMA transfers, setup the lost descriptors on
>> resume.
>
> Hm, is this really enough? How have you tested it - with a complete STR 
> with powering the CPU and all peripherals down and only keeping the SDRAM 
> in self-refresh or with some sort of a low-power "standby" mode? I think, 
> when the CPU is really powered off, and this is what the suspend / resume 
> callbacks should support, something like calling pxa_camera_deactivate() / 
> pxa_camera_activate() should be done, i.e., disabling / enabling of 
> clocks, configuring clock modes on the camera interface, resetting and 
> powering down and up the camera. Please also notice, there're patches in 
> work (ping?) to move ->power and ->reset callbacks from per camera host to 
> per camera sensor. But you probably just can ignore those plans, even if 
> you do conflict with them we'll resolve the conflict later.

Yes, I have tested, with a complete suspend/resume cycle on a Mitac Mio A701
smartphone. And yes, the PXA suspend is based on SDRAM being in self refresh
state. I'm speaking of suspend, not standby, there's no confusion here.

Notice I always go into suspend while _not_ in active capturing. That could
change things.

Have you previously tested the pxa_camera driver in suspend ?

For history, my setup is :
 - a pxa272 on a Mio A701 board
 - a Micron MT9M111 chip (driver under construction)

For the camera part, by now, I'm using standard suspend/resume functions of the
platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
between the driver resume function and the availability of the I2C bus are not
properly chained. I'm still working on it.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sun, 27 Jul 2008, Robert Jarzmik wrote:

> Yes, I have tested, with a complete suspend/resume cycle on a Mitac Mio A701
> smartphone. And yes, the PXA suspend is based on SDRAM being in self refresh
> state. I'm speaking of suspend, not standby, there's no confusion here.
> 
> Notice I always go into suspend while _not_ in active capturing. That could
> change things.

Yes, this is the difference. The sensor is attached to the camera host 
only on open. In fact, I am not sure, how video applications should behave 
during a suspend / resume cycle. If you suspend, while, say, recording 
from your camera, should you directly continue recording after a wake up? 
How do currect drivers implement this? Or, in general, for example with 
audio - if you suspend while listening to a stream over the net, or to a 
CD, or to a mp3-file on your local disk, should the sound resume after a 
wake up? I added linux-pm for some authoritative answers:-)

If you know how a v4l2 device should handle suspend/resume, or when we get 
some answers, let's try to do it completely-

> Have you previously tested the pxa_camera driver in suspend ?

No, I have not. I didn't have power-management enabled on my board, and I 
don't know how easy such tests would be on my hardware. That's why I just 
removed all suspend/resume code from the pxa270 driver completely.

> For history, my setup is :
>  - a pxa272 on a Mio A701 board
>  - a Micron MT9M111 chip (driver under construction)
> 
> For the camera part, by now, I'm using standard suspend/resume functions of the
> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> between the driver resume function and the availability of the I2C bus are not
> properly chained. I'm still working on it.

Yes, we have to clarify this too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sun, 27 Jul 2008, Robert Jarzmik wrote:

> Yes, I have tested, with a complete suspend/resume cycle on a Mitac Mio A701
> smartphone. And yes, the PXA suspend is based on SDRAM being in self refresh
> state. I'm speaking of suspend, not standby, there's no confusion here.
> 
> Notice I always go into suspend while _not_ in active capturing. That could
> change things.

Yes, this is the difference. The sensor is attached to the camera host 
only on open. In fact, I am not sure, how video applications should behave 
during a suspend / resume cycle. If you suspend, while, say, recording 
from your camera, should you directly continue recording after a wake up? 
How do currect drivers implement this? Or, in general, for example with 
audio - if you suspend while listening to a stream over the net, or to a 
CD, or to a mp3-file on your local disk, should the sound resume after a 
wake up? I added linux-pm for some authoritative answers:-)

If you know how a v4l2 device should handle suspend/resume, or when we get 
some answers, let's try to do it completely-

> Have you previously tested the pxa_camera driver in suspend ?

No, I have not. I didn't have power-management enabled on my board, and I 
don't know how easy such tests would be on my hardware. That's why I just 
removed all suspend/resume code from the pxa270 driver completely.

> For history, my setup is :
>  - a pxa272 on a Mio A701 board
>  - a Micron MT9M111 chip (driver under construction)
> 
> For the camera part, by now, I'm using standard suspend/resume functions of the
> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> between the driver resume function and the availability of the I2C bus are not
> properly chained. I'm still working on it.

Yes, we have to clarify this too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Alan Stern]

On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:

> Yes, this is the difference. The sensor is attached to the camera host 
> only on open. In fact, I am not sure, how video applications should behave 
> during a suspend / resume cycle. If you suspend, while, say, recording 
> from your camera, should you directly continue recording after a wake up? 
> How do currect drivers implement this? Or, in general, for example with 
> audio - if you suspend while listening to a stream over the net, or to a 
> CD, or to a mp3-file on your local disk, should the sound resume after a 
> wake up? I added linux-pm for some authoritative answers:-)

IMO, suspend should be as transparent as possible for userspace.  So if
a user program has started an audio or video stream before suspend,
then after resume the stream should still be playing.

Alan Stern

Re: [linux-pm] [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:
>
> IMO, suspend should be as transparent as possible for userspace.  So if
> a user program has started an audio or video stream before suspend,
> then after resume the stream should still be playing.
Yes, that's what I think too.

The only little thing that should change will be timeout problem (userspace
waits on a poll to get a video frame within 1 second for example).
I'm not sure how well this is handled, I didn't check.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Pavel Machek]

On Mon 2008-07-28 20:37:25, Robert Jarzmik wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:
> >
> > IMO, suspend should be as transparent as possible for userspace.  So if
> > a user program has started an audio or video stream before suspend,
> > then after resume the stream should still be playing.
> Yes, that's what I think too.
> 
> The only little thing that should change will be timeout problem (userspace
> waits on a poll to get a video frame within 1 second for example).
> I'm not sure how well this is handled, I didn't check.

Userrland problem. Could hit you if you go deeply into swap, for
example. Fix userland.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:
>
> IMO, suspend should be as transparent as possible for userspace.  So if
> a user program has started an audio or video stream before suspend,
> then after resume the stream should still be playing.
Yes, that's what I think too.

The only little thing that should change will be timeout problem (userspace
waits on a poll to get a video frame within 1 second for example).
I'm not sure how well this is handled, I didn't check.

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 27 Jul 2008, Robert Jarzmik wrote:
>
> Yes, this is the difference. The sensor is attached to the camera host 
> only on open. In fact, I am not sure, how video applications should behave 
> during a suspend / resume cycle. If you suspend, while, say, recording 
> from your camera, should you directly continue recording after a wake up? 
> How do currect drivers implement this? Or, in general, for example with 
> audio - if you suspend while listening to a stream over the net, or to a 
> CD, or to a mp3-file on your local disk, should the sound resume after a 
> wake up? I added linux-pm for some authoritative answers:-)
AFAIK, on resume, sound streams continues. So the normal behaviour would be to
continue video stream too (as done in ALSA). This supposes the whole video chip
state is saved on suspend and restored on resume, of course.

> If you know how a v4l2 device should handle suspend/resume, or when we get 
> some answers, let's try to do it completely-
Of course. In a week or two, my mt9m111 driver will be ready for submission, and
in the review process I'll post a submission for complete suspend/resume.

>> For the camera part, by now, I'm using standard suspend/resume functions of the
>> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
>> between the driver resume function and the availability of the I2C bus are not
>> properly chained. I'm still working on it.
>
> Yes, we have to clarify this too.
Yes.

So, to sum up :
 - I finish the mt9m111 driver
 - I submit it
 - I cook up a clean suspend/resume (unless you did it first of course :)

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Mon, 28 Jul 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > On Sun, 27 Jul 2008, Robert Jarzmik wrote:
> >
> > Yes, this is the difference. The sensor is attached to the camera host 
> > only on open. In fact, I am not sure, how video applications should behave 
> > during a suspend / resume cycle. If you suspend, while, say, recording 
> > from your camera, should you directly continue recording after a wake up? 
> > How do currect drivers implement this? Or, in general, for example with 
> > audio - if you suspend while listening to a stream over the net, or to a 
> > CD, or to a mp3-file on your local disk, should the sound resume after a 
> > wake up? I added linux-pm for some authoritative answers:-)
> AFAIK, on resume, sound streams continues. So the normal behaviour would be to
> continue video stream too (as done in ALSA). This supposes the whole video chip
> state is saved on suspend and restored on resume, of course.

Ok, that's what I also thought.

> > If you know how a v4l2 device should handle suspend/resume, or when we get 
> > some answers, let's try to do it completely-
> Of course. In a week or two, my mt9m111 driver will be ready for submission, and
> in the review process I'll post a submission for complete suspend/resume.
> 
> >> For the camera part, by now, I'm using standard suspend/resume functions of the
> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> >> between the driver resume function and the availability of the I2C bus are not
> >> properly chained. I'm still working on it.
> >
> > Yes, we have to clarify this too.
> Yes.
> 
> So, to sum up :
>  - I finish the mt9m111 driver
>  - I submit it
>  - I cook up a clean suspend/resume (unless you did it first of course :)

Good plan! You don't have to worry - I will not do this before you:-)

And, I am still waiting for the patch to move .power and .reset to 
camera-link (see my another post on the V4L ML today). With that patch in 
place your power-management would also become easier and more logical.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> >> For the camera part, by now, I'm using standard suspend/resume functions of the
>> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
>> >> between the driver resume function and the availability of the I2C bus are not
>> >> properly chained. I'm still working on it.
>> >
>> > Yes, we have to clarify this too.
All right, I have my mind clarified, let's discuss now.

>>  - I cook up a clean suspend/resume (unless you did it first of course :)
Well, let's expose what we're facing here :
 - our video chip driver (ex: mt9m111) is an i2c driver
  => its resume function is called when i2c bus is resumed, so all is fine here

 - our video chip needs an external clock to work
  => example: mt9m111 needs a clock from pxa camera interface to have its i2c
  unit enabled
  => the mt9m111 driver resume function is unusable, as pxa_camera is resumed
  _after_ mt9m111, and thus mt9m111's i2c unit is not available at that moment

 - a working suspend/resume restores fully the video chip state
  => restores width/height/bpp
  => restores autoexposure, brightness, etc ...
  => all that insures userland is not impacted by suspend/resume

So, the only way I see to have suspend/resume working is :
 - modify soc_camera_ops to add suspend and resume functions
 - add suspend and resume functions in each chip driver (mt9m001, mt9m111, ...)
 - modify soc_camera.c (or pxa_camera.c ?) to call icd->ops->suspend() and
 icd->ops->resume()
 - modify pxa_camera.c (the patch I sent before)

Would you find that acceptable, or is there a better way ?

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Wed, 30 Jul 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> >> >> For the camera part, by now, I'm using standard suspend/resume functions of the
> >> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> >> >> between the driver resume function and the availability of the I2C bus are not
> >> >> properly chained. I'm still working on it.
> >> >
> >> > Yes, we have to clarify this too.
> All right, I have my mind clarified, let's discuss now.
> 
> >>  - I cook up a clean suspend/resume (unless you did it first of course :)
> Well, let's expose what we're facing here :
>  - our video chip driver (ex: mt9m111) is an i2c driver
>   => its resume function is called when i2c bus is resumed, so all is fine here
> 
>  - our video chip needs an external clock to work
>   => example: mt9m111 needs a clock from pxa camera interface to have its i2c
>   unit enabled
>   => the mt9m111 driver resume function is unusable, as pxa_camera is resumed
>   _after_ mt9m111, and thus mt9m111's i2c unit is not available at that moment
> 
>  - a working suspend/resume restores fully the video chip state
>   => restores width/height/bpp
>   => restores autoexposure, brightness, etc ...
>   => all that insures userland is not impacted by suspend/resume
> 
> So, the only way I see to have suspend/resume working is :
>  - modify soc_camera_ops to add suspend and resume functions
>  - add suspend and resume functions in each chip driver (mt9m001, mt9m111, ...)
>  - modify soc_camera.c (or pxa_camera.c ?) to call icd->ops->suspend() and
>  icd->ops->resume()
>  - modify pxa_camera.c (the patch I sent before)
> 
> Would you find that acceptable, or is there a better way ?

Ok, you're suggesting to add suspend() and resume() to 
soc_camera_bus_type, right? But are we sure that its resume will be called 
after both camera (so far i2c) and host (so far platform, can also be PCI 
or USB...) busses are resumed? If not, we might have to do something 
similar to scan_add_host() / scan_add_device() - accept signals from the 
host and the camera and when both are ready actually resume them...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Ok, you're suggesting to add suspend() and resume() to 
> soc_camera_bus_type, right? But are we sure that its resume will be called 
> after both camera (so far i2c) and host (so far platform, can also be PCI 
> or USB...) busses are resumed? If not, we might have to do something 
> similar to scan_add_host() / scan_add_device() - accept signals from the 
> host and the camera and when both are ready actually resume them...

As far as my comprehension goes for resume order :
 - mt9m111 is an i2c client, and so it will always be resumed after i2c bus
 driver
 - mt9m111 registers itself to the soc_camera bus, so soc_camera_bus will be
 resumed after mt9m111
 - pxa27-camera registers to soc_camera_host, so soc_camera_host will be resumed
 after pxa27-camera
 - I didn't check the link between soc_camera_host and soc_camera_bus, but if
 there is one, soc_camera bus is resumed last.

So, if I have it sorted out correctly, soc_camera_bus should hold the suspend()
and resume() functions, which will call icd->ops->suspend() and
icd->ops->resume().

I'm still investigating these points (under test ATM). It does work, but I still
haven't got the formal proof of the ordering ...

Would you by any chance have a little ascii art of all
camera/camera_host/camera_bus ... device graph ? A thing that could easily be
pushed into Documentation/video4linux/... ?

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Thu, 31 Jul 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Ok, you're suggesting to add suspend() and resume() to 
> > soc_camera_bus_type, right? But are we sure that its resume will be called 
> > after both camera (so far i2c) and host (so far platform, can also be PCI 
> > or USB...) busses are resumed? If not, we might have to do something 
> > similar to scan_add_host() / scan_add_device() - accept signals from the 
> > host and the camera and when both are ready actually resume them...
> 
> As far as my comprehension goes for resume order :
>  - mt9m111 is an i2c client, and so it will always be resumed after i2c bus
>  driver
>  - mt9m111 registers itself to the soc_camera bus, so soc_camera_bus will be
>  resumed after mt9m111
>  - pxa27-camera registers to soc_camera_host, so soc_camera_host will be resumed
>  after pxa27-camera
>  - I didn't check the link between soc_camera_host and soc_camera_bus, but if
>  there is one, soc_camera bus is resumed last.

I thought devices get woken up after busses they are on and after their 
parents, am I wrong? If I am right, the last one to be woken up were the 
camera device.

> So, if I have it sorted out correctly, soc_camera_bus should hold the suspend()
> and resume() functions, which will call icd->ops->suspend() and
> icd->ops->resume().
> 
> I'm still investigating these points (under test ATM). It does work, but I still
> haven't got the formal proof of the ordering ...
> 
> Would you by any chance have a little ascii art of all
> camera/camera_host/camera_bus ... device graph ? A thing that could easily be
> pushed into Documentation/video4linux/... ?

I started documenting the API, honestly:-) But ATM I have no time for that 
again... As for the device hierarchy, the device embedded into the camera 
device object (struct soc_camera_device) is registered on the soc-camera 
bus (soc_camera_bus_type), and has the device from the camera host object 
as parent. In case of the pxa-camera driver that device object in turn has 
the platform device as a parent. From my system:

# This is the camera device
~ ls -l /sys/bus/soc-camera/devices/
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 0-0 -> ../../../devices/platform/pxa27x-camera.0/camera_host0/0-0

# It is linked with the "control" link with the respective i2c device and 
# provides the video0 device
~ ls -l /sys/bus/soc-camera/devices/0-0/
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 bus -> ../../../../../bus/soc-camera
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 control -> ../../../../../class/i2c-adapter/i2c-0/0-0048
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 driver -> ../../../../../bus/soc-camera/drivers/camera
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 subsystem -> ../../../../../bus/soc-camera
-rw-r--r--    1 root     root         4096 Jan  1 00:01 uevent
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 video4linux:video0 -> ../../../../../class/video4linux/video0

# As a parent it has the camera host device
~ ls -l /sys/bus/platform/devices/pxa27x-camera.0/
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 bus -> ../../../bus/platform
drwxr-xr-x    3 root     root            0 Jan  1 00:01 camera_host0
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 driver -> ../../../bus/platform/drivers/pxa27x-camera
-r--r--r--    1 root     root         4096 Jan  1 00:01 modalias
lrwxrwxrwx    1 root     root            0 Jan  1 00:01 subsystem -> ../../../bus/platform
-rw-r--r--    1 root     root         4096 Jan  1 00:00 uevent

# And here's again the child of the camera host device
~ ls -l /sys/bus/platform/devices/pxa27x-camera.0/camera_host0/
drwxr-xr-x    2 root     root            0 Jan  1 00:01 0-0
-rw-r--r--    1 root     root         4096 Jan  1 00:01 uevent

HTH
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

Hi Robert

Hope you don't mind me resuming an almost 3 year old mail thread;) I'm 
referring to your patches to soc-camera core and pxa-camera, adding PM 
support to them. Below is your message again, explaining, why the standard 
pm hooks cannot be used to suspend and resume the camera host and the 
camera sensor. While trying to make soc-camera play nicer with the V4L2 
generic framework, I was trying to eliminate as many redundant pieces from 
soc-camera as possible and replace them with standard methods. This made 
me re-consider those your patches. Let's have a look at your 
argumentation:

On Thu, 31 Jul 2008, Guennadi Liakhovetski wrote:

> On Wed, 30 Jul 2008, Robert Jarzmik wrote:
> 
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> > 
> > >> >> For the camera part, by now, I'm using standard suspend/resume functions of the
> > >> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> > >> >> between the driver resume function and the availability of the I2C bus are not
> > >> >> properly chained. I'm still working on it.
> > >> >
> > >> > Yes, we have to clarify this too.
> > All right, I have my mind clarified, let's discuss now.
> > 
> > >>  - I cook up a clean suspend/resume (unless you did it first of course :)
> > Well, let's expose what we're facing here :
> >  - our video chip driver (ex: mt9m111) is an i2c driver
> >   => its resume function is called when i2c bus is resumed, so all is fine here
> > 
> >  - our video chip needs an external clock to work
> >   => example: mt9m111 needs a clock from pxa camera interface to have its i2c
> >   unit enabled
> >   => the mt9m111 driver resume function is unusable, as pxa_camera is resumed
> >   _after_ mt9m111, and thus mt9m111's i2c unit is not available at that moment
> > 
> >  - a working suspend/resume restores fully the video chip state
> >   => restores width/height/bpp
> >   => restores autoexposure, brightness, etc ...
> >   => all that insures userland is not impacted by suspend/resume
> > 
> > So, the only way I see to have suspend/resume working is :
> >  - modify soc_camera_ops to add suspend and resume functions
> >  - add suspend and resume functions in each chip driver (mt9m001, mt9m111, ...)
> >  - modify soc_camera.c (or pxa_camera.c ?) to call icd->ops->suspend() and
> >  icd->ops->resume()
> >  - modify pxa_camera.c (the patch I sent before)

So, we currently have 3 instances: soc-camera bus, i2c bus, and pxa-camera 
platform device driver. You say, i2c resumes as first, then at some point 
pxa-camera and soc-camera - in this or reverse order. This is why we 
cannoe use i2c-resume to bring the sensor up before pxa-camera has 
restored its master clock. So, currently we hook onto the soc-camera bus, 
which then calls pxa-camera's resume, which then restores camera host's 
state and resumes the sensor. Now, the question: wouldn't this also work, 
if we eliminate the soc-camera resume path? And instead just used 
pxa-camera resume method to bring up the sensor? Coule you please test the 
below patch?

Thanks
Guennadi

> > 
> > Would you find that acceptable, or is there a better way ?
> 
> Ok, you're suggesting to add suspend() and resume() to 
> soc_camera_bus_type, right? But are we sure that its resume will be called 
> after both camera (so far i2c) and host (so far platform, can also be PCI 
> or USB...) busses are resumed? If not, we might have to do something 
> similar to scan_add_host() / scan_add_device() - accept signals from the 
> host and the camera and when both are ready actually resume them...
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> 
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Subject: [PATCH] V4L: remove soc-camera PM, use camera host driver PM instead

Using soc-camera bus power management to suspend and resume video devices 
introduces a redundant indirection level. It can easily be removed by 
using camera host driver's own PM hooks.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index ebebed9..ec9e829 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -727,7 +727,7 @@ static const struct v4l2_queryctrl mt9m111_controls[] = {
 };
 
 static int mt9m111_resume(struct soc_camera_device *icd);
-static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state);
+static int mt9m111_suspend(struct soc_camera_device *icd);
 
 static struct soc_camera_ops mt9m111_ops = {
 	.suspend		= mt9m111_suspend,
@@ -901,7 +901,7 @@ static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
 	return ret;
 }
 
-static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state)
+static int mt9m111_suspend(struct soc_camera_device *icd)
 {
 	struct i2c_client *client = to_i2c_client(to_soc_camera_control(icd));
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index b42bfa5..9cbf7f8 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -1584,9 +1584,9 @@ static int pxa_camera_querycap(struct soc_camera_host *ici,
 	return 0;
 }
 
-static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
+static int pxa_camera_suspend(struct device *dev)
 {
-	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(dev);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	int i = 0, ret = 0;
 
@@ -1596,15 +1596,15 @@ static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
 	pcdev->save_cicr[i++] = __raw_readl(pcdev->base + CICR3);
 	pcdev->save_cicr[i++] = __raw_readl(pcdev->base + CICR4);
 
-	if ((pcdev->icd) && (pcdev->icd->ops->suspend))
-		ret = pcdev->icd->ops->suspend(pcdev->icd, state);
+	if (pcdev->icd && pcdev->icd->ops->suspend)
+		ret = pcdev->icd->ops->suspend(pcdev->icd);
 
 	return ret;
 }
 
-static int pxa_camera_resume(struct soc_camera_device *icd)
+static int pxa_camera_resume(struct device *dev)
 {
-	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(dev);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	int i = 0, ret = 0;
 
@@ -1618,7 +1618,7 @@ static int pxa_camera_resume(struct soc_camera_device *icd)
 	__raw_writel(pcdev->save_cicr[i++], pcdev->base + CICR3);
 	__raw_writel(pcdev->save_cicr[i++], pcdev->base + CICR4);
 
-	if ((pcdev->icd) && (pcdev->icd->ops->resume))
+	if (pcdev->icd && pcdev->icd->ops->resume)
 		ret = pcdev->icd->ops->resume(pcdev->icd);
 
 	/* Restart frame capture if active buffer exists */
@@ -1632,8 +1632,6 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.owner		= THIS_MODULE,
 	.add		= pxa_camera_add_device,
 	.remove		= pxa_camera_remove_device,
-	.suspend	= pxa_camera_suspend,
-	.resume		= pxa_camera_resume,
 	.set_crop	= pxa_camera_set_crop,
 	.get_formats	= pxa_camera_get_formats,
 	.put_formats	= pxa_camera_put_formats,
@@ -1818,9 +1816,15 @@ static int __devexit pxa_camera_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct dev_pm_ops pxa_camera_pm = {
+	.suspend	= pxa_camera_suspend,
+	.resume		= pxa_camera_resume,
+};
+
 static struct platform_driver pxa_camera_driver = {
 	.driver 	= {
 		.name	= PXA_CAM_DRV_NAME,
+		.pm	= &pxa_camera_pm,
 	},
 	.probe		= pxa_camera_probe,
 	.remove		= __devexit_p(pxa_camera_remove),
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 4e4d412..76fd30f 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -1216,36 +1216,10 @@ static int soc_camera_remove(struct device *dev)
 	return 0;
 }
 
-static int soc_camera_suspend(struct device *dev, pm_message_t state)
-{
-	struct soc_camera_device *icd = to_soc_camera_dev(dev);
-	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
-	int ret = 0;
-
-	if (ici->ops->suspend)
-		ret = ici->ops->suspend(icd, state);
-
-	return ret;
-}
-
-static int soc_camera_resume(struct device *dev)
-{
-	struct soc_camera_device *icd = to_soc_camera_dev(dev);
-	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
-	int ret = 0;
-
-	if (ici->ops->resume)
-		ret = ici->ops->resume(icd);
-
-	return ret;
-}
-
 struct bus_type soc_camera_bus_type = {
 	.name		= "soc-camera",
 	.probe		= soc_camera_probe,
 	.remove		= soc_camera_remove,
-	.suspend	= soc_camera_suspend,
-	.resume		= soc_camera_resume,
 };
 EXPORT_SYMBOL_GPL(soc_camera_bus_type);
 
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 238bd33..a256f74 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -66,8 +66,6 @@ struct soc_camera_host_ops {
 	struct module *owner;
 	int (*add)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
-	int (*suspend)(struct soc_camera_device *, pm_message_t);
-	int (*resume)(struct soc_camera_device *);
 	/*
 	 * .get_formats() is called for each client device format, but
 	 * .put_formats() is only called once. Further, if any of the calls to
@@ -207,7 +205,7 @@ struct soc_camera_format_xlate {
 };
 
 struct soc_camera_ops {
-	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*suspend)(struct soc_camera_device *);
 	int (*resume)(struct soc_camera_device *);
 	unsigned long (*query_bus_param)(struct soc_camera_device *);
 	int (*set_bus_param)(struct soc_camera_device *, unsigned long);

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

On 06/28/2011 03:47 PM, Guennadi Liakhovetski wrote:
> Hi Robert
>
> Hope you don't mind me resuming an almost 3 year old mail thread;) I'm
> referring to your patches to soc-camera core and pxa-camera, adding PM
> support to them. Below is your message again, explaining, why the standard
> pm hooks cannot be used to suspend and resume the camera host and the
> camera sensor. While trying to make soc-camera play nicer with the V4L2
> generic framework, I was trying to eliminate as many redundant pieces from
> soc-camera as possible and replace them with standard methods. This made
> me re-consider those your patches. Let's have a look at your
> argumentation:
>
> So, we currently have 3 instances: soc-camera bus, i2c bus, and pxa-camera
> platform device driver. You say, i2c resumes as first, then at some point
> pxa-camera and soc-camera - in this or reverse order. This is why we
> cannoe use i2c-resume to bring the sensor up before pxa-camera has
> restored its master clock. So, currently we hook onto the soc-camera bus,
> which then calls pxa-camera's resume, which then restores camera host's
> state and resumes the sensor. Now, the question: wouldn't this also work,
> if we eliminate the soc-camera resume path? And instead just used
> pxa-camera resume method to bring up the sensor? Coule you please test the
> below patch?

Tested and working, the resume works OK, which means your thinking is 
good :)

Cheers.

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Mon, 28 Jul 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > On Sun, 27 Jul 2008, Robert Jarzmik wrote:
> >
> > Yes, this is the difference. The sensor is attached to the camera host 
> > only on open. In fact, I am not sure, how video applications should behave 
> > during a suspend / resume cycle. If you suspend, while, say, recording 
> > from your camera, should you directly continue recording after a wake up? 
> > How do currect drivers implement this? Or, in general, for example with 
> > audio - if you suspend while listening to a stream over the net, or to a 
> > CD, or to a mp3-file on your local disk, should the sound resume after a 
> > wake up? I added linux-pm for some authoritative answers:-)
> AFAIK, on resume, sound streams continues. So the normal behaviour would be to
> continue video stream too (as done in ALSA). This supposes the whole video chip
> state is saved on suspend and restored on resume, of course.

Ok, that's what I also thought.

> > If you know how a v4l2 device should handle suspend/resume, or when we get 
> > some answers, let's try to do it completely-
> Of course. In a week or two, my mt9m111 driver will be ready for submission, and
> in the review process I'll post a submission for complete suspend/resume.
> 
> >> For the camera part, by now, I'm using standard suspend/resume functions of the
> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> >> between the driver resume function and the availability of the I2C bus are not
> >> properly chained. I'm still working on it.
> >
> > Yes, we have to clarify this too.
> Yes.
> 
> So, to sum up :
>  - I finish the mt9m111 driver
>  - I submit it
>  - I cook up a clean suspend/resume (unless you did it first of course :)

Good plan! You don't have to worry - I will not do this before you:-)

And, I am still waiting for the patch to move .power and .reset to 
camera-link (see my another post on the V4L ML today). With that patch in 
place your power-management would also become easier and more logical.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

> So, to sum up :
>  - I finish the mt9m111 driver
>  - I submit it
>  - I cook up a clean suspend/resume (unless you did it first of course :)

All right, I finished the pxa_camera part. The suspend/resume does work with a
opened video stream. The capture begins before the suspend and finished after
the resume.

I post the patch here attached for information. I'll submit later with the
complete suspend/resume serie. This is just for preliminary comments. Of course,
this patch superseeds the origin patch posted in this thread, which didn't work
for an opened video stream.

--
Robert

>From fb38f10c233a5b4e13f5ad42cf1c381ecc4215e9 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Sun, 27 Jul 2008 00:52:22 +0200
Subject: [PATCH] Fix suspend/resume of pxa_camera driver

PXA suspend switches off DMA core, which looses all context
of previously assigned descriptors. As pxa_camera driver
relies on DMA transfers, setup the lost descriptors on
resume and retrigger frame acquisition if needed.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |   49 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index efb2d19..f00844c 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -128,6 +128,8 @@ struct pxa_camera_dev {
 
 	struct pxa_buffer	*active;
 	struct pxa_dma_desc	*sg_tail[3];
+
+	u32			save_CICR[5];
 };
 
 static const char *pxa_cam_driver_description = "PXA_Camera";
@@ -1017,6 +1019,51 @@ static struct soc_camera_host pxa_soc_camera_host = {
 	.ops			= &pxa_soc_camera_host_ops,
 };
 
+static int pxa_camera_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
+	int i = 0;
+
+	pcdev->save_CICR[i++] = CICR0;
+	pcdev->save_CICR[i++] = CICR1;
+	pcdev->save_CICR[i++] = CICR2;
+	pcdev->save_CICR[i++] = CICR3;
+	pcdev->save_CICR[i++] = CICR4;
+
+	return 0;
+}
+
+static int pxa_camera_resume(struct platform_device *pdev)
+{
+	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
+	int i = 0;
+
+	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
+	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
+	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
+
+	CICR0 = pcdev->save_CICR[i++] & ~CICR0_ENB;
+	CICR1 = pcdev->save_CICR[i++];
+	CICR2 = pcdev->save_CICR[i++];
+	CICR3 = pcdev->save_CICR[i++];
+	CICR4 = pcdev->save_CICR[i++];
+
+	if ((pcdev->icd) && (pcdev->icd->ops->resume))
+		pcdev->icd->ops->resume(pcdev->icd);
+
+	/* Restart frame capture if active buffer exists */
+	if (pcdev->active) {
+		/* Reset the FIFOs */
+		CIFR |= CIFR_RESET_F;
+		/* Enable End-Of-Frame Interrupt */
+		CICR0 &= ~CICR0_EOFM;
+		/* Restart the Capture Interface */
+		CICR0 |= CICR0_ENB;
+	}
+
+	return 0;
+}
+
 static int pxa_camera_probe(struct platform_device *pdev)
 {
 	struct pxa_camera_dev *pcdev;
@@ -1188,6 +1235,8 @@ static struct platform_driver pxa_camera_driver = {
 	},
 	.probe		= pxa_camera_probe,
 	.remove		= __exit_p(pxa_camera_remove),
+	.suspend	= pxa_camera_suspend,
+	.resume		= pxa_camera_resume,
 };
 
 
-- 
1.5.5.3

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Thu, 31 Jul 2008, Robert Jarzmik wrote:

> > So, to sum up :
> >  - I finish the mt9m111 driver
> >  - I submit it
> >  - I cook up a clean suspend/resume (unless you did it first of course :)
> 
> All right, I finished the pxa_camera part. The suspend/resume does work with a
> opened video stream. The capture begins before the suspend and finished after
> the resume.
> 
> I post the patch here attached for information. I'll submit later with the
> complete suspend/resume serie. This is just for preliminary comments. Of course,
> this patch superseeds the origin patch posted in this thread, which didn't work
> for an opened video stream.

Ok, some preliminary comments.

> >From fb38f10c233a5b4e13f5ad42cf1c381ecc4215e9 Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Sun, 27 Jul 2008 00:52:22 +0200
> Subject: [PATCH] Fix suspend/resume of pxa_camera driver
> 
> PXA suspend switches off DMA core, which looses all context

I think, you mean "loses" - with one "o".

> of previously assigned descriptors. As pxa_camera driver
> relies on DMA transfers, setup the lost descriptors on
> resume and retrigger frame acquisition if needed.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |   49 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index efb2d19..f00844c 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -128,6 +128,8 @@ struct pxa_camera_dev {
>  
>  	struct pxa_buffer	*active;
>  	struct pxa_dma_desc	*sg_tail[3];
> +
> +	u32			save_CICR[5];

I think, it would look better in  plane lowercase, just name it "cicr" or 
even "save_cicr" if you prefer.

>  };
>  
>  static const char *pxa_cam_driver_description = "PXA_Camera";
> @@ -1017,6 +1019,51 @@ static struct soc_camera_host pxa_soc_camera_host = {
>  	.ops			= &pxa_soc_camera_host_ops,
>  };
>  
> +static int pxa_camera_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	pcdev->save_CICR[i++] = CICR0;
> +	pcdev->save_CICR[i++] = CICR1;
> +	pcdev->save_CICR[i++] = CICR2;
> +	pcdev->save_CICR[i++] = CICR3;
> +	pcdev->save_CICR[i++] = CICR4;
> +
> +	return 0;
> +}
> +
> +static int pxa_camera_resume(struct platform_device *pdev)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
> +	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
> +	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
> +
> +	CICR0 = pcdev->save_CICR[i++] & ~CICR0_ENB;
> +	CICR1 = pcdev->save_CICR[i++];
> +	CICR2 = pcdev->save_CICR[i++];
> +	CICR3 = pcdev->save_CICR[i++];
> +	CICR4 = pcdev->save_CICR[i++];
> +
> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
> +		pcdev->icd->ops->resume(pcdev->icd);

Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
quite convinced by your argumentation in a previous email regarding in 
which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
far as I understood, devices get resumed simply in the order they got 
registered. This does guarantee, that children are resumed after parents, 
but otherwise there are no guarantees. I guess, you load pxa-camera after 
i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
almost prepared to bet, your resume will not work then:-)

I think, I have an idea. Our soc_camera_device is registered the last - it 
is registered after the respective i2c device (at least in all drivers so 
far, and future drivers better keep it this way), and after the camera 
host it is on (see soc_camera.c::device_register_link()). So, all we have 
to do is add a suspend and a resume to soc_camera_bus_type and to 
soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
from soc_camera_bus_type .resume and .suspend. Now this should work, what 
do you think?

> +
> +	/* Restart frame capture if active buffer exists */
> +	if (pcdev->active) {
> +		/* Reset the FIFOs */
> +		CIFR |= CIFR_RESET_F;
> +		/* Enable End-Of-Frame Interrupt */
> +		CICR0 &= ~CICR0_EOFM;
> +		/* Restart the Capture Interface */
> +		CICR0 |= CICR0_ENB;
> +	}
> +
> +	return 0;
> +}
> +
>  static int pxa_camera_probe(struct platform_device *pdev)
>  {
>  	struct pxa_camera_dev *pcdev;
> @@ -1188,6 +1235,8 @@ static struct platform_driver pxa_camera_driver = {
>  	},
>  	.probe		= pxa_camera_probe,
>  	.remove		= __exit_p(pxa_camera_remove),
> +	.suspend	= pxa_camera_suspend,
> +	.resume		= pxa_camera_resume,
>  };

If we agree on the above just move these two to pxa_soc_camera_host_ops.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Thu, 31 Jul 2008, Robert Jarzmik wrote:

> > So, to sum up :
> >  - I finish the mt9m111 driver
> >  - I submit it
> >  - I cook up a clean suspend/resume (unless you did it first of course :)
> 
> All right, I finished the pxa_camera part. The suspend/resume does work with a
> opened video stream. The capture begins before the suspend and finished after
> the resume.
> 
> I post the patch here attached for information. I'll submit later with the
> complete suspend/resume serie. This is just for preliminary comments. Of course,
> this patch superseeds the origin patch posted in this thread, which didn't work
> for an opened video stream.

Ok, some preliminary comments.

> >From fb38f10c233a5b4e13f5ad42cf1c381ecc4215e9 Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Sun, 27 Jul 2008 00:52:22 +0200
> Subject: [PATCH] Fix suspend/resume of pxa_camera driver
> 
> PXA suspend switches off DMA core, which looses all context

I think, you mean "loses" - with one "o".

> of previously assigned descriptors. As pxa_camera driver
> relies on DMA transfers, setup the lost descriptors on
> resume and retrigger frame acquisition if needed.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |   49 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index efb2d19..f00844c 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -128,6 +128,8 @@ struct pxa_camera_dev {
>  
>  	struct pxa_buffer	*active;
>  	struct pxa_dma_desc	*sg_tail[3];
> +
> +	u32			save_CICR[5];

I think, it would look better in  plane lowercase, just name it "cicr" or 
even "save_cicr" if you prefer.

>  };
>  
>  static const char *pxa_cam_driver_description = "PXA_Camera";
> @@ -1017,6 +1019,51 @@ static struct soc_camera_host pxa_soc_camera_host = {
>  	.ops			= &pxa_soc_camera_host_ops,
>  };
>  
> +static int pxa_camera_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	pcdev->save_CICR[i++] = CICR0;
> +	pcdev->save_CICR[i++] = CICR1;
> +	pcdev->save_CICR[i++] = CICR2;
> +	pcdev->save_CICR[i++] = CICR3;
> +	pcdev->save_CICR[i++] = CICR4;
> +
> +	return 0;
> +}
> +
> +static int pxa_camera_resume(struct platform_device *pdev)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
> +	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
> +	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
> +
> +	CICR0 = pcdev->save_CICR[i++] & ~CICR0_ENB;
> +	CICR1 = pcdev->save_CICR[i++];
> +	CICR2 = pcdev->save_CICR[i++];
> +	CICR3 = pcdev->save_CICR[i++];
> +	CICR4 = pcdev->save_CICR[i++];
> +
> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
> +		pcdev->icd->ops->resume(pcdev->icd);

Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
quite convinced by your argumentation in a previous email regarding in 
which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
far as I understood, devices get resumed simply in the order they got 
registered. This does guarantee, that children are resumed after parents, 
but otherwise there are no guarantees. I guess, you load pxa-camera after 
i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
almost prepared to bet, your resume will not work then:-)

I think, I have an idea. Our soc_camera_device is registered the last - it 
is registered after the respective i2c device (at least in all drivers so 
far, and future drivers better keep it this way), and after the camera 
host it is on (see soc_camera.c::device_register_link()). So, all we have 
to do is add a suspend and a resume to soc_camera_bus_type and to 
soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
from soc_camera_bus_type .resume and .suspend. Now this should work, what 
do you think?

> +
> +	/* Restart frame capture if active buffer exists */
> +	if (pcdev->active) {
> +		/* Reset the FIFOs */
> +		CIFR |= CIFR_RESET_F;
> +		/* Enable End-Of-Frame Interrupt */
> +		CICR0 &= ~CICR0_EOFM;
> +		/* Restart the Capture Interface */
> +		CICR0 |= CICR0_ENB;
> +	}
> +
> +	return 0;
> +}
> +
>  static int pxa_camera_probe(struct platform_device *pdev)
>  {
>  	struct pxa_camera_dev *pcdev;
> @@ -1188,6 +1235,8 @@ static struct platform_driver pxa_camera_driver = {
>  	},
>  	.probe		= pxa_camera_probe,
>  	.remove		= __exit_p(pxa_camera_remove),
> +	.suspend	= pxa_camera_suspend,
> +	.resume		= pxa_camera_resume,
>  };

If we agree on the above just move these two to pxa_soc_camera_host_ops.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, you mean "loses" - with one "o".
Typo. Will be fixed.

> I think, it would look better in  plane lowercase, just name it "cicr" or 
> even "save_cicr" if you prefer.
OK. Will be fixed.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		pcdev->icd->ops->resume(pcdev->icd);
>
> Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
> quite convinced by your argumentation in a previous email regarding in 
> which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
> far as I understood, devices get resumed simply in the order they got 
> registered. This does guarantee, that children are resumed after parents, 
> but otherwise there are no guarantees. I guess, you load pxa-camera after 
> i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
> almost prepared to bet, your resume will not work then:-)

And you're probably right. I tested ... The order is bus before devices, parents
before childs, but I see no link between i2c-pxa and pxa-camera.

Yesterday, I moved that 2 lines into soc_camera_resume(), and
soc_camera_suspend() was added too. I came to the same conclusion, which is that
we can only be sure of the order if called from soc_camera_bus.

> I think, I have an idea. Our soc_camera_device is registered the last - it 
> is registered after the respective i2c device (at least in all drivers so 
> far, and future drivers better keep it this way), and after the camera 
> host it is on (see soc_camera.c::device_register_link()). So, all we have 
> to do is add a suspend and a resume to soc_camera_bus_type and to 
> soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
> from soc_camera_bus_type .resume and .suspend. Now this should work, what 
> do you think?

Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
brings me to another question, in which order :
 a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
 b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()

For me, the only working order can be (a), because I need
soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
camera. Do you think the same ?

> If we agree on the above just move these two to pxa_soc_camera_host_ops.
Agreed for me.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Fri, 1 Aug 2008, Robert Jarzmik wrote:

> Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
> call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
> brings me to another question, in which order :
>  a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
>  b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()
> 
> For me, the only working order can be (a), because I need
> soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
> usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
> camera. Do you think the same ?

On resume we have to do this exactly as you have done it in your last 
patch: first restore general parameters on the host, then resume the 
camera, and then continue with the FIFOs and activating the DMA. So, I 
think, we have no choice but to only call host's resume, passing it the 
camera device as a parameter, and let it decide when it wants to resume 
the camera. Similar on suspend. This will also be consistent with how 
pxa_camera_add_device() calls icd->ops->init(icd) and 
pxa_camera_remove_device() calls icd->ops->release(icd).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Fri, 1 Aug 2008, Robert Jarzmik wrote:

> Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
> call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
> brings me to another question, in which order :
>  a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
>  b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()
> 
> For me, the only working order can be (a), because I need
> soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
> usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
> camera. Do you think the same ?

On resume we have to do this exactly as you have done it in your last 
patch: first restore general parameters on the host, then resume the 
camera, and then continue with the FIFOs and activating the DMA. So, I 
think, we have no choice but to only call host's resume, passing it the 
camera device as a parameter, and let it decide when it wants to resume 
the camera. Similar on suspend. This will also be consistent with how 
pxa_camera_add_device() calls icd->ops->init(icd) and 
pxa_camera_remove_device() calls icd->ops->release(icd).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

All right, let's try this serie. It includes as much as I could gather from your
previous comments. I'm expecting another set of comments :)

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] Add suspend/resume capabilities to soc_camera.

[Robert Jarzmik]

Add suspend/resume hooks to call soc operation specific
suspend and resume functions. This ensures the camera
chip has been previously resumed, as well as the camera
bus.
These hooks in camera chip drivers should save/restore
chip context between suspend and resume time.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/soc_camera.c |   26 ++++++++++++++++++++++++++
 include/media/soc_camera.h       |    5 +++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index a1b9244..dc85182 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -751,10 +751,36 @@ static int soc_camera_remove(struct device *dev)
 	return 0;
 }
 
+static int soc_camera_suspend(struct device *dev, pm_message_t state)
+{
+	struct soc_camera_device *icd = to_soc_camera_dev(dev);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int ret = 0;
+
+	if (ici->ops->suspend)
+		ret = ici->ops->suspend(icd, state);
+
+	return ret;
+}
+
+static int soc_camera_resume(struct device *dev)
+{
+	struct soc_camera_device *icd = to_soc_camera_dev(dev);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	int ret = 0;
+
+	if (ici->ops->resume)
+		ret = ici->ops->resume(icd);
+
+	return ret;
+}
+
 static struct bus_type soc_camera_bus_type = {
 	.name		= "soc-camera",
 	.probe		= soc_camera_probe,
 	.remove		= soc_camera_remove,
+	.suspend	= soc_camera_suspend,
+	.resume		= soc_camera_resume,
 };
 
 static struct device_driver ic_drv = {
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 6a8c8be..1984427 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -14,6 +14,7 @@
 
 #include <linux/videodev2.h>
 #include <media/videobuf-dma-sg.h>
+#include <linux/pm.h>
 
 struct soc_camera_device {
 	struct list_head list;
@@ -66,6 +67,8 @@ struct soc_camera_host_ops {
 	struct module *owner;
 	int (*add)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
+	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*resume)(struct soc_camera_device *);
 	int (*set_fmt_cap)(struct soc_camera_device *, __u32,
 			   struct v4l2_rect *);
 	int (*try_fmt_cap)(struct soc_camera_device *, struct v4l2_format *);
@@ -114,6 +117,8 @@ struct soc_camera_ops {
 	struct module *owner;
 	int (*probe)(struct soc_camera_device *);
 	void (*remove)(struct soc_camera_device *);
+	int (*suspend)(struct soc_camera_device *, pm_message_t state);
+	int (*resume)(struct soc_camera_device *);
 	int (*init)(struct soc_camera_device *);
 	int (*release)(struct soc_camera_device *);
 	int (*start_capture)(struct soc_camera_device *);
-- 
1.5.5.3

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

PXA suspend switches off DMA core, which loses all context
of previously assigned descriptors. As pxa_camera driver
relies on DMA transfers, setup the lost descriptors on
resume and retrigger frame acquisition if needed.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |   56 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 7cc8e9b..1ac8422 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -127,6 +127,8 @@ struct pxa_camera_dev {
 
 	struct pxa_buffer	*active;
 	struct pxa_dma_desc	*sg_tail[3];
+
+	u32			save_cicr[5];
 };
 
 static const char *pxa_cam_driver_description = "PXA_Camera";
@@ -992,10 +994,64 @@ static spinlock_t *pxa_camera_spinlock_alloc(struct soc_camera_file *icf)
 	return &pcdev->lock;
 }
 
+static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
+{
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+	int i = 0, ret = 0;
+
+	pcdev->save_cicr[i++] = CICR0;
+	pcdev->save_cicr[i++] = CICR1;
+	pcdev->save_cicr[i++] = CICR2;
+	pcdev->save_cicr[i++] = CICR3;
+	pcdev->save_cicr[i++] = CICR4;
+
+	if ((pcdev->icd) && (pcdev->icd->ops->resume))
+		ret = pcdev->icd->ops->resume(pcdev->icd);
+
+	return ret;
+}
+
+static int pxa_camera_resume(struct soc_camera_device *icd)
+{
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+	int i = 0, ret = 0;
+
+	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
+	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
+	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
+
+	CICR0 = pcdev->save_cicr[i++] & ~CICR0_ENB;
+	CICR1 = pcdev->save_cicr[i++];
+	CICR2 = pcdev->save_cicr[i++];
+	CICR3 = pcdev->save_cicr[i++];
+	CICR4 = pcdev->save_cicr[i++];
+
+	if ((pcdev->icd) && (pcdev->icd->ops->resume))
+		ret = pcdev->icd->ops->resume(pcdev->icd);
+
+	/* Restart frame capture if active buffer exists */
+	if (!ret && pcdev->active) {
+		/* Reset the FIFOs */
+		CIFR |= CIFR_RESET_F;
+		/* Enable End-Of-Frame Interrupt */
+		CICR0 &= ~CICR0_EOFM;
+		/* Restart the Capture Interface */
+		CICR0 |= CICR0_ENB;
+	}
+
+	return ret;
+}
+
 static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.owner		= THIS_MODULE,
 	.add		= pxa_camera_add_device,
 	.remove		= pxa_camera_remove_device,
+	.suspend	= pxa_camera_suspend,
+	.resume		= pxa_camera_resume,
 	.set_fmt_cap	= pxa_camera_set_fmt_cap,
 	.try_fmt_cap	= pxa_camera_try_fmt_cap,
 	.reqbufs	= pxa_camera_reqbufs,
-- 
1.5.5.3

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sat, 2 Aug 2008, Robert Jarzmik wrote:

> PXA suspend switches off DMA core, which loses all context
> of previously assigned descriptors. As pxa_camera driver
> relies on DMA transfers, setup the lost descriptors on
> resume and retrigger frame acquisition if needed.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

_Conditionally_ applied: I changed the subject to "Add suspend/resume 
to...", upgraded to the current Linus' top-of-tree, and, most importantly, 
changed this:

> +static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
> +{
> +	struct soc_camera_host *ici =
> +		to_soc_camera_host(icd->dev.parent);
> +	struct pxa_camera_dev *pcdev = ici->priv;
> +	int i = 0, ret = 0;
> +
> +	pcdev->save_cicr[i++] = CICR0;
> +	pcdev->save_cicr[i++] = CICR1;
> +	pcdev->save_cicr[i++] = CICR2;
> +	pcdev->save_cicr[i++] = CICR3;
> +	pcdev->save_cicr[i++] = CICR4;
> +
> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
> +		ret = pcdev->icd->ops->resume(pcdev->icd);

To 

+	if ((pcdev->icd) && (pcdev->icd->ops->suspend))
+		ret = pcdev->icd->ops->suspend(pcdev->icd);

Which I assume was a typo. Please, test these patches with this my change, 
and confirm they are ok now. I'll push both of them upstream then.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sat, 2 Aug 2008, Robert Jarzmik wrote:
>
>> PXA suspend switches off DMA core, which loses all context
>> of previously assigned descriptors. As pxa_camera driver
>> relies on DMA transfers, setup the lost descriptors on
>> resume and retrigger frame acquisition if needed.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> _Conditionally_ applied: I changed the subject to "Add suspend/resume 
> to...", upgraded to the current Linus' top-of-tree, and, most importantly, 
> changed this:
OK.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		ret = pcdev->icd->ops->resume(pcdev->icd);
>
> To 
>
>
> Which I assume was a typo. Please, test these patches with this my change, 
> and confirm they are ok now. I'll push both of them upstream then.

To be exact: wrong copy/paste. I'm sorry not have spotted this, I have no
suspend function in mt9m111, I only used the resume one to restore the state ...

And the two lines should be :
+	if ((pcdev->icd) && (pcdev->icd->ops->suspend))
+		ret = pcdev->icd->ops->suspend(pcdev->icd, state);
                                                           ^
                                                           compile error without

Apart from that, I tested, and it's OK.

Do you have an exported git tree I can sync with ?

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sat, 2 Aug 2008, Robert Jarzmik wrote:

> To be exact: wrong copy/paste. I'm sorry not have spotted this, I have no
> suspend function in mt9m111, I only used the resume one to restore the state ...
> 
> And the two lines should be :
> +	if ((pcdev->icd) && (pcdev->icd->ops->suspend))
> +		ret = pcdev->icd->ops->suspend(pcdev->icd, state);
>                                                            ^
>                                                            compile error without
> 
> Apart from that, I tested, and it's OK.

Right:-) Hope, we get it right this time. A pull request just went out to 
the v4l-dvb-maintainer list.

> Do you have an exported git tree I can sync with ?

No, sorry, just a mercurial tree:

http://linuxtv.org/hg/~gliakhovetski/v4l-dvb

After Mauro pulls it, it will at some point go into his git-tree on 
kernel.org:

http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git;a=summary

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> After Mauro pulls it, it will at some point go into his git-tree on 
> kernel.org:
> http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git;a=summary
Excellent. That settles that serie.
Thanks for your review.

--
Robert

PS: Expect shortly another, the mt9m111 driver itself ...

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Add suspend/resume capabilities to soc_camera.

[Guennadi Liakhovetski]

On Sat, 2 Aug 2008, Robert Jarzmik wrote:

> Add suspend/resume hooks to call soc operation specific
> suspend and resume functions. This ensures the camera
> chip has been previously resumed, as well as the camera
> bus.
> These hooks in camera chip drivers should save/restore
> chip context between suspend and resume time.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Applied, thanks
Guennadi

> ---
>  drivers/media/video/soc_camera.c |   26 ++++++++++++++++++++++++++
>  include/media/soc_camera.h       |    5 +++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index a1b9244..dc85182 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -751,10 +751,36 @@ static int soc_camera_remove(struct device *dev)
>  	return 0;
>  }
>  
> +static int soc_camera_suspend(struct device *dev, pm_message_t state)
> +{
> +	struct soc_camera_device *icd = to_soc_camera_dev(dev);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	int ret = 0;
> +
> +	if (ici->ops->suspend)
> +		ret = ici->ops->suspend(icd, state);
> +
> +	return ret;
> +}
> +
> +static int soc_camera_resume(struct device *dev)
> +{
> +	struct soc_camera_device *icd = to_soc_camera_dev(dev);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	int ret = 0;
> +
> +	if (ici->ops->resume)
> +		ret = ici->ops->resume(icd);
> +
> +	return ret;
> +}
> +
>  static struct bus_type soc_camera_bus_type = {
>  	.name		= "soc-camera",
>  	.probe		= soc_camera_probe,
>  	.remove		= soc_camera_remove,
> +	.suspend	= soc_camera_suspend,
> +	.resume		= soc_camera_resume,
>  };
>  
>  static struct device_driver ic_drv = {
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 6a8c8be..1984427 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -14,6 +14,7 @@
>  
>  #include <linux/videodev2.h>
>  #include <media/videobuf-dma-sg.h>
> +#include <linux/pm.h>
>  
>  struct soc_camera_device {
>  	struct list_head list;
> @@ -66,6 +67,8 @@ struct soc_camera_host_ops {
>  	struct module *owner;
>  	int (*add)(struct soc_camera_device *);
>  	void (*remove)(struct soc_camera_device *);
> +	int (*suspend)(struct soc_camera_device *, pm_message_t state);
> +	int (*resume)(struct soc_camera_device *);
>  	int (*set_fmt_cap)(struct soc_camera_device *, __u32,
>  			   struct v4l2_rect *);
>  	int (*try_fmt_cap)(struct soc_camera_device *, struct v4l2_format *);
> @@ -114,6 +117,8 @@ struct soc_camera_ops {
>  	struct module *owner;
>  	int (*probe)(struct soc_camera_device *);
>  	void (*remove)(struct soc_camera_device *);
> +	int (*suspend)(struct soc_camera_device *, pm_message_t state);
> +	int (*resume)(struct soc_camera_device *);
>  	int (*init)(struct soc_camera_device *);
>  	int (*release)(struct soc_camera_device *);
>  	int (*start_capture)(struct soc_camera_device *);
> -- 
> 1.5.5.3
> 
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, you mean "loses" - with one "o".
Typo. Will be fixed.

> I think, it would look better in  plane lowercase, just name it "cicr" or 
> even "save_cicr" if you prefer.
OK. Will be fixed.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		pcdev->icd->ops->resume(pcdev->icd);
>
> Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
> quite convinced by your argumentation in a previous email regarding in 
> which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
> far as I understood, devices get resumed simply in the order they got 
> registered. This does guarantee, that children are resumed after parents, 
> but otherwise there are no guarantees. I guess, you load pxa-camera after 
> i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
> almost prepared to bet, your resume will not work then:-)

And you're probably right. I tested ... The order is bus before devices, parents
before childs, but I see no link between i2c-pxa and pxa-camera.

Yesterday, I moved that 2 lines into soc_camera_resume(), and
soc_camera_suspend() was added too. I came to the same conclusion, which is that
we can only be sure of the order if called from soc_camera_bus.

> I think, I have an idea. Our soc_camera_device is registered the last - it 
> is registered after the respective i2c device (at least in all drivers so 
> far, and future drivers better keep it this way), and after the camera 
> host it is on (see soc_camera.c::device_register_link()). So, all we have 
> to do is add a suspend and a resume to soc_camera_bus_type and to 
> soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
> from soc_camera_bus_type .resume and .suspend. Now this should work, what 
> do you think?

Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
brings me to another question, in which order :
 a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
 b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()

For me, the only working order can be (a), because I need
soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
camera. Do you think the same ?

> If we agree on the above just move these two to pxa_soc_camera_host_ops.
Agreed for me.

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, you mean "loses" - with one "o".
Typo. Will be fixed.

> I think, it would look better in  plane lowercase, just name it "cicr" or 
> even "save_cicr" if you prefer.
OK. Will be fixed.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		pcdev->icd->ops->resume(pcdev->icd);
>
> Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
> quite convinced by your argumentation in a previous email regarding in 
> which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
> far as I understood, devices get resumed simply in the order they got 
> registered. This does guarantee, that children are resumed after parents, 
> but otherwise there are no guarantees. I guess, you load pxa-camera after 
> i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
> almost prepared to bet, your resume will not work then:-)

And you're probably right. I tested ... The order is bus before devices, parents
before childs, but I see no link between i2c-pxa and pxa-camera.

Yesterday, I moved that 2 lines into soc_camera_resume(), and
soc_camera_suspend() was added too. I came to the same conclusion, which is that
we can only be sure of the order if called from soc_camera_bus.

> I think, I have an idea. Our soc_camera_device is registered the last - it 
> is registered after the respective i2c device (at least in all drivers so 
> far, and future drivers better keep it this way), and after the camera 
> host it is on (see soc_camera.c::device_register_link()). So, all we have 
> to do is add a suspend and a resume to soc_camera_bus_type and to 
> soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
> from soc_camera_bus_type .resume and .suspend. Now this should work, what 
> do you think?

Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
brings me to another question, in which order :
 a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
 b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()

For me, the only working order can be (a), because I need
soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
camera. Do you think the same ?

> If we agree on the above just move these two to pxa_soc_camera_host_ops.
Agreed for me.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 27 Jul 2008, Robert Jarzmik wrote:
>
> Yes, this is the difference. The sensor is attached to the camera host 
> only on open. In fact, I am not sure, how video applications should behave 
> during a suspend / resume cycle. If you suspend, while, say, recording 
> from your camera, should you directly continue recording after a wake up? 
> How do currect drivers implement this? Or, in general, for example with 
> audio - if you suspend while listening to a stream over the net, or to a 
> CD, or to a mp3-file on your local disk, should the sound resume after a 
> wake up? I added linux-pm for some authoritative answers:-)
AFAIK, on resume, sound streams continues. So the normal behaviour would be to
continue video stream too (as done in ALSA). This supposes the whole video chip
state is saved on suspend and restored on resume, of course.

> If you know how a v4l2 device should handle suspend/resume, or when we get 
> some answers, let's try to do it completely-
Of course. In a week or two, my mt9m111 driver will be ready for submission, and
in the review process I'll post a submission for complete suspend/resume.

>> For the camera part, by now, I'm using standard suspend/resume functions of the
>> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
>> between the driver resume function and the availability of the I2C bus are not
>> properly chained. I'm still working on it.
>
> Yes, we have to clarify this too.
Yes.

So, to sum up :
 - I finish the mt9m111 driver
 - I submit it
 - I cook up a clean suspend/resume (unless you did it first of course :)

--
Robert