On Tue, Jun 01, 2010 at 09:32:38PM +1000, Stuart Longland wrote:
I know this isn't a proper submission but a few comments below. This looks like it'd be relatively easy to get submitted by stripping out a lot of the commented out code and custom interfaces (like the DA7210 driver).
The CODEC driver claims total control of the I2C device, and therefore makes it impossible to alter registers using i2c-tools. However, as a work-around; I have provided read/write access to the registers via sysfs... we use the AIC3204 attached to I2C bus 0; the CODEC therefore lives under:
/sys/bus/i2c/devices/0-0018
There are two files:
- regsel: Takes or reports back the 16-bit register address in hexadecimal
- regdata: Reads or writes the value of the register
ASoC already provides register read/write access via debugfs as standard, there's no need to implement this.
struct aic3204_setup_data { unsigned int gpio_func[2]; };
This would normally be platform data in a file in include/sound so it can be set by the architecture code when the device is registered.
/* TODO: PLL */ /* #define ENABLE_PLL */
/* SYSFS Interface -- we should move this to debugfs */ static ssize_t aic3204_show_regsel(struct device *dev, struct device_attribute *attr, char *buf); static ssize_t aic3204_store_regsel(struct device *dev, struct device_attribute *attr, const char *buf, size_t count); static ssize_t aic3204_show_regdata(struct device *dev, struct device_attribute *attr, char *buf); static ssize_t aic3204_store_regdata(struct device *dev, struct device_attribute *attr, const char *buf, size_t count); static DEVICE_ATTR(regsel, S_IWUSR | S_IRUGO, aic3204_show_regsel, aic3204_store_regsel); static DEVICE_ATTR(regdata, S_IWUSR | S_IRUGO, aic3204_show_regdata, aic3204_store_regdata);
As I said above this is redundant and can be removed.
#if 0 printk( KERN_INFO "%s: pg %d reg %d[%04x] => %02x\n", __func__, reg >> 8, reg & 0xff, reg, value[0] ); #endif
dev_dbg().
}
/*
- Perform a read/modify/write cycle on a register.
- This is a shorthand function, it reads the specified register, masks out the
- bits in and_mask, applies bits in or_mask, then writes out the result to the
- register.
- It returns the modified value; or a negative error code.
*/
There's a standard snd_soc_update_bits() function in ASoC.
/*
- All input lines are connected when !0xf and disconnected with 0xf bit field,
- so we have to use specific dapm_put call for input mixer
*/
Could you explain in more detial what this is doing? I'm not immediately seeing what this is doing but I suspect it might be a value mux?
#if 0
Just drop if 0ed sections.
#define LDAC_ENUM 0 #define RDAC_ENUM 1 #define LHPCOM_ENUM 2 #define RHPCOM_ENUM 3 #define LINE1L_ENUM 4 #define LINE1R_ENUM 5 #define LINE2L_ENUM 6 #define LINE2R_ENUM 7 #define ADC_HPF_ENUM 8
static const struct soc_enum aic3204_enum[] = { SOC_ENUM_SINGLE(DAC_LINE_MUX, 6, 3, aic3204_left_dac_mux),
Use individually named variables rather than a table for legibility.
/* Turn on ADC or DAC */ if ( substream->stream == SNDRV_PCM_STREAM_PLAYBACK ) { aic3204_write(codec, AIC3204_DACS1, AIC3204_DACS1_LDAC_UP | AIC3204_DACS1_RDAC_UP | AIC3204_DACS1_LDACD_LEFT | AIC3204_DACS1_RDACD_RIGHT | AIC3204_DACS1_SOFT_DIS );
DAPM ought to be figuring this out for you...
/* This all needs to be done elsewhere */
Yes.
void aic3204_set_headset_detection(struct snd_soc_codec *codec, int detect, int headset_debounce, int button_debounce) { #if 0
There's standard ASoC jack detection which this should integrate with.
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) /*
- AIC3204 2 wire address can be up to 4 devices with device addresses
- 0x18, 0x19, 0x1A, 0x1B
*/
/*
- If the i2c layer weren't so broken, we could pass this kind of data
- around
*/ static int aic3204_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
Use standard device model registration - the driver you were basing this on has been converted now...