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