On Sat, 2012-06-16 at 18:52 +0300, Antti Palosaari wrote:
> On 06/16/2012 03:06 PM, Malcolm Priestley wrote:
> > On Sat, 2012-06-16 at 04:16 +0300, Antti Palosaari wrote:
> >> I did example for you, here you are :)
> >>
> >> On 06/16/2012 03:55 AM, Antti Palosaari wrote:
> >>> On 06/16/2012 03:35 AM, Malcolm Priestley wrote:
> >>>> On Sat, 2012-06-16 at 01:54 +0300, Antti Palosaari wrote:
> >>>>> Hello Malcolm,
> >>>>>
> >>>>> On 06/16/2012 01:11 AM, Malcolm Priestley wrote:
> >>>>>> Hi Antti
> >>>>>>
> >>>>>> You can't have dvb_usb_device_properties as constant structure pointer.
> >>>>>>
> >>>>>> At run time it needs to be copied to a private area.
> >>>>>
> >>>>> Having constant structure for properties was one of main idea of whole
> >>>>> change. Earlier it causes some problems when driver changes those values
> >>>>> - for example remote configuration based info from the eeprom.
> >>>>>
> >>>>>> Two or more devices of the same type on the system will be pointing to
> >>>>>> the same structure.
> >>>>>
> >>>>> Yes and no. You can define struct dvb_usb_device_properties for each
> >>>>> USB ID.
> >>>>>
> >>>>>> Any changes they make to the structure will be common to all.
> >>>>>
> >>>>> For those devices having same USB ID only.
> >>>>> Changing dvb_usb_device_properties is *not* allowed. It is constant and
> >>>>> should be. That was how I designed it. Due to that I introduced those
> >>>>> new callbacks to resolve needed values dynamically.
> >>>> Yes, but it does make run-time tweaks difficult.
> >>>>
> >>>>> If there is still something that is needed to resolve at runtime I am
> >>>>> happy to add new callback. For example PID filter configuration is
> >>>>> static currently as per adapter and if it is needed to to reconfigure at
> >>>>> runtime new callback is needed.
> >>>> I will look at the PID filter later, it defaulted to off.
> >>>>
> >>>> However, in my builds for ARM devices it is defaulted on. I will be
> >>>> testing this later. I can't see any problems.
> >>>>
> >>>>>
> >>>>> Could you say what is your problem I can likely say how to resolve it.
> >>>>>
> >>>>
> >>>> Well, the problem is, I now need two separate structures for LME2510 and
> >>>> LME2510C as in the existing driver, the hope was to merge them as one.
> >>>> The only difference being the stream endpoint number.
> >>>
> >>> Then you should use .get_usb_stream_config() instead of static stream
> >>> configuration. That is now supported.
> >>>
> >>> I have found one logical error in my current implementation.
> >>> get_usb_stream_config gets frontend as a parameter, but it is called
> >>> frontend == NULL when attaching adapters and after that frontend is real
> >>> value when called (just before streaming is started). Now what happens
> >>> if we has multiple adapters? We cannot know which adapter is requested
> >>> during attach since FE is NULL :)
> >>> But that is problem case only if you have multiple adapters. I will find
> >>> out better solution next few days. It is ugly now.
> >>> You can just skip fe checking or something. See AF9015 for example.
> >>>
> >>>> Currently, it is implemented in identify_state on dvb_usb_v2.
> >>>>
> >>>> The get_usb_stream_config has no access to device to to allow a run-time
> >>>> change there.
> >>>
> >>> Hmmm, what you try to say? As FE is given as a pointer, you have also
> >>> adapter and device. Those are nested, device is root, then adapter is
> >>> under that and finally frontend are under the adapter.
> >>>
> >>>
> >>> .
> >>> └── device
> >>> ├── adapter0
> >>> │ ├── frontend0
> >>> │ ├── frontend1
> >>> │ └── frontend2
> >>> └── adapter1
> >>> ├── frontend0
> >>> ├── frontend1
> >>> └── frontend2
> >>>
> >>>
> >>> regards
> >>> Antti
> >>
> >> static int lme2510_get_usb_stream_config(struct dvb_frontend *fe,
> >> struct usb_data_stream_properties *stream)
> >> {
> >> struct dvb_usb_adapter *adap;
> >> struct lme2510_state *state;
> >>
> >> stream->type = USB_BULK;
> >> stream->count = 10;
> >> stream->u.bulk.buffersize = 4096;
> >>
> >> // ugly part begins
> >> if (fe == NULL)
> >> return 0;
> >> adap = fe->dvb->priv;
> >> state = adap->dev->priv;
> >> // ugly part ends
> >>
> >> if (state->chip_id == lme2510c)
> >> stream->endpoint = 8;
> >> else
> >> stream->endpoint = 6;
> >>
> >> return 0;
> >> }
> >>
> >> Ugly part between comments is what I am going to change, but currently
> >> it works as is.
> >>
> > This doesn't work because the code will return the stream->endpoint
> > unset and then try to initialise usb_urb_init.
>
> So what? It happens only once when device is initialized. We do not need
> to know endpoint at that phase as we do not stream yet.
Please read your code you submit the urbs.
int dvb_usb_adapter_stream_init(struct dvb_usb_adapter *adap)
{
int ret;
struct usb_data_stream_properties stream_props;
adap->stream.udev = adap->dev->udev;
adap->stream.user_priv = adap;
/* resolve USB stream configuration for buffer alloc */
if (adap->dev->props->get_usb_stream_config) {
ret = adap->dev->props->get_usb_stream_config(NULL,
&stream_props);
if (ret < 0)
return ret;
} else {
stream_props = adap->props->stream;
}
/* FIXME: can be removed as set later in anyway */
adap->stream.complete = dvb_usb_data_complete;
return usb_urb_init(&adap->stream, &stream_props); <<<<----here
}
>
> > It would be far better to have get_usb_stream_config point to adapter
> > and point to adap->fe[adap->active_fe] inside if need be.
>
> Why?
Again read your code. why do we need frontend ? ... we want adapter.
Always pointing to same adapter regardless of frontend number.
static int af9015_get_usb_stream_config(struct dvb_frontend *fe,
struct usb_data_stream_properties *stream)
{
struct dvb_usb_adapter *adap;
deb_info("%s: fe=%p\n", __func__, fe);
stream->type = USB_BULK;
stream->count = 8;
stream->endpoint = 0x84;
stream->u.bulk.buffersize = TS_USB20_FRAME_SIZE;
if (fe == NULL)
return 0;
adap = fe->dvb->priv; <<< always same adapter
if (adap->id == 1)
stream->endpoint = 0x85;
if (adap->dev->udev->speed == USB_SPEED_FULL)
stream->u.bulk.buffersize = TS_USB11_FRAME_SIZE;
return 0;
}
> As we are going to configure stream based _frontend_ current needs we
> just need to know frontend and nothing more or less. If you pass adapter
> as a parameter then you don't know what is frontend in question as
> adapter coould have multiple frontends.
>
> active_fe member variable is meant to be internal use of DVB USB
> framework. You should not use it from the driver. You can see active
> frontend id just looking current frontend id (fe->id).
>
> > This is also the case with af9015, the code is extracting adapter
> > through another module.
>
> I don't understand what you mean.
>
> Also I would like to ask if you test code I given at all?
Yes, system crash. Endpoint not set.
>
> You seems not to understand that callback - get_usb_stream_config() - is
> there for configuring stream dynamically.
With get_usb_stream_config as;
int (*get_usb_stream_config) (struct dvb_usb_adapter *,
struct usb_data_stream_properties *);
If we really need the frontend we do this.
static int driverxxx_get_usb_stream_config(struct dvb_usb_adapter *adap,
struct usb_data_stream_properties *stream)
{
struct dvb_frontend *fe = adap->fe[adap->active_fe];
....
Regards
Malcolm
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
