At Thu, 23 Oct 2008 00:04:50 +0200, Matthias Nyffenegger wrote: > > From: Matthias Nyffenegger <matthias.nyffenegger@xxxxxxxxxx> > > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
