Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
> Add attribute and show the components string. This is useful to see
> what is provided to userspace and e.g. used by UCM to understand the
> card configuration:
> 
> root@plb:~# more /sys/class/sound/card0/components
> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
> 
> Note that the style uses what's recommended by checkpatch.pl and is
> slightly different from other sysfs attributes.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/core/init.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
 
I have a concern about this patch in a point of userspace interface.

The 'component' field of 'struct snd_ctl_card_info' is just defined to
have strings with space-separated chunks, and its actual value is not
so fine-documented, thus it's largely different depending of developers
of each driver.

$ cat include/uapi/sound/asound.h
     ...
 941 struct snd_ctl_card_info {
         ...
 950     unsigned char components[128];  /* card components / fine identification, delimited with one space (AC97 etc..) */
 951 };

On the other hand, the node of sysfs is quite common in Linux because
it's tightly coupled to kernel objects. Let you see files under
'Documentation/ABI/'. We can find efforts to maintain sysfs node so
safe and stable. Due to this reason, it's better to take more care when
adding new node.

Would I request you the reason to add this node and the reason that
current ALSA control interface doesn't satisfy your requirement?

> diff --git a/sound/core/init.c b/sound/core/init.c
> index b02a99766351..decaf944a8ad 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev,
>  
>  static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
>  
> +static ssize_t
> +components_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->components);
> +}
> +
> +static DEVICE_ATTR_RO(components);
> +
>  static struct attribute *card_dev_attrs[] = {
>  	&dev_attr_id.attr,
>  	&dev_attr_number.attr,
> +	&dev_attr_components.attr,
>  	NULL
>  };
>  
> -- 
> 2.20.1
 

Regards

Takashi Sakamoto