On Thu, Nov 10, 2011 at 14:11, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
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涎 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
Thanks, for review, I'll send the incremental patches in the near future.
+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.
You are right.
+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).
If it possible, I would prefer to wait with this. I'll not be able to test the regmap interface properly because it was implemented for the 3.0 tree, and my development machine doesn't support it yet.
Sure, firstly I'll try to apply your patches on our tree (http://lwn.net/Articles/451178/)
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.
Ok, thanks.
-- Leon Romanovsky | Independent Linux Consultant www.leon.nu | leon@leon.nu