Date:   Mon, 27 Feb 2023 17:47:22 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     =?utf-8?B?4oCcUnlhbg==?= <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,
        ryans.lee@analog.com
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
Message-ID: <Y/zsqjOWFKrpDtl8@sirena.org.uk>
References: <20230224010814.504016-1-ryan.lee.analog@gmail.com>
 <0fb47fe7-719b-0773-fc14-3d62d7d33619@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="X8+/QatuS77Fl/Hx"
Content-Disposition: inline
In-Reply-To: <0fb47fe7-719b-0773-fc14-3d62d7d33619@linux.intel.com>
Precedence: bulk


--X8+/QatuS77Fl/Hx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--X8+/QatuS77Fl/Hx
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmP87KkACgkQJNaLcl1U
h9Bv7Qf/YeP3QSU5T9ffyKOEPzEa0RwdpuLjBCjiDvdaCTaKpPSEZ74eMbh7LDJa
Pp+Nl3FhE3e3gYMZgJsU92VB2blKAyJ2ucRVdVon2R4KibZcmQo5uzJMm2Atc65e
psqnyn8ivHkOD7VNLgISErLRKchM+1tWbpchvxGXFrB/1atwRQ0w4I8AAr7tAeya
ArpGpzwcry/mZxUkqNlaVPkvCWnVQ2bSTBFg1VRGPkDAd3Ut97UWE8rN3/JVdUbL
GucL8qXeDkpIQsS9e6oNOgS6smmVpl7tMVOY+P1B1O6K8mZywLJW/1SKvuj9f8fl
yokBKgAoCkD7b0UzeepBdGDDtu7Vkg==
=FsGr
-----END PGP SIGNATURE-----

--X8+/QatuS77Fl/Hx--