lkml.org 
[lkml]   [2006]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast
    A few quick comments.. though looking at the earlier thread, most of
    these were already pointed out..

    On Mon, Apr 17, 2006 at 01:13:04AM +0100, Adrian McMenamin wrote:
    > diff -ruN --exclude=acore --exclude='.#*' ./sh/aica.c /home/adrian/alsa-driver/sh/aica.c
    > --- ./sh/aica.c 1970-01-01 01:00:00.000000000 +0100
    > +++ /home/adrian/alsa-driver/sh/aica.c 2006-04-16 23:13:59.000000000 +0100

    -p1 format please.

    > +#include <linux/init.h>
    > +#include <linux/jiffies.h>
    > +#include <linux/slab.h>
    > +#include <linux/time.h>
    > +#include <linux/wait.h>
    > +#include <linux/moduleparam.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/firmware.h>
    > +#include <linux/timer.h>
    > +#include <linux/delay.h>
    > +#include <asm-sh/io.h>
    > +#include <asm-sh/dma.h>
    > +#include <asm-sh/dreamcast/sysasic.h>

    What's wrong with asm/? asm/ includes should also be last.

    > +
    > +

    This patch adds a lot of superfluous whitespace, please trim it.

    > +static char *id = NULL;

    Pointless initializer.

    > +
    > +/* Spinlocks */
    > +spinlock_t spu_memlock = SPIN_LOCK_UNLOCKED;
    > +spinlock_t spu_dmalock = SPIN_LOCK_UNLOCKED;
    > +
    DEFINE_SPINLOCK()..

    > +
    > +/* SPU specific functions */
    > +
    > +/* spu_write_wait - wait for G2-SH FIFO to clear */
    > +static inline void spu_write_wait(void)
    > +{
    > + int time_count;
    > + time_count = 0;
    > + while (1) {
    > + if (!(readl(G2_FIFO) & 0x11))
    > + break;
    > + /* To ensure hardware failure doesn't wedge kernel */
    > + time_count++;
    > + if (time_count > 0x10000)
    > + break;
    > + }
    > +
    > + return;
    > +}
    > +
    Useless return.

    > +
    > +/* spu_memset - write to memory in SPU address space */
    > +static void spu_memset(uint32_t toi, void __iomem * what, int length)
    > +{
    > + uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
    > + int i;
    > +
    > +
    > + if (length % 4)
    > + length = (length / 4) + 1;
    > + else
    > + length = length / 4;
    > + spu_write_wait();
    > + for (i = 0; i < length; i++) {
    > + spin_lock(&spu_memlock);
    > + writel(what, to);
    > + spin_unlock(&spu_memlock);
    > + to++;
    > + if (i && !(i % 8))
    > + spu_write_wait();
    > + }
    > + return;
    > +}
    > +
    Likewise.

    > +static void spu_memload(uint32_t toi, void __iomem * from, int length)
    > +{
    > + uint32_t __iomem *froml = from;
    > + uint32_t __iomem *to =
    > + (uint32_t __iomem *) (SPU_MEMORY_BASE + toi);
    > + int i, val;
    > + if (length % 4)
    > + length = (length / 4) + 1;
    > + else
    > + length = length / 4;
    > + spu_write_wait();
    > + for (i = 0; i < length; i++) {
    > + val = *froml;
    > + spin_lock(&spu_memlock);
    > + writel(val, to);
    > + spin_unlock(&spu_memlock);
    > + froml++;
    > + to++;
    > + if (i && !(i % 8))
    > + spu_write_wait();
    > + }
    > + return;
    > +
    > +}
    > +
    And again.

    > +/* Halt the sound processor,
    > + clear the memory,
    > + load some default ARM7 code,
    > + and then restart ARM7
    > +*/
    > +static int spu_init(void)
    > +{
    > +
    > + spu_disable();
    > + spu_memset(0, 0, 0x200000 / 4);
    > + *(uint32_t *) SPU_MEMORY_BASE = 0xea000002;

    Why are you not using ctrl_outl() or something similar? This should also
    be documented a bit better if it's not magic..

    > +/* aica_chn_start - write to spu to start playback */
    > +inline static void aica_chn_start(void)

    static inline void..

    > +/* aica_chn_halt - write to spu to halt playback */
    > +inline static void aica_chn_halt(void)
    > +{
    Likewise..

    > +/* Simple platform device */
    > +static struct platform_device *pd = NULL;
    > +
    > +
    > +
    Useless initializer and whitespace damage.

    > +static int stereo_buffer_transfer(struct snd_pcm_substream *substream,
    > + int buffer_size, int period)
    > +{
    > + int transferred;
    > + struct snd_pcm_runtime *runtime;
    > + int period_offset;
    > + period_offset = period;
    > + period_offset %= (AICA_PERIOD_NUMBER / 2);
    > + runtime = substream->runtime;
    > +
    > + /* transfer left and then right */
    > + spin_lock(&spu_dmalock);
    > + dma_xfer(0,
    > + runtime->dma_area + (AICA_PERIOD_SIZE * period_offset),
    > + AICA_CHANNEL0_OFFSET + (AICA_PERIOD_SIZE * period_offset),
    > + buffer_size / 2, 5);
    > + /* wait for completion */
    > + do {
    > + udelay(5);
    > + transferred = get_dma_residue(0);
    > + }
    > + while (transferred < buffer_size / 2);

    You can't be serious, you're trying to setup, transfer, and wait for
    completion for a DMA transfer while holding a spinlock? The fact this
    hasn't blown up on you is sheer luck. Use dma_wait_for_completion(), it
    does the right thing.

    Additionaly you need a timeout here if you were for some reason intent on
    doing this while working against the DMA subsystem, if your DMA gets
    stuck this will blow up.

    > +void static aica_period_elapsed(unsigned long timer_var)
    > +{

    static void..

    > + int transferred;
    > + int play_period;
    > + struct snd_pcm_runtime *runtime;
    > + struct snd_pcm_substream *substream;
    > + struct snd_card_aica *dreamcastcard;
    > + substream = (struct snd_pcm_substream *) timer_var;
    > + runtime = substream->runtime;
    > + dreamcastcard =
    > + (struct snd_card_aica *) (substream->pcm->private_data);

    Useless cast.

    > + } else {
    > + spin_lock(&spu_dmalock);
    > + dma_xfer(0,
    > + runtime->dma_area +
    > + (AICA_PERIOD_SIZE * dreamcastcard->clicks),
    > + AICA_CHANNEL0_OFFSET +
    > + (AICA_PERIOD_SIZE * dreamcastcard->clicks),
    > + AICA_PERIOD_SIZE, 5);
    > + do {
    > + /* Try to fine tune the delay to keep it as short as possible */
    > + udelay(5);
    > + /* get_dma_residue reports residue until completion when it reports total bytes transferred */
    > + transferred = get_dma_residue(0);
    > + }
    > + while (transferred < AICA_PERIOD_SIZE);
    > + spin_unlock(&spu_dmalock);
    > + }
    > +
    spinlock / DMA brain damage.

    > + snd_pcm_period_elapsed(dreamcastcard->substream);
    > + dreamcastcard->clicks++;
    > + dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
    > + /* reschedule the timer */
    > + dreamcastcard->timer.expires = jiffies + 1;
    > + add_timer(&(dreamcastcard->timer));
    > + return;
    > +}
    > +
    Superfluous return.

    > +static int snd_aicapcm_pcm_open(struct snd_pcm_substream *substream)
    > +{
    > + struct snd_pcm_runtime *runtime;
    > + struct aica_channel *channel;
    > + struct snd_card_aica *dreamcastcard;
    > +
    > + if (!enable)
    > + return -ENOENT;

    unlikely()?

    > + dreamcastcard =
    > + (struct snd_card_aica *) (substream->pcm->private_data);

    Useless cast.

    > +static int snd_aicapcm_pcm_close(struct snd_pcm_substream *substream)
    > +{
    > + struct snd_card_aica *dreamcastcard =
    > + (struct snd_card_aica *) (substream->pcm->private_data);
    Likewise.

    > +static int snd_aicapcm_pcm_prepare(struct snd_pcm_substream *substream)
    > +{
    > +
    > + struct snd_card_aica *dreamcastcard =
    > + (struct snd_card_aica *) (substream->pcm->private_data);

    And again.

    > +static void startup_aica(struct snd_card_aica *dreamcastcard)
    > +{
    > + spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
    > + (uint8_t *) dreamcastcard->channel,
    > + sizeof(struct aica_channel));
    > + aica_chn_start();
    > + return;
    > +}
    > +
    Useless return.

    > +
    > +static void spu_begin_dma(struct snd_pcm_substream *substream)
    > +{
    > + int buffer_size;
    > + snd_pcm_runtime_t *runtime;
    > + struct snd_card_aica *dreamcastcard =
    > + (struct snd_card_aica *) (substream->pcm->private_data);

    Superfluous cast.

    > + runtime = substream->runtime;
    > + buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
    > + if (runtime->channels == 1) {
    > + spin_lock(&spu_dmalock);
    > + dma_xfer(0, runtime->dma_area, AICA_CHANNEL0_OFFSET,
    > + buffer_size, 5);
    > + spin_unlock(&spu_dmalock);

    Broken locking.

    > +static int __init snd_aicapcmchip(struct snd_card_aica *dreamcastcard,
    > + int pcm_index)
    > +{
    > + struct snd_pcm *pcm;
    > + int err;
    > +
    > + /* Can we lock the memory */
    > +
    > + if (request_mem_region(ARM_RESET_REGISTER, 4, "AICA ARM control")
    > + == NULL)
    > + return -ENOMEM;
    > + if (request_mem_region(SPU_MEMORY_BASE, 0x200000, "AICA Sound RAM")
    > + == NULL) {
    > + release_mem_region(ARM_RESET_REGISTER, 0x4);
    > + return -ENOMEM;
    > + }
    > +
    Why aren't these setup in the platform device?

    > + /* AICA has no capture ability */
    > + if ((err =
    > + snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
    > + &pcm)) < 0)
    > + return err;

    Weird notation, linux kernel style would be:

    err = snc_pcm_new(...);
    if (unlikely(err < 0))
    return err;

    please refactor accordingly.

    > +static int aica_pcmvolume_put(struct snd_kcontrol *kcontrol,
    > + struct snd_ctl_elem_value *ucontrol)
    > +{
    > + struct snd_card_aica *dreamcastcard;
    > + dreamcastcard = (struct snd_card_aica *) kcontrol->private_value;
    > +
    > + if (!dreamcastcard->channel) {
    > + snd_printk("No channel yet\n");
    > + return -ETXTBSY; /* too soon */
    > + }
    > +
    > + else if (dreamcastcard->channel->vol ==
    > + ucontrol->value.integer.value[0])
    > + return 0;
    > +
    > + else {

    The if/elses are all over the place, please see
    Documentation/CodingStyle.

    > +static struct device_driver aica_driver = {
    > + .name = "AICA",
    > + .bus = &platform_bus_type,
    > + .remove = remove_dreamcastcard,
    > +};
    > +
    Deprecated, see struct platform_device.

    > + pd = platform_device_register_simple(dreamcastcard->card->driver,
    > + -1, NULL, 0);
    > +
    This is also going away, you'll want to update this for the new platform
    device semantics.

    > + freepcm:
    > + snd_aicapcm_free();
    > +
    > + freedreamcast:
    > + snd_card_free(dreamcastcard->card);
    > +
    Why are these goto labels in magical locations?

    > +static void __exit aica_exit(void)
    > +{
    > + struct device_driver *drv = (&pd->dev)->driver;
    > + device_release_driver(&pd->dev);
    > + driver_unregister(drv);
    > + platform_device_unregister(pd);
    > + /* Kill any sound still playing and reset ARM7 to safe state */
    > + spu_init();
    > +
    > +
    > + return;
    > +}
    > +
    And another useless return.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-04-17 03:32    [W:0.045 / U:0.656 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site