Re: [PATCH v2 4/5] ASoC: meson: s4:support audio tocodec
From: Jerome Brunet <jbrunet@baylibre.com>
Date: 2025-02-14 09:51:48
Also in:
linux-amlogic, linux-clk, linux-devicetree, linux-sound, lkml
On Fri 14 Feb 2025 at 10:13, jiebing chen via B4 Relay [off-list ref] wrote:
From: jiebing chen <redacted> Add the audio tocodec for s4, add the 8 lane support, add the mclk and sclk enable event when start data enable auto switch
Again, incomplete description and mixing things together.
quoted hunk
Signed-off-by: jiebing chen <redacted> --- sound/soc/meson/axg-card.c | 3 +- sound/soc/meson/g12a-toacodec.c | 64 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-)diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c index a2dfccb7990f3a53f508fc6724b21de53b4494d8..5cef069c3370257d4aaf24d7270482651babcfe1 100644 --- a/sound/soc/meson/axg-card.c +++ b/sound/soc/meson/axg-card.c@@ -303,7 +303,8 @@ static int axg_card_cpu_is_tdm_iface(struct device_node *np) static int axg_card_cpu_is_codec(struct device_node *np) { return of_device_is_compatible(np, DT_PREFIX "g12a-tohdmitx") || - of_device_is_compatible(np, DT_PREFIX "g12a-toacodec"); + of_device_is_compatible(np, DT_PREFIX "g12a-toacodec") || + of_device_is_compatible(np, DT_PREFIX "s4-toacodec");
There is no need to extend that indefinitely, use fall-back
quoted hunk
} static int axg_card_add_link(struct snd_soc_card *card, struct device_node *np,diff --git a/sound/soc/meson/g12a-toacodec.c b/sound/soc/meson/g12a-toacodec.c index 531bb8707a3ec4c47814d6a0676d5c62c705da75..a93a91136e8ea00e856c3981b9c1e7e08d927a3b 100644 --- a/sound/soc/meson/g12a-toacodec.c +++ b/sound/soc/meson/g12a-toacodec.c@@ -41,6 +41,9 @@ #define CTRL0_BCLK_SEL_LSB 4 #define CTRL0_MCLK_SEL GENMASK(2, 0) +#define CTRL0_BCLK_ENABLE_SHIFT 30 +#define CTRL0_MCLK_ENABLE_SHIFT 29 + #define TOACODEC_OUT_CHMAX 2 struct g12a_toacodec {@@ -107,6 +110,33 @@ static int g12a_toacodec_mux_put_enum(struct snd_kcontrol *kcontrol, return 1; } +static int tocodec_clk_enable(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *control, + int event) +{ + int ret = 0; + unsigned int mask = 0, val = 0; + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); +
Over complicated for no reason
+ switch (event) {
+ case SND_SOC_DAPM_PRE_PMU:
+ mask = 1 << CTRL0_MCLK_ENABLE_SHIFT | 1 << CTRL0_BCLK_ENABLE_SHIFT;
+ val = 1 << CTRL0_MCLK_ENABLE_SHIFT | 1 << CTRL0_BCLK_ENABLE_SHIFT;
+ ret = snd_soc_component_update_bits(component, TOACODEC_CTRL0, mask, val);All this could be done in one line and be actually readable if you properly used the BIT() macro.
+ break; + case SND_SOC_DAPM_PRE_PMD: + mask = 1 << CTRL0_MCLK_ENABLE_SHIFT | 1 << CTRL0_BCLK_ENABLE_SHIFT; + val = 0 << CTRL0_MCLK_ENABLE_SHIFT | 0 << CTRL0_BCLK_ENABLE_SHIFT; + ret = snd_soc_component_update_bits(component, TOACODEC_CTRL0, mask, val); + break; + default: + dev_err(component->dev, "Unexpected event %d\n", event); + return -EINVAL; + }
... and nothing explains what is being done and why ...
quoted hunk
+ + return ret; +} + static SOC_ENUM_SINGLE_DECL(g12a_toacodec_mux_enum, TOACODEC_CTRL0, CTRL0_DAT_SEL_LSB, g12a_toacodec_mux_texts);@@ -143,6 +173,14 @@ static const struct snd_soc_dapm_widget sm1_toacodec_widgets[] = { &g12a_toacodec_out_enable), }; +static const struct snd_soc_dapm_widget s4_toacodec_widgets[] = { + SND_SOC_DAPM_MUX("SRC", SND_SOC_NOPM, 0, 0, + &sm1_toacodec_mux), + SND_SOC_DAPM_SWITCH_E("OUT EN", SND_SOC_NOPM, 0, 0, + &g12a_toacodec_out_enable, tocodec_clk_enable, + (SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD)),
Drops the .autodisable without a even comment AFAICT, could be done like the other SoC with SOC_SINGLE_AUTODISABLE() with properly chosen values.
quoted hunk
+}; + static int g12a_toacodec_input_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)@@ -236,6 +274,10 @@ static const struct snd_kcontrol_new sm1_toacodec_controls[] = { SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL_SM1, 3, 0), }; +static const struct snd_kcontrol_new s4_toacodec_controls[] = { + SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL_SM1, 7, 0), +}; +
No. there is no reason to add that for s4 and not for sm1 which has 8 line HW support too. That clearly shows up with #define used. If you must do that, please do it correctly without leaving the other platforms behind.
quoted hunk
static const struct snd_soc_component_driver g12a_toacodec_component_drv = { .probe = g12a_toacodec_component_probe, .controls = g12a_toacodec_controls,@@ -258,6 +300,17 @@ static const struct snd_soc_component_driver sm1_toacodec_component_drv = { .endianness = 1, }; +static const struct snd_soc_component_driver s4_toacodec_component_drv = { + .probe = sm1_toacodec_component_probe, + .controls = s4_toacodec_controls, + .num_controls = ARRAY_SIZE(s4_toacodec_controls), + .dapm_widgets = s4_toacodec_widgets, + .num_dapm_widgets = ARRAY_SIZE(s4_toacodec_widgets), + .dapm_routes = g12a_toacodec_routes, + .num_dapm_routes = ARRAY_SIZE(g12a_toacodec_routes), + .endianness = 1, +}; + static const struct regmap_config g12a_toacodec_regmap_cfg = { .reg_bits = 32, .val_bits = 32,@@ -278,6 +331,13 @@ static const struct g12a_toacodec_match_data sm1_toacodec_match_data = { .field_bclk_sel = REG_FIELD(TOACODEC_CTRL0, 4, 6), }; +static const struct g12a_toacodec_match_data s4_toacodec_match_data = { + .component_drv = &s4_toacodec_component_drv, + .field_dat_sel = REG_FIELD(TOACODEC_CTRL0, 19, 20), + .field_lrclk_sel = REG_FIELD(TOACODEC_CTRL0, 12, 14), + .field_bclk_sel = REG_FIELD(TOACODEC_CTRL0, 4, 6), +}; + static const struct of_device_id g12a_toacodec_of_match[] = { { .compatible = "amlogic,g12a-toacodec",@@ -287,6 +347,10 @@ static const struct of_device_id g12a_toacodec_of_match[] = { .compatible = "amlogic,sm1-toacodec", .data = &sm1_toacodec_match_data, }, + { + .compatible = "amlogic,s4-toacodec", + .data = &s4_toacodec_match_data, + }, {} }; MODULE_DEVICE_TABLE(of, g12a_toacodec_of_match);
-- Jerome