Skip to contentSkip to conversation

ASoC: sta350: Add codec driver

PATCH Superseded Daniel Mack 2 participants 3 messages Mar 27, 2014

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ASoC: sta350: Add codec driver

From: Sven Brandau <[email protected]>

The TI STA350 is an integrated 2.1-channel power amplifier that is controllable over I2C. This patch adds an ASoC driver for it.

At a glance, this chip is very similar to the STA320 for which a driver already exists. In details, however, the register maps contain subtle differences which made a whole new driver easier to write and maintain.

[[email protected]: cleanups, DT property rework, rebased on asoc-next]

---
I'm sending this patch on behalf of Sven Brandau, who wrote the driver, based on earlier bits for the STA320.

In case of any pending issue, I will amend and resubmit.

Thanks,
Daniel

 .../devicetree/bindings/sound/st,sta350.txt        |  103 ++
 include/sound/sta350.h                             |   52 +
 sound/soc/codecs/Kconfig                           |    4 +
 sound/soc/codecs/Makefile                          |    2 +
 sound/soc/codecs/sta350.c                          | 1277 ++++++++++++++++++++
 sound/soc/codecs/sta350.h                          |  228 ++++
 6 files changed, 1666 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/st,sta350.txt
 create mode 100644 include/sound/sta350.h
 create mode 100644 sound/soc/codecs/sta350.c
 create mode 100644 sound/soc/codecs/sta350.h
Documentation/devicetree/bindings/sound/st,sta350.txt new file +103 -0
new file mode 100644
index 0000000..ae0630b
@@ -0,0 +1,103 @@
+STA350 audio CODEC
+
+The driver for this device only supports I2C.
+
+Required properties:
+
+  - compatible: "st,sta350"
+  - reg: the I2C address of the device for I2C
+  - reset-gpio: a GPIO spec for the reset pin. If specified, it will be
+		deasserted before communication to the codec starts.
+
+  - power-down-gpio: a GPIO spec for the power down pin. If specified,
+		     it will be deasserted before communication to the codec
+		     starts.
+
+Optional properties:
+
+  -  st,output-conf: number, Selects the output configuration:
+	0: 2-channel (full-bridge) power, 2-channel data-out
+	1: 2 (half-bridge). 1 (full-bridge) on-board power
+	2: 2 Channel (Full-Bridge) Power, 1 Channel FFX
+	3: 1 Channel Mono-Parallel
+	If parameter is missing, mode 0 will be enabled.
+
+  -  st,ch1-output-mapping: Channel 1 output mapping
+  -  st,ch2-output-mapping: Channel 2 output mapping
+  -  st,ch3-output-mapping: Channel 3 output mapping
+	0: Channel 1
+	1: Channel 2
+	2: Channel 3
+	If parameter is missing, channel 1 is choosen.
+
+  -  st,thermal-warning-recover:
+	If present, thermal warning recovery is enabled.
+
+  -  st,thermal-warning-adjustment:
+	If present, thermal warning adjustment is enabled.
+
+  -  st,fault-detect-recovery:
+	If present, then fault recovery will be enabled.
+
+  -  st,ffx-power-output-mode: string
+	The FFX power output mode selects how the FFX output timing is
+	configured. Must be one of these values:
+	  -  "drop-compensation"
+	  -  "tapered-compensation"
+	  -  "full-power-mode"
+	  -  "variable-drop-compensation" (default)
+
+  -  st,drop-compensation-ns: number
+	Only required for "st,ffx-power-output-mode" ==
+	"variable-drop-compensation".
+	Specifies the drop compensation in nanoseconds.
+	The value must be in the range of 0..300, and only
+	multiples of 20 are allowed. Default is 140ns.
+
+  -  st,overcurrent-warning-adjustment:
+	If present, overcurrent warning adjustment is enabled.
+
+  -  st,max-power-use-mpcc:
+	If present, then MPCC bits are used for MPC coefficients,
+	otherwise standard MPC coefficients are used.
+
+  -  st,max-power-corr:
+	If present, power bridge correction for THD reduction near maximum
+	power output is enabled.
+
+  -  st,am-reduction-mode:
+	If present, FFX mode runs in AM reduction mode, otherwise normal
+	FFX mode is used.
+
+  -  st,odd-pwm-speed-mode:
+	If present, PWM speed mode run on odd speed mode (341.3 kHz) on all
+	channels. If not present, normal PWM spped mode (384 kHz) will be used.
+
+  -  st,distortion-compensation:
+	If present, distortion compensation variable uses DCC coefficient.
+	If not present, preset DC coefficient is used.
+
+  -  st,invalid-input-detect-mute:
+	If not present, automatic invalid input detect mute is enabled.
+
+
+
+Example:
+
+codec: sta350@38 {
+	compatible = "st,sta350";
+	reg = <0x1c>;
+	reset-gpio = <&gpio1 19 0>;
+	power-down-gpio = <&gpio1 16 0>;
+	st,output-conf = <0x3>;			// set output to 2-channel
+						// (full-bridge) power,
+						// 2-channel data-out
+	st,ch1-output-mapping = <0>;		// set channel 1 output ch 1
+	st,ch2-output-mapping = <0>;		// set channel 2 output ch 1
+	st,ch3-output-mapping = <0>;		// set channel 3 output ch 1
+	st,max-power-correction;		// enables power bridge
+						// correction for THD reduction
+						// near maximum power output
+	st,invalid-input-detect-mute;		// mute if no valid digital
+						// audio signal is provided.
+};
include/sound/sta350.h new file +52 -0

52 lines not on this page · raw message

sound/soc/codecs/Kconfig +4 -0
index f0e8401..6211889 100644
@@ -80,6 +80,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_SSM2602_SPI if SPI_MASTER
 	select SND_SOC_SSM2602_I2C if I2C
 	select SND_SOC_STA32X if I2C
+	select SND_SOC_STA350 if I2C
 	select SND_SOC_STA529 if I2C
 	select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
 	select SND_SOC_TAS5086 if I2C
@@ -435,6 +436,9 @@ config SND_SOC_SSM2602_I2C
 config SND_SOC_STA32X
 	tristate
 
+config SND_SOC_STA350
+	tristate
+
 config SND_SOC_STA529
 	tristate
 
sound/soc/codecs/Makefile +2 -0
index 3c4d275..efdb4d0 100644
@@ -74,6 +74,7 @@ snd-soc-ssm2602-objs := ssm2602.o
 snd-soc-ssm2602-spi-objs := ssm2602-spi.o
 snd-soc-ssm2602-i2c-objs := ssm2602-i2c.o
 snd-soc-sta32x-objs := sta32x.o
+snd-soc-sta350-objs := sta350.o
 snd-soc-sta529-objs := sta529.o
 snd-soc-stac9766-objs := stac9766.o
 snd-soc-tas5086-objs := tas5086.o
@@ -221,6 +222,7 @@ obj-$(CONFIG_SND_SOC_SSM2602)	+= snd-soc-ssm2602.o
 obj-$(CONFIG_SND_SOC_SSM2602_SPI)	+= snd-soc-ssm2602-spi.o
 obj-$(CONFIG_SND_SOC_SSM2602_I2C)	+= snd-soc-ssm2602-i2c.o
 obj-$(CONFIG_SND_SOC_STA32X)   += snd-soc-sta32x.o
+obj-$(CONFIG_SND_SOC_STA350)   += snd-soc-sta350.o
 obj-$(CONFIG_SND_SOC_STA529)   += snd-soc-sta529.o
 obj-$(CONFIG_SND_SOC_STAC9766)	+= snd-soc-stac9766.o
 obj-$(CONFIG_SND_SOC_TAS5086)	+= snd-soc-tas5086.o
sound/soc/codecs/sta350.c new file +1277 -0

1,277 lines not on this page · raw message

sound/soc/codecs/sta350.h new file +228 -0

228 lines not on this page · raw message

Signature · 1 line
1.8.5.3
maintainer
#1 lore raw
Quoted 8 lines · Daniel Mack

From: Sven Brandau <[email protected]>

The TI STA350 is an integrated 2.1-channel power amplifier that is controllable over I2C. This patch adds an ASoC driver for it.

At a glance, this chip is very similar to the STA320 for which a driver already exists. In details, however, the register maps contain subtle differences which made a whole new driver easier to write and maintain.

A few issues below, some may already be present in the sta320 driver, generally newer idioms and so on, but that's no reason to not fix them here!

Quoted 6 lines
[[email protected]: cleanups, DT property rework, rebased on asoc-next]

---
I'm sending this patch on behalf of Sven Brandau, who wrote the driver, based on earlier bits for the STA320.

You need to add a Signed-off-by if you're the one sending it.

sound/soc/codecs/Kconfig config SND_SOC_SSM2602_I2C · line 441

Quoted 3 lines
sound/soc/codecs/Kconfig +3 -0
+config SND_SOC_STA350
+	tristate
+

DT capable drivers should be visible in Kconfig so they can be used with simple-audio.

sound/soc/codecs/sta350.c line 165

Quoted 6 lines
sound/soc/codecs/sta350.c new file +6 -0
+/* regulator power supply names */
+static const char const *sta350_supply_names[] = {
+	"vdd-dig",	/* digital supply, 3.3V */
+	"vdd-pll",	/* pll supply, 3.3V */
+	"vcc"		/* power amp supply, 5V - 26V */
+};

These should be documented as mandatory properties in the DT binding but weren't mentioned at all.

sound/soc/codecs/sta350.c line 216

Quoted 3 lines
sound/soc/codecs/sta350.c new file +3 -0
+static const char const *sta350_noise_shaper_type[] = {
+	"Third order", "Forth order"
+};

Fourth.

sound/soc/codecs/sta350.c line 290

Quoted 3 lines
sound/soc/codecs/sta350.c new file +3 -0
+static const struct soc_enum sta350_limiter2_release_rate_enum =
+	SOC_ENUM_SINGLE(STA350_L2AR, STA350_LxR_SHIFT,
+			16, sta350_limiter_release_rate);

Use SOC_ENUM_SINGLE_DECL if you can, it makes things a bit less error prone.

Quoted 21 lines
sound/soc/codecs/sta350.c new file +21 -0
+static int sta350_coefficient_get(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	struct sta350_priv *sta350 = snd_soc_codec_get_drvdata(codec);
+	int numcoef = kcontrol->private_value >> 16;
+	int index = kcontrol->private_value & 0xffff;
+	unsigned int cfud, val;
+	int i;
+
+	/* preserve reserved bits in STA350_CFUD */
+	regmap_read(sta350->regmap, STA350_CFUD, &cfud);
+	cfud &= 0xf0;
+	/*
+	 * chip documentation does not say if the bits are self clearing,
+	 * so do it explicitly
+	 */
+	regmap_write(sta350->regmap, STA350_CFUD, cfud);
+
+	regmap_write(sta350->regmap, STA350_CFADDR2, index);
+	if (numcoef == 1)

sound/soc/codecs/sta350.c line 339

Quoted 10 lines
sound/soc/codecs/sta350.c new file +10 -0
+		regmap_write(sta350->regmap, STA350_CFUD, cfud | 0x04);
+	else if (numcoef == 5)
+		regmap_write(sta350->regmap, STA350_CFUD, cfud | 0x08);
+	else
+		return -EINVAL;
+
+	for (i = 0; i < 3 * numcoef; i++) {
+		regmap_read(sta350->regmap, STA350_B1CF1 + i, &val);
+		ucontrol->value.bytes.data[i] = val;
+	}

You need some sort of locking around these windows I think?

Quoted 1 line

+SND_SOC_DAPM_DAC("DAC", "Playback", SND_SOC_NOPM, 0, 0),

Don't specify a stream on the DAC, use DAPM to connect the DAI to the DAC (the DAI name is a DAPM widget name).

sound/soc/codecs/sta350.c line 613

Quoted 9 lines
sound/soc/codecs/sta350.c new file +9 -0
+static int sta350_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+				 int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct sta350_priv *sta350 = snd_soc_codec_get_drvdata(codec);
+
+	pr_debug("mclk=%u\n", freq);
+	sta350->mclk = freq;
+

dev_dbg() here and for some of the rest of the code too.

Review

Quoted 1 line
(quoted diff) +1 -0
+	regmap_read(sta350->regmap, STA350_CONFB, &confb);

Or just use _update_bits() at the end?

sound/soc/codecs/sta350.c line 703

Quoted 6 lines
sound/soc/codecs/sta350.c new file +6 -0
+	for (i = 0; i < ARRAY_SIZE(interpolation_ratios); i++)
+		if (interpolation_ratios[i].fs == rate) {
+			ir = interpolation_ratios[i].ir;
+			break;
+		}
+	if (ir < 0) {

Can we have some braces around the for () loop please - it'd be easier to read (similarly for others).

sound/soc/codecs/sta350.c line 731

Quoted 9 lines
sound/soc/codecs/sta350.c new file +9 -0
+	regmap_read(sta350->regmap, STA350_CONFB, &confb);
+	confb &= ~(STA350_CONFB_SAI_MASK | STA350_CONFB_SAIFB);
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S24_LE:
+	case SNDRV_PCM_FORMAT_S24_BE:
+	case SNDRV_PCM_FORMAT_S24_3LE:
+	case SNDRV_PCM_FORMAT_S24_3BE:
+		pr_debug("24bit\n");
+		/* fall through */

Use params_width() and this all gets shorter.

sound/soc/codecs/sta350.c line 817

Quoted 13 lines
sound/soc/codecs/sta350.c new file +13 -0
+static int sta350_startup_sequence(struct sta350_priv *sta350)
+{
+	if (gpio_is_valid(sta350->gpio_power_down))
+		gpio_set_value(sta350->gpio_power_down, 1);
+
+	if (gpio_is_valid(sta350->gpio_nreset)) {
+		gpio_set_value(sta350->gpio_nreset, 1);
+		mdelay(1);
+		gpio_set_value(sta350->gpio_nreset, 0);
+		mdelay(1);
+		gpio_set_value(sta350->gpio_nreset, 1);
+		mdelay(1);
+	}

That sequence looks odd - we bring the device out of reset, reset it again and then bring it back to reset. It'd be more normal to leave the device in reset when idle and then just bring it out of reset rather than something like this. Why the odd sequence?

Quoted 7 lines
sound/soc/codecs/sta350.c new file +7 -0
+static int sta350_probe(struct snd_soc_codec *codec)
+{
+	struct sta350_priv *sta350 = snd_soc_codec_get_drvdata(codec);
+	struct sta350_platform_data *pdata = sta350->pdata;
+	int i, ret = 0, thermal = 0;
+
+	if (gpio_is_valid(sta350->gpio_nreset))

sound/soc/codecs/sta350.c line 938

Quoted 10 lines
sound/soc/codecs/sta350.c new file +10 -0
+		if (devm_gpio_request_one(codec->dev, sta350->gpio_nreset,
+					  GPIOF_OUT_INIT_HIGH,
+					  "ST350 Reset"))
+			sta350->gpio_nreset = -EINVAL;
+
+	if (gpio_is_valid(sta350->gpio_power_down))
+		if (devm_gpio_request_one(codec->dev, sta350->gpio_power_down,
+					  GPIOF_OUT_INIT_HIGH,
+					  "ST350 Power-Down"))
+			sta350->gpio_power_down = -EINVAL;

This doesn't handle probe deferral - it needs to special case -EPROBE_DEFER (or just pass back the error it gets). The driver should also be doing this at the I2C level probe not the ASoC level one.

sound/soc/codecs/sta350.c line 951

Quoted 13 lines
sound/soc/codecs/sta350.c new file +13 -0
+
+	ret = sta350_startup_sequence(sta350);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to startup device\n");
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(sta350->supplies),
+				    sta350->supplies);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}

I would have expected to run through the startup sequence after applying power rather than before otherwise the startup sequence may not be observed.

sound/soc/codecs/sta350.c line 957

Quoted 5 lines
sound/soc/codecs/sta350.c new file +5 -0
+	/*
+	 * Tell ASoC what kind of I/O to use to read the registers.  ASoC will
+	 * then do the I2C transactions itself.
+	 */
+	codec->control_data = sta350->regmap;

Shouldn't be required any more, dev_get_regmap() will DTRT.

Quoted 6 lines
sound/soc/codecs/sta350.c new file +6 -0
+	/*
+	 * Chip documentation explicitly requires that the reset values
+	 * of reserved register bits are left untouched.
+	 * Write the register default value to cache for reserved registers,
+	 * so the write to the these registers are suppressed by the cache
+	 * restore code when it skips writes of default registers.

sound/soc/codecs/sta350.c line 974

Quoted 10 lines
sound/soc/codecs/sta350.c new file +10 -0
+	 */
+	regcache_cache_only(sta350->regmap, true);
+	regmap_write(sta350->regmap, STA350_CONFC, 0xc2);
+	regmap_write(sta350->regmap, STA350_CONFE, 0xc2);
+	regmap_write(sta350->regmap, STA350_CONFF, 0x5c);
+	regmap_write(sta350->regmap, STA350_MMUTE, 0x10);
+	regmap_write(sta350->regmap, STA350_AUTO1, 0x60);
+	regmap_write(sta350->regmap, STA350_AUTO3, 0x00);
+	regmap_write(sta350->regmap, STA350_C3CFG, 0x40);
+	regcache_cache_only(sta350->regmap, false);

I don't understand this? If defaults are provided (and they are) we will already default to them so this should have no effect.

sound/soc/codecs/sta350.c line 1077

Quoted 12 lines
sound/soc/codecs/sta350.c new file +12 -0
+static int sta350_remove(struct snd_soc_codec *codec)
+{
+	struct sta350_priv *sta350 = snd_soc_codec_get_drvdata(codec);
+
+	if (gpio_is_valid(sta350->gpio_nreset))
+		gpio_set_value(sta350->gpio_nreset, 0);
+
+	if (gpio_is_valid(sta350->gpio_power_down))
+		gpio_set_value(sta350->gpio_power_down, 0);
+
+	sta350_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	regulator_bulk_disable(ARRAY_SIZE(sta350->supplies), sta350->supplies);

I'd expect the GPIO and regulator stuff to be happening over suspend too. _BIAS_OFF does the power down but not the reset.

sound/soc/codecs/sta350.c line 1221

Quoted 7 lines
sound/soc/codecs/sta350.c new file +7 -0
+#ifdef CONFIG_OF
+	if (of_match_device(st350_dt_ids, &i2c->dev)) {
+		ret = sta350_probe_dt(&i2c->dev, sta350);
+		if (ret < 0)
+			return ret;
+	}
+#endif

This is weird - normally we just go and try to parse the DT?

#2 lore raw

Hi Mark,

Quoted 6 lines · Mark Brown
Quoted 5 lines · Daniel Mack

From: Sven Brandau <[email protected]>

The TI STA350 is an integrated 2.1-channel power amplifier that is controllable over I2C. This patch adds an ASoC driver for it.

Quoted 7 lines
Quoted 3 lines

At a glance, this chip is very similar to the STA320 for which a driver already exists. In details, however, the register maps contain subtle differences which made a whole new driver easier to write and maintain.

A few issues below, some may already be present in the sta320 driver, generally newer idioms and so on, but that's no reason to not fix them here!

Thanks a lot for your review! Much appreciated.

All points are agreed upon, and are already fixed locally. Will send a v2 soon.

Daniel