[alsa-devel] [PATCH 0/2] ALSA: bebob/fireworks: remove module parameters to reduce maintenance work
Hi,
This patchset removes some module parameters from ALSA bebob and fireworks driver.
These two modules have three parameters for users to assign preferable number and strings to a sound card as some modules in ALSA kernel land. However, they're not used so widely, and practically I suspect their advantages.
To simplify and unify the shape of modules in ALSA firewire stack, I'd like to remove them.
Takashi Sakamoto (2): ALSA: bebob: remove module parameters to reduce maintenance work ALSA: fireworks: remove module parameters to reduce maintenance work
sound/firewire/bebob/bebob.c | 49 +++++------------------------------- sound/firewire/bebob/bebob.h | 1 - sound/firewire/fireworks/fireworks.c | 43 +++---------------------------- sound/firewire/fireworks/fireworks.h | 1 - 4 files changed, 10 insertions(+), 84 deletions(-)
ALSA bebob driver has some parameters; 'index', 'id', 'enable'. Each parameter consists of array. When a bebob unit is probed, it consumes a set of elements from head of the array. Users can set preferred value to the array, to assign these values to a card relevant to the unit.
The meaning of each parameter is: - index: used as 'card number' maintained by 'snd' module. The scope of card number is system wide. When duplicated value is set, the module detect it and the card fails to be probed. - id: used as 'identification name'. For example, users can see it in '/proc/asound/cards' - enable: whether the element of array can be used or unused for a sound card instance. If not enabled, it skips and next element is used.
Well, this commit is going to purge these parameters. In alsa-lib, the card number is used for users to identify preferred card, however ID name can be also used for the same purpose (i.e. 'hw:F1814,0' can be used instead of 'hw:1,0' in a case that the unit is identified as card number 1). Users are not necessarily forced to use the card number. Therefore, the 'index' parameter has less advantage.
Additionally, most users have one unit. If they have several units, these units might not the same models. Thus, the 'id' parameter is useless because these units mostly have the different names. If using units with the same model, they can know which card corresponds to a unit, because units on IEEE 1394 has global unique ID (GUID) and drivers in ALSA firewire stack except for dice driver pushes the GUID to 'longname' field of sound card. As another way, hwdep interface give a way for users to get the correspondence between the sound card and GUID. For example, libhinawa is such the hwdep application.
Removal of the parameters reduces relevant codes, thus reduces maintenance work. This commit purge them.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/bebob/bebob.c | 49 ++++++-------------------------------------- sound/firewire/bebob/bebob.h | 1 - 2 files changed, 6 insertions(+), 44 deletions(-)
diff --git a/sound/firewire/bebob/bebob.c b/sound/firewire/bebob/bebob.c index 932901d..e429e54 100644 --- a/sound/firewire/bebob/bebob.c +++ b/sound/firewire/bebob/bebob.c @@ -18,20 +18,6 @@ MODULE_DESCRIPTION("BridgeCo BeBoB driver"); MODULE_AUTHOR("Takashi Sakamoto o-takashi@sakamocchi.jp"); MODULE_LICENSE("GPL v2");
-static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; -static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; -static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP; - -module_param_array(index, int, NULL, 0444); -MODULE_PARM_DESC(index, "card index"); -module_param_array(id, charp, NULL, 0444); -MODULE_PARM_DESC(id, "ID string"); -module_param_array(enable, bool, NULL, 0444); -MODULE_PARM_DESC(enable, "enable BeBoB sound card"); - -static DEFINE_MUTEX(devices_mutex); -static DECLARE_BITMAP(devices_used, SNDRV_CARDS); - /* Offsets from information register. */ #define INFO_OFFSET_BEBOB_VERSION 0x08 #define INFO_OFFSET_GUID 0x10 @@ -142,12 +128,6 @@ bebob_card_free(struct snd_card *card)
kfree(bebob->maudio_special_quirk);
- if (bebob->card_index >= 0) { - mutex_lock(&devices_mutex); - clear_bit(bebob->card_index, devices_used); - mutex_unlock(&devices_mutex); - } - mutex_destroy(&bebob->mutex); }
@@ -183,20 +163,8 @@ bebob_probe(struct fw_unit *unit, struct snd_card *card; struct snd_bebob *bebob; const struct snd_bebob_spec *spec; - unsigned int card_index; int err;
- mutex_lock(&devices_mutex); - - for (card_index = 0; card_index < SNDRV_CARDS; card_index++) { - if (!test_bit(card_index, devices_used) && enable[card_index]) - break; - } - if (card_index >= SNDRV_CARDS) { - err = -ENOENT; - goto end; - } - if ((entry->vendor_id == VEN_FOCUSRITE) && (entry->model_id == MODEL_FOCUSRITE_SAFFIRE_BOTH)) spec = get_saffire_spec(unit); @@ -210,19 +178,16 @@ bebob_probe(struct fw_unit *unit, if (spec == NULL) { if ((entry->vendor_id == VEN_MAUDIO1) || (entry->vendor_id == VEN_MAUDIO2)) - err = snd_bebob_maudio_load_firmware(unit); + return snd_bebob_maudio_load_firmware(unit); else - err = -ENOSYS; - goto end; + return -ENOSYS; }
- err = snd_card_new(&unit->device, index[card_index], id[card_index], - THIS_MODULE, sizeof(struct snd_bebob), &card); + err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE, + sizeof(struct snd_bebob), &card); if (err < 0) - goto end; + return err; bebob = card->private_data; - bebob->card_index = card_index; - set_bit(card_index, devices_used); card->private_free = bebob_card_free;
bebob->card = card; @@ -291,11 +256,9 @@ bebob_probe(struct fw_unit *unit, }
dev_set_drvdata(&unit->device, bebob); -end: - mutex_unlock(&devices_mutex); + return err; error: - mutex_unlock(&devices_mutex); snd_card_free(card); return err; } diff --git a/sound/firewire/bebob/bebob.h b/sound/firewire/bebob/bebob.h index b50bb33d..2f336a1 100644 --- a/sound/firewire/bebob/bebob.h +++ b/sound/firewire/bebob/bebob.h @@ -78,7 +78,6 @@ struct snd_bebob_spec { struct snd_bebob { struct snd_card *card; struct fw_unit *unit; - int card_index;
struct mutex mutex; spinlock_t lock;
ALSA fireworks driver has some parameters; 'index', 'id', 'enable'. Each parameter consists of array. When a fireworks unit is probed, it consumes a set of elements from head of the array. Users can set preferred value to the array, to assign these values to a card relevant to the unit.
The meaning of each parameter is: - index: used as 'card number' maintained by 'snd' module. The scope of card number is system wide. When duplicated value is set, the module detect it and the card fails to be probed. - id: used as 'identification name'. For example, users can see it in '/proc/asound/cards' - enable: whether the element of array can be used or unused for a sound card instance. If not enabled, it skips and next element is used.
Well, this commit is going to purge these parameters. In alsa-lib, the card number is used for users to identify preferred card, however ID name can be also used for the same purpose (i.e. 'hw:AudioFire2,0' can be used instead of 'hw:1,0' in a case that the unit is identified as card number 1). Users are not necessarily forced to use the card number. Therefore, the 'index' parameter has less advantage.
Additionally, most users have one unit. If they have several units, these units might not the same models. Thus, the 'id' parameter is useless because these units mostly have the different names. If using units with the same model, they can know which card corresponds to a unit, because units on IEEE 1394 has global unique ID (GUID) and drivers in ALSA firewire stack except for dice driver pushes the GUID to 'longname' field of sound card. As another way, hwdep interface give a way for users to get the correspondence between the sound card and GUID. For example, libhinawa is such the hwdep application.
Removal of the parameters reduces relevant codes, thus reduces maintenance work. This commit purge them.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/fireworks/fireworks.c | 43 ++++-------------------------------- sound/firewire/fireworks/fireworks.h | 1 - 2 files changed, 4 insertions(+), 40 deletions(-)
diff --git a/sound/firewire/fireworks/fireworks.c b/sound/firewire/fireworks/fireworks.c index 8380fb5..cbc0847 100644 --- a/sound/firewire/fireworks/fireworks.c +++ b/sound/firewire/fireworks/fireworks.c @@ -21,27 +21,15 @@ MODULE_DESCRIPTION("Echo Fireworks driver"); MODULE_AUTHOR("Takashi Sakamoto o-takashi@sakamocchi.jp"); MODULE_LICENSE("GPL v2");
-static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; -static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; -static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP; unsigned int snd_efw_resp_buf_size = 1024; bool snd_efw_resp_buf_debug = false;
-module_param_array(index, int, NULL, 0444); -MODULE_PARM_DESC(index, "card index"); -module_param_array(id, charp, NULL, 0444); -MODULE_PARM_DESC(id, "ID string"); -module_param_array(enable, bool, NULL, 0444); -MODULE_PARM_DESC(enable, "enable Fireworks sound card"); module_param_named(resp_buf_size, snd_efw_resp_buf_size, uint, 0444); MODULE_PARM_DESC(resp_buf_size, "response buffer size (max 4096, default 1024)"); module_param_named(resp_buf_debug, snd_efw_resp_buf_debug, bool, 0444); MODULE_PARM_DESC(resp_buf_debug, "store all responses to buffer");
-static DEFINE_MUTEX(devices_mutex); -static DECLARE_BITMAP(devices_used, SNDRV_CARDS); - #define VENDOR_LOUD 0x000ff2 #define MODEL_MACKIE_400F 0x00400f #define MODEL_MACKIE_1200F 0x01200f @@ -201,12 +189,6 @@ efw_card_free(struct snd_card *card)
kfree(efw->resp_buf);
- if (efw->card_index >= 0) { - mutex_lock(&devices_mutex); - clear_bit(efw->card_index, devices_used); - mutex_unlock(&devices_mutex); - } - mutex_destroy(&efw->mutex); }
@@ -216,27 +198,13 @@ efw_probe(struct fw_unit *unit, { struct snd_card *card; struct snd_efw *efw; - int card_index, err; - - mutex_lock(&devices_mutex); - - /* check registered cards */ - for (card_index = 0; card_index < SNDRV_CARDS; ++card_index) { - if (!test_bit(card_index, devices_used) && enable[card_index]) - break; - } - if (card_index >= SNDRV_CARDS) { - err = -ENOENT; - goto end; - } + int err;
- err = snd_card_new(&unit->device, index[card_index], id[card_index], - THIS_MODULE, sizeof(struct snd_efw), &card); + err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE, + sizeof(struct snd_efw), &card); if (err < 0) - goto end; + return err; efw = card->private_data; - efw->card_index = card_index; - set_bit(card_index, devices_used); card->private_free = efw_card_free;
efw->card = card; @@ -287,12 +255,9 @@ efw_probe(struct fw_unit *unit, }
dev_set_drvdata(&unit->device, efw); -end: - mutex_unlock(&devices_mutex); return err; error: snd_efw_transaction_remove_instance(efw); - mutex_unlock(&devices_mutex); snd_card_free(card); return err; } diff --git a/sound/firewire/fireworks/fireworks.h b/sound/firewire/fireworks/fireworks.h index 96c4e0c..57f9af5 100644 --- a/sound/firewire/fireworks/fireworks.h +++ b/sound/firewire/fireworks/fireworks.h @@ -60,7 +60,6 @@ struct snd_efw_phys_grp { struct snd_efw { struct snd_card *card; struct fw_unit *unit; - int card_index;
struct mutex mutex; spinlock_t lock;
On Tue, 29 Mar 2016 15:22:13 +0200, Takashi Sakamoto wrote:
Hi,
This patchset removes some module parameters from ALSA bebob and fireworks driver.
These two modules have three parameters for users to assign preferable number and strings to a sound card as some modules in ALSA kernel land. However, they're not used so widely, and practically I suspect their advantages.
To simplify and unify the shape of modules in ALSA firewire stack, I'd like to remove them.
Well, "it simplifies the code" alone doesn't justify the removal of these long-time existing parameters. You can't know how it's been used only from the narrow range surveys. These are mostly workarounds for some special setups, so they don't hit usually.
The usefulness of these parameters is another question, though. They are far from perfect, and have logic flaws. They are present, however, mostly for consistency and historical reasons. (In some cases it's useful, e.g. a device gives some id string you don't want to spell. The parameter allows you to rename it. Or, in most cases, the index is used for reserving the first slot to onboard chip for some legacy apps.)
So, the question is how much benefit we can get by this action. Is this small piece of code really so much burden to maintain? If yes, I'd like to hear it at first. If not, we should evaluate the risk of possible breakage as the first priority.
thanks,
Takashi
Takashi Sakamoto (2): ALSA: bebob: remove module parameters to reduce maintenance work ALSA: fireworks: remove module parameters to reduce maintenance work
sound/firewire/bebob/bebob.c | 49 +++++------------------------------- sound/firewire/bebob/bebob.h | 1 - sound/firewire/fireworks/fireworks.c | 43 +++---------------------------- sound/firewire/fireworks/fireworks.h | 1 - 4 files changed, 10 insertions(+), 84 deletions(-)
-- 2.7.3
Hi,
On Mar 29 2016 22:49, Takashi Iwai wrote:
On Tue, 29 Mar 2016 15:22:13 +0200, Takashi Sakamoto wrote:
Hi,
This patchset removes some module parameters from ALSA bebob and fireworks driver.
These two modules have three parameters for users to assign preferable number and strings to a sound card as some modules in ALSA kernel land. However, they're not used so widely, and practically I suspect their advantages.
To simplify and unify the shape of modules in ALSA firewire stack, I'd like to remove them.
Well, "it simplifies the code" alone doesn't justify the removal of these long-time existing parameters. You can't know how it's been used only from the narrow range surveys. These are mostly workarounds for some special setups, so they don't hit usually.
The usefulness of these parameters is another question, though. They are far from perfect, and have logic flaws. They are present, however, mostly for consistency and historical reasons. (In some cases it's useful, e.g. a device gives some id string you don't want to spell. The parameter allows you to rename it. Or, in most cases, the index is used for reserving the first slot to onboard chip for some legacy apps.)
So, the question is how much benefit we can get by this action. Is this small piece of code really so much burden to maintain? If yes, I'd like to hear it at first. If not, we should evaluate the risk of possible breakage as the first priority.
At least, to me. When purging these codes, I do never bother anymore the unpractical parameters and the differences of shape between drivers in ALSA firewire stack. It's good that I have no need to explain how these parameters works and when they don't works as expected.
Actually, ALSA firewire stack is not attractive for most of users and developers. It's preferable to keep the amount of codes in the stack as small as possible because I can't expect to gain dedicated testers and developers. In short, I hope to keep the codes within my capacity and exclude the confusion.
If it was as major as HDA, or it had impact to actual market as ASoC, I would use the other logic and strategy for software. But in fact, it's not.
I understand what you say. It's within my expectation. However, I hope.
Regards
Takashi Sakamoto (2): ALSA: bebob: remove module parameters to reduce maintenance work ALSA: fireworks: remove module parameters to reduce maintenance work
sound/firewire/bebob/bebob.c | 49 +++++------------------------------- sound/firewire/bebob/bebob.h | 1 - sound/firewire/fireworks/fireworks.c | 43 +++---------------------------- sound/firewire/fireworks/fireworks.h | 1 - 4 files changed, 10 insertions(+), 84 deletions(-)
-- 2.7.3
participants (2)
-
Takashi Iwai -
Takashi Sakamoto