Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> This adds sound for the Yamaha AICA "Super Intelligent Sound
> Processor" (PCM) device on the SEGA Dreamcast
> 
> Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> 
I've had no comments back on this - I am thinking of committing to the
linux-sh cvs, though it really belongs in ALSA.

Any reason why I shouldn't?

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Lee Revell]

On Sat, 2006-06-03 at 12:39 +0100, Adrian McMenamin wrote:
> On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > Processor" (PCM) device on the SEGA Dreamcast
> > 
> > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > 
> I've had no comments back on this - I am thinking of committing to the
> linux-sh cvs, though it really belongs in ALSA.
> 
> Any reason why I shouldn't?
> 

Did you fix all of the issues that were raised by Paul and Takashi-san?

Lee

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Sat, 2006-06-03 at 11:16 -0400, Lee Revell wrote:
> On Sat, 2006-06-03 at 12:39 +0100, Adrian McMenamin wrote:
> > On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > > Processor" (PCM) device on the SEGA Dreamcast
> > > 
> > > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > > 
> > I've had no comments back on this - I am thinking of committing to the
> > linux-sh cvs, though it really belongs in ALSA.
> > 
> > Any reason why I shouldn't?
> > 
> 
> Did you fix all of the issues that were raised by Paul and Takashi-san?
> 
> Lee
Those ones that were fixable, yes. At least I think so :)

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Sat, 03 Jun 2006 12:39:48 +0100,
Adrian McMenamin wrote:
> 
> On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > Processor" (PCM) device on the SEGA Dreamcast
> > 
> > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > 
> I've had no comments back on this - I am thinking of committing to the
> linux-sh cvs, though it really belongs in ALSA.

Sorry for the delay.  I've had a long weekend.

> Any reason why I shouldn't?

As Paul already pointed, the platform_device things must be fixed.
Also, better to clean up the code directly accessing hardcoded
addresses.

Another big concern is that spu_dma_work is initialized/rewritten
dynamically in spu_begin_dma() and aica_period_elapsed() via
INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
racy.


Takashi

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:

> 
> As Paul already pointed, the platform_device things must be fixed.
> Also, better to clean up the code directly accessing hardcoded
> addresses.

Working on that, some new code in my personal CVS now - but I suspect it
will be the weekend before that gets fully fixed.


> 
> Another big concern is that spu_dma_work is initialized/rewritten
> dynamically in spu_begin_dma() and aica_period_elapsed() via
> INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> racy.

Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
queue but ask it to schedule the execution of two different (if very
similar) functions start_spu_dma() - which does the initial transfer and
more_spu_dma - which tops up the dma transfers.

So I think I've got that right.

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Wed, 07 Jun 2006 00:10:48 +0100,
Adrian McMenamin wrote:
> 
> On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> 
> > Another big concern is that spu_dma_work is initialized/rewritten
> > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > racy.
> 
> Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> queue but ask it to schedule the execution of two different (if very
> similar) functions start_spu_dma() - which does the initial transfer and
> more_spu_dma - which tops up the dma transfers.
> 
> So I think I've got that right.

What's wrong with using two individual work struct so that you
initialize them only once?
I wonder it because you already have unused fields work and work2...


Takashi

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Wed, 2006-06-07 at 11:51 +0200, Takashi Iwai wrote:
> At Wed, 07 Jun 2006 00:10:48 +0100,
> Adrian McMenamin wrote:
> > 
> > On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> > 
> > > Another big concern is that spu_dma_work is initialized/rewritten
> > > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > > racy.
> > 
> > Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> > queue but ask it to schedule the execution of two different (if very
> > similar) functions start_spu_dma() - which does the initial transfer and
> > more_spu_dma - which tops up the dma transfers.
> > 
> > So I think I've got that right.
> 
> What's wrong with using two individual work struct so that you
> initialize them only once?
> I wonder it because you already have unused fields work and work2...
> 
They've gone actually.

I need to initialise them because every call is a discrete processing
job - ie I send one thing to go, then when a period has elapsed I
schedule another transfer to run on the kernel thread.

Isn't this the way it is meant to work?

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Wed, 07 Jun 2006 19:16:42 +0100,
Adrian McMenamin wrote:
> 
> On Wed, 2006-06-07 at 11:51 +0200, Takashi Iwai wrote:
> > At Wed, 07 Jun 2006 00:10:48 +0100,
> > Adrian McMenamin wrote:
> > > 
> > > On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> > > 
> > > > Another big concern is that spu_dma_work is initialized/rewritten
> > > > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > > > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > > > racy.
> > > 
> > > Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> > > queue but ask it to schedule the execution of two different (if very
> > > similar) functions start_spu_dma() - which does the initial transfer and
> > > more_spu_dma - which tops up the dma transfers.
> > > 
> > > So I think I've got that right.
> > 
> > What's wrong with using two individual work struct so that you
> > initialize them only once?
> > I wonder it because you already have unused fields work and work2...
> > 
> They've gone actually.
> 
> I need to initialise them because every call is a discrete processing
> job - ie I send one thing to go, then when a period has elapsed I
> schedule another transfer to run on the kernel thread.
> 
> Isn't this the way it is meant to work?

It's uncoventional that a work struct is re-initialized on the fly.

IMO, it's better to introduce a flag indicating the stream is already
running and use a single function/work struct.  For example,

	static void do_first_event(struct stream *str)
	{
		...
		str->running = 1;
	}

	static void do_more_event(struct stream *str)
	{
		...
	}

	static void work_event(void *data)
	{
		struct stream *str = data;

		if (str->running)
			do_more_event(str);
		else
			do_first_event(str);
	}

	trigger_start()
	{
		queue_work(queue, &this_work);
	}

	interrupt_handler()
	{
		...
		queue_work(queue, &this_work);
		...
	}

	initialization()
	{
		...
		queue = create_workqueue("xxx");
		INIT_WORK(&this_work, work_event, str);
		...
	}	

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Thu, 2006-06-08 at 12:35 +0200, Takashi Iwai wrote:

> 
> It's uncoventional that a work struct is re-initialized on the fly.
> 
> IMO, it's better to introduce a flag indicating the stream is already
> running and use a single function/work struct.  For example,

Yes, I've did this sort of thing last night :)

Re: [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Paul Mundt]

[-- Attachment #1.1: Type: text/plain, Size: 7945 bytes --]

On Thu, Jun 01, 2006 at 11:31:11PM +0100, Adrian McMenamin wrote:
>    obj-y += last.o
> diff -ruN a/linux-2.6.16.14/sound/sh/aica.c b/linux-2.6.16.14/sound/sh/aica.c
> --- a/linux-2.6.16.14/sound/sh/aica.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/linux-2.6.16.14/sound/sh/aica.c	2006-06-01 23:01:39.000000000 +0100
> @@ -0,0 +1,692 @@
> +/*
> +* This code is licenced under 
> +* the General Public Licence
> +* version 2
> +*
> +* Copyright Adrian McMenamin 2005, 2006
> +* <adrian@mcmen.demon.co.uk>
> +* See also http://newgolddream.dyndns.info/cgi-bin/cvsweb
> +* 
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of version 2 of the GNU General Public License as published by
> +* the Free Software Foundation.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program; if not, write to the Free Software
> +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +*
> +*/
> +
> +
Superfluous whitespace still..

> +/* Simple platform device */
> +static struct platform_device *pd;
> +static struct resource aica_memory_space[2] = {
> +	{
> +	 	.name = "AICA ARM CONTROL",
> +	 	.start = ARM_RESET_REGISTER,
> +	 	.flags = IORESOURCE_MEM,
> +	 	.end = ARM_RESET_REGISTER + 3,
> +	 },
> +	{
> +	 	.name = "AICA Sound RAM",
> +	 	.start = SPU_MEMORY_BASE,
> +	 	.flags = IORESOURCE_MEM,
> +	 	.end = SPU_MEMORY_BASE + 0x200000 - 1,
> +	 },
> +};
> +
This is still a bit broken. These resources should be part of the
platform device defined by the board-specific dreamcast code (see for
example what things like microdev do for smc91x for an example), incase
there are other AICA users in the future.

> +/* 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;
> +	}
> +}
> +
> +/* spu_memset - write to memory in SPU address space */
> +static void spu_memset(uint32_t toi, uint32_t what, int length)
> +{
> +	int i;
> +	snd_assert(length % 4 == 0, return);
> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		writel(what, toi + SPU_MEMORY_BASE);
> +		toi++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +}
> +
You should not have the memory base hardcoded as you can already fetch it
from the memory resource. You should have an IORESOURCE_IO for the
control registers, and an IORESOURCE_MEM for the SPU RAM, then fetch
those from the platform device resources accordingly so that these remain
configurable from the board-specific code.

> +/* spu_memload - write to SPU address space */
> +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);

Same here. Again, you really want your own spu_readl()/spu_writel() that
hides this for you.

> +/* spu_disable - set spu registers to stop sound output */
> +static void spu_disable(void)
> +{
> +	int i;
> +	uint32_t regval;
> +	spu_write_wait();
> +	regval = readl(ARM_RESET_REGISTER);
> +	regval |= 1;
> +	spu_write_wait();
> +	writel(regval, ARM_RESET_REGISTER);

Same thing here..

> +	for (i = 0; i < 64; i++) {
> +		spu_write_wait();
> +		regval = readl(SPU_REGISTER_BASE + (i * 0x80));
> +		regval = (regval & ~0x4000) | 0x8000;
> +		spu_write_wait();
> +		writel(regval, SPU_REGISTER_BASE + (i * 0x80));
> +	}

And here..

> +/* Halt the sound processor,
> +   clear the memory,
> +   load some default ARM7 code,
> +   and then restart ARM7
> +*/

Please use more conventional commenting styles, as opposed to inventing
your own.

> +static void spu_init(void)

This can probably be inlined..

> +/* aica_chn_start - write to spu to start playback */
> +static void aica_chn_start(void)
> +{
> +	spu_write_wait();
> +	writel(AICA_CMD_KICK | AICA_CMD_START,
> +	       (uint32_t *) AICA_CONTROL_POINT);
> +}
> +
> +/* aica_chn_halt - write to spu to halt playback */
> +static void aica_chn_halt(void)
> +{
> +	spu_write_wait();
> +	writel(AICA_CMD_KICK | AICA_CMD_STOP,
> +	       (uint32_t *) AICA_CONTROL_POINT);
> +}
> +
These too.

> +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;
> +}
> +
Pointless return.

> +/* TO DO: set up to handle more than one pcm instance */
> +static int __init snd_aicapcmchip(struct snd_card_aica
> +				  *dreamcastcard, int pcm_index)
> +{
> +	struct snd_pcm *pcm;
> +	int err;
> +	/* AICA has no capture ability */
> +	if ((err =
> +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> +			 &pcm)) < 0)
> +		return err;

Again, move the assignment out of the if context.

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

This if/else mess is still painful to read, and your indentation level is
broken, which doesn't make it any easier to follow. If you're returning
anyways, then just kill the else in both cases.

> +/* Fill up the members of the embedded device structure */
> +static void populate_dreamcastaicadev(struct device *dev)
> +{
> +	dev->bus = &platform_bus_type;
> +	dev->driver = &aica_driver;
> +	driver_register(dev->driver);
> +	device_bind_driver(dev);
> +}
> +
This is still not something you should be doing yourself, use
the platform_driver instead.

> +static int __devinit add_aicamixer_controls(struct snd_card_aica
> +					    *dreamcastcard)
> +{
> +	int err;
> +	err = snd_ctl_add
> +	    (dreamcastcard->card,
> +	     snd_ctl_new1(&snd_aica_pcmvolume_control, dreamcastcard));

More mangled whitespace. indent may have been responsible for it, you're
still the one submitting the patch.

> +	return 0;
> +      freepcm:
> +      freedreamcast:

goto labels are still in bizzarely indented locations.

> +static void __exit aica_exit(void)
> +{
> +	struct device_driver *drv = (&pd->dev)->driver;
> +	flush_workqueue(aica_queue);
> +	destroy_workqueue(aica_queue);
> +	device_release_driver(&pd->dev);
> +	driver_unregister(drv);
> +	platform_device_unregister(pd);

Your driver model handling really needs to be reworked a bit..

> +	/* Kill any sound still playing and reset ARM7 to safe state */
> +	spu_init();

Perhaps spu_reset() is a bit more sensible of a name, spu_init() is a bit
non-sensical to call from an exit routine..

> diff -ruN a/linux-2.6.16.14/sound/sh/Kconfig b/linux-2.6.16.14/sound/sh/Kconfig
> --- a/linux-2.6.16.14/sound/sh/Kconfig	1970-01-01 01:00:00.000000000 +0100
> +++ b/linux-2.6.16.14/sound/sh/Kconfig	2006-06-01 22:59:14.000000000 +0100
> @@ -0,0 +1,15 @@
> +menu "SH (Super-H) devices"
> +       depends on SND!=n && SUPERH
> +
Please just use SuperH, it's not hyphenated, and it's more readable than
just SH.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Mon, 2006-06-05 at 12:53 +0300, Paul Mundt wrote:

> > +static void spu_init(void)
> 
> This can probably be inlined..
> 
> > +/* aica_chn_start - write to spu to start playback */
> > +static void aica_chn_start(void)
> > +{
> > +	spu_write_wait();
> > +	writel(AICA_CMD_KICK | AICA_CMD_START,
> > +	       (uint32_t *) AICA_CONTROL_POINT);
> > +}
> > +
> > +/* aica_chn_halt - write to spu to halt playback */
> > +static void aica_chn_halt(void)
> > +{
> > +	spu_write_wait();
> > +	writel(AICA_CMD_KICK | AICA_CMD_STOP,
> > +	       (uint32_t *) AICA_CONTROL_POINT);
> > +}
> > +
> These too.
> 

No point in inlining these as they all wait on a FIFO to clear - ie the
benefit of inling will be tiny. I notice a stray inline still lurking
from the code that can go...