RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
From: Lee, RyanS
Date: Thu Mar 09 2023 - 20:35:07 EST
> -----Original Message-----
> From: Nuno Sá <noname.nuno@xxxxxxxxx>
> Sent: Thursday, February 23, 2023 11:57 PM
> To: “Ryan <ryan.lee.analog@xxxxxxxxx>; lgirdwood@xxxxxxxxx;
> broonie@xxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx;
> krzysztof.kozlowski@xxxxxxxxxx; rf@xxxxxxxxxxxxxxxxxxxxx;
> ckeepax@xxxxxxxxxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx;
> herve.codina@xxxxxxxxxxx; wangweidong.a@xxxxxxxxxx;
> james.schulman@xxxxxxxxxx; ajye_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;
> shumingf@xxxxxxxxxxx; povik+lin@xxxxxxxxxxx; flatmax@xxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Lee, RyanS <RyanS.Lee@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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!