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...
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
