[PATCH 2/5] tm6000: bugfix register setting

[PATCH 2/5] tm6000: bugfix register setting

[linuxtv]

From: Stefan Ringel <linuxtv@stefanringel.de>

Signed-off-by: Stefan Ringel <linuxtv@stefanringel.de>
---
 drivers/media/video/tm6000/tm6000-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-core.c b/drivers/media/video/tm6000/tm6000-core.c
index 9783616..c007e6d 100644
--- a/drivers/media/video/tm6000/tm6000-core.c
+++ b/drivers/media/video/tm6000/tm6000-core.c
@@ -125,14 +125,14 @@ int tm6000_set_reg_mask(struct tm6000_core *dev, u8 req, u16 value,
 	u8 new_index;
 
 	rc = tm6000_read_write_usb(dev, USB_DIR_IN | USB_TYPE_VENDOR, req,
-					value, index, buf, 1);
+					value, 0, buf, 1);
 
 	if (rc < 0)
 		return rc;
 
 	new_index = (buf[0] & ~mask) | (index & mask);
 
-	if (new_index == index)
+	if (new_index == buf[0])
 		return 0;
 
 	return tm6000_read_write_usb(dev, USB_DIR_OUT | USB_TYPE_VENDOR,
-- 
1.7.7

[PATCH 3/5] tm6000: bugfix interrupt reset

[linuxtv]

From: Stefan Ringel <linuxtv@stefanringel.de>

Signed-off-by: Stefan Ringel <linuxtv@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

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3654 bytes --]

* linuxtv@stefanringel.de wrote:
> From: Stefan Ringel <linuxtv@stefanringel.de>
> 
> Signed-off-by: Stefan Ringel <linuxtv@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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Mauro Carvalho Chehab]

On 05-12-2011 05:21, Thierry Reding wrote:
> * linuxtv@stefanringel.de wrote:
>> From: Stefan Ringel<linuxtv@stefanringel.de>
>>
>> Signed-off-by: Stefan Ringel<linuxtv@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

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

* Mauro Carvalho Chehab wrote:
> On 05-12-2011 05:21, Thierry Reding wrote:
> >* linuxtv@stefanringel.de wrote:
> >>From: Stefan Ringel<linuxtv@stefanringel.de>
> >>
> >>Signed-off-by: Stefan Ringel<linuxtv@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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[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:
>>> * linuxtv@stefanringel.de wrote:
>>>> From: Stefan Ringel<linuxtv@stefanringel.de>
>>>>
>>>> Signed-off-by: Stefan Ringel<linuxtv@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

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Stefan Ringel]

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:
>>>> * linuxtv@stefanringel.de wrote:
>>>>> From: Stefan Ringel<linuxtv@stefanringel.de>
>>>>>
>>>>> Signed-off-by: Stefan Ringel<linuxtv@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 majordomo@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]

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:
>>>>> * linuxtv@stefanringel.de wrote:
>>>>>> From: Stefan Ringel<linuxtv@stefanringel.de>
>>>>>>
>>>>>> Signed-off-by: Stefan Ringel<linuxtv@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 majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

* 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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

* 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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Mauro Carvalho Chehab]

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

[PATCH] [media] tm6000: Fix fast USB access quirk

[Thierry Reding]

The original patch used the fast USB quirk to enable fast access to
registers in the tm6000_read_write_usb(). The applied patch moved the
check to the tm6000_reset(), probably due to some merge conflicts.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/media/video/tm6000/tm6000-core.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-core.c b/drivers/media/video/tm6000/tm6000-core.c
index 59dd63d..5a10bf3 100644
--- a/drivers/media/video/tm6000/tm6000-core.c
+++ b/drivers/media/video/tm6000/tm6000-core.c
@@ -88,7 +88,9 @@ int tm6000_read_write_usb(struct tm6000_core *dev, u8 req_type, u8 req,
 	}
 
 	kfree(data);
-	msleep(5);
+
+	if ((dev->quirks & TM6000_QUIRK_NO_USB_DELAY) == 0)
+		msleep(5);
 
 	mutex_unlock(&dev->usb_lock);
 	return ret;
-- 
1.7.8

Re: [PATCH 3/5] tm6000: bugfix interrupt reset

[Mauro Carvalho Chehab]

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

[PATCH 4/5] tm6000: bugfix bulk transfer

[linuxtv]

From: Stefan Ringel <linuxtv@stefanringel.de>

Signed-off-by: Stefan Ringel <linuxtv@stefanringel.de>
---
 drivers/media/video/tm6000/tm6000-dvb.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-dvb.c b/drivers/media/video/tm6000/tm6000-dvb.c
index 5e6c129..db6a561 100644
--- a/drivers/media/video/tm6000/tm6000-dvb.c
+++ b/drivers/media/video/tm6000/tm6000-dvb.c
@@ -89,9 +89,19 @@ static void tm6000_urb_received(struct urb *urb)
 	int ret;
 	struct tm6000_core *dev = urb->context;
 
-	if (urb->status != 0)
+	switch (urb->status) {
+	case 0:
+	case -ETIMEDOUT:
+		break;
+	case -ENOENT:
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+		return;
+	default:
 		print_err_status(dev, 0, urb->status);
-	else if (urb->actual_length > 0)
+	}
+
+	if (urb->actual_length > 0)
 		dvb_dmx_swfilter(&dev->dvb->demux, urb->transfer_buffer,
 						   urb->actual_length);
 
@@ -151,7 +161,7 @@ static int tm6000_start_stream(struct tm6000_core *dev)
 		printk(KERN_ERR "tm6000: pipe resetted\n");
 
 /*	mutex_lock(&tm6000_driver.open_close_mutex); */
-	ret = usb_submit_urb(dvb->bulk_urb, GFP_KERNEL);
+	ret = usb_submit_urb(dvb->bulk_urb, GFP_ATOMIC);
 
 /*	mutex_unlock(&tm6000_driver.open_close_mutex); */
 	if (ret) {
-- 
1.7.7

[PATCH 5/5] tm6000: bugfix data check

[linuxtv]

From: Stefan Ringel <linuxtv@stefanringel.de>

beholder use a map with 3 bytes, but many rc maps have 2 bytes, so I add a workaround for beholder rc.

Signed-off-by: Stefan Ringel <linuxtv@stefanringel.de>
---
 drivers/media/video/tm6000/tm6000-input.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-input.c b/drivers/media/video/tm6000/tm6000-input.c
index 405d127..ae7772e 100644
--- a/drivers/media/video/tm6000/tm6000-input.c
+++ b/drivers/media/video/tm6000/tm6000-input.c
@@ -178,9 +178,21 @@ static int default_polling_getkey(struct tm6000_IR *ir,
 			poll_result->rc_data = ir->urb_data[0];
 			break;
 		case RC_TYPE_NEC:
-			if (ir->urb_data[1] == ((ir->key_addr >> 8) & 0xff)) {
+			switch (dev->model) {
+			case 10:
+			case 11:
+			case 14:
+			case 15:
+				if (ir->urb_data[1] ==
+					((ir->key_addr >> 8) & 0xff)) {
+					poll_result->rc_data =
+					ir->urb_data[0]
+					| ir->urb_data[1] << 8;
+				}
+				break;
+			default:
 				poll_result->rc_data = ir->urb_data[0]
-							| ir->urb_data[1] << 8;
+					| ir->urb_data[1] << 8;
 			}
 			break;
 		default:
@@ -238,8 +250,6 @@ static void tm6000_ir_handle_key(struct tm6000_IR *ir)
 		return;
 	}
 
-	dprintk("ir->get_key result data=%04x\n", poll_result.rc_data);
-
 	if (ir->pwled) {
 		if (ir->pwledcnt >= PWLED_OFF) {
 			ir->pwled = 0;
@@ -250,6 +260,7 @@ static void tm6000_ir_handle_key(struct tm6000_IR *ir)
 	}
 
 	if (ir->key) {
+		dprintk("ir->get_key result data=%04x\n", poll_result.rc_data);
 		rc_keydown(ir->rc, poll_result.rc_data, 0);
 		ir->key = 0;
 		ir->pwled = 1;
@@ -333,7 +344,7 @@ int tm6000_ir_int_start(struct tm6000_core *dev)
 		ir->int_urb->transfer_buffer, size,
 		tm6000_ir_urb_received, dev,
 		dev->int_in.endp->desc.bInterval);
-	err = usb_submit_urb(ir->int_urb, GFP_KERNEL);
+	err = usb_submit_urb(ir->int_urb, GFP_ATOMIC);
 	if (err) {
 		kfree(ir->int_urb->transfer_buffer);
 		usb_free_urb(ir->int_urb);
-- 
1.7.7

Re: [PATCH 5/5] tm6000: bugfix data check

[Mauro Carvalho Chehab]

Hi Stefan and Dmitri,

The entire RC input looked badly implemented. It never worked with HVR-900H,
and it were doing polling even on URB interrupt mode.

I've rewritten the entire code, and fixed it to be more reliable.

I've tested both interrupt and polling modes, with HVR-900H. On HVR-900H,
polling mode returns 0 when a scancode arrives, 0xff otherwise. This is
not the expected behavior on polling mode, but it seems to e due to the
way this device is wired. Anyway, the test were enough to test both ways
of receiving scancodes.

So, I broke the code for interrupt-driven and for polling-driven IR. This
made the code simpler to understand and more reliable. Also, on interrupt
driven mode, the CPU won't need to wake on every 50ms due to IR.

Both NEC and RC-5 protocols were tested, and I tried to make sure that the
driver would support 1 byte scancode, where devices can't provide
two bytes.

Please double check the patches I've made against your devices, in order
to be sure that nothing went wrong. Maybe something more would be needed
for IR on tm5600/tm6000.

I should be applying them upstream later today or tomorrow.

Regards,
Mauro

On 28-11-2011 17:46, linuxtv@stefanringel.de wrote:
> From: Stefan Ringel<linuxtv@stefanringel.de>
>
> beholder use a map with 3 bytes, but many rc maps have 2 bytes, so I add a workaround for beholder rc.
>
> Signed-off-by: Stefan Ringel<linuxtv@stefanringel.de>
> ---
>   drivers/media/video/tm6000/tm6000-input.c |   21 ++++++++++++++++-----
>   1 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/video/tm6000/tm6000-input.c b/drivers/media/video/tm6000/tm6000-input.c
> index 405d127..ae7772e 100644
> --- a/drivers/media/video/tm6000/tm6000-input.c
> +++ b/drivers/media/video/tm6000/tm6000-input.c
> @@ -178,9 +178,21 @@ static int default_polling_getkey(struct tm6000_IR *ir,
>   			poll_result->rc_data = ir->urb_data[0];
>   			break;
>   		case RC_TYPE_NEC:
> -			if (ir->urb_data[1] == ((ir->key_addr>>  8)&  0xff)) {
> +			switch (dev->model) {
> +			case 10:
> +			case 11:
> +			case 14:
> +			case 15:

Using magic numbers here is a very bad idea.

> +				if (ir->urb_data[1] ==
> +					((ir->key_addr>>  8)&  0xff)) {
> +					poll_result->rc_data =
> +					ir->urb_data[0]
> +					| ir->urb_data[1]<<  8;
> +				}

Despite your comment, this is a 2 bytes scancode.

> +				break;
> +			default:
>   				poll_result->rc_data = ir->urb_data[0]
> -							| ir->urb_data[1]<<  8;
> +					| ir->urb_data[1]<<  8;
>   			}
>   			break;
>   		default:
> @@ -238,8 +250,6 @@ static void tm6000_ir_handle_key(struct tm6000_IR *ir)
>   		return;
>   	}
>
> -	dprintk("ir->get_key result data=%04x\n", poll_result.rc_data);
> -
>   	if (ir->pwled) {
>   		if (ir->pwledcnt>= PWLED_OFF) {
>   			ir->pwled = 0;
> @@ -250,6 +260,7 @@ static void tm6000_ir_handle_key(struct tm6000_IR *ir)
>   	}
>
>   	if (ir->key) {
> +		dprintk("ir->get_key result data=%04x\n", poll_result.rc_data);
>   		rc_keydown(ir->rc, poll_result.rc_data, 0);
>   		ir->key = 0;
>   		ir->pwled = 1;
> @@ -333,7 +344,7 @@ int tm6000_ir_int_start(struct tm6000_core *dev)
>   		ir->int_urb->transfer_buffer, size,
>   		tm6000_ir_urb_received, dev,
>   		dev->int_in.endp->desc.bInterval);
> -	err = usb_submit_urb(ir->int_urb, GFP_KERNEL);
> +	err = usb_submit_urb(ir->int_urb, GFP_ATOMIC);
>   	if (err) {
>   		kfree(ir->int_urb->transfer_buffer);
>   		usb_free_urb(ir->int_urb);

[PATCH 1/2] [media] tm6000: Fix check for interrupt endpoint

[Thierry Reding]

Checking for &dev->int_in is useless because it returns the address of
the embedded struct tm6000_endpoint, which will always be positive and
therefore true.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/media/video/tm6000/tm6000-input.c |    2 +-
 drivers/media/video/tm6000/tm6000-video.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-input.c b/drivers/media/video/tm6000/tm6000-input.c
index af4bcf5..89e003c 100644
--- a/drivers/media/video/tm6000/tm6000-input.c
+++ b/drivers/media/video/tm6000/tm6000-input.c
@@ -426,7 +426,7 @@ int tm6000_ir_init(struct tm6000_core *dev)
 	rc->scanmask = 0xffff;
 	rc->priv = ir;
 	rc->change_protocol = tm6000_ir_change_protocol;
-	if (&dev->int_in) {
+	if (dev->int_in.endp) {
 		rc->open    = __tm6000_ir_int_start;
 		rc->close   = __tm6000_ir_int_stop;
 		INIT_DELAYED_WORK(&ir->work, tm6000_ir_int_work);
diff --git a/drivers/media/video/tm6000/tm6000-video.c b/drivers/media/video/tm6000/tm6000-video.c
index e147d92..87eb909 100644
--- a/drivers/media/video/tm6000/tm6000-video.c
+++ b/drivers/media/video/tm6000/tm6000-video.c
@@ -1647,7 +1647,7 @@ static int tm6000_release(struct file *file)
 
 		usb_reset_configuration(dev->udev);
 
-		if (&dev->int_in)
+		if (dev->int_in.endp)
 			usb_set_interface(dev->udev,
 			dev->isoc_in.bInterfaceNumber,
 			2);
-- 
1.7.8

[PATCH 2/2] [media] tm6000: Fix bad indentation.

[Thierry Reding]

Function parameters on subsequent lines should never be aligned with the
function name but rather be indented.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/media/video/tm6000/tm6000-video.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/tm6000/tm6000-video.c b/drivers/media/video/tm6000/tm6000-video.c
index 87eb909..a15fd9d 100644
--- a/drivers/media/video/tm6000/tm6000-video.c
+++ b/drivers/media/video/tm6000/tm6000-video.c
@@ -1649,12 +1649,10 @@ static int tm6000_release(struct file *file)
 
 		if (dev->int_in.endp)
 			usb_set_interface(dev->udev,
-			dev->isoc_in.bInterfaceNumber,
-			2);
+					dev->isoc_in.bInterfaceNumber, 2);
 		else
 			usb_set_interface(dev->udev,
-			dev->isoc_in.bInterfaceNumber,
-			0);
+					dev->isoc_in.bInterfaceNumber, 0);
 
 		/* Start interrupt USB pipe */
 		tm6000_ir_int_start(dev);
-- 
1.7.8

Re: [PATCH 2/2] [media] tm6000: Fix bad indentation.

[Antti Palosaari]

That question is related to that kind of indentation generally, not only 
that patch.

On 12/06/2011 03:39 PM, Thierry Reding wrote:
> Function parameters on subsequent lines should never be aligned with the
> function name but rather be indented.
[...]
>   			usb_set_interface(dev->udev,
> -			dev->isoc_in.bInterfaceNumber,
> -			0);
> +					dev->isoc_in.bInterfaceNumber, 0);

Which kind of indentation should be used when function params are 
slitted to multiple lines?

In that case two tabs are used (related to function indentation).
example:
	ret= function(param1,
			param2);

Other generally used is only one tab (related to function indentation).
example:
	ret= function(param1,
		param2);

And last generally used is multiple tabs + spaces until same location 
where first param is meet (related to function indentation). I see that 
bad since use of tabs, with only spaces I see it fine. And this many 
times leads situation param level are actually different whilst 
originally idea was to put those same level.
example:
	ret= function(param1,
		      param2);


regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 2/2] [media] tm6000: Fix bad indentation.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

* Antti Palosaari wrote:
> That question is related to that kind of indentation generally, not
> only that patch.
> 
> On 12/06/2011 03:39 PM, Thierry Reding wrote:
> >Function parameters on subsequent lines should never be aligned with the
> >function name but rather be indented.
> [...]
> >  			usb_set_interface(dev->udev,
> >-			dev->isoc_in.bInterfaceNumber,
> >-			0);
> >+					dev->isoc_in.bInterfaceNumber, 0);
> 
> Which kind of indentation should be used when function params are
> slitted to multiple lines?

I don't think this is documented anywhere and there are no hard rules with
regard to this. I guess anything is fine as long as it is indented at all.

> In that case two tabs are used (related to function indentation).
> example:
> 	ret= function(param1,
> 			param2);

I usually use that because it is my text editor's default.

> Other generally used is only one tab (related to function indentation).
> example:
> 	ret= function(param1,
> 		param2);

I think that's okay as well.

> And last generally used is multiple tabs + spaces until same
> location where first param is meet (related to function
> indentation). I see that bad since use of tabs, with only spaces I
> see it fine. And this many times leads situation param level are
> actually different whilst originally idea was to put those same
> level.
> example:
> 	ret= function(param1,
> 		      param2);

Whether this works or not always depends on the tab-width. I think most
variations are okay here. Some people like to align them, other people
don't.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2] [media] tm6000: Fix bad indentation.

[Mauro Carvalho Chehab]

On 06-12-2011 12:13, Thierry Reding wrote:
> * Antti Palosaari wrote:
>> That question is related to that kind of indentation generally, not
>> only that patch.
>>
>> On 12/06/2011 03:39 PM, Thierry Reding wrote:
>>> Function parameters on subsequent lines should never be aligned with the
>>> function name but rather be indented.
>> [...]
>>>   			usb_set_interface(dev->udev,
>>> -			dev->isoc_in.bInterfaceNumber,
>>> -			0);
>>> +					dev->isoc_in.bInterfaceNumber, 0);
>>
>> Which kind of indentation should be used when function params are
>> slitted to multiple lines?

Documentation/CodingStyle currently says:

	Statements longer than 80 columns will be broken into sensible chunks, unless
	exceeding 80 columns significantly increases readability and does not hide
	information. Descendants are always substantially shorter than the parent and
	are placed substantially to the right. The same applies to function headers
	with a long argument list. However, never break user-visible strings such as
	printk messages, because that breaks the ability to grep for them.

So, it should be: "substantially to the right" whatever this means.

> I don't think this is documented anywhere and there are no hard rules with
> regard to this. I guess anything is fine as long as it is indented at all.
>
>> In that case two tabs are used (related to function indentation).
>> example:
>> 	ret= function(param1,
>> 			param2);
>
> I usually use that because it is my text editor's default.
>
>> Other generally used is only one tab (related to function indentation).
>> example:
>> 	ret= function(param1,
>> 		param2);
>
> I think that's okay as well.

One tab can hardly be interpreted as "substantially to the right".

>
>> And last generally used is multiple tabs + spaces until same
>> location where first param is meet (related to function
>> indentation). I see that bad since use of tabs, with only spaces I
>> see it fine. And this many times leads situation param level are
>> actually different whilst originally idea was to put those same
>> level.
>> example:
>> 	ret= function(param1,
>> 		      param2);

In practice, this is the most commonly used way, from what I noticed, not only
at drivers/media. A good place to look for commonly used CodingStyle are the
most used headers at include/linux. As far as I noticed, they all use this
style.

>
> Whether this works or not always depends on the tab-width. I think most
> variations are okay here. Some people like to align them, other people
> don't.

Tab width is always 8, according with the CodingStyle:

	"Tabs are 8 characters, and thus indentations are also 8 characters."

Regards,
Mauro

Re: [PATCH 2/2] [media] tm6000: Fix bad indentation.

[Antti Palosaari]

On 12/06/2011 10:58 PM, Mauro Carvalho Chehab wrote:
> On 06-12-2011 12:13, Thierry Reding wrote:
>> * Antti Palosaari wrote:
>>> That question is related to that kind of indentation generally, not
>>> only that patch.
>>>
>>> On 12/06/2011 03:39 PM, Thierry Reding wrote:
>>>> Function parameters on subsequent lines should never be aligned with
>>>> the
>>>> function name but rather be indented.
>>> [...]
>>>> usb_set_interface(dev->udev,
>>>> - dev->isoc_in.bInterfaceNumber,
>>>> - 0);
>>>> + dev->isoc_in.bInterfaceNumber, 0);
>>>
>>> Which kind of indentation should be used when function params are
>>> slitted to multiple lines?
>
> Documentation/CodingStyle currently says:
>
> Statements longer than 80 columns will be broken into sensible chunks,
> unless
> exceeding 80 columns significantly increases readability and does not hide
> information. Descendants are always substantially shorter than the
> parent and
> are placed substantially to the right. The same applies to function headers
> with a long argument list. However, never break user-visible strings
> such as
> printk messages, because that breaks the ability to grep for them.
>
> So, it should be: "substantially to the right" whatever this means.
>
>> I don't think this is documented anywhere and there are no hard rules
>> with
>> regard to this. I guess anything is fine as long as it is indented at
>> all.
>>
>>> In that case two tabs are used (related to function indentation).
>>> example:
>>> ret= function(param1,
>>> param2);
>>
>> I usually use that because it is my text editor's default.
>>
>>> Other generally used is only one tab (related to function indentation).
>>> example:
>>> ret= function(param1,
>>> param2);
>>
>> I think that's okay as well.
>
> One tab can hardly be interpreted as "substantially to the right".
>
>>
>>> And last generally used is multiple tabs + spaces until same
>>> location where first param is meet (related to function
>>> indentation). I see that bad since use of tabs, with only spaces I
>>> see it fine. And this many times leads situation param level are
>>> actually different whilst originally idea was to put those same
>>> level.
>>> example:
>>> ret= function(param1,
>>> param2);
>
> In practice, this is the most commonly used way, from what I noticed,
> not only
> at drivers/media. A good place to look for commonly used CodingStyle are
> the
> most used headers at include/linux. As far as I noticed, they all use this
> style.

Yes, but it is not correct according to CodingStyle if you use spaces 
even when mixing with tabs.

Correct seems to be intend it adding only tabs, as many as possible, 
still not to exceed 80 char line len limit.


>> Whether this works or not always depends on the tab-width. I think most
>> variations are okay here. Some people like to align them, other people
>> don't.
>
> Tab width is always 8, according with the CodingStyle:
>
> "Tabs are 8 characters, and thus indentations are also 8 characters."
>
> Regards,
> Mauro
>


-- 
http://palosaari.fi/

Re: [PATCH 2/2] [media] tm6000: Fix bad indentation.

[Mauro Carvalho Chehab]

On 06-12-2011 19:03, Antti Palosaari wrote:
> On 12/06/2011 10:58 PM, Mauro Carvalho Chehab wrote:
>> On 06-12-2011 12:13, Thierry Reding wrote:
>>> * Antti Palosaari wrote:
>>>> That question is related to that kind of indentation generally, not
>>>> only that patch.
>>>>
>>>> On 12/06/2011 03:39 PM, Thierry Reding wrote:
>>>>> Function parameters on subsequent lines should never be aligned with
>>>>> the
>>>>> function name but rather be indented.
>>>> [...]
>>>>> usb_set_interface(dev->udev,
>>>>> - dev->isoc_in.bInterfaceNumber,
>>>>> - 0);
>>>>> + dev->isoc_in.bInterfaceNumber, 0);
>>>>
>>>> Which kind of indentation should be used when function params are
>>>> slitted to multiple lines?
>>
>> Documentation/CodingStyle currently says:
>>
>> Statements longer than 80 columns will be broken into sensible chunks,
>> unless
>> exceeding 80 columns significantly increases readability and does not hide
>> information. Descendants are always substantially shorter than the
>> parent and
>> are placed substantially to the right. The same applies to function headers
>> with a long argument list. However, never break user-visible strings
>> such as
>> printk messages, because that breaks the ability to grep for them.
>>
>> So, it should be: "substantially to the right" whatever this means.
>>
>>> I don't think this is documented anywhere and there are no hard rules
>>> with
>>> regard to this. I guess anything is fine as long as it is indented at
>>> all.
>>>
>>>> In that case two tabs are used (related to function indentation).
>>>> example:
>>>> ret= function(param1,
>>>> param2);
>>>
>>> I usually use that because it is my text editor's default.
>>>
>>>> Other generally used is only one tab (related to function indentation).
>>>> example:
>>>> ret= function(param1,
>>>> param2);
>>>
>>> I think that's okay as well.
>>
>> One tab can hardly be interpreted as "substantially to the right".
>>
>>>
>>>> And last generally used is multiple tabs + spaces until same
>>>> location where first param is meet (related to function
>>>> indentation). I see that bad since use of tabs, with only spaces I
>>>> see it fine. And this many times leads situation param level are
>>>> actually different whilst originally idea was to put those same
>>>> level.
>>>> example:
>>>> ret= function(param1,
>>>> param2);
>>
>> In practice, this is the most commonly used way, from what I noticed,
>> not only
>> at drivers/media. A good place to look for commonly used CodingStyle are
>> the
>> most used headers at include/linux. As far as I noticed, they all use this
>> style.
>
> Yes, but it is not correct according to CodingStyle if you use spaces even when mixing with tabs.
>
> Correct seems to be intend it adding only tabs, as many as possible, still not to exceed 80 char line len limit.

This is not indentation, it is long line breaking. There's nothing there
saying that white spaces are not allowed on line breaks, nor checkpatch
complains about it.

So, it seems to be the better way for doing it, although CodingStyle doesn't
enforce it.

>
>
>>> Whether this works or not always depends on the tab-width. I think most
>>> variations are okay here. Some people like to align them, other people
>>> don't.
>>
>> Tab width is always 8, according with the CodingStyle:
>>
>> "Tabs are 8 characters, and thus indentations are also 8 characters."
>>
>> Regards,
>> Mauro
>>
>
>