lkml.org 
[lkml]   [2012]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124
    On Thu, Feb 23, 2012 at 02:46:49PM +0900, Tomoya MORINAGA wrote:

    > + SOC_SINGLE_TLV("Playback Limitter Min Input Volume",
    > + ML26124_PL_MAXMIN_GAIN, 0, 7, 0, mingain),

    Limiter.

    > + SOC_SINGLE("Zero Cross Comparator Switch", ML26124_PW_ZCCMP_PW_MNG, 1,
    > + 1, 0),

    Usually just "ZC Switch".

    > + SOC_SINGLE("Digital Switch", ML26124_DVOL_CTL, 4, 1, 0),

    This should probably be Digital Playback Switch so it lines up with the
    volume control in the UIs (and the Volume controls should be Digital X
    Volume).

    > +static int ml26124_update_bits(struct snd_soc_codec *codec, unsigned short reg,
    > + unsigned int mask, unsigned int value)

    Why are you open coding this in your driver? There is no point in
    replicating subsystem functionality.

    > +if (!priv)
    > + return -1;
    > +

    Coding style and return real error codes (though this looks like it
    should never happen).

    > +static int ml26124_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
    > + int div_id, int div)
    > +{
    > + struct snd_soc_codec *codec = codec_dai->codec;
    > +
    > + switch (div_id) {
    > + case ML26124_MCLK:
    > + ml26124_update_bits(codec, ML26124_CLK_CTL,
    > + BIT(0) | BIT(1), div);
    > + break;

    Why can't the driver calculate this automatically given the MCLK?

    > + case SND_SOC_BIAS_ON:
    > + pr_debug("%s: level=ON priv=%p\n", __func__, priv);

    There's already more than enough logging in the subsystem for this.

    > + /* VMID ON */
    > + ml26124_update_bits(codec, ML26124_PW_REF_PW_MNG,
    > + ML26124_VMID, ML26124_VMID);
    > + msleep(500);

    This looks like it should be _STANDBY - usually VMID must be on to power
    everything else.

    > +static int ml26124_pcm_trigger(struct snd_pcm_substream *substream,
    > + int cmd, struct snd_soc_dai *codec_dai)
    > +{
    > + struct snd_soc_codec *codec = codec_dai->codec;
    > +
    > + switch (cmd) {
    > + case SNDRV_PCM_TRIGGER_STOP:
    > + case SNDRV_PCM_TRIGGER_SUSPEND:
    > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
    > + ml26124_update_bits(codec, ML26124_REC_PLYBAK_RUN, 0x3,
    > + 0);

    I'm not sure you've tested this, you can't do I2C I/O from atomic
    context and trigger is atomic. What does this control actually do?

    > + return 0;
    > + break;

    Duplicate return and break here.

    > +static int ml26124_resume(struct snd_soc_codec *codec)
    > +{
    > + struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
    > +
    > + regcache_sync(priv->regmap);
    > + ml26124_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

    Setting the bias level will duplicate the sync.

    > +static int ml26124_probe(struct snd_soc_codec *codec)
    > +{
    > + int ret;
    > + struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
    > + codec->control_data = priv->regmap;
    > +
    > + ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);
    > + if (ret < 0) {
    > + dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
    > + return ret;
    > + }

    You're mixing regmap API usage and ASoC level I2C, this should be
    _REGMAP.

    > +/* power down chip */
    > +static struct snd_soc_codec_driver soc_codec_dev_ml26124 = {

    This comment is misplaced.

    > +static struct i2c_driver ml26124_i2c_driver = {
    > + .driver = {
    > + .name = "ml26124-codec",

    No -codec, the device only does one thing.

    > +static int __init ml26124_modinit(void)
    > +{
    > + int ret;
    > +
    > + ret = i2c_add_driver(&ml26124_i2c_driver);
    > + if (ret != 0)

    module_i2c_driver().
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-03-01 00:55    [W:0.026 / U:2.708 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site