[alsa-devel] [PATCH] Improved support for different bt87x board configurations

[Takashi Iwai]

At Thu, 30 Aug 2007 03:21:52 -0700 (PDT),
Trent Piepho wrote:
> 
> After applying this patch, the bt87x.patch file in alsa-driver doesn't apply.
> I'm not sure what I'm supposed to do about that?

Would be helpful, of course :)  But I can do it myself, too.  Don't
worry about that.

> 
> # HG changeset patch
> # User Trent Piepho <xyzzy at speakeasy.org>
> # Date 1188469025 25200
> # Node ID 33d453db23d246dade155a6fc3b91d8437a4b7f5
> # Parent  52dfc5244360d2b0b119786596962ff5d0c9f338
> snd-bt87x: Improve support for different board types
> 
> Different cards have different audio configurations, but the driver didn't
> support this.  The only setting it had was the digital rate.
> 
> This patch adds a board configuration list.  Currently, configurable items are
> the digital rate and the digital data format (for cards with an external ADC),
> a flag for the absence of an external ADC, and a flag for no connection to the
> Bt87x internal ADC.
> 
> This allows cards that don't use the internal ADC to omit the ALSA "Bt87x
> analog" device and related controls.  Cards without an external ADC can omit
> the "Bt87x digital" device.
> 
> In order to support the CS5331A ADC used on the Osprey 440 and 2x0 cards, the
> digital format needs to be different than the default.
> 
> Support could be added for defining:
>   The connections or lack of them to the Bt87x's internal ADC mux
>   Multiple sample rates for an external ADC (e.g. Osprey)
>   Control of an external mux for an external ADC (e.g. Osprey)
> 
> The card definitions for cards other than the Ospreys are kept equivalent to
> their old values.  This is likely inaccurate for most cards, as it is doubtful
> that both an external and the internal ADC would be used.  Lacking information
> on those cards, the behavior is left unchanged.
> 
> Signed-off-by: Trent Piepho <xyzzy at speakeasy.org>

The patch looks fine, but checkpatch.pl complains about some coding
style issuse.

One thin I don't like so much is:

> +#define BT_DEVICE(chip, subvend, subdev, id) \
>  	{ .vendor = PCI_VENDOR_ID_BROOKTREE, \
> -	  .device = chip, \
> +	  .device = PCI_DEVICE_ID_BROOKTREE ## chip, \
>  	  .subvendor = subvend, .subdevice = subdev, \
> -	  .driver_data = rate }
> -
> -/* driver_data is the default digital_rate value for that device */
> +	  .driver_data = id }
> +/* driver_data is the card id for that device */
> +
>  static struct pci_device_id snd_bt87x_ids[] = {
>  	/* Hauppauge WinTV series */
> -	BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
> +	BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878),
>  	/* Hauppauge WinTV series */

The word _878 looks strange.  PCI_DEVICE_ID_BROOKTREE_878 is more
obvious that it's a macro and the readability isn't so bad.
I'd say, let it be.


Thanks!

Takashi