Re: Object code duplication in sound/pci/echoaudio/

[Denys Vlasenko <dvlasenk@redhat.com>]

On 05/14/2015 10:32 PM, Giuliano Pochini wrote:
> On Mon, 11 May 2015 22:15:28 +0200
> Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
>> There are fourteen files in sound/pci/echoaudio/, namely:
>>
>> darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
>> indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
>> layla24.c mia.c mona.c
>>
>> Which use the following method of "code reuse":
>>
>> #include "echoaudio_dsp.c"
>> #include "echoaudio_gml.c"
>> #include "echoaudio.c"
>>
>> echoaudio.c is not a header file, it contains a bunch of
>> static functions, some of a considerable size.
>> This makes those functions to be duplicated many times over.
> 
> Not really. It is quite unlikely that there are two different Echoaudio cards installed.

Distros tend to build almost all drivers in the tree.
On Fedora, kernel has a config where the following drivers
are built from sources in sound/pci/echoaudio/:

$ ls /lib/modules/4.0.0/kernel/sound/pci/echoaudio/
snd-darla20.ko	snd-gina20.ko	 snd-indigodjx.ko  snd-indigo.ko   snd-mia.ko
snd-darla24.ko	snd-gina24.ko	 snd-indigoio.ko   snd-layla20.ko  snd-mona.ko
snd-echo3g.ko	snd-indigodj.ko  snd-indigoiox.ko  snd-layla24.ko

>> For instance, there are fourteen instances of init_engine(),
>> each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
>> long.
>>
>> 11 get_firmware
>> 10 free_firmware
>> 13 audiopipe_free
>> 14 init_hw
>> 14 hw_rule_capture_format_by_channels
>> 14 hw_rule_capture_channels_by_format
>> 14 hw_rule_playback_format_by_channels
>> 14 hw_rule_playback_channels_by_format
>>
>> and so on.
>>
>> In my humble opinion, this is not a good coding practice.
>> You should not duplicate functions like this.
>> Where possible, you need to reuse a single instance of a function.
> 
> One option is to make a single driver which supports all the cards.
> There is not any duplicated code, but there is a lot of unused code.
> The other way (the one I choosed) is to build many specialized drivers.

If you have a lot of common code, you just need to have it factored out
into a separate module, say, echoaudio-core.c,
and make individual drivers depend on it.

This is done all over the kernel (there are nearly 400 *core*.c files
in the tree).