[PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[“Ryan]

From: Ryan Lee <ryans.lee@analog.com>

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 <ryans.lee@analog.com>
---
 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

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 4621674e68bf..0638b27c6494 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -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
@@ -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
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index a0a61554548e..f4fbeceed816 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -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 @@ 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 --git a/sound/soc/codecs/max98363.c b/sound/soc/codecs/max98363.c
new file mode 100644
index 000000000000..e766c733f8e4
--- /dev/null
+++ b/sound/soc/codecs/max98363.c
@@ -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;
+	}
+};
+
+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;
+	}
+}
+
+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,
+	.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) {
+		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);
+	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;
+	struct sdw_dpn_prop *dpn;
+
+	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
+
+	/* 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);
+	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;
+	}
+
+	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);
+
+	return 0;
+}
+
+#define MAX98363_RATES SNDRV_PCM_RATE_8000_192000
+#define MAX98363_FORMATS (SNDRV_PCM_FMTBIT_S32_LE)
+
+static int max98363_sdw_dai_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 max98363_priv *max98363 =
+		snd_soc_component_get_drvdata(component);
+
+	struct sdw_stream_config stream_config;
+	struct sdw_port_config port_config;
+	enum sdw_data_direction direction;
+	struct sdw_stream_data *stream;
+	int ret;
+
+	stream = snd_soc_dai_get_dma_data(dai, substream);
+
+	if (!stream)
+		return -EINVAL;
+
+	if (!max98363->slave)
+		return -EINVAL;
+
+	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
+		return -EINVAL;
+
+	direction = SDW_DATA_DIR_RX;
+	port_config.num = 1;
+
+	stream_config.frame_rate = params_rate(params);
+	stream_config.bps = snd_pcm_format_width(params_format(params));
+	stream_config.direction = direction;
+
+	if (max98363->slot) {
+		stream_config.ch_count = max98363->slot;
+		port_config.ch_mask = max98363->rx_mask;
+	} else {
+		stream_config.ch_count = params_channels(params);
+		port_config.ch_mask = GENMASK((int)stream_config.ch_count - 1, 0);
+	}
+
+	ret = sdw_stream_add_slave(max98363->slave, &stream_config,
+				   &port_config, 1, stream->sdw_stream);
+	if (ret) {
+		dev_err(dai->dev, "Unable to configure port\n");
+		return ret;
+	}
+
+	if (params_channels(params) > 16) {
+		dev_err(component->dev, "Unsupported channels %d\n",
+			params_channels(params));
+		return -EINVAL;
+	}
+
+	dev_dbg(component->dev, "Format supported %d", params_format(params));
+
+	return 0;
+}
+
+static int max98363_pcm_hw_free(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct max98363_priv *max98363 =
+		snd_soc_component_get_drvdata(component);
+	struct sdw_stream_data *stream =
+		snd_soc_dai_get_dma_data(dai, substream);
+
+	if (!max98363->slave)
+		return -EINVAL;
+
+	sdw_stream_remove_slave(max98363->slave, stream->sdw_stream);
+
+	return 0;
+}
+
+static int max98363_set_sdw_stream(struct snd_soc_dai *dai,
+				   void *sdw_stream, int direction)
+{
+	struct sdw_stream_data *stream;
+
+	if (!sdw_stream)
+		return 0;
+
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (!stream)
+		return -ENOMEM;
+
+	stream->sdw_stream = sdw_stream;
+
+	/* Use tx_mask or rx_mask to configure stream tag and set dma_data */
+	snd_soc_dai_dma_data_set(dai, direction, stream);
+
+	return 0;
+}
+
+static void max98363_shutdown(struct snd_pcm_substream *substream,
+			      struct snd_soc_dai *dai)
+{
+	struct sdw_stream_data *stream;
+
+	stream = snd_soc_dai_get_dma_data(dai, substream);
+	snd_soc_dai_set_dma_data(dai, substream, NULL);
+	kfree(stream);
+}
+
+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;
+
+	max98363->rx_mask = rx_mask;
+	max98363->slot = slots;
+
+	return 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,
+	.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
+ * port_prep are not defined for now
+ */
+static struct sdw_slave_ops max98363_slave_ops = {
+	.read_prop = max98363_read_prop,
+	.update_status = max98363_update_status,
+	.bus_config = NULL,
+};
+
+static DECLARE_TLV_DB_SCALE(max98363_digital_tlv, -6350, 50, 1);
+static const DECLARE_TLV_DB_RANGE(max98363_spk_tlv,
+	0, 5, TLV_DB_SCALE_ITEM(-300, 300, 0),
+);
+
+static const char * const max98363_tone_cfg_text[] = {
+	"Reserved", "0", "+FS/2", "-FS/2", "1KHz",
+	"12KHz", "8KHz", "6KHz", "4KHz", "3KHz",
+	"2KHz",	"1.5KHz", "Reserved", "500Hz", "250Hz"
+};
+
+static SOC_ENUM_SINGLE_DECL(max98363_tone_cfg_enum,
+			    MAX98363_R2030_TONE_GEN_CFG, 0,
+			    max98363_tone_cfg_text);
+
+static const char * const max98363_spkmon_duration_text[] = {
+	"8ms", "20ms", "40ms", "60ms",
+	"80ms", "160ms", "240ms", "320ms",
+	"400ms", "480ms", "560ms", "640ms",
+	"720ms", "800ms", "880ms", "960ms"
+};
+
+static SOC_ENUM_SINGLE_DECL(max98363_spkmon_duration_enum,
+			    MAX98363_R2023_SPK_MON_DURATION, 0,
+			    max98363_spkmon_duration_text);
+
+static const struct snd_kcontrol_new max98363_snd_controls[] = {
+	SOC_SINGLE_TLV("Digital Volume", MAX98363_R2040_AMP_VOL,
+		       0, 0x7F, 1, max98363_digital_tlv),
+	SOC_SINGLE_TLV("Speaker Volume", MAX98363_R2041_AMP_GAIN,
+		       0, 10, 0, max98363_spk_tlv),
+	SOC_SINGLE("Tone Generator Switch", MAX98363_R203F_TONE_GEN_EN,
+		   0, 1, 0),
+	SOC_ENUM("Tone Config", max98363_tone_cfg_enum),
+	SOC_SINGLE("Ramp Switch", MAX98363_R2042_DSP_CFG,
+		   MAX98363_AMP_DSP_CFG_RMP_SHIFT, 1, 0),
+	SOC_SINGLE("CLK Monitor Switch", MAX98363_R2021_ERR_MON_CTRL,
+		   MAX98363_CLOCK_MON_SHIFT, 1, 0),
+	SOC_SINGLE("SPKMON Monitor Switch", MAX98363_R2021_ERR_MON_CTRL,
+		   MAX98363_SPKMON_SHIFT, 1, 0),
+	SOC_SINGLE("SPKMON Thresh", MAX98363_R2022_SPK_MON_THRESH, 0, 0xFF, 0),
+	SOC_ENUM("SPKMON Duration", max98363_spkmon_duration_enum),
+};
+
+static const struct snd_soc_dapm_widget max98363_dapm_widgets[] = {
+	SND_SOC_DAPM_AIF_IN("AIFIN", "HiFi Playback", 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_OUTPUT("BE_OUT"),
+};
+
+static const struct snd_soc_dapm_route max98363_audio_map[] = {
+	/* Plabyack */
+	{"BE_OUT", NULL, "AIFIN"},
+};
+
+const struct snd_soc_component_driver soc_codec_dev_max98363 = {
+	.probe			= NULL,
+	.controls		= max98363_snd_controls,
+	.num_controls		= ARRAY_SIZE(max98363_snd_controls),
+	.dapm_widgets		= max98363_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(max98363_dapm_widgets),
+	.dapm_routes		= max98363_audio_map,
+	.num_dapm_routes	= ARRAY_SIZE(max98363_audio_map),
+	.use_pmdown_time	= 1,
+	.endianness		= 1,
+};
+
+static void max98363_supply_disable(void *r)
+{
+	regulator_disable((struct regulator *)r);
+}
+
+static int max98363_init(struct sdw_slave *slave, struct regmap *regmap)
+{
+	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;
+	max98363->slave = slave;
+
+	max98363->hw_init = false;
+	max98363->first_hw_init = false;
+
+	max98363->vdd = devm_regulator_get_optional(dev, "vdd");
+	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;
+
+		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;
+	}
+
+	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);
+
+	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);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return max98363_init(slave, regmap);
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id max98363_of_match[] = {
+	{ .compatible = "adi,max98363", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max98363_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+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);
+
+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),
+		.pm = &max98363_pm,
+	},
+	.probe = max98363_sdw_probe,
+	.remove = NULL,
+	.ops = &max98363_slave_ops,
+	.id_table = max98363_id,
+};
+
+module_sdw_driver(max98363_sdw_driver);
+
+MODULE_DESCRIPTION("ASoC MAX98363 driver SDW");
+MODULE_AUTHOR("Ryan Lee <ryans.lee@analog.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/max98363.h b/sound/soc/codecs/max98363.h
new file mode 100644
index 000000000000..1c55017a9fc0
--- /dev/null
+++ b/sound/soc/codecs/max98363.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (c) 2022 Analog Devices Inc. */
+
+#ifndef _MAX98363_H
+#define _MAX98363_H
+
+/* SoundWire Slave Control Port (SCP)  */
+#define MAX98363_R0040_SCP_INIT_STAT_1		0x0040
+#define MAX98363_R0041_SCP_INIT_MASK_1		0x0041
+#define MAX98363_R0042_SCP_INIT_STAT_2		0x0042
+#define MAX98363_R0044_SCP_CTRL			0x0044
+#define MAX98363_R0045_SCP_SYSTEM_CTRL		0x0045
+#define MAX98363_R0046_SCP_DEV_NUMBER		0x0046
+#define MAX98363_R004D_SCP_BUS_CLK		0x004D
+#define MAX98363_R0050_SCP_DEV_ID_0		0x0050
+#define MAX98363_R0051_SCP_DEV_ID_1		0x0051
+#define MAX98363_R0052_SCP_DEV_ID_2		0x0052
+#define MAX98363_R0053_SCP_DEV_ID_3		0x0053
+#define MAX98363_R0054_SCP_DEV_ID_4		0x0054
+#define MAX98363_R0055_SCP_DEV_ID_5		0x0055
+#define MAX98363_R0060_SCP_FRAME_CTRL		0x0060
+#define MAX98363_R0062_SCP_CLK_SCALE_BANK0	0x0062
+#define MAX98363_R0070_SCP_FRAME_CTRL		0x0070
+#define MAX98363_R0072_SCP_CLK_SCALE_BANK1	0x0072
+#define MAX98363_R0080_SCP_PHYOUTCTRL_0		0x0080
+
+/* SoundWire Device Data Port (DP)  */
+/* Data Port 1 Registers */
+#define MAX98363_R0100_DP1_INIT_STAT		0x0100
+#define MAX98363_R0101_DP1_INIT_MASK		0x0101
+#define MAX98363_R0102_DP1_PORT_CTRL		0x0102
+#define MAX98363_R0103_DP1_BLOCK_CTRL_1		0x0103
+#define MAX98363_R0104_DP1_PREPARE_STATUS	0x0104
+#define MAX98363_R0105_DP1_PREPARE_CTRL		0x0105
+/* Data Port 1 Bank 0 Registers */
+#define MAX98363_R0120_DP1_CHANNEL_EN		0x0120
+#define MAX98363_R0122_DP1_SAMPLE_CTRL1		0x0122
+#define MAX98363_R0123_DP1_SAMPLE_CTRL2		0x0123
+#define MAX98363_R0124_DP1_OFFSET_CTRL1		0x0124
+#define MAX98363_R0125_DP1_OFFSET_CTRL2		0x0125
+#define MAX98363_R0126_DP1_HCTRL		0x0126
+#define MAX98363_R0127_DP1_BLOCK_CTRL3		0x0127
+/* Data Port 1 Bank 1 Registers */
+#define MAX98363_R0130_DP1_CHANNEL_EN		0x0130
+#define MAX98363_R0132_DP1_SAMPLE_CTRL1		0x0132
+#define MAX98363_R0133_DP1_SAMPLE_CTRL2		0x0133
+#define MAX98363_R0134_DP1_OFFSET_CTRL1		0x0134
+#define MAX98363_R0135_DP1_OFFSET_CTRL2		0x0135
+#define MAX98363_R0136_DP1_HCTRL		0x0136
+#define MAX98363_R0137_DP1_BLOCK_CTRL3		0x0137
+/* Data Port 3 Registers */
+#define MAX98363_R0300_DP3_INIT_STAT		0x0300
+#define MAX98363_R0301_DP3_INIT_MASK		0x0301
+#define MAX98363_R0302_DP3_PORT_CTRL		0x0302
+#define MAX98363_R0303_DP3_BLOCK_CTRL_1		0x0303
+#define MAX98363_R0304_DP3_PREPARE_STATUS	0x0304
+#define MAX98363_R0305_DP3_PREPARE_CTRL		0x0305
+/* Data Port 3 Bank 0 Registers */
+#define MAX98363_R0320_DP3_CHANNEL_EN		0x0320
+#define MAX98363_R0322_DP3_SAMPLE_CTRL1		0x0322
+#define MAX98363_R0323_DP3_SAMPLE_CTRL2		0x0323
+#define MAX98363_R0324_DP3_OFFSET_CTRL1		0x0324
+#define MAX98363_R0325_DP3_OFFSET_CTRL2		0x0325
+#define MAX98363_R0326_DP3_HCTRL		0x0326
+#define MAX98363_R0327_DP3_BLOCK_CTRL3		0x0327
+/* Data Port 3 Bank 1 Registers */
+#define MAX98363_R0330_DP3_CHANNEL_EN		0x0330
+#define MAX98363_R0332_DP3_SAMPLE_CTRL1		0x0332
+#define MAX98363_R0333_DP3_SAMPLE_CTRL2		0x0333
+#define MAX98363_R0334_DP3_OFFSET_CTRL1		0x0334
+#define MAX98363_R0335_DP3_OFFSET_CTRL2		0x0335
+#define MAX98363_R0336_DP3_HCTRL		0x0336
+#define MAX98363_R0337_DP3_BLOCK_CTRL3		0x0337
+/* Amp custom registers */
+#define MAX98363_R2000_SW_RESET			0x2000
+#define MAX98363_R2001_INTR_RAW			0x2001
+#define MAX98363_R2003_INTR_STATE		0x2003
+#define MAX98363_R2005_INTR_FALG		0x2005
+#define MAX98363_R2007_INTR_EN			0x2007
+#define MAX98363_R2009_INTR_CLR			0x2009
+#define MAX98363_R2021_ERR_MON_CTRL		0x2021
+#define MAX98363_R2022_SPK_MON_THRESH		0x2022
+#define MAX98363_R2023_SPK_MON_DURATION		0x2023
+#define MAX98363_R2030_TONE_GEN_CFG		0x2030
+#define MAX98363_R203F_TONE_GEN_EN		0x203F
+#define MAX98363_R2040_AMP_VOL			0x2040
+#define MAX98363_R2041_AMP_GAIN			0x2041
+#define MAX98363_R2042_DSP_CFG			0x2042
+#define MAX98363_R21FF_REV_ID			0x21FF
+
+/* MAX98363_R2021_ERR_MON_CTRL */
+#define MAX98363_SPKMON_SHIFT			(3)
+#define MAX98363_CLOCK_MON_SHIFT		(0)
+
+/* MAX98363_R2042_DSP_CFG */
+#define MAX98363_AMP_DSP_CFG_RMP_SHIFT		(3)
+
+struct max98363_priv {
+	struct regmap *regmap;
+	struct regulator *vdd, *dvddio;
+	bool tdm_mode;
+	struct sdw_slave *slave;
+	int slot;
+	unsigned int rx_mask;
+	bool hw_init;
+	bool first_hw_init;
+};
+#endif
-- 
2.34.1

[PATCH 2/2] ASoC: dt-bindings: max98363: add soundwire amplifier driver

[“Ryan]

From: Ryan Lee <ryans.lee@analog.com>

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

Signed-off-by: Ryan Lee <ryans.lee@analog.com>
---
 .../bindings/sound/adi,max98363.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/adi,max98363.yaml

diff --git a/Documentation/devicetree/bindings/sound/adi,max98363.yaml b/Documentation/devicetree/bindings/sound/adi,max98363.yaml
new file mode 100644
index 000000000000..fda571d04a64
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/adi,max98363.yaml
@@ -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 <ryans.lee@analog.com>
+
+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
+    description: Peripheral-device unique ID decoded from pin
+
+  vdd-supply:
+    description:
+      A 2.5V to 5.5V supply that powers up the VDD pin.
+
+  dvddio-supply:
+    description:
+      A 1.7V or 1.9V supply that powers up the DVDDIO pin.
+      This property is only needed for MAX98363A/B.
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - dvddio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    soundwire {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        max98363: amplifier@3 {
+            compatible = "adi,max98363";
+            reg = <0x3>;
+            vdd-supply = <&regulator_vdd>;
+            dvddio-supply = <&regulator_1v8>;
+        };
+    };
-- 
2.34.1

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Nuno Sá]

On Thu, 2023-02-23 at 17:08 -0800, “Ryan wrote:
> From: Ryan Lee <ryans.lee@analog.com>
> 
> 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 <ryans.lee@analog.com>
> ---
>  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
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 4621674e68bf..0638b27c6494 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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
> @@ -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
>  
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index a0a61554548e..f4fbeceed816 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -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 @@ 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 --git a/sound/soc/codecs/max98363.c
> b/sound/soc/codecs/max98363.c
> new file mode 100644
> index 000000000000..e766c733f8e4
> --- /dev/null
> +++ b/sound/soc/codecs/max98363.c
> @@ -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;
> +       }
> +};
> +
> +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;
> +       }
> +}
> +
> +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,
> +       .cache_type = REGCACHE_RBTREE,
> +       .use_single_read = true,
> +       .use_single_write = true,
> +};
> +
> +static __maybe_unused int max98363_suspend(struct device *dev)

can drop '__maybe_unused'...

> +{
> +       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)

ditto...

> +{
> +       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) {
> +               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);
> +       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)
> +};
> +

DEFINE_RUNTIME_DEV_PM_OPS()...

> +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)
> +{
> +       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;
> +       max98363->slave = slave;
> +
> +       max98363->hw_init = false;
> +       max98363->first_hw_init = false;
> +
> +       max98363->vdd = devm_regulator_get_optional(dev, "vdd");

vdd is not an optional regulator, right?

> +       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...

> +       if (IS_ERR(max98363->dvddio)) {
> +               if (PTR_ERR(max98363->dvddio) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +
> +               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;
> +       }
> +
> +       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;
> +       }
> +

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().

> +       /* 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);
> +
> +       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);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       return max98363_init(slave, regmap);
> +}
> +
> +#if defined(CONFIG_OF)

no need for this guard...

> +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

> +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);
> +
> +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.

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

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

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

no need for this.

> +       .ops = &max98363_slave_ops,
> +       .id_table = max98363_id,
> +};
> +
> +module_sdw_driver(max98363_sdw_driver);
> +
> 

- Nuno Sá

Re: [PATCH 2/2] ASoC: dt-bindings: max98363: add soundwire amplifier driver

[Krzysztof Kozlowski]

On 24/02/2023 02:08, “Ryan wrote:
> From: Ryan Lee <ryans.lee@analog.com>

Thank you for your patch. There is something to discuss/improve.

Your patch is corrupted because you modified it after creating it. It
won't apply correctly and won't work. The bot's response with error is
about this.

Except broken patch, here are some more comments:

> 
> This patch adds dt-bindings information for Analog Devices MAX98363

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> SoundWire Amplifier.
> 

Subject: drop driver. Bindings are about hardware, not drivers.

> Signed-off-by: Ryan Lee <ryans.lee@analog.com>
> ---
>  .../bindings/sound/adi,max98363.yaml          | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/adi,max98363.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/adi,max98363.yaml b/Documentation/devicetree/bindings/sound/adi,max98363.yaml
> new file mode 100644
> index 000000000000..fda571d04a64
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/adi,max98363.yaml
> @@ -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 <ryans.lee@analog.com>
> +
> +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
> +    description: Peripheral-device unique ID decoded from pin
> +
> +  vdd-supply:
> +    description:
> +      A 2.5V to 5.5V supply that powers up the VDD pin.
> +
> +  dvddio-supply:
> +    description:
> +      A 1.7V or 1.9V supply that powers up the DVDDIO pin.
> +      This property is only needed for MAX98363A/B.

It's not a DAI?

> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - dvddio-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>

Drop the header, you do not use it.

> +    soundwire {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        max98363: amplifier@3 {

also drop label (max98363).

> +            compatible = "adi,max98363";
> +            reg = <0x3>;
> +            vdd-supply = <&regulator_vdd>;
> +            dvddio-supply = <&regulator_1v8>;
> +        };
> +    };

Best regards,
Krzysztof

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Pierre-Louis Bossart]

> +#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?

> +#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.

> +
> +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},

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

> +	{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;
> +	}
> +}
> +
> +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?

> +	.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) {
> +		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'

> +
> +	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;
> +	struct sdw_dpn_prop *dpn;
> +
> +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +
> +	/* 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?

> +	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;
> +	}
> +
> +	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.

> +
> +	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;
> +
> +	max98363->rx_mask = rx_mask;
> +	max98363->slot = slots;
> +
> +	return 0;
> +}

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

> +
> +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?

> +	.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
> + * port_prep are not defined for now
> + */
> +static struct sdw_slave_ops max98363_slave_ops = {
> +	.read_prop = max98363_read_prop,
> +	.update_status = max98363_update_status,
> +	.bus_config = NULL,

not needed

> +};
> +

> +#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?

> +	{},
> +};
> +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);
> +
> +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.

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

not needed

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

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Mark Brown]

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

On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:

> > +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},

> 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.

> > +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?

> > +	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'

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

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

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Pierre-Louis Bossart]

On 2/27/23 12:47, Mark Brown wrote:
> On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:
> 
>>> +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},
> 
>> 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.

>>> +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?

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.


> 
>>> +	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'
> 
> It's also buggy to have regulators treated as optional unless they may
> be physically absent.

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Mark Brown]

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

On Mon, Feb 27, 2023 at 01:19:15PM -0500, Pierre-Louis Bossart wrote:
> On 2/27/23 12:47, Mark Brown wrote:
> > On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:

> >> 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.

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.

> > ...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.

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.

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

RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, February 27, 2023 10:45 AM
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: “Ryan <ryan.lee.analog@gmail.com>; lgirdwood@gmail.com;
> perex@perex.cz; tiwai@suse.com; krzysztof.kozlowski@linaro.org;
> rf@opensource.cirrus.com; ckeepax@opensource.cirrus.com;
> herve.codina@bootlin.com; wangweidong.a@awinic.com;
> james.schulman@cirrus.com; ajye_huang@compal.corp-partner.google.com;
> shumingf@realtek.com; povik+lin@cutebit.org; flatmax@flatmax.com; linux-
> kernel@vger.kernel.org; alsa-devel@alsa-project.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; Lee, RyanS <RyanS.Lee@analog.com>
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> On Mon, Feb 27, 2023 at 01:19:15PM -0500, Pierre-Louis Bossart wrote:
> > On 2/27/23 12:47, Mark Brown wrote:
> > > On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:
> 
> > >> 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.
> 
> 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.
> 
> > > ...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.
> 
> 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.)

Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Pierre-Louis Bossart]

>>> 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.)

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

RE: [PATCH 2/2] ASoC: dt-bindings: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, February 23, 2023 8:30 PM
> To: “Ryan <ryan.lee.analog@gmail.com>
> Cc: Lee, RyanS <RyanS.Lee@analog.com>; alsa-devel@alsa-project.org;
> pierre-louis.bossart@linux.intel.com; rf@opensource.cirrus.com;
> wangweidong.a@awinic.com; broonie@kernel.org; robh+dt@kernel.org;
> povik+lin@cutebit.org; perex@perex.cz; linux-kernel@vger.kernel.org;
> james.schulman@cirrus.com; lgirdwood@gmail.com; shumingf@realtek.com;
> tiwai@suse.com; ajye_huang@compal.corp-partner.google.com;
> ckeepax@opensource.cirrus.com; herve.codina@bootlin.com;
> krzysztof.kozlowski@linaro.org; devicetree@vger.kernel.org;
> flatmax@flatmax.com
> Subject: Re: [PATCH 2/2] ASoC: dt-bindings: max98363: add soundwire
> amplifier driver
> 
> [External]
> 
> 
> On Thu, 23 Feb 2023 17:08:14 -0800, “Ryan wrote:
> > From: Ryan Lee <ryans.lee@analog.com>
> >
> > This patch adds dt-bindings information for Analog Devices MAX98363
> > SoundWire Amplifier.
> >
> > Signed-off-by: Ryan Lee <ryans.lee@analog.com>
> > ---
> >  .../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-
> ryan.lee.analog@gmail.com__;!!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

Thank you. I did run the 'dt_binding_check' build, but some module was not installed.
I shall modify the error and submit it again.

> 
> 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.

RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Thursday, February 23, 2023 11:57 PM
> To: “Ryan <ryan.lee.analog@gmail.com>; lgirdwood@gmail.com;
> broonie@kernel.org; perex@perex.cz; tiwai@suse.com;
> krzysztof.kozlowski@linaro.org; rf@opensource.cirrus.com;
> ckeepax@opensource.cirrus.com; pierre-louis.bossart@linux.intel.com;
> herve.codina@bootlin.com; wangweidong.a@awinic.com;
> james.schulman@cirrus.com; ajye_huang@compal.corp-partner.google.com;
> shumingf@realtek.com; povik+lin@cutebit.org; flatmax@flatmax.com; linux-
> kernel@vger.kernel.org; alsa-devel@alsa-project.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; Lee, RyanS <RyanS.Lee@analog.com>
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> On Thu, 2023-02-23 at 17:08 -0800, “Ryan wrote:
> > From: Ryan Lee <ryans.lee@analog.com>
> >
> > 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 <ryans.lee@analog.com>
> > ---
> >  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
> >
> > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index
> > 4621674e68bf..0638b27c6494 100644
> > --- a/sound/soc/codecs/Kconfig
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -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
> > @@ -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
> >
> > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> > index a0a61554548e..f4fbeceed816 100644
> > --- a/sound/soc/codecs/Makefile
> > +++ b/sound/soc/codecs/Makefile
> > @@ -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 @@
> > 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
> > --git a/sound/soc/codecs/max98363.c b/sound/soc/codecs/max98363.c
> new
> > file mode 100644 index 000000000000..e766c733f8e4
> > --- /dev/null
> > +++ b/sound/soc/codecs/max98363.c
> > @@ -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;
> > +       }
> > +};
> > +
> > +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;
> > +       }
> > +}
> > +
> > +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,
> > +       .cache_type = REGCACHE_RBTREE,
> > +       .use_single_read = true,
> > +       .use_single_write = true,
> > +};
> > +
> > +static __maybe_unused int max98363_suspend(struct device *dev)
> 
> can drop '__maybe_unused'...

Thanks. I shall remove it.
> 
> > +{
> > +       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)
> 
> ditto...

Same here. Thank you.

> 
> > +{
> > +       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) {
> > +               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);
> > +       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) };
> > +
> 
> DEFINE_RUNTIME_DEV_PM_OPS()...
> 
> > +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)
> > +{
> > +       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;
> > +       max98363->slave = slave;
> > +
> > +       max98363->hw_init = false;
> > +       max98363->first_hw_init = false;
> > +
> > +       max98363->vdd = devm_regulator_get_optional(dev, "vdd");
> 
> vdd is not an optional regulator, right?

Yes, it is a mandatory power supply and it seems like these regulator controls
are inappropriate for the SoundWire device. I shall remove the part.

> 
> > +       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...
> 
> > +       if (IS_ERR(max98363->dvddio)) {
> > +               if (PTR_ERR(max98363->dvddio) == -EPROBE_DEFER)
> > +                       return -EPROBE_DEFER;
> > +
> > +               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;
> > +       }
> > +
> > +       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;
> > +       }
> > +
> 
> 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().
> 
> > +       /* 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);
> > +
> > +       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);
> > +       if (IS_ERR(regmap))
> > +               return PTR_ERR(regmap);
> > +
> > +       return max98363_init(slave, regmap); }
> > +
> > +#if defined(CONFIG_OF)
> 
> no need for this guard...

It was left over from the I2S driver and is no longer necessary for the SoundWire
driver. I shall remove the CONFIG_OF part.

> 
> > +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
> 
> > +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);
> > +
> > +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.

I shall remove the ACPI-related part because it is not necessary for the SoundWire driver.

> 
> > +               .acpi_match_table = ACPI_PTR(max98363_acpi_match),
> > +               .pm = &max98363_pm,
> 
> use pm_ptr(). With this macro, no need for '__maybe_unused'
> annotations.

I shall apply your recommendation. Thanks.

> 
> > +       },
> > +       .probe = max98363_sdw_probe,
> > +       .remove = NULL,
> 
> no need for this.

Agreed. I shall remove it.

> 
> > +       .ops = &max98363_slave_ops,
> > +       .id_table = max98363_id,
> > +};
> > +
> > +module_sdw_driver(max98363_sdw_driver);
> > +
> >
> 
> - Nuno Sá
Thanks for the review!

RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Monday, February 27, 2023 7:18 AM
> To: “Ryan <ryan.lee.analog@gmail.com>; lgirdwood@gmail.com;
> broonie@kernel.org; perex@perex.cz; tiwai@suse.com;
> krzysztof.kozlowski@linaro.org; rf@opensource.cirrus.com;
> ckeepax@opensource.cirrus.com; herve.codina@bootlin.com;
> wangweidong.a@awinic.com; james.schulman@cirrus.com;
> ajye_huang@compal.corp-partner.google.com; shumingf@realtek.com;
> povik+lin@cutebit.org; flatmax@flatmax.com; linux-kernel@vger.kernel.org;
> alsa-devel@alsa-project.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; Lee, RyanS <RyanS.Lee@analog.com>
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> 
> > +#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?

I shall fix this in alphabetical order.

> 
> > +#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.

I shall remove the abstraction.

> 
> > +
> > +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},
> 
> That seems wrong, why would you declare standard registers that are known
> to the bus and required to be implemented?

Agreed. I shall remove the standard registers from the table.

> 
> > +	{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;
> > +	}
> > +}
> > +
> > +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?

I agree. It is not necessary. I shall remove it.

> 
> > +	.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) {
> > +		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'

VDD and DVDDIO are mandatory power supplies to use SoundWire interface
of the amplifier. I think providing a power dependency is not a good idea.
I do not see a similar case on other SoundWire driver like you mentioned.
I shall remove this part.

> 
> > +
> > +	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;
> > +	struct sdw_dpn_prop *dpn;
> > +
> > +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH |
> SDW_SCP_INT1_PARITY;
> > +
> > +	/* 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?

You are right.
This amp only supports playback. I shall remove the configuration for the source ports.

> 
> > +	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;
> > +	}
> > +
> > +	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.

I am sorry but I do not fully get this question.
May I know what the regulator handling work means here?
Anyways, the regulator handing part will be removed.

> 
> > +
> > +	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;
> > +
> > +	max98363->rx_mask = rx_mask;
> > +	max98363->slot = slots;
> > +
> > +	return 0;
> > +}
> 
> this would not be used for a SoundWire device? Why is this needed?

This was a leftover from the old I2S driver. I shall remove the TDM slot config function.

> 
> > +
> > +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?

I think .shutdown is not necessary because what it does is also done by
.hw_free function. It clears dma_data pointer after use, but it is also
cleared by .hw_free function. I shall remove .shutdown.

> 
> > +	.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
> > + * port_prep are not defined for now
> > + */
> > +static struct sdw_slave_ops max98363_slave_ops = {
> > +	.read_prop = max98363_read_prop,
> > +	.update_status = max98363_update_status,
> > +	.bus_config = NULL,
> 
> not needed

I shall remove this line.

> 
> > +};
> > +
> 
> > +#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?

Yes, I shall remove ACPI related part.

> 
> > +	{},
> > +};
> > +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);
> > +
> > +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.

Agreed. I shall remove this part.

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

Agreed. I shall remove this part. Thanks for the review!

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

RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Monday, February 27, 2023 3:38 PM
> To: Lee, RyanS <RyanS.Lee@analog.com>; Mark Brown
> <broonie@kernel.org>
> Cc: “Ryan <ryan.lee.analog@gmail.com>; lgirdwood@gmail.com;
> tiwai@suse.com; krzysztof.kozlowski@linaro.org; rf@opensource.cirrus.com;
> ckeepax@opensource.cirrus.com; herve.codina@bootlin.com;
> wangweidong.a@awinic.com; james.schulman@cirrus.com;
> ajye_huang@compal.corp-partner.google.com; shumingf@realtek.com;
> povik+lin@cutebit.org; flatmax@flatmax.com; linux-kernel@vger.kernel.org;
> alsa-devel@alsa-project.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> 
> >>> 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.)
> 
> 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

Thank you for the useful information. Then, there is no reason to keep
standard registers in the driver. I shall remove stand registers from
the amp register map.