Re: [PATCH] ASoC: rt5514: add rt5514 codec driver

[Mark Brown <broonie@kernel.org>]

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

On Tue, Jan 19, 2016 at 08:02:28PM +0800, Oder Chiou wrote:

> +	pr_warn("Base clock rate %d is too high\n", rate);
> +	return -EINVAL;

dev_warn()

> +static int rt5514_adc_power_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct rt5514_priv *rt5514 = snd_soc_codec_get_drvdata(codec);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		regmap_update_bits(rt5514->regmap, RT5514_PWR_ANA1,
> +			RT5514_POW_LDO18_IN | RT5514_POW_LDO18_ADC |
> +			RT5514_POW_LDO21 | RT5514_POW_BG_LDO18_IN |
> +			RT5514_POW_BG_LDO21,
> +			RT5514_POW_LDO18_IN | RT5514_POW_LDO18_ADC |
> +			RT5514_POW_LDO21 | RT5514_POW_BG_LDO18_IN |
> +			RT5514_POW_BG_LDO21);
> +		regmap_update_bits(rt5514->regmap, RT5514_PWR_ANA2,
> +			RT5514_POW_BG_MBIAS | RT5514_POW_MBIAS |
> +			RT5514_POW_VREF2 | RT5514_POW_VREF1,
> +			RT5514_POW_BG_MBIAS | RT5514_POW_MBIAS |
> +			RT5514_POW_VREF2 | RT5514_POW_VREF1);

This looks suspicously like there are a lot of supplies here that could
be handled by supply widgets?

> +	bclk_ms = frame_size > 32 ? 1 : 0;

Please write normal if statements, it makes things easier to read.

> +static int rt5514_remove(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int rt5514_suspend(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}
> +
> +static int rt5514_resume(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}

Just remove empty functions.

> +static int rt5514_i2c_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	struct rt5514_priv *rt5514 = i2c_get_clientdata(client);
> +
> +	regmap_read(rt5514->regmap_physical, reg | RT5514_DSP_MAPPING, val);

Some comments or something in the changelog explaining what's going on
with this multi-level register map would be good.  The naming of these
functions is a bit odd, I'd expect _i2c_ for the physical regmap but
actually these are for accessing the DSP register map AIUI?

I *think* this is OK but the explanation would help me be sure (and
hopefully anyone working with the driver in the future).

> +static void rt5514_reset(struct rt5514_priv *rt5514)
> +{
> +	regmap_write(rt5514->regmap_physical, 0x1800101c, 0x00000000);
> +	regmap_write(rt5514->regmap_physical, 0x18001100, 0x0000031f);

This is a fun set of magic numbers!  I'm wondering if it might be better
as a regmap patch?  Normally patches are corrections to the register
settings that get applied after something is reset but perhaps this 
also applies here (or to some of the sequence)...  It's the fact that
it's a large set of magic numbers getting written in around reset, it
looks like some of the purpose is the same.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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