Skip to contentSkip to conversation

ASoC: codecs: some more improvements for adau1701

PATCH Superseded Daniel Mack 3 participants 18 messages Jun 20, 2013 – Jun 21, 2013

Subsystems (4)
Device tree bindings Sound ASoC Analog devices inc ASoC codec drivers

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

v2 cover ASoC: codecs: some more improvements for adau1701

Sorry Lars, it's been a while since the last round.

I hope I addressed all the details you mentioned.

Thanks for having another look,
Daniel

Daniel Mack (3):
ASoC: codecs: adau1701: allow configuration of PLL mode pins
ASoC: codecs: adau1701: switch to direct regmap API usage
ASoC: codecs: adau1701: add support for pin muxing

 .../devicetree/bindings/sound/adi,adau1701.txt     |  18 ++
 sound/soc/codecs/adau1701.c                        | 250 ++++++++++++++++-----
 sound/soc/codecs/adau1701.h                        |   4 +
 3 files changed, 216 insertions(+), 56 deletions(-)
Signature · 1 line
1.8.1.4
author
#1 lore raw

PATCH v2 1/3 ASoC: codecs: adau1701: allow configuration of PLL mode pins

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 <[email protected]>
---
 .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +14 -0
index 3afeda7..a0d7e92 100644
@@ -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>;
 		};
 	};
sound/soc/codecs/adau1701.c +86 -21

183 lines not on this page · raw message

sound/soc/codecs/adau1701.h +4 -0
index 8d0949a..bdcdddc 100644
@@ -14,4 +14,8 @@ enum adau1701_clk_src {
 	ADAU1701_CLK_SRC_MCLK,
 };
 
+enum adau1701_clkdiv {
+	ADAU1701_CLKDIV_MCLK_LRCLK,
+};
+
 #endif
Signature · 1 line
1.8.1.4
author
#10 lore raw

PATCH v2 2/3 ASoC: codecs: adau1701: switch to direct regmap API usage

In reply to Daniel Mack

The hardware I/O has to be open-coded due to registers of unequal sizes. Other than that, the transition is straight forward.

Signed-off-by Daniel Mack <[email protected]>
---
 sound/soc/codecs/adau1701.c | 118 +++++++++++++++++++++++++++++++-------------
 1 file changed, 83 insertions(+), 35 deletions(-)
sound/soc/codecs/adau1701.c +83 -35
index e6ce4fe..5b83b64 100644
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
+#include <linux/regmap.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -24,16 +25,16 @@
 #include "sigmadsp.h"
 #include "adau1701.h"
 
-#define ADAU1701_DSPCTRL	0x1c
-#define ADAU1701_SEROCTL	0x1e
-#define ADAU1701_SERICTL	0x1f
+#define ADAU1701_DSPCTRL	0x081c
+#define ADAU1701_SEROCTL	0x081e
+#define ADAU1701_SERICTL	0x081f
 
-#define ADAU1701_AUXNPOW	0x22
+#define ADAU1701_AUXNPOW	0x0822
 
-#define ADAU1701_OSCIPOW	0x26
-#define ADAU1701_DACSET		0x27
+#define ADAU1701_OSCIPOW	0x0826
+#define ADAU1701_DACSET		0x0827
 
-#define ADAU1701_NUM_REGS	0x28
+#define ADAU1701_MAX_REGISTER	0x0828
 
 #define ADAU1701_DSPCTRL_CR		(1 << 2)
 #define ADAU1701_DSPCTRL_DAM		(1 << 3)

@@ -96,6 +97,7 @@ struct adau1701 {
 	unsigned int dai_fmt;
 	unsigned int pll_clkdiv;
 	unsigned int sysclk;
+	struct regmap *regmap;
 };
 
 static const struct snd_kcontrol_new adau1701_controls[] = {

@@ -127,7 +129,7 @@ static const struct snd_soc_dapm_route adau1701_dapm_routes[] = {
 	{ "ADC", NULL, "IN1" },
 };
 
-static unsigned int adau1701_register_size(struct snd_soc_codec *codec,
+static unsigned int adau1701_register_size(struct device *dev,
 		unsigned int reg)
 {
 	switch (reg) {

@@ -141,33 +143,42 @@ static unsigned int adau1701_register_size(struct snd_soc_codec *codec,
 		return 1;
 	}
 
-	dev_err(codec->dev, "Unsupported register address: %d\n", reg);
+	dev_err(dev, "Unsupported register address: %d\n", reg);
 	return 0;
 }
 
-static int adau1701_write(struct snd_soc_codec *codec, unsigned int reg,
-		unsigned int value)
+static bool adau1701_volatile_reg(struct device *dev, unsigned int reg)
 {
+	switch (reg) {
+	case ADAU1701_DACSET:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int adau1701_reg_write(void *context, unsigned int reg,
+			      unsigned int value)
+{
+	struct i2c_client *client = context;
 	unsigned int i;
 	unsigned int size;
 	uint8_t buf[4];
 	int ret;
 
-	size = adau1701_register_size(codec, reg);
+	size = adau1701_register_size(&client->dev, reg);
 	if (size == 0)
 		return -EINVAL;
 
-	snd_soc_cache_write(codec, reg, value);
-
-	buf[0] = 0x08;
-	buf[1] = reg;
+	buf[0] = reg >> 8;
+	buf[1] = reg & 0xff;
 
 	for (i = size + 1; i >= 2; --i) {
 		buf[i] = value;
 		value >>= 8;
 	}
 
-	ret = i2c_master_send(to_i2c_client(codec->dev), buf, size + 2);
+	ret = i2c_master_send(client, buf, size + 2);
 	if (ret == size + 2)
 		return 0;
 	else if (ret < 0)

@@ -176,16 +187,45 @@ static int adau1701_write(struct snd_soc_codec *codec, unsigned int reg,
 		return -EIO;
 }
 
-static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg)
+static int adau1701_reg_read(void *context, unsigned int reg,
+			     unsigned int *value)
 {
-	unsigned int value;
-	unsigned int ret;
+	int ret;
+	unsigned int i;
+	unsigned int size;
+	uint8_t send_buf[2], recv_buf[3];
+	struct i2c_client *client = context;
+	struct i2c_msg msgs[2];
+
+	size = adau1701_register_size(&client->dev, reg);
+	if (size == 0)
+		return -EINVAL;
 
-	ret = snd_soc_cache_read(codec, reg, &value);
-	if (ret)
+	send_buf[0] = reg >> 8;
+	send_buf[1] = reg & 0xff;
+
+	msgs[0].addr = client->addr;
+	msgs[0].len = sizeof(send_buf);
+	msgs[0].buf = send_buf;
+	msgs[0].flags = 0;
+
+	msgs[1].addr = client->addr;
+	msgs[1].len = size;
+	msgs[1].buf = recv_buf;
+	msgs[1].flags = I2C_M_RD | I2C_M_STOP;
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
 		return ret;
+	else if (ret != ARRAY_SIZE(msgs))
+		return -EIO;
 
-	return value;
+	*value = 0;
+
+	for (i = 0; i < size; i++)
+		*value |= recv_buf[i] << (i * 8);
+
+	return 0;
 }
 
 static void adau1701_reset(struct snd_soc_codec *codec, unsigned int clkdiv)

@@ -407,8 +447,8 @@ static int adau1701_set_dai_fmt(struct snd_soc_dai *codec_dai,
 
 	adau1701->dai_fmt = fmt & SND_SOC_DAIFMT_FORMAT_MASK;
 
-	snd_soc_write(codec, ADAU1701_SERICTL, serictl);
-	snd_soc_update_bits(codec, ADAU1701_SEROCTL,
+	regmap_write(adau1701->regmap, ADAU1701_SERICTL, serictl);
+	regmap_update_bits(adau1701->regmap, ADAU1701_SEROCTL,
 		~ADAU1701_SEROCTL_WORD_LEN_MASK, seroctl);
 
 	return 0;

@@ -523,8 +563,6 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
 
-	codec->control_data = to_i2c_client(codec->dev);
-
 	/* initalize with pre-configured pll mode settings */
 	adau1701_reset(codec, adau1701->pll_clkdiv);
 

@@ -534,8 +572,9 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 		return ret;
 	}
 
-	snd_soc_write(codec, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
-	snd_soc_write(codec, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
+	regcache_sync(adau1701->regmap);
+	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
+	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
 
 	return 0;
 }

@@ -545,9 +584,6 @@ static struct snd_soc_codec_driver adau1701_codec_drv = {
 	.set_bias_level		= adau1701_set_bias_level,
 	.idle_bias_off		= true,
 
-	.reg_cache_size		= ADAU1701_NUM_REGS,
-	.reg_word_size		= sizeof(u16),
-
 	.controls		= adau1701_controls,
 	.num_controls		= ARRAY_SIZE(adau1701_controls),
 	.dapm_widgets		= adau1701_dapm_widgets,

@@ -555,12 +591,19 @@ static struct snd_soc_codec_driver adau1701_codec_drv = {
 	.dapm_routes		= adau1701_dapm_routes,
 	.num_dapm_routes	= ARRAY_SIZE(adau1701_dapm_routes),
 
-	.write			= adau1701_write,
-	.read			= adau1701_read,
-
 	.set_sysclk		= adau1701_set_sysclk,
 };
 
+static const struct regmap_config adau1701_regmap = {
+	.reg_bits		= 16,
+	.val_bits		= 32,
+	.max_register		= ADAU1701_MAX_REGISTER,
+	.cache_type		= REGCACHE_RBTREE,
+	.volatile_reg		= adau1701_volatile_reg,
+	.reg_write		= adau1701_reg_write,
+	.reg_read		= adau1701_reg_read,
+};
+
 static int adau1701_i2c_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {

@@ -575,6 +618,11 @@ static int adau1701_i2c_probe(struct i2c_client *client,
 	if (!adau1701)
 		return -ENOMEM;
 
+	adau1701->regmap = devm_regmap_init(dev, NULL, client,
+					    &adau1701_regmap);
+	if (IS_ERR(adau1701->regmap))
+		return PTR_ERR(adau1701->regmap);
+
 	if (dev->of_node) {
 		gpio_nreset = of_get_named_gpio(dev->of_node, "reset-gpio", 0);
 		if (gpio_nreset < 0 && gpio_nreset != -ENOENT)
Signature · 1 line
1.8.1.4
author
#13 lore raw

PATCH v2 3/3 ASoC: codecs: adau1701: add support for pin muxing

In reply to Daniel Mack

The ADAU1701 has 12 pins that can be configured depending on the system configuration. Allow settting the corresponding registers from DT.

Signed-off-by Daniel Mack <[email protected]>
---
 .../devicetree/bindings/sound/adi,adau1701.txt     |  6 +++++
 sound/soc/codecs/adau1701.c                        | 29 ++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +6 -0
index a0d7e92..7391698 100644
@@ -24,6 +24,10 @@ Optional properties:
 			The state of the pins are set according to the
 			configured clock divider on ASoC side before the
 			firmware is loaded.
+ - adi,pin-config:	An array of 12 numerical values selecting one of the
+			pin configurations as described in the datasheet,
+			table 53. Note that the value of this property has
+			to be prefixed with '/bits/ 8'.
 
 Examples:
 

@@ -33,5 +37,7 @@ Examples:
 			reg = <0x34>;
 			reset-gpio = <&gpio 23 0>;
 			adi,pll-mode-gpios = <&gpio 24 0 &gpio 25 0>;
+			adi,pin-config = /bits/ 8 <0x4 0x7 0x5 0x5 0x4 0x4
+                                                           0x4 0x4 0x4 0x4 0x4 0x4>;
 		};
 	};
sound/soc/codecs/adau1701.c +27 -2
index 5b83b64..b938e57 100644
@@ -30,6 +30,9 @@
 #define ADAU1701_SERICTL	0x081f
 
 #define ADAU1701_AUXNPOW	0x0822
+#define ADAU1701_PINCONF_0	0x0820
+#define ADAU1701_PINCONF_1	0x0821
+#define ADAU1701_AUXNPOW	0x0822
 
 #define ADAU1701_OSCIPOW	0x0826
 #define ADAU1701_DACSET		0x0827

@@ -98,6 +101,7 @@ struct adau1701 {
 	unsigned int pll_clkdiv;
 	unsigned int sysclk;
 	struct regmap *regmap;
+	u8 pin_config[12];
 };
 
 static const struct snd_kcontrol_new adau1701_controls[] = {

@@ -133,6 +137,9 @@ static unsigned int adau1701_register_size(struct device *dev,
 		unsigned int reg)
 {
 	switch (reg) {
+	case ADAU1701_PINCONF_0:
+	case ADAU1701_PINCONF_1:
+		return 3;
 	case ADAU1701_DSPCTRL:
 	case ADAU1701_SEROCTL:
 	case ADAU1701_AUXNPOW:

@@ -163,7 +170,7 @@ static int adau1701_reg_write(void *context, unsigned int reg,
 	struct i2c_client *client = context;
 	unsigned int i;
 	unsigned int size;
-	uint8_t buf[4];
+	uint8_t buf[5];
 	int ret;
 
 	size = adau1701_register_size(&client->dev, reg);

@@ -559,7 +566,8 @@ MODULE_DEVICE_TABLE(of, adau1701_dt_ids);
 
 static int adau1701_probe(struct snd_soc_codec *codec)
 {
-	int ret;
+	int ret, i;
+	unsigned int val;
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
 

@@ -576,6 +584,19 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
 	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
 
+	/* set up pin config */
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_0, val);
+
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i + 6] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_1, val);
+
 	return 0;
 }
 

@@ -640,6 +661,10 @@ static int adau1701_i2c_probe(struct i2c_client *client,
 
 		of_property_read_u32(dev->of_node, "adi,pll-clkdiv",
 				     &adau1701->pll_clkdiv);
+
+		of_property_read_u8_array(dev->of_node, "adi,pin-config",
+					  adau1701->pin_config,
+					  ARRAY_SIZE(adau1701->pin_config));
 	}
 
 	if (gpio_is_valid(gpio_nreset)) {
Signature · 1 line
1.8.1.4
#2 lore raw

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

In reply to Daniel Mack

Daniel,

Quoted 56 lines · Daniel Mack

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<[email protected]>
---
  .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +14 -0
index 3afeda7..a0d7e92 100644
@@ -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>;
  		};
  	};
sound/soc/codecs/adau1701.c +0 -0
index b6b1a77..e6ce4fe 100644

sound/soc/codecs/adau1701.c line 95

Quoted 5 lines
sound/soc/codecs/adau1701.c +2 -0
@@ -91,7 +91,11 @@
 
  struct adau1701 {
  	int gpio_nreset;
+	int gpio_pll_mode0;
+	int gpio_pll_mode1;

combine in single line.

Quoted 12 lines
sound/soc/codecs/adau1701.c +3 -1
  	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)

sound/soc/codecs/adau1701.c static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg) · line 201

Quoted 10 lines
sound/soc/codecs/adau1701.c +4 -0
  {
  	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?

Quoted 49 lines
sound/soc/codecs/adau1701.c +20 -18
+			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)
  {

sound/soc/codecs/adau1701.c static int adau1701_hw_params(struct snd_pcm_substream *substream, · line 309

Quoted 9 lines
sound/soc/codecs/adau1701.c +3 -0
@@ -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.

Best Regards
Rajeev

Quoted 120 lines
sound/soc/codecs/adau1701.c +54 -2
+
+		/*
+		 * If the mclk/lrclk ratio changes, the chip needs updated PLL
+		 * mode GPIO settings, and a full reset cycle, including a new
+		 * firmware upload.
+		 */
+		if (clkdiv != adau1701->pll_clkdiv)
+			adau1701_reset(codec, clkdiv);
+	}
+
  	switch (params_rate(params)) {
  	case 192000:
  		val = ADAU1701_DSPCTRL_SR_192;

@@ -435,6 +458,7 @@ static int adau1701_set_sysclk(struct snd_soc_codec *codec, int clk_id,
  	int source, unsigned int freq, int dir)
  {
  	unsigned int val;
+	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
 
  	switch (clk_id) {
  	case ADAU1701_CLK_SRC_OSC:

@@ -448,6 +472,7 @@ static int adau1701_set_sysclk(struct snd_soc_codec *codec, int clk_id,
  	}
 
  	snd_soc_update_bits(codec, ADAU1701_OSCIPOW, ADAU1701_OSCIPOW_OPD, val);
+	adau1701->sysclk = freq;
 
  	return 0;
  }

@@ -495,13 +520,21 @@ MODULE_DEVICE_TABLE(of, adau1701_dt_ids);
  static int adau1701_probe(struct snd_soc_codec *codec)
  {
  	int ret;
+	struct i2c_client *client = to_i2c_client(codec->dev);
+	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
 
  	codec->control_data = to_i2c_client(codec->dev);
 
-	ret = adau1701_init(codec);
-	if (ret)
+	/* initalize with pre-configured pll mode settings */
+	adau1701_reset(codec, adau1701->pll_clkdiv);
+
+	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);
  	snd_soc_write(codec, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
 
  	return 0;

@@ -534,6 +567,8 @@ static int adau1701_i2c_probe(struct i2c_client *client,
  	struct adau1701 *adau1701;
  	struct device *dev =&client->dev;
  	int gpio_nreset = -EINVAL;
+	int gpio_pll_mode0 = -EINVAL;
+	int gpio_pll_mode1 = -EINVAL;
  	int ret;
 
  	adau1701 = devm_kzalloc(dev, sizeof(*adau1701), GFP_KERNEL);

@@ -544,6 +579,19 @@ static int adau1701_i2c_probe(struct i2c_client *client,
  		gpio_nreset = of_get_named_gpio(dev->of_node, "reset-gpio", 0);
  		if (gpio_nreset<  0&&  gpio_nreset != -ENOENT)
  			return gpio_nreset;
+
+		gpio_pll_mode0 = of_get_named_gpio(dev->of_node,
+						   "adi,pll-mode-gpios", 0);
+		if (gpio_pll_mode0<  0&&  gpio_pll_mode0 != -ENOENT)
+			return gpio_pll_mode0;
+
+		gpio_pll_mode1 = of_get_named_gpio(dev->of_node,
+						   "adi,pll-mode-gpios", 1);
+		if (gpio_pll_mode1<  0&&  gpio_pll_mode1 != -ENOENT)
+			return gpio_pll_mode1;
+
+		of_property_read_u32(dev->of_node, "adi,pll-clkdiv",
+				&adau1701->pll_clkdiv);
  	}
 
  	if (gpio_is_valid(gpio_nreset)) {

@@ -553,7 +601,24 @@ static int adau1701_i2c_probe(struct i2c_client *client,
  			return ret;
  	}
 
+	if (gpio_is_valid(gpio_pll_mode0)&&
+	    gpio_is_valid(gpio_pll_mode1)) {
+		ret = devm_gpio_request_one(dev, gpio_pll_mode0,
+					    GPIOF_OUT_INIT_LOW,
+					    "ADAU1701 PLL mode 0");
+		if (ret<  0)
+			return ret;
+
+		ret = devm_gpio_request_one(dev, gpio_pll_mode1,
+					    GPIOF_OUT_INIT_LOW,
+					    "ADAU1701 PLL mode 1");
+		if (ret<  0)
+			return ret;
+	}
+
  	adau1701->gpio_nreset = gpio_nreset;
+	adau1701->gpio_pll_mode0 = gpio_pll_mode0;
+	adau1701->gpio_pll_mode1 = gpio_pll_mode1;
 
  	i2c_set_clientdata(client, adau1701);
  	ret = snd_soc_register_codec(&client->dev,&adau1701_codec_drv,
sound/soc/codecs/adau1701.h +4 -0
index 8d0949a..bdcdddc 100644
@@ -14,4 +14,8 @@ enum adau1701_clk_src {
  	ADAU1701_CLK_SRC_MCLK,
  };
 
+enum adau1701_clkdiv {
+	ADAU1701_CLKDIV_MCLK_LRCLK,
+};
+
  #endif
author
#3 lore raw

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

Quoted 60 lines · Rajeev kumar

Daniel,

Quoted 57 lines · Daniel Mack

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<[email protected]>
---
  .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +14 -0
index 3afeda7..a0d7e92 100644
@@ -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>;
  		};
  	};
sound/soc/codecs/adau1701.c +0 -0
index b6b1a77..e6ce4fe 100644
@@ -91,7 +91,11 @@
Quoted 6 lines

sound/soc/codecs/adau1701.c line 95

Quoted 4 lines
sound/soc/codecs/adau1701.c +2 -0
  struct adau1701 {
  	int gpio_nreset;
+	int gpio_pll_mode0;
+	int gpio_pll_mode1;

combine in single line.

I disagree for the sake of readability.

Quoted 19 lines
Quoted 19 lines
sound/soc/codecs/adau1701.c +3 -1
  	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;
Quoted 6 lines

sound/soc/codecs/adau1701.c static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg) · line 201

Quoted 4 lines
sound/soc/codecs/adau1701.c +4 -0
+	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?

Quoted 57 lines
Quoted 57 lines
sound/soc/codecs/adau1701.c +21 -18
+			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;
Quoted 4 lines

sound/soc/codecs/adau1701.c static int adau1701_hw_params(struct snd_pcm_substream *substream, · line 309

Quoted 2 lines
sound/soc/codecs/adau1701.c +2 -0
+	if (adau1701->sysclk) {
+		unsigned int clkdiv = adau1701->sysclk / params_rate(params);

It will give warning.

Please elaborate.

Thanks,
Daniel

maintainer
#7 lore raw

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

In reply to Daniel Mack
Quoted 26 lines · Daniel Mack

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 <[email protected]>
---
 .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +0 -0
index 3afeda7..a0d7e92 100644
@@ -11,6 +11,19 @@ Optional properties:
  - reset-gpio: 		A GPIO spec to define which pin is connected to the

Documentation/devicetree/bindings/sound/adi,adau1701.txt Optional properties: · line 21

Quoted 10 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +8 -0
 			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?

Quoted 20 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +6 -0
+ - 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>;
 		};
 	};
sound/soc/codecs/adau1701.c +0 -0
index b6b1a77..e6ce4fe 100644

sound/soc/codecs/adau1701.c line 95

Quoted 5 lines
sound/soc/codecs/adau1701.c +2 -0
@@ -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.

Quoted 11 lines
sound/soc/codecs/adau1701.c +2 -1
 	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)

sound/soc/codecs/adau1701.c static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg) · line 200

Quoted 10 lines
sound/soc/codecs/adau1701.c +4 -0
+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.

Quoted 25 lines
sound/soc/codecs/adau1701.c +21 -0
+		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);

sound/soc/codecs/adau1701.c static void adau1701_reset(struct snd_soc_codec *codec) · line 227

Quoted 1 line
sound/soc/codecs/adau1701.c +0 -0
@@ -199,24 +227,6 @@ static void adau1701_reset(struct snd_soc_codec *codec)
 	mdelay(85);

}
[...]

sound/soc/codecs/adau1701.h enum adau1701_clk_src { · line 20

Quoted 12 lines
sound/soc/codecs/adau1701.h +4 -0
index 8d0949a..bdcdddc 100644
@@ -14,4 +14,8 @@ enum adau1701_clk_src {
 	ADAU1701_CLK_SRC_MCLK,
 };
 
+enum adau1701_clkdiv {
+	ADAU1701_CLKDIV_MCLK_LRCLK,
+};
+

This is unused now.

Quoted 1 line
sound/soc/codecs/adau1701.h +0 -0
 #endif
maintainer
#14 lore raw

Re: [PATCH 3/3] ASoC: codecs: adau1701: add support for pin muxing

In reply to Daniel Mack
Quoted 15 lines · Daniel Mack

[...]
static int adau1701_probe(struct snd_soc_codec *codec) {

sound/soc/codecs/adau1701.c +4 -1
-	int ret;
+	int ret, i;
+	unsigned int val;
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);

@@ -576,6 +584,19 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
 	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
 
+	/* set up pin config */
+	val = 0;

sound/soc/codecs/adau1701.c static int adau1701_probe(struct snd_soc_codec *codec) · line 598

Quoted 10 lines
sound/soc/codecs/adau1701.c +10 -0
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_0, val);
+
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i + 6] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_1, val);

Will the config survive a reset?

author
#15 lore raw

Re: [PATCH 3/3] ASoC: codecs: adau1701: add support for pin muxing

Quoted 14 lines · Lars-Peter Clausen
Quoted 13 lines · Daniel Mack

[...]
static int adau1701_probe(struct snd_soc_codec *codec) {

sound/soc/codecs/adau1701.c +2 -1
-	int ret;
+	int ret, i;
+	unsigned int val;
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);

@@ -576,6 +584,19 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
 	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
Quoted 15 lines

sound/soc/codecs/adau1701.c static int adau1701_probe(struct snd_soc_codec *codec) · line 598

Quoted 12 lines
sound/soc/codecs/adau1701.c +12 -0
+	/* set up pin config */
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_0, val);
+
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i + 6] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_1, val);

Will the config survive a reset?

That will be synced back via regcache_sync(), no?

maintainer Acked-by on v2 2/3
#11 lore raw

Re: [PATCH 2/3] ASoC: codecs: adau1701: switch to direct regmap API usage

In reply to Daniel Mack
Quoted 4 lines · Daniel Mack

The hardware I/O has to be open-coded due to registers of unequal sizes. Other than that, the transition is straight forward.

Signed-off-by Daniel Mack <[email protected]>
Acked-by Lars-Peter Clausen <[email protected]>

Thanks. One minor nitpick though.

Quoted 1 line

---

[...]

sound/soc/codecs/adau1701.c static int adau1701_write(struct snd_soc_codec *codec, unsigned int reg, · line 215

Quoted 9 lines
sound/soc/codecs/adau1701.c +9 -0
+	msgs[0].addr = client->addr;
+	msgs[0].len = sizeof(send_buf);
+	msgs[0].buf = send_buf;
+	msgs[0].flags = 0;
+
+	msgs[1].addr = client->addr;
+	msgs[1].len = size;
+	msgs[1].buf = recv_buf;
+	msgs[1].flags = I2C_M_RD | I2C_M_STOP;

We don't need to set a explicit stop flag for the last message in the transfer.

author
#8 lore raw

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

In reply to Lars-Peter Clausen
Quoted 32 lines · Lars-Peter Clausen
Quoted 31 lines · Daniel Mack

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 <[email protected]>
---
 .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +3 -0
index 3afeda7..a0d7e92 100644
@@ -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.
Quoted 8 lines

Documentation/devicetree/bindings/sound/adi,adau1701.txt Optional properties: · line 21

Quoted 5 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +5 -0
+			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?

Yes, I noticed that as well, but given that changing the PLL mode setup is expensive, and because we need some sort of firmware to start up at least, I wanted to provide a way to set these values up-front, so systems with stable clock dividers can minimize the number of firmware transfers.

Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +1 -1
-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;
Quoted 6 lines

Documentation/devicetree/bindings/sound/adi,adau1701.txt

Quoted 3 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +3 -0
+	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.

Very true. It didn't hit me as in my setup, the PLL mode is passed in via DT.

author
#12 lore raw

Re: [PATCH 2/3] ASoC: codecs: adau1701: switch to direct regmap API usage

In reply to Lars-Peter Clausen
Quoted 12 lines · Lars-Peter Clausen
Quoted 4 lines · Daniel Mack

The hardware I/O has to be open-coded due to registers of unequal sizes. Other than that, the transition is straight forward.

Signed-off-by Daniel Mack <[email protected]>
Acked-by Lars-Peter Clausen <[email protected]>

Thanks. One minor nitpick though.

Quoted 1 line

---

[...]

Quoted 12 lines

sound/soc/codecs/adau1701.c static int adau1701_write(struct snd_soc_codec *codec, unsigned int reg, · line 215

Quoted 9 lines
sound/soc/codecs/adau1701.c +9 -0
+	msgs[0].addr = client->addr;
+	msgs[0].len = sizeof(send_buf);
+	msgs[0].buf = send_buf;
+	msgs[0].flags = 0;
+
+	msgs[1].addr = client->addr;
+	msgs[1].len = size;
+	msgs[1].buf = recv_buf;
+	msgs[1].flags = I2C_M_RD | I2C_M_STOP;

We don't need to set a explicit stop flag for the last message in the transfer.

Right. I'll send a v3 of the entire series. Hang on.

maintainer
#16 lore raw

Re: [PATCH 3/3] ASoC: codecs: adau1701: add support for pin muxing

In reply to Daniel Mack
Quoted 31 lines · Daniel Mack
Quoted 14 lines · Lars-Peter Clausen
Quoted 13 lines · Daniel Mack

[...]
static int adau1701_probe(struct snd_soc_codec *codec) {

sound/soc/codecs/adau1701.c +2 -1
-	int ret;
+	int ret, i;
+	unsigned int val;
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);

@@ -576,6 +584,19 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
 	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
Quoted 15 lines

sound/soc/codecs/adau1701.c static int adau1701_probe(struct snd_soc_codec *codec) · line 598

Quoted 12 lines
sound/soc/codecs/adau1701.c +12 -0
+	/* set up pin config */
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_0, val);
+
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i + 6] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_1, val);

Will the config survive a reset?

Quoted 2 lines

That will be synced back via regcache_sync(), no?

Oh, sorry I missed the regcache_sync(), but you still need to call regcache_mark_dirty() first, otherwise regcache_sync() won't do anything.

- Lars

maintainer
#17 lore raw

Re: [PATCH 3/3] ASoC: codecs: adau1701: add support for pin muxing

Quoted 32 lines · Lars-Peter Clausen
Quoted 31 lines · Daniel Mack
Quoted 29 lines · Lars-Peter Clausen
Quoted 15 lines · Daniel Mack

[...]
static int adau1701_probe(struct snd_soc_codec *codec) {

sound/soc/codecs/adau1701.c +4 -1
-	int ret;
+	int ret, i;
+	unsigned int val;
 	struct i2c_client *client = to_i2c_client(codec->dev);
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);

@@ -576,6 +584,19 @@ static int adau1701_probe(struct snd_soc_codec *codec)
 	regmap_write(adau1701->regmap, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
 	regmap_write(adau1701->regmap, ADAU1701_DSPCTRL, ADAU1701_DSPCTRL_CR);
 
+	/* set up pin config */
+	val = 0;

sound/soc/codecs/adau1701.c static int adau1701_probe(struct snd_soc_codec *codec) · line 598

Quoted 10 lines
sound/soc/codecs/adau1701.c +10 -0
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_0, val);
+
+	val = 0;
+	for (i = 0; i < 6; i++)
+		val |= adau1701->pin_config[i + 6] << (i * 4);
+
+	regmap_write(adau1701->regmap, ADAU1701_PINCONF_1, val);

Will the config survive a reset?

Quoted 4 lines
Quoted 2 lines

That will be synced back via regcache_sync(), no?

Oh, sorry I missed the regcache_sync(), but you still need to call regcache_mark_dirty() first, otherwise regcache_sync() won't do anything.

wait... the regcache_sync() is only in adau1761_probe(), but not in adau1761_reset().

maintainer
#9 lore raw

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

In reply to Daniel Mack
Quoted 42 lines · Daniel Mack
Quoted 32 lines · Lars-Peter Clausen
Quoted 31 lines · Daniel Mack

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 <[email protected]>
---
 .../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(-)
Documentation/devicetree/bindings/sound/adi,adau1701.txt +3 -0
index 3afeda7..a0d7e92 100644
@@ -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.
Quoted 8 lines

Documentation/devicetree/bindings/sound/adi,adau1701.txt Optional properties: · line 21

Quoted 5 lines
Documentation/devicetree/bindings/sound/adi,adau1701.txt +5 -0
+			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?

Quoted 5 lines

Yes, I noticed that as well, but given that changing the PLL mode setup is expensive, and because we need some sort of firmware to start up at least, I wanted to provide a way to set these values up-front, so systems with stable clock dividers can minimize the number of firmware transfers.

Ok, fair enough, but the documentation for the property needs to be updated.

- Lars

#4 lore raw

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

In reply to Daniel Mack
Quoted 10 lines · Daniel Mack
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
    	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.

you are declaring something (unsigned int clkdiv) in between code.

~Rajeev

author
#5 lore raw

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

Quoted 6 lines · Rajeev kumar
Quoted 5 lines · Daniel Mack
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
    	struct snd_soc_codec *codec = dai->codec;
 +	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
    	snd_pcm_format_t format;
    	unsigned int val;
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

+ if (adau1701->sysclk) {
+ unsigned int clkdiv = adau1701->sysclk / params_rate(params);

It will give warning.

Please elaborate.

you are declaring something (unsigned int clkdiv) in between code.

It's not in between code, it's at the beginning of a new block ('{'). That's totally fine and done everywhere else in the kernel.

Daniel

maintainer
#6 lore raw

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

In reply to Rajeev kumar
Quoted 6 lines · Rajeev kumar
Quoted 5 lines · Daniel Mack
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
        struct snd_soc_codec *codec = dai->codec;
 +    struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
        snd_pcm_format_t format;
        unsigned int val;
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

+ if (adau1701->sysclk) {
+ unsigned int clkdiv = adau1701->sysclk / params_rate(params);

It will give warning.

Please elaborate.

you are declaring something (unsigned int clkdiv) in between code.

It's at the beginning at a block, that's actually fine. Neither checkpatch or gcc will generate a warning.