[PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

 Share more product information, for user space utils such as PA and UCM to
 distinguish different products.
  1. Add core APIs to register and cleanup DMI names for card.
  2. Apply the APIs to bytcr-rt5640 driver.

 changes on V7:
  1. Remove inconsistent API description
  
 changes on V6:
  1. Use dynamic allocate and cleanup for card long name
  2. Remove unneccessary arguments to simplify the API
  
 changes on V5:
  1. Use independent space to store card long_name, to avoid irrelavant
  info sharing from card component
  2. Use letter ';' instead of ':' to separate strings in long name, in
  case name strings may also contain ':' and confuse user
  3. Fix error that vendor name and firmware name were not optional 
   
 changes on V4:
  1. Replace kmalloc() and snprintf() with ksaprintf() to simplify code
    
 changes on V3:
  1. Split the core API and the API call to two patches
  2. Replace misused strcat() with snprintf()
  3. Code and comment fix 

Lu, Han (2):
  ASoC: core: add API for registering and cleaning up DMI card names
  ASoC: bytcr-rt5640: register DMI names for card

 include/sound/soc.h                   |  3 ++
 sound/soc/intel/boards/bytcr_rt5640.c | 18 ++++++++
 sound/soc/soc-core.c                  | 85 +++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+)

-- 
2.5.0

[PATCH V7 1/2] ASoC: core: add API for registering and cleaning up DMI card names

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Add core API for registering and cleaning up DMI card names, so user
space utils such as PA and UCM can distinguish various products.
Previously on ASoC, the card short name, driver name and long name were
all the same as the machine driver name.
The patch adds more board information:
  card driver name     --->  machine driver name
  card short name      --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
  card long name and
  card component       --->  short name;driver name;(DMI_SYS_VENDOR,
                             optional);(the firmware name, optional)

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 02b4a21..911d09e 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream);
 int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 	unsigned int dai_fmt);
 
+int snd_soc_set_card_names(struct snd_soc_card *card, const char *firmware);
+void snd_soc_cleanup_card_names(struct snd_soc_card *card);
+
 /* Utility functions to get clock rates from various things */
 int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
 int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index d2e62b15..c1f3520 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -34,6 +34,7 @@
 #include <linux/ctype.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/dmi.h>
 #include <sound/core.h>
 #include <sound/jack.h>
 #include <sound/pcm.h>
@@ -1828,6 +1829,90 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 }
 EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
 
+/**
+ * snd_soc_set_card_names() - Register DMI names to card
+ * @card: The card to register DMI names
+ * @firmware: The firmware name, optional
+ *
+ * This function registers DMI names to card for the userspace to distinguish
+ * different boards/products:
+ *   card driver name     --->  machine driver name
+ *   card short name      --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
+ *   card long name and
+ *   card component       --->  short name;driver name;(DMI_SYS_VENDOR,
+ *                              optional);(firmware name, optional)
+ *
+ * Returns 0 on success, otherwise a negative error code.
+ */
+int snd_soc_set_card_names(struct snd_soc_card *card, const char *firmware)
+{
+	int ret = 0;
+	size_t buf_size, name_size;
+	const char *board, *vendor;
+	char *longname;
+
+	board = dmi_get_system_info(DMI_BOARD_NAME);
+	if (!board)
+		board = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (!board) {
+		dev_err(card->dev, "ASoC: the board/product name is empty!\n");
+		return -EINVAL;
+	}
+
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+
+	/* card driver name */
+	card->driver_name = card->name;
+
+	/* card short name */
+	card->name = board;
+
+	/* card long name */
+	buf_size = sizeof(card->snd_card->longname);
+	name_size = strlen(card->name) + strlen(card->driver_name) + 4;
+	if (vendor)
+		name_size += strlen(vendor);
+	if (firmware)
+		name_size += strlen(firmware);
+	if (name_size > buf_size)
+		return -ENOMEM;
+
+	longname = kmalloc(buf_size, GFP_KERNEL);
+	if (!longname)
+		return -ENOMEM;
+	snprintf(longname, buf_size, "%s;%s;",
+			card->name, card->driver_name);
+	if (vendor)
+		strlcat(longname, vendor, buf_size);
+	strlcat(longname, ";", buf_size);
+	if (firmware)
+		strlcat(longname, firmware, buf_size);
+
+	card->long_name = longname;
+
+	/* card component */
+	if (sizeof(card->snd_card->components) < name_size
+			+ strlen(card->snd_card->components))
+		return -ENOMEM;
+
+	ret = snd_component_add(card->snd_card, card->long_name);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
+
+/**
+ * snd_soc_cleanup_card_names() - cleanup registered DMI names
+ * @card: The card to cleanup
+ *
+ * This function cleanup the registered DMI names from card
+ */
+void snd_soc_cleanup_card_names(struct snd_soc_card *card)
+{
+	kfree(card->long_name);
+}
+EXPORT_SYMBOL_GPL(snd_soc_cleanup_card_names);
+
 static int snd_soc_instantiate_card(struct snd_soc_card *card)
 {
 	struct snd_soc_codec *codec;
-- 
2.5.0

[PATCH V7 2/2] ASoC: bytcr-rt5640: register DMI names for card

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Register DMI names for products using bytcr-rt5640 driver, such as ASUS
T100TA and Dell Venue 8 Pro 5830, for user space utils to distinguish
different products.

For example, previously on T100TA:
  $ cat /proc/asound/cards
    0 [bytcrrt5640 ]: bytcr-rt5640 -  bytcr-rt5640
                      bytcr-rt5640
  $ amixer -c0 info
  Card hw:0 'bytcrrt5640'/'bytcr-rt5640'
    Mixer name    : ''
    Components    : ''
    Controls      : 256
    Simple ctrls  : 228

After apply the patch:
  $ cat /proc/asound/cards
    0 [T100TA      ]: bytcr-rt5640 -  T100TA
                      T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_
  sst_0f28.bin
  $ amixer -c0 info
  Card hw:0 'T100TA'/'T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_
  sst_0f28.bin'
    Mixer name    : ''
    Components    : 'T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_s
  st_0f28.bin'
    Controls      : 256
    Simple ctrls  : 228

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 032a2e7..014b549 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -152,6 +152,8 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = {
 	{}
 };
 
+static struct snd_soc_card byt_rt5640_card;
+
 static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
@@ -159,6 +161,13 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 	struct snd_soc_card *card = runtime->card;
 	const struct snd_soc_dapm_route *custom_map;
 	int num_routes;
+	struct sst_acpi_mach *mach = byt_rt5640_card.dev->platform_data;
+
+	ret = snd_soc_set_card_names(card, mach->fw_filename);
+	if (ret < 0) {
+		dev_err(card->dev, "unable to register card names\n");
+		return ret;
+	}
 
 	card->dapm.idle_bias_off = true;
 
@@ -380,12 +389,21 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	return ret_val;
 }
 
+static int snd_byt_rt5640_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = &byt_rt5640_card;
+
+	snd_soc_cleanup_card_names(card);
+	return 0;
+}
+
 static struct platform_driver snd_byt_rt5640_mc_driver = {
 	.driver = {
 		.name = "bytcr_rt5640",
 		.pm = &snd_soc_pm_ops,
 	},
 	.probe = snd_byt_rt5640_mc_probe,
+	.remove = snd_byt_rt5640_mc_remove,
 };
 
 module_platform_driver(snd_byt_rt5640_mc_driver);
-- 
2.5.0

Re: [PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[Takashi Iwai]

On Wed, 06 Apr 2016 09:29:17 +0200,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
>  Share more product information, for user space utils such as PA and UCM to
>  distinguish different products.
>   1. Add core APIs to register and cleanup DMI names for card.
>   2. Apply the APIs to bytcr-rt5640 driver.

Han, one good advice is: be patient.  This is no urgent fix, so there
is no many reason to rush too much with a pile of newer patchset.
Give some more time for other people to review.

About the patch: I still have a few concerns, and some are in the
fundamental level:

- Is calling dmi_*() function in ASoC core is appropriate and
  preferred?

- When is this function supposed to be called?  Since you're accessing
  card->snd_card, it must be after instantiation, that is,
  snd_soc_register_card().  If so, it should be documented.  And, in
  that case, there is no need to allocate a buffer; you can set the 
  strings directly in card->snd_card. (For snd_component_add(), you
  may pass the card->snd_card->longname[]).

  OTOH, if the function may be called before instantiation, the code
  needs more rethink, including the string allocation and release.

- A semicolon is no taboo character, either.  A firmware or vendor
  string may contain such a letter, too.  You need to either escape or
  replace such a letter instead of praying the well-mannered
  firmware.


thanks,

Takashi

> 
>  changes on V7:
>   1. Remove inconsistent API description
>   
>  changes on V6:
>   1. Use dynamic allocate and cleanup for card long name
>   2. Remove unneccessary arguments to simplify the API
>   
>  changes on V5:
>   1. Use independent space to store card long_name, to avoid irrelavant
>   info sharing from card component
>   2. Use letter ';' instead of ':' to separate strings in long name, in
>   case name strings may also contain ':' and confuse user
>   3. Fix error that vendor name and firmware name were not optional 
>    
>  changes on V4:
>   1. Replace kmalloc() and snprintf() with ksaprintf() to simplify code
>     
>  changes on V3:
>   1. Split the core API and the API call to two patches
>   2. Replace misused strcat() with snprintf()
>   3. Code and comment fix 
> 
> Lu, Han (2):
>   ASoC: core: add API for registering and cleaning up DMI card names
>   ASoC: bytcr-rt5640: register DMI names for card
> 
>  include/sound/soc.h                   |  3 ++
>  sound/soc/intel/boards/bytcr_rt5640.c | 18 ++++++++
>  sound/soc/soc-core.c                  | 85 +++++++++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+)
> 
> -- 
> 2.5.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH V7 1/2] ASoC: core: add API for registering and cleaning up DMI card names

[Takashi Sakamoto]

Hi,

On Apr 6 2016 16:29, han.lu@intel.com wrote:
> From: "Lu, Han" <han.lu@intel.com>
> 
> Add core API for registering and cleaning up DMI card names, so user
> space utils such as PA and UCM can distinguish various products.
> Previously on ASoC, the card short name, driver name and long name were
> all the same as the machine driver name.
> The patch adds more board information:
>   card driver name     --->  machine driver name
>   card short name      --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>   card long name and
>   card component       --->  short name;driver name;(DMI_SYS_VENDOR,
>                              optional);(the firmware name, optional)
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 02b4a21..911d09e 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream);
>  int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>  	unsigned int dai_fmt);
>  
> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *firmware);
> +void snd_soc_cleanup_card_names(struct snd_soc_card *card);
> +
>  /* Utility functions to get clock rates from various things */
>  int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
>  int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index d2e62b15..c1f3520 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> (omit)
> +/**
> + * snd_soc_cleanup_card_names() - cleanup registered DMI names
> + * @card: The card to cleanup
> + *
> + * This function cleanup the registered DMI names from card
> + */
> +void snd_soc_cleanup_card_names(struct snd_soc_card *card)
> +{
> +	kfree(card->long_name);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_cleanup_card_names);
> +

In linux kernel development, such a function with a little statements
tends to be 'static inline function' defined in a header, instead of
maintaining more symbols. I think some APIs of Linux workqueue are good
examples to you. See 'include/linux/workqueue.h'.


Regards

Takashi Sakamoto

Re: [PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 380 bytes --]

On Wed, Apr 06, 2016 at 10:53:09AM +0200, Takashi Iwai wrote:

> Han, one good advice is: be patient.  This is no urgent fix, so there
> is no many reason to rush too much with a pile of newer patchset.
> Give some more time for other people to review.

FWIW given that every one of the many versions of this series has got a
bunch of review comments I've not looked at it yet...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[Han Lu]

On 04/06/2016 04:53 PM, Takashi Iwai wrote:
> On Wed, 06 Apr 2016 09:29:17 +0200,
> han.lu@intel.com wrote:
>> From: "Lu, Han" <han.lu@intel.com>
>>
>>   Share more product information, for user space utils such as PA and UCM to
>>   distinguish different products.
>>    1. Add core APIs to register and cleanup DMI names for card.
>>    2. Apply the APIs to bytcr-rt5640 driver.
> Han, one good advice is: be patient.  This is no urgent fix, so there
> is no many reason to rush too much with a pile of newer patchset.
> Give some more time for other people to review.

sorry, I was too hurry. I'll update patch after more review.
>
> About the patch: I still have a few concerns, and some are in the
> fundamental level:
>
> - Is calling dmi_*() function in ASoC core is appropriate and
>    preferred?

I was intended to add a common function for each machine driver to call, and
to add firmware name if necessary. I can move all dmi_*() functions to 
machine
drivers too, only pass the name strings as arguments.
>
> - When is this function supposed to be called?  Since you're accessing
>    card->snd_card, it must be after instantiation, that is,
>    snd_soc_register_card().  If so, it should be documented.  And, in
>    that case, there is no need to allocate a buffer; you can set the
>    strings directly in card->snd_card. (For snd_component_add(), you
>    may pass the card->snd_card->longname[]).
>
>    OTOH, if the function may be called before instantiation, the code
>    needs more rethink, including the string allocation and release.
the intention was to set the card->long_name, before the accessing to it in
snd_soc_instantiate_card():
     ...
     snprintf(card->snd_card->longname, sizeof(card->snd_card->longname),
          "%s", card->long_name ? card->long_name : card->name);
     ...
or it may be better to overwrite the card->snd_card->longname[] after
instantiation, so no need to allocate and release at all?
>
> - A semicolon is no taboo character, either.  A firmware or vendor
>    string may contain such a letter, too.  You need to either escape or
>    replace such a letter instead of praying the well-mannered
>    firmware.
so any printable ASCII character may have the same risk, may I use a control
code such as '\t', or a non ASCII value like 0x80?

BR,
Han
>
> thanks,
>
> Takashi
>
>>   changes on V7:
>>    1. Remove inconsistent API description
>>    
>>   changes on V6:
>>    1. Use dynamic allocate and cleanup for card long name
>>    2. Remove unneccessary arguments to simplify the API
>>    
>>   changes on V5:
>>    1. Use independent space to store card long_name, to avoid irrelavant
>>    info sharing from card component
>>    2. Use letter ';' instead of ':' to separate strings in long name, in
>>    case name strings may also contain ':' and confuse user
>>    3. Fix error that vendor name and firmware name were not optional
>>     
>>   changes on V4:
>>    1. Replace kmalloc() and snprintf() with ksaprintf() to simplify code
>>      
>>   changes on V3:
>>    1. Split the core API and the API call to two patches
>>    2. Replace misused strcat() with snprintf()
>>    3. Code and comment fix
>>
>> Lu, Han (2):
>>    ASoC: core: add API for registering and cleaning up DMI card names
>>    ASoC: bytcr-rt5640: register DMI names for card
>>
>>   include/sound/soc.h                   |  3 ++
>>   sound/soc/intel/boards/bytcr_rt5640.c | 18 ++++++++
>>   sound/soc/soc-core.c                  | 85 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 106 insertions(+)
>>
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH V7 1/2] ASoC: core: add API for registering and cleaning up DMI card names

[Han Lu]

On 04/06/2016 05:48 PM, Takashi Sakamoto wrote:
> Hi,
>
> On Apr 6 2016 16:29, han.lu@intel.com wrote:
>> From: "Lu, Han" <han.lu@intel.com>
>>
>> Add core API for registering and cleaning up DMI card names, so user
>> space utils such as PA and UCM can distinguish various products.
>> Previously on ASoC, the card short name, driver name and long name were
>> all the same as the machine driver name.
>> The patch adds more board information:
>>    card driver name     --->  machine driver name
>>    card short name      --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>    card long name and
>>    card component       --->  short name;driver name;(DMI_SYS_VENDOR,
>>                               optional);(the firmware name, optional)
>>
>> Signed-off-by: Lu, Han <han.lu@intel.com>
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index 02b4a21..911d09e 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream);
>>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>   	unsigned int dai_fmt);
>>   
>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *firmware);
>> +void snd_soc_cleanup_card_names(struct snd_soc_card *card);
>> +
>>   /* Utility functions to get clock rates from various things */
>>   int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
>>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index d2e62b15..c1f3520 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> (omit)
>> +/**
>> + * snd_soc_cleanup_card_names() - cleanup registered DMI names
>> + * @card: The card to cleanup
>> + *
>> + * This function cleanup the registered DMI names from card
>> + */
>> +void snd_soc_cleanup_card_names(struct snd_soc_card *card)
>> +{
>> +	kfree(card->long_name);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_cleanup_card_names);
>> +
> In linux kernel development, such a function with a little statements
> tends to be 'static inline function' defined in a header, instead of
> maintaining more symbols. I think some APIs of Linux workqueue are good
> examples to you. See 'include/linux/workqueue.h'.

Thanks. Since we need to pass the string (firmware name) from machine
driver to core anyway, I guess one API is a must, but the cleanup API may
not necessary if there's no allocation. I'll rethink of it.

BR,
Han
>
>
> Regards
>
> Takashi Sakamoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH V7 0/2] ASoC: Add core API to register and cleanup DMI names for card

[Han Lu]

On 04/07/2016 01:04 AM, Mark Brown wrote:
> On Wed, Apr 06, 2016 at 10:53:09AM +0200, Takashi Iwai wrote:
>
>> Han, one good advice is: be patient.  This is no urgent fix, so there
>> is no many reason to rush too much with a pile of newer patchset.
>> Give some more time for other people to review.
> FWIW given that every one of the many versions of this series has got a
> bunch of review comments I've not looked at it yet...
>
Sorry, I'll slow down with more thinking.

BR,
Han
>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel