Re: PATCH: Query DVB frontend capabilities

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

Em 11-11-2011 12:30, Andreas Oberritter escreveu:
> On 11.11.2011 07:26, Manu Abraham wrote:
>> 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	Fri Nov 11 06:05:40 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,11 @@
>>  		c = &cdetected;
>>  	}
>>  
>> +	dprintk("%s\n", __func__);
>> +
>>  	switch(tvp->cmd) {
>> +	case DTV_DELIVERY_CAPS:
> 
> It would be nice to have a default implementation inserted at this point, e.g. something like:
> 
> static void dtv_set_default_delivery_caps(const struct dvb_frontend *fe, struct dtv_property *p)
> {
> 	const struct dvb_frontend_info *info = &fe->ops.info;
> 	u32 ncaps = 0;
> 
> 	switch (info->type) {
> 	case FE_QPSK:
> 		p->u.buffer.data[ncaps++] = SYS_DVBS;
> 		if (info->caps & FE_CAN_2G_MODULATION)
> 			p->u.buffer.data[ncaps++] = SYS_DVBS2;
> 		if (info->caps & FE_CAN_TURBO_FEC)
> 			p->u.buffer.data[ncaps++] = SYS_TURBO;
> 		break;
> 	case FE_QAM:
> 		p->u.buffer.data[ncaps++] = SYS_DVBC_ANNEX_AC;
> 		break;
> 	case FE_OFDM:
> 		p->u.buffer.data[ncaps++] = SYS_DVBT;
> 		if (info->caps & FE_CAN_2G_MODULATION)
> 			p->u.buffer.data[ncaps++] = SYS_DVBT2;
> 		break;
> 	case FE_ATSC:
> 		if (info->caps & (FE_CAN_8VSB | FE_CAN_16VSB))
> 			p->u.buffer.data[ncaps++] = SYS_ATSC;
> 		if (info->caps & (FE_CAN_QAM_16 | FE_CAN_QAM_64 | FE_CAN_QAM_128 | FE_CAN_QAM_256))
> 			p->u.buffer.data[ncaps++] = SYS_DVBC_ANNEX_B;
> 	}
> 
> 	p->u.buffer.len = ncaps;
> }
> 
> I think this would be sufficient for a lot of drivers and would thus save a lot of work.
> 
>> +		break;
>>  	case DTV_FREQUENCY:
>>  		tvp->u.data = c->frequency;
>>  		break;
>> @@ -1350,7 +1356,7 @@
>>  		if (r < 0)
>>  			return r;
>>  	}
>> -
>> +done:
> 
> This label is unused now and should be removed.
> 
>>  	dtv_property_dump(tvp);
>>  
>>  	return 0;
>> 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	Fri Nov 11 06:05:40 2011 +0530
>> @@ -1605,6 +1605,22 @@
>>  	return DVBFE_ALGO_CUSTOM;
>>  }
>>  
>> +static int stb0899_get_property(struct dvb_frontend *fe, struct dtv_property *p)
>> +{
>> +	switch (p->cmd) {
>> +	case DTV_DELIVERY_CAPS:
>> +		p->u.buffer.data[0] = SYS_DSS;
>> +		p->u.buffer.data[1] = SYS_DVBS;
>> +		p->u.buffer.data[2] = SYS_DVBS2;
>> +		p->u.buffer.len = 3;
>> +		break;
>> +	default:
>> +		return -EINVAL;
> 
> You should ignore all unhandled properties. Otherwise all properties handled
> by the core will have result set to -EINVAL.

IMHO, the better is to set all parameters via stb0899_get_property(). We should
work on deprecate the old way, as, by having all frontends implementing the
get/set property ops, we can remove part of the legacy code inside the DVB core.

> 
>> +	}
>> +	return 0;
>> +}
>> +
>> +
>>  static struct dvb_frontend_ops stb0899_ops = {
>>  
>>  	.info = {
>> @@ -1647,6 +1663,8 @@
>>  	.diseqc_send_master_cmd		= stb0899_send_diseqc_msg,
>>  	.diseqc_recv_slave_reply	= stb0899_recv_slave_reply,
>>  	.diseqc_send_burst		= stb0899_send_diseqc_burst,
>> +
>> +	.get_property			= stb0899_get_property,
>>  };
>>  
>>  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	Fri Nov 11 06:05:40 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,
>>
>