Re: [PATCH v4-resend 1/7] ASoC: sirf: Add SiRF internal audio codec driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3081 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]