| Message ID | 87a7zpwfae.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 14 2017 15:31, Kuninori Morimoto wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Power Down Release Command (PMVR, PMDAC, RSTN, PMDA1-PMDA6) > which are located on PW_MGMT1 / PW_MGMT3 register must be > write again after at least 5 LRCK cycle or later from the first > command. Otherwise, Playback volume will be 0dB. > This patch adds dummy write when Playback Start timing. > ... > + > + delay = 6000000 / priv->rate; > + priv->component = &codec->component; > + schedule_delayed_work(&priv->dummy_write_work, > + usecs_to_jiffies(delay)); In my understanding, kernel's service of delayed work relies on kernel's timer service. Resolution of the service is quite coarse in this purpose, at least for several phases of word select clock. In the worst case when task scheduling is too delay, many audio samples are not presented by the DAC in the beginning of playback. I can easily imagine that users get confused. For this purpose, hrtimer kernel service is better. However, in design of ALSA PCM core, .trigger callback can be done in any IRQ context, basically. This means that we can't call task scheduler in the callback as long as issuing the command in the callback. This is a reason that you use workqueue for this purpose. In your patch description, 5 word select phases after 'the first command'. According to "Digital Attenuator" section per page 44 of AK4613 English datasheet (AsahiKASEI 2015, MS1052-E-05)[1], your 'the first command' seems to be the previous command issueing of 'power-down release command'. In my understanding, in ak4613 codec driver, issueing of the previous command is done in 'ak4613_set_bias_level()'. If there's a guarantee for DAPM stuffs to call this function on any non-IRQ context, it's better to call the subsequent commands in the function with interval of sleep functionality with finer resolution (e.g. usleep_range()) but I don't know exactly that any components has guarantee to receive word select clock when the function is called. [1] http://www.akm.com/akm/en/file/datasheet/AK4613VQ.pdf Regards Takashi Sakamoto
On Tue, Nov 14, 2017 at 05:27:42PM +0900, Takashi Sakamoto wrote: > In my understanding, kernel's service of delayed work relies on kernel's > timer service. Resolution of the service is quite coarse in this purpose, at > least for several phases of word select clock. In the worst case when task > scheduling is too delay, many audio samples are not presented by the DAC in > the beginning of playback. I can easily imagine that users get confused. Yes, the delayed work might be delayed a lot more than expected under load (or possibly for other reasons). > In your patch description, 5 word select phases after 'the first command'. > According to "Digital Attenuator" section per page 44 of AK4613 English > datasheet (AsahiKASEI 2015, MS1052-E-05)[1], your 'the first command' seems > to be the previous command issueing of 'power-down release command'. In my > understanding, in ak4613 codec driver, issueing of the previous command is > done in 'ak4613_set_bias_level()'. If there's a guarantee for DAPM stuffs to > call this function on any non-IRQ context, it's better to call the > subsequent commands in the function with interval of sleep functionality > with finer resolution (e.g. usleep_range()) but I don't know exactly that > any components has guarantee to receive word select clock when the function > is called. The problem with set_bias_level() for this might be that the clocks aren't all running until audio is triggered... it's tricky to find a good place to put this that's in line. I'm thinking we might want to add a post trigger callback for cases like this, digital_mute() kind of has that job right now (since it exists mainly to cover up things like controllers that output nonsense during startup) but it'd be a bit of an abuse to just use it as-is. Probably easiest though.
Hi Sakamoto-san, Mark > > In your patch description, 5 word select phases after 'the first command'. > > According to "Digital Attenuator" section per page 44 of AK4613 English > > datasheet (AsahiKASEI 2015, MS1052-E-05)[1], your 'the first command' seems > > to be the previous command issueing of 'power-down release command'. In my > > understanding, in ak4613 codec driver, issueing of the previous command is > > done in 'ak4613_set_bias_level()'. If there's a guarantee for DAPM stuffs to > > call this function on any non-IRQ context, it's better to call the > > subsequent commands in the function with interval of sleep functionality > > with finer resolution (e.g. usleep_range()) but I don't know exactly that > > any components has guarantee to receive word select clock when the function > > is called. Thank you for your feedback. It seems better idea, will create v2 patch > The problem with set_bias_level() for this might be that the clocks > aren't all running until audio is triggered... it's tricky to find a > good place to put this that's in line. I'm thinking we might want to > add a post trigger callback for cases like this, digital_mute() kind of > has that job right now (since it exists mainly to cover up things like > controllers that output nonsense during startup) but it'd be a bit of an > abuse to just use it as-is. Probably easiest though. Yeah, clocks comming after trigger is one of other problem, actually. Best regards --- Kuninori Morimoto
Hi Sakamoto-san > > > In your patch description, 5 word select phases after 'the first command'. > > > According to "Digital Attenuator" section per page 44 of AK4613 English > > > datasheet (AsahiKASEI 2015, MS1052-E-05)[1], your 'the first command' seems > > > to be the previous command issueing of 'power-down release command'. In my > > > understanding, in ak4613 codec driver, issueing of the previous command is > > > done in 'ak4613_set_bias_level()'. If there's a guarantee for DAPM stuffs to > > > call this function on any non-IRQ context, it's better to call the > > > subsequent commands in the function with interval of sleep functionality > > > with finer resolution (e.g. usleep_range()) but I don't know exactly that > > > any components has guarantee to receive word select clock when the function > > > is called. I asked detail to AsahiKASEI. This "the first command" was confusable expression. We need dummy write every after command, and everytime needs 5LRCKs. 1. PowerDownRelease by Power Management1 <= call 1.x after 5LRCK 1.x Dummy write to Power Management1 2. PowerDownRelease by Power Management3 <= call 2.x after 5LRCK 2.x Dummy write to Power Management3 To avoid too many dummy write, this patch is merging it 1. PowerDownRelease by Power Management1 2. PowerDownRelease by Power Management3 <= call 2.x after 5LRCK 2.x Dummy write to Power Management1/3 <= merge both dummy write Best regards --- Kuninori Morimoto
diff --git a/sound/soc/codecs/ak4613.c b/sound/soc/codecs/ak4613.c index ee9e822..836c111 100644 --- a/sound/soc/codecs/ak4613.c +++ b/sound/soc/codecs/ak4613.c @@ -15,6 +15,7 @@ */ #include <linux/clk.h> +#include <linux/delay.h> #include <linux/i2c.h> #include <linux/slab.h> #include <linux/of_device.h> @@ -95,6 +96,9 @@ struct ak4613_priv { struct mutex lock; const struct ak4613_interface *iface; struct snd_pcm_hw_constraint_list constraint; + struct delayed_work dummy_write_work; + struct snd_soc_component *component; + unsigned int rate; unsigned int sysclk; unsigned int fmt; @@ -392,6 +396,7 @@ static int ak4613_dai_hw_params(struct snd_pcm_substream *substream, default: return -EINVAL; } + priv->rate = rate; /* * FIXME @@ -467,11 +472,64 @@ static int ak4613_set_bias_level(struct snd_soc_component *component, return 0; } +static void ak4613_dummy_write(struct work_struct *work) +{ + struct ak4613_priv *priv = container_of(work, + struct ak4613_priv, + dummy_write_work.work); + struct snd_soc_component *component = priv->component; + unsigned int mgmt1; + unsigned int mgmt3; + + snd_soc_component_read(component, PW_MGMT1, &mgmt1); + snd_soc_component_read(component, PW_MGMT3, &mgmt3); + + snd_soc_component_write(component, PW_MGMT1, mgmt1); + snd_soc_component_write(component, PW_MGMT3, mgmt3); +} + +static int ak4613_dai_trigger(struct snd_pcm_substream *substream, int cmd, + struct snd_soc_dai *dai) +{ + struct snd_soc_codec *codec = dai->codec; + struct ak4613_priv *priv = snd_soc_codec_get_drvdata(codec); + unsigned long delay; + + /* + * FIXME + * + * PW_MGMT1 / PW_MGMT3 needs dummy write at least after 5 LR clocks + * from Power Down Release. Otherwise, Playback volume will be 0dB. + * To avoid complex multiple delayed_work call from + * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...), + * call it once here. Here Let's call it after 6 LR + */ + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + break; + default: + return 0; + } + + if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK) + return 0; + + delay = 6000000 / priv->rate; + priv->component = &codec->component; + schedule_delayed_work(&priv->dummy_write_work, + usecs_to_jiffies(delay)); + + return 0; +} + static const struct snd_soc_dai_ops ak4613_dai_ops = { .startup = ak4613_dai_startup, .shutdown = ak4613_dai_shutdown, .set_sysclk = ak4613_dai_set_sysclk, .set_fmt = ak4613_dai_set_fmt, + .trigger = ak4613_dai_trigger, .hw_params = ak4613_dai_hw_params, }; @@ -591,6 +649,7 @@ static int ak4613_i2c_probe(struct i2c_client *i2c, priv->iface = NULL; priv->cnt = 0; priv->sysclk = 0; + INIT_DELAYED_WORK(&priv->dummy_write_work, ak4613_dummy_write); mutex_init(&priv->lock);