linux-kernel - Re: [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANKRQngOkGTPM9AWEaHoaNQCyN65B_ESB24ctcT_DiEen050fg@mail.gmail.com>
Date:	Fri, 2 Mar 2012 17:16:04 +0900
From:	Tomoya MORINAGA <tomoya.rohm@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
Subject: Re: [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124

2012年3月1日8:51 Mark Brown <broonie@...nsource.wolfsonmicro.com>:
>> +     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).

No, not only Playback but also for both Playback and Capture.

>> +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?
Sorry, I can't understand your saying.
Why does this driver need to calculate ?
I think this driver can use "div" value directly.

>> +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?

This control starts ether Recording or Playing or both.
Anyway, this processing must be moved somewhere.

>
>> +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.
>

Currently, it seems codec control via i2c works well.
Let me know why I should  use SND_SOC_REGMAP not _I2C.


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

Sorry, I can't understand your saying.

thanks.
-- 
ROHM Co., Ltd.
tomoya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/