Re: [PATCH] ASoC: rt5631: first commit

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Thu, Sep 01, 2011 at 08:21:14PM +0800, johnnyhsu@realtek.com wrote:

> Subject: [PATCH] ASoC: rt5631: first commit

"Add driver for..." or something.

> +static int rt5631_volatile_register(struct snd_soc_codec *codec,
> +				    unsigned int reg)
> +{
> +	switch (reg) {
> +	case RT5631_VENDOR_ID:
> +	case RT5631_VENDOR_ID1:
> +	case RT5631_VENDOR_ID2:
> +		return 0;
> +	default:
> +		return 1;
> +	}
> +}

This looks to be exactly the wrong way round - the only registers we can
cache are the ID registers.

> +/* MIC Boost Volume */
> +static const char *rt5631_mic_boost[] = {"Bypass", "+20db", "+24db", "+30db",
> +			"+35db", "+40db", "+44db", "+50db", "+52db"};

TLV for volume information.

> +	rt5631->bclk_rate = snd_soc_params_to_bclk(params);
> +	if (rt5631->bclk_rate < 0) {
> +		dev_err(codec->dev, "Unsupported BCLK rate: %d\n",
> +					rt5631->bclk_rate);
> +		return rt5631->bclk_rate;
> +	}

The error message here is inacurate, there was an error obtaining the
BCLK rate rather than a problem supporting it.

> +static int rt5631_codec_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
> +		int source, unsigned int freq_in, unsigned int freq_out)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct rt5631_priv *rt5631 = snd_soc_codec_get_drvdata(codec);
> +	int i, ret = -EINVAL;
> +
> +	dev_dbg(codec->dev, "enter %s\n", __func__);
> +
> +	if (!freq_in || !freq_out) {
> +		dev_dbg(codec->dev, "PLL disabled\n");
> +		return -EINVAL;
> +	}

This shouldn't be an error condition, disabling the PLL is a perfectly
normal thing to do.

> +	if (rt5631->master) {
> +		for (i = 0; i < ARRAY_SIZE(codec_master_pll_div); i++)
> +			if (freq_in == codec_master_pll_div[i].pll_in &&

Since the PLL configuration depends on the master/slave configuration
you should either warn or reconfigure if the PLL is set up and the user
changes master/slave configuration.

> +/**
> + * rt5631_index_reg_show - sysfs file for dumping index registers of 2nd layer
> + */
> +static ssize_t rt5631_index_reg_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{

No, use the standard facilities.  Please don't ignore review comments,
it's not helpful.