em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Dennis Sperlich]

Hi,

I have a Terratec Cinergy HTC Stick an tried the new support for the 
DVB-C part. It works for SD material (at least for free receivable 
stations, I tried afair only QAM64), but did not for HD stations 
(QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD 
(via KabelDeutschland). The HD material was just digital artefacts, as 
far as mplayer could decode it. When I did a dumpstream and looked at 
the resulting file size I got something about 1MB/s which seems a little 
too low, because SD was already about 870kB/s. After looking around I 
found a solution in increasing the isoc_dvb_max_packetsize from 752 to 
940 (multiple of 188). Then an HD stream was about 1.4MB/s and looked 
good. I'm not sure, whether this is the correct fix, but it works for me.

If you need more testing pleas tell.

Regards,
Dennis



index 804a4ab..c518d13 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
                  * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
                  * but not enough for 44 Mbit DVB-C.
                  */
-               packet_size = 752;
+               packet_size = 940;
         }

         return packet_size;

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Mauro Carvalho Chehab]

On 24-12-2011 19:58, Dennis Sperlich wrote:
> Hi,
> 
> I have a Terratec Cinergy HTC Stick an tried the new support for the DVB-C part. It works for SD material (at least for free receivable stations, I tried afair only QAM64), but did not for HD stations (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD (via KabelDeutschland). The HD material was just digital artefacts, as far as mplayer could decode it. When I did a dumpstream and looked at the resulting file size I got something about 1MB/s which seems a little too low, because SD was already about 870kB/s. After looking around I found a solution in increasing the isoc_dvb_max_packetsize from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s and looked good. I'm not sure, whether this is the correct fix, but it works for me.
> 
> If you need more testing pleas tell.
> 
> Regards,
> Dennis
> 
> 
> 
> index 804a4ab..c518d13 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>                  * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
>                  * but not enough for 44 Mbit DVB-C.
>                  */
> -               packet_size = 752;
> +               packet_size = 940;
>         }
> 
>         return packet_size;

As you can see there at the code, the packet size depends on the chipset, as
not all will support 940 for packet size.

Could you please provide us what was the chip detected id?

It should be a message on your dmesg like:

	chip ID is em2870
or
	em28xx chip ID = 38

The patch should change it only for your specific chipset model, in order to
avoid regressions to other supported chipsets, like:

int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
{
        unsigned int chip_cfg2;
        unsigned int packet_size;

        switch (dev->chip_id) {
        case CHIP_ID_EM2710:
        case CHIP_ID_EM2750:
        case CHIP_ID_EM2800:
        case CHIP_ID_EM2820:
...
	case CHIP_ID_foo:
		packet_size = 940;
...
        }

        return packet_size;
}

The case you're touching seems to be for em2884, but the switch covers both
em2884 and em28174 (plus the default for newer chipsets).

Maybe Michael/Devin may have something to say, with regards to a way to detect
the maximum supported packet size by each specific em28xx model.

Regards,
Mauro

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Dennis Sperlich]

On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
> On 24-12-2011 19:58, Dennis Sperlich wrote:
>> Hi,
>>
>> I have a Terratec Cinergy HTC Stick an tried the new support for the DVB-C part. It works for SD material (at least for free receivable stations, I tried afair only QAM64), but did not for HD stations (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD (via KabelDeutschland). The HD material was just digital artefacts, as far as mplayer could decode it. When I did a dumpstream and looked at the resulting file size I got something about 1MB/s which seems a little too low, because SD was already about 870kB/s. After looking around I found a solution in increasing the isoc_dvb_max_packetsize from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s and looked good. I'm not sure, whether this is the correct fix, but it works for me.
>>
>> If you need more testing pleas tell.
>>
>> Regards,
>> Dennis
>>
>>
>>
>> index 804a4ab..c518d13 100644
>> --- a/drivers/media/video/em28xx/em28xx-core.c
>> +++ b/drivers/media/video/em28xx/em28xx-core.c
>> @@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>>                   * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
>>                   * but not enough for 44 Mbit DVB-C.
>>                   */
>> -               packet_size = 752;
>> +               packet_size = 940;
>>          }
>>
>>          return packet_size;
> As you can see there at the code, the packet size depends on the chipset, as
> not all will support 940 for packet size.
>
> Could you please provide us what was the chip detected id?
>
> It should be a message on your dmesg like:
>
> 	chip ID is em2870
> or
> 	em28xx chip ID = 38
>
> The patch should change it only for your specific chipset model, in order to
> avoid regressions to other supported chipsets, like:
>
> int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
> {
>          unsigned int chip_cfg2;
>          unsigned int packet_size;
>
>          switch (dev->chip_id) {
>          case CHIP_ID_EM2710:
>          case CHIP_ID_EM2750:
>          case CHIP_ID_EM2800:
>          case CHIP_ID_EM2820:
> ...
> 	case CHIP_ID_foo:
> 		packet_size = 940;
> ...
>          }
>
>          return packet_size;
> }
>
> The case you're touching seems to be for em2884, but the switch covers both
> em2884 and em28174 (plus the default for newer chipsets).

dmesg says: em28xx #0: chip ID is em2884

so a patch would be more like:

index 804a4ab..9280251 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -1151,6 +1151,8 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
                 packet_size = 564;
                 break;
         case CHIP_ID_EM2884:
+               packet_size = 940;
+               break;
         case CHIP_ID_EM28174:
         default:
                 /*

> Maybe Michael/Devin may have something to say, with regards to a way to detect
> the maximum supported packet size by each specific em28xx model.

This would be fine, I tried also 1128 (6 times 188), but then mplayer 
did not play any more. I then tried 1034, but then I got the message " 
submit of urb 0 failed (error=-90)", so I guess there have to be integer 
multiples of 188.

Regards,
Dennis

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Hans Petter Selasky]

On Sunday 25 December 2011 15:04:17 Dennis Sperlich wrote:
> On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
> > On 24-12-2011 19:58, Dennis Sperlich wrote:
> >> Hi,
> >> 
> >> I have a Terratec Cinergy HTC Stick an tried the new support for the
> >> DVB-C part. It works for SD material (at least for free receivable
> >> stations, I tried afair only QAM64), but did not for HD stations
> >> (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD
> >> (via KabelDeutschland). The HD material was just digital artefacts, as
> >> far as mplayer could decode it. When I did a dumpstream and looked at
> >> the resulting file size I got something about 1MB/s which seems a
> >> little too low, because SD was already about 870kB/s. After looking
> >> around I found a solution in increasing the isoc_dvb_max_packetsize
> >> from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s
> >> and looked good. I'm not sure, whether this is the correct fix, but it
> >> works for me.
> >> 
> >> If you need more testing pleas tell.
> >> 
> >> Regards,
> >> Dennis
> >> 

These numbers should not be hardcoded, but extracted from the USB endpoint 
descriptor!

--HPS

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Devin Heitmueller]

On Sun, Dec 25, 2011 at 9:11 AM, Hans Petter Selasky <hselasky@c2i.net> wrote:
> These numbers should not be hardcoded, but extracted from the USB endpoint
> descriptor!
>
> --HPS

Hans is correct.  I only hard-coded it at 564 as a quick hack when I
was bootstrapping the em2784 support.  The code really should be
cleaned up to base it off of the endpoint descriptors.

Patches certainly welcome as this is indeed a known issue.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Mauro Carvalho Chehab]

On 25-12-2011 12:11, Hans Petter Selasky wrote:
> On Sunday 25 December 2011 15:04:17 Dennis Sperlich wrote:
>> On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
>>> On 24-12-2011 19:58, Dennis Sperlich wrote:
>>>> Hi,
>>>>
>>>> I have a Terratec Cinergy HTC Stick an tried the new support for the
>>>> DVB-C part. It works for SD material (at least for free receivable
>>>> stations, I tried afair only QAM64), but did not for HD stations
>>>> (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD
>>>> (via KabelDeutschland). The HD material was just digital artefacts, as
>>>> far as mplayer could decode it. When I did a dumpstream and looked at
>>>> the resulting file size I got something about 1MB/s which seems a
>>>> little too low, because SD was already about 870kB/s. After looking
>>>> around I found a solution in increasing the isoc_dvb_max_packetsize
>>>> from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s
>>>> and looked good. I'm not sure, whether this is the correct fix, but it
>>>> works for me.
>>>>
>>>> If you need more testing pleas tell.
>>>>
>>>> Regards,
>>>> Dennis
>>>>
> 
> These numbers should not be hardcoded, but extracted from the USB endpoint 
> descriptor!

The driver retrieves the values from the USB endpoints during probe. This
is used for analog mode. However, on DVB mode, the values are hardcoded by
the ones that had/have access to Empiatech datasheets (Unfortunately,
I don't have them).

So, it is hard to know if the current limit is due to some chipset bug that
doesn't report well the maximum packet size, or if it is just due to the lack
of time for the ones that wrote the code to actually use the reported values.

Fixing the driver to use the USB descriptors is easy. The hard part is to be
sure that it won't break for the ones with older chipsets.

Regards,
Mauro.

> 
> --HPS

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Mauro Carvalho Chehab]

On 25-12-2011 12:04, Dennis Sperlich wrote:
> On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
>> On 24-12-2011 19:58, Dennis Sperlich wrote:
>>> Hi,
>>>
>>> I have a Terratec Cinergy HTC Stick an tried the new support for the DVB-C part. It works for SD material (at least for free receivable stations, I tried afair only QAM64), but did not for HD stations (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD (via KabelDeutschland). The HD material was just digital artefacts, as far as mplayer could decode it. When I did a dumpstream and looked at the resulting file size I got something about 1MB/s which seems a little too low, because SD was already about 870kB/s. After looking around I found a solution in increasing the isoc_dvb_max_packetsize from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s and looked good. I'm not sure, whether this is the correct fix, but it works for me.
>>>
>>> If you need more testing pleas tell.
>>>
>>> Regards,
>>> Dennis
>>>
>>>
>>>
>>> index 804a4ab..c518d13 100644
>>> --- a/drivers/media/video/em28xx/em28xx-core.c
>>> +++ b/drivers/media/video/em28xx/em28xx-core.c
>>> @@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>>>                   * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
>>>                   * but not enough for 44 Mbit DVB-C.
>>>                   */
>>> -               packet_size = 752;
>>> +               packet_size = 940;
>>>          }
>>>
>>>          return packet_size;
>> As you can see there at the code, the packet size depends on the chipset, as
>> not all will support 940 for packet size.
>>
>> Could you please provide us what was the chip detected id?
>>
>> It should be a message on your dmesg like:
>>
>>     chip ID is em2870
>> or
>>     em28xx chip ID = 38
>>
>> The patch should change it only for your specific chipset model, in order to
>> avoid regressions to other supported chipsets, like:
>>
>> int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>> {
>>          unsigned int chip_cfg2;
>>          unsigned int packet_size;
>>
>>          switch (dev->chip_id) {
>>          case CHIP_ID_EM2710:
>>          case CHIP_ID_EM2750:
>>          case CHIP_ID_EM2800:
>>          case CHIP_ID_EM2820:
>> ...
>>     case CHIP_ID_foo:
>>         packet_size = 940;
>> ...
>>          }
>>
>>          return packet_size;
>> }
>>
>> The case you're touching seems to be for em2884, but the switch covers both
>> em2884 and em28174 (plus the default for newer chipsets).
> 
> dmesg says: em28xx #0: chip ID is em2884
> 
> so a patch would be more like:
> 
> index 804a4ab..9280251 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -1151,6 +1151,8 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>                 packet_size = 564;
>                 break;
>         case CHIP_ID_EM2884:
> +               packet_size = 940;
> +               break;
>         case CHIP_ID_EM28174:
>         default:
>                 /*
> 
>> Maybe Michael/Devin may have something to say, with regards to a way to detect
>> the maximum supported packet size by each specific em28xx model.
> 
> This would be fine, I tried also 1128 (6 times 188), but then mplayer did not play any more.
> I then tried 1034, but then I got the message " submit of urb 0 failed (error=-90)", so I guess 
> there have to be integer multiples of 188.

The maximum packet size is described at the USB descriptors.

The code at em28xx_set_alternate() seeks for the alternates and selects
the one that will get the best alternate for the analog mode.

There, you'll see a code that gets the best alternate for analog,
and selects the proper packet size for it.

For DVB, it should be a little different, as, on DVB, the driver doesn't
know, in advance, what's the streaming rate. So, the driver needs to get
a high enough packet size to fit for the bandwidth needs.

Also, for DVB, the driver does:
	usb_set_interface(dev->udev, 0, 1);

(e. g. it always use alternate 1)

So, in thesis, that function could be as simple as:
	dev->alt_max_pkt_size[1];

(if the max packet size for alternate 1 would work for all chips)

Or, eventually, the code might be using other alternates for HD, 
but I'm not sure if those chipsets support a different alternate for DVB.

Regards,
Mauro

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Malcolm Priestley]

On Sun, 2011-12-25 at 15:11 +0100, Hans Petter Selasky wrote:
> On Sunday 25 December 2011 15:04:17 Dennis Sperlich wrote:
> > On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
> > > On 24-12-2011 19:58, Dennis Sperlich wrote:
> > >> Hi,
> > >> 
> > >> I have a Terratec Cinergy HTC Stick an tried the new support for the
> > >> DVB-C part. It works for SD material (at least for free receivable
> > >> stations, I tried afair only QAM64), but did not for HD stations
> > >> (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD
> > >> (via KabelDeutschland). The HD material was just digital artefacts, as
> > >> far as mplayer could decode it. When I did a dumpstream and looked at
> > >> the resulting file size I got something about 1MB/s which seems a
> > >> little too low, because SD was already about 870kB/s. After looking
> > >> around I found a solution in increasing the isoc_dvb_max_packetsize
> > >> from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s
> > >> and looked good. I'm not sure, whether this is the correct fix, but it
> > >> works for me.
> > >> 

Would not increasing EM28XX_DVB_NUM_BUFS currently set at 5 to say 10
have a better effect?

Malcolm

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Dennis Sperlich]

On 25.12.2011 20:42, Malcolm Priestley wrote:
> On Sun, 2011-12-25 at 15:11 +0100, Hans Petter Selasky wrote:
>> On Sunday 25 December 2011 15:04:17 Dennis Sperlich wrote:
>>> On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
>>>> On 24-12-2011 19:58, Dennis Sperlich wrote:
>>>>> Hi,
>>>>>
>>>>> I have a Terratec Cinergy HTC Stick an tried the new support for the
>>>>> DVB-C part. It works for SD material (at least for free receivable
>>>>> stations, I tried afair only QAM64), but did not for HD stations
>>>>> (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD
>>>>> (via KabelDeutschland). The HD material was just digital artefacts, as
>>>>> far as mplayer could decode it. When I did a dumpstream and looked at
>>>>> the resulting file size I got something about 1MB/s which seems a
>>>>> little too low, because SD was already about 870kB/s. After looking
>>>>> around I found a solution in increasing the isoc_dvb_max_packetsize
>>>>> from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s
>>>>> and looked good. I'm not sure, whether this is the correct fix, but it
>>>>> works for me.
>>>>>
> Would not increasing EM28XX_DVB_NUM_BUFS currently set at 5 to say 10
> have a better effect?
This does not work, I just tried it.

Regards,
Dennis

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Dennis Sperlich]

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

On 25.12.2011 19:13, Mauro Carvalho Chehab wrote:
> On 25-12-2011 12:04, Dennis Sperlich wrote:
>> On 25.12.2011 11:52, Mauro Carvalho Chehab wrote:
>>> On 24-12-2011 19:58, Dennis Sperlich wrote:
>>>> Hi,
>>>>
>>>> I have a Terratec Cinergy HTC Stick an tried the new support for the DVB-C part. It works for SD material (at least for free receivable stations, I tried afair only QAM64), but did not for HD stations (QAM256). I have only access to unencrypted ARD HD, ZDF HD and arte HD (via KabelDeutschland). The HD material was just digital artefacts, as far as mplayer could decode it. When I did a dumpstream and looked at the resulting file size I got something about 1MB/s which seems a little too low, because SD was already about 870kB/s. After looking around I found a solution in increasing the isoc_dvb_max_packetsize from 752 to 940 (multiple of 188). Then an HD stream was about 1.4MB/s and looked good. I'm not sure, whether this is the correct fix, but it works for me.
>>>>
>>>> If you need more testing pleas tell.
>>>>
>>>> Regards,
>>>> Dennis
>>>>
>>>>
>>>>
>>>> index 804a4ab..c518d13 100644
>>>> --- a/drivers/media/video/em28xx/em28xx-core.c
>>>> +++ b/drivers/media/video/em28xx/em28xx-core.c
>>>> @@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>>>>                    * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
>>>>                    * but not enough for 44 Mbit DVB-C.
>>>>                    */
>>>> -               packet_size = 752;
>>>> +               packet_size = 940;
>>>>           }
>>>>
>>>>           return packet_size;
>>> As you can see there at the code, the packet size depends on the chipset, as
>>> not all will support 940 for packet size.
>>>
>>> Could you please provide us what was the chip detected id?
>>>
>>> It should be a message on your dmesg like:
>>>
>>>      chip ID is em2870
>>> or
>>>      em28xx chip ID = 38
>>>
>>> The patch should change it only for your specific chipset model, in order to
>>> avoid regressions to other supported chipsets, like:
>>>
>>> int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>>> {
>>>           unsigned int chip_cfg2;
>>>           unsigned int packet_size;
>>>
>>>           switch (dev->chip_id) {
>>>           case CHIP_ID_EM2710:
>>>           case CHIP_ID_EM2750:
>>>           case CHIP_ID_EM2800:
>>>           case CHIP_ID_EM2820:
>>> ...
>>>      case CHIP_ID_foo:
>>>          packet_size = 940;
>>> ...
>>>           }
>>>
>>>           return packet_size;
>>> }
>>>
>>> The case you're touching seems to be for em2884, but the switch covers both
>>> em2884 and em28174 (plus the default for newer chipsets).
>> dmesg says: em28xx #0: chip ID is em2884
>>
>> so a patch would be more like:
>>
>> index 804a4ab..9280251 100644
>> --- a/drivers/media/video/em28xx/em28xx-core.c
>> +++ b/drivers/media/video/em28xx/em28xx-core.c
>> @@ -1151,6 +1151,8 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>>                  packet_size = 564;
>>                  break;
>>          case CHIP_ID_EM2884:
>> +               packet_size = 940;
>> +               break;
>>          case CHIP_ID_EM28174:
>>          default:
>>                  /*
>>
>>> Maybe Michael/Devin may have something to say, with regards to a way to detect
>>> the maximum supported packet size by each specific em28xx model.
>> This would be fine, I tried also 1128 (6 times 188), but then mplayer did not play any more.
>> I then tried 1034, but then I got the message " submit of urb 0 failed (error=-90)", so I guess
>> there have to be integer multiples of 188.
> The maximum packet size is described at the USB descriptors.
>
> The code at em28xx_set_alternate() seeks for the alternates and selects
> the one that will get the best alternate for the analog mode.
>
> There, you'll see a code that gets the best alternate for analog,
> and selects the proper packet size for it.
>
> For DVB, it should be a little different, as, on DVB, the driver doesn't
> know, in advance, what's the streaming rate. So, the driver needs to get
> a high enough packet size to fit for the bandwidth needs.
>
> Also, for DVB, the driver does:
> 	usb_set_interface(dev->udev, 0, 1);
>
> (e. g. it always use alternate 1)
>
> So, in thesis, that function could be as simple as:
> 	dev->alt_max_pkt_size[1];
>
> (if the max packet size for alternate 1 would work for all chips)
>
> Or, eventually, the code might be using other alternates for HD,
> but I'm not sure if those chipsets support a different alternate for DVB.
I just tried, replacing
     max_dvb_packet_size = em28xx_isoc_dvb_max_packetsize(dev);
by
     max_dvb_packet_size = dev->alt_max_pkt_size[1];

but it did not work. Was this the correct replacement?

     printk(KERN_INFO "dev->alt_max_pkt_size[1] is 
%i\n",dev->alt_max_pkt_size[1]);

then said, dev->alt_max_pkt_size[1] is 0.

I also attachted  a lsusb -v output for the Terratec Cinergy HTC Stick. 
I don't know, which of these endpoints the dvb-c part is, but it may be 
anyway usefull.

Regard,
Dennis

[-- Attachment #2: lsusb --]
[-- Type: text/plain, Size: 16063 bytes --]


Bus 001 Device 009: ID 0ccd:00b2 TerraTec Electronic GmbH 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x0ccd TerraTec Electronic GmbH
  idProduct          0x00b2 
  bcdDevice            1.00
  iManufacturer           2 TERRATEC
  iProduct                1 Cinergy_HTC_Stick
  iSerial                 3 02?TERRATE
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength          305
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0000  1x 0 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0000  1x 0 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0000  1x 0 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       1
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0000  1x 0 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       2
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0ad0  2x 720 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       3
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0c00  2x 1024 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       4
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x1300  3x 768 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       5
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x1380  3x 896 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       6
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x13c0  3x 960 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       7
      bNumEndpoints           4
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol    255 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0001  1x 1 bytes
        bInterval              11
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x1400  3x 1024 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x00c4  1x 196 bytes
        bInterval               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            1
          Transfer Type            Isochronous
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x03ac  1x 940 bytes
        bInterval               1
Device Qualifier (for other device speed):
  bLength                10
  bDescriptorType         6
  bcdUSB               2.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  bNumConfigurations      1
Device Status:     0x0000
  (Bus Powered)

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Holger Nelson]

Hi!

On Sun, 25 Dec 2011, Dennis Sperlich wrote:

> I just tried, replacing
>    max_dvb_packet_size = em28xx_isoc_dvb_max_packetsize(dev);
> by
>    max_dvb_packet_size = dev->alt_max_pkt_size[1];
>
> but it did not work. Was this the correct replacement?
>
>    printk(KERN_INFO "dev->alt_max_pkt_size[1] is 
> %i\n",dev->alt_max_pkt_size[1]);
>
> then said, dev->alt_max_pkt_size[1] is 0.
>
> I also attachted  a lsusb -v output for the Terratec Cinergy HTC Stick. I 
> don't know, which of these endpoints the dvb-c part is, but it may be anyway 
> usefull.

Is it possible, that dev->alt_max_pkt_size gets the maximum packet sizes 
for the analog video input endpoint (0x82 in lsusb output)? The patch 
below worked during a small test, but I doubt that it is the best way to 
do it.

While still looking at it: Is there a reason to allocate 32 bytes per 
alternate interface configuration if we only store one unsigned int per 
configuration in it? - I just copied the allocation code from above.

Holger

diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 1704da0..70866c5 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3269,6 +3273,30 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  		dev->alt_max_pkt_size[i] = size;
  	}

+	dev->alt_dvb_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);
+
+	if (dev->alt_dvb_max_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
+	}
+
+	for (i = 0; i < dev->num_alt ; i++) {
+	        int ep;
+		for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
+			struct usb_host_endpoint *e = &interface->altsetting[i].endpoint[ep];
+			if (e->desc.bEndpointAddress == 0x84) {
+				u16 tmp = le16_to_cpu(e->desc.wMaxPacketSize);
+				unsigned int size = tmp & 0x7ff;
+				if (udev->speed == USB_SPEED_HIGH)
+					size = size * hb_mult(tmp);
+
+				dev->alt_dvb_max_pkt_size[i] = size;
+			}
+		}
+	}
+
  	if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
  		dev->model = card[nr];

@@ -3281,6 +3309,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  	retval = em28xx_init_dev(&dev, udev, interface, nr);
  	if (retval) {
  		mutex_unlock(&dev->lock);
+		kfree(dev->alt_dvb_max_pkt_size);
  		kfree(dev->alt_max_pkt_size);
  		kfree(dev);
  		goto err;
@@ -3365,6 +3394,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
  	em28xx_close_extension(dev);

  	if (!dev->users) {
+		kfree(dev->alt_dvb_max_pkt_size);
  		kfree(dev->alt_max_pkt_size);
  		kfree(dev);
  	}
diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index 804a4ab..e7d3541 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
  		 * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
  		 * but not enough for 44 Mbit DVB-C.
  		 */
-		packet_size = 752;
+		packet_size = dev->alt_dvb_max_pkt_size[1];
  	}

  	return packet_size;
diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
index 9b4557a..2491a2c 100644
--- a/drivers/media/video/em28xx/em28xx-video.c
+++ b/drivers/media/video/em28xx/em28xx-video.c
@@ -2254,6 +2254,7 @@ static int em28xx_v4l2_close(struct file *filp)
  		   free the remaining resources */
  		if (dev->state & DEV_DISCONNECTED) {
  			em28xx_release_resources(dev);
+			kfree(dev->alt_dvb_max_pkt_size);
  			kfree(dev->alt_max_pkt_size);
  			kfree(dev);
  			return 0;
diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index b1199ef..793c85a 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -597,6 +597,7 @@ struct em28xx {
  	int max_pkt_size;	/* max packet size of isoc transaction */
  	int num_alt;		/* Number of alternative settings */
  	unsigned int *alt_max_pkt_size;	/* array of wMaxPacketSize */
+	unsigned int *alt_dvb_max_pkt_size;
  	struct urb *urb[EM28XX_NUM_BUFS];	/* urb for isoc transfers */
  	char *transfer_buffer[EM28XX_NUM_BUFS];	/* transfer buffers for isoc
  						   transfer */

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Mauro Carvalho Chehab]

On 26-12-2011 03:55, Holger Nelson wrote:
> Hi!
> 
> On Sun, 25 Dec 2011, Dennis Sperlich wrote:
> 
>> I just tried, replacing
>>    max_dvb_packet_size = em28xx_isoc_dvb_max_packetsize(dev);
>> by
>>    max_dvb_packet_size = dev->alt_max_pkt_size[1];
>>
>> but it did not work. Was this the correct replacement?
>>
>>    printk(KERN_INFO "dev->alt_max_pkt_size[1] is %i\n",dev->alt_max_pkt_size[1]);
>>
>> then said, dev->alt_max_pkt_size[1] is 0.
>>
>> I also attachted  a lsusb -v output for the Terratec Cinergy HTC Stick. I don't know, which of these endpoints the dvb-c part is, but it may be anyway usefull.
> 
> Is it possible, that dev->alt_max_pkt_size gets the maximum packet sizes for the analog video input endpoint (0x82 in lsusb output)? The patch below worked during a small test, but I doubt that it is the best way to do it.
> 
> While still looking at it: Is there a reason to allocate 32 bytes per alternate interface configuration if we only store one unsigned int per configuration in it? - I just copied the allocation code from above.
> 
> Holger
> 
> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
> index 1704da0..70866c5 100644
> --- a/drivers/media/video/em28xx/em28xx-cards.c
> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> @@ -3269,6 +3273,30 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>          dev->alt_max_pkt_size[i] = size;
>      }
> 
> +    dev->alt_dvb_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);

It is likely over-allocating memory. The proper way would be to replace 32 by
sizeof(dev->alt_dvb_max_pkt_size[0]).

> +
> +    if (dev->alt_dvb_max_pkt_size == NULL) {
> +        em28xx_errdev("out of memory!\n");
> +        kfree(dev);
> +        retval = -ENOMEM;
> +        goto err;
> +    }
> +
> +    for (i = 0; i < dev->num_alt ; i++) {
> +            int ep;
> +        for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
> +            struct usb_host_endpoint *e = &interface->altsetting[i].endpoint[ep];
> +            if (e->desc.bEndpointAddress == 0x84) {

Yeah, this looks correct. 
Yet, I would avoid create another loop here. Also, there are too many
hacks at the em28xx probe utility. Take a look at the tm6000-cards.c.
It used to be similar to em28xx probe, but the code now looks better.

IMO, we should add, at em28xx-regs:

#define EM28XX_EP_ANALOG	0x82
#define EM28XX_EP_AUDIO		0x83
#define EM28XX_EP_DIGITAL	0x84

and use those macros a	

> +                u16 tmp = le16_to_cpu(e->desc.wMaxPacketSize);
> +                unsigned int size = tmp & 0x7ff;
> +                if (udev->speed == USB_SPEED_HIGH)
> +                    size = size * hb_mult(tmp);
> +
> +                dev->alt_dvb_max_pkt_size[i] = size;
> +            }
> +        }
> +    }
> +
>      if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
>          dev->model = card[nr];
> 
> @@ -3281,6 +3309,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>      retval = em28xx_init_dev(&dev, udev, interface, nr);
>      if (retval) {
>          mutex_unlock(&dev->lock);
> +        kfree(dev->alt_dvb_max_pkt_size);
>          kfree(dev->alt_max_pkt_size);
>          kfree(dev);
>          goto err;
> @@ -3365,6 +3394,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>      em28xx_close_extension(dev);
> 
>      if (!dev->users) {
> +        kfree(dev->alt_dvb_max_pkt_size);
>          kfree(dev->alt_max_pkt_size);
>          kfree(dev);
>      }
> diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
> index 804a4ab..e7d3541 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -1157,7 +1157,7 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>           * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
>           * but not enough for 44 Mbit DVB-C.
>           */
> -        packet_size = 752;
> +        packet_size = dev->alt_dvb_max_pkt_size[1];
>      }
> 
>      return packet_size;
> diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
> index 9b4557a..2491a2c 100644
> --- a/drivers/media/video/em28xx/em28xx-video.c
> +++ b/drivers/media/video/em28xx/em28xx-video.c
> @@ -2254,6 +2254,7 @@ static int em28xx_v4l2_close(struct file *filp)
>             free the remaining resources */
>          if (dev->state & DEV_DISCONNECTED) {
>              em28xx_release_resources(dev);
> +            kfree(dev->alt_dvb_max_pkt_size);
>              kfree(dev->alt_max_pkt_size);
>              kfree(dev);
>              return 0;
> diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
> index b1199ef..793c85a 100644
> --- a/drivers/media/video/em28xx/em28xx.h
> +++ b/drivers/media/video/em28xx/em28xx.h
> @@ -597,6 +597,7 @@ struct em28xx {
>      int max_pkt_size;    /* max packet size of isoc transaction */
>      int num_alt;        /* Number of alternative settings */
>      unsigned int *alt_max_pkt_size;    /* array of wMaxPacketSize */
> +    unsigned int *alt_dvb_max_pkt_size;
>      struct urb *urb[EM28XX_NUM_BUFS];    /* urb for isoc transfers */
>      char *transfer_buffer[EM28XX_NUM_BUFS];    /* transfer buffers for isoc
>                             transfer */

The above code looks promising.

I'm currently without time right now to work on a patch, but I think that several hacks
inside the em28xx probe should be removed, including the one that detects the endpoint
based on the packet size.

As it is easier to code than to explain in words, the code below could be
a start (ok, it doesn't compile, doesn't remove all hacks, doesn't free memory, etc...)
Feel free to use it as a start for a real patch, if you wish.


diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 1704da0..1bd4ef5 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3087,8 +3087,8 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	struct usb_device *udev;
 	struct em28xx *dev = NULL;
 	int retval;
-	bool is_audio_only = false, has_audio = false;
-	int i, nr, isoc_pipe;
+	bool has_audio = false, has_video = false, has_dvb = false;
+	int i, nr, sizedescr, size;
 	const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 	char *speed;
 	char descr[255] = "";
@@ -3120,6 +3120,32 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		goto err;
 	}
 
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		em28xx_err(DRIVER_NAME ": out of memory!\n");
+		retval = -ENOMEM;
+		goto err;
+	}
+
+	/* compute alternate max packet sizes */
+	dev->alt_max_pkt_size = kmalloc(sizeof(dev->alt_max_pkt_size[0] *  interface->num_altsetting, GFP_KERNEL);
+	if (dev->alt_max_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
+	}
+
+	dev->alt_dvb_pkt_size = kmalloc(sizeof(dev->alt_max_pkt_size[0] *  interface->num_altsetting, GFP_KERNEL);
+	if (dev->alt_dvb_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree();
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
+	}
+
 	/* Get endpoints */
 	for (i = 0; i < interface->num_altsetting; i++) {
 		int ep;
@@ -3128,8 +3154,23 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 			struct usb_host_endpoint	*e;
 			e = &interface->altsetting[i].endpoint[ep];
 
-			if (e->desc.bEndpointAddress == 0x83)
+			sizedescr = le16_to_cpu(interface->altsetting[i].endpoint[ep].desc.wMaxPacketSize);
+			size = sizedescr & 0x7fff;
+			if (udev->speed == USB_SPEED_HIGH)
+				size = size * hb_mult(sizedescr);
+
+			switch (e->desc.bEndpointAddress) {
+			case EM28XX_EP_AUDIO:
 				has_audio = true;
+				break;
+			case EM28XX_EP_ANALOG:
+				has_video = true;
+				dev->alt_max_pkt_size[i] = size;
+				break;
+			case EM28XX_EP_DIGITAL:
+				has_dvb = true;
+				dev->alt_dvb_pkt_size[i] = size;
+				break;
 		}
 	}
 
@@ -3223,14 +3264,6 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		goto err;
 	}
 
-	/* allocate memory for our device state and initialize it */
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (dev == NULL) {
-		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		retval = -ENOMEM;
-		goto err;
-	}
-
 	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index 804a4ab..b73aaa5 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -1070,7 +1070,7 @@ int em28xx_init_isoc(struct em28xx *dev, int max_packets,
 			should also be using 'desc.bInterval'
 		 */
 		pipe = usb_rcvisocpipe(dev->udev,
-			dev->mode == EM28XX_ANALOG_MODE ? 0x82 : 0x84);
+			dev->mode == EM28XX_ANALOG_MODE ? EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
 
 		usb_fill_int_urb(urb, dev->udev, pipe,
 				 dev->isoc_ctl.transfer_buffer[i], sb_size,
diff --git a/drivers/media/video/em28xx/em28xx-reg.h b/drivers/media/video/em28xx/em28xx-reg.h
index 66f7923..2f62685 100644
--- a/drivers/media/video/em28xx/em28xx-reg.h
+++ b/drivers/media/video/em28xx/em28xx-reg.h
@@ -12,6 +12,11 @@
 #define EM_GPO_2   (1 << 2)
 #define EM_GPO_3   (1 << 3)
 
+/* em28xx endpoints */
+#define EM28XX_EP_ANALOG	0x82
+#define EM28XX_EP_AUDIO		0x83
+#define EM28XX_EP_DIGITAL	0x84
+
 /* em2800 registers */
 #define EM2800_R08_AUDIOSRC 0x08
 

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Holger Nelson]

On Mon, 26 Dec 2011, Mauro Carvalho Chehab wrote:

> I'm currently without time right now to work on a patch, but I think that several hacks
> inside the em28xx probe should be removed, including the one that detects the endpoint
> based on the packet size.
>
> As it is easier to code than to explain in words, the code below could be
> a start (ok, it doesn't compile, doesn't remove all hacks, doesn't free memory, etc...)
> Feel free to use it as a start for a real patch, if you wish.

I think, I filled the missing parts and removed most of the hacks in the 
probe code. The code works with my Cinergy HTC USB XS.

Holger

diff --git a/drivers/media/video/em28xx/em28xx-audio.c b/drivers/media/video/em28xx/em28xx-audio.c
index cff0768..e2a7b77 100644
--- a/drivers/media/video/em28xx/em28xx-audio.c
+++ b/drivers/media/video/em28xx/em28xx-audio.c
@@ -193,7 +193,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)

  		urb->dev = dev->udev;
  		urb->context = dev;
-		urb->pipe = usb_rcvisocpipe(dev->udev, 0x83);
+		urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
  		urb->transfer_flags = URB_ISO_ASAP;
  		urb->transfer_buffer = dev->adev.transfer_buffer[i];
  		urb->interval = 1;
diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index a7cfded..8082914 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3087,12 +3087,11 @@ unregister_dev:
  static int em28xx_usb_probe(struct usb_interface *interface,
  			    const struct usb_device_id *id)
  {
-	const struct usb_endpoint_descriptor *endpoint;
  	struct usb_device *udev;
  	struct em28xx *dev = NULL;
  	int retval;
-	bool is_audio_only = false, has_audio = false;
-	int i, nr, isoc_pipe;
+	bool has_audio = false, has_video = false, has_dvb = false;
+	int i, nr, sizedescr, size;
  	const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
  	char *speed;
  	char descr[255] = "";
@@ -3124,56 +3123,69 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  		goto err;
  	}

-	/* Get endpoints */
-	for (i = 0; i < interface->num_altsetting; i++) {
-		int ep;
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		em28xx_err(DRIVER_NAME ": out of memory!\n");
+		retval = -ENOMEM;
+		goto err;
+	}

-		for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
-			struct usb_host_endpoint	*e;
-			e = &interface->altsetting[i].endpoint[ep];
+	/* compute alternate max packet sizes */
+	dev->alt_video_max_pkt_size = kmalloc(sizeof(dev->alt_video_max_pkt_size[0]) * interface->num_altsetting, GFP_KERNEL);
+	if (dev->alt_video_max_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
+	}

-			if (e->desc.bEndpointAddress == 0x83)
-				has_audio = true;
-		}
+	dev->alt_dvb_max_pkt_size = kmalloc(sizeof(dev->alt_dvb_max_pkt_size[0]) * interface->num_altsetting, GFP_KERNEL);
+	if (dev->alt_dvb_max_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree(dev->alt_video_max_pkt_size);
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
  	}

-	endpoint = &interface->cur_altsetting->endpoint[0].desc;
+	/* Get endpoints */
+	for (i = 0; i < interface->num_altsetting; i++) {
+		int ep;

-	/* check if the device has the iso in endpoint at the correct place */
-	if (usb_endpoint_xfer_isoc(endpoint)
-	    &&
-	    (interface->altsetting[1].endpoint[0].desc.wMaxPacketSize == 940)) {
-		/* It's a newer em2874/em2875 device */
-		isoc_pipe = 0;
-	} else {
-		int check_interface = 1;
-		isoc_pipe = 1;
-		endpoint = &interface->cur_altsetting->endpoint[1].desc;
-		if (!usb_endpoint_xfer_isoc(endpoint))
-			check_interface = 0;
-
-		if (usb_endpoint_dir_out(endpoint))
-			check_interface = 0;
-
-		if (!check_interface) {
-			if (has_audio) {
-				is_audio_only = true;
-			} else {
-				em28xx_err(DRIVER_NAME " video device (%04x:%04x): "
-					"interface %i, class %i found.\n",
-					le16_to_cpu(udev->descriptor.idVendor),
-					le16_to_cpu(udev->descriptor.idProduct),
-					ifnum,
-					interface->altsetting[0].desc.bInterfaceClass);
-				em28xx_err(DRIVER_NAME " This is an anciliary "
-					"interface not used by the driver\n");
-
-				retval = -ENODEV;
-				goto err;
+		for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
+			const struct usb_endpoint_descriptor *e;
+ 
+			e = &interface->altsetting[i].endpoint[ep].desc;
+
+			sizedescr = le16_to_cpu(e->wMaxPacketSize);
+			size = sizedescr & 0x7fff;
+			if (udev->speed == USB_SPEED_HIGH)
+				size = size * hb_mult(sizedescr);
+
+			if (usb_endpoint_xfer_isoc(e) && usb_endpoint_dir_in(e)) {
+				switch (e->bEndpointAddress) {
+				case EM28XX_EP_AUDIO:
+					has_audio = true;
+					break;
+				case EM28XX_EP_ANALOG:
+					has_video = true;
+					dev->alt_video_max_pkt_size[i] = size;
+					break;
+				case EM28XX_EP_DIGITAL:
+					has_dvb = true;
+					dev->alt_dvb_max_pkt_size[i] = size;
+					break;
+				}
  			}
  		}
  	}
-
+ 
+	if (!(has_audio||has_video||has_dvb)) {
+        	retval=-ENODEV;
+		goto err_free_all;
+	}
+
  	switch (udev->speed) {
  	case USB_SPEED_LOW:
  		speed = "1.5";
@@ -3197,6 +3209,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  			strlcat(descr, " ", sizeof(descr));
  		strlcat(descr, udev->product, sizeof(descr));
  	}
+
  	if (*descr)
  		strlcat(descr, " ", sizeof(descr));

@@ -3213,6 +3226,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  		printk(KERN_INFO DRIVER_NAME
  		       ": Audio Vendor Class interface %i found\n",
  		       ifnum);
+	if (has_video)
+		printk(KERN_INFO DRIVER_NAME
+		       ": Video interface %i found\n",
+		       ifnum);
+	if (has_dvb)
+		printk(KERN_INFO DRIVER_NAME
+		       ": DVB interface %i found\n",
+		       ifnum);

  	/*
  	 * Make sure we have 480 Mbps of bandwidth, otherwise things like
@@ -3224,22 +3245,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  		printk(DRIVER_NAME ": Device must be connected to a high-speed"
  		       " USB 2.0 port.\n");
  		retval = -ENODEV;
-		goto err;
-	}
-
-	/* allocate memory for our device state and initialize it */
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (dev == NULL) {
-		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		retval = -ENOMEM;
-		goto err;
+		goto err_free_all;
  	}

  	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
  	dev->devno = nr;
  	dev->model = id->driver_info;
  	dev->alt   = -1;
-	dev->is_audio_only = is_audio_only;
+	dev->is_audio_only = has_audio&&!(has_video||has_dvb);
  	dev->has_alsa_audio = has_audio;
  	dev->audio_ifnum = ifnum;

@@ -3252,26 +3265,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  		}
  	}

-	/* compute alternate max packet sizes */
  	dev->num_alt = interface->num_altsetting;
-	dev->alt_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);
-
-	if (dev->alt_max_pkt_size == NULL) {
-		em28xx_errdev("out of memory!\n");
-		kfree(dev);
-		retval = -ENOMEM;
-		goto err;
-	}
-
-	for (i = 0; i < dev->num_alt ; i++) {
-		u16 tmp = le16_to_cpu(interface->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
-		unsigned int size = tmp & 0x7ff;
-
-		if (udev->speed == USB_SPEED_HIGH)
-			size = size * hb_mult(tmp);
-
-		dev->alt_max_pkt_size[i] = size;
-	}

  	if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
  		dev->model = card[nr];
@@ -3285,9 +3279,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
  	retval = em28xx_init_dev(&dev, udev, interface, nr);
  	if (retval) {
  		mutex_unlock(&dev->lock);
-		kfree(dev->alt_max_pkt_size);
-		kfree(dev);
-		goto err;
+		goto err_free_all;
  	}

  	request_modules(dev);
@@ -3306,6 +3298,11 @@ static int em28xx_usb_probe(struct usb_interface *interface,

  	return 0;

+err_free_all:
+	kfree(dev->alt_dvb_max_pkt_size);
+	kfree(dev->alt_video_max_pkt_size);
+	kfree(dev);
+
  err:
  	clear_bit(nr, &em28xx_devused);

@@ -3369,7 +3366,8 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
  	em28xx_close_extension(dev);

  	if (!dev->users) {
-		kfree(dev->alt_max_pkt_size);
+		kfree(dev->alt_dvb_max_pkt_size);
+		kfree(dev->alt_video_max_pkt_size);
  		kfree(dev);
  	}
  }
diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index 804a4ab..74608f9 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -829,14 +829,14 @@ int em28xx_set_alternate(struct em28xx *dev)

  	for (i = 0; i < dev->num_alt; i++) {
  		/* stop when the selected alt setting offers enough bandwidth */
-		if (dev->alt_max_pkt_size[i] >= min_pkt_size) {
+		if (dev->alt_video_max_pkt_size[i] >= min_pkt_size) {
  			dev->alt = i;
  			break;
  		/* otherwise make sure that we end up with the maximum bandwidth
  		   because the min_pkt_size equation might be wrong...
  		*/
-		} else if (dev->alt_max_pkt_size[i] >
-			   dev->alt_max_pkt_size[dev->alt])
+		} else if (dev->alt_video_max_pkt_size[i] >
+			   dev->alt_video_max_pkt_size[dev->alt])
  			dev->alt = i;
  	}

@@ -844,7 +844,7 @@ set_alt:
  	if (dev->alt != prev_alt) {
  		em28xx_coredbg("minimum isoc packet size: %u (alt=%d)\n",
  				min_pkt_size, dev->alt);
-		dev->max_pkt_size = dev->alt_max_pkt_size[dev->alt];
+		dev->max_pkt_size = dev->alt_video_max_pkt_size[dev->alt];
  		em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
  			       dev->alt, dev->max_pkt_size);
  		errCode = usb_set_interface(dev->udev, 0, dev->alt);
@@ -1070,7 +1070,7 @@ int em28xx_init_isoc(struct em28xx *dev, int max_packets,
  			should also be using 'desc.bInterval'
  		 */
  		pipe = usb_rcvisocpipe(dev->udev,
-			dev->mode == EM28XX_ANALOG_MODE ? 0x82 : 0x84);
+			dev->mode == EM28XX_ANALOG_MODE ? EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);

  		usb_fill_int_urb(urb, dev->udev, pipe,
  				 dev->isoc_ctl.transfer_buffer[i], sb_size,
@@ -1144,20 +1144,10 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
  		}
  		break;
  	case CHIP_ID_EM2874:
-		/*
-		 * FIXME: for now assumes 564 like it was before, but the
-		 * em2874 code should be added to return the proper value
-		 */
-		packet_size = 564;
-		break;
  	case CHIP_ID_EM2884:
  	case CHIP_ID_EM28174:
  	default:
-		/*
-		 * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
-		 * but not enough for 44 Mbit DVB-C.
-		 */
-		packet_size = 752;
+		packet_size = dev->alt_dvb_max_pkt_size[1];;
  	}

  	return packet_size;
diff --git a/drivers/media/video/em28xx/em28xx-reg.h b/drivers/media/video/em28xx/em28xx-reg.h
index 66f7923..2f62685 100644
--- a/drivers/media/video/em28xx/em28xx-reg.h
+++ b/drivers/media/video/em28xx/em28xx-reg.h
@@ -12,6 +12,11 @@
  #define EM_GPO_2   (1 << 2)
  #define EM_GPO_3   (1 << 3)

+/* em28xx endpoints */
+#define EM28XX_EP_ANALOG	0x82
+#define EM28XX_EP_AUDIO		0x83
+#define EM28XX_EP_DIGITAL	0x84
+
  /* em2800 registers */
  #define EM2800_R08_AUDIOSRC 0x08

diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
index 9b4557a..af9f0b7 100644
--- a/drivers/media/video/em28xx/em28xx-video.c
+++ b/drivers/media/video/em28xx/em28xx-video.c
@@ -2254,7 +2254,8 @@ static int em28xx_v4l2_close(struct file *filp)
  		   free the remaining resources */
  		if (dev->state & DEV_DISCONNECTED) {
  			em28xx_release_resources(dev);
-			kfree(dev->alt_max_pkt_size);
+			kfree(dev->alt_dvb_max_pkt_size);
+			kfree(dev->alt_video_max_pkt_size);
  			kfree(dev);
  			return 0;
  		}
diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index b1199ef..f73f028 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -596,7 +596,8 @@ struct em28xx {
  	int alt;		/* alternate */
  	int max_pkt_size;	/* max packet size of isoc transaction */
  	int num_alt;		/* Number of alternative settings */
-	unsigned int *alt_max_pkt_size;	/* array of wMaxPacketSize */
+	unsigned int *alt_video_max_pkt_size;	/* array of wMaxPacketSize */
+	unsigned int *alt_dvb_max_pkt_size;	/* array of wMaxPacketSize */
  	struct urb *urb[EM28XX_NUM_BUFS];	/* urb for isoc transfers */
  	char *transfer_buffer[EM28XX_NUM_BUFS];	/* transfer buffers for isoc
  						   transfer */

Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick)

[Mauro Carvalho Chehab]

On 28-12-2011 01:50, Holger Nelson wrote:
> On Mon, 26 Dec 2011, Mauro Carvalho Chehab wrote:
> 
>> I'm currently without time right now to work on a patch, but I think that several hacks
>> inside the em28xx probe should be removed, including the one that detects the endpoint
>> based on the packet size.
>>
>> As it is easier to code than to explain in words, the code below could be
>> a start (ok, it doesn't compile, doesn't remove all hacks, doesn't free memory, etc...)
>> Feel free to use it as a start for a real patch, if you wish.
> 
> I think, I filled the missing parts and removed most of the hacks in the probe code. The code works with my Cinergy HTC USB XS.

The code looks sane on my eyes. Didn't test it through.

On the final version, please include a proper description for the patch and your
Signed-off-by:, according with:
	http://linuxtv.org/wiki/index.php/Development:_How_to_submit_patches


> 
> Holger
> 
> diff --git a/drivers/media/video/em28xx/em28xx-audio.c b/drivers/media/video/em28xx/em28xx-audio.c
> index cff0768..e2a7b77 100644
> --- a/drivers/media/video/em28xx/em28xx-audio.c
> +++ b/drivers/media/video/em28xx/em28xx-audio.c
> @@ -193,7 +193,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
> 
>          urb->dev = dev->udev;
>          urb->context = dev;
> -        urb->pipe = usb_rcvisocpipe(dev->udev, 0x83);
> +        urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
>          urb->transfer_flags = URB_ISO_ASAP;
>          urb->transfer_buffer = dev->adev.transfer_buffer[i];
>          urb->interval = 1;
> diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
> index a7cfded..8082914 100644
> --- a/drivers/media/video/em28xx/em28xx-cards.c
> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> @@ -3087,12 +3087,11 @@ unregister_dev:
>  static int em28xx_usb_probe(struct usb_interface *interface,
>                  const struct usb_device_id *id)
>  {
> -    const struct usb_endpoint_descriptor *endpoint;
>      struct usb_device *udev;
>      struct em28xx *dev = NULL;
>      int retval;
> -    bool is_audio_only = false, has_audio = false;
> -    int i, nr, isoc_pipe;
> +    bool has_audio = false, has_video = false, has_dvb = false;
> +    int i, nr, sizedescr, size;
>      const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
>      char *speed;
>      char descr[255] = "";
> @@ -3124,56 +3123,69 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>          goto err;
>      }
> 
> -    /* Get endpoints */
> -    for (i = 0; i < interface->num_altsetting; i++) {
> -        int ep;
> +    /* allocate memory for our device state and initialize it */
> +    dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +    if (dev == NULL) {
> +        em28xx_err(DRIVER_NAME ": out of memory!\n");
> +        retval = -ENOMEM;
> +        goto err;
> +    }
> 
> -        for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
> -            struct usb_host_endpoint    *e;
> -            e = &interface->altsetting[i].endpoint[ep];
> +    /* compute alternate max packet sizes */
> +    dev->alt_video_max_pkt_size = kmalloc(sizeof(dev->alt_video_max_pkt_size[0]) * interface->num_altsetting, GFP_KERNEL);
> +    if (dev->alt_video_max_pkt_size == NULL) {
> +        em28xx_errdev("out of memory!\n");
> +        kfree(dev);
> +        retval = -ENOMEM;
> +        goto err;
> +    }
> 
> -            if (e->desc.bEndpointAddress == 0x83)
> -                has_audio = true;
> -        }
> +    dev->alt_dvb_max_pkt_size = kmalloc(sizeof(dev->alt_dvb_max_pkt_size[0]) * interface->num_altsetting, GFP_KERNEL);
> +    if (dev->alt_dvb_max_pkt_size == NULL) {
> +        em28xx_errdev("out of memory!\n");
> +        kfree(dev->alt_video_max_pkt_size);
> +        kfree(dev);
> +        retval = -ENOMEM;
> +        goto err;
>      }
> 
> -    endpoint = &interface->cur_altsetting->endpoint[0].desc;
> +    /* Get endpoints */
> +    for (i = 0; i < interface->num_altsetting; i++) {
> +        int ep;
> 
> -    /* check if the device has the iso in endpoint at the correct place */
> -    if (usb_endpoint_xfer_isoc(endpoint)
> -        &&
> -        (interface->altsetting[1].endpoint[0].desc.wMaxPacketSize == 940)) {
> -        /* It's a newer em2874/em2875 device */
> -        isoc_pipe = 0;
> -    } else {
> -        int check_interface = 1;
> -        isoc_pipe = 1;
> -        endpoint = &interface->cur_altsetting->endpoint[1].desc;
> -        if (!usb_endpoint_xfer_isoc(endpoint))
> -            check_interface = 0;
> -
> -        if (usb_endpoint_dir_out(endpoint))
> -            check_interface = 0;
> -
> -        if (!check_interface) {
> -            if (has_audio) {
> -                is_audio_only = true;
> -            } else {
> -                em28xx_err(DRIVER_NAME " video device (%04x:%04x): "
> -                    "interface %i, class %i found.\n",
> -                    le16_to_cpu(udev->descriptor.idVendor),
> -                    le16_to_cpu(udev->descriptor.idProduct),
> -                    ifnum,
> -                    interface->altsetting[0].desc.bInterfaceClass);
> -                em28xx_err(DRIVER_NAME " This is an anciliary "
> -                    "interface not used by the driver\n");
> -
> -                retval = -ENODEV;
> -                goto err;
> +        for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
> +            const struct usb_endpoint_descriptor *e;
> + +            e = &interface->altsetting[i].endpoint[ep].desc;

There is an extra "+" here.

> +
> +            sizedescr = le16_to_cpu(e->wMaxPacketSize);
> +            size = sizedescr & 0x7fff;
> +            if (udev->speed == USB_SPEED_HIGH)
> +                size = size * hb_mult(sizedescr);
> +
> +            if (usb_endpoint_xfer_isoc(e) && usb_endpoint_dir_in(e)) {
> +                switch (e->bEndpointAddress) {
> +                case EM28XX_EP_AUDIO:
> +                    has_audio = true;
> +                    break;
> +                case EM28XX_EP_ANALOG:
> +                    has_video = true;
> +                    dev->alt_video_max_pkt_size[i] = size;
> +                    break;
> +                case EM28XX_EP_DIGITAL:
> +                    has_dvb = true;
> +                    dev->alt_dvb_max_pkt_size[i] = size;
> +                    break;
> +                }
>              }
>          }
>      }
> -
> + +    if (!(has_audio||has_video||has_dvb)) {

There is an extra "+" here. Also Linux CodingStyle requires spaces before
and after operators:
	if (!(has_audio || has_video || has_dvb)) {

> +            retval=-ENODEV;
> +        goto err_free_all;
> +    }
> +
>      switch (udev->speed) {
>      case USB_SPEED_LOW:
>          speed = "1.5";
> @@ -3197,6 +3209,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>              strlcat(descr, " ", sizeof(descr));
>          strlcat(descr, udev->product, sizeof(descr));
>      }
> +
>      if (*descr)
>          strlcat(descr, " ", sizeof(descr));
> 
> @@ -3213,6 +3226,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>          printk(KERN_INFO DRIVER_NAME
>                 ": Audio Vendor Class interface %i found\n",
>                 ifnum);
> +    if (has_video)
> +        printk(KERN_INFO DRIVER_NAME
> +               ": Video interface %i found\n",
> +               ifnum);
> +    if (has_dvb)
> +        printk(KERN_INFO DRIVER_NAME
> +               ": DVB interface %i found\n",
> +               ifnum);
> 
>      /*
>       * Make sure we have 480 Mbps of bandwidth, otherwise things like
> @@ -3224,22 +3245,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>          printk(DRIVER_NAME ": Device must be connected to a high-speed"
>                 " USB 2.0 port.\n");
>          retval = -ENODEV;
> -        goto err;
> -    }
> -
> -    /* allocate memory for our device state and initialize it */
> -    dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -    if (dev == NULL) {
> -        em28xx_err(DRIVER_NAME ": out of memory!\n");
> -        retval = -ENOMEM;
> -        goto err;
> +        goto err_free_all;
>      }
> 
>      snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
>      dev->devno = nr;
>      dev->model = id->driver_info;
>      dev->alt   = -1;
> -    dev->is_audio_only = is_audio_only;
> +    dev->is_audio_only = has_audio&&!(has_video||has_dvb);

CodingStyle: it should be, instead:
	dev->is_audio_only = has_audio && !(has_video || has_dvb);

>      dev->has_alsa_audio = has_audio;
>      dev->audio_ifnum = ifnum;
> 
> @@ -3252,26 +3265,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>          }
>      }
> 
> -    /* compute alternate max packet sizes */
>      dev->num_alt = interface->num_altsetting;
> -    dev->alt_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);
> -
> -    if (dev->alt_max_pkt_size == NULL) {
> -        em28xx_errdev("out of memory!\n");
> -        kfree(dev);
> -        retval = -ENOMEM;
> -        goto err;
> -    }
> -
> -    for (i = 0; i < dev->num_alt ; i++) {
> -        u16 tmp = le16_to_cpu(interface->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
> -        unsigned int size = tmp & 0x7ff;
> -
> -        if (udev->speed == USB_SPEED_HIGH)
> -            size = size * hb_mult(tmp);
> -
> -        dev->alt_max_pkt_size[i] = size;
> -    }
> 
>      if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
>          dev->model = card[nr];
> @@ -3285,9 +3279,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>      retval = em28xx_init_dev(&dev, udev, interface, nr);
>      if (retval) {
>          mutex_unlock(&dev->lock);
> -        kfree(dev->alt_max_pkt_size);
> -        kfree(dev);
> -        goto err;
> +        goto err_free_all;

Maybe you could also move the mutex_unlock to the error path, like:

	goto unlock_and_free;

>      }
> 
>      request_modules(dev);
> @@ -3306,6 +3298,11 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> 
>      return 0;
> 
> +err_free_all:
> +    kfree(dev->alt_dvb_max_pkt_size);
> +    kfree(dev->alt_video_max_pkt_size);
> +    kfree(dev);
> +
>  err:
>      clear_bit(nr, &em28xx_devused);
> 
> @@ -3369,7 +3366,8 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
>      em28xx_close_extension(dev);
> 
>      if (!dev->users) {
> -        kfree(dev->alt_max_pkt_size);
> +        kfree(dev->alt_dvb_max_pkt_size);
> +        kfree(dev->alt_video_max_pkt_size);
>          kfree(dev);
>      }
>  }
> diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
> index 804a4ab..74608f9 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -829,14 +829,14 @@ int em28xx_set_alternate(struct em28xx *dev)
> 
>      for (i = 0; i < dev->num_alt; i++) {
>          /* stop when the selected alt setting offers enough bandwidth */
> -        if (dev->alt_max_pkt_size[i] >= min_pkt_size) {
> +        if (dev->alt_video_max_pkt_size[i] >= min_pkt_size) {
>              dev->alt = i;
>              break;
>          /* otherwise make sure that we end up with the maximum bandwidth
>             because the min_pkt_size equation might be wrong...
>          */
> -        } else if (dev->alt_max_pkt_size[i] >
> -               dev->alt_max_pkt_size[dev->alt])
> +        } else if (dev->alt_video_max_pkt_size[i] >
> +               dev->alt_video_max_pkt_size[dev->alt])
>              dev->alt = i;
>      }
> 
> @@ -844,7 +844,7 @@ set_alt:
>      if (dev->alt != prev_alt) {
>          em28xx_coredbg("minimum isoc packet size: %u (alt=%d)\n",
>                  min_pkt_size, dev->alt);
> -        dev->max_pkt_size = dev->alt_max_pkt_size[dev->alt];
> +        dev->max_pkt_size = dev->alt_video_max_pkt_size[dev->alt];
>          em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
>                     dev->alt, dev->max_pkt_size);
>          errCode = usb_set_interface(dev->udev, 0, dev->alt);
> @@ -1070,7 +1070,7 @@ int em28xx_init_isoc(struct em28xx *dev, int max_packets,
>              should also be using 'desc.bInterval'
>           */
>          pipe = usb_rcvisocpipe(dev->udev,
> -            dev->mode == EM28XX_ANALOG_MODE ? 0x82 : 0x84);
> +            dev->mode == EM28XX_ANALOG_MODE ? EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
> 
>          usb_fill_int_urb(urb, dev->udev, pipe,
>                   dev->isoc_ctl.transfer_buffer[i], sb_size,
> @@ -1144,20 +1144,10 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
>          }
>          break;
>      case CHIP_ID_EM2874:
> -        /*
> -         * FIXME: for now assumes 564 like it was before, but the
> -         * em2874 code should be added to return the proper value
> -         */
> -        packet_size = 564;
> -        break;
>      case CHIP_ID_EM2884:
>      case CHIP_ID_EM28174:
>      default:
> -        /*
> -         * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
> -         * but not enough for 44 Mbit DVB-C.
> -         */
> -        packet_size = 752;
> +        packet_size = dev->alt_dvb_max_pkt_size[1];;

There are two ";;" above.

It could make sense to do a loop here and get the biggest max_pkt_size. 
Of course, this would mean that, at em28xx_start_streaming, the code 
should be changed from:
        usb_set_interface(dev->udev, 0, 1);
to:
        usb_set_interface(dev->udev, 0, dev->dvb_alternate);

and that the loop would fill dvb_alternate with the alternate associated
with the selected packet size.

Now that the packet_size is coming from the endpoint, I think that the chipset-specific
code there could be removed completely.

>      }
> 
>      return packet_size;
> diff --git a/drivers/media/video/em28xx/em28xx-reg.h b/drivers/media/video/em28xx/em28xx-reg.h
> index 66f7923..2f62685 100644
> --- a/drivers/media/video/em28xx/em28xx-reg.h
> +++ b/drivers/media/video/em28xx/em28xx-reg.h
> @@ -12,6 +12,11 @@
>  #define EM_GPO_2   (1 << 2)
>  #define EM_GPO_3   (1 << 3)
> 
> +/* em28xx endpoints */
> +#define EM28XX_EP_ANALOG    0x82
> +#define EM28XX_EP_AUDIO        0x83
> +#define EM28XX_EP_DIGITAL    0x84
> +
>  /* em2800 registers */
>  #define EM2800_R08_AUDIOSRC 0x08
> 
> diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
> index 9b4557a..af9f0b7 100644
> --- a/drivers/media/video/em28xx/em28xx-video.c
> +++ b/drivers/media/video/em28xx/em28xx-video.c
> @@ -2254,7 +2254,8 @@ static int em28xx_v4l2_close(struct file *filp)
>             free the remaining resources */
>          if (dev->state & DEV_DISCONNECTED) {
>              em28xx_release_resources(dev);
> -            kfree(dev->alt_max_pkt_size);
> +            kfree(dev->alt_dvb_max_pkt_size);
> +            kfree(dev->alt_video_max_pkt_size);
>              kfree(dev);
>              return 0;
>          }
> diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
> index b1199ef..f73f028 100644
> --- a/drivers/media/video/em28xx/em28xx.h
> +++ b/drivers/media/video/em28xx/em28xx.h
> @@ -596,7 +596,8 @@ struct em28xx {
>      int alt;        /* alternate */
>      int max_pkt_size;    /* max packet size of isoc transaction */
>      int num_alt;        /* Number of alternative settings */
> -    unsigned int *alt_max_pkt_size;    /* array of wMaxPacketSize */
> +    unsigned int *alt_video_max_pkt_size;    /* array of wMaxPacketSize */
> +    unsigned int *alt_dvb_max_pkt_size;    /* array of wMaxPacketSize */
>      struct urb *urb[EM28XX_NUM_BUFS];    /* urb for isoc transfers */
>      char *transfer_buffer[EM28XX_NUM_BUFS];    /* transfer buffers for isoc
>                             transfer */
> -- 
> 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

[PATCH] em28xx: Reworked probe code to get rid of some hacks (was: Re: em28xx_isoc_dvb_max_packetsize for EM2884 (Terratec Cinergy HTC Stick))

[Holger Nelson]

Reworked device probing to get rid of hacks to guess the maximum size of 
dvb iso transfer packets. The new code also selects the first alternate 
config which supports the largest possible iso transfers for dvb. 

Signed-off-by: Holger Nelson <hnelson@hnelson.de>

diff --git a/drivers/media/video/em28xx/em28xx-audio.c b/drivers/media/video/em28xx/em28xx-audio.c
index cff0768..e2a7b77 100644
--- a/drivers/media/video/em28xx/em28xx-audio.c
+++ b/drivers/media/video/em28xx/em28xx-audio.c
@@ -193,7 +193,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
 
 		urb->dev = dev->udev;
 		urb->context = dev;
-		urb->pipe = usb_rcvisocpipe(dev->udev, 0x83);
+		urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
 		urb->transfer_flags = URB_ISO_ASAP;
 		urb->transfer_buffer = dev->adev.transfer_buffer[i];
 		urb->interval = 1;
diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index a7cfded..027f769 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3087,12 +3087,11 @@ unregister_dev:
 static int em28xx_usb_probe(struct usb_interface *interface,
 			    const struct usb_device_id *id)
 {
-	const struct usb_endpoint_descriptor *endpoint;
 	struct usb_device *udev;
 	struct em28xx *dev = NULL;
 	int retval;
-	bool is_audio_only = false, has_audio = false;
-	int i, nr, isoc_pipe;
+	bool has_audio = false, has_video = false, has_dvb = false;
+	int i, nr;
 	const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
 	char *speed;
 	char descr[255] = "";
@@ -3124,54 +3123,63 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		goto err;
 	}
 
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		em28xx_err(DRIVER_NAME ": out of memory!\n");
+		retval = -ENOMEM;
+		goto err;
+	}
+
+	/* compute alternate max packet sizes */
+	dev->alt_max_pkt_size = kmalloc(sizeof(dev->alt_max_pkt_size[0]) * interface->num_altsetting, GFP_KERNEL);
+	if (dev->alt_max_pkt_size == NULL) {
+		em28xx_errdev("out of memory!\n");
+		kfree(dev);
+		retval = -ENOMEM;
+		goto err;
+	}
+
 	/* Get endpoints */
 	for (i = 0; i < interface->num_altsetting; i++) {
 		int ep;
 
 		for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++) {
-			struct usb_host_endpoint	*e;
-			e = &interface->altsetting[i].endpoint[ep];
-
-			if (e->desc.bEndpointAddress == 0x83)
-				has_audio = true;
+			const struct usb_endpoint_descriptor *e;
+			int sizedescr, size;
+
+			e = &interface->altsetting[i].endpoint[ep].desc;
+
+			sizedescr = le16_to_cpu(e->wMaxPacketSize);
+			size = sizedescr & 0x7ff;
+
+			if (udev->speed == USB_SPEED_HIGH)
+				size = size * hb_mult(sizedescr);
+
+			if (usb_endpoint_xfer_isoc(e) && usb_endpoint_dir_in(e)) {
+				switch (e->bEndpointAddress) {
+				case EM28XX_EP_AUDIO:
+					has_audio = true;
+					break;
+				case EM28XX_EP_ANALOG:
+					has_video = true;
+					dev->alt_max_pkt_size[i] = size;
+					break;
+				case EM28XX_EP_DIGITAL:
+					has_dvb = true;
+					if (size > dev->dvb_max_pkt_size) {
+						dev->dvb_max_pkt_size = size;
+						dev->dvb_alt = i;
+					}
+					break;
+				}
+			}
 		}
 	}
 
-	endpoint = &interface->cur_altsetting->endpoint[0].desc;
-
-	/* check if the device has the iso in endpoint at the correct place */
-	if (usb_endpoint_xfer_isoc(endpoint)
-	    &&
-	    (interface->altsetting[1].endpoint[0].desc.wMaxPacketSize == 940)) {
-		/* It's a newer em2874/em2875 device */
-		isoc_pipe = 0;
-	} else {
-		int check_interface = 1;
-		isoc_pipe = 1;
-		endpoint = &interface->cur_altsetting->endpoint[1].desc;
-		if (!usb_endpoint_xfer_isoc(endpoint))
-			check_interface = 0;
-
-		if (usb_endpoint_dir_out(endpoint))
-			check_interface = 0;
-
-		if (!check_interface) {
-			if (has_audio) {
-				is_audio_only = true;
-			} else {
-				em28xx_err(DRIVER_NAME " video device (%04x:%04x): "
-					"interface %i, class %i found.\n",
-					le16_to_cpu(udev->descriptor.idVendor),
-					le16_to_cpu(udev->descriptor.idProduct),
-					ifnum,
-					interface->altsetting[0].desc.bInterfaceClass);
-				em28xx_err(DRIVER_NAME " This is an anciliary "
-					"interface not used by the driver\n");
-
-				retval = -ENODEV;
-				goto err;
-			}
-		}
+	if (!(has_audio || has_video || has_dvb)) {
+		retval=-ENODEV;
+		goto err_free;
 	}
 
 	switch (udev->speed) {
@@ -3197,6 +3205,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 			strlcat(descr, " ", sizeof(descr));
 		strlcat(descr, udev->product, sizeof(descr));
 	}
+
 	if (*descr)
 		strlcat(descr, " ", sizeof(descr));
 
@@ -3213,6 +3222,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		printk(KERN_INFO DRIVER_NAME
 		       ": Audio Vendor Class interface %i found\n",
 		       ifnum);
+	if (has_video)
+		printk(KERN_INFO DRIVER_NAME
+		       ": Video interface %i found\n",
+		       ifnum);
+	if (has_dvb)
+		printk(KERN_INFO DRIVER_NAME
+		       ": DVB interface %i found\n",
+		       ifnum);
 
 	/*
 	 * Make sure we have 480 Mbps of bandwidth, otherwise things like
@@ -3224,22 +3241,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		printk(DRIVER_NAME ": Device must be connected to a high-speed"
 		       " USB 2.0 port.\n");
 		retval = -ENODEV;
-		goto err;
-	}
-
-	/* allocate memory for our device state and initialize it */
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (dev == NULL) {
-		em28xx_err(DRIVER_NAME ": out of memory!\n");
-		retval = -ENOMEM;
-		goto err;
+		goto err_free;
 	}
 
 	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
 	dev->alt   = -1;
-	dev->is_audio_only = is_audio_only;
+	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
 	dev->audio_ifnum = ifnum;
 
@@ -3252,26 +3261,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		}
 	}
 
-	/* compute alternate max packet sizes */
 	dev->num_alt = interface->num_altsetting;
-	dev->alt_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);
-
-	if (dev->alt_max_pkt_size == NULL) {
-		em28xx_errdev("out of memory!\n");
-		kfree(dev);
-		retval = -ENOMEM;
-		goto err;
-	}
-
-	for (i = 0; i < dev->num_alt ; i++) {
-		u16 tmp = le16_to_cpu(interface->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
-		unsigned int size = tmp & 0x7ff;
-
-		if (udev->speed == USB_SPEED_HIGH)
-			size = size * hb_mult(tmp);
-
-		dev->alt_max_pkt_size[i] = size;
-	}
 
 	if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
 		dev->model = card[nr];
@@ -3284,10 +3274,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	mutex_lock(&dev->lock);
 	retval = em28xx_init_dev(&dev, udev, interface, nr);
 	if (retval) {
-		mutex_unlock(&dev->lock);
-		kfree(dev->alt_max_pkt_size);
-		kfree(dev);
-		goto err;
+		goto unlock_and_free;
 	}
 
 	request_modules(dev);
@@ -3306,6 +3293,13 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 
 	return 0;
 
+unlock_and_free:
+	mutex_unlock(&dev->lock);
+
+err_free:
+	kfree(dev->alt_max_pkt_size);
+	kfree(dev);
+
 err:
 	clear_bit(nr, &em28xx_devused);
 
diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index 804a4ab..bc1d5dd 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -1070,7 +1070,7 @@ int em28xx_init_isoc(struct em28xx *dev, int max_packets,
 			should also be using 'desc.bInterval'
 		 */
 		pipe = usb_rcvisocpipe(dev->udev,
-			dev->mode == EM28XX_ANALOG_MODE ? 0x82 : 0x84);
+			dev->mode == EM28XX_ANALOG_MODE ? EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
 
 		usb_fill_int_urb(urb, dev->udev, pipe,
 				 dev->isoc_ctl.transfer_buffer[i], sb_size,
@@ -1108,62 +1108,6 @@ int em28xx_init_isoc(struct em28xx *dev, int max_packets,
 }
 EXPORT_SYMBOL_GPL(em28xx_init_isoc);
 
-/* Determine the packet size for the DVB stream for the given device
-   (underlying value programmed into the eeprom) */
-int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
-{
-	unsigned int chip_cfg2;
-	unsigned int packet_size;
-
-	switch (dev->chip_id) {
-	case CHIP_ID_EM2710:
-	case CHIP_ID_EM2750:
-	case CHIP_ID_EM2800:
-	case CHIP_ID_EM2820:
-	case CHIP_ID_EM2840:
-	case CHIP_ID_EM2860:
-		/* No DVB support */
-		return -EINVAL;
-	case CHIP_ID_EM2870:
-	case CHIP_ID_EM2883:
-		/* TS max packet size stored in bits 1-0 of R01 */
-		chip_cfg2 = em28xx_read_reg(dev, EM28XX_R01_CHIPCFG2);
-		switch (chip_cfg2 & EM28XX_CHIPCFG2_TS_PACKETSIZE_MASK) {
-		case EM28XX_CHIPCFG2_TS_PACKETSIZE_188:
-			packet_size = 188;
-			break;
-		case EM28XX_CHIPCFG2_TS_PACKETSIZE_376:
-			packet_size = 376;
-			break;
-		case EM28XX_CHIPCFG2_TS_PACKETSIZE_564:
-			packet_size = 564;
-			break;
-		case EM28XX_CHIPCFG2_TS_PACKETSIZE_752:
-			packet_size = 752;
-			break;
-		}
-		break;
-	case CHIP_ID_EM2874:
-		/*
-		 * FIXME: for now assumes 564 like it was before, but the
-		 * em2874 code should be added to return the proper value
-		 */
-		packet_size = 564;
-		break;
-	case CHIP_ID_EM2884:
-	case CHIP_ID_EM28174:
-	default:
-		/*
-		 * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
-		 * but not enough for 44 Mbit DVB-C.
-		 */
-		packet_size = 752;
-	}
-
-	return packet_size;
-}
-EXPORT_SYMBOL_GPL(em28xx_isoc_dvb_max_packetsize);
-
 /*
  * em28xx_wake_i2c()
  * configure i2c attached devices
diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
index 3868c1e..52ff128 100644
--- a/drivers/media/video/em28xx/em28xx-dvb.c
+++ b/drivers/media/video/em28xx/em28xx-dvb.c
@@ -163,12 +163,12 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 	struct em28xx *dev = dvb->adapter.priv;
 	int max_dvb_packet_size;
 
-	usb_set_interface(dev->udev, 0, 1);
+	usb_set_interface(dev->udev, 0, dev->dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
 
-	max_dvb_packet_size = em28xx_isoc_dvb_max_packetsize(dev);
+	max_dvb_packet_size = dev->dvb_max_pkt_size;
 	if (max_dvb_packet_size < 0)
 		return max_dvb_packet_size;
 	dprintk(1, "Using %d buffers each with %d bytes\n",
diff --git a/drivers/media/video/em28xx/em28xx-reg.h b/drivers/media/video/em28xx/em28xx-reg.h
index 66f7923..2f62685 100644
--- a/drivers/media/video/em28xx/em28xx-reg.h
+++ b/drivers/media/video/em28xx/em28xx-reg.h
@@ -12,6 +12,11 @@
 #define EM_GPO_2   (1 << 2)
 #define EM_GPO_3   (1 << 3)
 
+/* em28xx endpoints */
+#define EM28XX_EP_ANALOG	0x82
+#define EM28XX_EP_AUDIO		0x83
+#define EM28XX_EP_DIGITAL	0x84
+
 /* em2800 registers */
 #define EM2800_R08_AUDIOSRC 0x08
 
diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index b1199ef..85394cb 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -597,6 +597,8 @@ struct em28xx {
 	int max_pkt_size;	/* max packet size of isoc transaction */
 	int num_alt;		/* Number of alternative settings */
 	unsigned int *alt_max_pkt_size;	/* array of wMaxPacketSize */
+	int dvb_alt;				/* alternate for DVB */
+	unsigned int dvb_max_pkt_size;		/* wMaxPacketSize for DVB */
 	struct urb *urb[EM28XX_NUM_BUFS];	/* urb for isoc transfers */
 	char *transfer_buffer[EM28XX_NUM_BUFS];	/* transfer buffers for isoc
 						   transfer */