[alsa-devel] [PATCH v4] ASoC: Add new Realtek ALC5632 CODEC driver.

[Leon Romanovsky]

On Thu, Nov 10, 2011 at 14:11, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
>
> On Sat, Nov 05, 2011 at 12:38:02PM +0200, leon at leon.nu wrote:
> > From: Leon Romanovsky <leon at 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 at leon.nu