At Thu, 23 Oct 2008 00:04:50 +0200, Matthias Nyffenegger wrote:
From: Matthias Nyffenegger matthias.nyffenegger@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