At Sun, 28 May 2006 21:00:30 +0200,
Johannes Berg wrote:
> --- /dev/null
> +++ b/sound/aoa/codecs/snd-aoa-codec-onyx.c
> +/* both return 0 if all ok, else on error */
> +static int onyx_read_register(struct onyx *onyx, u8 reg, u8 *value)
> +{
> + s32 v;
> +
> + 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" ?
> + return 0;
> +}
> +
> +static int onyx_write_register(struct onyx *onyx, u8 reg, u8 value)
> +{
> + int result;
> +
> + result = i2c_smbus_write_byte_data(&onyx->i2c, reg, value);
> + if (!result)
> + onyx->cache[reg-FIRSTREGISTER] = value;
> + return result;
> +}
> +
> +/* alsa stuff */
> +
> +static int onyx_dev_register(struct snd_device *dev)
> +{
> + return 0;
> +}
> +
> +static struct snd_device_ops ops = {
> + .dev_register = onyx_dev_register,
> +};
> +
> +static int onyx_snd_vol_info(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_info *uinfo)
> +{
> + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> + uinfo->count = 2;
> + uinfo->value.integer.min = -128+128;
> + uinfo->value.integer.max = -1+128;
I'd define a constant for 128.
> + return 0;
> +}
> +
> +static int onyx_snd_vol_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct onyx *onyx = snd_kcontrol_chip(kcontrol);
> + s8 l,r;
> +
> + mutex_lock(&onyx->mutex);
> + onyx_read_register(onyx, ONYX_REG_DAC_ATTEN_LEFT, &l);
> + onyx_read_register(onyx, ONYX_REG_DAC_ATTEN_RIGHT, &r);
> + mutex_unlock(&onyx->mutex);
> +
> + ucontrol->value.integer.value[0] = l+128;
> + ucontrol->value.integer.value[1] = r+128;
> +
> + return 0;
> +}
> +
> +static int onyx_snd_vol_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct onyx *onyx = snd_kcontrol_chip(kcontrol);
> +
> + mutex_lock(&onyx->mutex);
> + onyx_write_register(onyx, ONYX_REG_DAC_ATTEN_LEFT, ucontrol->value.integer.value[0]-128);
> + onyx_write_register(onyx, ONYX_REG_DAC_ATTEN_RIGHT, ucontrol->value.integer.value[1]-128);
Fold lines to fit with 80 columns (heh, blaming other one's code is
easy :)
> + /* 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).
(snip)
> +static u8 register_map[] = {
> + ONYX_REG_DAC_ATTEN_LEFT,
> + ONYX_REG_DAC_ATTEN_RIGHT,
> + ONYX_REG_CONTROL,
> + ONYX_REG_DAC_CONTROL,
> + ONYX_REG_DAC_DEEMPH,
> + ONYX_REG_DAC_FILTER,
> + ONYX_REG_DAC_OUTPHASE,
> + ONYX_REG_ADC_CONTROL,
> + ONYX_REG_ADC_HPF_BYPASS,
> + ONYX_REG_DIG_INFO1,
> + ONYX_REG_DIG_INFO2,
> + ONYX_REG_DIG_INFO3,
> + ONYX_REG_DIG_INFO4
> +};
> +
> +static u8 initial_values[] = {
Should have ARRAY_SIZE(register_map) since this size must be identical
with register_map.
> + 0x80, 0x80, /* muted */
> + ONYX_MRST | ONYX_SRST, /* but handled specially! */
> + ONYX_MUTE_LEFT | ONYX_MUTE_RIGHT,
> + 0, /* no deemphasis */
> + ONYX_DAC_FILTER_ALWAYS,
> + ONYX_OUTPHASE_INVERTED,
> + (-1 /*dB*/ + 8) & 0xF, /* line in selected, -1 dB gain*/
> + ONYX_ADC_HPF_ALWAYS,
> + (1<<2), /* pcm audio */
> + 2, /* category: pcm coder */
> + 0, /* sampling frequency 44.1 kHz, clock accuracy level II */
> + 1 /* 24 bit depth */
> +};
> +
> +/* reset registers of chip, either to initial or to previous values */
> +static int onyx_register_init(struct onyx *onyx)
> +{
> + int i;
> + u8 val;
> + u8 regs[sizeof(initial_values)];
> +
> + if (!onyx->initialised) {
> + memcpy(regs, initial_values, sizeof(initial_values));
> + if (onyx_read_register(onyx, ONYX_REG_CONTROL, &val))
> + return -1;
> + val &= ~ONYX_SILICONVERSION;
> + val |= initial_values[3];
> + regs[3] = val;
> + } else {
> + for (i=0; i<sizeof(register_map); i++)
> + regs[i] = onyx->cache[register_map[i]-FIRSTREGISTER];
> + }
> +
> + for (i=0; i<sizeof(register_map); i++) {
> + if (onyx_write_register(onyx, register_map[i], regs[i]))
> + return -1;
> + }
> + onyx->initialised = 1;
> + return 0;
> +}
> +
> +static struct transfer_info onyx_transfers[] = {
> + /* this is first so we can skip it if no input is present...
> + * No hardware exists with that, but it's here as an example
> + * of what to do :) */
> + {
> + /* analog input */
> + .formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE | SNDRV_PCM_FMTBIT_S24_BE,
Too long lines ;)
(snip)
> + if (onyx->codec.soundbus_dev->attach_codec(onyx->codec.soundbus_dev,
> + aoa_get_card(),
> + ci, onyx)) {
> + printk(KERN_ERR PFX "error creating onyx pcm\n");
> + return -ENODEV;
> + }
> +#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?
> + /* we try to read from register ONYX_REG_CONTROL
> + * to check if the codec is present */
> + if (onyx_read_register(onyx, ONYX_REG_CONTROL, &dummy) != 0) {
> + i2c_detach_client(&onyx->i2c);
> + printk(KERN_ERR PFX "failed to read control register\n");
> + goto fail;
> + }
> +
> + strncpy(onyx->codec.name, "onyx", MAX_CODEC_NAME_LEN);
Use strlcpy, or MAX_CODEC_NAME_LEN-1. Similar lines are found in
tas driver too.
> --- /dev/null
> +++ b/sound/aoa/codecs/snd-aoa-codec-tas.c
> @@ -0,0 +1,669 @@
(snip)
> +struct tas {
> + u32 primary_magic;
> + struct aoa_codec codec;
> + /* see comment at top of file */
> + u32 secondary_magic;
> + struct aoa_codec secondary;
> + struct i2c_client i2c;
> + u32 muted_l:1, muted_r:1,
> + controls_created:1;
> + u8 cached_volume_l, cached_volume_r;
> + u8 mixer_l[3], mixer_r[3];
> + u8 acr;
> +};
> +
> +static struct tas *codec_to_tas(struct aoa_codec *codec)
> +{
> + u32 *tmp = (u32*)codec;
> + switch (*(tmp-1)) {
> + case TAS_PRIMARY_MAGIC:
> + return container_of(codec, struct tas, codec);
> + case TAS_SECONDARY_MAGIC:
> + return container_of(codec, struct tas, secondary);
> + default:
> + return NULL;
> + }
> +}
Looks a bit too hacky. IMO, it's better to define a struct
struct tas_codec {
struct aoa_codec codec;
struct tas *tas;
}
to make the above simpler like:
static struct tas *codec_to_tas(struct aoa_codec *codec)
{
return ((struct tas_codec *)codec)->tas;
}
The tas struct becomes:
struct as {
struct tas_codec primary;
struct tas_codec secondary;
...
}
and is initialized like:
tas->primary.tas = tas;
tas->secondary.tas = tas;
> +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[]
> +static int tas_reset_init(struct tas *tas)
> +{
> + u8 tmp;
> +/*
> + char write[8];
> + union i2c_smbus_data read = { 0 };
> + int r1, r2;
> +*/
> + tas->codec.gpio->methods->set_hw_reset(tas->codec.gpio, 0);
> + msleep(1);
> + tas->codec.gpio->methods->set_hw_reset(tas->codec.gpio, 1);
> + msleep(1);
> + tas->codec.gpio->methods->set_hw_reset(tas->codec.gpio, 0);
> + msleep(1);
> +
> + tas->acr &= ~TAS_ACR_ANALOG_PDOWN;
> + tas->acr |= TAS_ACR_B_MONAUREAL | TAS_ACR_B_MON_SEL_RIGHT;
> + if (tas_write_reg(tas, TAS_REG_ACR, 1, &tas->acr))
> + return -ENODEV;
> +
> + tmp = TAS_MCS_SCLK64 | TAS_MCS_SPORT_MODE_I2S | TAS_MCS_SPORT_WL_24BIT;
> + if (tas_write_reg(tas, TAS_REG_MCS, 1, &tmp))
> + return -ENODEV;
> +
> + tmp = 0;
> + if (tas_write_reg(tas, TAS_REG_MCS2, 1, &tmp))
> + return -ENODEV;
> +/* I need help here!
Use ifdef. Nested comments are bad.
> + /* This is a bit tricky, but serves to detect if there really
> + * is a tas codec present.
> + * First, we set the volume register to 00,00,01 (on both channels).
> + * This is almost muted. Then, we read back the last 6 bytes we
> + * wrote to the chip, and check if they are the same.
> + *
> + write[0] = 7;
> + write[1] = TAS_REG_VOL;
> + write[2] = write[3] = 0;
> + write[4] = 1;
> + write[5] = write[6] = 0;
> + write[7] = 1;
> + r1 = tas_write_reg(tas, TAS_REG_VOL, 6, &write[1]);
> + /* Hmm, how am I supposed to do the i2c sequence that
> + * is mentioned on page 45 of the tas3004 datasheet?
> + * This doesn't cut it: *
> + read.block[0] = 7;
> + r2 = i2c_smbus_xfer(tas->i2c.adapter, tas->i2c.addr, tas->i2c.flags,
> + I2C_SMBUS_READ, TAS_REG_VOL,
> + I2C_SMBUS_BLOCK_DATA, &read);
> +
> + printk(KERN_DEBUG "r1 = %d, r2 = %d, read=%x %x %x %x %x %x %x %x\n", r1, r2, read.block[0], read.block[1], read.block[2], read.block[3], read.block[4], read.block[5], read.block[6], read.block[7]);
> +
> + if (r1 || r2 || memcmp(write, read.block, 8))
> + return -ENODEV;
> +*/
> +
> + return 0;
> +}
> +static int tas_init_codec(struct aoa_codec *codec)
> +{
(snip)
> + aoa_snd_ctl_add(snd_ctl_new1(&volume_control, tas));
Error checks please.
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
