Re: [PATCH] lmedm04 2.06 conversion to dvb-usb-v2 version 2

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 07-08-2012 13:28, Antti Palosaari escreveu:
> On 08/06/2012 11:21 PM, Malcolm Priestley wrote:
>> Conversion of lmedm04 to dvb-usb-v2
>>
>> Functional changes m88rs2000 tuner now uses all callbacks.
>> TODO migrate other tuners to the callbacks.
>>
>> This patch is applied on top of [BUG] Re: dvb_usb_lmedm04 crash Kernel (rs2000)
>> http://patchwork.linuxtv.org/patch/13584/
>>
>>
>> Signed-off-by: Malcolm Priestley <tvboxspy@gmail.com>
> 
> Could you try to make this driver more generic?
> 
> You use some internals of dvb-usb directly and most likely those are without a reason.
> For example data streaming, lme2510_kill_urb() kills directly urbs allocated
> and submitted by dvb-usb. Guess that driver is broken just after someone changes
>  dvb-usb streaming code.

Yeah, it is better to replace it by the dvb-usb-v2 solution (usb_clear_halt),
as some special treatment there might be needed due to suspend/resume.

> lme2510_usb_talk() could be replaced by generic dvb_usbv2_generic_rw().
> 
> What is function of lme2510_int_read() ? I see you use own low level URB routines for here too. 
> It starts "polling", reads remote, tuner, demod, etc, and updates state. You would better to 
> implement I2C-adapter correctly and then start Kernel work-queue, which reads same information 
> using I2C-adapter. Or you could even abuse remote controller polling function provided by dvb-usb.

While I don't know any technical details about this device, this looks
similar to what dib0700_core does.

Instead of polling IR, with is expensive, it sets an special endpoint
to receive IR data, and sends an URB request. That URB request will
be pending until someone kicks the IR. If nothing is pressed, no URB
is received. This is a way better than polling, as no traffic
happens while a key is not pressed.

>From what I saw at the driver, the mpeg stream is at endpoint 0x08, and
it uses bulk transfer; for IR data, it uses endpoint 0x0a, and interrupt
URB.

So, this extra control is needed. It may make sense to move part of this
code to the dvb-usb-v2 core (the part that starts/stops the URB handling),
in order to properly handle device suspend/resume, as, depending on the
suspend level, you might need to stop it, restarting at resume.

The payload handling should be driver-specific, of course.

So, in summary, that seems to be OK, for the current dvb-usb-v2 core,
requiring further changes for suspend/resume to work properly.

> 
> lme2510_get_stream_config() enables pid-filter again over the dvb-usb, but I can live with it because there is no dynamic configuration for that. Anyhow, is that really needed?
> 
> I can live with the pid-filter "abuse", but killing stream URBs on behalf of DVB-USB is something I don't like to see. If you have very good explanation and I cannot fix DVB USB to meet it I could consider that kind of hack. And it should be documented clearly adding necessary comments to code.
> 
> Re-implementing that driver to use 100% DVB-USB services will reduce around 50% of code or more.

The thing that bother me at the code is the implementation of a device-specific
set_voltage() callback. This should be part of dvb-usb-v2 core, and, during
suspend, it makes sense to set voltage to OFF, returning it to its original
state during resume.

Regards,
Mauro