Re: [alsa-devel] [RFC] SGI O2 MACE audio ALSA module

[Takashi Iwai <tiwai@suse.de>]

At Mon, 9 Jul 2007 22:07:31 +0100,
TJ wrote:
> 
> >
> > Other things I noticed through a quick glance:
> >
> > - Follow the standard coding style, e.g. 80 chars in a line, don't put
> >   if-block in a single line, etc.
> 
> I tidily hard wrapped lines that i spotted over 80. How many people
> still using 80 column terminals? Maybe 132 would be a better choice
> these days?
> 
> Is something like:
> if (0 > err) goto ERROR_EXIT;
> really that bad? In two lines it isn't much clearer and just user more space.

Yes, that matters very much.  The point is that we should keep the
same coding style.  It's not for saving spaces but improves
readability and makes easier (not only for you but for other
reviewers) to find bugs.

> --- linux-2.6.21.6-b/sound/mips/ad1843.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.21.6/sound/mips/ad1843.c	2007-07-09 20:09:15.000000000 +0100
(snip)
> +typedef struct ad1843_gain {
> +       int     negative;               /* nonzero if gain is negative. */
> +       const ad1843_bitfield_t *lfield;
> +       const ad1843_bitfield_t *rfield;
> +} ad1843_gain_t;
> +
> +const ad1843_gain_t ad1843_gain_RECLEV
> +                               = { 0, &ad1843_LIG,   &ad1843_RIG };
> +const ad1843_gain_t ad1843_gain_LINE
> +                               = { 1, &ad1843_LX1M,  &ad1843_RX1M };
> +const ad1843_gain_t ad1843_gain_CD
> +                               = { 1, &ad1843_LX2M,  &ad1843_RX2M };
> +const ad1843_gain_t ad1843_gain_MIC
> +                               = { 1, &ad1843_LMCM,  &ad1843_RMCM };
> +const ad1843_gain_t ad1843_gain_PCM_0
> +                               = { 1, &ad1843_LDA1G, &ad1843_RDA1G };
> +const ad1843_gain_t ad1843_gain_PCM_1
> +                               = { 1, &ad1843_LDA2G, &ad1843_RDA2G };

Forgotten static here?

> +
> +const ad1843_gain_t *ad1843_gain[AD1843_GAIN_SIZE] = {
> +  &ad1843_gain_RECLEV,
> +  &ad1843_gain_LINE,
> +  &ad1843_gain_CD,
> +  &ad1843_gain_MIC,
> +  &ad1843_gain_PCM_0,
> +  &ad1843_gain_PCM_1,
> +};

Ditto.

> +/*
> + * Enable/disable digital resample mode in the AD1843.
> + *
> + * The AD1843 requires that ADL, ADR, DA1 and DA2 be powered down
> + * while switching modes.  So we save DA's state, power them down,
> + * switch into/out of resample mode, power them up, and restore state.
> + *
> + * This will cause audible glitches if D/A or A/D is going on, so the
> + * driver disallows that (in mixer_write_ioctl()).
> + *
> + * The open question is, is this worth doing?  I'm leaving it in,
> + * because it's written, but...
> + */
> +
> +void ad1843_set_resample_mode(ad1843_t *ad1843, int onoff)
> +{
> +       /* Save DA's mute and gain (addr 9/10 is analog gain/attenuation) */
> +       int save_da1 = ad1843->read(ad1843->chip, 9);
> +       int save_da2 = ad1843->read(ad1843->chip, 10);
> +
> +       /* Power down A/D and D/A. */
> +       ad1843_write_multi(ad1843, 4,
> +                          &ad1843_DA1EN, 0,
> +                          &ad1843_DA2EN, 0,
> +                          &ad1843_ADLEN, 0,
> +                          &ad1843_ADREN, 0);
> +
> +       /* Switch mode */
> +       ad1843_write_bits(ad1843, &ad1843_DRSFLT, onoff);
> +
> +       /* Power up A/D and D/A. */
> +       ad1843_write_multi(ad1843, 3,
> +                          &ad1843_DA1EN, 1,
> +                          &ad1843_DA2EN, 1,
> +                          &ad1843_ADLEN, 1,
> +                          &ad1843_ADREN, 1);

Isn't it 4?

Maybe better to use NULL as the terminal pointer for varargs instead
of passing the number of arguments?

> +/*
> + * Fully initialize the ad1843.  As described in the AD1843 data
> + * sheet, section "START-UP SEQUENCE".  The numbered comments are
> + * subsection headings from the data sheet.  See the data sheet, pages
> + * 52-54, for more info.
> + *
> + * return 0 on success, -errno on failure.  */
> +
> +int ad1843_init(ad1843_t *ad1843)
> +{
> +       unsigned long later;
> +
> +	/* 1. Power up in hardware */
> +
> +	/* 2. Assert ^RESET^ to 0, wait 100ns
> +	 * 3. Deassert ^RESET^ _400 to 800us, poll INIT to see when ready
> +	 * Done in calling driver */
> +	udelay(800);

mdelay(1) would be scheduler-friendly (if the longer delay doesn't
matter).

> +	/* Did we come out of standby ? */
> +	while (ad1843_read_bits(ad1843, &ad1843_PDNO)) {
> +		if (time_after(jiffies, later)) {
> +			printk(KERN_ERR "ad1843: AD1843 won't power up\n");
> +                       return -EIO;
> +		}
> +		schedule();

I'd use schedule_timeout(1) here.

> --- linux-2.6.21.6-b/sound/mips/mace_audio.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.21.6/sound/mips/mace_audio.c	2007-07-09 20:29:50.000000000 +0100
(snip)
> +/* Global needed for module exit */
> +snd_mace_audio_t *snd_mace_audio_chip = NULL;

Missing static.

> +/* constructer */
> +static int __devinit sma_probe(void)

In the current code, you call this directly from module init entry,
thus it's actually __init.  But, if you rewrite the code for the newer
style using struct device (perhaps better with platform_device or so),
__devinit is the right attribute.


Takashi