[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PATCH: Query DVB frontend capabilities



Hi Andreas,

On Thu, Nov 10, 2011 at 8:14 PM, Andreas Oberritter <obi@xxxxxxxxxxx> wrote:
> Hello Manu,
>
> please see my comments below:
>
> On 10.11.2011 15:18, Manu Abraham wrote:
>> Hi,
>>
>> Currently, for a multi standard frontend it is assumed that it just
>> has a single standard capability. This is fine in some cases, but
>> makes things hard when there are incompatible standards in conjuction.
>> Eg: DVB-S can be seen as a subset of DVB-S2, but the same doesn't hold
>> the same for DSS. This is not specific to any driver as it is, but a
>> generic issue. This was handled correctly in the multiproto tree,
>> while such functionality is missing from the v5 API update.
>>
>> http://www.linuxtv.org/pipermail/vdr/2008-November/018417.html
>>
>> Later on a FE_CAN_2G_MODULATION was added as a hack to workaround this
>> issue in the v5 API, but that hack is incapable of addressing the
>> issue, as it can be used to simply distinguish between DVB-S and
>> DVB-S2 alone, or another x vs X2 modulation. If there are more
>> systems, then you have a potential issue.
>>
>> In addition to the patch, for illustrative purposes the stb0899 driver
>> is depicted providing the said capability information.
>>
>> An application needs to query the device capabilities before
>> requesting an operation from the device.
>>
>> If people don't have any objections, Probably other drivers can be
>> adapted similarly. In fact the change is quite simple.
>>
>> Comments ?
>>
>> Regards,
>> Manu
>>
>>
>> query_frontend_capabilities.diff
>>
>>
>> diff -r b6eb04718aa9 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Wed Nov 09 19:52:36 2011 +0530
>> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Thu Nov 10 13:51:35 2011 +0530
>> @@ -973,6 +973,8 @@
>>       _DTV_CMD(DTV_GUARD_INTERVAL, 0, 0),
>>       _DTV_CMD(DTV_TRANSMISSION_MODE, 0, 0),
>>       _DTV_CMD(DTV_HIERARCHY, 0, 0),
>> +
>> +     _DTV_CMD(DTV_DELIVERY_CAPS, 0, 0),
>>  };
>>
>>  static void dtv_property_dump(struct dtv_property *tvp)
>> @@ -1226,7 +1228,18 @@
>>               c = &cdetected;
>>       }
>>
>> +     dprintk("%s\n", __func__);
>> +
>>       switch(tvp->cmd) {
>> +     case DTV_DELIVERY_CAPS:
>> +             if (fe->ops.delivery_caps) {
>> +                     r = fe->ops.delivery_caps(fe, tvp);
>> +                     if (r < 0)
>> +                             return r;
>> +                     else
>> +                             goto done;
>
> What's the reason for introducing that label and goto?


The idea was to avoid using get_property callback being called.



>
>> +             }
>> +             break;
>>       case DTV_FREQUENCY:
>>               tvp->u.data = c->frequency;
>>               break;
>> @@ -1350,7 +1363,7 @@
>>               if (r < 0)
>>                       return r;
>>       }
>> -
>> +done:
>>       dtv_property_dump(tvp);
>>
>>       return 0;
>> diff -r b6eb04718aa9 linux/drivers/media/dvb/dvb-core/dvb_frontend.h
>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.h Wed Nov 09 19:52:36 2011 +0530
>> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.h Thu Nov 10 13:51:35 2011 +0530
>> @@ -305,6 +305,8 @@
>>
>>       int (*set_property)(struct dvb_frontend* fe, struct dtv_property* tvp);
>>       int (*get_property)(struct dvb_frontend* fe, struct dtv_property* tvp);
>> +
>> +     int (*delivery_caps)(struct dvb_frontend *fe, struct dtv_property *tvp);
>>  };
>
> I don't think that another function pointer is required. Drivers can
> implement this in their get_property callback. The core could provide a
> default implementation, returning values derived from fe->ops.info.type
> and the 2G flag.


I see your point. While trying to address the issue I had originally
get_property being used. One Issue that came across to me was that:

 - while currently this is just one capability field, things do look
fine. As we move ahead we will possibly have more capability related
information, given the idea that with shared hardware infrastructure
on frontends, this list of properties could grow.

- Once we have a few properties, then we will need to have switches in
that callback, given that it is generic and that length will grow.
Considering different hardware capabilities, we will have quite a bit
of shared code amongst drivers in that large switch, causing code
duplication.
Eg: with hardware having shared infrastructure - while globally it
might have a larger set of capabilities, when a sub part of it is run
in some mode, the other sub parts would have reduced set of
capabilities, which won't be the same as that exists globally for the
device. The driver alone will know the change depending on what's
being used.


- Additionally, though insignificant a separate callback for each
capability will make the drivers look a bit more consistent with
regards to code style.

The basic issue is a switch that will be duplicated across drivers and
that which is expected to grow in length, eventually readability a
question. That said, this shouldn't be an issue at all. Given the
situation at this exact moment, having a generic callback is just
fine, as that will be the only which would be there, right now.


>
>>  #define MAX_EVENT 8
>> @@ -362,6 +364,8 @@
>>
>>       /* DVB-T2 specifics */
>>       u32                     dvbt2_plp_id;
>> +
>> +     fe_delivery_system_t    delivery_caps[32];
>>  };
>
> This array seems to be unused.


That just walked in astray while I worked on it. Didn't notice at all. :-)
I will send an updated patch, do you still think the generic callback
is better ?



>
> Regards,
> Andreas
>
>>  struct dvb_frontend {
>> diff -r b6eb04718aa9 linux/drivers/media/dvb/frontends/stb0899_drv.c
>> --- a/linux/drivers/media/dvb/frontends/stb0899_drv.c Wed Nov 09 19:52:36 2011 +0530
>> +++ b/linux/drivers/media/dvb/frontends/stb0899_drv.c Thu Nov 10 13:51:35 2011 +0530
>> @@ -1605,6 +1605,19 @@
>>       return DVBFE_ALGO_CUSTOM;
>>  }
>>
>> +static int stb0899_delivery_caps(struct dvb_frontend *fe, struct dtv_property *caps)
>> +{
>> +     struct stb0899_state *state             = fe->demodulator_priv;
>> +
>> +     dprintk(state->verbose, FE_DEBUG, 1, "Get caps");
>> +     caps->u.buffer.data[0] = SYS_DSS;
>> +     caps->u.buffer.data[1] = SYS_DVBS;
>> +     caps->u.buffer.data[2] = SYS_DVBS2;
>> +     caps->u.buffer.len = 3;
>> +
>> +     return 0;
>> +}
>> +
>>  static struct dvb_frontend_ops stb0899_ops = {
>>
>>       .info = {
>> @@ -1647,6 +1660,8 @@
>>       .diseqc_send_master_cmd         = stb0899_send_diseqc_msg,
>>       .diseqc_recv_slave_reply        = stb0899_recv_slave_reply,
>>       .diseqc_send_burst              = stb0899_send_diseqc_burst,
>> +
>> +     .delivery_caps                  = stb0899_delivery_caps,
>>  };
>>
>>  struct dvb_frontend *stb0899_attach(struct stb0899_config *config, struct i2c_adapter *i2c)
>> diff -r b6eb04718aa9 linux/include/linux/dvb/frontend.h
>> --- a/linux/include/linux/dvb/frontend.h      Wed Nov 09 19:52:36 2011 +0530
>> +++ b/linux/include/linux/dvb/frontend.h      Thu Nov 10 13:51:35 2011 +0530
>> @@ -316,7 +316,9 @@
>>
>>  #define DTV_DVBT2_PLP_ID     43
>>
>> -#define DTV_MAX_COMMAND                              DTV_DVBT2_PLP_ID
>> +#define DTV_DELIVERY_CAPS    44
>> +
>> +#define DTV_MAX_COMMAND                              DTV_DELIVERY_CAPS
>>
>>  typedef enum fe_pilot {
>>       PILOT_ON,
>>
>
>

Thanks,
Manu
--
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


[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux