Re: [cdi-devel] [PATCH] CDI.audio - without "#define TYNDUR"

[Kevin Wolf <kevin@xxxxxxxxxx>]

Matt, can you have a quick look at it, too? If you still haven't got
around to learning German, ignore the comments for now - there is noone
who actually cares about specifications anyway. ;-)

On Mon, Oct 05, 2009 at 08:07:58PM +0200, max@xxxxxxxxxx wrote:
> From: Max Reitz <max@xxxxxxxxxx>
> 
> + A new approach on CDI.audio (aka CDI.sound). Up to now, it just
>   consists of a single C header describing the interface between
>   operating system and sound driver (this patch does not contain
>   a "#define TYNDUR" in "include/cdi.h").

Just a comment on writing patch descriptions for a changed patch: Things
like "now without #define TYNDUR" are meaningless in the git history.
The summary should describe what the patch is doing, not what previous
(not committed) patches did wrong.

Usually you would put [PATCH v2] into the subject so that it's clear that
this is meant to replace the first version. Things like what you changed
in v2 you best describe below the --- line, either directly before or
after the diffstat. This way we can read the explanation when reviewing
the patch, but it won't be included in the git commit comment.

> Signed-off-by: Max Reitz <max@xxxxxxxxxx>
> ---
>  include/cdi.h       |    1 +
>  include/cdi/audio.h |  147 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 148 insertions(+), 0 deletions(-)
>  create mode 100644 include/cdi/audio.h
> 
> diff --git a/include/cdi.h b/include/cdi.h
> index d803751..4f5385c 100644
> --- a/include/cdi.h
> +++ b/include/cdi.h
> @@ -25,6 +25,7 @@ typedef enum {
>      CDI_NETWORK         = 1,
>      CDI_STORAGE         = 2,
>      CDI_SCSI            = 3,
> +    CDI_AUDIO           = 4,
>  } cdi_device_type_t;
>  
>  struct cdi_driver;
> diff --git a/include/cdi/audio.h b/include/cdi/audio.h
> new file mode 100644
> index 0000000..bd1fb1a
> --- /dev/null
> +++ b/include/cdi/audio.h
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (c) 2009 Max Reitz
> + *
> + * This program is free software. It comes without any warranty, to
> + * the extent permitted by applicable law. You can redistribute it 
> + * and/or modify it under the terms of the Do What The Fuck You Want 
> + * To Public License, Version 2, as published by Sam Hocevar. See
> + * http://sam.zoy.org/projects/COPYING.WTFPL for more details.
> + */
> +
> +/**
> + * Treiber fuer Soundkarten
> + * \defgroup net
> + */
> +/*\@{*/
> +
> +#ifndef _CDI_AUDIO_H_
> +#define _CDI_AUDIO_H_
> +
> +#include <stddef.h>
> +
> +#include "cdi.h"
> +#include "cdi/list.h"
> +
> +//Moegliche Operation
> +typedef enum {
> +    //Abspielen
> +    CDI_AUDIO_PLAY = 0,
> +    //Aufnehmen
> +    CDI_AUDIO_RECORD,
> +
> +    CDI_AUDIO_NUM_ACTIONS
> +} cdi_audio_action_t;
> +
> +struct cdi_audio_device {
> +    struct cdi_device dev;
> +
> +    //Unterstuetzte Modi fuer eine bestimmte Operation. Ist dieser Wert NULL
> +    //oder die Liste ganz einfach leer, dann wird die Operation nicht
> +    //unterstuetzt.
> +    cdi_list_t        modes[CDI_AUDIO_NUM_ACTIONS];
> +};

This contains cdi_audio_modes, right? Maybe mention that in the comment.

> +
> +struct cdi_audio_mode {
> +    //Samplerate
> +    int sample_rate;

Which unit?

> +    //Anzahl der Channels (1 = Mono; 2 = Stereo; ...)
> +    int channels;
> +    //Bits pro Sample
> +    int bits_per_sample;
> +};
> +
> +struct cdi_audio_driver {
> +    struct cdi_driver drv;
> +
> +    /**
> +     * Aendert den Modus der Soundkarte fuer eine bestimmte Operation
> +     *
> +     * @param device Soundkarte
> +     * @param action Betreffende Operation
> +     * @param mode Gewuenschter Modus (muss in device->modes[action] enthalten
> +     *             sein!)
> +     *
> +     * @return 0, wenn der neue Modus dem angegebenen entspricht, sonst -1.
> +     */
> +    int (*change_mode)(struct cdi_audio_device* device,
> +        cdi_audio_action_t action, struct cdi_audio_mode* mode);
> +
> +    /**
> +     * Liefert den Modus der Soundkarte fuer eine bestimmte Operation
> +     *
> +     * @param device Soundkarte
> +     * @param action Betreffende Operation
> +     * @param mode Pointer zur Struktur, in der die Informationen zum Modus
> +     *             hinterlegt werden sollen
> +     *
> +     * @return 0, wenn die Informationen in *mode gespeichert wurden, sonst -1.
> +     */
> +    int (*get_mode)(struct cdi_audio_device* device, cdi_audio_action_t action,
> +        struct cdi_audio_mode* mode);
> +
> +    /**
> +     * Legt die Lautstaerke der angegebenen Operation fest
> +     *
> +     * @param device Soundkarte
> +     * @param action Betreffende Operation
> +     * @param volume Neue Lautstaerke (zwischen 0 und 255, wobei 255 am
> +     *               lautesten ist; bei -1 wird die Lautstaerke nicht
> +     *               veraendert)
> +     *
> +     * @return Die neue Lautstaerke (nuetzlich mit volume == -1)
> +     */
> +    int (*set_volume)(struct cdi_audio_device* device,
> +        cdi_audio_action_t action, int volume);

I think we'll want to control more than a single channel. I'd suggest
adding an channel parameter and introducing another function that allows
enumerating the channels the device supports.

> +
> +    /**
> +     * Fuehrt eine Operation aus
> +     *
> +     * @param device Soundkarte
> +     * @param action Auszufuehrende Operation
> +     * @param buffer Speicher, in dem die Operation durchgefuehrt werden soll
> +     * @param size Groesse des Speichers
> +     * @param flags Eventuelle Flags, die die Operation genauer festlegen
> +     *
> +     * @return 0 bei Erfolg, -1 im Fehlerfall.
> +     */
> +    int (*act)(struct cdi_audio_device* device, cdi_audio_action_t action,
> +        void* buffer, size_t size, int flags);

You define that -1 is returned in error case, but when is the function
supposed to fail? When the device is already playing/recording? How
would one detect that new data needs to be sent?