ASoC: max98363: add soundwire amplifier driver

From: Ryan Lee <[email protected]>

This patch adds dt-bindings information for Analog Devices MAX98363

SoundWire Amplifier.

Signed-off-by Ryan Lee <[email protected]>

---
 .../bindings/sound/adi,max98363.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/adi,max98363.yaml

Documentation/devicetree/bindings/sound/adi,max98363.yaml new file +23 -0

new file mode 100644
index 000000000000..fda571d04a64
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/adi,max98363.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX98363 SoundWire Amplifier
+
+maintainers:
+  - Ryan Lee <[email protected]>
+
+description:
+  The MAX98363 is a SoundWire input Class D mono amplifier that
+  supports MIPI SoundWire v1.2-compatible digital interface for
+  audio and control data.
+
+properties:
+  compatible:
+    enum:
+      - adi,max98363
+
+  reg:
+    maxItems: 1

-----Original Message-----
From: Rob Herring <[email protected]>
Sent: Thursday, February 23, 2023 8:30 PM
To: “Ryan <[email protected]>
Cc: Lee, RyanS <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/2] ASoC: dt-bindings: max98363: add soundwire amplifier driver

[External]

Quoted 5 lines · “Ryan

From: Ryan Lee <[email protected]>

This patch adds dt-bindings information for Analog Devices MAX98363 SoundWire Amplifier.

Quoted 7 lines

Signed-off-by Ryan Lee <[email protected]>

---
 .../bindings/sound/adi,max98363.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/sound/adi,max98363.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error:
Documentation/devicetree/bindings/sound/adi,max98363.example.dts:29.3 -30.1 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:434:
Documentation/devicetree/bindings/sound/adi,max98363.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See
https://urldefense.com/v3/__https://patchwork.ozlabs.org/project/devicet
ree-bindings/patch/20230224010814.504016-2- [email protected]__;!!A3Ni8CS0y2Y!7mTDqdmHdbwbp5Dxm5RJ60i TMlM6mf6W3Z9PQwXMB92aBcnAkwZODw5olBaFKtmbimORUpXmQ2j2$

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

From: Ryan Lee <[email protected]>

Added Analog Devices MAX98363 SoundWire Amplifier Driver. The MAX98363 is a SoundWire peripheral device that supports MIPI SoundWire v1.2-compatible digital interface for audio and control data.

Signed-off-by Ryan Lee <[email protected]>

---
 sound/soc/codecs/Kconfig    |  11 +
 sound/soc/codecs/Makefile   |   2 +
 sound/soc/codecs/max98363.c | 699
++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/max98363.h | 108 ++++++

 4 files changed, 820 insertions(+)

 create mode 100644 sound/soc/codecs/max98363.c
 create mode 100644 sound/soc/codecs/max98363.h

sound/soc/codecs/Kconfig +0 -0

index 4621674e68bf..0638b27c6494 100644
@@ -130,6 +130,7 @@ config SND_SOC_ALL_CODECS

        imply SND_SOC_MAX98925
        imply SND_SOC_MAX98926
        imply SND_SOC_MAX98927
+       imply SND_SOC_MAX98363
        imply SND_SOC_MAX98373_I2C
        imply SND_SOC_MAX98373_SDW
        imply SND_SOC_MAX98390

sound/soc/codecs/Kconfig +0 -0

@@ -1094,6 +1095,16 @@ config SND_SOC_MAX98520

          To compile this driver as a module, choose M here.
+config SND_SOC_MAX98363
+       tristate "Analog Devices MAX98363 Soundwire Speaker Amplifier"
+       depends on SOUNDWIRE
+       select REGMAP_SOUNDWIRE
+       help
+         Enable support for Analog Devices MAX98363 Soundwire +         amplifier. MAX98363 supports the MIPI SoundWire v1.2 +         compatible interface for audio and control data. +         This amplifier does not support I2C and I2S. +
 config SND_SOC_MAX98373
        tristate

sound/soc/codecs/Makefile +0 -0

index a0a61554548e..f4fbeceed816 100644
@@ -144,6 +144,7 @@ snd-soc-max98925-objs := max98925.o

 snd-soc-max98926-objs := max98926.o
 snd-soc-max98927-objs := max98927.o
 snd-soc-max98520-objs := max98520.o
+snd-soc-max98363-objs := max98363.o
 snd-soc-max98373-objs := max98373.o
 snd-soc-max98373-i2c-objs := max98373-i2c.o
 snd-soc-max98373-sdw-objs := max98373-sdw.o

sound/soc/codecs/Makefile +0 -0

@@ -507,6 +508,7 @@ obj-$(CONFIG_SND_SOC_MAX98925)      += snd-soc-

max98925.o
 obj-$(CONFIG_SND_SOC_MAX98926) += snd-soc-max98926.o obj-$(CONFIG_SND_SOC_MAX98927) += snd-soc-max98927.o obj-$(CONFIG_SND_SOC_MAX98520) += snd-soc-max98520.o +obj-$(CONFIG_SND_SOC_MAX98363) += snd-soc-max98363.o obj-$(CONFIG_SND_SOC_MAX98373) += snd-soc-max98373.o obj-$(CONFIG_SND_SOC_MAX98373_I2C)   += snd-soc-max98373-i2c.o obj-$(CONFIG_SND_SOC_MAX98373_SDW)   += snd-soc-max98373-sdw.o

sound/soc/codecs/max98363.c new file +77 -0

new file mode 100644
index 000000000000..e766c733f8e4
@@ -0,0 +1,699 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2022, Analog Devices Inc.
+
+#include <linux/acpi.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <linux/of.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <linux/regulator/consumer.h>
+#include "max98363.h"
+
+struct sdw_stream_data {
+       struct sdw_stream_runtime *sdw_stream;
+};
+
+static struct reg_default max98363_reg[] = {
+       {MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+       {MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+       {MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
+       {MAX98363_R0044_SCP_CTRL, 0x00},
+       {MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
+       {MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
+       {MAX98363_R004D_SCP_BUS_CLK, 0x00},
+       {MAX98363_R0050_SCP_DEV_ID_0, 0x21},
+       {MAX98363_R0051_SCP_DEV_ID_1, 0x01},
+       {MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
+       {MAX98363_R0053_SCP_DEV_ID_3, 0x87},
+       {MAX98363_R0054_SCP_DEV_ID_4, 0x08},
+       {MAX98363_R0055_SCP_DEV_ID_5, 0x00},
+       {MAX98363_R0060_SCP_FRAME_CTRL, 0x00},
+       {MAX98363_R0062_SCP_CLK_SCALE_BANK0, 0x00},
+       {MAX98363_R0070_SCP_FRAME_CTRL, 0x00},
+       {MAX98363_R0072_SCP_CLK_SCALE_BANK1, 0x00},
+       {MAX98363_R0080_SCP_PHYOUTCTRL_0, 0x00},
+       {MAX98363_R0100_DP1_INIT_STAT, 0x00},
+       {MAX98363_R0101_DP1_INIT_MASK, 0x00},
+       {MAX98363_R0102_DP1_PORT_CTRL, 0x00},
+       {MAX98363_R0103_DP1_BLOCK_CTRL_1, 0x00},
+       {MAX98363_R0104_DP1_PREPARE_STATUS, 0x00},
+       {MAX98363_R0105_DP1_PREPARE_CTRL, 0x00},
+       {MAX98363_R0120_DP1_CHANNEL_EN, 0x00},
+       {MAX98363_R0122_DP1_SAMPLE_CTRL1, 0x00},
+       {MAX98363_R0123_DP1_SAMPLE_CTRL2, 0x00},
+       {MAX98363_R0124_DP1_OFFSET_CTRL1, 0x00},
+       {MAX98363_R0125_DP1_OFFSET_CTRL2, 0x00},
+       {MAX98363_R0126_DP1_HCTRL, 0x00},
+       {MAX98363_R0127_DP1_BLOCK_CTRL3, 0x00},
+       {MAX98363_R0130_DP1_CHANNEL_EN, 0x00},
+       {MAX98363_R0132_DP1_SAMPLE_CTRL1, 0x00},
+       {MAX98363_R0133_DP1_SAMPLE_CTRL2, 0x00},
+       {MAX98363_R0134_DP1_OFFSET_CTRL1, 0x00},
+       {MAX98363_R0135_DP1_OFFSET_CTRL2, 0x00},
+       {MAX98363_R0136_DP1_HCTRL, 0x0136},
+       {MAX98363_R0137_DP1_BLOCK_CTRL3, 0x00},
+       {MAX98363_R2001_INTR_RAW, 0x0},
+       {MAX98363_R2003_INTR_STATE, 0x0},
+       {MAX98363_R2005_INTR_FALG, 0x0},
+       {MAX98363_R2007_INTR_EN, 0x0},
+       {MAX98363_R2009_INTR_CLR, 0x0},
+       {MAX98363_R2021_ERR_MON_CTRL, 0x0},
+       {MAX98363_R2022_SPK_MON_THRESH, 0x0},
+       {MAX98363_R2023_SPK_MON_DURATION, 0x0},
+       {MAX98363_R2030_TONE_GEN_CFG, 0x0},
+       {MAX98363_R203F_TONE_GEN_EN, 0x0},
+       {MAX98363_R2040_AMP_VOL, 0x0},
+       {MAX98363_R2041_AMP_GAIN, 0x5},
+       {MAX98363_R2042_DSP_CFG, 0x0},
+       {MAX98363_R21FF_REV_ID, 0x0},
+};
+
+static bool max98363_readable_register(struct device *dev, unsigned

int reg)
+{
+       switch (reg) {
+       /* SoundWire Control Port Registers */
+       case MAX98363_R0040_SCP_INIT_STAT_1 ...
MAX98363_R0046_SCP_DEV_NUMBER:
+       case MAX98363_R004D_SCP_BUS_CLK:
+       case MAX98363_R0050_SCP_DEV_ID_0 ...
MAX98363_R0055_SCP_DEV_ID_5:
+       case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+       case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+       case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+       /* Soundwire Data Port 1 Registers */
+       case MAX98363_R0100_DP1_INIT_STAT ...
MAX98363_R0105_DP1_PREPARE_CTRL:
+       case MAX98363_R0120_DP1_CHANNEL_EN ...
MAX98363_R0127_DP1_BLOCK_CTRL3:
+       case MAX98363_R0130_DP1_CHANNEL_EN:
+       case MAX98363_R0132_DP1_SAMPLE_CTRL1...
MAX98363_R0137_DP1_BLOCK_CTRL3:
+       /* MAX98363 Amp Control Registers */
+       case MAX98363_R2001_INTR_RAW:
+       case MAX98363_R2003_INTR_STATE:
+       case MAX98363_R2005_INTR_FALG:
+       case MAX98363_R2007_INTR_EN:
+       case MAX98363_R2009_INTR_CLR:
+       case MAX98363_R2021_ERR_MON_CTRL ...
MAX98363_R2023_SPK_MON_DURATION:
+       case MAX98363_R2030_TONE_GEN_CFG:
+       case MAX98363_R203F_TONE_GEN_EN:
+       case MAX98363_R2040_AMP_VOL:
+       case MAX98363_R2041_AMP_GAIN:
+       case MAX98363_R2042_DSP_CFG:
+       case MAX98363_R21FF_REV_ID:
+               return true;
+       default:
+               return false;
+       }

sound/soc/codecs/max98363.c new file +3 -0

+};
+
+static bool max98363_volatile_reg(struct device *dev, unsigned int

reg)
+{
+       switch (reg) {
+       /* SoundWire Control Port Registers */
+       case MAX98363_R0040_SCP_INIT_STAT_1 ...
MAX98363_R0046_SCP_DEV_NUMBER:
+       case MAX98363_R004D_SCP_BUS_CLK:
+       case MAX98363_R0050_SCP_DEV_ID_0 ...
MAX98363_R0055_SCP_DEV_ID_5:
+       case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+       case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+       case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+       /* Soundwire Data Port 1 Registers */
+       case MAX98363_R0100_DP1_INIT_STAT ...
MAX98363_R0105_DP1_PREPARE_CTRL:
+       case MAX98363_R0120_DP1_CHANNEL_EN ...
MAX98363_R0127_DP1_BLOCK_CTRL3:
+       case MAX98363_R0130_DP1_CHANNEL_EN:
+       case MAX98363_R0132_DP1_SAMPLE_CTRL1...
MAX98363_R0137_DP1_BLOCK_CTRL3:
+       /* MAX98363 Amp Control Registers */
+       case MAX98363_R2001_INTR_RAW:
+       case MAX98363_R2003_INTR_STATE:
+       case MAX98363_R2005_INTR_FALG:
+       case MAX98363_R2007_INTR_EN:
+       case MAX98363_R2009_INTR_CLR:
+       case MAX98363_R21FF_REV_ID:
+               return true;
+       default:
+               return false;
+       }

sound/soc/codecs/max98363.c new file +6 -0

+}
+
+static const struct regmap_config max98363_sdw_regmap = {
+       .reg_bits = 32,
+       .val_bits = 8,
+       .max_register = MAX98363_R21FF_REV_ID,

sound/soc/codecs/max98363.c new file +8 -0

+{
+       struct max98363_priv *max98363 = dev_get_drvdata(dev);
+
+       regcache_cache_only(max98363->regmap, true);
+       regcache_mark_dirty(max98363->regmap);
+
+       if (max98363->dvddio)
+               regulator_disable(max98363->dvddio);

sound/soc/codecs/max98363.c new file +13 -0

+{
+       struct sdw_slave *slave = dev_to_sdw_dev(dev);
+       struct max98363_priv *max98363 = dev_get_drvdata(dev);
+       unsigned long time;
+       int ret;
+
+       if (!max98363->first_hw_init)
+               return 0;
+
+       if (!slave->unattach_request)
+               goto regmap_sync;
+
+       time = wait_for_completion_timeout(&slave-

Quoted 1 line

initialization_complete,

+
msecs_to_jiffies(MAX98363_PROBE_TIMEOUT));
+       if (!time) {
+               dev_err(dev, "Initialization not complete, timed out\n");
+               return -ETIMEDOUT;
+       }

sound/soc/codecs/max98363.c new file +17 -0

+
+regmap_sync:
+
+       if (max98363->dvddio) {
+               ret = regulator_enable(max98363->dvddio);
+               if (ret < 0)
+                       return ret;
+       }
+
+       if (max98363->vdd) {
+               ret = regulator_enable(max98363->vdd);
+               if (ret < 0)
+                       return ret;
+       }
+
+       slave->unattach_request = 0;
+       regcache_cache_only(max98363->regmap, false);

sound/soc/codecs/max98363.c new file +8 -0

+static int max98363_read_prop(struct sdw_slave *slave)
+{
+       struct sdw_slave_prop *prop = &slave->prop;
+       int nval, i;
+       u32 bit;
+       unsigned long addr;
+       struct sdw_dpn_prop *dpn;
+

+static int max98363_init(struct sdw_slave *slave, struct regmap *regmap)

sound/soc/codecs/max98363.c new file +8 -0

+{
+       struct max98363_priv *max98363;
+       int ret;
+       struct device *dev = &slave->dev;
+
+       /*  Allocate and assign private driver data structure  */
+       max98363 = devm_kzalloc(dev, sizeof(*max98363), GFP_KERNEL);
+       if (!max98363)

+       if (IS_ERR(max98363->vdd)) {
+               if (PTR_ERR(max98363->vdd) == -EPROBE_DEFER) +                       return -EPROBE_DEFER;
+
+               max98363->vdd = NULL;
+       }
+       max98363->dvddio = devm_regulator_get_optional(dev, "dvddio");

+       if (IS_ERR(max98363->dvddio)) {
+               if (PTR_ERR(max98363->dvddio) == -EPROBE_DEFER) +                       return -EPROBE_DEFER;

sound/soc/codecs/max98363.c new file +11 -0

+
+               max98363->dvddio = NULL;
+       }
+
+       if (max98363->vdd) {
+               ret = regulator_enable(max98363->vdd);
+               if (ret < 0)
+                       return ret;
+
+               ret = devm_add_action_or_reset(dev,
+                                             

max98363_supply_disable,
+                                              max98363->vdd); +               if (ret < 0)
+                       return ret;
+       }

sound/soc/codecs/max98363.c new file +8 -0

+
+       if (max98363->dvddio) {
+               ret = regulator_enable(max98363->dvddio);
+               if (ret < 0)
+                       return ret;
+
+               ret = devm_add_action_or_reset(dev,
+                                             

max98363_supply_disable,
+                                              max98363->dvddio); +               if (ret < 0)
+                       return ret;
+       }
+

+       /* codec registration  */
+       ret = devm_snd_soc_register_component(dev, &soc_codec_dev_max98363,
+                                             max98363_dai, +
ARRAY_SIZE(max98363_dai));
+       if (ret < 0)
+               dev_err(dev, "Failed to register codec: %d\n", ret);

sound/soc/codecs/max98363.c new file +8 -0

+
+       return ret;
+}
+
+static int max98363_sdw_probe(struct sdw_slave *slave,
+                             const struct sdw_device_id *id)
+{
+       struct regmap *regmap;

sound/soc/codecs/max98363.c new file +8 -0

+static const struct acpi_device_id max98363_acpi_match[] = {
+       { "ADS8363", 0 },
+       {},
+};
+MODULE_DEVICE_TABLE(acpi, max98363_acpi_match);
+#endif
+
+static const struct sdw_device_id max98363_id[] = {

+               .acpi_match_table = ACPI_PTR(max98363_acpi_match), +               .pm = &max98363_pm,

+       },
+       .probe = max98363_sdw_probe,
+       .remove = NULL,

-----Original Message-----
From: Nuno Sá <[email protected]>
Sent: Thursday, February 23, 2023 11:57 PM
To: “Ryan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; Lee, RyanS <[email protected]>
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[External]

Quoted 60 lines · “Ryan

From: Ryan Lee <[email protected]>

Added Analog Devices MAX98363 SoundWire Amplifier Driver. The MAX98363 is a SoundWire peripheral device that supports MIPI SoundWire v1.2-compatible digital interface for audio and control data.

Signed-off-by Ryan Lee <[email protected]>

---
 sound/soc/codecs/Kconfig    |  11 +
 sound/soc/codecs/Makefile   |   2 +
 sound/soc/codecs/max98363.c | 699
++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/max98363.h | 108 ++++++

 4 files changed, 820 insertions(+)

 create mode 100644 sound/soc/codecs/max98363.c
 create mode 100644 sound/soc/codecs/max98363.h

sound/soc/codecs/Kconfig index renamed from sound/soc/codecs/Kconfig +0 -0

4621674e68bf..0638b27c6494 100644

sound/soc/codecs/Kconfig +0 -0

@@ -130,6 +130,7 @@ config SND_SOC_ALL_CODECS

        imply SND_SOC_MAX98925
        imply SND_SOC_MAX98926
        imply SND_SOC_MAX98927
+       imply SND_SOC_MAX98363
        imply SND_SOC_MAX98373_I2C
        imply SND_SOC_MAX98373_SDW
        imply SND_SOC_MAX98390

sound/soc/codecs/Kconfig +0 -0

@@ -1094,6 +1095,16 @@ config SND_SOC_MAX98520
 

          To compile this driver as a module, choose M here.

+config SND_SOC_MAX98363
+       tristate "Analog Devices MAX98363 Soundwire Speaker Amplifier"
+       depends on SOUNDWIRE
+       select REGMAP_SOUNDWIRE
+       help
+         Enable support for Analog Devices MAX98363 Soundwire +         amplifier. MAX98363 supports the MIPI SoundWire v1.2 +         compatible interface for audio and control data. +         This amplifier does not support I2C and I2S. +
 config SND_SOC_MAX98373
        tristate

sound/soc/codecs/Makefile +0 -0

index a0a61554548e..f4fbeceed816 100644
@@ -144,6 +144,7 @@ snd-soc-max98925-objs := max98925.o

 snd-soc-max98926-objs := max98926.o snd-soc-max98927-objs := max98927.o snd-soc-max98520-objs := max98520.o +snd-soc-max98363-objs := max98363.o snd-soc-max98373-objs := max98373.o snd-soc-max98373-i2c-objs := max98373-i2c.o snd-soc-max98373-sdw-objs := max98373-sdw.o @@ -507,6 +508,7 @@

Quoted 8 lines

obj-$(CONFIG_SND_SOC_MAX98925)      += snd-soc- max98925.o obj-$(CONFIG_SND_SOC_MAX98926) += snd-soc-max98926.o obj-$(CONFIG_SND_SOC_MAX98927) += snd-soc-max98927.o obj-$(CONFIG_SND_SOC_MAX98520) += snd-soc-max98520.o +obj-$(CONFIG_SND_SOC_MAX98363) += snd-soc-max98363.o obj-$(CONFIG_SND_SOC_MAX98373) += snd-soc-max98373.o obj-$(CONFIG_SND_SOC_MAX98373_I2C)   += snd-soc-max98373-i2c.o obj-$(CONFIG_SND_SOC_MAX98373_SDW)   += snd-soc-max98373-sdw.o

diff

Quoted 1 line

--git a/sound/soc/codecs/max98363.c b/sound/soc/codecs/max98363.c

new

Quoted 161 lines

file mode 100644 index 000000000000..e766c733f8e4

sound/soc/codecs/max98363.c new file +75 -0

@@ -0,0 +1,699 @@
+// SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2022,
+Analog Devices Inc.
+
+#include <linux/acpi.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <linux/of.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h> #include
+<linux/soundwire/sdw_registers.h>
+#include <linux/regulator/consumer.h> #include "max98363.h"
+
+struct sdw_stream_data {
+       struct sdw_stream_runtime *sdw_stream; };
+
+static struct reg_default max98363_reg[] = {
+       {MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+       {MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+       {MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
+       {MAX98363_R0044_SCP_CTRL, 0x00},
+       {MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
+       {MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
+       {MAX98363_R004D_SCP_BUS_CLK, 0x00},
+       {MAX98363_R0050_SCP_DEV_ID_0, 0x21},
+       {MAX98363_R0051_SCP_DEV_ID_1, 0x01},
+       {MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
+       {MAX98363_R0053_SCP_DEV_ID_3, 0x87},
+       {MAX98363_R0054_SCP_DEV_ID_4, 0x08},
+       {MAX98363_R0055_SCP_DEV_ID_5, 0x00},
+       {MAX98363_R0060_SCP_FRAME_CTRL, 0x00},
+       {MAX98363_R0062_SCP_CLK_SCALE_BANK0, 0x00},
+       {MAX98363_R0070_SCP_FRAME_CTRL, 0x00},
+       {MAX98363_R0072_SCP_CLK_SCALE_BANK1, 0x00},
+       {MAX98363_R0080_SCP_PHYOUTCTRL_0, 0x00},
+       {MAX98363_R0100_DP1_INIT_STAT, 0x00},
+       {MAX98363_R0101_DP1_INIT_MASK, 0x00},
+       {MAX98363_R0102_DP1_PORT_CTRL, 0x00},
+       {MAX98363_R0103_DP1_BLOCK_CTRL_1, 0x00},
+       {MAX98363_R0104_DP1_PREPARE_STATUS, 0x00},
+       {MAX98363_R0105_DP1_PREPARE_CTRL, 0x00},
+       {MAX98363_R0120_DP1_CHANNEL_EN, 0x00},
+       {MAX98363_R0122_DP1_SAMPLE_CTRL1, 0x00},
+       {MAX98363_R0123_DP1_SAMPLE_CTRL2, 0x00},
+       {MAX98363_R0124_DP1_OFFSET_CTRL1, 0x00},
+       {MAX98363_R0125_DP1_OFFSET_CTRL2, 0x00},
+       {MAX98363_R0126_DP1_HCTRL, 0x00},
+       {MAX98363_R0127_DP1_BLOCK_CTRL3, 0x00},
+       {MAX98363_R0130_DP1_CHANNEL_EN, 0x00},
+       {MAX98363_R0132_DP1_SAMPLE_CTRL1, 0x00},
+       {MAX98363_R0133_DP1_SAMPLE_CTRL2, 0x00},
+       {MAX98363_R0134_DP1_OFFSET_CTRL1, 0x00},
+       {MAX98363_R0135_DP1_OFFSET_CTRL2, 0x00},
+       {MAX98363_R0136_DP1_HCTRL, 0x0136},
+       {MAX98363_R0137_DP1_BLOCK_CTRL3, 0x00},
+       {MAX98363_R2001_INTR_RAW, 0x0},
+       {MAX98363_R2003_INTR_STATE, 0x0},
+       {MAX98363_R2005_INTR_FALG, 0x0},
+       {MAX98363_R2007_INTR_EN, 0x0},
+       {MAX98363_R2009_INTR_CLR, 0x0},
+       {MAX98363_R2021_ERR_MON_CTRL, 0x0},
+       {MAX98363_R2022_SPK_MON_THRESH, 0x0},
+       {MAX98363_R2023_SPK_MON_DURATION, 0x0},
+       {MAX98363_R2030_TONE_GEN_CFG, 0x0},
+       {MAX98363_R203F_TONE_GEN_EN, 0x0},
+       {MAX98363_R2040_AMP_VOL, 0x0},
+       {MAX98363_R2041_AMP_GAIN, 0x5},
+       {MAX98363_R2042_DSP_CFG, 0x0},
+       {MAX98363_R21FF_REV_ID, 0x0},
+};
+
+static bool max98363_readable_register(struct device *dev, unsigned

int reg)
+{
+       switch (reg) {
+       /* SoundWire Control Port Registers */
+       case MAX98363_R0040_SCP_INIT_STAT_1 ...
MAX98363_R0046_SCP_DEV_NUMBER:
+       case MAX98363_R004D_SCP_BUS_CLK:
+       case MAX98363_R0050_SCP_DEV_ID_0 ...
MAX98363_R0055_SCP_DEV_ID_5:
+       case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+       case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+       case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+       /* Soundwire Data Port 1 Registers */
+       case MAX98363_R0100_DP1_INIT_STAT ...
MAX98363_R0105_DP1_PREPARE_CTRL:
+       case MAX98363_R0120_DP1_CHANNEL_EN ...
MAX98363_R0127_DP1_BLOCK_CTRL3:
+       case MAX98363_R0130_DP1_CHANNEL_EN:
+       case MAX98363_R0132_DP1_SAMPLE_CTRL1...
MAX98363_R0137_DP1_BLOCK_CTRL3:
+       /* MAX98363 Amp Control Registers */
+       case MAX98363_R2001_INTR_RAW:
+       case MAX98363_R2003_INTR_STATE:
+       case MAX98363_R2005_INTR_FALG:
+       case MAX98363_R2007_INTR_EN:
+       case MAX98363_R2009_INTR_CLR:
+       case MAX98363_R2021_ERR_MON_CTRL ...
MAX98363_R2023_SPK_MON_DURATION:
+       case MAX98363_R2030_TONE_GEN_CFG:
+       case MAX98363_R203F_TONE_GEN_EN:
+       case MAX98363_R2040_AMP_VOL:
+       case MAX98363_R2041_AMP_GAIN:
+       case MAX98363_R2042_DSP_CFG:
+       case MAX98363_R21FF_REV_ID:
+               return true;
+       default:
+               return false;
+       }

sound/soc/codecs/max98363.c new file +3 -0

+};
+
+static bool max98363_volatile_reg(struct device *dev, unsigned int

reg)
+{
+       switch (reg) {
+       /* SoundWire Control Port Registers */
+       case MAX98363_R0040_SCP_INIT_STAT_1 ...
MAX98363_R0046_SCP_DEV_NUMBER:
+       case MAX98363_R004D_SCP_BUS_CLK:
+       case MAX98363_R0050_SCP_DEV_ID_0 ...
MAX98363_R0055_SCP_DEV_ID_5:
+       case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+       case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+       case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+       /* Soundwire Data Port 1 Registers */
+       case MAX98363_R0100_DP1_INIT_STAT ...
MAX98363_R0105_DP1_PREPARE_CTRL:
+       case MAX98363_R0120_DP1_CHANNEL_EN ...
MAX98363_R0127_DP1_BLOCK_CTRL3:
+       case MAX98363_R0130_DP1_CHANNEL_EN:
+       case MAX98363_R0132_DP1_SAMPLE_CTRL1...
MAX98363_R0137_DP1_BLOCK_CTRL3:
+       /* MAX98363 Amp Control Registers */
+       case MAX98363_R2001_INTR_RAW:
+       case MAX98363_R2003_INTR_STATE:
+       case MAX98363_R2005_INTR_FALG:
+       case MAX98363_R2007_INTR_EN:
+       case MAX98363_R2009_INTR_CLR:
+       case MAX98363_R21FF_REV_ID:
+               return true;
+       default:
+               return false;
+       }

sound/soc/codecs/max98363.c new file +10 -0

+}
+
+static const struct regmap_config max98363_sdw_regmap = {
+       .reg_bits = 32,
+       .val_bits = 8,
+       .max_register = MAX98363_R21FF_REV_ID,
+       .reg_defaults  = max98363_reg,
+       .num_reg_defaults = ARRAY_SIZE(max98363_reg),
+       .readable_reg = max98363_readable_register,
+       .volatile_reg = max98363_volatile_reg,

sound/soc/codecs/max98363.c line 152

Quoted 6 lines

+       .cache_type = REGCACHE_RBTREE,
+       .use_single_read = true,
+       .use_single_write = true,

sound/soc/codecs/max98363.c new file +3 -0

+};
+
+static __maybe_unused int max98363_suspend(struct device *dev)

can drop '__maybe_unused'...

Quoted 13 lines

sound/soc/codecs/max98363.c new file +12 -0

+{
+       struct max98363_priv *max98363 = dev_get_drvdata(dev);
+
+       regcache_cache_only(max98363->regmap, true);
+       regcache_mark_dirty(max98363->regmap);
+
+       if (max98363->dvddio)
+               regulator_disable(max98363->dvddio);
+
+       if (max98363->vdd)
+               regulator_disable(max98363->vdd);
+

sound/soc/codecs/max98363.c line 170

Quoted 6 lines

+       return 0;

sound/soc/codecs/max98363.c new file +5 -0

+}
+
+#define MAX98363_PROBE_TIMEOUT 5000
+
+static __maybe_unused int max98363_resume(struct device *dev)

ditto...

Quoted 36 lines

sound/soc/codecs/max98363.c new file +13 -0

+{
+       struct sdw_slave *slave = dev_to_sdw_dev(dev);
+       struct max98363_priv *max98363 = dev_get_drvdata(dev);
+       unsigned long time;
+       int ret;
+
+       if (!max98363->first_hw_init)
+               return 0;
+
+       if (!slave->unattach_request)
+               goto regmap_sync;
+
+       time = wait_for_completion_timeout(&slave-

Quoted 1 line

initialization_complete,

+
msecs_to_jiffies(MAX98363_PROBE_TIMEOUT));
+       if (!time) {
+               dev_err(dev, "Initialization not complete, timed out\n");
+               return -ETIMEDOUT;
+       }

sound/soc/codecs/max98363.c new file +14 -0

+
+regmap_sync:
+
+       if (max98363->dvddio) {
+               ret = regulator_enable(max98363->dvddio);
+               if (ret < 0)
+                       return ret;
+       }
+
+       if (max98363->vdd) {
+               ret = regulator_enable(max98363->vdd);
+               if (ret < 0)
+                       return ret;
+       }

sound/soc/codecs/max98363.c

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+
+       slave->unattach_request = 0;
+       regcache_cache_only(max98363->regmap, false);
+       regcache_sync(max98363->regmap);
+
+       return 0;
+}
+
+static const struct dev_pm_ops max98363_pm = {
+       SET_SYSTEM_SLEEP_PM_OPS(max98363_suspend,

max98363_resume)

Quoted 1 line

+       SET_RUNTIME_PM_OPS(max98363_suspend, max98363_resume,

NULL) };

Quoted 1 line

+

DEFINE_RUNTIME_DEV_PM_OPS()...

Quoted 7 lines

sound/soc/codecs/max98363.c new file +7 -0

+static int max98363_read_prop(struct sdw_slave *slave) {
+       struct sdw_slave_prop *prop = &slave->prop;
+       int nval, i;
+       u32 bit;
+       unsigned long addr;
+       struct sdw_dpn_prop *dpn;
+

...

Quoted 14 lines

+static int max98363_init(struct sdw_slave *slave, struct regmap *regmap)

sound/soc/codecs/max98363.c new file +12 -0

+{
+       struct max98363_priv *max98363;
+       int ret;
+       struct device *dev = &slave->dev;
+
+       /*  Allocate and assign private driver data structure  */
+       max98363 = devm_kzalloc(dev, sizeof(*max98363), GFP_KERNEL);
+       if (!max98363)
+               return -ENOMEM;
+
+       dev_set_drvdata(dev, max98363);
+       max98363->regmap = regmap;

sound/soc/codecs/max98363.c line 597

Quoted 6 lines

+       max98363->slave = slave;

sound/soc/codecs/max98363.c new file +5 -0

+
+       max98363->hw_init = false;
+       max98363->first_hw_init = false;
+
+       max98363->vdd = devm_regulator_get_optional(dev, "vdd");

vdd is not an optional regulator, right?

Quoted 9 lines

+       if (IS_ERR(max98363->vdd)) {
+               if (PTR_ERR(max98363->vdd) == -EPROBE_DEFER) +                       return -EPROBE_DEFER;
+
+               max98363->vdd = NULL;
+       }
+       max98363->dvddio = devm_regulator_get_optional(dev, "dvddio");

ditto...

Quoted 27 lines

+       if (IS_ERR(max98363->dvddio)) {
+               if (PTR_ERR(max98363->dvddio) == -EPROBE_DEFER) +                       return -EPROBE_DEFER;

sound/soc/codecs/max98363.c new file +11 -0

+
+               max98363->dvddio = NULL;
+       }
+
+       if (max98363->vdd) {
+               ret = regulator_enable(max98363->vdd);
+               if (ret < 0)
+                       return ret;
+
+               ret = devm_add_action_or_reset(dev,
+

max98363_supply_disable,
+                                              max98363->vdd); +               if (ret < 0)
+                       return ret;
+       }

sound/soc/codecs/max98363.c new file +8 -0

+
+       if (max98363->dvddio) {
+               ret = regulator_enable(max98363->dvddio);
+               if (ret < 0)
+                       return ret;
+
+               ret = devm_add_action_or_reset(dev,
+

Quoted 6 lines

max98363_supply_disable,
+                                              max98363->dvddio); +               if (ret < 0)
+                       return ret;
+       }
+

Being said the regulators are not optional, you can use devm_regulator_get_enable(). If there's no constrains in power up/down ordering, consider devm_regulator_bulk_get_enable().

Quoted 18 lines

+       /* codec registration  */
+       ret = devm_snd_soc_register_component(dev, &soc_codec_dev_max98363,
+                                             max98363_dai, +
ARRAY_SIZE(max98363_dai));
+       if (ret < 0)
+               dev_err(dev, "Failed to register codec: %d\n", ret);

sound/soc/codecs/max98363.c new file +10 -0

+
+       return ret;
+}
+
+static int max98363_sdw_probe(struct sdw_slave *slave,
+                             const struct sdw_device_id *id) {
+       struct regmap *regmap;
+
+       /* Regmap Initialization */
+       regmap = devm_regmap_init_sdw(slave, &max98363_sdw_regmap);

sound/soc/codecs/max98363.c line 659

Quoted 6 lines

+       if (IS_ERR(regmap))
+               return PTR_ERR(regmap);

sound/soc/codecs/max98363.c new file +4 -0

+
+       return max98363_init(slave, regmap); }
+
+#if defined(CONFIG_OF)

no need for this guard...

sound/soc/codecs/max98363.c line 667

Quoted 8 lines

sound/soc/codecs/max98363.c new file +7 -0

+static const struct of_device_id max98363_of_match[] = {
+       { .compatible = "adi,max98363", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, max98363_of_match); #endif
+
+#ifdef CONFIG_ACPI

and this. See below

Quoted 12 lines

sound/soc/codecs/max98363.c new file +12 -0

+static const struct acpi_device_id max98363_acpi_match[] = {
+       { "ADS8363", 0 },
+       {},
+};
+MODULE_DEVICE_TABLE(acpi, max98363_acpi_match); #endif
+
+static const struct sdw_device_id max98363_id[] = {
+       SDW_SLAVE_ENTRY(0x019F, 0x8363, 0),
+       {},
+};
+MODULE_DEVICE_TABLE(sdw, max98363_id);
+

Quoted 5 lines

+static struct sdw_driver max98363_sdw_driver = {
+       .driver = {
+               .name = "max98363",
+               .owner = THIS_MODULE,
+               .of_match_table = of_match_ptr(max98363_of_match),

I think you can drop 'acpi_match_table'. If you drop of_match_ptr() and always define the of_device_id table, ACPI can use it to probe the device.

Quoted 3 lines

+               .acpi_match_table = ACPI_PTR(max98363_acpi_match), +               .pm = &max98363_pm,

use pm_ptr(). With this macro, no need for '__maybe_unused' annotations.

Quoted 4 lines

+       },
+       .probe = max98363_sdw_probe,
+       .remove = NULL,

no need for this.

sound/soc/codecs/max98363.c line 696

Quoted 8 lines

+       .ops = &max98363_slave_ops,
+       .id_table = max98363_id,

sound/soc/codecs/max98363.c new file +4 -0

+};
+
+module_sdw_driver(max98363_sdw_driver);
+

- Nuno Sá

sound/soc/codecs/max98363.c new file +5 -0

+
+static struct reg_default max98363_reg[] = {
+	{MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+	{MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+	{MAX98363_R0042_SCP_INIT_STAT_2, 0x00},

sound/soc/codecs/max98363.c new file +101 -0

+	{MAX98363_R0060_SCP_FRAME_CTRL, 0x00},
+	{MAX98363_R0062_SCP_CLK_SCALE_BANK0, 0x00},
+	{MAX98363_R0070_SCP_FRAME_CTRL, 0x00},
+	{MAX98363_R0072_SCP_CLK_SCALE_BANK1, 0x00},
+	{MAX98363_R0080_SCP_PHYOUTCTRL_0, 0x00},
+	{MAX98363_R0100_DP1_INIT_STAT, 0x00},
+	{MAX98363_R0101_DP1_INIT_MASK, 0x00},
+	{MAX98363_R0102_DP1_PORT_CTRL, 0x00},
+	{MAX98363_R0103_DP1_BLOCK_CTRL_1, 0x00},
+	{MAX98363_R0104_DP1_PREPARE_STATUS, 0x00},
+	{MAX98363_R0105_DP1_PREPARE_CTRL, 0x00},
+	{MAX98363_R0120_DP1_CHANNEL_EN, 0x00},
+	{MAX98363_R0122_DP1_SAMPLE_CTRL1, 0x00},
+	{MAX98363_R0123_DP1_SAMPLE_CTRL2, 0x00},
+	{MAX98363_R0124_DP1_OFFSET_CTRL1, 0x00},
+	{MAX98363_R0125_DP1_OFFSET_CTRL2, 0x00},
+	{MAX98363_R0126_DP1_HCTRL, 0x00},
+	{MAX98363_R0127_DP1_BLOCK_CTRL3, 0x00},
+	{MAX98363_R0130_DP1_CHANNEL_EN, 0x00},
+	{MAX98363_R0132_DP1_SAMPLE_CTRL1, 0x00},
+	{MAX98363_R0133_DP1_SAMPLE_CTRL2, 0x00},
+	{MAX98363_R0134_DP1_OFFSET_CTRL1, 0x00},
+	{MAX98363_R0135_DP1_OFFSET_CTRL2, 0x00},
+	{MAX98363_R0136_DP1_HCTRL, 0x0136},
+	{MAX98363_R0137_DP1_BLOCK_CTRL3, 0x00},
+	{MAX98363_R2001_INTR_RAW, 0x0},
+	{MAX98363_R2003_INTR_STATE, 0x0},
+	{MAX98363_R2005_INTR_FALG, 0x0},
+	{MAX98363_R2007_INTR_EN, 0x0},
+	{MAX98363_R2009_INTR_CLR, 0x0},
+	{MAX98363_R2021_ERR_MON_CTRL, 0x0},
+	{MAX98363_R2022_SPK_MON_THRESH, 0x0},
+	{MAX98363_R2023_SPK_MON_DURATION, 0x0},
+	{MAX98363_R2030_TONE_GEN_CFG, 0x0},
+	{MAX98363_R203F_TONE_GEN_EN, 0x0},
+	{MAX98363_R2040_AMP_VOL, 0x0},
+	{MAX98363_R2041_AMP_GAIN, 0x5},
+	{MAX98363_R2042_DSP_CFG, 0x0},
+	{MAX98363_R21FF_REV_ID, 0x0},
+};
+
+static bool max98363_readable_register(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	/* SoundWire Control Port Registers */
+	case MAX98363_R0040_SCP_INIT_STAT_1 ... MAX98363_R0046_SCP_DEV_NUMBER:
+	case MAX98363_R004D_SCP_BUS_CLK:
+	case MAX98363_R0050_SCP_DEV_ID_0 ... MAX98363_R0055_SCP_DEV_ID_5:
+	case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+	case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+	case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+	/* Soundwire Data Port 1 Registers */
+	case MAX98363_R0100_DP1_INIT_STAT ... MAX98363_R0105_DP1_PREPARE_CTRL:
+	case MAX98363_R0120_DP1_CHANNEL_EN ... MAX98363_R0127_DP1_BLOCK_CTRL3:
+	case MAX98363_R0130_DP1_CHANNEL_EN:
+	case MAX98363_R0132_DP1_SAMPLE_CTRL1... MAX98363_R0137_DP1_BLOCK_CTRL3:
+	/* MAX98363 Amp Control Registers */
+	case MAX98363_R2001_INTR_RAW:
+	case MAX98363_R2003_INTR_STATE:
+	case MAX98363_R2005_INTR_FALG:
+	case MAX98363_R2007_INTR_EN:
+	case MAX98363_R2009_INTR_CLR:
+	case MAX98363_R2021_ERR_MON_CTRL ... MAX98363_R2023_SPK_MON_DURATION:
+	case MAX98363_R2030_TONE_GEN_CFG:
+	case MAX98363_R203F_TONE_GEN_EN:
+	case MAX98363_R2040_AMP_VOL:
+	case MAX98363_R2041_AMP_GAIN:
+	case MAX98363_R2042_DSP_CFG:
+	case MAX98363_R21FF_REV_ID:
+		return true;
+	default:
+		return false;
+	}
+};
+
+static bool max98363_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	/* SoundWire Control Port Registers */
+	case MAX98363_R0040_SCP_INIT_STAT_1 ... MAX98363_R0046_SCP_DEV_NUMBER:
+	case MAX98363_R004D_SCP_BUS_CLK:
+	case MAX98363_R0050_SCP_DEV_ID_0 ... MAX98363_R0055_SCP_DEV_ID_5:
+	case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+	case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+	case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+	/* Soundwire Data Port 1 Registers */
+	case MAX98363_R0100_DP1_INIT_STAT ... MAX98363_R0105_DP1_PREPARE_CTRL:
+	case MAX98363_R0120_DP1_CHANNEL_EN ... MAX98363_R0127_DP1_BLOCK_CTRL3:
+	case MAX98363_R0130_DP1_CHANNEL_EN:
+	case MAX98363_R0132_DP1_SAMPLE_CTRL1... MAX98363_R0137_DP1_BLOCK_CTRL3:
+	/* MAX98363 Amp Control Registers */
+	case MAX98363_R2001_INTR_RAW:
+	case MAX98363_R2003_INTR_STATE:
+	case MAX98363_R2005_INTR_FALG:
+	case MAX98363_R2007_INTR_EN:
+	case MAX98363_R2009_INTR_CLR:
+	case MAX98363_R21FF_REV_ID:
+		return true;
+	default:
+		return false;
+	}

sound/soc/codecs/max98363.c new file +46 -0

+	.cache_type = REGCACHE_RBTREE,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static __maybe_unused int max98363_suspend(struct device *dev)
+{
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+
+	regcache_cache_only(max98363->regmap, true);
+	regcache_mark_dirty(max98363->regmap);
+
+	if (max98363->dvddio)
+		regulator_disable(max98363->dvddio);
+
+	if (max98363->vdd)
+		regulator_disable(max98363->vdd);
+
+	return 0;
+}
+
+#define MAX98363_PROBE_TIMEOUT 5000
+
+static __maybe_unused int max98363_resume(struct device *dev)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+	unsigned long time;
+	int ret;
+
+	if (!max98363->first_hw_init)
+		return 0;
+
+	if (!slave->unattach_request)
+		goto regmap_sync;
+
+	time = wait_for_completion_timeout(&slave->initialization_complete,
+					   msecs_to_jiffies(MAX98363_PROBE_TIMEOUT));
+	if (!time) {
+		dev_err(dev, "Initialization not complete, timed out\n");
+		return -ETIMEDOUT;
+	}
+
+regmap_sync:
+
+	if (max98363->dvddio) {

sound/soc/codecs/max98363.c new file +19 -0

+
+	slave->unattach_request = 0;
+	regcache_cache_only(max98363->regmap, false);
+	regcache_sync(max98363->regmap);
+
+	return 0;
+}
+
+static const struct dev_pm_ops max98363_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(max98363_suspend, max98363_resume)
+	SET_RUNTIME_PM_OPS(max98363_suspend, max98363_resume, NULL)
+};
+
+static int max98363_read_prop(struct sdw_slave *slave)
+{
+	struct sdw_slave_prop *prop = &slave->prop;
+	int nval, i;
+	u32 bit;
+	unsigned long addr;

sound/soc/codecs/max98363.c new file +80 -0

+	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
+					  sizeof(*prop->src_dpn_prop),
+					  GFP_KERNEL);
+	if (!prop->src_dpn_prop)
+		return -ENOMEM;
+
+	i = 0;
+	dpn = prop->src_dpn_prop;
+	addr = prop->source_ports;
+	for_each_set_bit(bit, &addr, 32) {
+		dpn[i].num = bit;
+		dpn[i].type = SDW_DPN_FULL;
+		dpn[i].simple_ch_prep_sm = true;
+		dpn[i].ch_prep_timeout = 10;
+		i++;
+	}
+
+	/* do this again for sink now */
+	nval = hweight32(prop->sink_ports);
+	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
+					   sizeof(*prop->sink_dpn_prop),
+					   GFP_KERNEL);
+	if (!prop->sink_dpn_prop)
+		return -ENOMEM;
+
+	i = 0;
+	dpn = prop->sink_dpn_prop;
+	addr = prop->sink_ports;
+	for_each_set_bit(bit, &addr, 32) {
+		dpn[i].num = bit;
+		dpn[i].type = SDW_DPN_FULL;
+		dpn[i].simple_ch_prep_sm = true;
+		dpn[i].ch_prep_timeout = 10;
+		i++;
+	}
+
+	/* set the timeout values */
+	prop->clk_stop_timeout = 20;
+	prop->simple_clk_stop_capable = true;
+	prop->clock_reg_supported = true;
+
+	return 0;
+}
+
+static int max98363_io_init(struct sdw_slave *slave)
+{
+	struct device *dev = &slave->dev;
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+	int ret, reg;
+
+	if (max98363->first_hw_init) {
+		regcache_cache_only(max98363->regmap, false);
+		regcache_cache_bypass(max98363->regmap, true);
+	}
+
+	/*
+	 * PM runtime is only enabled when a Slave reports as Attached
+	 */
+	if (!max98363->first_hw_init) {
+		/* set autosuspend parameters */
+		pm_runtime_set_autosuspend_delay(dev, 3000);
+		pm_runtime_use_autosuspend(dev);
+
+		/* update count of parent 'active' children */
+		pm_runtime_set_active(dev);
+
+		/* make sure the device does not suspend immediately */
+		pm_runtime_mark_last_busy(dev);
+
+		pm_runtime_enable(dev);
+	}
+
+	pm_runtime_get_noresume(dev);
+
+	ret = regmap_read(max98363->regmap, MAX98363_R21FF_REV_ID, &reg);
+	if (!ret) {
+		dev_info(dev, "Revision ID: %X\n", reg);
+		return ret;
+	}
+

sound/soc/codecs/max98363.c new file +16 -0

+
+	return 0;
+}
 
+static int max98363_sdw_set_tdm_slot(struct snd_soc_dai *dai,
+				     unsigned int tx_mask,
+				     unsigned int rx_mask,
+				     int slots, int slot_width)
+{
+	struct snd_soc_component *component = dai->component;
+	struct max98363_priv *max98363 =
+		snd_soc_component_get_drvdata(component);
+
+	/* tx_mask is not supported */
+	if (tx_mask)
+		return -EINVAL;
+

sound/soc/codecs/max98363.c new file +33 -0

+	.set_tdm_slot = max98363_sdw_set_tdm_slot,
+};
+
+static struct snd_soc_dai_driver max98363_dai[] = {
+	{
+		.name = "max98363-aif1",
+		.playback = {
+			.stream_name = "HiFi Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = MAX98363_RATES,
+			.formats = MAX98363_FORMATS,
+		},
+		.ops = &max98363_dai_sdw_ops,
+	}
+};
+
+static int max98363_update_status(struct sdw_slave *slave,
+				  enum sdw_slave_status status)
+{
+	struct max98363_priv *max98363 = dev_get_drvdata(&slave->dev);
+
+	if (status == SDW_SLAVE_UNATTACHED)
+		max98363->hw_init = false;
+
+	/*
+	 * Perform initialization only if slave status is SDW_SLAVE_ATTACHED
+	 */
+	if (max98363->hw_init || status != SDW_SLAVE_ATTACHED)
+		return 0;
+
+	/* perform I/O transfers required for Slave initialization */
+	return max98363_io_init(slave);

sound/soc/codecs/max98363.c new file +7 -0

+	{},
+};
+MODULE_DEVICE_TABLE(acpi, max98363_acpi_match);
+#endif
+
+static const struct sdw_device_id max98363_id[] = {
+	SDW_SLAVE_ENTRY(0x019F, 0x8363, 0),

sound/soc/codecs/max98363.c new file +4 -0

+	.ops = &max98363_slave_ops,
+	.id_table = max98363_id,
+};
+

Quoted 9 lines

sound/soc/codecs/max98363.c new file +9 -0

+static struct reg_default max98363_reg[] = {
+	{MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+	{MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+	{MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
+	{MAX98363_R0044_SCP_CTRL, 0x00},
+	{MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
+	{MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
+	{MAX98363_R004D_SCP_BUS_CLK, 0x00},
+	{MAX98363_R0050_SCP_DEV_ID_0, 0x21},

sound/soc/codecs/max98363.c line 35

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	{MAX98363_R0051_SCP_DEV_ID_1, 0x01},
+	{MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
+	{MAX98363_R0053_SCP_DEV_ID_3, 0x87},
+	{MAX98363_R0054_SCP_DEV_ID_4, 0x08},
+	{MAX98363_R0055_SCP_DEV_ID_5, 0x00},

That seems wrong, why would you declare standard registers that are known to the bus and required to be implemented?

sound/soc/codecs/max98363.c line 146

Quoted 8 lines

sound/soc/codecs/max98363.c new file +8 -0

+static const struct regmap_config max98363_sdw_regmap = {
+	.reg_bits = 32,
+	.val_bits = 8,
+	.max_register = MAX98363_R21FF_REV_ID,
+	.reg_defaults  = max98363_reg,
+	.num_reg_defaults = ARRAY_SIZE(max98363_reg),
+	.readable_reg = max98363_readable_register,
+	.volatile_reg = max98363_volatile_reg,

I don't see why the SoundWire standard registers are part of regmap?

sound/soc/codecs/max98363.c line 202

Quoted 11 lines

sound/soc/codecs/max98363.c new file +11 -0

+	if (max98363->dvddio) {
+		ret = regulator_enable(max98363->dvddio);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (max98363->vdd) {
+		ret = regulator_enable(max98363->vdd);
+		if (ret < 0)
+			return ret;
+	}

that is very very odd. It's the first time we see a SoundWire codec driver that has a power dependency, and it's quite likely that it's too late to enable power resources *AFTER* dealing with all the initialization and enumeration.

It's not even clear to me how this device would be enumerated.

You'd need to explain what part of the amplifier is controlled by those regulator, otherwise it's impossible to review and understand if the driver does the 'right thing'

Quoted 9 lines · Pierre-Louis Bossart

Quoted 9 lines

sound/soc/codecs/max98363.c new file +9 -0

+static struct reg_default max98363_reg[] = {
+	{MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+	{MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+	{MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
+	{MAX98363_R0044_SCP_CTRL, 0x00},
+	{MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
+	{MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
+	{MAX98363_R004D_SCP_BUS_CLK, 0x00},
+	{MAX98363_R0050_SCP_DEV_ID_0, 0x21},

Quoted 8 lines

sound/soc/codecs/max98363.c line 35

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	{MAX98363_R0051_SCP_DEV_ID_1, 0x01},
+	{MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
+	{MAX98363_R0053_SCP_DEV_ID_3, 0x87},
+	{MAX98363_R0054_SCP_DEV_ID_4, 0x08},
+	{MAX98363_R0055_SCP_DEV_ID_5, 0x00},

That seems wrong, why would you declare standard registers that are known to the bus and required to be implemented?

This is the register defaults table, it gets used to initialise the register cache and optimise resync after suspend - all this does is supply defaults for the cache. That said...

I would suggest it's better to not supply defaults for ID registers and read them back from the device otherwise things might get confused.

Quoted 10 lines

sound/soc/codecs/max98363.c line 146

Quoted 8 lines

sound/soc/codecs/max98363.c new file +8 -0

+static const struct regmap_config max98363_sdw_regmap = {
+	.reg_bits = 32,
+	.val_bits = 8,
+	.max_register = MAX98363_R21FF_REV_ID,
+	.reg_defaults  = max98363_reg,
+	.num_reg_defaults = ARRAY_SIZE(max98363_reg),
+	.readable_reg = max98363_readable_register,
+	.volatile_reg = max98363_volatile_reg,

I don't see why the SoundWire standard registers are part of regmap?

...if there's an issue with the SoundWire core modifying the registers directly then the driver would need to mark all the core registers as volatile so that they're not cached otherwise there will be collisions. Or is it the case that we always need to go via the SoundWire core for the generic registers, so they should just never be written at all?

Quoted 18 lines

sound/soc/codecs/max98363.c line 202

Quoted 12 lines

sound/soc/codecs/max98363.c new file +11 -0

+	if (max98363->dvddio) {
+		ret = regulator_enable(max98363->dvddio);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (max98363->vdd) {
+		ret = regulator_enable(max98363->vdd);
+		if (ret < 0)
+			return ret;
+	}

that is very very odd. It's the first time we see a SoundWire codec driver that has a power dependency, and it's quite likely that it's too late to enable power resources *AFTER* dealing with all the initialization and enumeration.

Quoted 5 lines

It's not even clear to me how this device would be enumerated.

You'd need to explain what part of the amplifier is controlled by those regulator, otherwise it's impossible to review and understand if the driver does the 'right thing'

It's also buggy to have regulators treated as optional unless they may be physically absent.

Quoted 11 lines · Mark Brown

Quoted 2 lines · Pierre-Louis Bossart

That seems wrong, why would you declare standard registers that are known to the bus and required to be implemented?

This is the register defaults table, it gets used to initialise the register cache and optimise resync after suspend - all this does is supply defaults for the cache. That said...

I would suggest it's better to not supply defaults for ID registers and read them back from the device otherwise things might get confused.

The 'device_id' register is the good counter example: it includes a 'unique_id' field to deal with cases where there are identical devices on the same link. The unique_id is usually set with board-specific pin-strapping, so there's no good default value here. In previous Maxim 98373 amplifier configurations the unique IDs were 3 and 7 IIRC. The codec driver should not, rather shall not, assume any specific value here.

Quoted 5 lines

...if there's an issue with the SoundWire core modifying the registers directly then the driver would need to mark all the core registers as volatile so that they're not cached otherwise there will be collisions. Or is it the case that we always need to go via the SoundWire core for the generic registers, so they should just never be written at all?

It's really that the SoundWire core will 'own' or take care of all 'standard' programming registers. There is no good reason for a codec driver to interfere with standard port programming or clock stop. The bus provides a set of callbacks that can be used for vendor-specific registers and sequences.

Put differently, SoundWire codec drivers should only deal with non-standard vendor-specific registers.

-----Original Message-----
From: Mark Brown <[email protected]>
Sent: Monday, February 27, 2023 10:45 AM
To: Pierre-Louis Bossart <[email protected]>
Cc: “Ryan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; Lee, RyanS <[email protected]>
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[External]

Quoted 13 lines · Pierre-Louis Bossart

Quoted 11 lines · Mark Brown

Quoted 2 lines · Pierre-Louis Bossart

That seems wrong, why would you declare standard registers that are known to the bus and required to be implemented?

This is the register defaults table, it gets used to initialise the register cache and optimise resync after suspend - all this does is supply defaults for the cache. That said...

I would suggest it's better to not supply defaults for ID registers and read them back from the device otherwise things might get confused.

Quoted 7 lines

The 'device_id' register is the good counter example: it includes a 'unique_id' field to deal with cases where there are identical devices on the same link. The unique_id is usually set with board-specific pin-strapping, so there's no good default value here. In previous Maxim
98373 amplifier configurations the unique IDs were 3 and 7 IIRC. The codec driver should not, rather shall not, assume any specific value here.

Yes, as I said above ID registers in particular are often better off handled as volatile even ignoring any potential for them to show variable configuration information.

Quoted 3 lines

Quoted 3 lines

...if there's an issue with the SoundWire core modifying the registers directly then the driver would need to mark all the core registers as volatile so that they're not cached otherwise there will be

collisions.

Quoted 8 lines

Quoted 2 lines

Or is it the case that we always need to go via the SoundWire core for the generic registers, so they should just never be written at all?

It's really that the SoundWire core will 'own' or take care of all 'standard' programming registers. There is no good reason for a codec driver to interfere with standard port programming or clock stop. The bus provides a set of callbacks that can be used for vendor-specific registers and sequences.

Quoted 2 lines

Put differently, SoundWire codec drivers should only deal with non-standard vendor-specific registers.

OK, it'd be good to be clear about what the issue is when reviewing things. The registers *are* in the device's register map but the driver shouldn't be referencing them at all and should instead be going via the SoundWire core for anything in there.

Quoted 8 lines

Quoted 3 lines

Put differently, SoundWire codec drivers should only deal with non-standard vendor-specific registers.

OK, it'd be good to be clear about what the issue is when reviewing things. The registers *are* in the device's register map but the driver shouldn't be referencing them at all and should instead be going via the SoundWire core for anything in there.

Thanks for the comment.
The only reason I added standard SoundWire registers to the amp driver is to check the values for the debugging purpose because these registers values are important to understand the device status, but it is not visible from the regmap debugfs if those registers are not included on the regmap table of the driver. The driver never controls the standard SoundWire registers by itself. Do you recommend removing the standard SoundWire registers from the driver or keeping it non-volatile?
(The reg_default values in the table are all amp reset values and those registers are treated as volatile. I shall clear 'unique ID' field because it is determined by the hardware pin connection.)

-----Original Message-----
From: Pierre-Louis Bossart <[email protected]>
Sent: Monday, February 27, 2023 3:38 PM
To: Lee, RyanS <[email protected]>; Mark Brown <[email protected]>
Cc: “Ryan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[External]

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

Put differently, SoundWire codec drivers should only deal with non-standard vendor-specific registers.

OK, it'd be good to be clear about what the issue is when reviewing things. The registers *are* in the device's register map but the driver shouldn't be referencing them at all and should instead be going via the SoundWire core for anything in there.

Quoted 5 lines

Thanks for the comment.
The only reason I added standard SoundWire registers to the amp driver is to check the values for the debugging purpose because these registers values are important to understand the device status, but it is not visible from the regmap debugfs if those registers are not included

on the regmap table of the driver.

Quoted 6 lines

The driver never controls the standard SoundWire registers by itself. Do you recommend removing the standard SoundWire registers from the driver or keeping it non-volatile?
(The reg_default values in the table are all amp reset values and those registers are treated as volatile. I shall clear 'unique ID' field because it is determined by the hardware pin connection.)

We already have debugfs support for those registers, see sdw_slave_reg_show() in drivers/soundwire/debugfs.c

It's not the same file as regmap debugfs but the information is already there, see e.g. an example on the SOF CI devices:

cd /sys/kernel/debug/soundwire/master-0-1/sdw:1:025d:0700:00 more registers

Register Value

DP0
0 0
1 0
2 0
3 0
4 0
5 1
Bank0
20 0
22 0
23 0
24 0
25 0
26 0
27 XX
28 XX
Bank1
30 0
32 0
33 0
34 0
35 0
36 0
37 XX
38 XX

SCP
40 0
41 7
42 0
43 0
44 20
45 9
46 4
47 XX
48 XX
49 XX
4a XX
4b XX
50 10
51 2
52 5d
53 7
54 0
55 0

DP1
100 0
101 0
102 0

-----Original Message-----
From: Pierre-Louis Bossart <[email protected]>
Sent: Monday, February 27, 2023 7:18 AM
To: “Ryan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Lee, RyanS <[email protected]>
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[External]

sound/soc/codecs/max98363.c line 15

Quoted 12 lines

sound/soc/codecs/max98363.c new file +12 -0

+#include <linux/acpi.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <linux/of.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h> #include
+<linux/soundwire/sdw_registers.h>
+#include <linux/regulator/consumer.h>

alphabetical order?

sound/soc/codecs/max98363.c

Quoted 5 lines

sound/soc/codecs/max98363.c new file +4 -0

+#include "max98363.h"
+
+struct sdw_stream_data {
+	struct sdw_stream_runtime *sdw_stream; };

this abstraction isn't needed. We need to remove it for all existing codec drivers, and for new ones it's better to avoid it.

Quoted 11 lines

sound/soc/codecs/max98363.c new file +10 -0

+
+static struct reg_default max98363_reg[] = {
+	{MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
+	{MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
+	{MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
+	{MAX98363_R0044_SCP_CTRL, 0x00},
+	{MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
+	{MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
+	{MAX98363_R004D_SCP_BUS_CLK, 0x00},
+	{MAX98363_R0050_SCP_DEV_ID_0, 0x21},

sound/soc/codecs/max98363.c line 35

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	{MAX98363_R0051_SCP_DEV_ID_1, 0x01},
+	{MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
+	{MAX98363_R0053_SCP_DEV_ID_3, 0x87},
+	{MAX98363_R0054_SCP_DEV_ID_4, 0x08},
+	{MAX98363_R0055_SCP_DEV_ID_5, 0x00},

That seems wrong, why would you declare standard registers that are known to the bus and required to be implemented?

Quoted 37 lines

sound/soc/codecs/max98363.c new file +36 -0

+	{MAX98363_R0060_SCP_FRAME_CTRL, 0x00},
+	{MAX98363_R0062_SCP_CLK_SCALE_BANK0, 0x00},
+	{MAX98363_R0070_SCP_FRAME_CTRL, 0x00},
+	{MAX98363_R0072_SCP_CLK_SCALE_BANK1, 0x00},
+	{MAX98363_R0080_SCP_PHYOUTCTRL_0, 0x00},
+	{MAX98363_R0100_DP1_INIT_STAT, 0x00},
+	{MAX98363_R0101_DP1_INIT_MASK, 0x00},
+	{MAX98363_R0102_DP1_PORT_CTRL, 0x00},
+	{MAX98363_R0103_DP1_BLOCK_CTRL_1, 0x00},
+	{MAX98363_R0104_DP1_PREPARE_STATUS, 0x00},
+	{MAX98363_R0105_DP1_PREPARE_CTRL, 0x00},
+	{MAX98363_R0120_DP1_CHANNEL_EN, 0x00},
+	{MAX98363_R0122_DP1_SAMPLE_CTRL1, 0x00},
+	{MAX98363_R0123_DP1_SAMPLE_CTRL2, 0x00},
+	{MAX98363_R0124_DP1_OFFSET_CTRL1, 0x00},
+	{MAX98363_R0125_DP1_OFFSET_CTRL2, 0x00},
+	{MAX98363_R0126_DP1_HCTRL, 0x00},
+	{MAX98363_R0127_DP1_BLOCK_CTRL3, 0x00},
+	{MAX98363_R0130_DP1_CHANNEL_EN, 0x00},
+	{MAX98363_R0132_DP1_SAMPLE_CTRL1, 0x00},
+	{MAX98363_R0133_DP1_SAMPLE_CTRL2, 0x00},
+	{MAX98363_R0134_DP1_OFFSET_CTRL1, 0x00},
+	{MAX98363_R0135_DP1_OFFSET_CTRL2, 0x00},
+	{MAX98363_R0136_DP1_HCTRL, 0x0136},
+	{MAX98363_R0137_DP1_BLOCK_CTRL3, 0x00},
+	{MAX98363_R2001_INTR_RAW, 0x0},
+	{MAX98363_R2003_INTR_STATE, 0x0},
+	{MAX98363_R2005_INTR_FALG, 0x0},
+	{MAX98363_R2007_INTR_EN, 0x0},
+	{MAX98363_R2009_INTR_CLR, 0x0},
+	{MAX98363_R2021_ERR_MON_CTRL, 0x0},
+	{MAX98363_R2022_SPK_MON_THRESH, 0x0},
+	{MAX98363_R2023_SPK_MON_DURATION, 0x0},
+	{MAX98363_R2030_TONE_GEN_CFG, 0x0},
+	{MAX98363_R203F_TONE_GEN_EN, 0x0},
+	{MAX98363_R2040_AMP_VOL, 0x0},

sound/soc/codecs/max98363.c

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+	{MAX98363_R2041_AMP_GAIN, 0x5},
+	{MAX98363_R2042_DSP_CFG, 0x0},
+	{MAX98363_R21FF_REV_ID, 0x0},
+};
+
+static bool max98363_readable_register(struct device *dev, unsigned
+int reg) {
+	switch (reg) {
+	/* SoundWire Control Port Registers */
+	case MAX98363_R0040_SCP_INIT_STAT_1 ...

MAX98363_R0046_SCP_DEV_NUMBER:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	case MAX98363_R004D_SCP_BUS_CLK:
+	case MAX98363_R0050_SCP_DEV_ID_0 ...

MAX98363_R0055_SCP_DEV_ID_5:

sound/soc/codecs/max98363.c

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+	case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+	case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+	/* Soundwire Data Port 1 Registers */
+	case MAX98363_R0100_DP1_INIT_STAT ...

MAX98363_R0105_DP1_PREPARE_CTRL:

Review

Quoted 1 line

(quoted diff) +1 -0

+	case MAX98363_R0120_DP1_CHANNEL_EN ...

MAX98363_R0127_DP1_BLOCK_CTRL3:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	case MAX98363_R0130_DP1_CHANNEL_EN:
+	case MAX98363_R0132_DP1_SAMPLE_CTRL1...

MAX98363_R0137_DP1_BLOCK_CTRL3:

sound/soc/codecs/max98363.c

Quoted 7 lines

sound/soc/codecs/max98363.c new file +7 -0

+	/* MAX98363 Amp Control Registers */
+	case MAX98363_R2001_INTR_RAW:
+	case MAX98363_R2003_INTR_STATE:
+	case MAX98363_R2005_INTR_FALG:
+	case MAX98363_R2007_INTR_EN:
+	case MAX98363_R2009_INTR_CLR:
+	case MAX98363_R2021_ERR_MON_CTRL ...

MAX98363_R2023_SPK_MON_DURATION:

Quoted 7 lines

sound/soc/codecs/max98363.c new file +7 -0

+	case MAX98363_R2030_TONE_GEN_CFG:
+	case MAX98363_R203F_TONE_GEN_EN:
+	case MAX98363_R2040_AMP_VOL:
+	case MAX98363_R2041_AMP_GAIN:
+	case MAX98363_R2042_DSP_CFG:
+	case MAX98363_R21FF_REV_ID:
+		return true;

sound/soc/codecs/max98363.c

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+	default:
+		return false;
+	}
+};
+
+static bool max98363_volatile_reg(struct device *dev, unsigned int
+reg) {
+	switch (reg) {
+	/* SoundWire Control Port Registers */
+	case MAX98363_R0040_SCP_INIT_STAT_1 ...

MAX98363_R0046_SCP_DEV_NUMBER:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	case MAX98363_R004D_SCP_BUS_CLK:
+	case MAX98363_R0050_SCP_DEV_ID_0 ...

MAX98363_R0055_SCP_DEV_ID_5:

sound/soc/codecs/max98363.c

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	case MAX98363_R0062_SCP_CLK_SCALE_BANK0:
+	case MAX98363_R0072_SCP_CLK_SCALE_BANK1:
+	case MAX98363_R0080_SCP_PHYOUTCTRL_0:
+	/* Soundwire Data Port 1 Registers */
+	case MAX98363_R0100_DP1_INIT_STAT ...

MAX98363_R0105_DP1_PREPARE_CTRL:

Review

Quoted 1 line

(quoted diff) +1 -0

+	case MAX98363_R0120_DP1_CHANNEL_EN ...

MAX98363_R0127_DP1_BLOCK_CTRL3:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	case MAX98363_R0130_DP1_CHANNEL_EN:
+	case MAX98363_R0132_DP1_SAMPLE_CTRL1...

MAX98363_R0137_DP1_BLOCK_CTRL3:

Quoted 15 lines

sound/soc/codecs/max98363.c new file +15 -0

+	/* MAX98363 Amp Control Registers */
+	case MAX98363_R2001_INTR_RAW:
+	case MAX98363_R2003_INTR_STATE:
+	case MAX98363_R2005_INTR_FALG:
+	case MAX98363_R2007_INTR_EN:
+	case MAX98363_R2009_INTR_CLR:
+	case MAX98363_R21FF_REV_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config max98363_sdw_regmap = {
+	.reg_bits = 32,

sound/soc/codecs/max98363.c line 146

Quoted 6 lines

sound/soc/codecs/max98363.c new file +6 -0

+	.val_bits = 8,
+	.max_register = MAX98363_R21FF_REV_ID,
+	.reg_defaults  = max98363_reg,
+	.num_reg_defaults = ARRAY_SIZE(max98363_reg),
+	.readable_reg = max98363_readable_register,
+	.volatile_reg = max98363_volatile_reg,

I don't see why the SoundWire standard registers are part of regmap?

Quoted 36 lines

sound/soc/codecs/max98363.c new file +35 -0

+	.cache_type = REGCACHE_RBTREE,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static __maybe_unused int max98363_suspend(struct device *dev) {
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+
+	regcache_cache_only(max98363->regmap, true);
+	regcache_mark_dirty(max98363->regmap);
+
+	if (max98363->dvddio)
+		regulator_disable(max98363->dvddio);
+
+	if (max98363->vdd)
+		regulator_disable(max98363->vdd);
+
+	return 0;
+}
+
+#define MAX98363_PROBE_TIMEOUT 5000
+
+static __maybe_unused int max98363_resume(struct device *dev) {
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+	unsigned long time;
+	int ret;
+
+	if (!max98363->first_hw_init)
+		return 0;
+
+	if (!slave->unattach_request)
+		goto regmap_sync;
+
+	time = wait_for_completion_timeout(&slave-

Quoted 2 lines

initialization_complete,
+

msecs_to_jiffies(MAX98363_PROBE_TIMEOUT));

Quoted 8 lines

sound/soc/codecs/max98363.c new file +8 -0

+	if (!time) {
+		dev_err(dev, "Initialization not complete, timed out\n");
+		return -ETIMEDOUT;
+	}
+
+regmap_sync:
+
+	if (max98363->dvddio) {

sound/soc/codecs/max98363.c line 202

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+		ret = regulator_enable(max98363->dvddio);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (max98363->vdd) {
+		ret = regulator_enable(max98363->vdd);
+		if (ret < 0)
+			return ret;
+	}

that is very very odd. It's the first time we see a SoundWire codec driver that has a power dependency, and it's quite likely that it's too late to enable power resources *AFTER* dealing with all the initialization and enumeration.

It's not even clear to me how this device would be enumerated.

You'd need to explain what part of the amplifier is controlled by those regulator, otherwise it's impossible to review and understand if the driver does the 'right thing'

sound/soc/codecs/max98363.c

Quoted 11 lines

sound/soc/codecs/max98363.c new file +10 -0

+
+	slave->unattach_request = 0;
+	regcache_cache_only(max98363->regmap, false);
+	regcache_sync(max98363->regmap);
+
+	return 0;
+}
+
+static const struct dev_pm_ops max98363_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(max98363_suspend,

max98363_resume)

Review

Quoted 1 line

(quoted diff) +1 -0

+	SET_RUNTIME_PM_OPS(max98363_suspend, max98363_resume,

NULL) };

sound/soc/codecs/max98363.c

Quoted 9 lines

sound/soc/codecs/max98363.c new file +9 -0

+
+static int max98363_read_prop(struct sdw_slave *slave) {
+	struct sdw_slave_prop *prop = &slave->prop;
+	int nval, i;
+	u32 bit;
+	unsigned long addr;
+	struct sdw_dpn_prop *dpn;
+
+	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH |

SDW_SCP_INT1_PARITY;

sound/soc/codecs/max98363.c line 231

Quoted 7 lines

sound/soc/codecs/max98363.c new file +7 -0

+
+	/* BITMAP: 00000010  Dataport 1 is active */
+	prop->sink_ports = BIT(1);
+	prop->paging_support = true;
+	prop->clk_stop_timeout = 20;
+
+	nval = hweight32(prop->source_ports);

you don't seem to have any source ports, so allocating a zero-size chunk of data is useless. You can still this entire section, no?

Quoted 65 lines

sound/soc/codecs/max98363.c new file +64 -0

+	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
+					  sizeof(*prop->src_dpn_prop),
+					  GFP_KERNEL);
+	if (!prop->src_dpn_prop)
+		return -ENOMEM;
+
+	i = 0;
+	dpn = prop->src_dpn_prop;
+	addr = prop->source_ports;
+	for_each_set_bit(bit, &addr, 32) {
+		dpn[i].num = bit;
+		dpn[i].type = SDW_DPN_FULL;
+		dpn[i].simple_ch_prep_sm = true;
+		dpn[i].ch_prep_timeout = 10;
+		i++;
+	}
+
+	/* do this again for sink now */
+	nval = hweight32(prop->sink_ports);
+	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
+					   sizeof(*prop->sink_dpn_prop),
+					   GFP_KERNEL);
+	if (!prop->sink_dpn_prop)
+		return -ENOMEM;
+
+	i = 0;
+	dpn = prop->sink_dpn_prop;
+	addr = prop->sink_ports;
+	for_each_set_bit(bit, &addr, 32) {
+		dpn[i].num = bit;
+		dpn[i].type = SDW_DPN_FULL;
+		dpn[i].simple_ch_prep_sm = true;
+		dpn[i].ch_prep_timeout = 10;
+		i++;
+	}
+
+	/* set the timeout values */
+	prop->clk_stop_timeout = 20;
+	prop->simple_clk_stop_capable = true;
+	prop->clock_reg_supported = true;
+
+	return 0;
+}
+
+static int max98363_io_init(struct sdw_slave *slave) {
+	struct device *dev = &slave->dev;
+	struct max98363_priv *max98363 = dev_get_drvdata(dev);
+	int ret, reg;
+
+	if (max98363->first_hw_init) {
+		regcache_cache_only(max98363->regmap, false);
+		regcache_cache_bypass(max98363->regmap, true);
+	}
+
+	/*
+	 * PM runtime is only enabled when a Slave reports as Attached
+	 */
+	if (!max98363->first_hw_init) {
+		/* set autosuspend parameters */
+		pm_runtime_set_autosuspend_delay(dev, 3000);
+		pm_runtime_use_autosuspend(dev);
+
+		/* update count of parent 'active' children */
+		pm_runtime_set_active(dev);

sound/soc/codecs/max98363.c

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+
+		/* make sure the device does not suspend immediately */
+		pm_runtime_mark_last_busy(dev);
+
+		pm_runtime_enable(dev);
+	}
+
+	pm_runtime_get_noresume(dev);
+
+	ret = regmap_read(max98363->regmap, MAX98363_R21FF_REV_ID,

&reg);

Quoted 5 lines

sound/soc/codecs/max98363.c new file +5 -0

+	if (!ret) {
+		dev_info(dev, "Revision ID: %X\n", reg);
+		return ret;
+	}
+

sound/soc/codecs/max98363.c line 321

Quoted 10 lines

sound/soc/codecs/max98363.c new file +10 -0

+	if (max98363->first_hw_init) {
+		regcache_cache_bypass(max98363->regmap, false);
+		regcache_mark_dirty(max98363->regmap);
+	}
+
+	max98363->first_hw_init = true;
+	max98363->hw_init = true;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);

so if there isn't a cycle of suspend-resume, how would the regulator handling work?

Something's really off here.

Quoted 22 lines

sound/soc/codecs/max98363.c new file +20 -0

+
+	return 0;
+}
 
+static int max98363_sdw_set_tdm_slot(struct snd_soc_dai *dai,
+				     unsigned int tx_mask,
+				     unsigned int rx_mask,
+				     int slots, int slot_width)
+{
+	struct snd_soc_component *component = dai->component;
+	struct max98363_priv *max98363 =
+		snd_soc_component_get_drvdata(component);
+
+	/* tx_mask is not supported */
+	if (tx_mask)
+		return -EINVAL;
+
+	if (!rx_mask && !slots && !slot_width)
+		max98363->tdm_mode = false;
+	else
+		max98363->tdm_mode = true;

sound/soc/codecs/max98363.c line 456

Quoted 6 lines

sound/soc/codecs/max98363.c new file +6 -0

+
+	max98363->rx_mask = rx_mask;
+	max98363->slot = slots;
+
+	return 0;
+}

this would not be used for a SoundWire device? Why is this needed?

sound/soc/codecs/max98363.c line 462

Quoted 7 lines

sound/soc/codecs/max98363.c new file +6 -0

+
+static const struct snd_soc_dai_ops max98363_dai_sdw_ops = {
+	.hw_params = max98363_sdw_dai_hw_params,
+	.hw_free = max98363_pcm_hw_free,
+	.set_stream = max98363_set_sdw_stream,
+	.shutdown = max98363_shutdown,

I am not clear why there is a .shutdown but no .startup, is this really needed?

Quoted 37 lines

sound/soc/codecs/max98363.c new file +35 -0

+	.set_tdm_slot = max98363_sdw_set_tdm_slot, };
 
+static struct snd_soc_dai_driver max98363_dai[] = {
+	{
+		.name = "max98363-aif1",
+		.playback = {
+			.stream_name = "HiFi Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = MAX98363_RATES,
+			.formats = MAX98363_FORMATS,
+		},
+		.ops = &max98363_dai_sdw_ops,
+	}
+};
+
+static int max98363_update_status(struct sdw_slave *slave,
+				  enum sdw_slave_status status)
+{
+	struct max98363_priv *max98363 = dev_get_drvdata(&slave->dev);
+
+	if (status == SDW_SLAVE_UNATTACHED)
+		max98363->hw_init = false;
+
+	/*
+	 * Perform initialization only if slave status is SDW_SLAVE_ATTACHED
+	 */
+	if (max98363->hw_init || status != SDW_SLAVE_ATTACHED)
+		return 0;
+
+	/* perform I/O transfers required for Slave initialization */
+	return max98363_io_init(slave);
+}
+
+/*
+ * slave_ops: callbacks for get_clock_stop_mode, clock_stop and

sound/soc/codecs/max98363.c line 505

Quoted 6 lines

+ * port_prep are not defined for now
+ */

sound/soc/codecs/max98363.c new file +4 -0

+static struct sdw_slave_ops max98363_slave_ops = {
+	.read_prop = max98363_read_prop,
+	.update_status = max98363_update_status,
+	.bus_config = NULL,

not needed

Quoted 3 lines

(quoted diff) +2 -0

+};
+

sound/soc/codecs/max98363.c line 669

Quoted 3 lines

sound/soc/codecs/max98363.c new file +3 -0

+#ifdef CONFIG_ACPI
+static const struct acpi_device_id max98363_acpi_match[] = {
+	{ "ADS8363", 0 },

Why is this needed? If this is a SoundWire device, only the _ADR is used, the HID is irrelevant.

Could this be a left-over from a I2S version?

Quoted 11 lines

sound/soc/codecs/max98363.c new file +10 -0

+	{},
+};
+MODULE_DEVICE_TABLE(acpi, max98363_acpi_match); #endif
+
+static const struct sdw_device_id max98363_id[] = {
+	SDW_SLAVE_ENTRY(0x019F, 0x8363, 0),
+	{},
+};
+MODULE_DEVICE_TABLE(sdw, max98363_id);
+

sound/soc/codecs/max98363.c line 686

Quoted 6 lines

sound/soc/codecs/max98363.c new file +6 -0

+static struct sdw_driver max98363_sdw_driver = {
+	.driver = {
+		.name = "max98363",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(max98363_of_match),
+		.acpi_match_table = ACPI_PTR(max98363_acpi_match),

not needed, only the id_table will be used.

sound/soc/codecs/max98363.c line 690

Quoted 5 lines

sound/soc/codecs/max98363.c new file +4 -0

+		.pm = &max98363_pm,
+	},
+	.probe = max98363_sdw_probe,
+	.remove = NULL,

not needed

Quoted 5 lines

sound/soc/codecs/max98363.c new file +4 -0

+	.ops = &max98363_slave_ops,
+	.id_table = max98363_id,
+};
+