[PATCH v1 00/10] ASoC: Some issues about loongson i2s

[PATCH v1 00/10] ASoC: Some issues about loongson i2s

[Binbin Zhou]

Hi all:

This patch set is mainly about Loongson i2s related issues.

Please allow me to briefly explain this patch set:
Patch 1-2: Add ES8323 codec required on Loongson-2K2000
Patch 3-4: Add uda1342 codec required on Loongson-2K1000
Patch 5: Improve code readability
Patch 6: Fix the problem of unable to detect codec under FDT system.
Patch 7-8: Add Loongson i2s platform device support
Patch 9-10: Related DTS support.

Thanks.

base-commit: 097a44db5747403b19d05a9664e8ec6adba27e3b

Binbin Zhou (10):
  ASoC: dt-bindings: Add Everest ES8323 Codec
  ASoC: codecs: Add support for ES8323
  ASoC: dt-bindings: Add NXP uda1342 Codec
  ASoC: codecs: Add uda1342 codec driver
  ASoC: loongson: Improve code readability
  ASoC: loongson: Fix codec detection failure on FDT systems
  ASoC: dt-bindings: Add Loongson I2S controller
  ASoC: loongson: Add I2S controller driver as platform device
  LoongArch: dts: Add I2S support to Loongson-2K1000
  LoongArch: dts: Add I2S support to Loongson-2K2000

 .../bindings/sound/everest,es8323.yaml        |  49 +
 .../bindings/sound/loongson,ls2k-i2s.yaml     |  66 ++
 .../bindings/sound/nxp,uda1342.yaml           |  42 +
 arch/loongarch/boot/dts/loongson-2k1000.dtsi  |  17 +-
 arch/loongarch/boot/dts/loongson-2k2000.dtsi  |  22 +-
 sound/soc/codecs/Kconfig                      |  13 +
 sound/soc/codecs/Makefile                     |   4 +
 sound/soc/codecs/es8323.c                     | 849 ++++++++++++++++++
 sound/soc/codecs/es8323.h                     |  77 ++
 sound/soc/codecs/uda1342.c                    | 397 ++++++++
 sound/soc/codecs/uda1342.h                    |  77 ++
 sound/soc/loongson/Kconfig                    |  12 +-
 sound/soc/loongson/Makefile                   |   3 +
 sound/soc/loongson/loongson_card.c            | 217 +++--
 sound/soc/loongson/loongson_dma.c             |  10 +-
 sound/soc/loongson/loongson_i2s.c             | 110 +--
 sound/soc/loongson/loongson_i2s.h             |  24 +-
 sound/soc/loongson/loongson_i2s_pci.c         |  51 +-
 sound/soc/loongson/loongson_i2s_plat.c        | 186 ++++
 19 files changed, 2030 insertions(+), 196 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/everest,es8323.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/nxp,uda1342.yaml
 create mode 100644 sound/soc/codecs/es8323.c
 create mode 100644 sound/soc/codecs/es8323.h
 create mode 100644 sound/soc/codecs/uda1342.c
 create mode 100644 sound/soc/codecs/uda1342.h
 create mode 100644 sound/soc/loongson/loongson_i2s_plat.c

-- 
2.43.5

[PATCH v1 01/10] ASoC: dt-bindings: Add Everest ES8323 Codec

[Binbin Zhou]

Add Everest Semiconductor ES8316 low power audio CODEC binding with DT
schema format using json-schema.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/sound/everest,es8323.yaml        | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/everest,es8323.yaml

diff --git a/Documentation/devicetree/bindings/sound/everest,es8323.yaml b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
new file mode 100644
index 000000000000..0450d0f49d03
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/everest,es8323.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Everest ES8323 audio CODECs
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    const: everest,es8323
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: clock for master clock (MCLK)
+    maxItems: 1
+
+  clock-names:
+    const: mclk
+
+  '#sound-dai-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - '#sound-dai-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      codec@10 {
+        compatible = "everest,es8323";
+        reg = <0x10>;
+        #sound-dai-cells = <0>;
+      };
+    };
-- 
2.43.5

Re: [PATCH v1 01/10] ASoC: dt-bindings: Add Everest ES8323 Codec

[Krzysztof Kozlowski]

On Thu, Sep 05, 2024 at 03:02:14PM +0800, Binbin Zhou wrote:
> Add Everest Semiconductor ES8316 low power audio CODEC binding with DT
> schema format using json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/sound/everest,es8323.yaml        | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/everest,es8323.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/everest,es8323.yaml b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
> new file mode 100644
> index 000000000000..0450d0f49d03
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/everest,es8323.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Everest ES8323 audio CODECs
> +
> +maintainers:
> +  - Binbin Zhou <zhoubinbin@loongson.cn>
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: everest,es8323
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: clock for master clock (MCLK)
> +    maxItems: 1
> +
> +  clock-names:
> +    const: mclk
> +
> +  '#sound-dai-cells':
> +    const: 0

No audio-graph-port? Are you sure? It looks exactly like
everest,es8316...

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#sound-dai-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      codec@10 {
> +        compatible = "everest,es8323";
> +        reg = <0x10>;
> +        #sound-dai-cells = <0>;

Make the example complete. Assuming this binding stays...

Best regards,
Krzysztof

Re: [PATCH v1 01/10] ASoC: dt-bindings: Add Everest ES8323 Codec

[Binbin Zhou]

Hi Krzysztof:

Thanks for your review.

On Fri, Sep 6, 2024 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:14PM +0800, Binbin Zhou wrote:
> > Add Everest Semiconductor ES8316 low power audio CODEC binding with DT
> > schema format using json-schema.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../bindings/sound/everest,es8323.yaml        | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/everest,es8323.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/everest,es8323.yaml b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
> > new file mode 100644
> > index 000000000000..0450d0f49d03
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/everest,es8323.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/everest,es8323.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Everest ES8323 audio CODECs
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > +
> > +allOf:
> > +  - $ref: dai-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: everest,es8323
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: clock for master clock (MCLK)
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: mclk
> > +
> > +  '#sound-dai-cells':
> > +    const: 0
>
> No audio-graph-port? Are you sure? It looks exactly like
> everest,es8316...

I checked and they are indeed very similar and it seems I just need to
add to everest,es8316.yaml instead of a new file.

Thanks.
Binbin
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#sound-dai-cells'
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      codec@10 {
> > +        compatible = "everest,es8323";
> > +        reg = <0x10>;
> > +        #sound-dai-cells = <0>;
>
> Make the example complete. Assuming this binding stays...
>
> Best regards,
> Krzysztof
>

[PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Binbin Zhou]

Add a codec driver for the Everest ES8323. It supports two separate
audio outputs and two separate audio inputs.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 sound/soc/codecs/Kconfig  |   5 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/es8323.c | 849 ++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/es8323.h |  77 ++++
 4 files changed, 933 insertions(+)
 create mode 100644 sound/soc/codecs/es8323.c
 create mode 100644 sound/soc/codecs/es8323.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index b5e6d0a986c8..85270aa43512 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -112,6 +112,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_DA9055
 	imply SND_SOC_DMIC
 	imply SND_SOC_ES8316
+	imply SND_SOC_ES8323
 	imply SND_SOC_ES8326
 	imply SND_SOC_ES8328_SPI
 	imply SND_SOC_ES8328_I2C
@@ -1142,6 +1143,10 @@ config SND_SOC_ES8316
 	tristate "Everest Semi ES8316 CODEC"
 	depends on I2C
 
+config SND_SOC_ES8323
+	tristate "Everest Semi ES8323 CODEC"
+	depends on I2C
+
 config SND_SOC_ES8326
 	tristate "Everest Semi ES8326 CODEC"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 622e360f0086..00ff3fdf0133 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -125,6 +125,7 @@ snd-soc-es7241-y := es7241.o
 snd-soc-es83xx-dsm-common-y := es83xx-dsm-common.o
 snd-soc-es8311-y := es8311.o
 snd-soc-es8316-y := es8316.o
+snd-soc-es8323-y := es8323.o
 snd-soc-es8326-y := es8326.o
 snd-soc-es8328-y := es8328.o
 snd-soc-es8328-i2c-y := es8328-i2c.o
@@ -531,6 +532,7 @@ obj-$(CONFIG_SND_SOC_ES7241)	+= snd-soc-es7241.o
 obj-$(CONFIG_SND_SOC_ES83XX_DSM_COMMON)    += snd-soc-es83xx-dsm-common.o
 obj-$(CONFIG_SND_SOC_ES8311)    += snd-soc-es8311.o
 obj-$(CONFIG_SND_SOC_ES8316)    += snd-soc-es8316.o
+obj-$(CONFIG_SND_SOC_ES8323)	+= snd-soc-es8323.o
 obj-$(CONFIG_SND_SOC_ES8326)    += snd-soc-es8326.o
 obj-$(CONFIG_SND_SOC_ES8328)	+= snd-soc-es8328.o
 obj-$(CONFIG_SND_SOC_ES8328_I2C)+= snd-soc-es8328-i2c.o
diff --git a/sound/soc/codecs/es8323.c b/sound/soc/codecs/es8323.c
new file mode 100644
index 000000000000..596eba3214ea
--- /dev/null
+++ b/sound/soc/codecs/es8323.c
@@ -0,0 +1,847 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * es8323.c -- es8323 ALSA SoC audio driver
+ *
+ * Copyright Rockchip Electronics Co. Ltd.
+ * Copyright Everest Semiconductor Co.,Ltd
+ *
+ * Author: Mark Brown <will@everset-semi.com>
+ *         Jianqun Xu <jay.xu@rock-chips.com>
+ *         Nickey Yang <nickey.yang@rock-chips.com>
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of_gpio.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/tlv.h>
+
+#include "es8323.h"
+
+#define ES8323_CODEC_SET_SPK	1
+#define ES8323_CODEC_SET_HP	2
+
+#define ES8323_DEF_VOL		0x1b
+
+/*
+ * es8323 register cache
+ * We can't read the es8323 register space when we
+ * are using 2 wire for device control, so we cache them instead.
+ */
+static u16 es8323_reg[] = {
+	0x06, 0x1C, 0xC3, 0xFC,	/*  0 */
+	0xC0, 0x00, 0x00, 0x7C,	/*  4 */
+	0x80, 0x00, 0x00, 0x06,	/*  8 */
+	0x00, 0x06, 0x30, 0x30,	/* 12 */
+	0xC0, 0xC0, 0x38, 0xB0,	/* 16 */
+	0x32, 0x06, 0x00, 0x00,	/* 20 */
+	0x06, 0x30, 0xC0, 0xC0,	/* 24 */
+	0x08, 0x06, 0x1F, 0xF7,	/* 28 */
+	0xFD, 0xFF, 0x1F, 0xF7,	/* 32 */
+	0xFD, 0xFF, 0x00, 0x38,	/* 36 */
+	0x38, 0x38, 0x38, 0x38,	/* 40 */
+	0x38, 0x00, 0x00, 0x00,	/* 44 */
+	0x00, 0x00, 0x00, 0x00,	/* 48 */
+	0x00, 0x00, 0x00, 0x00,	/* 52 */
+};
+
+/* codec private data */
+struct es8323_priv {
+	unsigned int sysclk;
+	struct clk *mclk;
+	struct snd_pcm_hw_constraint_list *sysclk_constraints;
+	struct snd_soc_component *component;
+	struct i2c_client *i2c;
+	int spk_ctl_gpio;
+	bool hp_inserted;
+	bool spk_gpio_level;
+};
+
+static const char *const es8323_line_texts[] = {
+	"Line 1", "Line 2", "PGA"
+};
+
+static const unsigned int es8323_line_values[] = {
+	0, 1, 3
+};
+
+static const char *const es8323_pga_sell[] = { "Line 1L", "Line 2L", "NC", "DifferentialL" };
+static const char *const es8323_pga_selr[] = { "Line 1R", "Line 2R", "NC", "DifferentialR" };
+static const char *const es8323_lin_sell[] = { "Line 1L", "Line 2L", "NC", "MicL" };
+static const char *const es8323_lin_selr[] = { "Line 1R", "Line 2R", "NC", "MicR" };
+
+static const char *const stereo_3d_txt[] = { "No 3D  ", "Level 1", "Level 2", "Level 3",
+					     "Level 4", "Level 5", "Level 6", "Level 7" };
+static const char *const ng_type_txt[] = { "Constant PGA Gain", "Mute ADC Output" };
+static const char *const deemph_txt[] = { "None", "32Khz", "44.1Khz", "48Khz" };
+static const char *const adcpol_txt[] = { "Normal", "L Invert", "R Invert", "L + R Invert" };
+
+static const char *const es8323_mono_mux[] = { "Stereo", "Mono (Left)", "Mono (Right)" };
+static const char *const es8323_diff_sel[] = { "Line 1", "Line 2" };
+
+static SOC_ENUM_SINGLE_DECL(es8323_left_dac_enum, ES8323_ADCCONTROL2, 6, es8323_pga_sell);
+static SOC_ENUM_SINGLE_DECL(es8323_right_dac_enum, ES8323_ADCCONTROL2, 4, es8323_pga_selr);
+static SOC_ENUM_SINGLE_DECL(es8323_diff_enum, ES8323_ADCCONTROL3, 7, es8323_diff_sel);
+static SOC_ENUM_SINGLE_DECL(es8323_llin_enum, ES8323_DACCONTROL16, 3, es8323_lin_sell);
+static SOC_ENUM_SINGLE_DECL(es8323_rlin_enum, ES8323_DACCONTROL16, 0, es8323_lin_selr);
+static SOC_ENUM_SINGLE_DECL(es8323_mono_enum, ES8323_ADCCONTROL3, 3, es8323_mono_mux);
+
+static const struct soc_enum es8323_enum[] = {
+	SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts),
+			      es8323_line_texts, es8323_line_values),	/* LLINE */
+	SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts),
+			      es8323_line_texts, es8323_line_values),	/* RLINE */
+	SOC_VALUE_ENUM_SINGLE(ES8323_ADCCONTROL2, 6, 3, ARRAY_SIZE(es8323_pga_sell),
+			      es8323_line_texts, es8323_line_values),	/* Left PGA Mux */
+	SOC_VALUE_ENUM_SINGLE(ES8323_ADCCONTROL2, 4, 3, ARRAY_SIZE(es8323_pga_sell),
+			      es8323_line_texts, es8323_line_values),	/* Right PGA Mux */
+	SOC_ENUM_SINGLE(ES8323_DACCONTROL7, 2, 8, stereo_3d_txt),	/* stereo-3d */
+	SOC_ENUM_SINGLE(ES8323_ADCCONTROL14, 1, 2, ng_type_txt),	/* noise gate type */
+	SOC_ENUM_SINGLE(ES8323_DACCONTROL6, 6, 4, deemph_txt),	/* Playback De-emphasis */
+	SOC_ENUM_SINGLE(ES8323_ADCCONTROL6, 6, 4, adcpol_txt),
+	SOC_ENUM_SINGLE(ES8323_ADCCONTROL3, 3, 3, es8323_mono_mux),
+	SOC_ENUM_SINGLE(ES8323_ADCCONTROL3, 7, 2, es8323_diff_sel),
+};
+
+static const DECLARE_TLV_DB_SCALE(pga_tlv, 0, 300, 0);
+static const DECLARE_TLV_DB_SCALE(adc_tlv, -9600, 50, 1);
+static const DECLARE_TLV_DB_SCALE(dac_tlv, -9600, 50, 1);
+static const DECLARE_TLV_DB_SCALE(out_tlv, -4500, 150, 0);
+static const DECLARE_TLV_DB_SCALE(bypass_tlv, 0, 300, 0);
+static const DECLARE_TLV_DB_SCALE(bypass_tlv2, -15, 300, 0);
+
+static const struct snd_kcontrol_new es8323_snd_controls[] = {
+	SOC_ENUM("3D Mode", es8323_enum[4]),
+	SOC_ENUM("ALC Capture Function", es8323_enum[5]),
+	SOC_SINGLE("ALC Capture ZC Switch", ES8323_ADCCONTROL13, 6, 1, 0),
+	SOC_SINGLE("ALC Capture Decay Time", ES8323_ADCCONTROL12, 4, 15, 0),
+	SOC_SINGLE("ALC Capture Attack Time", ES8323_ADCCONTROL12, 0, 15, 0),
+	SOC_SINGLE("ALC Capture NG Threshold", ES8323_ADCCONTROL14, 3, 31, 0),
+	SOC_ENUM("ALC Capture NG Type", es8323_enum[6]),
+	SOC_SINGLE("ALC Capture NG Switch", ES8323_ADCCONTROL14, 0, 1, 0),
+	SOC_SINGLE("ZC Timeout Switch", ES8323_ADCCONTROL13, 6, 1, 0),
+	SOC_DOUBLE_R_TLV("Capture Digital Volume", ES8323_LADC_VOL,
+			 ES8323_RADC_VOL, 0, 192, 1, adc_tlv),
+	SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0),
+	SOC_SINGLE_TLV("Left Channel Capture Volume", ES8323_ADCCONTROL1, 4, 8,
+		       0, bypass_tlv),
+	SOC_SINGLE_TLV("Right Channel Capture Volume", ES8323_ADCCONTROL1, 0,
+		       8, 0, bypass_tlv),
+	SOC_ENUM("Playback De-emphasis", es8323_enum[7]),
+	SOC_ENUM("Capture Polarity", es8323_enum[8]),
+	SOC_DOUBLE_R_TLV("PCM Volume", ES8323_LDAC_VOL, ES8323_RDAC_VOL,
+			 0, 192, 1, dac_tlv),
+	SOC_SINGLE_TLV("Left Mixer Left Bypass Volume", ES8323_DACCONTROL17, 3,
+		       7, 1, bypass_tlv2),
+	SOC_SINGLE_TLV("Right Mixer Right Bypass Volume", ES8323_DACCONTROL20,
+		       3, 7, 1, bypass_tlv2),
+	SOC_DOUBLE_R_TLV("Output 1 Playback Volume", ES8323_LOUT1_VOL,
+			 ES8323_ROUT1_VOL, 0, 33, 0, out_tlv),
+	SOC_DOUBLE_R_TLV("Output 2 Playback Volume", ES8323_LOUT2_VOL,
+			 ES8323_ROUT2_VOL, 0, 33, 0, out_tlv),
+};
+
+static const struct snd_kcontrol_new es8323_left_dac_mux_controls =
+	SOC_DAPM_ENUM("Route", es8323_left_dac_enum);
+
+static const struct snd_kcontrol_new es8323_right_dac_mux_controls =
+	SOC_DAPM_ENUM("Route", es8323_right_dac_enum);
+
+static const struct snd_kcontrol_new es8323_left_line_controls =
+	SOC_DAPM_ENUM("LLIN Mux", es8323_llin_enum);
+
+static const struct snd_kcontrol_new es8323_right_line_controls =
+	SOC_DAPM_ENUM("RLIN Mux", es8323_rlin_enum);
+
+/* Differential Mux */
+static const struct snd_kcontrol_new es8323_diffmux_controls =
+	SOC_DAPM_ENUM("Route2", es8323_diff_enum);
+
+/* Mono ADC Mux */
+static const struct snd_kcontrol_new es8323_monomux_controls =
+	SOC_DAPM_ENUM("Mono Mux", es8323_mono_enum);
+
+/* Right PGA Mux */
+static const struct snd_kcontrol_new es8323_right_pga_controls =
+	SOC_DAPM_ENUM("Route", es8323_enum[3]);
+
+/* Left Mixer */
+static const struct snd_kcontrol_new es8323_left_mixer_controls[] = {
+	SOC_DAPM_SINGLE("Left Playback Switch", SND_SOC_NOPM, 7, 1, 1),
+	SOC_DAPM_SINGLE("Left Bypass Switch", ES8323_DACCONTROL17, 6, 1, 0),
+};
+
+/* Right Mixer */
+static const struct snd_kcontrol_new es8323_right_mixer_controls[] = {
+	SOC_DAPM_SINGLE("Right Playback Switch", SND_SOC_NOPM, 6, 1, 1),
+	SOC_DAPM_SINGLE("Right Bypass Switch", ES8323_DACCONTROL20, 6, 1, 0),
+};
+
+static const struct snd_soc_dapm_widget es8323_dapm_widgets[] = {
+	SND_SOC_DAPM_INPUT("LINPUT1"),
+	SND_SOC_DAPM_INPUT("LINPUT2"),
+	SND_SOC_DAPM_INPUT("RINPUT1"),
+	SND_SOC_DAPM_INPUT("RINPUT2"),
+
+	SND_SOC_DAPM_MUX("Left PGA Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_left_dac_mux_controls),
+	SND_SOC_DAPM_MUX("Right PGA Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_right_dac_mux_controls),
+	SND_SOC_DAPM_MICBIAS("Mic Bias", SND_SOC_NOPM, 3, 1),
+
+	SND_SOC_DAPM_MUX("Differential Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_diffmux_controls),
+
+	SND_SOC_DAPM_MUX("Left ADC Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_monomux_controls),
+	SND_SOC_DAPM_MUX("Right ADC Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_monomux_controls),
+	SND_SOC_DAPM_MUX("Left Line Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_left_line_controls),
+	SND_SOC_DAPM_MUX("Right Line Mux", SND_SOC_NOPM, 0, 0,
+			 &es8323_right_line_controls),
+	SND_SOC_DAPM_ADC("Right ADC", "Right Capture", SND_SOC_NOPM, 4, 1),
+	SND_SOC_DAPM_ADC("Left ADC", "Left Capture", SND_SOC_NOPM, 5, 1),
+	/* gModify.Cmmt Implement when suspend/startup */
+	SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1),
+	SND_SOC_DAPM_DAC("Left DAC", "Left Playback", SND_SOC_NOPM, 7, 1),
+	SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0,
+			   &es8323_left_mixer_controls[0],
+			   ARRAY_SIZE(es8323_left_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Right Mixer", SND_SOC_NOPM, 0, 0,
+			   &es8323_right_mixer_controls[0],
+			   ARRAY_SIZE(es8323_right_mixer_controls)),
+
+	SND_SOC_DAPM_PGA("Right ADC Power", SND_SOC_NOPM, 6, 1, NULL, 0),
+	SND_SOC_DAPM_PGA("Left ADC Power", SND_SOC_NOPM, 7, 1, NULL, 0),
+	SND_SOC_DAPM_PGA("Right Out 2", SND_SOC_NOPM, 2, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Left Out 2", SND_SOC_NOPM, 3, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Right Out 1", SND_SOC_NOPM, 4, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Left Out 1", SND_SOC_NOPM, 5, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("LAMP", ES8323_ADCCONTROL1, 4, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("RAMP", ES8323_ADCCONTROL1, 0, 0, NULL, 0),
+
+	SND_SOC_DAPM_OUTPUT("LOUT1"),
+	SND_SOC_DAPM_OUTPUT("ROUT1"),
+	SND_SOC_DAPM_OUTPUT("LOUT2"),
+	SND_SOC_DAPM_OUTPUT("ROUT2"),
+	SND_SOC_DAPM_OUTPUT("VREF"),
+};
+
+static const struct snd_soc_dapm_route audio_map[] = {
+	/*12.22*/
+	{"Left PGA Mux", "Line 1L", "LINPUT1"},
+	{"Left PGA Mux", "Line 2L", "LINPUT2"},
+	{"Left PGA Mux", "DifferentialL", "Differential Mux"},
+
+	{"Right PGA Mux", "Line 1R", "RINPUT1"},
+	{"Right PGA Mux", "Line 2R", "RINPUT2"},
+	{"Right PGA Mux", "DifferentialR", "Differential Mux"},
+
+	{"Differential Mux", "Line 1", "LINPUT1"},
+	{"Differential Mux", "Line 1", "RINPUT1"},
+	{"Differential Mux", "Line 2", "LINPUT2"},
+	{"Differential Mux", "Line 2", "RINPUT2"},
+
+	{"Left ADC Mux", "Stereo", "Right PGA Mux"},
+	{"Left ADC Mux", "Stereo", "Left PGA Mux"},
+	{"Left ADC Mux", "Mono (Left)", "Left PGA Mux"},
+
+	{"Right ADC Mux", "Stereo", "Left PGA Mux"},
+	{"Right ADC Mux", "Stereo", "Right PGA Mux"},
+	{"Right ADC Mux", "Mono (Right)", "Right PGA Mux"},
+
+	{"Left ADC Power", NULL, "Left ADC Mux"},
+	{"Right ADC Power", NULL, "Right ADC Mux"},
+	{"Left ADC", NULL, "Left ADC Power"},
+	{"Right ADC", NULL, "Right ADC Power"},
+
+	{"Left Line Mux", "Line 1L", "LINPUT1"},
+	{"Left Line Mux", "Line 2L", "LINPUT2"},
+	{"Left Line Mux", "MicL", "Left PGA Mux"},
+
+	{"Right Line Mux", "Line 1R", "RINPUT1"},
+	{"Right Line Mux", "Line 2R", "RINPUT2"},
+	{"Right Line Mux", "MicR", "Right PGA Mux"},
+
+	{"Left Mixer", "Left Playback Switch", "Left DAC"},
+	{"Left Mixer", "Left Bypass Switch", "Left Line Mux"},
+
+	{"Right Mixer", "Right Playback Switch", "Right DAC"},
+	{"Right Mixer", "Right Bypass Switch", "Right Line Mux"},
+
+	{"Left Out 1", NULL, "Left Mixer"},
+	{"LOUT1", NULL, "Left Out 1"},
+	{"Right Out 1", NULL, "Right Mixer"},
+	{"ROUT1", NULL, "Right Out 1"},
+
+	{"Left Out 2", NULL, "Left Mixer"},
+	{"LOUT2", NULL, "Left Out 2"},
+	{"Right Out 2", NULL, "Right Mixer"},
+	{"ROUT2", NULL, "Right Out 2"},
+};
+
+struct coeff_div {
+	u32 mclk;
+	u32 rate;
+	u16 fs;
+	u8 sr:4;
+	u8 usb:1;
+};
+
+/* codec hifi mclk clock divider coefficients */
+static const struct coeff_div es8323_coeff_div[] = {
+	/* 8k */
+	{12288000, 8000, 1536, 0xa, 0x0},
+	{11289600, 8000, 1408, 0x9, 0x0},
+	{18432000, 8000, 2304, 0xc, 0x0},
+	{16934400, 8000, 2112, 0xb, 0x0},
+	{12000000, 8000, 1500, 0xb, 0x1},
+
+	/* 11.025k */
+	{11289600, 11025, 1024, 0x7, 0x0},
+	{16934400, 11025, 1536, 0xa, 0x0},
+	{12000000, 11025, 1088, 0x9, 0x1},
+
+	/* 16k */
+	{12288000, 16000, 768, 0x6, 0x0},
+	{18432000, 16000, 1152, 0x8, 0x0},
+	{12000000, 16000, 750, 0x7, 0x1},
+
+	/* 22.05k */
+	{11289600, 22050, 512, 0x4, 0x0},
+	{16934400, 22050, 768, 0x6, 0x0},
+	{12000000, 22050, 544, 0x6, 0x1},
+
+	/* 32k */
+	{12288000, 32000, 384, 0x3, 0x0},
+	{18432000, 32000, 576, 0x5, 0x0},
+	{12000000, 32000, 375, 0x4, 0x1},
+
+	/* 44.1k */
+	{11289600, 44100, 256, 0x2, 0x0},
+	{16934400, 44100, 384, 0x3, 0x0},
+	{12000000, 44100, 272, 0x3, 0x1},
+
+	/* 48k */
+	{12288000, 48000, 256, 0x2, 0x0},
+	{18432000, 48000, 384, 0x3, 0x0},
+	{12000000, 48000, 250, 0x2, 0x1},
+
+	/* 88.2k */
+	{11289600, 88200, 128, 0x0, 0x0},
+	{16934400, 88200, 192, 0x1, 0x0},
+	{12000000, 88200, 136, 0x1, 0x1},
+
+	/* 96k */
+	{12288000, 96000, 128, 0x0, 0x0},
+	{18432000, 96000, 192, 0x1, 0x0},
+	{12000000, 96000, 125, 0x0, 0x1},
+};
+
+/* The set of rates we can generate from the above for each SYSCLK */
+
+static unsigned int rates_12288[] = {
+	8000, 12000, 16000, 24000, 24000, 32000, 48000, 96000,
+};
+
+static struct snd_pcm_hw_constraint_list constraints_12288 = {
+	.count = ARRAY_SIZE(rates_12288),
+	.list = rates_12288,
+};
+
+static unsigned int rates_112896[] = {
+	8000, 11025, 22050, 44100,
+};
+
+static struct snd_pcm_hw_constraint_list constraints_112896 = {
+	.count = ARRAY_SIZE(rates_112896),
+	.list = rates_112896,
+};
+
+static unsigned int rates_12[] = {
+	8000, 11025, 12000, 16000, 22050, 24000,
+	32000, 44100, 48000, 48000, 88235, 96000,
+};
+
+static struct snd_pcm_hw_constraint_list constraints_12 = {
+	.count = ARRAY_SIZE(rates_12),
+	.list = rates_12,
+};
+
+static inline int get_coeff(int mclk, int rate)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(es8323_coeff_div); i++) {
+		if (es8323_coeff_div[i].rate == rate &&
+		    es8323_coeff_div[i].mclk == mclk)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static void es8323_set_gpio(int gpio, bool level, int spk_ctl_gpio)
+{
+	if (!(gpio & ES8323_CODEC_SET_SPK) || spk_ctl_gpio == -1)
+		return;
+
+	gpio_set_value(spk_ctl_gpio, level);
+}
+
+static int es8323_write(struct snd_soc_component *component,
+			unsigned int reg, unsigned int value)
+{
+	int ret;
+	u8 data[2];
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+
+	data[0] = reg;
+	data[1] = value & 0x00ff;
+
+	if (reg < ARRAY_SIZE(es8323_reg))
+		es8323_reg[reg] = value;
+
+	ret = i2c_master_send(es8323->i2c, data, 2);
+	if (ret == 2)
+		return 0;
+
+	return (ret < 0) ? ret : -EIO;
+}
+
+static unsigned int es8323_read_reg_cache(struct snd_soc_component *component,
+					  unsigned int reg)
+{
+	if (reg >= ARRAY_SIZE(es8323_reg))
+		return -1;
+
+	return es8323_reg[reg];
+}
+
+static void es8323_reset(struct snd_soc_component *component)
+{
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x80);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x00);
+}
+
+/*
+ * Note that this should be called from init rather than from hw_params.
+ */
+static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+				 int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_component *component = codec_dai->component;
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+
+	switch (freq) {
+	case 11289600:
+	case 18432000:
+	case 22579200:
+	case 36864000:
+		es8323->sysclk_constraints = &constraints_112896;
+		es8323->sysclk = freq;
+		break;
+	case 12288000:
+	case 16934400:
+	case 24576000:
+	case 33868800:
+		es8323->sysclk_constraints = &constraints_12288;
+		es8323->sysclk = freq;
+		break;
+	case 12000000:
+	case 24000000:
+		es8323->sysclk_constraints = &constraints_12;
+		es8323->sysclk = freq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int es8323_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = codec_dai->component;
+	u8 iface = snd_soc_component_read(component, ES8323_MASTERMODE);
+	u8 adciface = snd_soc_component_read(component, ES8323_ADC_IFACE);
+	u8 daciface = snd_soc_component_read(component, ES8323_DAC_IFACE);
+
+	/* set master/slave audio interface */
+	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBM_CFM:	/* MASTER MODE */
+		iface |= 0x80;
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:	/* SLAVE MODE */
+		iface &= 0x7F;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* interface format */
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		adciface &= 0xFC;
+		daciface &= 0xF9;
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+	case SND_SOC_DAIFMT_LEFT_J:
+	case SND_SOC_DAIFMT_DSP_A:
+	case SND_SOC_DAIFMT_DSP_B:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* clock inversion */
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF:
+		iface &= 0xDF;
+		adciface &= 0xDF;
+		daciface &= 0xBF;
+		break;
+	case SND_SOC_DAIFMT_IB_IF:
+		iface |= 0x20;
+		adciface |= 0x20;
+		daciface |= 0x40;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		iface |= 0x20;
+		adciface &= 0xDF;
+		daciface &= 0xBF;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		iface &= 0xDF;
+		adciface |= 0x20;
+		daciface |= 0x40;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	snd_soc_component_write(component, ES8323_MASTERMODE, iface);
+	snd_soc_component_write(component, ES8323_ADC_IFACE, adciface);
+	snd_soc_component_write(component, ES8323_DAC_IFACE, daciface);
+
+	return 0;
+}
+
+static int es8323_pcm_startup(struct snd_pcm_substream *substream,
+			      struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+
+	if (es8323->sysclk) {
+		snd_pcm_hw_constraint_list(substream->runtime, 0,
+					   SNDRV_PCM_HW_PARAM_RATE,
+					   es8323->sysclk_constraints);
+	}
+
+	return 0;
+}
+
+static int es8323_pcm_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+	u16 srate = snd_soc_component_read(component, ES8323_MASTERMODE) & 0x80;
+	u16 adciface = snd_soc_component_read(component, ES8323_ADC_IFACE) & 0xE3;
+	u16 daciface = snd_soc_component_read(component, ES8323_DAC_IFACE) & 0xC7;
+	int coeff;
+
+	coeff = get_coeff(es8323->sysclk, params_rate(params));
+	if (coeff < 0) {
+		coeff = get_coeff(es8323->sysclk / 2, params_rate(params));
+		srate |= 0x40;
+	}
+
+	if (coeff < 0) {
+		dev_err(component->dev,
+			"Unable to configure sample rate %dHz with %dHz MCLK\n",
+			params_rate(params), es8323->sysclk);
+		return coeff;
+	}
+
+	/* bit size */
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		adciface |= 0x000C;
+		daciface |= 0x0018;
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		adciface |= 0x0004;
+		daciface |= 0x0008;
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		adciface |= 0x0010;
+		daciface |= 0x0020;
+		break;
+	}
+
+	/* set iface & srate */
+	snd_soc_component_write(component, ES8323_DAC_IFACE, daciface);
+	snd_soc_component_write(component, ES8323_ADC_IFACE, adciface);
+
+	snd_soc_component_write(component, ES8323_MASTERMODE, srate);
+	snd_soc_component_write(component, ES8323_ADCCONTROL5,
+				es8323_coeff_div[coeff].sr |
+				(es8323_coeff_div[coeff].usb) << 4);
+	snd_soc_component_write(component, ES8323_DACCONTROL2,
+				es8323_coeff_div[coeff].sr |
+				(es8323_coeff_div[coeff].usb) << 4);
+
+	/* open dac output here */
+	snd_soc_component_write(component, ES8323_DACPOWER, 0x3c);
+
+	return 0;
+}
+
+static int es8323_mute(struct snd_soc_dai *dai, int mute,  int stream)
+{
+	struct snd_soc_component *component = dai->component;
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+
+	if (mute) {
+		es8323_set_gpio(ES8323_CODEC_SET_SPK,
+				!es8323->spk_gpio_level, es8323->spk_ctl_gpio);
+		usleep_range(2000, 3000);
+		snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
+	} else {
+		snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02);
+		usleep_range(2000, 3000);
+		if (!es8323->hp_inserted)
+			es8323_set_gpio(ES8323_CODEC_SET_SPK,
+					es8323->spk_gpio_level, es8323->spk_ctl_gpio);
+		usleep_range(2000, 3000);
+	}
+
+	return 0;
+}
+
+#define ES8323_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
+			SNDRV_PCM_FMTBIT_S24_LE)
+
+static const struct snd_soc_dai_ops es8323_ops = {
+	.startup	= es8323_pcm_startup,
+	.hw_params	= es8323_pcm_hw_params,
+	.set_fmt	= es8323_set_dai_fmt,
+	.set_sysclk	= es8323_set_dai_sysclk,
+	.mute_stream	= es8323_mute,
+};
+
+static struct snd_soc_dai_driver es8323_dai = {
+	.name = "ES8323 HiFi",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = ES8323_FORMATS,
+	},
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = ES8323_FORMATS,
+	},
+	.ops = &es8323_ops,
+	.symmetric_rate = 1,
+};
+
+static int es8323_suspend(struct snd_soc_component *component)
+{
+	snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
+	snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_DACCONTROL17, 0x38);
+	snd_soc_component_write(component, ES8323_DACCONTROL20, 0x38);
+	snd_soc_component_write(component, ES8323_DACPOWER, 0x00);
+	snd_soc_component_write(component, ES8323_DACPOWER, 0xc0);
+	snd_soc_component_write(component, ES8323_ADCPOWER, 0xff);
+	snd_soc_component_write(component, ES8323_CHIPPOWER, 0xf3);
+	snd_soc_component_write(component, ES8323_DACCONTROL21, 0x9c);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x06);
+	snd_soc_component_write(component, ES8323_CONTROL2, 0x58);
+	usleep_range(18000, 20000);
+
+	return 0;
+}
+
+static void es8323_init_component_regs(struct snd_soc_component *component)
+{
+	snd_soc_component_write(component, ES8323_CONTROL2, 0x60);
+	snd_soc_component_write(component, ES8323_CHIPPOWER, 0xF3);
+	snd_soc_component_write(component, ES8323_CHIPPOWER, 0xF0);
+	snd_soc_component_write(component, ES8323_DACCONTROL21, 0x80);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x36);
+	snd_soc_component_write(component, ES8323_MASTERMODE, 0x00);
+	snd_soc_component_write(component, ES8323_DACPOWER, 0x00);
+	snd_soc_component_write(component, ES8323_CHIPLOPOW2, 0xC3);
+	snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02);
+	snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88);
+	snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0);
+	snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02);
+	snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C);
+	snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02);
+	snd_soc_component_write(component, ES8323_LADC_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_RADC_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_ADCCONTROL10, 0xea);
+	snd_soc_component_write(component, ES8323_ADCCONTROL11, 0xa2);
+	snd_soc_component_write(component, ES8323_ADCCONTROL12, 0x32);
+	snd_soc_component_write(component, ES8323_DACCONTROL1, 0x18);
+	snd_soc_component_write(component, ES8323_DACCONTROL2, 0x02);
+	snd_soc_component_write(component, ES8323_LDAC_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_RDAC_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);
+	snd_soc_component_write(component, ES8323_DACCONTROL20, 0xB8);
+	usleep_range(18000, 20000);
+	snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x1e);
+	snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x1e);
+	snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x1e);
+	snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x1e);
+	snd_soc_component_write(component, ES8323_ADCPOWER, 0x09);
+	snd_soc_component_write(component, ES8323_CHIPPOWER, 0x00);
+	usleep_range(18000, 20000);
+}
+
+static int es8323_resume(struct snd_soc_component *component)
+{
+	es8323_init_component_regs(component);
+	/* open dac output */
+	snd_soc_component_write(component, ES8323_DACPOWER, 0x3c);
+
+	return 0;
+}
+
+static void es8323_remove(struct snd_soc_component *component)
+{
+	snd_soc_component_write(component, ES8323_CONTROL2, 0x58);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x32);
+	snd_soc_component_write(component, ES8323_CHIPPOWER, 0xf3);
+	snd_soc_component_write(component, ES8323_DACPOWER, 0xc0);
+	mdelay(50);
+	snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00);
+	snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00);
+	mdelay(50);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x30);
+	snd_soc_component_write(component, ES8323_CONTROL1, 0x34);
+}
+
+static int es8323_probe(struct snd_soc_component *component)
+{
+	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
+
+	es8323->mclk = devm_clk_get_optional_enabled(component->dev, "mclk");
+	if (IS_ERR(es8323->mclk)) {
+		dev_err(component->dev, "mclk is missing or invalid\n");
+		return PTR_ERR(es8323->mclk);
+	}
+
+	es8323->component = component;
+	es8323_reset(component);
+	es8323_init_component_regs(component);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver soc_codec_dev_es8323 = {
+	.probe			= es8323_probe,
+	.remove			= es8323_remove,
+	.suspend		= es8323_suspend,
+	.resume			= es8323_resume,
+	.read			= es8323_read_reg_cache,
+	.write			= es8323_write,
+	.dapm_widgets		= es8323_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(es8323_dapm_widgets),
+	.dapm_routes		= audio_map,
+	.num_dapm_routes	= ARRAY_SIZE(audio_map),
+	.controls		= es8323_snd_controls,
+	.num_controls		= ARRAY_SIZE(es8323_snd_controls),
+};
+
+static int es8323_i2c_probe(struct i2c_client *i2c)
+{
+	struct es8323_priv *es8323;
+	struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_I2C\n");
+		return -EIO;
+	}
+
+	es8323 = devm_kzalloc(&i2c->dev, sizeof(*es8323), GFP_KERNEL);
+	if (IS_ERR(es8323))
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, es8323);
+	es8323->i2c = i2c;
+
+	return devm_snd_soc_register_component(&i2c->dev,
+					       &soc_codec_dev_es8323,
+					       &es8323_dai, 1);
+}
+
+static void es8323_i2c_shutdown(struct i2c_client *client)
+{
+	struct es8323_priv *es8323 = i2c_get_clientdata(client);
+	struct snd_soc_component *component = es8323->component;
+
+	es8323_set_gpio(ES8323_CODEC_SET_SPK,
+			!es8323->spk_gpio_level, es8323->spk_ctl_gpio);
+
+	es8323_remove(component);
+}
+
+static const struct i2c_device_id es8323_i2c_id[] = {
+	{ "es8323", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, es8323_i2c_id);
+
+static const struct acpi_device_id es8323_acpi_match[] = {
+	{ "ESSX8323", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, es8323_acpi_match);
+
+static const struct of_device_id es8323_of_match[] = {
+	{ .compatible = "everest,es8323" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, es8323_of_match);
+
+static struct i2c_driver es8323_i2c_driver = {
+	.driver = {
+		.name = "ES8323",
+		.acpi_match_table = es8323_acpi_match,
+		.of_match_table = es8323_of_match,
+	},
+	.shutdown = es8323_i2c_shutdown,
+	.probe = es8323_i2c_probe,
+	.id_table = es8323_i2c_id,
+};
+module_i2c_driver(es8323_i2c_driver);
+
+MODULE_DESCRIPTION("Everest Semi ES8323 ALSA SoC Codec Driver");
+MODULE_AUTHOR("Mark Brown <will@everset-semi.com>");
+MODULE_AUTHOR("Binbin Zhou <zhoubinbin@loongson.cn>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/es8323.h b/sound/soc/codecs/es8323.h
new file mode 100644
index 000000000000..d379dcfc4fed
--- /dev/null
+++ b/sound/soc/codecs/es8323.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright Openedhand Ltd.
+ *
+ * Author: Richard Purdie <richard@openedhand.com>
+ *
+ */
+
+#ifndef _ES8323_H
+#define _ES8323_H
+
+/* ES8323 register space */
+
+/* Chip Control and Power Management */
+#define ES8323_CONTROL1		0x00
+#define ES8323_CONTROL2		0x01
+#define ES8323_CHIPPOWER	0x02
+#define ES8323_ADCPOWER		0x03
+#define ES8323_DACPOWER		0x04
+#define ES8323_CHIPLOPOW1	0x05
+#define ES8323_CHIPLOPOW2	0x06
+#define ES8323_ANAVOLMANAG	0x07
+#define ES8323_MASTERMODE	0x08
+
+/* ADC Control */
+#define ES8323_ADCCONTROL1	0x09
+#define ES8323_ADCCONTROL2	0x0a
+#define ES8323_ADCCONTROL3	0x0b
+#define ES8323_ADCCONTROL4	0x0c
+#define ES8323_ADCCONTROL5	0x0d
+#define ES8323_ADCCONTROL6	0x0e
+#define ES8323_ADC_MUTE		0x0f
+#define ES8323_LADC_VOL		0x10
+#define ES8323_RADC_VOL		0x11
+#define ES8323_ADCCONTROL10	0x12
+#define ES8323_ADCCONTROL11	0x13
+#define ES8323_ADCCONTROL12	0x14
+#define ES8323_ADCCONTROL13	0x15
+#define ES8323_ADCCONTROL14	0x16
+
+/* DAC Control */
+#define ES8323_DACCONTROL1	0x17
+#define ES8323_DACCONTROL2	0x18
+#define ES8323_DAC_MUTE		0x19
+#define ES8323_LDAC_VOL		0x1a
+#define ES8323_RDAC_VOL		0x1b
+#define ES8323_DACCONTROL6	0x1c
+#define ES8323_DACCONTROL7	0x1d
+#define ES8323_DACCONTROL8	0x1e
+#define ES8323_DACCONTROL9	0x1f
+#define ES8323_DACCONTROL10	0x20
+#define ES8323_DACCONTROL11	0x21
+#define ES8323_DACCONTROL12	0x22
+#define ES8323_DACCONTROL13	0x23
+#define ES8323_DACCONTROL14	0x24
+#define ES8323_DACCONTROL15	0x25
+#define ES8323_DACCONTROL16	0x26
+#define ES8323_DACCONTROL17	0x27
+#define ES8323_DACCONTROL18	0x28
+#define ES8323_DACCONTROL19	0x29
+#define ES8323_DACCONTROL20	0x2a
+#define ES8323_DACCONTROL21	0x2b
+#define ES8323_DACCONTROL22	0x2c
+#define ES8323_DACCONTROL23	0x2d
+#define ES8323_LOUT1_VOL	0x2e
+#define ES8323_ROUT1_VOL	0x2f
+#define ES8323_LOUT2_VOL	0x30
+#define ES8323_ROUT2_VOL	0x31
+#define ES8323_DACCONTROL28	0x32
+#define ES8323_DACCONTROL29	0x33
+#define ES8323_DACCONTROL30	0x34
+
+#define ES8323_ADC_IFACE	ES8323_ADCCONTROL4
+#define ES8323_ADC_SRATE	ES8323_ADCCONTROL5
+#define ES8323_DAC_IFACE	ES8323_DACCONTROL1
+#define ES8323_DAC_SRATE	ES8323_DACCONTROL2
+#endif
-- 
2.43.5

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5200 bytes --]

On Thu, Sep 05, 2024 at 03:02:15PM +0800, Binbin Zhou wrote:

This looks like it was based on some *extremely* old code and needs
quite a few updates to bring it up to modern standards.

> + * Author: Mark Brown <will@everset-semi.com>

Oh?

> +/*
> + * es8323 register cache
> + * We can't read the es8323 register space when we
> + * are using 2 wire for device control, so we cache them instead.
> + */
> +static u16 es8323_reg[] = {
> +	0x06, 0x1C, 0xC3, 0xFC,	/*  0 */
> +	0xC0, 0x00, 0x00, 0x7C,	/*  4 */

There's a bunch of regmap reimplementation in the driver, the cache and
I/O code all looks totally generic.  Just use regmap.

> +static const struct soc_enum es8323_enum[] = {
> +	SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts),
> +			      es8323_line_texts, es8323_line_values),	/* LLINE */
> +	SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts),
> +			      es8323_line_texts, es8323_line_values),	/* RLINE */

Use named variables for the enums rather than putting them into an array
that's not otherwise used...

> +static const struct snd_kcontrol_new es8323_snd_controls[] = {
> +	SOC_ENUM("3D Mode", es8323_enum[4]),
> +	SOC_ENUM("ALC Capture Function", es8323_enum[5]),

...it's *vastly* more readable and maintainable.

> +	SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0),

On/off switches should end in Switch, see control-names.rst.

> +	/* gModify.Cmmt Implement when suspend/startup */
> +	SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1),

gModify.Cmmt?

> +/*
> + * Note that this should be called from init rather than from hw_params.
> + */
> +static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +				 int clk_id, unsigned int freq, int dir)

Why?

> +	/* set master/slave audio interface */
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:	/* MASTER MODE */
> +		iface |= 0x80;
> +		break;
> +	case SND_SOC_DAIFMT_CBS_CFS:	/* SLAVE MODE */

Please use the modern naming with _CBP_CFP and so on.

> +static int es8323_mute(struct snd_soc_dai *dai, int mute,  int stream)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
> +
> +	if (mute) {
> +		es8323_set_gpio(ES8323_CODEC_SET_SPK,
> +				!es8323->spk_gpio_level, es8323->spk_ctl_gpio);
> +		usleep_range(2000, 3000);
> +		snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> +	} else {
> +		snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02);
> +		usleep_range(2000, 3000);
> +		if (!es8323->hp_inserted)
> +			es8323_set_gpio(ES8323_CODEC_SET_SPK,
> +					es8323->spk_gpio_level, es8323->spk_ctl_gpio);

This appears to be controlling the speaker based on jack detection.
Unless there is some hardware restriction this should be done via DAPM
and the user allowed to manage when the speaker is used depending on
their use case.

> +static int es8323_suspend(struct snd_soc_component *component)
> +{
> +	snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> +	snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00);
> +	snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00);
> +	snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00);
> +	snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00);

This will overwrite user settings when suspending, the controls should
be unaffected by suspend.  If the device needs this then use cache
bypass to do the writes during suspend and resync the cache on resume.

> +	snd_soc_component_write(component, ES8323_CONTROL1, 0x06);
> +	snd_soc_component_write(component, ES8323_CONTROL2, 0x58);
> +	usleep_range(18000, 20000);

Use fsleep().

> +static void es8323_init_component_regs(struct snd_soc_component *component)
> +{

> +	snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88);
> +	snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0);
> +	snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02);
> +	snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C);
> +	snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02);
> +	snd_soc_component_write(component, ES8323_LADC_VOL, 0x00);
> +	snd_soc_component_write(component, ES8323_RADC_VOL, 0x00);

User visible controls should use the chip defaults as standard,
userspace can configure what it needs and this avoids us worrying about
individual use cases in the driver.

> +static int es8323_resume(struct snd_soc_component *component)
> +{
> +	es8323_init_component_regs(component);
> +	/* open dac output */
> +	snd_soc_component_write(component, ES8323_DACPOWER, 0x3c);
> +
> +	return 0;
> +}

This doesn't restore any user settings.

> +static int es8323_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct es8323_priv *es8323;
> +	struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
> +		dev_warn(&adapter->dev,
> +			 "I2C-Adapter doesn't support I2C_FUNC_I2C\n");
> +		return -EIO;
> +	}

Does the device really need this?  It seems to be doing very basic I/O
which should be SMBus compatible.  In any case when you move to regmap
this should be redundant.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Binbin Zhou]

Hi Mark:

Thanks for your detailed review.

On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:15PM +0800, Binbin Zhou wrote:
>
> This looks like it was based on some *extremely* old code and needs
> quite a few updates to bring it up to modern standards.
>
> > + * Author: Mark Brown <will@everset-semi.com>
>
> Oh?
>
> > +/*
> > + * es8323 register cache
> > + * We can't read the es8323 register space when we
> > + * are using 2 wire for device control, so we cache them instead.
> > + */
> > +static u16 es8323_reg[] = {
> > +     0x06, 0x1C, 0xC3, 0xFC, /*  0 */
> > +     0xC0, 0x00, 0x00, 0x7C, /*  4 */
>
> There's a bunch of regmap reimplementation in the driver, the cache and
> I/O code all looks totally generic.  Just use regmap.

Sorry, I don't really understand this point.
Do you mean to use regmap_read()/regmap_write() instead of
snd_soc_component_read()/snd_soc_component_write()?

If so, are there any rules for using snd_soc_component_xxx()?
>
> > +static const struct soc_enum es8323_enum[] = {
> > +     SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts),
> > +                           es8323_line_texts, es8323_line_values),   /* LLINE */
> > +     SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts),
> > +                           es8323_line_texts, es8323_line_values),   /* RLINE */
>
> Use named variables for the enums rather than putting them into an array
> that's not otherwise used...

OK, I will use macro like SOC_ENUM_SINGLE_DECL() to define them.
>
> > +static const struct snd_kcontrol_new es8323_snd_controls[] = {
> > +     SOC_ENUM("3D Mode", es8323_enum[4]),
> > +     SOC_ENUM("ALC Capture Function", es8323_enum[5]),
>
> ...it's *vastly* more readable and maintainable.
>
> > +     SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0),
>
> On/off switches should end in Switch, see control-names.rst.
>
> > +     /* gModify.Cmmt Implement when suspend/startup */
> > +     SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1),
>
> gModify.Cmmt?
>
> > +/*
> > + * Note that this should be called from init rather than from hw_params.
> > + */
> > +static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> > +                              int clk_id, unsigned int freq, int dir)
>
> Why?
>
> > +     /* set master/slave audio interface */
> > +     switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +     case SND_SOC_DAIFMT_CBM_CFM:    /* MASTER MODE */
> > +             iface |= 0x80;
> > +             break;
> > +     case SND_SOC_DAIFMT_CBS_CFS:    /* SLAVE MODE */
>
> Please use the modern naming with _CBP_CFP and so on.

Get. SND_SOC_DAIFMT_BC_FP/SND_SOC_DAIFMT_BC_FC will be used.
>
> > +static int es8323_mute(struct snd_soc_dai *dai, int mute,  int stream)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
> > +
> > +     if (mute) {
> > +             es8323_set_gpio(ES8323_CODEC_SET_SPK,
> > +                             !es8323->spk_gpio_level, es8323->spk_ctl_gpio);
> > +             usleep_range(2000, 3000);
> > +             snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> > +     } else {
> > +             snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02);
> > +             usleep_range(2000, 3000);
> > +             if (!es8323->hp_inserted)
> > +                     es8323_set_gpio(ES8323_CODEC_SET_SPK,
> > +                                     es8323->spk_gpio_level, es8323->spk_ctl_gpio);
>
> This appears to be controlling the speaker based on jack detection.
> Unless there is some hardware restriction this should be done via DAPM
> and the user allowed to manage when the speaker is used depending on
> their use case.

In this version of the patch, I plan to remove the jack related
functions, and I will add them later.
>
> > +static int es8323_suspend(struct snd_soc_component *component)
> > +{
> > +     snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> > +     snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00);
>
> This will overwrite user settings when suspending, the controls should
> be unaffected by suspend.  If the device needs this then use cache
> bypass to do the writes during suspend and resync the cache on resume.

regcache_cache_only() and regcache_sync() will be used.
>
> > +     snd_soc_component_write(component, ES8323_CONTROL1, 0x06);
> > +     snd_soc_component_write(component, ES8323_CONTROL2, 0x58);
> > +     usleep_range(18000, 20000);
>
> Use fsleep().
>
> > +static void es8323_init_component_regs(struct snd_soc_component *component)
> > +{
>
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02);
> > +     snd_soc_component_write(component, ES8323_LADC_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_RADC_VOL, 0x00);
>
> User visible controls should use the chip defaults as standard,
> userspace can configure what it needs and this avoids us worrying about
> individual use cases in the driver.
>
> > +static int es8323_resume(struct snd_soc_component *component)
> > +{
> > +     es8323_init_component_regs(component);
> > +     /* open dac output */
> > +     snd_soc_component_write(component, ES8323_DACPOWER, 0x3c);
> > +
> > +     return 0;
> > +}
>
> This doesn't restore any user settings.
>
> > +static int es8323_i2c_probe(struct i2c_client *i2c)
> > +{
> > +     struct es8323_priv *es8323;
> > +     struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
> > +
> > +     if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
> > +             dev_warn(&adapter->dev,
> > +                      "I2C-Adapter doesn't support I2C_FUNC_I2C\n");
> > +             return -EIO;
> > +     }
>
> Does the device really need this?  It seems to be doing very basic I/O
> which should be SMBus compatible.  In any case when you move to regmap
> this should be redundant.

Emm, I will drop it.

Thanks.
Binbin

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On Fri, Sep 13, 2024 at 08:02:02AM +0600, Binbin Zhou wrote:
> On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:

> > > +/*
> > > + * es8323 register cache
> > > + * We can't read the es8323 register space when we
> > > + * are using 2 wire for device control, so we cache them instead.
> > > + */
> > > +static u16 es8323_reg[] = {
> > > +     0x06, 0x1C, 0xC3, 0xFC, /*  0 */
> > > +     0xC0, 0x00, 0x00, 0x7C, /*  4 */

> > There's a bunch of regmap reimplementation in the driver, the cache and
> > I/O code all looks totally generic.  Just use regmap.

> Sorry, I don't really understand this point.
> Do you mean to use regmap_read()/regmap_write() instead of
> snd_soc_component_read()/snd_soc_component_write()?

> If so, are there any rules for using snd_soc_component_xxx()?

No, it's fine to use the component wrappers if you happen to have a
component to hand.  What's an issue is things like writing your own
register cache code (the above bit is part of an open coded cache
implementation), or I2C read/write functions if there's not something
unusual with how the device does I/O.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Binbin Zhou]

Hi Mark:

Thanks for your explanation.

On Fri, Sep 13, 2024 at 10:44 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 13, 2024 at 08:02:02AM +0600, Binbin Zhou wrote:
> > On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > +/*
> > > > + * es8323 register cache
> > > > + * We can't read the es8323 register space when we
> > > > + * are using 2 wire for device control, so we cache them instead.
> > > > + */
> > > > +static u16 es8323_reg[] = {
> > > > +     0x06, 0x1C, 0xC3, 0xFC, /*  0 */
> > > > +     0xC0, 0x00, 0x00, 0x7C, /*  4 */
>
> > > There's a bunch of regmap reimplementation in the driver, the cache and
> > > I/O code all looks totally generic.  Just use regmap.
>
> > Sorry, I don't really understand this point.
> > Do you mean to use regmap_read()/regmap_write() instead of
> > snd_soc_component_read()/snd_soc_component_write()?
>
> > If so, are there any rules for using snd_soc_component_xxx()?
>
> No, it's fine to use the component wrappers if you happen to have a
> component to hand.  What's an issue is things like writing your own
> register cache code (the above bit is part of an open coded cache
> implementation), or I2C read/write functions if there's not something
> unusual with how the device does I/O.

Indeed, I misunderstood it before. As I understand it, we should use
regmap_config.reg_defaults instead of
snd_soc_component_driver.{read/write}.

Thanks.
Binbin

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Wed, Sep 18, 2024 at 03:24:45PM +0600, Binbin Zhou wrote:
> On Fri, Sep 13, 2024 at 10:44 PM Mark Brown <broonie@kernel.org> wrote:

> > No, it's fine to use the component wrappers if you happen to have a
> > component to hand.  What's an issue is things like writing your own
> > register cache code (the above bit is part of an open coded cache
> > implementation), or I2C read/write functions if there's not something
> > unusual with how the device does I/O.

> Indeed, I misunderstood it before. As I understand it, we should use
> regmap_config.reg_defaults instead of
> snd_soc_component_driver.{read/write}.

I think so - I'll need to see your actual patch to confirm but that
sounds like the right direction.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[kernel test robot]

Hi Binbin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 097a44db5747403b19d05a9664e8ec6adba27e3b]

url:    https://github.com/intel-lab-lkp/linux/commits/Binbin-Zhou/ASoC-dt-bindings-Add-Everest-ES8323-Codec/20240905-150958
base:   097a44db5747403b19d05a9664e8ec6adba27e3b
patch link:    https://lore.kernel.org/r/c44a6525d77941ef235825a58a9ea91f9f7d7042.1725518229.git.zhoubinbin%40loongson.cn
patch subject: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323
config: parisc-randconfig-r071-20240906 (https://download.01.org/0day-ci/archive/20240906/202409060459.dJs4SsBG-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240906/202409060459.dJs4SsBG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409060459.dJs4SsBG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/soc/codecs/es8323.c:173:38: warning: 'es8323_right_pga_controls' defined but not used [-Wunused-const-variable=]
     173 | static const struct snd_kcontrol_new es8323_right_pga_controls =
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/sound/tlv.h:10,
                    from sound/soc/codecs/es8323.c:25:
>> sound/soc/codecs/es8323.c:114:35: warning: 'pga_tlv' defined but not used [-Wunused-const-variable=]
     114 | static const DECLARE_TLV_DB_SCALE(pga_tlv, 0, 300, 0);
         |                                   ^~~~~~~
   include/uapi/sound/tlv.h:53:22: note: in definition of macro 'SNDRV_CTL_TLVD_DECLARE_DB_SCALE'
      53 |         unsigned int name[] = { \
         |                      ^~~~
   sound/soc/codecs/es8323.c:114:14: note: in expansion of macro 'DECLARE_TLV_DB_SCALE'
     114 | static const DECLARE_TLV_DB_SCALE(pga_tlv, 0, 300, 0);
         |              ^~~~~~~~~~~~~~~~~~~~


vim +/es8323_right_pga_controls +173 sound/soc/codecs/es8323.c

   113	
 > 114	static const DECLARE_TLV_DB_SCALE(pga_tlv, 0, 300, 0);
   115	static const DECLARE_TLV_DB_SCALE(adc_tlv, -9600, 50, 1);
   116	static const DECLARE_TLV_DB_SCALE(dac_tlv, -9600, 50, 1);
   117	static const DECLARE_TLV_DB_SCALE(out_tlv, -4500, 150, 0);
   118	static const DECLARE_TLV_DB_SCALE(bypass_tlv, 0, 300, 0);
   119	static const DECLARE_TLV_DB_SCALE(bypass_tlv2, -15, 300, 0);
   120	
   121	static const struct snd_kcontrol_new es8323_snd_controls[] = {
   122		SOC_ENUM("3D Mode", es8323_enum[4]),
   123		SOC_ENUM("ALC Capture Function", es8323_enum[5]),
   124		SOC_SINGLE("ALC Capture ZC Switch", ES8323_ADCCONTROL13, 6, 1, 0),
   125		SOC_SINGLE("ALC Capture Decay Time", ES8323_ADCCONTROL12, 4, 15, 0),
   126		SOC_SINGLE("ALC Capture Attack Time", ES8323_ADCCONTROL12, 0, 15, 0),
   127		SOC_SINGLE("ALC Capture NG Threshold", ES8323_ADCCONTROL14, 3, 31, 0),
   128		SOC_ENUM("ALC Capture NG Type", es8323_enum[6]),
   129		SOC_SINGLE("ALC Capture NG Switch", ES8323_ADCCONTROL14, 0, 1, 0),
   130		SOC_SINGLE("ZC Timeout Switch", ES8323_ADCCONTROL13, 6, 1, 0),
   131		SOC_DOUBLE_R_TLV("Capture Digital Volume", ES8323_LADC_VOL,
   132				 ES8323_RADC_VOL, 0, 192, 1, adc_tlv),
   133		SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0),
   134		SOC_SINGLE_TLV("Left Channel Capture Volume", ES8323_ADCCONTROL1, 4, 8,
   135			       0, bypass_tlv),
   136		SOC_SINGLE_TLV("Right Channel Capture Volume", ES8323_ADCCONTROL1, 0,
   137			       8, 0, bypass_tlv),
   138		SOC_ENUM("Playback De-emphasis", es8323_enum[7]),
   139		SOC_ENUM("Capture Polarity", es8323_enum[8]),
   140		SOC_DOUBLE_R_TLV("PCM Volume", ES8323_LDAC_VOL, ES8323_RDAC_VOL,
   141				 0, 192, 1, dac_tlv),
   142		SOC_SINGLE_TLV("Left Mixer Left Bypass Volume", ES8323_DACCONTROL17, 3,
   143			       7, 1, bypass_tlv2),
   144		SOC_SINGLE_TLV("Right Mixer Right Bypass Volume", ES8323_DACCONTROL20,
   145			       3, 7, 1, bypass_tlv2),
   146		SOC_DOUBLE_R_TLV("Output 1 Playback Volume", ES8323_LOUT1_VOL,
   147				 ES8323_ROUT1_VOL, 0, 33, 0, out_tlv),
   148		SOC_DOUBLE_R_TLV("Output 2 Playback Volume", ES8323_LOUT2_VOL,
   149				 ES8323_ROUT2_VOL, 0, 33, 0, out_tlv),
   150	};
   151	
   152	static const struct snd_kcontrol_new es8323_left_dac_mux_controls =
   153		SOC_DAPM_ENUM("Route", es8323_left_dac_enum);
   154	
   155	static const struct snd_kcontrol_new es8323_right_dac_mux_controls =
   156		SOC_DAPM_ENUM("Route", es8323_right_dac_enum);
   157	
   158	static const struct snd_kcontrol_new es8323_left_line_controls =
   159		SOC_DAPM_ENUM("LLIN Mux", es8323_llin_enum);
   160	
   161	static const struct snd_kcontrol_new es8323_right_line_controls =
   162		SOC_DAPM_ENUM("RLIN Mux", es8323_rlin_enum);
   163	
   164	/* Differential Mux */
   165	static const struct snd_kcontrol_new es8323_diffmux_controls =
   166		SOC_DAPM_ENUM("Route2", es8323_diff_enum);
   167	
   168	/* Mono ADC Mux */
   169	static const struct snd_kcontrol_new es8323_monomux_controls =
   170		SOC_DAPM_ENUM("Mono Mux", es8323_mono_enum);
   171	
   172	/* Right PGA Mux */
 > 173	static const struct snd_kcontrol_new es8323_right_pga_controls =
   174		SOC_DAPM_ENUM("Route", es8323_enum[3]);
   175	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v1 03/10] ASoC: dt-bindings: Add NXP uda1342 Codec

[Binbin Zhou]

Add NXP uda1342 CODEC binding with DT schema format using json-schema.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/sound/nxp,uda1342.yaml           | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/nxp,uda1342.yaml

diff --git a/Documentation/devicetree/bindings/sound/nxp,uda1342.yaml b/Documentation/devicetree/bindings/sound/nxp,uda1342.yaml
new file mode 100644
index 000000000000..71c6a5a2f5bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/nxp,uda1342.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/nxp,uda1342.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP uda1342 audio CODECs
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    const: nxp,uda1342
+
+  reg:
+    maxItems: 1
+
+  '#sound-dai-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - '#sound-dai-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      codec@1a {
+        compatible = "nxp,uda1342";
+        reg = <0x1a>;
+        #sound-dai-cells = <0>;
+      };
+    };
-- 
2.43.5

Re: [PATCH v1 03/10] ASoC: dt-bindings: Add NXP uda1342 Codec

[Krzysztof Kozlowski]

On Thu, Sep 05, 2024 at 03:02:16PM +0800, Binbin Zhou wrote:
> Add NXP uda1342 CODEC binding with DT schema format using json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/sound/nxp,uda1342.yaml           | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/nxp,uda1342.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver

[Binbin Zhou]

The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA
mic inputs), stereo audio DAC, with basic audio processing.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 sound/soc/codecs/Kconfig   |   8 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/uda1342.c | 397 +++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/uda1342.h |  77 +++++++
 4 files changed, 484 insertions(+)
 create mode 100644 sound/soc/codecs/uda1342.c
 create mode 100644 sound/soc/codecs/uda1342.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 85270aa43512..347ac5e8eca7 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -281,6 +281,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_TWL4030
 	imply SND_SOC_TWL6040
 	imply SND_SOC_UDA1334
+	imply SND_SOC_UDA1342
 	imply SND_SOC_UDA1380
 	imply SND_SOC_WCD9335
 	imply SND_SOC_WCD934X
@@ -2118,6 +2119,13 @@ config SND_SOC_UDA1334
 	  and has basic features such as de-emphasis (at 44.1 kHz sampling
 	  rate) and mute.
 
+config SND_SOC_UDA1342
+	tristate "NXP UDA1342 CODEC"
+	depends on I2C
+	help
+	  The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA mic inputs),
+	  stereo audio DAC, with basic audio processing.
+
 config SND_SOC_UDA1380
 	tristate
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 00ff3fdf0133..f5b1c8499d6c 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -319,6 +319,7 @@ snd-soc-ts3a227e-y := ts3a227e.o
 snd-soc-twl4030-y := twl4030.o
 snd-soc-twl6040-y := twl6040.o
 snd-soc-uda1334-y := uda1334.o
+snd-soc-uda1342-y := uda1342.o
 snd-soc-uda1380-y := uda1380.o
 snd-soc-wcd-classh-y := wcd-clsh-v2.o
 snd-soc-wcd-mbhc-y := wcd-mbhc-v2.o
@@ -723,6 +724,7 @@ obj-$(CONFIG_SND_SOC_TS3A227E)	+= snd-soc-ts3a227e.o
 obj-$(CONFIG_SND_SOC_TWL4030)	+= snd-soc-twl4030.o
 obj-$(CONFIG_SND_SOC_TWL6040)	+= snd-soc-twl6040.o
 obj-$(CONFIG_SND_SOC_UDA1334)	+= snd-soc-uda1334.o
+obj-$(CONFIG_SND_SOC_UDA1342)	+= snd-soc-uda1342.o
 obj-$(CONFIG_SND_SOC_UDA1380)	+= snd-soc-uda1380.o
 obj-$(CONFIG_SND_SOC_WCD_CLASSH)	+= snd-soc-wcd-classh.o
 obj-$(CONFIG_SND_SOC_WCD_MBHC)	+= snd-soc-wcd-mbhc.o
diff --git a/sound/soc/codecs/uda1342.c b/sound/soc/codecs/uda1342.c
new file mode 100644
index 000000000000..e26aa4148434
--- /dev/null
+++ b/sound/soc/codecs/uda1342.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * uda1342.c  --  UDA1342 ALSA SoC Codec driver
+ *
+ * Modifications by Christian Pellegrin <chripell@evolware.org>
+ *
+ * Copyright 2007 Dension Audio Systems Ltd.
+ * Author: Zoltan Devai
+ *
+ * Based on the WM87xx drivers by Liam Girdwood and Richard Purdie
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/initval.h>
+#include <sound/tlv.h>
+
+#include "uda1342.h"
+
+#define UDA134X_FORMATS	(SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S20_3LE)
+
+struct uda1342_priv {
+	int sysclk;
+	int dai_fmt;
+
+	struct snd_pcm_substream *master_substream;
+	struct snd_pcm_substream *slave_substream;
+
+	struct regmap *regmap;
+	struct i2c_client *i2c;
+};
+
+static const struct reg_default uda1342_reg_defaults[] = {
+	{ 0x00, 0x1042 },
+	{ 0x01, 0x0000 },
+	{ 0x10, 0x0088 },
+	{ 0x11, 0x0000 },
+	{ 0x12, 0x0000 },
+	{ 0x20, 0x0080 },
+	{ 0x21, 0x0080 },
+};
+
+static inline void uda1342_reset(struct uda1342_priv *uda1342)
+{
+	unsigned int mask = BIT(15);
+
+	regmap_write(uda1342->regmap, 0x00, mask);
+}
+
+static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction)
+{
+	struct snd_soc_component *component = dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+	unsigned int mask;
+	unsigned int val = 0;
+
+	dev_info(&uda1342->i2c->dev, "mute: %d\n", mute);
+
+	/* Master mute */
+	mask = BIT(5);
+	val = mute ? mask : 0;
+
+	return regmap_update_bits(uda1342->regmap, 0x10, mask, val);
+}
+
+static int uda1342_startup(struct snd_pcm_substream *substream,
+			   struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+	struct snd_pcm_runtime *master_runtime;
+
+	if (uda1342->master_substream) {
+		master_runtime = uda1342->master_substream->runtime;
+
+		snd_pcm_hw_constraint_single(substream->runtime,
+					     SNDRV_PCM_HW_PARAM_RATE, master_runtime->rate);
+		snd_pcm_hw_constraint_single(substream->runtime,
+					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
+					     master_runtime->sample_bits);
+
+		uda1342->slave_substream = substream;
+	} else {
+		uda1342->master_substream = substream;
+	}
+
+	return 0;
+}
+
+static void uda1342_shutdown(struct snd_pcm_substream *substream,
+			     struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+
+	if (uda1342->master_substream == substream)
+		uda1342->master_substream = uda1342->slave_substream;
+
+	uda1342->slave_substream = NULL;
+}
+
+static int uda1342_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+	struct device *dev = &uda1342->i2c->dev;
+	unsigned int hw_params = 0;
+
+	if (substream == uda1342->slave_substream) {
+		dev_info(dev, "ignoring hw_params for slave substream\n");
+		return 0;
+	}
+
+	/* set SYSCLK / fs ratio */
+	switch (uda1342->sysclk / params_rate(params)) {
+	case 512:
+		break;
+	case 384:
+		hw_params |= BIT(4);
+		break;
+	case 256:
+		hw_params |= BIT(5);
+		break;
+	default:
+		dev_err(dev, "unsupported frequency\n");
+		return -EINVAL;
+	}
+
+	/* set DAI format and word length */
+	switch (uda1342->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		switch (params_width(params)) {
+		case 16:
+			hw_params |= BIT(1);
+			break;
+		case 18:
+			hw_params |= BIT(2);
+			break;
+		case 20:
+			hw_params |= BIT(2) | BIT(1);
+			break;
+		default:
+			dev_err(dev, "unsupported format (right)\n");
+			return -EINVAL;
+		}
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		hw_params |= BIT(3);
+		break;
+	default:
+		dev_err(dev, "unsupported format\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int uda1342_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+				  int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_component *component = codec_dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+	struct device *dev = &uda1342->i2c->dev;
+
+	/*
+	 * Anything between 256fs*8Khz and 512fs*48Khz should be acceptable
+	 * because the codec is slave. Of course limitations of the clock
+	 * master (the IIS controller) apply.
+	 * We'll error out on set_hw_params if it's not OK
+	 */
+	if ((freq >= (256 * 8000)) && (freq <= (512 * 48000))) {
+		uda1342->sysclk = freq;
+		return 0;
+	}
+
+	dev_err(dev, "unsupported sysclk\n");
+
+	return -EINVAL;
+}
+
+static int uda1342_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = codec_dai->component;
+	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
+	struct device *dev = &uda1342->i2c->dev;
+
+	/* codec supports only full slave mode */
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
+		dev_err(dev, "unsupported slave mode.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * We can't setup DAI format here as it depends on the word bit num,
+	 * so let's just store the value for later
+	 */
+	uda1342->dai_fmt = fmt;
+
+	return 0;
+}
+
+static int uda1342_set_bias_level(struct snd_soc_component *component,
+				  enum snd_soc_bias_level level)
+{
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+		break;
+	case SND_SOC_BIAS_PREPARE:
+		break;
+	case SND_SOC_BIAS_STANDBY:
+		break;
+	case SND_SOC_BIAS_OFF:
+		break;
+	}
+
+	return 0;
+}
+
+static const char *const uda1342_deemph[] = {"None", "32Khz", "44.1Khz", "48Khz"};
+static const char *const uda1342_mixmode[] = {"Differential", "Analog1", "Analog2", "Both"};
+
+static const struct soc_enum uda1342_mixer_enum[] = {
+	SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph),
+	SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode),
+};
+
+static const struct snd_kcontrol_new uda1342_snd_controls[] = {
+	SOC_SINGLE("Master Playback Volume", 0x11, 0, 0x3F, 1),
+	SOC_SINGLE("Analog1 Volume", 0x12, 0, 0x1F, 1),
+};
+
+/* Common DAPM widgets */
+static const struct snd_soc_dapm_widget uda1342_dapm_widgets[] = {
+	SND_SOC_DAPM_INPUT("VINL1"),
+	SND_SOC_DAPM_INPUT("VINR1"),
+	SND_SOC_DAPM_INPUT("VINL2"),
+	SND_SOC_DAPM_INPUT("VINR2"),
+
+	SND_SOC_DAPM_DAC("DAC", "Playback", 0, 1, 0),
+	SND_SOC_DAPM_ADC("ADC", "Capture", 0, 9, 0),
+
+	SND_SOC_DAPM_OUTPUT("VOUTL"),
+	SND_SOC_DAPM_OUTPUT("VOUTR"),
+};
+
+static const struct snd_soc_dapm_route uda1342_dapm_routes[] = {
+	{ "ADC", NULL, "VINL1" },
+	{ "ADC", NULL, "VINR1" },
+	{ "ADC", NULL, "VINL2" },
+	{ "ADC", NULL, "VINR2" },
+	{ "VOUTL", NULL, "DAC" },
+	{ "VOUTR", NULL, "DAC" },
+};
+
+static const struct snd_soc_dai_ops uda1342_dai_ops = {
+	.startup	= uda1342_startup,
+	.shutdown	= uda1342_shutdown,
+	.hw_params	= uda1342_hw_params,
+	.mute_stream	= uda1342_mute,
+	.set_sysclk	= uda1342_set_dai_sysclk,
+	.set_fmt	= uda1342_set_dai_fmt,
+};
+
+static struct snd_soc_dai_driver uda1342_dai = {
+	.name = "uda1342-hifi",
+	/* playback capabilities */
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = UDA134X_FORMATS,
+	},
+	/* capture capabilities */
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = UDA134X_FORMATS,
+	},
+	/* pcm operations */
+	.ops = &uda1342_dai_ops,
+};
+
+static int uda1342_soc_probe(struct snd_soc_component *component)
+{
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
+
+	snd_soc_add_component_controls(component, uda1342_snd_controls,
+				       ARRAY_SIZE(uda1342_snd_controls));
+	snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets,
+				  ARRAY_SIZE(uda1342_dapm_widgets));
+	snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes,
+				ARRAY_SIZE(uda1342_dapm_routes));
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver soc_component_dev_uda1342 = {
+	.probe			= uda1342_soc_probe,
+	.set_bias_level		= uda1342_set_bias_level,
+	.suspend_bias_off	= 1,
+	.idle_bias_on		= 1,
+	.use_pmdown_time	= 1,
+	.endianness		= 1,
+};
+
+static const struct regmap_config uda1342_regmap = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0x21,
+	.reg_defaults = uda1342_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults),
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int uda1342_i2c_probe(struct i2c_client *i2c)
+{
+	struct uda1342_priv *uda1342;
+	int ret;
+
+	uda1342 = devm_kzalloc(&i2c->dev, sizeof(*uda1342), GFP_KERNEL);
+	if (!uda1342)
+		return -ENOMEM;
+
+	uda1342->regmap = devm_regmap_init_i2c(i2c, &uda1342_regmap);
+	if (IS_ERR(uda1342->regmap))
+		return PTR_ERR(uda1342->regmap);
+
+	i2c_set_clientdata(i2c, uda1342);
+	uda1342->i2c = i2c;
+
+	return devm_snd_soc_register_component(&i2c->dev,
+					       &soc_component_dev_uda1342,
+					       &uda1342_dai, 1);
+}
+
+static int uda1342_suspend(struct device *dev)
+{
+	struct uda1342_priv *uda1342 = dev_get_drvdata(dev);
+
+	regcache_cache_only(uda1342->regmap, true);
+
+	return 0;
+}
+
+static int uda1342_resume(struct device *dev)
+{
+	struct uda1342_priv *uda1342 = dev_get_drvdata(dev);
+
+	regcache_mark_dirty(uda1342->regmap);
+	regcache_sync(uda1342->regmap);
+
+	return 0;
+}
+
+static const struct dev_pm_ops uda1342_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(uda1342_suspend, uda1342_resume)
+};
+
+static const struct i2c_device_id uda1342_i2c_id[] = {
+	 { "uda1342", 0 },
+	 { }
+};
+MODULE_DEVICE_TABLE(i2c, uda1342_i2c_id);
+
+static const struct of_device_id uda1342_of_match[] = {
+	 { .compatible = "nxp,uda1342" },
+	 { }
+};
+MODULE_DEVICE_TABLE(of, uda1342_of_match);
+
+static struct i2c_driver uda1342_i2c_driver = {
+	.driver = {
+		.name =  "uda1342",
+		.of_match_table = uda1342_of_match,
+		.pm = pm_sleep_ptr(&uda1342_pm),
+	 },
+	.probe = uda1342_i2c_probe,
+	.id_table = uda1342_i2c_id,
+};
+module_i2c_driver(uda1342_i2c_driver);
+
+MODULE_DESCRIPTION("UDA1342 ALSA soc codec driver");
+MODULE_AUTHOR("Zoltan Devai, Christian Pellegrin <chripell@evolware.org>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/uda1342.h b/sound/soc/codecs/uda1342.h
new file mode 100644
index 000000000000..1f4632643ddd
--- /dev/null
+++ b/sound/soc/codecs/uda1342.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Audio support for NXP UDA1342
+ *
+ * Copyright (c) 2005 Giorgio Padrin <giorgio@mandarinlogiq.org>
+ * Copyright (c) 2024 Loongson Technology Corporation Limited.
+ */
+
+#ifndef _UDA1342_H
+#define _UDA1342_H
+
+#define UDA1342_CLK	0x00
+#define UDA1342_IFACE	0x01
+#define UDA1342_PM	0x02
+#define UDA1342_AMIX	0x03
+#define UDA1342_HP	0x04
+#define UDA1342_MVOL	0x11
+#define UDA1342_MIXVOL	0x12
+#define UDA1342_MODE	0x12
+#define UDA1342_DEEMP	0x13
+#define UDA1342_MIXER	0x14
+#define UDA1342_INTSTAT	0x18
+#define UDA1342_DEC	0x20
+#define UDA1342_PGA	0x21
+#define UDA1342_ADC	0x22
+#define UDA1342_AGC	0x23
+#define UDA1342_DECSTAT	0x28
+#define UDA1342_RESET	0x7f
+
+#define UDA1342_CACHEREGNUM 0x24
+
+/* Register flags */
+#define R00_EN_ADC	0x0800
+#define R00_EN_DEC	0x0400
+#define R00_EN_DAC	0x0200
+#define R00_EN_INT	0x0100
+#define R00_DAC_CLK	0x0010
+#define R01_SFORI_I2S   0x0000
+#define R01_SFORI_LSB16 0x0100
+#define R01_SFORI_LSB18 0x0200
+#define R01_SFORI_LSB20 0x0300
+#define R01_SFORI_MSB   0x0500
+#define R01_SFORI_MASK  0x0700
+#define R01_SFORO_I2S   0x0000
+#define R01_SFORO_LSB16 0x0001
+#define R01_SFORO_LSB18 0x0002
+#define R01_SFORO_LSB20 0x0003
+#define R01_SFORO_LSB24 0x0004
+#define R01_SFORO_MSB   0x0005
+#define R01_SFORO_MASK  0x0007
+#define R01_SEL_SOURCE  0x0040
+#define R01_SIM		0x0010
+#define R02_PON_PLL	0x8000
+#define R02_PON_HP	0x2000
+#define R02_PON_DAC	0x0400
+#define R02_PON_BIAS	0x0100
+#define R02_EN_AVC	0x0080
+#define R02_PON_AVC	0x0040
+#define R02_PON_LNA	0x0010
+#define R02_PON_PGAL	0x0008
+#define R02_PON_ADCL	0x0004
+#define R02_PON_PGAR	0x0002
+#define R02_PON_ADCR	0x0001
+#define R13_MTM		0x4000
+#define R14_SILENCE	0x0080
+#define R14_SDET_ON	0x0040
+#define R21_MT_ADC	0x8000
+#define R22_SEL_LNA	0x0008
+#define R22_SEL_MIC	0x0004
+#define R22_SKIP_DCFIL	0x0002
+#define R23_AGC_EN	0x0001
+
+#define UDA1342_DAI_DUPLEX	0 /* playback and capture on single DAI */
+#define UDA1342_DAI_PLAYBACK	1 /* playback DAI */
+#define UDA1342_DAI_CAPTURE	2 /* capture DAI */
+
+#endif /* _UDA1342_H */
-- 
2.43.5

Re: [PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]

On Thu, Sep 05, 2024 at 03:02:53PM +0800, Binbin Zhou wrote:

> The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA
> mic inputs), stereo audio DAC, with basic audio processing.

This looks basically fine overall, there's some issues below but they're
mostly fairly small and stylistic rather than anything major.

> --- /dev/null
> +++ b/sound/soc/codecs/uda1342.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * uda1342.c  --  UDA1342 ALSA SoC Codec driver

Please keep the entire comment a C++ one so things look more
intentional.

> +static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> +	unsigned int mask;
> +	unsigned int val = 0;
> +
> +	dev_info(&uda1342->i2c->dev, "mute: %d\n", mute);

This is far too noisy to be logged and will DoS the logs, please just
remove it.

> +
> +	/* Master mute */
> +	mask = BIT(5);
> +	val = mute ? mask : 0;

Please use normal conditional statements to improve legibility.

> +static void uda1342_shutdown(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> +
> +	if (uda1342->master_substream == substream)
> +		uda1342->master_substream = uda1342->slave_substream;

Please avoid using master/slave terminology, we've tended to go for
provider/consumer in ASoC.

> +static int uda1342_hw_params(struct snd_pcm_substream *substream,
> +			     struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = &uda1342->i2c->dev;
> +	unsigned int hw_params = 0;
> +
> +	if (substream == uda1342->slave_substream) {
> +		dev_info(dev, "ignoring hw_params for slave substream\n");
> +		return 0;
> +	}

This is also too noisy, it should be _dbg() at most.

> +	/* codec supports only full slave mode */
> +	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
> +		dev_err(dev, "unsupported slave mode.\n");
> +		return -EINVAL;
> +	}

Use the more modern _CBC_CFC.

> +	/*
> +	 * We can't setup DAI format here as it depends on the word bit num,
> +	 * so let's just store the value for later
> +	 */
> +	uda1342->dai_fmt = fmt;

No need to even store it if only one value is supported.

> +static int uda1342_set_bias_level(struct snd_soc_component *component,
> +				  enum snd_soc_bias_level level)
> +{
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		break;
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +	case SND_SOC_BIAS_STANDBY:
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		break;
> +	}
> +
> +	return 0;
> +}

This does nothing so it can just be removed.

> +static const struct soc_enum uda1342_mixer_enum[] = {
> +	SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph),
> +	SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode),
> +};

This doesn't seem to be referenced anywhere so can be removed, or should
be added to the controls or DAPM?

> +static int uda1342_soc_probe(struct snd_soc_component *component)
> +{
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> +
> +	snd_soc_add_component_controls(component, uda1342_snd_controls,
> +				       ARRAY_SIZE(uda1342_snd_controls));
> +	snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets,
> +				  ARRAY_SIZE(uda1342_dapm_widgets));
> +	snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes,
> +				ARRAY_SIZE(uda1342_dapm_routes));

You can point to these arrays from the component struct and have the
core register them for you.

> +static const struct regmap_config uda1342_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0x21,
> +	.reg_defaults = uda1342_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults),
> +	.cache_type = REGCACHE_RBTREE,

In general _MAPLE is preferred for new things unless you have a specific
reason to use _RBTREE, it uses a more modern data structure and should
generally do better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver

[Binbin Zhou]

Hi Mark:

Thanks for your detailed review.

On Thu, Sep 5, 2024 at 8:28 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:53PM +0800, Binbin Zhou wrote:
>
> > The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA
> > mic inputs), stereo audio DAC, with basic audio processing.
>
> This looks basically fine overall, there's some issues below but they're
> mostly fairly small and stylistic rather than anything major.
>
> > --- /dev/null
> > +++ b/sound/soc/codecs/uda1342.c
> > @@ -0,0 +1,397 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * uda1342.c  --  UDA1342 ALSA SoC Codec driver
>
> Please keep the entire comment a C++ one so things look more
> intentional.
>
> > +static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +     unsigned int mask;
> > +     unsigned int val = 0;
> > +
> > +     dev_info(&uda1342->i2c->dev, "mute: %d\n", mute);
>
> This is far too noisy to be logged and will DoS the logs, please just
> remove it.

OK, I will drop it.
>
> > +
> > +     /* Master mute */
> > +     mask = BIT(5);
> > +     val = mute ? mask : 0;
>
> Please use normal conditional statements to improve legibility.

OK, the code as following:

if (mute)
                val = mask;
>
> > +static void uda1342_shutdown(struct snd_pcm_substream *substream,
> > +                          struct snd_soc_dai *dai)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +
> > +     if (uda1342->master_substream == substream)
> > +             uda1342->master_substream = uda1342->slave_substream;
>
> Please avoid using master/slave terminology, we've tended to go for
> provider/consumer in ASoC.

OK, I see, I will  rename it.
>
> > +static int uda1342_hw_params(struct snd_pcm_substream *substream,
> > +                          struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +     struct device *dev = &uda1342->i2c->dev;
> > +     unsigned int hw_params = 0;
> > +
> > +     if (substream == uda1342->slave_substream) {
> > +             dev_info(dev, "ignoring hw_params for slave substream\n");
> > +             return 0;
> > +     }
>
> This is also too noisy, it should be _dbg() at most.

OK, I will drop it.
>
> > +     /* codec supports only full slave mode */
> > +     if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
> > +             dev_err(dev, "unsupported slave mode.\n");
> > +             return -EINVAL;
> > +     }
>
> Use the more modern _CBC_CFC.

OK, it should be replaced by SND_SOC_DAIFMT_BC_FC.
>
> > +     /*
> > +      * We can't setup DAI format here as it depends on the word bit num,
> > +      * so let's just store the value for later
> > +      */
> > +     uda1342->dai_fmt = fmt;
>
> No need to even store it if only one value is supported.

Emm, I will put the fmt setting here from the hw_param function.
Also, the dai_fmt could be dropped.
>
> > +static int uda1342_set_bias_level(struct snd_soc_component *component,
> > +                               enum snd_soc_bias_level level)
> > +{
> > +     switch (level) {
> > +     case SND_SOC_BIAS_ON:
> > +             break;
> > +     case SND_SOC_BIAS_PREPARE:
> > +             break;
> > +     case SND_SOC_BIAS_STANDBY:
> > +             break;
> > +     case SND_SOC_BIAS_OFF:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
>
> This does nothing so it can just be removed.

I will drop it.
>
> > +static const struct soc_enum uda1342_mixer_enum[] = {
> > +     SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph),
> > +     SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode),
> > +};
>
> This doesn't seem to be referenced anywhere so can be removed, or should
> be added to the controls or DAPM?

I will drop it.
>
> > +static int uda1342_soc_probe(struct snd_soc_component *component)
> > +{
> > +     struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> > +
> > +     snd_soc_add_component_controls(component, uda1342_snd_controls,
> > +                                    ARRAY_SIZE(uda1342_snd_controls));
> > +     snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets,
> > +                               ARRAY_SIZE(uda1342_dapm_widgets));
> > +     snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes,
> > +                             ARRAY_SIZE(uda1342_dapm_routes));
>
> You can point to these arrays from the component struct and have the
> core register them for you.

OK, I will do it.

Thanks.
Binbin
>
> > +static const struct regmap_config uda1342_regmap = {
> > +     .reg_bits = 8,
> > +     .val_bits = 16,
> > +     .max_register = 0x21,
> > +     .reg_defaults = uda1342_reg_defaults,
> > +     .num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults),
> > +     .cache_type = REGCACHE_RBTREE,
>
> In general _MAPLE is preferred for new things unless you have a specific
> reason to use _RBTREE, it uses a more modern data structure and should
> generally do better.

Re: [PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver

[kernel test robot]

Hi Binbin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 097a44db5747403b19d05a9664e8ec6adba27e3b]

url:    https://github.com/intel-lab-lkp/linux/commits/Binbin-Zhou/ASoC-dt-bindings-Add-Everest-ES8323-Codec/20240905-150958
base:   097a44db5747403b19d05a9664e8ec6adba27e3b
patch link:    https://lore.kernel.org/r/3fd0c3a04f5f3bd293168732db457f6854db706e.1725518229.git.zhoubinbin%40loongson.cn
patch subject: [PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver
config: parisc-randconfig-r071-20240906 (https://download.01.org/0day-ci/archive/20240906/202409060936.0UeDj3S7-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240906/202409060936.0UeDj3S7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409060936.0UeDj3S7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   sound/soc/codecs/uda1342.c: In function 'uda1342_i2c_probe':
>> sound/soc/codecs/uda1342.c:331:13: warning: unused variable 'ret' [-Wunused-variable]
     331 |         int ret;
         |             ^~~
   sound/soc/codecs/uda1342.c: At top level:
>> sound/soc/codecs/uda1342.c:358:12: warning: 'uda1342_resume' defined but not used [-Wunused-function]
     358 | static int uda1342_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~
>> sound/soc/codecs/uda1342.c:349:12: warning: 'uda1342_suspend' defined but not used [-Wunused-function]
     349 | static int uda1342_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~
>> sound/soc/codecs/uda1342.c:232:30: warning: 'uda1342_mixer_enum' defined but not used [-Wunused-const-variable=]
     232 | static const struct soc_enum uda1342_mixer_enum[] = {
         |                              ^~~~~~~~~~~~~~~~~~


vim +/ret +331 sound/soc/codecs/uda1342.c

   231	
 > 232	static const struct soc_enum uda1342_mixer_enum[] = {
   233		SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph),
   234		SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode),
   235	};
   236	
   237	static const struct snd_kcontrol_new uda1342_snd_controls[] = {
   238		SOC_SINGLE("Master Playback Volume", 0x11, 0, 0x3F, 1),
   239		SOC_SINGLE("Analog1 Volume", 0x12, 0, 0x1F, 1),
   240	};
   241	
   242	/* Common DAPM widgets */
   243	static const struct snd_soc_dapm_widget uda1342_dapm_widgets[] = {
   244		SND_SOC_DAPM_INPUT("VINL1"),
   245		SND_SOC_DAPM_INPUT("VINR1"),
   246		SND_SOC_DAPM_INPUT("VINL2"),
   247		SND_SOC_DAPM_INPUT("VINR2"),
   248	
   249		SND_SOC_DAPM_DAC("DAC", "Playback", 0, 1, 0),
   250		SND_SOC_DAPM_ADC("ADC", "Capture", 0, 9, 0),
   251	
   252		SND_SOC_DAPM_OUTPUT("VOUTL"),
   253		SND_SOC_DAPM_OUTPUT("VOUTR"),
   254	};
   255	
   256	static const struct snd_soc_dapm_route uda1342_dapm_routes[] = {
   257		{ "ADC", NULL, "VINL1" },
   258		{ "ADC", NULL, "VINR1" },
   259		{ "ADC", NULL, "VINL2" },
   260		{ "ADC", NULL, "VINR2" },
   261		{ "VOUTL", NULL, "DAC" },
   262		{ "VOUTR", NULL, "DAC" },
   263	};
   264	
   265	static const struct snd_soc_dai_ops uda1342_dai_ops = {
   266		.startup	= uda1342_startup,
   267		.shutdown	= uda1342_shutdown,
   268		.hw_params	= uda1342_hw_params,
   269		.mute_stream	= uda1342_mute,
   270		.set_sysclk	= uda1342_set_dai_sysclk,
   271		.set_fmt	= uda1342_set_dai_fmt,
   272	};
   273	
   274	static struct snd_soc_dai_driver uda1342_dai = {
   275		.name = "uda1342-hifi",
   276		/* playback capabilities */
   277		.playback = {
   278			.stream_name = "Playback",
   279			.channels_min = 1,
   280			.channels_max = 2,
   281			.rates = SNDRV_PCM_RATE_8000_48000,
   282			.formats = UDA134X_FORMATS,
   283		},
   284		/* capture capabilities */
   285		.capture = {
   286			.stream_name = "Capture",
   287			.channels_min = 1,
   288			.channels_max = 2,
   289			.rates = SNDRV_PCM_RATE_8000_48000,
   290			.formats = UDA134X_FORMATS,
   291		},
   292		/* pcm operations */
   293		.ops = &uda1342_dai_ops,
   294	};
   295	
   296	static int uda1342_soc_probe(struct snd_soc_component *component)
   297	{
   298		struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
   299	
   300		snd_soc_add_component_controls(component, uda1342_snd_controls,
   301					       ARRAY_SIZE(uda1342_snd_controls));
   302		snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets,
   303					  ARRAY_SIZE(uda1342_dapm_widgets));
   304		snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes,
   305					ARRAY_SIZE(uda1342_dapm_routes));
   306	
   307		return 0;
   308	}
   309	
   310	static const struct snd_soc_component_driver soc_component_dev_uda1342 = {
   311		.probe			= uda1342_soc_probe,
   312		.set_bias_level		= uda1342_set_bias_level,
   313		.suspend_bias_off	= 1,
   314		.idle_bias_on		= 1,
   315		.use_pmdown_time	= 1,
   316		.endianness		= 1,
   317	};
   318	
   319	static const struct regmap_config uda1342_regmap = {
   320		.reg_bits = 8,
   321		.val_bits = 16,
   322		.max_register = 0x21,
   323		.reg_defaults = uda1342_reg_defaults,
   324		.num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults),
   325		.cache_type = REGCACHE_RBTREE,
   326	};
   327	
   328	static int uda1342_i2c_probe(struct i2c_client *i2c)
   329	{
   330		struct uda1342_priv *uda1342;
 > 331		int ret;
   332	
   333		uda1342 = devm_kzalloc(&i2c->dev, sizeof(*uda1342), GFP_KERNEL);
   334		if (!uda1342)
   335			return -ENOMEM;
   336	
   337		uda1342->regmap = devm_regmap_init_i2c(i2c, &uda1342_regmap);
   338		if (IS_ERR(uda1342->regmap))
   339			return PTR_ERR(uda1342->regmap);
   340	
   341		i2c_set_clientdata(i2c, uda1342);
   342		uda1342->i2c = i2c;
   343	
   344		return devm_snd_soc_register_component(&i2c->dev,
   345						       &soc_component_dev_uda1342,
   346						       &uda1342_dai, 1);
   347	}
   348	
 > 349	static int uda1342_suspend(struct device *dev)
   350	{
   351		struct uda1342_priv *uda1342 = dev_get_drvdata(dev);
   352	
   353		regcache_cache_only(uda1342->regmap, true);
   354	
   355		return 0;
   356	}
   357	
 > 358	static int uda1342_resume(struct device *dev)
   359	{
   360		struct uda1342_priv *uda1342 = dev_get_drvdata(dev);
   361	
   362		regcache_mark_dirty(uda1342->regmap);
   363		regcache_sync(uda1342->regmap);
   364	
   365		return 0;
   366	}
   367	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v1 05/10] ASoC: loongson: Improve code readability

[Binbin Zhou]

This patch attempts to clean up driver code formatting issues.
Mainly as follows:
1. Use the BIT macro;
2. Use dev_err_probe() in every error path in probe in loongson_card
   driver;
3. Introduce loongson_card_acpi_find_device() to streamlined code.

No functional change intended.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 sound/soc/loongson/loongson_card.c    | 144 +++++++++++++-------------
 sound/soc/loongson/loongson_dma.c     |  10 +-
 sound/soc/loongson/loongson_i2s.c     | 110 ++++++++++----------
 sound/soc/loongson/loongson_i2s.h     |  24 ++---
 sound/soc/loongson/loongson_i2s_pci.c |  51 ++++-----
 5 files changed, 166 insertions(+), 173 deletions(-)

diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
index 2c8dbdba27c5..a25287efdd5c 100644
--- a/sound/soc/loongson/loongson_card.c
+++ b/sound/soc/loongson/loongson_card.c
@@ -24,27 +24,27 @@ static int loongson_card_hw_params(struct snd_pcm_substream *substream,
 				   struct snd_pcm_hw_params *params)
 {
 	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
-	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
-	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
 	struct loongson_card_data *ls_card = snd_soc_card_get_drvdata(rtd->card);
+	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
+	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
 	int ret, mclk;
 
-	if (ls_card->mclk_fs) {
-		mclk = ls_card->mclk_fs * params_rate(params);
-		ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk,
-					     SND_SOC_CLOCK_OUT);
-		if (ret < 0) {
-			dev_err(codec_dai->dev, "cpu_dai clock not set\n");
-			return ret;
-		}
+	if (!ls_card->mclk_fs)
+		return 0;
 
-		ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
-					     SND_SOC_CLOCK_IN);
-		if (ret < 0) {
-			dev_err(codec_dai->dev, "codec_dai clock not set\n");
-			return ret;
-		}
+	mclk = ls_card->mclk_fs * params_rate(params);
+	ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, SND_SOC_CLOCK_OUT);
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "cpu_dai clock not set\n");
+		return ret;
 	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "codec_dai clock not set\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -68,54 +68,61 @@ static struct snd_soc_dai_link loongson_dai_links[] = {
 	},
 };
 
-static int loongson_card_parse_acpi(struct loongson_card_data *data)
+static struct acpi_device *
+loongson_card_acpi_find_device(struct snd_soc_card *card, const char *name)
 {
-	struct snd_soc_card *card = &data->snd_card;
 	struct fwnode_handle *fwnode = card->dev->fwnode;
 	struct fwnode_reference_args args;
+	int status;
+
+	memset(&args, 0, sizeof(args));
+	status = acpi_node_get_property_reference(fwnode, name, 0, &args);
+	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode)) {
+		dev_err(card->dev, "No matching phy in ACPI table\n");
+		return NULL;
+	}
+
+	return to_acpi_device_node(args.fwnode);
+}
+
+static int loongson_card_parse_acpi(struct loongson_card_data *data)
+{
+	struct snd_soc_card *card = &data->snd_card;
 	const char *codec_dai_name;
 	struct acpi_device *adev;
 	struct device *phy_dev;
-	int ret, i;
+	int i;
 
 	/* fixup platform name based on reference node */
-	memset(&args, 0, sizeof(args));
-	ret = acpi_node_get_property_reference(fwnode, "cpu", 0, &args);
-	if (ret || !is_acpi_device_node(args.fwnode)) {
-		dev_err(card->dev, "No matching phy in ACPI table\n");
-		return ret ?: -ENOENT;
-	}
-	adev = to_acpi_device_node(args.fwnode);
+	adev = loongson_card_acpi_find_device(card, "cpu");
+	if (!adev)
+		return -ENOENT;
+
 	phy_dev = acpi_get_first_physical_node(adev);
 	if (!phy_dev)
 		return -EPROBE_DEFER;
-	for (i = 0; i < card->num_links; i++)
-		loongson_dai_links[i].platforms->name = dev_name(phy_dev);
 
 	/* fixup codec name based on reference node */
-	memset(&args, 0, sizeof(args));
-	ret = acpi_node_get_property_reference(fwnode, "codec", 0, &args);
-	if (ret || !is_acpi_device_node(args.fwnode)) {
-		dev_err(card->dev, "No matching phy in ACPI table\n");
-		return ret ?: -ENOENT;
-	}
-	adev = to_acpi_device_node(args.fwnode);
+	adev = loongson_card_acpi_find_device(card, "codec");
+	if (!adev)
+		return -ENOENT;
 	snprintf(codec_name, sizeof(codec_name), "i2c-%s", acpi_dev_name(adev));
-	for (i = 0; i < card->num_links; i++)
-		loongson_dai_links[i].codecs->name = codec_name;
 
-	device_property_read_string(card->dev, "codec-dai-name",
-				    &codec_dai_name);
-	for (i = 0; i < card->num_links; i++)
+	device_property_read_string(card->dev, "codec-dai-name", &codec_dai_name);
+
+	for (i = 0; i < card->num_links; i++) {
+		loongson_dai_links[i].platforms->name = dev_name(phy_dev);
+		loongson_dai_links[i].codecs->name = codec_name;
 		loongson_dai_links[i].codecs->dai_name = codec_dai_name;
+	}
 
 	return 0;
 }
 
 static int loongson_card_parse_of(struct loongson_card_data *data)
 {
-	struct device_node *cpu, *codec;
 	struct snd_soc_card *card = &data->snd_card;
+	struct device_node *cpu, *codec;
 	struct device *dev = card->dev;
 	int ret, i;
 
@@ -124,77 +131,68 @@ static int loongson_card_parse_of(struct loongson_card_data *data)
 		dev_err(dev, "platform property missing or invalid\n");
 		return -EINVAL;
 	}
+
 	codec = of_get_child_by_name(dev->of_node, "codec");
 	if (!codec) {
 		dev_err(dev, "audio-codec property missing or invalid\n");
-		of_node_put(cpu);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free_cpu;
 	}
 
 	for (i = 0; i < card->num_links; i++) {
 		ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
-		if (ret < 0) {
+		if (ret) {
 			dev_err(dev, "getting cpu dlc error (%d)\n", ret);
-			goto err;
+			goto free_codec;
 		}
 
 		ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
-		if (ret < 0) {
+		if (ret) {
 			dev_err(dev, "getting codec dlc error (%d)\n", ret);
-			goto err;
+			goto free_codec;
 		}
 	}
 
-	of_node_put(cpu);
+free_codec:
 	of_node_put(codec);
-
-	return 0;
-
-err:
+free_cpu:
 	of_node_put(cpu);
-	of_node_put(codec);
 	return ret;
 }
 
 static int loongson_asoc_card_probe(struct platform_device *pdev)
 {
 	struct loongson_card_data *ls_priv;
+	struct device *dev = &pdev->dev;
 	struct snd_soc_card *card;
 	int ret;
 
-	ls_priv = devm_kzalloc(&pdev->dev, sizeof(*ls_priv), GFP_KERNEL);
+	ls_priv = devm_kzalloc(dev, sizeof(*ls_priv), GFP_KERNEL);
 	if (!ls_priv)
 		return -ENOMEM;
 
 	card = &ls_priv->snd_card;
 
-	card->dev = &pdev->dev;
+	card->dev = dev;
 	card->owner = THIS_MODULE;
 	card->dai_link = loongson_dai_links;
 	card->num_links = ARRAY_SIZE(loongson_dai_links);
 	snd_soc_card_set_drvdata(card, ls_priv);
 
-	ret = device_property_read_string(&pdev->dev, "model", &card->name);
-	if (ret) {
-		dev_err(&pdev->dev, "Error parsing card name: %d\n", ret);
-		return ret;
-	}
-	ret = device_property_read_u32(&pdev->dev, "mclk-fs", &ls_priv->mclk_fs);
-	if (ret) {
-		dev_err(&pdev->dev, "Error parsing mclk-fs: %d\n", ret);
-		return ret;
-	}
+	ret = device_property_read_string(dev, "model", &card->name);
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing card name.\n");
 
-	if (has_acpi_companion(&pdev->dev))
-		ret = loongson_card_parse_acpi(ls_priv);
-	else
-		ret = loongson_card_parse_of(ls_priv);
-	if (ret < 0)
-		return ret;
+	ret = device_property_read_u32(dev, "mclk-fs", &ls_priv->mclk_fs);
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing mclk-fs.\n");
 
-	ret = devm_snd_soc_register_card(&pdev->dev, card);
+	ret = has_acpi_companion(dev) ? loongson_card_parse_acpi(ls_priv)
+				      : loongson_card_parse_of(ls_priv);
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing acpi/of properties.\n");
 
-	return ret;
+	return devm_snd_soc_register_card(dev, card);
 }
 
 static const struct of_device_id loongson_asoc_dt_ids[] = {
diff --git a/sound/soc/loongson/loongson_dma.c b/sound/soc/loongson/loongson_dma.c
index 0238f88bc674..20e4a0641340 100644
--- a/sound/soc/loongson/loongson_dma.c
+++ b/sound/soc/loongson/loongson_dma.c
@@ -17,11 +17,11 @@
 #include "loongson_i2s.h"
 
 /* DMA dma_order Register */
-#define DMA_ORDER_STOP          (1 << 4) /* DMA stop */
-#define DMA_ORDER_START         (1 << 3) /* DMA start */
-#define DMA_ORDER_ASK_VALID     (1 << 2) /* DMA ask valid flag */
-#define DMA_ORDER_AXI_UNCO      (1 << 1) /* Uncache access */
-#define DMA_ORDER_ADDR_64       (1 << 0) /* 64bits address support */
+#define DMA_ORDER_STOP          BIT(4) /* DMA stop */
+#define DMA_ORDER_START         BIT(3) /* DMA start */
+#define DMA_ORDER_ASK_VALID     BIT(2) /* DMA ask valid flag */
+#define DMA_ORDER_AXI_UNCO      BIT(1) /* Uncache access */
+#define DMA_ORDER_ADDR_64       BIT(0) /* 64bits address support */
 
 #define DMA_ORDER_ASK_MASK      (~0x1fUL) /* Ask addr mask */
 #define DMA_ORDER_CTRL_MASK     (0x0fUL)  /* Control mask  */
diff --git a/sound/soc/loongson/loongson_i2s.c b/sound/soc/loongson/loongson_i2s.c
index 3b9786076501..40bbf3205391 100644
--- a/sound/soc/loongson/loongson_i2s.c
+++ b/sound/soc/loongson/loongson_i2s.c
@@ -21,34 +21,33 @@
 			SNDRV_PCM_FMTBIT_S20_3LE | \
 			SNDRV_PCM_FMTBIT_S24_LE)
 
+#define LOONGSON_I2S_TX_ENABLE	(I2S_CTRL_TX_EN | I2S_CTRL_TX_DMA_EN)
+#define LOONGSON_I2S_RX_ENABLE	(I2S_CTRL_RX_EN | I2S_CTRL_RX_DMA_EN)
+
+#define LOONGSON_I2S_DEF_DELAY		10
+#define LOONGSON_I2S_DEF_TIMEOUT	500000
+
 static int loongson_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 				struct snd_soc_dai *dai)
 {
 	struct loongson_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+	unsigned int mask;
 	int ret = 0;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					   I2S_CTRL_TX_EN | I2S_CTRL_TX_DMA_EN,
-					   I2S_CTRL_TX_EN | I2S_CTRL_TX_DMA_EN);
-		else
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					   I2S_CTRL_RX_EN | I2S_CTRL_RX_DMA_EN,
-					   I2S_CTRL_RX_EN | I2S_CTRL_RX_DMA_EN);
+		mask = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
+		       LOONGSON_I2S_TX_ENABLE : LOONGSON_I2S_RX_ENABLE;
+		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, mask, mask);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					I2S_CTRL_TX_EN | I2S_CTRL_TX_DMA_EN, 0);
-		else
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					I2S_CTRL_RX_EN | I2S_CTRL_RX_DMA_EN, 0);
+		mask = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
+		       LOONGSON_I2S_TX_ENABLE : LOONGSON_I2S_RX_ENABLE;
+		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, mask, 0);
 		break;
 	default:
 		ret = -EINVAL;
@@ -123,10 +122,40 @@ static int loongson_i2s_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
 	return 0;
 }
 
+static int loongson_i2s_enable_mclk(struct loongson_i2s *i2s)
+{
+	u32 val;
+
+	if (i2s->rev_id == 0)
+		return 0;
+
+	regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
+			   I2S_CTRL_MCLK_EN, I2S_CTRL_MCLK_EN);
+
+	return regmap_read_poll_timeout_atomic(i2s->regmap,
+					       LS_I2S_CTRL, val,
+					       val & I2S_CTRL_MCLK_READY,
+					       LOONGSON_I2S_DEF_DELAY,
+					       LOONGSON_I2S_DEF_TIMEOUT);
+}
+
+static int loongson_i2s_enable_bclk(struct loongson_i2s *i2s)
+{
+	u32 val;
+
+	if (i2s->rev_id == 0)
+		return 0;
+
+	return regmap_read_poll_timeout_atomic(i2s->regmap,
+					       LS_I2S_CTRL, val,
+					       val & I2S_CTRL_CLK_READY,
+					       LOONGSON_I2S_DEF_DELAY,
+					       LOONGSON_I2S_DEF_TIMEOUT);
+}
+
 static int loongson_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct loongson_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-	u32 val;
 	int ret;
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
@@ -148,54 +177,29 @@ static int loongson_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		/* Enable master mode */
 		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_MASTER,
 				   I2S_CTRL_MASTER);
-		if (i2s->rev_id == 1) {
-			ret = regmap_read_poll_timeout_atomic(i2s->regmap,
-						LS_I2S_CTRL, val,
-						val & I2S_CTRL_CLK_READY,
-						10, 500000);
-			if (ret < 0)
-				dev_warn(dai->dev, "wait BCLK ready timeout\n");
-		}
+		ret = loongson_i2s_enable_bclk(i2s);
+		if (ret < 0)
+			dev_warn(dai->dev, "wait BCLK ready timeout\n");
 		break;
 	case SND_SOC_DAIFMT_BC_FP:
 		/* Enable MCLK */
-		if (i2s->rev_id == 1) {
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					   I2S_CTRL_MCLK_EN,
-					   I2S_CTRL_MCLK_EN);
-			ret = regmap_read_poll_timeout_atomic(i2s->regmap,
-						LS_I2S_CTRL, val,
-						val & I2S_CTRL_MCLK_READY,
-						10, 500000);
-			if (ret < 0)
-				dev_warn(dai->dev, "wait MCLK ready timeout\n");
-		}
+		ret = loongson_i2s_enable_mclk(i2s);
+		if (ret < 0)
+			dev_warn(dai->dev, "wait MCLK ready timeout\n");
 		break;
 	case SND_SOC_DAIFMT_BP_FP:
 		/* Enable MCLK */
-		if (i2s->rev_id == 1) {
-			regmap_update_bits(i2s->regmap, LS_I2S_CTRL,
-					   I2S_CTRL_MCLK_EN,
-					   I2S_CTRL_MCLK_EN);
-			ret = regmap_read_poll_timeout_atomic(i2s->regmap,
-						LS_I2S_CTRL, val,
-						val & I2S_CTRL_MCLK_READY,
-						10, 500000);
-			if (ret < 0)
-				dev_warn(dai->dev, "wait MCLK ready timeout\n");
-		}
+		ret = loongson_i2s_enable_mclk(i2s);
+		if (ret < 0)
+			dev_warn(dai->dev, "wait MCLK ready timeout\n");
 
 		/* Enable master mode */
 		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_MASTER,
 				   I2S_CTRL_MASTER);
-		if (i2s->rev_id == 1) {
-			ret = regmap_read_poll_timeout_atomic(i2s->regmap,
-						LS_I2S_CTRL, val,
-						val & I2S_CTRL_CLK_READY,
-						10, 500000);
-			if (ret < 0)
-				dev_warn(dai->dev, "wait BCLK ready timeout\n");
-		}
+
+		ret = loongson_i2s_enable_bclk(i2s);
+		if (ret < 0)
+			dev_warn(dai->dev, "wait BCLK ready timeout\n");
 		break;
 	default:
 		return -EINVAL;
diff --git a/sound/soc/loongson/loongson_i2s.h b/sound/soc/loongson/loongson_i2s.h
index 89492eebf834..c8052a762c1b 100644
--- a/sound/soc/loongson/loongson_i2s.h
+++ b/sound/soc/loongson/loongson_i2s.h
@@ -27,18 +27,18 @@
 #define LS_I2S_RX_ORDER 0x110 /* RX DMA Order */
 
 /* Loongson I2S Control Register */
-#define I2S_CTRL_MCLK_READY	(1 << 16) /* MCLK ready */
-#define I2S_CTRL_MASTER		(1 << 15) /* Master mode */
-#define I2S_CTRL_MSB		(1 << 14) /* MSB bit order */
-#define I2S_CTRL_RX_EN		(1 << 13) /* RX enable */
-#define I2S_CTRL_TX_EN		(1 << 12) /* TX enable */
-#define I2S_CTRL_RX_DMA_EN	(1 << 11) /* DMA RX enable */
-#define I2S_CTRL_CLK_READY	(1 << 8)  /* BCLK ready */
-#define I2S_CTRL_TX_DMA_EN	(1 << 7)  /* DMA TX enable */
-#define I2S_CTRL_RESET		(1 << 4)  /* Controller soft reset */
-#define I2S_CTRL_MCLK_EN	(1 << 3)  /* Enable MCLK */
-#define I2S_CTRL_RX_INT_EN	(1 << 1)  /* RX interrupt enable */
-#define I2S_CTRL_TX_INT_EN	(1 << 0)  /* TX interrupt enable */
+#define I2S_CTRL_MCLK_READY	BIT(16) /* MCLK ready */
+#define I2S_CTRL_MASTER		BIT(15) /* Master mode */
+#define I2S_CTRL_MSB		BIT(14) /* MSB bit order */
+#define I2S_CTRL_RX_EN		BIT(13) /* RX enable */
+#define I2S_CTRL_TX_EN		BIT(12) /* TX enable */
+#define I2S_CTRL_RX_DMA_EN	BIT(11) /* DMA RX enable */
+#define I2S_CTRL_CLK_READY	BIT(8)  /* BCLK ready */
+#define I2S_CTRL_TX_DMA_EN	BIT(7)  /* DMA TX enable */
+#define I2S_CTRL_RESET		BIT(4)  /* Controller soft reset */
+#define I2S_CTRL_MCLK_EN	BIT(3)  /* Enable MCLK */
+#define I2S_CTRL_RX_INT_EN	BIT(1)  /* RX interrupt enable */
+#define I2S_CTRL_TX_INT_EN	BIT(0)  /* TX interrupt enable */
 
 #define LS_I2S_DRVNAME		"loongson-i2s"
 
diff --git a/sound/soc/loongson/loongson_i2s_pci.c b/sound/soc/loongson/loongson_i2s_pci.c
index ec18b122cd79..3872b1d8fce0 100644
--- a/sound/soc/loongson/loongson_i2s_pci.c
+++ b/sound/soc/loongson/loongson_i2s_pci.c
@@ -75,34 +75,34 @@ static int loongson_i2s_pci_probe(struct pci_dev *pdev,
 {
 	const struct fwnode_handle *fwnode = pdev->dev.fwnode;
 	struct loongson_dma_data *tx_data, *rx_data;
+	struct device *dev = &pdev->dev;
 	struct loongson_i2s *i2s;
 	int ret;
 
 	if (pcim_enable_device(pdev)) {
-		dev_err(&pdev->dev, "pci_enable_device failed\n");
+		dev_err(dev, "pci_enable_device failed\n");
 		return -ENODEV;
 	}
 
-	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
+	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
 	if (!i2s)
 		return -ENOMEM;
 
 	i2s->rev_id = pdev->revision;
-	i2s->dev = &pdev->dev;
+	i2s->dev = dev;
 	pci_set_drvdata(pdev, i2s);
 
-	ret = pcim_iomap_regions(pdev, 1 << 0, dev_name(&pdev->dev));
+	ret = pcim_iomap_regions(pdev, 1 << 0, dev_name(dev));
 	if (ret < 0) {
-		dev_err(&pdev->dev, "iomap_regions failed\n");
+		dev_err(dev, "iomap_regions failed\n");
 		return ret;
 	}
+
 	i2s->reg_base = pcim_iomap_table(pdev)[0];
-	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, i2s->reg_base,
+	i2s->regmap = devm_regmap_init_mmio(dev, i2s->reg_base,
 					    &loongson_i2s_regmap_config);
-	if (IS_ERR(i2s->regmap)) {
-		dev_err(&pdev->dev, "regmap_init_mmio failed\n");
-		return PTR_ERR(i2s->regmap);
-	}
+	if (IS_ERR(i2s->regmap))
+		dev_err_probe(dev, PTR_ERR(i2s->regmap), "regmap_init_mmio failed\n");
 
 	tx_data = &i2s->tx_dma_data;
 	rx_data = &i2s->rx_dma_data;
@@ -114,37 +114,28 @@ static int loongson_i2s_pci_probe(struct pci_dev *pdev,
 	rx_data->order_addr = i2s->reg_base + LS_I2S_RX_ORDER;
 
 	tx_data->irq = fwnode_irq_get_byname(fwnode, "tx");
-	if (tx_data->irq < 0) {
-		dev_err(&pdev->dev, "dma tx irq invalid\n");
-		return tx_data->irq;
-	}
+	if (tx_data->irq < 0)
+		dev_err_probe(dev, tx_data->irq, "dma tx irq invalid\n");
 
 	rx_data->irq = fwnode_irq_get_byname(fwnode, "rx");
-	if (rx_data->irq < 0) {
-		dev_err(&pdev->dev, "dma rx irq invalid\n");
-		return rx_data->irq;
-	}
+	if (rx_data->irq < 0)
+		dev_err_probe(dev, rx_data->irq, "dma rx irq invalid\n");
 
-	device_property_read_u32(&pdev->dev, "clock-frequency", &i2s->clk_rate);
-	if (!i2s->clk_rate) {
-		dev_err(&pdev->dev, "clock-frequency property invalid\n");
-		return -EINVAL;
-	}
+	ret = device_property_read_u32(dev, "clock-frequency", &i2s->clk_rate);
+	if (ret)
+		dev_err_probe(dev, ret, "clock-frequency property invalid\n");
 
-	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
 
 	if (i2s->rev_id == 1) {
 		regmap_write(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET);
 		udelay(200);
 	}
 
-	ret = devm_snd_soc_register_component(&pdev->dev,
-					      &loongson_i2s_component,
+	ret = devm_snd_soc_register_component(dev, &loongson_i2s_component,
 					      &loongson_i2s_dai, 1);
-	if (ret) {
-		dev_err(&pdev->dev, "register DAI failed %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		dev_err_probe(dev, ret, "register DAI failed\n");
 
 	return 0;
 }
-- 
2.43.5

Re: [PATCH v1 05/10] ASoC: loongson: Improve code readability

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Thu, Sep 05, 2024 at 03:02:54PM +0800, Binbin Zhou wrote:
> This patch attempts to clean up driver code formatting issues.
> Mainly as follows:
> 1. Use the BIT macro;
> 2. Use dev_err_probe() in every error path in probe in loongson_card
>    driver;
> 3. Introduce loongson_card_acpi_find_device() to streamlined code.

Please split these out into three separate patches to make them easier
to review.  I see there's also some other code reordering and
reformatting in there which should also be split out separately.  I
think the changes are probably good overall but there's too much
different stuff going on in one patch to check properly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 05/10] ASoC: loongson: Improve code readability

[Binbin Zhou]

On Thu, Sep 5, 2024 at 8:31 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:54PM +0800, Binbin Zhou wrote:
> > This patch attempts to clean up driver code formatting issues.
> > Mainly as follows:
> > 1. Use the BIT macro;
> > 2. Use dev_err_probe() in every error path in probe in loongson_card
> >    driver;
> > 3. Introduce loongson_card_acpi_find_device() to streamlined code.
>
> Please split these out into three separate patches to make them easier
> to review.  I see there's also some other code reordering and
> reformatting in there which should also be split out separately.  I
> think the changes are probably good overall but there's too much
> different stuff going on in one patch to check properly.

Hi Mark:

Thanks for your reply.

That was my fault, indeed this patch is too cluttered. I will try to
split them up and make them as a separate patch series.

Thanks.
Binbin

[PATCH v1 06/10] ASoC: loongson: Fix codec detection failure on FDT systems

[Binbin Zhou]

When the Codec is compiled into a module, we can't use
snd_soc_of_get_dlc() to get the codec dai_name, use
snd_soc_get_dai_name() instead.

Also, for the cpu dailink, its dai_name is already defined as
"loongson-i2s", so just get the corresponding of_node attribute here.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 26 deletions(-)

diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
index a25287efdd5c..d45a3e77cb90 100644
--- a/sound/soc/loongson/loongson_card.c
+++ b/sound/soc/loongson/loongson_card.c
@@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
 	return 0;
 }
 
-static int loongson_card_parse_of(struct loongson_card_data *data)
+static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node)
 {
-	struct snd_soc_card *card = &data->snd_card;
-	struct device_node *cpu, *codec;
-	struct device *dev = card->dev;
-	int ret, i;
+	struct device_node *cpu;
+	int ret = 0;
 
 	cpu = of_get_child_by_name(dev->of_node, "cpu");
-	if (!cpu) {
-		dev_err(dev, "platform property missing or invalid\n");
+	if (!cpu)
 		return -EINVAL;
-	}
+
+	*dai_node = of_parse_phandle(cpu, "sound-dai", 0);
+	if (!*dai_node)
+		ret = -EINVAL;
+
+	of_node_put(cpu);
+	return ret;
+}
+
+static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
+				const char **dai_name)
+{
+	struct of_phandle_args args;
+	struct device_node *codec;
+	int ret = 0;
 
 	codec = of_get_child_by_name(dev->of_node, "codec");
-	if (!codec) {
-		dev_err(dev, "audio-codec property missing or invalid\n");
+	if (!codec)
+		return -EINVAL;
+
+	ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args);
+	if (ret)
+		goto free_codec;
+
+	ret = snd_soc_get_dai_name(&args, dai_name);
+	if (ret)
+		goto free_codec;
+
+	*dai_node = of_parse_phandle(codec, "sound-dai", 0);
+	if (!*dai_node)
 		ret = -EINVAL;
-		goto free_cpu;
+
+free_codec:
+	of_node_put(codec);
+	return ret;
+}
+
+static int loongson_card_parse_of(struct loongson_card_data *data)
+{
+	struct device_node *codec_dai_node, *cpu_dai_node;
+	struct snd_soc_card *card = &data->snd_card;
+	struct device *dev = card->dev;
+	const char *codec_dai_name;
+	int ret = 0, i;
+
+	ret = loongson_parse_cpu(dev, &cpu_dai_node);
+	if (ret) {
+		dev_err(dev, "cpu property missing or invalid.\n");
+		goto out;
+	}
+
+	ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name);
+	if (ret) {
+		dev_err(dev, "audio-codec property missing or invalid.\n");
+		goto out;
 	}
 
 	for (i = 0; i < card->num_links; i++) {
-		ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
-		if (ret) {
-			dev_err(dev, "getting cpu dlc error (%d)\n", ret);
-			goto free_codec;
-		}
-
-		ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
-		if (ret) {
-			dev_err(dev, "getting codec dlc error (%d)\n", ret);
-			goto free_codec;
-		}
+		loongson_dai_links[i].platforms->of_node = cpu_dai_node;
+		loongson_dai_links[i].cpus->of_node = cpu_dai_node;
+		loongson_dai_links[i].codecs->of_node = codec_dai_node;
+		loongson_dai_links[i].codecs->dai_name = codec_dai_name;
 	}
 
-free_codec:
-	of_node_put(codec);
-free_cpu:
-	of_node_put(cpu);
+out:
+	of_node_put(codec_dai_node);
+	of_node_put(cpu_dai_node);
 	return ret;
 }
 
-- 
2.43.5

Re: [PATCH v1 06/10] ASoC: loongson: Fix codec detection failure on FDT systems

[Krzysztof Kozlowski]

On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote:
> When the Codec is compiled into a module, we can't use
> snd_soc_of_get_dlc() to get the codec dai_name, use
> snd_soc_get_dai_name() instead.
> 
> Also, for the cpu dailink, its dai_name is already defined as
> "loongson-i2s", so just get the corresponding of_node attribute here.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++---------
>  1 file changed, 63 insertions(+), 26 deletions(-)
> 
> diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
> index a25287efdd5c..d45a3e77cb90 100644
> --- a/sound/soc/loongson/loongson_card.c
> +++ b/sound/soc/loongson/loongson_card.c
> @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
>  	return 0;
>  }
>  
> -static int loongson_card_parse_of(struct loongson_card_data *data)
> +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node)
>  {
> -	struct snd_soc_card *card = &data->snd_card;
> -	struct device_node *cpu, *codec;
> -	struct device *dev = card->dev;
> -	int ret, i;
> +	struct device_node *cpu;
> +	int ret = 0;
>  
>  	cpu = of_get_child_by_name(dev->of_node, "cpu");
> -	if (!cpu) {
> -		dev_err(dev, "platform property missing or invalid\n");
> +	if (!cpu)
>  		return -EINVAL;
> -	}
> +
> +	*dai_node = of_parse_phandle(cpu, "sound-dai", 0);
> +	if (!*dai_node)
> +		ret = -EINVAL;
> +
> +	of_node_put(cpu);

This goes just after of_parse_phandle, which simplifies your code.

> +	return ret;
> +}
> +
> +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
> +				const char **dai_name)
> +{
> +	struct of_phandle_args args;
> +	struct device_node *codec;
> +	int ret = 0;
>  
>  	codec = of_get_child_by_name(dev->of_node, "codec");
> -	if (!codec) {
> -		dev_err(dev, "audio-codec property missing or invalid\n");
> +	if (!codec)

Hm? So you exit here and then caller does of_node_put on stack value.
This is buggy.

> +		return -EINVAL;
> +
> +	ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args);
> +	if (ret)
> +		goto free_codec;
> +
> +	ret = snd_soc_get_dai_name(&args, dai_name);
> +	if (ret)

Your error paths should clean here. Each function is responsible to
clean up after itself in case of errors, not rely on caller.

> +		goto free_codec;
> +
> +	*dai_node = of_parse_phandle(codec, "sound-dai", 0);
> +	if (!*dai_node)
>  		ret = -EINVAL;
> -		goto free_cpu;
> +
> +free_codec:

You are not freeing any resources here (at least not directly). You are
dropping reference. Use matching label name. free is associated with
kalloc()..


> +	of_node_put(codec);
> +	return ret;
> +}
> +
> +static int loongson_card_parse_of(struct loongson_card_data *data)
> +{
> +	struct device_node *codec_dai_node, *cpu_dai_node;
> +	struct snd_soc_card *card = &data->snd_card;
> +	struct device *dev = card->dev;
> +	const char *codec_dai_name;
> +	int ret = 0, i;
> +
> +	ret = loongson_parse_cpu(dev, &cpu_dai_node);
> +	if (ret) {
> +		dev_err(dev, "cpu property missing or invalid.\n");
> +		goto out;
> +	}
> +
> +	ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name);
> +	if (ret) {
> +		dev_err(dev, "audio-codec property missing or invalid.\n");
> +		goto out;
>  	}
>  
>  	for (i = 0; i < card->num_links; i++) {
> -		ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
> -		if (ret) {
> -			dev_err(dev, "getting cpu dlc error (%d)\n", ret);
> -			goto free_codec;
> -		}
> -
> -		ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
> -		if (ret) {
> -			dev_err(dev, "getting codec dlc error (%d)\n", ret);
> -			goto free_codec;
> -		}
> +		loongson_dai_links[i].platforms->of_node = cpu_dai_node;
> +		loongson_dai_links[i].cpus->of_node = cpu_dai_node;
> +		loongson_dai_links[i].codecs->of_node = codec_dai_node;
> +		loongson_dai_links[i].codecs->dai_name = codec_dai_name;
>  	}
>  
> -free_codec:
> -	of_node_put(codec);
> -free_cpu:
> -	of_node_put(cpu);
> +out:
> +	of_node_put(codec_dai_node);

Yeah, so here we see drop of reference from stack-based pointer...

Best regards,
Krzysztof

Re: [PATCH v1 06/10] ASoC: loongson: Fix codec detection failure on FDT systems

[Binbin Zhou]

Hi Krzysztof:

Thanks for your detailed review.

On Fri, Sep 6, 2024 at 4:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote:
> > When the Codec is compiled into a module, we can't use
> > snd_soc_of_get_dlc() to get the codec dai_name, use
> > snd_soc_get_dai_name() instead.
> >
> > Also, for the cpu dailink, its dai_name is already defined as
> > "loongson-i2s", so just get the corresponding of_node attribute here.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++---------
> >  1 file changed, 63 insertions(+), 26 deletions(-)
> >
> > diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
> > index a25287efdd5c..d45a3e77cb90 100644
> > --- a/sound/soc/loongson/loongson_card.c
> > +++ b/sound/soc/loongson/loongson_card.c
> > @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
> >       return 0;
> >  }
> >
> > -static int loongson_card_parse_of(struct loongson_card_data *data)
> > +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node)
> >  {
> > -     struct snd_soc_card *card = &data->snd_card;
> > -     struct device_node *cpu, *codec;
> > -     struct device *dev = card->dev;
> > -     int ret, i;
> > +     struct device_node *cpu;
> > +     int ret = 0;
> >
> >       cpu = of_get_child_by_name(dev->of_node, "cpu");
> > -     if (!cpu) {
> > -             dev_err(dev, "platform property missing or invalid\n");
> > +     if (!cpu)
> >               return -EINVAL;
> > -     }
> > +
> > +     *dai_node = of_parse_phandle(cpu, "sound-dai", 0);
> > +     if (!*dai_node)
> > +             ret = -EINVAL;
> > +
> > +     of_node_put(cpu);
>
> This goes just after of_parse_phandle, which simplifies your code.

OK.. the ret param is unnecessary also.
>
> > +     return ret;
> > +}
> > +
> > +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
> > +                             const char **dai_name)
> > +{
> > +     struct of_phandle_args args;
> > +     struct device_node *codec;
> > +     int ret = 0;
> >
> >       codec = of_get_child_by_name(dev->of_node, "codec");
> > -     if (!codec) {
> > -             dev_err(dev, "audio-codec property missing or invalid\n");
> > +     if (!codec)
>
> Hm? So you exit here and then caller does of_node_put on stack value.
> This is buggy.

Sorry, I can not get your point, I think there is nothing that should be put.
>
> > +             return -EINVAL;
> > +
> > +     ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args);
> > +     if (ret)
> > +             goto free_codec;
> > +
> > +     ret = snd_soc_get_dai_name(&args, dai_name);
> > +     if (ret)
>
> Your error paths should clean here. Each function is responsible to
> clean up after itself in case of errors, not rely on caller.

Yes, I should of_node_put(args.np); here
>
> > +             goto free_codec;
> > +
> > +     *dai_node = of_parse_phandle(codec, "sound-dai", 0);
> > +     if (!*dai_node)
> >               ret = -EINVAL;
> > -             goto free_cpu;
> > +
> > +free_codec:
>
> You are not freeing any resources here (at least not directly). You are
> dropping reference. Use matching label name. free is associated with
> kalloc()..
>

OK, I will rename the label name as codec_put.

Thanks.
Binbin
>
> > +     of_node_put(codec);
> > +     return ret;
> > +}
> > +
> > +static int loongson_card_parse_of(struct loongson_card_data *data)
> > +{
> > +     struct device_node *codec_dai_node, *cpu_dai_node;
> > +     struct snd_soc_card *card = &data->snd_card;
> > +     struct device *dev = card->dev;
> > +     const char *codec_dai_name;
> > +     int ret = 0, i;
> > +
> > +     ret = loongson_parse_cpu(dev, &cpu_dai_node);
> > +     if (ret) {
> > +             dev_err(dev, "cpu property missing or invalid.\n");
> > +             goto out;
> > +     }
> > +
> > +     ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name);
> > +     if (ret) {
> > +             dev_err(dev, "audio-codec property missing or invalid.\n");
> > +             goto out;
> >       }
> >
> >       for (i = 0; i < card->num_links; i++) {
> > -             ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
> > -             if (ret) {
> > -                     dev_err(dev, "getting cpu dlc error (%d)\n", ret);
> > -                     goto free_codec;
> > -             }
> > -
> > -             ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
> > -             if (ret) {
> > -                     dev_err(dev, "getting codec dlc error (%d)\n", ret);
> > -                     goto free_codec;
> > -             }
> > +             loongson_dai_links[i].platforms->of_node = cpu_dai_node;
> > +             loongson_dai_links[i].cpus->of_node = cpu_dai_node;
> > +             loongson_dai_links[i].codecs->of_node = codec_dai_node;
> > +             loongson_dai_links[i].codecs->dai_name = codec_dai_name;
> >       }
> >
> > -free_codec:
> > -     of_node_put(codec);
> > -free_cpu:
> > -     of_node_put(cpu);
> > +out:
> > +     of_node_put(codec_dai_node);
>
> Yeah, so here we see drop of reference from stack-based pointer...
>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 06/10] ASoC: loongson: Fix codec detection failure on FDT systems

[Krzysztof Kozlowski]

On 09/09/2024 09:51, Binbin Zhou wrote:
>>
>>> +     return ret;
>>> +}
>>> +
>>> +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
>>> +                             const char **dai_name)
>>> +{
>>> +     struct of_phandle_args args;
>>> +     struct device_node *codec;
>>> +     int ret = 0;
>>>
>>>       codec = of_get_child_by_name(dev->of_node, "codec");
>>> -     if (!codec) {
>>> -             dev_err(dev, "audio-codec property missing or invalid\n");
>>> +     if (!codec)
>>
>> Hm? So you exit here and then caller does of_node_put on stack value.
>> This is buggy.
> 
> Sorry, I can not get your point, I think there is nothing that should be put.

You drop reference from a pointer which is a random stack value.

Best regards,
Krzysztof

[PATCH v1 07/10] ASoC: dt-bindings: Add Loongson I2S controller

[Binbin Zhou]

Add Loongson I2S controller binding with DT schema format using
json-schema.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/sound/loongson,ls2k-i2s.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml

diff --git a/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
new file mode 100644
index 000000000000..a2e3bbe00dab
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/loongson,ls2k-i2s.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-2K I2S controller
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    const: loongson,ls2k1000-i2s
+
+  reg:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  dmas:
+    maxItems: 2
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+  '#sound-dai-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - dmas
+  - dma-names
+  - '#sound-dai-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2s@1fe2d000 {
+        compatible = "loongson,ls2k1000-i2s";
+        reg = <0x1fe2d000 0x14>,
+              <0x1fe00438 0x8>;
+        interrupt-parent = <&liointc0>;
+        interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk LOONGSON2_APB_CLK>;
+        dmas = <&apbdma2 0>, <&apbdma3 0>;
+        dma-names = "tx", "rx";
+        #sound-dai-cells = <0>;
+    };
+...
-- 
2.43.5

Re: [PATCH v1 07/10] ASoC: dt-bindings: Add Loongson I2S controller

[Krzysztof Kozlowski]

On Thu, Sep 05, 2024 at 03:02:56PM +0800, Binbin Zhou wrote:
> Add Loongson I2S controller binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/sound/loongson,ls2k-i2s.yaml     | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
> new file mode 100644
> index 000000000000..a2e3bbe00dab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml

Filename matching compatible.

> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/loongson,ls2k-i2s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson-2K I2S controller
> +
> +maintainers:
> +  - Binbin Zhou <zhoubinbin@loongson.cn>
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: loongson,ls2k1000-i2s
> +
> +  reg:
> +    maxItems: 2

List and describe items instead.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  '#sound-dai-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - dmas
> +  - dma-names
> +  - '#sound-dai-cells'
> +
> +additionalProperties: false

Instead:
unevaluatedProperties: false

Best regards,
Krzysztof

Re: [PATCH v1 07/10] ASoC: dt-bindings: Add Loongson I2S controller

[Binbin Zhou]

Hi Krzysztof:

Thanks for your reply.

On Fri, Sep 6, 2024 at 4:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:56PM +0800, Binbin Zhou wrote:
> > Add Loongson I2S controller binding with DT schema format using
> > json-schema.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../bindings/sound/loongson,ls2k-i2s.yaml     | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
> > new file mode 100644
> > index 000000000000..a2e3bbe00dab
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/loongson,ls2k-i2s.yaml
>
> Filename matching compatible.

I will rename it as loongson,ls2k1000-i2s.yaml
>
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/loongson,ls2k-i2s.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson-2K I2S controller
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > +
> > +allOf:
> > +  - $ref: dai-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: loongson,ls2k1000-i2s
> > +
> > +  reg:
> > +    maxItems: 2
>
> List and describe items instead.

OK..
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  dmas:
> > +    maxItems: 2
> > +
> > +  dma-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +
> > +  '#sound-dai-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - dmas
> > +  - dma-names
> > +  - '#sound-dai-cells'
> > +
> > +additionalProperties: false
>
> Instead:
> unevaluatedProperties: false

OK...

Thanks.
Binbin
>
> Best regards,
> Krzysztof
>

[PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Binbin Zhou]

The Loongson I2S controller exists not only in PCI form (LS7A bridge
chip), but also in platform device form (Loongson-2K1000 SoC).

This patch adds support for platform device I2S controller.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 sound/soc/loongson/Kconfig             |  12 +-
 sound/soc/loongson/Makefile            |   3 +
 sound/soc/loongson/loongson_i2s_plat.c | 186 +++++++++++++++++++++++++
 3 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/loongson/loongson_i2s_plat.c

diff --git a/sound/soc/loongson/Kconfig b/sound/soc/loongson/Kconfig
index b8d7e2bade24..cd298f8b54cd 100644
--- a/sound/soc/loongson/Kconfig
+++ b/sound/soc/loongson/Kconfig
@@ -13,10 +13,20 @@ config SND_SOC_LOONGSON_I2S_PCI
 	  The controller is found in loongson bridge chips or SoCs,
 	  and work as a PCI device.
 
+config SND_SOC_LOONGSON_I2S_PLATFORM
+	tristate "Loongson I2S controller as platform device"
+	select SND_SOC_GENERIC_DMAENGINE_PCM
+	help
+	  Say Y or M if you want to add support for I2S driver for
+	  Loongson I2S controller.
+
+	  The controller work as a platform device, found in loongson
+	  SoCs.
+
 config SND_SOC_LOONGSON_CARD
 	tristate "Loongson Sound Card Driver"
 	select SND_SOC_LOONGSON_I2S_PCI
-	depends on PCI
+	select SND_SOC_LOONGSON_I2S_PLATFORM
 	help
 	  Say Y or M if you want to add support for SoC audio using
 	  loongson I2S controller.
diff --git a/sound/soc/loongson/Makefile b/sound/soc/loongson/Makefile
index 578030ad6563..f396259244a3 100644
--- a/sound/soc/loongson/Makefile
+++ b/sound/soc/loongson/Makefile
@@ -3,6 +3,9 @@
 snd-soc-loongson-i2s-pci-y := loongson_i2s_pci.o loongson_i2s.o loongson_dma.o
 obj-$(CONFIG_SND_SOC_LOONGSON_I2S_PCI) += snd-soc-loongson-i2s-pci.o
 
+snd-soc-loongson-i2s-plat-y := loongson_i2s_plat.o loongson_i2s.o
+obj-$(CONFIG_SND_SOC_LOONGSON_I2S_PLATFORM) += snd-soc-loongson-i2s-plat.o
+
 #Machine Support
 snd-soc-loongson-card-y := loongson_card.o
 obj-$(CONFIG_SND_SOC_LOONGSON_CARD) += snd-soc-loongson-card.o
diff --git a/sound/soc/loongson/loongson_i2s_plat.c b/sound/soc/loongson/loongson_i2s_plat.c
new file mode 100644
index 000000000000..668067753b1c
--- /dev/null
+++ b/sound/soc/loongson/loongson_i2s_plat.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson I2S controller master mode dirver(platform device)
+ *
+ * Copyright (C) 2023-2024 Loongson Technology Corporation Limited
+ *
+ * Author: Yingkun Meng <mengyingkun@loongson.cn>
+ *         Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of_dma.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "loongson_i2s.h"
+
+#define LOONGSON_I2S_RX_DMA_OFFSET	21
+#define LOONGSON_I2S_TX_DMA_OFFSET	18
+
+#define LOONGSON_DMA0_CONF	0x0
+#define LOONGSON_DMA1_CONF	0x1
+#define LOONGSON_DMA2_CONF	0x2
+#define LOONGSON_DMA3_CONF	0x3
+#define LOONGSON_DMA4_CONF	0x4
+
+/* periods_max = PAGE_SIZE / sizeof(struct ls_dma_chan_reg) */
+static const struct snd_pcm_hardware loongson_pcm_hardware = {
+	.info = SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_RESUME |
+		SNDRV_PCM_INFO_PAUSE,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		   SNDRV_PCM_FMTBIT_S20_3LE |
+		   SNDRV_PCM_FMTBIT_S24_LE,
+	.period_bytes_min = 128,
+	.period_bytes_max = 128 * 1024,
+	.periods_min = 1,
+	.periods_max = 64,
+	.buffer_bytes_max = 1024 * 1024,
+};
+
+static const struct snd_dmaengine_pcm_config loongson_dmaengine_pcm_config = {
+	.pcm_hardware = &loongson_pcm_hardware,
+	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+	.prealloc_buffer_size = 128 * 1024,
+};
+
+static int loongson_pcm_open(struct snd_soc_component *component,
+			     struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	if (substream->pcm->device & 1) {
+		runtime->hw.info &= ~SNDRV_PCM_INFO_INTERLEAVED;
+		runtime->hw.info |= SNDRV_PCM_INFO_NONINTERLEAVED;
+	}
+
+	if (substream->pcm->device & 2)
+		runtime->hw.info &= ~(SNDRV_PCM_INFO_MMAP |
+				      SNDRV_PCM_INFO_MMAP_VALID);
+	/*
+	 * For mysterious reasons (and despite what the manual says)
+	 * playback samples are lost if the DMA count is not a multiple
+	 * of the DMA burst size.  Let's add a rule to enforce that.
+	 */
+	snd_pcm_hw_constraint_step(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 128);
+	snd_pcm_hw_constraint_step(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 128);
+	snd_pcm_hw_constraint_integer(substream->runtime,
+				      SNDRV_PCM_HW_PARAM_PERIODS);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver loongson_i2s_component_driver = {
+	.name   = LS_I2S_DRVNAME,
+	.open	= loongson_pcm_open,
+};
+
+static const struct regmap_config loongson_i2s_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = 0x14,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static int loongson_i2s_apbdma_config(struct platform_device *pdev)
+{
+	int val;
+	void __iomem *regs;
+
+	regs = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	val = readl(regs);
+	val |= LOONGSON_DMA2_CONF << LOONGSON_I2S_TX_DMA_OFFSET;
+	val |= LOONGSON_DMA3_CONF << LOONGSON_I2S_RX_DMA_OFFSET;
+	writel(val, regs);
+
+	return 0;
+}
+
+static int loongson_i2s_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct loongson_i2s *i2s;
+	struct resource *res;
+	struct clk *i2s_clk;
+	int ret;
+
+	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
+	if (!i2s)
+		return -ENOMEM;
+
+	ret = loongson_i2s_apbdma_config(pdev);
+	if (ret)
+		return ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2s->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2s->reg_base))
+		return dev_err_probe(dev, PTR_ERR(i2s->reg_base),
+				     "devm_ioremap_resource failed\n");
+
+	i2s->regmap = devm_regmap_init_mmio(dev, i2s->reg_base,
+					    &loongson_i2s_regmap_config);
+	if (IS_ERR(i2s->regmap))
+		return dev_err_probe(dev, PTR_ERR(i2s->regmap),
+				     "devm_regmap_init_mmio failed\n");
+
+	i2s->playback_dma_data.addr = res->start + LS_I2S_TX_DATA;
+	i2s->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	i2s->playback_dma_data.maxburst = 4;
+
+	i2s->capture_dma_data.addr = res->start + LS_I2S_RX_DATA;
+	i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	i2s->capture_dma_data.maxburst = 4;
+
+	i2s_clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(i2s_clk))
+		return dev_err_probe(dev, PTR_ERR(i2s_clk), "clock property invalid\n");
+	i2s->clk_rate = clk_get_rate(i2s_clk);
+
+	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	dev_set_name(dev, LS_I2S_DRVNAME);
+	dev_set_drvdata(dev, i2s);
+
+	ret = devm_snd_soc_register_component(dev, &loongson_i2s_component_driver,
+					      &loongson_i2s_dai, 1);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register DAI\n");
+
+	return devm_snd_dmaengine_pcm_register(dev, &loongson_dmaengine_pcm_config,
+					       SND_DMAENGINE_PCM_FLAG_COMPAT);
+}
+
+static const struct of_device_id loongson_i2s_ids[] = {
+	{ .compatible = "loongson,ls2k1000-i2s" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, loongson_i2s_ids);
+
+static struct platform_driver loongson_i2s_driver = {
+	.probe = loongson_i2s_plat_probe,
+	.driver = {
+		.name = "loongson-i2s-plat",
+		.pm = pm_sleep_ptr(&loongson_i2s_pm),
+		.of_match_table = loongson_i2s_ids,
+	},
+};
+module_platform_driver(loongson_i2s_driver);
+
+MODULE_DESCRIPTION("Loongson I2S Master Mode ASoC Driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");
-- 
2.43.5

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On Thu, Sep 05, 2024 at 03:07:21PM +0800, Binbin Zhou wrote:
> The Loongson I2S controller exists not only in PCI form (LS7A bridge
> chip), but also in platform device form (Loongson-2K1000 SoC).
> 
> This patch adds support for platform device I2S controller.

Can some of this be shared with the PCI version - is it the same IP in a
different wrapper, or is it a new IP?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Binbin Zhou]

On Thu, Sep 5, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 03:07:21PM +0800, Binbin Zhou wrote:
> > The Loongson I2S controller exists not only in PCI form (LS7A bridge
> > chip), but also in platform device form (Loongson-2K1000 SoC).
> >
> > This patch adds support for platform device I2S controller.
>
> Can some of this be shared with the PCI version - is it the same IP in a
> different wrapper, or is it a new IP?

Hi Mark:

Thanks for your reply.

To be exact, they are similar, such as the definition of the
controller registers.
But for example, DMA data processing is different. In the pci version
of i2s, the DMA controller is built-in, while the DMA controller here
is external, using ls2x-apbdma (drivers/dma/ls2x-apb-dma.c)

Thanks.
Binbin

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Sat, Sep 07, 2024 at 02:08:08PM +0600, Binbin Zhou wrote:
> On Thu, Sep 5, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:

> > Can some of this be shared with the PCI version - is it the same IP in a
> > different wrapper, or is it a new IP?

> To be exact, they are similar, such as the definition of the
> controller registers.
> But for example, DMA data processing is different. In the pci version
> of i2s, the DMA controller is built-in, while the DMA controller here
> is external, using ls2x-apbdma (drivers/dma/ls2x-apb-dma.c)

OK, if all the registers are the same it might be worth trying to share
that code but possibly not with the DMA being split like this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[kernel test robot]

Hi Binbin,

kernel test robot noticed the following build errors:

[auto build test ERROR on 097a44db5747403b19d05a9664e8ec6adba27e3b]

url:    https://github.com/intel-lab-lkp/linux/commits/Binbin-Zhou/ASoC-dt-bindings-Add-Everest-ES8323-Codec/20240905-150958
base:   097a44db5747403b19d05a9664e8ec6adba27e3b
patch link:    https://lore.kernel.org/r/282dadefdaac7917fd681a6e84a5f0f07d0557bc.1725518229.git.zhoubinbin%40loongson.cn
patch subject: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240906/202409060840.Rm0gPgE4-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240906/202409060840.Rm0gPgE4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409060840.Rm0gPgE4-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> sound/soc/loongson/loongson_i2s_pci.c:157:1: warning: data definition has no type or storage class
     157 | module_pci_driver(loongson_i2s_driver);
         | ^~~~~~~~~~~~~~~~~
>> sound/soc/loongson/loongson_i2s_pci.c:157:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Wimplicit-int]
>> sound/soc/loongson/loongson_i2s_pci.c:157:1: error: parameter names (without types) in function declaration [-Wdeclaration-missing-parameter-type]
>> sound/soc/loongson/loongson_i2s_pci.c:149:26: warning: 'loongson_i2s_driver' defined but not used [-Wunused-variable]
     149 | static struct pci_driver loongson_i2s_driver = {
         |                          ^~~~~~~~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_SOC_LOONGSON_I2S_PCI
   Depends on [n]: SOUND [=m] && SND [=m] && SND_SOC [=m] && (LOONGARCH || COMPILE_TEST [=y]) && PCI [=n]
   Selected by [m]:
   - SND_SOC_LOONGSON_CARD [=m] && SOUND [=m] && SND [=m] && SND_SOC [=m] && (LOONGARCH || COMPILE_TEST [=y])


vim +/loongson_i2s_driver +149 sound/soc/loongson/loongson_i2s_pci.c

d84881e06836dc1 Yingkun Meng 2023-06-15  148  
d84881e06836dc1 Yingkun Meng 2023-06-15 @149  static struct pci_driver loongson_i2s_driver = {
d84881e06836dc1 Yingkun Meng 2023-06-15  150  	.name = "loongson-i2s-pci",
d84881e06836dc1 Yingkun Meng 2023-06-15  151  	.id_table = loongson_i2s_ids,
d84881e06836dc1 Yingkun Meng 2023-06-15  152  	.probe = loongson_i2s_pci_probe,
d84881e06836dc1 Yingkun Meng 2023-06-15  153  	.driver = {
d84881e06836dc1 Yingkun Meng 2023-06-15  154  		.pm = pm_sleep_ptr(&loongson_i2s_pm),
d84881e06836dc1 Yingkun Meng 2023-06-15  155  	},
d84881e06836dc1 Yingkun Meng 2023-06-15  156  };
d84881e06836dc1 Yingkun Meng 2023-06-15 @157  module_pci_driver(loongson_i2s_driver);
d84881e06836dc1 Yingkun Meng 2023-06-15  158  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[kernel test robot]

Hi Binbin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 097a44db5747403b19d05a9664e8ec6adba27e3b]

url:    https://github.com/intel-lab-lkp/linux/commits/Binbin-Zhou/ASoC-dt-bindings-Add-Everest-ES8323-Codec/20240905-150958
base:   097a44db5747403b19d05a9664e8ec6adba27e3b
patch link:    https://lore.kernel.org/r/282dadefdaac7917fd681a6e84a5f0f07d0557bc.1725518229.git.zhoubinbin%40loongson.cn
patch subject: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device
config: i386-kismet-CONFIG_SND_SOC_LOONGSON_I2S_PCI-CONFIG_SND_SOC_LOONGSON_CARD-0-0 (https://download.01.org/0day-ci/archive/20240906/202409061419.RBYUF8ou-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20240906/202409061419.RBYUF8ou-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409061419.RBYUF8ou-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for SND_SOC_LOONGSON_I2S_PCI when selected by SND_SOC_LOONGSON_CARD
   WARNING: unmet direct dependencies detected for SND_SOC_LOONGSON_I2S_PCI
     Depends on [n]: SOUND [=y] && SND [=y] && SND_SOC [=y] && (LOONGARCH || COMPILE_TEST [=y]) && PCI [=n]
     Selected by [y]:
     - SND_SOC_LOONGSON_CARD [=y] && SOUND [=y] && SND [=y] && SND_SOC [=y] && (LOONGARCH || COMPILE_TEST [=y])

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Geert Uytterhoeven]

Hi Binbin,

On Thu, Sep 5, 2024 at 9:07 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
> The Loongson I2S controller exists not only in PCI form (LS7A bridge
> chip), but also in platform device form (Loongson-2K1000 SoC).
>
> This patch adds support for platform device I2S controller.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

Thanks for your patch!

> --- a/sound/soc/loongson/Kconfig
> +++ b/sound/soc/loongson/Kconfig
> @@ -13,10 +13,20 @@ config SND_SOC_LOONGSON_I2S_PCI
>           The controller is found in loongson bridge chips or SoCs,
>           and work as a PCI device.
>
> +config SND_SOC_LOONGSON_I2S_PLATFORM
> +       tristate "Loongson I2S controller as platform device"

depends on LOONGARCH || COMPILE_TEST

> +       select SND_SOC_GENERIC_DMAENGINE_PCM
> +       help
> +         Say Y or M if you want to add support for I2S driver for
> +         Loongson I2S controller.
> +
> +         The controller work as a platform device, found in loongson
> +         SoCs.
> +
>  config SND_SOC_LOONGSON_CARD
>         tristate "Loongson Sound Card Driver"
>         select SND_SOC_LOONGSON_I2S_PCI
> -       depends on PCI

"select SND_SOC_LOONGSON_I2S_PCI if PCI"?

> +       select SND_SOC_LOONGSON_I2S_PLATFORM

Or perhaps do it the other way around, i,e. let
SND_SOC_LOONGSON_I2S_{PCI,PLATFORM} select SND_SOC_LOONGSON_CARD?
That would be similar to SPI_LOONGSON_{PCI,PLATFORM}, which select
SPI_LOONGSON_CORE.

>         help
>           Say Y or M if you want to add support for SoC audio using
>           loongson I2S controller.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 08/10] ASoC: loongson: Add I2S controller driver as platform device

[Binbin Zhou]

Hi Geert:

Thanks for your reply.

On Fri, Sep 6, 2024 at 5:37 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Binbin,
>
> On Thu, Sep 5, 2024 at 9:07 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
> > The Loongson I2S controller exists not only in PCI form (LS7A bridge
> > chip), but also in platform device form (Loongson-2K1000 SoC).
> >
> > This patch adds support for platform device I2S controller.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>
> Thanks for your patch!
>
> > --- a/sound/soc/loongson/Kconfig
> > +++ b/sound/soc/loongson/Kconfig
> > @@ -13,10 +13,20 @@ config SND_SOC_LOONGSON_I2S_PCI
> >           The controller is found in loongson bridge chips or SoCs,
> >           and work as a PCI device.
> >
> > +config SND_SOC_LOONGSON_I2S_PLATFORM
> > +       tristate "Loongson I2S controller as platform device"
>
> depends on LOONGARCH || COMPILE_TEST

This is will put under SND_SOC_LOONGSON_CARD.
>
> > +       select SND_SOC_GENERIC_DMAENGINE_PCM
> > +       help
> > +         Say Y or M if you want to add support for I2S driver for
> > +         Loongson I2S controller.
> > +
> > +         The controller work as a platform device, found in loongson
> > +         SoCs.
> > +
> >  config SND_SOC_LOONGSON_CARD
> >         tristate "Loongson Sound Card Driver"
> >         select SND_SOC_LOONGSON_I2S_PCI
> > -       depends on PCI
>
> "select SND_SOC_LOONGSON_I2S_PCI if PCI"?
>
> > +       select SND_SOC_LOONGSON_I2S_PLATFORM
>
> Or perhaps do it the other way around, i,e. let
> SND_SOC_LOONGSON_I2S_{PCI,PLATFORM} select SND_SOC_LOONGSON_CARD?
> That would be similar to SPI_LOONGSON_{PCI,PLATFORM}, which select
> SPI_LOONGSON_CORE.

Yes, it would be clearer. I'll modify it in the next version.

Thanks.
Binbin
>
> >         help
> >           Say Y or M if you want to add support for SoC audio using
> >           loongson I2S controller.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[PATCH v1 09/10] LoongArch: dts: Add I2S support to Loongson-2K1000

[Binbin Zhou]

The module is supported, enable it.

Not all Loongson-2K1000 development boards have an i2s interface, here
is an example of adding one:

sound {
	compatible = "loongson,ls-audio-card";
	model = "Loongson-ASoC";
	mclk-fs = <512>;

	cpu {
		sound-dai = <&i2s>;
	};

	codec {
		sound-dai = <&uda1342>;
	};
};

&apbdma2 {
	status = "okay";
};

&apbdma3 {
	status = "okay";
};

&i2c3 {
	status = "okay";

	pinctrl-0 = <&i2c1_pins_default>;
	pinctrl-names = "default";

	#address-cells = <1>;
	#size-cells = <0>;

	uda1342: codec@1a {
		compatible = "nxp,uda1342";
		reg = <0x1a>;
		#sound-dai-cells = <0>;
	};
};

&i2s {
	status = "okay";

	pinctrl-0 = <&hda_pins_default>;
	pinctrl-names = "default";
};

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 arch/loongarch/boot/dts/loongson-2k1000.dtsi | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/boot/dts/loongson-2k1000.dtsi b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
index 92180140eb56..8dff2aa52417 100644
--- a/arch/loongarch/boot/dts/loongson-2k1000.dtsi
+++ b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
@@ -266,7 +266,7 @@ dma-controller@1fe00c10 {
 			status = "disabled";
 		};
 
-		dma-controller@1fe00c20 {
+		apbdma2: dma-controller@1fe00c20 {
 			compatible = "loongson,ls2k1000-apbdma";
 			reg = <0x0 0x1fe00c20 0x0 0x8>;
 			interrupt-parent = <&liointc1>;
@@ -276,7 +276,7 @@ dma-controller@1fe00c20 {
 			status = "disabled";
 		};
 
-		dma-controller@1fe00c30 {
+		apbdma3: dma-controller@1fe00c30 {
 			compatible = "loongson,ls2k1000-apbdma";
 			reg = <0x0 0x1fe00c30 0x0 0x8>;
 			interrupt-parent = <&liointc1>;
@@ -352,6 +352,19 @@ rtc0: rtc@1fe27800 {
 			status = "disabled";
 		};
 
+		i2s: i2s@1fe2d000 {
+			compatible = "loongson,ls2k1000-i2s";
+			reg = <0 0x1fe2d000 0 0x14>,
+			      <0 0x1fe00438 0 0x8>;
+			interrupt-parent = <&liointc0>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk LOONGSON2_APB_CLK>;
+			dmas = <&apbdma2 0>, <&apbdma3 0>;
+			dma-names = "tx", "rx";
+			#sound-dai-cells = <0>;
+			status = "disabled";
+		};
+
 		spi0: spi@1fff0220 {
 			compatible = "loongson,ls2k1000-spi";
 			reg = <0x0 0x1fff0220 0x0 0x10>;
-- 
2.43.5

[PATCH v1 10/10] LoongArch: dts: Add I2S support to Loongson-2K2000

[Binbin Zhou]

The module is supported, enable it.

Not all Loongson-2K2000 development boards have an i2s interface, here
is an example of adding one:

sound {
	compatible = "loongson,ls-audio-card";
	model = "Loongson-ASoC";
	mclk-fs = <512>;

	cpu {
		sound-dai = <&i2s>;
	};

	codec {
		sound-dai = <&es8323>;
	};
};

&i2c1 {
	status = "okay";
	#address-cells = <1>;
	#size-cells = <0>;

	es8323:es8323@10 {
		compatible = "everest,es8323";
		reg = <0x10>;
		#sound-dai-cells = <0>;
	};
};

&i2s {
	status = "okay";

	clock-frequency = <175000000>;
	#sound-dai-cells = <0>;
 };

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 arch/loongarch/boot/dts/loongson-2k2000.dtsi | 22 ++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/boot/dts/loongson-2k2000.dtsi b/arch/loongarch/boot/dts/loongson-2k2000.dtsi
index 0953c5707825..b4ff55a33e90 100644
--- a/arch/loongarch/boot/dts/loongson-2k2000.dtsi
+++ b/arch/loongarch/boot/dts/loongson-2k2000.dtsi
@@ -173,6 +173,22 @@ rtc0: rtc@100d0100 {
 			status = "disabled";
 		};
 
+		i2c@1fe00120 {
+			compatible = "loongson,ls2k-i2c";
+			reg = <0x0 0x1fe00120 0x0 0x8>;
+			interrupt-parent = <&liointc>;
+			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		i2c@1fe00130 {
+			compatible = "loongson,ls2k-i2c";
+			reg = <0x0 0x1fe00130 0x0 0x8>;
+			interrupt-parent = <&liointc>;
+			interrupts = <9 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
 		uart0: serial@1fe001e0 {
 			compatible = "ns16550a";
 			reg = <0x0 0x1fe001e0 0x0 0x10>;
@@ -243,9 +259,11 @@ display@6,1 {
 				status = "disabled";
 			};
 
-			hda@7,0 {
+			i2s@7,0 {
 				reg = <0x3800 0x0 0x0 0x0 0x0>;
-				interrupts = <58 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <78 IRQ_TYPE_LEVEL_HIGH>,
+					     <79 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "tx", "rx";
 				interrupt-parent = <&pic>;
 				status = "disabled";
 			};
-- 
2.43.5

Re: [PATCH v1 00/10] ASoC: Some issues about loongson i2s

[Krzysztof Kozlowski]

On 05/09/2024 09:02, Binbin Zhou wrote:
> Hi all:
> 
> This patch set is mainly about Loongson i2s related issues.
> 
> Please allow me to briefly explain this patch set:
> Patch 1-2: Add ES8323 codec required on Loongson-2K2000
> Patch 3-4: Add uda1342 codec required on Loongson-2K1000
> Patch 5: Improve code readability
> Patch 6: Fix the problem of unable to detect codec under FDT system.
> Patch 7-8: Add Loongson i2s platform device support
> Patch 9-10: Related DTS support.

This was based on some old tree... or you do not use get_maintainers.pl.

Best regards,
Krzysztof

Re: [PATCH v1 00/10] ASoC: Some issues about loongson i2s

[Binbin Zhou]

On Thu, Sep 5, 2024 at 2:14 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/09/2024 09:02, Binbin Zhou wrote:
> > Hi all:
> >
> > This patch set is mainly about Loongson i2s related issues.
> >
> > Please allow me to briefly explain this patch set:
> > Patch 1-2: Add ES8323 codec required on Loongson-2K2000
> > Patch 3-4: Add uda1342 codec required on Loongson-2K1000
> > Patch 5: Improve code readability
> > Patch 6: Fix the problem of unable to detect codec under FDT system.
> > Patch 7-8: Add Loongson i2s platform device support
> > Patch 9-10: Related DTS support.
>
> This was based on some old tree... or you do not use get_maintainers.pl.

Hi Krzysztof:

I used the following method to obtain the email address I need to copy:

scripts/get_maintainer.pl sound/soc/
Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
DYNAMIC AUDIO POWER MANAGEM...)
Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
linux-sound@vger.kernel.org (open list:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
linux-kernel@vger.kernel.org (open list)

The code repository from MAINTAINERS:

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next
base commit: 097a44db5747403b19d05a9664e8ec6adba27e3b

Is there anything I'm missing, or where I'm going wrong?

Thanks.
Binbin
>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 00/10] ASoC: Some issues about loongson i2s

[Krzysztof Kozlowski]

On 05/09/2024 10:34, Binbin Zhou wrote:
> On Thu, Sep 5, 2024 at 2:14 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/09/2024 09:02, Binbin Zhou wrote:
>>> Hi all:
>>>
>>> This patch set is mainly about Loongson i2s related issues.
>>>
>>> Please allow me to briefly explain this patch set:
>>> Patch 1-2: Add ES8323 codec required on Loongson-2K2000
>>> Patch 3-4: Add uda1342 codec required on Loongson-2K1000
>>> Patch 5: Improve code readability
>>> Patch 6: Fix the problem of unable to detect codec under FDT system.
>>> Patch 7-8: Add Loongson i2s platform device support
>>> Patch 9-10: Related DTS support.
>>
>> This was based on some old tree... or you do not use get_maintainers.pl.
> 
> Hi Krzysztof:
> 
> I used the following method to obtain the email address I need to copy:
> 
> scripts/get_maintainer.pl sound/soc/

That's not how you run get_maintainer.pl in typical process. You run it
on the patches.

> Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
> DYNAMIC AUDIO POWER MANAGEM...)
> Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
> Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
> linux-sound@vger.kernel.org (open list:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> linux-kernel@vger.kernel.org (open list)
> 
> The code repository from MAINTAINERS:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next
> base commit: 097a44db5747403b19d05a9664e8ec6adba27e3b
> 
> Is there anything I'm missing, or where I'm going wrong?


Yeah, the list is incomplete and wrong emails are used.

Best regards,
Krzysztof