ASoC: tas27{64,70}: improve support for Apple codec variants
Subsystems (5)
v2 cover ASoC: tas27{64,70}: improve support for Apple codec variants
Hi all,
This series introduces a number of changes to the drivers for the Texas Instruments TAS2764 and TAS2770 amplifiers in order to introduce (and improve in the case of TAS2770) support for the variants of these amps found in Apple Silicon Macs.
Apple's variant of TAS2764 is known as SN012776, and as always with Apple is a subtly incompatible variant with a number of quirks. It is not publicly available. The TAS2770 variant is known as TAS5770L, and does not require incompatible handling.
Much as with the Cirrus codec patches, I do not
expect that we will get any official acknowledgement that these parts exist from TI, however I would be delighted to be proven wrong.
This series has been living in the downstream Asahi kernel tree[1] for over two years, and has been tested by many thousands of users by this point[2].
[1] https://github.com/AsahiLinux/linux/tree/asahi-wip
[2] https://stats.asahilinux.org/
---
Changes in v2:
- Changed author field of patch to match Martin's Signed-off-by
- Added Neal's Reviewed-by to reviewed patches
- Moved fixes to existing code to the top of the series
- Removed tas2764's explicit dependency on OF
- Removed complicated single-use tas2764 quirks macro and replaced with
if block
- Added hwmon interface for codec die temp
- Fixed a malformed commit message
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Hector Martin (14):
ASoC: tas2764: Fix power control mask
ASoC: tas2770: Fix volume scale
ASoC: tas2764: Enable main IRQs
ASoC: tas2764: Power up/down amp on mute ops
ASoC: tas2764: Add SDZ regulator
ASoC: tas2764: Add reg defaults for TAS2764_INT_CLK_CFG
ASoC: tas2764: Mark SW_RESET as volatile
ASoC: tas2764: Wait for ramp-down after shutdown
ASoC: tas2770: Add SDZ regulator
ASoC: tas2770: Power cycle amp on ISENSE/VSENSE change
ASoC: tas2770: Add zero-fill and pull-down controls
ASoC: tas2770: Support setting the PDM TX slot
ASoC: tas2764: Set the SDOUT polarity correctly
ASoC: tas2770: Set the SDOUT polarity correctly
James Calligeros (4):
ASoC: dt-bindings: tas27xx: add compatible for SN012776
ASoC: dt-bindings: tas2770: add compatible for TAS5770L
ASoC: tas2770: expose die temp to hwmon
ASoC: tas2764: expose die temp to hwmon
Martin Povišer (11):
ASoC: tas2764: Extend driver to SN012776
ASoC: tas2764: Add control concerning overcurrent events
ASoC: tas2770: Factor out set_ivsense_slots
ASoC: tas2770: Fix and redo I/V sense TDM slot setting logic
ASoC: tas2764: Reinit cache on part reset
ASoC: tas2764: Configure zeroing of SDOUT slots
ASoC: tas2764: Apply Apple quirks
ASoC: tas2764: Raise regmap range maximum
ASoC: tas2770: Export 'die_temp' to sysfs
ASoC: tas2764: Export 'die_temp' to sysfs
ASoC: tas2764: Crop SDOUT zero-out mask based on BCLK ratio
.../bindings/sound/ti,tas2770.yaml | 1 + .../bindings/sound/ti,tas27xx.yaml | 1 + sound/soc/codecs/tas2764-quirks.h | 180 ++++++++++++ sound/soc/codecs/tas2764.c | 368 ++++++++++++++++++++++--- sound/soc/codecs/tas2764.h | 29 +- sound/soc/codecs/tas2770.c | 333 ++++++++++++++++++---- sound/soc/codecs/tas2770.h | 20 ++ 7 files changed, 839 insertions(+), 93 deletions(-)
---
base-commit: cc7708ae5e2aab296203fcec774695fc9d995f48
change-id: 20250214-apple-codec-changes-6e656dc1e24d
Best regards,
Signature · 1 line
James Calligeros <[email protected]>
PATCH v2 1/29 ASoC: tas2764: Fix power control mask
From: Hector Martin <[email protected]>
--- sound/soc/codecs/tas2764.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/tas2764.h +1 -1
@@ -25,7 +25,7 @@/* Power Control */ #define TAS2764_PWR_CTRL TAS2764_REG(0X0, 0x02) -#define TAS2764_PWR_CTRL_MASK GENMASK(1, 0) +#define TAS2764_PWR_CTRL_MASK GENMASK(2, 0)#define TAS2764_PWR_CTRL_ACTIVE 0x0#define TAS2764_PWR_CTRL_MUTE BIT(0)#define TAS2764_PWR_CTRL_SHUTDOWN BIT(1)
Signature · 1 line
2.48.1
PATCH v2 2/29 ASoC: tas2770: Fix volume scale
In reply to James CalligerosFrom: Hector Martin <[email protected]>
The scale starts at -100dB, not -128dB.
--- sound/soc/codecs/tas2770.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/tas2770.c +1 -1
@@ -506,7 +506,7 @@ static int tas2770_codec_probe(struct snd_soc_component *component) } static DECLARE_TLV_DB_SCALE(tas2770_digital_tlv, 1100, 50, 0); -static DECLARE_TLV_DB_SCALE(tas2770_playback_volume, -12750, 50, 0); +static DECLARE_TLV_DB_SCALE(tas2770_playback_volume, -10050, 50, 0);static const struct snd_kcontrol_new tas2770_snd_controls[] = { SOC_SINGLE_TLV("Speaker Playback Volume", TAS2770_PLAY_CFG_REG2,
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 02/29] ASoC: tas2770: Fix volume scale
Quoted 3 lines · James Calligeros
From: Hector Martin <[email protected]>
The scale starts at -100dB, not -128dB.
As mentioned on your prior posting:
PATCH v2 3/29 ASoC: dt-bindings: tas27xx: add compatible for SN012776
In reply to James CalligerosThe TI SN012776 is a variant of TAS2764 found in Apple Silicon Macs. It continues Apple's long-standing policy of getting vendors to spin out subtly incompatible and Apple-exclusive variants of their publicly available parts.
--- .../bindings/sound/ti,tas27xx.yaml | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/sound/ti,tas27xx.yaml +1 -0
@@ -24,6 +24,7 @@ properties: enum: - ti,tas2764 - ti,tas2780 + - ti,sn012776 # Apple variant of TAS2764 reg: maxItems: 1
Signature · 1 line
2.48.1
Re: [PATCH v2 03/29] ASoC: dt-bindings: tas27xx: add compatible for SN012776
Quoted 11 lines · James Calligeros
The TI SN012776 is a variant of TAS2764 found in Apple Silicon Macs. It continues Apple's long-standing policy of getting vendors to spin out subtly incompatible and Apple-exclusive variants of their publicly available parts.
Reviewed-by Neal Gompa <[email protected]>Signed-off-by James Calligeros <[email protected]>--- .../bindings/sound/ti,tas27xx.yaml | 1 + 1 file changed, 1 insertion(+)
PATCH v2 4/29 ASoC: dt-bindings: tas2770: add compatible for TAS5770L
In reply to James CalligerosA variant of TAS2770 known as TAS5770L is found in Apple Silicon Macs.
--- .../bindings/sound/ti,tas2770.yaml | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/sound/ti,tas2770.yaml +1 -0
@@ -23,6 +23,7 @@ properties: compatible: enum: - ti,tas2770 + - ti,tas5770l # Apple variant reg: maxItems: 1
Signature · 1 line
2.48.1
Re: [PATCH v2 04/29] ASoC: dt-bindings: tas2770: add compatible for TAS5770L
Quoted 8 lines · James Calligeros
A variant of TAS2770 known as TAS5770L is found in Apple Silicon Macs.
Reviewed-by Neal Gompa <[email protected]>Signed-off-by James Calligeros <[email protected]>--- .../bindings/sound/ti,tas2770.yaml | 1 + 1 file changed, 1 insertion(+)
PATCH v2 5/29 ASoC: tas2764: Extend driver to SN012776
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
SN012776 is a speaker amp chip found in Apple's 2021 laptops. It appears similar and more-or-less compatible to TAS2764. Extend the TAS2764 driver with some SN012776 specifics and configure the chip assuming it's in one of the Apple machines.
--- sound/soc/codecs/tas2764.c | 42 +++++++++++++++++++++++-- sound/soc/codecs/tas2764.h | 3 ++ 2 files changed, 42 insertions(+), 3 deletions(-)
sound/soc/codecs/tas2764.c +39 -3
@@ -14,6 +14,7 @@#include <linux/regulator/consumer.h> #include <linux/regmap.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/slab.h> #include <sound/soc.h> #include <sound/pcm.h>
@@ -23,6 +24,11 @@#include "tas2764.h" +enum tas2764_devid { +DEVID_TAS2764 =0 , +DEVID_SN012776 =1 +}; +struct tas2764_priv {struct snd_soc_component *component;struct gpio_desc *reset_gpio;
@@ -30,7 +36,8 @@ struct tas2764_priv {struct regmap *regmap;struct device *dev;int irq; - +enum tas2764_devid devid; +int v_sense_slot;int i_sense_slot;
@@ -526,10 +533,16 @@ static struct snd_soc_dai_driver tas2764_dai_driver[] = { }, }; +static uint8_t sn012776_bop_presets[] = { +0x01 ,0x32 ,0x02 ,0x22 ,0x83 ,0x2d ,0x80 ,0x02 ,0x06 , +0x32 ,0x46 ,0x30 ,0x02 ,0x06 ,0x38 ,0x40 ,0x30 ,0x02 , +0x06 ,0x3e ,0x37 ,0x30 ,0xff ,0xe6 +}; +static int tas2764_codec_probe (struct snd_soc_component *component) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component); -int ret; +int ret, i; tas2764->component = component;
@@ -578,6 +591,23 @@ static int tas2764_codec_probe(struct snd_soc_component *component)if (ret <0 )return ret; +if (tas2764->devid ==DEVID_SN012776 ) { + ret =snd_soc_component_update_bits (component,TAS2764_PWR_CTRL , +TAS2764_PWR_CTRL_BOP_SRC , +TAS2764_PWR_CTRL_BOP_SRC ); +if (ret <0 ) +return ret; + +for (i =0 ; i <ARRAY_SIZE (sn012776_bop_presets); i++) { + ret =snd_soc_component_write (component, +TAS2764_BOP_CFG0 + i, + sn012776_bop_presets[i]); + +if (ret <0 ) +return ret; + } + } +return 0 ; }
@@ -707,6 +737,11 @@ static int tas2764_i2c_probe(struct i2c_client *client)if (!tas2764)return -ENOMEM ; +if (device_is_compatible (&client->dev,"ti,sn012776" )) + tas2764->devid =DEVID_SN012776 ; +else + tas2764->devid =DEVID_TAS2764 ; + tas2764->dev = &client->dev; tas2764->irq = client->irq;i2c_set_clientdata (client, tas2764);
@@ -743,7 +778,8 @@ MODULE_DEVICE_TABLE(i2c, tas2764_i2c_id);#if defined(CONFIG_OF )static const struct of_device_id tas2764_of_match[] = { - { .compatible ="ti,tas2764" }, + { .compatible ="ti,tas2764" , }, + { .compatible ="ti,sn012776" , }, {}, };MODULE_DEVICE_TABLE (of, tas2764_of_match);
sound/soc/codecs/tas2764.h +3 -0
@@ -29,6 +29,7 @@#define TAS2764_PWR_CTRL_ACTIVE 0x0#define TAS2764_PWR_CTRL_MUTE BIT(0)#define TAS2764_PWR_CTRL_SHUTDOWN BIT(1) +#define TAS2764_PWR_CTRL_BOP_SRC BIT(7)#define TAS2764_VSENSE_POWER_EN 3#define TAS2764_ISENSE_POWER_EN 4
@@ -110,4 +111,6 @@#define TAS2764_INT_CLK_CFG TAS2764_REG(0x0, 0x5c)#define TAS2764_INT_CLK_CFG_IRQZ_CLR BIT(2) +#define TAS2764_BOP_CFG0 TAS2764_REG(0X0, 0x1d) +#endif /* __TAS2764__ */
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 05/29] ASoC: tas2764: Extend driver to SN012776
sound/soc/codecs/tas2764.c static int tas2764_codec_probe(struct snd_soc_component *component) · line 597
Quoted 4 lines · James Calligeros
sound/soc/codecs/tas2764.c +4
-0+if (tas2764->devid ==DEVID_SN012776 ) { + ret =snd_soc_component_update_bits (component,TAS2764_PWR_CTRL , +TAS2764_PWR_CTRL_BOP_SRC , +TAS2764_PWR_CTRL_BOP_SRC );
This sort of code is generally better written with switch statements for extensibility.
Review
Quoted 11 lines
(quoted diff) +7
-1+ if (device_is_compatible(&client->dev, "ti,sn012776")) + tas2764->devid = DEVID_SN012776; + else + tas2764->devid = DEVID_TAS2764; + #if defined(CONFIG_OF) static const struct of_device_id tas2764_of_match[] = { - { .compatible = "ti,tas2764" }, + { .compatible = "ti,tas2764", }, + { .compatible = "ti,sn012776", },
It would be more usual to use the data field in the of_device_id to look up the enum.
PATCH v2 6/29 ASoC: tas2764: Add control concerning overcurrent events
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
Add control to expose the option of autoretry behavior on overcurrent events in the codec.
--- sound/soc/codecs/tas2764.c | 9 +++++++++ sound/soc/codecs/tas2764.h | 4 ++++ 2 files changed, 13 insertions(+)
sound/soc/codecs/tas2764.c +9 -0
@@ -623,12 +623,21 @@ static SOC_ENUM_SINGLE_DECL( tas2764_hpf_enum,TAS2764_DC_BLK0 , TAS2764_DC_BLK0_HPF_FREQ_PB_SHIFT, tas2764_hpf_texts); +static const char *const tas2764_oce_texts[] = { +"Disable" ,"Retry" , +}; + +static SOC_ENUM_SINGLE_DECL ( + tas2764_oce_enum, TAS2764_MISC_CFG1, + TAS2764_MISC_CFG1_OCE_RETRY_SHIFT, tas2764_oce_texts); +static const struct snd_kcontrol_new tas2764_snd_controls[] = {SOC_SINGLE_TLV ("Speaker Volume" ,TAS2764_DVC ,0 ,TAS2764_DVC_MAX ,1 , tas2764_playback_volume),SOC_SINGLE_TLV ("Amp Gain Volume" ,TAS2764_CHNL_0 ,1 ,0x14 ,0 , tas2764_digital_tlv),SOC_ENUM ("HPF Corner Frequency" , tas2764_hpf_enum), +SOC_ENUM ("OCE Handling" , tas2764_oce_enum), };static const struct snd_soc_component_driver soc_component_driver_tas2764 = {
sound/soc/codecs/tas2764.h +4 -0
@@ -44,6 +44,10 @@#define TAS2764_CHNL_0 TAS2764_REG(0X0, 0x03) +/* Miscellaneous */ +#define TAS2764_MISC_CFG1 TAS2764_REG(0x0, 0x06) +#define TAS2764_MISC_CFG1_OCE_RETRY_SHIFT 5 +/* TDM Configuration Reg0 */ #define TAS2764_TDM_CFG0 TAS2764_REG(0X0, 0x08)#define TAS2764_TDM_CFG0_SMP_MASK BIT(5)
Signature · 1 line
2.48.1
PATCH v2 7/29 ASoC: tas2770: Factor out set_ivsense_slots
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
Add a new explicit function for the setting of I/V sense TDM slots.
--- sound/soc/codecs/tas2770.c | 40 +++++++++++++++++-------- 1 file changed, 27 insertions(+), 13 deletions(-)
sound/soc/codecs/tas2770.c +27 -13
@@ -189,6 +189,31 @@ static int tas2770_mute(struct snd_soc_dai *dai, int mute, int direction)return tas2770_update_pwr_ctrl (tas2770); } +static int tas2770_set_ivsense_transmit (struct tas2770_priv *tas2770, +int i_slot,int v_slot) +{ +struct snd_soc_component *component = tas2770->component; +int ret; + + ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG5 , +TAS2770_TDM_CFG_REG5_VSNS_MASK | +TAS2770_TDM_CFG_REG5_50_MASK , +TAS2770_TDM_CFG_REG5_VSNS_ENABLE | + v_slot); +if (ret <0 ) +return ret; + + ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG6 , +TAS2770_TDM_CFG_REG6_ISNS_MASK | +TAS2770_TDM_CFG_REG6_50_MASK , +TAS2770_TDM_CFG_REG6_ISNS_ENABLE | + i_slot); +if (ret <0 ) +return ret; + +return 0 ; +} +static int tas2770_set_bitwidth (struct tas2770_priv *tas2770,int bitwidth) {int ret;
@@ -221,19 +246,8 @@ static int tas2770_set_bitwidth(struct tas2770_priv *tas2770, int bitwidth)if (ret <0 )return ret; - ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG5 , -TAS2770_TDM_CFG_REG5_VSNS_MASK | -TAS2770_TDM_CFG_REG5_50_MASK , -TAS2770_TDM_CFG_REG5_VSNS_ENABLE | - tas2770->v_sense_slot); -if (ret <0 ) -return ret; - - ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG6 , -TAS2770_TDM_CFG_REG6_ISNS_MASK | -TAS2770_TDM_CFG_REG6_50_MASK , -TAS2770_TDM_CFG_REG6_ISNS_ENABLE | - tas2770->i_sense_slot); + ret =tas2770_set_ivsense_transmit (tas2770, tas2770->i_sense_slot, + tas2770->v_sense_slot);if (ret <0 )return ret;
Signature · 1 line
2.48.1
PATCH v2 8/29 ASoC: tas2770: Fix and redo I/V sense TDM slot setting logic
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
The former code sets the V slot from inside set_bitwidth according to the bitwidth of the PCM format. That's wrong, since:
* It overrides the V slot parsed from DT binding.
* The V slot is set shifted behind the I slot by the length of the PCM bitwidth, but the PCM bitwidth has no assured relation to the TDM slot width.
Replace the former logic by setting up the I/V sense transmission only in case of both I/V slots being specified in devicetree, and never override those values. In case the slots are left unspecified, disable the transmission completely.
There's an improbable case someone is relying on the old behavior, but if so, that's a setup that only works by accident, and cannot be sanely supported going forward. There's no indication anyone is consuming the I/V sense data up to today, so break the former behavior.
--- sound/soc/codecs/tas2770.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
sound/soc/codecs/tas2770.c +11 -10
@@ -224,19 +224,16 @@ static int tas2770_set_bitwidth(struct tas2770_priv *tas2770, int bitwidth) ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG2 ,TAS2770_TDM_CFG_REG2_RXW_MASK ,TAS2770_TDM_CFG_REG2_RXW_16BITS ); - tas2770->v_sense_slot = tas2770->i_sense_slot +2 ;break ;case SNDRV_PCM_FORMAT_S24_LE : ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG2 ,TAS2770_TDM_CFG_REG2_RXW_MASK ,TAS2770_TDM_CFG_REG2_RXW_24BITS ); - tas2770->v_sense_slot = tas2770->i_sense_slot +4 ;break ;case SNDRV_PCM_FORMAT_S32_LE : ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG2 ,TAS2770_TDM_CFG_REG2_RXW_MASK ,TAS2770_TDM_CFG_REG2_RXW_32BITS ); - tas2770->v_sense_slot = tas2770->i_sense_slot +4 ;break ;default :
@@ -246,11 +243,6 @@ static int tas2770_set_bitwidth(struct tas2770_priv *tas2770, int bitwidth)if (ret <0 )return ret; - ret =tas2770_set_ivsense_transmit (tas2770, tas2770->i_sense_slot, - tas2770->v_sense_slot); -if (ret <0 ) -return ret; -return 0 ; }
@@ -505,6 +497,7 @@ static int tas2770_codec_probe(struct snd_soc_component *component) {struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component); +int ret; tas2770->component = component;
@@ -516,6 +509,14 @@ static int tas2770_codec_probe(struct snd_soc_component *component)tas2770_reset (tas2770);regmap_reinit_cache (tas2770->regmap, &tas2770_i2c_regmap); +if (tas2770->i_sense_slot !=-1 && tas2770->v_sense_slot !=-1 ) { + ret =tas2770_set_ivsense_transmit (tas2770, tas2770->i_sense_slot, + tas2770->v_sense_slot); + +if (ret <0 ) +return ret; + } +return 0 ; }
@@ -643,7 +644,7 @@ static int tas2770_parse_dt(struct device *dev, struct tas2770_priv *tas2770)dev_info (tas2770->dev,"Property %s is missing setting default slot\n" ,"ti,imon-slot-no" ); - tas2770->i_sense_slot =0 ; + tas2770->i_sense_slot =-1 ; } rc = fwnode_property_read_u32(dev->fwnode, "ti,vmon-slot-no",
@@ -652,7 +653,7 @@ static int tas2770_parse_dt(struct device *dev, struct tas2770_priv *tas2770)dev_info (tas2770->dev,"Property %s is missing setting default slot\n" ,"ti,vmon-slot-no" ); - tas2770->v_sense_slot =2 ; + tas2770->v_sense_slot =-1 ; } tas2770->sdz_gpio =devm_gpiod_get_optional (dev,"shutdown" ,GPIOD_OUT_HIGH );
Signature · 1 line
2.48.1
PATCH v2 9/29 ASoC: tas2764: Reinit cache on part reset
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
When the part is reset in component_probe, do not forget to reinit the regcache, otherwise the cache can get out of sync with the part's actual state. This fix is similar to commit 0a0342ede303 ("ASoC: tas2770: Reinit regcache on reset") which concerned the tas2770 driver.
--- sound/soc/codecs/tas2764.c | 3 +++ 1 file changed, 3 insertions(+)
sound/soc/codecs/tas2764.c +3 -0
@@ -539,6 +539,8 @@ static uint8_t sn012776_bop_presets[] = { 0x06, 0x3e, 0x37, 0x30, 0xff, 0xe6 }; +static const struct regmap_config tas2764_i2c_regmap; +static int tas2764_codec_probe (struct snd_soc_component *component) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component);
@@ -552,6 +554,7 @@ static int tas2764_codec_probe(struct snd_soc_component *component) }tas2764_reset (tas2764); +regmap_reinit_cache (tas2764->regmap, &tas2764_i2c_regmap);if (tas2764->irq) { ret =snd_soc_component_write (tas2764->component,TAS2764_INT_MASK0 ,0xff );
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 09/29] ASoC: tas2764: Reinit cache on part reset
Quoted 7 lines · James Calligeros
From: Martin Povišer <[email protected]>
When the part is reset in
component_probe, do not forget to reinit the regcache, otherwise the cache can get out of sync with the part's actual state. This fix is similar to commit0a0342ede303("ASoC: tas2770: Reinit regcache on reset") which concerned the tas2770 driver.
Fixes should go at the start of a series so they can be applied easily without spurious dependencies.
PATCH v2 10/29 ASoC: tas2764: Configure zeroing of SDOUT slots
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
The codec has an option to zero out certain TDM slots on its SDOUT output according to a preconfigured mask (otherwise the output is, for the duration of unused slots, in a Hi-Z state). Configure this feature based on a mask read from the devicetree.
--- sound/soc/codecs/tas2764.c | 23 +++++++++++++++++++++++ sound/soc/codecs/tas2764.h | 11 +++++++++++ 2 files changed, 34 insertions(+)
sound/soc/codecs/tas2764.c +23 -0
@@ -40,6 +40,7 @@ struct tas2764_priv {int v_sense_slot;int i_sense_slot; +u32 sdout_zero_mask;bool dac_powered;bool unmuted;
@@ -594,6 +595,23 @@ static int tas2764_codec_probe(struct snd_soc_component *component)if (ret <0 )return ret; +if (tas2764->sdout_zero_mask) { +for (i =0 ; i <4 ; i++) { + ret =snd_soc_component_write (component,TAS2764_SDOUT_HIZ_1 + i, + (tas2764->sdout_zero_mask >> (i *8 )) &0xff ); + +if (ret <0 ) +return ret; + } + + ret =snd_soc_component_update_bits (component,TAS2764_SDOUT_HIZ_9 , +TAS2764_SDOUT_HIZ_9_FORCE_0_EN , +TAS2764_SDOUT_HIZ_9_FORCE_0_EN ); + +if (ret <0 ) +return ret; + } +if (tas2764->devid ==DEVID_SN012776 ) { ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL, TAS2764_PWR_CTRL_BOP_SRC,
@@ -736,6 +754,11 @@ static int tas2764_parse_dt(struct device *dev, struct tas2764_priv *tas2764)if (ret) tas2764->v_sense_slot =2 ; + ret =fwnode_property_read_u32 (dev->fwnode,"ti,sdout-force-zero-mask" , + &tas2764->sdout_zero_mask); +if (ret) + tas2764->sdout_zero_mask =0 ; +return 0 ; }
sound/soc/codecs/tas2764.h +11 -0
@@ -117,4 +117,15 @@#define TAS2764_BOP_CFG0 TAS2764_REG(0X0, 0x1d) +#define TAS2764_SDOUT_HIZ_1 TAS2764_REG(0x1, 0x3d) +#define TAS2764_SDOUT_HIZ_2 TAS2764_REG(0x1, 0x3e) +#define TAS2764_SDOUT_HIZ_3 TAS2764_REG(0x1, 0x3f) +#define TAS2764_SDOUT_HIZ_4 TAS2764_REG(0x1, 0x40) +#define TAS2764_SDOUT_HIZ_5 TAS2764_REG(0x1, 0x41) +#define TAS2764_SDOUT_HIZ_6 TAS2764_REG(0x1, 0x42) +#define TAS2764_SDOUT_HIZ_7 TAS2764_REG(0x1, 0x43) +#define TAS2764_SDOUT_HIZ_8 TAS2764_REG(0x1, 0x44) +#define TAS2764_SDOUT_HIZ_9 TAS2764_REG(0x1, 0x45) +#define TAS2764_SDOUT_HIZ_9_FORCE_0_EN BIT(7) +#endif /* __TAS2764__ */
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 10/29] ASoC: tas2764: Configure zeroing of SDOUT slots
Quoted 10 lines · James Calligeros
From: Martin Povišer <[email protected]>
The codec has an option to zero out certain TDM slots on its SDOUT output according to a preconfigured mask (otherwise the output is, for the duration of unused slots, in a Hi-Z state). Configure this feature based on a mask read from the devicetree.
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Martin Povišer <[email protected]>Signed-off-by James Calligeros <[email protected]>
This adds a DT property so needs a corresponding binding update. It'd be good to note why it's a binding rather than a function usable by machine drivers too.
PATCH v2 11/29 ASoC: tas2764: Apply Apple quirks
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
Apple's SN012776 driver has some peculiar aspects to its behavior that are suspected to work around issues in the codec part. Add a module parameter for enabling individual quirks that should be imitated after the Apple driver.
Setting some of these by default seems to be required. For example, setting 0xf fixes an issue with transient overcurrent errors which can crash the chip until the next system reboot. To be safe, let's enable all of them by default.
--- sound/soc/codecs/tas2764-quirks.h | 180 +++++++++++++++++++++++++ sound/soc/codecs/tas2764.c | 38 ++++++ 2 files changed, 218 insertions(+)
sound/soc/codecs/tas2764-quirks.h new file +180 -0
@@ -0,0 +1,180 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __TAS2764_QUIRKS__ +#define __TAS2764_QUIRKS__ + +#include <linux/regmap.h> + +#include "tas2764.h" + +/* Bitmask of enabled Apple quirks */ +#define ENABLED_APPLE_QUIRKS 0x3f + +/* +* Disable noise gate and flip down reserved bit in NS_CFG0 +*/ +#define TAS2764_NOISE_GATE_DISABLE BIT(0) + +static const struct reg_sequence tas2764_noise_gate_dis_seq[] = { +REG_SEQ0 (TAS2764_REG (0x0 ,0x35 ),0xb0 ) +}; + +/* +* CONV_VBAT_PVDD_MODE=1 +*/ +#define TAS2764_CONV_VBAT_PVDD_MODE BIT(1) + +static const struct reg_sequence tas2764_conv_vbat_pvdd_mode_seq[] = { +REG_SEQ0 (TAS2764_REG (0x0 ,0x6b ),0x41 ) +}; + +/* +* Reset of DAC modulator when DSP is OFF +*/ +#define TAS2764_DMOD_RST BIT(2) + +static const struct reg_sequence tas2764_dmod_rst_seq[] = { +REG_SEQ0 (TAS2764_REG (0x0 ,0x76 ),0x0 ) +}; + +/* +* Unknown 0x133/0x137 writes (maybe TDM related) +*/ +#define TAS2764_UNK_SEQ0 BIT(3) + +static const struct reg_sequence tas2764_unk_seq0[] = { +REG_SEQ0 (TAS2764_REG (0x1 ,0x33 ),0x80 ), +REG_SEQ0 (TAS2764_REG (0x1 ,0x37 ),0x3a ), +}; + +/* +* Unknown 0x614 - 0x61f writes +*/ +#define TAS2764_APPLE_UNK_SEQ1 BIT(4) + +static const struct reg_sequence tas2764_unk_seq1[] = { +REG_SEQ0 (TAS2764_REG (0x6 ,0x14 ),0x0 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x15 ),0x13 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x16 ),0x52 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x17 ),0x0 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x18 ),0xe4 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x19 ),0xc ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x16 ),0xaa ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x1b ),0x0 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x1c ),0x12 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x1d ),0xa0 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x1e ),0xd8 ), +REG_SEQ0 (TAS2764_REG (0x6 ,0x1f ),0x0 ), +}; + +/* +* Unknown writes in the 0xfd page (with secondary paging inside) +*/ +#define TAS2764_APPLE_UNK_SEQ2 BIT(5) + +static const struct reg_sequence tas2764_unk_seq2[] = { +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0xd ), +REG_SEQ0 (TAS2764_REG (0xfd ,0x6c ),0x2 ), +REG_SEQ0 (TAS2764_REG (0xfd ,0x6d ),0xf ), +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0x0 ), +}; + +/* +* Disable 'Thermal Threshold 1' +*/ +#define TAS2764_THERMAL_TH1_DISABLE BIT(6) + +static const struct reg_sequence tas2764_thermal_th1_dis_seq[] = { +REG_SEQ0 (TAS2764_REG (0x1 ,0x47 ),0x2 ), +}; + +/* +* Imitate Apple's shutdown dance +*/ +#define TAS2764_SHUTDOWN_DANCE BIT(7) + +static const struct reg_sequence tas2764_shutdown_dance_init_seq[] = { +/* +* SDZ_MODE=01 (immediate) +* +* We want the shutdown to happen under the influence of +* the magic writes in the 0xfdXX region, so make sure +* the shutdown is immediate and there's no grace period +* followed by the codec part. +*/ +REG_SEQ0 (TAS2764_REG (0x0 ,0x7 ),0x60 ), +}; + +static const struct reg_sequence tas2764_pre_shutdown_seq[] = { +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0xd ),/* switch hidden page */ +REG_SEQ0 (TAS2764_REG (0xfd ,0x64 ),0x4 ),/* do write (unknown semantics) */ +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0x0 ),/* switch hidden page back */ +}; + +static const struct reg_sequence tas2764_post_shutdown_seq[] = { +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0xd ), +REG_SEQ0 (TAS2764_REG (0xfd ,0x64 ),0x0 ),/* revert write from pre sequence */ +REG_SEQ0 (TAS2764_REG (0xfd ,0x0d ),0x0 ), +}; + +static int tas2764_do_quirky_pwr_ctrl_change (struct tas2764_priv *tas2764, +unsigned int target) +{ +unsigned int curr; +int ret; + + curr =snd_soc_component_read_field (tas2764->component, +TAS2764_PWR_CTRL , +TAS2764_PWR_CTRL_MASK ); + +if (target == curr) +return 0 ; + +/* Handle power state transition to shutdown */ +if (target ==TAS2764_PWR_CTRL_SHUTDOWN && + (curr ==TAS2764_PWR_CTRL_MUTE || curr ==TAS2764_PWR_CTRL_ACTIVE )) { + ret =regmap_multi_reg_write (tas2764->regmap, tas2764_pre_shutdown_seq, +ARRAY_SIZE (tas2764_pre_shutdown_seq)); +if (!ret) + ret =snd_soc_component_update_bits (tas2764->component, +TAS2764_PWR_CTRL , +TAS2764_PWR_CTRL_MASK , +TAS2764_PWR_CTRL_SHUTDOWN ); +if (!ret) + ret =regmap_multi_reg_write (tas2764->regmap, + tas2764_post_shutdown_seq, +ARRAY_SIZE (tas2764_post_shutdown_seq)); + } + + ret =snd_soc_component_update_bits (tas2764->component,TAS2764_PWR_CTRL , +TAS2764_PWR_CTRL_MASK , target); + +return ret; +} + +/* +* Via devicetree (TODO): +* - switch from spread spectrum to class-D switching +* - disable edge control +* - set BOP settings (the BOP config bits *and* BOP_SRC) +*/ + +/* +* Other setup TODOs: +* - DVC ramp rate +*/ + +static const struct tas2764_quirk_init_sequence { +const struct reg_sequence *seq; +int len; +} tas2764_quirk_init_sequences[] = { + { tas2764_noise_gate_dis_seq,ARRAY_SIZE (tas2764_noise_gate_dis_seq) }, + { tas2764_dmod_rst_seq,ARRAY_SIZE (tas2764_dmod_rst_seq) }, + { tas2764_conv_vbat_pvdd_mode_seq,ARRAY_SIZE (tas2764_conv_vbat_pvdd_mode_seq) }, + { tas2764_unk_seq0,ARRAY_SIZE (tas2764_unk_seq0) }, + { tas2764_unk_seq1,ARRAY_SIZE (tas2764_unk_seq1) }, + { tas2764_unk_seq2,ARRAY_SIZE (tas2764_unk_seq2) }, + { tas2764_thermal_th1_dis_seq,ARRAY_SIZE (tas2764_thermal_th1_dis_seq) }, + { tas2764_shutdown_dance_init_seq,ARRAY_SIZE (tas2764_shutdown_dance_init_seq) }, +}; + +#endif /* __TAS2764_QUIRKS__ */
sound/soc/codecs/tas2764.c +38 -0
@@ -46,6 +46,8 @@ struct tas2764_priv {bool unmuted; }; +#include "tas2764-quirks.h" +static const char *tas2764_int_ltch0_msgs[8 ] = {"fault: over temperature" ,/* INT_LTCH0 & BIT(0) */ "fault: over current" ,
@@ -123,6 +125,9 @@ static int tas2764_update_pwr_ctrl(struct tas2764_priv *tas2764)else val =TAS2764_PWR_CTRL_SHUTDOWN ; +if (ENABLED_APPLE_QUIRKS &TAS2764_SHUTDOWN_DANCE ) +return tas2764_do_quirky_pwr_ctrl_change (tas2764, val); + ret =snd_soc_component_update_bits (component,TAS2764_PWR_CTRL ,TAS2764_PWR_CTRL_MASK , val);if (ret <0 )
@@ -542,6 +547,30 @@ static uint8_t sn012776_bop_presets[] = {static const struct regmap_config tas2764_i2c_regmap; +static int tas2764_apply_init_quirks (struct tas2764_priv *tas2764) +{ +int ret, i; + +for (i =0 ; i <ARRAY_SIZE (tas2764_quirk_init_sequences); i++) { +const struct tas2764_quirk_init_sequence *init_seq = + &tas2764_quirk_init_sequences[i]; + +if (!init_seq->seq) +continue ; + +if (!(BIT (i) &ENABLED_APPLE_QUIRKS )) +continue ; + + ret =regmap_multi_reg_write (tas2764->regmap, init_seq->seq, + init_seq->len); + +if (ret <0 ) +return ret; + } + +return 0 ; +} +static int tas2764_codec_probe (struct snd_soc_component *component) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component);
@@ -627,6 +656,12 @@ static int tas2764_codec_probe(struct snd_soc_component *component)if (ret <0 )return ret; } + +/* Apply all enabled Apple quirks */ + ret =tas2764_apply_init_quirks (tas2764); + +if (ret <0 ) +return ret; }return 0 ;
@@ -706,6 +741,9 @@ static bool tas2764_volatile_register(struct device *dev, unsigned int reg) case TAS2764_INT_LTCH0 ... TAS2764_INT_LTCH4:case TAS2764_INT_CLK_CFG :return true; + case TAS2764_REG(0xf0, 0x0) ... TAS2764_REG(0xff, 0x0): +/* TI's undocumented registers for the application of quirks */ +return true;default :return false; }
Signature · 1 line
2.48.1
PATCH v2 12/29 ASoC: tas2764: Raise regmap range maximum
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
--- sound/soc/codecs/tas2764.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
sound/soc/codecs/tas2764.c +2 -2
@@ -726,7 +726,7 @@ static const struct reg_default tas2764_reg_defaults[] = {static const struct regmap_range_cfg tas2764_regmap_ranges[] = { { .range_min =0 , - .range_max =1 *128 , + .range_max =0xffff , .selector_reg =TAS2764_PAGE , .selector_mask =0xff , .selector_shift =0 ,
@@ -758,7 +758,7 @@ static const struct regmap_config tas2764_i2c_regmap = { .cache_type = REGCACHE_RBTREE, .ranges = tas2764_regmap_ranges, .num_ranges = ARRAY_SIZE(tas2764_regmap_ranges), - .max_register = 1 * 128, + .max_register = 0xffff, };static int tas2764_parse_dt (struct device *dev,struct tas2764_priv *tas2764)
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 12/29] ASoC: tas2764: Raise regmap range maximum
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Martin Povišer <[email protected]>Signed-off-by James Calligeros <[email protected]>
Because...? At least some changelog would be nice - why are we doing this, why does the new value make sense?
PATCH v2 13/29 ASoC: tas2770: Export 'die_temp' to sysfs
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
Export a file for the readout of die temperature measurements. As per the datasheet, the temperature can be calculated by dividing the register value by 16 and then subtracting 93.
--- sound/soc/codecs/tas2770.c | 57 +++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
sound/soc/codecs/tas2770.c +57 -0
@@ -20,6 +20,7 @@#include <linux/regmap.h> #include <linux/of.h> #include <linux/slab.h> +#include <linux/sysfs.h> #include <sound/soc.h> #include <sound/pcm.h> #include <sound/pcm_params.h>
@@ -491,6 +492,51 @@ static struct snd_soc_dai_driver tas2770_dai_driver[] = { }, }; +static int tas2770_read_die_temp (struct tas2770_priv *tas2770,int *result) +{ +int ret, reading; + + ret =snd_soc_component_read (tas2770->component,TAS2770_TEMP_MSB ); +if (ret <0 ) +return ret; + reading = ret <<4 ; + + ret =snd_soc_component_read (tas2770->component,TAS2770_TEMP_LSB ); +if (ret <0 ) +return ret; + reading |= ret >>4 ; + +/* +* As per datasheet: divide register by 16 and subtract 93. We don't +* want to divide just yet though. +*/ + *result = reading - (93 *16 ); +return 0 ; +} + +static ssize_t die_temp_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct tas2770_priv *tas2770 =i2c_get_clientdata (to_i2c_client (dev)); +int ret, temp; + + ret =tas2770_read_die_temp (tas2770, &temp); + +if (ret <0 ) +return ret; + +return sysfs_emit (buf,"%d.%03d C\n" , temp /16 , + (temp *1000 /16 ) %1000 ); +} + +static DEVICE_ATTR_RO (die_temp); + +static struct attribute *tas2770_sysfs_attrs[] = { + &dev_attr_die_temp.attr, +NULL +}; +ATTRIBUTE_GROUPS (tas2770_sysfs); +static const struct regmap_config tas2770_i2c_regmap;static int tas2770_codec_probe (struct snd_soc_component *component)
@@ -517,9 +563,19 @@ static int tas2770_codec_probe(struct snd_soc_component *component)return ret; } + ret =sysfs_create_groups (&component->dev->kobj, tas2770_sysfs_groups); + +if (ret <0 ) +return ret; +return 0 ; } +static void tas2770_codec_remove (struct snd_soc_component *component) +{ +sysfs_remove_groups (&component->dev->kobj, tas2770_sysfs_groups); +} + static DECLARE_TLV_DB_SCALE(tas2770_digital_tlv, 1100, 50, 0); static DECLARE_TLV_DB_SCALE(tas2770_playback_volume, -10050, 50, 0);
@@ -532,6 +588,7 @@ static const struct snd_kcontrol_new tas2770_snd_controls[] = {static const struct snd_soc_component_driver soc_component_driver_tas2770 = { .probe = tas2770_codec_probe, + .remove = tas2770_codec_remove, .suspend = tas2770_codec_suspend, .resume = tas2770_codec_resume, .controls = tas2770_snd_controls,
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 13/29] ASoC: tas2770: Export 'die_temp' to sysfs
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Export a file for the readout of die temperature measurements. As per the datasheet, the temperature can be calculated by dividing the register value by 16 and then subtracting 93.
Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
Re: [PATCH v2 13/29] ASoC: tas2770: Export 'die_temp' to sysfs
Quoted 7 lines · Mark Brown
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Export a file for the readout of die temperature measurements. As per the datasheet, the temperature can be calculated by dividing the register value by 16 and then subtracting 93.
Quoted 6 lines
Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
Which review comment...? The only thing I could find was about hwmon exporting, which James added a patch for in v2 ("ASoC: tas2770: expose die temp to hwmon"). Maybe I missed an email?
Re: [PATCH v2 13/29] ASoC: tas2770: Export 'die_temp' to sysfs
In reply to Mark BrownQuoted 7 lines · Mark Brown
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Export a file for the readout of die temperature measurements. As per the datasheet, the temperature can be calculated by dividing the register value by 16 and then subtracting 93.
Quoted 6 lines
Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
Oh, I see your later email. Serves me right for replying without reading the whole thing, sorry!
PATCH v2 14/29 ASoC: tas2770: expose die temp to hwmon
In reply to James CalligerosCreate and register a hwmon device to export the die temperature to the hwmon interface
--- sound/soc/codecs/tas2770.c | 69 +++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
sound/soc/codecs/tas2770.c +69 -0
@@ -12,6 +12,7 @@#include <linux/err.h> #include <linux/init.h> #include <linux/delay.h> +#include <linux/hwmon.h> #include <linux/pm.h> #include <linux/i2c.h> #include <linux/gpio/consumer.h>
@@ -537,6 +538,61 @@ static struct attribute *tas2770_sysfs_attrs[] = { };ATTRIBUTE_GROUPS (tas2770_sysfs); +#if defined(CONFIG_HWMON ) +static umode_t tas2770_hwmon_is_visible (const void *data, +enum hwmon_sensor_types type,u32 attr, +int channel) +{ +if (type != hwmon_temp) +return 0 ; + +switch (attr) { +case hwmon_temp_input: +return 0444 ; +default : +break ; + } + +return 0 ; +} + +static int tas2770_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +struct tas2770_priv *tas2770 =i2c_get_clientdata (to_i2c_client (dev)); +int ret; + +switch (attr) { +case hwmon_temp_input: + ret =tas2770_read_die_temp (tas2770, (int *)val); +if (!ret) + *val *=1000 ; +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } + +return ret; +} + +static const struct hwmon_channel_info *const tas2770_hwmon_info[] = { +HWMON_CHANNEL_INFO (temp,HWMON_T_INPUT ), +NULL +}; + +static const struct hwmon_ops tas2770_hwmon_ops = { + .is_visible = tas2770_hwmon_is_visible, + .read = tas2770_hwmon_read, +}; + +static const struct hwmon_chip_info tas2770_hwmon_chip_info = { + .ops = &tas2770_hwmon_ops, + .info = tas2770_hwmon_info, +}; +#endif +static const struct regmap_config tas2770_i2c_regmap;static int tas2770_codec_probe (struct snd_soc_component *component)
@@ -768,6 +824,19 @@ static int tas2770_i2c_probe(struct i2c_client *client)if (result)dev_err (tas2770->dev,"Register codec failed.\n" ); +if (IS_REACHABLE (CONFIG_HWMON )) { +struct device *hwmon; + + hwmon =devm_hwmon_device_register_with_info (&client->dev,"tas2770" , + tas2770, + &tas2770_hwmon_chip_info, +NULL ); +if (IS_ERR (hwmon)) { +return dev_err_probe (&client->dev,PTR_ERR (hwmon), +"Failed to register temp sensor\n" ); + } + } +return result; }
Signature · 1 line
2.48.1
Guenter Roeck
maintainerRe: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon
Quoted 21 lines · James Calligeros
Create and register a hwmon device to export the die temperature to the hwmon interface
Signed-off-by James Calligeros <[email protected]>--- sound/soc/codecs/tas2770.c | 69 +++++++++++++++++++++++++ 1 file changed, 69 insertions(+)sound/soc/codecs/tas2770.c +1
-0@@ -12,6 +12,7 @@#include <linux/err.h> #include <linux/init.h> #include <linux/delay.h> +#include <linux/hwmon.h> #include <linux/pm.h> #include <linux/i2c.h> #include <linux/gpio/consumer.h>
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 541
Quoted 4 lines
sound/soc/codecs/tas2770.c +1
-0@@ -537,6 +538,61 @@ static struct attribute *tas2770_sysfs_attrs[] = { };ATTRIBUTE_GROUPS (tas2770_sysfs); +#if defined(CONFIG_HWMON )
If this works with CONFIG_TAS2770=y and CONFIG_HWMON=m, the ifdef is unnececessary because IS_REACHABLE() is used below and the code will be optimized away if it is not reachable.
Quoted 17 lines
sound/soc/codecs/tas2770.c +17
-0+static umode_t tas2770_hwmon_is_visible (const void *data, +enum hwmon_sensor_types type,u32 attr, +int channel) +{ +if (type != hwmon_temp) +return 0 ; + +switch (attr) { +case hwmon_temp_input: +return 0444 ; +default : +break ; + } + +return 0 ; +} +
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 568
Quoted 10 lines
sound/soc/codecs/tas2770.c +10
-0+static int tas2770_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +struct tas2770_priv *tas2770 =i2c_get_clientdata (to_i2c_client (dev)); +int ret; + +switch (attr) { +case hwmon_temp_input: + ret =tas2770_read_die_temp (tas2770, (int *)val);
Type casting a pointer like this is never a good idea. This only works if sizeof(int) == sizeof(long).
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 570
Quoted 2 lines
sound/soc/codecs/tas2770.c +2
-0+if (!ret) + *val *=1000 ;
The calculations in the previous patch suggest that this is wrong.
Either case, this is redundant. The temperature is already displayed as device specific sysfs attribute. Displaying it twice does not make sense. I would suggest to either drop the sysfs attribute in the previous patch or to drop this patch.
Guenter
Quoted 49 lines
sound/soc/codecs/tas2770.c +38
-0+break ; +default : + ret = -EOPNOTSUPP ; + break; + } + +return ret; +} + +static const struct hwmon_channel_info *const tas2770_hwmon_info[] = { +HWMON_CHANNEL_INFO (temp,HWMON_T_INPUT ), +NULL +}; + +static const struct hwmon_ops tas2770_hwmon_ops = { + .is_visible = tas2770_hwmon_is_visible, + .read = tas2770_hwmon_read, +}; + +static const struct hwmon_chip_info tas2770_hwmon_chip_info = { + .ops = &tas2770_hwmon_ops, + .info = tas2770_hwmon_info, +}; +#endif +static const struct regmap_config tas2770_i2c_regmap;static int tas2770_codec_probe (struct snd_soc_component *component)@@ -768,6 +824,19 @@ static int tas2770_i2c_probe(struct i2c_client *client)if (result)dev_err (tas2770->dev,"Register codec failed.\n" ); +if (IS_REACHABLE (CONFIG_HWMON )) { +struct device *hwmon; + + hwmon =devm_hwmon_device_register_with_info (&client->dev,"tas2770" , + tas2770, + &tas2770_hwmon_chip_info, +NULL ); +if (IS_ERR (hwmon)) { +return dev_err_probe (&client->dev,PTR_ERR (hwmon), +"Failed to register temp sensor\n" ); + } + } +return result; }
Re: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon
Quoted 15 lines · Guenter Roeck
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 568
Quoted 10 lines · James Calligeros
sound/soc/codecs/tas2770.c +10
-0+static int tas2770_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +struct tas2770_priv *tas2770 =i2c_get_clientdata (to_i2c_client (dev)); +int ret; + +switch (attr) { +case hwmon_temp_input: + ret =tas2770_read_die_temp (tas2770, (int *)val);Type casting a pointer like this is never a good idea. This only works if sizeof(int) == sizeof(long).
I will rework this when dropping the die temp sysfs interface. This was mostly so that
I didn't have to change any of the code there, but since we're going to drop that
anyway it's redundant.
Quoted 9 lines
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 570
Quoted 2 lines
sound/soc/codecs/tas2770.c +2
-0+if (!ret) + *val *=1000 ;The calculations in the previous patch suggest that this is wrong.
Either case, this is redundant. The temperature is already displayed as device specific sysfs attribute. Displaying it twice does not make sense. I would suggest to either drop the sysfs attribute in the previous patch or to drop this patch.
The calculation in the datasheet yields the temperature in degrees Celsius. hwmon consumers expect temperatures in "millidegrees" Celsius as per the sysfs interface documentation[1]. Regardless, as above I will likely rework this when dropping the die temp sysfs interface so that things are a little more logical.
Regards,
James
[1] https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface
Guenter Roeck
maintainerRe: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon
Quoted 28 lines · James Calligeros
Quoted 15 lines · Guenter Roeck
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 568
Quoted 10 lines · James Calligeros
sound/soc/codecs/tas2770.c +10
-0+static int tas2770_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +struct tas2770_priv *tas2770 =i2c_get_clientdata (to_i2c_client (dev)); +int ret; + +switch (attr) { +case hwmon_temp_input: + ret =tas2770_read_die_temp (tas2770, (int *)val);Type casting a pointer like this is never a good idea. This only works if sizeof(int) == sizeof(long).
I will rework this when dropping the die temp sysfs interface. This was mostly so that
I didn't have to change any of the code there, but since we're going to drop that
anyway it's redundant.Quoted 5 lines
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 570
Quoted 2 lines
sound/soc/codecs/tas2770.c +2
-0+if (!ret) + *val *=1000 ;The calculations in the previous patch suggest that this is wrong.
Quoted 8 lines
Quoted 4 lines
Either case, this is redundant. The temperature is already displayed as device specific sysfs attribute. Displaying it twice does not make sense. I would suggest to either drop the sysfs attribute in the previous patch or to drop this patch.
The calculation in the datasheet yields the temperature in degrees Celsius. hwmon consumers expect temperatures in "millidegrees" Celsius as per the sysfs interface documentation[1]. Regardless, as above I will likely rework this
Yes, I am well aware of that.
Quoted 3 lines
when dropping the die temp sysfs interface so that things are a little more logical.
Unless I really misread the code, tas2770_read_die_temp() doesn't return the temperature in degrees C.
Guenter
Re: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon
Quoted 40 lines · Australian Eastern Standard Time Guenter Roeck
Quoted 32 lines · James Calligeros
Quoted 14 lines · Guenter Roeck
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 568
Quoted 10 lines · James Calligeros
sound/soc/codecs/tas2770.c +5
-0+static int tas2770_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ + struct tas2770_priv *tas2770 =i2c_get_clientdata(to_i2c_client(dev)); + int ret;
sound/soc/codecs/tas2770.c +4
-0+ +switch (attr) { +case hwmon_temp_input: + ret =tas2770_read_die_temp (tas2770, (int *)val);Type casting a pointer like this is never a good idea. This only works if sizeof(int) == sizeof(long).
I will rework this when dropping the die temp sysfs interface. This was mostly so that
I didn't have to change any of the code there, but since we're going to drop that
anyway it's redundant.Quoted 9 lines
sound/soc/codecs/tas2770.c static struct attribute *tas2770_sysfs_attrs[] = { · line 570
Quoted 2 lines
sound/soc/codecs/tas2770.c +2
-0+if (!ret) + *val *=1000 ;The calculations in the previous patch suggest that this is wrong.
Either case, this is redundant. The temperature is already displayed as device specific sysfs attribute. Displaying it twice does not make sense. I would suggest to either drop the sysfs attribute in the previous patch or to drop this patch.
Quoted 5 lines
The calculation in the datasheet yields the temperature in degrees Celsius.
hwmon consumers expect temperatures in "millidegrees" Celsius as per the sysfs interface documentation[1]. Regardless, as above I will likely rework thisYes, I am well aware of that.
Quoted 7 lines
Quoted 2 lines
when dropping the die temp sysfs interface so that things are a little more logical.
Unless I really misread the code,
tas2770_read_die_temp()doesn't return the temperature in degrees C.Guenter
My mistake. We return an intermediate value that is then manipulated in die_temp_show() to yield degrees. I will clean this up for the next submission since we will no longer require the sysfs interface at all. Apologies for the confusion.
Regards,
James
Mark Brown
maintainerRe: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon
In reply to James CalligerosQuoted 2 lines · James Calligeros
Create and register a hwmon device to export the die temperature to the hwmon interface
Oh, so there is actualy a hwmon device added (which was why I thought you were ignoring my review comments on the last patch...). The question then becomes why also have the custom ABI for this as well?
sysfs files are also supposed to be documented in Documention/ABI, though actal enforcement of that is a bit patchy.
PATCH v2 15/29 ASoC: tas2764: Export 'die_temp' to sysfs
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
Export a file for the readout of die temperature measurements. As per the datasheet, the temperature can be calculated by subtracting the value in the register by 93.
--- sound/soc/codecs/tas2764.c | 45 +++++++++++++++++++++++++ sound/soc/codecs/tas2764.h | 3 ++ 2 files changed, 48 insertions(+)
sound/soc/codecs/tas2764.c +45 -0
@@ -16,6 +16,7 @@#include <linux/of.h> #include <linux/of_device.h> #include <linux/slab.h> +#include <linux/sysfs.h> #include <sound/soc.h> #include <sound/pcm.h> #include <sound/pcm_params.h>
@@ -571,6 +572,39 @@ static int tas2764_apply_init_quirks(struct tas2764_priv *tas2764)return 0 ; } +static int tas2764_read_die_temp (struct tas2764_priv *tas2764,int *result) +{ +int ret; + + ret =snd_soc_component_read (tas2764->component,TAS2764_TEMP ); +if (ret <0 ) +return ret; + *result = ret -93 ; +return 0 ; +} + +static ssize_t die_temp_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct tas2764_priv *tas2764 =i2c_get_clientdata (to_i2c_client (dev)); +int ret, temp; + + ret =tas2764_read_die_temp (tas2764, &temp); + +if (ret <0 ) +return ret; + +return sysfs_emit (buf,"%d C\n" , temp); +} + +static DEVICE_ATTR_RO (die_temp); + +static struct attribute *tas2764_sysfs_attrs[] = { + &dev_attr_die_temp.attr, +NULL +}; +ATTRIBUTE_GROUPS (tas2764_sysfs); +static int tas2764_codec_probe (struct snd_soc_component *component) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component);
@@ -664,9 +698,19 @@ static int tas2764_codec_probe(struct snd_soc_component *component)return ret; } + ret =sysfs_create_groups (&component->dev->kobj, tas2764_sysfs_groups); + +if (ret <0 ) +return ret; +return 0 ; } +static void tas2764_codec_remove (struct snd_soc_component *component) +{ +sysfs_remove_groups (&component->dev->kobj, tas2764_sysfs_groups); +} + static DECLARE_TLV_DB_SCALE(tas2764_digital_tlv, 1100, 50, 0); static DECLARE_TLV_DB_SCALE(tas2764_playback_volume, -10050, 50, 1);
@@ -698,6 +742,7 @@ static const struct snd_kcontrol_new tas2764_snd_controls[] = {static const struct snd_soc_component_driver soc_component_driver_tas2764 = { .probe = tas2764_codec_probe, + .remove = tas2764_codec_remove, .suspend = tas2764_codec_suspend, .resume = tas2764_codec_resume, .controls = tas2764_snd_controls,
sound/soc/codecs/tas2764.h +3 -0
@@ -111,6 +111,9 @@#define TAS2764_INT_LTCH3 TAS2764_REG(0x0, 0x50)#define TAS2764_INT_LTCH4 TAS2764_REG(0x0, 0x51) +/* Readout Registers */ +#define TAS2764_TEMP TAS2764_REG(0x0, 0x56) +/* Clock/IRQ Settings */ #define TAS2764_INT_CLK_CFG TAS2764_REG(0x0, 0x5c)#define TAS2764_INT_CLK_CFG_IRQZ_CLR BIT(2)
Signature · 1 line
2.48.1
PATCH v2 16/29 ASoC: tas2764: expose die temp to hwmon
In reply to James CalligerosExpose the codec die temperature to the hwmon interface
--- sound/soc/codecs/tas2764.c | 70 +++++++++++++++++++++++++ 1 file changed, 70 insertions(+)
sound/soc/codecs/tas2764.c +70 -0
@@ -8,6 +8,7 @@#include <linux/err.h> #include <linux/init.h> #include <linux/delay.h> +#include <linux/hwmon.h> #include <linux/pm.h> #include <linux/i2c.h> #include <linux/gpio/consumer.h>
@@ -605,6 +606,61 @@ static struct attribute *tas2764_sysfs_attrs[] = { };ATTRIBUTE_GROUPS (tas2764_sysfs); +#if defined(CONFIG_HWMON ) +static umode_t tas2764_hwmon_is_visible (const void *data, +enum hwmon_sensor_types type,u32 attr, +int channel) +{ +if (type != hwmon_temp) +return 0 ; + +switch (attr) { +case hwmon_temp_input: +return 0444 ; +default : +break ; + } + +return 0 ; +} + +static int tas2764_hwmon_read (struct device *dev, +enum hwmon_sensor_types type, +u32 attr,int channel,long *val) +{ +struct tas2764_priv *tas2764 =i2c_get_clientdata (to_i2c_client (dev)); +int ret; + +switch (attr) { +case hwmon_temp_input: + ret =tas2764_read_die_temp (tas2764, (int *)val); +if (!ret) + *val *=1000 ; +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } + +return ret; +} + +static const struct hwmon_channel_info *const tas2764_hwmon_info[] = { +HWMON_CHANNEL_INFO (temp,HWMON_T_INPUT ), +NULL +}; + +static const struct hwmon_ops tas2764_hwmon_ops = { + .is_visible = tas2764_hwmon_is_visible, + .read = tas2764_hwmon_read, +}; + +static const struct hwmon_chip_info tas2764_hwmon_chip_info = { + .ops = &tas2764_hwmon_ops, + .info = tas2764_hwmon_info, +}; +#endif +static int tas2764_codec_probe (struct snd_soc_component *component) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component);
@@ -882,6 +938,20 @@ static int tas2764_i2c_probe(struct i2c_client *client) } } +if (IS_REACHABLE (CONFIG_HWMON )) { +struct device *hwmon; + + hwmon =devm_hwmon_device_register_with_info (&client->dev,"tas2764" , + tas2764, + &tas2764_hwmon_chip_info, +NULL ); +if (IS_ERR (hwmon)) { +return dev_err_probe (&client->dev,PTR_ERR (hwmon), +"Failed to register temp sensor\n" ); + } + } + + return devm_snd_soc_register_component(tas2764->dev, &soc_component_driver_tas2764, tas2764_dai_driver,
Signature · 1 line
2.48.1
PATCH v2 17/29 ASoC: tas2764: Crop SDOUT zero-out mask based on BCLK ratio
In reply to James CalligerosFrom: Martin Povišer <[email protected]>
--- sound/soc/codecs/tas2764.c | 56 +++++++++++++++++-------- 1 file changed, 39 insertions(+), 17 deletions(-)
sound/soc/codecs/tas2764.c +39 -17
@@ -376,6 +376,44 @@ static int tas2764_hw_params(struct snd_pcm_substream *substream,return tas2764_set_samplerate (tas2764,params_rate (params)); } +static int tas2764_write_sdout_zero_mask (struct tas2764_priv *tas2764,int bclk_ratio) +{ +struct snd_soc_component *component = tas2764->component; +int nsense_slots = bclk_ratio /8 ; +u32 cropped_mask; +int i, ret; + +if (!tas2764->sdout_zero_mask) +return 0 ; + + cropped_mask = tas2764->sdout_zero_mask &GENMASK (nsense_slots -1 ,0 ); + +for (i =0 ; i <4 ; i++) { + ret =snd_soc_component_write (component,TAS2764_SDOUT_HIZ_1 + i, + (cropped_mask >> (i *8 )) &0xff ); + +if (ret <0 ) +return ret; + } + + ret =snd_soc_component_update_bits (component,TAS2764_SDOUT_HIZ_9 , +TAS2764_SDOUT_HIZ_9_FORCE_0_EN , +TAS2764_SDOUT_HIZ_9_FORCE_0_EN ); + +if (ret <0 ) +return ret; + +return 0 ; +} + +static int tas2764_set_bclk_ratio (struct snd_soc_dai *dai,unsigned int ratio) +{ +struct snd_soc_component *component = dai->component; +struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component); + +return tas2764_write_sdout_zero_mask (tas2764, ratio); +} +static int tas2764_set_fmt (struct snd_soc_dai *dai,unsigned int fmt) {struct snd_soc_component *component = dai->component;
@@ -507,6 +545,7 @@ static int tas2764_set_dai_tdm_slot(struct snd_soc_dai *dai,static const struct snd_soc_dai_ops tas2764_dai_ops = { .mute_stream = tas2764_mute, .hw_params = tas2764_hw_params, + .set_bclk_ratio = tas2764_set_bclk_ratio, .set_fmt = tas2764_set_fmt, .set_tdm_slot = tas2764_set_dai_tdm_slot, .no_capture_mute =1 ,
@@ -714,23 +753,6 @@ static int tas2764_codec_probe(struct snd_soc_component *component)if (ret <0 )return ret; -if (tas2764->sdout_zero_mask) { -for (i =0 ; i <4 ; i++) { - ret =snd_soc_component_write (component,TAS2764_SDOUT_HIZ_1 + i, - (tas2764->sdout_zero_mask >> (i *8 )) &0xff ); - -if (ret <0 ) -return ret; - } - - ret =snd_soc_component_update_bits (component,TAS2764_SDOUT_HIZ_9 , -TAS2764_SDOUT_HIZ_9_FORCE_0_EN , -TAS2764_SDOUT_HIZ_9_FORCE_0_EN ); - -if (ret <0 ) -return ret; - } -if (tas2764->devid ==DEVID_SN012776 ) { ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL, TAS2764_PWR_CTRL_BOP_SRC,
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 17/29] ASoC: tas2764: Crop SDOUT zero-out mask based on BCLK ratio
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Martin Povišer <[email protected]>Signed-off-by James Calligeros <[email protected]>
Again, why do this? Is this a bug fix of some kind (ie, does the hardware misbehave if we get this wrong) or is it just for neatness?
Re: [PATCH v2 17/29] ASoC: tas2764: Crop SDOUT zero-out mask based on BCLK ratio
Quoted 12 lines · Mark Brown
Quoted 5 lines · James Calligeros
From: Martin Povišer <[email protected]>
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Martin Povišer <[email protected]>Signed-off-by James Calligeros <[email protected]>Again, why do this? Is this a bug fix of some kind (ie, does the hardware misbehave if we get this wrong) or is it just for neatness?
James,
I’ve found the language in the datasheet which made me make this change:
"This register to be programmed as zero in case the slot is not valid as per valid FSRATIO”
Martin
PATCH v2 18/29 ASoC: tas2764: Enable main IRQs
In reply to James CalligerosFrom: Hector Martin <[email protected]>
--- sound/soc/codecs/tas2764.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/tas2764.c +1 -1
@@ -716,7 +716,7 @@ static int tas2764_codec_probe(struct snd_soc_component *component)regmap_reinit_cache (tas2764->regmap, &tas2764_i2c_regmap);if (tas2764->irq) { - ret =snd_soc_component_write (tas2764->component,TAS2764_INT_MASK0 ,0xff ); + ret =snd_soc_component_write (tas2764->component,TAS2764_INT_MASK0 ,0x00 );if (ret <0 )return ret;
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 18/29] ASoC: tas2764: Enable main IRQs
Quoted 5 lines · James Calligeros
From: Hector Martin <[email protected]>
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Hector Martin <[email protected]>Signed-off-by James Calligeros <[email protected]>
Changelogs please...
sound/soc/codecs/tas2764.c static int tas2764_codec_probe(struct snd_soc_component *component) · line 721
Quoted 5 lines
sound/soc/codecs/tas2764.c +1
-1if (tas2764->irq) { - ret =snd_soc_component_write (tas2764->component,TAS2764_INT_MASK0 ,0xff ); + ret =snd_soc_component_write (tas2764->component,TAS2764_INT_MASK0 ,0x00 );if (ret <0 )return ret;
Do we have handling for these interrupts already?
PATCH v2 19/29 ASoC: tas2764: Power up/down amp on mute ops
In reply to James CalligerosFrom: Hector Martin <[email protected]>
The ASoC convention is that clocks are removed after codec mute, and power up/down is more about top level power management. For these chips, the "mute" state still expects a TDM clock, and yanking the clock in this state will trigger clock errors. So, do the full shutdown<->mute<->active transition on the mute operation, so the amp is in software shutdown by the time the clocks are removed.
This fixes TDM clock errors when streams are stopped.
--- sound/soc/codecs/tas2764.c | 51 ++++++++++--------------- 1 file changed, 21 insertions(+), 30 deletions(-)
sound/soc/codecs/tas2764.c +21 -30
@@ -195,33 +195,6 @@ static SOC_ENUM_SINGLE_DECL(static const struct snd_kcontrol_new tas2764_asi1_mux =SOC_DAPM_ENUM ("ASI1 Source" , tas2764_ASI1_src_enum); -static int tas2764_dac_event (struct snd_soc_dapm_widget *w, -struct snd_kcontrol *kcontrol,int event) -{ -struct snd_soc_component *component =snd_soc_dapm_to_component (w->dapm); -struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component); -int ret; - -switch (event) { -case SND_SOC_DAPM_POST_PMU : - tas2764->dac_powered = true; - ret =tas2764_update_pwr_ctrl (tas2764); -break ; -case SND_SOC_DAPM_PRE_PMD : - tas2764->dac_powered = false; - ret =tas2764_update_pwr_ctrl (tas2764); -break ; -default : -dev_err (tas2764->dev,"Unsupported event\n" ); -return -EINVAL ; - } - -if (ret <0 ) -return ret; - -return 0 ; -} -static const struct snd_kcontrol_new isense_switch =SOC_DAPM_SINGLE ("Switch" ,TAS2764_PWR_CTRL ,TAS2764_ISENSE_POWER_EN ,1 ,1 ); static const struct snd_kcontrol_new vsense_switch =
@@ -234,8 +207,7 @@ static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = { 1, &isense_switch),SND_SOC_DAPM_SWITCH ("VSENSE" ,TAS2764_PWR_CTRL ,TAS2764_VSENSE_POWER_EN ,1 , &vsense_switch), -SND_SOC_DAPM_DAC_E ("DAC" ,NULL ,SND_SOC_NOPM ,0 ,0 , tas2764_dac_event, -SND_SOC_DAPM_POST_PMU |SND_SOC_DAPM_PRE_PMD ), +SND_SOC_DAPM_DAC ("DAC" ,NULL ,SND_SOC_NOPM ,0 ,0 ),SND_SOC_DAPM_OUTPUT ("OUT" ),SND_SOC_DAPM_SIGGEN ("VMON" ),SND_SOC_DAPM_SIGGEN ("IMON" )
@@ -256,9 +228,28 @@ static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction) {struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (dai->component); +int ret; + +if (!mute) { + tas2764->dac_powered = true; + ret =tas2764_update_pwr_ctrl (tas2764); +if (ret) +return ret; + } tas2764->unmuted = !mute; -return tas2764_update_pwr_ctrl (tas2764); + ret =tas2764_update_pwr_ctrl (tas2764); +if (ret) +return ret; + +if (mute) { + tas2764->dac_powered = false; + ret =tas2764_update_pwr_ctrl (tas2764); +if (ret) +return ret; + } + +return 0 ; }static int tas2764_set_bitwidth (struct tas2764_priv *tas2764,int bitwidth)
Signature · 1 line
2.48.1
PATCH v2 20/29 ASoC: tas2764: Add SDZ regulator
In reply to James CalligerosFrom: Hector Martin <[email protected]>
Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for this breaks, as there is no concept of refcounting/sharing. In order to model these platforms, introduce support for an SDZ "regulator". This allows us to represent the SDZ GPIO as a simple regulator-fixed, and then the regulator core takes care of refcounting so that all codecs are only powered down once all the driver instances are in the suspend state.
--- sound/soc/codecs/tas2764.c | 39 ++++++++++++++++++++----- 1 file changed, 32 insertions(+), 7 deletions(-)
sound/soc/codecs/tas2764.c +32 -7
@@ -35,6 +35,7 @@ struct tas2764_priv {struct snd_soc_component *component;struct gpio_desc *reset_gpio;struct gpio_desc *sdz_gpio; +struct regulator *sdz_reg;struct regmap *regmap;struct device *dev;int irq;
@@ -154,6 +155,8 @@ static int tas2764_codec_suspend(struct snd_soc_component *component)if (tas2764->sdz_gpio)gpiod_set_value_cansleep (tas2764->sdz_gpio,0 ); +regulator_disable (tas2764->sdz_reg); +regcache_cache_only (tas2764->regmap, true);regcache_mark_dirty (tas2764->regmap);
@@ -165,19 +168,26 @@ static int tas2764_codec_resume(struct snd_soc_component *component)struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component);int ret; + ret =regulator_enable (tas2764->sdz_reg); + +if (ret) { +dev_err (tas2764->dev,"Failed to enable regulator\n" ); +return ret; + } +if (tas2764->sdz_gpio) {gpiod_set_value_cansleep (tas2764->sdz_gpio,1 ); -usleep_range (1000 ,2000 ); } - ret =tas2764_update_pwr_ctrl (tas2764); +usleep_range (1000 ,2000 ); +regcache_cache_only (tas2764->regmap, false); + + ret =regcache_sync (tas2764->regmap);if (ret <0 )return ret; -regcache_cache_only (tas2764->regmap, false); - - return regcache_sync(tas2764->regmap); + return tas2764_update_pwr_ctrl(tas2764); }#else #define tas2764_codec_suspend NULL
@@ -210,7 +220,7 @@ static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = {SND_SOC_DAPM_DAC ("DAC" ,NULL ,SND_SOC_NOPM ,0 ,0 ),SND_SOC_DAPM_OUTPUT ("OUT" ),SND_SOC_DAPM_SIGGEN ("VMON" ), -SND_SOC_DAPM_SIGGEN ("IMON" ) + SND_SOC_DAPM_SIGGEN("IMON"), }; static const struct snd_soc_dapm_route tas2764_audio_map[] = {
@@ -698,11 +708,18 @@ static int tas2764_codec_probe(struct snd_soc_component *component) tas2764->component = component; + ret =regulator_enable (tas2764->sdz_reg); +if (ret !=0 ) { +dev_err (tas2764->dev,"Failed to enable regulator: %d\n" , ret); +return ret; + } +if (tas2764->sdz_gpio) {gpiod_set_value_cansleep (tas2764->sdz_gpio,1 ); -usleep_range (1000 ,2000 ); } +usleep_range (1000 ,2000 ); +tas2764_reset (tas2764);regmap_reinit_cache (tas2764->regmap, &tas2764_i2c_regmap);
@@ -777,6 +794,9 @@ static int tas2764_codec_probe(struct snd_soc_component *component)static void tas2764_codec_remove (struct snd_soc_component *component) { +struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component); + +regulator_disable (tas2764->sdz_reg);sysfs_remove_groups (&component->dev->kobj, tas2764_sysfs_groups); }
@@ -879,6 +899,11 @@ static int tas2764_parse_dt(struct device *dev, struct tas2764_priv *tas2764) {int ret =0 ; + tas2764->sdz_reg =devm_regulator_get (dev,"SDZ" ); +if (IS_ERR (tas2764->sdz_reg)) +return dev_err_probe (dev,PTR_ERR (tas2764->sdz_reg), +"Failed to get SDZ supply\n" ); + tas2764->reset_gpio =devm_gpiod_get_optional (tas2764->dev,"reset" ,GPIOD_OUT_HIGH );if (IS_ERR (tas2764->reset_gpio)) {
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 20/29] ASoC: tas2764: Add SDZ regulator
Quoted 7 lines · James Calligeros
Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for this breaks, as there is no concept of refcounting/sharing. In order to model these platforms, introduce support for an SDZ "regulator". This allows us to represent the SDZ GPIO as a simple regulator-fixed, and then the regulator core takes care of refcounting so that all codecs are only powered down once all the driver instances are in the suspend state.
I get that the reference counting that the regulator API does is useful here but this isn't a regulator so shouldn't be exposed as such, particularly since this winds up being visible in the DT ABI. I could've sworn that someone did some helpers for this case but now I go looking I can't find them, we certainly don't use any in the regulator core.
sound/soc/codecs/tas2764.c static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = { · line 225
Quoted 8 lines
sound/soc/codecs/tas2764.c +1
-1@@ -210,7 +220,7 @@ static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = {SND_SOC_DAPM_DAC ("DAC" ,NULL ,SND_SOC_NOPM ,0 ,0 ),SND_SOC_DAPM_OUTPUT ("OUT" ),SND_SOC_DAPM_SIGGEN ("VMON" ), - SND_SOC_DAPM_SIGGEN("IMON") + SND_SOC_DAPM_SIGGEN("IMON"), };
Spurious unrelated change.
Re: [PATCH v2 20/29] ASoC: tas2764: Add SDZ regulator
Hi Mark,
Quoted 11 lines · Mark Brown
Quoted 7 lines · James Calligeros
Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for this breaks, as there is no concept of refcounting/sharing. In order to model these platforms, introduce support for an SDZ "regulator". This allows us to represent the SDZ GPIO as a simple regulator-fixed, and then the regulator core takes care of refcounting so that all codecs are only powered down once all the driver instances are in the suspend state.
Quoted 6 lines
I get that the reference counting that the regulator API does is useful here but this isn't a regulator so shouldn't be exposed as such, particularly since this winds up being visible in the DT ABI. I could've sworn that someone did some helpers for this case but now I go looking I can't find them, we certainly don't use any in the regulator core.
From what I recall, no attempt at shared GPIO infrastructure has actually landed. The multiple {de}assertions of SDZ put each chip on the same line into an unusable state that requires a full power cycle to clear, so we can't live without
handling the shared GPIO somewhat sensibly.
One alternative off the top of my head is adding a dummy reset controller to the DTs and integrating it into the ASoC machine driver (which we have downstream). We could then put the GPIO behind a shared reset line, and hit that instead of the GPIO. This does seem a little complex/odd, and IIRC we considered this at some point and decided against it.
Is there any other option that may work here? I'm open to ideas.
Regards,
James
Mark Brown
maintainerRe: [PATCH v2 20/29] ASoC: tas2764: Add SDZ regulator
Quoted 10 lines · James Calligeros
Quoted 8 lines · Mark Brown
On Tue, Feb 18, 2025 at 06:35:54PM +1000, James Calligeros wrote:
I get that the reference counting that the regulator API does is useful here but this isn't a regulator so shouldn't be exposed as such, particularly since this winds up being visible in the DT ABI. I could've sworn that someone did some helpers for this case but now I go looking I can't find them, we certainly don't use any in the regulator core.
Quoted 2 lines
From what I recall, no attempt at shared GPIO infrastructure has actually landed. The multiple {de}assertions of SDZ put each chip on the same line
Yeah, I can't find anything. Perhaps I was thinking of the reset API, most of the other users were reset lines so it's plausible someone started and then just ended up with the reset API instead.
Quoted 9 lines
into an unusable state that requires a full power cycle to clear, so we can't live without
handling the shared GPIO somewhat sensibly.One alternative off the top of my head is adding a dummy reset controller to the DTs and integrating it into the ASoC machine driver (which we have downstream). We could then put the GPIO behind a shared reset line, and hit that instead of the GPIO. This does seem a little complex/odd, and IIRC we considered this at some point and decided against it.
I'm not sure that's particularly better than the regulator version TBH, it's still got the problem of showing up in the device ABI.
Quoted 1 line
Is there any other option that may work here? I'm open to ideas.
Perhaps it's time to bite the bullet and do the shared GPIO API? regulator could certainly use it (and has a bunch of code, we could probably just pull that out and wrap an API around it?) and now there's this too.
You could possibly also open code, but that does beg the question about the shared API.
PATCH v2 21/29 ASoC: tas2764: Add reg defaults for TAS2764_INT_CLK_CFG
In reply to James CalligerosFrom: Hector Martin <[email protected]>
--- sound/soc/codecs/tas2764.c | 1 + 1 file changed, 1 insertion(+)
sound/soc/codecs/tas2764.c +1 -0
@@ -855,6 +855,7 @@ static const struct reg_default tas2764_reg_defaults[] = { { TAS2764_TDM_CFG2, 0x0a }, { TAS2764_TDM_CFG3, 0x10 }, { TAS2764_TDM_CFG5, 0x42 }, + { TAS2764_INT_CLK_CFG, 0x19 }, }; static const struct regmap_range_cfg tas2764_regmap_ranges[] = {
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 21/29] ASoC: tas2764: Add reg defaults for TAS2764_INT_CLK_CFG
Quoted 5 lines · James Calligeros
From: Hector Martin <[email protected]>
Reviewed-by Neal Gompa <[email protected]>Signed-off-by Hector Martin <[email protected]>Signed-off-by James Calligeros <[email protected]>
This is now upstream already, as are another couple of these patches. If nothing else please base your submissions on current code to avoid spurious conflicts.
PATCH v2 22/29 ASoC: tas2764: Mark SW_RESET as volatile
In reply to James CalligerosFrom: Hector Martin <[email protected]>
Since the bit is self-clearing.
--- sound/soc/codecs/tas2764.c | 1 + 1 file changed, 1 insertion(+)
sound/soc/codecs/tas2764.c +1 -0
@@ -873,6 +873,7 @@ static const struct regmap_range_cfg tas2764_regmap_ranges[] = {static bool tas2764_volatile_register (struct device *dev,unsigned int reg) {switch (reg) { +case TAS2764_SW_RST : case TAS2764_INT_LTCH0 ... TAS2764_INT_LTCH4:case TAS2764_INT_CLK_CFG :return true;
Signature · 1 line
2.48.1
PATCH v2 23/29 ASoC: tas2764: Wait for ramp-down after shutdown
In reply to James CalligerosFrom: Hector Martin <[email protected]>
When we shut down the amp, we need to wait for the built-in ramp-down before we can remove the TDM clocks. There is no documented status regiter to poll, so the best we can do is a delay. Datasheet says 5.9ms for ramp-down and 1.5ms between shutdown and next startup, so let's do 6ms after mute and 2ms after shutdown. That gives us a cumulative 8ms for ramp-down and guaratees the required minimum shutdown time.
--- sound/soc/codecs/tas2764.c | 8 ++++++++ 1 file changed, 8 insertions(+)
sound/soc/codecs/tas2764.c +8 -0
@@ -160,6 +160,8 @@ static int tas2764_codec_suspend(struct snd_soc_component *component)regcache_cache_only (tas2764->regmap, true);regcache_mark_dirty (tas2764->regmap); +usleep_range (6000 ,7000 ); +return 0 ; }
@@ -253,10 +255,16 @@ static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction)return ret;if (mute) { +/* Wait for ramp-down */ +usleep_range (6000 ,7000 ); + tas2764->dac_powered = false; ret =tas2764_update_pwr_ctrl (tas2764);if (ret)return ret; + +/* Wait a bit after shutdown */ +usleep_range (2000 ,3000 ); }return 0 ;
Signature · 1 line
2.48.1
PATCH v2 24/29 ASoC: tas2770: Add SDZ regulator
In reply to James CalligerosFrom: Hector Martin <[email protected]>
Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for this breaks, as there is no concept of refcounting/sharing. In order to model these platforms, introduce support for an SDZ "regulator". This allows us to represent the SDZ GPIO as a simple regulator-fixed, and then the regulator core takes care of refcounting so that all codecs are only powered down once all the driver instances are in the suspend state.
This also reworks the sleep/resume logic to copy what tas2764 does, which makes more sense.
--- sound/soc/codecs/tas2770.c | 67 ++++++++++++++++--------- sound/soc/codecs/tas2770.h | 1 + 2 files changed, 45 insertions(+), 23 deletions(-)
sound/soc/codecs/tas2770.c +44 -23
@@ -72,23 +72,21 @@ static int tas2770_codec_suspend(struct snd_soc_component *component)struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component);int ret =0 ; -regcache_cache_only (tas2770->regmap, true); -regcache_mark_dirty (tas2770->regmap); + ret =snd_soc_component_update_bits (component,TAS2770_PWR_CTRL , +TAS2770_PWR_CTRL_MASK , +TAS2770_PWR_CTRL_SHUTDOWN ); +if (ret <0 ) +return ret; -if (tas2770->sdz_gpio) { +if (tas2770->sdz_gpio)gpiod_set_value_cansleep (tas2770->sdz_gpio,0 ); - }else { - ret =snd_soc_component_update_bits (component,TAS2770_PWR_CTRL , -TAS2770_PWR_CTRL_MASK , -TAS2770_PWR_CTRL_SHUTDOWN ); -if (ret <0 ) { -regcache_cache_only (tas2770->regmap, false); -regcache_sync (tas2770->regmap); -return ret; - } - ret =0 ; - } +regulator_disable (tas2770->sdz_reg); + +regcache_cache_only (tas2770->regmap, true); +regcache_mark_dirty (tas2770->regmap); + +usleep_range (6000 ,7000 );return ret; }
@@ -98,18 +96,26 @@ static int tas2770_codec_resume(struct snd_soc_component *component)struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component);int ret; -if (tas2770->sdz_gpio) { -gpiod_set_value_cansleep (tas2770->sdz_gpio,1 ); -usleep_range (1000 ,2000 ); - }else { - ret =tas2770_update_pwr_ctrl (tas2770); -if (ret <0 ) -return ret; + ret =regulator_enable (tas2770->sdz_reg); + +if (ret) { +dev_err (tas2770->dev,"Failed to enable regulator\n" ); +return ret; } +if (tas2770->sdz_gpio) +gpiod_set_value_cansleep (tas2770->sdz_gpio,1 ); + + +usleep_range (1000 ,2000 ); +regcache_cache_only (tas2770->regmap, false); - return regcache_sync(tas2770->regmap); + ret =regcache_sync (tas2770->regmap); +if (ret <0 ) +return ret; + + return tas2770_update_pwr_ctrl(tas2770); }#else #define tas2770_codec_suspend NULL
@@ -603,11 +609,18 @@ static int tas2770_codec_probe(struct snd_soc_component *component) tas2770->component = component; + ret =regulator_enable (tas2770->sdz_reg); +if (ret !=0 ) { +dev_err (tas2770->dev,"Failed to enable regulator: %d\n" , ret); +return ret; + } +if (tas2770->sdz_gpio) {gpiod_set_value_cansleep (tas2770->sdz_gpio,1 ); -usleep_range (1000 ,2000 ); } +usleep_range (1000 ,2000 ); +tas2770_reset (tas2770);regmap_reinit_cache (tas2770->regmap, &tas2770_i2c_regmap);
@@ -629,7 +642,10 @@ static int tas2770_codec_probe(struct snd_soc_component *component)static void tas2770_codec_remove (struct snd_soc_component *component) { +struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component); +sysfs_remove_groups (&component->dev->kobj, tas2770_sysfs_groups); +regulator_disable (tas2770->sdz_reg); } static DECLARE_TLV_DB_SCALE(tas2770_digital_tlv, 1100, 50, 0);
@@ -769,6 +785,11 @@ static int tas2770_parse_dt(struct device *dev, struct tas2770_priv *tas2770) tas2770->v_sense_slot =-1 ; } + tas2770->sdz_reg =devm_regulator_get (dev,"SDZ" ); +if (IS_ERR (tas2770->sdz_reg)) +return dev_err_probe (dev,PTR_ERR (tas2770->sdz_reg), +"Failed to get SDZ supply\n" ); + tas2770->sdz_gpio =devm_gpiod_get_optional (dev,"shutdown" ,GPIOD_OUT_HIGH );if (IS_ERR (tas2770->sdz_gpio)) {if (PTR_ERR (tas2770->sdz_gpio) == -EPROBE_DEFER )
sound/soc/codecs/tas2770.h +1 -0
@@ -134,6 +134,7 @@ struct tas2770_priv {struct snd_soc_component *component;struct gpio_desc *reset_gpio;struct gpio_desc *sdz_gpio; +struct regulator *sdz_reg;struct regmap *regmap;struct device *dev;int v_sense_slot;
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 24/29] ASoC: tas2770: Add SDZ regulator
Quoted 9 lines · James Calligeros
From: Hector Martin <[email protected]>
Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for this breaks, as there is no concept of refcounting/sharing. In order to model these platforms, introduce support for an SDZ "regulator". This allows us to represent the SDZ GPIO as a simple regulator-fixed, and then the regulator core takes care of refcounting so that all codecs are only powered down once all the driver instances are in the suspend state.
Same issue with the use of the regulator API winding up as ABI here as with the earlier patch - I get why you're doing this pragmatically and we should just have a general helper for refcounted GPIO enables.
PATCH v2 25/29 ASoC: tas2770: Power cycle amp on ISENSE/VSENSE change
In reply to James CalligerosFrom: Hector Martin <[email protected]>
The ISENSE/VSENSE blocks are only powered up when the amplifier transitions from shutdown to active. This means that if those controls are flipped on while the amplifier is already playing back audio, they will have no effect.
Fix this by forcing a power cycle around transitions in those controls.
--- sound/soc/codecs/tas2770.c | 30 +++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
sound/soc/codecs/tas2770.c +28 -2
@@ -164,11 +164,37 @@ static const struct snd_kcontrol_new isense_switch =static const struct snd_kcontrol_new vsense_switch =SOC_DAPM_SINGLE ("Switch" ,TAS2770_PWR_CTRL ,2 ,1 ,1 ); +static int sense_event (struct snd_soc_dapm_widget *w, +struct snd_kcontrol *kcontrol,int event) +{ +struct snd_soc_component *component =snd_soc_dapm_to_component (w->dapm); +struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component); + +/* +* Powering up ISENSE/VSENSE requires a trip through the shutdown state. +* Do that here to ensure that our changes are applied properly, otherwise +* we might end up with non-functional IVSENSE if playback started earlier, +* which would break software speaker protection. +*/ +switch (event) { +case SND_SOC_DAPM_PRE_REG : +return snd_soc_component_update_bits (component,TAS2770_PWR_CTRL , +TAS2770_PWR_CTRL_MASK , +TAS2770_PWR_CTRL_SHUTDOWN ); +case SND_SOC_DAPM_POST_REG : +return tas2770_update_pwr_ctrl (tas2770); +default : +return 0 ; + } +} +static const struct snd_soc_dapm_widget tas2770_dapm_widgets[] = {SND_SOC_DAPM_AIF_IN ("ASI1" ,"ASI1 Playback" ,0 ,SND_SOC_NOPM ,0 ,0 ),SND_SOC_DAPM_MUX ("ASI1 Sel" ,SND_SOC_NOPM ,0 ,0 , &tas2770_asi1_mux), -SND_SOC_DAPM_SWITCH ("ISENSE" ,TAS2770_PWR_CTRL ,3 ,1 , &isense_switch), -SND_SOC_DAPM_SWITCH ("VSENSE" ,TAS2770_PWR_CTRL ,2 ,1 , &vsense_switch), +SND_SOC_DAPM_SWITCH_E ("ISENSE" ,TAS2770_PWR_CTRL ,3 ,1 , &isense_switch, + sense_event,SND_SOC_DAPM_PRE_REG |SND_SOC_DAPM_POST_REG ), +SND_SOC_DAPM_SWITCH_E ("VSENSE" ,TAS2770_PWR_CTRL ,2 ,1 , &vsense_switch, + sense_event,SND_SOC_DAPM_PRE_REG |SND_SOC_DAPM_POST_REG ),SND_SOC_DAPM_DAC_E ("DAC" ,NULL ,SND_SOC_NOPM ,0 ,0 , tas2770_dac_event,SND_SOC_DAPM_POST_PMU |SND_SOC_DAPM_PRE_PMD ),SND_SOC_DAPM_OUTPUT ("OUT" ),
Signature · 1 line
2.48.1
PATCH v2 26/29 ASoC: tas2770: Add zero-fill and pull-down controls
In reply to James CalligerosFrom: Hector Martin <[email protected]>
Expose the bits that control the behavior of the SDOUT pin when not actively transmitting slot data. Zero-fill is useful when there is a single amp on the SDOUT bus (e.g. Apple machines with mono speakers or a single stereo pair, where L/R are on separate buses).
Pull-down is useful, though not perfect, when multiple amps share a bus. It typically takes around 2 bits for the line to transition from high to low after going Hi-Z, with the pull-down.
--- sound/soc/codecs/tas2770.c | 17 +++++++++++++++++ sound/soc/codecs/tas2770.h | 13 +++++++++++++ 2 files changed, 30 insertions(+)
sound/soc/codecs/tas2770.c +17 -0
@@ -658,6 +658,20 @@ static int tas2770_codec_probe(struct snd_soc_component *component)return ret; } + ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG4 , +TAS2770_TDM_CFG_REG4_TX_FILL , + tas2770->sdout_zfill ?0 : +TAS2770_TDM_CFG_REG4_TX_FILL ); +if (ret <0 ) +return ret; + + ret =snd_soc_component_update_bits (component,TAS2770_DIN_PD , +TAS2770_DIN_PD_SDOUT , + tas2770->sdout_pd ? +TAS2770_DIN_PD_SDOUT :0 ); +if (ret <0 ) +return ret; + ret =sysfs_create_groups (&component->dev->kobj, tas2770_sysfs_groups);if (ret <0 )
@@ -811,6 +825,9 @@ static int tas2770_parse_dt(struct device *dev, struct tas2770_priv *tas2770) tas2770->v_sense_slot =-1 ; } + tas2770->sdout_pd =fwnode_property_read_bool (dev->fwnode,"ti,sdout-pull-down" ); + tas2770->sdout_zfill =fwnode_property_read_bool (dev->fwnode,"ti,sdout-zero-fill" ); + tas2770->sdz_reg =devm_regulator_get (dev,"SDZ" );if (IS_ERR (tas2770->sdz_reg)) return dev_err_probe(dev, PTR_ERR(tas2770->sdz_reg),
sound/soc/codecs/tas2770.h +13 -0
@@ -67,6 +67,14 @@#define TAS2770_TDM_CFG_REG3_RXS_SHIFT 0x4#define TAS2770_TDM_CFG_REG3_30_MASK GENMASK(3, 0)#define TAS2770_TDM_CFG_REG3_30_SHIFT 0 +/* TDM Configuration Reg4 */ +#define TAS2770_TDM_CFG_REG4 TAS2770_REG(0X0, 0x0E) +#define TAS2770_TDM_CFG_REG4_TX_LSB_CFG BIT(7) +#define TAS2770_TDM_CFG_REG4_TX_KEEPER_CFG BIT(6) +#define TAS2770_TDM_CFG_REG4_TX_KEEPER BIT(5) +#define TAS2770_TDM_CFG_REG4_TX_FILL BIT(4) +#define TAS2770_TDM_CFG_REG4_TX_OFFSET_MASK GENMASK(3, 1) +#define TAS2770_TDM_CFG_REG4_TX_EDGE_FALLING BIT(0)/* TDM Configuration Reg5 */ #define TAS2770_TDM_CFG_REG5 TAS2770_REG(0X0, 0x0F)#define TAS2770_TDM_CFG_REG5_VSNS_MASK BIT(6)
@@ -110,6 +118,9 @@#define TAS2770_TEMP_LSB TAS2770_REG(0X0, 0x2A)/* Interrupt Configuration */ #define TAS2770_INT_CFG TAS2770_REG(0X0, 0x30) +/* Data In Pull-Down */ +#define TAS2770_DIN_PD TAS2770_REG(0X0, 0x31) +#define TAS2770_DIN_PD_SDOUT BIT(7)/* Misc IRQ */ #define TAS2770_MISC_IRQ TAS2770_REG(0X0, 0x32)/* Clock Configuration */
@@ -139,6 +150,8 @@ struct tas2770_priv {struct device *dev;int v_sense_slot;int i_sense_slot; +bool sdout_pd; +bool sdout_zfill;bool dac_powered;bool unmuted; };
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 26/29] ASoC: tas2770: Add zero-fill and pull-down controls
Quoted 6 lines · James Calligeros
From: Hector Martin <[email protected]>
Expose the bits that control the behavior of the SDOUT pin when not actively transmitting slot data. Zero-fill is useful when there is a single amp on the SDOUT bus (e.g. Apple machines with mono speakers or a single stereo pair, where L/R are on separate buses).
This is adding a DT property, please add a bindings update.
PATCH v2 27/29 ASoC: tas2770: Support setting the PDM TX slot
In reply to James CalligerosFrom: Hector Martin <[email protected]>
We don't actually support configuring the PDM input right now. Rather, this is useful as a hack.
On Apple Silicon machines, amps are split between two I2S buses which are logically ANDed internally at the SoC. Odd and even slot groups are driven by amps on either bus respectively. Since the signals are ANDed, unused slot groups must be driven as zero to avoid corrupting the data from the other side.
On most recent machines (TAS2764-based), this is accomplished using the "SDOUT zero mask" feature of that chip. Unfortunately, TAS2770 does not support this. It does support zeroing out *all* unused slots, which works well for machines with a single amp per I2S bus. That is all, except one.
The 13" M1 MacBook Pro is the only machine using TAS2770 and two amps per I2S bus:
L Bus: SPK0I SPK0V Hi-Z Hi-Z SPK2I SPK2V Hi-Z Hi-Z R Bus: Hi-Z Hi-Z SPK1I SPK2V Hi-Z Hi-Z SPK3I SPK3V
To ensure uncorrupted data, we need to force all the Hi-Z periods to zero. We cannot use the "force all zero" feature, as that would cause a bus conflict between both amps. We can use the pull-down feature, but that leaves a few bits of garbage on the trailing edge of the speaker data, since the pull-down is weak.
This is where the PDM transmit feature comes in. With PDM grounded and disabled (the default state), the PDM slot is transmitted as all zeroes. We can use that to force a zero 16-bit slot after the voltage data for each speaker, cleaning it up. Then the pull-down ensures the line stays low for the subsequent slot:
L Bus: SPK0I SPK0V PDM0 PulDn SPK2I SPK2V PDM0 PulDn R Bus: PDM0 PulDn SPK1I SPK2V PDM0 PulDn SPK3I SPK3V
Yes, this is a horrible hack, but it beats adding dummy slots that would be visible to the userspace capture side. There may be some other way to fix the logical AND behavior on the MCA side... that would make this unnecessary.
("How does Apple deal with this"? - they don't, macOS does not use IVSENSE on TAS2770 machines even though it's physically wired up, but we want to do so on Linux.)
--- sound/soc/codecs/tas2770.c | 25 +++++++++++++++++++++++++ sound/soc/codecs/tas2770.h | 6 ++++++ 2 files changed, 31 insertions(+)
sound/soc/codecs/tas2770.c +25 -0
@@ -248,6 +248,19 @@ static int tas2770_set_ivsense_transmit(struct tas2770_priv *tas2770,return 0 ; } +static int tas2770_set_pdm_transmit (struct tas2770_priv *tas2770,int slot) +{ +struct snd_soc_component *component = tas2770->component; +int ret; + + ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG7 , +TAS2770_TDM_CFG_REG7_PDM_MASK | +TAS2770_TDM_CFG_REG7_50_MASK , +TAS2770_TDM_CFG_REG7_PDM_ENABLE | + slot); +return ret; +} +static int tas2770_set_bitwidth (struct tas2770_priv *tas2770,int bitwidth) {int ret;
@@ -658,6 +671,13 @@ static int tas2770_codec_probe(struct snd_soc_component *component)return ret; } +if (tas2770->pdm_slot !=-1 ) { + ret =tas2770_set_pdm_transmit (tas2770, tas2770->pdm_slot); + +if (ret <0 ) +return ret; + } + ret = snd_soc_component_update_bits(component, TAS2770_TDM_CFG_REG4, TAS2770_TDM_CFG_REG4_TX_FILL, tas2770->sdout_zfill ? 0 :
@@ -825,6 +845,11 @@ static int tas2770_parse_dt(struct device *dev, struct tas2770_priv *tas2770) tas2770->v_sense_slot =-1 ; } + rc =fwnode_property_read_u32 (dev->fwnode,"ti,pdm-slot-no" , + &tas2770->pdm_slot); +if (rc) + tas2770->pdm_slot =-1 ; + tas2770->sdout_pd =fwnode_property_read_bool (dev->fwnode,"ti,sdout-pull-down" ); tas2770->sdout_zfill =fwnode_property_read_bool (dev->fwnode,"ti,sdout-zero-fill" );
sound/soc/codecs/tas2770.h +6 -0
@@ -85,6 +85,11 @@#define TAS2770_TDM_CFG_REG6_ISNS_MASK BIT(6)#define TAS2770_TDM_CFG_REG6_ISNS_ENABLE BIT(6)#define TAS2770_TDM_CFG_REG6_50_MASK GENMASK(5, 0) +/* TDM Configuration Reg10 */ +#define TAS2770_TDM_CFG_REG7 TAS2770_REG(0X0, 0x11) +#define TAS2770_TDM_CFG_REG7_PDM_MASK BIT(6) +#define TAS2770_TDM_CFG_REG7_PDM_ENABLE BIT(6) +#define TAS2770_TDM_CFG_REG7_50_MASK GENMASK(5, 0)/* Brown Out Prevention Reg0 */ #define TAS2770_BO_PRV_REG0 TAS2770_REG(0X0, 0x1B)/* Interrupt MASK Reg0 */
@@ -150,6 +155,7 @@ struct tas2770_priv {struct device *dev;int v_sense_slot;int i_sense_slot; +int pdm_slot;bool sdout_pd;bool sdout_zfill;bool dac_powered;
Signature · 1 line
2.48.1
PATCH v2 28/29 ASoC: tas2764: Set the SDOUT polarity correctly
In reply to James CalligerosFrom: Hector Martin <[email protected]>
TX launch polarity needs to be the opposite of RX capture polarity, to generate the right bit slot alignment.
--- sound/soc/codecs/tas2764.c | 10 +++++++++- sound/soc/codecs/tas2764.h | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-)
sound/soc/codecs/tas2764.c +9 -1
@@ -427,7 +427,7 @@ static int tas2764_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) {struct snd_soc_component *component = dai->component;struct tas2764_priv *tas2764 =snd_soc_component_get_drvdata (component); -u8 tdm_rx_start_slot =0 , asi_cfg_0 =0 , asi_cfg_1 =0 ; +u8 tdm_rx_start_slot =0 , asi_cfg_0 =0 , asi_cfg_1 =0 , asi_cfg_4 =0 ;int ret;switch (fmt &SND_SOC_DAIFMT_INV_MASK ) {
@@ -436,12 +436,14 @@ static int tas2764_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) fallthrough;case SND_SOC_DAIFMT_NB_NF : asi_cfg_1 =TAS2764_TDM_CFG1_RX_RISING ; + asi_cfg_4 =TAS2764_TDM_CFG4_TX_FALLING ;break ;case SND_SOC_DAIFMT_IB_IF : asi_cfg_0 ^=TAS2764_TDM_CFG0_FRAME_START ; fallthrough;case SND_SOC_DAIFMT_IB_NF : asi_cfg_1 =TAS2764_TDM_CFG1_RX_FALLING ; + asi_cfg_4 =TAS2764_TDM_CFG4_TX_RISING ;break ; }
@@ -451,6 +453,12 @@ static int tas2764_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)if (ret <0 )return ret; + ret =snd_soc_component_update_bits (component,TAS2764_TDM_CFG4 , +TAS2764_TDM_CFG4_TX_MASK , + asi_cfg_4); +if (ret <0 ) +return ret; +switch (fmt &SND_SOC_DAIFMT_FORMAT_MASK ) {case SND_SOC_DAIFMT_I2S : asi_cfg_0 ^=TAS2764_TDM_CFG0_FRAME_START ;
sound/soc/codecs/tas2764.h +6 -0
@@ -84,6 +84,12 @@#define TAS2764_TDM_CFG3_RXS_SHIFT 0x4#define TAS2764_TDM_CFG3_MASK GENMASK(3, 0) +/* TDM Configuration Reg4 */ +#define TAS2764_TDM_CFG4 TAS2764_REG(0X0, 0x0d) +#define TAS2764_TDM_CFG4_TX_MASK BIT(0) +#define TAS2764_TDM_CFG4_TX_RISING 0x0 +#define TAS2764_TDM_CFG4_TX_FALLING BIT(0) +/* TDM Configuration Reg5 */ #define TAS2764_TDM_CFG5 TAS2764_REG(0X0, 0x0e)#define TAS2764_TDM_CFG5_VSNS_MASK BIT(6)
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: [PATCH v2 28/29] ASoC: tas2764: Set the SDOUT polarity correctly
Quoted 4 lines · James Calligeros
From: Hector Martin <[email protected]>
TX launch polarity needs to be the opposite of RX capture polarity, to generate the right bit slot alignment.
Fixes should go at the start of the series.
PATCH v2 29/29 ASoC: tas2770: Set the SDOUT polarity correctly
In reply to James CalligerosFrom: Hector Martin <[email protected]>
TX launch polarity needs to be the opposite of RX capture polarity, to generate the right bit slot alignment.
--- sound/soc/codecs/tas2770.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
sound/soc/codecs/tas2770.c +8 -1
@@ -359,7 +359,7 @@ static int tas2770_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)struct snd_soc_component *component = dai->component;struct tas2770_priv *tas2770 =snd_soc_component_get_drvdata (component); -u8 tdm_rx_start_slot =0 , invert_fpol =0 , fpol_preinv =0 , asi_cfg_1 =0 ; +u8 tdm_rx_start_slot =0 , invert_fpol =0 , fpol_preinv =0 , asi_cfg_1 =0 , asi_cfg_4 =0 ;int ret;switch (fmt &SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK ) {
@@ -376,6 +376,7 @@ static int tas2770_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) fallthrough;case SND_SOC_DAIFMT_NB_NF : asi_cfg_1 |=TAS2770_TDM_CFG_REG1_RX_RSING ; + asi_cfg_4 |=TAS2770_TDM_CFG_REG4_TX_EDGE_FALLING ;break ;case SND_SOC_DAIFMT_IB_IF : invert_fpol =1 ;
@@ -394,6 +395,12 @@ static int tas2770_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)if (ret <0 )return ret; + ret =snd_soc_component_update_bits (component,TAS2770_TDM_CFG_REG4 , +TAS2770_TDM_CFG_REG4_TX_EDGE_FALLING , + asi_cfg_4); +if (ret <0 ) +return ret; +switch (fmt &SND_SOC_DAIFMT_FORMAT_MASK ) {case SND_SOC_DAIFMT_I2S : tdm_rx_start_slot =1 ;
Signature · 1 line
2.48.1
Mark Brown
maintainerRe: (subset) [PATCH v2 00/29] ASoC: tas27{64,70}: improve support for Apple codec variants
In reply to James CalligerosQuoted 11 lines · James Calligeros
This series introduces a number of changes to the drivers for the Texas Instruments TAS2764 and TAS2770 amplifiers in order to introduce (and improve in the case of TAS2770) support for the variants of these amps found in Apple Silicon Macs.
Apple's variant of TAS2764 is known as SN012776, and as always with Apple is a subtly incompatible variant with a number of quirks. It is not publicly available. The TAS2770 variant is known as TAS5770L, and does not require incompatible handling.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[01/29] ASoC: tas2764: Fix power control mask
commit: a3f172359e22b2c11b750d23560481a55bf86af1
[09/29] ASoC: tas2764: Reinit cache on part reset
(no commit info)
[28/29] ASoC: tas2764: Set the SDOUT polarity correctly
commit: f5468beeab1b1adfc63c2717b1f29ef3f49a5fab[29/29] ASoC: tas2770: Set the SDOUT polarity correctly
(no commit info)
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