Re: [PATCH] ASoC: Add driver for rt5631

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

On Tue, Sep 06, 2011 at 06:32:42PM +0800, johnnyhsu@realtek.com wrote:

>  	switch (reg) {
> -	case RT5631_VENDOR_ID:
> -	case RT5631_VENDOR_ID1:
> -	case RT5631_VENDOR_ID2:
> -		return 0;
> -	default:
> +	case RT5631_RESET:
> +	case RT5631_INT_ST_IRQ_CTRL_2:
> +	case RT5631_INDEX_ADD:
> +	case RT5631_INDEX_DATA:
> +	case RT5631_EQ_CTRL:
>  		return 1;
> +	default:
> +		return 0;

This is an incremental patch against your previous driver, not a full
patch.  You should always send a patches which can be applied against
current kernels.

> -	pr_info("enter %s, syclk=%d\n", __func__, freq);
> +	dev_info(codec->dev, "enter %s, syclk=%d\n", __func__, freq);

This should be a debug level print at most, otherwise you just spam the
console.

>  	if (!freq_in || !freq_out) {
>  		dev_dbg(codec->dev, "PLL disabled\n");
> -		return -EINVAL;
> +		return 0;

I'd expect to see some sort of register write when the PLL is disabled?

>  /**
> - * rt5631_index_reg_show - sysfs file for dumping index registers of 2nd layer
> + * rt5631_index_reg_show: Show private register of rt5631 codec.
> + * @dev: device to query.
> + * @attr: device attribute.
> + * @buf: buffer to display.
> + *
> + * To show private registers which are not changed often by user.
> + * You have to access them through register 0x6a and 0x6c.
>   */
>  static ssize_t rt5631_index_reg_show(struct device *dev,
>  	struct device_attribute *attr, char *buf)

As I've told you repeatedly you should be using the standard facilities
for debug access to registers - you should not open coding this in your
driver.  Once again, ignoring review comments isn't helpful.