On Sat, Nov 05, 2011 at 12:38:02PM +0200, leon@leon.nu wrote:
From: Leon Romanovsky leon@leon.nu
This driver implements basic functionality, using I²C for the control channel.
This is mostly looking good so I've applied it to save on review time but there are a few fairly minor issues which you really should address in follow on patches
+static int alc5632_mute(struct snd_soc_dai *dai, int mute) +{
- struct snd_soc_codec *codec = dai->codec;
- u16 hp_mute = ALC5632_MISC_HP_DEPOP_MUTE_L \
|ALC5632_MISC_HP_DEPOP_MUTE_R;
No need for the \ here.
+#define ALC5632_ADC_REC_GAIN_COMP(x) (int)((x - ALC5632_ADC_REC_GAIN_BASE) \
/ ALC5632_ADC_REC_GAIN_STEP)+#define ALC5632_MIC_BOOST_COMP(x) (int)(x / ALC5632_MIC_BOOST_STEP)
+#define ALC5632_SPK_OUT_VOL_COMP(x) (int)(x / ALC5632_SPK_OUT_VOL_STEP)
These aren't used and should probably be deleted.
+static int alc5632_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)+{
- struct alc5632_priv *alc5632;
- int ret, vid1, vid2;
- vid1 = i2c_smbus_read_word_data(client, ALC5632_VENDOR_ID1);
- if (vid1 < 0) {
dev_err(&client->dev, "failed to read I2C\n");return -EIO;- } else {
dev_info(&client->dev, "got vid1: %x\n", vid1);- }
- vid1 = ((vid1 & 0xff) << 8) | (vid1 >> 8);
Ideally you should convert the driver over to work with the regmap API and make use of the cache code it provides. This is going to be required at some point in the future and would mean that you could use the same register I/O functionality here where you're identifying the device (which is definitely good practice).
I'd also do an initial device reset here to make sure that in the interval between the driver registering and ASoC starting the hardware is in a known good state.