Re: [PATCH 3/5] tm6000: bugfix interrupt reset
* Thierry Reding wrote: * Mauro Carvalho Chehab wrote: That means that all we need is to get rid of TM6000_QUIRK_NO_USB_DELAY. I've just reviewed my patches again and it seems that no-USB-delay quirk patch was only partially applied. The actual location where it was introduced was in tm6000_read_write_usb() to allow the msleep(5) to be skipped, which on some devices isn't required and significantly speeds up firmware upload. So I don't think we should get rid of it. If it helps I can rebase the code against your branch (which one would that be exactly?) and resend the rest of the series. Looking more closely, I think my original patch was applied wrongly. If you look at the original patch: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/37552 and look at the applied version in this commit: 42845708363fc92a190f5c47e6fe750e3919f867 Then you see that the hunk from the tm6000_read_write_usb() function was applied to the tm6000_reset() function instead. Thierry pgpPTm5JQtnf4.pgp Description: PGP signature
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
On 06-12-2011 04:51, Thierry Reding wrote: * Mauro Carvalho Chehab wrote: That means that all we need is to get rid of TM6000_QUIRK_NO_USB_DELAY. I've just reviewed my patches again and it seems that no-USB-delay quirk patch was only partially applied. The actual location where it was introduced was in tm6000_read_write_usb() to allow the msleep(5) to be skipped, which on some devices isn't required and significantly speeds up firmware upload. So I don't think we should get rid of it. If it helps I can rebase the code against your branch (which one would that be exactly?) and resend the rest of the series. Please do it. branch is staging/for_v3.3 Thierry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
On 06-12-2011 06:12, Thierry Reding wrote: * Thierry Reding wrote: * Mauro Carvalho Chehab wrote: That means that all we need is to get rid of TM6000_QUIRK_NO_USB_DELAY. I've just reviewed my patches again and it seems that no-USB-delay quirk patch was only partially applied. The actual location where it was introduced was in tm6000_read_write_usb() to allow the msleep(5) to be skipped, which on some devices isn't required and significantly speeds up firmware upload. So I don't think we should get rid of it. If it helps I can rebase the code against your branch (which one would that be exactly?) and resend the rest of the series. Looking more closely, I think my original patch was applied wrongly. If you look at the original patch: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/37552 and look at the applied version in this commit: 42845708363fc92a190f5c47e6fe750e3919f867 Then you see that the hunk from the tm6000_read_write_usb() function was applied to the tm6000_reset() function instead. Hmmm... probably merge conflicts. It is rare, but such things sometimes happen, if two functions are very similar, and there were enough changes on a driver for the patch logic to do the wrong thing. In any case, please send me the patch again, against my tree. Thanks, Mauro Thierry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
On 05-12-2011 05:21, Thierry Reding wrote:
* linu...@stefanringel.de wrote:
From: Stefan Ringellinu...@stefanringel.de
Signed-off-by: Stefan Ringellinu...@stefanringel.de
Your commit message needs more details. Why do you think this is a bugfix?
Also this commit seems to effectively revert (and then partially reimplement)
a patch that I posted some months ago.
Thierry,
I noticed this. I tested tm6000 with those changes with both the first gen
tm5600 devices I have and HVR900H and I didn't notice any bad thing with this
approach, and changing from one standard to another is now faster.
So, I decided to apply it (with the remaining patches I've made to
fix audio for PAL/M and NTSC/M).
I also noticed that TM6000_QUIRK_NO_USB_DELAY is not needed anymore
(still, Stefan's patches didn't remove it completely).
Could you please test if the problems you've solved with your approach
are still occurring?
Regards,
Mauro
---
drivers/media/video/tm6000/tm6000-core.c | 49 -
drivers/media/video/tm6000/tm6000-video.c | 21 ++--
2 files changed, 17 insertions(+), 53 deletions(-)
diff --git a/drivers/media/video/tm6000/tm6000-core.c
b/drivers/media/video/tm6000/tm6000-core.c
index c007e6d..920299e 100644
--- a/drivers/media/video/tm6000/tm6000-core.c
+++ b/drivers/media/video/tm6000/tm6000-core.c
@@ -599,55 +599,6 @@ int tm6000_init(struct tm6000_core *dev)
return rc;
}
-int tm6000_reset(struct tm6000_core *dev)
-{
- int pipe;
- int err;
-
- msleep(500);
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 0);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 0\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- err = usb_reset_configuration(dev-udev);
- if (err 0) {
- tm6000_err(failed to reset configuration\n);
- return err;
- }
-
- if ((dev-quirks TM6000_QUIRK_NO_USB_DELAY) == 0)
- msleep(5);
-
- /*
-* Not all devices have int_in defined
-*/
- if (!dev-int_in.endp)
- return 0;
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 2);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 2\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- msleep(5);
-
- pipe = usb_rcvintpipe(dev-udev,
- dev-int_in.endp-desc.bEndpointAddress
USB_ENDPOINT_NUMBER_MASK);
-
- err = usb_clear_halt(dev-udev, pipe);
- if (err 0) {
- tm6000_err(usb_clear_halt failed: %d\n, err);
- return err;
- }
-
- return 0;
-}
int tm6000_set_audio_bitrate(struct tm6000_core *dev, int bitrate)
{
diff --git a/drivers/media/video/tm6000/tm6000-video.c
b/drivers/media/video/tm6000/tm6000-video.c
index 1e5ace0..4db3535 100644
--- a/drivers/media/video/tm6000/tm6000-video.c
+++ b/drivers/media/video/tm6000/tm6000-video.c
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
+ usb_reset_configuration(dev-udev);
+
+ if (dev-int_in)
This check is wrong,dev-int_in will always be true.
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 2);
+ else
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 0);
This would need better indentation.
+
+ /* Start interrupt USB pipe */
+ tm6000_ir_int_start(dev);
+
Why do you restart the IR interrupt pipe when the device is being released?
if (!fh-radio)
videobuf_mmap_free(fh-vb_vidq);
-
- err = tm6000_reset(dev);
- if (err 0)
- dev_err(vdev-dev, reset failed: %d\n, err);
}
kfree(fh);
I think this whole patch should be much shorter. Something along the lines
of:
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
if (!fh-radio)
videobuf_mmap_free(fh-vb_vidq);
Thierry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
* Mauro Carvalho Chehab wrote: On 05-12-2011 05:21, Thierry Reding wrote: * linu...@stefanringel.de wrote: From: Stefan Ringellinu...@stefanringel.de Signed-off-by: Stefan Ringellinu...@stefanringel.de Your commit message needs more details. Why do you think this is a bugfix? Also this commit seems to effectively revert (and then partially reimplement) a patch that I posted some months ago. Thierry, I noticed this. I tested tm6000 with those changes with both the first gen tm5600 devices I have and HVR900H and I didn't notice any bad thing with this approach, and changing from one standard to another is now faster. So, I decided to apply it (with the remaining patches I've made to fix audio for PAL/M and NTSC/M). I also noticed that TM6000_QUIRK_NO_USB_DELAY is not needed anymore (still, Stefan's patches didn't remove it completely). Could you please test if the problems you've solved with your approach are still occurring? Unfortunately I don't have any hardware available anymore. I will see if I can get my hands on some of the devices, but that may take a while. I guess you'll just have to apply without me testing them first. My comments should be addressed anyway, though. Thierry pgpJuZnCVVj6o.pgp Description: PGP signature
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
On 05-12-2011 13:38, Thierry Reding wrote: * Mauro Carvalho Chehab wrote: On 05-12-2011 05:21, Thierry Reding wrote: * linu...@stefanringel.de wrote: From: Stefan Ringellinu...@stefanringel.de Signed-off-by: Stefan Ringellinu...@stefanringel.de Your commit message needs more details. Why do you think this is a bugfix? Also this commit seems to effectively revert (and then partially reimplement) a patch that I posted some months ago. Thierry, I noticed this. I tested tm6000 with those changes with both the first gen tm5600 devices I have and HVR900H and I didn't notice any bad thing with this approach, and changing from one standard to another is now faster. So, I decided to apply it (with the remaining patches I've made to fix audio for PAL/M and NTSC/M). I also noticed that TM6000_QUIRK_NO_USB_DELAY is not needed anymore (still, Stefan's patches didn't remove it completely). Could you please test if the problems you've solved with your approach are still occurring? Unfortunately I don't have any hardware available anymore. I will see if I can get my hands on some of the devices, but that may take a while. I guess you'll just have to apply without me testing them first. Ok. My comments should be addressed anyway, though. Sure. Stefan, Could you better explain a little more about this change? Also, if this is not required anymore, please send us a patch removing the TM6000_QUIRK_NO_USB_DELAY quirk. Regards, Mauro. Thierry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
Am 05.12.2011 19:21, schrieb Mauro Carvalho Chehab: On 05-12-2011 13:38, Thierry Reding wrote: * Mauro Carvalho Chehab wrote: On 05-12-2011 05:21, Thierry Reding wrote: * linu...@stefanringel.de wrote: From: Stefan Ringellinu...@stefanringel.de Signed-off-by: Stefan Ringellinu...@stefanringel.de Your commit message needs more details. Why do you think this is a bugfix? Also this commit seems to effectively revert (and then partially reimplement) a patch that I posted some months ago. Thierry, I noticed this. I tested tm6000 with those changes with both the first gen tm5600 devices I have and HVR900H and I didn't notice any bad thing with this approach, and changing from one standard to another is now faster. So, I decided to apply it (with the remaining patches I've made to fix audio for PAL/M and NTSC/M). I also noticed that TM6000_QUIRK_NO_USB_DELAY is not needed anymore (still, Stefan's patches didn't remove it completely). Could you please test if the problems you've solved with your approach are still occurring? Unfortunately I don't have any hardware available anymore. I will see if I can get my hands on some of the devices, but that may take a while. I guess you'll just have to apply without me testing them first. Ok. My comments should be addressed anyway, though. Sure. Stefan, Could you better explain a little more about this change? After add Thierry's patch the interrupt endpoint don't send data anymore. I tested different ways to bring the interrupt endpoint in working. First in the function tm6000_uninit_isoc() - nothing, but if I remove the function tm6000_reset(), then works. The next what I tested are directly in the function tm6000_reset(), but it froze in. Now I am adding this lines in function tm600_relese() in position it call tm6000() (after videobuf_mmap_free() and it froze in, but before videobuf_mmap_free() it don't froze in and I have now data over the interrupt endpoint, and IR works. better now? Stefan Also, if this is not required anymore, please send us a patch removing the TM6000_QUIRK_NO_USB_DELAY quirk. In a few days, if I have tested my next patch (signal detection) Regards, Mauro. Thierry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
On 05-12-2011 18:02, Stefan Ringel wrote: Am 05.12.2011 19:21, schrieb Mauro Carvalho Chehab: On 05-12-2011 13:38, Thierry Reding wrote: * Mauro Carvalho Chehab wrote: On 05-12-2011 05:21, Thierry Reding wrote: * linu...@stefanringel.de wrote: From: Stefan Ringellinu...@stefanringel.de Signed-off-by: Stefan Ringellinu...@stefanringel.de Your commit message needs more details. Why do you think this is a bugfix? Also this commit seems to effectively revert (and then partially reimplement) a patch that I posted some months ago. Thierry, I noticed this. I tested tm6000 with those changes with both the first gen tm5600 devices I have and HVR900H and I didn't notice any bad thing with this approach, and changing from one standard to another is now faster. So, I decided to apply it (with the remaining patches I've made to fix audio for PAL/M and NTSC/M). I also noticed that TM6000_QUIRK_NO_USB_DELAY is not needed anymore (still, Stefan's patches didn't remove it completely). Could you please test if the problems you've solved with your approach are still occurring? Unfortunately I don't have any hardware available anymore. I will see if I can get my hands on some of the devices, but that may take a while. I guess you'll just have to apply without me testing them first. Ok. My comments should be addressed anyway, though. Sure. Stefan, Could you better explain a little more about this change? After add Thierry's patch the interrupt endpoint don't send data anymore. I tested different ways to bring the interrupt endpoint in working. First in the function tm6000_uninit_isoc() - nothing, but if I remove the function tm6000_reset(), then works. The next what I tested are directly in the function tm6000_reset(), but it froze in. Now I am adding this lines in function tm600_relese() in position it call tm6000() (after videobuf_mmap_free() and it froze in, but before videobuf_mmap_free() it don't froze in and I have now data over the interrupt endpoint, and IR works. better now? Ah, OK. Well, the IR code were re-written. If, for any reason, the interrupt endpoint refuses to accept an usb_submission, the IR code will defer work to re-try it again after 100ms. That means that all we need is to get rid of TM6000_QUIRK_NO_USB_DELAY. Stefan Also, if this is not required anymore, please send us a patch removing the TM6000_QUIRK_NO_USB_DELAY quirk. In a few days, if I have tested my next patch (signal detection) Regards, Mauro. Thierry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
* Mauro Carvalho Chehab wrote: That means that all we need is to get rid of TM6000_QUIRK_NO_USB_DELAY. I've just reviewed my patches again and it seems that no-USB-delay quirk patch was only partially applied. The actual location where it was introduced was in tm6000_read_write_usb() to allow the msleep(5) to be skipped, which on some devices isn't required and significantly speeds up firmware upload. So I don't think we should get rid of it. If it helps I can rebase the code against your branch (which one would that be exactly?) and resend the rest of the series. Thierry pgpi5YiWYQ4GP.pgp Description: PGP signature
Re: [PATCH 3/5] tm6000: bugfix interrupt reset
* linu...@stefanringel.de wrote:
From: Stefan Ringel linu...@stefanringel.de
Signed-off-by: Stefan Ringel linu...@stefanringel.de
Your commit message needs more details. Why do you think this is a bugfix?
Also this commit seems to effectively revert (and then partially reimplement)
a patch that I posted some months ago.
---
drivers/media/video/tm6000/tm6000-core.c | 49
-
drivers/media/video/tm6000/tm6000-video.c | 21 ++--
2 files changed, 17 insertions(+), 53 deletions(-)
diff --git a/drivers/media/video/tm6000/tm6000-core.c
b/drivers/media/video/tm6000/tm6000-core.c
index c007e6d..920299e 100644
--- a/drivers/media/video/tm6000/tm6000-core.c
+++ b/drivers/media/video/tm6000/tm6000-core.c
@@ -599,55 +599,6 @@ int tm6000_init(struct tm6000_core *dev)
return rc;
}
-int tm6000_reset(struct tm6000_core *dev)
-{
- int pipe;
- int err;
-
- msleep(500);
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 0);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 0\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- err = usb_reset_configuration(dev-udev);
- if (err 0) {
- tm6000_err(failed to reset configuration\n);
- return err;
- }
-
- if ((dev-quirks TM6000_QUIRK_NO_USB_DELAY) == 0)
- msleep(5);
-
- /*
- * Not all devices have int_in defined
- */
- if (!dev-int_in.endp)
- return 0;
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 2);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 2\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- msleep(5);
-
- pipe = usb_rcvintpipe(dev-udev,
- dev-int_in.endp-desc.bEndpointAddress
USB_ENDPOINT_NUMBER_MASK);
-
- err = usb_clear_halt(dev-udev, pipe);
- if (err 0) {
- tm6000_err(usb_clear_halt failed: %d\n, err);
- return err;
- }
-
- return 0;
-}
int tm6000_set_audio_bitrate(struct tm6000_core *dev, int bitrate)
{
diff --git a/drivers/media/video/tm6000/tm6000-video.c
b/drivers/media/video/tm6000/tm6000-video.c
index 1e5ace0..4db3535 100644
--- a/drivers/media/video/tm6000/tm6000-video.c
+++ b/drivers/media/video/tm6000/tm6000-video.c
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
+ usb_reset_configuration(dev-udev);
+
+ if (dev-int_in)
This check is wrong, dev-int_in will always be true.
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 2);
+ else
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 0);
This would need better indentation.
+
+ /* Start interrupt USB pipe */
+ tm6000_ir_int_start(dev);
+
Why do you restart the IR interrupt pipe when the device is being released?
if (!fh-radio)
videobuf_mmap_free(fh-vb_vidq);
-
- err = tm6000_reset(dev);
- if (err 0)
- dev_err(vdev-dev, reset failed: %d\n, err);
}
kfree(fh);
I think this whole patch should be much shorter. Something along the lines
of:
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
if (!fh-radio)
videobuf_mmap_free(fh-vb_vidq);
Thierry
pgpMorYsK0ys3.pgp
Description: PGP signature
[PATCH 3/5] tm6000: bugfix interrupt reset
From: Stefan Ringel linu...@stefanringel.de
Signed-off-by: Stefan Ringel linu...@stefanringel.de
---
drivers/media/video/tm6000/tm6000-core.c | 49 -
drivers/media/video/tm6000/tm6000-video.c | 21 ++--
2 files changed, 17 insertions(+), 53 deletions(-)
diff --git a/drivers/media/video/tm6000/tm6000-core.c
b/drivers/media/video/tm6000/tm6000-core.c
index c007e6d..920299e 100644
--- a/drivers/media/video/tm6000/tm6000-core.c
+++ b/drivers/media/video/tm6000/tm6000-core.c
@@ -599,55 +599,6 @@ int tm6000_init(struct tm6000_core *dev)
return rc;
}
-int tm6000_reset(struct tm6000_core *dev)
-{
- int pipe;
- int err;
-
- msleep(500);
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 0);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 0\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- err = usb_reset_configuration(dev-udev);
- if (err 0) {
- tm6000_err(failed to reset configuration\n);
- return err;
- }
-
- if ((dev-quirks TM6000_QUIRK_NO_USB_DELAY) == 0)
- msleep(5);
-
- /*
-* Not all devices have int_in defined
-*/
- if (!dev-int_in.endp)
- return 0;
-
- err = usb_set_interface(dev-udev, dev-isoc_in.bInterfaceNumber, 2);
- if (err 0) {
- tm6000_err(failed to select interface %d, alt. setting 2\n,
- dev-isoc_in.bInterfaceNumber);
- return err;
- }
-
- msleep(5);
-
- pipe = usb_rcvintpipe(dev-udev,
- dev-int_in.endp-desc.bEndpointAddress
USB_ENDPOINT_NUMBER_MASK);
-
- err = usb_clear_halt(dev-udev, pipe);
- if (err 0) {
- tm6000_err(usb_clear_halt failed: %d\n, err);
- return err;
- }
-
- return 0;
-}
int tm6000_set_audio_bitrate(struct tm6000_core *dev, int bitrate)
{
diff --git a/drivers/media/video/tm6000/tm6000-video.c
b/drivers/media/video/tm6000/tm6000-video.c
index 1e5ace0..4db3535 100644
--- a/drivers/media/video/tm6000/tm6000-video.c
+++ b/drivers/media/video/tm6000/tm6000-video.c
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
+ usb_reset_configuration(dev-udev);
+
+ if (dev-int_in)
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 2);
+ else
+ usb_set_interface(dev-udev,
+ dev-isoc_in.bInterfaceNumber,
+ 0);
+
+ /* Start interrupt USB pipe */
+ tm6000_ir_int_start(dev);
+
if (!fh-radio)
videobuf_mmap_free(fh-vb_vidq);
-
- err = tm6000_reset(dev);
- if (err 0)
- dev_err(vdev-dev, reset failed: %d\n, err);
}
kfree(fh);
--
1.7.7
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
