Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari <crope@iki.fi>]

On 09/07/2011 08:45 PM, Michael Krufky wrote:
> On Wed, Sep 7, 2011 at 1:41 PM, Michael Krufky<mkrufky@kernellabs.com>  wrote:
>> On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari<crope@iki.fi>  wrote:
>>> On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
>>>>
>>>> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>>>>>
>>>>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>>>>>
>>>>>> One bad thing I noticed with the API is that it calls
>>>>>> adap->props.frontend_attach(adap)
>>>>>> several times, instead of just one, without even passing an argument for
>>>>>> the driver to
>>>>>> know that it was called twice.
>>>>>>
>>>>>> IMO, there are two ways of doing the attach:
>>>>>>
>>>>>> 1) call it only once, and, inside the driver, it will loop to add the
>>>>>> other FE's;
>>>>>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>>>>>
>>>>>> I think (1) is preferred, as it is more flexible, allowing the driver to
>>>>>> test for
>>>>>> several types of frontends.
>>>
>>> I am planning to change DVB USB MFE call .frontend_attach() only once. Is
>>> there any comments about that?
>>>
>>> Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change
>>> is needed ASAP before more MFE devices are coming.
>>>
>>> Also .num_frontends can be removed after that, since DVB USB will just loop
>>> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
>>>
>>> CURRENTLY:
>>> ==========
>>> .frontend_attach()
>>>         if (adap->fe_adap[0].fe == NULL)
>>>                 adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>         else if (adap->fe_adap[1].fe == NULL)
>>>                 adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>         else if (adap->fe_adap[2].fe == NULL)
>>>                 adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>
>>> PLANNED:
>>> ========
>>> .frontend_attach()
>>>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> Antti,
>>
>> I don't understand exactly what you are proposing -- Is this a change
>> for the anysee driver?  ...or is it a change for the dvb-usb
>> framework?  ...or is it a change to dvb-core, and every driver in the
>> subsystem?
>>
>> In the anysee driver, I see that you are using this:
>>
>> .frontend_attach()
>>          if (adap->fe_adap[0].fe == NULL)
>>                  adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>          else if (adap->fe_adap[1].fe == NULL)
>>                  adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>          else if (adap->fe_adap[2].fe == NULL)
>>                  adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> I have no problem if you want to change the anysee driver to remove
>> the second dvb_usb_adap_fe_props context, and replace with the
>> following:
>>
>>
>> .frontend_attach()
>>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> I believe this will work in the anysee driver for you, even with my
>> changes that got merged yesterday... However, I do *not* believe that
>> such change should propogate to the dvb-usb framework or dvb-core
>> itself, because it can have a large negative impact on the drivers
>> using it.
>>
>> For example, my mxl111sf driver was merged yesterday.  Since it is the
>> initial driver merge, it currently only supports one frontend (ATSC).
>> The device also supports two other delivery systems, and has two
>> additional dtv demodulators, each attached via a separate input bus to
>> the USB device, each streaming on a separate USB endpoint.
>>
>> Many demod drivers do an ID test or some other kind of initialization
>> during the _attach() function.  A device like the mxl111sf would have
>> to manipulate the USB device state and alter the bus operations before
>> and after each frontend attachment in order for the _attach() calls to
>> succeed, not to mention the separate calls needed for bus negotiation
>> to power on the correct demodulator and initialize its streaming data
>> path.
>>
>> I repeat, if this is a change that is specific to your anysee driver,
>> then it seems like a good idea to me.  However, if your plan is to
>> change dvb-usb itself, and / or dvb-core, then I'd really like to have
>> a better idea of the implications that this change will bring forth.
>>
>> So, to help reduce the confusion, can you clarify exactly what code
>> you plan to change, and what impact it will have on the drivers that
>> exist today?
>>
>> Best Regards,
>>
>> Michael Krufky
>>
>
> ADDENDUM:
>
> For the anysee driver, for your single .frontend_attach()
>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> ...for this to work in today's dvb-usb code without modification to
> the dvb-usb framework, i believe that you should do a test for
> (adap->fe_adap[0].fe&&  adap->fe_adap[1].fe&&  adap->fe_adap[2].fe )
> ... if null, then attach, if not null, then exit -- this will prevent
> the second context's initialization from occurring twice.

Yes, I now saw when looked latest anysee driver that you moved 
.streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend 
property. OK, it is not then relevant anymore to change register all as 
once.

What is size_of_priv used?

regards
Antti




-- 
http://palosaari.fi/