ASoC: rt5575: Add the codec driver for the ALC5575
Subsystems (4)
Oder Chiou
maintainerPATCH v6 1/2 ASoC: rt5575: Add the codec driver for the ALC5575
The ALC5575 integrates an audio DSP that typically loads its firmware from an external flash via its own SPI host interface. In certain hardware configurations, the firmware can alternatively be loaded through the SPI client interface. The driver provides basic mute and volume control functions. When the SPI client interface is enabled, firmware loading is handled by the SPI driver.
--- sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 4 + sound/soc/codecs/rt5575-spi.c | 95 +++++++++ sound/soc/codecs/rt5575-spi.h | 16 ++ sound/soc/codecs/rt5575.c | 366 ++++++++++++++++++++++++++++++++++ sound/soc/codecs/rt5575.h | 54 +++++ 6 files changed, 545 insertions(+) create mode 100644 sound/soc/codecs/rt5575-spi.c create mode 100644 sound/soc/codecs/rt5575-spi.h create mode 100644 sound/soc/codecs/rt5575.c create mode 100644 sound/soc/codecs/rt5575.h
sound/soc/codecs/Kconfig +10 -0
@@ -211,6 +211,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_RT1305 imply SND_SOC_RT1308 imply SND_SOC_RT5514 + imply SND_SOC_RT5575 imply SND_SOC_RT5616 imply SND_SOC_RT5631 imply SND_SOC_RT5640
@@ -1767,6 +1768,15 @@ config SND_SOC_RT5514_SPI_BUILTIN bool # force RT5514_SPI to be built-in to avoid link errors default SND_SOC_RT5514=y && SND_SOC_RT5514_SPI=m +config SND_SOC_RT5575 + tristate + depends on I2C + +config SND_SOC_RT5575_SPI + tristate + depends on SPI_MASTER + select SND_SOC_RT5575 + config SND_SOC_RT5616 tristate "Realtek RT5616 CODEC" depends on I2C
sound/soc/codecs/Makefile +4 -0
@@ -252,6 +252,8 @@ snd-soc-rt286-y := rt286.o snd-soc-rt298-y := rt298.o snd-soc-rt5514-y := rt5514.o snd-soc-rt5514-spi-y := rt5514-spi.o +snd-soc-rt5575-y := rt5575.o +snd-soc-rt5575-spi-y := rt5575-spi.o snd-soc-rt5616-y := rt5616.o snd-soc-rt5631-y := rt5631.o snd-soc-rt5640-y := rt5640.o
@@ -684,6 +686,8 @@ obj-$(CONFIG_SND_SOC_RT298) += snd-soc-rt298.o obj-$(CONFIG_SND_SOC_RT5514) += snd-soc-rt5514.o obj-$(CONFIG_SND_SOC_RT5514_SPI) += snd-soc-rt5514-spi.o obj-$(CONFIG_SND_SOC_RT5514_SPI_BUILTIN) += snd-soc-rt5514-spi.o +obj-$(CONFIG_SND_SOC_RT5575) += snd-soc-rt5575.o +obj-$(CONFIG_SND_SOC_RT5575_SPI) += snd-soc-rt5575-spi.o obj-$(CONFIG_SND_SOC_RT5616) += snd-soc-rt5616.o obj-$(CONFIG_SND_SOC_RT5631) += snd-soc-rt5631.o obj-$(CONFIG_SND_SOC_RT5640) += snd-soc-rt5640.o
sound/soc/codecs/rt5575-spi.c new file +95 -0
95 lines not on this page · raw message
sound/soc/codecs/rt5575-spi.h new file +16 -0
@@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* +* rt5575-spi.h -- ALC5575 SPI driver +* +* Copyright(c) 2025 Realtek Semiconductor Corp. +* +*/ + +#ifndef __RT5575_SPI_H__ +#define __RT5575_SPI_H__ + +extern bool rt5575_spi_ready; + +int rt5575_spi_burst_write (u32 addr,const u8 *txbuf,size_t len); + +#endif /* __RT5575_SPI_H__ */
sound/soc/codecs/rt5575.c new file +366 -0
366 lines not on this page · raw message
sound/soc/codecs/rt5575.h new file +54 -0
54 lines not on this page · raw message
Signature · 1 line
2.51.1
Oder Chiou
maintainerPATCH v6 2/2 ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575
The ALC5575 integrates an audio DSP that typically loads its firmware from an external flash via its own SPI host interface. In certain hardware configurations, the firmware can alternatively be loaded through the SPI client interface. The driver provides basic mute and volume control functions. When the SPI client interface is enabled, firmware loading is handled by the SPI driver.
--- .../bindings/sound/realtek,rt5575.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5575.yaml
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +42 -0
@@ -0,0 +1,42 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/realtek,rt5575.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ALC5575 audio CODEC + +maintainers: + - Oder Chiou <[email protected]> + +description: | + The device supports both I2C and SPI. I2C is mandatory, while SPI is optional depending on the + hardware configuration. + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + - $ref: dai-common.yaml# + +properties: + compatible: + const: realtek,rt5575 + + reg: + maxItems: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + codec@57 { + compatible = "realtek,rt5575"; + reg = <0x57>; + }; + };
Signature · 1 line
2.51.1
Re: [PATCH v6 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575
Quoted 4 lines · Oder Chiou
The ALC5575 integrates an audio DSP that typically loads its firmware from an external flash via its own SPI host interface. In certain hardware configurations, the firmware can alternatively be loaded through the SPI client interface. The driver provides basic mute and
Don't describe driver, but hardware.
Quoted 2 lines
volume control functions. When the SPI client interface is enabled, firmware loading is handled by the SPI driver.
<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.
Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about b4 prep --auto-to-cc if you added new patches to the patchset.
You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time.
Please kindly resend and include all necessary To/Cc entries. </form letter>
Quoted 16 lines
Signed-off-by Oder Chiou <[email protected]>--- .../bindings/sound/realtek,rt5575.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5575.yamlDocumentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +3
-0@@ -0,0 +1,42 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +---
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +10
-0+$id: http://devicetree.org/schemas/sound/realtek,rt5575.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ALC5575 audio CODEC + +maintainers: + - Oder Chiou <[email protected]> + +description: | + The device supports both I2C and SPI. I2C is mandatory, while SPI is optional depending on the
Please wrap at 80 (see coding style) and drop |.
Best regards,
Krzysztof
Re: [PATCH v6 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575
Quoted 24 lines · Krzysztof Kozlowski
Quoted 4 lines · Oder Chiou
The ALC5575 integrates an audio DSP that typically loads its firmware from an external flash via its own SPI host interface. In certain hardware configurations, the firmware can alternatively be loaded through the SPI client interface. The driver provides basic mute and
Don't describe driver, but hardware.
Quoted 2 lines
volume control functions. When the SPI client interface is enabled, firmware loading is handled by the SPI driver.
<form letter>
Please usescripts/get_maintainers.plto get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.Tools like b4 or
scripts/get_maintainer.plprovide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember aboutb4 prep --auto-to-ccif you added new patches to the patchset.
Quoted 7 lines
You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time.
Please kindly resend and include all necessary To/Cc entries. </form letter>
Now I see I already asked you all this. I also asked for changelog and you just ignored me.
NAK
Best regards,
Krzysztof
Quoted 6 lines · Oder Chiou
The ALC5575 integrates an audio DSP that typically loads its firmware from an external flash via its own SPI host interface. In certain hardware configurations, the firmware can alternatively be loaded through the SPI client interface. The driver provides basic mute and volume control functions. When the SPI client interface is enabled, firmware loading is handled by the SPI driver.
The patch looks much nicer. Thank you for the update, Oder. Let's move to round 2.
Quoted 28 lines
Signed-off-by Oder Chiou <[email protected]>--- sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 4 + sound/soc/codecs/rt5575-spi.c | 95 +++++++++ sound/soc/codecs/rt5575-spi.h | 16 ++ sound/soc/codecs/rt5575.c | 366 ++++++++++++++++++++++++++++++++++ sound/soc/codecs/rt5575.h | 54 +++++ 6 files changed, 545 insertions(+)create mode 100644
sound/soc/codecs/rt5575-spi.ccreate mode 100644sound/soc/codecs/rt5575-spi.hcreate mode 100644sound/soc/codecs/rt5575.ccreate mode 100644sound/soc/codecs/rt5575.hsound/soc/codecs/Kconfig +1
-0@@ -211,6 +211,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_RT1305 imply SND_SOC_RT1308 imply SND_SOC_RT5514 + imply SND_SOC_RT5575 imply SND_SOC_RT5616 imply SND_SOC_RT5631 imply SND_SOC_RT5640
sound/soc/codecs/Kconfig config SND_SOC_RT5514_SPI_BUILTIN · line 1776
Quoted 9 lines
sound/soc/codecs/Kconfig +6
-0@@ -1767,6 +1768,15 @@ config SND_SOC_RT5514_SPI_BUILTIN bool # force RT5514_SPI to be built-in to avoid link errors default SND_SOC_RT5514=y && SND_SOC_RT5514_SPI=m +config SND_SOC_RT5575 + tristate + depends on I2C + +config SND_SOC_RT5575_SPI + tristate
I'd suggest to add a caption for both tristates. Examples are present all around that very file.
Quoted 38 lines
sound/soc/codecs/Kconfig +3
-0+ depends on SPI_MASTER + select SND_SOC_RT5575 + config SND_SOC_RT5616 tristate "Realtek RT5616 CODEC" depends on I2Csound/soc/codecs/Makefile +4
-0@@ -252,6 +252,8 @@ snd-soc-rt286-y := rt286.o snd-soc-rt298-y := rt298.o snd-soc-rt5514-y := rt5514.o snd-soc-rt5514-spi-y := rt5514-spi.o +snd-soc-rt5575-y := rt5575.o +snd-soc-rt5575-spi-y := rt5575-spi.o snd-soc-rt5616-y := rt5616.o snd-soc-rt5631-y := rt5631.o snd-soc-rt5640-y := rt5640.o@@ -684,6 +686,8 @@ obj-$(CONFIG_SND_SOC_RT298) += snd-soc-rt298.o obj-$(CONFIG_SND_SOC_RT5514) += snd-soc-rt5514.o obj-$(CONFIG_SND_SOC_RT5514_SPI) += snd-soc-rt5514-spi.o obj-$(CONFIG_SND_SOC_RT5514_SPI_BUILTIN) += snd-soc-rt5514-spi.o +obj-$(CONFIG_SND_SOC_RT5575) += snd-soc-rt5575.o +obj-$(CONFIG_SND_SOC_RT5575_SPI) += snd-soc-rt5575-spi.o obj-$(CONFIG_SND_SOC_RT5616) += snd-soc-rt5616.o obj-$(CONFIG_SND_SOC_RT5631) += snd-soc-rt5631.o obj-$(CONFIG_SND_SOC_RT5640) += snd-soc-rt5640.osound/soc/codecs/rt5575-spi.c new file +4
-0@@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* rt5575-spi.c -- ALC5575 SPI driver +*
sound/soc/codecs/rt5575-spi.c line 14
Quoted 10 lines
sound/soc/codecs/rt5575-spi.c new file +10
-0+* Copyright(c) 2025 Realtek Semiconductor Corp. +* +*/ + +#include <linux/of.h> +#include <linux/spi/spi.h> + +#include "rt5575-spi.h" + +#define RT5575_SPI_BUF_LEN 240
Let's move that right before its first usage - the struct below.
sound/soc/codecs/rt5575-spi.c line 23
Quoted 9 lines
sound/soc/codecs/rt5575-spi.c new file +9
-0+ +/* SPI Command */ +enum { +RT5575_SPI_CMD_16_READ =0 , +RT5575_SPI_CMD_16_WRITE , +RT5575_SPI_CMD_32_READ , +RT5575_SPI_CMD_32_WRITE , +RT5575_SPI_CMD_BURST_READ , +RT5575_SPI_CMD_BURST_WRITE ,
Looks like only the last one, RT5575_SPI_CMD_BURST_WRITE, is used. Any plans for follow up patches utilizing these? If not, perhaps it's good idea to just drop them.
If someone insists on having them here regardless of lack of usage i.e.: code-as-documentation approach, then at least flag them, e.g.:
/* Available SPI commands, 16_READ till BURST_READ reserved. */
enum {
RT5575_SPI_CMD_16_READ = 0,
RT5575_SPI_CMD_16_WRITE,
RT5575_SPI_CMD_32_READ,
RT5575_SPI_CMD_32_WRITE,
RT5575_SPI_CMD_BURST_READ,
RT5575_SPI_CMD_BURST_WRITE,sound/soc/codecs/rt5575-spi.c line 26
Quoted 3 lines
sound/soc/codecs/rt5575-spi.c new file +3
-0+}; + +struct rt5575_spi_burst_write {
Naming is weird - it's not a 'BURST_WRITE' command until the 'cmd' field says so. I'd rather have this named 'struct rt5575_spi_cmd' as its description and usage suggests.
sound/soc/codecs/rt5575-spi.c line 39
Quoted 13 lines
sound/soc/codecs/rt5575-spi.c new file +13
-0+u8 cmd; +u32 addr; +u8 data[RT5575_SPI_BUF_LEN ]; +u8 dummy; +} __packed; + +bool rt5575_spi_ready; +static struct spi_device *rt5575_spi; + +/** +* rt5575_spi_burst_write - Write data to SPI by rt5575 address. +* @addr: Start address. +* @txbuf: Data buffer for writng.
Spelling. You can use --codespell when utilizing checkpatch.pl, before the patches are sent. Should find most of such stuff for you.
sound/soc/codecs/rt5575-spi.c line 47
Quoted 8 lines
sound/soc/codecs/rt5575-spi.c new file +8
-0+* @len: Data length. +* +*/ +int rt5575_spi_burst_write (u32 addr,const u8 *txbuf,size_t len) +{ +struct rt5575_spi_burst_write buf = { + .cmd =RT5575_SPI_CMD_BURST_WRITE + };
Depending on whether you plan to add more commands than 'just' BURST_WRITE, adding a macro may save you some LOCs:
#define RT5575_SPI_CMD(cmd_type) \
{ \
.cmd = RT5575_SPI_CMD_##cmd_type, \
}// usage:
struct rt5575_spi_cmd buf = RT5575_SPI_CMD(BURST_WRITE);
Gets handy if you're planning to implement 2+ rt5575_spi_cmd_xxx() functions. This is just a suggestion and if you're going to stick to just one cmd, you can skip it.
sound/soc/codecs/rt5575-spi.c line 60
Quoted 13 lines
sound/soc/codecs/rt5575-spi.c new file +13
-0+unsigned int end, offset =0 ; + +while (offset < len) { +if (offset +RT5575_SPI_BUF_LEN <= len) + end =RT5575_SPI_BUF_LEN ; +else + end = len %RT5575_SPI_BUF_LEN ; + + buf.addr =cpu_to_le32 (addr + offset); + +memcpy (&buf.data, &txbuf[offset], end); + +spi_write (rt5575_spi, &buf,sizeof (struct rt5575_spi_burst_write ));
sizeof(buf), easier to maintain than sizeof(struct).
sound/soc/codecs/rt5575-spi.c line 73
Quoted 13 lines
sound/soc/codecs/rt5575-spi.c new file +13
-0+ + offset +=RT5575_SPI_BUF_LEN ; + } + +return 0 ; +} +EXPORT_SYMBOL_GPL (rt5575_spi_burst_write); + +static int rt5575_spi_probe (struct spi_device *spi) +{ + rt5575_spi = spi; + + rt5575_spi_ready = true;
This looks weird. If rt5575_spi_ready is 'true' unconditionally on probe(), why not just replace it with '!rt5575_spi' checks or IS_ERR_OR_NULL(rt5575_spi)?
Quoted 94 lines
sound/soc/codecs/rt5575-spi.c new file +22
-0+ +return 0 ; +} + +static const struct of_device_id rt5575_of_match[] = { + { .compatible ="realtek,rt5575" }, + { } +}; +MODULE_DEVICE_TABLE (of, rt5575_of_match); + +static struct spi_driver rt5575_spi_driver = { + .driver = { + .name ="rt5575" , + .of_match_table =of_match_ptr (rt5575_of_match), + }, + .probe = rt5575_spi_probe, +}; +module_spi_driver (rt5575_spi_driver); + +MODULE_DESCRIPTION ("ALC5575 SPI driver" ); +MODULE_AUTHOR ("Oder Chiou <[email protected]>" ); +MODULE_LICENSE ("GPL" );sound/soc/codecs/rt5575-spi.h new file +16
-0@@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* +* rt5575-spi.h -- ALC5575 SPI driver +* +* Copyright(c) 2025 Realtek Semiconductor Corp. +* +*/ + +#ifndef __RT5575_SPI_H__ +#define __RT5575_SPI_H__ + +extern bool rt5575_spi_ready; + +int rt5575_spi_burst_write (u32 addr,const u8 *txbuf,size_t len); + +#endif /* __RT5575_SPI_H__ */ sound/soc/codecs/rt5575.c new file +44
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* rt5575.c -- ALC5575 ALSA SoC audio component driver +* +* Copyright(c) 2025 Realtek Semiconductor Corp. +* +*/ + +#include <linux/firmware.h> +#include <linux/i2c.h> +#include <sound/soc.h> +#include <sound/tlv.h> + +#include "rt5575.h" +#if IS_ENABLED (CONFIG_SND_SOC_RT5575_SPI ) +#include "rt5575-spi.h" +#endif + +static bool rt5575_readable_register (struct device *dev,unsigned int reg) +{ +switch (reg) { +case RT5575_ID : +case RT5575_ID_1 : +case RT5575_MIXL_VOL : +case RT5575_MIXR_VOL : +case RT5575_PROMPT_VOL : +case RT5575_SPK01_VOL : +case RT5575_SPK23_VOL : +case RT5575_MIC1_VOL : +case RT5575_MIC2_VOL : +case RT5575_WNC_CTRL : +case RT5575_MODE_CTRL : +case RT5575_I2S_RATE_CTRL : +case RT5575_SLEEP_CTRL : +case RT5575_ALG_BYPASS_CTRL : +case RT5575_PINMUX_CTRL_2 : +case RT5575_GPIO_CTRL_1 : +case RT5575_DSP_BUS_CTRL : +case RT5575_SW_INT : +case RT5575_DSP_BOOT_ERR : +case RT5575_DSP_READY : +case RT5575_DSP_CMD_ADDR : +case RT5575_EFUSE_DATA_2 : +case RT5575_EFUSE_DATA_3 :
sound/soc/codecs/rt5575.c line 54
Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10
-0+return true; +default : +return false; + } +} + +static const DECLARE_TLV_DB_SCALE(ob_tlv, -9525, 75, 0); + +#if IS_ENABLED (CONFIG_SND_SOC_RT5575_SPI ) +static void rt5575_spi_fw_loaded (const struct firmware *fw,void *context)
This has still not been addressed since my previous review. I do not understand the logic here. We have a 'common' file yet filled plagued by SPI specifics. On one hand we have rt5575_spi_burst_write() exported and part of SPI-specific entity, on the other we have rt5575_spi_fw_loaded() hidden here.
Design if off, choose one or the other. In general, prefer the common code to be actually _common_. Makes it easier to Realtek and non-Realtek to maintain the code going forward. Leave specifics within the specifics-files.
sound/soc/codecs/rt5575.c line 61
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7
-0+{ +struct rt5575_priv *rt5575 = context; +struct i2c_client *i2c = rt5575->i2c; +const struct firmware *firmware; +static const char *const fw_path[] = { +"realtek/rt5575/rt5575_fw2.bin" ,"realtek/rt5575/rt5575_fw3.bin" , +"realtek/rt5575/rt5575_fw4.bin"
Nitpick: newline between fw2 and fw3.
sound/soc/codecs/rt5575.c line 68
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7
-0+ }; +static const u32 fw_addr[] = {0x5f600000 ,0x5f7fe000 ,0x5f7ff000 }; +int i, ret; + +regmap_write (rt5575->dsp_regmap,0xfafafafa ,0x00000004 ); +regmap_write (rt5575->dsp_regmap,0x18008064 ,0x00000000 ); +regmap_write (rt5575->dsp_regmap,0x18008068 ,0x0002ffff );
Looks better, thank you for addresing this. The magic numbers are still a headache though.
sound/soc/codecs/rt5575.c line 75
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7
-0+ +rt5575_spi_burst_write (0x5f400000 , fw->data, fw->size); +release_firmware (fw); + +for (i =0 ; i <ARRAY_SIZE (fw_addr); i++) { + ret =request_firmware (&firmware, fw_path[i], &i2c->dev); +if (ret ==0 ) {
!ret
sound/soc/codecs/rt5575.c line 77
Quoted 2 lines
sound/soc/codecs/rt5575.c new file +2
-0+rt5575_spi_burst_write (fw_addr[i], firmware->data, firmware->size); +release_firmware (firmware);
And if this fails, should the previous firmware still be there?
sound/soc/codecs/rt5575.c line 88
Quoted 11 lines
sound/soc/codecs/rt5575.c new file +11
-0+ } + } + +regmap_write (rt5575->dsp_regmap,0x18000000 ,0x00000000 ); + +regmap_update_bits (rt5575->regmap,RT5575_SW_INT ,1 ,1 ); + +regmap_read_poll_timeout (rt5575->regmap,RT5575_SW_INT , ret, !ret,100000 ,10000000 ); + +if (ret) +dev_err (&i2c->dev,"Firmware failure\n" );
At least dump the 'ret' code.
sound/soc/codecs/rt5575.c line 96
Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8
-0+} +#endif + +static const struct snd_kcontrol_new rt5575_snd_controls[] = { +SOC_DOUBLE ("Speaker01 Playback Switch" ,RT5575_SPK01_VOL ,31 ,15 ,1 ,1 ), +SOC_DOUBLE_TLV ("Speaker01 Playback Volume" ,RT5575_SPK01_VOL ,17 ,1 ,167 ,0 , ob_tlv), +SOC_DOUBLE ("Speaker23 Playback Switch" ,RT5575_SPK23_VOL ,31 ,15 ,1 ,1 ), +SOC_DOUBLE_TLV ("Speaker23 Playback Volume" ,RT5575_SPK23_VOL ,17 ,1 ,167 ,0 , ob_tlv),
You and I may understand the Speaker01 and Speaker23 concept, Joe from the street may not. Either move to 4channel kcontrol for "Speaker Playback Switch" or be more explicit with the naming if said approach is not feasible e.g.:
"Speaker CH-01 Playback Switch"
"Speaker CH-23 Playback Switch"
Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10
-0+SOC_DOUBLE ("Mic1 Capture Switch" ,RT5575_MIC1_VOL ,31 ,15 ,1 ,1 ), +SOC_DOUBLE_TLV ("Mic1 Capture Volume" ,RT5575_MIC1_VOL ,17 ,1 ,167 ,0 , ob_tlv), +SOC_DOUBLE ("Mic2 Capture Switch" ,RT5575_MIC2_VOL ,31 ,15 ,1 ,1 ), +SOC_DOUBLE_TLV ("Mic2 Capture Volume" ,RT5575_MIC2_VOL ,17 ,1 ,167 ,0 , ob_tlv), +SOC_DOUBLE_R ("Mix Playback Switch" ,RT5575_MIXL_VOL ,RT5575_MIXR_VOL ,31 ,1 ,1 ), +SOC_DOUBLE_R_TLV ("Mix Playback Volume" ,RT5575_MIXL_VOL ,RT5575_MIXR_VOL ,1 ,127 ,0 , + ob_tlv), +SOC_DOUBLE ("Prompt Playback Switch" ,RT5575_PROMPT_VOL ,31 ,15 ,1 ,1 ), + SOC_DOUBLE_TLV("Prompt Playback Volume", RT5575_PROMPT_VOL, 17, 1, 167, 0, ob_tlv), +};
Oder Chiou
maintainerQuoted 10 lines
-----Original Message-----
From: Cezary Rojewski <[email protected]>
Sent: Thursday, November 6, 2025 5:58 PM
To: Oder Chiou <[email protected]>
Cc: [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方 德義] <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH v6 1/2] ASoC: rt5575: Add the codec driver for the ALC5575
Quoted 2 lines
The patch looks much nicer. Thank you for the update, Oder. Let's move to round 2.
Thanks very much for the suggestions.
Quoted 7 lines
sound/soc/codecs/rt5575-spi.c line 26
Quoted 3 lines
sound/soc/codecs/rt5575-spi.c new file +3
-0+}; + +struct rt5575_spi_burst_write {Naming is weird - it's not a '
BURST_WRITE' command until the 'cmd' field says so. I'd rather have this named 'structrt5575_spi_cmd' as its description and usage suggests.
The structure is only for the 'BURST_WRITE' command, and other commands are different structure.
Quoted 9 lines
Quoted 9 lines
sound/soc/codecs/rt5575-spi.c new file +9
-0+ + offset +=RT5575_SPI_BUF_LEN ; + } + +return 0 ; +} +EXPORT_SYMBOL_GPL (rt5575_spi_burst_write); + +static int rt5575_spi_probe (struct spi_device *spi)
Quoted 8 lines
sound/soc/codecs/rt5575-spi.c line 73
Quoted 4 lines
sound/soc/codecs/rt5575-spi.c new file +4
-0+{ + rt5575_spi = spi; + + rt5575_spi_ready = true;This looks weird. If
rt5575_spi_readyis 'true' unconditionally onprobe(), why not just replace it with '!rt5575_spi' checks orIS_ERR_OR_NULL(rt5575_spi)?
If the kernel failed to allocate memory for the structure or couldn't successfully match the device, the probe function would not be called.
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI) +static void rt5575_spi_fw_loaded(const struct firmware *fw, void *context)This has still not been addressed since my previous review. I do not understand the logic here. We have a 'common' file yet filled plagued by SPI specifics. On one hand we have
rt5575_spi_burst_write()exported and part of SPI-specific entity, on the other we havert5575_spi_fw_loaded()hidden here.
Quoted 4 lines
Design if off, choose one or the other. In general, prefer the common code to be actually _common_. Makes it easier to Realtek and non-Realtek to maintain the code going forward. Leave specifics within the specifics-files.
The function name will be changed as rt5575_fw_load_by_spi.