[alsa-devel] [PATCH 1/3] saa7146: Emagic Audiowerk8 low-level ALSA driver
Takashi Iwai
tiwai at suse.de
Thu Oct 23 11:30:10 CEST 2008
At Thu, 23 Oct 2008 00:04:50 +0200,
Matthias Nyffenegger wrote:
>
> From: Matthias Nyffenegger <matthias.nyffenegger at bluewin.ch>
>
> Low-level ALSA driver for Emagic Audiowerk8 sound card.
> Project page: http://sourceforge.net/projects/aw8-alsa
Thanks for the patch.
> Built and tested with Vanilla 2.6.25.16
Any chance to test, at least, on 2.6.27?
Some brief review comments below.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/log.h
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/log.h
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/log.h 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/log.h 2008-10-22 23:34:05.000000000 +0200
(snip)
> +#ifdef LOG_ENABLE
> +# define LOG_ERROR(fmt, args...) \
> + printk(KERN_ERR SAA7146_SUBSYS_LOG_TAG ": %s:%d:" fmt "\n", \
> + __func__, __LINE__, ## args);
> +# define LOG_WARN(fmt, args...) \
> + printk(KERN_WARNING SAA7146_SUBSYS_LOG_TAG ": %s:%d:" fmt "\n", \
> + __func__, __LINE__, ## args);
> +# define LOG_INFO(fmt, args...) \
> + printk(KERN_INFO SAA7146_SUBSYS_LOG_TAG ": %s:%d:" fmt "\n", \
> + __func__, __LINE__, ## args);
> +#else
> +# define LOG_ERROR(fmt, args...)
> +# define LOG_WARN(fmt, args...)
> +# define LOG_INFO(fmt, args...)
> +#endif /* LOG_ENABLE */
> +
> +#endif /*LOG_H_*/
Use the standard log function instead of a home-made one.
There are different functions, such as pr_err(), and
the one tied with the device such as dev_err().
The latter one is preferred nowadays if possible.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146audio.c
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146audio.c
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146audio.c 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146audio.c 2008-10-22 23:34:05.000000000 +0200
> +/**
> + * Address maps for Audio DMA Control PCI_ADP, BaseAx_x, ProtAx_x, PageAx_x and
> + * MC1 Audio Transfer Enable: 1st dim. index corresponds to
> + * enum audio_interfaces's, 2nd to enum directions's.
> + * TODO: decouple from saa7146i2s.h by providing a function that makes a
> + * reliable mapping (independent of int values of enum members).
> + */
Don't use "/**" unless you give kernel-doc style comments.
> +static const int Base[2][2] = {{BaseA1_in, BaseA1_out},
> + {BaseA2_in, BaseA2_out} };
Remove the space before the closing brace.
> +static const int Prot[2][2] = {{ProtA1_in, ProtA1_out},
> + {ProtA2_in, ProtA2_out} };
> +static const int Page[2][2] = {{PageA1_in, PageA1_out},
> + {PageA2_in, PageA2_out} };
> +static const int TR_E[2][2] = {{TR_E_A1_IN, TR_E_A1_OUT},
> + {TR_E_A2_IN, TR_E_A2_OUT} };
> +static const int PCI_ADP[2][2] = {{PCI_ADP2, PCI_ADP1},
> + {PCI_ADP4, PCI_ADP3} };
Ditto.
> +/**
> + * see saa7146audio.h
> + */
> +struct audio_stream *saa7146_stream_prepare_capture(
saa7146 is used in other devices such as video.
Thus saa7146_ prefix may conflict. Please add snd_ prefix to be sure
for global (even unexported) functions and variables.
> +/**
> + * see saa7146audio.h
> + */
> +void saa7146_stream_unprepare(struct saa7146audio *chipaudio,
> + struct audio_stream *stream)
"unprepare" is no common word. Better to use "cleanup" or so.
> +{
> + int i = 0;
> + struct i2s_device *device = NULL;
No need to initialize.
> +
> + if (stream != NULL) {
In the kernel code, more common style is
if (stream) {
...
> +static struct audio_stream *stream_prepare(struct saa7146audio *chipaudio,
> + unsigned int stream_nr,
> + unsigned long buffer_base_addr,
> + int buffer_size,
> + int channel_count,
> + int sample_length,
> + enum endian endian,
> + enum directions direction)
> +{
> + struct audio_stream *stream = NULL;
> + int max_streams = (direction == in ?
> + MAX_IN_AUDIO_STREAMS : MAX_OUT_AUDIO_STREAMS);
Use unsigned int in this case. Or check negative value, too.
> + struct audio_stream *streams = (direction == in ?
> + chipaudio->in_streams : chipaudio->out_streams);
> +
> + if (stream_nr > max_streams || streams[stream_nr].state == enabled) {
> + LOG_WARN("Stream nr=%d not available", stream_nr);
> + return NULL;
> + }
> + stream = &streams[stream_nr];
> + if (stream_check_resources(chipaudio, stream_nr, channel_count,
> + sample_length, endian, direction) != 0)
> + return NULL;
> + if (stream_init(chipaudio, stream, stream_nr, buffer_base_addr,
> + buffer_size, channel_count, sample_length, endian, direction)
> + != 0)
> + return NULL;
> + if (stream_setup_dma(chipaudio, stream) != 0)
> + return NULL;
> + return stream;
> +}
> +
> +/**
> + * Check if the requested resources (channels, sample-length, endian) are
> + * available.
> + */
> +static int stream_check_resources(struct saa7146audio *chipaudio,
> + unsigned int stream_nr,
> + int channel_count,
> + int sample_length,
> + enum endian endian,
> + enum directions direction)
> +{
> + int i = 0;
> + int available_channels = 0;
> + int available_frame_length = 0;
> + struct i2s_device *device = NULL;
> + struct audio_interface *ai = NULL;
Unnecessary initializations (can be seen in every place)...
> +static int stream_setup_dma(struct saa7146audio *chipaudio,
> + struct audio_stream *stream)
> +{
> + int exp = 0;
> + int limit = 0;
> + enum audio_interfaces ai_nr = stream->audio_interface->id;
> + enum directions dir = stream->direction;
> + struct saa7146reg *chip = &chipaudio->chipi2s.chip;
> +
> + if (!is_valid_period(stream->buffer_size >> 1)) {
> + LOG_ERROR("Invalid buffer size %d", stream->buffer_size);
> + return -1;
> + }
> + /* find exponent for period, required to figure out the DMA IRQ limit */
> + limit = stream->buffer_size >> 1; /* period is buffer_size/2 */
> + for (exp = 0; limit > 0; limit >>= 1, exp++);
Put ";" in the next line.
But, this calculation can be replaced with ilog2() (+1) in
linux/log2.h.
> +/**
> + * Period size must be >= 64bytes <= 1Mb and a power of 2.
> + * This is a restriction imposed by the SAA7146 DMA capabilities: the period at
> + * which DMA IRQs are generated is 64*2^n where n={1-14}.
> + * @return 1 if the given period is valid.
> + */
> +static int is_valid_period(int period)
> +{
> + return (period >= 64) && (period <= (1 << 20)) && is_pow_of_2(period);
Don't use magic numbers here.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146audio.h
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146audio.h
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146audio.h 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146audio.h 2008-10-22 23:34:05.000000000 +0200
(snip)
> +/**
> + * TODO: description
> + * @param chipaudio used for SAA7146 I2S ctrl and register access over PCI bus,
> + * must not be NULL.
> + * @return A reference to an audio_stream structure, or NULL in case the
> + * stream could not be opened.
> + */
Put the function description around the definition (i.e. *.c), not
around the declaration in the header.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146.h
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146.h
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146.h 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146.h 2008-10-22 23:34:05.000000000 +0200
> +/**
> + * Read an SAA7146 register, mind that PCI is little-endian.
> + * @param chip used for SAA7146 register access over PCI bus
> + * @param offset register address offset
> + * @return contents of register at the given address offset
> + */
> +static inline uint32_t saa7146_read(struct saa7146reg *chip, int offset)
> +{
> + return __le32_to_cpu(readl(chip->iobase_virt + offset));
No need to change the endian here. readl() does it already.
> +}
> +
> +/**
> + * Write an SAA7146 register, mind that PCI is little-endian.
> + * @param chip used for SAA7146 register access over PCI bus
> + * @param offset register address offset
> + * @param data the data to be written at the given address offset
> + */
> +static inline void saa7146_write(struct saa7146reg *chip,
> + int offset,
> + uint32_t data)
> +{
> + writel(__cpu_to_le32(data), chip->iobase_virt + offset);
Ditto.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2c.c
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2c.c
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2c.c 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2c.c 2008-10-22 23:34:05.000000000 +0200
(snip)
> +/* Convenience macros */
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
Use the standard one.
> +
> +/**
> + * See SAA7146 tech. specification p.122 - 126 for details.
> + */
> +
> +#define IICSTA_ABORT 0x80
> +#define IICSTA_ERR_BSY 0x7f
> +#define MC1_EI2C 0x01000000
> +#define MC2_UPLD_IIC 0x00010000
> +#define START 3
> +#define CONT 2
> +#define STOP 1
> +#define NOP 0
> +#define RW 1
I'm afraid these names are too simple and can be easily misused...
> +static int i2c_open(struct saa7146reg *chip,
> + enum saa7146_i2c_clock clk,
> + int read,
> + uint8_t dev_addr,
> + uint8_t *sub_addr,
> + unsigned int sub_addr_size)
> +{
> + int sub_addr_bytes_transmitted = 0;
> + uint8_t addr = 0;
> +
> + i2c_prepare(chip, clk);
> + addr = (dev_addr<<1)&~RW;
Fix spaces.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2s.c
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2s.c
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2s.c 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2s.c 2008-10-22 23:34:05.000000000 +0200
(snip0
> +/* Convenience macros */
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
Use standard macros.
> diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2s.h
> ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2s.h
> --- ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/saa7146i2s.h 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/saa7146i2s.h 2008-10-22 23:34:05.000000000 +0200
(snip)
> +/**
> + * Enumeration of SAA7146 audio-interfaces A1 and A2.
> + * Order matters (see head of saa7146i2s.c: address and bit offset maps)
> + */
> +enum audio_interfaces {a1, a2};
We are in the world of C where enum is always global.
So please use more distinctive words. "a1" and "a2" look very like
variable names.
> +/**
> + * Enumeration of SAA7146 i2c WS lines.
> + * Order matters (see head of saa7146i2s.c: address and bit offset maps)
> + */
> +enum ws_lines {ws0, ws1, ws2, ws3, ws4};
Ditto.
> +/**
> + * Enumeration of SAA7146 i2c SD lines.
> + * sd0_i_a2: SD0 is always input on SAA7146 audiointerface A2
> + * sd0_o_a1: SD0 is always output on SAA7146 audiointerface A1
> + * sd4_i_a1: SD4 is always input on SAA7146 audiointerface A1
> + * sd4_o_a2: SD4 is always output on SAA7146 audiointerface A2
> + * Order matters (see head of saa7146i2s.c: address and bit offset maps)
> + */
> +enum sd_lines {sd0_i_a2, sd0_o_a1, sd1_io_ax, sd2_io_ax, sd3_io_ax, sd4_i_a1,
> + sd4_o_a2};
Ditto.
> +enum endian {le, be};
Ditto.
(The endian notion would be better rather as a bool flag such as
is_big_endian.)
> +enum directions {in, out};
Ditto.
> +enum states {disabled, enabled};
Ditto. This should be really a bool.
> +enum hw_mon {source, destination, notmonitored};
Ditto.
> +/**
> + * @return 1 if the given value is a power of 2.
> + */
> +static inline int is_pow_of_2(int val)
> +{
> + return ((val != 0) && ((val & (val - 1)) == 0));
> +}
There is already is_power_of_2() in linux/log2.h.
thanks,
Takashi
More information about the Alsa-devel
mailing list