On Wed, Feb 26, 2014 at 03:22:16PM +0800, RongJun Ying wrote:
Overall looks fairly good, a few issues below but they're quite small.
+static struct sirf_audio_codec_reg_bits sirf_audio_codec_reg_bits_prima2 = {
- .dig_mic_en_bits = 20,
- .dig_mic_freq_bits = 21,
- .adc14b_12_bits = 22,
- .firdac_hsl_en_bits = 23,
- .firdac_hsr_en_bits = 24,
- .firdac_lout_en_bits = 25,
- .por_bits = 26,
- .codec_clk_en_bits = 27,
+};
This looks like the sort of thing that the regmap_field layer was supposed to hide?
+static int adc_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- struct snd_soc_codec *codec = w->codec;
- struct sirf_audio_codec *sirf_audio_codec = dev_get_drvdata(codec->dev);
- u32 val;
- val = sirfsoc_rtc_iobrg_readl(sirf_audio_codec->sys_pwrc_reg_base +
PWRC_PDN_CTRL_OFFSET);- switch (event) {
- case SND_SOC_DAPM_POST_PMU:
/* Enable capture power of codec*/val |= (1 << AUDIO_POWER_EN_BIT);break;
Looking at this code (and many of the other controls) I can't help but feel that there's some room for more abstraction here. The controls are all setting two bits rather than just one but for example...
- case SND_SOC_DAPM_PRE_PMU:
val = (1 << sirf_audio_codec->reg_bits->firdac_hsl_en_bits);break;
...this looks like a supply widget could be used to manage the bits controlled by the pre and post PMU events? The same is true for most of the events.
- dn = of_find_compatible_node(dn, NULL, "sirf,prima2-pwrc");
- if (!dn) {
dev_err(&pdev->dev, "Failed to get sirf,prima2-pwrc node!\n");return -ENODEV;- }
- ret = of_property_read_u32(dn, "reg", &sirf_audio_codec->sys_pwrc_reg_base);
- if (ret < 0) {
dev_err(&pdev->dev, "Failed tp get pwrc register base address\n");return -EINVAL;- }
Should prima2-pwrc be a syscon? Not a blocker for merging this.
+#ifdef CONFIG_PM_RUNTIME +static int sirf_audio_codec_runtime_suspend(struct device *dev) +{
- struct sirf_audio_codec *sirf_audio_codec = dev_get_drvdata(dev);
- regmap_update_bits(sirf_audio_codec->regmap, AUDIO_IC_CODEC_CTRL1,
(1 << sirf_audio_codec->reg_bits->codec_clk_en_bits),0);- return 0;
+}
Can you disable the clock in the clock API as well? This might allow further supply clocks to be disabled and is genarally good practice.
+static int sirf_audio_codec_suspend(struct device *dev) +{
- struct sirf_audio_codec *sirf_audio_codec = dev_get_drvdata(dev);
- regmap_read(sirf_audio_codec->regmap, AUDIO_IC_CODEC_CTRL0,
&sirf_audio_codec->reg_ctrl0);- regmap_read(sirf_audio_codec->regmap, AUDIO_IC_CODEC_CTRL1,
&sirf_audio_codec->reg_ctrl1);- sirf_audio_codec_runtime_suspend(dev);
Have you seen the series Ulf has been posting regarding the integration of runtime PM and system suspend? There's all sorts of nasty cases with this stuff unfortunately. Not sure it should be a blocker for merging though since we haven't really figured out how this stuff is supposed to work.