Ideas. Soundcard suport in HAL

[David Zeuthen]

Hi,

On Mon, 2005-02-28 at 19:01 +0000, Richard Hughes wrote:
>> On Sun, 27 Feb 2005 18:21:17 -0500, David Zeuthen wrote:
>>> I don't think we'll need a huge thread about this - just send the patch
>>> and we'll take it from there :-)
>
>--- classdev.c.orig	2005-02-28 18:49:44.000000000 +0000
>+++ classdev.c	2005-02-28 18:52:02.000000000 +0000
>@@ -438,6 +438,66 @@
> 	return TRUE;
> }
> 
>+static HalDevice *
>+sound_add (const gchar *sysfs_path, const gchar *device_file, HalDevice *physdev, const gchar *sysfs_path_in_devices)
>+{
>+	HalDevice *d;
>+	d = NULL;
>+	if (physdev == NULL || sysfs_path_in_devices == NULL || device_file == NULL) {
>+		goto out;
>+	}
>+
>+	d = hal_device_new ();
>+	hal_device_property_set_string (d, "linux.sysfs_path", sysfs_path);
>+	hal_device_property_set_string (d, "info.parent", physdev->udi);
>+	hal_device_property_set_string (d, "info.category", "sound");
>+	hal_device_add_capability (d, "alsa");

info.category and info.capabilities are pretty much the same with the
one difference that the former is the prominent one of the strings in
the latter; so you should use "alsa" both places.

>+	hal_device_property_set_string (d, "alsa.physical_device", sysfs_path);

No, this is wrong; it needs to be the UDI of the physical device so
s/sysfs_path/physdev.udi/

>+
>+	gchar *device;

All variable decl's need to be in the top of a compound statement or
function.

>+	device = (gchar*) hal_util_get_last_element(sysfs_path);

You really don't want to change this from const gchar *.

>+
>+	if (strlen (device) == 9 && strncmp (device, "control", 7) == 0) {
>+		hal_device_property_set_string (d, "alsa.stream", "control");
>+		hal_device_property_set_string (d, "info.product", "ALSA Control Device");
>+		hal_device_property_set_int (d, "alsa.card", atoi (device+8));
>+	}
>+
>+	if (strlen (device) == 8 && strncmp (device, "pcm", 3) == 0) {
>+		hal_device_property_set_string (d, "alsa.stream_type", "pcm");
>+		hal_device_property_set_int (d, "alsa.card", atoi (device+4));
>+		hal_device_property_set_int (d, "alsa.device", atoi (device+6));
>+		if (*(device+7) == 'p') {
>+			hal_device_property_set_string (d, "alsa.stream", "playback");
>+			hal_device_property_set_string (d, "info.product", "ALSA Playback Device");
>+		}
>+		else if (*(device+7) == 'c') {
>+			hal_device_property_set_string (d, "alsa.stream", "capture");
>+			hal_device_property_set_string (d, "info.product", "ALSA Capture Device");
>+		}
>+		else {
>+			hal_device_property_set_string (d, "alsa.stream", "other");
>+			hal_device_property_set_string (d, "info.product", "ALSA Device");
>+		}

This code looks a bit fragile; suggest to 

 if (sscanf (device, "controlC%d", &cardnum) == 1) {
   /* do control things */
 } else if (sscanf (device, "pcmC%dD%dc", &cardnum, &devicenum) == 2) {
   /* do capture things */
 } else if (sscanf (device, "pcmC%dD%dp", &cardnum, &devicenum) == 2) {
   /* do playback things */
 } else {
   g_object_unref (d);
   d = NULL;
   goto out;
 }

Also suggest to extract 'name field' from /proc/asound/card0/pcm0c/info
(but this can wait).

Also suggest to call it alsa.type rather than alsa.stream.

>+	}
>+out:
>+	return d;
>+}
>+
>+static gboolean
>+sound_compute_udi (HalDevice *d)
>+{
>+	if (d==NULL) exit(1);
>+	gchar udi[256];
>+	hal_util_compute_udi (hald_get_gdl (), udi, sizeof (udi),
>+			      "%s_logicaldev_sound",
>+			      hal_device_property_get_string (d, "info.parent"));

Suggest to use values of alsa.card and alsa.device and alsa.type here
otherwise we will get dups

>+	hal_device_set_udi (d, udi);
>+	hal_device_property_set_string (d, "info.udi", udi);
>+
>+	return TRUE;
>+}
>+
> /*--------------------------------------------------------------------------------------------------------------*/
> 
> static gboolean
>@@ -513,12 +573,23 @@
> 	.remove       = classdev_remove
> };
> 
>+static ClassDevHandler classdev_handler_sound = 
>+{ 
>+	.subsystem    = "sound",
>+	.add          = sound_add,
>+	.get_prober   = NULL,
>+	.post_probing = NULL,
>+	.compute_udi  = sound_compute_udi,
>+	.remove       = classdev_remove
>+};
>+
> static ClassDevHandler *classdev_handlers[] = {
> 	&classdev_handler_input,
> 	&classdev_handler_bluetooth,
> 	&classdev_handler_net,
> 	&classdev_handler_scsi_host,
> 	&classdev_handler_usbclass,
>+	&classdev_handler_sound,
> 	NULL
> };
> 
>Comments? Richard

As a general issue, please send patches as attachments, not inline, and
remember to also use the -p option for the 'cvs diff'. Also, you need to
write an ChangeLog entry and include that in the patch. You also want to
'cvs update' as I just committed some code - it shouldn't change
anything here though.

>p.s. screenshot at http://hughsie.no-ip.com/write/add-screenshot-hal-screenshot.png

Screenshots are nice :-)

Cheers,
David


_______________________________________________
hal mailing list
hal at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/hal