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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
