On Sat, 26 Dec 2015 04:35:22 +0100,
Takashi Sakamoto wrote:
>
> Before allocating an instance of sound card, ALSA dice driver checks
> chip_ID_hi in Bus information block of Config ROM, then also checks
> subaddresses. The former operation reads cache of Config ROM in Linux
> FireWire subsystem, while the latter operation sends read transaction.
> The latter can be merged into initialization of transaction system.
>
> This commit splits these two operations to reduce needless transactions
> in probe processing.
>
> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> ---
> sound/firewire/dice/dice-transaction.c | 89 ++++++++++++++++++++++++++--------
> sound/firewire/dice/dice.c | 72 +++------------------------
> 2 files changed, 77 insertions(+), 84 deletions(-)
>
> diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
> index aee7461..75a2125 100644
> --- a/sound/firewire/dice/dice-transaction.c
> +++ b/sound/firewire/dice/dice-transaction.c
> @@ -331,39 +331,59 @@ int snd_dice_transaction_reinit(struct snd_dice *dice)
> return register_notification_address(dice, false);
> }
>
> -int snd_dice_transaction_init(struct snd_dice *dice)
> +static int get_subaddrs(struct snd_dice *dice)
> {
> - struct fw_address_handler *handler = &dice->notification_handler;
> + static const int min_values[10] = {
> + 10, 0x64 / 4,
> + 10, 0x18 / 4,
> + 10, 0x18 / 4,
> + 0, 0,
> + 0, 0,
> + };
> __be32 *pointers;
> + u32 data;
> + unsigned int i;
> int err;
>
> - /* Use the same way which dice_interface_check() does. */
> - pointers = kmalloc(sizeof(__be32) * 10, GFP_KERNEL);
> + pointers = kmalloc_array(ARRAY_SIZE(min_values), sizeof(__be32),
> + GFP_KERNEL);
> if (pointers == NULL)
> return -ENOMEM;
>
> - /* Get offsets for sub-addresses */
> + /*
> + * Check that the sub address spaces exist and are located inside the
> + * private address space. The minimum values are chosen so that all
> + * minimally required registers are included.
> + */
> err = snd_fw_transaction(dice->unit, TCODE_READ_BLOCK_REQUEST,
> - DICE_PRIVATE_SPACE,
> - pointers, sizeof(__be32) * 10, 0);
> + DICE_PRIVATE_SPACE, pointers,
> + sizeof(__be32) * ARRAY_SIZE(min_values), 0);
> if (err < 0)
> goto end;
>
> - /* Allocation callback in address space over host controller */
> - handler->length = 4;
> - handler->address_callback = dice_notification;
> - handler->callback_data = dice;
> - err = fw_core_add_address_handler(handler, &fw_high_memory_region);
> - if (err < 0) {
> - handler->callback_data = NULL;
> - goto end;
> + for (i = 0; i < ARRAY_SIZE(min_values); ++i) {
> + data = be32_to_cpu(pointers[i]);
> + if (data < min_values[i] || data >= 0x40000) {
> + err = -ENODEV;
> + goto end;
> + }
> }
>
> - /* Register the address space */
> - err = register_notification_address(dice, true);
> - if (err < 0) {
> - fw_core_remove_address_handler(handler);
> - handler->callback_data = NULL;
> + /*
> + * Check that the implemented DICE driver specification major version
> + * number matches.
> + */
> + err = snd_fw_transaction(dice->unit, TCODE_READ_QUADLET_REQUEST,
> + DICE_PRIVATE_SPACE +
> + be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
> + &data, sizeof(data), 0);
It's confusing to reuse a single variable as both u32 and be32.
Use the specific variable like the original code instead.
thanks,
Takashi
> + if (err < 0)
> + goto end;
> +
> + if ((data & cpu_to_be32(0xff000000)) != cpu_to_be32(0x01000000)) {
> + dev_err(&dice->unit->device,
> + "unknown DICE version: 0x%08x\n", be32_to_cpu(data));
> + err = -ENODEV;
> goto end;
> }
>
> @@ -380,3 +400,32 @@ end:
> kfree(pointers);
> return err;
> }
> +
> +int snd_dice_transaction_init(struct snd_dice *dice)
> +{
> + struct fw_address_handler *handler = &dice->notification_handler;
> + int err;
> +
> + err = get_subaddrs(dice);
> + if (err < 0)
> + return err;
> +
> + /* Allocation callback in address space over host controller */
> + handler->length = 4;
> + handler->address_callback = dice_notification;
> + handler->callback_data = dice;
> + err = fw_core_add_address_handler(handler, &fw_high_memory_region);
> + if (err < 0) {
> + handler->callback_data = NULL;
> + return err;
> + }
> +
> + /* Register the address space */
> + err = register_notification_address(dice, true);
> + if (err < 0) {
> + fw_core_remove_address_handler(handler);
> + handler->callback_data = NULL;
> + }
> +
> + return err;
> +}
> diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
> index 0cda05c..26271cc 100644
> --- a/sound/firewire/dice/dice.c
> +++ b/sound/firewire/dice/dice.c
> @@ -18,27 +18,12 @@ MODULE_LICENSE("GPL v2");
> #define WEISS_CATEGORY_ID 0x00
> #define LOUD_CATEGORY_ID 0x10
>
> -static int dice_interface_check(struct fw_unit *unit)
> +static int check_dice_category(struct fw_unit *unit)
> {
> - static const int min_values[10] = {
> - 10, 0x64 / 4,
> - 10, 0x18 / 4,
> - 10, 0x18 / 4,
> - 0, 0,
> - 0, 0,
> - };
> struct fw_device *device = fw_parent_device(unit);
> struct fw_csr_iterator it;
> - int key, val, vendor = -1, model = -1, err;
> - unsigned int category, i;
> - __be32 *pointers;
> - u32 value;
> - __be32 version;
> -
> - pointers = kmalloc_array(ARRAY_SIZE(min_values), sizeof(__be32),
> - GFP_KERNEL);
> - if (pointers == NULL)
> - return -ENOMEM;
> + int key, val, vendor = -1, model = -1;
> + unsigned int category;
>
> /*
> * Check that GUID and unit directory are constructed according to DICE
> @@ -64,51 +49,10 @@ static int dice_interface_check(struct fw_unit *unit)
> else
> category = DICE_CATEGORY_ID;
> if (device->config_rom[3] != ((vendor << 8) | category) ||
> - device->config_rom[4] >> 22 != model) {
> - err = -ENODEV;
> - goto end;
> - }
> -
> - /*
> - * Check that the sub address spaces exist and are located inside the
> - * private address space. The minimum values are chosen so that all
> - * minimally required registers are included.
> - */
> - err = snd_fw_transaction(unit, TCODE_READ_BLOCK_REQUEST,
> - DICE_PRIVATE_SPACE, pointers,
> - sizeof(__be32) * ARRAY_SIZE(min_values), 0);
> - if (err < 0) {
> - err = -ENODEV;
> - goto end;
> - }
> - for (i = 0; i < ARRAY_SIZE(min_values); ++i) {
> - value = be32_to_cpu(pointers[i]);
> - if (value < min_values[i] || value >= 0x40000) {
> - err = -ENODEV;
> - goto end;
> - }
> - }
> + device->config_rom[4] >> 22 != model)
> + return -ENODEV;
>
> - /*
> - * Check that the implemented DICE driver specification major version
> - * number matches.
> - */
> - err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
> - DICE_PRIVATE_SPACE +
> - be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
> - &version, 4, 0);
> - if (err < 0) {
> - err = -ENODEV;
> - goto end;
> - }
> - if ((version & cpu_to_be32(0xff000000)) != cpu_to_be32(0x01000000)) {
> - dev_err(&unit->device,
> - "unknown DICE version: 0x%08x\n", be32_to_cpu(version));
> - err = -ENODEV;
> - goto end;
> - }
> -end:
> - return err;
> + return 0;
> }
>
> static int highest_supported_mode_rate(struct snd_dice *dice,
> @@ -254,9 +198,9 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
> struct snd_dice *dice;
> int err;
>
> - err = dice_interface_check(unit);
> + err = check_dice_category(unit);
> if (err < 0)
> - goto end;
> + return -ENODEV;
>
> err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE,
> sizeof(*dice), &card);
> --
> 2.5.0
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
