On 05/12/17 16:37, Greg KH wrote:
On Tue, Dec 05, 2017 at 04:14:25PM +0000, Michael Drake wrote:These descriptor definitions descibe how raw descriptor data should be interpreted. --- desc-defs.c | 647 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ desc-defs.h | 153 ++++++++++++++ 2 files changed, 800 insertions(+)Don't we need to add the file to the build here?
As noted in the other patch review, its added to the build in the patch that update lsusb to use it.
create mode 100644 desc-defs.c create mode 100644 desc-defs.h diff --git a/desc-defs.c b/desc-defs.c new file mode 100644 index 0000000..6bc558e --- /dev/null +++ b/desc-defs.c @@ -0,0 +1,647 @@ +/*****************************************************************************/ + +/* + * desc-defs.c -- USB descriptor definitions + * + * Copyright (C) 2017 Michael Drake <michael.drake@xxxxxxxxxxxxxxx> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +/*****************************************************************************/Wow that's a large boiler plate of a header. Not your fault, you are copying lsusb.c, I'll fix that up later :)
Yes, I actually copied the boiler plate from names.{c|h}, since its
an existing module used by lsusb.c.
+ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endifWhy #ifdef this?
Purely because names.c did. I've removed the #ifdef and it still builds.
+ +#include <stdbool.h> +#include <stdlib.h> +#include <stdio.h> + +#include "desc-defs.h" + +static const char * const uac2_interface_header_bmcontrols[] = { + "Latency control", + NULL +};All of these arrays have to be in the specific order, right? Do we need a comment somewhere saying this?
Yes, I've added designated initializers and comments to make this clearer.
+const struct desc * const desc_audio_ac_processing_unit[3] = { + desc_audio_1_ac_processing_unit, + desc_audio_2_ac_processing_unit, + NULL, /* UAC3 not implemented yet */ +}; + + +/** UAC1: 4.3.2.5 Feature Unit Descriptor; Table 4-7. */ +static const struct desc desc_audio_1_ac_feature_unit[] = { + { .field = "bUnitID", .size = 1, .type = DESC_NUMBER }, + { .field = "bSourceID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bControlSize", .size = 1, .type = DESC_NUMBER }, + { .field = "bmaControls", .size_field = "bControlSize", .type = DESC_BMCONTROL_1, + .bmcontrol = uac_feature_unit_bmcontrols, .array = { .array = true } }, + { .field = "iFeature", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +/** UAC2: 4.7.2.8 Feature Unit Descriptor; Table 4-13. */ +static const struct desc desc_audio_2_ac_feature_unit[] = { + { .field = "bUnitID", .size = 1, .type = DESC_NUMBER }, + { .field = "bSourceID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bmaControls", .size = 4, .type = DESC_BMCONTROL_2, + .bmcontrol = uac_feature_unit_bmcontrols, .array = { .array = true } }, + { .field = "iFeature", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_feature_unit[3] = { + desc_audio_1_ac_feature_unit, + desc_audio_2_ac_feature_unit, + NULL, /* UAC3 not implemented yet */ +}; + +Just put 1 blank line between everything, makes it simpler to manage over time and to read :)
Done.
+/** UAC1: 4.3.2.7 Extension Unit Descriptor; Table 4-15. */ +static const struct desc desc_audio_1_ac_extension_unit[] = { + { .field = "bUnitID", .size = 1, .type = DESC_NUMBER }, + { .field = "wExtensionCode", .size = 2, .type = DESC_CONSTANT }, + { .field = "bNrInPins", .size = 1, .type = DESC_NUMBER }, + { .field = "baSourceID", .size = 1, .type = DESC_NUMBER, + .array = { .array = true, .length_field1 = "bNrInPins" } }, + { .field = "bNrChannels", .size = 1, .type = DESC_NUMBER }, + { .field = "wChannelConfig", .size = 2, .type = DESC_BITMAP_STRINGS, + .bitmap_strings = { .strings = uac1_channel_names, .count = 12 } }, + { .field = "iChannelNames", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = "bControlSize", .size = 1, .type = DESC_NUMBER }, + { .field = "bmControls", .size = 1, .type = DESC_BITMAP, + .array = { .array = true, .length_field1 = "bControlSize" } }, + { .field = "iExtension", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +/** UAC2: 4.7.2.12 Extension Unit Descriptor; Table 4-24. */ +static const struct desc desc_audio_2_ac_extension_unit[] = { + { .field = "bUnitID", .size = 1, .type = DESC_NUMBER }, + { .field = "wExtensionCode", .size = 2, .type = DESC_CONSTANT }, + { .field = "bNrInPins", .size = 1, .type = DESC_NUMBER }, + { .field = "baSourceID", .size = 1, .type = DESC_NUMBER, + .array = { .array = true, .length_field1 = "bNrInPins" } }, + { .field = "bNrChannels", .size = 1, .type = DESC_NUMBER }, + { .field = "bmChannelConfig", .size = 4, .type = DESC_BITMAP_STRINGS, + .bitmap_strings = { .strings = uac2_channel_names, .count = 26 } }, + { .field = "iChannelNames", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = "bmControls", .size = 1, .type = DESC_BMCONTROL_2, + .bmcontrol = uac2_extension_unit_bmcontrols }, + { .field = "iExtension", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_extension_unit[3] = { + desc_audio_1_ac_extension_unit, + desc_audio_2_ac_extension_unit, + NULL, /* UAC3 not implemented yet */ +}; + + +static const char * const uac2_clk_src_bmattr[] = { + "External", + "Internal fixed", + "Internal variable", + "Internal programmable" +}; +static const char * const uac3_clk_src_bmattr[] = { + "External", + "Internal", + "(asynchronous)", + "(synchronized to SOF)" +}; + +/** Special rendering function for UAC2 clock source bmAttributes */ +static void desc_snowflake_dump_uac2_clk_src_bmattr( + unsigned long long value, + unsigned int indent) +{ + printf(" %s clock %s\n", + uac2_clk_src_bmattr[value & 0x3], + (value & 0x4) ? uac3_clk_src_bmattr[3] : ""); +} + +/** UAC2: 4.7.2.1 Clock Source Descriptor; Table 4-6. */ +static const struct desc desc_audio_2_ac_clock_source[] = { + { .field = "bClockID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bmAttributes", .size = 1, .type = DESC_SNOWFLAKE, + .snowflake = desc_snowflake_dump_uac2_clk_src_bmattr }, + { .field = "bmControls", .size = 1, .type = DESC_BMCONTROL_2, + .bmcontrol = uac2_clock_source_bmcontrols }, + { .field = "bAssocTerminal", .size = 1, .type = DESC_CONSTANT }, + { .field = "iClockSource", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_clock_source[3] = { + NULL, /* UAC1 not supported */ + desc_audio_2_ac_clock_source, + NULL, /* UAC3 not implemented yet */ +}; + + +/** UAC2: 4.7.2.2 Clock Selector Descriptor; Table 4-7. */ +static const struct desc desc_audio_2_ac_clock_selector[] = { + { .field = "bClockID", .size = 1, .type = DESC_NUMBER }, + { .field = "bNrInPins", .size = 1, .type = DESC_NUMBER }, + { .field = "baCSourceID", .size = 1, .type = DESC_NUMBER, + .array = { .array = true, .length_field1 = "bNrInPins" } }, + { .field = "bmControls", .size = 1, .type = DESC_BMCONTROL_2, + .bmcontrol = uac2_clock_selector_bmcontrols }, + { .field = "iClockSelector", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_clock_selector[3] = { + NULL, /* UAC1 not supported */ + desc_audio_2_ac_clock_selector, + NULL, /* UAC3 not implemented yet */ +}; + + +/** UAC2: 4.7.2.3 Clock Multiplier Descriptor; Table 4-8. */ +static const struct desc desc_audio_2_ac_clock_multiplier[] = { + { .field = "bClockID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bCSourceID", .size = 1, .type = DESC_NUMBER }, + { .field = "bmControls", .size = 1, .type = DESC_BMCONTROL_2, + .bmcontrol = uac2_clock_multiplier_bmcontrols }, + { .field = "iClockMultiplier", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_clock_multiplier[3] = { + NULL, /* UAC1 not supported */ + desc_audio_2_ac_clock_multiplier, + NULL, /* UAC3 not implemented yet */ +}; + + +/** UAC2: 4.7.2.9 Sampling Rate Converter Descriptor; Table 4-14. */ +static const struct desc desc_audio_2_ac_sample_rate_converter[] = { + { .field = "bUnitID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bSourceID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bCSourceInID", .size = 1, .type = DESC_CONSTANT }, + { .field = "bCSourceOutID", .size = 1, .type = DESC_CONSTANT }, + { .field = "iSRC", .size = 1, .type = DESC_STR_DESC_INDEX }, + { .field = NULL } +}; +const struct desc * const desc_audio_ac_sample_rate_converter[3] = { + NULL, /* UAC1 not supported */ + desc_audio_2_ac_sample_rate_converter, + NULL, /* UAC3 not implemented yet */ +}; + + +static const char * const uac2_as_interface_bmcontrols[] = { + "Active Alternate Setting", + "Valid Alternate Setting", + NULL +}; +static const char * const audio_data_format_type_i[] = { + "TYPE_I_UNDEFINED", + "PCM", + "PCM8", + "IEEE_FLOAT", + "ALAW", + "MULAW" +}; +static const char * const audio_data_format_type_ii[] = { + "TYPE_II_UNDEFINED", + "MPEG", + "AC-3" +}; +static const char * const audio_data_format_type_iii[] = { + "TYPE_III_UNDEFINED", + "IEC1937_AC-3", + "IEC1937_MPEG-1_Layer1", + "IEC1937_MPEG-Layer2/3/NOEXT", + "IEC1937_MPEG-2_EXT", + "IEC1937_MPEG-2_Layer1_LS", + "IEC1937_MPEG-2_Layer2/3_LS" +}; + +/** Special rendering function for UAC1 AS interface wFormatTag */ +static void desc_snowflake_dump_uac1_as_interface_wformattag( + unsigned long long value, + unsigned int indent) +{ + const char *format_string = "undefined"; + + if (value <= 5) {Isn't that 5 a #define somewhere?
No, I've changed to composing the values using a combination of #defines for the type, which gets shifted up and the array element length for the format value max for the type.
+ format_string = audio_data_format_type_i[value]; + + } else if (value >= 0x1000 && value <= 0x1002) { + format_string = audio_data_format_type_ii[value & 0xfff]; + + } else if (value >= 0x2000 && value <= 0x2006) {Same for 0x1000, 0x1002, 0x2000, and 0x2006, aren't they defined somewhere?
As I responded above.
--- /dev/null +++ b/desc-defs.h @@ -0,0 +1,153 @@ +/*****************************************************************************/ + +/* + * desc-defs.h -- USB descriptor definitions + * + * Copyright (C) 2017 Michael Drake <michael.drake@xxxxxxxxxxxxxxx> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +/*****************************************************************************/ + +#ifndef _DESC_DEFS_H +#define _DESC_DEFS_H + +/* ---------------------------------------------------------------------- */ + +/** + * Descriptor field value type. + * + * Note that there are more types here than exist in the descriptor definitions + * in the specifications. This is because the type here is used to do `lsusb` + * specific rendering of certain fields. + * + * Note that the use of certain types mandates the setting of various entries + * in the type-specific anonymous union in `struct desc`. + */ +enum desc_type { + DESC_CONSTANT, /** Plain numerical value; no annotation. */ + DESC_NUMBER, /** Plain numerical value; no annotation. */ + DESC_NUMBER_POSTFIX, /**< Number with a postfix string. */ + DESC_BITMAP, /**< Plain hex rendered value; no annotation. */ + DESC_BCD, /**< Binary coded decimal */ + DESC_BMCONTROL_1, /**< UAC1 style bmControl field */ + DESC_BMCONTROL_2, /**< UAC2/UAC3 style bmControl field */ + DESC_STR_DESC_INDEX, /**< String index. */ + DESC_TERMINAL_STR, /**< Audio terminal string. */ + DESC_BITMAP_STRINGS, /**< Bitfield with string per bit. */ + DESC_NUMBER_STRINGS, /**< Use for enum-style value to string. */ + DESC_SNOWFLAKE, /**< Value with custom annotation callback function. */ +}; + +/** + * Callback function for the DESC_SNOWFLAKE descriptor field value type. + * + * This is used when some special rendering of the value is required, which + * is specific to the field in question, so no generic type's rendering is + * suitable. + * + * The core descriptor dumping code will have already dumped the numerical + * value for the field, but not the trailing newline character. It is up + * to the callback function to ensure it always finishes by writing a '\n' + * character to stdout. + * + * \param[in] value The value to dump a human-readable representation of. + * \param[in] indent The current indent level. + */What is the style you are using for this formatting of the documentation? I don't think we use anything today, is it really needed?
The \param parts of the comment? They're Doxygen style comments. I think they're harmless here, where the parameters are pretty obvious. However there is some value in the other ones, such as for desc_dump() in desc-dump.c, where there are some intricacies with the API for programmers to be aware of.
+typedef void (*desc_snowflake_dump_fn)( + unsigned long long value, + unsigned int indent); + + +/** + * Descriptor field definition. + * + * Whole descriptors can be defined as NULL terminated arrays of these + * structures. + */ +struct desc { + const char *field; /**< Field's name */ + unsigned int size; /**< Byte size of field, if (size_field == NULL) */ + const char *size_field; /**< Name of field specifying field size. */ + enum desc_type type; /**< Field's value type. */ + + /** Anonymous union containing type-specific data. */ + union { + /** + * Corresponds to types DESC_BMCONTROL_1 and DESC_BMCONTROL_2. + * + * Must be a NULL terminated array of '\0' terminated strings. + */ + const char * const *bmcontrol; + /** Corresponds to type DESC_BITMAP_STRINGS */ + struct { + /** Must contain '\0' terminated strings. */ + const char * const *strings; + /** Number of strings in strings array. */ + unsigned int count; + } bitmap_strings; + /** + * Corresponds to type DESC_NUMBER_STRINGS. + * + * Must be a NULL terminated array of '\0' terminated strings. + */ + const char * const *number_strings; + /** + * Corresponds to type DESC_NUMBER_POSTFIX. + * + * Must be a '\0' terminated string. + */ + const char *number_postfix; + /** + * Corresponds to type DESC_SNOWFLAKE. + * + * Callback function called to annotate snowflake value type. + */ + desc_snowflake_dump_fn snowflake; + }; + + /** Grouping of array-specific fields. */ + struct { + bool array; /**< True if entry is an array. */ + bool bits; /**< True if array length is specified in bits */You use 1 for these two values, shouldn't you use 'true' instead?
I could not find any instances of this. In desc-defs.c, these members were always set to 'true', or else left unset for static initialization to zero. Cheers, -- Michael Drake http://www.codethink.co.uk/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
