[PATCH 0/3] Some additional az6007 cleanup patches

[PATCH 0/3] Some additional az6007 cleanup patches

[Mauro Carvalho Chehab]

Those are mostly cleanup patches. With regards to suspend/resume,
this is not working properly yet. I suspect that it is due to the lack
of dvb-usb-v2 support for reset_resume. So, document it.

Mauro Carvalho Chehab (3):
  [media] az6007: rename "st" to "state" at az6007_power_ctrl()
  [media] az6007: make all functions static
  [media] az6007: handle CI during suspend/resume

 drivers/media/dvb/dvb-usb-v2/az6007.c | 37 +++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

-- 
1.7.11.2

[PATCH 1/3] [media] az6007: rename "st" to "state" at az6007_power_ctrl()

[Mauro Carvalho Chehab]

On all other parts, this var is called state. So, use the same
name here, to be consistent.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/dvb/dvb-usb-v2/az6007.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb-v2/az6007.c b/drivers/media/dvb/dvb-usb-v2/az6007.c
index 4671eaa..bb7f61d 100644
--- a/drivers/media/dvb/dvb-usb-v2/az6007.c
+++ b/drivers/media/dvb/dvb-usb-v2/az6007.c
@@ -637,13 +637,13 @@ static int az6007_tuner_attach(struct dvb_usb_adapter *adap)
 
 int az6007_power_ctrl(struct dvb_usb_device *d, int onoff)
 {
-	struct az6007_device_state *st = d_to_priv(d);
+	struct az6007_device_state *state = d_to_priv(d);
 	int ret;
 
 	pr_debug("%s()\n", __func__);
 
-	if (!st->warm) {
-		mutex_init(&st->mutex);
+	if (!state->warm) {
+		mutex_init(&state->mutex);
 
 		ret = az6007_write(d, AZ6007_POWER, 0, 2, NULL, 0);
 		if (ret < 0)
@@ -674,7 +674,7 @@ int az6007_power_ctrl(struct dvb_usb_device *d, int onoff)
 		if (ret < 0)
 			return ret;
 
-		st->warm = true;
+		state->warm = true;
 
 		return 0;
 	}
-- 
1.7.11.2

[PATCH 2/3] [media] az6007: make all functions static

[Mauro Carvalho Chehab]

There's no reason why those functions shouldn't be static.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/dvb/dvb-usb-v2/az6007.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb-v2/az6007.c b/drivers/media/dvb/dvb-usb-v2/az6007.c
index bb7f61d..4a0ee64 100644
--- a/drivers/media/dvb/dvb-usb-v2/az6007.c
+++ b/drivers/media/dvb/dvb-usb-v2/az6007.c
@@ -635,7 +635,7 @@ static int az6007_tuner_attach(struct dvb_usb_adapter *adap)
 	return 0;
 }
 
-int az6007_power_ctrl(struct dvb_usb_device *d, int onoff)
+static int az6007_power_ctrl(struct dvb_usb_device *d, int onoff)
 {
 	struct az6007_device_state *state = d_to_priv(d);
 	int ret;
@@ -784,7 +784,7 @@ static struct i2c_algorithm az6007_i2c_algo = {
 	.functionality = az6007_i2c_func,
 };
 
-int az6007_identify_state(struct dvb_usb_device *d, const char **name)
+static int az6007_identify_state(struct dvb_usb_device *d, const char **name)
 {
 	int ret;
 	u8 *mac;
-- 
1.7.11.2

[PATCH 3/3] [media] az6007: handle CI during suspend/resume

[Mauro Carvalho Chehab]

The dvb-usb-v2 core doesn't know anything about CI. So, the
driver needs to handle it by hand. This patch stops CI just
before stopping URB's/RC, and restarts it before URB/RC start.

It should be noticed that suspend/resume is not yet working properly,
as the PM model requires the implementation of reset_resume:
	dvb_usb_az6007 1-6:1.0: no reset_resume for driver dvb_usb_az6007?
But this is not implemented there at dvb-usb-v2 yet.

Cc: Antti Palosaari <crope@iki.fi>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/dvb/dvb-usb-v2/az6007.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb-v2/az6007.c b/drivers/media/dvb/dvb-usb-v2/az6007.c
index 4a0ee64..420cb62 100644
--- a/drivers/media/dvb/dvb-usb-v2/az6007.c
+++ b/drivers/media/dvb/dvb-usb-v2/az6007.c
@@ -876,16 +876,37 @@ static struct usb_device_id az6007_usb_table[] = {
 
 MODULE_DEVICE_TABLE(usb, az6007_usb_table);
 
+static int az6007_suspend(struct usb_interface *intf, pm_message_t msg)
+{
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+
+	az6007_ci_uninit(d);
+	return dvb_usbv2_suspend(intf, msg);
+}
+
+static int az6007_resume(struct usb_interface *intf)
+{
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+	struct dvb_usb_adapter *adap = &d->adapter[0];
+
+	az6007_ci_init(adap);
+	return dvb_usbv2_resume(intf);
+}
+
 /* usb specific object needed to register this driver with the usb subsystem */
 static struct usb_driver az6007_usb_driver = {
 	.name		= KBUILD_MODNAME,
 	.id_table	= az6007_usb_table,
 	.probe		= dvb_usbv2_probe,
 	.disconnect	= az6007_usb_disconnect,
-	.suspend	= dvb_usbv2_suspend,
-	.resume		= dvb_usbv2_resume,
 	.no_dynamic_id	= 1,
 	.soft_unbind	= 1,
+	/*
+	 * FIXME: need to implement reset_resume, likely with
+	 * dvb-usb-v2 core support
+	 */
+	.suspend	= az6007_suspend,
+	.resume		= az6007_resume,
 };
 
 module_usb_driver(az6007_usb_driver);
-- 
1.7.11.2

Re: [PATCH 3/3] [media] az6007: handle CI during suspend/resume

[Antti Palosaari]

On 08/05/2012 08:44 PM, Mauro Carvalho Chehab wrote:
> The dvb-usb-v2 core doesn't know anything about CI. So, the
> driver needs to handle it by hand. This patch stops CI just
> before stopping URB's/RC, and restarts it before URB/RC start.
>
> It should be noticed that suspend/resume is not yet working properly,
> as the PM model requires the implementation of reset_resume:
> 	dvb_usb_az6007 1-6:1.0: no reset_resume for driver dvb_usb_az6007?
> But this is not implemented there at dvb-usb-v2 yet.

That is true, but it is coming:
http://blog.palosaari.fi/2012/07/dvb-power-management-on-suspend.html
http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb_core3

At the time I added initial suspend/resume support for dvb-usb-v2 I left 
those out purposely as I saw some study and changes are needed for 
DVB-core/frontend.

Normally suspend keeps USB-device powered and calls .resume() on resume. 
But on certain conditions USB device could lose power during suspend and 
on that case reset_resume() is called, and if there is no reset_resume() 
is calls disconnect() (and probe() after that).

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 0/3] Some additional az6007 cleanup patches

[Roger Mårtensson]

Mauro Carvalho Chehab skrev 2012-08-05 19:44:
> Those are mostly cleanup patches. With regards to suspend/resume,
> this is not working properly yet. I suspect that it is due to the lack
> of dvb-usb-v2 support for reset_resume. So, document it.
>
> Mauro Carvalho Chehab (3):
>    [media] az6007: rename "st" to "state" at az6007_power_ctrl()
>    [media] az6007: make all functions static
>    [media] az6007: handle CI during suspend/resume
>
>   drivers/media/dvb/dvb-usb-v2/az6007.c | 37 +++++++++++++++++++++++++++--------
>   1 file changed, 29 insertions(+), 8 deletions(-)
>

Will all the latest patches also fix the problem with not being able to 
tune to a new encrypted channel? (Terratec H7, DVB-C. Can watch an 
encrypted channel i Kaffeine but not tune to another. Have to restart 
Kaffeine. Can tune to an unencrypted channel.)

Re: [PATCH 0/3] Some additional az6007 cleanup patches

[Mauro Carvalho Chehab]

Em 06-08-2012 12:28, Roger Mårtensson escreveu:
> Mauro Carvalho Chehab skrev 2012-08-05 19:44:
>> Those are mostly cleanup patches. With regards to suspend/resume,
>> this is not working properly yet. I suspect that it is due to the lack
>> of dvb-usb-v2 support for reset_resume. So, document it.
>>
>> Mauro Carvalho Chehab (3):
>>    [media] az6007: rename "st" to "state" at az6007_power_ctrl()
>>    [media] az6007: make all functions static
>>    [media] az6007: handle CI during suspend/resume
>>
>>   drivers/media/dvb/dvb-usb-v2/az6007.c | 37 +++++++++++++++++++++++++++--------
>>   1 file changed, 29 insertions(+), 8 deletions(-)
>>
> 
> Will all the latest patches also fix the problem with not being able to tune to a new encrypted channel?
> (Terratec H7, DVB-C. Can watch an encrypted channel i Kaffeine but not tune to another. Have to restart Kaffeine. Can tune to an unencrypted channel.)

I doubt, except if there are some bugs at dvb-usb related to CI.

They shouldn't be touching at the CI part of the driver, except for trivial
changes.

I don't have any CI card here, so I'm not able to test or work with
the CI functionality.

Regards,
Mauro

Re: [PATCH 3/3] [media] az6007: handle CI during suspend/resume

[Mauro Carvalho Chehab]

Em 06-08-2012 09:21, Antti Palosaari escreveu:
> On 08/05/2012 08:44 PM, Mauro Carvalho Chehab wrote:
>> The dvb-usb-v2 core doesn't know anything about CI. So, the
>> driver needs to handle it by hand. This patch stops CI just
>> before stopping URB's/RC, and restarts it before URB/RC start.
>>
>> It should be noticed that suspend/resume is not yet working properly,
>> as the PM model requires the implementation of reset_resume:
>>     dvb_usb_az6007 1-6:1.0: no reset_resume for driver dvb_usb_az6007?
>> But this is not implemented there at dvb-usb-v2 yet.
> 
> That is true, but it is coming:
> http://blog.palosaari.fi/2012/07/dvb-power-management-on-suspend.html
> http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb_core3
> 
> At the time I added initial suspend/resume support for dvb-usb-v2 I left those out purposely as I saw some study and changes are needed for DVB-core/frontend.
> 
> Normally suspend keeps USB-device powered and calls .resume() on resume. But on certain conditions USB device could lose power
> during suspend and on that case reset_resume() is called, and if there is no reset_resume() is calls disconnect() (and probe() after that).

This should depend on BIOS settings, and what of the following type of suspend[1]
was done: 
	S1: All processor caches are flushed, and the CPU(s) stops executing instructions.
	    Power to the CPU(s) and RAM is maintained; devices that do not indicate they 
	    must remain on may be powered down.
	S2: CPU powered off. Dirty cache is flushed to RAM.
	S3: Commonly referred to as Standby, Sleep, or Suspend to RAM. RAM remains powered
	S4: Hibernation or Suspend to Disk. All content of main memory is saved to non-volatile
	    memory such as a hard drive, and is powered down.

[1] http://en.wikipedia.org/wiki/Advanced_Configuration_and_Power_Interface

There are also some per-device sysfs nodes that control how PM will work for them.
See:

 $ tree /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
/sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
├── dev
├── device -> ../../../1-8
├── power
│   ├── async
│   ├── autosuspend_delay_ms
│   ├── control
│   ├── runtime_active_kids
│   ├── runtime_active_time
│   ├── runtime_enabled
│   ├── runtime_status
│   ├── runtime_suspended_time
│   └── runtime_usage
├── subsystem -> ../../../../../../../class/dvb
└── uevent

There are a number of pm functions that can change the power management behavior
as well.

Not sure how to control it, but, IMHO, for a media device, it only makes sense
to keep it powered on suspend if the device has IR and if the power button of 
the IR could be used to wake up the hardware. Otherwise, the better is to just
power it off, to save battery (for notebooks).

Maybe it makes sense to talk with Raphael Wysocki to be sure that it will cover
all possible cases: auto-suspend, S1/S2/S3/S4 and "wake on IR").


> regards
> Antti

Re: [PATCH 3/3] [media] az6007: handle CI during suspend/resume

[Antti Palosaari]

On 08/07/2012 02:41 PM, Mauro Carvalho Chehab wrote:
> Em 06-08-2012 09:21, Antti Palosaari escreveu:
>> On 08/05/2012 08:44 PM, Mauro Carvalho Chehab wrote:
>>> The dvb-usb-v2 core doesn't know anything about CI. So, the
>>> driver needs to handle it by hand. This patch stops CI just
>>> before stopping URB's/RC, and restarts it before URB/RC start.
>>>
>>> It should be noticed that suspend/resume is not yet working properly,
>>> as the PM model requires the implementation of reset_resume:
>>>      dvb_usb_az6007 1-6:1.0: no reset_resume for driver dvb_usb_az6007?
>>> But this is not implemented there at dvb-usb-v2 yet.
>>
>> That is true, but it is coming:
>> http://blog.palosaari.fi/2012/07/dvb-power-management-on-suspend.html
>> http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb_core3
>>
>> At the time I added initial suspend/resume support for dvb-usb-v2 I left those out purposely as I saw some study and changes are needed for DVB-core/frontend.
>>
>> Normally suspend keeps USB-device powered and calls .resume() on resume. But on certain conditions USB device could lose power
>> during suspend and on that case reset_resume() is called, and if there is no reset_resume() is calls disconnect() (and probe() after that).
>
> This should depend on BIOS settings, and what of the following type of suspend[1]
> was done:
> 	S1: All processor caches are flushed, and the CPU(s) stops executing instructions.
> 	    Power to the CPU(s) and RAM is maintained; devices that do not indicate they
> 	    must remain on may be powered down.
> 	S2: CPU powered off. Dirty cache is flushed to RAM.
> 	S3: Commonly referred to as Standby, Sleep, or Suspend to RAM. RAM remains powered
> 	S4: Hibernation or Suspend to Disk. All content of main memory is saved to non-volatile
> 	    memory such as a hard drive, and is powered down.
>
> [1] http://en.wikipedia.org/wiki/Advanced_Configuration_and_Power_Interface

That was something I was already aware. There is even S5 and S4b 
mentioned by Kernel documentation. But in real life you have to care only:
S3, Suspend, suspend to ram
S4, Hibernation, suspend to disk

And from the USB-driver point of view those are covered by there three 
callbacks:
.suspend()
.resume()
.reset_resume()
* if reset_resume() does not exits .disconnect() + .probe() is called 
instead

What is my current understanding S3 level leaves USB/PCI powered 
normally, but device driver should drop device to low power state. In 
case of DVB -device this means all sub-drivers should put sleep.

S4 naturally powers everything off. Also worth to mention laptops will 
switch from S3 to S4 if battery drains empty during S3.

> There are also some per-device sysfs nodes that control how PM will work for them.
> See:
>
>   $ tree /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
> /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
> ├── dev
> ├── device -> ../../../1-8
> ├── power
> │   ├── async
> │   ├── autosuspend_delay_ms
> │   ├── control
> │   ├── runtime_active_kids
> │   ├── runtime_active_time
> │   ├── runtime_enabled
> │   ├── runtime_status
> │   ├── runtime_suspended_time
> │   └── runtime_usage
> ├── subsystem -> ../../../../../../../class/dvb
> └── uevent
>
> There are a number of pm functions that can change the power management behavior
> as well.
>
> Not sure how to control it, but, IMHO, for a media device, it only makes sense
> to keep it powered on suspend if the device has IR and if the power button of
> the IR could be used to wake up the hardware. Otherwise, the better is to just
> power it off, to save battery (for notebooks).

yeah, and it was already done.

> Maybe it makes sense to talk with Raphael Wysocki to be sure that it will cover
> all possible cases: auto-suspend, S1/S2/S3/S4 and "wake on IR").

That IR was something I wasn't noticed at all. Currently it stops IR 
polling too. If that kind of functionality is needed it is surely some 
more work as you cannot stop IR-polling. Maybe I will skip it that time 
as I don't have time for it currently :) If someone wish to learn how 
USB polling remote could be used for wake-up computer then feel free to 
do that.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 3/3] [media] az6007: handle CI during suspend/resume

[Mauro Carvalho Chehab]

Em 07-08-2012 09:12, Antti Palosaari escreveu:
> On 08/07/2012 02:41 PM, Mauro Carvalho Chehab wrote:
>> Em 06-08-2012 09:21, Antti Palosaari escreveu:
>>> On 08/05/2012 08:44 PM, Mauro Carvalho Chehab wrote:
>>>> The dvb-usb-v2 core doesn't know anything about CI. So, the
>>>> driver needs to handle it by hand. This patch stops CI just
>>>> before stopping URB's/RC, and restarts it before URB/RC start.
>>>>
>>>> It should be noticed that suspend/resume is not yet working properly,
>>>> as the PM model requires the implementation of reset_resume:
>>>>      dvb_usb_az6007 1-6:1.0: no reset_resume for driver dvb_usb_az6007?
>>>> But this is not implemented there at dvb-usb-v2 yet.
>>>
>>> That is true, but it is coming:
>>> http://blog.palosaari.fi/2012/07/dvb-power-management-on-suspend.html
>>> http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb_core3
>>>
>>> At the time I added initial suspend/resume support for dvb-usb-v2 I left those out purposely as I saw some study and changes are needed for DVB-core/frontend.
>>>
>>> Normally suspend keeps USB-device powered and calls .resume() on resume. But on certain conditions USB device could lose power
>>> during suspend and on that case reset_resume() is called, and if there is no reset_resume() is calls disconnect() (and probe() after that).
>>
>> This should depend on BIOS settings, and what of the following type of suspend[1]
>> was done:
>>     S1: All processor caches are flushed, and the CPU(s) stops executing instructions.
>>         Power to the CPU(s) and RAM is maintained; devices that do not indicate they
>>         must remain on may be powered down.
>>     S2: CPU powered off. Dirty cache is flushed to RAM.
>>     S3: Commonly referred to as Standby, Sleep, or Suspend to RAM. RAM remains powered
>>     S4: Hibernation or Suspend to Disk. All content of main memory is saved to non-volatile
>>         memory such as a hard drive, and is powered down.
>>
>> [1] http://en.wikipedia.org/wiki/Advanced_Configuration_and_Power_Interface
> 
> That was something I was already aware. There is even S5 and S4b mentioned by Kernel documentation. But in real life you have to care only:
> S3, Suspend, suspend to ram
> S4, Hibernation, suspend to disk

At least on some of my machines, BIOS allow to select between S1 and S3 for suspend.
Not sure how USB PM suspend works for either case.

> And from the USB-driver point of view those are covered by there three callbacks:
> .suspend()
> .resume()
> .reset_resume()
> * if reset_resume() does not exits .disconnect() + .probe() is called instead
> 
> What is my current understanding S3 level leaves USB/PCI powered normally, but device 
> driver should drop device to low power state. In case of DVB -device this means all 
> sub-drivers should put sleep.

Yes. It might make sense to keep IR working (maybe at a lower polling rate, for non-
interrupt based drivers), in order to wake machine up, if the power button is pressed,
but this would be an additional feature, and I've no idea how this would be implemented.

> S4 naturally powers everything off. Also worth to mention laptops will switch from S3 to S4 if battery drains empty during S3.

I'm not a PM expert, but as BIOS may support features like wake on LAN, it would make
sense to keep USB power, at least on those devices that may wake up the device (hid
and network devices, for example).

> 
>> There are also some per-device sysfs nodes that control how PM will work for them.
>> See:
>>
>>   $ tree /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
>> /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8/dvb/dvb0.frontend0
>> ├── dev
>> ├── device -> ../../../1-8
>> ├── power
>> │   ├── async
>> │   ├── autosuspend_delay_ms
>> │   ├── control
>> │   ├── runtime_active_kids
>> │   ├── runtime_active_time
>> │   ├── runtime_enabled
>> │   ├── runtime_status
>> │   ├── runtime_suspended_time
>> │   └── runtime_usage
>> ├── subsystem -> ../../../../../../../class/dvb
>> └── uevent
>>
>> There are a number of pm functions that can change the power management behavior
>> as well.
>>
>> Not sure how to control it, but, IMHO, for a media device, it only makes sense
>> to keep it powered on suspend if the device has IR and if the power button of
>> the IR could be used to wake up the hardware. Otherwise, the better is to just
>> power it off, to save battery (for notebooks).
> 
> yeah, and it was already done.
> 
>> Maybe it makes sense to talk with Raphael Wysocki to be sure that it will cover
>> all possible cases: auto-suspend, S1/S2/S3/S4 and "wake on IR").
> 
> That IR was something I wasn't noticed at all. Currently it stops IR polling too.
> If that kind of functionality is needed it is surely some more work as you cannot 
> stop IR-polling. 

Yes. There's also an addidional case: dib0700, for example, doesn't do IR polling. 
Instead, they send an URB on a separate endpoint. When a key is pressed, the device
answers to that pending URB request with the keypress.

> Maybe I will skip it that time as I don't have time for it currently :) 
> If someone wish to learn how USB polling remote could be used for wake-up computer 
> then feel free to do that.

This is likely important for people working with embedded devices, and for people
who use softwares like mythtv to do their media centers.

Regards,
Mauro