At Tue, 19 Feb 2008 18:11:45 +0100,
Cédric Brégardis wrote:
>
> At Tuesday 19 February 2008 11:37:09, Takashi Iwai wrote:
> > At Tue, 19 Feb 2008 00:38:54 +0100,
> >
> > Cédric Brégardis wrote:
> > > Signed-off-by: Cedric Bregardis <cedric.bregardis@xxxxxxx>
> > >
> > > Signed-off-by: Jean-Christian Hassler <jhassler@xxxxxxx>
> >
> > Could you avoid HTML mail? I cannot get the patch properly.
> > If embedding text is difficult, rather use an attachment.
>
>
> Oh ! I am really sorry. I don't understand why my mailer has sent this mail in
> HTML... And as a side effect the mail has been refused by the list (too big).
>
> So, this time you will find the patch in attachment, as suggested.
Thanks. I've checked again right now, and found some remaining
issues.
> + /* check PCI availability (32bit DMA) */
> + if ((pci_set_dma_mask(pci, DMA_32BIT_MASK) < 0) ||
> + (pci_set_consistent_dma_mask(pci, DMA_32BIT_MASK) < 0)) {
> + printk(KERN_ERR "Impossible to set 32bit mask DMA\n");
The error message should have some prefix to indicate the driver
name. Otherwise you'll have no idea who wrote it.
> + if (chip->saa7146.iobase_virt == NULL) {
> + printk(KERN_ERR "unable to remap memory region");
Ditto.
> + if (request_irq(pci->irq, snd_aw2_saa7146_interrupt,
> + IRQF_SHARED, "Audiowerk2", (void *)chip)) {
The cast to void* isn't needed.
> + printk(KERN_ERR "Cannot grab irq %d\n", pci->irq);
Add prefix.
> +/* constructor */
> +static int __devinit snd_aw2_probe(struct pci_dev *pci,
> + const struct pci_device_id *pci_id)
(snip)
> + /* initialize semaphore */
> + init_MUTEX(&(chip->saa7146.sem));
Use struct mutex instead of semaphore. Also, the parenthesis around
'&' isn't needed.
> + /* init spinlock */
> + spin_lock_init(&chip->saa7146.reg_lock);
The spinlock is actually local to aw2-alsa.c, isn't it? Then it
should be outside saa7146.
> + /* initialize wait queue */
> + init_waitqueue_head(&chip->saa7146.wait_iic);
... and I'd suggest to the initializations of struct saa7146 to
aw2-saa7146.c. mutex should be initialized there, too.
> +/* create a pcm device */
> +static int __devinit snd_aw2_new_pcm(struct aw2 *chip)
(snip)
> + /* pre-allocation of buffers */
> + /* Preallocate continuous pages. */
> + err = snd_pcm_lib_preallocate_pages_for_all(pcm_playback_ana,
> + SNDRV_DMA_TYPE_DEV,
> + snd_dma_pci_data
> + (chip->pci),
> + 64 * 1024, 64 * 1024);
> + if (err) {
> + printk(KERN_ERR "snd_pcm_lib_preallocate_pages_for_all error "
> + "(0x%X)\n", err);
> + return err;
> + }
The error from preallocator isn't fatal. You can ignore it.
> +void snd_aw2_saa7146_pcm_trigger_start_playback(struct snd_aw2_saa7146 *chip,
> + int stream_number)
(snip)
> + } else if (stream_number == 1) {
> + WRITEREG((TR_E_A1_OUT << 16) | TR_E_A1_OUT, MC1);
> +
> + /* WS1_CTRL, WS1_SYNC: output TSL1, I2S */
> + acon1 |= 1 * WS1_CTRL;
> + WRITEREG(acon1, ACON1);
> + } else {
> + }
Remove the empty block.
> +void snd_aw2_saa7146_pcm_trigger_stop_playback(struct snd_aw2_saa7146 *chip,
> + int stream_number)
(snip)
> + } else if (stream_number == 1) {
> + /* WS1_CTRL, WS1_SYNC: output TSL1, I2S */
> + acon1 &= ~(3 * WS1_CTRL);
> + WRITEREG(acon1, ACON1);
> +
> + WRITEREG((TR_E_A1_OUT << 16), MC1);
> + } else {
> + }
Ditto.
> +void snd_aw2_saa7146_pcm_trigger_start_capture(struct snd_aw2_saa7146 *chip,
> + int stream_number)
> +{
> + /* In aw8 driver, dma transfert is always active. It is
> + started and stopped in a larger "space" */
> + if (stream_number == 0) {
> + WRITEREG((TR_E_A1_IN << 16) | TR_E_A1_IN, MC1);
> + } else {
> + }
Ditto.
> +void snd_aw2_saa7146_pcm_trigger_stop_capture(struct snd_aw2_saa7146 *chip,
> + int stream_number)
> +{
> + if (stream_number == 0) {
> + WRITEREG((TR_E_A1_IN << 16), MC1);
> + } else {
> + }
Ditto.
> +irqreturn_t snd_aw2_saa7146_interrupt(int irq, void *dev_id)
> +{
> + unsigned int isr;
> + unsigned int iicsta;
> + struct snd_aw2_saa7146 *chip = dev_id;
> +
> + isr = READREG(ISR);
> + if (!isr)
> + return IRQ_NONE;
> +
> + WRITEREG(isr, ISR);
> +
> + if (isr & (IIC_S | IIC_E)) {
> + iicsta = READREG(IICSTA);
> + WRITEREG(0x100, IICSTA);
> + wake_up(&chip->wait_iic);
> + } else if (isr) {
> + }
Ditto.
> +unsigned int snd_aw2_saa7146_get_hw_ptr_playback(struct snd_aw2_saa7146 *chip,
> + int stream_number,
> + unsigned char *start_addr,
> + unsigned int buffer_size)
(snip)
> + if (stream_number == 1) {
> + pci_adp = READREG(PCI_ADP1);
> + ptr = pci_adp - (size_t) start_addr;
> +
> + if (ptr == buffer_size)
> + ptr = 0;
> + } else {
> + }
Ditto.
> +int snd_aw2_saa7146_is_using_digital_input(struct snd_aw2_saa7146 *chip)
> +{
> + unsigned int reg_val = READREG(GPIO_CTRL);
> + if ((reg_val & 0xFF) == 0x40) {
> + return 1;
> + } else {
> + return 0;
> + }
Remove braces around the single if-statement.
Can be found in many other places.
(I thought this should be suggested by checkpatch.pl, but apparently
not perfectly detected...)
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
