[RFCv1] DVB-USB improvements [alternative 2]

[RFCv1] DVB-USB improvements [alternative 2]

[Antti Palosaari]

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 is likely Wednesday I am going to start implementing that one unless 
some big issues are raised. Making simple test code as a 
proof-of-concept  should not be very many days of work.

regards
Antti
-- 
http://palosaari.fi/

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

[VDR User]

On Sun, May 20, 2012 at 1:55 PM, Antti Palosaari <crope@iki.fi> wrote:
> 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.

> Functionality enhancement mentioned earlier RFC are valid too:
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg46352.html

One thing I didn't see mentioned in your post or the one your linked
is the rc dependency for _all_ usb devices, whether they even have rc
capability or not. It makes sense that is dvb usb is going to get
overhauled, that rc functionality be at the very least optional rather
than forced it on all usb devices.

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

[Devin Heitmueller]

On Sun, May 20, 2012 at 6:30 PM, VDR User <user.vdr@gmail.com> wrote:
> On Sun, May 20, 2012 at 1:55 PM, Antti Palosaari <crope@iki.fi> wrote:
>> 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.
>
>> Functionality enhancement mentioned earlier RFC are valid too:
>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg46352.html
>
> One thing I didn't see mentioned in your post or the one your linked
> is the rc dependency for _all_ usb devices, whether they even have rc
> capability or not. It makes sense that is dvb usb is going to get
> overhauled, that rc functionality be at the very least optional rather
> than forced it on all usb devices.

If you think this is important, then you should feel free to submit
patches to Antti's tree.  Otherwise, this is the sort of optimization
that brings so little value as to not really be worth the engineering
effort.  The time is better spent working on problems that *actually*
have a visible effect to users (and a few extra modules being loaded
does not fall into this category).

I think you'll find after spending a few hours trying to abstract out
the logic and the ugly solution that results that it *really* isn't
worth it.

Regards,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

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

[VDR User]

On Sun, May 20, 2012 at 4:10 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> If you think this is important, then you should feel free to submit
> patches to Antti's tree.  Otherwise, this is the sort of optimization
> that brings so little value as to not really be worth the engineering
> effort.  The time is better spent working on problems that *actually*
> have a visible effect to users (and a few extra modules being loaded
> does not fall into this category).
>
> I think you'll find after spending a few hours trying to abstract out
> the logic and the ugly solution that results that it *really* isn't
> worth it.

So you think that it makes more sense to ignore existing issues rather
than fix them. Isn't fixing issues & flaws the whole point of an
overhaul/redesign? Yes, it is. I do get the point you're trying to
make -- there are bigger fish to fry. But this is not an urgent
project and I disagree with the attitude to just disregard whatever
you deem unimportant. If you're going to do it, do it right.

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

[Antti Palosaari]

On 21.05.2012 03:36, VDR User wrote:
> On Sun, May 20, 2012 at 4:10 PM, Devin Heitmueller
> <dheitmueller@kernellabs.com>  wrote:
>> If you think this is important, then you should feel free to submit
>> patches to Antti's tree.  Otherwise, this is the sort of optimization
>> that brings so little value as to not really be worth the engineering
>> effort.  The time is better spent working on problems that *actually*
>> have a visible effect to users (and a few extra modules being loaded
>> does not fall into this category).
>>
>> I think you'll find after spending a few hours trying to abstract out
>> the logic and the ugly solution that results that it *really* isn't
>> worth it.
>
> So you think that it makes more sense to ignore existing issues rather
> than fix them. Isn't fixing issues&  flaws the whole point of an
> overhaul/redesign? Yes, it is. I do get the point you're trying to
> make -- there are bigger fish to fry. But this is not an urgent
> project and I disagree with the attitude to just disregard whatever
> you deem unimportant. If you're going to do it, do it right.

I am not sure what you trying to say. Do you mean I should try to get 
remote controller totally optional module which can be left out?

How much memory will be saved if remote can be left out as unloaded?

regards
Antti
-- 
http://palosaari.fi/

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

[VDR User]

On Sun, May 20, 2012 at 7:22 PM, Antti Palosaari <crope@iki.fi> wrote:
>> So you think that it makes more sense to ignore existing issues rather
>> than fix them. Isn't fixing issues&  flaws the whole point of an
>> overhaul/redesign? Yes, it is. I do get the point you're trying to
>> make -- there are bigger fish to fry. But this is not an urgent
>> project and I disagree with the attitude to just disregard whatever
>> you deem unimportant. If you're going to do it, do it right.
>
> I am not sure what you trying to say. Do you mean I should try to get remote
> controller totally optional module which can be left out?

I am saying it's a dependency that is currently forced onto every usb
device whether the device even supports rc or not. This is clearly
poor design. For that matter, the whole usb handling must be poor
design if it needs to be overhauled.

> How much memory will be saved if remote can be left out as unloaded?

I don't know. I'm merely pointing out just one of the issues that
should be resolved if the idea is to fix things that are wrong with
usb handling. If nobody cares, doesn't think it matters, or simply
doesn't want to bother, so be it. I guess I'm crazy but when I'm
facing an overhaul, especially when there's no rush, I compile a list
of _everything_ that's wrong and make sure the overhaul fixes it all.
I guess there's quite a difference between my idea of what an overhaul
should be, and other peoples.

If you really want, there's probably people who deal with embedded
systems where every wasted byte counts, that would agree cleaning up
the waste is a good idea.

Since nobody thinks it should be fixed, just pretend I didn't even
bring it up. Problem solved.

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

[Antti Palosaari]

ma 21.5.2012 5:42 VDR User kirjoitti:
> On Sun, May 20, 2012 at 7:22 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> So you think that it makes more sense to ignore existing issues rather
>>> than fix them. Isn't fixing issues&  flaws the whole point of an
>>> overhaul/redesign? Yes, it is. I do get the point you're trying to
>>> make -- there are bigger fish to fry. But this is not an urgent
>>> project and I disagree with the attitude to just disregard whatever
>>> you deem unimportant. If you're going to do it, do it right.
>>
>> I am not sure what you trying to say. Do you mean I should try to get
>> remote
>> controller totally optional module which can be left out?
>
> I am saying it's a dependency that is currently forced onto every usb
> device whether the device even supports rc or not. This is clearly
> poor design. For that matter, the whole usb handling must be poor
> design if it needs to be overhauled.
>
>> How much memory will be saved if remote can be left out as unloaded?
>
> I don't know. I'm merely pointing out just one of the issues that
> should be resolved if the idea is to fix things that are wrong with
> usb handling. If nobody cares, doesn't think it matters, or simply
> doesn't want to bother, so be it. I guess I'm crazy but when I'm
> facing an overhaul, especially when there's no rush, I compile a list
> of _everything_ that's wrong and make sure the overhaul fixes it all.
> I guess there's quite a difference between my idea of what an overhaul
> should be, and other peoples.
>
> If you really want, there's probably people who deal with embedded
> systems where every wasted byte counts, that would agree cleaning up
> the waste is a good idea.
>
> Since nobody thinks it should be fixed, just pretend I didn't even
> bring it up. Problem solved.

There is quite few devices that are shipped without remote. I agree that
it could be better and more modular. And it is asked multiple times during
these years. But my main motivation is to fix problems first and then do
enhancements. I will look if I can found some time for that too.

regards
Antti

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

[Mauro Carvalho Chehab]

Em 21-05-2012 00:20, Antti Palosaari escreveu:
> ma 21.5.2012 5:42 VDR User kirjoitti:
>> On Sun, May 20, 2012 at 7:22 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>> So you think that it makes more sense to ignore existing issues rather
>>>> than fix them. Isn't fixing issues&  flaws the whole point of an
>>>> overhaul/redesign? Yes, it is. I do get the point you're trying to
>>>> make -- there are bigger fish to fry. But this is not an urgent
>>>> project and I disagree with the attitude to just disregard whatever
>>>> you deem unimportant. If you're going to do it, do it right.
>>>
>>> I am not sure what you trying to say. Do you mean I should try to get
>>> remote
>>> controller totally optional module which can be left out?
>>
>> I am saying it's a dependency that is currently forced onto every usb
>> device whether the device even supports rc or not. This is clearly
>> poor design. For that matter, the whole usb handling must be poor
>> design if it needs to be overhauled.
>>
>>> How much memory will be saved if remote can be left out as unloaded?
>>
>> I don't know. I'm merely pointing out just one of the issues that
>> should be resolved if the idea is to fix things that are wrong with
>> usb handling. If nobody cares, doesn't think it matters, or simply
>> doesn't want to bother, so be it. I guess I'm crazy but when I'm
>> facing an overhaul, especially when there's no rush, I compile a list
>> of _everything_ that's wrong and make sure the overhaul fixes it all.
>> I guess there's quite a difference between my idea of what an overhaul
>> should be, and other peoples.
>>
>> If you really want, there's probably people who deal with embedded
>> systems where every wasted byte counts, that would agree cleaning up
>> the waste is a good idea.
>>
>> Since nobody thinks it should be fixed, just pretend I didn't even
>> bring it up. Problem solved.
> 
> There is quite few devices that are shipped without remote. I agree that
> it could be better and more modular. And it is asked multiple times during
> these years. But my main motivation is to fix problems first and then do
> enhancements. I will look if I can found some time for that too.

I see two separate issues here:

1) the dvb_usb properties struct needs to point if the device has RC or not.
It could be possible to optimize it if the remote code is not enabled, but
provided that the structure is properly designed, the only per-device information
is the RC keytable. Optimizing it when IR is not compiled will save just
a few bytes per card. Not worth, IMHO.

2) Remove the RC dependency from dvb-usb, and making the dvb-usb-remote code
as a module. This can bring some savings, as the RC core and IR decoders won't
be loaded.

I think (2) is valuable to do, but this can be done latter with a likely simple
patch, and won't require any changes at the DVB structures.

Regards,
Mauro

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

[Mauro Carvalho Chehab]

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

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

[Antti Palosaari]

On 21.05.2012 06:50, Mauro Carvalho Chehab wrote:
> 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.

I have now implemented some basic stuff. Most interesting is new way of 
map device id and properties for it. I found that I can use .driver_info 
field from the (struct usb_device_id) to carry pointer. I used it to 
carry all the other data to the DVB USB core. Thus that one big issue is 
now resolved. It reduces something like 8-9 kB of binary size which is 
huge improvement. Same will happen for every driver using multiple 
(struct dvb_usb_device_properties) - for more you are used more you save.

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

regards
Antti
-- 
http://palosaari.fi/

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

[Antti Palosaari]

On 25.05.2012 20:44, Antti Palosaari wrote:
> I have now implemented some basic stuff. Most interesting is new way of
> map device id and properties for it. I found that I can use .driver_info
> field from the (struct usb_device_id) to carry pointer. I used it to
> carry all the other data to the DVB USB core. Thus that one big issue is
> now resolved. It reduces something like 8-9 kB of binary size which is
> huge improvement. Same will happen for every driver using multiple
> (struct dvb_usb_device_properties) - for more you are used more you save.

Argh, reduces 8-9kB from the *af9015* as it was defining three (struct 
dvb_usb_device_properties). Also line count reduces 350 lines and will 
reduce some more after all those planned callbacks are done.

> Here is 3 example drivers I have converted to that new style:
> http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

Antti
-- 
http://palosaari.fi/

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

[Mauro Carvalho Chehab]

Em 25-05-2012 14:44, Antti Palosaari escreveu:
> On 21.05.2012 06:50, Mauro Carvalho Chehab wrote:
>> 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.
> 
> I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.
> 
> Here is 3 example drivers I have converted to that new style:
> http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

It will be better if you inline a diff at the email, instead of pointing 
to the changed files.

Also, instead of doing:

static struct usb_device_id af9015_usb_table[] = {
	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
		.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
			.name = "Afatech AF9015 reference design",
			.props = &af9015_properties })},

I suggest you to add a macro for it, converting the above to something similar to:

	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design", &af9015_properties),

or, even better, to:

	DVB_USB_DEVICE(USB_VID_AFATECH,
		       USB_PID_AFATECH_AF9015_9015,
		       "Afatech AF9015 reference design", 
		       &af9015_properties),

Btw, I still think that you should move things like RC type to be stored outside
the properties struct.

Regards,
Mauro

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

[Mauro Carvalho Chehab]

Em 25-05-2012 15:32, Mauro Carvalho Chehab escreveu:
> Em 25-05-2012 14:44, Antti Palosaari escreveu:
>> On 21.05.2012 06:50, Mauro Carvalho Chehab wrote:
>>> 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.
>>
>> I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.
>>
>> Here is 3 example drivers I have converted to that new style:
>> http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/
> 
> It will be better if you inline a diff at the email, instead of pointing 
> to the changed files.
> 
> Also, instead of doing:
> 
> static struct usb_device_id af9015_usb_table[] = {
> 	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
> 		.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
> 			.name = "Afatech AF9015 reference design",
> 			.props = &af9015_properties })},
> 
> I suggest you to add a macro for it, converting the above to something similar to:
> 
> 	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design", &af9015_properties),
> 
> or, even better, to:
> 
> 	DVB_USB_DEVICE(USB_VID_AFATECH,
> 		       USB_PID_AFATECH_AF9015_9015,
> 		       "Afatech AF9015 reference design", 
> 		       &af9015_properties),
> 
> Btw, I still think that you should move things like RC type to be stored outside
> the properties struct.

Ah, I suggest you to try to convert a driver like dib0700 (with has 83 types of devices
and several options of tuners/demods). 

It shold be noticed that dib0700, has two tables (well, there are actually several 
different keymaps there - but they ended to be converted into just two tables - 
mainly because of my lack of time and device specifics knowledge to break the NEC 
and RC5 tables on a per-device table). Any dvb-usb driver newer than RC core that
supports multiple vendors will have a different RC table for each device.

Regards,
Mauro
> 
> Regards,
> Mauro

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

[Antti Palosaari]

On 25.05.2012 21:32, Mauro Carvalho Chehab wrote:
> Em 25-05-2012 14:44, Antti Palosaari escreveu:
>> On 21.05.2012 06:50, Mauro Carvalho Chehab wrote:
>>> 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.
>>
>> I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.
>>
>> Here is 3 example drivers I have converted to that new style:
>> http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/
>
> It will be better if you inline a diff at the email, instead of pointing
> to the changed files.
>
> Also, instead of doing:
>
> static struct usb_device_id af9015_usb_table[] = {
> 	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
> 		.driver_info = (kernel_ulong_t)&((struct dvb_usb_driver_info) {
> 			.name = "Afatech AF9015 reference design",
> 			.props =&af9015_properties })},
>
> I suggest you to add a macro for it, converting the above to something similar to:
>
> 	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design",&af9015_properties),
>
> or, even better, to:
>
> 	DVB_USB_DEVICE(USB_VID_AFATECH,
> 		       USB_PID_AFATECH_AF9015_9015,
> 		       "Afatech AF9015 reference design",
> 		&af9015_properties),

Using macro you will save mostly one line but it looks maybe a little 
bit cleaner. There was such macros used by some other USB drivers.

> Btw, I still think that you should move things like RC type to be stored outside
> the properties struct.

Keymap is moved similarly as a name
.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
	.name = "TerraTec Cinergy T Stick Dual RC",
	.rc_map = RC_MAP_TERRATEC_SLIM,
	.props = &af9015_properties })},


For the other remote configurations I will introduce callback. Also 
callback for the stream and pid filters, which is called every-time when 
streaming is started. IIRC stream was the reason Michael introduced 
stream configuration per frontend. Also sometimes it could be nice to 
configure smaller stream buffers runtime for example in case of pid 
filters are enabled.

My development tree is here:
http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb-usb


Antti
-- 
http://palosaari.fi/