Re: [PATCH] ASoC: Add driver for rt5631

Re: [PATCH] ASoC: Add driver for rt5631

[Mark Brown]

On Wed, Sep 07, 2011 at 11:16:35AM +0800, johnnyhsu@realtek.com wrote:
> From d291cbcdd5c1faebd4c37b4564b33c322df51987 Mon Sep 17 00:00:00 2001
> From: Johnny Hsu <johnnyhsu@realtek.com>
> Date: Wed, 7 Sep 2011 11:05:19 +0800
> Subject: [PATCH] ASoC: Add driver for rt5631

Applied, thanks.  It'd be really helpful if where things aren't clear
you could reply to code reviews - now that it's clear what the index
register stuff is doing I suspect the sysfs stuff would be OK though it
ought to go in debugfs not sysfs.

Re: [PATCH] ASoC: Add driver for rt5631

[Mark Brown]

On Thu, Sep 22, 2011 at 09:48:55AM +0800, johnnyhsu wrote:

> Besides, could you show me the hint or address where I can post questions
> about driver? I wonder how to work between modem and codec and listen to
> others' experience.

The alsa-devel list is the place for discussions like that.

Re: [PATCH] ASoC: Add driver for rt5631

[Mark Brown]

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.