Message-ID: <SJ0PR03MB6681234100F5F32B7C7A5F9D8ABA9@SJ0PR03MB6681.namprd03.prod.outlook.com>
Date:   Fri, 10 Mar 2023 01:36:46 +0000
From:   "Lee, RyanS" <RyanS.Lee@...log.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        “Ryan <ryan.lee.analog@...il.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
        "rf@...nsource.cirrus.com" <rf@...nsource.cirrus.com>,
        "ckeepax@...nsource.cirrus.com" <ckeepax@...nsource.cirrus.com>,
        "herve.codina@...tlin.com" <herve.codina@...tlin.com>,
        "wangweidong.a@...nic.com" <wangweidong.a@...nic.com>,
        "james.schulman@...rus.com" <james.schulman@...rus.com>,
        "ajye_huang@...pal.corp-partner.google.com" 
        <ajye_huang@...pal.corp-partner.google.com>,
        "shumingf@...ltek.com" <shumingf@...ltek.com>,
        "povik+lin@...ebit.org" <povik+lin@...ebit.org>,
        "flatmax@...tmax.com" <flatmax@...tmax.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Monday, February 27, 2023 7:18 AM
> To: “Ryan <ryan.lee.analog@...il.com>; lgirdwood@...il.com;
> broonie@...nel.org; perex@...ex.cz; tiwai@...e.com;
> krzysztof.kozlowski@...aro.org; rf@...nsource.cirrus.com;
> ckeepax@...nsource.cirrus.com; herve.codina@...tlin.com;
> wangweidong.a@...nic.com; james.schulman@...rus.com;
> ajye_huang@...pal.corp-partner.google.com; shumingf@...ltek.com;
> povik+lin@...ebit.org; flatmax@...tmax.com; linux-kernel@...r.kernel.org;
> alsa-devel@...a-project.org; robh+dt@...nel.org;
> devicetree@...r.kernel.org; Lee, RyanS <RyanS.Lee@...log.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,
> > +};
> > +

Powered by blists - more mailing lists