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

On 21.06.2013 06:46, Rajeev kumar wrote:

Daniel,

On 6/20/2013 10:59 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 Mackzonque@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.
  

• • 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;

combine in single line.

I disagree for the sake of readability.

...

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) {
  

• case 64:
  

magic number?

That's a divider value. How and why would you possibly add a #define for that?

...

• 	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); }

-static int adau1701_init(struct snd_soc_codec *codec) -{

• int ret;
• struct i2c_client *client = to_i2c_client(codec->dev);
• adau1701_reset(codec);
• ret = process_sigma_firmware(client, ADAU1701_FIRMWARE);
• if (ret) {

• dev_warn(codec->dev, "Failed to load firmware\n");
  

• return ret;
  

• }
• snd_soc_write(codec, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
• return 0;

-}

• static int adau1701_set_capture_pcm_format(struct snd_soc_codec *codec, snd_pcm_format_t format) {

@@ -291,9 +301,22 @@ static int adau1701_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) { struct snd_soc_codec *codec = dai->codec;

• struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec); snd_pcm_format_t format; unsigned int val;

• if (adau1701->sysclk) {

• unsigned int clkdiv = adau1701->sysclk / params_rate(params);
  

It will give warning.

Please elaborate.

Thanks, Daniel