Re: [PATCH 2/2] ALSA sound driver for the AT73C213 DAC using Atmel SSC driver

[Takashi Iwai <tiwai@suse.de>]

At Mon, 16 Jul 2007 16:14:59 +0200,
Hans-Christian Egtvedt wrote:
> 
> --- /dev/null
> +++ b/include/linux/spi/at73c213.h
> @@ -0,0 +1,25 @@
> +/*
> + * Board-specific data used to set up AT73c213 audio DAC driver.
> + */
> +
> +#ifndef __LINUX_SPI_AT73C213_H
> +#define __LINUX_SPI_AT73C213_H
> +
> +/**
> + * at73c213_board_info - how the external DAC is wired to the device.
> + *
> + * @ssc_id: SSC platform_driver id the DAC shall use to stream the audio.
> + * @dac_clk: the external clock used to provide master clock to the DAC.
> + * @shortname: a short discription for the DAC, seen by userspace tools.
> + *
> + * This struct contains the configuration of the hardware connection to the
> + * external DAC. The DAC needs a master clock and a I2S audio stream. It also
> + * provides a name which is used to identify it in userspace tools.
> + */
> +struct at73c213_board_info {
> +	int		ssc_id;
> +	struct clk	*dac_clk;
> +	char		shortname[32];
> +};
> +
> +#endif /* __LINUX_SPI_AT73C213_H */

Any reason to put this into include/linux?  Are (or will be) there
users of this except for ALSA driver?

> diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
> new file mode 100644
> index 0000000..ea6126b
> --- /dev/null
> +++ b/sound/spi/at73c213.c
(snip)
> +#include <linux/kmod.h>

Unneeded.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +#include <sound/initval.h>
> +#include <sound/driver.h>

Please put sound/driver.h at first.  This will make it for us much
easier to maintain the out-of-kernel tree.

> +/*
> + * Calculate and set bitrate and divisions.
> + */
> +static int snd_at73c213_set_bitrate(struct snd_at73c213 *chip)
> +{
> +	unsigned long ssc_rate = clk_get_rate(chip->ssc->clk);
> +	unsigned long dac_rate_new, ssc_div, status;
> +	unsigned long ssc_div_max, ssc_div_min;
> +	int max_tries;
> +
> +	/*
> +	 * We connect two clocks here, picking divisors so the I2S clocks
> +	 * out data at the same rate the DAC clocks it in ... and as close
> +	 * as practical to the desired target rate.
> +	 *
> +	 * The DAC master clock (MCLK) is programmable, and is either 256
> +	 * or (not here) 384 times the I2S output clock (BCLK).
> +	 */
> +
> +	/* SSC clock / (bitrate * stereo * 16-bit). */
> +	ssc_div = ssc_rate / (BITRATE_TARGET * 2 * 16);
> +	ssc_div_min = ssc_rate / (BITRATE_MAX * 2 * 16);
> +	ssc_div_max = ssc_rate / (BITRATE_MIN * 2 * 16);
> +	max_tries = (ssc_div_max - ssc_div_min) / 2;
> +
> +	/* ssc_div must be a power of 2. */
> +	ssc_div = (ssc_div + 1) & ~1UL;
> +
> +	if ((ssc_rate / (ssc_div * 2 * 16)) < BITRATE_MIN) {
> +		ssc_div -= 2;
> +		if ((ssc_rate / (ssc_div * 2 * 16)) > BITRATE_MAX) {
> +			return -ENXIO;
> +		}

Remove braces for a single-if block.

> +	}
> +
> +	/* Search for a possible bitrate. */
> +	do {
> +		/* SSC clock / (ssc divider * 16-bit * stereo). */
> +		if ((ssc_rate / (ssc_div * 2 * 16)) < BITRATE_MIN)
> +			return -ENXIO;
> +
> +		/* 256 / (2 * 16) = 8 */
> +		dac_rate_new = 8 * (ssc_rate / ssc_div);
> +
> +		status = clk_round_rate(chip->board->dac_clk, dac_rate_new);
> +		if (status < 0)
> +			return status;
> +
> +		/* Ignore difference smaller than 256 Hz. */
> +		if ((status/256) == (dac_rate_new/256))
> +			goto set_rate;
> +
> +		ssc_div += 2;
> +	} while (--max_tries);

Can't be max_tries = 0 in theory?

> +static int snd_at73c213_pcm_trigger(struct snd_pcm_substream *substream,
> +				   int cmd)
> +{
> +	struct snd_at73c213 *chip = snd_pcm_substream_chip(substream);
> +	int retval = 0;
> +	unsigned long flags = 0;
> +
> +	spin_lock_irqsave(&chip->lock, flags);

The trigger callback is already spinlock+irqsaved.
So, spin_lock() / spin_unlock() should be enough.

> +	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &at73c213_playback_ops);
> +
> +	retval = snd_pcm_lib_preallocate_pages_for_all(chip->pcm,
> +			SNDRV_DMA_TYPE_DEV,
> +			snd_dma_continuous_data(GFP_KERNEL),
> +			64 * 1024, 64 * 1024);

For SNDRV_DMA_TYPE_DEV, pass the struct device pointer of the device
to the third argument.  Then the pre-allocator will allocate the
memory via dma_alloc_coherent() with the givn device.  If it's not
appropriate, SNDRV_DMA_TYPE_CONTINUOUS type.  Anyway,
snd_dma_continuous_data() is only for SNDRV_DMA_TYPE_CONTINUOUS.

> +/*
> + * Mixer functions.
> + */
> +static int snd_at73c213_mono_get(struct snd_kcontrol *kcontrol,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_at73c213 *chip = snd_kcontrol_chip(kcontrol);
> +	unsigned long flags;
> +	int reg = kcontrol->private_value & 0xff;
> +	int shift = (kcontrol->private_value >> 8) & 0xff;
> +	int mask = (kcontrol->private_value >> 16) & 0xff;
> +	int invert = (kcontrol->private_value >> 24) & 0xff;
> +
> +	spin_lock_irqsave(&chip->lock, flags);

The mixer (control) callbacks are all schedulable, so you need no
irqsave but spin_lock_irq() should be fine.

> +static int __devinit snd_at73c213_mixer(struct snd_at73c213 *chip)
> +{
> +	struct snd_card *card;
> +	int errval, idx;
> +
> +	if (chip == NULL || chip->pcm == NULL)
> +		return -EINVAL;
> +
> +	card = chip->card;
> +
> +	strcpy(card->mixername, chip->pcm->name);
> +
> +	for (idx = 0; idx < ARRAY_SIZE(snd_at73c213_controls); idx++) {
> +		if ((errval = snd_ctl_add(card,
> +				snd_ctl_new1(&snd_at73c213_controls[idx],
> +				chip))) < 0) {
> +			goto cleanup;
> +		}

Split errval = ; and if (errval < 0) ...
Also drop braces for a single if block.


thanks,

Takashi