[alsa-devel] [PATCH 1/4] ASoC: codecs: tas5720: add basic support for TAS5722 devices
From: Andreas Dannenberg dannenberg@ti.com
The TI TAS5722 digital amplifier is very similar to the TAS5720 from an overall and register map perspective. Therefore the existing driver can be extended easily to support this additional device. This commit allows TAS5722 devices to be used in a "subset" type of fashion, without exposing any of the additional features they offer.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com --- .../devicetree/bindings/sound/tas5720.txt | 4 ++- sound/soc/codecs/tas5720.c | 38 ++++++++++++++++++---- sound/soc/codecs/tas5720.h | 1 + 3 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/tas5720.txt b/Documentation/devicetree/bindings/sound/tas5720.txt index 40d94f82beb3..7481653fe8e3 100644 --- a/Documentation/devicetree/bindings/sound/tas5720.txt +++ b/Documentation/devicetree/bindings/sound/tas5720.txt @@ -6,10 +6,12 @@ audio playback. For more product information please see the links below:
http://www.ti.com/product/TAS5720L http://www.ti.com/product/TAS5720M +http://www.ti.com/product/TAS5722L
Required properties:
-- compatible : "ti,tas5720" +- compatible : "ti,tas5720", + "ti,tas5722" - reg : I2C slave address - dvdd-supply : phandle to a 3.3-V supply for the digital circuitry - pvdd-supply : phandle to a supply used for the Class-D amp and the analog diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index a736a2a6976c..5def54d1336d 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -36,6 +36,11 @@ /* Define how often to check (and clear) the fault status register (in ms) */ #define TAS5720_FAULT_CHECK_INTERVAL 200
+enum tas572x_type { + TAS5720, + TAS5722, +}; + static const char * const tas5720_supply_names[] = { "dvdd", /* Digital power supply. Connect to 3.3-V supply. */ "pvdd", /* Class-D amp and analog power supply (connected). */ @@ -47,6 +52,7 @@ struct tas5720_data { struct snd_soc_codec *codec; struct regmap *regmap; struct i2c_client *tas5720_client; + enum tas572x_type devtype; struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES]; struct delayed_work fault_check_work; unsigned int last_fault; @@ -264,7 +270,7 @@ static void tas5720_fault_check_work(struct work_struct *work) static int tas5720_codec_probe(struct snd_soc_codec *codec) { struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec); - unsigned int device_id; + unsigned int device_id, expected_device_id; int ret;
tas5720->codec = codec; @@ -276,6 +282,11 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) return ret; }
+ /* + * Take a liberal approach to checking the device ID to allow the + * driver to be used even if the device ID does not match, however + * issue a warning if there is a mismatch. + */ ret = regmap_read(tas5720->regmap, TAS5720_DEVICE_ID_REG, &device_id); if (ret < 0) { dev_err(codec->dev, "failed to read device ID register: %d\n", @@ -283,13 +294,22 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) goto probe_fail; }
- if (device_id != TAS5720_DEVICE_ID) { - dev_err(codec->dev, "wrong device ID. expected: %u read: %u\n", - TAS5720_DEVICE_ID, device_id); - ret = -ENODEV; - goto probe_fail; + switch (tas5720->devtype) { + case TAS5720: + expected_device_id = TAS5720_DEVICE_ID; + break; + case TAS5722: + expected_device_id = TAS5722_DEVICE_ID; + break; + default: + dev_err(codec->dev, "unexpected private driver data\n"); + return -EINVAL; }
+ if (device_id != expected_device_id) + dev_warn(codec->dev, "wrong device ID. expected: %u read: %u\n", + expected_device_id, device_id); + /* Set device to mute */ ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG, TAS5720_MUTE, TAS5720_MUTE); @@ -552,6 +572,8 @@ static int tas5720_probe(struct i2c_client *client, return -ENOMEM;
data->tas5720_client = client; + data->devtype = id->driver_data; + data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config); if (IS_ERR(data->regmap)) { ret = PTR_ERR(data->regmap); @@ -592,7 +614,8 @@ static int tas5720_remove(struct i2c_client *client) }
static const struct i2c_device_id tas5720_id[] = { - { "tas5720", 0 }, + { "tas5720", TAS5720 }, + { "tas5722", TAS5722 }, { } }; MODULE_DEVICE_TABLE(i2c, tas5720_id); @@ -600,6 +623,7 @@ MODULE_DEVICE_TABLE(i2c, tas5720_id); #if IS_ENABLED(CONFIG_OF) static const struct of_device_id tas5720_of_match[] = { { .compatible = "ti,tas5720", }, + { .compatible = "ti,tas5722", }, { }, }; MODULE_DEVICE_TABLE(of, tas5720_of_match); diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h index 3d077c779b12..bef802afcc69 100644 --- a/sound/soc/codecs/tas5720.h +++ b/sound/soc/codecs/tas5720.h @@ -32,6 +32,7 @@
/* TAS5720_DEVICE_ID_REG */ #define TAS5720_DEVICE_ID 0x01 +#define TAS5722_DEVICE_ID 0x12
/* TAS5720_POWER_CTRL_REG */ #define TAS5720_DIG_CLIP_MASK GENMASK(7, 2)
From: Andreas Dannenberg dannenberg@ti.com
Introduce a custom super-set register map and associated bit definitions to allow driver access to all TAS5722 device functionality.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com --- sound/soc/codecs/tas5720.c | 23 ++++++++++++++++++++++- sound/soc/codecs/tas5720.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index 5def54d1336d..f3006f301fe8 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -466,6 +466,15 @@ static const struct regmap_config tas5720_regmap_config = { .volatile_reg = tas5720_is_volatile_reg, };
+static const struct regmap_config tas5722_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = TAS5722_MAX_REG, + .cache_type = REGCACHE_RBTREE, + .volatile_reg = tas5720_is_volatile_reg, +}; + /* * DAC analog gain. There are four discrete values to select from, ranging * from 19.2 dB to 26.3dB. @@ -564,6 +573,7 @@ static int tas5720_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct tas5720_data *data; + const struct regmap_config *regmap_config; int ret; int i;
@@ -574,7 +584,18 @@ static int tas5720_probe(struct i2c_client *client, data->tas5720_client = client; data->devtype = id->driver_data;
- data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config); + switch (id->driver_data) { + case TAS5720: + regmap_config = &tas5720_regmap_config; + break; + case TAS5722: + regmap_config = &tas5722_regmap_config; + break; + default: + dev_err(dev, "unexpected private driver data\n"); + return -EINVAL; + } + data->regmap = devm_regmap_init_i2c(client, regmap_config); if (IS_ERR(data->regmap)) { ret = PTR_ERR(data->regmap); dev_err(dev, "failed to allocate register map: %d\n", ret); diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h index bef802afcc69..1dda3095961d 100644 --- a/sound/soc/codecs/tas5720.h +++ b/sound/soc/codecs/tas5720.h @@ -30,6 +30,11 @@ #define TAS5720_DIGITAL_CLIP1_REG 0x11 #define TAS5720_MAX_REG TAS5720_DIGITAL_CLIP1_REG
+/* Additional TAS5722-specific Registers */ +#define TAS5722_DIGITAL_CTRL2_REG 0x13 +#define TAS5722_ANALOG_CTRL2_REG 0x14 +#define TAS5722_MAX_REG TAS5722_ANALOG_CTRL2_REG + /* TAS5720_DEVICE_ID_REG */ #define TAS5720_DEVICE_ID 0x01 #define TAS5722_DEVICE_ID 0x12 @@ -52,6 +57,7 @@ #define TAS5720_SAIF_FORMAT_MASK GENMASK(2, 0)
/* TAS5720_DIGITAL_CTRL2_REG */ +#define TAS5722_VOL_RAMP_RATE BIT(6) #define TAS5720_MUTE BIT(4) #define TAS5720_TDM_SLOT_SEL_MASK GENMASK(2, 0)
@@ -88,4 +94,28 @@ #define TAS5720_CLIP1_MASK GENMASK(7, 2) #define TAS5720_CLIP1_SHIFT (0x2)
+/* TAS5722_DIGITAL_CTRL2_REG */ +#define TAS5722_HPF_3_7HZ (0x0 << 5) +#define TAS5722_HPF_7_4HZ (0x1 << 5) +#define TAS5722_HPF_14_9HZ (0x2 << 5) +#define TAS5722_HPF_29_7HZ (0x3 << 5) +#define TAS5722_HPF_59_4HZ (0x4 << 5) +#define TAS5722_HPF_118_4HZ (0x5 << 5) +#define TAS5722_HPF_235_0HZ (0x6 << 5) +#define TAS5722_HPF_463_2HZ (0x7 << 5) +#define TAS5722_HPF_MASK GENMASK(7, 5) +#define TAS5722_AUTO_SLEEP_OFF (0x0 << 3) +#define TAS5722_AUTO_SLEEP_1024LR (0x1 << 3) +#define TAS5722_AUTO_SLEEP_65536LR (0x2 << 3) +#define TAS5722_AUTO_SLEEP_262144LR (0x3 << 3) +#define TAS5722_AUTO_SLEEP_MASK GENMASK(4, 3) +#define TAS5722_TDM_SLOT_16B BIT(2) +#define TAS5722_MCLK_PIN_CFG BIT(1) +#define TAS5722_VOL_CONTROL_LSB BIT(0) + +/* TAS5722_ANALOG_CTRL2_REG */ +#define TAS5722_FAULTZ_PU BIT(3) +#define TAS5722_VREG_LVL BIT(2) +#define TAS5722_PWR_TUNE BIT(0) + #endif /* __TAS5720_H__ */
The patch
ASoC: tas5720: add TAS5722 register support
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From d5eb436acc8104b5c789359aa8c923ff5fafcd62 Mon Sep 17 00:00:00 2001
From: Andreas Dannenberg dannenberg@ti.com Date: Mon, 11 Dec 2017 13:01:55 -0600 Subject: [PATCH] ASoC: tas5720: add TAS5722 register support
Introduce a custom super-set register map and associated bit definitions to allow driver access to all TAS5722 device functionality.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/tas5720.c | 23 ++++++++++++++++++++++- sound/soc/codecs/tas5720.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index 5def54d1336d..f3006f301fe8 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -466,6 +466,15 @@ static const struct regmap_config tas5720_regmap_config = { .volatile_reg = tas5720_is_volatile_reg, };
+static const struct regmap_config tas5722_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = TAS5722_MAX_REG, + .cache_type = REGCACHE_RBTREE, + .volatile_reg = tas5720_is_volatile_reg, +}; + /* * DAC analog gain. There are four discrete values to select from, ranging * from 19.2 dB to 26.3dB. @@ -564,6 +573,7 @@ static int tas5720_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct tas5720_data *data; + const struct regmap_config *regmap_config; int ret; int i;
@@ -574,7 +584,18 @@ static int tas5720_probe(struct i2c_client *client, data->tas5720_client = client; data->devtype = id->driver_data;
- data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config); + switch (id->driver_data) { + case TAS5720: + regmap_config = &tas5720_regmap_config; + break; + case TAS5722: + regmap_config = &tas5722_regmap_config; + break; + default: + dev_err(dev, "unexpected private driver data\n"); + return -EINVAL; + } + data->regmap = devm_regmap_init_i2c(client, regmap_config); if (IS_ERR(data->regmap)) { ret = PTR_ERR(data->regmap); dev_err(dev, "failed to allocate register map: %d\n", ret); diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h index bef802afcc69..1dda3095961d 100644 --- a/sound/soc/codecs/tas5720.h +++ b/sound/soc/codecs/tas5720.h @@ -30,6 +30,11 @@ #define TAS5720_DIGITAL_CLIP1_REG 0x11 #define TAS5720_MAX_REG TAS5720_DIGITAL_CLIP1_REG
+/* Additional TAS5722-specific Registers */ +#define TAS5722_DIGITAL_CTRL2_REG 0x13 +#define TAS5722_ANALOG_CTRL2_REG 0x14 +#define TAS5722_MAX_REG TAS5722_ANALOG_CTRL2_REG + /* TAS5720_DEVICE_ID_REG */ #define TAS5720_DEVICE_ID 0x01 #define TAS5722_DEVICE_ID 0x12 @@ -52,6 +57,7 @@ #define TAS5720_SAIF_FORMAT_MASK GENMASK(2, 0)
/* TAS5720_DIGITAL_CTRL2_REG */ +#define TAS5722_VOL_RAMP_RATE BIT(6) #define TAS5720_MUTE BIT(4) #define TAS5720_TDM_SLOT_SEL_MASK GENMASK(2, 0)
@@ -88,4 +94,28 @@ #define TAS5720_CLIP1_MASK GENMASK(7, 2) #define TAS5720_CLIP1_SHIFT (0x2)
+/* TAS5722_DIGITAL_CTRL2_REG */ +#define TAS5722_HPF_3_7HZ (0x0 << 5) +#define TAS5722_HPF_7_4HZ (0x1 << 5) +#define TAS5722_HPF_14_9HZ (0x2 << 5) +#define TAS5722_HPF_29_7HZ (0x3 << 5) +#define TAS5722_HPF_59_4HZ (0x4 << 5) +#define TAS5722_HPF_118_4HZ (0x5 << 5) +#define TAS5722_HPF_235_0HZ (0x6 << 5) +#define TAS5722_HPF_463_2HZ (0x7 << 5) +#define TAS5722_HPF_MASK GENMASK(7, 5) +#define TAS5722_AUTO_SLEEP_OFF (0x0 << 3) +#define TAS5722_AUTO_SLEEP_1024LR (0x1 << 3) +#define TAS5722_AUTO_SLEEP_65536LR (0x2 << 3) +#define TAS5722_AUTO_SLEEP_262144LR (0x3 << 3) +#define TAS5722_AUTO_SLEEP_MASK GENMASK(4, 3) +#define TAS5722_TDM_SLOT_16B BIT(2) +#define TAS5722_MCLK_PIN_CFG BIT(1) +#define TAS5722_VOL_CONTROL_LSB BIT(0) + +/* TAS5722_ANALOG_CTRL2_REG */ +#define TAS5722_FAULTZ_PU BIT(3) +#define TAS5722_VREG_LVL BIT(2) +#define TAS5722_PWR_TUNE BIT(0) + #endif /* __TAS5720_H__ */
From: Andreas Dannenberg dannenberg@ti.com
The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB steps on the TAS5720). Introduce a custom mixer control that allows taking advantage of this finer output volume granularity.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com --- sound/soc/codecs/tas5720.c | 88 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index f3006f301fe8..042068964afb 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -491,11 +491,59 @@ static const DECLARE_TLV_DB_RANGE(dac_analog_tlv, * setting the gain below -100 dB (register value <0x7) is effectively a MUTE * as per device datasheet. */ -static DECLARE_TLV_DB_SCALE(dac_tlv, -10350, 50, 0); +static DECLARE_TLV_DB_SCALE(tas5720_dac_tlv, -10350, 50, 0); +
static const struct snd_kcontrol_new tas5720_snd_controls[] = { SOC_SINGLE_TLV("Speaker Driver Playback Volume", - TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv), + TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv), + SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, + TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), +}; + +/* + * DAC digital volumes. From -103.5 to 24 dB in 0.25 dB steps. Note that + * setting the gain below -100 dB (register value <0x8) is effectively a MUTE + * as per device datasheet. + * + * Note that for the TAS5722 the digital volume controls are actually split + * over two registers, so we need custom getters/setters for access. + */ +static DECLARE_TLV_DB_SCALE(tas5722_dac_tlv, -10350, 25, 0); + +static int tas5722_volume_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + unsigned int val; + + val = snd_soc_read(codec, TAS5720_VOLUME_CTRL_REG); + ucontrol->value.integer.value[0] = val << 1; + + val = snd_soc_read(codec, TAS5722_DIGITAL_CTRL2_REG); + ucontrol->value.integer.value[0] |= val & TAS5722_VOL_CONTROL_LSB; + + return 0; +} + +static int tas5722_volume_set(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + unsigned int sel = ucontrol->value.integer.value[0]; + + snd_soc_write(codec, TAS5720_VOLUME_CTRL_REG, sel >> 1); + snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG, + TAS5722_VOL_CONTROL_LSB, sel); + + return 0; +} + +static const struct snd_kcontrol_new tas5722_snd_controls[] = { + SOC_SINGLE_EXT_TLV("Speaker Driver Playback Volume", + 0, 0, 511, 0, + tas5722_volume_get, tas5722_volume_set, + tas5722_dac_tlv), SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), }; @@ -528,6 +576,22 @@ static const struct snd_soc_codec_driver soc_codec_dev_tas5720 = { }, };
+static struct snd_soc_codec_driver soc_codec_dev_tas5722 = { + .probe = tas5720_codec_probe, + .remove = tas5720_codec_remove, + .suspend = tas5720_suspend, + .resume = tas5720_resume, + + .component_driver = { + .controls = tas5722_snd_controls, + .num_controls = ARRAY_SIZE(tas5722_snd_controls), + .dapm_widgets = tas5720_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets), + .dapm_routes = tas5720_audio_map, + .num_dapm_routes = ARRAY_SIZE(tas5720_audio_map), + } +}; + /* PCM rates supported by the TAS5720 driver */ #define TAS5720_RATES (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\ SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000) @@ -614,9 +678,23 @@ static int tas5720_probe(struct i2c_client *client,
dev_set_drvdata(dev, data);
- ret = snd_soc_register_codec(&client->dev, - &soc_codec_dev_tas5720, - tas5720_dai, ARRAY_SIZE(tas5720_dai)); + switch (id->driver_data) { + case TAS5720: + ret = snd_soc_register_codec(&client->dev, + &soc_codec_dev_tas5720, + tas5720_dai, + ARRAY_SIZE(tas5720_dai)); + break; + case TAS5722: + ret = snd_soc_register_codec(&client->dev, + &soc_codec_dev_tas5722, + tas5720_dai, + ARRAY_SIZE(tas5720_dai)); + break; + default: + dev_err(dev, "unexpected private driver data\n"); + return -EINVAL; + } if (ret < 0) { dev_err(dev, "failed to register codec: %d\n", ret); return ret;
On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote:
The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB steps on the TAS5720). Introduce a custom mixer control that allows taking advantage of this finer output volume granularity.
Don't do this, it's just making things more complicated. Instead do what other drivers do and register different sets of controls depending on which part you're working with. The normal thing is to have a big table for all the shared controls that are the same on all variants then register additional tables during probe with those that vary for the individul devices.
static const struct snd_kcontrol_new tas5720_snd_controls[] = { SOC_SINGLE_TLV("Speaker Driver Playback Volume",
TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv),
TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv),- SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG,
TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv),
As ever all volume controls should end in Volume (like the immediately adjacent control does).
On 12/12/2017 06:01 AM, Mark Brown wrote:
On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote:
The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB steps on the TAS5720). Introduce a custom mixer control that allows taking advantage of this finer output volume granularity.
Don't do this, it's just making things more complicated. Instead do what other drivers do and register different sets of controls depending on which part you're working with. The normal thing is to have a big table for all the shared controls that are the same on all variants then register additional tables during probe with those that vary for the individul devices.
That is what we are doing here, the reason for the custom mixer control is that the controlled bits span two registers in a odd way that is not supported by the standard handlers.
static const struct snd_kcontrol_new tas5720_snd_controls[] = { SOC_SINGLE_TLV("Speaker Driver Playback Volume",
TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv),
TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv),- SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG,
TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv),As ever all volume controls should end in Volume (like the immediately adjacent control does).
This was done so this table exactly matches the existing table. If you would like me to change this then I can, and can do it for the other table as well, up to you.
On Mon, Jan 15, 2018 at 08:50:09AM -0600, Andrew F. Davis wrote:
On 12/12/2017 06:01 AM, Mark Brown wrote:
On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote:
The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB steps on the TAS5720). Introduce a custom mixer control that allows taking advantage of this finer output volume granularity.
Don't do this, it's just making things more complicated. Instead do what other drivers do and register different sets of controls depending on which part you're working with. The normal thing is to have a big table for all the shared controls that are the same on all variants then register additional tables during probe with those that vary for the individul devices.
That is what we are doing here, the reason for the custom mixer control is that the controlled bits span two registers in a odd way that is not supported by the standard handlers.
That's not clear from the commit message, it sounds like you're introducing an an extra control rather than replacing the one that's currently there.
As ever all volume controls should end in Volume (like the immediately adjacent control does).
This was done so this table exactly matches the existing table. If you would like me to change this then I can, and can do it for the other table as well, up to you.
Of course fixes for bugs in existing code are welcome.
From: Andreas Dannenberg dannenberg@ti.com
Unlike the TAS5720, the TAS5722 can be configured to utilize 16-bit wide slots in TDM mode. This can help easing audio clocking/frequency requirements.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com --- sound/soc/codecs/tas5720.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index 042068964afb..08e0bfc9390c 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -152,6 +152,7 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai, int slots, int slot_width) { struct snd_soc_codec *codec = dai->codec; + struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec); unsigned int first_slot; int ret;
@@ -185,6 +186,16 @@ static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai, if (ret < 0) goto error_snd_soc_update_bits;
+ /* Configure TDM slot width. This is only applicable to TAS5722. */ + if (tas5720->devtype == TAS5722) { + ret = snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG, + TAS5722_TDM_SLOT_16B, + slot_width == 16 ? + TAS5722_TDM_SLOT_16B : 0); + if (ret < 0) + goto error_snd_soc_update_bits; + } + return 0;
error_snd_soc_update_bits:
On Mon, Dec 11, 2017 at 01:01:57PM -0600, Andrew F. Davis wrote:
- /* Configure TDM slot width. This is only applicable to TAS5722. */
- if (tas5720->devtype == TAS5722) {
ret = snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG,TAS5722_TDM_SLOT_16B,slot_width == 16 ?TAS5722_TDM_SLOT_16B : 0);if (ret < 0)goto error_snd_soc_update_bits;- }
Use a switch statement, that way additional variants can be handled more sensibly.
On 12/12/2017 06:02 AM, Mark Brown wrote:
On Mon, Dec 11, 2017 at 01:01:57PM -0600, Andrew F. Davis wrote:
- /* Configure TDM slot width. This is only applicable to TAS5722. */
- if (tas5720->devtype == TAS5722) {
ret = snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG,TAS5722_TDM_SLOT_16B,slot_width == 16 ?TAS5722_TDM_SLOT_16B : 0);if (ret < 0)goto error_snd_soc_update_bits;- }
Use a switch statement, that way additional variants can be handled more sensibly.
Will fix.
The patch
ASoC: tas5720: add basic support for TAS5722 devices
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 872bcad246e30f0ae8009a0f8c13874009601445 Mon Sep 17 00:00:00 2001
From: Andreas Dannenberg dannenberg@ti.com Date: Mon, 11 Dec 2017 13:01:54 -0600 Subject: [PATCH] ASoC: tas5720: add basic support for TAS5722 devices
The TI TAS5722 digital amplifier is very similar to the TAS5720 from an overall and register map perspective. Therefore the existing driver can be extended easily to support this additional device. This commit allows TAS5722 devices to be used in a "subset" type of fashion, without exposing any of the additional features they offer.
Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Andrew F. Davis afd@ti.com Signed-off-by: Mark Brown broonie@kernel.org --- .../devicetree/bindings/sound/tas5720.txt | 4 ++- sound/soc/codecs/tas5720.c | 38 ++++++++++++++++++---- sound/soc/codecs/tas5720.h | 1 + 3 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/tas5720.txt b/Documentation/devicetree/bindings/sound/tas5720.txt index 40d94f82beb3..7481653fe8e3 100644 --- a/Documentation/devicetree/bindings/sound/tas5720.txt +++ b/Documentation/devicetree/bindings/sound/tas5720.txt @@ -6,10 +6,12 @@ audio playback. For more product information please see the links below:
http://www.ti.com/product/TAS5720L http://www.ti.com/product/TAS5720M +http://www.ti.com/product/TAS5722L
Required properties:
-- compatible : "ti,tas5720" +- compatible : "ti,tas5720", + "ti,tas5722" - reg : I2C slave address - dvdd-supply : phandle to a 3.3-V supply for the digital circuitry - pvdd-supply : phandle to a supply used for the Class-D amp and the analog diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index a736a2a6976c..5def54d1336d 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -36,6 +36,11 @@ /* Define how often to check (and clear) the fault status register (in ms) */ #define TAS5720_FAULT_CHECK_INTERVAL 200
+enum tas572x_type { + TAS5720, + TAS5722, +}; + static const char * const tas5720_supply_names[] = { "dvdd", /* Digital power supply. Connect to 3.3-V supply. */ "pvdd", /* Class-D amp and analog power supply (connected). */ @@ -47,6 +52,7 @@ struct tas5720_data { struct snd_soc_codec *codec; struct regmap *regmap; struct i2c_client *tas5720_client; + enum tas572x_type devtype; struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES]; struct delayed_work fault_check_work; unsigned int last_fault; @@ -264,7 +270,7 @@ static void tas5720_fault_check_work(struct work_struct *work) static int tas5720_codec_probe(struct snd_soc_codec *codec) { struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec); - unsigned int device_id; + unsigned int device_id, expected_device_id; int ret;
tas5720->codec = codec; @@ -276,6 +282,11 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) return ret; }
+ /* + * Take a liberal approach to checking the device ID to allow the + * driver to be used even if the device ID does not match, however + * issue a warning if there is a mismatch. + */ ret = regmap_read(tas5720->regmap, TAS5720_DEVICE_ID_REG, &device_id); if (ret < 0) { dev_err(codec->dev, "failed to read device ID register: %d\n", @@ -283,13 +294,22 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) goto probe_fail; }
- if (device_id != TAS5720_DEVICE_ID) { - dev_err(codec->dev, "wrong device ID. expected: %u read: %u\n", - TAS5720_DEVICE_ID, device_id); - ret = -ENODEV; - goto probe_fail; + switch (tas5720->devtype) { + case TAS5720: + expected_device_id = TAS5720_DEVICE_ID; + break; + case TAS5722: + expected_device_id = TAS5722_DEVICE_ID; + break; + default: + dev_err(codec->dev, "unexpected private driver data\n"); + return -EINVAL; }
+ if (device_id != expected_device_id) + dev_warn(codec->dev, "wrong device ID. expected: %u read: %u\n", + expected_device_id, device_id); + /* Set device to mute */ ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG, TAS5720_MUTE, TAS5720_MUTE); @@ -552,6 +572,8 @@ static int tas5720_probe(struct i2c_client *client, return -ENOMEM;
data->tas5720_client = client; + data->devtype = id->driver_data; + data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config); if (IS_ERR(data->regmap)) { ret = PTR_ERR(data->regmap); @@ -592,7 +614,8 @@ static int tas5720_remove(struct i2c_client *client) }
static const struct i2c_device_id tas5720_id[] = { - { "tas5720", 0 }, + { "tas5720", TAS5720 }, + { "tas5722", TAS5722 }, { } }; MODULE_DEVICE_TABLE(i2c, tas5720_id); @@ -600,6 +623,7 @@ MODULE_DEVICE_TABLE(i2c, tas5720_id); #if IS_ENABLED(CONFIG_OF) static const struct of_device_id tas5720_of_match[] = { { .compatible = "ti,tas5720", }, + { .compatible = "ti,tas5722", }, { }, }; MODULE_DEVICE_TABLE(of, tas5720_of_match); diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h index 3d077c779b12..bef802afcc69 100644 --- a/sound/soc/codecs/tas5720.h +++ b/sound/soc/codecs/tas5720.h @@ -32,6 +32,7 @@
/* TAS5720_DEVICE_ID_REG */ #define TAS5720_DEVICE_ID 0x01 +#define TAS5722_DEVICE_ID 0x12
/* TAS5720_POWER_CTRL_REG */ #define TAS5720_DIG_CLIP_MASK GENMASK(7, 2)
participants (2)
-
Andrew F. Davis -
Mark Brown