Re: [Alsa-devel] [RFC 4/7] snd-aoa: add codecs

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

On Mon, 2006-05-29 at 14:08 +0200, Takashi Iwai wrote:
> > +	if (reg != ONYX_REG_CONTROL) {
> > +		*value = onyx->cache[reg-FIRSTREGISTER];
> > +		return 0;
> > +	}
> > +	v = i2c_smbus_read_byte_data(&onyx->i2c, reg);
> > +	if (v < 0)
> > +		return -1;
> > +	*value = (u8)v;
> > +	onyx->cache[ONYX_REG_CONTROL-FIRSTREGISTER] = *value;
> 
> Isn't it "reg - FIRSTREGISTER" ?

Nah, look at the first line I quoted :)


> I'd define a constant for 128.

#define OFFSET_BECAUSE_ALSA_USERSPACE_PROGRAMS_SUCK 128
;)

Just kidding, will do.

> Fold lines to fit with 80 columns (heh, blaming other one's code is
> easy :)

Heh. I thought I'd Lindent'ed most of it.

> > +	/* FIXME: we could be checking if anything changed */
> > +	mutex_unlock(&onyx->mutex);
> > +
> > +	return 1;
> 
> The put callback is supposed to return 0 if the values are unchanged
> (although most apps ignore the return value).

Does it have to? This way there's an event, but...

> > +static u8 initial_values[] = {
> 
> Should have ARRAY_SIZE(register_map) since this size must be identical
> with register_map.

Good point.

> > +#define ADDCTL(n)							\
> > +	do {								\
> > +		ctl = snd_ctl_new1(&n, onyx);				\
> > +		if (ctl) {						\
> > +			ctl->id.device =				\
> > +				onyx->codec.soundbus_dev->pcm->device;	\
> > +			aoa_snd_ctl_add(ctl);				\
> 
> No error check?

Hmm, yeah, I guess it should have error checks. Will revisit that.

> Use strlcpy, or MAX_CODEC_NAME_LEN-1.  Similar lines are found in
> tas driver too.

Right.

> Looks a bit too hacky.  IMO, it's better to define a struct

Maybe I'll just get rid of it completely, it's never hooked up as far as
I know. But yeah, I guess you're right.

> > +static int tas_snd_capture_source_info(struct snd_kcontrol *kcontrol,
> > +	struct snd_ctl_elem_info *uinfo)
> > +{
> > +	static char* texts[] = { "Line-In", "Microphone" };
> 
> char *texts[]

Any particular reason?

> > +/* I need help here!
> 
> Use ifdef.  Nested comments are bad.

I know. Temporary code, probably going away completely.

> > +	aoa_snd_ctl_add(snd_ctl_new1(&volume_control, tas));
> 
> Error checks please.

What should it do on such errors?

Thanks,
johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]