[oss-devel] Xonar DX patch (experimental)

[Clemens Ladisch]

Mario Goebbels wrote:
> I've cleaned up the little hacking I've did here and there this week.
> The patch works against build 1015 (CDDL). Comments about code quality
> would be nice, since I'm jumping into cold water here in regards to
> writing C.

> +#define ASUS_VENDOR_ID	0x1043 // Is it?

It is.

> +/* Xonar specific */
> +#define XONAR_DX_FRONTDAC	0x9e
> +#define XONAR_DX_GPIO_OUTPUT	0x01
> ...
> +/* Xonar DX */
> +#define XONAR_DX_OUTPUT	   0x0001
> +#define XONAR_ADDR_FRONT   0x9e

?

> +  /* Card model */
> +  int card_model;

I think you can use the model field for this purpose.

> +  /* Wait for it to stop being busy */
> +  while((INW(devc->osdev, TWO_WIRE_CTRL) & 0x1) && (count > 0))
> +  ...
>    MUTEX_ENTER_IRQDISABLE (devc->low_mutex, flags);

Waiting outside the mutex means that this functions isn't reentrant.

> +  oss_udelay(100);
> +
>    MUTEX_EXIT_IRQRESTORE (devc->low_mutex, flags);

Why is this wait inside the mutex?

I think it would be a better idea to wrap the entire function in a mutex
that doesn't disable interupts.

> +  // Left justified PCM (DAC and 8788 support I2S, but doesn't work).

How does it not work?

> +    /* SPI communication */
> +    bVal |= 0x80;

SPI is actually enabled by clearing bit 6.  Bit 7 enables SPI outputs 4
and 5.

> +      /* Must set master clock. */
> +      sDac |= 0x0010;

Please mention "256".

> +  /* Enable Xonar output */
> +  unsigned short output_enable;
> +  switch(devc->card_model)
> +  {
> +    case SUBID_XONAR_DX:
> +      output_enable = XONAR_DX_OUTPUT;
> +      break;
> +  }
> +  OUTW(devc->osdev, output_enable, GPIO_CONTROL);
> +  OUTW(devc->osdev, output_enable, GPIO_DATA);

output_enable is not initialized when the model is not a Xonar DX.


Regards,
Clemens