Re: [PATCH 1/3] ASoC: codecs: adau1701: allow configuration of PLL mode pins

[Lars-Peter Clausen <lars@metafoo.de>]

On 06/20/2013 07:29 PM, Daniel Mack wrote:
> The ADAU1701 has 2 hardware pins to configure the PLL mode in accordance
> to the MCLK-to-LRCLK ratio. These pins have to be stable before the chip
> is released from reset, and a full reset cycle, including a new firmware
> download is needed whenever they change.
> 
> This patch adds GPIO properties to the DT bindings of the Codec, and
> implements makes the set_sysclk memorize the configured sysclk.
> 
> To avoid excessive reset cycles and firmware downloads, the default
> clock divider can be specified in DT as well. Whenever a ratio change is
> detected in the hw_params callback, the PLL mode lines are updates and a
> full reset cycle is issued.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  .../devicetree/bindings/sound/adi,adau1701.txt     |  14 +++
>  sound/soc/codecs/adau1701.c                        | 107 +++++++++++++++++----
>  sound/soc/codecs/adau1701.h                        |   4 +
>  3 files changed, 104 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/adi,adau1701.txt b/Documentation/devicetree/bindings/sound/adi,adau1701.txt
> index 3afeda7..a0d7e92 100644
> --- a/Documentation/devicetree/bindings/sound/adi,adau1701.txt
> +++ b/Documentation/devicetree/bindings/sound/adi,adau1701.txt
> @@ -11,6 +11,19 @@ Optional properties:
>   - reset-gpio: 		A GPIO spec to define which pin is connected to the
>  			chip's !RESET pin. If specified, the driver will
>  			assert a hardware reset at probe time.
> + - adi,pll-clkdiv: 	The PLL clock divider, specifing the ratio between
> +			MCLK and fsclk. The value is used to determine the
> +			correct state of the two mode pins below.
> +			Note that this value can be overridden at runtime
> +			by passing the ADAU1701_CLKDIV_MCLK_LRCLK divider
> +			with ASoC calls. However, the chips needs a full
> +			reset cycle and a new firmware download each time
> +			the configuration changes.

Considering that this is now done all automatically at runtime, do we
actually still need this property?

> + - adi,pll-mode-gpios:	An array of two GPIO specs to describe the GPIOs
> +			the ADAU's PLL config pins are connected to.
> +			The state of the pins are set according to the
> +			configured clock divider on ASoC side before the
> +			firmware is loaded.
>  
>  Examples:
>  
> @@ -19,5 +32,6 @@ Examples:
>  			compatible = "adi,adau1701";
>  			reg = <0x34>;
>  			reset-gpio = <&gpio 23 0>;
> +			adi,pll-mode-gpios = <&gpio 24 0 &gpio 25 0>;
>  		};
>  	};
> diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c
> index b6b1a77..e6ce4fe 100644
> --- a/sound/soc/codecs/adau1701.c
> +++ b/sound/soc/codecs/adau1701.c
> @@ -91,7 +91,11 @@
>  
>  struct adau1701 {
>  	int gpio_nreset;
> +	int gpio_pll_mode0;
> +	int gpio_pll_mode1;

I would have made this an array, but I guess either solution is fine.

>  	unsigned int dai_fmt;
> +	unsigned int pll_clkdiv;
> +	unsigned int sysclk;
>  };
>  
>  static const struct snd_kcontrol_new adau1701_controls[] = {
> @@ -184,13 +188,37 @@ static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg)
>  	return value;
>  }
>  
> -static void adau1701_reset(struct snd_soc_codec *codec)
> +static void adau1701_reset(struct snd_soc_codec *codec, unsigned int clkdiv)
>  {
>  	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
>  
>  	if (!gpio_is_valid(adau1701->gpio_nreset))
>  		return;
>  
> +	if (gpio_is_valid(adau1701->gpio_pll_mode0) &&
> +	    gpio_is_valid(adau1701->gpio_pll_mode1)) {
> +		switch (adau1701->pll_clkdiv) {

Considering that adau1701->pll_clkdiv is only assigned below this should
either be clkdiv, or the assigment should be moved up.

> +		case 64:
> +			gpio_set_value(adau1701->gpio_pll_mode0, 0);
> +			gpio_set_value(adau1701->gpio_pll_mode1, 0);
> +			break;
> +		case 256:
> +			gpio_set_value(adau1701->gpio_pll_mode0, 0);
> +			gpio_set_value(adau1701->gpio_pll_mode1, 1);
> +			break;
> +		case 384:
> +			gpio_set_value(adau1701->gpio_pll_mode0, 1);
> +			gpio_set_value(adau1701->gpio_pll_mode1, 0);
> +			break;
> +		case 512:
> +			gpio_set_value(adau1701->gpio_pll_mode0, 1);
> +			gpio_set_value(adau1701->gpio_pll_mode1, 1);
> +			break;
> +		}
> +	}
> +
> +	adau1701->pll_clkdiv = clkdiv;
> +
>  	gpio_set_value(adau1701->gpio_nreset, 0);
>  	/* minimum reset time is 20ns */
>  	udelay(1);
> @@ -199,24 +227,6 @@ static void adau1701_reset(struct snd_soc_codec *codec)
>  	mdelay(85);
}
[...]
> diff --git a/sound/soc/codecs/adau1701.h b/sound/soc/codecs/adau1701.h
> index 8d0949a..bdcdddc 100644
> --- a/sound/soc/codecs/adau1701.h
> +++ b/sound/soc/codecs/adau1701.h
> @@ -14,4 +14,8 @@ enum adau1701_clk_src {
>  	ADAU1701_CLK_SRC_MCLK,
>  };
>  
> +enum adau1701_clkdiv {
> +	ADAU1701_CLKDIV_MCLK_LRCLK,
> +};
> +

This is unused now.

>  #endif