Re: [RFCv1] DVB-USB improvements [alternative 2]

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

Em 20-05-2012 17:55, Antti Palosaari escreveu:
> I did some more planning and made alternative RFC.
> As the earlier alternative was more like changing old functionality that new one goes much more deeper.
> 
> As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
> static int frontend_attach(struct dvb_usb_adapter *adap)
> {
>   if (adap->id == 0)
>     return frontend_attach_1();
>   else
>     return frontend_attach_2();
> }
> 
> Functionality enhancement mentioned earlier RFC are valid too:
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg46352.html
> 
> As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":
> 
> struct dvb_usb_device_properties = {
>   /* download_firmware() success return values to signal what happens next */
>   #define RECONNECTS_USB                  (1 << 0)
>   #define RECONNECTS_USB_USING_NEW_ID     (1 << 1)
> 
>  .size_of_priv = sizeof(struct 'state'),
> 
>   /* firmware download */
>   .identify_state(struct dvb_usb_device *d, int *cold),
>   .get_firmware_name(struct dvb_usb_device *d, char *firmware_name),
>   .download_firmware(struct dvb_usb_device *d, const struct firmware *fw),
>   .allow_dynamic_id = true,
> 
>   .power_ctrl(struct dvb_usb_device *d, int onoff),
>   .read_config(struct dvb_usb_device *d, u8 mac[6]),
>   .get_adapter_count(struct dvb_usb_device *d, int *count),
> 
>   .frontend_attach(struct dvb_usb_adapter *adap),
>   .tuner_attach(struct dvb_usb_adapter *adap),
> 
>   .init(struct dvb_usb_device *d),
> 
>   .get_rc(struct dvb_rc *),
>   .i2c_algo = (struct i2c_algorithm),
> 
>   .frontend_ctrl(struct dvb_frontend *fe, int onoff),
>   .get_stream_props(struct usb_data_stream_properties *),
>   .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),
> 
>   .generic_bulk_ctrl_endpoint = (int),
>   .generic_bulk_ctrl_endpoint_response = (int),
> 
>   .devices = (struct dvb_usb_device)[],
> };
> 
> struct dvb_usb_device dvb_usb_devices {
>   char *name = "name",
>   .rc_map = RC_MAP_EMPTY,
>   .device_id = (struct usb_device_id),
> }

It looks OK to me. It may make sense to add an optional
per-device field, to allow drivers to add more board-specific
information, if they need, in order to avoid duplicating
things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
	int foo;
	long bar;
	...
}

And, inside the core, use the container_of() macro to go from
the device-specific table to struct dvb_usb_device.

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}

And complex drivers can add more stuff there.

Regards,
Mauro